[v2,09/15] leds: trigger: blkdev: Check devices for activity and blink LEDs

Use a delayed workqueue to periodically check configured block devices for
activity since the last check.  Blink LEDs associated with devices on which
the configured type of activity (read/write) has occurred.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 drivers/leds/trigger/ledtrig-blkdev.c | 103 ++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

On Thu,  9 Sep 2021 17:25:07 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> +static void blkdev_blink(const struct ledtrig_blkdev_led *const led)
> +{

Why are you declaring the led variable as const? This is not needed.
Sure, you do not change it, but I have never seen this being used in
this way in kernel.

> +	unsigned long delay_on = READ_ONCE(led->blink_msec);
> +	unsigned long delay_off = 1;	/* 0 leaves LED turned on */
> +
> +	led_blink_set_oneshot(led->led_dev, &delay_on, &delay_off, 0);
> +}
> +
> +static void blkdev_update_disk(struct ledtrig_blkdev_disk *const disk,
> +			       const unsigned int generation)
> +{
> +	const struct block_device *const part0 = disk->gd->part0;
> +	const unsigned long read_ios = part_stat_read(part0, ios[STAT_READ]);
> +	const unsigned long write_ios = part_stat_read(part0, ios[STAT_WRITE])
> +				+ part_stat_read(part0, ios[STAT_DISCARD])
> +				+ part_stat_read(part0, ios[STAT_FLUSH]);

Again, yes, you do not change part0, read_ios or write_ios in this
function, but this does not mean you need to declare them const.

Const is good when you want to declare that a place where a pointer
points to should be constant. You don't need to do it for the pointer
itself, I don't see any benefit from this.

> +
> +	if (disk->read_ios != read_ios) {
> +		disk->read_act = true;
> +		disk->read_ios = read_ios;
> +	} else {
> +		disk->read_act = false;
> +	}
> +
> +	if (disk->write_ios != write_ios) {
> +		disk->write_act = true;
> +		disk->write_ios = write_ios;
> +	} else {
> +		disk->write_act = false;
> +	}
> +
> +	disk->generation = generation;
> +}
> +
> +static bool blkdev_read_mode(const enum ledtrig_blkdev_mode mode)
> +{
> +	return mode != LEDTRIG_BLKDEV_MODE_WO;
> +}
> +
> +static bool blkdev_write_mode(const enum ledtrig_blkdev_mode mode)
> +{
> +	return mode != LEDTRIG_BLKDEV_MODE_RO;
> +}

It would be better to simply do the comparison where it is needed,
since it is so short. These functions aren't needed, they do not
shorten code in any significant way, nor do they make it more readable
(in fact, they make it a little less readable).

> +
> +static void blkdev_process(struct work_struct *const work)

You are again using const where it is not needed.
In fact the work_func_t type does not have it:
  typedef void (*work_func_t)(struct work_struct *work);

> +{
> +	static unsigned int generation;
> +
> +	struct ledtrig_blkdev_led *led;
> +	struct ledtrig_blkdev_link *link;
> +	unsigned long delay;
> +
> +	if (!mutex_trylock(&ledtrig_blkdev_mutex))
> +		goto exit_reschedule;
> +
> +	hlist_for_each_entry(led, &ledtrig_blkdev_leds, leds_node) {
> +
> +		hlist_for_each_entry(link, &led->disks, led_disks_node) {
> +
> +			struct ledtrig_blkdev_disk *const disk = link->disk;
> +
> +			if (disk->generation != generation)
> +				blkdev_update_disk(disk, generation);
> +
> +			if (disk->read_act && blkdev_read_mode(led->mode)) {
> +				blkdev_blink(led);
> +				break;
> +			}
> +
> +			if (disk->write_act && blkdev_write_mode(led->mode)) {
> +				blkdev_blink(led);
> +				break;
> +			}

These two blinks should be one blink, i.e.
  if ((read_act && read_mode) || (write_act && write_mode))
    blink();

> +		}
> +	}
> +
> +	++generation;
> +
> +	mutex_unlock(&ledtrig_blkdev_mutex);
> +
> +exit_reschedule:
> +	delay = READ_ONCE(ledtrig_blkdev_interval);
> +	WARN_ON_ONCE(!schedule_delayed_work(&ledtrig_blkdev_work, delay));
> +}
> +
>  
>  /*
>   *

On Thu,  9 Sep 2021 17:25:07 -0500
Ian Pilcher <arequipeno@gmail.com> wrote:

> +static void blkdev_update_disk(struct ledtrig_blkdev_disk *const disk,
> +			       const unsigned int generation)
> +{
> +	const struct block_device *const part0 = disk->gd->part0;
> +	const unsigned long read_ios = part_stat_read(part0, ios[STAT_READ]);
> +	const unsigned long write_ios = part_stat_read(part0, ios[STAT_WRITE])
> +				+ part_stat_read(part0, ios[STAT_DISCARD])
> +				+ part_stat_read(part0, ios[STAT_FLUSH]);

So your code allows me to use a partition block device (like sda2) to
register with the blkdev LED trigger, but when I do this, the code will
disregard that I just want the LED to blink on activity on that one
partition. Instead you will blink for whole sda, since you are looking
at stats of only part0.

Am I right?

If so, this in unacceptable. The whole point of blkdev trigger is that
you can reliably use it for any block device, and then it will blink
the LED for that device, be it partition or whole disk.

Marek