[PATCH 00/10] Fix a bunch of W=1 warnings in Misc

[PATCH 00/10] Fix a bunch of W=1 warnings in Misc

[Lee Jones]

Attempting to clean-up W=1 kernel builds, which are currently
overwhelmingly riddled with niggly little warnings.

Lee Jones (10):
  misc: c2port: core: Ensure source size does not equal destination size
    in strncpy()
  misc: ti-st: st_core: Tidy-up bespoke commentry
  misc: ti-st: st_kim: Tidy-up bespoke commentry
  misc: lkdtm: bugs: At least try to use popuated variable
  misc: lkdtm: Always provide prototype for lkdtm_DOUBLE_FAULT()
  misc: eeprom: eeprom_93cx6: Repair function arg descriptions
  misc: mic: vop: vop_main: Remove set but unused variable 'ret'
  misc: cb710: sgbuf2: Add missing documentation for
    cb710_sg_dwiter_write_next_block()'s 'data' arg
  misc: habanalabs: irq: Add missing struct identifier for 'struct
    hl_eqe_work'
  misc: pti: Fix documentation for bit-rotted function
    pti_tty_driver_write()

 drivers/misc/c2port/core.c         |  2 +-
 drivers/misc/cb710/sgbuf2.c        |  1 +
 drivers/misc/eeprom/eeprom_93cx6.c |  4 +-
 drivers/misc/habanalabs/irq.c      |  3 +-
 drivers/misc/lkdtm/bugs.c          |  4 +-
 drivers/misc/lkdtm/lkdtm.h         |  2 -
 drivers/misc/mic/vop/vop_main.c    |  3 +-
 drivers/misc/pti.c                 |  5 +-
 drivers/misc/ti-st/st_core.c       | 79 ++++++++++++++++++------------
 drivers/misc/ti-st/st_kim.c        | 71 +++++++++++++++++----------
 10 files changed, 104 insertions(+), 70 deletions(-)

-- 
2.25.1

[PATCH 01/10] misc: c2port: core: Ensure source size does not equal destination size in strncpy()

[Lee Jones]

We need to ensure there's a place for the NULL terminator.

Fixes the following W=1 warning(s):

 In file included from include/linux/bitmap.h:9,
 from include/linux/nodemask.h:95,
 from include/linux/mmzone.h:17,
 from include/linux/gfp.h:6,
 from include/linux/umh.h:4,
 from include/linux/kmod.h:9,
 from include/linux/module.h:16,
 from drivers/misc/c2port/core.c:9:
 In function ‘strncpy’,
 inlined from ‘c2port_device_register’ at drivers/misc/c2port/core.c:926:2:
 include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
 297 | #define __underlying_strncpy __builtin_strncpy
 | ^
 include/linux/string.h:307:9: note: in expansion of macro ‘__underlying_strncpy’
 307 | return __underlying_strncpy(p, q, size);
 | ^~~~~~~~~~~~~~~~~~~~

Cc: Rodolfo Giometti <giometti@linux.it>
Cc: "Eurotech S.p.A" <info@eurotech.it>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/c2port/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c
index 33bba18022892..80d87e8a0bea9 100644
--- a/drivers/misc/c2port/core.c
+++ b/drivers/misc/c2port/core.c
@@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char *name,
 	}
 	dev_set_drvdata(c2dev->dev, c2dev);
 
-	strncpy(c2dev->name, name, C2PORT_NAME_LEN);
+	strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1);
 	c2dev->ops = ops;
 	mutex_init(&c2dev->mutex);
 
-- 
2.25.1

Re: [PATCH 01/10] misc: c2port: core: Ensure source size does not equal destination size in strncpy()

[Rodolfo Giometti]

On 26/06/2020 15:05, Lee Jones wrote:
> We need to ensure there's a place for the NULL terminator.
> 
> Fixes the following W=1 warning(s):
> 
>  In file included from include/linux/bitmap.h:9,
>  from include/linux/nodemask.h:95,
>  from include/linux/mmzone.h:17,
>  from include/linux/gfp.h:6,
>  from include/linux/umh.h:4,
>  from include/linux/kmod.h:9,
>  from include/linux/module.h:16,
>  from drivers/misc/c2port/core.c:9:
>  In function ‘strncpy’,
>  inlined from ‘c2port_device_register’ at drivers/misc/c2port/core.c:926:2:
>  include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
>  297 | #define __underlying_strncpy __builtin_strncpy
>  | ^
>  include/linux/string.h:307:9: note: in expansion of macro ‘__underlying_strncpy’
>  307 | return __underlying_strncpy(p, q, size);
>  | ^~~~~~~~~~~~~~~~~~~~
> 
> Cc: Rodolfo Giometti <giometti@linux.it>
> Cc: "Eurotech S.p.A" <info@eurotech.it>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/misc/c2port/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c
> index 33bba18022892..80d87e8a0bea9 100644
> --- a/drivers/misc/c2port/core.c
> +++ b/drivers/misc/c2port/core.c
> @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char *name,
>  	}
>  	dev_set_drvdata(c2dev->dev, c2dev);
>  
> -	strncpy(c2dev->name, name, C2PORT_NAME_LEN);
> +	strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1);
>  	c2dev->ops = ops;
>  	mutex_init(&c2dev->mutex);
>  

Acked-by: Rodolfo Giometti <giometti@enneenne.com>

Note that giometti@linux.it is just an alias. My main e-mail address is
giometti@enneenne.com

Rodolfo Giometti

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH 01/10] misc: c2port: core: Ensure source size does not equal destination size in strncpy()

[Geert Uytterhoeven]

Hi Lee,

On Fri, Jun 26, 2020 at 3:06 PM Lee Jones <lee.jones@linaro.org> wrote:
> We need to ensure there's a place for the NULL terminator.

But who's filling that space with a NUL (not NULL) terminator?

> Fixes the following W=1 warning(s):
>
>  In file included from include/linux/bitmap.h:9,
>  from include/linux/nodemask.h:95,
>  from include/linux/mmzone.h:17,
>  from include/linux/gfp.h:6,
>  from include/linux/umh.h:4,
>  from include/linux/kmod.h:9,
>  from include/linux/module.h:16,
>  from drivers/misc/c2port/core.c:9:
>  In function ‘strncpy’,
>  inlined from ‘c2port_device_register’ at drivers/misc/c2port/core.c:926:2:
>  include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
>  297 | #define __underlying_strncpy __builtin_strncpy
>  | ^
>  include/linux/string.h:307:9: note: in expansion of macro ‘__underlying_strncpy’
>  307 | return __underlying_strncpy(p, q, size);
>  | ^~~~~~~~~~~~~~~~~~~~
>
> Cc: Rodolfo Giometti <giometti@linux.it>
> Cc: "Eurotech S.p.A" <info@eurotech.it>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/misc/c2port/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c
> index 33bba18022892..80d87e8a0bea9 100644
> --- a/drivers/misc/c2port/core.c
> +++ b/drivers/misc/c2port/core.c
> @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char *name,
>         }
>         dev_set_drvdata(c2dev->dev, c2dev);

c2dev is allocated using:

        c2dev = kmalloc(sizeof(struct c2port_device), GFP_KERNEL);

hence the allocated memory is not zeroed.

>
> -       strncpy(c2dev->name, name, C2PORT_NAME_LEN);
> +       strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1);

strncpy()
  1. does not terminate the destination with a NUL if the source length
      is C2PORT_NAME_LEN - 1,
  2. fills all remaining space in the destination buffer with NUL characters.

So c2dev.name[C2PORT_NAME_LEN - 1] always contains an uninitialized
value.

Now, it seems the only caller of c2port_device_register() passes
"uc" as the name.  Which means in practice c2dev.name[] will be
NUL-terminated. However, the last byte will still be uninitialized, and
if the buffer is ever copied to userspace, your patch will have introduced
a leak.

>         c2dev->ops = ops;
>         mutex_init(&c2dev->mutex);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 01/10] misc: c2port: core: Ensure source size does not equal destination size in strncpy()

[Lee Jones]

On Mon, 13 Jul 2020, Geert Uytterhoeven wrote:

> Hi Lee,
> 
> On Fri, Jun 26, 2020 at 3:06 PM Lee Jones <lee.jones@linaro.org> wrote:
> > We need to ensure there's a place for the NULL terminator.
> 
> But who's filling that space with a NUL (not NULL) terminator?
> 
> > Fixes the following W=1 warning(s):
> >
> >  In file included from include/linux/bitmap.h:9,
> >  from include/linux/nodemask.h:95,
> >  from include/linux/mmzone.h:17,
> >  from include/linux/gfp.h:6,
> >  from include/linux/umh.h:4,
> >  from include/linux/kmod.h:9,
> >  from include/linux/module.h:16,
> >  from drivers/misc/c2port/core.c:9:
> >  In function ‘strncpy’,
> >  inlined from ‘c2port_device_register’ at drivers/misc/c2port/core.c:926:2:
> >  include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
> >  297 | #define __underlying_strncpy __builtin_strncpy
> >  | ^
> >  include/linux/string.h:307:9: note: in expansion of macro ‘__underlying_strncpy’
> >  307 | return __underlying_strncpy(p, q, size);
> >  | ^~~~~~~~~~~~~~~~~~~~
> >
> > Cc: Rodolfo Giometti <giometti@linux.it>
> > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/misc/c2port/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c
> > index 33bba18022892..80d87e8a0bea9 100644
> > --- a/drivers/misc/c2port/core.c
> > +++ b/drivers/misc/c2port/core.c
> > @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char *name,
> >         }
> >         dev_set_drvdata(c2dev->dev, c2dev);
> 
> c2dev is allocated using:
> 
>         c2dev = kmalloc(sizeof(struct c2port_device), GFP_KERNEL);
> 
> hence the allocated memory is not zeroed.
> 
> >
> > -       strncpy(c2dev->name, name, C2PORT_NAME_LEN);
> > +       strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1);
> 
> strncpy()
>   1. does not terminate the destination with a NUL if the source length
>       is C2PORT_NAME_LEN - 1,
>   2. fills all remaining space in the destination buffer with NUL characters.
> 
> So c2dev.name[C2PORT_NAME_LEN - 1] always contains an uninitialized
> value.
> 
> Now, it seems the only caller of c2port_device_register() passes
> "uc" as the name.  Which means in practice c2dev.name[] will be
> NUL-terminated. However, the last byte will still be uninitialized, and
> if the buffer is ever copied to userspace, your patch will have introduced
> a leak.

Quite right.  Good spot.  I must have made the assumption that the
destination buffer would be pre-initialised.  Not sure why it's not in
this case.  Seems like an odd practice.

So we have a choice.  We can either enlarge the destination buffer to
*actually* allow a full length (32 byte in this case) naming string,
or zero the buffer.

Or even both!

Do you have a preference?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/10] misc: c2port: core: Ensure source size does not equal destination size in strncpy()

[Geert Uytterhoeven]

Hi Lee,

On Tue, Jul 14, 2020 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 13 Jul 2020, Geert Uytterhoeven wrote:
> > On Fri, Jun 26, 2020 at 3:06 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > We need to ensure there's a place for the NULL terminator.
> >
> > But who's filling that space with a NUL (not NULL) terminator?
> >
> > > Fixes the following W=1 warning(s):
> > >
> > >  In file included from include/linux/bitmap.h:9,
> > >  from include/linux/nodemask.h:95,
> > >  from include/linux/mmzone.h:17,
> > >  from include/linux/gfp.h:6,
> > >  from include/linux/umh.h:4,
> > >  from include/linux/kmod.h:9,
> > >  from include/linux/module.h:16,
> > >  from drivers/misc/c2port/core.c:9:
> > >  In function ‘strncpy’,
> > >  inlined from ‘c2port_device_register’ at drivers/misc/c2port/core.c:926:2:
> > >  include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
> > >  297 | #define __underlying_strncpy __builtin_strncpy
> > >  | ^
> > >  include/linux/string.h:307:9: note: in expansion of macro ‘__underlying_strncpy’
> > >  307 | return __underlying_strncpy(p, q, size);
> > >  | ^~~~~~~~~~~~~~~~~~~~
> > >
> > > Cc: Rodolfo Giometti <giometti@linux.it>
> > > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/misc/c2port/core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c
> > > index 33bba18022892..80d87e8a0bea9 100644
> > > --- a/drivers/misc/c2port/core.c
> > > +++ b/drivers/misc/c2port/core.c
> > > @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char *name,
> > >         }
> > >         dev_set_drvdata(c2dev->dev, c2dev);
> >
> > c2dev is allocated using:
> >
> >         c2dev = kmalloc(sizeof(struct c2port_device), GFP_KERNEL);
> >
> > hence the allocated memory is not zeroed.
> >
> > >
> > > -       strncpy(c2dev->name, name, C2PORT_NAME_LEN);
> > > +       strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1);
> >
> > strncpy()
> >   1. does not terminate the destination with a NUL if the source length
> >       is C2PORT_NAME_LEN - 1,
> >   2. fills all remaining space in the destination buffer with NUL characters.
> >
> > So c2dev.name[C2PORT_NAME_LEN - 1] always contains an uninitialized
> > value.
> >
> > Now, it seems the only caller of c2port_device_register() passes
> > "uc" as the name.  Which means in practice c2dev.name[] will be
> > NUL-terminated. However, the last byte will still be uninitialized, and
> > if the buffer is ever copied to userspace, your patch will have introduced
> > a leak.
>
> Quite right.  Good spot.  I must have made the assumption that the
> destination buffer would be pre-initialised.  Not sure why it's not in
> this case.  Seems like an odd practice.
>
> So we have a choice.  We can either enlarge the destination buffer to
> *actually* allow a full length (32 byte in this case) naming string,
> or zero the buffer.
>
> Or even both!
>
> Do you have a preference?

Do we know if the buffer or full c2dev struct is ever copied to userspace?
If it may be copied => kalloc().
If it will never be copied => strlcpy() (no NUL-padding, only NUL-terminator).

Oh, and there is a newer one on the block (which I always have to lookup),
which is preferred over strlcpy() and strncpy(): strscpy().
And reading lib/string.c, there's strscpy_pad(), too ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 01/10] misc: c2port: core: Ensure source size does not equal destination size in strncpy()

[Lee Jones]

On Tue, 14 Jul 2020, Geert Uytterhoeven wrote:

> Hi Lee,
> 
> On Tue, Jul 14, 2020 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 13 Jul 2020, Geert Uytterhoeven wrote:
> > > On Fri, Jun 26, 2020 at 3:06 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > We need to ensure there's a place for the NULL terminator.
> > >
> > > But who's filling that space with a NUL (not NULL) terminator?
> > >
> > > > Fixes the following W=1 warning(s):
> > > >
> > > >  In file included from include/linux/bitmap.h:9,
> > > >  from include/linux/nodemask.h:95,
> > > >  from include/linux/mmzone.h:17,
> > > >  from include/linux/gfp.h:6,
> > > >  from include/linux/umh.h:4,
> > > >  from include/linux/kmod.h:9,
> > > >  from include/linux/module.h:16,
> > > >  from drivers/misc/c2port/core.c:9:
> > > >  In function ‘strncpy’,
> > > >  inlined from ‘c2port_device_register’ at drivers/misc/c2port/core.c:926:2:
> > > >  include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
> > > >  297 | #define __underlying_strncpy __builtin_strncpy
> > > >  | ^
> > > >  include/linux/string.h:307:9: note: in expansion of macro ‘__underlying_strncpy’
> > > >  307 | return __underlying_strncpy(p, q, size);
> > > >  | ^~~~~~~~~~~~~~~~~~~~
> > > >
> > > > Cc: Rodolfo Giometti <giometti@linux.it>
> > > > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/misc/c2port/core.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c
> > > > index 33bba18022892..80d87e8a0bea9 100644
> > > > --- a/drivers/misc/c2port/core.c
> > > > +++ b/drivers/misc/c2port/core.c
> > > > @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char *name,
> > > >         }
> > > >         dev_set_drvdata(c2dev->dev, c2dev);
> > >
> > > c2dev is allocated using:
> > >
> > >         c2dev = kmalloc(sizeof(struct c2port_device), GFP_KERNEL);
> > >
> > > hence the allocated memory is not zeroed.
> > >
> > > >
> > > > -       strncpy(c2dev->name, name, C2PORT_NAME_LEN);
> > > > +       strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1);
> > >
> > > strncpy()
> > >   1. does not terminate the destination with a NUL if the source length
> > >       is C2PORT_NAME_LEN - 1,
> > >   2. fills all remaining space in the destination buffer with NUL characters.
> > >
> > > So c2dev.name[C2PORT_NAME_LEN - 1] always contains an uninitialized
> > > value.
> > >
> > > Now, it seems the only caller of c2port_device_register() passes
> > > "uc" as the name.  Which means in practice c2dev.name[] will be
> > > NUL-terminated. However, the last byte will still be uninitialized, and
> > > if the buffer is ever copied to userspace, your patch will have introduced
> > > a leak.
> >
> > Quite right.  Good spot.  I must have made the assumption that the
> > destination buffer would be pre-initialised.  Not sure why it's not in
> > this case.  Seems like an odd practice.
> >
> > So we have a choice.  We can either enlarge the destination buffer to
> > *actually* allow a full length (32 byte in this case) naming string,
> > or zero the buffer.
> >
> > Or even both!
> >
> > Do you have a preference?
> 
> Do we know if the buffer or full c2dev struct is ever copied to userspace?

I don't know that, but I think we should err on the side of caution.

> If it may be copied => kalloc().

Do you mean kzalloc()?

> If it will never be copied => strlcpy() (no NUL-padding, only NUL-terminator).
> 
> Oh, and there is a newer one on the block (which I always have to lookup),
> which is preferred over strlcpy() and strncpy(): strscpy().
> And reading lib/string.c, there's strscpy_pad(), too ;-)

Let's not get too crazy. ;)

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/10] misc: c2port: core: Ensure source size does not equal destination size in strncpy()

[Geert Uytterhoeven]

On Tue, Jul 14, 2020 at 10:01 AM Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 14 Jul 2020, Geert Uytterhoeven wrote:
> > On Tue, Jul 14, 2020 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > On Mon, 13 Jul 2020, Geert Uytterhoeven wrote:
> > > > On Fri, Jun 26, 2020 at 3:06 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > We need to ensure there's a place for the NULL terminator.
> > > >
> > > > But who's filling that space with a NUL (not NULL) terminator?
> > > >
> > > > > Fixes the following W=1 warning(s):
> > > > >
> > > > >  In file included from include/linux/bitmap.h:9,
> > > > >  from include/linux/nodemask.h:95,
> > > > >  from include/linux/mmzone.h:17,
> > > > >  from include/linux/gfp.h:6,
> > > > >  from include/linux/umh.h:4,
> > > > >  from include/linux/kmod.h:9,
> > > > >  from include/linux/module.h:16,
> > > > >  from drivers/misc/c2port/core.c:9:
> > > > >  In function ‘strncpy’,
> > > > >  inlined from ‘c2port_device_register’ at drivers/misc/c2port/core.c:926:2:
> > > > >  include/linux/string.h:297:30: warning: ‘__builtin_strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
> > > > >  297 | #define __underlying_strncpy __builtin_strncpy
> > > > >  | ^
> > > > >  include/linux/string.h:307:9: note: in expansion of macro ‘__underlying_strncpy’
> > > > >  307 | return __underlying_strncpy(p, q, size);
> > > > >  | ^~~~~~~~~~~~~~~~~~~~
> > > > >
> > > > > Cc: Rodolfo Giometti <giometti@linux.it>
> > > > > Cc: "Eurotech S.p.A" <info@eurotech.it>
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/misc/c2port/core.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/misc/c2port/core.c b/drivers/misc/c2port/core.c
> > > > > index 33bba18022892..80d87e8a0bea9 100644
> > > > > --- a/drivers/misc/c2port/core.c
> > > > > +++ b/drivers/misc/c2port/core.c
> > > > > @@ -923,7 +923,7 @@ struct c2port_device *c2port_device_register(char *name,
> > > > >         }
> > > > >         dev_set_drvdata(c2dev->dev, c2dev);
> > > >
> > > > c2dev is allocated using:
> > > >
> > > >         c2dev = kmalloc(sizeof(struct c2port_device), GFP_KERNEL);
> > > >
> > > > hence the allocated memory is not zeroed.
> > > >
> > > > >
> > > > > -       strncpy(c2dev->name, name, C2PORT_NAME_LEN);
> > > > > +       strncpy(c2dev->name, name, C2PORT_NAME_LEN - 1);
> > > >
> > > > strncpy()
> > > >   1. does not terminate the destination with a NUL if the source length
> > > >       is C2PORT_NAME_LEN - 1,
> > > >   2. fills all remaining space in the destination buffer with NUL characters.
> > > >
> > > > So c2dev.name[C2PORT_NAME_LEN - 1] always contains an uninitialized
> > > > value.
> > > >
> > > > Now, it seems the only caller of c2port_device_register() passes
> > > > "uc" as the name.  Which means in practice c2dev.name[] will be
> > > > NUL-terminated. However, the last byte will still be uninitialized, and
> > > > if the buffer is ever copied to userspace, your patch will have introduced
> > > > a leak.
> > >
> > > Quite right.  Good spot.  I must have made the assumption that the
> > > destination buffer would be pre-initialised.  Not sure why it's not in
> > > this case.  Seems like an odd practice.
> > >
> > > So we have a choice.  We can either enlarge the destination buffer to
> > > *actually* allow a full length (32 byte in this case) naming string,
> > > or zero the buffer.
> > >
> > > Or even both!
> > >
> > > Do you have a preference?
> >
> > Do we know if the buffer or full c2dev struct is ever copied to userspace?
>
> I don't know that, but I think we should err on the side of caution.
>
> > If it may be copied => kalloc().
>
> Do you mean kzalloc()?

Sorry, kzalloc.

> > If it will never be copied => strlcpy() (no NUL-padding, only NUL-terminator).
> >
> > Oh, and there is a newer one on the block (which I always have to lookup),
> > which is preferred over strlcpy() and strncpy(): strscpy().
> > And reading lib/string.c, there's strscpy_pad(), too ;-)
>
> Let's not get too crazy. ;)

The side of caution is kzalloc(), so strscpy() is OK.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 02/10] misc: ti-st: st_core: Tidy-up bespoke commentry

[Lee Jones]

If it's still in use and worth the effort, it sure looks like
this driver could do with a good scrub (clean).

This patch conserns itself with the non-standard comments located
thoughout the file.

It also fixes the following W=1 warnings by demoting the kerneldoc
function headers to standard comments, since there doesn't appear
to be a requirement for the function args to be documented:

 drivers/misc/ti-st/st_core.c:132: warning: Function parameter or member 'st_gdata' not described in 'st_reg_complete'
 drivers/misc/ti-st/st_core.c:132: warning: Function parameter or member 'err' not described in 'st_reg_complete'
 drivers/misc/ti-st/st_core.c:197: warning: Function parameter or member 'st_gdata' not described in 'st_wakeup_ack'
 drivers/misc/ti-st/st_core.c:197: warning: Function parameter or member 'cmd' not described in 'st_wakeup_ack'
 drivers/misc/ti-st/st_core.c:226: warning: Function parameter or member 'disc_data' not described in 'st_int_recv'
 drivers/misc/ti-st/st_core.c:226: warning: Function parameter or member 'data' not described in 'st_int_recv'
 drivers/misc/ti-st/st_core.c:226: warning: Function parameter or member 'count' not described in 'st_int_recv'
 drivers/misc/ti-st/st_core.c:387: warning: Function parameter or member 'st_gdata' not described in 'st_int_dequeue'
 drivers/misc/ti-st/st_core.c:409: warning: Function parameter or member 'st_gdata' not described in 'st_int_enqueue'
 drivers/misc/ti-st/st_core.c:409: warning: Function parameter or member 'skb' not described in 'st_int_enqueue'

Cc: Pavan Savoy <pavan_savoy@ti.com>
Cc: Naveen Jain <naveen_jain@ti.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/ti-st/st_core.c | 79 ++++++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 30 deletions(-)

diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
index 14136d2cc8f93..f4ddd1e670151 100644
--- a/drivers/misc/ti-st/st_core.c
+++ b/drivers/misc/ti-st/st_core.c
@@ -18,7 +18,8 @@
 
 extern void st_kim_recv(void *, const unsigned char *, long);
 void st_int_recv(void *, const unsigned char *, long);
-/* function pointer pointing to either,
+/*
+ * function pointer pointing to either,
  * st_kim_recv during registration to receive fw download responses
  * st_int_recv after registration to receive proto stack responses
  */
@@ -60,7 +61,8 @@ int st_get_uart_wr_room(struct st_data_s *st_gdata)
 	return tty->ops->write_room(tty);
 }
 
-/* can be called in from
+/*
+ * can be called in from
  * -- KIM (during fw download)
  * -- ST Core (during st_write)
  *
@@ -100,7 +102,8 @@ static void st_send_frame(unsigned char chnl_id, struct st_data_s *st_gdata)
 		kfree_skb(st_gdata->rx_skb);
 		return;
 	}
-	/* this cannot fail
+	/*
+	 * this cannot fail
 	 * this shouldn't take long
 	 * - should be just skb_queue_tail for the
 	 *   protocol stack driver
@@ -121,9 +124,8 @@ static void st_send_frame(unsigned char chnl_id, struct st_data_s *st_gdata)
 	return;
 }
 
-/**
- * st_reg_complete -
- * to call registration complete callbacks
+/*
+ * st_reg_complete - to call registration complete callbacks
  * of all protocol stack drivers
  * This function is being called with spin lock held, protocol drivers are
  * only expected to complete their waits and do nothing more than that.
@@ -156,21 +158,24 @@ static inline int st_check_data_len(struct st_data_s *st_gdata,
 	pr_debug("len %d room %d", len, room);
 
 	if (!len) {
-		/* Received packet has only packet header and
+		/*
+		 * Received packet has only packet header and
 		 * has zero length payload. So, ask ST CORE to
 		 * forward the packet to protocol driver (BT/FM/GPS)
 		 */
 		st_send_frame(chnl_id, st_gdata);
 
 	} else if (len > room) {
-		/* Received packet's payload length is larger.
+		/*
+		 * Received packet's payload length is larger.
 		 * We can't accommodate it in created skb.
 		 */
 		pr_err("Data length is too large len %d room %d", len,
 			   room);
 		kfree_skb(st_gdata->rx_skb);
 	} else {
-		/* Packet header has non-zero payload length and
+		/*
+		 * Packet header has non-zero payload length and
 		 * we have enough space in created skb. Lets read
 		 * payload data */
 		st_gdata->rx_state = ST_W4_DATA;
@@ -178,8 +183,7 @@ static inline int st_check_data_len(struct st_data_s *st_gdata,
 		return len;
 	}
 
-	/* Change ST state to continue to process next
-	 * packet */
+	/* Change ST state to continue to process next packet */
 	st_gdata->rx_state = ST_W4_PACKET_TYPE;
 	st_gdata->rx_skb = NULL;
 	st_gdata->rx_count = 0;
@@ -188,7 +192,7 @@ static inline int st_check_data_len(struct st_data_s *st_gdata,
 	return 0;
 }
 
-/**
+/*
  * st_wakeup_ack - internal function for action when wake-up ack
  *	received
  */
@@ -199,7 +203,8 @@ static inline void st_wakeup_ack(struct st_data_s *st_gdata,
 	unsigned long flags = 0;
 
 	spin_lock_irqsave(&st_gdata->lock, flags);
-	/* de-Q from waitQ and Q in txQ now that the
+	/*
+	 * de-Q from waitQ and Q in txQ now that the
 	 * chip is awake
 	 */
 	while ((waiting_skb = skb_dequeue(&st_gdata->tx_waitq)))
@@ -213,7 +218,7 @@ static inline void st_wakeup_ack(struct st_data_s *st_gdata,
 	st_tx_wakeup(st_gdata);
 }
 
-/**
+/*
  * st_int_recv - ST's internal receive function.
  *	Decodes received RAW data and forwards to corresponding
  *	client drivers (Bluetooth,FM,GPS..etc).
@@ -262,8 +267,10 @@ void st_int_recv(void *disc_data,
 			/* Waiting for complete packet ? */
 			case ST_W4_DATA:
 				pr_debug("Complete pkt received");
-				/* Ask ST CORE to forward
-				 * the packet to protocol driver */
+				/*
+				 * Ask ST CORE to forward
+				 * the packet to protocol driver
+				 */
 				st_send_frame(st_gdata->rx_chnl, st_gdata);
 
 				st_gdata->rx_state = ST_W4_PACKET_TYPE;
@@ -276,7 +283,7 @@ void st_int_recv(void *disc_data,
 				&st_gdata->rx_skb->data
 				[proto->offset_len_in_hdr];
 				pr_debug("plen pointing to %x\n", *plen);
-				if (proto->len_size == 1)/* 1 byte len field */
+				if (proto->len_size == 1) /* 1 byte len field */
 					payload_len = *(unsigned char *)plen;
 				else if (proto->len_size == 2)
 					payload_len =
@@ -294,18 +301,23 @@ void st_int_recv(void *disc_data,
 		}
 
 		/* end of if rx_count */
-		/* Check first byte of packet and identify module
-		 * owner (BT/FM/GPS) */
+
+		/*
+		 * Check first byte of packet and identify module
+		 * owner (BT/FM/GPS)
+		 */
 		switch (*ptr) {
 		case LL_SLEEP_IND:
 		case LL_SLEEP_ACK:
 		case LL_WAKE_UP_IND:
 			pr_debug("PM packet");
-			/* this takes appropriate action based on
+			/*
+			 * this takes appropriate action based on
 			 * sleep state received --
 			 */
 			st_ll_sleep_state(st_gdata, *ptr);
-			/* if WAKEUP_IND collides copy from waitq to txq
+			/*
+			 * if WAKEUP_IND collides copy from waitq to txq
 			 * and assume chip awake
 			 */
 			spin_unlock_irqrestore(&st_gdata->lock, flags);
@@ -331,7 +343,8 @@ void st_int_recv(void *disc_data,
 		default:
 			type = *ptr;
 
-			/* Default case means non-HCILL packets,
+			/*
+			 * Default case means non-HCILL packets,
 			 * possibilities are packets for:
 			 * (a) valid protocol -  Supported Protocols within
 			 *     the ST_MAX_CHANNELS.
@@ -377,7 +390,7 @@ void st_int_recv(void *disc_data,
 	return;
 }
 
-/**
+/*
  * st_int_dequeue - internal de-Q function.
  *	If the previous data set was not written
  *	completely, return that skb which has the pending data.
@@ -396,7 +409,7 @@ static struct sk_buff *st_int_dequeue(struct st_data_s *st_gdata)
 	return skb_dequeue(&st_gdata->txq);
 }
 
-/**
+/*
  * st_int_enqueue - internal Q-ing function.
  *	Will either Q the skb to txq or the tx_waitq
  *	depending on the ST LL state.
@@ -561,7 +574,8 @@ long st_register(struct st_proto_s *new_proto)
 		/* release lock previously held - re-locked below */
 		spin_unlock_irqrestore(&st_gdata->lock, flags);
 
-		/* this may take a while to complete
+		/*
+		 * this may take a while to complete
 		 * since it involves BT fw download
 		 */
 		err = st_kim_start(st_gdata->kim_data);
@@ -583,7 +597,8 @@ long st_register(struct st_proto_s *new_proto)
 		clear_bit(ST_REG_IN_PROGRESS, &st_gdata->st_state);
 		st_recv = st_int_recv;
 
-		/* this is where all pending registration
+		/*
+		 * this is where all pending registration
 		 * are signalled to be complete by calling callback functions
 		 */
 		if ((st_gdata->protos_registered != ST_EMPTY) &&
@@ -593,7 +608,8 @@ long st_register(struct st_proto_s *new_proto)
 		}
 		clear_bit(ST_REG_PENDING, &st_gdata->st_state);
 
-		/* check for already registered once more,
+		/*
+		 * check for already registered once more,
 		 * since the above check is old
 		 */
 		if (st_gdata->is_registered[new_proto->chnl_id] == true) {
@@ -622,7 +638,8 @@ long st_register(struct st_proto_s *new_proto)
 }
 EXPORT_SYMBOL_GPL(st_register);
 
-/* to unregister a protocol -
+/*
+ * to unregister a protocol -
  * to be called from protocol stack driver
  */
 long st_unregister(struct st_proto_s *proto)
@@ -742,7 +759,8 @@ static void st_tty_close(struct tty_struct *tty)
 
 	pr_info("%s ", __func__);
 
-	/* TODO:
+	/*
+	 * TODO:
 	 * if a protocol has been registered & line discipline
 	 * un-installed for some reason - what should be done ?
 	 */
@@ -795,7 +813,8 @@ static void st_tty_receive(struct tty_struct *tty, const unsigned char *data,
 	pr_debug("done %s", __func__);
 }
 
-/* wake-up function called in from the TTY layer
+/*
+ * wake-up function called in from the TTY layer
  * inside the internal wakeup function will be called
  */
 static void st_tty_wakeup(struct tty_struct *tty)
-- 
2.25.1

Re: [PATCH 02/10] misc: ti-st: st_core: Tidy-up bespoke commentry

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> If it's still in use and worth the effort, it sure looks like
> this driver could do with a good scrub (clean).
>
> This patch conserns itself with the non-standard comments located
> thoughout the file.
>
> It also fixes the following W=1 warnings by demoting the kerneldoc
> function headers to standard comments, since there doesn't appear
> to be a requirement for the function args to be documented:
>
>  drivers/misc/ti-st/st_core.c:132: warning: Function parameter or member 'st_gdata' not described in 'st_reg_complete'
>  drivers/misc/ti-st/st_core.c:132: warning: Function parameter or member 'err' not described in 'st_reg_complete'
>  drivers/misc/ti-st/st_core.c:197: warning: Function parameter or member 'st_gdata' not described in 'st_wakeup_ack'
>  drivers/misc/ti-st/st_core.c:197: warning: Function parameter or member 'cmd' not described in 'st_wakeup_ack'
>  drivers/misc/ti-st/st_core.c:226: warning: Function parameter or member 'disc_data' not described in 'st_int_recv'
>  drivers/misc/ti-st/st_core.c:226: warning: Function parameter or member 'data' not described in 'st_int_recv'
>  drivers/misc/ti-st/st_core.c:226: warning: Function parameter or member 'count' not described in 'st_int_recv'
>  drivers/misc/ti-st/st_core.c:387: warning: Function parameter or member 'st_gdata' not described in 'st_int_dequeue'
>  drivers/misc/ti-st/st_core.c:409: warning: Function parameter or member 'st_gdata' not described in 'st_int_enqueue'
>  drivers/misc/ti-st/st_core.c:409: warning: Function parameter or member 'skb' not described in 'st_int_enqueue'
>
> Cc: Pavan Savoy <pavan_savoy@ti.com>
> Cc: Naveen Jain <naveen_jain@ti.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 03/10] misc: ti-st: st_kim: Tidy-up bespoke commentry

[Lee Jones]

If it's still in use and worth the effort, it sure looks like
this driver could do with a good scrub (clean).

This patch conserns itself with the non-standard comments located
thoughout the file.

It also fixes the following W=1 warnings by demoting the kerneldoc
function headers to standard comments, since there doesn't appear
to be a requirement for the function args to be documented:

 /drivers/misc/ti-st/st_kim.c:42: warning: Function parameter or member 'id' not described in 'st_get_plat_device'
 /drivers/misc/ti-st/st_kim.c:53: warning: Function parameter or member 'kim_gdata' not described in 'validate_firmware_response'
 /drivers/misc/ti-st/st_kim.c:126: warning: Function parameter or member 'kim_gdata' not described in 'kim_int_recv'
 /drivers/misc/ti-st/st_kim.c:126: warning: Function parameter or member 'data' not described in 'kim_int_recv'
 /drivers/misc/ti-st/st_kim.c:126: warning: Function parameter or member 'count' not described in 'kim_int_recv'
 /drivers/misc/ti-st/st_kim.c:272: warning: Function parameter or member 'kim_gdata' not described in 'download_firmware'
 /drivers/misc/ti-st/st_kim.c:445: warning: Function parameter or member 'kim_data' not described in 'st_kim_start'
 /drivers/misc/ti-st/st_kim.c:509: warning: Function parameter or member 'kim_data' not described in 'st_kim_stop'
 /drivers/misc/ti-st/st_kim.c:661: warning: Function parameter or member 'core_data' not described in 'st_kim_ref'
 /drivers/misc/ti-st/st_kim.c:661: warning: Function parameter or member 'id' not described in 'st_kim_ref'

Cc: Pavan Savoy <pavan_savoy@ti.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/ti-st/st_kim.c | 71 +++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 27 deletions(-)

diff --git a/drivers/misc/ti-st/st_kim.c b/drivers/misc/ti-st/st_kim.c
index a36ed1ff5967f..f2f6cab97c086 100644
--- a/drivers/misc/ti-st/st_kim.c
+++ b/drivers/misc/ti-st/st_kim.c
@@ -30,7 +30,7 @@ static struct platform_device *st_kim_devices[MAX_ST_DEVICES];
 /**********************************************************************/
 /* internal functions */
 
-/**
+/*
  * st_get_plat_device -
  *	function which returns the reference to the platform device
  *	requested by id. As of now only 1 such device exists (id=0)
@@ -43,7 +43,7 @@ static struct platform_device *st_get_plat_device(int id)
 	return st_kim_devices[id];
 }
 
-/**
+/*
  * validate_firmware_response -
  *	function to return whether the firmware response was proper
  *	in case of error don't complete so that waiting for proper
@@ -55,7 +55,8 @@ static void validate_firmware_response(struct kim_data_s *kim_gdata)
 	if (!skb)
 		return;
 
-	/* these magic numbers are the position in the response buffer which
+	/*
+	 * these magic numbers are the position in the response buffer which
 	 * allows us to distinguish whether the response is for the read
 	 * version info. command
 	 */
@@ -79,7 +80,8 @@ static void validate_firmware_response(struct kim_data_s *kim_gdata)
 	kfree_skb(skb);
 }
 
-/* check for data len received inside kim_int_recv
+/*
+ * check for data len received inside kim_int_recv
  * most often hit the last case to update state to waiting for data
  */
 static inline int kim_check_data_len(struct kim_data_s *kim_gdata, int len)
@@ -91,14 +93,16 @@ static inline int kim_check_data_len(struct kim_data_s *kim_gdata, int len)
 	if (!len) {
 		validate_firmware_response(kim_gdata);
 	} else if (len > room) {
-		/* Received packet's payload length is larger.
+		/*
+		 * Received packet's payload length is larger.
 		 * We can't accommodate it in created skb.
 		 */
 		pr_err("Data length is too large len %d room %d", len,
 			   room);
 		kfree_skb(kim_gdata->rx_skb);
 	} else {
-		/* Packet header has non-zero payload length and
+		/*
+		 * Packet header has non-zero payload length and
 		 * we have enough space in created skb. Lets read
 		 * payload data */
 		kim_gdata->rx_state = ST_W4_DATA;
@@ -106,8 +110,10 @@ static inline int kim_check_data_len(struct kim_data_s *kim_gdata, int len)
 		return len;
 	}
 
-	/* Change ST LL state to continue to process next
-	 * packet */
+	/*
+	 * Change ST LL state to continue to process next
+	 * packet
+	 */
 	kim_gdata->rx_state = ST_W4_PACKET_TYPE;
 	kim_gdata->rx_skb = NULL;
 	kim_gdata->rx_count = 0;
@@ -115,7 +121,7 @@ static inline int kim_check_data_len(struct kim_data_s *kim_gdata, int len)
 	return 0;
 }
 
-/**
+/*
  * kim_int_recv - receive function called during firmware download
  *	firmware download responses on different UART drivers
  *	have been observed to come in bursts of different
@@ -216,7 +222,8 @@ static long read_local_version(struct kim_data_s *kim_gdata, char *bts_scr_name)
 		return timeout ? -ERESTARTSYS : -ETIMEDOUT;
 	}
 	reinit_completion(&kim_gdata->kim_rcvd);
-	/* the positions 12 & 13 in the response buffer provide with the
+	/*
+	 * the positions 12 & 13 in the response buffer provide with the
 	 * chip, major & minor numbers
 	 */
 
@@ -263,7 +270,7 @@ static void skip_change_remote_baud(unsigned char **ptr, long *len)
 	}
 }
 
-/**
+/*
  * download_firmware -
  *	internal function which parses through the .bts firmware
  *	script file intreprets SEND, DELAY actions only as of now
@@ -295,7 +302,8 @@ static long download_firmware(struct kim_data_s *kim_gdata)
 	}
 	ptr = (void *)kim_gdata->fw_entry->data;
 	len = kim_gdata->fw_entry->size;
-	/* bts_header to remove out magic number and
+	/*
+	 * bts_header to remove out magic number and
 	 * version
 	 */
 	ptr += sizeof(struct bts_header);
@@ -313,8 +321,10 @@ static long download_firmware(struct kim_data_s *kim_gdata)
 			if (unlikely
 			    (((struct hci_command *)action_ptr)->opcode ==
 			     0xFF36)) {
-				/* ignore remote change
-				 * baud rate HCI VS command */
+				/*
+				 * ignore remote change
+				 * baud rate HCI VS command
+				 */
 				pr_warn("change remote baud"
 				    " rate command in firmware");
 				skip_change_remote_baud(&ptr, &len);
@@ -346,7 +356,8 @@ static long download_firmware(struct kim_data_s *kim_gdata)
 				release_firmware(kim_gdata->fw_entry);
 				return -ETIMEDOUT;
 			}
-			/* reinit completion before sending for the
+			/*
+			 * reinit completion before sending for the
 			 * relevant wait
 			 */
 			reinit_completion(&kim_gdata->kim_rcvd);
@@ -418,14 +429,16 @@ void st_kim_recv(void *disc_data, const unsigned char *data, long count)
 	struct st_data_s	*st_gdata = (struct st_data_s *)disc_data;
 	struct kim_data_s	*kim_gdata = st_gdata->kim_data;
 
-	/* proceed to gather all data and distinguish read fw version response
+	/*
+	 * proceed to gather all data and distinguish read fw version response
 	 * from other fw responses when data gathering is complete
 	 */
 	kim_int_recv(kim_gdata, data, count);
 	return;
 }
 
-/* to signal completion of line discipline installation
+/*
+ * to signal completion of line discipline installation
  * called from ST Core, upon tty_open
  */
 void st_kim_complete(void *kim_data)
@@ -434,7 +447,7 @@ void st_kim_complete(void *kim_data)
 	complete(&kim_gdata->ldisc_installed);
 }
 
-/**
+/*
  * st_kim_start - called from ST Core upon 1st registration
  *	This involves toggling the chip enable gpio, reading
  *	the firmware version from chip, forming the fw file name
@@ -472,8 +485,10 @@ long st_kim_start(void *kim_data)
 		err = wait_for_completion_interruptible_timeout(
 			&kim_gdata->ldisc_installed, msecs_to_jiffies(LDISC_TIME));
 		if (!err) {
-			/* ldisc installation timeout,
-			 * flush uart, power cycle BT_EN */
+			/*
+			 * ldisc installation timeout,
+			 * flush uart, power cycle BT_EN
+			 */
 			pr_err("ldisc installation timeout");
 			err = st_kim_stop(kim_gdata);
 			continue;
@@ -482,8 +497,10 @@ long st_kim_start(void *kim_data)
 			pr_info("line discipline installed");
 			err = download_firmware(kim_gdata);
 			if (err != 0) {
-				/* ldisc installed but fw download failed,
-				 * flush uart & power cycle BT_EN */
+				/*
+				 * ldisc installed but fw download failed,
+				 * flush uart & power cycle BT_EN
+				 */
 				pr_err("download firmware failed");
 				err = st_kim_stop(kim_gdata);
 				continue;
@@ -495,7 +512,7 @@ long st_kim_start(void *kim_data)
 	return err;
 }
 
-/**
+/*
  * st_kim_stop - stop communication with chip.
  *	This can be called from ST Core/KIM, on the-
  *	(a) last un-register when chip need not be powered there-after,
@@ -650,7 +667,7 @@ static const struct attribute_group uim_attr_grp = {
 	.attrs = uim_attrs,
 };
 
-/**
+/*
  * st_kim_ref - reference the core's data
  *	This references the per-ST platform device in the arch/xx/
  *	board-xx.c file.
@@ -729,8 +746,7 @@ static int kim_probe(struct platform_device *pdev)
 		pr_err(" unable to configure gpio %d", kim_gdata->nshutdown);
 		goto err_sysfs_group;
 	}
-	/* get reference of pdev for request_firmware
-	 */
+	/* get reference of pdev for request_firmware */
 	kim_gdata->kim_pdev = pdev;
 	init_completion(&kim_gdata->kim_rcvd);
 	init_completion(&kim_gdata->ldisc_installed);
@@ -772,7 +788,8 @@ static int kim_remove(struct platform_device *pdev)
 
 	kim_gdata = platform_get_drvdata(pdev);
 
-	/* Free the Bluetooth/FM/GPIO
+	/*
+	 * Free the Bluetooth/FM/GPIO
 	 * nShutdown gpio from the system
 	 */
 	gpio_free(pdata->nshutdown_gpio);
-- 
2.25.1

Re: [PATCH 03/10] misc: ti-st: st_kim: Tidy-up bespoke commentry

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> If it's still in use and worth the effort, it sure looks like
> this driver could do with a good scrub (clean).
>
> This patch conserns itself with the non-standard comments located
> thoughout the file.
>
> It also fixes the following W=1 warnings by demoting the kerneldoc
> function headers to standard comments, since there doesn't appear
> to be a requirement for the function args to be documented:
>
>  /drivers/misc/ti-st/st_kim.c:42: warning: Function parameter or member 'id' not described in 'st_get_plat_device'
>  /drivers/misc/ti-st/st_kim.c:53: warning: Function parameter or member 'kim_gdata' not described in 'validate_firmware_response'
>  /drivers/misc/ti-st/st_kim.c:126: warning: Function parameter or member 'kim_gdata' not described in 'kim_int_recv'
>  /drivers/misc/ti-st/st_kim.c:126: warning: Function parameter or member 'data' not described in 'kim_int_recv'
>  /drivers/misc/ti-st/st_kim.c:126: warning: Function parameter or member 'count' not described in 'kim_int_recv'
>  /drivers/misc/ti-st/st_kim.c:272: warning: Function parameter or member 'kim_gdata' not described in 'download_firmware'
>  /drivers/misc/ti-st/st_kim.c:445: warning: Function parameter or member 'kim_data' not described in 'st_kim_start'
>  /drivers/misc/ti-st/st_kim.c:509: warning: Function parameter or member 'kim_data' not described in 'st_kim_stop'
>  /drivers/misc/ti-st/st_kim.c:661: warning: Function parameter or member 'core_data' not described in 'st_kim_ref'
>  /drivers/misc/ti-st/st_kim.c:661: warning: Function parameter or member 'id' not described in 'st_kim_ref'
>
> Cc: Pavan Savoy <pavan_savoy@ti.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 04/10] misc: lkdtm: bugs: At least try to use popuated variable

[Lee Jones]

The result may not be intereresting, but not using a set variable
is bad form and causes W=1 kernel builds to complain.

Fixes the following W=1 warning(s):

 drivers/misc/lkdtm/bugs.c: In function ‘lkdtm_STACK_GUARD_PAGE_LEADING’:
 drivers/misc/lkdtm/bugs.c:331:25: warning: variable ‘byte’ set but not used [-Wunused-but-set-variable]
 331 | volatile unsigned char byte;
 | ^~~~
 drivers/misc/lkdtm/bugs.c: In function ‘lkdtm_STACK_GUARD_PAGE_TRAILING’:
 drivers/misc/lkdtm/bugs.c:345:25: warning: variable ‘byte’ set but not used [-Wunused-but-set-variable]
 345 | volatile unsigned char byte;
 | ^~~~

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/lkdtm/bugs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index 736675f0a2464..4f94296fc58ad 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -334,7 +334,7 @@ void lkdtm_STACK_GUARD_PAGE_LEADING(void)
 
 	byte = *ptr;
 
-	pr_err("FAIL: accessed page before stack!\n");
+	pr_err("FAIL: accessed page before stack! (byte: %x)\n", byte);
 }
 
 /* Test that VMAP_STACK is actually allocating with a trailing guard page */
@@ -348,7 +348,7 @@ void lkdtm_STACK_GUARD_PAGE_TRAILING(void)
 
 	byte = *ptr;
 
-	pr_err("FAIL: accessed page after stack!\n");
+	pr_err("FAIL: accessed page after stack! (byte: %x)\n", byte);
 }
 
 void lkdtm_UNSET_SMEP(void)
-- 
2.25.1

Re: [PATCH 04/10] misc: lkdtm: bugs: At least try to use popuated variable

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> The result may not be intereresting, but not using a set variable
> is bad form and causes W=1 kernel builds to complain.
>
> Fixes the following W=1 warning(s):
>
>  drivers/misc/lkdtm/bugs.c: In function ‘lkdtm_STACK_GUARD_PAGE_LEADING’:
>  drivers/misc/lkdtm/bugs.c:331:25: warning: variable ‘byte’ set but not used [-Wunused-but-set-variable]
>  331 | volatile unsigned char byte;
>  | ^~~~
>  drivers/misc/lkdtm/bugs.c: In function ‘lkdtm_STACK_GUARD_PAGE_TRAILING’:
>  drivers/misc/lkdtm/bugs.c:345:25: warning: variable ‘byte’ set but not used [-Wunused-but-set-variable]
>  345 | volatile unsigned char byte;
>  | ^~~~
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

I think a clearer way to address this would be to add a cast to void than
to actually use the variable.

Looking at the implementation, it also seems odd to use a 'const char *' as
the source and a 'volatile char' as the destination, I would have expected
the opposite (marking the source volatile to force the access),
though I suppose the effect is the same.

    Arnd

Re: [PATCH 04/10] misc: lkdtm: bugs: At least try to use popuated variable

[Kees Cook]

On Fri, Jun 26, 2020 at 02:05:19PM +0100, Lee Jones wrote:
> The result may not be intereresting, but not using a set variable
> is bad form and causes W=1 kernel builds to complain.
> 
> Fixes the following W=1 warning(s):
> 
>  drivers/misc/lkdtm/bugs.c: In function ‘lkdtm_STACK_GUARD_PAGE_LEADING’:
>  drivers/misc/lkdtm/bugs.c:331:25: warning: variable ‘byte’ set but not used [-Wunused-but-set-variable]
>  331 | volatile unsigned char byte;
>  | ^~~~
>  drivers/misc/lkdtm/bugs.c: In function ‘lkdtm_STACK_GUARD_PAGE_TRAILING’:
>  drivers/misc/lkdtm/bugs.c:345:25: warning: variable ‘byte’ set but not used [-Wunused-but-set-variable]
>  345 | volatile unsigned char byte;
>  | ^~~~
> 
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Ah yeah, this looks like a reasonable way to deal with it. Thanks!

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH 05/10] misc: lkdtm: Always provide prototype for lkdtm_DOUBLE_FAULT()

[Lee Jones]

lkdtm_DOUBLE_FAULT() already has internal logic to handle
!CONFIG_X86_32.  Compiling out the prototype actually prevents
that logic from being useful.

Fixes the following W=1 warning:

 drivers/misc/lkdtm/bugs.c: At top level:
 drivers/misc/lkdtm/bugs.c:420:6: warning: no previous prototype for ‘lkdtm_DOUBLE_FAULT’ [-Wmissing-prototypes]
 420 | void lkdtm_DOUBLE_FAULT(void)
 | ^~~~~~~~~~~~~~~~~~

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/lkdtm/lkdtm.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index 601a2156a0d48..8878538b2c132 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -31,9 +31,7 @@ void lkdtm_CORRUPT_USER_DS(void);
 void lkdtm_STACK_GUARD_PAGE_LEADING(void);
 void lkdtm_STACK_GUARD_PAGE_TRAILING(void);
 void lkdtm_UNSET_SMEP(void);
-#ifdef CONFIG_X86_32
 void lkdtm_DOUBLE_FAULT(void);
-#endif
 void lkdtm_CORRUPT_PAC(void);
 
 /* lkdtm_heap.c */
-- 
2.25.1

Re: [PATCH 05/10] misc: lkdtm: Always provide prototype for lkdtm_DOUBLE_FAULT()

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> lkdtm_DOUBLE_FAULT() already has internal logic to handle
> !CONFIG_X86_32.  Compiling out the prototype actually prevents
> that logic from being useful.
>
> Fixes the following W=1 warning:
>
>  drivers/misc/lkdtm/bugs.c: At top level:
>  drivers/misc/lkdtm/bugs.c:420:6: warning: no previous prototype for ‘lkdtm_DOUBLE_FAULT’ [-Wmissing-prototypes]
>  420 | void lkdtm_DOUBLE_FAULT(void)
>  | ^~~~~~~~~~~~~~~~~~
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 05/10] misc: lkdtm: Always provide prototype for lkdtm_DOUBLE_FAULT()

[Kees Cook]

On Fri, Jun 26, 2020 at 02:05:20PM +0100, Lee Jones wrote:
> lkdtm_DOUBLE_FAULT() already has internal logic to handle
> !CONFIG_X86_32.  Compiling out the prototype actually prevents
> that logic from being useful.
> 
> Fixes the following W=1 warning:
> 
>  drivers/misc/lkdtm/bugs.c: At top level:
>  drivers/misc/lkdtm/bugs.c:420:6: warning: no previous prototype for ‘lkdtm_DOUBLE_FAULT’ [-Wmissing-prototypes]
>  420 | void lkdtm_DOUBLE_FAULT(void)
>  | ^~~~~~~~~~~~~~~~~~
> 
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Thanks! This change is actually already part of another
patch and is waiting for Greg to pick up:
https://lore.kernel.org/lkml/20200529200347.2464284-5-keescook@chromium.org/

-- 
Kees Cook

[PATCH 06/10] misc: eeprom: eeprom_93cx6: Repair function arg descriptions

[Lee Jones]

Copy-paste issue.  Looks like the kerneldoc style descriptions for
these functions were taken from existing functions with slightly
different argument names.

Fixes the following W=1 warnings:

 drivers/misc/eeprom/eeprom_93cx6.c:239: warning: Function parameter or member 'byte' not described in 'eeprom_93cx6_readb'
 drivers/misc/eeprom/eeprom_93cx6.c:239: warning: Excess function parameter 'word' description in 'eeprom_93cx6_readb'
 drivers/misc/eeprom/eeprom_93cx6.c:280: warning: Function parameter or member 'bytes' not described in 'eeprom_93cx6_multireadb'
 drivers/misc/eeprom/eeprom_93cx6.c:280: warning: Excess function parameter 'words' description in 'eeprom_93cx6_multireadb'

Cc: Wolfram Sang <wsa@kernel.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/eeprom/eeprom_93cx6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93cx6.c b/drivers/misc/eeprom/eeprom_93cx6.c
index 36a2eb837371b..9627294fe3e95 100644
--- a/drivers/misc/eeprom/eeprom_93cx6.c
+++ b/drivers/misc/eeprom/eeprom_93cx6.c
@@ -228,7 +228,7 @@ EXPORT_SYMBOL_GPL(eeprom_93cx6_multiread);
 /**
  * eeprom_93cx6_readb - Read a byte from eeprom
  * @eeprom: Pointer to eeprom structure
- * @word: Byte index from where we should start reading
+ * @byte: Byte index from where we should start reading
  * @data: target pointer where the information will have to be stored
  *
  * This function will read a byte of the eeprom data
@@ -270,7 +270,7 @@ EXPORT_SYMBOL_GPL(eeprom_93cx6_readb);
  * @eeprom: Pointer to eeprom structure
  * @byte: Index from where we should start reading
  * @data: target pointer where the information will have to be stored
- * @words: Number of bytes that should be read.
+ * @bytes: Number of bytes that should be read.
  *
  * This function will read all requested bytes from the eeprom,
  * this is done by calling eeprom_93cx6_readb() multiple times.
-- 
2.25.1

Re: [PATCH 06/10] misc: eeprom: eeprom_93cx6: Repair function arg descriptions

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> Copy-paste issue.  Looks like the kerneldoc style descriptions for
> these functions were taken from existing functions with slightly
> different argument names.
>
> Fixes the following W=1 warnings:
>
>  drivers/misc/eeprom/eeprom_93cx6.c:239: warning: Function parameter or member 'byte' not described in 'eeprom_93cx6_readb'
>  drivers/misc/eeprom/eeprom_93cx6.c:239: warning: Excess function parameter 'word' description in 'eeprom_93cx6_readb'
>  drivers/misc/eeprom/eeprom_93cx6.c:280: warning: Function parameter or member 'bytes' not described in 'eeprom_93cx6_multireadb'
>  drivers/misc/eeprom/eeprom_93cx6.c:280: warning: Excess function parameter 'words' description in 'eeprom_93cx6_multireadb'
>
> Cc: Wolfram Sang <wsa@kernel.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 06/10] misc: eeprom: eeprom_93cx6: Repair function arg descriptions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]

> @@ -270,7 +270,7 @@ EXPORT_SYMBOL_GPL(eeprom_93cx6_readb);
>   * @eeprom: Pointer to eeprom structure
>   * @byte: Index from where we should start reading
>   * @data: target pointer where the information will have to be stored
> - * @words: Number of bytes that should be read.
> + * @bytes: Number of bytes that should be read.

Now we have 'byte' and 'bytes' here as arguments which is confusing. I
think renaming 'words' into 'num_bytes' would be even better.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/10] misc: eeprom: eeprom_93cx6: Repair function arg descriptions

[Lee Jones]

On Sat, 27 Jun 2020, Wolfram Sang wrote:

> > @@ -270,7 +270,7 @@ EXPORT_SYMBOL_GPL(eeprom_93cx6_readb);
> >   * @eeprom: Pointer to eeprom structure
> >   * @byte: Index from where we should start reading
> >   * @data: target pointer where the information will have to be stored
> > - * @words: Number of bytes that should be read.
> > + * @bytes: Number of bytes that should be read.
> 
> Now we have 'byte' and 'bytes' here as arguments which is confusing. I
> think renaming 'words' into 'num_bytes' would be even better.

I await your patch with bated breath. :)

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 06/10] misc: eeprom: eeprom_93cx6: Repair function arg descriptions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]


> > > @@ -270,7 +270,7 @@ EXPORT_SYMBOL_GPL(eeprom_93cx6_readb);
> > >   * @eeprom: Pointer to eeprom structure
> > >   * @byte: Index from where we should start reading
> > >   * @data: target pointer where the information will have to be stored
> > > - * @words: Number of bytes that should be read.
> > > + * @bytes: Number of bytes that should be read.
> > 
> > Now we have 'byte' and 'bytes' here as arguments which is confusing. I
> > think renaming 'words' into 'num_bytes' would be even better.
> 
> I await your patch with bated breath. :)

? You are touching it already, why a second patch?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/10] misc: eeprom: eeprom_93cx6: Repair function arg descriptions

[Lee Jones]

On Mon, 29 Jun 2020, Wolfram Sang wrote:

> 
> > > > @@ -270,7 +270,7 @@ EXPORT_SYMBOL_GPL(eeprom_93cx6_readb);
> > > >   * @eeprom: Pointer to eeprom structure
> > > >   * @byte: Index from where we should start reading
> > > >   * @data: target pointer where the information will have to be stored
> > > > - * @words: Number of bytes that should be read.
> > > > + * @bytes: Number of bytes that should be read.
> > > 
> > > Now we have 'byte' and 'bytes' here as arguments which is confusing. I
> > > think renaming 'words' into 'num_bytes' would be even better.
> > 
> > I await your patch with bated breath. :)
> 
> ? You are touching it already, why a second patch?

Because it's a different change.  One that's orthogonal to this set,
which is designed simply to ensure the documentation matches reality.

The author decided on this (less than ideal [in our humble opinion])
nomenclature from the function's inception back in 2013.  Maybe there
are good reasons for it to be this way.  Either way, it might require
a dialogue.  For this set I'd rather stick to the script.

That said, I genuinely don't mind drafting a patch to fix this.  If I
am to do so, it would also be as part as a subsequent effort.

You or me - your call.  Happy either way.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 07/10] misc: mic: vop: vop_main: Remove set but unused variable 'ret'

[Lee Jones]

Hasn't been checked since its conception 2 years ago.

Squashes W=1 warning:

 drivers/misc/mic/vop/vop_main.c: In function ‘_vop_scan_devices’:
 drivers/misc/mic/vop/vop_main.c:617:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable]
 617 | int ret;
 | ^~~

Cc: Sudeep Dutt <sudeep.dutt@intel.com>
Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/mic/vop/vop_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index 85942f6717c57..4e63cb1360921 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -614,7 +614,6 @@ static void _vop_scan_devices(void __iomem *dp, struct vop_device *vpdev,
 	struct mic_device_desc __iomem *d;
 	struct mic_device_ctrl __iomem *dc;
 	struct device *dev;
-	int ret;
 
 	for (i = sizeof(struct mic_bootparam);
 			i < MIC_DP_SIZE; i += _vop_total_desc_size(d)) {
@@ -644,7 +643,7 @@ static void _vop_scan_devices(void __iomem *dp, struct vop_device *vpdev,
 					 &dc->config_change);
 			put_device(dev);
 			_vop_handle_config_change(d, i, vpdev);
-			ret = _vop_remove_device(d, i, vpdev);
+			_vop_remove_device(d, i, vpdev);
 			if (remove) {
 				iowrite8(0, &dc->config_change);
 				iowrite8(0, &dc->guest_ack);
-- 
2.25.1

Re: [PATCH 07/10] misc: mic: vop: vop_main: Remove set but unused variable 'ret'

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> Hasn't been checked since its conception 2 years ago.
>
> Squashes W=1 warning:
>
>  drivers/misc/mic/vop/vop_main.c: In function ‘_vop_scan_devices’:
>  drivers/misc/mic/vop/vop_main.c:617:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable]
>  617 | int ret;
>  | ^~~
>
> Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

This is a correct change, but I'd take it one step further and make the
_vop_remove_device() function return 'void' if you don't mind
respinning the patch.

Either way

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 07/10] misc: mic: vop: vop_main: Remove set but unused variable 'ret'

[Lee Jones]

On Fri, 26 Jun 2020, Arnd Bergmann wrote:

> On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > Hasn't been checked since its conception 2 years ago.
> >
> > Squashes W=1 warning:
> >
> >  drivers/misc/mic/vop/vop_main.c: In function ‘_vop_scan_devices’:
> >  drivers/misc/mic/vop/vop_main.c:617:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable]
> >  617 | int ret;
> >  | ^~~
> >
> > Cc: Sudeep Dutt <sudeep.dutt@intel.com>
> > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> This is a correct change, but I'd take it one step further and make the
> _vop_remove_device() function return 'void' if you don't mind
> respinning the patch.
> 
> Either way
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks.

Do you mind if I handle your request as a subsequent patch?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 07/10] misc: mic: vop: vop_main: Remove set but unused variable 'ret'

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 5:29 PM Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 26 Jun 2020, Arnd Bergmann wrote:
> > On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
> > This is a correct change, but I'd take it one step further and make the
> > _vop_remove_device() function return 'void' if you don't mind
> > respinning the patch.
> >
> > Either way
> >
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> Do you mind if I handle your request as a subsequent patch?

That also works for me, thanks!

      Arnd

[PATCH 08/10] misc: cb710: sgbuf2: Add missing documentation for cb710_sg_dwiter_write_next_block()'s 'data' arg

[Lee Jones]

An attempt was made to provide a proper kerneldoc header for
cb710_sg_dwiter_write_next_block(), but a description for it's 'data'
argument was missed.

Squashes W=1 kernel build warning:

 drivers/misc/cb710/sgbuf2.c:131: warning: Function parameter or member 'data' not described in 'cb710_sg_dwiter_write_next_block'

Cc: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/cb710/sgbuf2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/cb710/sgbuf2.c b/drivers/misc/cb710/sgbuf2.c
index dfd2969e36289..e5a4ed3701eb8 100644
--- a/drivers/misc/cb710/sgbuf2.c
+++ b/drivers/misc/cb710/sgbuf2.c
@@ -117,6 +117,7 @@ static void sg_dwiter_write_slow(struct sg_mapping_iter *miter, uint32_t data)
 /**
  * cb710_sg_dwiter_write_next_block() - write next 32-bit word to sg buffer
  * @miter: sg mapping iterator used for writing
+ * @data: data to write to sg buffer
  *
  * Description:
  *   Writes 32-bit word starting at byte pointed to by @miter@
-- 
2.25.1

Re: [PATCH 08/10] misc: cb710: sgbuf2: Add missing documentation for cb710_sg_dwiter_write_next_block()'s 'data' arg

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> An attempt was made to provide a proper kerneldoc header for
> cb710_sg_dwiter_write_next_block(), but a description for it's 'data'
> argument was missed.
>
> Squashes W=1 kernel build warning:
>
>  drivers/misc/cb710/sgbuf2.c:131: warning: Function parameter or member 'data' not described in 'cb710_sg_dwiter_write_next_block'
>
> Cc: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 08/10] misc: cb710: sgbuf2: Add missing documentation for cb710_sg_dwiter_write_next_block()'s 'data' arg

[Michał Mirosław]

On Fri, Jun 26, 2020 at 02:05:23PM +0100, Lee Jones wrote:
> An attempt was made to provide a proper kerneldoc header for
> cb710_sg_dwiter_write_next_block(), but a description for it's 'data'
> argument was missed.
> 
> Squashes W=1 kernel build warning:
> 
>  drivers/misc/cb710/sgbuf2.c:131: warning: Function parameter or member 'data' not described in 'cb710_sg_dwiter_write_next_block'
[...]

Acked-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

[PATCH 09/10] misc: habanalabs: irq: Add missing struct identifier for 'struct hl_eqe_work'

[Lee Jones]

In kerneldoc format, data structures have to start with 'struct'
else the kerneldoc tooling/parsers/validators get confused.

Squashes the following W=1 warning:

 drivers/misc/habanalabs/irq.c:19: warning: cannot understand function prototype: 'struct hl_eqe_work '

Cc: Oded Gabbay <oded.gabbay@gmail.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/habanalabs/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/habanalabs/irq.c b/drivers/misc/habanalabs/irq.c
index fac65fbd70e81..4e77a73857793 100644
--- a/drivers/misc/habanalabs/irq.c
+++ b/drivers/misc/habanalabs/irq.c
@@ -10,7 +10,8 @@
 #include <linux/slab.h>
 
 /**
- * This structure is used to schedule work of EQ entry and armcp_reset event
+ * struct hl_eqe_work - This structure is used to schedule work of EQ
+ *                      entry and armcp_reset event
  *
  * @eq_work          - workqueue object to run when EQ entry is received
  * @hdev             - pointer to device structure
-- 
2.25.1

Re: [PATCH 09/10] misc: habanalabs: irq: Add missing struct identifier for 'struct hl_eqe_work'

[Oded Gabbay]

On Fri, Jun 26, 2020 at 4:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> In kerneldoc format, data structures have to start with 'struct'
> else the kerneldoc tooling/parsers/validators get confused.
>
> Squashes the following W=1 warning:
>
>  drivers/misc/habanalabs/irq.c:19: warning: cannot understand function prototype: 'struct hl_eqe_work '
>
> Cc: Oded Gabbay <oded.gabbay@gmail.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/misc/habanalabs/irq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/habanalabs/irq.c b/drivers/misc/habanalabs/irq.c
> index fac65fbd70e81..4e77a73857793 100644
> --- a/drivers/misc/habanalabs/irq.c
> +++ b/drivers/misc/habanalabs/irq.c
> @@ -10,7 +10,8 @@
>  #include <linux/slab.h>
>
>  /**
> - * This structure is used to schedule work of EQ entry and armcp_reset event
> + * struct hl_eqe_work - This structure is used to schedule work of EQ
> + *                      entry and armcp_reset event
>   *
>   * @eq_work          - workqueue object to run when EQ entry is received
>   * @hdev             - pointer to device structure
> --
> 2.25.1
>

This patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>

Applied to my -fixes tree.
Oded

Re: [PATCH 09/10] misc: habanalabs: irq: Add missing struct identifier for 'struct hl_eqe_work'

[Oded Gabbay]

On Fri, Jun 26, 2020 at 4:45 PM Oded Gabbay <oded.gabbay@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 4:05 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > In kerneldoc format, data structures have to start with 'struct'
> > else the kerneldoc tooling/parsers/validators get confused.
> >
> > Squashes the following W=1 warning:
> >
> >  drivers/misc/habanalabs/irq.c:19: warning: cannot understand function prototype: 'struct hl_eqe_work '
> >
> > Cc: Oded Gabbay <oded.gabbay@gmail.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/misc/habanalabs/irq.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/habanalabs/irq.c b/drivers/misc/habanalabs/irq.c
> > index fac65fbd70e81..4e77a73857793 100644
> > --- a/drivers/misc/habanalabs/irq.c
> > +++ b/drivers/misc/habanalabs/irq.c
> > @@ -10,7 +10,8 @@
> >  #include <linux/slab.h>
> >
> >  /**
> > - * This structure is used to schedule work of EQ entry and armcp_reset event
> > + * struct hl_eqe_work - This structure is used to schedule work of EQ
> > + *                      entry and armcp_reset event
> >   *
> >   * @eq_work          - workqueue object to run when EQ entry is received
> >   * @hdev             - pointer to device structure
> > --
> > 2.25.1
> >
>
> This patch is:
> Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>
>
> Applied to my -fixes tree.
> Oded
Ah, just saw it is part of a series, so I'm not applying it to my tree.
Oded

[PATCH 10/10] misc: pti: Fix documentation for bit-rotted function pti_tty_driver_write()

[Lee Jones]

The API has moved on since the original function header was
authored.  This changes brings the function's documentation
back into line with reality, complete descriptions of the
latest arguments to be used.

Squashes the following W=1 kernel build warnings:

 drivers/misc/pti.c:510: warning: Function parameter or member 'tty' not described in 'pti_tty_driver_wr
 drivers/misc/pti.c:510: warning: Function parameter or member 'buf' not described in 'pti_tty_driver_wr
 drivers/misc/pti.c:510: warning: Excess function parameter 'filp' description in 'pti_tty_driver_write'
 drivers/misc/pti.c:510: warning: Excess function parameter 'data' description in 'pti_tty_driver_write'

Cc: J Freyensee <james_p_freyensee@linux.intel.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/misc/pti.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/pti.c b/drivers/misc/pti.c
index b7f510676cd61..07e9da7918ebd 100644
--- a/drivers/misc/pti.c
+++ b/drivers/misc/pti.c
@@ -496,9 +496,8 @@ static void pti_tty_cleanup(struct tty_struct *tty)
  * pti_tty_driver_write()-  Write trace debugging data through the char
  * interface to the PTI HW.  Part of the misc device implementation.
  *
- * @filp: Contains private data which is used to obtain
- *        master, channel write ID.
- * @data: trace data to be written.
+ * @tty: tty struct containing pti information.
+ * @buf: trace data to be written.
  * @len:  # of byte to write.
  *
  * Returns:
-- 
2.25.1

Re: [PATCH 10/10] misc: pti: Fix documentation for bit-rotted function pti_tty_driver_write()

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:05 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> The API has moved on since the original function header was
> authored.  This changes brings the function's documentation
> back into line with reality, complete descriptions of the
> latest arguments to be used.
>
> Squashes the following W=1 kernel build warnings:
>
>  drivers/misc/pti.c:510: warning: Function parameter or member 'tty' not described in 'pti_tty_driver_wr
>  drivers/misc/pti.c:510: warning: Function parameter or member 'buf' not described in 'pti_tty_driver_wr
>  drivers/misc/pti.c:510: warning: Excess function parameter 'filp' description in 'pti_tty_driver_write'
>  drivers/misc/pti.c:510: warning: Excess function parameter 'data' description in 'pti_tty_driver_write'
>
> Cc: J Freyensee <james_p_freyensee@linux.intel.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>