Re: [PATCH v2 2/2] bus: Add Altera AXI Timeout Bridge driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] bus: wrong base and 32-bit truncation in altera_timeout_bridge_isr()
- [High] bus: uninitialized spinlock exposed via sysfs in altera_timeout_bridge_probe()
- [Medium] bus: altera-timeout-bridge: NULL deref or probe failure when CONFIG_SYSFS=n
- [Low] MAINTAINERS: missing colon on M tag for Altera AXI Timeout Bridge driver
--

commit 2fab3abca0c9e4fa16264b963ed344d58e87a918
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

bus: Add Altera AXI Timeout Bridge driver

This commit adds a bus driver for the Altera AXI Timeout Bridge that populates
child devices behind the bridge and handles timeout interrupts by logging the
timed-out address and exposing irq_count and last_timeout_address via sysfs.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b5cab72d851b..27f7ffd6f0cb2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -945,6 +945,13 @@ ALPS PS/2 TOUCHPAD DRIVER
>  R:	Pali Rohár <pali@kernel.org>
>  F:	drivers/input/mouse/alps.*
>  
> +ALTERA AXI TIMEOUT BRIDGE DRIVER
> +M	Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

[Severity: Low]
Should the M tag in MAINTAINERS under ALTERA AXI TIMEOUT BRIDGE DRIVER have a
colon after M ("M:") so scripts/parse-maintainers.pl and
scripts/get_maintainer.pl can parse the entry without failing?

> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-driver-altera-timeout-bridge
> +F:	Documentation/devicetree/bindings/bus/altr,axi-timeout-bridge-1.0.yaml
> +F:	drivers/bus/altera-timeout-bridge.c
> +
>  ALTERA MAILBOX DRIVER
>  M:	Tien Sung Ang <tiensung.ang@altera.com>
>  S:	Maintained

[ ... ]

> diff --git a/drivers/bus/altera-timeout-bridge.c b/drivers/bus/altera-timeout-bridge.c
> new file mode 100644
> index 0000000000000..87886c3ad7941
> --- /dev/null
> +++ b/drivers/bus/altera-timeout-bridge.c
> @@ -0,0 +1,173 @@

[ ... ]

> +static ssize_t last_timeout_address_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct altera_timeout_bridge_data *driver_data = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	u64 addr;
> +
> +	spin_lock_irqsave(&driver_data->lock, flags);
> +	addr = driver_data->last_timeout_address;
> +	spin_unlock_irqrestore(&driver_data->lock, flags);
> +
> +	return sysfs_emit(buf, "0x%016llx\n", addr);
> +}

[ ... ]

> +static irqreturn_t altera_timeout_bridge_isr(int irq, void *data)
> +{
> +	struct altera_timeout_bridge_data *driver_data = data;
> +	u64 timed_out_address;
> +	resource_size_t effective_base_address;
> +	resource_size_t effective_address;
> +
> +	atomic_inc(&driver_data->irq_count);
> +
> +	// Read and print the timed-out address (encoded in the lower nibble
> +	// 0x8..0xF according to AXI Timeout Bridge IP (CSR Interrupt Status Information)
> +	timed_out_address = lo_hi_readq(driver_data->bridge_registers +
> +		ALTERA_TIMEOUT_BRIDGE_REG_TIMED_OUT_ADDRESS_LO);
> +	effective_base_address = driver_data->bridge_register_resource->start;
> +	effective_address = effective_base_address + timed_out_address;
> +
> +	spin_lock(&driver_data->lock);
> +	driver_data->last_timeout_address = effective_address;
> +	spin_unlock(&driver_data->lock);

[Severity: High]
Does altera_timeout_bridge_isr() in drivers/bus/altera-timeout-bridge.c
compute the wrong physical address and truncate 64-bit addresses on 32-bit
platforms?

In altera_timeout_bridge_probe(), devm_platform_get_and_ioremap_resource()
populates driver_data->bridge_register_resource from reg index 0, which is the
bridge's own 16-byte CSR register block rather than the subordinate bus
address window described by ranges:

Documentation/devicetree/bindings/bus/altr,axi-timeout-bridge-1.0.yaml:
    reg = <0x10000 0x10>;
    interrupts = <0 12 IRQ_TYPE_LEVEL_HIGH>;
    ranges = <0x0 0x10010 0x1000>;

    timeout-device@100 {
        compatible = "timeout-device";
        reg = <0x100 0x10>;
    };

Adding driver_data->bridge_register_resource->start to timed_out_address adds
the CSR base address (0x10000 in the binding example, yielding 0x10100 instead
of 0x10110) rather than translating the subordinate bus address through
ranges.

In addition, timed_out_address and driver_data->last_timeout_address are u64,
whereas effective_base_address and effective_address are resource_size_t. On
32-bit platforms without CONFIG_PHYS_ADDR_T_64BIT (such as arm32 without
LPAE), resource_size_t is 32 bits wide, so assigning the sum to
effective_address truncates the upper 32 bits before storing into
driver_data->last_timeout_address.

> +
> +	dev_err_ratelimited(driver_data->dev, "timeout at address 0x%llx\n",
> +		(unsigned long long)effective_address);

[ ... ]

> +static int altera_timeout_bridge_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct altera_timeout_bridge_data *driver_data;
> +	int ret;
> +	int irq;
> +
> +	driver_data =
> +		devm_kzalloc(&pdev->dev, sizeof(*driver_data), GFP_KERNEL);
> +	if (!driver_data)
> +		return -ENOMEM;
> +
> +	driver_data->dev = dev;
> +	platform_set_drvdata(pdev, driver_data);
> +
> +	driver_data->bridge_registers =
> +		devm_platform_get_and_ioremap_resource(pdev, 0,
> +			&driver_data->bridge_register_resource);
> +	if (IS_ERR(driver_data->bridge_registers))
> +		return PTR_ERR(driver_data->bridge_registers);
> +
> +	ret = devm_device_add_group(dev, &altera_timeout_bridge_sysfs_group);
> +	if (ret)
> +		return ret;
> +
> +	driver_data->irq_node = sysfs_get_dirent(dev->kobj.sd, "last_timeout_address");
> +	if (!driver_data->irq_node)
> +		return -ENOENT;

[Severity: Medium]
What happens in altera_timeout_bridge_probe() when CONFIG_SYSFS=n?

In drivers/bus/Kconfig, config ALTERA_TIMEOUT_BRIDGE depends only on
OF && HAS_IOMEM. When CONFIG_SYSFS=n, devm_device_add_group() succeeds as a
stub while dev->kobj.sd remains NULL.

Calling sysfs_get_dirent(dev->kobj.sd, "last_timeout_address") then passes a
NULL parent into kernfs_find_and_get_ns():

altera_timeout_bridge_probe()
  sysfs_get_dirent(dev->kobj.sd, "last_timeout_address")
    kernfs_find_and_get(NULL, "last_timeout_address")
      kernfs_find_and_get_ns(NULL, "last_timeout_address", NULL)
        kernfs_root(parent)

If CONFIG_KERNFS=y (for instance with CONFIG_CGROUPS=y), kernfs_root(parent)
dereferences parent->__parent on a NULL pointer. If CONFIG_KERNFS=n,
sysfs_get_dirent() returns NULL and altera_timeout_bridge_probe() fails with
-ENOENT before devm_of_platform_populate() can populate the child devices on
the bus.

> +
> +	ret = devm_add_action_or_reset(dev, altera_timeout_bridge_put_dirent,
> +				       driver_data->irq_node);
> +	if (ret)
> +		return ret;
> +
> +	spin_lock_init(&driver_data->lock);

[Severity: High]
Can last_timeout_address_show() in drivers/bus/altera-timeout-bridge.c acquire
driver_data->lock before spin_lock_init() initializes it?

In altera_timeout_bridge_probe(), devm_device_add_group() creates the
last_timeout_address sysfs attribute before spin_lock_init(&driver_data->lock)
is called. Once the sysfs group is registered, a userspace reader (such as
udev) opening and reading last_timeout_address invokes
last_timeout_address_show(), which calls
spin_lock_irqsave(&driver_data->lock, flags) on an uninitialized spinlock_t
(or during error unwind if sysfs_get_dirent() or devm_add_action_or_reset()
returns an error).

Should spin_lock_init(&driver_data->lock) be called before
devm_device_add_group()?

[ ... ]

> +}
> +
> +static const struct of_device_id altera_timeout_bridge_of_match[] = {

[ ... ]
 Let's double check everything in the response: all rules followed, plain text, wrapped at 78 characters, ends with a blank line. We are done!commit 2fab3abca0c9e4fa16264b963ed344d58e87a918
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

bus: Add Altera AXI Timeout Bridge driver

This commit adds a bus driver for the Altera AXI Timeout Bridge that populates
child devices behind the bridge and handles timeout interrupts by logging the
timed-out address and exposing irq_count and last_timeout_address via sysfs.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b5cab72d851b..27f7ffd6f0cb2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -945,6 +945,13 @@ ALPS PS/2 TOUCHPAD DRIVER
>  R:	Pali Rohár <pali@kernel.org>
>  F:	drivers/input/mouse/alps.*
>  
> +ALTERA AXI TIMEOUT BRIDGE DRIVER
> +M	Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>

[Severity: Low]
Should the M tag in MAINTAINERS under ALTERA AXI TIMEOUT BRIDGE DRIVER have a
colon after M ("M:") so scripts/parse-maintainers.pl and
scripts/get_maintainer.pl can parse the entry without failing?

> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-driver-altera-timeout-bridge
> +F:	Documentation/devicetree/bindings/bus/altr,axi-timeout-bridge-1.0.yaml
> +F:	drivers/bus/altera-timeout-bridge.c
> +
>  ALTERA MAILBOX DRIVER
>  M:	Tien Sung Ang <tiensung.ang@altera.com>
>  S:	Maintained

[ ... ]

> diff --git a/drivers/bus/altera-timeout-bridge.c b/drivers/bus/altera-timeout-bridge.c
> new file mode 100644
> index 0000000000000..87886c3ad7941
> --- /dev/null
> +++ b/drivers/bus/altera-timeout-bridge.c
> @@ -0,0 +1,173 @@

[ ... ]

> +static ssize_t last_timeout_address_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct altera_timeout_bridge_data *driver_data = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	u64 addr;
> +
> +	spin_lock_irqsave(&driver_data->lock, flags);
> +	addr = driver_data->last_timeout_address;
> +	spin_unlock_irqrestore(&driver_data->lock, flags);
> +
> +	return sysfs_emit(buf, "0x%016llx\n", addr);
> +}

[ ... ]

> +static irqreturn_t altera_timeout_bridge_isr(int irq, void *data)
> +{
> +	struct altera_timeout_bridge_data *driver_data = data;
> +	u64 timed_out_address;
> +	resource_size_t effective_base_address;
> +	resource_size_t effective_address;
> +
> +	atomic_inc(&driver_data->irq_count);
> +
> +	// Read and print the timed-out address (encoded in the lower nibble
> +	// 0x8..0xF according to AXI Timeout Bridge IP (CSR Interrupt Status Information)
> +	timed_out_address = lo_hi_readq(driver_data->bridge_registers +
> +		ALTERA_TIMEOUT_BRIDGE_REG_TIMED_OUT_ADDRESS_LO);
> +	effective_base_address = driver_data->bridge_register_resource->start;
> +	effective_address = effective_base_address + timed_out_address;
> +
> +	spin_lock(&driver_data->lock);
> +	driver_data->last_timeout_address = effective_address;
> +	spin_unlock(&driver_data->lock);

[Severity: High]
Does altera_timeout_bridge_isr() in drivers/bus/altera-timeout-bridge.c
compute the wrong physical address and truncate 64-bit addresses on 32-bit
platforms?

In altera_timeout_bridge_probe(), devm_platform_get_and_ioremap_resource()
populates driver_data->bridge_register_resource from reg index 0, which is the
bridge's own 16-byte CSR register block rather than the subordinate bus
address window described by ranges:

Documentation/devicetree/bindings/bus/altr,axi-timeout-bridge-1.0.yaml:
    reg = <0x10000 0x10>;
    interrupts = <0 12 IRQ_TYPE_LEVEL_HIGH>;
    ranges = <0x0 0x10010 0x1000>;

    timeout-device@100 {
        compatible = "timeout-device";
        reg = <0x100 0x10>;
    };

Adding driver_data->bridge_register_resource->start to timed_out_address adds
the CSR base address (0x10000 in the binding example, yielding 0x10100 instead
of 0x10110) rather than translating the subordinate bus address through
ranges.

In addition, timed_out_address and driver_data->last_timeout_address are u64,
whereas effective_base_address and effective_address are resource_size_t. On
32-bit platforms without CONFIG_PHYS_ADDR_T_64BIT (such as arm32 without
LPAE), resource_size_t is 32 bits wide, so assigning the sum to
effective_address truncates the upper 32 bits before storing into
driver_data->last_timeout_address.

> +
> +	dev_err_ratelimited(driver_data->dev, "timeout at address 0x%llx\n",
> +		(unsigned long long)effective_address);

[ ... ]

> +static int altera_timeout_bridge_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct altera_timeout_bridge_data *driver_data;
> +	int ret;
> +	int irq;
> +
> +	driver_data =
> +		devm_kzalloc(&pdev->dev, sizeof(*driver_data), GFP_KERNEL);
> +	if (!driver_data)
> +		return -ENOMEM;
> +
> +	driver_data->dev = dev;
> +	platform_set_drvdata(pdev, driver_data);
> +
> +	driver_data->bridge_registers =
> +		devm_platform_get_and_ioremap_resource(pdev, 0,
> +			&driver_data->bridge_register_resource);
> +	if (IS_ERR(driver_data->bridge_registers))
> +		return PTR_ERR(driver_data->bridge_registers);
> +
> +	ret = devm_device_add_group(dev, &altera_timeout_bridge_sysfs_group);
> +	if (ret)
> +		return ret;
> +
> +	driver_data->irq_node = sysfs_get_dirent(dev->kobj.sd, "last_timeout_address");
> +	if (!driver_data->irq_node)
> +		return -ENOENT;

[Severity: Medium]
What happens in altera_timeout_bridge_probe() when CONFIG_SYSFS=n?

In drivers/bus/Kconfig, config ALTERA_TIMEOUT_BRIDGE depends only on
OF && HAS_IOMEM. When CONFIG_SYSFS=n, devm_device_add_group() succeeds as a
stub while dev->kobj.sd remains NULL.

Calling sysfs_get_dirent(dev->kobj.sd, "last_timeout_address") then passes a
NULL parent into kernfs_find_and_get_ns():

altera_timeout_bridge_probe()
  sysfs_get_dirent(dev->kobj.sd, "last_timeout_address")
    kernfs_find_and_get(NULL, "last_timeout_address")
      kernfs_find_and_get_ns(NULL, "last_timeout_address", NULL)
        kernfs_root(parent)

If CONFIG_KERNFS=y (for instance with CONFIG_CGROUPS=y), kernfs_root(parent)
dereferences parent->__parent on a NULL pointer. If CONFIG_KERNFS=n,
sysfs_get_dirent() returns NULL and altera_timeout_bridge_probe() fails with
-ENOENT before devm_of_platform_populate() can populate the child devices on
the bus.

> +
> +	ret = devm_add_action_or_reset(dev, altera_timeout_bridge_put_dirent,
> +				       driver_data->irq_node);
> +	if (ret)
> +		return ret;
> +
> +	spin_lock_init(&driver_data->lock);

[Severity: High]
Can last_timeout_address_show() in drivers/bus/altera-timeout-bridge.c acquire
driver_data->lock before spin_lock_init() initializes it?

In altera_timeout_bridge_probe(), devm_device_add_group() creates the
last_timeout_address sysfs attribute before spin_lock_init(&driver_data->lock)
is called. Once the sysfs group is registered, a userspace reader (such as
udev) opening and reading last_timeout_address invokes
last_timeout_address_show(), which calls
spin_lock_irqsave(&driver_data->lock, flags) on an uninitialized spinlock_t
(or during error unwind if sysfs_get_dirent() or devm_add_action_or_reset()
returns an error).

Should spin_lock_init(&driver_data->lock) be called before
devm_device_add_group()?

[ ... ]

> +}
> +
> +static const struct of_device_id altera_timeout_bridge_of_match[] = {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-feature-timeout-ip-v2-0-a037be089518@bruker.com?part=2