Hi Jens, this series first cleans up various drivers to not rely on the internal GENHD_FL_UP to decided if they need to call del_gendisk and then removes the flag entirely. Diffstat: block/genhd.c | 6 - block/partitions/core.c | 4 - drivers/block/sx8.c | 2 drivers/md/bcache/super.c | 26 ++++--- drivers/md/md.h | 4 - drivers/mmc/core/block.c | 143 +++++++++++++++++------------------------- drivers/nvme/host/core.c | 16 ++-- drivers/nvme/host/multipath.c | 2 fs/block_dev.c | 2 include/linux/genhd.h | 9 +- 10 files changed, 93 insertions(+), 121 deletions(-)
Pass the attribute group for the attributes on the gendisk to device_add_disk so that they are created atomically with the disk creation. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/mmc/core/block.c | 102 +++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 57 deletions(-) diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index ce8aed562929..4ac3e1b93e7e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -128,8 +128,6 @@ struct mmc_blk_data { * track of the current selected device partition. */ unsigned int part_curr; - struct device_attribute force_ro; - struct device_attribute power_ro_lock; int area_type; /* debugfs files (only in main mmc_blk_data) */ @@ -281,6 +279,9 @@ static ssize_t power_ro_lock_store(struct device *dev, return count; } +static DEVICE_ATTR(ro_lock_until_next_power_on, 0, + power_ro_lock_show, power_ro_lock_store); + static ssize_t force_ro_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -313,6 +314,44 @@ static ssize_t force_ro_store(struct device *dev, struct device_attribute *attr, return ret; } +static DEVICE_ATTR(force_ro, 0644, force_ro_show, force_ro_store); + +static struct attribute *mmc_disk_attrs[] = { + &dev_attr_force_ro.attr, + &dev_attr_ro_lock_until_next_power_on.attr, + NULL, +}; + +static umode_t mmc_disk_attrs_is_visible(struct kobject *kobj, + struct attribute *a, int n) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev)); + umode_t mode = a->mode; + + if (a == &dev_attr_ro_lock_until_next_power_on.attr && + (md->area_type & MMC_BLK_DATA_AREA_BOOT) && + md->queue.card->ext_csd.boot_ro_lockable) { + mode = S_IRUGO; + if (!(md->queue.card->ext_csd.boot_ro_lock & + EXT_CSD_BOOT_WP_B_PWR_WP_DIS)) + mode |= S_IWUSR; + } + + mmc_blk_put(md); + return mode; +} + +static const struct attribute_group mmc_disk_attr_group = { + .is_visible = mmc_disk_attrs_is_visible, + .attrs = mmc_disk_attrs, +}; + +static const struct attribute_group *mmc_disk_attr_groups[] = { + &mmc_disk_attr_group, + NULL, +}; + static int mmc_blk_open(struct block_device *bdev, fmode_t mode) { struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk); @@ -2644,15 +2683,8 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md) * from being accepted. */ card = md->queue.card; - if (md->disk->flags & GENHD_FL_UP) { - device_remove_file(disk_to_dev(md->disk), &md->force_ro); - if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) && - card->ext_csd.boot_ro_lockable) - device_remove_file(disk_to_dev(md->disk), - &md->power_ro_lock); - + if (md->disk->flags & GENHD_FL_UP) del_gendisk(md->disk); - } mmc_cleanup_queue(&md->queue); mmc_blk_put(md); } @@ -2679,51 +2711,6 @@ static void mmc_blk_remove_parts(struct mmc_card *card, } } -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, NULL); - md->force_ro.show = force_ro_show; - md->force_ro.store = force_ro_store; - sysfs_attr_init(&md->force_ro.attr); - md->force_ro.attr.name = "force_ro"; - md->force_ro.attr.mode = S_IRUGO | S_IWUSR; - ret = device_create_file(disk_to_dev(md->disk), &md->force_ro); - if (ret) - goto force_ro_fail; - - if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) && - card->ext_csd.boot_ro_lockable) { - umode_t mode; - - if (card->ext_csd.boot_ro_lock & EXT_CSD_BOOT_WP_B_PWR_WP_DIS) - mode = S_IRUGO; - else - mode = S_IRUGO | S_IWUSR; - - md->power_ro_lock.show = power_ro_lock_show; - md->power_ro_lock.store = power_ro_lock_store; - sysfs_attr_init(&md->power_ro_lock.attr); - md->power_ro_lock.attr.mode = mode; - md->power_ro_lock.attr.name = - "ro_lock_until_next_power_on"; - ret = device_create_file(disk_to_dev(md->disk), - &md->power_ro_lock); - if (ret) - goto power_ro_lock_fail; - } - return ret; - -power_ro_lock_fail: - device_remove_file(disk_to_dev(md->disk), &md->force_ro); -force_ro_fail: - del_gendisk(md->disk); - - return ret; -} - #ifdef CONFIG_DEBUG_FS static int mmc_dbg_card_status_get(void *data, u64 *val) @@ -2919,12 +2906,13 @@ static int mmc_blk_probe(struct mmc_card *card) dev_set_drvdata(&card->dev, md); - ret = mmc_add_disk(md); + device_add_disk(md->parent, md->disk, mmc_disk_attr_groups); if (ret) goto out; list_for_each_entry(part_md, &md->part, part) { - ret = mmc_add_disk(part_md); + device_add_disk(part_md->parent, part_md->disk, + mmc_disk_attr_groups); if (ret) goto out; } -- 2.30.2
Restructure mmc_blk_probe to avoid a failure path with a half created disk. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/mmc/core/block.c | 49 ++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 4ac3e1b93e7e..4c11f171e56d 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2328,7 +2328,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, sector_t size, bool default_ro, const char *subname, - int area_type) + int area_type, + unsigned int part_type) { struct mmc_blk_data *md; int devidx, ret; @@ -2375,6 +2376,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, kref_init(&md->kref); md->queue.blkdata = md; + md->part_type = part_type; md->disk->major = MMC_BLOCK_MAJOR; md->disk->minors = perdev_minors; @@ -2427,6 +2429,10 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, md->disk->disk_name, mmc_card_id(card), mmc_card_name(card), cap_str, md->read_only ? "(ro)" : ""); + /* used in ->open, must be set before add_disk: */ + if (area_type == MMC_BLK_DATA_AREA_MAIN) + dev_set_drvdata(&card->dev, md); + device_add_disk(md->parent, md->disk, mmc_disk_attr_groups); return md; err_kfree: @@ -2456,7 +2462,7 @@ static struct mmc_blk_data *mmc_blk_alloc(struct mmc_card *card) } return mmc_blk_alloc_req(card, &card->dev, size, false, NULL, - MMC_BLK_DATA_AREA_MAIN); + MMC_BLK_DATA_AREA_MAIN, 0); } static int mmc_blk_alloc_part(struct mmc_card *card, @@ -2470,10 +2476,9 @@ static int mmc_blk_alloc_part(struct mmc_card *card, struct mmc_blk_data *part_md; part_md = mmc_blk_alloc_req(card, disk_to_dev(md->disk), size, default_ro, - subname, area_type); + subname, area_type, part_type); if (IS_ERR(part_md)) return PTR_ERR(part_md); - part_md->part_type = part_type; list_add(&part_md->part, &md->part); return 0; @@ -2674,20 +2679,13 @@ static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md) static void mmc_blk_remove_req(struct mmc_blk_data *md) { - struct mmc_card *card; - - if (md) { - /* - * Flush remaining requests and free queues. It - * is freeing the queue that stops new requests - * from being accepted. - */ - card = md->queue.card; - if (md->disk->flags & GENHD_FL_UP) - del_gendisk(md->disk); - mmc_cleanup_queue(&md->queue); - mmc_blk_put(md); - } + /* + * Flush remaining requests and free queues. It is freeing the queue + * that stops new requests from being accepted. + */ + del_gendisk(md->disk); + mmc_cleanup_queue(&md->queue); + mmc_blk_put(md); } static void mmc_blk_remove_parts(struct mmc_card *card, @@ -2876,7 +2874,7 @@ static void mmc_blk_remove_debugfs(struct mmc_card *card, static int mmc_blk_probe(struct mmc_card *card) { - struct mmc_blk_data *md, *part_md; + struct mmc_blk_data *md; int ret = 0; /* @@ -2904,19 +2902,6 @@ static int mmc_blk_probe(struct mmc_card *card) if (ret) goto out; - dev_set_drvdata(&card->dev, md); - - device_add_disk(md->parent, md->disk, mmc_disk_attr_groups); - if (ret) - goto out; - - list_for_each_entry(part_md, &md->part, part) { - device_add_disk(part_md->parent, part_md->disk, - mmc_disk_attr_groups); - if (ret) - goto out; - } - /* Add two debugfs entries */ mmc_blk_add_debugfs(card, md); -- 2.30.2
Early probe failure never reaches nvme_ns_remove, so GENHD_FL_UP must be set at this point. Remove the check. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/nvme/host/core.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index dfd9dec0c1f6..e24a0143fd87 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3826,14 +3826,12 @@ static void nvme_ns_remove(struct nvme_ns *ns) nvme_mpath_clear_current_path(ns); synchronize_srcu(&ns->head->srcu); /* wait for concurrent submissions */ - if (ns->disk->flags & GENHD_FL_UP) { - if (!nvme_ns_head_multipath(ns->head)) - nvme_cdev_del(&ns->cdev, &ns->cdev_device); - del_gendisk(ns->disk); - blk_cleanup_queue(ns->queue); - if (blk_get_integrity(ns->disk)) - blk_integrity_unregister(ns->disk); - } + if (!nvme_ns_head_multipath(ns->head)) + nvme_cdev_del(&ns->cdev, &ns->cdev_device); + del_gendisk(ns->disk); + blk_cleanup_queue(ns->queue); + if (blk_get_integrity(ns->disk)) + blk_integrity_unregister(ns->disk); down_write(&ns->ctrl->namespaces_rwsem); list_del_init(&ns->list); -- 2.30.2
Use the nvme-internal NVME_NSHEAD_DISK_LIVE flag instead of abusing the block layer state. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/nvme/host/multipath.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index 3f32c5e86bfc..37ce3e8b1db2 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -765,7 +765,7 @@ void nvme_mpath_shutdown_disk(struct nvme_ns_head *head) if (!head->disk) return; kblockd_schedule_work(&head->requeue_work); - if (head->disk->flags & GENHD_FL_UP) { + if (test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) { nvme_cdev_del(&head->cdev, &head->cdev_device); del_gendisk(head->disk); } -- 2.30.2
Remove usage of the block layer internal GENHD_FL_UP by just looking at the host state. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/block/sx8.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c index 7b54353ee92b..420cd952ddc4 100644 --- a/drivers/block/sx8.c +++ b/drivers/block/sx8.c @@ -1373,7 +1373,7 @@ static void carm_free_disk(struct carm_host *host, unsigned int port_no) if (!disk) return; - if (disk->flags & GENHD_FL_UP) + if (host->state > HST_DEV_ACTIVATE) del_gendisk(disk); blk_cleanup_disk(disk); } -- 2.30.2
Except for the IDA none of the allocations in bcache_device_init is unwound on error, fix that. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/md/bcache/super.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index 185246a0d855..d0f08e946453 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -931,20 +931,20 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size, n = BITS_TO_LONGS(d->nr_stripes) * sizeof(unsigned long); d->full_dirty_stripes = kvzalloc(n, GFP_KERNEL); if (!d->full_dirty_stripes) - return -ENOMEM; + goto out_free_stripe_sectors_dirty; idx = ida_simple_get(&bcache_device_idx, 0, BCACHE_DEVICE_IDX_MAX, GFP_KERNEL); if (idx < 0) - return idx; + goto out_free_full_dirty_stripes; if (bioset_init(&d->bio_split, 4, offsetof(struct bbio, bio), BIOSET_NEED_BVECS|BIOSET_NEED_RESCUER)) - goto err; + goto out_ida_remove; d->disk = blk_alloc_disk(NUMA_NO_NODE); if (!d->disk) - goto err; + goto out_bioset_exit; set_capacity(d->disk, sectors); snprintf(d->disk->disk_name, DISK_NAME_LEN, "bcache%i", idx); @@ -987,8 +987,14 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size, return 0; -err: +out_bioset_exit: + bioset_exit(&d->bio_split); +out_ida_remove: ida_simple_remove(&bcache_device_idx, idx); +out_free_full_dirty_stripes: + kvfree(d->full_dirty_stripes); +out_free_stripe_sectors_dirty: + kvfree(d->stripe_sectors_dirty); return -ENOMEM; } -- 2.30.2
Let the callers call del_gendisk so that we can check if add_disk has been called properly for the cached device case instead of relying on the block layer internal GENHD_FL_UP flag. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/md/bcache/super.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index d0f08e946453..f2874c77ff79 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -885,11 +885,6 @@ static void bcache_device_free(struct bcache_device *d) bcache_device_detach(d); if (disk) { - bool disk_added = (disk->flags & GENHD_FL_UP) != 0; - - if (disk_added) - del_gendisk(disk); - blk_cleanup_disk(disk); ida_simple_remove(&bcache_device_idx, first_minor_to_idx(disk->first_minor)); @@ -1371,8 +1366,10 @@ static void cached_dev_free(struct closure *cl) mutex_lock(&bch_register_lock); - if (atomic_read(&dc->running)) + if (atomic_read(&dc->running)) { bd_unlink_disk_holder(dc->bdev, dc->disk.disk); + del_gendisk(dc->disk.disk); + } bcache_device_free(&dc->disk); list_del(&dc->list); @@ -1518,6 +1515,7 @@ static void flash_dev_free(struct closure *cl) mutex_lock(&bch_register_lock); atomic_long_sub(bcache_dev_sectors_dirty(d), &d->c->flash_dev_dirty_sectors); + del_gendisk(d->disk); bcache_device_free(d); mutex_unlock(&bch_register_lock); kobject_put(&d->kobj); -- 2.30.2
Just check inode_unhashed on the whole device bdev inode instead, and provide a helper to check for that information. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/genhd.c | 6 ++---- block/partitions/core.c | 4 ++-- drivers/md/md.h | 4 +--- drivers/nvme/host/core.c | 2 +- fs/block_dev.c | 2 +- include/linux/genhd.h | 9 +++++---- 6 files changed, 12 insertions(+), 15 deletions(-) diff --git a/block/genhd.c b/block/genhd.c index a4817e42f3a3..9021c8ce3869 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -77,7 +77,8 @@ bool set_capacity_and_notify(struct gendisk *disk, sector_t size) * initial capacity during probing. */ if (size == capacity || - (disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP) + !disk_live(disk) || + (disk->flags & GENHD_FL_HIDDEN)) return false; pr_info("%s: detected capacity change from %lld to %lld\n", @@ -519,8 +520,6 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, disk->flags |= GENHD_FL_EXT_DEVT; } - disk->flags |= GENHD_FL_UP; - disk_alloc_events(disk); if (disk->flags & GENHD_FL_HIDDEN) { @@ -604,7 +603,6 @@ void del_gendisk(struct gendisk *disk) mutex_lock(&disk->open_mutex); remove_inode_hash(disk->part0->bd_inode); - disk->flags &= ~GENHD_FL_UP; blk_drop_partitions(disk); mutex_unlock(&disk->open_mutex); diff --git a/block/partitions/core.c b/block/partitions/core.c index fb3a556cacce..c6738ccbcee5 100644 --- a/block/partitions/core.c +++ b/block/partitions/core.c @@ -459,7 +459,7 @@ int bdev_add_partition(struct block_device *bdev, int partno, int ret; mutex_lock(&disk->open_mutex); - if (!(disk->flags & GENHD_FL_UP)) { + if (!disk_live(disk)) { ret = -ENXIO; goto out; } @@ -669,7 +669,7 @@ int bdev_disk_changed(struct gendisk *disk, bool invalidate) lockdep_assert_held(&disk->open_mutex); - if (!(disk->flags & GENHD_FL_UP)) + if (!disk_live(disk)) return -ENXIO; rescan: diff --git a/drivers/md/md.h b/drivers/md/md.h index 832547cf038f..4c96c36bd01a 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -764,9 +764,7 @@ struct md_rdev *md_find_rdev_rcu(struct mddev *mddev, dev_t dev); static inline bool is_mddev_broken(struct md_rdev *rdev, const char *md_type) { - int flags = rdev->bdev->bd_disk->flags; - - if (!(flags & GENHD_FL_UP)) { + if (!disk_live(rdev->bdev->bd_disk)) { if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags)) pr_warn("md: %s: %s array has a missing/failed member\n", mdname(rdev->mddev), md_type); diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index e24a0143fd87..cb12d8b94e82 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1822,7 +1822,7 @@ static void nvme_update_disk_info(struct gendisk *disk, static inline bool nvme_first_scan(struct gendisk *disk) { /* nvme_alloc_ns() scans the disk prior to adding it */ - return !(disk->flags & GENHD_FL_UP); + return !disk_live(disk); } static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id) diff --git a/fs/block_dev.c b/fs/block_dev.c index 6658f40ae492..ab50428b328b 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -1374,7 +1374,7 @@ struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode, void *holder) mutex_lock(&disk->open_mutex); ret = -ENXIO; - if (!(disk->flags & GENHD_FL_UP)) + if (!disk_live(disk)) goto abort_claiming; if (bdev_is_partition(bdev)) ret = blkdev_get_part(bdev, mode); diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 849486de81c6..ddd02def1ed4 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -60,9 +60,6 @@ struct partition_meta_info { * device. * Affects responses to the ``CDROM_GET_CAPABILITY`` ioctl. * - * ``GENHD_FL_UP`` (0x0010): indicates that the block device is "up", - * with a similar meaning to network interfaces. - * * ``GENHD_FL_SUPPRESS_PARTITION_INFO`` (0x0020): don't include * partition information in ``/proc/partitions`` or in the output of * printk_all_partitions(). @@ -97,7 +94,6 @@ struct partition_meta_info { /* 2 is unused (used to be GENHD_FL_DRIVERFS) */ /* 4 is unused (used to be GENHD_FL_MEDIA_CHANGE_NOTIFY) */ #define GENHD_FL_CD 0x0008 -#define GENHD_FL_UP 0x0010 #define GENHD_FL_SUPPRESS_PARTITION_INFO 0x0020 #define GENHD_FL_EXT_DEVT 0x0040 #define GENHD_FL_NATIVE_CAPACITY 0x0080 @@ -175,6 +171,11 @@ struct gendisk { u64 diskseq; }; +static inline bool disk_live(struct gendisk *disk) +{ + return !inode_unhashed(disk->part0->bd_inode); +} + /* * The gendisk is refcounted by the part0 block_device, and the bd_device * therein is also used for device model presentation in sysfs. -- 2.30.2
On Mon, 9 Aug 2021 at 08:43, Christoph Hellwig <hch@lst.de> wrote: > > Pass the attribute group for the attributes on the gendisk to > device_add_disk so that they are created atomically with the > disk creation. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> Let's try to funnel this via Jens' tree. As long as his tree is based upon v5.14-rc3 or later I don't think we should have any problem with conflicts. Kind regards Uffe > --- > drivers/mmc/core/block.c | 102 +++++++++++++++++---------------------- > 1 file changed, 45 insertions(+), 57 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index ce8aed562929..4ac3e1b93e7e 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -128,8 +128,6 @@ struct mmc_blk_data { > * track of the current selected device partition. > */ > unsigned int part_curr; > - struct device_attribute force_ro; > - struct device_attribute power_ro_lock; > int area_type; > > /* debugfs files (only in main mmc_blk_data) */ > @@ -281,6 +279,9 @@ static ssize_t power_ro_lock_store(struct device *dev, > return count; > } > > +static DEVICE_ATTR(ro_lock_until_next_power_on, 0, > + power_ro_lock_show, power_ro_lock_store); > + > static ssize_t force_ro_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > @@ -313,6 +314,44 @@ static ssize_t force_ro_store(struct device *dev, struct device_attribute *attr, > return ret; > } > > +static DEVICE_ATTR(force_ro, 0644, force_ro_show, force_ro_store); > + > +static struct attribute *mmc_disk_attrs[] = { > + &dev_attr_force_ro.attr, > + &dev_attr_ro_lock_until_next_power_on.attr, > + NULL, > +}; > + > +static umode_t mmc_disk_attrs_is_visible(struct kobject *kobj, > + struct attribute *a, int n) > +{ > + struct device *dev = container_of(kobj, struct device, kobj); > + struct mmc_blk_data *md = mmc_blk_get(dev_to_disk(dev)); > + umode_t mode = a->mode; > + > + if (a == &dev_attr_ro_lock_until_next_power_on.attr && > + (md->area_type & MMC_BLK_DATA_AREA_BOOT) && > + md->queue.card->ext_csd.boot_ro_lockable) { > + mode = S_IRUGO; > + if (!(md->queue.card->ext_csd.boot_ro_lock & > + EXT_CSD_BOOT_WP_B_PWR_WP_DIS)) > + mode |= S_IWUSR; > + } > + > + mmc_blk_put(md); > + return mode; > +} > + > +static const struct attribute_group mmc_disk_attr_group = { > + .is_visible = mmc_disk_attrs_is_visible, > + .attrs = mmc_disk_attrs, > +}; > + > +static const struct attribute_group *mmc_disk_attr_groups[] = { > + &mmc_disk_attr_group, > + NULL, > +}; > + > static int mmc_blk_open(struct block_device *bdev, fmode_t mode) > { > struct mmc_blk_data *md = mmc_blk_get(bdev->bd_disk); > @@ -2644,15 +2683,8 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md) > * from being accepted. > */ > card = md->queue.card; > - if (md->disk->flags & GENHD_FL_UP) { > - device_remove_file(disk_to_dev(md->disk), &md->force_ro); > - if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) && > - card->ext_csd.boot_ro_lockable) > - device_remove_file(disk_to_dev(md->disk), > - &md->power_ro_lock); > - > + if (md->disk->flags & GENHD_FL_UP) > del_gendisk(md->disk); > - } > mmc_cleanup_queue(&md->queue); > mmc_blk_put(md); > } > @@ -2679,51 +2711,6 @@ static void mmc_blk_remove_parts(struct mmc_card *card, > } > } > > -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, NULL); > - md->force_ro.show = force_ro_show; > - md->force_ro.store = force_ro_store; > - sysfs_attr_init(&md->force_ro.attr); > - md->force_ro.attr.name = "force_ro"; > - md->force_ro.attr.mode = S_IRUGO | S_IWUSR; > - ret = device_create_file(disk_to_dev(md->disk), &md->force_ro); > - if (ret) > - goto force_ro_fail; > - > - if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) && > - card->ext_csd.boot_ro_lockable) { > - umode_t mode; > - > - if (card->ext_csd.boot_ro_lock & EXT_CSD_BOOT_WP_B_PWR_WP_DIS) > - mode = S_IRUGO; > - else > - mode = S_IRUGO | S_IWUSR; > - > - md->power_ro_lock.show = power_ro_lock_show; > - md->power_ro_lock.store = power_ro_lock_store; > - sysfs_attr_init(&md->power_ro_lock.attr); > - md->power_ro_lock.attr.mode = mode; > - md->power_ro_lock.attr.name = > - "ro_lock_until_next_power_on"; > - ret = device_create_file(disk_to_dev(md->disk), > - &md->power_ro_lock); > - if (ret) > - goto power_ro_lock_fail; > - } > - return ret; > - > -power_ro_lock_fail: > - device_remove_file(disk_to_dev(md->disk), &md->force_ro); > -force_ro_fail: > - del_gendisk(md->disk); > - > - return ret; > -} > - > #ifdef CONFIG_DEBUG_FS > > static int mmc_dbg_card_status_get(void *data, u64 *val) > @@ -2919,12 +2906,13 @@ static int mmc_blk_probe(struct mmc_card *card) > > dev_set_drvdata(&card->dev, md); > > - ret = mmc_add_disk(md); > + device_add_disk(md->parent, md->disk, mmc_disk_attr_groups); > if (ret) > goto out; > > list_for_each_entry(part_md, &md->part, part) { > - ret = mmc_add_disk(part_md); > + device_add_disk(part_md->parent, part_md->disk, > + mmc_disk_attr_groups); > if (ret) > goto out; > } > -- > 2.30.2 >
On Mon, 9 Aug 2021 at 08:44, Christoph Hellwig <hch@lst.de> wrote: > > Restructure mmc_blk_probe to avoid a failure path with a half created > disk. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Acked-by: Ulf Hansson <ulf.hansson@linaro.org> Let's try to funnel this via Jens' tree. As long as his tree is based upon v5.14-rc3 or later I don't think we should have any problem with conflicts. Kind regards Uffe > --- > drivers/mmc/core/block.c | 49 ++++++++++++++-------------------------- > 1 file changed, 17 insertions(+), 32 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index 4ac3e1b93e7e..4c11f171e56d 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -2328,7 +2328,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > sector_t size, > bool default_ro, > const char *subname, > - int area_type) > + int area_type, > + unsigned int part_type) > { > struct mmc_blk_data *md; > int devidx, ret; > @@ -2375,6 +2376,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > kref_init(&md->kref); > > md->queue.blkdata = md; > + md->part_type = part_type; > > md->disk->major = MMC_BLOCK_MAJOR; > md->disk->minors = perdev_minors; > @@ -2427,6 +2429,10 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card, > md->disk->disk_name, mmc_card_id(card), mmc_card_name(card), > cap_str, md->read_only ? "(ro)" : ""); > > + /* used in ->open, must be set before add_disk: */ > + if (area_type == MMC_BLK_DATA_AREA_MAIN) > + dev_set_drvdata(&card->dev, md); > + device_add_disk(md->parent, md->disk, mmc_disk_attr_groups); > return md; > > err_kfree: > @@ -2456,7 +2462,7 @@ static struct mmc_blk_data *mmc_blk_alloc(struct mmc_card *card) > } > > return mmc_blk_alloc_req(card, &card->dev, size, false, NULL, > - MMC_BLK_DATA_AREA_MAIN); > + MMC_BLK_DATA_AREA_MAIN, 0); > } > > static int mmc_blk_alloc_part(struct mmc_card *card, > @@ -2470,10 +2476,9 @@ static int mmc_blk_alloc_part(struct mmc_card *card, > struct mmc_blk_data *part_md; > > part_md = mmc_blk_alloc_req(card, disk_to_dev(md->disk), size, default_ro, > - subname, area_type); > + subname, area_type, part_type); > if (IS_ERR(part_md)) > return PTR_ERR(part_md); > - part_md->part_type = part_type; > list_add(&part_md->part, &md->part); > > return 0; > @@ -2674,20 +2679,13 @@ static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md) > > static void mmc_blk_remove_req(struct mmc_blk_data *md) > { > - struct mmc_card *card; > - > - if (md) { > - /* > - * Flush remaining requests and free queues. It > - * is freeing the queue that stops new requests > - * from being accepted. > - */ > - card = md->queue.card; > - if (md->disk->flags & GENHD_FL_UP) > - del_gendisk(md->disk); > - mmc_cleanup_queue(&md->queue); > - mmc_blk_put(md); > - } > + /* > + * Flush remaining requests and free queues. It is freeing the queue > + * that stops new requests from being accepted. > + */ > + del_gendisk(md->disk); > + mmc_cleanup_queue(&md->queue); > + mmc_blk_put(md); > } > > static void mmc_blk_remove_parts(struct mmc_card *card, > @@ -2876,7 +2874,7 @@ static void mmc_blk_remove_debugfs(struct mmc_card *card, > > static int mmc_blk_probe(struct mmc_card *card) > { > - struct mmc_blk_data *md, *part_md; > + struct mmc_blk_data *md; > int ret = 0; > > /* > @@ -2904,19 +2902,6 @@ static int mmc_blk_probe(struct mmc_card *card) > if (ret) > goto out; > > - dev_set_drvdata(&card->dev, md); > - > - device_add_disk(md->parent, md->disk, mmc_disk_attr_groups); > - if (ret) > - goto out; > - > - list_for_each_entry(part_md, &md->part, part) { > - device_add_disk(part_md->parent, part_md->disk, > - mmc_disk_attr_groups); > - if (ret) > - goto out; > - } > - > /* Add two debugfs entries */ > mmc_blk_add_debugfs(card, md); > > -- > 2.30.2 >
On 8/9/21 2:40 PM, Christoph Hellwig wrote: > Let the callers call del_gendisk so that we can check if add_disk > has been called properly for the cached device case instead of relying > on the block layer internal GENHD_FL_UP flag. > > Signed-off-by: Christoph Hellwig <hch@lst.de> It looks good to me. Reviewed-by: Coly Li <colyli@suse.de> Thanks. Coly Li > --- > drivers/md/bcache/super.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index d0f08e946453..f2874c77ff79 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -885,11 +885,6 @@ static void bcache_device_free(struct bcache_device *d) > bcache_device_detach(d); > > if (disk) { > - bool disk_added = (disk->flags & GENHD_FL_UP) != 0; > - > - if (disk_added) > - del_gendisk(disk); > - > blk_cleanup_disk(disk); > ida_simple_remove(&bcache_device_idx, > first_minor_to_idx(disk->first_minor)); > @@ -1371,8 +1366,10 @@ static void cached_dev_free(struct closure *cl) > > mutex_lock(&bch_register_lock); > > - if (atomic_read(&dc->running)) > + if (atomic_read(&dc->running)) { > bd_unlink_disk_holder(dc->bdev, dc->disk.disk); > + del_gendisk(dc->disk.disk); > + } > bcache_device_free(&dc->disk); > list_del(&dc->list); > > @@ -1518,6 +1515,7 @@ static void flash_dev_free(struct closure *cl) > mutex_lock(&bch_register_lock); > atomic_long_sub(bcache_dev_sectors_dirty(d), > &d->c->flash_dev_dirty_sectors); > + del_gendisk(d->disk); > bcache_device_free(d); > mutex_unlock(&bch_register_lock); > kobject_put(&d->kobj);
On 8/9/21 2:40 PM, Christoph Hellwig wrote: > Except for the IDA none of the allocations in bcache_device_init is > unwound on error, fix that. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Acked-by: Coly Li <colyli@suse.de> Thanks. Coly Li > --- > drivers/md/bcache/super.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index 185246a0d855..d0f08e946453 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -931,20 +931,20 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size, > n = BITS_TO_LONGS(d->nr_stripes) * sizeof(unsigned long); > d->full_dirty_stripes = kvzalloc(n, GFP_KERNEL); > if (!d->full_dirty_stripes) > - return -ENOMEM; > + goto out_free_stripe_sectors_dirty; > > idx = ida_simple_get(&bcache_device_idx, 0, > BCACHE_DEVICE_IDX_MAX, GFP_KERNEL); > if (idx < 0) > - return idx; > + goto out_free_full_dirty_stripes; > > if (bioset_init(&d->bio_split, 4, offsetof(struct bbio, bio), > BIOSET_NEED_BVECS|BIOSET_NEED_RESCUER)) > - goto err; > + goto out_ida_remove; > > d->disk = blk_alloc_disk(NUMA_NO_NODE); > if (!d->disk) > - goto err; > + goto out_bioset_exit; > > set_capacity(d->disk, sectors); > snprintf(d->disk->disk_name, DISK_NAME_LEN, "bcache%i", idx); > @@ -987,8 +987,14 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size, > > return 0; > > -err: > +out_bioset_exit: > + bioset_exit(&d->bio_split); > +out_ida_remove: > ida_simple_remove(&bcache_device_idx, idx); > +out_free_full_dirty_stripes: > + kvfree(d->full_dirty_stripes); > +out_free_stripe_sectors_dirty: > + kvfree(d->stripe_sectors_dirty); > return -ENOMEM; > > }
On 8/9/21 12:40 AM, Christoph Hellwig wrote:
> Hi Jens,
>
> this series first cleans up various drivers to not rely on the
> internal GENHD_FL_UP to decided if they need to call del_gendisk
> and then removes the flag entirely.
Applied, thanks.
--
Jens Axboe