| Message ID | 20241001221931.9309-4-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: partition table OF support | expand |
Thanks, this looks much better. A few minor nitpicks, though: > -int __must_check device_add_disk(struct device *parent, struct gendisk *disk, > - const struct attribute_group **groups) > +static int __device_add_disk(struct device *parent, struct gendisk *disk, > + const struct attribute_group **groups, > + struct fwnode_handle *fwnode) I don't think we need a separate helper if device_add_disk simply wraps the OF version by passing a NULL fwnode. > +int __must_check device_add_of_disk(struct device *parent, struct gendisk *disk, > + const struct attribute_group **groups, > + struct fwnode_handle *fwnode) > +{ > + return __device_add_disk(parent, disk, groups, fwnode); > +} I'd name this as add_disk_fwnode as the of in device_add_of_disk reads as in add the device of the disk, and the fwnode is what gets passed. The device_ is a bit redundant and just there for historic reasons as the original add_disk predates the device model. Can you also add a kerneldoc comment for the new helper? > +EXPORT_SYMBOL(device_add_of_disk); EXPORT_SYMBO_GPL, please.
On Wed, Oct 02, 2024 at 01:40:58AM -0700, Christoph Hellwig wrote: > Thanks, > > this looks much better. A few minor nitpicks, though: > Very happy you like it, yes I wasn't sure what was the correct way to introduce the helper. If you notice in the blkdev.h we have also add_disk() that is a static inline wrapper for device_add_disk(). Wonder if device_add_disk() should have the same treatement? No idea if it would cause problem with symbol with external modules, that is why I used the wrapper. > > -int __must_check device_add_disk(struct device *parent, struct gendisk *disk, > > - const struct attribute_group **groups) > > +static int __device_add_disk(struct device *parent, struct gendisk *disk, > > + const struct attribute_group **groups, > > + struct fwnode_handle *fwnode) > > I don't think we need a separate helper if device_add_disk simply > wraps the OF version by passing a NULL fwnode. > > > +int __must_check device_add_of_disk(struct device *parent, struct gendisk *disk, > > + const struct attribute_group **groups, > > + struct fwnode_handle *fwnode) > > +{ > > + return __device_add_disk(parent, disk, groups, fwnode); > > +} > > I'd name this as add_disk_fwnode as the of in device_add_of_disk > reads as in add the device of the disk, and the fwnode is what gets > passed. The device_ is a bit redundant and just there for historic > reasons as the original add_disk predates the device model. > > Can you also add a kerneldoc comment for the new helper? > sure! I will wait the usual 24h to respin this. > > +EXPORT_SYMBOL(device_add_of_disk); > > EXPORT_SYMBO_GPL, please. > ack.
On Wed, Oct 02, 2024 at 10:46:46AM +0200, Christian Marangi wrote: > Very happy you like it, yes I wasn't sure what was the correct way to > introduce the helper. If you notice in the blkdev.h we have also add_disk() > that is a static inline wrapper for device_add_disk(). > > Wonder if device_add_disk() should have the same treatement? No idea if > it would cause problem with symbol with external modules, that is why I > used the wrapper. We could make it an inline wrapper, but it's not in a high performance path so there isn't really much of a point in doing so. I don't remember why it was done for add_disk.
diff --git a/block/genhd.c b/block/genhd.c index 1c05dd4c6980..0fc595895f1d 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -391,8 +391,9 @@ int disk_scan_partitions(struct gendisk *disk, blk_mode_t mode) * This function registers the partitioning information in @disk * with the kernel. */ -int __must_check device_add_disk(struct device *parent, struct gendisk *disk, - const struct attribute_group **groups) +static int __device_add_disk(struct device *parent, struct gendisk *disk, + const struct attribute_group **groups, + struct fwnode_handle *fwnode) { struct device *ddev = disk_to_dev(disk); @@ -452,6 +453,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, ddev->parent = parent; ddev->groups = groups; dev_set_name(ddev, "%s", disk->disk_name); + if (fwnode) + device_set_node(ddev, fwnode); if (!(disk->flags & GENHD_FL_HIDDEN)) ddev->devt = MKDEV(disk->major, disk->first_minor); ret = device_add(ddev); @@ -553,8 +556,22 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk, elevator_exit(disk->queue); return ret; } + +int __must_check device_add_disk(struct device *parent, struct gendisk *disk, + const struct attribute_group **groups) +{ + return __device_add_disk(parent, disk, groups, NULL); +} EXPORT_SYMBOL(device_add_disk); +int __must_check device_add_of_disk(struct device *parent, struct gendisk *disk, + const struct attribute_group **groups, + struct fwnode_handle *fwnode) +{ + return __device_add_disk(parent, disk, groups, fwnode); +} +EXPORT_SYMBOL(device_add_of_disk); + static void blk_report_disk_dead(struct gendisk *disk, bool surprise) { struct block_device *bdev; diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index bf1aa951fda2..7d41f35f1065 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -725,6 +725,9 @@ static inline unsigned int blk_queue_depth(struct request_queue *q) #define for_each_bio(_bio) \ for (; _bio; _bio = _bio->bi_next) +int __must_check device_add_of_disk(struct device *parent, struct gendisk *disk, + const struct attribute_group **groups, + struct fwnode_handle *fwnode); int __must_check device_add_disk(struct device *parent, struct gendisk *disk, const struct attribute_group **groups); static inline int __must_check add_disk(struct gendisk *disk)
Introduce device_add_of_disk() as a variant of device_add_disk() that permits to pass and attach a fwnode to disk dev. This variant can be useful for eMMC that might have the partition table for the disk defined in DT. A parser can later make use of the attached fwnode to parse the related table and init the hardcoded partition for the disk. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- block/genhd.c | 21 +++++++++++++++++++-- include/linux/blkdev.h | 3 +++ 2 files changed, 22 insertions(+), 2 deletions(-)