Messages in this thread | | | From | Saravana Kannan <> | Date | Mon, 18 Jul 2022 18:51:29 -0700 | Subject | Re: [PATCH v4] amba: Remove deferred device addition |
| |
On Tue, Jul 12, 2022 at 11:53 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi Saravana, > > On 12.07.2022 21:38, Saravana Kannan wrote: > > On Tue, Jul 12, 2022 at 5:34 AM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> On 12.07.2022 14:25, Marek Szyprowski wrote: > >>> On 05.07.2022 10:39, Saravana Kannan wrote: > >>>> The uevents generated for an amba device need PID and CID information > >>>> that's available only when the amba device is powered on, clocked and > >>>> out of reset. So, if those resources aren't available, the information > >>>> can't be read to generate the uevents. To workaround this requirement, > >>>> if the resources weren't available, the device addition was deferred and > >>>> retried periodically. > >>>> > >>>> However, this deferred addition retry isn't based on resources becoming > >>>> available. Instead, it's retried every 5 seconds and causes arbitrary > >>>> probe delays for amba devices and their consumers. > >>>> > >>>> Also, maintaining a separate deferred-probe like mechanism is > >>>> maintenance headache. > >>>> > >>>> With this commit, instead of deferring the device addition, we simply > >>>> defer the generation of uevents for the device and probing of the device > >>>> (because drivers needs PID and CID to match) until the PID and CID > >>>> information can be read. This allows us to delete all the amba specific > >>>> deferring code and also avoid the arbitrary probing delays. > >>>> > >>>> Cc: Rob Herring <robh@kernel.org> > >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org> > >>>> Cc: John Stultz <john.stultz@linaro.org> > >>>> Cc: Saravana Kannan <saravanak@google.com> > >>>> Cc: Linus Walleij <linus.walleij@linaro.org> > >>>> Cc: Sudeep Holla <sudeep.holla@arm.com> > >>>> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> > >>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com> > >>>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> > >>>> Cc: Russell King <linux@armlinux.org.uk> > >>>> Signed-off-by: Saravana Kannan <saravanak@google.com> > >>>> --- > >>>> > >>>> v1 -> v2: > >>>> - Dropped RFC tag > >>>> - Complete rewrite to not use stub devices. > >>>> > >>>> v2 -> v3: > >>>> - Flipped the if() condition for hard-coded periphids. > >>>> - Added a stub driver to handle the case where all amba drivers are > >>>> modules loaded by uevents. > >>>> - Cc Marek after I realized I forgot to add him. > >>>> > >>>> v3 -> v4: > >>>> - Finally figured out and fixed the issue reported by Kefeng (bus match > >>>> can't return an error other than -EPROBE_DEFER). > >>>> - I tested the patch on "V2P-CA15" on qemu > >>>> - Marek tested v3, but that was so long ago and the rebase wasn't clean, > >>>> so I didn't include the tested-by. > >>>> > >>>> Marek/Kefeng, > >>>> > >>>> Mind giving a Tested-by? > >>> > >>> Yes, it looks that it still works fine. > >>> > >>> I've tested it by changing the Exynos power domain driver to > >>> initialize from late_initcall. This in turn lead me to a bug in > >>> generic pm_domains code in __genpd_dev_pm_attach(), which returns -2 > >>> if the pm domain driver is not yet registered. After fixing that, I've > >>> successfully observed the deferred probe of PL330 driver on Exynos > >>> 4210 based boards both with this patch and without (with the old timer > >>> based code). > > Thanks for testing it again Marek! I was hoping you'll hit the crash > > that Sudeep was hitting and it would give me some more clues. > > > > Sudeep, > > > > This makes me think the issue you are seeing is related to your > > hardware drivers. Can you look into those please? I'm leaning towards > > merging this amba clean up and adding delays (say 1ms) to your > > clock/power domain drivers to avoid the crash you are seeing. And then > > you can figure out the actual delays needed and update it. > > > >> While preparing a fix for the above issue in genpd I found that it has > >> been introduced by your commit 5a46079a9645 ("PM: domains: Delete usage > >> of driver_deferred_probe_check_state()"). I didn't analyze it enough, > >> but it looks that something is missing there if we are trying to probe > >> amba device. I assume that returning -EPROBE_DEFER unconditionally there > >> is also not a valid solution? > > Yeah, the unconditionally returning -EPROBE_DEFER wouldn't work > > because if the supplier is optional but not present, the consumer > > driver would never stop waiting for it. I'm looking into issues > > similar to the one you saw in other threads [1]. The problem always > > boils down to the supplier device's DT node not having "compatible" > > property and therefore fw_devlink creating the device link between the > > consumer and the supplier's parent. > > > > Basically if the drivers/DT are implemented "properly", you would > > never get to the failure case (-2) if the driver is actually present. > Well, I don't get what do you mean by not having the proper 'comaptible' > property. Both affected devices (amba's pl330 and its power domain) have > compatible strings: 'arm,pl330' and 'samsung,exynos4210-pd', but the > devlinks doesn't help. Is it related to the custom device addition code > in the amba bus?
Thanks for pointing this out Marek. This is an interaction between the two separate series I sent out.
TL;DR is that device links don't block bus->match() attempts. That's the reason. That's a separate optimization that's in my todo list for a while.
Longer explanation follows:
5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()") correctly assumed fw_devlink will block calls to __genpd_dev_pm_attach() before the power domain has probed or we have given up waiting on suppliers at the driver core level. So, __genpd_dev_pm_attach() returning -2 was not a problem (well, there are other issues, but we'll pretend they don't exist for now).
Until this amba patch, that was true because really_probe() calls device_links_check_suppliers() before you'll get anywhere near __genpd_dev_pm_attach().
But with this amba patch, we try to get power domains before we get to really_probe() and that doesn't get the device links check. So, amba_match() has to always return -EPROBE_DEFER on any error until we optimize out match() calls for devices whose suppliers aren't ready yet. I'm considering reverting 5a46079a9645 due to other issues, so I think v4 might be okay as is.
-Saravana
> > I have some other ideas on how to get these to work better (not sure > > if it'll be for 100% of the cases), but until I get those ideas sorted > > out, I might do a partial revert of the change you mentioned. > > > > [1] - https://lore.kernel.org/lkml/4799738.LvFx2qVVIh@steina-w/ > > > ... > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
| |