Re: [PATCH v3 3/3] PCI/NPEM: Add _DSM PCIe SSD status LED management

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Fri, 5 Jul 2024, Mariusz Tkaczyk wrote:

> Device Specific Method PCIe SSD Status LED Management (_DSM) defined in
> PCI Firmware Spec r3.3 sec 4.7 provides a way to manage LEDs via ACPI.
> 
> The design is similar to NPEM defined in PCIe Base Specification r6.1
> sec 6.28:
>   - both standards are indication oriented,
>   - _DSM supported bits are corresponding to NPEM capability
>     register bits
>   - _DSM control bits are corresponding to NPEM control register
>     bits.
> 
> _DSM does not support enclosure specific indications and special NPEM
> commands NPEM_ENABLE and NPEM_RESET.
> 
> _DSM is implemented as a second op in NPEM driver. The standard used
> in background is not presented to user. The interface is accessed same
> as NPEM.
> 
> According to spec, _DSM has higher priority and availability  of _DSM
> in not limited to devices with NPEM support. It is followed in
> implementation.
> 
> Link: https://members.pcisig.com/wg/PCI-SIG/document/14025
> Link: https://members.pcisig.com/wg/PCI-SIG/document/15350
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>  drivers/pci/npem.c | 147 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 144 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/npem.c b/drivers/pci/npem.c
> index fd3366bc3fb0..e3bc28d089d3 100644
> --- a/drivers/pci/npem.c
> +++ b/drivers/pci/npem.c
> @@ -11,6 +11,14 @@
>   *	PCIe Base Specification r6.1 sec 6.28
>   *	PCIe Base Specification r6.1 sec 7.9.19
>   *
> + * _DSM Definitions for PCIe SSD Status LED
> + *	 PCI Firmware Specification, r3.3 sec 4.7
> + *
> + * Two backends are supported to manipulate indications:  Direct NPEM register
> + * access (npem_ops) and indirect access through the ACPI _DSM (dsm_ops).
> + * _DSM is used if supported, else NPEM.
> + *
> + * Copyright (c) 2021-2022 Dell Inc.
>   * Copyright (c) 2023-2024 Intel Corporation
>   *	Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
>   */
> @@ -55,6 +63,21 @@ static const struct indication npem_indications[] = {
>  	{0,			NULL}
>  };
>  
> +/* _DSM PCIe SSD LED States are corresponding to NPEM register values */
> +static const struct indication dsm_indications[] = {
> +	{PCI_NPEM_IND_OK,	"enclosure:ok"},
> +	{PCI_NPEM_IND_LOCATE,	"enclosure:locate"},
> +	{PCI_NPEM_IND_FAIL,	"enclosure:fail"},
> +	{PCI_NPEM_IND_REBUILD,	"enclosure:rebuild"},
> +	{PCI_NPEM_IND_PFA,	"enclosure:pfa"},
> +	{PCI_NPEM_IND_HOTSPARE,	"enclosure:hotspare"},
> +	{PCI_NPEM_IND_ICA,	"enclosure:ica"},
> +	{PCI_NPEM_IND_IFA,	"enclosure:ifa"},
> +	{PCI_NPEM_IND_IDT,	"enclosure:idt"},
> +	{PCI_NPEM_IND_DISABLED,	"enclosure:disabled"},
> +	{0,			NULL}
> +};
> +
>  #define for_each_indication(ind, inds) \
>  	for (ind = inds; ind->bit; ind++)
>  
> @@ -250,6 +273,120 @@ static bool npem_has_dsm(struct pci_dev *pdev)
>  			      BIT(GET_STATE_DSM) | BIT(SET_STATE_DSM));
>  }
>  
> +struct dsm_output {
> +	u16 status;
> +	u8 function_specific_err;
> +	u8 vendor_specific_err;
> +	u32 state;
> +} __packed;

As mentioned by Christoph, __packed is not required here due to natural 
alignment (Using __packed will cause other effects beyond just preventing 
compiler from adding padding which is why it's good to avoid it where 
it's not needed).

With that removed,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.