linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ian Pilcher <arequipeno@gmail.com>
To: "Marek Behún" <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
Subject: Re: [RFC PATCH v2 00/10] Add configurable block device LED triggers
Date: Mon, 9 Aug 2021 14:54:26 -0500	[thread overview]
Message-ID: <81c128a1-c1b8-0f1e-a77b-6704bade26c0@gmail.com> (raw)
In-Reply-To: <20210809205633.4300bbea@thinkpad>

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!
========================================================================

  parent reply	other threads:[~2021-08-09 19:54 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-09  3:32 [RFC PATCH v2 00/10] Add configurable block device LED triggers Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 01/10] docs: Add block device LED trigger documentation Ian Pilcher
2021-08-10 13:49   ` Pavel Machek
2021-08-09  3:32 ` [RFC PATCH v2 02/10] block: Add file (blk-ledtrig.c) for block device LED trigger implementation Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 03/10] block: Add block device LED trigger fields to gendisk structure Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 04/10] block: Add functions to set & clear block device LEDs Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 05/10] block: Add block device sysfs attribute to set/clear/show LED Ian Pilcher
2021-08-09  4:21   ` Jackie Liu
2021-08-09 15:44     ` Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 06/10] block: Add activate and deactivate functions for block device LED trigger Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 07/10] block: Add sysfs attributes to LEDs associated with blkdev trigger Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 08/10] block: Add init function for block device LED trigger Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 09/10] block: Blink device LED (if any) when request is sent to its driver Ian Pilcher
2021-08-09  3:32 ` [RFC PATCH v2 10/10] block: Add config option to enable block device LED triggers Ian Pilcher
2021-08-09 18:56 ` [RFC PATCH v2 00/10] Add configurable " Marek Behún
2021-08-09 19:07   ` Pali Rohár
2021-08-09 19:54   ` Ian Pilcher [this message]
2021-08-09 22:43     ` Marek Behún
2021-08-09 23:50       ` Ian Pilcher
2021-08-10  6:35         ` Greg KH
2021-08-10 13:38           ` Marek Behún
2021-08-10 14:48             ` Greg KH
2021-08-10 15:55               ` Ian Pilcher
2021-08-10 16:24                 ` Greg KH
2021-08-10 16:39                   ` Marek Behún
2021-08-10 16:43                   ` Ian Pilcher
2021-08-11  6:26         ` Christoph Hellwig
2021-08-11 10:50           ` Marek Behún

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=81c128a1-c1b8-0f1e-a77b-6704bade26c0@gmail.com \
    --to=arequipeno@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=kabel@kernel.org \
    --cc=kernelnewbies@kernelnewbies.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pali@kernel.org \
    --cc=pavel@ucw.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).