Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 10 Sep 2021 03:48:38 +0200
From:   Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>
To:     Ian Pilcher <arequipeno@gmail.com>
Cc:     axboe@kernel.dk, pavel@ucw.cz, linux-leds@vger.kernel.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        gregkh@linuxfoundation.org
Subject: Re: [PATCH v2 09/15] leds: trigger: blkdev: Check devices for
 activity and blink LEDs
Message-ID: <20210910034838.4331512d@thinkpad>
In-Reply-To: <20210909222513.2184795-10-arequipeno@gmail.com>
References: <20210909222513.2184795-1-arequipeno@gmail.com>
        <20210909222513.2184795-10-arequipeno@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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));
> +}
> +
>  
>  /*
>   *