Re: [PATCH 2/2] PCI/NPEM: Add Native PCIe Enclosure Management support

[Lukas Wunner <lukas@wunner.de>]

On Wed, Mar 06, 2024 at 04:40:08PM -0600, Bjorn Helgaas wrote:
> On Thu, Feb 15, 2024 at 03:23:45PM +0100, Mariusz Tkaczyk wrote:
> > The interface is ready to support enclosures where patterns are not
> > mutually exclusive, driver may clear other LEDs if they cannot be enabled
> > together.
> 
> I don't think this code does anything like "clearing other LEDs," does
> it?  I agree that this code doesn't impose any constraints about
> indications being mutually exclusive, etc.  It merely sets or clears
> indication bits, and the hardware implementation is free to interpret
> that as it sees fit.

I guess the paragraph needs to be rephrased.  The point is that
if this NPEM driver sets bit A and another bit B is already set,
and the hardware is incapable of lighting up whatever is controlled
by these two bits *simultaneously*, the hardware might clear bit B
when setting bit A.  The driver can cope with that because
npem_set() reads back the register contents with npem_read_reg()
after calling npem_set_active_patterns().


> > +	/*
> > +	 * For the case where a NPEM command has not completed immediately,
> > +	 * it is recommended that software not continuously ???spin??? on polling
> > +	 * the status register, but rather poll under interrupt at a reduced
> > +	 * rate; for example at 10 ms intervals.
> > +	 *
> > +	 * PCIe r6.1 sec 6.28 "Implementation Note: Software Polling of NPEM
> > +	 * Command Completed"
> 
> The implementation note also recommends reversing the order, i.e.,
> polling for completion of previous command and then writing a new
> command, but it looks like we don't use that strategy?

I think the leds subsystem expects the LED to be lit up by the time
the ->brightness_set_blocking() callback returns.  If the driver waited
for command completion *before* setting a bit instead of afterwards,
it could happen that npem_set() (the ->brightness_set_blocking() callback)
returns even though the command hasn't completed yet and the LED thus
isn't lit up yet.

Thanks,

Lukas