| Message ID | 20210902025528.1017391-3-saravanak@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Ulf reported an issue[1] with fw_devlink. This series tries to fix that issue. | expand |
On Thu, Sep 2, 2021 at 2:29 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > We don't want fw_devlink creating device links for bus devices as > > > > > they'll never probe. So mark those device node with this flag. > > > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > --- > > > > > drivers/of/platform.c | 16 ++++++++++++++++ > > > > > 1 file changed, 16 insertions(+) > > > > > > > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > > > > > index 74afbb7a4f5e..42b3936d204a 100644 > > > > > --- a/drivers/of/platform.c > > > > > +++ b/drivers/of/platform.c > > > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus, > > > > > if (!dev || !of_match_node(matches, bus)) > > > > > return 0; > > > > > > > > > > + /* > > > > > + * If the bus node has only one compatible string value and it has > > > > > + * matched as a bus node, it's never going to get probed by a device > > > > > + * driver. So flag it as such so that fw_devlink knows not to create > > > > > + * device links with this device. > > > > > + * > > > > > + * This doesn't catch all devices that'll never probe, but this is good > > > > > + * enough for now. > > > > > + * > > > > > + * This doesn't really work for PPC because of how it uses > > > > > + * of_platform_bus_probe() to add normal devices. So ignore PPC cases. > > > > > + */ > > > > > + if (!IS_ENABLED(CONFIG_PPC) && > > > > > + of_property_count_strings(bus, "compatible") == 1) > > > > > + bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE; > > > > > > > > This looks fragile relying on 1 compatible string, and the DT flags in > > > > this code have been fragile too. I'm pretty sure we have cases of > > > > simple-bus or simple-mfd that also have another compatible. > > > > > > > > Couldn't we solve this with a simple driver? > > > > > > Oh, I didn't think you'd like that. I'd lean towards that option too > > > if we can address some of the other concerns below. > > > > > > > Make 'simple-pm-bus' > > > > driver work for other cases? > > > > > > > BTW, this patch doesn't even work for > > > > simple-pm-bus. > > > > > > How do you mean? Because simple-pm-bus already has a driver and > > > doesn't set "matches" param when it calls of_platform_populate() and > > > this flag won't be set. So at least for simple-pm-bus I don't see any > > > issue. > > > > You're right. > > > > > I was trying to reuse of_default_bus_match_table without explicitly > > > referring to it, but if it's confusing I can add a separate list of > > > compatible strings and use those here instead of using "matches". > > > > What happens with a non-default table? I'm not sure we can assume the > > same behavior. > > > > > > A driver for simple-bus may cause issues if there's a > > > > more specific driver to bind to as we don't handle that. It's simply > > > > whichever matches first. > > > > > > Right, this is my worry. Especially for devices like this (there are > > > plenty of cases like this) which have a driver that probes them but > > > also lists simple-bus > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299 > > > > Uhh, that one is certainly a leakage of wanting an soc_device in the > > hierarchy, not any real bus structure reflecting the h/w. I'm not a > > fan of the soc_device stuff and its optional nature. Everything is an > > SoC, so it should always be there? Or your device hierarchy should > > change when you decide to add a soc_device? > > > > > So as long as there's a compatible string that's not one of the > > > "transparent" busses, this driver shouldn't match. So, I don't think I > > > can get away from checking the compatible strings. > > > > > > How about I check here to make sure all the "compatible" strings are > > > from an approved transparent bus list, and if it's true, I use > > > driver_override to force match it to a transparent bus driver? Would > > > you be okay with that? > > > > Can't we do that within a driver? We check this and fail probe if > > there's a more specific compatible. Then another driver can match and > > probe. > > I was thinking that initially, but if we fail a probe, the driver core > will permanently give up (won't search further) or might end up > retrying with the same driver and never get to the other driver. I'll > send out a v2 with what I described above. It's not too bad and it > will also allow us to handle the PPC cases (we'll just need to keep > adding the simple-bus equivalent entries to a table). I wasn't sure, but I traced the calls and it looks like based on __driver_attach() that if a driver fails probe another one matching should get to probe: /* * Ignore errors returned by ->probe so that the next driver can try * its luck. */ The PPC case is about descending nodes without a compatible string. Rob
On Thu, Sep 2, 2021 at 8:16 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Sep 2, 2021 at 5:53 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Thu, Sep 2, 2021 at 2:29 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > > > > > We don't want fw_devlink creating device links for bus devices as > > > > > > > they'll never probe. So mark those device node with this flag. > > > > > > > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > > --- > > > > > > > drivers/of/platform.c | 16 ++++++++++++++++ > > > > > > > 1 file changed, 16 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > > > > > > > index 74afbb7a4f5e..42b3936d204a 100644 > > > > > > > --- a/drivers/of/platform.c > > > > > > > +++ b/drivers/of/platform.c > > > > > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus, > > > > > > > if (!dev || !of_match_node(matches, bus)) > > > > > > > return 0; > > > > > > > > > > > > > > + /* > > > > > > > + * If the bus node has only one compatible string value and it has > > > > > > > + * matched as a bus node, it's never going to get probed by a device > > > > > > > + * driver. So flag it as such so that fw_devlink knows not to create > > > > > > > + * device links with this device. > > > > > > > + * > > > > > > > + * This doesn't catch all devices that'll never probe, but this is good > > > > > > > + * enough for now. > > > > > > > + * > > > > > > > + * This doesn't really work for PPC because of how it uses > > > > > > > + * of_platform_bus_probe() to add normal devices. So ignore PPC cases. > > > > > > > + */ > > > > > > > + if (!IS_ENABLED(CONFIG_PPC) && > > > > > > > + of_property_count_strings(bus, "compatible") == 1) > > > > > > > + bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE; > > > > > > > > > > > > This looks fragile relying on 1 compatible string, and the DT flags in > > > > > > this code have been fragile too. I'm pretty sure we have cases of > > > > > > simple-bus or simple-mfd that also have another compatible. > > > > > > > > > > > > Couldn't we solve this with a simple driver? > > > > > > > > > > Oh, I didn't think you'd like that. I'd lean towards that option too > > > > > if we can address some of the other concerns below. > > > > > > > > > > > Make 'simple-pm-bus' > > > > > > driver work for other cases? > > > > > > > > > > > BTW, this patch doesn't even work for > > > > > > simple-pm-bus. > > > > > > > > > > How do you mean? Because simple-pm-bus already has a driver and > > > > > doesn't set "matches" param when it calls of_platform_populate() and > > > > > this flag won't be set. So at least for simple-pm-bus I don't see any > > > > > issue. > > > > > > > > You're right. > > > > > > > > > I was trying to reuse of_default_bus_match_table without explicitly > > > > > referring to it, but if it's confusing I can add a separate list of > > > > > compatible strings and use those here instead of using "matches". > > > > > > > > What happens with a non-default table? I'm not sure we can assume the > > > > same behavior. > > > > > > > > > > A driver for simple-bus may cause issues if there's a > > > > > > more specific driver to bind to as we don't handle that. It's simply > > > > > > whichever matches first. > > > > > > > > > > Right, this is my worry. Especially for devices like this (there are > > > > > plenty of cases like this) which have a driver that probes them but > > > > > also lists simple-bus > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299 > > > > > > > > Uhh, that one is certainly a leakage of wanting an soc_device in the > > > > hierarchy, not any real bus structure reflecting the h/w. I'm not a > > > > fan of the soc_device stuff and its optional nature. Everything is an > > > > SoC, so it should always be there? Or your device hierarchy should > > > > change when you decide to add a soc_device? > > > > > > > > > So as long as there's a compatible string that's not one of the > > > > > "transparent" busses, this driver shouldn't match. So, I don't think I > > > > > can get away from checking the compatible strings. > > > > > > > > > > How about I check here to make sure all the "compatible" strings are > > > > > from an approved transparent bus list, and if it's true, I use > > > > > driver_override to force match it to a transparent bus driver? Would > > > > > you be okay with that? > > > > > > > > Can't we do that within a driver? We check this and fail probe if > > > > there's a more specific compatible. Then another driver can match and > > > > probe. > > > > > > I was thinking that initially, but if we fail a probe, the driver core > > > will permanently give up (won't search further) or might end up > > > retrying with the same driver and never get to the other driver. I'll > > > send out a v2 with what I described above. It's not too bad and it > > > will also allow us to handle the PPC cases (we'll just need to keep > > > adding the simple-bus equivalent entries to a table). > > > > I wasn't sure, but I traced the calls and it looks like based on > > __driver_attach() that if a driver fails probe another one matching > > should get to probe: > > __driver_attach() is called over every device already in a bus. It's > called only when a new driver is registered. So it makes sense that > one ignores the error returned from probe(). You don't want to fail > driver registration because one specific device needs to defer probe. The behavior should be the same no matter whether the device or driver is registered first. Deferred probe errors are handled differently AFAICT. > > The comment is actually from __device_attach_driver() > > > > > /* > > * Ignore errors returned by ->probe so that the next driver can try > > * its luck. > > */ > > I saw that comment too, but isn't the comment wrong/stale? I don't know... > > bus_probe_device() -> device_initial_probe() -> __device_attach(). > > In __device_attach() we have: > ret = bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver); > > If you look at bus_for_each_drv()'s comment: > * ...... If @fn returns anything but 0, we break out > * and return it. If @start is not NULL, we use it as the head > * of the list. > > Inside __device_attach_driver() we see: > /* > * Ignore errors returned by ->probe so that the next driver can try > * its luck. > */ > ret = driver_probe_device(drv, dev); > if (ret < 0) > return ret; > > So if probe() returned an error, we'd return it right back out. And > then bus_for_each_drv() will stop searching for more drivers that > match. With the exception of deferred probe, probe errors are made positive and then ignored. > So I don't think one driver can give up after a match and have another > driver give a device a shot. I think it just needs to be tried out... I would like the above to work because it at least partially solves the problem of multiple drivers matching. It only works if the fallback driver knows there's a better match though which is this case. If we don't know then a better match would have to unbind the first driver and bind to the better match. I'm not sure that could ever work generically. Rob
On Fri, Sep 3, 2021 at 3:16 AM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Sep 2, 2021 at 5:53 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Thu, Sep 2, 2021 at 2:29 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Thu, Sep 2, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > On Thu, Sep 2, 2021 at 11:57 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > On Thu, Sep 2, 2021 at 7:24 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > > > > > On Wed, Sep 1, 2021 at 9:55 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > > > > > We don't want fw_devlink creating device links for bus devices as > > > > > > > they'll never probe. So mark those device node with this flag. > > > > > > > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > > --- > > > > > > > drivers/of/platform.c | 16 ++++++++++++++++ > > > > > > > 1 file changed, 16 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > > > > > > > index 74afbb7a4f5e..42b3936d204a 100644 > > > > > > > --- a/drivers/of/platform.c > > > > > > > +++ b/drivers/of/platform.c > > > > > > > @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus, > > > > > > > if (!dev || !of_match_node(matches, bus)) > > > > > > > return 0; > > > > > > > > > > > > > > + /* > > > > > > > + * If the bus node has only one compatible string value and it has > > > > > > > + * matched as a bus node, it's never going to get probed by a device > > > > > > > + * driver. So flag it as such so that fw_devlink knows not to create > > > > > > > + * device links with this device. > > > > > > > + * > > > > > > > + * This doesn't catch all devices that'll never probe, but this is good > > > > > > > + * enough for now. > > > > > > > + * > > > > > > > + * This doesn't really work for PPC because of how it uses > > > > > > > + * of_platform_bus_probe() to add normal devices. So ignore PPC cases. > > > > > > > + */ > > > > > > > + if (!IS_ENABLED(CONFIG_PPC) && > > > > > > > + of_property_count_strings(bus, "compatible") == 1) > > > > > > > + bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE; > > > > > > > > > > > > This looks fragile relying on 1 compatible string, and the DT flags in > > > > > > this code have been fragile too. I'm pretty sure we have cases of > > > > > > simple-bus or simple-mfd that also have another compatible. > > > > > > > > > > > > Couldn't we solve this with a simple driver? > > > > > > > > > > Oh, I didn't think you'd like that. I'd lean towards that option too > > > > > if we can address some of the other concerns below. > > > > > > > > > > > Make 'simple-pm-bus' > > > > > > driver work for other cases? > > > > > > > > > > > BTW, this patch doesn't even work for > > > > > > simple-pm-bus. > > > > > > > > > > How do you mean? Because simple-pm-bus already has a driver and > > > > > doesn't set "matches" param when it calls of_platform_populate() and > > > > > this flag won't be set. So at least for simple-pm-bus I don't see any > > > > > issue. > > > > > > > > You're right. > > > > > > > > > I was trying to reuse of_default_bus_match_table without explicitly > > > > > referring to it, but if it's confusing I can add a separate list of > > > > > compatible strings and use those here instead of using "matches". > > > > > > > > What happens with a non-default table? I'm not sure we can assume the > > > > same behavior. > > > > > > > > > > A driver for simple-bus may cause issues if there's a > > > > > > more specific driver to bind to as we don't handle that. It's simply > > > > > > whichever matches first. > > > > > > > > > > Right, this is my worry. Especially for devices like this (there are > > > > > plenty of cases like this) which have a driver that probes them but > > > > > also lists simple-bus > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/arm-realview-pb11mp.dts?id=73f3af7b4611d77bdaea303fb639333eb28e37d7#n299 > > > > > > > > Uhh, that one is certainly a leakage of wanting an soc_device in the > > > > hierarchy, not any real bus structure reflecting the h/w. I'm not a > > > > fan of the soc_device stuff and its optional nature. Everything is an > > > > SoC, so it should always be there? Or your device hierarchy should > > > > change when you decide to add a soc_device? > > > > > > > > > So as long as there's a compatible string that's not one of the > > > > > "transparent" busses, this driver shouldn't match. So, I don't think I > > > > > can get away from checking the compatible strings. > > > > > > > > > > How about I check here to make sure all the "compatible" strings are > > > > > from an approved transparent bus list, and if it's true, I use > > > > > driver_override to force match it to a transparent bus driver? Would > > > > > you be okay with that? > > > > > > > > Can't we do that within a driver? We check this and fail probe if > > > > there's a more specific compatible. Then another driver can match and > > > > probe. > > > > > > I was thinking that initially, but if we fail a probe, the driver core > > > will permanently give up (won't search further) or might end up > > > retrying with the same driver and never get to the other driver. I'll > > > send out a v2 with what I described above. It's not too bad and it > > > will also allow us to handle the PPC cases (we'll just need to keep > > > adding the simple-bus equivalent entries to a table). > > > > I wasn't sure, but I traced the calls and it looks like based on > > __driver_attach() that if a driver fails probe another one matching > > should get to probe: > > __driver_attach() is called over every device already in a bus. It's > called only when a new driver is registered. So it makes sense that > one ignores the error returned from probe(). You don't want to fail > driver registration because one specific device needs to defer probe. > > The comment is actually from __device_attach_driver() > > > > > /* > > * Ignore errors returned by ->probe so that the next driver can try > > * its luck. > > */ > > I saw that comment too, but isn't the comment wrong/stale? > > bus_probe_device() -> device_initial_probe() -> __device_attach(). > > In __device_attach() we have: > ret = bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver); > > If you look at bus_for_each_drv()'s comment: > * ...... If @fn returns anything but 0, we break out > * and return it. If @start is not NULL, we use it as the head > * of the list. > > Inside __device_attach_driver() we see: > /* > * Ignore errors returned by ->probe so that the next driver can try > * its luck. > */ > ret = driver_probe_device(drv, dev); > if (ret < 0) > return ret; > > So if probe() returned an error, we'd return it right back out. And > then bus_for_each_drv() will stop searching for more drivers that > match. Well, not quite. If ->probe() returns an error, really_probe() will convert it into a positive number. __driver_probe_device() will then return as is and driver_probe_device() doesn't touch that value. Bottom line: you'll see a positive number here, so the check above will not trigger and 0 is returned, so bus_for_each_drv() will actually continue searching. > So I don't think one driver can give up after a match and have another > driver give a device a shot. Yes, it can.
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 74afbb7a4f5e..42b3936d204a 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -392,6 +392,22 @@ static int of_platform_bus_create(struct device_node *bus, if (!dev || !of_match_node(matches, bus)) return 0; + /* + * If the bus node has only one compatible string value and it has + * matched as a bus node, it's never going to get probed by a device + * driver. So flag it as such so that fw_devlink knows not to create + * device links with this device. + * + * This doesn't catch all devices that'll never probe, but this is good + * enough for now. + * + * This doesn't really work for PPC because of how it uses + * of_platform_bus_probe() to add normal devices. So ignore PPC cases. + */ + if (!IS_ENABLED(CONFIG_PPC) && + of_property_count_strings(bus, "compatible") == 1) + bus->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE; + for_each_child_of_node(bus, child) { pr_debug(" create child: %pOF\n", child); rc = of_platform_bus_create(child, matches, lookup, &dev->dev, strict);
We don't want fw_devlink creating device links for bus devices as they'll never probe. So mark those device node with this flag. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/of/platform.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)