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
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(-)
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
On Sun, Nov 18, 2018 at 4:36 AM Tomer Maimon <[email protected]> wrote:
>
> 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(-)
>
> 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
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.