| Message ID | 20230127001141.407071-5-saravanak@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fw_devlink improvements | expand |
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); + Blank line. > ret = gcdev_register(gdev, gpio_devt); > if (ret) > return ret;
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. >
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.
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.
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.
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
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);
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 --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;