| Message ID | 1438870315-18689-3-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 06, 2015 at 04:11:39PM +0200, Tomeu Vizoso wrote: > Walks the OF tree up and finds the closest ancestor that has a platform > device associated with it, probing it if isn't bound to a driver yet. > The above should ensure that the dependency represented by the passed OF > node is available, because probing a platform device should cause its > descendants to be probed as well. This sounds like it's going to break in the case where we have MFDs that represent their functions in DT (not a pattern I'm a fan of but it's a thing people do). We'll walk back to the platform device for the MFD function, try to probe it and then give up. Perhaps that's good enough anyway but it's not clear to me why we don't just try every parent we find? I'm also not a fan of the fact that the interface here is explicitly saying that we want to probe a platform device, that's an implementation detail that callers shouldn't need to know about. From the point of view of the callers what they're trying to do is kick any dependencies into being instantiated, the fact that we currently try to accomplish it with platform devices isn't something they care about.
On 7 August 2015 at 14:19, Mark Brown <broonie@kernel.org> wrote: > On Thu, Aug 06, 2015 at 04:11:39PM +0200, Tomeu Vizoso wrote: > >> Walks the OF tree up and finds the closest ancestor that has a platform >> device associated with it, probing it if isn't bound to a driver yet. > >> The above should ensure that the dependency represented by the passed OF >> node is available, because probing a platform device should cause its >> descendants to be probed as well. > > This sounds like it's going to break in the case where we have MFDs that > represent their functions in DT (not a pattern I'm a fan of but it's a > thing people do). We'll walk back to the platform device for the MFD > function, try to probe it and then give up. Perhaps that's good enough > anyway but it's not clear to me why we don't just try every parent we > find? Agreed. In the attempt at probing dependencies before a device is probed, I considered that a device's parent is also a dependency and that worked well. From what I saw, few devices will defer their probe if their parent hasn't been probed yet, assuming that it will have probed already. But with simple-mfd and simple-bus that shouldn't be relied upon as things will break if their parents defer their probe. With async probing enabled this failure scenario becomes more probable. > I'm also not a fan of the fact that the interface here is explicitly > saying that we want to probe a platform device, that's an implementation > detail that callers shouldn't need to know about. From the point of > view of the callers what they're trying to do is kick any dependencies > into being instantiated, the fact that we currently try to accomplish it > with platform devices isn't something they care about. Agreed. Thanks, Tomeu
On 11 August 2015 at 11:37, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: > On 7 August 2015 at 14:19, Mark Brown <broonie@kernel.org> wrote: >> On Thu, Aug 06, 2015 at 04:11:39PM +0200, Tomeu Vizoso wrote: >> >>> Walks the OF tree up and finds the closest ancestor that has a platform >>> device associated with it, probing it if isn't bound to a driver yet. >> >>> The above should ensure that the dependency represented by the passed OF >>> node is available, because probing a platform device should cause its >>> descendants to be probed as well. >> >> This sounds like it's going to break in the case where we have MFDs that >> represent their functions in DT (not a pattern I'm a fan of but it's a >> thing people do). We'll walk back to the platform device for the MFD >> function, try to probe it and then give up. Perhaps that's good enough >> anyway but it's not clear to me why we don't just try every parent we >> find? > > Agreed. In the attempt at probing dependencies before a device is > probed, I considered that a device's parent is also a dependency and > that worked well. From what I saw, few devices will defer their probe > if their parent hasn't been probed yet, assuming that it will have > probed already. But with simple-mfd and simple-bus that shouldn't be > relied upon as things will break if their parents defer their probe. > With async probing enabled this failure scenario becomes more > probable. Actually I'm not sure how we could probe the ascendants on demand, as currently the parent's device lock is taken when probing so trying to probe a sibling from within a probe callback will cause a deadlock. AFAICS this is only needed for USB interface devices and this behaviour could be limited to them, but I don't like much assuming that no USB device will ever have a dependency on a sibling (though that probably won't happen ever). Regards, Tomeu >> I'm also not a fan of the fact that the interface here is explicitly >> saying that we want to probe a platform device, that's an implementation >> detail that callers shouldn't need to know about. From the point of >> view of the callers what they're trying to do is kick any dependencies >> into being instantiated, the fact that we currently try to accomplish it >> with platform devices isn't something they care about. > > Agreed. > > Thanks, > > Tomeu
On Mon, Sep 07, 2015 at 02:31:06PM +0200, Tomeu Vizoso wrote: > On 11 August 2015 at 11:37, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: > > On 7 August 2015 at 14:19, Mark Brown <broonie@kernel.org> wrote: > >> This sounds like it's going to break in the case where we have MFDs that > >> represent their functions in DT (not a pattern I'm a fan of but it's a > >> thing people do). We'll walk back to the platform device for the MFD > >> function, try to probe it and then give up. Perhaps that's good enough > >> anyway but it's not clear to me why we don't just try every parent we > >> find? > > Agreed. In the attempt at probing dependencies before a device is > > probed, I considered that a device's parent is also a dependency and > Actually I'm not sure how we could probe the ascendants on demand, as > currently the parent's device lock is taken when probing so trying to > probe a sibling from within a probe callback will cause a deadlock. How do silbilings come into this? There is an issue there but it's going to happen anyway. > AFAICS this is only needed for USB interface devices and this > behaviour could be limited to them, but I don't like much assuming > that no USB device will ever have a dependency on a sibling (though > that probably won't happen ever). I don't see the connection with USB here, sorry - my initial thought was about MFDs?
On 11 September 2015 at 11:57, Mark Brown <broonie@kernel.org> wrote: > On Mon, Sep 07, 2015 at 02:31:06PM +0200, Tomeu Vizoso wrote: >> On 11 August 2015 at 11:37, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >> > On 7 August 2015 at 14:19, Mark Brown <broonie@kernel.org> wrote: > >> >> This sounds like it's going to break in the case where we have MFDs that >> >> represent their functions in DT (not a pattern I'm a fan of but it's a >> >> thing people do). We'll walk back to the platform device for the MFD >> >> function, try to probe it and then give up. Perhaps that's good enough >> >> anyway but it's not clear to me why we don't just try every parent we >> >> find? > >> > Agreed. In the attempt at probing dependencies before a device is >> > probed, I considered that a device's parent is also a dependency and > >> Actually I'm not sure how we could probe the ascendants on demand, as >> currently the parent's device lock is taken when probing so trying to >> probe a sibling from within a probe callback will cause a deadlock. > > How do silbilings come into this? There is an issue there but it's > going to happen anyway. Once a platform device (with the platform bus as its parent) is retrieved from the deferred queue, both the parent and the device in question are locked (because of the USB stuff mentioned below). If that device depends on another device whose parent is the platform bus and we try to probe it (useless, but I don't see a good way of avoiding it) then we'll deadlock when device_attach locks that device. >> AFAICS this is only needed for USB interface devices and this >> behaviour could be limited to them, but I don't like much assuming >> that no USB device will ever have a dependency on a sibling (though >> that probably won't happen ever). > > I don't see the connection with USB here, sorry - my initial thought was > about MFDs? This commit introduced locking of a device's parent before it's probed, mainly for USB interfaces: bf74ad5bc417 ("[PATCH] Hold the device's parent's lock during probe and remove") Regards, Tomeu
On Fri, Sep 11, 2015 at 04:06:07PM +0200, Tomeu Vizoso wrote: > Once a platform device (with the platform bus as its parent) is > retrieved from the deferred queue, both the parent and the device in > question are locked (because of the USB stuff mentioned below). If > that device depends on another device whose parent is the platform bus > and we try to probe it (useless, but I don't see a good way of > avoiding it) then we'll deadlock when device_attach locks that device. Ugh, that's nasty. Trying to fix this would most likely devolve into trying to shove things onto the deferred list in the right order but that's definitely a very different solution with problems.
On 11 September 2015 at 17:35, Mark Brown <broonie@kernel.org> wrote: > On Fri, Sep 11, 2015 at 04:06:07PM +0200, Tomeu Vizoso wrote: > >> Once a platform device (with the platform bus as its parent) is >> retrieved from the deferred queue, both the parent and the device in >> question are locked (because of the USB stuff mentioned below). If >> that device depends on another device whose parent is the platform bus >> and we try to probe it (useless, but I don't see a good way of >> avoiding it) then we'll deadlock when device_attach locks that device. > > Ugh, that's nasty. Trying to fix this would most likely devolve into > trying to shove things onto the deferred list in the right order but > that's definitely a very different solution with problems. Well, that's effectively what my "ordered probing" series did (and indeed didn't have this problem and parents were made sure to have probed before their children), but if we want to have dependency information before the probe starts, then we cannot reuse the implementation of the DT bindings that we already have in the resource getters as this series do. Regards, Tomeu
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 8a002d6151f2..a1930c0d1fee 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -192,6 +192,8 @@ static struct platform_device *of_platform_device_create_pdata( goto err_clear_flag; } + np->platform_dev = dev; + return dev; err_clear_flag: @@ -501,6 +503,65 @@ void of_platform_depopulate(struct device *parent) } EXPORT_SYMBOL_GPL(of_platform_depopulate); +/** + * of_platform_probe() - Probe platform device associated with OF node + * @np: node to probe + * + * Walks the OF tree up and finds the closest ancestor that has a platform + * device associated with it, probing it if isn't bound to a driver yet. + * + * The above should ensure that the dependency represented by the passed OF + * node is available, because probing a platform device should cause its + * descendants to be probed as well. + */ +void of_platform_probe(struct device_node *np) +{ + struct device_node *target; + struct platform_device *pdev = NULL; + + if (!of_root || !of_node_check_flag(of_root, OF_POPULATED_BUS)) + return; + + if (!np) + return; + + of_node_get(np); + + for (target = np; + !of_node_is_root(target); + target = of_get_next_parent(target)) + if (target->platform_dev) { + pdev = target->platform_dev; + break; + } + + of_node_put(target); + + if (!pdev) { + pr_warn("Couldn't find a platform device for node '%s'\n", + of_node_full_name(np)); + return; + } + + /* + * Device is bound or is being probed right now. If we have bad luck + * and the dependency isn't ready when it's needed, deferred probe + * will save us. + */ + if (pdev->dev.driver) + return; + + if (device_attach(&pdev->dev) != 1) + /* + * This cannot be a warning for now because clock nodes have a + * compatible string but the clock framework doesn't follow the + * device/driver model. + */ + pr_debug("Probe failed for %s (%s)\n", of_node_full_name(np), + dev_name(&pdev->dev)); +} +EXPORT_SYMBOL_GPL(of_platform_probe); + #ifdef CONFIG_OF_DYNAMIC static int of_platform_notify(struct notifier_block *nb, unsigned long action, void *arg) diff --git a/include/linux/of.h b/include/linux/of.h index edc068d19c79..2ace86a5fa2d 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -52,6 +52,7 @@ struct device_node { phandle phandle; const char *full_name; struct fwnode_handle fwnode; + struct platform_device *platform_dev; struct property *properties; struct property *deadprops; /* removed properties */ diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h index 611a691145c4..91ca92c7c061 100644 --- a/include/linux/of_platform.h +++ b/include/linux/of_platform.h @@ -73,6 +73,7 @@ extern int of_platform_populate(struct device_node *root, const struct of_dev_auxdata *lookup, struct device *parent); extern void of_platform_depopulate(struct device *parent); +extern void of_platform_probe(struct device_node *np); #else static inline int of_platform_populate(struct device_node *root, const struct of_device_id *matches, @@ -82,6 +83,7 @@ static inline int of_platform_populate(struct device_node *root, return -ENODEV; } static inline void of_platform_depopulate(struct device *parent) { } +static inline void of_platform_probe(struct device_node *np) { } #endif #if defined(CONFIG_OF_DYNAMIC) && defined(CONFIG_OF_ADDRESS)
Walks the OF tree up and finds the closest ancestor that has a platform device associated with it, probing it if isn't bound to a driver yet. The above should ensure that the dependency represented by the passed OF node is available, because probing a platform device should cause its descendants to be probed as well. Subsystems can use this when looking up resources for drivers, to reduce the chances of deferred probes because of the probing order of devices. Also adds a platform_dev member to struct device_node, pointing to the platform device that was registered based on that node, if any. Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> --- Changes in v3: - Set and use device_node.platform_dev instead of reversing the logic to find the platform device that encloses a device node. - Drop the fwnode API to probe firmware nodes and add OF-only API for now. I think this same scheme could be used for machines with ACPI, but I haven't been able to find one that had to defer its probes because of the device probe order. Changes in v2: None drivers/of/platform.c | 61 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/of.h | 1 + include/linux/of_platform.h | 2 ++ 3 files changed, 64 insertions(+)