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

Message ID 20181118123504.83082-2-tmaimon77@gmail.com
State Not Applicable, archived
Headers show
Series
  • npcm: fix uninitialized 'val' warning in receive function
Related show

Commit Message

Tomer Maimon Nov. 18, 2018, 12:35 p.m.
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(-)

Comments

Olof Johansson Nov. 18, 2018, 9:38 p.m. | #1
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
Tomer Maimon Nov. 20, 2018, 10:51 a.m. | #2
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
>
<div dir="ltr">Hi Olof,<br><br><div class="gmail_quote"><div dir="ltr">On Sun, 18 Nov 2018 at 23:38, Olof Johansson &lt;<a href="mailto:olof@lixom.net" target="_blank">olof@lixom.net</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sun, Nov 18, 2018 at 4:36 AM Tomer Maimon &lt;<a href="mailto:tmaimon77@gmail.com" target="_blank">tmaimon77@gmail.com</a>&gt; wrote:<br>
&gt;<br>
&gt; Fix uninitialized &#39;val&#39; warning receive function, send function<br>
&gt; has been modify to be aligned with the receive function.<br>
&gt;<br>
&gt; Signed-off-by: Tomer Maimon &lt;<a href="mailto:tmaimon77@gmail.com" target="_blank">tmaimon77@gmail.com</a>&gt;<br>
&gt; ---<br>
&gt;  drivers/spi/spi-npcm-pspi.c | 12 ++++++------<br>
&gt;  1 file changed, 6 insertions(+), 6 deletions(-)<br>
&gt;<br>
&gt; diff --git a/drivers/spi/spi-npcm-pspi.c b/drivers/spi/spi-npcm-pspi.c<br>
&gt; index 6dae91091143..f75df49ab84e 100644<br>
&gt; --- a/drivers/spi/spi-npcm-pspi.c<br>
&gt; +++ b/drivers/spi/spi-npcm-pspi.c<br>
&gt; @@ -199,11 +199,11 @@ static void npcm_pspi_send(struct npcm_pspi *priv)<br>
&gt;         wsize = min(bytes_per_word(priv-&gt;bits_per_word), priv-&gt;tx_bytes);<br>
&gt;         priv-&gt;tx_bytes -= wsize;<br>
&gt;<br>
&gt; -       if (priv-&gt;tx_buf) {<br>
&gt; -               if (wsize == 1)<br>
&gt; -                       iowrite8(*priv-&gt;tx_buf, NPCM_PSPI_DATA + priv-&gt;base);<br>
&gt; +       if (priv-&gt;tx_buf &amp;&amp; wsize) {<br>
<br>
In general, doing an early:<br>
        if (!condition)<br>
                return;<br>
<br>
is a pattern we prefer in the kernel. Setting up the assumptions at<br>
the beginning of the function makes it easier to follow the code flow,<br>
and saves a level of indentation.<br>
<br>
It&#39;s a matter of taste though, and this function has only one level.<br>
So, either way is OK. Just mentioning it.<br>
<br>
&gt;                 if (wsize == 2)<br>
&gt;                         iowrite16(*priv-&gt;tx_buf, NPCM_PSPI_DATA + priv-&gt;base);<br>
&gt; +               else<br>
&gt; +                       iowrite8(*priv-&gt;tx_buf, NPCM_PSPI_DATA + priv-&gt;base);<br>
<br>
I think this is broken? If wsize is something else than 1 or 2, you&#39;ll<br>
do a one-byte write but advance the buffer pointer with a different<br>
amount.<br>
<br>
It&#39;ll be fairly tricky to debug if this ever happens (it shouldn&#39;t,<br>
but still). This is why I added a WARN_ON_ONCE() in my patch instead.<br></blockquote><div><br></div><div>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 </div><div>get from priv-&gt;tx_bytes) and the only numbers that minimum function return are 0, 1 or 2.</div><div><br></div><div>But in the end of the day,  we don&#39;t have an issue with your solution as long it will be done also in the transfer function.<br></div><div><br></div><div>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).</div><div><br></div><div>Thanks again</div><div><br></div><div>Tomer</div><div><br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
-Olof<br>
</blockquote></div></div>
Mark Brown Nov. 20, 2018, 1:02 p.m. | #3
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.

Patch

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;