Skip to contentSkip to conversation

npcm: fix uninitialized 'val' warning in receive function

PATCH In review Tomer Maimon 3 participants 5 messages Nov 18, 2018 – Nov 20, 2018

Subsystems (2)
SPI Arm/nuvoton npcm architecture
lore
Share & tools
lore

Tomer Maimon

author maintainer
#0 lore raw

cover npcm: fix uninitialized 'val' warning in receive function

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(-)
Signature · 1 line
2.14.1
author maintainer
#1 lore raw

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

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

Signed-off-by Tomer Maimon <[email protected]>
---
 drivers/spi/spi-npcm-pspi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/spi/spi-npcm-pspi.c +6 -6
index 6dae91091143..f75df49ab84e 100644
@@ -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;
Signature · 1 line
2.14.1
#2 lore raw

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

Quoted 14 lines · 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 <[email protected]>
---
 drivers/spi/spi-npcm-pspi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/spi/spi-npcm-pspi.c +0 -0
index 6dae91091143..f75df49ab84e 100644

drivers/spi/spi-npcm-pspi.c static void npcm_pspi_send(struct npcm_pspi *priv) · line 202

Quoted 7 lines
drivers/spi/spi-npcm-pspi.c +1 -3
@@ -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.

drivers/spi/spi-npcm-pspi.c static void npcm_pspi_send(struct npcm_pspi *priv) · line 206

Quoted 4 lines
drivers/spi/spi-npcm-pspi.c +2 -0
                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

author maintainer
#3 lore raw

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

Hi Olof,

Quoted 40 lines · Olof Johansson
Quoted 14 lines · 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 <[email protected]>
---
 drivers/spi/spi-npcm-pspi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/spi/spi-npcm-pspi.c +0 -0
index 6dae91091143..f75df49ab84e 100644

drivers/spi/spi-npcm-pspi.c static void npcm_pspi_send(struct npcm_pspi *priv) · line 204

Quoted 6 lines
drivers/spi/spi-npcm-pspi.c +0 -3
@@ -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);

drivers/spi/spi-npcm-pspi.c static void npcm_pspi_send(struct npcm_pspi *priv) · line 202

Quoted 1 line
drivers/spi/spi-npcm-pspi.c +1 -0
+       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.

drivers/spi/spi-npcm-pspi.c static void npcm_pspi_send(struct npcm_pspi *priv) · line 204

Quoted 2 lines
drivers/spi/spi-npcm-pspi.c +0 -0
                if (wsize == 2)
                        iowrite16(*priv->tx_buf, NPCM_PSPI_DATA +

priv->base);

drivers/spi/spi-npcm-pspi.c static void npcm_pspi_send(struct npcm_pspi *priv) · line 206

Quoted 2 lines
drivers/spi/spi-npcm-pspi.c +2 -0
+               else
+                       iowrite8(*priv->tx_buf, NPCM_PSPI_DATA +
Quoted 9 lines

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

Quoted 1 line
maintainer
#4 lore raw

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

Quoted 7 lines · Tomer Maimon

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.