[v2] amba: Remove deferred device addition
diff mbox series

Message ID 20210304035958.3657121-1-saravanak@google.com
State New, archived
Headers show
Series
  • [v2] amba: Remove deferred device addition
Related show

Commit Message

Saravana Kannan March 4, 2021, 3:59 a.m. UTC
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: Russell King <linux@armlinux.org.uk>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/amba/bus.c | 293 ++++++++++++++++++---------------------------
 1 file changed, 115 insertions(+), 178 deletions(-)

Comments

Saravana Kannan March 4, 2021, 4:08 a.m. UTC | #1
On Wed, Mar 3, 2021 at 8:00 PM Saravana Kannan <saravanak@google.com> 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: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/amba/bus.c | 293 ++++++++++++++++++---------------------------
>  1 file changed, 115 insertions(+), 178 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 939ca220bf78..fac4110b2f58 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);

Greg,

This is the only "ugly" part of this patch. I hope this is not a
problem as this patch finally gets amba devices to behave normally and
deletes about 60 lines of code.

Marek,

I tested it and saw the device get added before the resources were
available and the uevent file looked okay. Would you mind testing it
further?

Thanks,
Saravana

> +       }
> +
>         /* When driver_override is set, only bind to the matching driver */
>         if (pcdev->driver_override)
>                 return !strcmp(pcdev->driver_override, drv->name);
> @@ -373,98 +463,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 +512,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
>
Saravana Kannan March 4, 2021, 4:44 a.m. UTC | #2
On Wed, Mar 3, 2021 at 8:00 PM Saravana Kannan <saravanak@google.com> 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: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/amba/bus.c | 293 ++++++++++++++++++---------------------------
>  1 file changed, 115 insertions(+), 178 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 939ca220bf78..fac4110b2f58 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);
> @@ -373,98 +463,43 @@ static void amba_device_release(struct device *dev)
>         kfree(d);
>  }

Heh... after sending this patch I went down a rabbit hole of
links/emails. I was wondering why the bus.match() op already supported
-EPROBE_DEFER and what bus needed it in the first place. Turns out it
was added for AMBA [1] and there was even a patch [2] that tried to do
what I'm doing here but wasn't complete. Looks like only [1] was
picked up for some reason with no user. Or is there some other bus
that returns -EPROBE_DEFER or match?

-Saravana

[1] - https://lore.kernel.org/lkml/1443517859-30376-2-git-send-email-tomeu.vizoso@collabora.com/
[2] - https://lore.kernel.org/lkml/1443517859-30376-3-git-send-email-tomeu.vizoso@collabora.com/

>
> -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 +512,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
>
Russell King - ARM Linux admin March 4, 2021, 2:12 p.m. UTC | #3
On Wed, Mar 03, 2021 at 08:08:44PM -0800, Saravana Kannan wrote:
> Marek,
> 
> I tested it and saw the device get added before the resources were
> available and the uevent file looked okay. Would you mind testing it
> further?

To put it bluntly, if you have tested this, the testing was not very
effective. Deleting the lines that are removed by the patch so we can
see what the new code looks like below:

> > +int amba_device_add(struct amba_device *dev, struct resource *parent)
> >  {
> > +       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)
> > +               return ret;
> >
> > +       /* If primecell ID isn't hard-coded, figure it out */
> > +       if (dev->periphid) {
> > +               ret = amba_read_periphid(dev);

So, if the peripheral ID has _already_ been set, we attempt to read the
peripheral ID from the device. Isn't that just wrong?

> > +               if (ret && ret != -EPROBE_DEFER)
> > +                       goto err_release;
> >                 /*
> > +                * 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 (ret)
> > +                       dev_set_uevent_suppress(&dev->dev, true);
> >         }

If the peripheral ID has not been set, we don't attempt to read it, and
we generate an add event when the amba device is added with a zero
peripheral ID.

I guess that if() statement should be negated - and with such an error,
I fail to see how this code could have been properly tested.
Saravana Kannan March 4, 2021, 3:48 p.m. UTC | #4
On Thu, Mar 4, 2021 at 6:12 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Mar 03, 2021 at 08:08:44PM -0800, Saravana Kannan wrote:
> > Marek,
> >
> > I tested it and saw the device get added before the resources were
> > available and the uevent file looked okay. Would you mind testing it
> > further?
>
> To put it bluntly, if you have tested this, the testing was not very
> effective. Deleting the lines that are removed by the patch so we can
> see what the new code looks like below:
>
> > > +int amba_device_add(struct amba_device *dev, struct resource *parent)
> > >  {
> > > +       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)
> > > +               return ret;
> > >
> > > +       /* If primecell ID isn't hard-coded, figure it out */
> > > +       if (dev->periphid) {
> > > +               ret = amba_read_periphid(dev);
>
> So, if the peripheral ID has _already_ been set, we attempt to read the
> peripheral ID from the device. Isn't that just wrong?
>
> > > +               if (ret && ret != -EPROBE_DEFER)
> > > +                       goto err_release;
> > >                 /*
> > > +                * 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 (ret)
> > > +                       dev_set_uevent_suppress(&dev->dev, true);
> > >         }
>
> If the peripheral ID has not been set, we don't attempt to read it, and
> we generate an add event when the amba device is added with a zero
> peripheral ID.
>
> I guess that if() statement should be negated - and with such an error,
> I fail to see how this code could have been properly tested.

Yeah, the if() needs to be flipped. I even flipped it and then
unflipped it before I sent the patch. Thanks for catching it.

It worked in my testing because the device didn't have hard coded PID.
So it worked out fine.

But I now realize I still have a chicken-and-egg problem if ALL amba
drivers are modules. amba_match() will never be called because none of
the amba drivers have been loaded. None of the amba drivers would be
loaded (depending on the set up) because none of the uevents were sent
out. But there's a simple fix for this. I'll send that as part of v3.

Marek,

It'd still be nice if you can test this with the if() above flipped.
If all your amba drivers are modules and loaded based on uevents,
manually loading one of them will kick off everything.

-Saravana

Patch
diff mbox series

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 939ca220bf78..fac4110b2f58 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);
@@ -373,98 +463,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 +512,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);