| Message ID | 20210202043345.3778765-1-saravanak@google.com |
|---|---|
| Headers | show |
| Series | Make fw_devlink=on more forgiving | expand |
On Tue, Feb 2, 2021 at 11:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Feb 2, 2021 at 11:44 PM Saravana Kannan <saravanak@google.com> wrote: > > On Tue, Feb 2, 2021 at 1:22 PM Martin Kaiser <martin@kaiser.cx> wrote: > > > Thus wrote Saravana Kannan (saravanak@google.com): > > > All of those drivers have a gpio in > > > their device-tree node, such as > > > > > > my_driver { > > > gpio_test1 = <&gpio1 0 0>; > > > ... > > > }; > > > > > > with gpio1 from arch/arm/boot/dts/imx25.dtsi. > > > > > > The probe function calls > > > > > > of_get_named_gpio(np, "gpio_test1", 0); > > > > > > to get the gpio. This fails with -EINVAL. > > > > And you didn't see this issue with the fsl,avic patch? > > > > The property you are using is not a standard GPIO binding (-gpios, > > gpio, gpios) and I'm not surprised it's not working. The gpio1 is > > probably getting probe deferred and ends up running after "my_driver". > > So my_driver doesn't support deferred probe, as of_get_named_gpio() > returns -EINVAL instead of -EPROBE_DEFER? > Converting my_driver from of_get_named_gpio() to the gpiod_*() API > should at least make the driver support probe deferral, after which I > expect it to start working again on reprobe? The way I understood the API/example, you can't just change the code and have it work. The DT itself isn't using standard bindings. And we can't make kernel changes that assume the DT has been changed to match the code. So, the best we could do is have of_get_named_gpio() return -EPROBE_DEFER if it doesn't find the GPIO -- assuming that doesn't break other users. Or have this specific driver remap the -EINVAL to -EPROBE_DEFER. -Saravana
Hi Saravana, On Wed, Feb 3, 2021 at 9:11 AM Saravana Kannan <saravanak@google.com> wrote: > On Tue, Feb 2, 2021 at 11:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Feb 2, 2021 at 11:44 PM Saravana Kannan <saravanak@google.com> wrote: > > > On Tue, Feb 2, 2021 at 1:22 PM Martin Kaiser <martin@kaiser.cx> wrote: > > > > Thus wrote Saravana Kannan (saravanak@google.com): > > > > All of those drivers have a gpio in > > > > their device-tree node, such as > > > > > > > > my_driver { > > > > gpio_test1 = <&gpio1 0 0>; > > > > ... > > > > }; > > > > > > > > with gpio1 from arch/arm/boot/dts/imx25.dtsi. > > > > > > > > The probe function calls > > > > > > > > of_get_named_gpio(np, "gpio_test1", 0); > > > > > > > > to get the gpio. This fails with -EINVAL. > > > > > > And you didn't see this issue with the fsl,avic patch? > > > > > > The property you are using is not a standard GPIO binding (-gpios, > > > gpio, gpios) and I'm not surprised it's not working. The gpio1 is > > > probably getting probe deferred and ends up running after "my_driver". > > > > So my_driver doesn't support deferred probe, as of_get_named_gpio() > > returns -EINVAL instead of -EPROBE_DEFER? > > Converting my_driver from of_get_named_gpio() to the gpiod_*() API > > should at least make the driver support probe deferral, after which I > > expect it to start working again on reprobe? > > The way I understood the API/example, you can't just change the code > and have it work. The DT itself isn't using standard bindings. And we Oh, right. > can't make kernel changes that assume the DT has been changed to match > the code. So, the best we could do is have of_get_named_gpio() return > -EPROBE_DEFER if it doesn't find the GPIO -- assuming that doesn't > break other users. Or have this specific driver remap the -EINVAL to > -EPROBE_DEFER. The latter would hide real errors, too, and would cause futile reprobes. Gr{oetje,eeting}s, Geert
Thus wrote Geert Uytterhoeven (geert@linux-m68k.org): > > The property you are using is not a standard GPIO binding (-gpios, > > gpio, gpios) and I'm not surprised it's not working. The gpio1 is > > probably getting probe deferred and ends up running after "my_driver". > So my_driver doesn't support deferred probe, I know that the gpio definition in the device-tree is non-standard (and should have been done differently). Apart from this, the driver uses module_platform_driver_probe. My understanding is that this prevents probe deferral. Does this mean that from now on, a driver which requests a gpio must not use module_platform_driver_probe? Thanks, Martin
This patch series solves two general issues with fw_devlink=on Patch 1/3 and 3/3 addresses the issue of firmware nodes that look like they'll have struct devices created for them, but will never actually have struct devices added for them. For example, DT nodes with a compatible property that don't have devices added for them. Patch 2/2 address (for static kernels) the issue of optional suppliers that'll never have a driver registered for them. So, if the device could have probed with fw_devlink=permissive with a static kernel, this patch should allow those devices to probe with a fw_devlink=on. This doesn't solve it for the case where modules are enabled because there's no way to tell if a driver will never be registered or it's just about to be registered. I have some other ideas for that, but it'll have to come later thinking about it a bit. Marek, Geert, I don't expect v2 to do any better for your cases. This series not making any difference for Marek is still a mystery to me. I guess one of the consumers doesn't take too well to its probe (and it's consumers' probe) being delayed till late_initcall(). I'll continue looking into it. Marc, This v2 should do better than v1 with gpiolib stub driver reverted. I forgot to take care of the case where more suppliers could link after I went and deleted some of the links. v2 handles that now. Tudor, You should still make the clock driver fix (because it's a bug), but I think this series will fix your issue too (even without the clock driver fix). Can you please give this a shot? Martin, If you tested this series, can you please give a Tested-by? Thanks, Saravana v1 -> v2: Patch 1: Added a flag to fwnodes that aren't devices. Patch 3: New patch to ise the flag set in patch 1 to not create bad links. Saravana Kannan (3): driver core: fw_devlink: Detect supplier devices that will never be added driver core: fw_devlink: Handle missing drivers for optional suppliers of: property: Don't add links to absent suppliers drivers/base/base.h | 2 + drivers/base/core.c | 135 +++++++++++++++++++++++++++++++++++------ drivers/base/dd.c | 5 ++ drivers/of/property.c | 4 +- include/linux/fwnode.h | 2 + 5 files changed, 127 insertions(+), 21 deletions(-)