Re: [PATCH v2] Add support for PCIe SSD status LED management

From: stuart hayes <stuart.w.hayes@gmail.com>
To: Pavel Machek <pavel@ucw.cz>
Cc: Randy Dunlap <rdunlap@infradead.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, Keith Busch <kbusch@kernel.org>,
	kw@linux.com,
	"linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>
Subject: Re: [PATCH v2] Add support for PCIe SSD status LED management
Date: Wed, 2 Jun 2021 10:36:03 -0500	[thread overview]
Message-ID: <552ca26f-c7ab-3496-743f-f95e80eb578b@gmail.com> (raw)
In-Reply-To: <20210602090504.GA10900@amd>

On 6/2/2021 4:05 AM, Pavel Machek wrote:
> Hi!
> 
>>>>>   [ok] [locate] failed rebuild pfa hotspare ica ifa invalid disabled
>>>
>>> So what does this do? Turns the LED on if driver is in "ok" or
>>> "locate" states?
>>>
>>
>> This would cause the system to somehow show the user that that this drive
>> (or drive slot) is the one that it wants a person to be able to physically
>> locate (possibly by flashing a blue LED on/near the drive), and also that
>> the drive is OK.  It would presumably do that by lighting the LEDs on/near
>> the drive with certain colors and/or flashing patterns, though it could, in
>> theory, put "OK" in an LCD on the drive slot.  How the states are displayed
>> to the user is beyond the scope of the specs.
>>
>> (The _DSM and NPEM specs provide for a way to control status LEDs on a drive
>> or drive slot.  Typically drives will have 2 or 3 LEDs that are illuminated
>> in different colors or flashing patterns to indicate various states (like
>> those listed in IBPI / SFF-8489), though the _DSM / NPEM specs do not
>> specify how the states are displayed.)
> 
> Well, LED subsystem expects to know how many LEDs are there and what
> the LEDs are, and expects individual control over them.
> 
> "2 or 3 LEDs with unknown patterns", or LCD display is outside of scope
> of LED subsystem, so this should be independend.
> 
> Best regards,
> 								Pavel
> 

Yes... I had originally submitted this without using the LED subsystem, 
and Greg K-H said I had to (see 
https://www.spinics.net/lists/linux-pci/msg102013.html).  Here's the 
relevant bit.

(Greg K-H:)
>> > And why isn't this code using the existing LED subsystem?  Don't create
>> > random new driver-specific sysfs files that do things the existing class
>> > drivers do please.
>> >
>> 
(Me:)
>> I did consider the LED subsystem, but I don't think it is a great match.
>> >> In spite of the name, this _DSM doesn't directly control individual 
LEDs--it
>> sets the state(s) of the PCI port to which an SSD may be connected, so that
>> it may be conveyed to the user... a processor (or at least some logic) will
>> decide how to show which states are active, probably by setting combinations
>> of LEDs to certain colors or blink patterns, or possibly on some other type
>> of display.
(Greg K-H:)
> If you are allowing LEDs to be controlled by the user, then yes, you
> have to use the LED subsystem as you should never try to create a brand
> new driver-specific user/kernel API just for your tiny driver right?
> Please work with the subsystems we have, they are unified for a reason.

So I'm not sure what to do.