| Message ID | 20180730071227.22887-5-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | genhd: register default groups with device_add_disk() | expand |
On Mon, Jul 30, 2018 at 09:12:25AM +0200, Hannes Reinecke wrote: > Use device_add_disk_with_groups() to avoid a race condition with > udev during startup. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > Cc: Ed L. Cachin <ed.cashin@acm.org> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
On Mon, Jul 30, 2018 at 3:12 AM Hannes Reinecke <hare@suse.de> wrote: > Use device_add_disk_with_groups() to avoid a race condition with > udev during startup. > I love the idea of getting rid of the race, but I am having trouble seeing what happened to the cleanup we had via sysfs_remove_group. You're storing a pointer to groups off the device, but I don't see it getting used for cleanup later in this patch set. Are you patching linux-next? -- Ed <div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jul 30, 2018 at 3:12 AM Hannes Reinecke <<a href="mailto:hare@suse.de">hare@suse.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Use device_add_disk_with_groups() to avoid a race condition with<br> udev during startup.<br></blockquote><div><br></div><div>I love the idea of getting rid of the race, but I am having trouble</div><div>seeing what happened to the cleanup we had via sysfs_remove_group.</div><div>You're storing a pointer to groups off the device, but I don't see it getting</div><div>used for cleanup later in this patch set. Are you patching linux-next?</div><div><br></div><div>-- Ed</div><div></div></div></div>
On 08/01/2018 03:00 AM, Ed Cashin wrote: > On Mon, Jul 30, 2018 at 3:12 AM Hannes Reinecke <hare@suse.de > <mailto:hare@suse.de>> wrote: > > Use device_add_disk_with_groups() to avoid a race condition with > udev during startup. > > > I love the idea of getting rid of the race, but I am having trouble > seeing what happened to the cleanup we had via sysfs_remove_group. > You're storing a pointer to groups off the device, but I don't see it > getting > used for cleanup later in this patch set. Are you patching linux-next? > And that's the beauty of this patch: you don't need to free/unlink the groups yourself. Unlinking is done in the driver core via device_del()->device_remove_attrs()->device_remove_groups(). So no separate patch needed. Cheers, Hannes
On Wed, Aug 1, 2018 at 2:57 AM Hannes Reinecke <hare@suse.de> wrote: > On 08/01/2018 03:00 AM, Ed Cashin wrote: > > On Mon, Jul 30, 2018 at 3:12 AM Hannes Reinecke <hare@suse.de > > <mailto:hare@suse.de>> wrote: > > > > Use device_add_disk_with_groups() to avoid a race condition with > > udev during startup. > > > > > > I love the idea of getting rid of the race, but I am having trouble > > seeing what happened to the cleanup we had via sysfs_remove_group. > > You're storing a pointer to groups off the device, but I don't see it > > getting > > used for cleanup later in this patch set. Are you patching linux-next? > > > And that's the beauty of this patch: you don't need to free/unlink the > groups yourself. > Unlinking is done in the driver core via > device_del()->device_remove_attrs()->device_remove_groups(). > > So no separate patch needed. > OK, thanks for the clarification.
diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h index c0ebda1283cc..015c68017a1c 100644 --- a/drivers/block/aoe/aoe.h +++ b/drivers/block/aoe/aoe.h @@ -201,7 +201,6 @@ int aoeblk_init(void); void aoeblk_exit(void); void aoeblk_gdalloc(void *); void aoedisk_rm_debugfs(struct aoedev *d); -void aoedisk_rm_sysfs(struct aoedev *d); int aoechr_init(void); void aoechr_exit(void); diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c index 429ebb84b592..ff770e7d9e52 100644 --- a/drivers/block/aoe/aoeblk.c +++ b/drivers/block/aoe/aoeblk.c @@ -177,10 +177,15 @@ static struct attribute *aoe_attrs[] = { NULL, }; -static const struct attribute_group attr_group = { +static const struct attribute_group aoe_attr_group = { .attrs = aoe_attrs, }; +static const struct attribute_group *aoe_attr_groups[] = { + &aoe_attr_group, + NULL, +}; + static const struct file_operations aoe_debugfs_fops = { .open = aoe_debugfs_open, .read = seq_read, @@ -220,17 +225,6 @@ aoedisk_rm_debugfs(struct aoedev *d) } static int -aoedisk_add_sysfs(struct aoedev *d) -{ - return sysfs_create_group(&disk_to_dev(d->gd)->kobj, &attr_group); -} -void -aoedisk_rm_sysfs(struct aoedev *d) -{ - sysfs_remove_group(&disk_to_dev(d->gd)->kobj, &attr_group); -} - -static int aoeblk_open(struct block_device *bdev, fmode_t mode) { struct aoedev *d = bdev->bd_disk->private_data; @@ -417,8 +411,7 @@ aoeblk_gdalloc(void *vp) spin_unlock_irqrestore(&d->lock, flags); - add_disk(gd); - aoedisk_add_sysfs(d); + device_add_disk(NULL, gd, aoe_attr_groups); aoedisk_add_debugfs(d); spin_lock_irqsave(&d->lock, flags); diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c index 697f735b07a4..d92fa1fe3580 100644 --- a/drivers/block/aoe/aoedev.c +++ b/drivers/block/aoe/aoedev.c @@ -275,7 +275,6 @@ freedev(struct aoedev *d) del_timer_sync(&d->timer); if (d->gd) { aoedisk_rm_debugfs(d); - aoedisk_rm_sysfs(d); del_gendisk(d->gd); put_disk(d->gd); blk_cleanup_queue(d->blkq);
Use device_add_disk_with_groups() to avoid a race condition with udev during startup. Signed-off-by: Hannes Reinecke <hare@suse.com> Cc: Ed L. Cachin <ed.cashin@acm.org> --- drivers/block/aoe/aoe.h | 1 - drivers/block/aoe/aoeblk.c | 21 +++++++-------------- drivers/block/aoe/aoedev.c | 1 - 3 files changed, 7 insertions(+), 16 deletions(-)