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;
Subject: Re: [RFC PATCH v2 00/10] Add configurable block device LED triggers
To:     =?UTF-8?Q?Marek_Beh=c3=ban?= <kabel@kernel.org>, pali@kernel.org
Cc:     linux-block@vger.kernel.org, linux-leds@vger.kernel.org,
        axboe@kernel.dk, pavel@ucw.cz, linux-kernel@vger.kernel.org,
        kernelnewbies@kernelnewbies.org
References: <20210809033217.1113444-1-arequipeno@gmail.com>
 <20210809205633.4300bbea@thinkpad>
From:   Ian Pilcher <arequipeno@gmail.com>
Message-ID: <81c128a1-c1b8-0f1e-a77b-6704bade26c0@gmail.com>
Date:   Mon, 9 Aug 2021 14:54:26 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210809205633.4300bbea@thinkpad>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Marek -

Thanks for taking the time to look at this.

On 8/9/21 1:56 PM, Marek Behún wrote:
> It may be simpler, but it is in contrast to how the netdev trigger
> works, which already is in upstream for many years. I really think we
> should try to have similar sysfs ABIs here. (I understand that the
> netdev trigger is currently unable to handle multiple network
> interfaces - but it is possible to extend it so.)

I'm not unalterably opposed to the idea, but I don't currently see a way
to do that without resolving block devices (struct gendisk) by name, and
that seems to be a no-no.

If you (or anyone else) has a suggestion on how to get around this
obstacle, I'd be willing to give it a shot.

> I think it is reasonable to be able to set something like this:
>    led0 : blink on activity on any of [sda, sdb, sdc]
>    led1 : blink on activity on sda
>    led2 : blink on activity on sdb
>    led3 : blink on activity on sdc
> 
> If I am reading your code correctly, it looks that only one LED can be
> configured for a block device. Is this true? If so, then the above
> configuration cannot be set.

You're correct that it's not possible with the current code.  Multiple
devices can be associated to with a single LED, but there's not
currently a way to drive more than 1 LED from a single device.  This
is something that could be changed.

> Also you are blinking the LED on any request to the block device. I
> would rather expect to be able to set the LED to blink on read and on
> write. (And possibly on other functions, like discard, or critical
> temperature, or error, ...) I would like to know what other people
> think about this.

I wanted to keep things as simple as possible for now.  I don't think
that there's any particular reason that separated LEDs couldn't be
configured for read and write requests.  (It looks like it should be
pretty easy to distinguish reads vs writes in a struct request.)

My feeling is that things like temperature, errors, etc. are better
monitored from user space, as they tend to require actively querying
the drive.

Like you, I'm interested in knowing if there is actually hardware out
there that has separate read/write LEDs.

All in all, I feel like I should be able to implement almost everything
that you've suggested, *if* I can figure out the block device lookup
issue, but I really don't have any ideas on that.

Thanks for your patience and feedback!

-- 
========================================================================
                  In Soviet Russia, Google searches you!
========================================================================