| Message ID | 20210909222513.2184795-5-arequipeno@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce block device LED trigger | expand |
On Thu, 9 Sep 2021 17:25:02 -0500 Ian Pilcher <arequipeno@gmail.com> wrote: > Add LED trigger disk info pointer to gendisk structure > > Call ledtrig_blkdev_disk_init() from device_add_disk() to ensure that > ledtrig is initialized to NULL, in case a driver allocates the structure > itself and doesn't use kzalloc() No, this is not needed. If someone does not use kzalloc(), they should use it. No need to fix other code here. Marek
On Thu, 9 Sep 2021 17:25:02 -0500 Ian Pilcher <arequipeno@gmail.com> wrote: > @@ -166,6 +166,9 @@ struct gendisk { > #endif /* CONFIG_BLK_DEV_INTEGRITY */ > #if IS_ENABLED(CONFIG_CDROM) > struct cdrom_device_info *cdi; > +#endif > +#if IS_ENABLED(CONFIG_LEDS_TRIGGER_BLKDEV) > + struct ledtrig_blkdev_disk *ledtrig; > #endif So in this patch you are adding pointer to a structure, but the structure is introduced in a later patch (07/15). Please send these changes as one patch, it is easier to review. Marek
On 9/9/21 8:23 PM, Marek Behún wrote: > On Thu, 9 Sep 2021 17:25:02 -0500 > Ian Pilcher <arequipeno@gmail.com> wrote: >> Call ledtrig_blkdev_disk_init() from device_add_disk() to ensure that >> ledtrig is initialized to NULL, in case a driver allocates the structure >> itself and doesn't use kzalloc() > > No, this is not needed. If someone does not use kzalloc(), they should > use it. No need to fix other code here. Yeah. I'm honestly not sure if this is necessary or not, as I don't know if there are any drivers that actually have this problem. I decided to include this for now, because an uninitialized pointer can cause memory corruption, etc., when the disk cleanup function follows a garbage pointer. This recent commit seems to indicate that until recently drivers were responsible for doing gendisk allocation. > commit f525464a8000f092c20b00eead3eaa9d849c599e > Author: Christoph Hellwig <hch@lst.de> > Date: Fri May 21 07:50:55 2021 +0200 > > block: add blk_alloc_disk and blk_cleanup_disk APIs > > Add two new APIs to allocate and free a gendisk including the > request_queue for use with BIO based drivers. This is to avoid > boilerplate code in drivers. Were those drivers expected to use kzalloc() or otherwise zero out the entire structure? I really don't know. I think that it makes sense to defer to the block subsystem maintainers on this question. -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
diff --git a/block/genhd.c b/block/genhd.c index 567549a011d1..6f340a717099 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -24,6 +24,7 @@ #include <linux/log2.h> #include <linux/pm_runtime.h> #include <linux/badblocks.h> +#include <linux/leds.h> #include "blk.h" @@ -390,6 +391,8 @@ int device_add_disk(struct device *parent, struct gendisk *disk, struct device *ddev = disk_to_dev(disk); int ret; + ledtrig_blkdev_disk_init(disk); + /* * The disk queue should now be all set with enough information about * the device for the elevator code to pick an adequate default @@ -559,6 +562,7 @@ void del_gendisk(struct gendisk *disk) if (WARN_ON_ONCE(!disk_live(disk) && !(disk->flags & GENHD_FL_HIDDEN))) return; + ledtrig_blkdev_disk_cleanup(disk); blk_integrity_del(disk); disk_del_events(disk); diff --git a/include/linux/genhd.h b/include/linux/genhd.h index c68d83c87f83..29039367ccad 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -166,6 +166,9 @@ struct gendisk { #endif /* CONFIG_BLK_DEV_INTEGRITY */ #if IS_ENABLED(CONFIG_CDROM) struct cdrom_device_info *cdi; +#endif +#if IS_ENABLED(CONFIG_LEDS_TRIGGER_BLKDEV) + struct ledtrig_blkdev_disk *ledtrig; #endif int node_id; struct badblocks *bb;
Add LED trigger disk info pointer to gendisk structure Call ledtrig_blkdev_disk_init() from device_add_disk() to ensure that ledtrig is initialized to NULL, in case a driver allocates the structure itself and doesn't use kzalloc() Call ledtrig_blkdev_disk_cleanup() from del_gendisk() to ensure that the LED trigger stops trying to check the disk for activity Signed-off-by: Ian Pilcher <arequipeno@gmail.com> --- block/genhd.c | 4 ++++ include/linux/genhd.h | 3 +++ 2 files changed, 7 insertions(+)