| Message ID | 20240705125436.26057-4-mariusz.tkaczyk@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCIe Enclosure LED Management | expand |
> +struct dsm_output { > + u16 status; > + u8 function_specific_err; > + u8 vendor_specific_err; > + u32 state; > +} __packed; This structure is naturally aligned, so no need for the __packed. Otherwise looks good (or at least as good as something can look while interacting with the weirdo ACPI interfaces..): Reviewed-by: Christoph Hellwig <hch@lst.de>
On Fri, Jul 05, 2024 at 06:04:39AM -0700, Christoph Hellwig wrote: > > +struct dsm_output { > > + u16 status; > > + u8 function_specific_err; > > + u8 vendor_specific_err; > > + u32 state; > > +} __packed; > > This structure is naturally aligned, so no need for the __packed. Isn't the compiler free to insert padding wherever it sees fit? structs passed to ACPI firmware would no longer comply with the spec-prescribed layout then and declaring them __packed seems to be the only way to ensure that doesn't happen. Thanks, Lukas
On Fri, Jul 05, 2024 at 04:07:59PM +0200, Lukas Wunner wrote: > On Fri, Jul 05, 2024 at 06:04:39AM -0700, Christoph Hellwig wrote: > > > +struct dsm_output { > > > + u16 status; > > > + u8 function_specific_err; > > > + u8 vendor_specific_err; > > > + u32 state; > > > +} __packed; > > > > This structure is naturally aligned, so no need for the __packed. > > Isn't the compiler free to insert padding wherever it sees fit? In terms of the standard: yes. It could even reorder members. In terms of not breaking the Linux kernel (or other low-level software) left, right and center: no. > structs passed to ACPI firmware would no longer comply with the > spec-prescribed layout then and declaring them __packed seems to be > the only way to ensure that doesn't happen. Take a look at just about any driver, file system or network protocol.
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>
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; + +/** + * dsm_evaluate() - send DSM PCIe SSD Status LED command + * @pdev: PCI device + * @dsm_func: DSM LED Function + * @output: buffer to copy DSM Response + * @value_to_set: value for SET_STATE_DSM function + * + * To not bother caller with ACPI context, the returned _DSM Output Buffer is + * copied. + */ +static int dsm_evaluate(struct pci_dev *pdev, u64 dsm_func, + struct dsm_output *output, u32 value_to_set) +{ + acpi_handle handle = ACPI_HANDLE(&pdev->dev); + union acpi_object *out_obj, arg3[2]; + union acpi_object *arg3_p = NULL; + + if (dsm_func == SET_STATE_DSM) { + arg3[0].type = ACPI_TYPE_PACKAGE; + arg3[0].package.count = 1; + arg3[0].package.elements = &arg3[1]; + + arg3[1].type = ACPI_TYPE_BUFFER; + arg3[1].buffer.length = 4; + arg3[1].buffer.pointer = (u8 *)&value_to_set; + + arg3_p = arg3; + } + + out_obj = acpi_evaluate_dsm_typed(handle, &dsm_guid, 0x1, dsm_func, + arg3_p, ACPI_TYPE_BUFFER); + if (!out_obj) + return -EIO; + + if (out_obj->buffer.length < sizeof(struct dsm_output)) { + ACPI_FREE(out_obj); + return -EIO; + } + + memcpy(output, out_obj->buffer.pointer, sizeof(struct dsm_output)); + + ACPI_FREE(out_obj); + return 0; +} + +static int dsm_get(struct pci_dev *pdev, u64 dsm_func, u32 *buf) +{ + struct dsm_output output; + int ret = dsm_evaluate(pdev, dsm_func, &output, 0); + + if (ret) + return ret; + + if (output.status != 0) + return -EIO; + + *buf = output.state; + return 0; +} + +static int dsm_get_active_indications(struct npem *npem, u32 *buf) +{ + int ret = dsm_get(npem->dev, GET_STATE_DSM, buf); + + /* Filter out not supported indications in response */ + *buf &= npem->supported_indications; + return ret; +} + +static int dsm_set_active_indications(struct npem *npem, u32 value) +{ + struct dsm_output output; + int ret = dsm_evaluate(npem->dev, SET_STATE_DSM, &output, value); + + if (ret) + return ret; + + switch (output.status) { + case 4: + /* + * Not all bits are set. If this bit is set, the platform + * disregarded some or all of the request state changes. OSPM + * should check the resulting PCIe SSD Status LED States to see + * what, if anything, has changed. + * + * PCI Firmware Specification, r3.3 Table 4-19. + */ + if (output.function_specific_err != 1) + return -EIO; + fallthrough; + case 0: + break; + default: + return -EIO; + } + + npem->active_indications = output.state; + + return 0; +} + +static const struct npem_ops dsm_ops = { + .inds = dsm_indications, + .get_active_indications = dsm_get_active_indications, + .set_active_indications = dsm_set_active_indications, +}; + static int npem_initialize_active_indications(struct npem *npem) { int ret; @@ -437,11 +574,15 @@ void pci_npem_create(struct pci_dev *dev) * OS should use the DSM for LED control if it is available * PCI Firmware Spec r3.3 sec 4.7. */ - return; + ret = dsm_get(dev, GET_SUPPORTED_STATES_DSM, &cap); + if (ret) + return; + + ops = &dsm_ops; } ret = pci_npem_init(dev, ops, pos, cap); if (ret) - pci_err(dev, "Failed to register PCIe Enclosure Management driver, err: %d\n", - ret); + pci_err(dev, "Failed to register %s PCIe Enclosure Management driver, err: %d\n", + (ops == &dsm_ops ? "_DSM" : "Native"), ret); }
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(-)