| Message ID | 20201104232356.4038506-1-saravanak@google.com |
|---|---|
| Headers | show |
| Series | Refactor fw_devlink to significantly improve boot time | expand |
On Wed, Nov 4, 2020 at 3:23 PM Saravana Kannan <saravanak@google.com> wrote: > > The current implementation of fw_devlink is very inefficient because it > tries to get away without creating fwnode links in the name of saving > memory usage. Past attempts to optimize runtime at the cost of memory > usage were blocked with request for data showing that the optimization > made significant improvement for real world scenarios. > > We have those scenarios now. There have been several reports of boot > time increase in the order of seconds in this thread [1]. Several OEMs > and SoC manufacturers have also privately reported significant > (350-400ms) increase in boot time due to all the parsing done by > fw_devlink. > > So this patch series refactors fw_devlink to be more efficient. The key > difference now is the addition of support for fwnode links -- just a few > simple APIs. This also allows most of the code to be moved out of > firmware specific (DT mostly) code into driver core. > > This brings the following benefits: > - Instead of parsing the device tree multiple times (complexity was > close to O(N^3) where N in the number of properties) during bootup, > fw_devlink parses each fwnode node/property only once and creates > fwnode links. The rest of the fw_devlink code then just looks at these > fwnode links to do rest of the work. > > - Makes it much easier to debug probe issue due to fw_devlink in the > future. fw_devlink=on blocks the probing of devices if they depend on > a device that hasn't been added yet. With this refactor, it'll be very > easy to tell what that device is because we now have a reference to > the fwnode of the device. > > - Much easier to add fw_devlink support to ACPI and other firmware > types. A refactor to move the common bits from DT specific code to > driver core was in my TODO list as a prerequisite to adding ACPI > support to fw_devlink. This series gets that done. > > Tomi/Laurent/Grygorii, > > If you can test this series, that'd be great! > > Thanks, > Saravana > > [1] - https://lore.kernel.org/linux-pm/CAGETcx-aiW251dhEXT1GNb9bi6YcX8W=jLBrro5CnPuEjGL09g@mail.gmail.com/ Forgot the update this link in the cover letter. Here's a much better link to the thread that discusses boot time regression: https://lore.kernel.org/linux-omap/ea02f57e-871d-cd16-4418-c1da4bbc4696@ti.com/ -Saravana
On Wed, Nov 04, 2020 at 03:23:38PM -0800, Saravana Kannan wrote: > This reverts commit 2451e746478a6a6e981cfa66b62b791ca93b90c8. > > Signed-off-by: Saravana Kannan <saravanak@google.com> You need to say _why_ you are doing this, it's obvious _what_ you are doing :) Same for the other reverts in this series. thanks, greg k-h
On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote: > Add helper functions __fw_devlink_link_to_consumers() and > __fw_devlink_link_to_suppliers() that convert fwnode links to device > links. > > __fw_devlink_link_to_consumers() is for creating: > - Device links between a newly added device and all its consumer devices > that have been added to driver core. > - Proxy SYNC_STATE_ONLY device links between the newly added device and > the parent devices of all its consumers that have not been added to > driver core yet. > > __fw_devlink_link_to_suppliers() is for creating: > - Device links between a newly added device and all its supplier devices > - Proxy SYNC_STATE_ONLY device links between the newly added device and > all the supplier devices of its child device nodes. > > Signed-off-by: Saravana Kannan <saravanak@google.com> Did you just add build warnings with these static functions that no one calls? thanks, greg k-h
On Thu, Nov 5, 2020 at 1:33 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Nov 04, 2020 at 03:23:38PM -0800, Saravana Kannan wrote: > > This reverts commit 2451e746478a6a6e981cfa66b62b791ca93b90c8. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > You need to say _why_ you are doing this, it's obvious _what_ you are > doing :) > > Same for the other reverts in this series. Sorry, forgot about this. Will fix it in v2. -Saravana
On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote: > > Add helper functions __fw_devlink_link_to_consumers() and > > __fw_devlink_link_to_suppliers() that convert fwnode links to device > > links. > > > > __fw_devlink_link_to_consumers() is for creating: > > - Device links between a newly added device and all its consumer devices > > that have been added to driver core. > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > the parent devices of all its consumers that have not been added to > > driver core yet. > > > > __fw_devlink_link_to_suppliers() is for creating: > > - Device links between a newly added device and all its supplier devices > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > all the supplier devices of its child device nodes. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Did you just add build warnings with these static functions that no one > calls? The next patch in this series uses it. I'm just splitting it up into a separate patch so that it's digestible and I can provide more details in the commit text. Couple of options: 1. Drop the static in this patch and add it back when it's used in patch 18/18. 2. Drop the commit text and squash this with 18/18 if you think the function documentation is clear enough and it won't make patch 18/18 too hard to review. Please let me know which one you'd prefer or if you have other options. I don't have a strong opinion on how the patches are split up as long as it's easy for the reviewers and future folks who look at git log to understand stuff. -Saravana
Hi Saravana, Thank you for working on this ! On Wed, Nov 04, 2020 at 03:23:37PM -0800, Saravana Kannan wrote: > The current implementation of fw_devlink is very inefficient because it > tries to get away without creating fwnode links in the name of saving > memory usage. Past attempts to optimize runtime at the cost of memory > usage were blocked with request for data showing that the optimization > made significant improvement for real world scenarios. > > We have those scenarios now. There have been several reports of boot > time increase in the order of seconds in this thread [1]. Several OEMs > and SoC manufacturers have also privately reported significant > (350-400ms) increase in boot time due to all the parsing done by > fw_devlink. > > So this patch series refactors fw_devlink to be more efficient. The key > difference now is the addition of support for fwnode links -- just a few > simple APIs. This also allows most of the code to be moved out of > firmware specific (DT mostly) code into driver core. > > This brings the following benefits: > - Instead of parsing the device tree multiple times (complexity was > close to O(N^3) where N in the number of properties) during bootup, > fw_devlink parses each fwnode node/property only once and creates > fwnode links. The rest of the fw_devlink code then just looks at these > fwnode links to do rest of the work. > > - Makes it much easier to debug probe issue due to fw_devlink in the > future. fw_devlink=on blocks the probing of devices if they depend on > a device that hasn't been added yet. With this refactor, it'll be very > easy to tell what that device is because we now have a reference to > the fwnode of the device. > > - Much easier to add fw_devlink support to ACPI and other firmware > types. A refactor to move the common bits from DT specific code to > driver core was in my TODO list as a prerequisite to adding ACPI > support to fw_devlink. This series gets that done. > > Tomi/Laurent/Grygorii, > > If you can test this series, that'd be great! I gave it a try, rebasing my branch from v5.9 to v5.10-rc2 first. On v5.10-rc2 the kernel dies when booting due to a deadlock (reported by lockdep, so hopefully not too hard to debug). *sigh*. Fortunately, it dies after the fw_devlink initialization, so I can still report results. Before your series: [ 0.743065] cpuidle: using governor menu [ 13.350259] No ATAGs? With your series applied: [ 0.722670] cpuidle: using governor menu [ 1.135859] No ATAGs? That's a very clear improvement :-) Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > [1] - https://lore.kernel.org/linux-pm/CAGETcx-aiW251dhEXT1GNb9bi6YcX8W=jLBrro5CnPuEjGL09g@mail.gmail.com/ > > Saravana Kannan (18): > Revert "driver core: Avoid deferred probe due to fw_devlink_pause/resume()" > Revert "driver core: Rename dev_links_info.defer_sync to defer_hook" > Revert "driver core: Don't do deferred probe in parallel with kernel_init thread" > Revert "driver core: Remove check in driver_deferred_probe_force_trigger()" > Revert "of: platform: Batch fwnode parsing when adding all top level devices" > Revert "driver core: fw_devlink: Add support for batching fwnode parsing" > driver core: Add fwnode_init() > driver core: Add fwnode link support > driver core: Allow only unprobed consumers for SYNC_STATE_ONLY device links > device property: Add fwnode_is_ancestor_of() > driver core: Redefine the meaning of fwnode_operations.add_links() > driver core: Add fw_devlink_parse_fwtree() > driver core: Add fwnode_get_next_parent_dev() helper function > driver core: Use device's fwnode to check if it is waiting for suppliers > of: property: Update implementation of add_links() to create fwnode links > efi: Update implementation of add_links() to create fwnode links > driver core: Add helper functions to convert fwnode links to device links > driver core: Refactor fw_devlink feature > > drivers/acpi/property.c | 2 +- > drivers/acpi/scan.c | 2 +- > drivers/base/core.c | 584 +++++++++++++++++++++----------- > drivers/base/property.c | 27 ++ > drivers/base/swnode.c | 2 +- > drivers/firmware/efi/efi-init.c | 31 +- > drivers/of/dynamic.c | 1 + > drivers/of/platform.c | 2 - > drivers/of/property.c | 150 +++----- > include/linux/device.h | 10 +- > include/linux/fwnode.h | 66 ++-- > include/linux/of.h | 2 +- > include/linux/property.h | 2 + > kernel/irq/irqdomain.c | 2 +- > 14 files changed, 490 insertions(+), 393 deletions(-)
On Thu, Nov 05, 2020 at 03:32:05PM -0800, Saravana Kannan wrote: > On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote: > > > Add helper functions __fw_devlink_link_to_consumers() and > > > __fw_devlink_link_to_suppliers() that convert fwnode links to device > > > links. > > > > > > __fw_devlink_link_to_consumers() is for creating: > > > - Device links between a newly added device and all its consumer devices > > > that have been added to driver core. > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > > the parent devices of all its consumers that have not been added to > > > driver core yet. > > > > > > __fw_devlink_link_to_suppliers() is for creating: > > > - Device links between a newly added device and all its supplier devices > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > > all the supplier devices of its child device nodes. > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > Did you just add build warnings with these static functions that no one > > calls? > > The next patch in this series uses it. I'm just splitting it up into a > separate patch so that it's digestible and I can provide more details > in the commit text. But you can not add build warnings, you know this :) > Couple of options: > 1. Drop the static in this patch and add it back when it's used in patch 18/18. > 2. Drop the commit text and squash this with 18/18 if you think the > function documentation is clear enough and it won't make patch 18/18 > too hard to review. It is hard to review new functions when you do not see them being used, otherwise you have to flip back and forth between patches, which is difficult. Add the functions, and use them, in the same patch. Otherwise we have no idea _HOW_ you are using them, or even if you end up using them at all. thanks, greg k-h
On Thu, Nov 5, 2020 at 11:23 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Nov 05, 2020 at 03:32:05PM -0800, Saravana Kannan wrote: > > On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote: > > > > Add helper functions __fw_devlink_link_to_consumers() and > > > > __fw_devlink_link_to_suppliers() that convert fwnode links to device > > > > links. > > > > > > > > __fw_devlink_link_to_consumers() is for creating: > > > > - Device links between a newly added device and all its consumer devices > > > > that have been added to driver core. > > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > > > the parent devices of all its consumers that have not been added to > > > > driver core yet. > > > > > > > > __fw_devlink_link_to_suppliers() is for creating: > > > > - Device links between a newly added device and all its supplier devices > > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > > > all the supplier devices of its child device nodes. > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > Did you just add build warnings with these static functions that no one > > > calls? > > > > The next patch in this series uses it. I'm just splitting it up into a > > separate patch so that it's digestible and I can provide more details > > in the commit text. > > But you can not add build warnings, you know this :) I know I can't break the build because that'll screw git bisect. But I thought warning that's fixed later in the series might be okay if it helps readability. I know now :) > > > Couple of options: > > 1. Drop the static in this patch and add it back when it's used in patch 18/18. > > 2. Drop the commit text and squash this with 18/18 if you think the > > function documentation is clear enough and it won't make patch 18/18 > > too hard to review. > > It is hard to review new functions when you do not see them being used, > otherwise you have to flip back and forth between patches, which is > difficult. > > Add the functions, and use them, in the same patch. Otherwise we have > no idea _HOW_ you are using them, or even if you end up using them at > all. Sounds good. I'll squash them. -Saravana
On Thu, Nov 5, 2020 at 9:09 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Saravana, > > Thank you for working on this ! > > On Wed, Nov 04, 2020 at 03:23:37PM -0800, Saravana Kannan wrote: > > The current implementation of fw_devlink is very inefficient because it > > tries to get away without creating fwnode links in the name of saving > > memory usage. Past attempts to optimize runtime at the cost of memory > > usage were blocked with request for data showing that the optimization > > made significant improvement for real world scenarios. > > > > We have those scenarios now. There have been several reports of boot > > time increase in the order of seconds in this thread [1]. Several OEMs > > and SoC manufacturers have also privately reported significant > > (350-400ms) increase in boot time due to all the parsing done by > > fw_devlink. > > > > So this patch series refactors fw_devlink to be more efficient. The key > > difference now is the addition of support for fwnode links -- just a few > > simple APIs. This also allows most of the code to be moved out of > > firmware specific (DT mostly) code into driver core. > > > > This brings the following benefits: > > - Instead of parsing the device tree multiple times (complexity was > > close to O(N^3) where N in the number of properties) during bootup, > > fw_devlink parses each fwnode node/property only once and creates > > fwnode links. The rest of the fw_devlink code then just looks at these > > fwnode links to do rest of the work. > > > > - Makes it much easier to debug probe issue due to fw_devlink in the > > future. fw_devlink=on blocks the probing of devices if they depend on > > a device that hasn't been added yet. With this refactor, it'll be very > > easy to tell what that device is because we now have a reference to > > the fwnode of the device. > > > > - Much easier to add fw_devlink support to ACPI and other firmware > > types. A refactor to move the common bits from DT specific code to > > driver core was in my TODO list as a prerequisite to adding ACPI > > support to fw_devlink. This series gets that done. > > > > Tomi/Laurent/Grygorii, > > > > If you can test this series, that'd be great! > > I gave it a try, rebasing my branch from v5.9 to v5.10-rc2 first. On > v5.10-rc2 the kernel dies when booting due to a deadlock (reported by > lockdep, so hopefully not too hard to debug). *sigh*. Fortunately, it > dies after the fw_devlink initialization, so I can still report results. Phew! For a sec I thought you said fw_devlink was causing a deadlock. > > Before your series: > > [ 0.743065] cpuidle: using governor menu > [ 13.350259] No ATAGs? > > With your series applied: > > [ 0.722670] cpuidle: using governor menu > [ 1.135859] No ATAGs? > > That's a very clear improvement :-) Thanks for testing. Great to hear it's helping! > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'll add it to my v2 series. -Saravana
On 06/11/2020 10:36, Saravana Kannan wrote: > On Thu, Nov 5, 2020 at 9:09 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Hi Saravana, >> >> Thank you for working on this ! >> >> On Wed, Nov 04, 2020 at 03:23:37PM -0800, Saravana Kannan wrote: >>> The current implementation of fw_devlink is very inefficient because it >>> tries to get away without creating fwnode links in the name of saving >>> memory usage. Past attempts to optimize runtime at the cost of memory >>> usage were blocked with request for data showing that the optimization >>> made significant improvement for real world scenarios. >>> >>> We have those scenarios now. There have been several reports of boot >>> time increase in the order of seconds in this thread [1]. Several OEMs >>> and SoC manufacturers have also privately reported significant >>> (350-400ms) increase in boot time due to all the parsing done by >>> fw_devlink. >>> >>> So this patch series refactors fw_devlink to be more efficient. The key >>> difference now is the addition of support for fwnode links -- just a few >>> simple APIs. This also allows most of the code to be moved out of >>> firmware specific (DT mostly) code into driver core. >>> >>> This brings the following benefits: >>> - Instead of parsing the device tree multiple times (complexity was >>> close to O(N^3) where N in the number of properties) during bootup, >>> fw_devlink parses each fwnode node/property only once and creates >>> fwnode links. The rest of the fw_devlink code then just looks at these >>> fwnode links to do rest of the work. >>> >>> - Makes it much easier to debug probe issue due to fw_devlink in the >>> future. fw_devlink=on blocks the probing of devices if they depend on >>> a device that hasn't been added yet. With this refactor, it'll be very >>> easy to tell what that device is because we now have a reference to >>> the fwnode of the device. >>> >>> - Much easier to add fw_devlink support to ACPI and other firmware >>> types. A refactor to move the common bits from DT specific code to >>> driver core was in my TODO list as a prerequisite to adding ACPI >>> support to fw_devlink. This series gets that done. >>> >>> Tomi/Laurent/Grygorii, >>> >>> If you can test this series, that'd be great! >> >> I gave it a try, rebasing my branch from v5.9 to v5.10-rc2 first. On >> v5.10-rc2 the kernel dies when booting due to a deadlock (reported by >> lockdep, so hopefully not too hard to debug). *sigh*. Fortunately, it >> dies after the fw_devlink initialization, so I can still report results. > > Phew! For a sec I thought you said fw_devlink was causing a deadlock. > >> >> Before your series: >> >> [ 0.743065] cpuidle: using governor menu >> [ 13.350259] No ATAGs? >> >> With your series applied: >> >> [ 0.722670] cpuidle: using governor menu >> [ 1.135859] No ATAGs? >> >> That's a very clear improvement :-) > > Thanks for testing. Great to hear it's helping! > >> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I'll add it to my v2 series. I've tried your series on top of 521b619acdc8 Merge tag 'linux-kselftest-kunit-fixes-5.10-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest on am571x-idk Before: [ 0.049395] cpuidle: using governor menu [ 1.654766] audit: type=2000 audit(0.040:1): state=initialized audit_enabled=0 res=1 [ 2.315266] No ATAGs? [ 2.315317] hw-breakpoint: found 5 (+1 reserved) breakpoint and 4 watchpoint registers. [ 2.315327] hw-breakpoint: maximum watchpoint size is 8 bytes. ... [ 6.549595] EXT4-fs (mmcblk0p2): mounted filesystem with ordered data mode. Opts: (null) [ 6.557794] VFS: Mounted root (ext4 filesystem) on device 179:26. [ 6.574103] devtmpfs: mounted [ 6.577749] Freeing unused kernel memory: 1024K [ 6.582433] Run /sbin/init as init process after: [ 0.049223] cpuidle: using governor menu [ 0.095893] audit: type=2000 audit(0.040:1): state=initialized audit_enabled=0 res=1 [ 0.102958] No ATAGs? [ 0.103010] hw-breakpoint: found 5 (+1 reserved) breakpoint and 4 watchpoint registers. [ 0.103020] hw-breakpoint: maximum watchpoint size is 8 bytes. ... [ 3.518623] EXT4-fs (mmcblk0p2): mounted filesystem with ordered data mode. Opts: (null) [ 3.526822] VFS: Mounted root (ext4 filesystem) on device 179:26. [ 3.543128] devtmpfs: mounted [ 3.546781] Freeing unused kernel memory: 1024K [ 3.551463] Run /sbin/init as init process So, it's much better. Thank you. Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > This patch adds support for creating supplier-consumer links between Generally speaking the "This patch" part is redundant. It is sufficient to simply say "Add ...". > fwnode. fwnodes (plural)? > It is intentionally kept simple and with limited APIs as it is > meant to be used only by driver core and firmware code (Eg: device tree, > ACPI, etc). I'd say "It is intended for internal use in the driver core and generic firmware support code (eg. Device Tree, ACPI), so it is simple by design and the API provided by it is limited." > > We can expand the APIs later if there is ever a need for > drivers/frameworks to start using them. The above is totally redundant IMO. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/base/core.c | 95 ++++++++++++++++++++++++++++++++++++++++++ > drivers/of/dynamic.c | 1 + > include/linux/fwnode.h | 14 +++++++ > 3 files changed, 110 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 31a76159f118..1a1d9a55645c 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -50,6 +50,101 @@ static LIST_HEAD(wait_for_suppliers); > static DEFINE_MUTEX(wfs_lock); > static LIST_HEAD(deferred_sync); > static unsigned int defer_sync_state_count = 1; > +static DEFINE_MUTEX(fwnode_link_lock); > + > +/** > + * fwnode_link_add - Create a link between two fwnode_handles. > + * @con: Consumer end of the link. > + * @sup: Supplier end of the link. > + * > + * Creates a fwnode link between two fwnode_handles. These fwnode links are Why don't you refer to the arguments here, that is "Create a link between fwnode handles @con and @sup ..." > + * used by the driver core to automatically generate device links. Attempts to > + * create duplicate links are simply ignored and there is no refcounting. And I'd generally write it this way: "Create a link between fwnode handles @con and @sup representing a pair of devices the first of which uses certain resources provided by the second one, respectively. The driver core will use that link to create a device link between the two device objects corresponding to @con and @sup when they are created and it will automatically delete the link between @con and @sup after doing that. Attempts to create a duplicate link between the same pair of fwnode handles are ignored and there is no reference counting." > + * > + * These links are automatically deleted once they are converted to device > + * links or when the fwnode_handles (or their corresponding devices) are > + * deleted. > + */ > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) Why doesn't it return a pointer to the new link or NULL? That would be consistent with device_link_add(). > +{ > + struct fwnode_link *link; > + int ret = 0; > + > + mutex_lock(&fwnode_link_lock); > + > + /* Duplicate requests are intentionally not refcounted. */ Is this comment really necessary? > + list_for_each_entry(link, &sup->consumers, s_hook) > + if (link->consumer == con) > + goto out; It is also necessary to look the other way around AFAICS, that is if there is a link between the two fwnode handles in the other direction already, the creation of a new one should fail. > + > + link = kzalloc(sizeof(*link), GFP_KERNEL); > + if (!link) { > + ret = -ENOMEM; > + goto out; > + } > + > + link->supplier = sup; > + INIT_LIST_HEAD(&link->s_hook); > + link->consumer = con; > + INIT_LIST_HEAD(&link->c_hook); > + > + list_add(&link->s_hook, &sup->consumers); > + list_add(&link->c_hook, &con->suppliers); > +out: > + mutex_unlock(&fwnode_link_lock); > + > + return ret; > +} > + > +/** > + * fwnode_links_purge_suppliers - Delete all supplier links of fwnode_handle. > + * @fwnode: fwnode whose supplier links needs to be deleted s/needs/need/ > + * > + * Deletes all supplier links connecting directly to a fwnode. I'd say "Delete all supplier links connecting directly to @fwnode." and analogously below. > + */ > +static void fwnode_links_purge_suppliers(struct fwnode_handle *fwnode) > +{ > + struct fwnode_link *link, *tmp; > + > + mutex_lock(&fwnode_link_lock); > + list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) { > + list_del(&link->s_hook); > + list_del(&link->c_hook); > + kfree(link); > + } > + mutex_unlock(&fwnode_link_lock); > +} > + > +/** > + * fwnode_links_purge_consumers - Delete all consumer links of fwnode_handle. > + * @fwnode: fwnode whose consumer links needs to be deleted > + * > + * Deletes all consumer links connecting directly to a fwnode. > + */ > +static void fwnode_links_purge_consumers(struct fwnode_handle *fwnode) > +{ > + struct fwnode_link *link, *tmp; > + > + mutex_lock(&fwnode_link_lock); > + list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) { > + list_del(&link->s_hook); > + list_del(&link->c_hook); > + kfree(link); I'd avoid the code duplication, even though it doesn't appear to be significant ATM. > + } > + mutex_unlock(&fwnode_link_lock); > +} > + > +/** > + * fwnode_links_purge - Delete all links connected to a fwnode_handle. > + * @fwnode: fwnode whose links needs to be deleted > + * > + * Deletes all links connecting directly to a fwnode. > + */ > +void fwnode_links_purge(struct fwnode_handle *fwnode) > +{ > + fwnode_links_purge_suppliers(fwnode); Dropping the lock here may turn out to be problematic at one point going forward. IMO it is better to hold it throughout the entire operation. > + fwnode_links_purge_consumers(fwnode); I'd get rid of the two functions above, add something like fwnode_link_del() and walk the lists directly here calling it for every link on the way. > +} > > #ifdef CONFIG_SRCU > static DEFINE_MUTEX(device_links_lock); > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c > index fe64430b438a..9a824decf61f 100644 > --- a/drivers/of/dynamic.c > +++ b/drivers/of/dynamic.c > @@ -356,6 +356,7 @@ void of_node_release(struct kobject *kobj) > > property_list_free(node->properties); > property_list_free(node->deadprops); > + fwnode_links_purge(of_fwnode_handle(node)); > > kfree(node->full_name); > kfree(node->data); > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > index 593fb8e58f21..afde643f37a2 100644 > --- a/include/linux/fwnode.h > +++ b/include/linux/fwnode.h > @@ -10,6 +10,7 @@ > #define _LINUX_FWNODE_H_ > > #include <linux/types.h> > +#include <linux/list.h> > > struct fwnode_operations; > struct device; > @@ -18,6 +19,15 @@ struct fwnode_handle { > struct fwnode_handle *secondary; > const struct fwnode_operations *ops; > struct device *dev; > + struct list_head suppliers; > + struct list_head consumers; > +}; > + > +struct fwnode_link { > + struct fwnode_handle *supplier; > + struct list_head s_hook; > + struct fwnode_handle *consumer; > + struct list_head c_hook; > }; > > /** > @@ -173,8 +183,12 @@ static inline void fwnode_init(struct fwnode_handle *fwnode, > const struct fwnode_operations *ops) > { > fwnode->ops = ops; > + INIT_LIST_HEAD(&fwnode->consumers); > + INIT_LIST_HEAD(&fwnode->suppliers); > } > > extern u32 fw_devlink_get_flags(void); > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup); > +void fwnode_links_purge(struct fwnode_handle *fwnode); > > #endif > -- > 2.29.1.341.ge80a0c044ae-goog >
On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > SYNC_STATE_ONLY device links only affect the behavior of sync_state() > callbacks. Specifically, they prevent sync_state() only callbacks from > being called on a device if one or more of its consumers haven't probed. > > So, creating a SYNC_STATE_ONLY device link from an already probed > consumer is useless. So, don't allow creating such device links. I'm wondering why this needs to be part of the series? It looks like it could go in separately, couldn't it? > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/base/core.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 1a1d9a55645c..4a0907574646 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -646,6 +646,17 @@ struct device_link *device_link_add(struct device *consumer, > goto out; > } > > + /* > + * SYNC_STATE_ONLY links are useless once a consumer device has probed. > + * So, only create it if the consumer hasn't probed yet. > + */ > + if (flags & DL_FLAG_SYNC_STATE_ONLY && > + consumer->links.status != DL_DEV_NO_DRIVER && > + consumer->links.status != DL_DEV_PROBING) { > + link = NULL; > + goto out; > + } Returning NULL at this point may be confusing if there is a link between these devices already. > + > /* > * DL_FLAG_AUTOREMOVE_SUPPLIER indicates that the link will be needed > * longer than for DL_FLAG_AUTOREMOVE_CONSUMER and setting them both > -- > 2.29.1.341.ge80a0c044ae-goog >
On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > This function is a wrapper around fwnode_operations.add_links(). > > This function parses each node in a fwnode tree and create fwnode links > for each of those nodes. The information for creating the fwnode links > (the supplier and consumer fwnode) is obtained by parsing the properties > in each of the fwnodes. > > This function also ensures that no fwnode is parsed more than once by > marking the fwnodes as parsed. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/base/core.c | 19 +++++++++++++++++++ > include/linux/fwnode.h | 3 +++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 4a0907574646..ee28d8c7ee85 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -1543,6 +1543,25 @@ static bool fw_devlink_is_permissive(void) > return fw_devlink_flags == DL_FLAG_SYNC_STATE_ONLY; > } > > +static void fw_devlink_parse_fwnode(struct fwnode_handle *fwnode) > +{ > + if (fwnode->flags & FWNODE_FLAG_LINKS_ADDED) > + return; Why is the flag needed? Duplicate links won't be created anyway and it doesn't cause the tree walk to be terminated. > + > + fwnode_call_int_op(fwnode, add_links, NULL); > + fwnode->flags |= FWNODE_FLAG_LINKS_ADDED; > +} > + > +static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode) > +{ > + struct fwnode_handle *child = NULL; > + > + fw_devlink_parse_fwnode(fwnode); > + > + while ((child = fwnode_get_next_available_child_node(fwnode, child))) I'd prefer for (child = NULL; child; child = fwnode_get_next_available_child_node(fwnode, child)) > + fw_devlink_parse_fwtree(child); > +} > + > static void fw_devlink_link_device(struct device *dev) > { > int fw_ret; > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > index ec02e1e939cc..9aaf9e4f3994 100644 > --- a/include/linux/fwnode.h > +++ b/include/linux/fwnode.h > @@ -15,12 +15,15 @@ > struct fwnode_operations; > struct device; > Description here, please. > +#define FWNODE_FLAG_LINKS_ADDED BIT(0) > + > struct fwnode_handle { > struct fwnode_handle *secondary; > const struct fwnode_operations *ops; > struct device *dev; > struct list_head suppliers; > struct list_head consumers; > + u32 flags; That's a bit wasteful. Maybe u8 would suffice for the time being? > }; > > struct fwnode_link { > -- > 2.29.1.341.ge80a0c044ae-goog >
On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > Given a fwnode, this function finds the closest ancestor fwnode that has > a corresponding struct device. The function returns this struct device. > This function will be used in a subsequent patch in this series. > > Signed-off-by: Saravana Kannan <saravanak@google.com> I would combine this one with patch [10/18]. > --- > drivers/base/core.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index ee28d8c7ee85..4ae5f2885ac5 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -1562,6 +1562,31 @@ static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode) > fw_devlink_parse_fwtree(child); > } > > +/** > + * fwnode_get_next_parent_dev - Find device of closest ancestor fwnode > + * @fwnode: firmware node > + * > + * Given a firmware node (@fwnode), this function finds its closest ancestor > + * firmware node that has a corresponding struct device and returns that struct > + * device. > + * > + * The caller of this function is expected to call put_device() on the returned > + * device when they are done. > + */ > +static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) > +{ > + struct device *dev = NULL; > + > + fwnode_handle_get(fwnode); > + do { > + fwnode = fwnode_get_next_parent(fwnode); > + if (fwnode) > + dev = get_dev_from_fwnode(fwnode); > + } while (fwnode && !dev); > + fwnode_handle_put(fwnode); > + return dev; > +} > + > static void fw_devlink_link_device(struct device *dev) > { > int fw_ret; > -- > 2.29.1.341.ge80a0c044ae-goog >
On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > To check if a device is still waiting for its supplier devices to be > added, we used to check if the devices is in a global > waiting_for_suppliers list. Since the global list will be deleted in > subsequent patches, this patch stops using this check. My kind of educated guess is that you want to drop waiting_for_suppliers and that's why you want to use supplier links here. > > Instead, this patch uses a more device specific check. It checks if the > device's fwnode has any fwnode links that haven't been converted to > device links yet. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/base/core.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 4ae5f2885ac5..d51dd564add1 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -51,6 +51,7 @@ static DEFINE_MUTEX(wfs_lock); > static LIST_HEAD(deferred_sync); > static unsigned int defer_sync_state_count = 1; > static DEFINE_MUTEX(fwnode_link_lock); > +static bool fw_devlink_is_permissive(void); > > /** > * fwnode_link_add - Create a link between two fwnode_handles. > @@ -994,13 +995,13 @@ int device_links_check_suppliers(struct device *dev) > * Device waiting for supplier to become available is not allowed to > * probe. > */ > - mutex_lock(&wfs_lock); > - if (!list_empty(&dev->links.needs_suppliers) && > - dev->links.need_for_probe) { > - mutex_unlock(&wfs_lock); > + mutex_lock(&fwnode_link_lock); > + if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) && > + !fw_devlink_is_permissive()) { > + mutex_unlock(&fwnode_link_lock); > return -EPROBE_DEFER; > } > - mutex_unlock(&wfs_lock); > + mutex_unlock(&fwnode_link_lock); > > device_links_write_lock(); > > @@ -1166,10 +1167,7 @@ static ssize_t waiting_for_supplier_show(struct device *dev, > bool val; > > device_lock(dev); > - mutex_lock(&wfs_lock); > - val = !list_empty(&dev->links.needs_suppliers) > - && dev->links.need_for_probe; > - mutex_unlock(&wfs_lock); Why isn't the lock needed any more? Or maybe it wasn't needed previously too? > + val = !list_empty(&dev->fwnode->suppliers); > device_unlock(dev); > return sysfs_emit(buf, "%u\n", val); > } > @@ -2226,7 +2224,7 @@ static int device_add_attrs(struct device *dev) > goto err_remove_dev_groups; > } > > - if (fw_devlink_flags && !fw_devlink_is_permissive()) { > + if (fw_devlink_flags && !fw_devlink_is_permissive() && dev->fwnode) { And why is this change needed? > error = device_create_file(dev, &dev_attr_waiting_for_supplier); > if (error) > goto err_remove_dev_online; > -- > 2.29.1.341.ge80a0c044ae-goog >
On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > Add helper functions __fw_devlink_link_to_consumers() and > __fw_devlink_link_to_suppliers() that convert fwnode links to device > links. > > __fw_devlink_link_to_consumers() is for creating: > - Device links between a newly added device and all its consumer devices > that have been added to driver core. > - Proxy SYNC_STATE_ONLY device links between the newly added device and > the parent devices of all its consumers that have not been added to > driver core yet. > > __fw_devlink_link_to_suppliers() is for creating: > - Device links between a newly added device and all its supplier devices > - Proxy SYNC_STATE_ONLY device links between the newly added device and > all the supplier devices of its child device nodes. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/base/core.c | 228 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 228 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index d51dd564add1..0c87ff949d81 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -1585,6 +1585,234 @@ static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) > return dev; > } > > +/** > + * fw_devlink_create_devlink - Create a device link from a consumer to fwnode Have you considered renaming "fw_devlink" to "fwnode_link"? That would be much less confusing IMO and the name of this function would be clearer. > + * @con - Consumer device for the device link > + * @sup - fwnode handle of supplier > + * > + * This function will try to create a device link between the consumer and the > + * supplier devices. "... between consumer device @con and the supplier device represented by @sup" (and see below). > + * > + * The supplier has to be provided as a fwnode because incorrect cycles in > + * fwnode links can sometimes cause the supplier device to never be created. > + * This function detects such cases and returns an error if a device link being > + * created in invalid. "... returns an error if it cannot create a device link from the consumer to a missing supplier." > + * > + * Returns, > + * 0 on successfully creating a device link > + * -EINVAL if the device link being attempted is invalid "if the device link cannot be created as expected" > + * -EAGAIN if the device link needs to be attempted again in the future "if the device link cannot be created right now, but it may be possible to do that in the future." > + */ > +static int fw_devlink_create_devlink(struct device *con, > + struct fwnode_handle *sup, u32 flags) I'd call the second arg sup_handle to indicate that it is not a struct device. > +{ > + struct device *sup_dev, *sup_par_dev; > + int ret = 0; > + > + sup_dev = get_dev_from_fwnode(sup); > + /* > + * If we can't find the supplier device from its fwnode, it might be > + * due to a cyclic dependcy between fwnodes. Some of these cycles can dependency > + * be broken by applying logic. Check for these types of cycles and > + * break them so that devices in the cycle probe properly. > + */ I would do if (sup_dev) { if (!device_link_add(con, sup_dev, flags)) ret = -EINVAL; put_device(sup_dev); return ret; } here and the cycle detection (error code path) below, possibly using the same dev pointer. > + if (!sup_dev) { > + /* > + * DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports > + * cycles. So cycle detection isn't necessary and shouldn't be > + * done. > + */ > + if (flags & DL_FLAG_SYNC_STATE_ONLY) > + return -EAGAIN; > + > + sup_par_dev = fwnode_get_next_parent_dev(sup); > + > + /* > + * If the supplier's parent is dependent on the consumer, then > + * the consumer-supplier dependency is a false dependency. So, > + * treat it as an invalid link. > + */ > + if (sup_par_dev && device_is_dependent(con, sup_par_dev)) { > + dev_dbg(con, "Not linking to %pfwP - False link\n", > + sup); > + ret = -EINVAL; > + } else { > + /* > + * Can't check for cycles or no cycles. So let's try > + * again later. > + */ > + ret = -EAGAIN; > + } > + > + put_device(sup_par_dev); > + return ret; > + } > + > + /* > + * If we get this far and fail, this is due to cycles in device links. > + * Just give up on this link and treat it as invalid. > + */ > + if (!device_link_add(con, sup_dev, flags)) > + ret = -EINVAL; > + put_device(sup_dev); > + > + return ret; > +} > + > +/** > + * __fw_devlink_link_to_consumers - Create device links to consumers of a device > + * @dev - Device that needs to be linked to its consumers > + * > + * This function looks at all the consumer fwnodes of @dev and creates device > + * links between the consumer device and @dev (supplier). > + * > + * If the consumer device has not been added yet, then this function creates a > + * SYNC_STATE_ONLY link between @dev (supplier) and the closest ancestor device > + * of the consumer fwnode. This is necessary to make sure @dev doesn't get a > + * sync_state() callback before the real consumer device gets to be added and > + * then probed. > + * > + * Once device links are created from the real consumer to @dev (supplier), the > + * fwnode links are deleted. > + */ > +static void __fw_devlink_link_to_consumers(struct device *dev) > +{ > + struct fwnode_handle *fwnode = dev->fwnode; > + struct fwnode_link *link, *tmp; > + > + list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) { > + u32 dl_flags = fw_devlink_get_flags(); > + struct device *con_dev; > + bool own_link = true; > + int ret; > + > + con_dev = get_dev_from_fwnode(link->consumer); > + /* > + * If consumer device is not available yet, make a "proxy" > + * SYNC_STATE_ONLY link from the consumer's parent device to > + * the supplier device. This is necessary to make sure the > + * supplier doesn't get a sync_state() callback before the real > + * consumer can create a device link to the supplier. > + * > + * This proxy link step is needed to handle the case where the > + * consumer's parent device is added before the supplier. > + */ > + if (!con_dev) { > + con_dev = fwnode_get_next_parent_dev(link->consumer); > + /* > + * However, if the consumer's parent device is also the > + * parent of the supplier, don't create a > + * consumer-supplier link from the parent to its child > + * device. Such a dependency is impossible. > + */ > + if (con_dev && > + fwnode_is_ancestor_of(con_dev->fwnode, fwnode)) { > + put_device(con_dev); > + con_dev = NULL; > + } else { > + own_link = false; > + dl_flags = DL_FLAG_SYNC_STATE_ONLY; > + } > + } > + > + if (!con_dev) > + continue; > + > + ret = fw_devlink_create_devlink(con_dev, fwnode, dl_flags); > + put_device(con_dev); > + if (!own_link || ret == -EAGAIN) > + continue; > + > + list_del(&link->s_hook); > + list_del(&link->c_hook); > + kfree(link); > + } > +} > + > +/** > + * __fw_devlink_link_to_suppliers - Create device links to suppliers of a device > + * @dev - The consumer device that needs to be linked to its suppliers > + * @fwnode - Root of the fwnode tree that is used to create device links > + * > + * This function looks at all the supplier fwnodes of fwnode tree rooted at > + * @fwnode and creates device links between @dev (consumer) and all the > + * supplier devices of the entire fwnode tree at @fwnode. See > + * fw_devlink_create_devlink() for more details. > + * > + * The function creates normal (non-SYNC_STATE_ONLY) device links between @dev > + * and the real suppliers of @dev. Once these device links are created, the > + * fwnode links are deleted. When such device links are successfully created, > + * this function is called recursively on those supplier devices. This is > + * needed to detect and break some invalid cycles in fwnode links. > + * > + * In addition, it also looks at all the suppliers of the entire fwnode tree > + * because some of the child devices of @dev that have not been added yet > + * (because @dev hasn't probed) might already have their suppliers added to > + * driver core. So, this function creates SYNC_STATE_ONLY device links between > + * @dev (consumer) and these suppliers to make sure they don't execute their > + * sync_state() callbacks before these child devices have a chance to create > + * their device links. The fwnode links that correspond to the child devices > + * aren't delete because they are needed later to create the device links > + * between the real consumer and supplier devices. > + */ > +static void __fw_devlink_link_to_suppliers(struct device *dev, > + struct fwnode_handle *fwnode) > +{ > + bool own_link = (dev->fwnode == fwnode); > + struct fwnode_link *link, *tmp; > + struct fwnode_handle *child = NULL; > + u32 dl_flags; > + > + if (own_link) > + dl_flags = fw_devlink_get_flags(); > + else > + dl_flags = DL_FLAG_SYNC_STATE_ONLY; > + > + list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) { > + int ret; > + struct device *sup_dev; > + struct fwnode_handle *sup = link->supplier; > + > + ret = fw_devlink_create_devlink(dev, sup, dl_flags); > + if (!own_link || ret == -EAGAIN) > + continue; > + > + list_del(&link->s_hook); > + list_del(&link->c_hook); > + kfree(link); > + > + /* If no device link was created, nothing more to do. */ > + if (ret) > + continue; > + > + /* > + * If a device link was successfully created to a supplier, we > + * now need to try and link the supplier to all its suppliers. > + * > + * This is needed to detect and delete false dependencies in > + * fwnode links that haven't been converted to a device link > + * yet. See comments in fw_devlink_create_devlink() for more > + * details on the false dependency. > + * > + * Without deleting these false dependencies, some devices will > + * never probe because they'll keep waiting for their false > + * dependency fwnode links to be converted to device links. > + */ > + sup_dev = get_dev_from_fwnode(sup); > + __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode); > + put_device(sup_dev); > + } > + > + /* > + * Make "proxy" SYNC_STATE_ONLY device links to represent the needs of > + * all the descendants. This proxy link step is needed to handle the > + * case where the supplier is added before the consumer's parent device > + * (@dev). > + */ > + while ((child = fwnode_get_next_available_child_node(fwnode, child))) > + __fw_devlink_link_to_suppliers(dev, child); > +} > + > static void fw_devlink_link_device(struct device *dev) > { > int fw_ret; > -- > 2.29.1.341.ge80a0c044ae-goog >
On Mon, Nov 16, 2020 at 7:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > > This patch adds support for creating supplier-consumer links between > > Generally speaking the "This patch" part is redundant. It is > sufficient to simply say "Add ...". > > > fwnode. > > fwnodes (plural)? > > > It is intentionally kept simple and with limited APIs as it is > > meant to be used only by driver core and firmware code (Eg: device tree, > > ACPI, etc). > > I'd say "It is intended for internal use in the driver core and > generic firmware support code (eg. Device Tree, ACPI), so it is simple > by design and the API provided by it is limited." > > > > > We can expand the APIs later if there is ever a need for > > drivers/frameworks to start using them. > > The above is totally redundant IMO. > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/base/core.c | 95 ++++++++++++++++++++++++++++++++++++++++++ > > drivers/of/dynamic.c | 1 + > > include/linux/fwnode.h | 14 +++++++ > > 3 files changed, 110 insertions(+) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index 31a76159f118..1a1d9a55645c 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -50,6 +50,101 @@ static LIST_HEAD(wait_for_suppliers); > > static DEFINE_MUTEX(wfs_lock); > > static LIST_HEAD(deferred_sync); > > static unsigned int defer_sync_state_count = 1; > > +static DEFINE_MUTEX(fwnode_link_lock); > > + > > +/** > > + * fwnode_link_add - Create a link between two fwnode_handles. > > + * @con: Consumer end of the link. > > + * @sup: Supplier end of the link. > > + * > > + * Creates a fwnode link between two fwnode_handles. These fwnode links are > > Why don't you refer to the arguments here, that is "Create a link > between fwnode handles @con and @sup ..." Ack/done to everything above. > > > + * used by the driver core to automatically generate device links. Attempts to > > + * create duplicate links are simply ignored and there is no refcounting. > > And I'd generally write it this way: > > "Create a link between fwnode handles @con and @sup representing a > pair of devices the first of which uses certain resources provided by > the second one, respectively. > > The driver core will use that link to create a device link between the > two device objects corresponding to @con and @sup when they are > created and it will automatically delete the link between @con and > @sup after doing that. > > Attempts to create a duplicate link between the same pair of fwnode > handles are ignored and there is no reference counting." Took most of this as is with some minor rewording. > > > + * > > + * These links are automatically deleted once they are converted to device > > + * links or when the fwnode_handles (or their corresponding devices) are > > + * deleted. > > + */ > > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > > Why doesn't it return a pointer to the new link or NULL? > > That would be consistent with device_link_add(). However, as opposed to device_link_add(), I don't want the caller holding any reference to the allocated link. So I don't want to return any pointer to them. > > +{ > > + struct fwnode_link *link; > > + int ret = 0; > > + > > + mutex_lock(&fwnode_link_lock); > > + > > + /* Duplicate requests are intentionally not refcounted. */ > > Is this comment really necessary? I guess with the function comment explicitly stating "no ref counting", this is kind of redundant. I can remove this. > > > + list_for_each_entry(link, &sup->consumers, s_hook) > > + if (link->consumer == con) > > + goto out; > > It is also necessary to look the other way around AFAICS, that is if > there is a link between the two fwnode handles in the other direction > already, the creation of a new one should fail. No, fwnode links can have cycles. At this state, we can't tell which ones are invali.d When we create device links out of this, we have more info at that point and we make sure not to add any device links that can cause cycles. There are a bunch of corner cases where we can't tell which one is the invalid fwnode link in the links that make up the cycle and in those cases, we have to make the device links as SYNC_STATE_ONLY device links. Long story short, cycles are allowed. > > > + > > + link = kzalloc(sizeof(*link), GFP_KERNEL); > > + if (!link) { > > + ret = -ENOMEM; > > + goto out; > > + } > > + > > + link->supplier = sup; > > + INIT_LIST_HEAD(&link->s_hook); > > + link->consumer = con; > > + INIT_LIST_HEAD(&link->c_hook); > > + > > + list_add(&link->s_hook, &sup->consumers); > > + list_add(&link->c_hook, &con->suppliers); > > +out: > > + mutex_unlock(&fwnode_link_lock); > > + > > + return ret; > > +} > > + > > +/** > > + * fwnode_links_purge_suppliers - Delete all supplier links of fwnode_handle. > > + * @fwnode: fwnode whose supplier links needs to be deleted > > s/needs/need/ Ack > > > + * > > + * Deletes all supplier links connecting directly to a fwnode. > > I'd say "Delete all supplier links connecting directly to @fwnode." > and analogously below. Ack > > > + */ > > +static void fwnode_links_purge_suppliers(struct fwnode_handle *fwnode) > > +{ > > + struct fwnode_link *link, *tmp; > > + > > + mutex_lock(&fwnode_link_lock); > > + list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) { > > + list_del(&link->s_hook); > > + list_del(&link->c_hook); > > + kfree(link); > > + } > > + mutex_unlock(&fwnode_link_lock); > > +} > > + > > +/** > > + * fwnode_links_purge_consumers - Delete all consumer links of fwnode_handle. > > + * @fwnode: fwnode whose consumer links needs to be deleted > > + * > > + * Deletes all consumer links connecting directly to a fwnode. > > + */ > > +static void fwnode_links_purge_consumers(struct fwnode_handle *fwnode) > > +{ > > + struct fwnode_link *link, *tmp; > > + > > + mutex_lock(&fwnode_link_lock); > > + list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) { > > + list_del(&link->s_hook); > > + list_del(&link->c_hook); > > + kfree(link); > > I'd avoid the code duplication, even though it doesn't appear to be > significant ATM. I'd like to leave this as is for now if that's okay. > > + } > > + mutex_unlock(&fwnode_link_lock); > > +} > > + > > +/** > > + * fwnode_links_purge - Delete all links connected to a fwnode_handle. > > + * @fwnode: fwnode whose links needs to be deleted > > + * > > + * Deletes all links connecting directly to a fwnode. > > + */ > > +void fwnode_links_purge(struct fwnode_handle *fwnode) > > +{ > > + fwnode_links_purge_suppliers(fwnode); > > Dropping the lock here may turn out to be problematic at one point > going forward. IMO it is better to hold it throughout the entire > operation. It's not really a problem as there's nothing that can happen in between these two calls that can cause a problem but won't be a problem if it happens after these two calls. I was trying to avoid repeating the purge supplier/consumer code here again. Can we leave this as is for now? > > > + fwnode_links_purge_consumers(fwnode); > > I'd get rid of the two functions above, add something like > fwnode_link_del() and walk the lists directly here calling it for > every link on the way. I need a fwnode_links_purge_suppliers() (as in, not purging consumer links) though. I used it later in the series. So instead of repeating that code for fwnode_links_purge(), I created fwnode_links_purge_consumers() and called both functions from here. Can we leave this as is? -Saravana > > > +} > > > > #ifdef CONFIG_SRCU > > static DEFINE_MUTEX(device_links_lock); > > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c > > index fe64430b438a..9a824decf61f 100644 > > --- a/drivers/of/dynamic.c > > +++ b/drivers/of/dynamic.c > > @@ -356,6 +356,7 @@ void of_node_release(struct kobject *kobj) > > > > property_list_free(node->properties); > > property_list_free(node->deadprops); > > + fwnode_links_purge(of_fwnode_handle(node)); > > > > kfree(node->full_name); > > kfree(node->data); > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > > index 593fb8e58f21..afde643f37a2 100644 > > --- a/include/linux/fwnode.h > > +++ b/include/linux/fwnode.h > > @@ -10,6 +10,7 @@ > > #define _LINUX_FWNODE_H_ > > > > #include <linux/types.h> > > +#include <linux/list.h> > > > > struct fwnode_operations; > > struct device; > > @@ -18,6 +19,15 @@ struct fwnode_handle { > > struct fwnode_handle *secondary; > > const struct fwnode_operations *ops; > > struct device *dev; > > + struct list_head suppliers; > > + struct list_head consumers; > > +}; > > + > > +struct fwnode_link { > > + struct fwnode_handle *supplier; > > + struct list_head s_hook; > > + struct fwnode_handle *consumer; > > + struct list_head c_hook; > > }; > > > > /** > > @@ -173,8 +183,12 @@ static inline void fwnode_init(struct fwnode_handle *fwnode, > > const struct fwnode_operations *ops) > > { > > fwnode->ops = ops; > > + INIT_LIST_HEAD(&fwnode->consumers); > > + INIT_LIST_HEAD(&fwnode->suppliers); > > } > > > > extern u32 fw_devlink_get_flags(void); > > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup); > > +void fwnode_links_purge(struct fwnode_handle *fwnode); > > > > #endif > > -- > > 2.29.1.341.ge80a0c044ae-goog > >
On Mon, Nov 16, 2020 at 7:57 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > > SYNC_STATE_ONLY device links only affect the behavior of sync_state() > > callbacks. Specifically, they prevent sync_state() only callbacks from > > being called on a device if one or more of its consumers haven't probed. > > > > So, creating a SYNC_STATE_ONLY device link from an already probed > > consumer is useless. So, don't allow creating such device links. > > I'm wondering why this needs to be part of the series? > > It looks like it could go in separately, couldn't it? Right, I just wrote this as part of the series as I noticed this gap in the error checking as I wrote this series. It can go in separately. > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/base/core.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index 1a1d9a55645c..4a0907574646 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -646,6 +646,17 @@ struct device_link *device_link_add(struct device *consumer, > > goto out; > > } > > > > + /* > > + * SYNC_STATE_ONLY links are useless once a consumer device has probed. > > + * So, only create it if the consumer hasn't probed yet. > > + */ > > + if (flags & DL_FLAG_SYNC_STATE_ONLY && > > + consumer->links.status != DL_DEV_NO_DRIVER && > > + consumer->links.status != DL_DEV_PROBING) { > > + link = NULL; > > + goto out; > > + } > > Returning NULL at this point may be confusing if there is a link > between these devices already. But the request is for a SYNC_STATE_ONLY link that can't be created when this condition is met. I see it similar to the error check above. I think returning the existing non-SYNC_STATE_ONLY link gives the wrong impression that the link was created successfully. Also, if I find the existing link and return it, then I need to refcount it (conditional on STATELESS?) and the caller who shouldn't be trying to create this link should now need to keep track of this and release it too. I think it's cleaner and simpler to just return NULL. -Saravana
On Mon, Nov 16, 2020 at 8:25 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > > This function is a wrapper around fwnode_operations.add_links(). > > > > This function parses each node in a fwnode tree and create fwnode links > > for each of those nodes. The information for creating the fwnode links > > (the supplier and consumer fwnode) is obtained by parsing the properties > > in each of the fwnodes. > > > > This function also ensures that no fwnode is parsed more than once by > > marking the fwnodes as parsed. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/base/core.c | 19 +++++++++++++++++++ > > include/linux/fwnode.h | 3 +++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index 4a0907574646..ee28d8c7ee85 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -1543,6 +1543,25 @@ static bool fw_devlink_is_permissive(void) > > return fw_devlink_flags == DL_FLAG_SYNC_STATE_ONLY; > > } > > > > +static void fw_devlink_parse_fwnode(struct fwnode_handle *fwnode) > > +{ > > + if (fwnode->flags & FWNODE_FLAG_LINKS_ADDED) > > + return; > > Why is the flag needed? > > Duplicate links won't be created anyway and it doesn't cause the tree > walk to be terminated. To avoid parsing a fwnode more than once. The cumulative impact of the repeated parsing is actually quite high. And I intentionally didn't do this check at the tree walk level because DT overlay can add/remove/change individual fwnodes and I want to reparse those when they are added while avoiding parsing other nodes that have already been parsed and not changed by DT overlay. > > > + > > + fwnode_call_int_op(fwnode, add_links, NULL); > > + fwnode->flags |= FWNODE_FLAG_LINKS_ADDED; > > +} > > + > > +static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode) > > +{ > > + struct fwnode_handle *child = NULL; > > + > > + fw_devlink_parse_fwnode(fwnode); > > + > > + while ((child = fwnode_get_next_available_child_node(fwnode, child))) > > I'd prefer > > for (child = NULL; child; child = > fwnode_get_next_available_child_node(fwnode, child)) I was about to change to this and then realized it won't work. It would have to be for (child = fwnode_get_next_available_child_node(fwnode, NULL)); child; child = fwnode_get_next_available_child_node(fwnode, child)) Is that really better? The while() seems a lot more readable to me. I don't have a strong opinion, so I'll go with whatever you say after reading this. > > > + fw_devlink_parse_fwtree(child); > > +} > > + > > static void fw_devlink_link_device(struct device *dev) > > { > > int fw_ret; > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > > index ec02e1e939cc..9aaf9e4f3994 100644 > > --- a/include/linux/fwnode.h > > +++ b/include/linux/fwnode.h > > @@ -15,12 +15,15 @@ > > struct fwnode_operations; > > struct device; > > > > Description here, please. Ack > > > +#define FWNODE_FLAG_LINKS_ADDED BIT(0) > > + > > struct fwnode_handle { > > struct fwnode_handle *secondary; > > const struct fwnode_operations *ops; > > struct device *dev; > > struct list_head suppliers; > > struct list_head consumers; > > + u32 flags; > > That's a bit wasteful. Maybe u8 would suffice for the time being? Ack. -Saravana
On Mon, Nov 16, 2020 at 8:27 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > > Given a fwnode, this function finds the closest ancestor fwnode that has > > a corresponding struct device. The function returns this struct device. > > This function will be used in a subsequent patch in this series. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > I would combine this one with patch [10/18]. Ack. -Saravana -Saravana > > > --- > > drivers/base/core.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index ee28d8c7ee85..4ae5f2885ac5 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -1562,6 +1562,31 @@ static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode) > > fw_devlink_parse_fwtree(child); > > } > > > > +/** > > + * fwnode_get_next_parent_dev - Find device of closest ancestor fwnode > > + * @fwnode: firmware node > > + * > > + * Given a firmware node (@fwnode), this function finds its closest ancestor > > + * firmware node that has a corresponding struct device and returns that struct > > + * device. > > + * > > + * The caller of this function is expected to call put_device() on the returned > > + * device when they are done. > > + */ > > +static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) > > +{ > > + struct device *dev = NULL; > > + > > + fwnode_handle_get(fwnode); > > + do { > > + fwnode = fwnode_get_next_parent(fwnode); > > + if (fwnode) > > + dev = get_dev_from_fwnode(fwnode); > > + } while (fwnode && !dev); > > + fwnode_handle_put(fwnode); > > + return dev; > > +} > > + > > static void fw_devlink_link_device(struct device *dev) > > { > > int fw_ret; > > -- > > 2.29.1.341.ge80a0c044ae-goog > >