Re: [PATCH v2] PCI: Introduce Cray ClusterStor E1000 NVMe slot LED driver

[Lukas Wunner <lukas@wunner.de>]

On Thu, Sep 26, 2024 at 04:02:59PM -0500, Bjorn Helgaas wrote:
> On Mon, Sep 23, 2024 at 05:06:05PM -0700, Tony Hutter wrote:
> > +++ b/drivers/pci/hotplug/pciehp_core.c
> > @@ -73,6 +73,13 @@ static int init_slot(struct controller *ctrl)
> >  		ops->get_attention_status = pciehp_get_raw_indicator_status;
> >  		ops->set_attention_status = pciehp_set_raw_indicator_status;
> >  	}
> > +#ifdef CONFIG_HOTPLUG_PCI_PCIE_CRAY_E1000
> > +	if (is_craye1k_slot(ctrl)) {
> > +		/* slots 1-24 on Cray E1000s are controlled differently */
> > +		ops->get_attention_status = craye1k_get_attention_status;
> > +		ops->set_attention_status = craye1k_set_attention_status;
> > +	}
> > +#endif
> 
> I'm not really thrilled about dropping device-specific code in here,
> but I don't have a better suggestion yet.

For acpiphp, we have an elaborate mechanism to register attention LED
drivers via acpiphp_register_attention() / acpiphp_unregister_attention().

So far it's only used by two drivers (acpiphp_ampere_altra.c and
acpiphp_ibm.c).

For pciehp we have one custom attention LED mechanism for VMD
(enabled through pci_dev->hotplug_user_indicators) and now
this Cray E1000 mechanism.

Personally I'm fine with keeping this lightweight and not add
a similar register/unregister mechanism as we did for acpiphp.

However I think it might make sense to drop the hotplug_user_indicators
flag and instead invoke the is_vmd() check directly from init_slot()
to make it more explicit what this is about and to allow the code to be
optimized away on non-x86.

Thanks,

Lukas