Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1698753 > unrolled thread

[PATCH] Staging: pi433: fix some warnings detected using sparse

Started byElia Geretto <elia.f.geretto@gmail.com>
First post2017-07-28 15:00 +0200
Last post2017-07-29 10:40 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] Staging: pi433: fix some warnings detected using sparse Elia Geretto <elia.f.geretto@gmail.com> - 2017-07-28 15:00 +0200
    Re: [PATCH] Staging: pi433: fix some warnings detected using sparse Dan Carpenter <dan.carpenter@oracle.com> - 2017-07-28 16:20 +0200
      Re: [PATCH] Staging: pi433: fix some warnings detected using sparse Elia Geretto <elia.f.geretto@gmail.com> - 2017-07-29 10:40 +0200

#1698753 — [PATCH] Staging: pi433: fix some warnings detected using sparse

FromElia Geretto <elia.f.geretto@gmail.com>
Date2017-07-28 15:00 +0200
Subject[PATCH] Staging: pi433: fix some warnings detected using sparse
Message-ID<u8d22-3Rv-13@gated-at.bofh.it>
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.

Signed-off-by: Elia Geretto <elia.f.geretto@gmail.com>
---
 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));
 	SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
 	SET_CHECKED(rf69_set_crc_enable	    (dev->spi, rx_cfg->enable_crc));
 
@@ -264,8 +267,11 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
 	{
 		SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
 	}
+	if (tx_cfg->enable_length_byte == optionOn)
+		SET_CHECKED(rf69_set_packet_format(dev->spi, packetLengthVar));
+	else
+		SET_CHECKED(rf69_set_packet_format(dev->spi, packetLengthFix));
 	SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
-	SET_CHECKED(rf69_set_packet_format(dev->spi, tx_cfg->enable_length_byte));
 	SET_CHECKED(rf69_set_crc_enable	  (dev->spi, tx_cfg->enable_crc));
 
 	/* configure sync, if enabled */
@@ -313,7 +319,7 @@ pi433_start_rx(struct pi433_device *dev)
 
 /*-------------------------------------------------------------------------*/
 
-int
+static int
 pi433_receive(void *data)
 {
 	struct pi433_device *dev = data;
@@ -463,7 +469,7 @@ pi433_receive(void *data)
 		return bytes_total;
 }
 
-int
+static int
 pi433_tx_thread(void *data)
 {
 	struct pi433_device *device = data;
@@ -1152,8 +1158,7 @@ static int pi433_probe(struct spi_device *spi)
 	device->tx_task_struct = kthread_run(pi433_tx_thread,
 					     device,
 					     "pi433_tx_task");
-	if (device->tx_task_struct < 0)
-	{
+	if (IS_ERR(device->tx_task_struct)) {
 		dev_dbg(device->dev, "start of send thread failed");
 		goto send_thread_failed;
 	}
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e391ce777bc7..52521fa18c9b 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -433,7 +433,9 @@ int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPer
 	return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
 }
 
-int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse mantisse, u8 exponent)
+static int
+rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
+			  enum mantisse mantisse, u8 exponent)
 {
 	u8 newValue;
 
-- 
2.13.3

[toc] | [next] | [standalone]

#1698831

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-07-28 16:20 +0200
Message-ID<u8ehr-4NW-9@gated-at.bofh.it>
In reply to#1698753
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 <elia.f.geretto@gmail.com>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

#1699264

FromElia Geretto <elia.f.geretto@gmail.com>
Date2017-07-29 10:40 +0200
Message-ID<u8vrX-84d-3@gated-at.bofh.it>
In reply to#1698831
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 <elia.f.geretto@gmail.com>
> > ---
> >  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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web