diff mbox series

[v7,1/3] leds: Init leds class earlier

Message ID 20240904104848.23480-2-mariusz.tkaczyk@linux.intel.com (mailing list archive)
State Accepted
Commit 78efa53e715e21a97c722dba20f8437a0860521e
Headers show
Series PCIe Enclosure LED Management | expand

Commit Message

Mariusz Tkaczyk Sept. 4, 2024, 10:48 a.m. UTC
NPEM driver will require leds class, there is an init-order conflict.
Make sure that LEDs initialization happens first and add comment.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 drivers/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Lee Jones Sept. 9, 2024, 9:03 a.m. UTC | #1
On Wed, 04 Sep 2024, Mariusz Tkaczyk wrote:

> NPEM driver will require leds class, there is an init-order conflict.
> Make sure that LEDs initialization happens first and add comment.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>

Do you have a link to this discussion?

> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>  drivers/Makefile | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index fe9ceb0d2288..45d1c3e630f7 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -17,6 +17,9 @@ obj-$(CONFIG_PINCTRL)		+= pinctrl/
>  obj-$(CONFIG_GPIOLIB)		+= gpio/
>  obj-y				+= pwm/
>  
> +# LEDs must come before PCI, it is needed by NPEM driver

This seems very fragile.

Isn't there a better way to describe the dependency in Kconfig etc?

> +obj-y				+= leds/
> +
>  obj-y				+= pci/
>  
>  obj-$(CONFIG_PARISC)		+= parisc/
> @@ -130,7 +133,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/
> -- 
> 2.35.3
>
Mariusz Tkaczyk Sept. 9, 2024, 9:28 a.m. UTC | #2
On Mon, 9 Sep 2024 10:03:40 +0100
Lee Jones <lee@kernel.org> wrote:

> On Wed, 04 Sep 2024, Mariusz Tkaczyk wrote:
> 
> > NPEM driver will require leds class, there is an init-order conflict.
> > Make sure that LEDs initialization happens first and add comment.
> > 
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>  
> 
> Do you have a link to this discussion?

https://lore.kernel.org/linux-pci/2024061429-handiness-waggle-d87a@gregkh/

I discussed this with Lukas and Dan offline and as a result I updated Makefile
instead initcall change.

> 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> > ---
> >  drivers/Makefile | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index fe9ceb0d2288..45d1c3e630f7 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -17,6 +17,9 @@ obj-$(CONFIG_PINCTRL)		+= pinctrl/
> >  obj-$(CONFIG_GPIOLIB)		+= gpio/
> >  obj-y				+= pwm/
> >  
> > +# LEDs must come before PCI, it is needed by NPEM driver  
> 
> This seems very fragile.
> 
> Isn't there a better way to describe the dependency in Kconfig etc?

I don't know if it is better. What I know is (according to Greg) that
maintainers are aware of possible ordering issues and not described dependencies
so they are not likely to allow changing it.

Mariusz
Lukas Wunner Sept. 9, 2024, 8:21 p.m. UTC | #3
On Mon, Sep 09, 2024 at 10:03:40AM +0100, Lee Jones wrote:
> On Wed, 04 Sep 2024, Mariusz Tkaczyk wrote:
> > NPEM driver will require leds class, there is an init-order conflict.
> > Make sure that LEDs initialization happens first and add comment.
[...]
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -17,6 +17,9 @@ obj-$(CONFIG_PINCTRL)		+= pinctrl/
> >  obj-$(CONFIG_GPIOLIB)		+= gpio/
> >  obj-y				+= pwm/
> >  
> > +# LEDs must come before PCI, it is needed by NPEM driver
> > +obj-y				+= leds/
> > +
> >  obj-y				+= pci/
> >  
> >  obj-$(CONFIG_PARISC)		+= parisc/
> > @@ -130,7 +133,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/
> 
> This seems very fragile.
> 
> Isn't there a better way to describe the dependency in Kconfig etc?

In v2 of this series, the correct init order was ensured by
running leds_init() at "arch" initcall level, instead of "subsys":

https://lore.kernel.org/linux-pci/20240528131940.16924-2-mariusz.tkaczyk@linux.intel.com/

If you prefer this alternative approach, then Bjorn or Krzysztof
can probably replace the commit on the pci.git/npem branch.

AFAIK, all topic branches in pci.git are to be considered drafts
that can be amended until the pull request is sent.

Thanks,

Lukas
diff mbox series

Patch

diff --git a/drivers/Makefile b/drivers/Makefile
index fe9ceb0d2288..45d1c3e630f7 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -17,6 +17,9 @@  obj-$(CONFIG_PINCTRL)		+= pinctrl/
 obj-$(CONFIG_GPIOLIB)		+= gpio/
 obj-y				+= pwm/
 
+# LEDs must come before PCI, it is needed by NPEM driver
+obj-y				+= leds/
+
 obj-y				+= pci/
 
 obj-$(CONFIG_PARISC)		+= parisc/
@@ -130,7 +133,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/