Re: [linux-next:master] BUILD REGRESSION 4662b7adea50bb62e993a67f611f3be625d3df0d

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 18.07.2022 09:05:35, Oliver Hartkopp wrote:
> Hi Dario,
> 
> On 18.07.22 08:52, Dario Binacchi wrote:
> > Hi Oliver,
> > 
> > On Sun, Jul 17, 2022 at 3:58 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > > 
> > > Hi Dario,
> > > 
> > > did you see this build regression too?
> > > 
> > > On 14.07.22 03:56, kernel test robot wrote:
> > > > tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> > > > branch HEAD: 4662b7adea50bb62e993a67f611f3be625d3df0d  Add linux-next specific files for 20220713
> > > > 
> > > > Error/Warning reports:
> > > 
> > > (..)
> > > 
> > > > drivers/net/can/slcan/slcan-core.c:601:14: sparse:    void *
> > > > drivers/net/can/slcan/slcan-core.c:601:14: sparse:    void [noderef] __rcu *
> > > > drivers/net/can/slcan/slcan-core.c:601:14: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > > 
> > 
> > IMHO I think that adding '__rcu' annotation would remove the warning:
> > 
> > diff --git a/include/linux/tty.h b/include/linux/tty.h
> > index 7b0a5d478ef6..278b84f04d20 100644
> > --- a/include/linux/tty.h
> > +++ b/include/linux/tty.h
> > @@ -239,7 +239,7 @@ struct tty_struct {
> >          wait_queue_head_t write_wait;
> >          wait_queue_head_t read_wait;
> >          struct work_struct hangup_work;
> > -       void *disc_data;
> > +       void __rcu *disc_data;
> >          void *driver_data;
> >          spinlock_t files_lock;
> >          struct list_head tty_files;
> > 
> > But in the paragraph "SPARSE CHECKING OF RCU-PROTECTED POINTERS" of
> > Documentation/RCU/rcu_dereference.rst
> > we read:
> > ...
> > Unfortunately, these sorts of bugs can be extremely hard to spot during
> > review.  This is where the sparse tool comes into play, along with the
> > "__rcu" marker.  If you mark a pointer declaration, whether in a structure
> > or as a formal parameter, with "__rcu", which tells sparse to complain if
> > this pointer is accessed directly.  It will also cause sparse to complain
> > if a pointer not marked with "__rcu" is accessed using rcu_dereference()
> > and friends.
> > ...
> > Use of "__rcu" is opt-in.  If you choose not to use it, then you should
> > ignore the sparse warnings.
> > ...
> > 
> > So, I think that by adding the '__rcu' annotation we would have new
> > warnings in all those points
> > where disc_data is accessed directly (which are many more than those
> > where rcu_dereference() is used).
> > 
> > If I'm not mistaken, the warning also refers to code that my series
> > hasn't touched. Also, in the 'BUILD REGRESSION'
> 
> Oh, I didn't check that myself.
> 
> So some old code just came into focus :-/
> 
> > report, the slcan warning is found under the 'Unverified Error /
> > Warning (likely false positive, please contact us if interested)'
> > section.
> > 
> > So, can it be okay to think about leaving everything as it is, and
> > then not apply any patches to remove this warning?
> 
> Yes. With this this background leaving the code as-is seems to be
> appropriate too. Thanks for the explanation!
> 
> Maybe Marc has another opinion. So let's wait for his feedback ...

This is not a regression, so leave as is for now.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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