All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pavel Machek <pavel@ucw.cz>
To: Randy Dunlap <rdunlap@infradead.org>
Cc: Stuart Hayes <stuart.w.hayes@gmail.com>,
	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 00:38:12 +0200	[thread overview]
Message-ID: <20210601223812.GA5128@amd> (raw)
In-Reply-To: <3d1272b8-4edc-f2b1-85ea-f5cea65b4871@infradead.org>

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

Hi!

> >  # echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
> >  # cat /sys/class/leds/0000:88:00.0::drive_status/states

This has two problems: ":" already has special meaning in LED name,
and we'd prefer not to have new "states" interface unless absolutely
neccessary.

> >  [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?

> > +Date:		April 2021
> > +Contact:	linux-pci@vger.kernel.org
> > +Description:
> > +		This attribute indicates the status states supported by a drive
> > +		or drive slot's LEDs, as defined in the "_DSM additions for PCIe
> > +		SSD Status LED Management" ECN to the PCI Firmware Specification
> > +		Revision 3.2, dated 12 February 2020, and in "Native PCIe
> > +		Enclosure Management", section 6.29 of the PCIe Base Spec 5.0.
> > +
> > +		Only supported states will be shown, and the currently active
> > +		states are shown in brackets.  The active state(s) can be written
> > +		by echoing a space or comma separated string of states to this
> > +		attribute.  For example:
> > +
> > +		# echo "ok locate" >/sys/class/leds/0000:88:00.0::drive_status/states
> > +		# cat /sys/class/leds/0000:88:00.0::drive_status/states
> > +		[ok] [locate] failed rebuild pfa hotspare ica ifa

This goes against "one value per file", really needs better
description, and probably needs different interface.

> > +config PCIE_SSD_LEDS
> > +	tristate "PCIe SSD status LED support"
> > +	depends on ACPI && NEW_LEDS
> 
> I expect that should be LEDS_CLASS instead of NEW_LEDS.
> Did you test it with NEW_LEDS=y and LEDS_CLASS not set?
> 
> 
> [adding Pavel and linux-leds m.l. for other review]

Thank you!
							Pavel
							
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

  reply	other threads:[~2021-06-01 22:38 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-01 20:38 [PATCH v2] Add support for PCIe SSD status LED management Stuart Hayes
2021-06-01 21:20 ` Randy Dunlap
2021-06-01 22:38   ` Pavel Machek [this message]
2021-06-02  3:18     ` stuart hayes
2021-06-02  9:05       ` Pavel Machek
2021-06-02 15:36         ` stuart hayes
2021-06-02 22:40           ` Pavel Machek
2021-06-04 20:13             ` stuart hayes
2021-06-02 16:33       ` Marek Behún
2021-06-01 23:39   ` Randy Dunlap
2021-06-01 23:48 ` kernel test robot
2021-06-01 23:48   ` kernel test robot
2021-06-02  7:05 ` Lukas Wunner

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=20210601223812.GA5128@amd \
    --to=pavel@ucw.cz \
    --cc=bhelgaas@google.com \
    --cc=kbusch@kernel.org \
    --cc=kw@linux.com \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=rdunlap@infradead.org \
    --cc=stuart.w.hayes@gmail.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.