Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CGME20220705083944eucas1p23419f52b9529c79c03c8cc23e2aaf4c5@eucas1p2.samsung.com>
 <20220705083934.3974140-1-saravanak@google.com> <7482d3af-4b02-4c1d-0386-b0a4ddf529da@samsung.com>
 <8a04332e-e7b1-8bc3-d569-5052427bcb13@samsung.com>
In-Reply-To: <8a04332e-e7b1-8bc3-d569-5052427bcb13@samsung.com>
From:   Saravana Kannan <saravanak@google.com>
Date:   Tue, 12 Jul 2022 12:38:33 -0700
Message-ID: <CAGETcx93rWqt-Cyz_8JZ4mxcLpJruzSM1QOAgpkPSM-G1sBXWg@mail.gmail.com>
Subject: Re: [PATCH v4] amba: Remove deferred device addition
To:     Marek Szyprowski <m.szyprowski@samsung.com>
Cc:     Russell King <linux@armlinux.org.uk>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Rob Herring <robh@kernel.org>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        John Stultz <john.stultz@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        kernel-team@android.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 12, 2022 at 5:34 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Saravana,
>
> On 12.07.2022 14:25, Marek Szyprowski wrote:
> > Hi Saravana,
> >
> > 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.
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/

-Saravana

>
>
> >
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >
> >
> >> -Saravana
> >>
> >>   drivers/amba/bus.c | 317 +++++++++++++++++++++------------------------
> >>   1 file changed, 145 insertions(+), 172 deletions(-)
> >>
> >> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> >> index 0e3ed5eb367b..9590c93b2aea 100644
> >> --- a/drivers/amba/bus.c
> >> +++ b/drivers/amba/bus.c
> >> @@ -130,11 +130,100 @@ static struct attribute *amba_dev_attrs[] = {
> >>   };
> >>   ATTRIBUTE_GROUPS(amba_dev);
> >>   +static int amba_read_periphid(struct amba_device *dev)
> >> +{
> >> +    struct reset_control *rstc;
> >> +    u32 size, pid, cid;
> >> +    void __iomem *tmp;
> >> +    int i, ret;
> >> +
> >> +    ret = dev_pm_domain_attach(&dev->dev, true);
> >> +    if (ret) {
> >> +        dev_dbg(&dev->dev, "can't get PM domain: %d\n", ret);
> >> +        goto err_out;
> >> +    }
> >> +
> >> +    ret = amba_get_enable_pclk(dev);
> >> +    if (ret) {
> >> +        dev_dbg(&dev->dev, "can't get pclk: %d\n", ret);
> >> +        goto err_pm;
> >> +    }
> >> +
> >> +    /*
> >> +     * Find reset control(s) of the amba bus and de-assert them.
> >> +     */
> >> +    rstc =
> >> of_reset_control_array_get_optional_shared(dev->dev.of_node);
> >> +    if (IS_ERR(rstc)) {
> >> +        ret = PTR_ERR(rstc);
> >> +        if (ret != -EPROBE_DEFER)
> >> +            dev_err(&dev->dev, "can't get reset: %d\n", ret);
> >> +        goto err_clk;
> >> +    }
> >> +    reset_control_deassert(rstc);
> >> +    reset_control_put(rstc);
> >> +
> >> +    size = resource_size(&dev->res);
> >> +    tmp = ioremap(dev->res.start, size);
> >> +    if (!tmp) {
> >> +        ret = -ENOMEM;
> >> +        goto err_clk;
> >> +    }
> >> +
> >> +    /*
> >> +     * Read pid and cid based on size of resource
> >> +     * they are located at end of region
> >> +     */
> >> +    for (pid = 0, i = 0; i < 4; i++)
> >> +        pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> >> +    for (cid = 0, i = 0; i < 4; i++)
> >> +        cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> >> +
> >> +    if (cid == CORESIGHT_CID) {
> >> +        /* set the base to the start of the last 4k block */
> >> +        void __iomem *csbase = tmp + size - 4096;
> >> +
> >> +        dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
> >> +        dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) &
> >> 0xff;
> >> +    }
> >> +
> >> +    if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> >> +        dev->periphid = pid;
> >> +        dev->cid = cid;
> >> +    }
> >> +
> >> +    if (!dev->periphid)
> >> +        ret = -ENODEV;
> >> +
> >> +    iounmap(tmp);
> >> +
> >> +err_clk:
> >> +    amba_put_disable_pclk(dev);
> >> +err_pm:
> >> +    dev_pm_domain_detach(&dev->dev, true);
> >> +err_out:
> >> +    return ret;
> >> +}
> >> +
> >>   static int amba_match(struct device *dev, struct device_driver *drv)
> >>   {
> >>       struct amba_device *pcdev = to_amba_device(dev);
> >>       struct amba_driver *pcdrv = to_amba_driver(drv);
> >>   +    if (!pcdev->periphid) {
> >> +        int ret = amba_read_periphid(pcdev);
> >> +
> >> +        /*
> >> +         * Returning any error other than -EPROBE_DEFER from bus match
> >> +         * can cause driver registration failure. So, if there's a
> >> +         * permanent failure in reading pid and cid, simply say that
> >> +         * none of the drivers match.
> >> +         */
> >> +        if (ret)
> >> +            return ret == -EPROBE_DEFER ? ret : 0;
> >> +        dev_set_uevent_suppress(dev, false);
> >> +        kobject_uevent(&dev->kobj, KOBJ_ADD);
> >> +    }
> >> +
> >>       /* When driver_override is set, only bind to the matching
> >> driver */
> >>       if (pcdev->driver_override)
> >>           return !strcmp(pcdev->driver_override, drv->name);
> >> @@ -368,6 +457,42 @@ static int __init amba_init(void)
> >>     postcore_initcall(amba_init);
> >>   +static int amba_proxy_probe(struct amba_device *adev,
> >> +                const struct amba_id *id)
> >> +{
> >> +    WARN(1, "Stub driver should never match any device.\n");
> >> +    return -ENODEV;
> >> +}
> >> +
> >> +static const struct amba_id amba_stub_drv_ids[] = {
> >> +    { 0, 0 },
> >> +};
> >> +
> >> +static struct amba_driver amba_proxy_drv = {
> >> +    .drv = {
> >> +        .name = "amba-proxy",
> >> +    },
> >> +    .probe = amba_proxy_probe,
> >> +    .id_table = amba_stub_drv_ids,
> >> +};
> >> +
> >> +static int __init amba_stub_drv_init(void)
> >> +{
> >> +    if (!IS_ENABLED(CONFIG_MODULES))
> >> +        return 0;
> >> +
> >> +    /*
> >> +     * The amba_match() function will get called only if there is at
> >> least
> >> +     * one amba driver registered. If all amba drivers are modules
> >> and are
> >> +     * only loaded based on uevents, then we'll hit a chicken-and-egg
> >> +     * situation where amba_match() is waiting on drivers and
> >> drivers are
> >> +     * waiting on amba_match(). So, register a stub driver to make sure
> >> +     * amba_match() is called even if no amba driver has been
> >> registered.
> >> +     */
> >> +    return amba_driver_register(&amba_proxy_drv);
> >> +}
> >> +late_initcall_sync(amba_stub_drv_init);
> >> +
> >>   /**
> >>    *    amba_driver_register - register an AMBA device driver
> >>    *    @drv: amba device driver structure
> >> @@ -410,160 +535,6 @@ static void amba_device_release(struct device
> >> *dev)
> >>       kfree(d);
> >>   }
> >>   -static int amba_read_periphid(struct amba_device *dev)
> >> -{
> >> -    struct reset_control *rstc;
> >> -    u32 size, pid, cid;
> >> -    void __iomem *tmp;
> >> -    int i, ret;
> >> -
> >> -    ret = dev_pm_domain_attach(&dev->dev, true);
> >> -    if (ret)
> >> -        goto err_out;
> >> -
> >> -    ret = amba_get_enable_pclk(dev);
> >> -    if (ret)
> >> -        goto err_pm;
> >> -
> >> -    /*
> >> -     * Find reset control(s) of the amba bus and de-assert them.
> >> -     */
> >> -    rstc =
> >> of_reset_control_array_get_optional_shared(dev->dev.of_node);
> >> -    if (IS_ERR(rstc)) {
> >> -        ret = PTR_ERR(rstc);
> >> -        if (ret != -EPROBE_DEFER)
> >> -            dev_err(&dev->dev, "can't get reset: %d\n", ret);
> >> -        goto err_clk;
> >> -    }
> >> -    reset_control_deassert(rstc);
> >> -    reset_control_put(rstc);
> >> -
> >> -    size = resource_size(&dev->res);
> >> -    tmp = ioremap(dev->res.start, size);
> >> -    if (!tmp) {
> >> -        ret = -ENOMEM;
> >> -        goto err_clk;
> >> -    }
> >> -
> >> -    /*
> >> -     * Read pid and cid based on size of resource
> >> -     * they are located at end of region
> >> -     */
> >> -    for (pid = 0, i = 0; i < 4; i++)
> >> -        pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
> >> -    for (cid = 0, i = 0; i < 4; i++)
> >> -        cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
> >> -
> >> -    if (cid == CORESIGHT_CID) {
> >> -        /* set the base to the start of the last 4k block */
> >> -        void __iomem *csbase = tmp + size - 4096;
> >> -
> >> -        dev->uci.devarch = readl(csbase + UCI_REG_DEVARCH_OFFSET);
> >> -        dev->uci.devtype = readl(csbase + UCI_REG_DEVTYPE_OFFSET) &
> >> 0xff;
> >> -    }
> >> -
> >> -    if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> >> -        dev->periphid = pid;
> >> -        dev->cid = cid;
> >> -    }
> >> -
> >> -    if (!dev->periphid)
> >> -        ret = -ENODEV;
> >> -
> >> -    iounmap(tmp);
> >> -
> >> -err_clk:
> >> -    amba_put_disable_pclk(dev);
> >> -err_pm:
> >> -    dev_pm_domain_detach(&dev->dev, true);
> >> -err_out:
> >> -    return ret;
> >> -}
> >> -
> >> -static int amba_device_try_add(struct amba_device *dev, struct
> >> resource *parent)
> >> -{
> >> -    int ret;
> >> -
> >> -    ret = request_resource(parent, &dev->res);
> >> -    if (ret)
> >> -        goto err_out;
> >> -
> >> -    /* Hard-coded primecell ID instead of plug-n-play */
> >> -    if (dev->periphid != 0)
> >> -        goto skip_probe;
> >> -
> >> -    ret = amba_read_periphid(dev);
> >> -    if (ret) {
> >> -        if (ret != -EPROBE_DEFER) {
> >> -            amba_device_put(dev);
> >> -            goto err_out;
> >> -        }
> >> -        goto err_release;
> >> -    }
> >> -
> >> -skip_probe:
> >> -    ret = device_add(&dev->dev);
> >> -err_release:
> >> -    if (ret)
> >> -        release_resource(&dev->res);
> >> -err_out:
> >> -    return ret;
> >> -}
> >> -
> >> -/*
> >> - * Registration of AMBA device require reading its pid and cid
> >> registers.
> >> - * To do this, the device must be turned on (if it is a part of
> >> power domain)
> >> - * and have clocks enabled. However in some cases those resources
> >> might not be
> >> - * yet available. Returning EPROBE_DEFER is not a solution in such
> >> case,
> >> - * because callers don't handle this special error code. Instead
> >> such devices
> >> - * are added to the special list and their registration is retried from
> >> - * periodic worker, until all resources are available and
> >> registration succeeds.
> >> - */
> >> -struct deferred_device {
> >> -    struct amba_device *dev;
> >> -    struct resource *parent;
> >> -    struct list_head node;
> >> -};
> >> -
> >> -static LIST_HEAD(deferred_devices);
> >> -static DEFINE_MUTEX(deferred_devices_lock);
> >> -
> >> -static void amba_deferred_retry_func(struct work_struct *dummy);
> >> -static DECLARE_DELAYED_WORK(deferred_retry_work,
> >> amba_deferred_retry_func);
> >> -
> >> -#define DEFERRED_DEVICE_TIMEOUT (msecs_to_jiffies(5 * 1000))
> >> -
> >> -static int amba_deferred_retry(void)
> >> -{
> >> -    struct deferred_device *ddev, *tmp;
> >> -
> >> -    mutex_lock(&deferred_devices_lock);
> >> -
> >> -    list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) {
> >> -        int ret = amba_device_try_add(ddev->dev, ddev->parent);
> >> -
> >> -        if (ret == -EPROBE_DEFER)
> >> -            continue;
> >> -
> >> -        list_del_init(&ddev->node);
> >> -        kfree(ddev);
> >> -    }
> >> -
> >> -    mutex_unlock(&deferred_devices_lock);
> >> -
> >> -    return 0;
> >> -}
> >> -late_initcall(amba_deferred_retry);
> >> -
> >> -static void amba_deferred_retry_func(struct work_struct *dummy)
> >> -{
> >> -    amba_deferred_retry();
> >> -
> >> -    if (!list_empty(&deferred_devices))
> >> -        schedule_delayed_work(&deferred_retry_work,
> >> -                      DEFERRED_DEVICE_TIMEOUT);
> >> -}
> >> -
> >>   /**
> >>    *    amba_device_add - add a previously allocated AMBA device
> >> structure
> >>    *    @dev: AMBA device allocated by amba_device_alloc
> >> @@ -575,28 +546,30 @@ static void amba_deferred_retry_func(struct
> >> work_struct *dummy)
> >>    */
> >>   int amba_device_add(struct amba_device *dev, struct resource *parent)
> >>   {
> >> -    int ret = amba_device_try_add(dev, parent);
> >> -
> >> -    if (ret == -EPROBE_DEFER) {
> >> -        struct deferred_device *ddev;
> >> -
> >> -        ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
> >> -        if (!ddev)
> >> -            return -ENOMEM;
> >> +    int ret;
> >>   -        ddev->dev = dev;
> >> -        ddev->parent = parent;
> >> -        ret = 0;
> >> +    ret = request_resource(parent, &dev->res);
> >> +    if (ret)
> >> +        return ret;
> >>   -        mutex_lock(&deferred_devices_lock);
> >> +    /* If primecell ID isn't hard-coded, figure it out */
> >> +    if (!dev->periphid) {
> >> +        /*
> >> +         * AMBA device uevents require reading its pid and cid
> >> +         * registers.  To do this, the device must be on, clocked and
> >> +         * out of reset.  However in some cases those resources might
> >> +         * not yet be available.  If that's the case, we suppress the
> >> +         * generation of uevents until we can read the pid and cid
> >> +         * registers.  See also amba_match().
> >> +         */
> >> +        if (amba_read_periphid(dev))
> >> +            dev_set_uevent_suppress(&dev->dev, true);
> >> +    }
> >>   -        if (list_empty(&deferred_devices))
> >> -            schedule_delayed_work(&deferred_retry_work,
> >> -                          DEFERRED_DEVICE_TIMEOUT);
> >> -        list_add_tail(&ddev->node, &deferred_devices);
> >> +    ret = device_add(&dev->dev);
> >> +    if (ret)
> >> +        release_resource(&dev->res);
> >>   -        mutex_unlock(&deferred_devices_lock);
> >> -    }
> >>       return ret;
> >>   }
> >>   EXPORT_SYMBOL_GPL(amba_device_add);
> >
> > Best regards
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>