From: sashiko-bot@kernel.org
To: "Vyacheslav Yurkov" <V.Yurkov.EXT@bruker.com>
Cc: devicetree@vger.kernel.org, robh@kernel.org, conor+dt@kernel.org
Subject: Re: [PATCH v2 2/2] bus: Add Altera AXI Timeout Bridge driver
Date: Wed, 21 Oct 2026 20:11:27 +0000 [thread overview]
Message-ID: <sashiko-outbox-180630@kernel.org> (raw)
In-Reply-To: <20261021-feature-timeout-ip-v2-2-a037be089518@bruker.com>
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
prev parent reply other threads:[~2026-10-21 20:11 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-21 19:56 [PATCH v2 0/2] Add Altera timeout bridge driver Vyacheslav Yurkov via B4 Relay
2026-10-21 19:56 ` [PATCH v2 1/2] dt-bindings: bus: Add Altera AXI Timeout Bridge Vyacheslav Yurkov via B4 Relay
2026-10-21 20:05 ` sashiko-bot
2026-10-22 2:29 ` Rob Herring (Arm)
2026-10-21 19:56 ` [PATCH v2 2/2] bus: Add Altera AXI Timeout Bridge driver Vyacheslav Yurkov via B4 Relay
2026-10-21 20:11 ` sashiko-bot [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-180630@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=V.Yurkov.EXT@bruker.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=robh@kernel.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox