[PATCH v1 0/1] npcm: fix uninitialized 'val' warning in receive function

[PATCH v1 0/1] npcm: fix uninitialized 'val' warning in receive function

[Tomer Maimon]

Addressed comments from:.
 - kbuild test robot : drivers/spi/spi-npcm-pspi.c:226:17: warning:
  'val' may be used uninitialized in this function.
  
Fix uninitialized 'val' warning receive function.
Send function has been modify to be aligned with 
the receive function.

The NPCM PSPI driver tested on NPCM750 evaluation board.

Tomer Maimon (1):
  spi: npcm: fix uninitialized 'val' warning in receive function

 drivers/spi/spi-npcm-pspi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.14.1

[PATCH v1 1/1] spi: npcm: fix uninitialized 'val' warning in receive function

[Tomer Maimon]

Fix uninitialized 'val' warning receive function, send function
has been modify to be aligned with the receive function.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 drivers/spi/spi-npcm-pspi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-npcm-pspi.c b/drivers/spi/spi-npcm-pspi.c
index 6dae91091143..f75df49ab84e 100644
--- a/drivers/spi/spi-npcm-pspi.c
+++ b/drivers/spi/spi-npcm-pspi.c
@@ -199,11 +199,11 @@ static void npcm_pspi_send(struct npcm_pspi *priv)
 	wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
 	priv->tx_bytes -= wsize;
 
-	if (priv->tx_buf) {
-		if (wsize == 1)
-			iowrite8(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);
+	if (priv->tx_buf && wsize) {
 		if (wsize == 2)
 			iowrite16(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);
+		else
+			iowrite8(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);
 
 		priv->tx_buf += wsize;
 	}
@@ -217,11 +217,11 @@ static void npcm_pspi_recv(struct npcm_pspi *priv)
 	rsize = min(bytes_per_word(priv->bits_per_word), priv->rx_bytes);
 	priv->rx_bytes -= rsize;
 
-	if (priv->rx_buf) {
-		if (rsize == 1)
-			val = ioread8(priv->base + NPCM_PSPI_DATA);
+	if (priv->rx_buf && rsize) {
 		if (rsize == 2)
 			val = ioread16(priv->base + NPCM_PSPI_DATA);
+		else
+			val = ioread8(priv->base + NPCM_PSPI_DATA);
 
 		*priv->rx_buf = val;
 		priv->rx_buf += rsize;
-- 
2.14.1

Re: [PATCH v1 1/1] spi: npcm: fix uninitialized 'val' warning in receive function

[Olof Johansson]

On Sun, Nov 18, 2018 at 4:36 AM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Fix uninitialized 'val' warning receive function, send function
> has been modify to be aligned with the receive function.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  drivers/spi/spi-npcm-pspi.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi-npcm-pspi.c b/drivers/spi/spi-npcm-pspi.c
> index 6dae91091143..f75df49ab84e 100644
> --- a/drivers/spi/spi-npcm-pspi.c
> +++ b/drivers/spi/spi-npcm-pspi.c
> @@ -199,11 +199,11 @@ static void npcm_pspi_send(struct npcm_pspi *priv)
>         wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
>         priv->tx_bytes -= wsize;
>
> -       if (priv->tx_buf) {
> -               if (wsize == 1)
> -                       iowrite8(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);
> +       if (priv->tx_buf && wsize) {

In general, doing an early:
        if (!condition)
                return;

is a pattern we prefer in the kernel. Setting up the assumptions at
the beginning of the function makes it easier to follow the code flow,
and saves a level of indentation.

It's a matter of taste though, and this function has only one level.
So, either way is OK. Just mentioning it.

>                 if (wsize == 2)
>                         iowrite16(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);
> +               else
> +                       iowrite8(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);

I think this is broken? If wsize is something else than 1 or 2, you'll
do a one-byte write but advance the buffer pointer with a different
amount.

It'll be fairly tricky to debug if this ever happens (it shouldn't,
but still). This is why I added a WARN_ON_ONCE() in my patch instead.


-Olof

Re: [PATCH v1 1/1] spi: npcm: fix uninitialized 'val' warning in receive function

[Tomer Maimon]

[-- Attachment #1: Type: text/plain, Size: 2478 bytes --]

Hi Olof,

On Sun, 18 Nov 2018 at 23:38, Olof Johansson <olof@lixom.net> wrote:

> On Sun, Nov 18, 2018 at 4:36 AM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> > Fix uninitialized 'val' warning receive function, send function
> > has been modify to be aligned with the receive function.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  drivers/spi/spi-npcm-pspi.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/spi/spi-npcm-pspi.c b/drivers/spi/spi-npcm-pspi.c
> > index 6dae91091143..f75df49ab84e 100644
> > --- a/drivers/spi/spi-npcm-pspi.c
> > +++ b/drivers/spi/spi-npcm-pspi.c
> > @@ -199,11 +199,11 @@ static void npcm_pspi_send(struct npcm_pspi *priv)
> >         wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
> >         priv->tx_bytes -= wsize;
> >
> > -       if (priv->tx_buf) {
> > -               if (wsize == 1)
> > -                       iowrite8(*priv->tx_buf, NPCM_PSPI_DATA +
> priv->base);
> > +       if (priv->tx_buf && wsize) {
>
> In general, doing an early:
>         if (!condition)
>                 return;
>
> is a pattern we prefer in the kernel. Setting up the assumptions at
> the beginning of the function makes it easier to follow the code flow,
> and saves a level of indentation.
>
> It's a matter of taste though, and this function has only one level.
> So, either way is OK. Just mentioning it.
>
> >                 if (wsize == 2)
> >                         iowrite16(*priv->tx_buf, NPCM_PSPI_DATA +
> priv->base);
> > +               else
> > +                       iowrite8(*priv->tx_buf, NPCM_PSPI_DATA +
> priv->base);
>
> I think this is broken? If wsize is something else than 1 or 2, you'll
> do a one-byte write but advance the buffer pointer with a different
> amount.
>
> It'll be fairly tricky to debug if this ever happens (it shouldn't,
> but still). This is why I added a WARN_ON_ONCE() in my patch instead.
>

We just tried to reduce the number of lines to minimum, so we have debug it
quite a lot (with all the numbers that can
get from priv->tx_bytes) and the only numbers that minimum function return
are 0, 1 or 2.

But in the end of the day,  we don't have an issue with your solution as
long it will be done also in the transfer function.

So if you can send a new patch with transfer function modification as well
it will be great (Please let me know if you like me to send the patch).

Thanks again

Tomer



-Olof
>

[-- Attachment #2: Type: text/html, Size: 3531 bytes --]

Re: [PATCH v1 1/1] spi: npcm: fix uninitialized 'val' warning in receive function

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

On Tue, Nov 20, 2018 at 12:51:19PM +0200, Tomer Maimon wrote:

> We just tried to reduce the number of lines to minimum, so we have debug it
> quite a lot (with all the numbers that can
> get from priv->tx_bytes) and the only numbers that minimum function return
> are 0, 1 or 2.

> But in the end of the day,  we don't have an issue with your solution as
> long it will be done also in the transfer function.

In general it's better to have the code be obviously correct than to try
to push down the line count - it saves people the effort of figuring out
if things are safe every time they look at it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]