* Re: [PATCH v2 2/2] PCI: hotplug: Add extension driver for Ampere Altra hotplug LED control [not found] ` <20231025180627.GA1756475@bhelgaas> @ 2023-10-26 10:47 ` Anders Roxell 2023-10-26 11:04 ` Bjorn Helgaas 0 siblings, 1 reply; 2+ messages in thread From: Anders Roxell @ 2023-10-26 10:47 UTC (permalink / raw) To: Bjorn Helgaas Cc: D Scott Phillips, linux-acpi, linux-pci, Rafael J. Wysocki, linux-kernel, Len Brown, Darren Hart, patches, sfr, linux-next On 2023-10-25 13:06, Bjorn Helgaas wrote: > On Wed, Oct 25, 2023 at 10:41:46AM -0700, D Scott Phillips wrote: > > Bjorn Helgaas <helgaas@kernel.org> writes: > > > > > On Fri, Sep 29, 2023 at 05:20:36PM -0700, D Scott Phillips wrote: > > >> On Ampere Altra, PCIe hotplug is handled through ACPI. A side interface is > > >> also present to request system firmware control of attention LEDs. Add an > > >> ACPI PCI Hotplug companion driver to support attention LED control. > > >> ... > > > > > >> + arm_smccc_smc(REQUEST, LED_CMD, led_status(status), LED_ATTENTION, > > >> + pci_domain_nr(bus) | (PCI_SLOT(root_port->devfn) << 4), 0, 0, > > > > > > pci_domain_nr() returns "int" (normally 32 bits), but since this is an > > > ACPI system, the domain comes from _SEG, which is defined to be 16 > > > bits (ACPI r6.5, sec 6.5.6). > > > > > > So it looks like ORing in the "slot << 4" clobbers the upper 12 bits > > > of _SEG. > > > > > > Is this code doing the right thing? > > > > Hi Bjorn, > > > > on these Altra platforms _SEG is limited within 0-11. I can add an `& > > 0xf` to pci_domain_nr() to make it clear that the segment number is > > encoded down into 4 bits in the smc request. > > If the following looks OK, we're all set. I put these on pci/hotplug > for v6.7, thanks! > > +static int set_attention_status(struct hotplug_slot *slot, u8 status) > +{ > + struct arm_smccc_res res; > + struct pci_bus *bus; > + struct pci_dev *root_port; > + unsigned long flags; > + u32 handle; > + int ret = 0; > + > + bus = slot->pci_slot->bus; > + root_port = pcie_find_root_port(bus->self); > + if (!root_port) > + return -ENODEV; > + > + local_irq_save(flags); > + arm_smccc_smc(HANDLE_OPEN, led_service_id[0], led_service_id[1], > + led_service_id[2], led_service_id[3], 0, 0, 0, &res); > + if (res.a0) { > + ret = -ENODEV; > + goto out; > + } > + handle = res.a1 & 0xffff0000; > + > + arm_smccc_smc(REQUEST, LED_CMD, led_status(status), LED_ATTENTION, > + PCI_SLOT(root_port->devfn) << 4 | pci_domain_nr(bus) & 0xf, > + 0, 0, handle, &res); > + if (res.a0) > + ret = -ENODEV; > + > + arm_smccc_smc(HANDLE_CLOSE, handle, 0, 0, 0, 0, 0, 0, &res); > + > + out: > + local_irq_restore(flags); > + return ret; > +} Hi, Building todays linux-next tag next-20231026 for arm64 with CONFIG_HOTPLUG_PCI_ACPI_AMPERE_ALTRA=m I see the following build error. drivers/pci/hotplug/acpiphp_ampere_altra.c: In function 'set_attention_status': drivers/pci/hotplug/acpiphp_ampere_altra.c:63:75: error: suggest parentheses around arithmetic in operand of '|' [-Werror=parentheses] 63 | PCI_SLOT(root_port->devfn) << 4 | pci_domain_nr(bus) & 0xf, | ~~~~~~~~~~~~~~~~~~~^~~~~ include/linux/arm-smccc.h:382:44: note: in definition of macro 'arm_smccc_smc' 382 | #define arm_smccc_smc(...) __arm_smccc_smc(__VA_ARGS__, NULL) | ^~~~~~~~~~~ cc1: all warnings being treated as errors make[6]: *** [scripts/Makefile.build:243: drivers/pci/hotplug/acpiphp_ampere_altra.o] Error 1 Looks like this is the problematic patch 13ba8a09c4f6 ("PCI: hotplug: Add Ampere Altra Attention Indicator extension driver") Cheers, Anders ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2 2/2] PCI: hotplug: Add extension driver for Ampere Altra hotplug LED control 2023-10-26 10:47 ` [PATCH v2 2/2] PCI: hotplug: Add extension driver for Ampere Altra hotplug LED control Anders Roxell @ 2023-10-26 11:04 ` Bjorn Helgaas 0 siblings, 0 replies; 2+ messages in thread From: Bjorn Helgaas @ 2023-10-26 11:04 UTC (permalink / raw) To: Anders Roxell Cc: D Scott Phillips, linux-acpi, linux-pci, Rafael J. Wysocki, linux-kernel, Len Brown, Darren Hart, patches, sfr, linux-next On Thu, Oct 26, 2023 at 12:47:31PM +0200, Anders Roxell wrote: > On 2023-10-25 13:06, Bjorn Helgaas wrote: > > On Wed, Oct 25, 2023 at 10:41:46AM -0700, D Scott Phillips wrote: > > > Bjorn Helgaas <helgaas@kernel.org> writes: > > > > > > > On Fri, Sep 29, 2023 at 05:20:36PM -0700, D Scott Phillips wrote: > > > >> On Ampere Altra, PCIe hotplug is handled through ACPI. A side interface is > > > >> also present to request system firmware control of attention LEDs. Add an > > > >> ACPI PCI Hotplug companion driver to support attention LED control. > > > >> ... > > > > > > > >> + arm_smccc_smc(REQUEST, LED_CMD, led_status(status), LED_ATTENTION, > > > >> + pci_domain_nr(bus) | (PCI_SLOT(root_port->devfn) << 4), 0, 0, > > > > > > > > pci_domain_nr() returns "int" (normally 32 bits), but since this is an > > > > ACPI system, the domain comes from _SEG, which is defined to be 16 > > > > bits (ACPI r6.5, sec 6.5.6). > > > > > > > > So it looks like ORing in the "slot << 4" clobbers the upper 12 bits > > > > of _SEG. > > > > > > > > Is this code doing the right thing? > > > > > > Hi Bjorn, > > > > > > on these Altra platforms _SEG is limited within 0-11. I can add an `& > > > 0xf` to pci_domain_nr() to make it clear that the segment number is > > > encoded down into 4 bits in the smc request. > > > > If the following looks OK, we're all set. I put these on pci/hotplug > > for v6.7, thanks! > > > > +static int set_attention_status(struct hotplug_slot *slot, u8 status) > > +{ > > + struct arm_smccc_res res; > > + struct pci_bus *bus; > > + struct pci_dev *root_port; > > + unsigned long flags; > > + u32 handle; > > + int ret = 0; > > + > > + bus = slot->pci_slot->bus; > > + root_port = pcie_find_root_port(bus->self); > > + if (!root_port) > > + return -ENODEV; > > + > > + local_irq_save(flags); > > + arm_smccc_smc(HANDLE_OPEN, led_service_id[0], led_service_id[1], > > + led_service_id[2], led_service_id[3], 0, 0, 0, &res); > > + if (res.a0) { > > + ret = -ENODEV; > > + goto out; > > + } > > + handle = res.a1 & 0xffff0000; > > + > > + arm_smccc_smc(REQUEST, LED_CMD, led_status(status), LED_ATTENTION, > > + PCI_SLOT(root_port->devfn) << 4 | pci_domain_nr(bus) & 0xf, > > + 0, 0, handle, &res); > > + if (res.a0) > > + ret = -ENODEV; > > + > > + arm_smccc_smc(HANDLE_CLOSE, handle, 0, 0, 0, 0, 0, 0, &res); > > + > > + out: > > + local_irq_restore(flags); > > + return ret; > > +} > > Hi, > > Building todays linux-next tag next-20231026 for arm64 with > CONFIG_HOTPLUG_PCI_ACPI_AMPERE_ALTRA=m I see the following build error. > > drivers/pci/hotplug/acpiphp_ampere_altra.c: In function 'set_attention_status': > drivers/pci/hotplug/acpiphp_ampere_altra.c:63:75: error: suggest parentheses around arithmetic in operand of '|' [-Werror=parentheses] > 63 | PCI_SLOT(root_port->devfn) << 4 | pci_domain_nr(bus) & 0xf, > | ~~~~~~~~~~~~~~~~~~~^~~~~ > include/linux/arm-smccc.h:382:44: note: in definition of macro 'arm_smccc_smc' > 382 | #define arm_smccc_smc(...) __arm_smccc_smc(__VA_ARGS__, NULL) > | ^~~~~~~~~~~ > cc1: all warnings being treated as errors > make[6]: *** [scripts/Makefile.build:243: drivers/pci/hotplug/acpiphp_ampere_altra.o] Error 1 My fault, fixed. Bjorn ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-10-26 11:04 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <86ttqe4n1h.fsf@scott-ph-mail.amperecomputing.com>
[not found] ` <20231025180627.GA1756475@bhelgaas>
2023-10-26 10:47 ` [PATCH v2 2/2] PCI: hotplug: Add extension driver for Ampere Altra hotplug LED control Anders Roxell
2023-10-26 11:04 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).