diff mbox series

[v2,04/11] gpiolib: Clear the gpio_device's fwnode initialized flag before adding

Message ID 20230127001141.407071-5-saravanak@google.com
State Superseded
Headers show
Series fw_devlink improvements | expand

Commit Message

Saravana Kannan Jan. 27, 2023, 12:11 a.m. UTC
Registering an irqdomain sets the flag for the fwnode. But having the
flag set when a device is added is interpreted by fw_devlink to mean the
device has already been initialized and will never probe. This prevents
fw_devlink from creating device links with the gpio_device as a
supplier. So, clear the flag before adding the device.

Signed-off-by: Saravana Kannan <saravanak@google.com>
Acked-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpiolib.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Saravana Kannan Jan. 28, 2023, 7:33 a.m. UTC | #1
On Fri, Jan 27, 2023 at 1:27 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jan 26, 2023 at 04:11:31PM -0800, Saravana Kannan wrote:
> > Registering an irqdomain sets the flag for the fwnode. But having the
> > flag set when a device is added is interpreted by fw_devlink to mean the
> > device has already been initialized and will never probe. This prevents
> > fw_devlink from creating device links with the gpio_device as a
> > supplier. So, clear the flag before adding the device.
>
> ...
>
> > +     /*
> > +      * If fwnode doesn't belong to another device, it's safe to clear its
> > +      * initialized flag.
> > +      */
> > +     if (!gdev->dev.fwnode->dev)
> > +             fwnode_dev_initialized(gdev->dev.fwnode, false);
>
> Do not dereference fwnode in struct device. Use dev_fwnode() for that.
>
>         struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
>
>         if (!fwnode->dev)
>                 fwnode_dev_initialized(fwnode, false);

Honestly, we should work towards NOT needing dev_fwnode(). The
function literally dereferences dev->fwnode or the one inside of_node.
So my dereference is fine. The whole "fwnode might not be set for
devices with of_node" is wrong and we should fix that instead of
writing wrappers to work around it.

Also, for now I'm going to leave this as if for the same reasons as I
mentioned in Patch 1.

>
> + Blank line.

Ack.


-Saravana

>
> >       ret = gcdev_register(gdev, gpio_devt);
> >       if (ret)
> >               return ret;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
Andy Shevchenko Jan. 30, 2023, 12:05 p.m. UTC | #2
On Fri, Jan 27, 2023 at 11:33:38PM -0800, Saravana Kannan wrote:
> On Fri, Jan 27, 2023 at 1:27 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Jan 26, 2023 at 04:11:31PM -0800, Saravana Kannan wrote:

...

> > > +     /*
> > > +      * If fwnode doesn't belong to another device, it's safe to clear its
> > > +      * initialized flag.
> > > +      */
> > > +     if (!gdev->dev.fwnode->dev)
> > > +             fwnode_dev_initialized(gdev->dev.fwnode, false);
> >
> > Do not dereference fwnode in struct device. Use dev_fwnode() for that.
> >
> >         struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
> >
> >         if (!fwnode->dev)
> >                 fwnode_dev_initialized(fwnode, false);
> 
> Honestly, we should work towards NOT needing dev_fwnode().

Honestly, it's that We SHOULD go to avoid any direct dereference of fwnode from
the struct device. I explained you in the comment in the other patch.

> The
> function literally dereferences dev->fwnode or the one inside of_node.
> So my dereference is fine. The whole "fwnode might not be set for
> devices with of_node" is wrong and we should fix that instead of
> writing wrappers to work around it.
> 
> Also, for now I'm going to leave this as if for the same reasons as I
> mentioned in Patch 1.

Same.
Sudeep Holla Jan. 30, 2023, 2:31 p.m. UTC | #3
On Thu, Jan 26, 2023 at 04:11:31PM -0800, Saravana Kannan wrote:
> Registering an irqdomain sets the flag for the fwnode. But having the
> flag set when a device is added is interpreted by fw_devlink to mean the
> device has already been initialized and will never probe. This prevents
> fw_devlink from creating device links with the gpio_device as a
> supplier. So, clear the flag before adding the device.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> Acked-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  drivers/gpio/gpiolib.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 939c776b9488..b23140c6485f 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -578,6 +578,12 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
>  {
>  	int ret;
>  
> +	/*
> +	 * If fwnode doesn't belong to another device, it's safe to clear its
> +	 * initialized flag.
> +	 */
> +	if (!gdev->dev.fwnode->dev)
> +		fwnode_dev_initialized(gdev->dev.fwnode, false);

This is the one causing the kernel crash during the boot on FVP which
Naresh has reported. Just reverted this and was able to boot, confirming
the issue with this patch.
Andy Shevchenko Jan. 30, 2023, 3:14 p.m. UTC | #4
On Mon, Jan 30, 2023 at 02:31:53PM +0000, Sudeep Holla wrote:
> On Thu, Jan 26, 2023 at 04:11:31PM -0800, Saravana Kannan wrote:
> > Registering an irqdomain sets the flag for the fwnode. But having the
> > flag set when a device is added is interpreted by fw_devlink to mean the
> > device has already been initialized and will never probe. This prevents
> > fw_devlink from creating device links with the gpio_device as a
> > supplier. So, clear the flag before adding the device.

...

> > +	/*
> > +	 * If fwnode doesn't belong to another device, it's safe to clear its
> > +	 * initialized flag.
> > +	 */
> > +	if (!gdev->dev.fwnode->dev)
> > +		fwnode_dev_initialized(gdev->dev.fwnode, false);
> 
> This is the one causing the kernel crash during the boot on FVP which
> Naresh has reported. Just reverted this and was able to boot, confirming
> the issue with this patch.

I'm wondering if

	if (!dev_fwnode(&gdev->dev)->dev)
		fwnode_dev_initialized(&dev_fwnode(gdev->dev), false);

works.
Saravana Kannan Jan. 31, 2023, 4:01 a.m. UTC | #5
On Mon, Jan 30, 2023 at 7:14 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Jan 30, 2023 at 02:31:53PM +0000, Sudeep Holla wrote:
> > On Thu, Jan 26, 2023 at 04:11:31PM -0800, Saravana Kannan wrote:
> > > Registering an irqdomain sets the flag for the fwnode. But having the
> > > flag set when a device is added is interpreted by fw_devlink to mean the
> > > device has already been initialized and will never probe. This prevents
> > > fw_devlink from creating device links with the gpio_device as a
> > > supplier. So, clear the flag before adding the device.
>
> ...
>
> > > +   /*
> > > +    * If fwnode doesn't belong to another device, it's safe to clear its
> > > +    * initialized flag.
> > > +    */
> > > +   if (!gdev->dev.fwnode->dev)
> > > +           fwnode_dev_initialized(gdev->dev.fwnode, false);
> >
> > This is the one causing the kernel crash during the boot on FVP which
> > Naresh has reported. Just reverted this and was able to boot, confirming
> > the issue with this patch.
>
> I'm wondering if
>
>         if (!dev_fwnode(&gdev->dev)->dev)
>                 fwnode_dev_initialized(&dev_fwnode(gdev->dev), false);
>
> works.

No, that won't help. The problem was that with arm32, we have gpio
devices created without any of_node or fwnode. So I can't assume
fwnode will always be present.

-Saravana
Sudeep Holla Jan. 31, 2023, 10:13 a.m. UTC | #6
On Mon, Jan 30, 2023 at 08:01:17PM -0800, Saravana Kannan wrote:
> On Mon, Jan 30, 2023 at 7:14 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Jan 30, 2023 at 02:31:53PM +0000, Sudeep Holla wrote:
> > > On Thu, Jan 26, 2023 at 04:11:31PM -0800, Saravana Kannan wrote:
> > > > Registering an irqdomain sets the flag for the fwnode. But having the
> > > > flag set when a device is added is interpreted by fw_devlink to mean the
> > > > device has already been initialized and will never probe. This prevents
> > > > fw_devlink from creating device links with the gpio_device as a
> > > > supplier. So, clear the flag before adding the device.
> >
> > ...
> >
> > > > +   /*
> > > > +    * If fwnode doesn't belong to another device, it's safe to clear its
> > > > +    * initialized flag.
> > > > +    */
> > > > +   if (!gdev->dev.fwnode->dev)
> > > > +           fwnode_dev_initialized(gdev->dev.fwnode, false);
> > >
> > > This is the one causing the kernel crash during the boot on FVP which
> > > Naresh has reported. Just reverted this and was able to boot, confirming
> > > the issue with this patch.
> >
> > I'm wondering if
> >
> >         if (!dev_fwnode(&gdev->dev)->dev)
> >                 fwnode_dev_initialized(&dev_fwnode(gdev->dev), false);
> >
> > works.
> 
> No, that won't help. The problem was that with arm32, we have gpio
> devices created without any of_node or fwnode. So I can't assume
> fwnode will always be present.
>

Correct, and this one is not even arm32. But it is just reusing a driver
that needs to be supported even on arm32.

Not sure on how to proceed. As a simple way to check, I added a NULL check
for fwnode building on top of Andy's suggestion[1]. That works.

Also the driver in question on arm64 FVP model is drivers/mfd/vexpress-sysreg.c
mfd_add_device() in drivers/mfd/mfd-core.c allows addition of devices without
of_node/fwnode. I am sure returning error like[2] will break many platforms
but I just wanted to confirm the root cause and [2] fixes the boot without
NULL check for fwnode in gpiochip_setup_dev().

Hope this helps.

--
Regards,
Sudeep

[1]

-->8
diff --git i/drivers/gpio/gpiolib.c w/drivers/gpio/gpiolib.c
index b23140c6485f..e162f13aa2c9 100644
--- i/drivers/gpio/gpiolib.c
+++ w/drivers/gpio/gpiolib.c
@@ -577,13 +577,15 @@ static void gpiodevice_release(struct device *dev)
 static int gpiochip_setup_dev(struct gpio_device *gdev)
 {
        int ret;
+       struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);

        /*
         * If fwnode doesn't belong to another device, it's safe to clear its
         * initialized flag.
         */
-       if (!gdev->dev.fwnode->dev)
-               fwnode_dev_initialized(gdev->dev.fwnode, false);
+       if (fwnode && !fwnode->dev)
+               fwnode_dev_initialized(fwnode, false);
+
        ret = gcdev_register(gdev, gpio_devt);
        if (ret)
                return ret;

[2]

-->8

diff --git i/drivers/mfd/mfd-core.c w/drivers/mfd/mfd-core.c
index 16d1861e9682..3b2c4b0e9a2a 100644
--- i/drivers/mfd/mfd-core.c
+++ w/drivers/mfd/mfd-core.c
@@ -231,9 +231,11 @@ static int mfd_add_device(struct device *parent, int id,
                        }
                }

-               if (!pdev->dev.of_node)
+               if (!pdev->dev.of_node) {
                        pr_warn("%s: Failed to locate of_node [id: %d]\n",
                                cell->name, platform_id);
+                       goto fail_alias;
+               }
        }

        mfd_acpi_add_device(cell, pdev);
Saravana Kannan Feb. 4, 2023, 10:32 p.m. UTC | #7
On Tue, Jan 31, 2023 at 2:13 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Jan 30, 2023 at 08:01:17PM -0800, Saravana Kannan wrote:
> > On Mon, Jan 30, 2023 at 7:14 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Jan 30, 2023 at 02:31:53PM +0000, Sudeep Holla wrote:
> > > > On Thu, Jan 26, 2023 at 04:11:31PM -0800, Saravana Kannan wrote:
> > > > > Registering an irqdomain sets the flag for the fwnode. But having the
> > > > > flag set when a device is added is interpreted by fw_devlink to mean the
> > > > > device has already been initialized and will never probe. This prevents
> > > > > fw_devlink from creating device links with the gpio_device as a
> > > > > supplier. So, clear the flag before adding the device.
> > >
> > > ...
> > >
> > > > > +   /*
> > > > > +    * If fwnode doesn't belong to another device, it's safe to clear its
> > > > > +    * initialized flag.
> > > > > +    */
> > > > > +   if (!gdev->dev.fwnode->dev)
> > > > > +           fwnode_dev_initialized(gdev->dev.fwnode, false);
> > > >
> > > > This is the one causing the kernel crash during the boot on FVP which
> > > > Naresh has reported. Just reverted this and was able to boot, confirming
> > > > the issue with this patch.
> > >
> > > I'm wondering if
> > >
> > >         if (!dev_fwnode(&gdev->dev)->dev)
> > >                 fwnode_dev_initialized(&dev_fwnode(gdev->dev), false);
> > >
> > > works.
> >
> > No, that won't help. The problem was that with arm32, we have gpio
> > devices created without any of_node or fwnode. So I can't assume
> > fwnode will always be present.
> >
>
> Correct, and this one is not even arm32. But it is just reusing a driver
> that needs to be supported even on arm32.
>
> Not sure on how to proceed. As a simple way to check, I added a NULL check
> for fwnode building on top of Andy's suggestion[1]. That works.
>
> Also the driver in question on arm64 FVP model is drivers/mfd/vexpress-sysreg.c
> mfd_add_device() in drivers/mfd/mfd-core.c allows addition of devices without
> of_node/fwnode. I am sure returning error like[2] will break many platforms
> but I just wanted to confirm the root cause and [2] fixes the boot without
> NULL check for fwnode in gpiochip_setup_dev().
>
> Hope this helps.

Thanks for debugging it for me Sudeep. Incorporated into my v3.

-Saravana

>
> --
> Regards,
> Sudeep
>
> [1]
>
> -->8
> diff --git i/drivers/gpio/gpiolib.c w/drivers/gpio/gpiolib.c
> index b23140c6485f..e162f13aa2c9 100644
> --- i/drivers/gpio/gpiolib.c
> +++ w/drivers/gpio/gpiolib.c
> @@ -577,13 +577,15 @@ static void gpiodevice_release(struct device *dev)
>  static int gpiochip_setup_dev(struct gpio_device *gdev)
>  {
>         int ret;
> +       struct fwnode_handle *fwnode = dev_fwnode(&gdev->dev);
>
>         /*
>          * If fwnode doesn't belong to another device, it's safe to clear its
>          * initialized flag.
>          */
> -       if (!gdev->dev.fwnode->dev)
> -               fwnode_dev_initialized(gdev->dev.fwnode, false);
> +       if (fwnode && !fwnode->dev)
> +               fwnode_dev_initialized(fwnode, false);
> +
>         ret = gcdev_register(gdev, gpio_devt);
>         if (ret)
>                 return ret;
>
> [2]
>
> -->8
>
> diff --git i/drivers/mfd/mfd-core.c w/drivers/mfd/mfd-core.c
> index 16d1861e9682..3b2c4b0e9a2a 100644
> --- i/drivers/mfd/mfd-core.c
> +++ w/drivers/mfd/mfd-core.c
> @@ -231,9 +231,11 @@ static int mfd_add_device(struct device *parent, int id,
>                         }
>                 }
>
> -               if (!pdev->dev.of_node)
> +               if (!pdev->dev.of_node) {
>                         pr_warn("%s: Failed to locate of_node [id: %d]\n",
>                                 cell->name, platform_id);
> +                       goto fail_alias;
> +               }
>         }
>
>         mfd_acpi_add_device(cell, pdev);
>
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 939c776b9488..b23140c6485f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -578,6 +578,12 @@  static int gpiochip_setup_dev(struct gpio_device *gdev)
 {
 	int ret;
 
+	/*
+	 * If fwnode doesn't belong to another device, it's safe to clear its
+	 * initialized flag.
+	 */
+	if (!gdev->dev.fwnode->dev)
+		fwnode_dev_initialized(gdev->dev.fwnode, false);
 	ret = gcdev_register(gdev, gpio_devt);
 	if (ret)
 		return ret;