From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 References: <20181118123504.83082-1-tmaimon77@gmail.com> <20181118123504.83082-2-tmaimon77@gmail.com> In-Reply-To: From: Tomer Maimon Date: Tue, 20 Nov 2018 12:51:19 +0200 Message-ID: Subject: Re: [PATCH v1 1/1] spi: npcm: fix uninitialized 'val' warning in receive function Content-Type: multipart/alternative; boundary="000000000000170726057b16540a" To: olof@lixom.net Cc: broonie@kernel.org, Rob Herring , Mark Rutland , Nancy Yuen , Patrick Venture , Brendan Higgins , Avi Fishman , Joel Stanley , linux-spi@vger.kernel.org, OpenBMC Maillist , Linux Kernel Mailing List , devicetree List-ID: --000000000000170726057b16540a Content-Type: text/plain; charset="UTF-8" Hi Olof, On Sun, 18 Nov 2018 at 23:38, Olof Johansson wrote: > On Sun, Nov 18, 2018 at 4:36 AM Tomer Maimon wrote: > > > > Fix uninitialized 'val' warning receive function, send function > > has been modify to be aligned with the receive function. > > > > Signed-off-by: Tomer Maimon > > --- > > 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 > --000000000000170726057b16540a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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 functio= n
> has been modify to be aligned with the receive function.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>=C2=A0 drivers/spi/spi-npcm-pspi.c | 12 ++++++------
>=C2=A0 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 *pri= v)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0wsize =3D min(bytes_per_word(priv->= ;bits_per_word), priv->tx_bytes);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0priv->tx_bytes -=3D wsize;
>
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0if (priv->tx_buf) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (wsize =3D= =3D 1)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0iowrite8(*priv->tx_buf, NPCM_PSPI_DATA + priv->base); > +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (priv->tx_buf && wsize) {
In general, doing an early:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!condition)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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.

>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (wsize= =3D=3D 2)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0iowrite16(*priv->tx_buf, NPCM_PSPI_DATA + priv->b= ase);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0iowrite8(*priv->tx_buf, NPCM_PSPI_DATA + priv->base);
I think this is broken? If wsize is something else than 1 or 2, you'll<= br> 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= =C2=A0
get from priv->tx_bytes) and the only numbers that mini= mum function return are 0, 1 or 2.

But in the end = of the day,=C2=A0 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
<= br>


-Olof
--000000000000170726057b16540a--