Re: [PATCH] PCI: hotplug: Add extension driver for Ampere Altra hotplug LED control

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Sep 28, 2023 at 02:02:07PM -0700, D Scott Phillips wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> > On Wed, Sep 27, 2023 at 01:23:47PM -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.

> >> +config HOTPLUG_PCI_ACPI_AMPERE_ALTRA
> >> +	tristate "ACPI PCI Hotplug driver Ampere Altra extensions"
> >> +	depends on HOTPLUG_PCI_ACPI
> >> +	depends on HAVE_ARM_SMCCC_DISCOVERY
> >> +	depends on m
> >
> > Why is this restricted to being a module?  It's not unprecedented, but
> > unless this only works as a module for some reason, I would leave that
> > choice up to the user.
> 
> I did that because acpiphp_register_attention() wouldn't register the
> handler unless it was built as a module. Maybe better would be this
> change first:
> 
> --- a/drivers/pci/hotplug/acpiphp_core.c
> +++ b/drivers/pci/hotplug/acpiphp_core.c
> @@ -78,8 +78,7 @@ int acpiphp_register_attention(struct acpiphp_attention_info *info)
>  {
>  	int retval = -EINVAL;
>  
> -	if (info && info->owner && info->set_attn &&
> -			info->get_attn && !attention_info) {
> +	if (info && info->set_attn && info->get_attn && !attention_info) {
>  		retval = 0;
>  		attention_info = info;
>  	}

I would investigate the history of that "info->owner" check to see if
it's required somewhere.  If not, it seems like we could drop it.

At one time, we did support building hotplug drivers, including
acpiphp, as modules, but we no longer do.  Maybe that test dates from
that time.

Bjorn