From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753534AbdG2IbA (ORCPT ); Sat, 29 Jul 2017 04:31:00 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35755 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753356AbdG2Ia4 (ORCPT ); Sat, 29 Jul 2017 04:30:56 -0400 Message-ID: <1501317055.20021.2.camel@gmail.com> Subject: Re: [PATCH] Staging: pi433: fix some warnings detected using sparse From: Elia Geretto To: Dan Carpenter Cc: Greg Kroah-Hartman , devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Date: Sat, 29 Jul 2017 10:30:55 +0200 In-Reply-To: <20170728141707.sdp7yy236247olad@mwanda> References: <20170728125626.17672-1-elia.f.geretto@gmail.com> <20170728141707.sdp7yy236247olad@mwanda> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.24.4 (3.24.4-1.fc26) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2017-07-28 at 17:17 +0300, Dan Carpenter wrote: > 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 > I will resend a separate patch containing the enum work; I apologize for the unclear changelog, I am still trying to understand how much in detail I should go. Next time I will be more precise. Regards, Elia Geretto