RE: [PATCH] usbip: fix warning in vhci_hcd_probe/lockdep_init_map

From: fx IWATA NOBUO
Date: Wed Dec 07 2016 - 23:13:30 EST

Dear Shuah,

> I noticed that in many places sysfs_attr_init() is called before populating
> the fields such as name etc. However, I don't think the order matters.
>
> sysfs_attr_init() doesn't depend on name or any other fields being set:
>
> #define sysfs_attr_init(attr) \
> do { \
> static struct lock_class_key __key; \
> \
> (attr)->key = &__key; \
> } while (0)
>
> Are you concerned about something else?

Yes.

If the macro name is 'attr_set_key', I don't have any concern.

The name is 'attr_init' so I think there's possibility that some other
items in attr may initialized in future.

Also I think 'initialize struct first, then set items' is idiomatic order.

Best Regards,

nobuo.iwata
//
> -----Original Message-----
> From: Shuah Khan [mailto:shuahkh@xxxxxxxxxxxxxxx]
> Sent: Thursday, December 08, 2016 11:51 AM
> To: fx IWATA NOBUO; Andrey Konovalov
> Cc: Greg Kroah-Hartman; Valentina Manea; Shuah Khan;
> linux-usb@xxxxxxxxxxxxxxx; LKML; Shuah Khan; Shuah Khan
> Subject: Re: [PATCH] usbip: fix warning in vhci_hcd_probe/lockdep_init_map
>
> Hi Nobuo.iwata,
>
> I noticed that in many places sysfs_attr_init() is called before populating
> the fields such as name etc. However, I don't think the order matters.
>
> sysfs_attr_init() doesn't depend on name or any other fields being set:
>
> #define sysfs_attr_init(attr) \
> do { \
> static struct lock_class_key __key; \
> \
> (attr)->key = &__key; \
> } while (0)
>
> Are you concerned about something else?
>
> thanks,
> -- Shuah
>
>
> On 12/07/2016 07:15 PM, fx IWATA NOBUO wrote:
> > Dear Shuah,
> >
> > I'm afraid there's one thing I have concerned.
> >
> > I think it's better to move sysfs_attr_init() before
> > status->attr.attr.name = status->name;
> >
> > Best Regards,
> >
> > nobuo.iwata
> > //
> >> -----Original Message-----
> >> From: linux-usb-owner@xxxxxxxxxxxxxxx
> >> [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Andrey
> >> Konovalov
> >> Sent: Tuesday, December 06, 2016 9:07 PM
> >> To: Shuah Khan
> >> Cc: Greg Kroah-Hartman; Valentina Manea; Shuah Khan;
> >> linux-usb@xxxxxxxxxxxxxxx; LKML
> >> Subject: Re: [PATCH] usbip: fix warning in
> >> vhci_hcd_probe/lockdep_init_map
> >>
> >> On Mon, Dec 5, 2016 at 9:00 PM, Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
> wrote:
> >>> Hi Andrey,
> >>>
> >>> On 12/05/2016 12:56 PM, Shuah Khan wrote:
> >>>> vhci_hcd calls sysfs_create_group() with dynamically allocated
> >>>> sysfs attributes triggering the lock-class key not persistent
> >>>> warning. Call
> >>>> sysfs_attr_init() for dynamically allocated sysfs attributes to fix
> it.
> >>>>
> >>>> vhci_hcd vhci_hcd: USB/IP Virtual Host Controller vhci_hcd vhci_hcd:
> >>>> new USB bus registered, assigned bus number 2
> >>>> BUG: key ffff88006a7e8d18 not in .data!
> >>>> ------------[ cut here ]------------
> >>>> WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:3131
> >>>> lockdep_init_map+0x60c/0x770
> >>>> DEBUG_LOCKS_WARN_ON(1)[ 1.567044] Modules linked in:
> >>>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc7+ #58 Hardware
> >>>> name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >>>> ffff88006bce6eb8 ffffffff81f96c8a ffffffff00000a02
> >>>> 1ffff1000d79cd6a
> >>>> ffffed000d79cd62 000000046bce6ed8 0000000041b58ab3
> >>>> ffffffff8598af40
> >>>> ffffffff81f969f8 0000000000000000 0000000041b58ab3
> >>>> 0000000000000200 Call Trace:
> >>>> [< inline >] __dump_stack lib/dump_stack.c:15
> >>>> [<ffffffff81f96c8a>] dump_stack+0x292/0x398 lib/dump_stack.c:51
> >>>> [<ffffffff812b808f>] __warn+0x19f/0x1e0 kernel/panic.c:550
> >>>> [<ffffffff812b8195>] warn_slowpath_fmt+0xc5/0x110
> >>>> kernel/panic.c:565 [<ffffffff813f3efc>]
> >>>> lockdep_init_map+0x60c/0x770
> >>>> kernel/locking/lockdep.c:3131 [<ffffffff819e43d4>]
> >>>> __kernfs_create_file+0x114/0x2a0 fs/kernfs/file.c:954
> >> [<ffffffff819e68f5>] sysfs_add_file_mode_ns+0x225/0x520
> >> fs/sysfs/file.c:305
> >>>> [< inline >] create_files fs/sysfs/group.c:64
> >>>> [<ffffffff819e8a89>] internal_create_group+0x239/0x8f0
> >>>> fs/sysfs/group.c:134 [<ffffffff819e915f>]
> >>>> sysfs_create_group+0x1f/0x30 fs/sysfs/group.c:156
> >>>> [<ffffffff8323de24>] vhci_start+0x5b4/0x7a0
> >>>> drivers/usb/usbip/vhci_hcd.c:978 [<ffffffff82c907ca>]
> >>>> usb_add_hcd+0x8da/0x1c60 drivers/usb/core/hcd.c:2867
> >>>> [<ffffffff8323bc57>] vhci_hcd_probe+0x97/0x130
> >>>> drivers/usb/usbip/vhci_hcd.c:1103
> >>>> ---
> >>>> ---
> >>>> ---[ end trace c33c7b202cf3aac8 ]---
> >>>>
> >>>> Signed-off-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
> >>>> Reported-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> >>>
> >>> Here is the fix. Fixed the warning I reproduced on my system.
> >>> Let me know if it works for you.
> >>
> >> Hi Shuah,
> >>
> >> This fixes the warning I've been seeing.
> >>
> >> Thanks!
> >>
> >>>
> >>> thanks,
> >>> -- Shuah
> >>>
> >>>> ---
> >>>> drivers/usb/usbip/vhci_sysfs.c | 1 +
> >>>> 1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/drivers/usb/usbip/vhci_sysfs.c
> >>>> b/drivers/usb/usbip/vhci_sysfs.c index c404017..b96e5b1 100644
> >>>> --- a/drivers/usb/usbip/vhci_sysfs.c
> >>>> +++ b/drivers/usb/usbip/vhci_sysfs.c
> >>>> @@ -361,6 +361,7 @@ static void set_status_attr(int id)
> >>>> status->attr.attr.name = status->name;
> >>>> status->attr.attr.mode = S_IRUGO;
> >>>> status->attr.show = status_show;
> >>>> + sysfs_attr_init(&status->attr.attr);
> >>>> }
> >>>>
> >>>> static int init_status_attrs(void)
> >>>>
> >>>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-usb"
> >> in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo
> >> info at http://vger.kernel.org/majordomo-info.html