| Message ID | 20181118123504.83082-2-tmaimon77@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | npcm: fix uninitialized 'val' warning in receive function | expand |
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
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.
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;
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(-)