| Message ID | 20240528131940.16924-2-mariusz.tkaczyk@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCIe Enclosure LED Management | expand |
Mariusz Tkaczyk wrote: > PCI subsystem is registered on subsys_initcall() same as leds class. > NPEM driver will require leds class, there is race possibility. s/race possibility/an init-order conflict/ > Register led class on arch_initcall() to avoid it. I assume a later patch selects CONFIG_LEDS_CLASS from CONFIG_PCI? Otherwise this change will not help. Another way to solve this without changing initcall levels is to just make sure that the leds subsys initcall happens first, i.e.: diff --git a/drivers/Makefile b/drivers/Makefile index fe9ceb0d2288..d47b4186108a 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_GENERIC_PHY) += phy/ obj-$(CONFIG_PINCTRL) += pinctrl/ obj-$(CONFIG_GPIOLIB) += gpio/ obj-y += pwm/ +obj-y += leds/ obj-y += pci/ @@ -130,7 +131,6 @@ obj-$(CONFIG_CPU_IDLE) += cpuidle/ obj-y += mmc/ obj-y += ufs/ obj-$(CONFIG_MEMSTICK) += memstick/ -obj-y += leds/ obj-$(CONFIG_INFINIBAND) += infiniband/ obj-y += firmware/ obj-$(CONFIG_CRYPTO) += crypto/ ...which may be more appropriate.
On Tue, May 28, 2024 at 09:22:22PM -0700, Dan Williams wrote: > Mariusz Tkaczyk wrote: > > PCI subsystem is registered on subsys_initcall() same as leds class. > > NPEM driver will require leds class, there is race possibility. > > Register led class on arch_initcall() to avoid it. > > Another way to solve this without changing initcall levels is to just > make sure that the leds subsys initcall happens first, i.e.: > > diff --git a/drivers/Makefile b/drivers/Makefile > index fe9ceb0d2288..d47b4186108a 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_GENERIC_PHY) += phy/ > obj-$(CONFIG_PINCTRL) += pinctrl/ > obj-$(CONFIG_GPIOLIB) += gpio/ > obj-y += pwm/ > +obj-y += leds/ > > obj-y += pci/ > > @@ -130,7 +131,6 @@ obj-$(CONFIG_CPU_IDLE) += cpuidle/ > obj-y += mmc/ > obj-y += ufs/ > obj-$(CONFIG_MEMSTICK) += memstick/ > -obj-y += leds/ > obj-$(CONFIG_INFINIBAND) += infiniband/ > obj-y += firmware/ > obj-$(CONFIG_CRYPTO) += crypto/ To me, this seems more fragile than changing the initcall level. The risk I see is that someone comes along and rearranges the Makefile in, say, alphabetic order because "why not?" and unwittingly breaks NPEM. Changing initcall levels at least explicitly sets the order in stone. However, perhaps a code comment is helpful to tell the casual code reader why this particular initcall level is needed. Something like... /* LEDs may already be registered in subsys initcall level */ ... may already be sufficient. Thanks, Lukas
On Fri, Jun 14, 2024 at 04:08:48PM +0200, Lukas Wunner wrote: > On Tue, May 28, 2024 at 09:22:22PM -0700, Dan Williams wrote: > > Mariusz Tkaczyk wrote: > > > PCI subsystem is registered on subsys_initcall() same as leds class. > > > NPEM driver will require leds class, there is race possibility. > > > Register led class on arch_initcall() to avoid it. > > > > Another way to solve this without changing initcall levels is to just > > make sure that the leds subsys initcall happens first, i.e.: > > > > diff --git a/drivers/Makefile b/drivers/Makefile > > index fe9ceb0d2288..d47b4186108a 100644 > > --- a/drivers/Makefile > > +++ b/drivers/Makefile > > @@ -16,6 +16,7 @@ obj-$(CONFIG_GENERIC_PHY) += phy/ > > obj-$(CONFIG_PINCTRL) += pinctrl/ > > obj-$(CONFIG_GPIOLIB) += gpio/ > > obj-y += pwm/ > > +obj-y += leds/ > > > > obj-y += pci/ > > > > @@ -130,7 +131,6 @@ obj-$(CONFIG_CPU_IDLE) += cpuidle/ > > obj-y += mmc/ > > obj-y += ufs/ > > obj-$(CONFIG_MEMSTICK) += memstick/ > > -obj-y += leds/ > > obj-$(CONFIG_INFINIBAND) += infiniband/ > > obj-y += firmware/ > > obj-$(CONFIG_CRYPTO) += crypto/ > > To me, this seems more fragile than changing the initcall level. > > The risk I see is that someone comes along and rearranges the Makefile in, > say, alphabetic order because "why not?" and unwittingly breaks NPEM. If they do that, then we have worse problems as many of us "know" that the order here matters a LOT. > Changing initcall levels at least explicitly sets the order in stone. For a while, until they change :) > However, perhaps a code comment is helpful to tell the casual > code reader why this particular initcall level is needed. > > Something like... > > /* LEDs may already be registered in subsys initcall level */ That would be good to have. thanks, greg k-h
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 24fcff682b24..bed7b6ea9b98 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -681,7 +681,7 @@ static void __exit leds_exit(void) class_unregister(&leds_class); } -subsys_initcall(leds_init); +arch_initcall(leds_init); module_exit(leds_exit); MODULE_AUTHOR("John Lenz, Richard Purdie");
PCI subsystem is registered on subsys_initcall() same as leds class. NPEM driver will require leds class, there is race possibility. Register led class on arch_initcall() to avoid it. Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> --- drivers/leds/led-class.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)