| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3a907e77-792d-4bf1-8685-db48f607b562@icloud.com>
Date: Mon, 9 Sep 2024 07:51:28 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Russell King <linux@...linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Saravana Kannan <saravanak@...gle.com>,
Isaac Manjarres <isaacmanjarres@...gle.com>,
Lu Baolu <baolu.lu@...ux.intel.com>, linux-kernel@...r.kernel.org,
Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH RFC 3/3] amba: bus: Move reading periphid operation from
amba_match() to amba_probe()
On 2024/9/9 07:37, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@...cinc.com>
>
> amba_match(), as bus_type @amba_bustype's match(), reads periphid from
> hardware and may return -EPROBE_DEFER consequently, and it is the only
> one that breaks below ideal rule in current kernel tree:
>
> bus_type's match() should only return bool type compatible integer 0 or
> 1 ideally since its main operations are lookup and comparison normally.
>
> fixed by moving reading periphid operation to amba_probe().
or move to amba_dma_configure() which is the first bus_type's function
called after match()?
>
> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
> ---
> drivers/amba/bus.c | 55 +++++++++++++++++++++++++++++-------------------
> include/linux/amba/bus.h | 1 -
> 2 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 033d626aff46..8fe2e054b5ce 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -188,7 +188,7 @@ static int amba_read_periphid(struct amba_device *dev)
> }
>
> if (cid == AMBA_CID || cid == CORESIGHT_CID) {
> - dev->periphid = pid;
> + WRITE_ONCE(dev->periphid, pid);
> dev->cid = cid;
> }
>
> @@ -246,24 +246,14 @@ static int amba_match(struct device *dev, const struct device_driver *drv)
> struct amba_device *pcdev = to_amba_device(dev);
> const struct amba_driver *pcdrv = to_amba_driver(drv);
>
> - mutex_lock(&pcdev->periphid_lock);
> - 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 map it to
> - * -EPROBE_DEFER.
> - */
> - if (ret) {
> - mutex_unlock(&pcdev->periphid_lock);
> - return -EPROBE_DEFER;
> - }
> - dev_set_uevent_suppress(dev, false);
> - kobject_uevent(&dev->kobj, KOBJ_ADD);
> - }
> - mutex_unlock(&pcdev->periphid_lock);
> + /*
> + * For an AMBA device without valid periphid, only read periphid
> + * in amba_probe() for it when try to bind @amba_proxy_drv.
> + * For @pcdev->periphid, Reading here has a little race with
> + * writing in amba_probe().
> + */
> + if (!READ_ONCE(pcdev->periphid))
> + return pcdrv == &amba_proxy_drv ? 1 : 0;
>
> /* When driver_override is set, only bind to the matching driver */
> if (pcdev->driver_override)
> @@ -315,10 +305,24 @@ static int amba_probe(struct device *dev)
> {
> struct amba_device *pcdev = to_amba_device(dev);
> struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> - const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> + const struct amba_id *id;
> int ret;
>
> do {
> + if (!pcdev->periphid) {
> + ret = amba_read_periphid(pcdev);
> + if (ret) {
> + dev_err_probe(dev, ret, "failed to read periphid\n");
> + } else {
> + dev_set_uevent_suppress(dev, false);
> + kobject_uevent(&dev->kobj, KOBJ_ADD);
> + }
> +
> + ret = -EPROBE_DEFER;
> + break;
> + }
> + id = amba_lookup(pcdrv->id_table, pcdev);
> +
or read periphid in this function later since it does some
preparation for hardware ready to read which also is done by
amba_read_periphid() ?
> ret = of_amba_device_decode_irq(pcdev);
> if (ret)
> break;
> @@ -389,10 +393,15 @@ static void amba_shutdown(struct device *dev)
>
> static int amba_dma_configure(struct device *dev)
> {
> + struct amba_device *pcdev = to_amba_device(dev);
> struct amba_driver *drv = to_amba_driver(dev->driver);
> enum dev_dma_attr attr;
> int ret = 0;
>
> + /* To successfully go to amba_probe() to read periphid */
> + if (!pcdev->periphid)
> + return 0;
> +
> if (dev->of_node) {
> ret = of_dma_configure(dev, dev->of_node, true);
> } else if (has_acpi_companion(dev)) {
> @@ -411,8 +420,12 @@ static int amba_dma_configure(struct device *dev)
>
> static void amba_dma_cleanup(struct device *dev)
> {
> + struct amba_device *pcdev = to_amba_device(dev);
> struct amba_driver *drv = to_amba_driver(dev->driver);
>
> + if (!pcdev->periphid)
> + return;
> +
> if (!drv->driver_managed_dma)
> iommu_device_unuse_default_domain(dev);
> }
> @@ -535,7 +548,6 @@ static void amba_device_release(struct device *dev)
> fwnode_handle_put(dev_fwnode(&d->dev));
> if (d->res.parent)
> release_resource(&d->res);
> - mutex_destroy(&d->periphid_lock);
> kfree(d);
> }
>
> @@ -593,7 +605,6 @@ static void amba_device_initialize(struct amba_device *dev, const char *name)
> dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
> dev->dev.dma_parms = &dev->dma_parms;
> dev->res.name = dev_name(&dev->dev);
> - mutex_init(&dev->periphid_lock);
> }
>
> /**
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index 958a55bcc708..4bb3467d9c3d 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -67,7 +67,6 @@ struct amba_device {
> struct clk *pclk;
> struct device_dma_parameters dma_parms;
> unsigned int periphid;
> - struct mutex periphid_lock;
> unsigned int cid;
> struct amba_cs_uci_id uci;
> unsigned int irq[AMBA_NR_IRQS];
>
Powered by blists - more mailing lists