* [PATCH 0/6] genhd: register default groups with device_add_disk()
@ 2018-07-30 7:12 Hannes Reinecke
2018-07-30 7:12 ` [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() Hannes Reinecke
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Hannes Reinecke @ 2018-07-30 7:12 UTC (permalink / raw)
To: Jens Axboe; +Cc: Christoph Hellwig, linux-block, Hannes Reinecke
device_add_disk() doesn't allow to register with default sysfs groups,
which introduces a race with udev as these groups have to be created after
the uevent has been sent.
This patchset updates device_add_disk() to accept a 'groups' argument to
avoid this race and updates the obvious drivers to use it.
There are some more drivers which might benefit from this (eg loop or md),
but the interface is not straightforward so I haven't attempted it.
As usual, comments and reviews are welcome.
Hannes Reinecke (6):
genhd: drop 'bool' argument from __device_add_disk()
block: genhd: add 'groups' argument to device_add_disk
nvme: register ns_id attributes as default sysfs groups
aoe: use device_add_disk_with_groups()
zram: register default groups with device_add_disk()
virtio-blk: modernize sysfs attribute creation
arch/um/drivers/ubd_kern.c | 2 +-
block/genhd.c | 39 ++++++++++++++-------
drivers/block/aoe/aoe.h | 1 -
drivers/block/aoe/aoeblk.c | 21 ++++--------
drivers/block/aoe/aoedev.c | 1 -
drivers/block/floppy.c | 2 +-
drivers/block/mtip32xx/mtip32xx.c | 2 +-
drivers/block/ps3disk.c | 2 +-
drivers/block/ps3vram.c | 2 +-
drivers/block/rsxx/dev.c | 2 +-
drivers/block/skd_main.c | 2 +-
drivers/block/sunvdc.c | 2 +-
drivers/block/virtio_blk.c | 68 +++++++++++++++++++++----------------
drivers/block/xen-blkfront.c | 2 +-
drivers/block/zram/zram_drv.c | 28 ++++-----------
drivers/ide/ide-cd.c | 2 +-
drivers/ide/ide-gd.c | 2 +-
drivers/memstick/core/ms_block.c | 2 +-
drivers/memstick/core/mspro_block.c | 2 +-
drivers/mmc/core/block.c | 2 +-
drivers/mtd/mtd_blkdevs.c | 2 +-
drivers/nvdimm/blk.c | 2 +-
drivers/nvdimm/btt.c | 2 +-
drivers/nvdimm/pmem.c | 2 +-
drivers/nvme/host/core.c | 13 ++++---
drivers/nvme/host/multipath.c | 15 +++-----
drivers/nvme/host/nvme.h | 2 +-
drivers/s390/block/dasd_genhd.c | 2 +-
drivers/s390/block/dcssblk.c | 2 +-
drivers/s390/block/scm_blk.c | 2 +-
drivers/scsi/sd.c | 2 +-
drivers/scsi/sr.c | 2 +-
include/linux/genhd.h | 5 +--
33 files changed, 117 insertions(+), 122 deletions(-)
--
2.12.3
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() 2018-07-30 7:12 [PATCH 0/6] genhd: register default groups with device_add_disk() Hannes Reinecke @ 2018-07-30 7:12 ` Hannes Reinecke 2018-07-30 8:56 ` Christoph Hellwig 2018-07-30 7:12 ` [PATCH 2/6] block: genhd: add 'groups' argument to device_add_disk Hannes Reinecke ` (4 subsequent siblings) 5 siblings, 1 reply; 22+ messages in thread From: Hannes Reinecke @ 2018-07-30 7:12 UTC (permalink / raw) To: Jens Axboe Cc: Christoph Hellwig, linux-block, Hannes Reinecke, Hannes Reinecke Split off the last part of __device_add_disk() and move it into the caller. Signed-off-by: Hannes Reinecke <hare@suse.com> --- block/genhd.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/block/genhd.c b/block/genhd.c index 8cc719a37b32..62aba3d8ae19 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -647,15 +647,13 @@ static void register_disk(struct device *parent, struct gendisk *disk) * __device_add_disk - add disk information to kernel list * @parent: parent device for the disk * @disk: per-device partitioning information - * @register_queue: register the queue if set to true * * This function registers the partitioning information in @disk * with the kernel. * * FIXME: error handling */ -static void __device_add_disk(struct device *parent, struct gendisk *disk, - bool register_queue) +static void __device_add_disk(struct device *parent, struct gendisk *disk) { dev_t devt; int retval; @@ -699,8 +697,13 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, exact_match, exact_lock, disk); } register_disk(parent, disk); - if (register_queue) - blk_register_queue(disk); +} + +void device_add_disk(struct device *parent, struct gendisk *disk) +{ + __device_add_disk(parent, disk); + + blk_register_queue(disk); /* * Take an extra ref on queue which will be put on disk_release() @@ -711,16 +714,19 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, disk_add_events(disk); blk_integrity_add(disk); } - -void device_add_disk(struct device *parent, struct gendisk *disk) -{ - __device_add_disk(parent, disk, true); -} EXPORT_SYMBOL(device_add_disk); void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk) { - __device_add_disk(parent, disk, false); + __device_add_disk(parent, disk); + /* + * Take an extra ref on queue which will be put on disk_release() + * so that it sticks around as long as @disk is there. + */ + WARN_ON_ONCE(!blk_get_queue(disk->queue)); + + disk_add_events(disk); + blk_integrity_add(disk); } EXPORT_SYMBOL(device_add_disk_no_queue_reg); -- 2.12.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() 2018-07-30 7:12 ` [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() Hannes Reinecke @ 2018-07-30 8:56 ` Christoph Hellwig 2018-07-30 8:57 ` Hannes Reinecke 0 siblings, 1 reply; 22+ messages in thread From: Christoph Hellwig @ 2018-07-30 8:56 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, Christoph Hellwig, linux-block, Hannes Reinecke I really don't see the point for this change. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() 2018-07-30 8:56 ` Christoph Hellwig @ 2018-07-30 8:57 ` Hannes Reinecke 2018-08-13 19:53 ` Bart Van Assche 0 siblings, 1 reply; 22+ messages in thread From: Hannes Reinecke @ 2018-07-30 8:57 UTC (permalink / raw) To: Christoph Hellwig, Hannes Reinecke; +Cc: Jens Axboe, linux-block On 07/30/2018 10:56 AM, Christoph Hellwig wrote: > I really don't see the point for this change. > Okay, I'll be dropping it on the next iteration. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.com +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() 2018-07-30 8:57 ` Hannes Reinecke @ 2018-08-13 19:53 ` Bart Van Assche 0 siblings, 0 replies; 22+ messages in thread From: Bart Van Assche @ 2018-08-13 19:53 UTC (permalink / raw) To: hch, hare, hare; +Cc: linux-block, axboe On Mon, 2018-07-30 at 10:57 +0200, Hannes Reinecke wrote: > On 07/30/2018 10:56 AM, Christoph Hellwig wrote: > > I really don't see the point for this change. >=20 > Okay, I'll be dropping it on the next iteration. Hello Hannes, Have you already decided when you will post the next iteration of this patc= h series? Thanks, Bart. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/6] block: genhd: add 'groups' argument to device_add_disk 2018-07-30 7:12 [PATCH 0/6] genhd: register default groups with device_add_disk() Hannes Reinecke 2018-07-30 7:12 ` [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() Hannes Reinecke @ 2018-07-30 7:12 ` Hannes Reinecke 2018-07-30 8:57 ` Christoph Hellwig 2018-07-30 7:12 ` [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups Hannes Reinecke ` (3 subsequent siblings) 5 siblings, 1 reply; 22+ messages in thread From: Hannes Reinecke @ 2018-07-30 7:12 UTC (permalink / raw) To: Jens Axboe Cc: Christoph Hellwig, linux-block, Hannes Reinecke, Martin Wilck, Hannes Reinecke Update device_add_disk() to take an 'groups' argument so that individual drivers can register a device with additional sysfs attributes. This avoids race condition the driver would otherwise have if these groups were to be created with sysfs_add_groups(). Signed-off-by: Martin Wilck <martin.wilck@suse.com> Signed-off-by: Hannes Reinecke <hare@suse.com> --- arch/um/drivers/ubd_kern.c | 2 +- block/genhd.c | 21 +++++++++++++++------ drivers/block/floppy.c | 2 +- drivers/block/mtip32xx/mtip32xx.c | 2 +- drivers/block/ps3disk.c | 2 +- drivers/block/ps3vram.c | 2 +- drivers/block/rsxx/dev.c | 2 +- drivers/block/skd_main.c | 2 +- drivers/block/sunvdc.c | 2 +- drivers/block/virtio_blk.c | 2 +- drivers/block/xen-blkfront.c | 2 +- drivers/ide/ide-cd.c | 2 +- drivers/ide/ide-gd.c | 2 +- drivers/memstick/core/ms_block.c | 2 +- drivers/memstick/core/mspro_block.c | 2 +- drivers/mmc/core/block.c | 2 +- drivers/mtd/mtd_blkdevs.c | 2 +- drivers/nvdimm/blk.c | 2 +- drivers/nvdimm/btt.c | 2 +- drivers/nvdimm/pmem.c | 2 +- drivers/nvme/host/core.c | 2 +- drivers/nvme/host/multipath.c | 2 +- drivers/s390/block/dasd_genhd.c | 2 +- drivers/s390/block/dcssblk.c | 2 +- drivers/s390/block/scm_blk.c | 2 +- drivers/scsi/sd.c | 2 +- drivers/scsi/sr.c | 2 +- include/linux/genhd.h | 5 +++-- 28 files changed, 44 insertions(+), 34 deletions(-) diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c index 83c470364dfb..6ee4c56032f7 100644 --- a/arch/um/drivers/ubd_kern.c +++ b/arch/um/drivers/ubd_kern.c @@ -891,7 +891,7 @@ static int ubd_disk_register(int major, u64 size, int unit, disk->private_data = &ubd_devs[unit]; disk->queue = ubd_devs[unit].queue; - device_add_disk(parent, disk); + device_add_disk(parent, disk, NULL); *disk_out = disk; return 0; diff --git a/block/genhd.c b/block/genhd.c index 62aba3d8ae19..500c2e8d6345 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -567,7 +567,8 @@ static int exact_lock(dev_t devt, void *data) return 0; } -static void register_disk(struct device *parent, struct gendisk *disk) +static void register_disk(struct device *parent, struct gendisk *disk, + const struct attribute_group **groups) { struct device *ddev = disk_to_dev(disk); struct block_device *bdev; @@ -582,6 +583,10 @@ static void register_disk(struct device *parent, struct gendisk *disk) /* delay uevents, until we scanned partition table */ dev_set_uevent_suppress(ddev, 1); + if (groups) { + WARN_ON(ddev->groups); + ddev->groups = groups; + } if (device_add(ddev)) return; if (!sysfs_deprecated) { @@ -647,13 +652,15 @@ static void register_disk(struct device *parent, struct gendisk *disk) * __device_add_disk - add disk information to kernel list * @parent: parent device for the disk * @disk: per-device partitioning information + * @groups: Additional per-device sysfs groups * * This function registers the partitioning information in @disk * with the kernel. * * FIXME: error handling */ -static void __device_add_disk(struct device *parent, struct gendisk *disk) +static void __device_add_disk(struct device *parent, struct gendisk *disk, + const struct attribute_group **groups) { dev_t devt; int retval; @@ -696,12 +703,13 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk) blk_register_region(disk_devt(disk), disk->minors, NULL, exact_match, exact_lock, disk); } - register_disk(parent, disk); + register_disk(parent, disk, groups); } -void device_add_disk(struct device *parent, struct gendisk *disk) +void device_add_disk(struct device *parent, struct gendisk *disk, + const struct attribute_group **groups) { - __device_add_disk(parent, disk); + __device_add_disk(parent, disk, groups); blk_register_queue(disk); @@ -718,7 +726,8 @@ EXPORT_SYMBOL(device_add_disk); void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk) { - __device_add_disk(parent, disk); + __device_add_disk(parent, disk, NULL); + /* * Take an extra ref on queue which will be put on disk_release() * so that it sticks around as long as @disk is there. diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c index 48f622728ce6..1bc99e9dfaee 100644 --- a/drivers/block/floppy.c +++ b/drivers/block/floppy.c @@ -4676,7 +4676,7 @@ static int __init do_floppy_init(void) /* to be cleaned up... */ disks[drive]->private_data = (void *)(long)drive; disks[drive]->flags |= GENHD_FL_REMOVABLE; - device_add_disk(&floppy_device[drive].dev, disks[drive]); + device_add_disk(&floppy_device[drive].dev, disks[drive], NULL); } return 0; diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c index c73626decb46..ac3b19ad8544 100644 --- a/drivers/block/mtip32xx/mtip32xx.c +++ b/drivers/block/mtip32xx/mtip32xx.c @@ -3873,7 +3873,7 @@ static int mtip_block_initialize(struct driver_data *dd) set_capacity(dd->disk, capacity); /* Enable the block device and add it to /dev */ - device_add_disk(&dd->pdev->dev, dd->disk); + device_add_disk(&dd->pdev->dev, dd->disk, NULL); dd->bdev = bdget_disk(dd->disk, 0); /* diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c index afe1508d82c6..29a4419e8ba3 100644 --- a/drivers/block/ps3disk.c +++ b/drivers/block/ps3disk.c @@ -500,7 +500,7 @@ static int ps3disk_probe(struct ps3_system_bus_device *_dev) gendisk->disk_name, priv->model, priv->raw_capacity >> 11, get_capacity(gendisk) >> 11); - device_add_disk(&dev->sbd.core, gendisk); + device_add_disk(&dev->sbd.core, gendisk, NULL); return 0; fail_cleanup_queue: diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c index 1e3d5de9d838..c0c50816a10b 100644 --- a/drivers/block/ps3vram.c +++ b/drivers/block/ps3vram.c @@ -769,7 +769,7 @@ static int ps3vram_probe(struct ps3_system_bus_device *dev) dev_info(&dev->core, "%s: Using %lu MiB of GPU memory\n", gendisk->disk_name, get_capacity(gendisk) >> 11); - device_add_disk(&dev->core, gendisk); + device_add_disk(&dev->core, gendisk, NULL); return 0; fail_cleanup_queue: diff --git a/drivers/block/rsxx/dev.c b/drivers/block/rsxx/dev.c index 1a92f9e65937..3894aa0f350b 100644 --- a/drivers/block/rsxx/dev.c +++ b/drivers/block/rsxx/dev.c @@ -226,7 +226,7 @@ int rsxx_attach_dev(struct rsxx_cardinfo *card) set_capacity(card->gendisk, card->size8 >> 9); else set_capacity(card->gendisk, 0); - device_add_disk(CARD_TO_DEV(card), card->gendisk); + device_add_disk(CARD_TO_DEV(card), card->gendisk, NULL); card->bdev_attached = 1; } diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c index 87b9e7fbf062..a85c9a622c41 100644 --- a/drivers/block/skd_main.c +++ b/drivers/block/skd_main.c @@ -3104,7 +3104,7 @@ static int skd_bdev_getgeo(struct block_device *bdev, struct hd_geometry *geo) static int skd_bdev_attach(struct device *parent, struct skd_device *skdev) { dev_dbg(&skdev->pdev->dev, "add_disk\n"); - device_add_disk(parent, skdev->disk); + device_add_disk(parent, skdev->disk, NULL); return 0; } diff --git a/drivers/block/sunvdc.c b/drivers/block/sunvdc.c index 5ca56bfae63c..09409edce384 100644 --- a/drivers/block/sunvdc.c +++ b/drivers/block/sunvdc.c @@ -850,7 +850,7 @@ static int probe_disk(struct vdc_port *port) port->vdisk_size, (port->vdisk_size >> (20 - 9)), port->vio.ver.major, port->vio.ver.minor); - device_add_disk(&port->vio.vdev->dev, g); + device_add_disk(&port->vio.vdev->dev, g, NULL); return 0; } diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 23752dc99b00..fe80560000a1 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -780,7 +780,7 @@ static int virtblk_probe(struct virtio_device *vdev) virtblk_update_capacity(vblk, false); virtio_device_ready(vdev); - device_add_disk(&vdev->dev, vblk->disk); + device_add_disk(&vdev->dev, vblk->disk, NULL); err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial); if (err) goto out_del_disk; diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index b5cedccb5d7d..020f570072cf 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -2402,7 +2402,7 @@ static void blkfront_connect(struct blkfront_info *info) for (i = 0; i < info->nr_rings; i++) kick_pending_request_queues(&info->rinfo[i]); - device_add_disk(&info->xbdev->dev, info->gd); + device_add_disk(&info->xbdev->dev, info->gd, NULL); info->is_ready = 1; return; diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c index 5f178384876f..d9b42ad7253b 100644 --- a/drivers/ide/ide-cd.c +++ b/drivers/ide/ide-cd.c @@ -1788,7 +1788,7 @@ static int ide_cd_probe(ide_drive_t *drive) ide_cd_read_toc(drive, &sense); g->fops = &idecd_ops; g->flags |= GENHD_FL_REMOVABLE | GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE; - device_add_disk(&drive->gendev, g); + device_add_disk(&drive->gendev, g, NULL); return 0; out_free_disk: diff --git a/drivers/ide/ide-gd.c b/drivers/ide/ide-gd.c index e823394ed543..04e008e8f6f9 100644 --- a/drivers/ide/ide-gd.c +++ b/drivers/ide/ide-gd.c @@ -416,7 +416,7 @@ static int ide_gd_probe(ide_drive_t *drive) if (drive->dev_flags & IDE_DFLAG_REMOVABLE) g->flags = GENHD_FL_REMOVABLE; g->fops = &ide_gd_ops; - device_add_disk(&drive->gendev, g); + device_add_disk(&drive->gendev, g, NULL); return 0; out_free_disk: diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c index 716fc8ed31d3..8a02f11076f9 100644 --- a/drivers/memstick/core/ms_block.c +++ b/drivers/memstick/core/ms_block.c @@ -2146,7 +2146,7 @@ static int msb_init_disk(struct memstick_dev *card) set_disk_ro(msb->disk, 1); msb_start(card); - device_add_disk(&card->dev, msb->disk); + device_add_disk(&card->dev, msb->disk, NULL); dbg("Disk added"); return 0; diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c index 5ee932631fae..0cd30dcb6801 100644 --- a/drivers/memstick/core/mspro_block.c +++ b/drivers/memstick/core/mspro_block.c @@ -1236,7 +1236,7 @@ static int mspro_block_init_disk(struct memstick_dev *card) set_capacity(msb->disk, capacity); dev_dbg(&card->dev, "capacity set %ld\n", capacity); - device_add_disk(&card->dev, msb->disk); + device_add_disk(&card->dev, msb->disk, NULL); msb->active = 1; return 0; diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index a0b9102c4c6e..de8e1a8be690 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2698,7 +2698,7 @@ static int mmc_add_disk(struct mmc_blk_data *md) int ret; struct mmc_card *card = md->queue.card; - device_add_disk(md->parent, md->disk); + device_add_disk(md->parent, md->disk, NULL); md->force_ro.show = force_ro_show; md->force_ro.store = force_ro_store; sysfs_attr_init(&md->force_ro.attr); diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c index 29c0bfd74e8a..6a41dfa3c36b 100644 --- a/drivers/mtd/mtd_blkdevs.c +++ b/drivers/mtd/mtd_blkdevs.c @@ -447,7 +447,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) if (new->readonly) set_disk_ro(gd, 1); - device_add_disk(&new->mtd->dev, gd); + device_add_disk(&new->mtd->dev, gd, NULL); if (new->disk_attributes) { ret = sysfs_create_group(&disk_to_dev(gd)->kobj, diff --git a/drivers/nvdimm/blk.c b/drivers/nvdimm/blk.c index 62e9cb167aad..db45c6bbb7bb 100644 --- a/drivers/nvdimm/blk.c +++ b/drivers/nvdimm/blk.c @@ -290,7 +290,7 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk) } set_capacity(disk, available_disk_size >> SECTOR_SHIFT); - device_add_disk(dev, disk); + device_add_disk(dev, disk, NULL); revalidate_disk(disk); return 0; } diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 0360c015f658..b123b0dcf274 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -1556,7 +1556,7 @@ static int btt_blk_init(struct btt *btt) } } set_capacity(btt->btt_disk, btt->nlba * btt->sector_size >> 9); - device_add_disk(&btt->nd_btt->dev, btt->btt_disk); + device_add_disk(&btt->nd_btt->dev, btt->btt_disk, NULL); btt->nd_btt->size = btt->nlba * (u64)btt->sector_size; revalidate_disk(btt->btt_disk); diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index dd17acd8fe68..dad84a1d23db 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -445,7 +445,7 @@ static int pmem_attach_disk(struct device *dev, gendev = disk_to_dev(disk); gendev->groups = pmem_attribute_groups; - device_add_disk(dev, disk); + device_add_disk(dev, disk, NULL); if (devm_add_action_or_reset(dev, pmem_release_disk, pmem)) return -ENOMEM; diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index e482f223f8fd..f1b61ebceaf1 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3061,7 +3061,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) nvme_get_ctrl(ctrl); - device_add_disk(ctrl->device, ns->disk); + device_add_disk(ctrl->device, ns->disk, NULL); if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj, &nvme_ns_id_attr_group)) pr_warn("%s: failed to create sysfs group for identification\n", diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index 2c131340b813..e4d786467129 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -282,7 +282,7 @@ static void nvme_mpath_set_live(struct nvme_ns *ns) return; if (!(head->disk->flags & GENHD_FL_UP)) { - device_add_disk(&head->subsys->dev, head->disk); + device_add_disk(&head->subsys->dev, head->disk, NULL); if (sysfs_create_group(&disk_to_dev(head->disk)->kobj, &nvme_ns_id_attr_group)) dev_warn(&head->subsys->dev, diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c index 7036a6c6f86f..5542d9eadfe0 100644 --- a/drivers/s390/block/dasd_genhd.c +++ b/drivers/s390/block/dasd_genhd.c @@ -76,7 +76,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) gdp->queue = block->request_queue; block->gdp = gdp; set_capacity(block->gdp, 0); - device_add_disk(&base->cdev->dev, block->gdp); + device_add_disk(&base->cdev->dev, block->gdp, NULL); return 0; } diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c index ed607288e696..a0738c6c4eab 100644 --- a/drivers/s390/block/dcssblk.c +++ b/drivers/s390/block/dcssblk.c @@ -685,7 +685,7 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char } get_device(&dev_info->dev); - device_add_disk(&dev_info->dev, dev_info->gd); + device_add_disk(&dev_info->dev, dev_info->gd, NULL); switch (dev_info->segment_type) { case SEG_TYPE_SR: diff --git a/drivers/s390/block/scm_blk.c b/drivers/s390/block/scm_blk.c index b1fcb76dd272..7559ac40f01b 100644 --- a/drivers/s390/block/scm_blk.c +++ b/drivers/s390/block/scm_blk.c @@ -499,7 +499,7 @@ int scm_blk_dev_setup(struct scm_blk_dev *bdev, struct scm_device *scmdev) /* 512 byte sectors */ set_capacity(bdev->gendisk, scmdev->size >> 9); - device_add_disk(&scmdev->dev, bdev->gendisk); + device_add_disk(&scmdev->dev, bdev->gendisk, NULL); return 0; out_queue: diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 9421d9877730..cc80a3d2cd1a 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3271,7 +3271,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie) } blk_pm_runtime_init(sdp->request_queue, dev); - device_add_disk(dev, gd); + device_add_disk(dev, gd, NULL); if (sdkp->capacity) sd_dif_config_host(sdkp); diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c index 3f3cb72e0c0c..f628b04e0920 100644 --- a/drivers/scsi/sr.c +++ b/drivers/scsi/sr.c @@ -745,7 +745,7 @@ static int sr_probe(struct device *dev) dev_set_drvdata(dev, cd); disk->flags |= GENHD_FL_REMOVABLE; - device_add_disk(&sdev->sdev_gendev, disk); + device_add_disk(&sdev->sdev_gendev, disk, NULL); sdev_printk(KERN_DEBUG, sdev, "Attached scsi CD-ROM %s\n", cd->cdi.name); diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 57864422a2c8..0b820ff05839 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -399,10 +399,11 @@ static inline void free_part_info(struct hd_struct *part) extern void part_round_stats(struct request_queue *q, int cpu, struct hd_struct *part); /* block/genhd.c */ -extern void device_add_disk(struct device *parent, struct gendisk *disk); +extern void device_add_disk(struct device *parent, struct gendisk *disk, + const struct attribute_group **groups); static inline void add_disk(struct gendisk *disk) { - device_add_disk(NULL, disk); + device_add_disk(NULL, disk, NULL); } extern void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk); static inline void add_disk_no_queue_reg(struct gendisk *disk) -- 2.12.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] block: genhd: add 'groups' argument to device_add_disk 2018-07-30 7:12 ` [PATCH 2/6] block: genhd: add 'groups' argument to device_add_disk Hannes Reinecke @ 2018-07-30 8:57 ` Christoph Hellwig 0 siblings, 0 replies; 22+ messages in thread From: Christoph Hellwig @ 2018-07-30 8:57 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, Christoph Hellwig, linux-block, Martin Wilck, Hannes Reinecke On Mon, Jul 30, 2018 at 09:12:23AM +0200, Hannes Reinecke wrote: > Update device_add_disk() to take an 'groups' argument so that > individual drivers can register a device with additional sysfs > attributes. > This avoids race condition the driver would otherwise have if these > groups were to be created with sysfs_add_groups(). > > Signed-off-by: Martin Wilck <martin.wilck@suse.com> > Signed-off-by: Hannes Reinecke <hare@suse.com> The first signoff should always be the author. Otherwise this looks fine (modulo any context changes due to patch 1): Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups 2018-07-30 7:12 [PATCH 0/6] genhd: register default groups with device_add_disk() Hannes Reinecke 2018-07-30 7:12 ` [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() Hannes Reinecke 2018-07-30 7:12 ` [PATCH 2/6] block: genhd: add 'groups' argument to device_add_disk Hannes Reinecke @ 2018-07-30 7:12 ` Hannes Reinecke 2018-07-30 8:59 ` Christoph Hellwig 2018-08-13 19:51 ` Bart Van Assche 2018-07-30 7:12 ` [PATCH 4/6] aoe: use device_add_disk_with_groups() Hannes Reinecke ` (2 subsequent siblings) 5 siblings, 2 replies; 22+ messages in thread From: Hannes Reinecke @ 2018-07-30 7:12 UTC (permalink / raw) To: Jens Axboe Cc: Christoph Hellwig, linux-block, Hannes Reinecke, Hannes Reinecke, Sagi Grimberg, Keith Busch We should be registering the ns_id attribute as default sysfs attribute groups, otherwise we have a race condition between the uevent and the attributes appearing in sysfs. Signed-off-by: Hannes Reinecke <hare@suse.com> Cc: Sagi Grimberg <sagi@grimberg.me> Cc: Keith Busch <keith.busch@intel.com> --- drivers/nvme/host/core.c | 13 ++++++------- drivers/nvme/host/multipath.c | 15 ++++----------- drivers/nvme/host/nvme.h | 2 +- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index f1b61ebceaf1..39bdfe806d1b 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -2696,6 +2696,11 @@ const struct attribute_group nvme_ns_id_attr_group = { .is_visible = nvme_ns_id_attrs_are_visible, }; +const struct attribute_group *nvme_ns_id_attr_groups[] = { + &nvme_ns_id_attr_group, + NULL, +}; + #define nvme_show_str_function(field) \ static ssize_t field##_show(struct device *dev, \ struct device_attribute *attr, char *buf) \ @@ -3061,11 +3066,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) nvme_get_ctrl(ctrl); - device_add_disk(ctrl->device, ns->disk, NULL); - if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj, - &nvme_ns_id_attr_group)) - pr_warn("%s: failed to create sysfs group for identification\n", - ns->disk->disk_name); + device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups); if (ns->ndev && nvme_nvm_register_sysfs(ns)) pr_warn("%s: failed to register lightnvm sysfs group for identification\n", ns->disk->disk_name); @@ -3094,8 +3095,6 @@ static void nvme_ns_remove(struct nvme_ns *ns) nvme_fault_inject_fini(ns); if (ns->disk && ns->disk->flags & GENHD_FL_UP) { - sysfs_remove_group(&disk_to_dev(ns->disk)->kobj, - &nvme_ns_id_attr_group); if (ns->ndev) nvme_nvm_unregister_sysfs(ns); del_gendisk(ns->disk); diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index e4d786467129..7d3c30324da9 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -281,13 +281,9 @@ static void nvme_mpath_set_live(struct nvme_ns *ns) if (!head->disk) return; - if (!(head->disk->flags & GENHD_FL_UP)) { - device_add_disk(&head->subsys->dev, head->disk, NULL); - if (sysfs_create_group(&disk_to_dev(head->disk)->kobj, - &nvme_ns_id_attr_group)) - dev_warn(&head->subsys->dev, - "failed to create id group.\n"); - } + if (!(head->disk->flags & GENHD_FL_UP)) + device_add_disk(&head->subsys->dev, head->disk, + nvme_ns_id_attr_groups); kblockd_schedule_work(&ns->head->requeue_work); } @@ -493,11 +489,8 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head) { if (!head->disk) return; - if (head->disk->flags & GENHD_FL_UP) { - sysfs_remove_group(&disk_to_dev(head->disk)->kobj, - &nvme_ns_id_attr_group); + if (head->disk->flags & GENHD_FL_UP) del_gendisk(head->disk); - } blk_set_queue_dying(head->disk->queue); /* make sure all pending bios are cleaned up */ kblockd_schedule_work(&head->requeue_work); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 8b356f1d941c..7f156367bd18 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -466,7 +466,7 @@ int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl); int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp, void *log, size_t size, u64 offset); -extern const struct attribute_group nvme_ns_id_attr_group; +extern const struct attribute_group *nvme_ns_id_attr_groups[]; extern const struct block_device_operations nvme_ns_head_ops; #ifdef CONFIG_NVME_MULTIPATH -- 2.12.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups 2018-07-30 7:12 ` [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups Hannes Reinecke @ 2018-07-30 8:59 ` Christoph Hellwig 2018-08-13 19:51 ` Bart Van Assche 1 sibling, 0 replies; 22+ messages in thread From: Christoph Hellwig @ 2018-07-30 8:59 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, Christoph Hellwig, linux-block, Hannes Reinecke, Sagi Grimberg, Keith Busch On Mon, Jul 30, 2018 at 09:12:24AM +0200, Hannes Reinecke wrote: > We should be registering the ns_id attribute as default sysfs > attribute groups, otherwise we have a race condition between > the uevent and the attributes appearing in sysfs. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > Cc: Sagi Grimberg <sagi@grimberg.me> > Cc: Keith Busch <keith.busch@intel.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups 2018-07-30 7:12 ` [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups Hannes Reinecke 2018-07-30 8:59 ` Christoph Hellwig @ 2018-08-13 19:51 ` Bart Van Assche 2018-08-14 6:19 ` Hannes Reinecke 1 sibling, 1 reply; 22+ messages in thread From: Bart Van Assche @ 2018-08-13 19:51 UTC (permalink / raw) To: hare, axboe; +Cc: hch, keith.busch, linux-block, hare, sagi On Mon, 2018-07-30 at 09:12 +0200, Hannes Reinecke wrote: > @@ -3061,11 +3066,7 @@ static void nvme_alloc_ns= (struct nvme_ctrl *ctrl, unsigned nsid) > =20 > nvme_get_ctrl(ctrl); > =20 > - device_add_disk(ctrl->device, ns->disk, NULL); > - if (sysfs_create_group(&disk_to_dev(ns->dis= k)->kobj, > - &nvme_ns_id_attr_group)) > - pr_warn("%s: failed to create sysfs group for identifica= tion\n", > - ns->disk->disk_name); > + device_add_disk(ctrl->device, ns->disk, nvme_n= s_id_attr_groups); > if (ns->ndev && nvme_nvm_register_sysfs(ns)) > pr_warn("%s: failed to register lightnvm sysfs group for= identification\n", > ns->disk->disk_name); Hello Hannes, Have you noticed that nvme_nvm_register_sysfs() also registers = sysfs attributes and hence that the attributes registered by that function should be merged = into the nvme_ns_id_attr_groups array? Thanks, Bart. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups 2018-08-13 19:51 ` Bart Van Assche @ 2018-08-14 6:19 ` Hannes Reinecke 0 siblings, 0 replies; 22+ messages in thread From: Hannes Reinecke @ 2018-08-14 6:19 UTC (permalink / raw) To: Bart Van Assche, axboe; +Cc: hch, keith.busch, linux-block, hare, sagi On 08/13/2018 09:51 PM, Bart Van Assche wrote: > On Mon, 2018-07-30 at 09:12 +0200, Hannes Reinecke wrote: >> @@ -3061,11 +3066,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) >> >> nvme_get_ctrl(ctrl); >> >> - device_add_disk(ctrl->device, ns->disk, NULL); >> - if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj, >> - &nvme_ns_id_attr_group)) >> - pr_warn("%s: failed to create sysfs group for identification\n", >> - ns->disk->disk_name); >> + device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups); >> if (ns->ndev && nvme_nvm_register_sysfs(ns)) >> pr_warn("%s: failed to register lightnvm sysfs group for identification\n", >> ns->disk->disk_name); > > Hello Hannes, > > Have you noticed that nvme_nvm_register_sysfs() also registers sysfs attributes > and hence that the attributes registered by that function should be merged into > the nvme_ns_id_attr_groups array? > Actually, no :-) I'll be looking into it and reposting the series. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/6] aoe: use device_add_disk_with_groups() 2018-07-30 7:12 [PATCH 0/6] genhd: register default groups with device_add_disk() Hannes Reinecke ` (2 preceding siblings ...) 2018-07-30 7:12 ` [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups Hannes Reinecke @ 2018-07-30 7:12 ` Hannes Reinecke 2018-07-30 9:01 ` Christoph Hellwig 2018-08-01 1:00 ` Ed Cashin 2018-07-30 7:12 ` [PATCH 5/6] zram: register default groups with device_add_disk() Hannes Reinecke 2018-07-30 7:12 ` [PATCH 6/6] virtio-blk: modernize sysfs attribute creation Hannes Reinecke 5 siblings, 2 replies; 22+ messages in thread From: Hannes Reinecke @ 2018-07-30 7:12 UTC (permalink / raw) To: Jens Axboe Cc: Christoph Hellwig, linux-block, Hannes Reinecke, Hannes Reinecke, Ed L . Cachin 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(-) 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); -- 2.12.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] aoe: use device_add_disk_with_groups() 2018-07-30 7:12 ` [PATCH 4/6] aoe: use device_add_disk_with_groups() Hannes Reinecke @ 2018-07-30 9:01 ` Christoph Hellwig 2018-08-01 1:00 ` Ed Cashin 1 sibling, 0 replies; 22+ messages in thread From: Christoph Hellwig @ 2018-07-30 9:01 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, Christoph Hellwig, linux-block, Hannes Reinecke, Ed L . Cachin 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> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] aoe: use device_add_disk_with_groups() 2018-07-30 7:12 ` [PATCH 4/6] aoe: use device_add_disk_with_groups() Hannes Reinecke 2018-07-30 9:01 ` Christoph Hellwig @ 2018-08-01 1:00 ` Ed Cashin 2018-08-01 6:57 ` Hannes Reinecke 1 sibling, 1 reply; 22+ messages in thread From: Ed Cashin @ 2018-08-01 1:00 UTC (permalink / raw) To: hare; +Cc: axboe, hch, linux-block, hare, Ed L. Cashin [-- Attachment #1: Type: text/plain, Size: 456 bytes --] 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 [-- Attachment #2: Type: text/html, Size: 788 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] aoe: use device_add_disk_with_groups() 2018-08-01 1:00 ` Ed Cashin @ 2018-08-01 6:57 ` Hannes Reinecke 2018-08-02 11:55 ` Ed Cashin 0 siblings, 1 reply; 22+ messages in thread From: Hannes Reinecke @ 2018-08-01 6:57 UTC (permalink / raw) To: Ed Cashin; +Cc: axboe, hch, linux-block, hare 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 -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] aoe: use device_add_disk_with_groups() 2018-08-01 6:57 ` Hannes Reinecke @ 2018-08-02 11:55 ` Ed Cashin 0 siblings, 0 replies; 22+ messages in thread From: Ed Cashin @ 2018-08-02 11:55 UTC (permalink / raw) To: hare; +Cc: Ed L. Cashin, axboe, hch, linux-block, hare [-- Attachment #1: Type: text/plain, Size: 944 bytes --] 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. -- Ed Cashin <ecashin@noserose.net> [-- Attachment #2: Type: text/html, Size: 1621 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/6] zram: register default groups with device_add_disk() 2018-07-30 7:12 [PATCH 0/6] genhd: register default groups with device_add_disk() Hannes Reinecke ` (3 preceding siblings ...) 2018-07-30 7:12 ` [PATCH 4/6] aoe: use device_add_disk_with_groups() Hannes Reinecke @ 2018-07-30 7:12 ` Hannes Reinecke 2018-07-30 9:02 ` Christoph Hellwig 2018-07-30 7:12 ` [PATCH 6/6] virtio-blk: modernize sysfs attribute creation Hannes Reinecke 5 siblings, 1 reply; 22+ messages in thread From: Hannes Reinecke @ 2018-07-30 7:12 UTC (permalink / raw) To: Jens Axboe Cc: Christoph Hellwig, linux-block, Hannes Reinecke, Hannes Reinecke, Minchan Kim, Nitin Gupta Register default sysfs groups during device_add-disk() to avoid a race condition with udev during startup. Signed-off-by: Hannes Reinecke <hare@suse.com> Cc: Minchan Kim <minchan@kernel.org> Cc: Nitin Gupta <ngupta@vflare.org> --- drivers/block/zram/zram_drv.c | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 2907a8156aaf..fc3626cb7fe5 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1618,6 +1618,11 @@ static const struct attribute_group zram_disk_attr_group = { .attrs = zram_disk_attrs, }; +static const struct attribute_group *zram_disk_attr_groups[] = { + &zram_disk_attr_group, + NULL, +}; + /* * Allocate and initialize new zram device. the function returns * '>= 0' device_id upon success, and negative value otherwise. @@ -1698,24 +1703,14 @@ static int zram_add(void) zram->disk->queue->backing_dev_info->capabilities |= (BDI_CAP_STABLE_WRITES | BDI_CAP_SYNCHRONOUS_IO); - add_disk(zram->disk); - - ret = sysfs_create_group(&disk_to_dev(zram->disk)->kobj, - &zram_disk_attr_group); - if (ret < 0) { - pr_err("Error creating sysfs group for device %d\n", - device_id); - goto out_free_disk; - } + device_add_disk(NULL, zram->disk, zram_disk_attr_groups); + strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor)); zram_debugfs_register(zram); pr_info("Added device: %s\n", zram->disk->disk_name); return device_id; -out_free_disk: - del_gendisk(zram->disk); - put_disk(zram->disk); out_free_queue: blk_cleanup_queue(queue); out_free_idr: @@ -1744,15 +1739,6 @@ static int zram_remove(struct zram *zram) mutex_unlock(&bdev->bd_mutex); zram_debugfs_unregister(zram); - /* - * Remove sysfs first, so no one will perform a disksize - * store while we destroy the devices. This also helps during - * hot_remove -- zram_reset_device() is the last holder of - * ->init_lock, no later/concurrent disksize_store() or any - * other sysfs handlers are possible. - */ - sysfs_remove_group(&disk_to_dev(zram->disk)->kobj, - &zram_disk_attr_group); /* Make sure all the pending I/O are finished */ fsync_bdev(bdev); -- 2.12.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] zram: register default groups with device_add_disk() 2018-07-30 7:12 ` [PATCH 5/6] zram: register default groups with device_add_disk() Hannes Reinecke @ 2018-07-30 9:02 ` Christoph Hellwig 0 siblings, 0 replies; 22+ messages in thread From: Christoph Hellwig @ 2018-07-30 9:02 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, Christoph Hellwig, linux-block, Hannes Reinecke, Minchan Kim, Nitin Gupta Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 6/6] virtio-blk: modernize sysfs attribute creation 2018-07-30 7:12 [PATCH 0/6] genhd: register default groups with device_add_disk() Hannes Reinecke ` (4 preceding siblings ...) 2018-07-30 7:12 ` [PATCH 5/6] zram: register default groups with device_add_disk() Hannes Reinecke @ 2018-07-30 7:12 ` Hannes Reinecke 2018-07-30 9:03 ` Christoph Hellwig 2018-07-31 15:11 ` Michael S. Tsirkin 5 siblings, 2 replies; 22+ messages in thread From: Hannes Reinecke @ 2018-07-30 7:12 UTC (permalink / raw) To: Jens Axboe Cc: Christoph Hellwig, linux-block, Hannes Reinecke, Hannes Reinecke, Michael Tsirkin, Jason Wang Use new-style DEVICE_ATTR_RO/DEVICE_ATTR_RW to create the sysfs attributes and register the disk with default sysfs attribute groups. Signed-off-by: Hannes Reinecke <hare@suse.com> Cc: Michael Tsirkin <mst@redhat.com> Cc: Jason Wang <jasowang@redhat.com> --- drivers/block/virtio_blk.c | 68 ++++++++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index fe80560000a1..086c6bb12baa 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -351,8 +351,8 @@ static int minor_to_index(int minor) return minor >> PART_BITS; } -static ssize_t virtblk_serial_show(struct device *dev, - struct device_attribute *attr, char *buf) +static ssize_t serial_show(struct device *dev, + struct device_attribute *attr, char *buf) { struct gendisk *disk = dev_to_disk(dev); int err; @@ -371,7 +371,7 @@ static ssize_t virtblk_serial_show(struct device *dev, return err; } -static DEVICE_ATTR(serial, 0444, virtblk_serial_show, NULL); +static DEVICE_ATTR_RO(serial); /* The queue's logical block size must be set before calling this */ static void virtblk_update_capacity(struct virtio_blk *vblk, bool resize) @@ -545,8 +545,8 @@ static const char *const virtblk_cache_types[] = { }; static ssize_t -virtblk_cache_type_store(struct device *dev, struct device_attribute *attr, - const char *buf, size_t count) +cache_type_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) { struct gendisk *disk = dev_to_disk(dev); struct virtio_blk *vblk = disk->private_data; @@ -564,8 +564,7 @@ virtblk_cache_type_store(struct device *dev, struct device_attribute *attr, } static ssize_t -virtblk_cache_type_show(struct device *dev, struct device_attribute *attr, - char *buf) +cache_type_show(struct device *dev, struct device_attribute *attr, char *buf) { struct gendisk *disk = dev_to_disk(dev); struct virtio_blk *vblk = disk->private_data; @@ -575,12 +574,38 @@ virtblk_cache_type_show(struct device *dev, struct device_attribute *attr, return snprintf(buf, 40, "%s\n", virtblk_cache_types[writeback]); } -static const struct device_attribute dev_attr_cache_type_ro = - __ATTR(cache_type, 0444, - virtblk_cache_type_show, NULL); -static const struct device_attribute dev_attr_cache_type_rw = - __ATTR(cache_type, 0644, - virtblk_cache_type_show, virtblk_cache_type_store); +static DEVICE_ATTR_RW(cache_type); + +static struct attribute *virtblk_attrs[] = { + &dev_attr_serial.attr, + &dev_attr_cache_type.attr, + NULL, +}; + +static umode_t virtblk_attrs_are_visible(struct kobject *kobj, + struct attribute *a, int n) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct gendisk *disk = dev_to_disk(dev); + struct virtio_blk *vblk = disk->private_data; + struct virtio_device *vdev = vblk->vdev; + + if (a == &dev_attr_cache_type.attr && + !virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE)) + return S_IRUGO; + + return a->mode; +} + +static const struct attribute_group virtblk_attr_group = { + .attrs = virtblk_attrs, + .is_visible = virtblk_attrs_are_visible, +}; + +static const struct attribute_group *virtblk_attr_groups[] = { + &virtblk_attr_group, + NULL, +}; static int virtblk_init_request(struct blk_mq_tag_set *set, struct request *rq, unsigned int hctx_idx, unsigned int numa_node) @@ -780,24 +805,9 @@ static int virtblk_probe(struct virtio_device *vdev) virtblk_update_capacity(vblk, false); virtio_device_ready(vdev); - device_add_disk(&vdev->dev, vblk->disk, NULL); - err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial); - if (err) - goto out_del_disk; - - if (virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE)) - err = device_create_file(disk_to_dev(vblk->disk), - &dev_attr_cache_type_rw); - else - err = device_create_file(disk_to_dev(vblk->disk), - &dev_attr_cache_type_ro); - if (err) - goto out_del_disk; + device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups); return 0; -out_del_disk: - del_gendisk(vblk->disk); - blk_cleanup_queue(vblk->disk->queue); out_free_tags: blk_mq_free_tag_set(&vblk->tag_set); out_put_disk: -- 2.12.3 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] virtio-blk: modernize sysfs attribute creation 2018-07-30 7:12 ` [PATCH 6/6] virtio-blk: modernize sysfs attribute creation Hannes Reinecke @ 2018-07-30 9:03 ` Christoph Hellwig 2018-07-31 15:11 ` Michael S. Tsirkin 1 sibling, 0 replies; 22+ messages in thread From: Christoph Hellwig @ 2018-07-30 9:03 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, Christoph Hellwig, linux-block, Hannes Reinecke, Michael Tsirkin, Jason Wang On Mon, Jul 30, 2018 at 09:12:27AM +0200, Hannes Reinecke wrote: > Use new-style DEVICE_ATTR_RO/DEVICE_ATTR_RW to create the sysfs attributes > and register the disk with default sysfs attribute groups. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > Cc: Michael Tsirkin <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] virtio-blk: modernize sysfs attribute creation 2018-07-30 7:12 ` [PATCH 6/6] virtio-blk: modernize sysfs attribute creation Hannes Reinecke 2018-07-30 9:03 ` Christoph Hellwig @ 2018-07-31 15:11 ` Michael S. Tsirkin 2018-07-31 15:19 ` Christoph Hellwig 1 sibling, 1 reply; 22+ messages in thread From: Michael S. Tsirkin @ 2018-07-31 15:11 UTC (permalink / raw) To: Hannes Reinecke Cc: Jens Axboe, Christoph Hellwig, linux-block, Hannes Reinecke, Jason Wang On Mon, Jul 30, 2018 at 09:12:27AM +0200, Hannes Reinecke wrote: > Use new-style DEVICE_ATTR_RO/DEVICE_ATTR_RW to create the sysfs attributes > and register the disk with default sysfs attribute groups. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > Cc: Michael Tsirkin <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> what's the right tree to merge this? > --- > drivers/block/virtio_blk.c | 68 ++++++++++++++++++++++++++-------------------- > 1 file changed, 39 insertions(+), 29 deletions(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index fe80560000a1..086c6bb12baa 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -351,8 +351,8 @@ static int minor_to_index(int minor) > return minor >> PART_BITS; > } > > -static ssize_t virtblk_serial_show(struct device *dev, > - struct device_attribute *attr, char *buf) > +static ssize_t serial_show(struct device *dev, > + struct device_attribute *attr, char *buf) > { > struct gendisk *disk = dev_to_disk(dev); > int err; > @@ -371,7 +371,7 @@ static ssize_t virtblk_serial_show(struct device *dev, > return err; > } > > -static DEVICE_ATTR(serial, 0444, virtblk_serial_show, NULL); > +static DEVICE_ATTR_RO(serial); > > /* The queue's logical block size must be set before calling this */ > static void virtblk_update_capacity(struct virtio_blk *vblk, bool resize) > @@ -545,8 +545,8 @@ static const char *const virtblk_cache_types[] = { > }; > > static ssize_t > -virtblk_cache_type_store(struct device *dev, struct device_attribute *attr, > - const char *buf, size_t count) > +cache_type_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > { > struct gendisk *disk = dev_to_disk(dev); > struct virtio_blk *vblk = disk->private_data; > @@ -564,8 +564,7 @@ virtblk_cache_type_store(struct device *dev, struct device_attribute *attr, > } > > static ssize_t > -virtblk_cache_type_show(struct device *dev, struct device_attribute *attr, > - char *buf) > +cache_type_show(struct device *dev, struct device_attribute *attr, char *buf) > { > struct gendisk *disk = dev_to_disk(dev); > struct virtio_blk *vblk = disk->private_data; > @@ -575,12 +574,38 @@ virtblk_cache_type_show(struct device *dev, struct device_attribute *attr, > return snprintf(buf, 40, "%s\n", virtblk_cache_types[writeback]); > } > > -static const struct device_attribute dev_attr_cache_type_ro = > - __ATTR(cache_type, 0444, > - virtblk_cache_type_show, NULL); > -static const struct device_attribute dev_attr_cache_type_rw = > - __ATTR(cache_type, 0644, > - virtblk_cache_type_show, virtblk_cache_type_store); > +static DEVICE_ATTR_RW(cache_type); > + > +static struct attribute *virtblk_attrs[] = { > + &dev_attr_serial.attr, > + &dev_attr_cache_type.attr, > + NULL, > +}; > + > +static umode_t virtblk_attrs_are_visible(struct kobject *kobj, > + struct attribute *a, int n) > +{ > + struct device *dev = container_of(kobj, struct device, kobj); > + struct gendisk *disk = dev_to_disk(dev); > + struct virtio_blk *vblk = disk->private_data; > + struct virtio_device *vdev = vblk->vdev; > + > + if (a == &dev_attr_cache_type.attr && > + !virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE)) > + return S_IRUGO; > + > + return a->mode; > +} > + > +static const struct attribute_group virtblk_attr_group = { > + .attrs = virtblk_attrs, > + .is_visible = virtblk_attrs_are_visible, > +}; > + > +static const struct attribute_group *virtblk_attr_groups[] = { > + &virtblk_attr_group, > + NULL, > +}; > > static int virtblk_init_request(struct blk_mq_tag_set *set, struct request *rq, > unsigned int hctx_idx, unsigned int numa_node) > @@ -780,24 +805,9 @@ static int virtblk_probe(struct virtio_device *vdev) > virtblk_update_capacity(vblk, false); > virtio_device_ready(vdev); > > - device_add_disk(&vdev->dev, vblk->disk, NULL); > - err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial); > - if (err) > - goto out_del_disk; > - > - if (virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE)) > - err = device_create_file(disk_to_dev(vblk->disk), > - &dev_attr_cache_type_rw); > - else > - err = device_create_file(disk_to_dev(vblk->disk), > - &dev_attr_cache_type_ro); > - if (err) > - goto out_del_disk; > + device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups); > return 0; > > -out_del_disk: > - del_gendisk(vblk->disk); > - blk_cleanup_queue(vblk->disk->queue); > out_free_tags: > blk_mq_free_tag_set(&vblk->tag_set); > out_put_disk: > -- > 2.12.3 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] virtio-blk: modernize sysfs attribute creation 2018-07-31 15:11 ` Michael S. Tsirkin @ 2018-07-31 15:19 ` Christoph Hellwig 0 siblings, 0 replies; 22+ messages in thread From: Christoph Hellwig @ 2018-07-31 15:19 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Hannes Reinecke, Jens Axboe, Christoph Hellwig, linux-block, Hannes Reinecke, Jason Wang On Tue, Jul 31, 2018 at 06:11:59PM +0300, Michael S. Tsirkin wrote: > On Mon, Jul 30, 2018 at 09:12:27AM +0200, Hannes Reinecke wrote: > > Use new-style DEVICE_ATTR_RO/DEVICE_ATTR_RW to create the sysfs attributes > > and register the disk with default sysfs attribute groups. > > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > Cc: Michael Tsirkin <mst@redhat.com> > > Cc: Jason Wang <jasowang@redhat.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > what's the right tree to merge this? The whole series should go in through the block tree I think. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-08-14 6:19 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-30 7:12 [PATCH 0/6] genhd: register default groups with device_add_disk() Hannes Reinecke 2018-07-30 7:12 ` [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk() Hannes Reinecke 2018-07-30 8:56 ` Christoph Hellwig 2018-07-30 8:57 ` Hannes Reinecke 2018-08-13 19:53 ` Bart Van Assche 2018-07-30 7:12 ` [PATCH 2/6] block: genhd: add 'groups' argument to device_add_disk Hannes Reinecke 2018-07-30 8:57 ` Christoph Hellwig 2018-07-30 7:12 ` [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups Hannes Reinecke 2018-07-30 8:59 ` Christoph Hellwig 2018-08-13 19:51 ` Bart Van Assche 2018-08-14 6:19 ` Hannes Reinecke 2018-07-30 7:12 ` [PATCH 4/6] aoe: use device_add_disk_with_groups() Hannes Reinecke 2018-07-30 9:01 ` Christoph Hellwig 2018-08-01 1:00 ` Ed Cashin 2018-08-01 6:57 ` Hannes Reinecke 2018-08-02 11:55 ` Ed Cashin 2018-07-30 7:12 ` [PATCH 5/6] zram: register default groups with device_add_disk() Hannes Reinecke 2018-07-30 9:02 ` Christoph Hellwig 2018-07-30 7:12 ` [PATCH 6/6] virtio-blk: modernize sysfs attribute creation Hannes Reinecke 2018-07-30 9:03 ` Christoph Hellwig 2018-07-31 15:11 ` Michael S. Tsirkin 2018-07-31 15:19 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.