| Message ID | 20230127001141.407071-5-saravanak@google.com |
|---|---|
| State | Superseded |
| 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 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;