From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga03-in.huawei.com ([119.145.14.66]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1YQuWC-0001GP-Eh for linux-mtd@lists.infradead.org; Thu, 26 Feb 2015 09:14:06 +0000 Message-ID: <54EEE32F.2000001@huawei.com> Date: Thu, 26 Feb 2015 17:11:11 +0800 From: hujianyang MIME-Version: 1.0 To: Takashi Iwai Subject: Re: [PATCH] UBI: Use static class and attribute groups References: <1423057871-9928-1-git-send-email-tiwai@suse.de> In-Reply-To: <1423057871-9928-1-git-send-email-tiwai@suse.de> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Cc: David Woodhouse , Brian Norris , linux-mtd@lists.infradead.org, Richard Weinberger , Artem Bityutskiy List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Takashi, I think it's a good attempt. Did you test this patch on your environment or just changing the code? How do you feel about this patch, Richard? I have some comments: On 2015/2/4 21:51, Takashi Iwai wrote: > This patch cleans up the manual device_create_file() or > class_create_file() calls by replacing with static attribute groups. > It simplifies the code and also avoids the possible races between the > device/class registration and sysfs creations. > > For the simplification, also make ubi_class a static instance with > initializers, too. > > Signed-off-by: Takashi Iwai > --- > drivers/mtd/ubi/build.c | 103 +++++++++++++++++------------------------------- > drivers/mtd/ubi/ubi.h | 2 +- > drivers/mtd/ubi/vmt.c | 95 ++++++++++---------------------------------- > 3 files changed, 59 insertions(+), 141 deletions(-) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index 3405be46ebe9..006f9aee292b 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -83,7 +83,7 @@ static struct mtd_dev_param __initdata mtd_dev_param[UBI_MAX_DEVICES]; > static bool fm_autoconvert; > #endif > /* Root UBI "class" object (corresponds to '//class/ubi/') */ > -struct class *ubi_class; > +struct class ubi_class; > > /* Slab cache for wear-leveling entries */ > struct kmem_cache *ubi_wl_entry_slab; > @@ -112,8 +112,10 @@ static ssize_t ubi_version_show(struct class *class, > } > > /* UBI version attribute ('//class/ubi/version') */ > -static struct class_attribute ubi_version = > - __ATTR(version, S_IRUGO, ubi_version_show, NULL); > +static struct class_attribute ubi_class_attrs[] = { > + __ATTR(version, S_IRUGO, ubi_version_show, NULL), > + {} Use '__ATTR_NULL' instead. > +}; > > static ssize_t dev_attribute_show(struct device *dev, > struct device_attribute *attr, char *buf); > @@ -385,6 +387,23 @@ static ssize_t dev_attribute_show(struct device *dev, > return ret; > } > > +static struct attribute *ubi_dev_attrs[] = { > + &dev_eraseblock_size.attr, > + &dev_avail_eraseblocks.attr, > + &dev_total_eraseblocks.attr, > + &dev_volumes_count.attr, > + &dev_max_ec.attr, > + &dev_reserved_for_bad.attr, > + &dev_bad_peb_count.attr, > + &dev_max_vol_count.attr, > + &dev_min_io_size.attr, > + &dev_bgt_enabled.attr, > + &dev_mtd_num.attr, > + NULL > +}; > + Remove the blank line. > +ATTRIBUTE_GROUPS(ubi_dev); > + > static void dev_release(struct device *dev) > { > struct ubi_device *ubi = container_of(dev, struct ubi_device, dev); > @@ -407,45 +426,15 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref) > > ubi->dev.release = dev_release; > ubi->dev.devt = ubi->cdev.dev; > - ubi->dev.class = ubi_class; > + ubi->dev.class = &ubi_class; > + ubi->dev.groups = ubi_dev_groups, > dev_set_name(&ubi->dev, UBI_NAME_STR"%d", ubi->ubi_num); > err = device_register(&ubi->dev); > if (err) > return err; > > *ref = 1; > - err = device_create_file(&ubi->dev, &dev_eraseblock_size); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_avail_eraseblocks); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_total_eraseblocks); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_volumes_count); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_max_ec); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_reserved_for_bad); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_bad_peb_count); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_max_vol_count); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_min_io_size); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_bgt_enabled); > - if (err) > - return err; > - err = device_create_file(&ubi->dev, &dev_mtd_num); > - return err; > + return 0; > } > > /** > @@ -454,17 +443,6 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref) > */ > static void ubi_sysfs_close(struct ubi_device *ubi) > { > - device_remove_file(&ubi->dev, &dev_mtd_num); > - device_remove_file(&ubi->dev, &dev_bgt_enabled); > - device_remove_file(&ubi->dev, &dev_min_io_size); > - device_remove_file(&ubi->dev, &dev_max_vol_count); > - device_remove_file(&ubi->dev, &dev_bad_peb_count); > - device_remove_file(&ubi->dev, &dev_reserved_for_bad); > - device_remove_file(&ubi->dev, &dev_max_ec); > - device_remove_file(&ubi->dev, &dev_volumes_count); > - device_remove_file(&ubi->dev, &dev_total_eraseblocks); > - device_remove_file(&ubi->dev, &dev_avail_eraseblocks); > - device_remove_file(&ubi->dev, &dev_eraseblock_size); > device_unregister(&ubi->dev); > } > > @@ -1213,6 +1191,12 @@ static struct mtd_info * __init open_mtd_device(const char *mtd_dev) > return mtd; > } > > +struct class ubi_class = { > + .name = UBI_NAME_STR, > + .owner = THIS_MODULE, > + .class_attrs = ubi_class_attrs, > +}; > + Could you move the definition of 'ubi_class' to the head? You'd declared it once. > static int __init ubi_init(void) > { > int err, i, k; > @@ -1228,23 +1212,14 @@ static int __init ubi_init(void) > } > > /* Create base sysfs directory and sysfs files */ > - ubi_class = class_create(THIS_MODULE, UBI_NAME_STR); > - if (IS_ERR(ubi_class)) { > - err = PTR_ERR(ubi_class); > - pr_err("UBI error: cannot create UBI class"); > - goto out; > - } > - > - err = class_create_file(ubi_class, &ubi_version); > - if (err) { > - pr_err("UBI error: cannot create sysfs file"); > - goto out_class; > - } > + err = class_register(&ubi_class); > + if (err < 0) > + return err; > > err = misc_register(&ubi_ctrl_cdev); > if (err) { > pr_err("UBI error: cannot register device"); > - goto out_version; > + goto out; > } > > ubi_wl_entry_slab = kmem_cache_create("ubi_wl_entry_slab", > @@ -1328,11 +1303,8 @@ out_slab: > kmem_cache_destroy(ubi_wl_entry_slab); > out_dev_unreg: > misc_deregister(&ubi_ctrl_cdev); > -out_version: > - class_remove_file(ubi_class, &ubi_version); > -out_class: > - class_destroy(ubi_class); > out: > + class_unregister(&ubi_class); > pr_err("UBI error: cannot initialize UBI, error %d", err); > return err; > } > @@ -1353,8 +1325,7 @@ static void __exit ubi_exit(void) > ubi_debugfs_exit(); > kmem_cache_destroy(ubi_wl_entry_slab); > misc_deregister(&ubi_ctrl_cdev); > - class_remove_file(ubi_class, &ubi_version); > - class_destroy(ubi_class); > + class_unregister(&ubi_class); > } > module_exit(ubi_exit); > > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h > index f80ffaba9058..59c673c60f85 100644 > --- a/drivers/mtd/ubi/ubi.h > +++ b/drivers/mtd/ubi/ubi.h > @@ -738,7 +738,7 @@ extern struct kmem_cache *ubi_wl_entry_slab; > extern const struct file_operations ubi_ctrl_cdev_operations; > extern const struct file_operations ubi_cdev_operations; > extern const struct file_operations ubi_vol_cdev_operations; > -extern struct class *ubi_class; > +extern struct class ubi_class; > extern struct mutex ubi_devices_mutex; > extern struct blocking_notifier_head ubi_notifiers; > > diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c > index ff4d97848d1c..7df0e26d1a90 100644 > --- a/drivers/mtd/ubi/vmt.c > +++ b/drivers/mtd/ubi/vmt.c > @@ -120,6 +120,20 @@ static ssize_t vol_attribute_show(struct device *dev, > return ret; > } > > +static struct attribute *volume_dev_attrs[] = { > + &attr_vol_reserved_ebs.attr, > + &attr_vol_type.attr, > + &attr_vol_name.attr, > + &attr_vol_corrupted.attr, > + &attr_vol_alignment.attr, > + &attr_vol_usable_eb_size.attr, > + &attr_vol_data_bytes.attr, > + &attr_vol_upd_marker.attr, > + NULL > +}; > + Remove the blank line. > +ATTRIBUTE_GROUPS(volume_dev); > + > /* Release method for volume devices */ > static void vol_release(struct device *dev) > { > @@ -130,64 +144,6 @@ static void vol_release(struct device *dev) > } > > /** > - * volume_sysfs_init - initialize sysfs for new volume. > - * @ubi: UBI device description object > - * @vol: volume description object > - * > - * This function returns zero in case of success and a negative error code in > - * case of failure. > - * > - * Note, this function does not free allocated resources in case of failure - > - * the caller does it. This is because this would cause release() here and the > - * caller would oops. > - */ > -static int volume_sysfs_init(struct ubi_device *ubi, struct ubi_volume *vol) > -{ > - int err; > - > - err = device_create_file(&vol->dev, &attr_vol_reserved_ebs); > - if (err) > - return err; > - err = device_create_file(&vol->dev, &attr_vol_type); > - if (err) > - return err; > - err = device_create_file(&vol->dev, &attr_vol_name); > - if (err) > - return err; > - err = device_create_file(&vol->dev, &attr_vol_corrupted); > - if (err) > - return err; > - err = device_create_file(&vol->dev, &attr_vol_alignment); > - if (err) > - return err; > - err = device_create_file(&vol->dev, &attr_vol_usable_eb_size); > - if (err) > - return err; > - err = device_create_file(&vol->dev, &attr_vol_data_bytes); > - if (err) > - return err; > - err = device_create_file(&vol->dev, &attr_vol_upd_marker); > - return err; > -} > - > -/** > - * volume_sysfs_close - close sysfs for a volume. > - * @vol: volume description object > - */ > -static void volume_sysfs_close(struct ubi_volume *vol) > -{ > - device_remove_file(&vol->dev, &attr_vol_upd_marker); > - device_remove_file(&vol->dev, &attr_vol_data_bytes); > - device_remove_file(&vol->dev, &attr_vol_usable_eb_size); > - device_remove_file(&vol->dev, &attr_vol_alignment); > - device_remove_file(&vol->dev, &attr_vol_corrupted); > - device_remove_file(&vol->dev, &attr_vol_name); > - device_remove_file(&vol->dev, &attr_vol_type); > - device_remove_file(&vol->dev, &attr_vol_reserved_ebs); > - device_unregister(&vol->dev); > -} > - > -/** > * ubi_create_volume - create volume. > * @ubi: UBI device description object > * @req: volume creation request > @@ -323,7 +279,8 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req) > vol->dev.release = vol_release; > vol->dev.parent = &ubi->dev; > vol->dev.devt = dev; > - vol->dev.class = ubi_class; > + vol->dev.class = &ubi_class; > + vol->dev.groups = volume_dev_groups; > > dev_set_name(&vol->dev, "%s_%d", ubi->ubi_name, vol->vol_id); > err = device_register(&vol->dev); > @@ -332,10 +289,6 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req) > goto out_cdev; > } > > - err = volume_sysfs_init(ubi, vol); > - if (err) > - goto out_sysfs; > - > /* Fill volume table record */ > memset(&vtbl_rec, 0, sizeof(struct ubi_vtbl_record)); > vtbl_rec.reserved_pebs = cpu_to_be32(vol->reserved_pebs); > @@ -372,7 +325,7 @@ out_sysfs: > */ > do_free = 0; > get_device(&vol->dev); > - volume_sysfs_close(vol); > + device_unregister(&vol->dev); > out_cdev: > cdev_del(&vol->cdev); > out_mapping: > @@ -440,7 +393,7 @@ int ubi_remove_volume(struct ubi_volume_desc *desc, int no_vtbl) > } > > cdev_del(&vol->cdev); > - volume_sysfs_close(vol); > + device_unregister(&vol->dev); > > spin_lock(&ubi->volumes_lock); > ubi->rsvd_pebs -= reserved_pebs; > @@ -653,19 +606,13 @@ int ubi_add_volume(struct ubi_device *ubi, struct ubi_volume *vol) > vol->dev.release = vol_release; > vol->dev.parent = &ubi->dev; > vol->dev.devt = dev; > - vol->dev.class = ubi_class; > + vol->dev.class = &ubi_class; > + vol->dev.groups = volume_dev_groups; > dev_set_name(&vol->dev, "%s_%d", ubi->ubi_name, vol->vol_id); > err = device_register(&vol->dev); > if (err) > goto out_cdev; > > - err = volume_sysfs_init(ubi, vol); > - if (err) { > - cdev_del(&vol->cdev); > - volume_sysfs_close(vol); > - return err; > - } > - > self_check_volumes(ubi); > return err; > > @@ -688,7 +635,7 @@ void ubi_free_volume(struct ubi_device *ubi, struct ubi_volume *vol) > > ubi->volumes[vol->vol_id] = NULL; > cdev_del(&vol->cdev); > - volume_sysfs_close(vol); > + device_unregister(&vol->dev); > } > > /** > Here is a problem about volume initialization, we have two function pairs before: ubi_sysfs_init/ubi_sysfs_close and volume_sysfs_init/volume_sysfs_close In your patch, you keep the front one but remove the pair of volume. I think you should keep them all. Thanks, Hu