| Message ID | 20230127001141.407071-5-saravanak@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fw_devlink improvements | expand |
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;