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

[Tomer Maimon <tmaimon77@gmail.com>]

[-- 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 --]