From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751945AbdG1ORV (ORCPT ); Fri, 28 Jul 2017 10:17:21 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:17948 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751737AbdG1ORU (ORCPT ); Fri, 28 Jul 2017 10:17:20 -0400 Date: Fri, 28 Jul 2017 17:17:07 +0300 From: Dan Carpenter To: Elia Geretto Cc: Greg Kroah-Hartman , devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] Staging: pi433: fix some warnings detected using sparse Message-ID: <20170728141707.sdp7yy236247olad@mwanda> References: <20170728125626.17672-1-elia.f.geretto@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170728125626.17672-1-elia.f.geretto@gmail.com> User-Agent: NeoMutt/20170113 (1.7.2) X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 28, 2017 at 02:56:26PM +0200, Elia Geretto wrote: > This patch corrects some visibility issues regarding some functions and > solves a warning related to a non-matching union. After this patch, > sparse produces only one other warning regarding a bitwise operator; > however, this behaviour seems to be intended. I can't understand this changelog at all.... :/ What are we fixing exactly? It seems like we're fixing something about bitwise operators... I guess let me check the Sparse warnings... Here they are from the latest linux-next: drivers/staging/pi433/pi433_if.c:211:9: warning: mixing different enum types drivers/staging/pi433/pi433_if.c:211:9: int enum optionOnOff versus drivers/staging/pi433/pi433_if.c:211:9: int enum packetFormat drivers/staging/pi433/pi433_if.c:211:9: warning: mixing different enum types drivers/staging/pi433/pi433_if.c:211:9: int enum optionOnOff versus drivers/staging/pi433/pi433_if.c:211:9: int enum packetFormat drivers/staging/pi433/pi433_if.c:268:9: warning: mixing different enum types drivers/staging/pi433/pi433_if.c:268:9: int enum optionOnOff versus drivers/staging/pi433/pi433_if.c:268:9: int enum packetFormat drivers/staging/pi433/pi433_if.c:268:9: warning: mixing different enum types drivers/staging/pi433/pi433_if.c:268:9: int enum optionOnOff versus drivers/staging/pi433/pi433_if.c:268:9: int enum packetFormat drivers/staging/pi433/pi433_if.c:317:1: warning: symbol 'pi433_receive' was not declared. Should it be static? drivers/staging/pi433/pi433_if.c:467:1: warning: symbol 'pi433_tx_thread' was not declared. Should it be static? drivers/staging/pi433/pi433_if.c:1155:36: error: incompatible types for operation (<) drivers/staging/pi433/pi433_if.c:1155:36: left side has type struct task_struct *tx_task_struct drivers/staging/pi433/pi433_if.c:1155:36: right side has type int drivers/staging/pi433/rf69.c:206:17: warning: dubious: x & !y drivers/staging/pi433/rf69.c:436:5: warning: symbol 'rf69_set_bandwidth_intern' was not declared. Should it be static? Each type of fix should be sent as a separate fix with a better changelog. People have already done the "static" fixes and IS_ERR() fixes, so don't worry about those. But I don't think anyway has fixed the enum issues so resend that. Also the bitwise thing is a real bug, but there is already a fix for that, it just hasn't been merged yet. > > Signed-off-by: Elia Geretto > --- > drivers/staging/pi433/pi433_if.c | 17 +++++++++++------ > drivers/staging/pi433/rf69.c | 4 +++- > 2 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c > index d9328ce5ec1d..f8219a53ce60 100644 > --- a/drivers/staging/pi433/pi433_if.c > +++ b/drivers/staging/pi433/pi433_if.c > @@ -208,7 +208,10 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg) > { > SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always)); > } > - SET_CHECKED(rf69_set_packet_format (dev->spi, rx_cfg->enable_length_byte)); > + if (rx_cfg->enable_length_byte == optionOn) > + SET_CHECKED(rf69_set_packet_format(dev->spi, packetLengthVar)); > + else > + SET_CHECKED(rf69_set_packet_format(dev->spi, packetLengthFix)); The SET_CHECKED() macro is total garbage. It has a hidden return and it calls the rf69_set_packet_format() twice on error it expands to: if (rf69_set_packet_format(dev->spi, rx_cfg->enable_length_byte)) < 0) return rf69_set_packet_format(dev->spi, rx_cfg->enable_length_byte); Mega turbo barf! Kill it with fire! regards, dan carpenter