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 <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Nicolas Saenz Julienne <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Saravana Kannan <[email protected]>
---
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.
Marek,
Would you mind testing this? It looks okay with my limited testing.
-Saravana
drivers/amba/bus.c | 329 +++++++++++++++++++++------------------------
1 file changed, 151 insertions(+), 178 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 939ca220bf78..836d6d23bba3 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -149,11 +149,101 @@ static struct attribute *amba_dev_attrs[] = {
};
ATTRIBUTE_GROUPS(amba_dev);
+static int amba_read_periphid(struct amba_device *dev)
+{
+ u32 size;
+ void __iomem *tmp;
+ u32 pid, cid;
+ struct reset_control *rstc;
+ int i, ret;
+
+ /*
+ * Dynamically calculate the size of the resource
+ * and use this for iomap
+ */
+ size = resource_size(&dev->res);
+ tmp = ioremap(dev->res.start, size);
+ if (!tmp)
+ return -ENOMEM;
+
+ ret = dev_pm_domain_attach(&dev->dev, true);
+ if (ret)
+ goto err_pm;
+
+ ret = amba_get_enable_pclk(dev);
+ if (ret)
+ goto err_clk;
+
+ /*
+ * 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_reset;
+ }
+ reset_control_deassert(rstc);
+ reset_control_put(rstc);
+
+ /*
+ * 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;
+ }
+
+ amba_put_disable_pclk(dev);
+
+ if (cid == AMBA_CID || cid == CORESIGHT_CID) {
+ dev->periphid = pid;
+ dev->cid = cid;
+ }
+
+ if (!dev->periphid)
+ ret = -ENODEV;
+
+ return ret;
+
+err_reset:
+ amba_put_disable_pclk(dev);
+err_clk:
+ dev_pm_domain_detach(&dev->dev, true);
+err_pm:
+ iounmap(tmp);
+ 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);
+
+ if (ret)
+ return ret;
+ 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);
@@ -332,6 +422,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
@@ -373,98 +499,43 @@ static void amba_device_release(struct device *dev)
kfree(d);
}
-static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
+/**
+ * amba_device_add - add a previously allocated AMBA device structure
+ * @dev: AMBA device allocated by amba_device_alloc
+ * @parent: resource parent for this devices resources
+ *
+ * Claim the resource, and read the device cell ID if not already
+ * initialized. Register the AMBA device with the Linux device
+ * manager.
+ */
+int amba_device_add(struct amba_device *dev, struct resource *parent)
{
- u32 size;
- void __iomem *tmp;
- int i, ret;
+ int ret;
WARN_ON(dev->irq[0] == (unsigned int)-1);
WARN_ON(dev->irq[1] == (unsigned int)-1);
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;
-
- /*
- * Dynamically calculate the size of the resource
- * and use this for iomap
- */
- size = resource_size(&dev->res);
- tmp = ioremap(dev->res.start, size);
- if (!tmp) {
- ret = -ENOMEM;
- goto err_release;
- }
-
- ret = dev_pm_domain_attach(&dev->dev, true);
- if (ret) {
- iounmap(tmp);
- goto err_release;
- }
-
- ret = amba_get_enable_pclk(dev);
- if (ret == 0) {
- u32 pid, cid;
- struct reset_control *rstc;
-
- /*
- * 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_reset;
- }
- reset_control_deassert(rstc);
- reset_control_put(rstc);
+ return ret;
+ /* If primecell ID isn't hard-coded, figure it out */
+ if (!dev->periphid) {
+ ret = amba_read_periphid(dev);
+ if (ret && ret != -EPROBE_DEFER)
+ goto err_release;
/*
- * Read pid and cid based on size of resource
- * they are located at end of region
+ * 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().
*/
- 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;
- }
-
- amba_put_disable_pclk(dev);
-
- if (cid == AMBA_CID || cid == CORESIGHT_CID) {
- dev->periphid = pid;
- dev->cid = cid;
- }
-
- if (!dev->periphid)
- ret = -ENODEV;
+ if (ret)
+ dev_set_uevent_suppress(&dev->dev, true);
}
- iounmap(tmp);
- dev_pm_domain_detach(&dev->dev, true);
-
- if (ret)
- goto err_release;
-
- skip_probe:
ret = device_add(&dev->dev);
if (ret)
goto err_release;
@@ -477,106 +548,8 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
return ret;
device_unregister(&dev->dev);
-
err_release:
release_resource(&dev->res);
- err_out:
- return ret;
-
- err_reset:
- amba_put_disable_pclk(dev);
- iounmap(tmp);
- dev_pm_domain_detach(&dev->dev, true);
- goto err_release;
-}
-
-/*
- * 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
- * @parent: resource parent for this devices resources
- *
- * Claim the resource, and read the device cell ID if not already
- * initialized. Register the AMBA device with the Linux device
- * manager.
- */
-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;
-
- ddev->dev = dev;
- ddev->parent = parent;
- ret = 0;
-
- mutex_lock(&deferred_devices_lock);
-
- if (list_empty(&deferred_devices))
- schedule_delayed_work(&deferred_retry_work,
- DEFERRED_DEVICE_TIMEOUT);
- list_add_tail(&ddev->node, &deferred_devices);
-
- mutex_unlock(&deferred_devices_lock);
- }
return ret;
}
EXPORT_SYMBOL_GPL(amba_device_add);
--
2.30.1.766.gb4fecdf3b7-goog
Hi Saravana,
On 04.03.2021 20:51, 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 <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: John Stultz <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Nicolas Saenz Julienne <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Saravana Kannan <[email protected]>
> ---
>
> 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.
>
> Marek,
>
> Would you mind testing this? It looks okay with my limited testing.
It looks it works fine on my test systems. I've checked current
linux-next and this patch. You can add:
Tested-by: Marek Szyprowski <[email protected]>
I've briefly scanned the code and I'm curious how does it work. Does it
depend on the recently introduced "fw_devlink=on" feature? I don't see
other mechanism, which would trigger matching amba device if pm domains,
clocks or resets were not available on time to read pid/cid while adding
a device...
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski
<[email protected]> wrote:
>
> Hi Saravana,
>
> On 04.03.2021 20:51, 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 <[email protected]>
> > Cc: Ulf Hansson <[email protected]>
> > Cc: John Stultz <[email protected]>
> > Cc: Saravana Kannan <[email protected]>
> > Cc: Linus Walleij <[email protected]>
> > Cc: Sudeep Holla <[email protected]>
> > Cc: Nicolas Saenz Julienne <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: Marek Szyprowski <[email protected]>
> > Cc: Russell King <[email protected]>
> > Signed-off-by: Saravana Kannan <[email protected]>
> > ---
> >
> > 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.
> >
> > Marek,
> >
> > Would you mind testing this? It looks okay with my limited testing.
>
> It looks it works fine on my test systems. I've checked current
> linux-next and this patch. You can add:
>
> Tested-by: Marek Szyprowski <[email protected]>
Hi Marek,
Thanks! Does your test set up have amda drivers that are loaded based
on uevents? That's the one I couldn't test.
> I've briefly scanned the code and I'm curious how does it work. Does it
> depend on the recently introduced "fw_devlink=on" feature? I don't see
> other mechanism, which would trigger matching amba device if pm domains,
> clocks or resets were not available on time to read pid/cid while adding
> a device...
No, it does not depend on fw_devlink or device links in any way.
When a device is attempted to be probed (when it's added or during
deferred probe), it's matched with all the drivers on the bus.
When a new driver is registered to a bus, all devices in that bus are
matched with the driver to see if they'll work together.
That's how match is called. And match() can return -EPROBE_DEFER and
that'll cause the device to be put in the deferred probe list by
driver core.
The tricky part in this patch was the uevent handling and the
chicken-and-egg issue I talk about in the comments.
Russell,
Does this look good now? Plan to pick it up some time?
Thanks,
Saravana
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
Hi Saravana,
On 05.03.2021 19:02, Saravana Kannan wrote:
> On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski
> <[email protected]> wrote:
>> On 04.03.2021 20:51, 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 <[email protected]>
>>> Cc: Ulf Hansson <[email protected]>
>>> Cc: John Stultz <[email protected]>
>>> Cc: Saravana Kannan <[email protected]>
>>> Cc: Linus Walleij <[email protected]>
>>> Cc: Sudeep Holla <[email protected]>
>>> Cc: Nicolas Saenz Julienne <[email protected]>
>>> Cc: Geert Uytterhoeven <[email protected]>
>>> Cc: Marek Szyprowski <[email protected]>
>>> Cc: Russell King <[email protected]>
>>> Signed-off-by: Saravana Kannan <[email protected]>
>>> ---
>>>
>>> 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.
>>>
>>> Marek,
>>>
>>> Would you mind testing this? It looks okay with my limited testing.
>> It looks it works fine on my test systems. I've checked current
>> linux-next and this patch. You can add:
>>
>> Tested-by: Marek Szyprowski <[email protected]>
> Hi Marek,
>
> Thanks! Does your test set up have amda drivers that are loaded based
> on uevents? That's the one I couldn't test.
I've checked both, the built-in and all amba drivers compiled as
modules, loaded by udev. Both works fine here.
>> I've briefly scanned the code and I'm curious how does it work. Does it
>> depend on the recently introduced "fw_devlink=on" feature? I don't see
>> other mechanism, which would trigger matching amba device if pm domains,
>> clocks or resets were not available on time to read pid/cid while adding
>> a device...
> No, it does not depend on fw_devlink or device links in any way.
>
> When a device is attempted to be probed (when it's added or during
> deferred probe), it's matched with all the drivers on the bus.
> When a new driver is registered to a bus, all devices in that bus are
> matched with the driver to see if they'll work together.
> That's how match is called. And match() can return -EPROBE_DEFER and
> that'll cause the device to be put in the deferred probe list by
> driver core.
>
> The tricky part in this patch was the uevent handling and the
> chicken-and-egg issue I talk about in the comments.
Thanks for the explanation. This EPROBE_DEFER support in match()
callback must be something added after I crafted that periodic retry
based workaround.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Sun, Mar 7, 2021 at 11:28 PM Marek Szyprowski
<[email protected]> wrote:
>
> Hi Saravana,
>
> On 05.03.2021 19:02, Saravana Kannan wrote:
> > On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski
> > <[email protected]> wrote:
> >> On 04.03.2021 20:51, 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 <[email protected]>
> >>> Cc: Ulf Hansson <[email protected]>
> >>> Cc: John Stultz <[email protected]>
> >>> Cc: Saravana Kannan <[email protected]>
> >>> Cc: Linus Walleij <[email protected]>
> >>> Cc: Sudeep Holla <[email protected]>
> >>> Cc: Nicolas Saenz Julienne <[email protected]>
> >>> Cc: Geert Uytterhoeven <[email protected]>
> >>> Cc: Marek Szyprowski <[email protected]>
> >>> Cc: Russell King <[email protected]>
> >>> Signed-off-by: Saravana Kannan <[email protected]>
> >>> ---
> >>>
> >>> 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.
> >>>
> >>> Marek,
> >>>
> >>> Would you mind testing this? It looks okay with my limited testing.
> >> It looks it works fine on my test systems. I've checked current
> >> linux-next and this patch. You can add:
> >>
> >> Tested-by: Marek Szyprowski <[email protected]>
> > Hi Marek,
> >
> > Thanks! Does your test set up have amda drivers that are loaded based
> > on uevents? That's the one I couldn't test.
>
> I've checked both, the built-in and all amba drivers compiled as
> modules, loaded by udev. Both works fine here.
>
> >> I've briefly scanned the code and I'm curious how does it work. Does it
> >> depend on the recently introduced "fw_devlink=on" feature? I don't see
> >> other mechanism, which would trigger matching amba device if pm domains,
> >> clocks or resets were not available on time to read pid/cid while adding
> >> a device...
> > No, it does not depend on fw_devlink or device links in any way.
> >
> > When a device is attempted to be probed (when it's added or during
> > deferred probe), it's matched with all the drivers on the bus.
> > When a new driver is registered to a bus, all devices in that bus are
> > matched with the driver to see if they'll work together.
> > That's how match is called. And match() can return -EPROBE_DEFER and
> > that'll cause the device to be put in the deferred probe list by
> > driver core.
> >
> > The tricky part in this patch was the uevent handling and the
> > chicken-and-egg issue I talk about in the comments.
>
> Thanks for the explanation. This EPROBE_DEFER support in match()
> callback must be something added after I crafted that periodic retry
> based workaround.
>
I think it got in just a few months before your patches, but your
patches worked :) I actually don't like match returning -EPROBE_DEFER,
but I can work around it for some of my fw_devlink optimization plans.
More context here:
https://lore.kernel.org/lkml/CAGETcx_qO4vxTSyBtBR2k7fd_3rGJF42iBbJH37HPNw=FheDKg@mail.gmail.com/
-Saravana
On Mon, Mar 8, 2021 at 11:15 AM Saravana Kannan <[email protected]> wrote:
>
> On Sun, Mar 7, 2021 at 11:28 PM Marek Szyprowski
> <[email protected]> wrote:
> >
> > Hi Saravana,
> >
> > On 05.03.2021 19:02, Saravana Kannan wrote:
> > > On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski
> > > <[email protected]> wrote:
> > >> On 04.03.2021 20:51, 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 <[email protected]>
> > >>> Cc: Ulf Hansson <[email protected]>
> > >>> Cc: John Stultz <[email protected]>
> > >>> Cc: Saravana Kannan <[email protected]>
> > >>> Cc: Linus Walleij <[email protected]>
> > >>> Cc: Sudeep Holla <[email protected]>
> > >>> Cc: Nicolas Saenz Julienne <[email protected]>
> > >>> Cc: Geert Uytterhoeven <[email protected]>
> > >>> Cc: Marek Szyprowski <[email protected]>
> > >>> Cc: Russell King <[email protected]>
> > >>> Signed-off-by: Saravana Kannan <[email protected]>
> > >>> ---
> > >>>
> > >>> 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.
> > >>>
> > >>> Marek,
> > >>>
> > >>> Would you mind testing this? It looks okay with my limited testing.
> > >> It looks it works fine on my test systems. I've checked current
> > >> linux-next and this patch. You can add:
> > >>
> > >> Tested-by: Marek Szyprowski <[email protected]>
> > > Hi Marek,
> > >
> > > Thanks! Does your test set up have amda drivers that are loaded based
> > > on uevents? That's the one I couldn't test.
> >
> > I've checked both, the built-in and all amba drivers compiled as
> > modules, loaded by udev. Both works fine here.
> >
> > >> I've briefly scanned the code and I'm curious how does it work. Does it
> > >> depend on the recently introduced "fw_devlink=on" feature? I don't see
> > >> other mechanism, which would trigger matching amba device if pm domains,
> > >> clocks or resets were not available on time to read pid/cid while adding
> > >> a device...
> > > No, it does not depend on fw_devlink or device links in any way.
> > >
> > > When a device is attempted to be probed (when it's added or during
> > > deferred probe), it's matched with all the drivers on the bus.
> > > When a new driver is registered to a bus, all devices in that bus are
> > > matched with the driver to see if they'll work together.
> > > That's how match is called. And match() can return -EPROBE_DEFER and
> > > that'll cause the device to be put in the deferred probe list by
> > > driver core.
> > >
> > > The tricky part in this patch was the uevent handling and the
> > > chicken-and-egg issue I talk about in the comments.
> >
> > Thanks for the explanation. This EPROBE_DEFER support in match()
> > callback must be something added after I crafted that periodic retry
> > based workaround.
> >
>
> I think it got in just a few months before your patches, but your
> patches worked :) I actually don't like match returning -EPROBE_DEFER,
> but I can work around it for some of my fw_devlink optimization plans.
>
> More context here:
> https://lore.kernel.org/lkml/CAGETcx_qO4vxTSyBtBR2k7fd_3rGJF42iBbJH37HPNw=FheDKg@mail.gmail.com/
I just noticed that this still hasn't been picked up.
Russell/Greg, can we please pick this up. This finally cleans up
deferred probing of AMBA devices so that they don't have any special
case.
-Saravana
On Tue, Aug 24, 2021 at 12:26:16PM -0700, Saravana Kannan wrote:
> On Mon, Mar 8, 2021 at 11:15 AM Saravana Kannan <[email protected]> wrote:
> >
> > On Sun, Mar 7, 2021 at 11:28 PM Marek Szyprowski
> > <[email protected]> wrote:
> > >
> > > Hi Saravana,
> > >
> > > On 05.03.2021 19:02, Saravana Kannan wrote:
> > > > On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski
> > > > <[email protected]> wrote:
> > > >> On 04.03.2021 20:51, 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 <[email protected]>
> > > >>> Cc: Ulf Hansson <[email protected]>
> > > >>> Cc: John Stultz <[email protected]>
> > > >>> Cc: Saravana Kannan <[email protected]>
> > > >>> Cc: Linus Walleij <[email protected]>
> > > >>> Cc: Sudeep Holla <[email protected]>
> > > >>> Cc: Nicolas Saenz Julienne <[email protected]>
> > > >>> Cc: Geert Uytterhoeven <[email protected]>
> > > >>> Cc: Marek Szyprowski <[email protected]>
> > > >>> Cc: Russell King <[email protected]>
> > > >>> Signed-off-by: Saravana Kannan <[email protected]>
> > > >>> ---
> > > >>>
> > > >>> 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.
> > > >>>
> > > >>> Marek,
> > > >>>
> > > >>> Would you mind testing this? It looks okay with my limited testing.
> > > >> It looks it works fine on my test systems. I've checked current
> > > >> linux-next and this patch. You can add:
> > > >>
> > > >> Tested-by: Marek Szyprowski <[email protected]>
> > > > Hi Marek,
> > > >
> > > > Thanks! Does your test set up have amda drivers that are loaded based
> > > > on uevents? That's the one I couldn't test.
> > >
> > > I've checked both, the built-in and all amba drivers compiled as
> > > modules, loaded by udev. Both works fine here.
> > >
> > > >> I've briefly scanned the code and I'm curious how does it work. Does it
> > > >> depend on the recently introduced "fw_devlink=on" feature? I don't see
> > > >> other mechanism, which would trigger matching amba device if pm domains,
> > > >> clocks or resets were not available on time to read pid/cid while adding
> > > >> a device...
> > > > No, it does not depend on fw_devlink or device links in any way.
> > > >
> > > > When a device is attempted to be probed (when it's added or during
> > > > deferred probe), it's matched with all the drivers on the bus.
> > > > When a new driver is registered to a bus, all devices in that bus are
> > > > matched with the driver to see if they'll work together.
> > > > That's how match is called. And match() can return -EPROBE_DEFER and
> > > > that'll cause the device to be put in the deferred probe list by
> > > > driver core.
> > > >
> > > > The tricky part in this patch was the uevent handling and the
> > > > chicken-and-egg issue I talk about in the comments.
> > >
> > > Thanks for the explanation. This EPROBE_DEFER support in match()
> > > callback must be something added after I crafted that periodic retry
> > > based workaround.
> > >
> >
> > I think it got in just a few months before your patches, but your
> > patches worked :) I actually don't like match returning -EPROBE_DEFER,
> > but I can work around it for some of my fw_devlink optimization plans.
> >
> > More context here:
> > https://lore.kernel.org/lkml/CAGETcx_qO4vxTSyBtBR2k7fd_3rGJF42iBbJH37HPNw=FheDKg@mail.gmail.com/
>
> I just noticed that this still hasn't been picked up.
>
> Russell/Greg, can we please pick this up. This finally cleans up
> deferred probing of AMBA devices so that they don't have any special
> case.
Now picked up, thanks.
greg k-h
On Fri, Aug 27, 2021 at 1:54 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Tue, Aug 24, 2021 at 12:26:16PM -0700, Saravana Kannan wrote:
> > On Mon, Mar 8, 2021 at 11:15 AM Saravana Kannan <[email protected]> wrote:
> > >
> > > On Sun, Mar 7, 2021 at 11:28 PM Marek Szyprowski
> > > <[email protected]> wrote:
> > > >
> > > > Hi Saravana,
> > > >
> > > > On 05.03.2021 19:02, Saravana Kannan wrote:
> > > > > On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski
> > > > > <[email protected]> wrote:
> > > > >> On 04.03.2021 20:51, 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 <[email protected]>
> > > > >>> Cc: Ulf Hansson <[email protected]>
> > > > >>> Cc: John Stultz <[email protected]>
> > > > >>> Cc: Saravana Kannan <[email protected]>
> > > > >>> Cc: Linus Walleij <[email protected]>
> > > > >>> Cc: Sudeep Holla <[email protected]>
> > > > >>> Cc: Nicolas Saenz Julienne <[email protected]>
> > > > >>> Cc: Geert Uytterhoeven <[email protected]>
> > > > >>> Cc: Marek Szyprowski <[email protected]>
> > > > >>> Cc: Russell King <[email protected]>
> > > > >>> Signed-off-by: Saravana Kannan <[email protected]>
> > > > >>> ---
> > > > >>>
> > > > >>> 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.
> > > > >>>
> > > > >>> Marek,
> > > > >>>
> > > > >>> Would you mind testing this? It looks okay with my limited testing.
> > > > >> It looks it works fine on my test systems. I've checked current
> > > > >> linux-next and this patch. You can add:
> > > > >>
> > > > >> Tested-by: Marek Szyprowski <[email protected]>
> > > > > Hi Marek,
> > > > >
> > > > > Thanks! Does your test set up have amda drivers that are loaded based
> > > > > on uevents? That's the one I couldn't test.
> > > >
> > > > I've checked both, the built-in and all amba drivers compiled as
> > > > modules, loaded by udev. Both works fine here.
> > > >
> > > > >> I've briefly scanned the code and I'm curious how does it work. Does it
> > > > >> depend on the recently introduced "fw_devlink=on" feature? I don't see
> > > > >> other mechanism, which would trigger matching amba device if pm domains,
> > > > >> clocks or resets were not available on time to read pid/cid while adding
> > > > >> a device...
> > > > > No, it does not depend on fw_devlink or device links in any way.
> > > > >
> > > > > When a device is attempted to be probed (when it's added or during
> > > > > deferred probe), it's matched with all the drivers on the bus.
> > > > > When a new driver is registered to a bus, all devices in that bus are
> > > > > matched with the driver to see if they'll work together.
> > > > > That's how match is called. And match() can return -EPROBE_DEFER and
> > > > > that'll cause the device to be put in the deferred probe list by
> > > > > driver core.
> > > > >
> > > > > The tricky part in this patch was the uevent handling and the
> > > > > chicken-and-egg issue I talk about in the comments.
> > > >
> > > > Thanks for the explanation. This EPROBE_DEFER support in match()
> > > > callback must be something added after I crafted that periodic retry
> > > > based workaround.
> > > >
> > >
> > > I think it got in just a few months before your patches, but your
> > > patches worked :) I actually don't like match returning -EPROBE_DEFER,
> > > but I can work around it for some of my fw_devlink optimization plans.
> > >
> > > More context here:
> > > https://lore.kernel.org/lkml/CAGETcx_qO4vxTSyBtBR2k7fd_3rGJF42iBbJH37HPNw=FheDKg@mail.gmail.com/
> >
> > I just noticed that this still hasn't been picked up.
> >
> > Russell/Greg, can we please pick this up. This finally cleans up
> > deferred probing of AMBA devices so that they don't have any special
> > case.
>
> Now picked up, thanks.
This patch is apparently causing issues in a specific platform.
https://lore.kernel.org/lkml/[email protected]/
Even though it worked fine for Marek:
https://lore.kernel.org/lkml/[email protected]/#t
Here's my current analysis:
https://lore.kernel.org/lkml/CAGETcx-N4+u0iw9n5ncx_9MNnTa3ViyesxsDD7xN3jtEPT-uBw@mail.gmail.com/
I'll leave it up to you on how to proceed -- revert or wait for another fix.
-Saravana
On Fri, Aug 27, 2021 at 12:15:10PM -0700, Saravana Kannan wrote:
> On Fri, Aug 27, 2021 at 1:54 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Tue, Aug 24, 2021 at 12:26:16PM -0700, Saravana Kannan wrote:
> > > On Mon, Mar 8, 2021 at 11:15 AM Saravana Kannan <[email protected]> wrote:
> > > >
> > > > On Sun, Mar 7, 2021 at 11:28 PM Marek Szyprowski
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Saravana,
> > > > >
> > > > > On 05.03.2021 19:02, Saravana Kannan wrote:
> > > > > > On Fri, Mar 5, 2021 at 3:45 AM Marek Szyprowski
> > > > > > <[email protected]> wrote:
> > > > > >> On 04.03.2021 20:51, 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 <[email protected]>
> > > > > >>> Cc: Ulf Hansson <[email protected]>
> > > > > >>> Cc: John Stultz <[email protected]>
> > > > > >>> Cc: Saravana Kannan <[email protected]>
> > > > > >>> Cc: Linus Walleij <[email protected]>
> > > > > >>> Cc: Sudeep Holla <[email protected]>
> > > > > >>> Cc: Nicolas Saenz Julienne <[email protected]>
> > > > > >>> Cc: Geert Uytterhoeven <[email protected]>
> > > > > >>> Cc: Marek Szyprowski <[email protected]>
> > > > > >>> Cc: Russell King <[email protected]>
> > > > > >>> Signed-off-by: Saravana Kannan <[email protected]>
> > > > > >>> ---
> > > > > >>>
> > > > > >>> 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.
> > > > > >>>
> > > > > >>> Marek,
> > > > > >>>
> > > > > >>> Would you mind testing this? It looks okay with my limited testing.
> > > > > >> It looks it works fine on my test systems. I've checked current
> > > > > >> linux-next and this patch. You can add:
> > > > > >>
> > > > > >> Tested-by: Marek Szyprowski <[email protected]>
> > > > > > Hi Marek,
> > > > > >
> > > > > > Thanks! Does your test set up have amda drivers that are loaded based
> > > > > > on uevents? That's the one I couldn't test.
> > > > >
> > > > > I've checked both, the built-in and all amba drivers compiled as
> > > > > modules, loaded by udev. Both works fine here.
> > > > >
> > > > > >> I've briefly scanned the code and I'm curious how does it work. Does it
> > > > > >> depend on the recently introduced "fw_devlink=on" feature? I don't see
> > > > > >> other mechanism, which would trigger matching amba device if pm domains,
> > > > > >> clocks or resets were not available on time to read pid/cid while adding
> > > > > >> a device...
> > > > > > No, it does not depend on fw_devlink or device links in any way.
> > > > > >
> > > > > > When a device is attempted to be probed (when it's added or during
> > > > > > deferred probe), it's matched with all the drivers on the bus.
> > > > > > When a new driver is registered to a bus, all devices in that bus are
> > > > > > matched with the driver to see if they'll work together.
> > > > > > That's how match is called. And match() can return -EPROBE_DEFER and
> > > > > > that'll cause the device to be put in the deferred probe list by
> > > > > > driver core.
> > > > > >
> > > > > > The tricky part in this patch was the uevent handling and the
> > > > > > chicken-and-egg issue I talk about in the comments.
> > > > >
> > > > > Thanks for the explanation. This EPROBE_DEFER support in match()
> > > > > callback must be something added after I crafted that periodic retry
> > > > > based workaround.
> > > > >
> > > >
> > > > I think it got in just a few months before your patches, but your
> > > > patches worked :) I actually don't like match returning -EPROBE_DEFER,
> > > > but I can work around it for some of my fw_devlink optimization plans.
> > > >
> > > > More context here:
> > > > https://lore.kernel.org/lkml/CAGETcx_qO4vxTSyBtBR2k7fd_3rGJF42iBbJH37HPNw=FheDKg@mail.gmail.com/
> > >
> > > I just noticed that this still hasn't been picked up.
> > >
> > > Russell/Greg, can we please pick this up. This finally cleans up
> > > deferred probing of AMBA devices so that they don't have any special
> > > case.
> >
> > Now picked up, thanks.
>
> This patch is apparently causing issues in a specific platform.
> https://lore.kernel.org/lkml/[email protected]/
>
> Even though it worked fine for Marek:
> https://lore.kernel.org/lkml/[email protected]/#t
>
> Here's my current analysis:
> https://lore.kernel.org/lkml/CAGETcx-N4+u0iw9n5ncx_9MNnTa3ViyesxsDD7xN3jtEPT-uBw@mail.gmail.com/
>
> I'll leave it up to you on how to proceed -- revert or wait for another fix.
I've dropped it from my tree for now. Let me know when you have
something that works better.
thanks,
greg k-h