Message ID | 1AA17CE09ACFBD499BB5F503A7513F5B43ADFB03@us0exc08.us.sonicwall.com |
---|---|
State | Superseded |
Headers | show |
Iosif, On Thu, Jul 21, 2016 at 4:07 AM, Iosif Harutyunov <iharutyunov@sonicwall.com> wrote: > > While implementing udev rules for UBI device I ran into the problem when udev would > ignore UBI rules I created to process attachment of volume. Interestingly, when I trigger > udev ubi subsystem "change" event manually after the attachment, rules would work just fine. > > I traced problem down to UBI sysfs processing, which turned out to be racing condition. > See patch below to address the problem. The symptom is that /dev/ubiX_Y appeared, udev got a notification but reading sysfs attributes always failed with -ENODEV? How did you debug this? Sounds like a painful issue to debug. ;-\ > Thanks. > Iosif,_ > > Signed-off-by: Iosif Harutyunov <iharutyunov@sonicwall.com> > --- > Once ubi is attached, make sure ubi_devices[] is initialized early before being > used in the dev_attribute_show(). > > This is to prevent race condition between udev ubi rules processing and ubi device > creation, which manifests itself ignoring udev ATTR rules. > > --- > drivers/mtd/ubi/build.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index ef36182..e6117d7 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -986,6 +986,8 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, > goto out_free; > } > > + ubi_devices[ubi_num] = ubi; > + Okay, the fix is to make sure that ubi_devices[ubi_num] is set before we init sysfs. Why do you add this before the autoresize code? I'd expect it right before uif_init().
> On Thu, Jul 21, 2016 at 4:07 AM, Iosif Harutyunov > <iharutyunov@sonicwall.com> wrote: > > > > While implementing udev rules for UBI device I ran into the problem > > when udev would ignore UBI rules I created to process attachment of > > volume. Interestingly, when I trigger udev ubi subsystem "change" event > manually after the attachment, rules would work just fine. > > > > I traced problem down to UBI sysfs processing, which turned out to be > racing condition. > > See patch below to address the problem. > > The symptom is that /dev/ubiX_Y appeared, udev got a notification but > reading sysfs attributes always failed with -ENODEV? > > How did you debug this? Sounds like a painful issue to debug. ;-\ Yes it was somewhat tricky. I actually found and debugged the problem on kernel 3.10, but when I isolated the issue, it became obvious that late initialization of ubi_devices[] is the problem. I instrumented vol_attribute_show attr() processing code with log messages and found that when udev reads attributes from sysfs, it returns ENODEV after calling ubi_get_device(vol->ubi->ubi_num) to obtain ubi descriptor from given device number. This is where I found that ubi_devices array element corresponding to ubi number is not initialized, which brought me to the ubi_attach_mtd_dev() function. I am surprised that this hasn't been caught before, but most likely not all platform implementations induce this racing condition between kernel and udev event processing. I my case platform is Octeon MIPS64. > > > > > Signed-off-by: Iosif Harutyunov <iharutyunov@sonicwall.com> > > --- > > Once ubi is attached, make sure ubi_devices[] is initialized early > > before being used in the dev_attribute_show(). > > > > This is to prevent race condition between udev ubi rules processing > > and ubi device creation, which manifests itself ignoring udev ATTR rules. > > > > --- > > drivers/mtd/ubi/build.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index > > ef36182..e6117d7 100644 > > --- a/drivers/mtd/ubi/build.c > > +++ b/drivers/mtd/ubi/build.c > > @@ -986,6 +986,8 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int > ubi_num, > > goto out_free; > > } > > > > + ubi_devices[ubi_num] = ubi; > > + > > Okay, the fix is to make sure that ubi_devices[ubi_num] is set before we init > sysfs. > Why do you add this before the autoresize code? > I'd expect it right before uif_init(). You are right. I think I was just being paranoid to initialize ubi_devices as early as possible after attachments is made in case if anybody else is using it. Agree with you that it should be right before uif_init(). Iosif,_
On Thu, 2016-07-21 at 02:07 +0000, Iosif Harutyunov wrote: > While implementing udev rules for UBI device I ran into the problem > when udev would > ignore UBI rules I created to process attachment of volume. > Interestingly, when I trigger > udev ubi subsystem "change" event manually after the attachment, > rules would work just fine. > > I traced problem down to UBI sysfs processing, which turned out to be > racing condition. > See patch below to address the problem. > > Thanks. > Iosif,_ > > Signed-off-by: Iosif Harutyunov <iharutyunov@sonicwall.com> > --- > Once ubi is attached, make sure ubi_devices[] is initialized early > before being > used in the dev_attribute_show(). > > This is to prevent race condition between udev ubi rules processing > and ubi device > creation, which manifests itself ignoring udev ATTR rules. Good catch! Quick feedback: this patch will probably work fine, we could apply the following logic and select a better place for that line of code. Here it is: 1. Initialize the device, make it usable. Do not make it available to anyone before that. This is the code before the 'uif_init()' invocation. BTW, "uif" means "user interfaces" in this case. 2. Make the device "available" (or "visible") by adding it to the 'ubi_devices' array. 3. Initialize user interfaces, done by 'uif_init()'. IOW, it put that line just before 'uif_init()'.
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index ef36182..e6117d7 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -986,6 +986,8 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, goto out_free; } + ubi_devices[ubi_num] = ubi; + if (ubi->autoresize_vol_id != -1) { err = autoresize(ubi, ubi->autoresize_vol_id); if (err) @@ -1036,7 +1038,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, wake_up_process(ubi->bgt_thread); spin_unlock(&ubi->wl_lock); - ubi_devices[ubi_num] = ubi; ubi_notify_all(ubi, UBI_VOLUME_ADDED, NULL); return ubi_num; @@ -1047,6 +1048,7 @@ out_uif: ubi_assert(ref); uif_close(ubi); out_detach: + ubi_devices[ubi_num] = NULL; ubi_wl_close(ubi); ubi_free_internal_volumes(ubi); vfree(ubi->vtbl);
While implementing udev rules for UBI device I ran into the problem when udev would ignore UBI rules I created to process attachment of volume. Interestingly, when I trigger udev ubi subsystem "change" event manually after the attachment, rules would work just fine. I traced problem down to UBI sysfs processing, which turned out to be racing condition. See patch below to address the problem. Thanks. Iosif,_ Signed-off-by: Iosif Harutyunov <iharutyunov@sonicwall.com> --- Once ubi is attached, make sure ubi_devices[] is initialized early before being used in the dev_attribute_show(). This is to prevent race condition between udev ubi rules processing and ubi device creation, which manifests itself ignoring udev ATTR rules. --- drivers/mtd/ubi/build.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)