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

[stuart hayes <stuart.w.hayes@gmail.com>]

On 6/12/2024 6:40 AM, Mariusz Tkaczyk wrote:
> Hi,
> Thanks for feedback Dan!
> 
> On Wed, 29 May 2024 11:38:12 +0200
> Lukas Wunner <lukas@wunner.de> wrote:
> 
>> On Tue, May 28, 2024 at 10:21:10PM -0700, Dan Williams wrote:
>>> Mariusz Tkaczyk wrote:
>>>> +config PCI_NPEM
>>>> +	bool "Native PCIe Enclosure Management"
>>>> +	depends on LEDS_CLASS=y
>>>
>>> I would have expected
>>>
>>>      depends on NEW_LEDS
>>>      select LEDS_CLASS
>>
>> Hm, a quick "git grep -C 2 'depends on NEW_LEDS'" shows that noone else
>> does that.  Everyone else either selects both NEW_LEDS and LEDS_CLASS
>> or depends on both or depends on just LEDS_CLASS.
>>
>> (Since LEDS_CLASS is constrained to "if NEW_LEDS", depending on both
>> seems pointless, so I'm not sure why some people do that.)
>>
>> I guess it would be good to get guidance from leds maintainers what
>> the preferred modus operandi is.
> 
> Pavel, could you please advice?
> I have no clue which way I should take so I prefer to keep current approach.
> 
>>
>>
>>>> +#define for_each_indication(ind, inds) \
>>>> +	for (ind = inds; ind->bit; ind++)
>>>> +
>>>> +/* To avoid confusion, do not keep any special bits in indications */
>>>
>>> I am confused by this comment. What "special bits" is this referring to?
>>
>> I think it's referring to bit 0 in the Status and Control register,
>> which is a master "NPEM Capable" and "NPEM Enable" bit.
> 
> Yes, there are 2 special bits for capability/control
> NPEM_CAP_CAPABLE/NPEM_ENABLE and NPEM_CAP_RESET/NPEM_RESET.
> 
> I wanted to highlight that these bits are not included in the cache. I will try
> to make it more precise in v3.
> 
>>
>>
>>>> +struct npem_ops {
>>>> +	const struct indication *inds;
>>>
>>> @inds is not an operation, it feels like something that belongs as
>>> another member in 'struct npem'. What drove this data to join 'struct
>>> npem_ops'?
>>
>> The native NPEM register interface supports enclosure-specific indications
>> which the DSM interface does not support.  So those indications are
>> present in the native npem_ops->inds and not present in the DSM
>> npem_ops->inds.
> 
> Yes, I need to differentiate DSM and NPEM indications. DSM has own indications
> list.
> 
>>
>>
>>>> --- a/include/uapi/linux/pci_regs.h
>>>> +++ b/include/uapi/linux/pci_regs.h
>> [...]
>>>> +#define  PCI_NPEM_IND_SPEC_0		0x00800000
>>>> +#define  PCI_NPEM_IND_SPEC_1		0x01000000
>>>> +#define  PCI_NPEM_IND_SPEC_2		0x02000000
>>>> +#define  PCI_NPEM_IND_SPEC_3		0x04000000
>>>> +#define  PCI_NPEM_IND_SPEC_4		0x08000000
>>>> +#define  PCI_NPEM_IND_SPEC_5		0x10000000
>>>> +#define  PCI_NPEM_IND_SPEC_6		0x20000000
>>>> +#define  PCI_NPEM_IND_SPEC_7		0x40000000
>>>
>>> Given no other driver needs this, I would define them locally in
>>> drivers/pci/npem.c.
>>
>> This is a uapi header, so could be used not just by other drivers
>> but by user space.
>>
>> It's common to add spec-defined register bits to this header file
>> even if they're only used by a single source file in the kernel.
>>
> 
> I will stay with current state while waiting for Bjorn's voice here.
> 
> I will send v3 with fixes requested by Dan and Ilpo but I still need Stuart
> feedback on DSM patch.
> 
> Thanks,
> Mariusz

I'm working on testing this now, sorry for the delay.
Thanks!  Stuart