* Re: [PATCH v7 10/10] arm64: dts: qcom: shikra: Add gpio-reserved-ranges to tlmm
From: Dmitry Baryshkov @ 2026-07-20 22:19 UTC (permalink / raw)
To: Komal Bajaj
Cc: Vinod Koul, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Krzysztof Kozlowski, Georgi Djakov, Bjorn Andersson,
Konrad Dybcio, linux-arm-msm, dmaengine, devicetree, linux-kernel,
linux-pm, Anurag Pateriya
In-Reply-To: <20260720-shikra-dt-m1-v7-10-7dc99100c6dd@oss.qualcomm.com>
On Mon, Jul 20, 2026 at 04:19:26PM +0530, Komal Bajaj wrote:
> Add gpio-reserved-ranges property to the TLMM node for both Shikra
> SoM variants (CQM and IQS). These reserved GPIOs are inaccessible
> from the non-secure world and dedicated to fixed functions.
>
> Signed-off-by: Anurag Pateriya <apateriy@qti.qualcomm.com>
> Signed-off-by: Komal Bajaj <komal.bajaj@oss.qualcomm.com>
Who is the author of the patch? Why does it have two SoBs? Should there
be a Co-developed-by?
Other than that:
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
> arch/arm64/boot/dts/qcom/shikra-cqm-som.dtsi | 6 ++++++
> arch/arm64/boot/dts/qcom/shikra-iqs-som.dtsi | 6 ++++++
> 2 files changed, 12 insertions(+)
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v3 4/4] phy: qualcomm: qmp-combo: Add support for Hawi SoC
From: Dmitry Baryshkov @ 2026-07-20 22:24 UTC (permalink / raw)
To: sashiko
Cc: Ronak Raheja, robh, krzk+dt, olteanv, devicetree, vkoul, conor+dt,
neil.armstrong, linux-phy, linux-arm-msm
In-Reply-To: <20260508232620.2FD04C2BCB0@smtp.kernel.org>
On Fri, May 08, 2026 at 11:26:19PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Missing DP PHY function pointers in `hawi_usb3dpphy_cfg` lead to NULL pointer dereferences when the DP PHY is initialized.
> - [High] Valid register offset `0x00` causes the AON toggle hardware reset override to be silently skipped.
Please add DP programming. There should be no NULL dereferences.
> > @@ -3699,9 +3915,9 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
> > SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
> >
> > /* override hardware control for reset of qmp phy */
> > - if (pcs_aon && cfg->regs[QPHY_AON_TOGGLE_ENABLE]) {
> > - qphy_clrbits(pcs_aon, cfg->regs[QPHY_AON_TOGGLE_ENABLE], 0x1);
> > - qphy_clrbits(pcs_aon, cfg->regs[QPHY_DP_AON_TOGGLE_ENABLE], 0x1);
> > + if (aon_toggle && cfg->regs[QPHY_AON_TOGGLE_ENABLE]) {
> > + qphy_clrbits(aon_toggle, cfg->regs[QPHY_AON_TOGGLE_ENABLE], 0x1);
> > + qphy_clrbits(aon_toggle, cfg->regs[QPHY_DP_AON_TOGGLE_ENABLE], 0x1);
> > }
>
> Is it possible for the hardware reset override to be silently skipped here?
>
> The V10 register layout maps QPHY_AON_TOGGLE_ENABLE to
> QPHY_V10_COM_AON_USB3_AON_TOGGLE_ENABLE, which is defined as 0x00 in
> phy-qcom-qmp-com-aon-v10.h.
>
> Because 0x00 is a valid register offset but evaluates to false in C, the
> check for cfg->regs[QPHY_AON_TOGGLE_ENABLE] will fail. This would skip the
> qphy_clrbits() calls entirely, bypassing the necessary hardware reset override
> sequence and leaving the PHY in a broken state.
>
> Could we use a different way to check if this register is supported by the
> layout without relying on the truthiness of the offset value?
This deserves a comment.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH] dt-bindings: power: reset: qcom,pon: Narrow allowed reboot modes
From: Sebastian Reichel @ 2026-07-20 23:12 UTC (permalink / raw)
To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Vinod Koul, linux-pm, devicetree, linux-kernel,
Krzysztof Kozlowski
In-Reply-To: <20260701105907.197150-2-krzysztof.kozlowski@oss.qualcomm.com>
On Wed, 01 Jul 2026 12:59:08 +0200, Krzysztof Kozlowski wrote:
> reboot-mode schema allows arbitrary "mode-.* properties but only a
> subset actually makes sense and is valid. Provide negative look-ahead
> pattern to disallow any modes not supported by the device, which
> tightens the binding.
>
>
Applied, thanks!
[1/1] dt-bindings: power: reset: qcom,pon: Narrow allowed reboot modes
commit: 792c48a7751346ea5e5069ec733f005d42a35b26
Best regards,
--
Sebastian Reichel <sebastian.reichel@collabora.com>
^ permalink raw reply
* Re: [PATCH] arm64: dts: allwinner: a523: x96qpro+: overvolting eMMC
From: Andre Przywara @ 2026-07-20 23:13 UTC (permalink / raw)
To: sashiko-bot; +Cc: sashiko-reviews, conor+dt, devicetree, robh, linux-sunxi
In-Reply-To: <20260720215035.ED2D51F000E9@smtp.kernel.org>
On Mon, 20 Jul 2026 21:50:35 +0000
sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Bumping the eMMC IO regulator (`reg_cldo1`) to 1.9V inadvertently misconfigures the SoC's IO bank (Port C) into 3.3V power mode instead of 1.8V mode, creating a logic threshold mismatch.
Huh, that's interesting, but might actually point to the real root
cause: it seems like the meaning of the bits in the withstand register
is inverted on the A523: 0 encodes 1.8V, a 1 encodes 3.3V. But that's
the opposite of the H6 definition, which we use in the kernel for the
A523 as well. Which means they were wrongly programmed before, but now
the wrong voltage selection turns out to trigger the really correct
setting.
So I will try playing around with those registers instead.
Cheers,
Andre.
> --
>
> --- Patch [1]: [PATCH] arm64: dts: allwinner: a523: x96qpro+: overvolting eMMC ---
> commit e4c261c388d6e406babdf0d56ac61d104d537230
> Author: Andre Przywara <andre.przywara@arm.com>
> Date: Mon Jul 20 21:38:32 2026 +0000
>
> arm64: dts: allwinner: a523: x96qpro+: overvolting eMMC
>
> This commit changes the eMMC IO regulator voltage on PortC from 1.8V to 1.9V
> for the X96QPro+ device. The goal is to improve eMMC stability and performance
> without drastically limiting the clock frequency.
>
> > diff --git a/arch/arm64/boot/dts/allwinner/sun55i-h728-x96qpro+.dts b/arch/arm64/boot/dts/allwinner/sun55i-h728-x96qpro+.dts
> > --- a/arch/arm64/boot/dts/allwinner/sun55i-h728-x96qpro+.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun55i-h728-x96qpro+.dts
> [ ... ]
> > @@ -194,8 +194,8 @@ reg_bldo4: bldo4 {
> >
> > reg_cldo1: cldo1 {
> > regulator-always-on;
> > - regulator-min-microvolt = <1800000>;
> > - regulator-max-microvolt = <1800000>;
> > + regulator-min-microvolt = <1900000>;
> > + regulator-max-microvolt = <1900000>;
>
> [Severity: High]
> Does bumping this regulator to 1.9V inadvertently force the SoC's IO bank
> (Port C) into 3.3V power mode?
>
> Looking at sunxi_pinctrl_set_io_bias_cfg() in
> drivers/pinctrl/sunxi/pinctrl-sunxi.c, the driver uses a strict check when
> evaluating the voltage:
>
> case BIAS_VOLTAGE_PIO_POW_MODE_SEL:
> val = uV <= 1800000 ? 1 : 0;
>
> Since 1900000 is greater than 1800000, this evaluates to 0 and clears the
> PIO_POW_MOD_SEL register bit, incorrectly configuring the IO bank for 3.3V
> mode while being powered at 1.9V.
>
> Could this mismatch alter input receiver thresholds and drive strengths,
> potentially causing signal integrity issues or long-term hardware
> degradation?
>
> > regulator-name = "vcc-codec-sd";
> > };
>
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] net: dsa: initial support for MT7628 embedded switch
From: Jakub Kicinski @ 2026-07-20 23:52 UTC (permalink / raw)
To: joey
Cc: Jakub Kicinski, netdev, horms, pabeni, edumazet, davem, olteanv,
andrew, devicetree, robh, krzk+dt, conor+dt, arinc.unal,
Landen.Chao, dqfext, sean.wang, daniel
In-Reply-To: <20260704105659.140970-5-joey@tinyisr.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: initial support for MT7628 embedded switch
This patch adds a DSA driver for the MT7628 embedded switch, which has
five 100Mbps user ports and a 1Gbps CPU port. tag_8021q is used to
isolate ports via the switch's 16-entry VLAN table, with the double tag
feature enabled to simulate VLAN-unaware behaviour.
> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> new file mode 100644
> index 000000000000..eccdb05fce9b
> --- /dev/null
> +++ b/drivers/net/dsa/mt7628.c
[ ... ]
> +static const struct dsa_switch_ops mt7628_switch_ops = {
> + .get_tag_protocol = mt7628_get_tag_proto,
> + .setup = mt7628_setup,
> + .teardown = mt7628_teardown,
> + .port_enable = mt7628_port_enable,
> + .port_disable = mt7628_port_disable,
> + .phylink_get_caps = mt7628_phylink_get_caps,
> + .tag_8021q_vlan_add = mt7628_dsa_8021q_vlan_add,
> + .tag_8021q_vlan_del = mt7628_dsa_8021q_vlan_del,
> +};
[Medium]
How is a user expected to place two of the user ports into a Linux bridge?
The ops table does not implement port_bridge_join or port_bridge_leave,
so dsa_switch_bridge_join() in net/dsa/switch.c returns -EOPNOTSUPP:
if (!ds->ops->port_bridge_join)
return -EOPNOTSUPP;
That makes ip link set swpN master brX fail for every user port, which
seems to contradict the commit message:
The switch hardware has a very limited 16 entry VLAN table.
Configuring VLANs is the only way to control switch forwarding.
Currently 6 entries are used by tag_8021q to isolate the ports.
Without a port_bridge_join hook, dsa_tag_8021q_bridge_join() is never
invoked, so there is no path to migrate a port's tag_8021q VID from its
standalone VID to a bridge VID.
Would wiring the tag_8021q helpers directly, similar to what
vitesse-vsc73xx-core.c does, be sufficient here:
.port_bridge_join = dsa_tag_8021q_bridge_join,
.port_bridge_leave = dsa_tag_8021q_bridge_leave,
Related bridge-lifecycle callbacks such as port_stp_state_set,
port_pre_bridge_flags, port_bridge_flags and port_fast_age also appear
to be absent, is that intentional for this initial submission?
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] net: dsa: initial support for MT7628 embedded switch
From: Jakub Kicinski @ 2026-07-20 23:52 UTC (permalink / raw)
To: joey
Cc: Jakub Kicinski, netdev, horms, pabeni, edumazet, davem, olteanv,
andrew, devicetree, robh, krzk+dt, conor+dt, arinc.unal,
Landen.Chao, dqfext, sean.wang, daniel
In-Reply-To: <20260704105659.140970-5-joey@tinyisr.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: initial support for MT7628 embedded switch
Add support for the MT7628 embedded switch. The switch has 5 built-in
100Mbps user ports and one 1Gbps CPU port.
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 4ab567c5bbafc..daa1d3d4ba609 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -63,6 +63,14 @@ config NET_DSA_MT7530_MMIO
> are directly mapped into the SoCs register space rather than being
> accessible via MDIO.
>
> +config NET_DSA_MT7628
> + tristate "MediaTek MT7628 Embedded Ethernet switch support"
> + select NET_DSA_TAG_MT7628
> + select MEDIATEK_FE_SOC_PHY
> + help
[Severity: Low]
Can this lead to a build or link failure on randconfig? It looks like this
is missing a select REGMAP_MMIO and depends on HAS_IOMEM, which might cause
devm_regmap_init_mmio() to be unresolved when CONFIG_REGMAP_MMIO is not set.
> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> new file mode 100644
> index 0000000000000..eccdb05fce9b7
> --- /dev/null
> +++ b/drivers/net/dsa/mt7628.c
> @@ -0,0 +1,649 @@
> [ ... ]
> +static int mt7628_setup_internal_mdio(struct dsa_switch *ds)
> +{
> + struct mt7628_esw *esw = ds->priv;
> + struct device *dev = ds->dev;
> + struct mii_bus *bus;
> +
> + bus = devm_mdiobus_alloc(dev);
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "MT7628 internal MDIO bus";
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
> + bus->priv = esw;
> + bus->read = mt7628_mii_read;
> + bus->write = mt7628_mii_write;
> + bus->parent = dev;
> +
> + ds->user_mii_bus = bus;
> + bus->phy_mask = ~ds->phys_mii_mask;
> +
> + return devm_mdiobus_register(dev, bus);
> +}
[Severity: High]
Does this cause a resource leak and sysfs name collision if the switch setup
callback runs multiple times?
If the DSA master device unbinds and rebinds, dsa_tree_teardown() and
dsa_tree_setup() can be triggered again. Because devm_mdiobus_alloc() and
devm_mdiobus_register() tie the resource to the platform device's lifetime
rather than the DSA tree setup/teardown cycle, the first registration will
leak, and the second might fail with -EEXIST.
> [ ... ]
> +static void mt7628_switch_init(struct dsa_switch *ds)
> +{
> + struct mt7628_esw *esw = ds->priv;
> +
> + regmap_write(esw->regmap, MT7628_ESW_REG_FCT0,
> + FIELD_PREP(MT7628_ESW_FCT0_DROP_SET_TH, 0x50) |
> + FIELD_PREP(MT7628_ESW_FCT0_DROP_RLS_TH, 0x78) |
> + FIELD_PREP(MT7628_ESW_FCT0_FC_SET_TH, 0xa0) |
> + FIELD_PREP(MT7628_ESW_FCT0_FC_RLS_TH, 0xc8));
[Severity: High]
Can traffic leak across isolated ports (like WAN and LAN) during boot?
It looks like mt7628_switch_init() resets the switch but fails to explicitly
set the MT7628_ESW_POC0_PORT_DISABLE bits for the user ports. Since the
DSA core relies on port_disable during ndo_close and doesn't automatically
disable ports upon initialization, does the hardware default to acting as
an unmanaged switch, bridging all networks until the interfaces are brought
up administratively?
^ permalink raw reply
* Re: [PATCH 2/2] firmware: stratix10-svc: add support for agilex5
From: Dinh Nguyen @ 2026-07-20 23:59 UTC (permalink / raw)
To: Adrian Ng Ho Yin, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
devicetree, linux-kernel
In-Reply-To: <c096c728b7becf027b6e9fcc4aa1cc5dfe3cef3a.1782135785.git.adrian.ho.yin.ng@altera.com>
On 6/22/26 08:44, Adrian Ng Ho Yin wrote:
> On Agilex5 the DDR base address starts at 0x8000_0000, which is
> outside the addressable range of the SDM. The SMMU is used to remap
> DDR-allocated buffers to an IOVA within the SDM-accessible 0-512MB
> window. Return -ENODEV at probe if no IOMMU domain is found for an
> intel,agilex5-svc device.
>
> Configure a 29-bit DMA mask to constrain IOVA allocations to the
> 0-512MB range accessible to the SDM. Agilex5 REV B introduced a
> hardware SDM address remapper; bypass it via SMC so no additional
> offset is applied to the IOVA, keeping the implementation
> consistent across all Agilex5 revisions.
>
> ATF validates FPGA_CONFIG_WRITE addresses against the DDR range
> starting at 0x8000_0000. Since IOVAs are below 0x2000_0000, the
> driver adds 0x8000_0000 to the IOVA before the SMC call so ATF's
> is_address_in_ddr_range() check passes. ATF then strips the offset
> and uses the SMMU to translate the remaining IOVA to the underlying
> physical memory for SDM access.
>
> The firmware COMPLETED_WRITE response returns the raw IOVA without
> the 0x8000_0000 offset. Compensate by storing dma_addr_offset in
> the controller and adding it back before the svc_pa_to_va() lookup.
> dma_addr_offset is zero on non-SMMU paths so existing platforms are
> unaffected.
>
> Fix a pre-existing bug in stratix10_svc_free_memory() where an
> unknown-address fallthrough called list_del(&svc_data_mem),
> corrupting the list head. Replace it with dev_warn().
>
> Register a devm cleanup action at probe to reclaim any DMA
> coherent buffers that service clients fail to free before driver
> unbind, preventing memory leaks across probe/remove cycles.
>
> Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
There's alot going on with this patch! Can you split it up into smaller
bits? And what part of this patch needed for the DTS patch?
Dinh
^ permalink raw reply
* Re: [PATCH net-next v7 4/5] net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA
From: Jakub Kicinski @ 2026-07-21 0:22 UTC (permalink / raw)
To: Linus Walleij
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vladimir Oltean,
David S. Miller, Eric Dumazet, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marek Vasut, Simon Horman,
Russell King, netdev, devicetree
In-Reply-To: <20260704-ks8995-to-ksz8-v7-4-2af0eaa545a8@kernel.org>
On Sat, 04 Jul 2026 21:39:36 +0200 Linus Walleij wrote:
> This adds support for the Microchip KSZ8995XA also known as the
> Micrel KS8995XA switch to the KSZ driver.
>
> Notice: there are also KSZ8995E and KSZ8995MA. These are BOTH
> different from the KSZ8995XA.
>
> The helper macros are named ksz_is_ksz8995xa() to make it
> possible to add E and MA support in the future.
Clang says:
../drivers/net/dsa/microchip/ksz8.c:263:13: warning: variable 'reg_4q' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
263 | } else if (ksz_is_ksz8995xa(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~
../drivers/net/dsa/microchip/ksz8.c:288:29: note: uninitialized use occurs here
288 | ret = ksz_prmw8(dev, port, reg_4q, mask_4q, data_4q);
| ^~~~~~
../drivers/net/dsa/microchip/ksz8.c:263:9: note: remove the 'if' if its condition is always false
263 | } else if (ksz_is_ksz8995xa(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
264 | /* This switch has no 4way split support */
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
265 | mask_2q = KSZ8795_PORT_2QUEUE_SPLIT_EN;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
266 | reg_2q = REG_PORT_CTRL_0;
| ~~~~~~~~~~~~~~~~~~~~~~~~~
267 | } else {
| ~~~~~~
../drivers/net/dsa/microchip/ksz8.c:237:11: note: initialize the variable 'reg_4q' to silence this warning
237 | u8 reg_4q, reg_2q;
| ^
| = '\0'
../drivers/net/dsa/microchip/ksz8.c:263:13: warning: variable 'mask_4q' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
263 | } else if (ksz_is_ksz8995xa(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~
../drivers/net/dsa/microchip/ksz8.c:288:37: note: uninitialized use occurs here
288 | ret = ksz_prmw8(dev, port, reg_4q, mask_4q, data_4q);
| ^~~~~~~
../drivers/net/dsa/microchip/ksz8.c:263:9: note: remove the 'if' if its condition is always false
263 | } else if (ksz_is_ksz8995xa(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
264 | /* This switch has no 4way split support */
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
265 | mask_2q = KSZ8795_PORT_2QUEUE_SPLIT_EN;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
266 | reg_2q = REG_PORT_CTRL_0;
| ~~~~~~~~~~~~~~~~~~~~~~~~~
267 | } else {
| ~~~~~~
../drivers/net/dsa/microchip/ksz8.c:236:12: note: initialize the variable 'mask_4q' to silence this warning
236 | u8 mask_4q, mask_2q;
| ^
| = '\0'
^ permalink raw reply
* Re: [PATCH 0/5] usb: typec: ps883x: fixes for older Thunderbolt 4 / USB4 docks
From: Sebastian Reichel @ 2026-07-21 0:43 UTC (permalink / raw)
To: jens.glathe
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Abel Vesa, Heikki Krogerus, Bjorn Andersson,
Konrad Dybcio, linux-usb, devicetree, linux-kernel, linux-arm-msm,
stable, Dr. David Alan Gilbert
In-Reply-To: <20260718-ps883x-disable-usb4-v1-0-cec86d0b909e@oldschoolsolutions.biz>
[-- Attachment #1: Type: text/plain, Size: 3768 bytes --]
Hello,
On Sat, Jul 18, 2026 at 07:06:28PM +0200, Jens Glathe via B4 Relay wrote:
> On Qualcomm X1E80100 platforms (e.g. Lenovo ThinkPad T14s Gen 6)
> using the Parade PS883x retimer, connecting USB4-capable docks such
> as the Lenovo 40B0 via a regular Type-C cable (which forces the dock
> into Type-C fallback mode) often results in working USB but no
> DisplayPort output.
>
> This series addresses the issue with two main changes:
>
> - Add a new optional DT property "parade,disable-usb4". When present,
> the PS883x driver rejects USB4 mode (-EOPNOTSUPP). This forces the
> Type-C stack to fall back to USB3 + DP Alt Mode, which works
> reliably with the 40B0.
>
> - Refactor DP altmode handling to also support the legacy
> TYPEC_DP_STATE_F request (deprecated since DP Alt Mode 1.0b) sent by
> the 40B0 and other docks (e.g. SSK SC220).
>
> - Add a short delay after writing configuration registers, which
> improves hotplug reliability.
>
> This is a temporary workaround until full USB4 DP tunneling support is
> available in the X1E USB4 controller and qmp-combo PHY stack.
>
> Note: The DT patch adds the new property to all currently upstream
> boards using the PS883x retimer (15 files). Happy to split it on v2
> if requested.
I don't think a kernel driver limitation is a good reason for the DT
property. I suggest to add something like this in the ps883x driver
instead:
/*
* Hamoa does not yet support USB4, disable it for now to gracefully
* fall back to USB3 + DP AltMode. This should be removed once USB4
* support landed for X1E.
*/
if (of_machine_is_compatible("qcom,x1e80100"))
disable_usb4 = true;
Greetings,
-- Sebastian
> As an additional observation, the same dock with type-c cable works well
> on Thinkpad X13s, Thinkbook 16 G7 QOY, Ideapad 5 14Q8X9, but doesn't need
> the ps883x changes (naturally).
>
> Signed-off-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> ---
> Jens Glathe (5):
> dt-bindings: usb: parade,ps8830: Add parade,disable-usb4 property
> usb: typec: ps883x: Return -EOPNOTSUPP for USB4 when parade,disable-usb4 is set
> usb: typec: mux: ps883x: refactor DP altmode handling and support TYPEC_DP_STATE_F
> usb: typec: mux: ps883x: add a delay after writing config regs
> arm64: dts: qcom: x1: disable ps883x USB4 capability
>
> .../devicetree/bindings/usb/parade,ps8830.yaml | 6 +++
> arch/arm64/boot/dts/qcom/hamoa-iot-evk.dts | 4 ++
> .../qcom/hamoa-lenovo-ideacentre-mini-01q8x10.dts | 2 +
> arch/arm64/boot/dts/qcom/purwa-iot-evk.dts | 6 +++
> arch/arm64/boot/dts/qcom/x1-asus-vivobook-s15.dtsi | 4 ++
> arch/arm64/boot/dts/qcom/x1-asus-zenbook-a14.dtsi | 4 ++
> arch/arm64/boot/dts/qcom/x1-crd.dtsi | 6 +++
> arch/arm64/boot/dts/qcom/x1-dell-thena.dtsi | 4 ++
> arch/arm64/boot/dts/qcom/x1-hp-omnibook-x14.dtsi | 2 +
> arch/arm64/boot/dts/qcom/x1-microsoft-denali.dtsi | 4 ++
> arch/arm64/boot/dts/qcom/x1e001de-devkit.dts | 6 +++
> .../dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi | 4 ++
> .../boot/dts/qcom/x1e80100-dell-xps13-9345.dts | 4 ++
> .../boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts | 6 +++
> .../dts/qcom/x1e80100-medion-sprchrgd-14-s1.dts | 2 +
> .../boot/dts/qcom/x1e80100-microsoft-romulus.dtsi | 4 ++
> drivers/usb/typec/mux/ps883x.c | 47 +++++++++++++++-------
> 17 files changed, 100 insertions(+), 15 deletions(-)
> ---
> base-commit: 0718283ab28bc3907e10b61a6b4be6fefa1cbb2f
> change-id: 20260718-ps883x-disable-usb4-19b0fc13b5ab
>
> Best regards,
> --
> Jens Glathe <jens.glathe@oldschoolsolutions.biz>
>
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* RE: [PATCH v3 3/4] dt-bindings: hwmon: (pmbus/max20830): add max20830c and max20840c support
From: Torreno, Alexis Czezar @ 2026-07-21 0:44 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev
Cc: linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
conor+dt@kernel.org, robh@kernel.org
In-Reply-To: <20260720032145.B5A6C1F000E9@smtp.kernel.org>
> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Monday, July 20, 2026 11:22 AM
> To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@analog.com>
> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> conor+dt@kernel.org; robh@kernel.org
> Subject: Re: [PATCH v3 3/4] dt-bindings: hwmon: (pmbus/max20830): add
> max20830c and max20840c support
>
> [External]
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
> consider:
> - [Medium] The max20830 driver will reject the newly added MAX20840C
> device during probe due to a hardcoded IC_DEVICE_ID check.
> --
>
> --- Patch [3]: [PATCH v3 3/4] dt-bindings: hwmon: (pmbus/max20830): add
> max20830c and max20840c support --- commit
> b032b17e50351faefc688ad31e1428a3c641197a
> Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
>
> dt-bindings: hwmon: (pmbus/max20830): add max20830c and max20840c
> support
>
> This commit adds compatible strings for the MAX20830C and MAX20840C
> variants to the max20830 device tree binding, using a fallback to adi,max20830.
>
> > diff --git
> > a/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
> > b/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
> > index 214adf1a3b9e9..b7d3b6152b582 100644
> > --- a/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
> > +++
> b/Documentation/devicetree/bindings/hwmon/pmbus/adi,max20830.yaml
> > @@ -22,7 +22,13 @@ allOf:
> >
> > properties:
> > compatible:
> > - const: adi,max20830
> > + oneOf:
> > + - const: adi,max20830
> > + - items:
> > + - enum:
> > + - adi,max20830c
> > + - adi,max20840c
>
> [Severity: Medium]
> Will the driver correctly probe for the 'adi,max20840c' variant?
>
> Looking at max20830_probe() in drivers/hwmon/pmbus/max20830.c, there is a
> hardcoded string comparison for the device ID:
>
Addressed in patch 4/4. Driver support for new variants is added in the next patch
in this series.
> drivers/hwmon/pmbus/max20830.c:max20830_probe() {
> ...
> if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
> return dev_err_probe(&client->dev, -ENODEV,
> "Unsupported device: '%s'\n", buf);
> ...
> }
>
> If a MAX20840C device is probed via the 'adi,max20830' fallback, it seems the
> buffer will start with "MAX20840", causing this check to fail and return -
> ENODEV.
>
> Does the driver need to be updated to recognize this new device ID so it does
> not reject the device?
>
> > + - const: adi,max20830
>
> --
> Sashiko AI review *
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260720-dev-
> max20830c-v3-0-9d06d27d6da3@analog.com?part=3__;Iw!!A3Ni8CS0y2Y!9Y1-
> 8bG621IAS4vRsBACPsWZTRxOMjvl6GwpDV_aXAdZxqf91aj0gx9M_D5G-
> LpeIpdqqGP6DniI1ScWRRNswxG98cP105I$
^ permalink raw reply
* RE: [PATCH v3 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
From: Torreno, Alexis Czezar @ 2026-07-21 0:44 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev
Cc: linux-hwmon@vger.kernel.org, conor+dt@kernel.org,
devicetree@vger.kernel.org, robh@kernel.org
In-Reply-To: <20260720032550.A4F891F000E9@smtp.kernel.org>
> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Monday, July 20, 2026 11:26 AM
> To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@analog.com>
> Cc: linux-hwmon@vger.kernel.org; conor+dt@kernel.org;
> devicetree@vger.kernel.org; robh@kernel.org
> Subject: Re: [PATCH v3 2/4] hwmon: (pmbus/max20830): add VOUT feedback
> resistor scaling support
>
> [External]
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
> consider:
> - [Medium] The voltage divider scaling calculation suffers from a 32-bit addition
> overflow and an unsafe 64-bit to 32-bit signed integer truncation before
> clamping.
> --
>
> --- Patch [2]: [PATCH v3 2/4] hwmon: (pmbus/max20830): add VOUT feedback
> resistor scaling support --- commit
> ac88893286286c633f117369db91c2cffd163afd
> Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
>
> hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
>
> This commit implements support for external voltage divider scaling using the
> adi,vout-rfb1-ohms and adi,vout-rfb2-ohms device tree properties. The driver
> reads these values and applies the scaling formula to the raw VOUT reading.
>
> > diff --git a/drivers/hwmon/pmbus/max20830.c
> > b/drivers/hwmon/pmbus/max20830.c index cb2c23672166d..7a5e67bd33c0a
> > 100644
> > --- a/drivers/hwmon/pmbus/max20830.c
> > +++ b/drivers/hwmon/pmbus/max20830.c
>
> [ ... ]
>
> > +static int max20830_read_word_data(struct i2c_client *client, int page,
> > + int phase, int reg)
> > +{
> > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> > + const struct max20830_data *data = container_of(info, struct
> max20830_data, info);
> > + int ret;
> > +
> > + switch (reg) {
> > + case PMBUS_READ_VOUT:
> > + ret = pmbus_read_word_data(client, page, phase, reg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Apply voltage divider scaling if resistors are non-zero */
> > + if (data->vout_rfb1 && data->vout_rfb2) {
> > + ret = DIV_ROUND_CLOSEST_ULL((u64)ret * (data-
> >vout_rfb1 +
> > + data->vout_rfb2), data-
> >vout_rfb2);
> > + ret = clamp_val(ret, 0, 0xFFFF);
>
> [Severity: Medium]
> Can this calculation suffer from a 32-bit addition overflow and an unsafe 64-bit
> to 32-bit signed integer truncation?
>
> If data->vout_rfb1 and data->vout_rfb2 sum to more than 2^32 - 1, the 32-bit
> addition will wrap around before the 64-bit multiplication.
>
> Additionally, since ret is a signed 32-bit integer, if the 64-bit result returned
> from DIV_ROUND_CLOSEST_ULL exceeds 2^31 - 1, the assignment to ret will
> overflow into a negative integer. The clamp_val() macro will then evaluate this
> negative number and incorrectly clamp it to 0 instead of 0xFFFF.
>
Although technically correct, is sashiko a bit paranoid?
Feedback resistors won't typically reach those values.
> > + }
> > + return ret;
>
> --
> Sashiko AI review *
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260720-dev-
> max20830c-v3-0-9d06d27d6da3@analog.com?part=2__;Iw!!A3Ni8CS0y2Y!-
> Nb04qCcfItlq4xUQf1LOznoGLxKoSEsno7xJWN0eYkdUVeuxpbLDHOwPO7BxUuG
> Ay8TA8LSvrg2p2pTJxd3gvxozfIGEIg$
^ permalink raw reply
* RE: [PATCH v3 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c
From: Torreno, Alexis Czezar @ 2026-07-21 0:44 UTC (permalink / raw)
To: sashiko-reviews@lists.linux.dev
Cc: robh@kernel.org, linux-hwmon@vger.kernel.org,
devicetree@vger.kernel.org, conor+dt@kernel.org
In-Reply-To: <20260720032956.5FB041F000E9@smtp.kernel.org>
> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Monday, July 20, 2026 11:30 AM
> To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@analog.com>
> Cc: robh@kernel.org; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; conor+dt@kernel.org
> Subject: Re: [PATCH v3 4/4] hwmon: (pmbus/max20830): add support for
> max20830c and max20840c
>
> [External]
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to
> consider:
> - [High] Missing compatible strings for MAX20830C and MAX20840C in the
> of_match_table.
> - [Medium] I2C block read path truncates the device ID for 9-character names,
> leading to potential false positive matches.
> - [Low] struct i2c_device_id array uses positional instead of named initializers.
> --
>
> --- Patch [4]: [PATCH v3 4/4] hwmon: (pmbus/max20830): add support for
> max20830c and max20840c --- commit
> dca1bbdb54e3f769bf22d26e97ead67754787a95
> Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
>
> hwmon: (pmbus/max20830): add support for max20830c and max20840c
>
> Add support for MAX20830C and MAX20840 step-down DC-DC switching
> regulator with PMBus interface. MAX20830C is a different packaging for
> MAX20830, and MAX20840C supports 40A regulation compared to
> MAX20830 that is only 30A.
>
> > diff --git a/drivers/hwmon/pmbus/max20830.c
> > b/drivers/hwmon/pmbus/max20830.c index 7a5e67bd33c0a..73fe59685cf15
> > 100644
> > --- a/drivers/hwmon/pmbus/max20830.c
> > +++ b/drivers/hwmon/pmbus/max20830.c
> > @@ -98,13 +98,12 @@ static int max20830_probe(struct i2c_client
> > *client)
>
> [ ... ]
>
> > } else {
> > - /* Reads 1 length byte + 9 Data bytes from MAX20830 */
> > + /* Reads 1 length byte + data bytes */
> > ret = i2c_smbus_read_i2c_block_data(client,
> PMBUS_IC_DEVICE_ID,
> >
> MAX20830_IC_DEVICE_ID_LENGTH + 1,
> > buf);
>
> [Severity: Medium]
> Since MAX20830C and MAX20840C have 9-character names, does this
> hardcoded MAX20830_IC_DEVICE_ID_LENGTH + 1 truncate the newly added
> device IDs?
>
> In max20830_probe(), if the I2C controller lacks SMBUS_READ_BLOCK_DATA
> support, the fallback i2c_smbus_read_i2c_block_data() call uses
> MAX20830_IC_DEVICE_ID_LENGTH. Because this length was initially sized for
> the 8-character "MAX20830" identifier, could it under-read the new longer
> device IDs and cause false positive matches in the I2C fallback path?
>
Non issue. MAX20830_IC_DEVICE_ID_LENGTH is already declared as 9.
The quirk is MAX20830 replies a 9th character of Null. Declaring them all as
9 length poses no issue to the string checks.
> [ ... ]
>
> > @@ -120,26 +119,28 @@ static int max20830_probe(struct i2c_client
> > *client)
>
> [ ... ]
>
> > static const struct i2c_device_id max20830_id[] = {
> > - {"max20830"},
> > + { "max20830" },
> > + { "max20830c" },
> > + { "max20840c" },
>
> [Severity: High]
> Are the compatible strings for max20830c and max20840c missing from the
> max20830_of_match[] table?
>
> Without adding them alongside the newly added i2c_device_id entries, could
> this break module autoloading on systems using Device Tree when the OS
> attempts to autoload the driver via the udev modalias?
>
max20830c and max20840c are functionally identical to max20830 and uses
the same configuration. DTS has adi,max20830 as a fallback compatible for the
new variants. Driver only needs to match the fallback compatible string.
> [Severity: Low]
> This isn't a bug, but do these entries violate the I2C subsystem coding style by
> using positional initializers?
>
> The I2C client guidelines require initialized arrays of type struct i2c_device_id to
> use named initializers (e.g., { .name = "max20830c" }).
>
max20830.c was using positional initializers, so for consistency the others were added
as positional initializers as well.
If requested, it is possible to change them all, including the original max20830, to named
initializers
> > { }
> > };
> > MODULE_DEVICE_TABLE(i2c, max20830_id);
>
> --
> Sashiko AI review *
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260720-dev-
> max20830c-v3-0-
> 9d06d27d6da3@analog.com?part=4__;Iw!!A3Ni8CS0y2Y!5gKYVqJULNay7eEZv_
> L_8_gucJb1N86NTpoIzkSsewioUyVMMHaaxOqZzCyePUogn-
> 8pmaMWp4WmbDN1rInYx6qi_lW_gTM$
^ permalink raw reply
* Re: [PATCH v9 01/14] dt-bindings: media: qcom,glymur-iris: Add glymur video codec
From: Vishnu Reddy @ 2026-07-21 1:00 UTC (permalink / raw)
To: Rob Herring; +Cc: sashiko-reviews, conor+dt, devicetree
In-Reply-To: <20260716165434.GA290489-robh@kernel.org>
On 7/16/2026 10:24 PM, Rob Herring wrote:
> On Thu, Jul 16, 2026 at 08:37:17AM +0530, Vishnu Reddy wrote:
>> On 7/15/2026 7:52 PM, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [Low] Unnecessary `#address-cells` and `#size-cells` properties on a node without addressable children.
>>> --
>>>
>>> --- Patch [1]: [PATCH v9 01/14] dt-bindings: media: qcom,glymur-iris: Add glymur video codec ---
>>> commit 8ca344fa9395fcf1d13bd1ea93de8bee21a5c176
>>> Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>>>
>>> dt-bindings: media: qcom,glymur-iris: Add glymur video codec
>>>
>>> This commit adds the device tree binding for the Qualcomm Glymur Iris video
>>> codec. It introduces a new dual-core architecture requiring additional
>>> power domains and clocks, and updates the maxItems limits in the common
>>> venus schema to accommodate these changes.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,glymur-iris.yaml b/Documentation/devicetree/bindings/media/qcom,glymur-iris.yaml
>>>> new file mode 100644
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/qcom,glymur-iris.yaml
>>>> @@ -0,0 +1,255 @@
>>> [ ... ]
>>>> + interconnect-names:
>>>> + items:
>>>> + - const: cpu-cfg
>>>> + - const: video-mem
>>>> +
>>>> + '#address-cells':
>>>> + const: 2
>>>> +
>>>> + '#size-cells':
>>>> + const: 2
>>> [Severity: Low]
>>> Are these properties necessary here? The schema strictly enforces const: 2
>>> for #address-cells and #size-cells at the root node, but none of the child
>>> nodes (non-pixel, pixel, firmware) define a reg property, and the parent
>>> does not define a ranges property.
>>>
>>> Will this cause the DeviceTree compiler (dtc) to emit an
>>> avoid_unnecessary_addr_size warning when compiling device trees using this
>>> node?
>>>
>>>> +
>>>> + non-pixel:
>>>> + type: object
>>> [ ... ]
>>>> + resets = <&gcc_video_axi0c_clk_ares>,
>>>> + <&gcc_video_axi0_clk_ares>,
>>>> + <&videocc_mvs0c_freerun_clk_ares>,
>>>> + <&videocc_mvs0_freerun_clk_ares>,
>>>> + <&gcc_video_axi1_clk_ares>,
>>>> + <&videocc_mvs1_freerun_clk_ares>;
>>>> + reset-names = "core_bus",
>>>> + "vcodec0_bus",
>>>> + "core",
>>>> + "vcodec0_core",
>>>> + "vcodec1_bus",
>>>> + "vcodec1_core";
>>>> +
>>>> + #address-cells = <2>;
>>>> + #size-cells = <2>;
>>> [Severity: Low]
>>> Does the example need #address-cells and #size-cells? It demonstrates their
>>> use without any child nodes containing reg properties, which directly causes a
>>> dtc warning during dt_binding_check validation.
>> Without #address-cells and #size-cells, getting the below kernel warning prints.
>> [ 6.953615] Missing '#address-cells' in /soc@0/video-codec@aa00000
>> [ 6.968288] WARNING: drivers/of/base.c:115 at of_bus_n_addr_cells+0xa4/0xf4
>>
>> [ 7.440909] Missing '#size-cells' in /soc@0/video-codec@aa00000
>> [ 7.447044] WARNING: drivers/of/base.c:142 at of_bus_n_size_cells+0xa4/0xf8
> Then the kernel is wrong.
Hi Rob,
Looking at the iris DT node from the link below:
https://lore.kernel.org/all/20260715-glymur-v9-13-8cf2cbe12a07@oss.qualcomm.com/
The iris is the parent node and non_pixel is the child node, which does not
have a reg property, but does have a memory-region reference containing
iommu-addresses. During of_translate_dma_region, #address-cells and
#size-cells are required from the parent node for address translation.
As per the Device Tree Specification:
https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#address-cells-and-size-cells
#address-cells and #size-cells cannot be inherited from ancestors and shall
be explicitly defined at the parent level. Therefore, adding them to the iris
DT parent node seems appropriate in this case.
Please let me know if I am misunderstanding something.
Regards,
Vishnu Reddy.
> Rob
^ permalink raw reply
* Re: [PATCH v3 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
From: Guenter Roeck @ 2026-07-21 1:05 UTC (permalink / raw)
To: Torreno, Alexis Czezar, sashiko-reviews@lists.linux.dev
Cc: linux-hwmon@vger.kernel.org, conor+dt@kernel.org,
devicetree@vger.kernel.org, robh@kernel.org
In-Reply-To: <PH0PR03MB635109584F166EBD4A5510B4F1C22@PH0PR03MB6351.namprd03.prod.outlook.com>
On 7/20/26 17:44, Torreno, Alexis Czezar wrote:
>
>
>> -----Original Message-----
>> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
>> Sent: Monday, July 20, 2026 11:26 AM
>> To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@analog.com>
>> Cc: linux-hwmon@vger.kernel.org; conor+dt@kernel.org;
>> devicetree@vger.kernel.org; robh@kernel.org
>> Subject: Re: [PATCH v3 2/4] hwmon: (pmbus/max20830): add VOUT feedback
>> resistor scaling support
>>
>> [External]
>>
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
>> consider:
>> - [Medium] The voltage divider scaling calculation suffers from a 32-bit addition
>> overflow and an unsafe 64-bit to 32-bit signed integer truncation before
>> clamping.
>> --
>>
>> --- Patch [2]: [PATCH v3 2/4] hwmon: (pmbus/max20830): add VOUT feedback
>> resistor scaling support --- commit
>> ac88893286286c633f117369db91c2cffd163afd
>> Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
>>
>> hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
>>
>> This commit implements support for external voltage divider scaling using the
>> adi,vout-rfb1-ohms and adi,vout-rfb2-ohms device tree properties. The driver
>> reads these values and applies the scaling formula to the raw VOUT reading.
>>
>>> diff --git a/drivers/hwmon/pmbus/max20830.c
>>> b/drivers/hwmon/pmbus/max20830.c index cb2c23672166d..7a5e67bd33c0a
>>> 100644
>>> --- a/drivers/hwmon/pmbus/max20830.c
>>> +++ b/drivers/hwmon/pmbus/max20830.c
>>
>> [ ... ]
>>
>>> +static int max20830_read_word_data(struct i2c_client *client, int page,
>>> + int phase, int reg)
>>> +{
>>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>>> + const struct max20830_data *data = container_of(info, struct
>> max20830_data, info);
>>> + int ret;
>>> +
>>> + switch (reg) {
>>> + case PMBUS_READ_VOUT:
>>> + ret = pmbus_read_word_data(client, page, phase, reg);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + /* Apply voltage divider scaling if resistors are non-zero */
>>> + if (data->vout_rfb1 && data->vout_rfb2) {
>>> + ret = DIV_ROUND_CLOSEST_ULL((u64)ret * (data-
>>> vout_rfb1 +
>>> + data->vout_rfb2), data-
>>> vout_rfb2);
>>> + ret = clamp_val(ret, 0, 0xFFFF);
>>
>> [Severity: Medium]
>> Can this calculation suffer from a 32-bit addition overflow and an unsafe 64-bit
>> to 32-bit signed integer truncation?
>>
>> If data->vout_rfb1 and data->vout_rfb2 sum to more than 2^32 - 1, the 32-bit
>> addition will wrap around before the 64-bit multiplication.
>>
>> Additionally, since ret is a signed 32-bit integer, if the 64-bit result returned
>> from DIV_ROUND_CLOSEST_ULL exceeds 2^31 - 1, the assignment to ret will
>> overflow into a negative integer. The clamp_val() macro will then evaluate this
>> negative number and incorrectly clamp it to 0 instead of 0xFFFF.
>>
>
> Although technically correct, is sashiko a bit paranoid?
> Feedback resistors won't typically reach those values.
>
It does what it is asked to do, which for hwmon drivers is to explicitly check
for over- and underflows because those happen surprisingly often in hwmon
drivers. There is a reason why more than 50 patches in drivers/hwmon
have either "overflow" or "underflow" in the subject line.
Guenter
^ permalink raw reply
* RE: [PATCH v3 2/4] hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
From: Torreno, Alexis Czezar @ 2026-07-21 1:13 UTC (permalink / raw)
To: Guenter Roeck, sashiko-reviews@lists.linux.dev
Cc: linux-hwmon@vger.kernel.org, conor+dt@kernel.org,
devicetree@vger.kernel.org, robh@kernel.org
In-Reply-To: <b0442a31-deda-413a-955e-1ab8c99b2033@roeck-us.net>
> >> [External]
> >>
> >> Thank you for your contribution! Sashiko AI review found 1 potential
> >> issue(s) to
> >> consider:
> >> - [Medium] The voltage divider scaling calculation suffers from a
> >> 32-bit addition overflow and an unsafe 64-bit to 32-bit signed
> >> integer truncation before clamping.
> >> --
> >>
> >> --- Patch [2]: [PATCH v3 2/4] hwmon: (pmbus/max20830): add VOUT
> >> feedback resistor scaling support --- commit
> >> ac88893286286c633f117369db91c2cffd163afd
> >> Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> >>
> >> hwmon: (pmbus/max20830): add VOUT feedback resistor scaling support
> >>
> >> This commit implements support for external voltage divider scaling
> >> using the adi,vout-rfb1-ohms and adi,vout-rfb2-ohms device tree
> >> properties. The driver reads these values and applies the scaling formula to
> the raw VOUT reading.
> >>
> >>> diff --git a/drivers/hwmon/pmbus/max20830.c
> >>> b/drivers/hwmon/pmbus/max20830.c index
> cb2c23672166d..7a5e67bd33c0a
> >>> 100644
> >>> --- a/drivers/hwmon/pmbus/max20830.c
> >>> +++ b/drivers/hwmon/pmbus/max20830.c
> >>
> >> [ ... ]
> >>
> >>> +static int max20830_read_word_data(struct i2c_client *client, int page,
> >>> + int phase, int reg)
> >>> +{
> >>> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> >>> + const struct max20830_data *data = container_of(info, struct
> >> max20830_data, info);
> >>> + int ret;
> >>> +
> >>> + switch (reg) {
> >>> + case PMBUS_READ_VOUT:
> >>> + ret = pmbus_read_word_data(client, page, phase, reg);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + /* Apply voltage divider scaling if resistors are non-zero */
> >>> + if (data->vout_rfb1 && data->vout_rfb2) {
> >>> + ret = DIV_ROUND_CLOSEST_ULL((u64)ret * (data-
> >>> vout_rfb1 +
> >>> + data->vout_rfb2), data-
> >>> vout_rfb2);
> >>> + ret = clamp_val(ret, 0, 0xFFFF);
> >>
> >> [Severity: Medium]
> >> Can this calculation suffer from a 32-bit addition overflow and an
> >> unsafe 64-bit to 32-bit signed integer truncation?
> >>
> >> If data->vout_rfb1 and data->vout_rfb2 sum to more than 2^32 - 1, the
> >> 32-bit addition will wrap around before the 64-bit multiplication.
> >>
> >> Additionally, since ret is a signed 32-bit integer, if the 64-bit
> >> result returned from DIV_ROUND_CLOSEST_ULL exceeds 2^31 - 1, the
> >> assignment to ret will overflow into a negative integer. The
> >> clamp_val() macro will then evaluate this negative number and incorrectly
> clamp it to 0 instead of 0xFFFF.
> >>
> >
> > Although technically correct, is sashiko a bit paranoid?
> > Feedback resistors won't typically reach those values.
> >
>
> It does what it is asked to do, which for hwmon drivers is to explicitly check for
> over- and underflows because those happen surprisingly often in hwmon
> drivers. There is a reason why more than 50 patches in drivers/hwmon have
> either "overflow" or "underflow" in the subject line.
>
I see, will improve this part. I'll be more mindful of this.
Thanks,
Alexis
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: iio: adc: Add AD7768
From: David Lechner @ 2026-07-21 1:39 UTC (permalink / raw)
To: Janani Sunil, Janani Sunil, Nuno Sá, Michael Hennerich,
Jonathan Cameron, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Olivier Moysan, Philipp Zabel,
Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Shuah Khan
Cc: linux, linux-iio, devicetree, linux-kernel, linux-gpio, linux-doc
In-Reply-To: <9dd16bb5-7a30-4024-88a7-4a4bf47c35e8@gmail.com>
On 7/20/26 9:00 AM, Janani Sunil wrote:
>
> On 7/9/26 17:43, David Lechner wrote:
>> On 7/9/26 3:50 AM, Janani Sunil wrote:
>>> Devicetree Bindings for AD7768-4 (4 channel) and AD7768 (8 channel)
>>> simultaneous sampling ADC
>>>
>>> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
>>> ---
>>>
>>> +
>>> + adi,power-mode:
>>> + $ref: /schemas/types.yaml#/definitions/string
>>> + enum:
>>> + - low
>>> + - median
>>> + - fast
>>> + description:
>>> + Power mode selection.
>> Unless there are pins that control this, it seems like it should be
>> left up to the driver to decide how to set this.
>>
>> In this case, it looks like the power mode also influences sample rate
>> which is normally something controlled at runtime.
Looking at this again, there is also an MCLK divider that influences
sample rate, so sampling_frequency to power mode is not straight-forward
anyway.
>
> Hi David,
>
> The reason we'd like to retain power mode control is that certain ODRs are supported across all three power modes (low/median/fast), and the RMS noise and power consumption differ significantly between them at the same ODR.
>
> The higher the power mode, the better the noise performance, but power consumption nearly doubles for every ~3 dB improvement in dynamic range. Silently selecting one power mode in the driver would remove a meaningful hardware tradeoff from the user.
>
> We'd like to propose the following instead:
> - Remove adi,power-mode from the DT as suggested.
> - Expose power mode as a per-device sysfs attribute.
> - in_voltage<N>_sampling_frequency_available dynamically reflects only the ODRs valid for the currently selected power mode.
>
> This keeps the DT clean while still giving the user explicit control over the noise versus power trade off. Would this approach be acceptable?
>
> Thanks,
> Jan
>
>
Jonathan usually pushes back against userspace power controls. We do have
this for accelerometers, but not ADCs currently.
If we can't think of anything better, maybe we could use this. It only
has low_noise and low_power options though, so the driver would still
need to chose the best power mode of the 3 based on the other requested
parameters. E.g. always make all sampling_frequency available and just
pick the highest power or lowest power mode that can provide that rate
based on the power_mode attribute.
I wanted to suggest maybe adding some kind of noise attribute instead,
but I'm not sure how we could do that in a way using SI units since the
value would depend on so many things (at least V_REF voltage, filter type,
temperature and even the physical input).
^ permalink raw reply
* [PATCH] dt-bindings: serial: 8250: Add Aspeed AST2600 and AST2700 uart compatible
From: Yu-Che Hsieh @ 2026-07-21 1:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lubomir Rintel, devicetree
Cc: linux-kernel, linux-serial, Yu-Che Hsieh
The AST2600 and AST2700 VUART controllers are identical to the AST2500
VUART controller.
Add the "aspeed,ast2600-vuart" and "aspeed,ast2700-vuart" compatible
strings and fall back to "aspeed,ast2500-vuart" for compatibility with
the existing driver.
Signed-off-by: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>
---
Documentation/devicetree/bindings/serial/8250.yaml | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index bb7b9c87a807..8c2c177faf89 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -23,7 +23,8 @@ allOf:
then:
properties:
compatible:
- const: aspeed,ast2500-vuart
+ contains:
+ const: aspeed,ast2500-vuart
- if:
properties:
compatible:
@@ -188,6 +189,11 @@ properties:
- loongson,ls2k2000-uart
- const: loongson,ls2k1500-uart
- const: ns16550a
+ - items:
+ - enum:
+ - aspeed,ast2600-vuart
+ - aspeed,ast2700-vuart
+ - const: aspeed,ast2500-vuart
reg:
maxItems: 1
---
base-commit: da7b5fd4e17f8e44c5590f2d603c01d499f056e6
change-id: 20260717-upstream-ast2700-vuart-support-d21b85ce1a52
Best regards,
--
Yu-Che Hsieh <yc_hsieh@aspeedtech.com>
^ permalink raw reply related
* Re: [PATCH 2/3] power: sequencing: pcie-m2: Match WCN6855 and WCN7851 UART BT variants by subdevice ID
From: Wei Deng @ 2026-07-21 1:51 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-arm-msm, devicetree, Manivannan Sadhasivam
In-Reply-To: <CAMRc=MeowD++93_eaYSBp0mKoY0nRROLFncLP6Ec_TdJn3jJEg@mail.gmail.com>
Hi Bart,
On Thu, 16 Jul 2026 06:56:36 -0700, Bartosz Golaszewski wrote:
> On Thu, 9 Jul 2026 09:29:41 +0200, Wei Deng <wei.deng@oss.qualcomm.com> said:
> > The WCN6855 and WCN7851 combo chips are available in M.2 card variants
> > that differ by their BT interface: some expose BT over UART while others
> > expose BT over USB. Both variants use the same PCIe device ID for the
> > WiFi interface, distinguished only by their sub-system device ID.
> >
> > Narrow the matches to UART variants only by using PCI_DEVICE_SUB with
> > their respective sub-system IDs. USB variants no longer match the table
> > and will be handled separately to deassert W_DISABLE2# for USB BT
> > enumeration.
> >
> > Signed-off-by: Wei Deng <wei.deng@oss.qualcomm.com>
>
> Do you want me to queue this independently from patch 3/3?
Yes, this can be queued independently. I'll send a new patch with an
updated commit message — the previous one mentioned "handled separately
to deassert W_DISABLE2# for USB BT enumeration" which was tied to
Patch 3/3. Since we are dropping Patch 3 in favor of Chen-Yu Tsai's
USB hub power sequencing series [1], the commit message should just say
that USB variants no longer trigger UART serdev creation.
[1] https://lore.kernel.org/all/20260715085348.3457359-1-wenst@chromium.org/
--
Best Regards,
Wei Deng
^ permalink raw reply
* Re: [PATCH v2 1/6] dt-bindings: hwmon: add binding for adi,adt7470
From: Luiz Angelo Daros de Luca @ 2026-07-21 2:12 UTC (permalink / raw)
To: Guenter Roeck
Cc: Krzysztof Kozlowski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chris Packham, Andrew Morton, Darrick J. Wong,
Uwe Kleine-König, linux-hwmon, devicetree, linux-kernel,
linux-pwm
In-Reply-To: <a8fba49c-bb72-4b3b-b598-4972304e0b4c@roeck-us.net>
> From the datasheet: "The ADT74701 controller is a multichannel
> temperature sensor and PWM fan controller and fan speed monitor".
>
> It is a fan controller which happens to use pwm signals to control
> fan speeds. It is not a PWM controller, and its pwm outputs are not
> intended to be used for anything but controlling fan speeds.
>
> Technically it is _possible_ to use the pwm outputs to control
> something else. But that does not mean that chip is a pwm controller.
> It is still a fan controller. I don't mind modeling outputs as
> non-fan-control pwm channels if that is what they are used for,
> but that must be clearly distinguished in both the driver code
> and the bindings.
Hi Guenter,
You are absolutely right. The datasheet explicitly defines the ADT7470
as a "PWM fan controller and fan speed monitor," and its internal
features (like tachometer inputs and thermal acoustic enhancement) are
heavily tailored for fan control. I agree that calling it a generic
PWM controller is technically inaccurate. I will revert the change and
describe it as a PWM Fan Controller, provided everyone is on board
with that.
Initially, I modeled the driver purely as a cooling device. However,
following Sashiko-dev's suggestion in a previous review, I converted
it to expose PWM channels so it could be used with the generic pwm-fan
driver (which indeed makes the code cleaner). My primary experience is
with DSA switches, so hwmon is a new area for me.
Whatever you and the DT maintainers agree on, I will gladly adjust for v3.
Best regards,
Luiz
^ permalink raw reply
* Re: [PATCH v3 6/8] dt-bindings: spi: dw-apb-ssi: add Axiado AX3005 SPI variant
From: Swark Yang @ 2026-07-21 2:37 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Harshit Shah,
Linus Walleij, Bartosz Golaszewski, Jan Kotas, Michal Simek,
Andi Shyti, Przemysław Gaj, Alexandre Belloni, Frank Li,
Boris Brezillon, Greg Kroah-Hartman, Jiri Slaby, Mathias Nyman,
devicetree, linux-arm-kernel, linux-kernel, linux-gpio, linux-i2c,
linux-i3c, linux-serial, linux-spi, linux-usb
In-Reply-To: <6d69cfd0-5f3b-4324-8d64-b4695cbb3e92@sirena.org.uk>
On 7/17/2026 8:02 PM, Mark Brown wrote:
> If there is no need for things to be part of a multi-subsystem series
> (eg, dependencies) it's normally better to send each subsystem
> separately.
Hi Mark,
Got it. I will split the series and send the SPI binding separately
in the next iteration.
Quick question on the versioning: should I send the newly split SPI
patch as a fresh v1, or continue with v4 to align with the rest of
the SoC series?
Best Regards,
Swark
^ permalink raw reply
* Re: [PATCH v3 6/8] dt-bindings: spi: dw-apb-ssi: add Axiado AX3005 SPI variant
From: Swark Yang @ 2026-07-21 2:47 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Harshit Shah,
Linus Walleij, Bartosz Golaszewski, Jan Kotas, Michal Simek,
Andi Shyti, Przemysław Gaj, Alexandre Belloni, Frank Li,
Boris Brezillon, Greg Kroah-Hartman, Jiri Slaby, Mark Brown,
Mathias Nyman, devicetree, linux-arm-kernel, linux-kernel,
linux-gpio, linux-i2c, linux-i3c, linux-serial, linux-spi,
linux-usb
In-Reply-To: <20260717-admirable-robust-adder-7aad70@quoll>
On 7/17/2026 4:34 PM, Krzysztof Kozlowski wrote:
> Rather: Vendor controllers compatible with v1.01a
>
> and then place it after the group with "description: Vendor controllers which
> use snps,dw-apb-ssi as fallback".
>
> Best regards,
> Krzysztof
>
Hi Krzysztof,
Will do. I'll change the first entry to an enum and move it after the
fallback group.
Note: Following Mark's advice, I will be splitting this out and sending
it separately to the SPI subsystem in the next iteration.
Best Regards,
Swark
^ permalink raw reply
* Re: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications
From: Abdurrahman Hussain @ 2026-07-21 2:49 UTC (permalink / raw)
To: sashiko-reviews, Abdurrahman Hussain; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260720071742.3F3D11F00A3A@smtp.kernel.org>
On Mon Jul 20, 2026 at 12:17 AM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
>
> New issues:
> - [Critical] Unconditional dereference of `rd->old_prop` on OF_RECONFIG_UPDATE_PROPERTY causes a NULL pointer panic.
> - [Critical] OF_RECONFIG_DETACH_NODE fails to remove dynamically allocated aliases, leading to a Memory Leak and Use-After-Free.
> - [High] Lockless modification of the global `aliases_lookup` list causes data races and memory corruption.
> - [High] Updating a boot-time alias creates duplicate entries mapping to the same ID, causing driver resource conflicts.
> - [Medium] OF_RECONFIG_ATTACH_NODE fails to scan the node's properties, silently ignoring aliases added in a newly attached /aliases node.
> - [Medium] The global `of_aliases` pointer mismanages node reference counting, causing leaks and weak references.
> - [Low] The struct alias_prop::owned documentation contains factually incorrect and misleading claims regarding memory ownership.
>
> Pre-existing issues:
> - [Critical] Creating an alias from an empty property causes a NULL pointer dereference in of_find_node_by_path().
> - [High] The alias parsing loop contains a 1-byte out-of-bounds read if the property name is an empty string.
> --
>
> --- Patch [1]: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications ---
> commit 172d1d81baebcd7bb4e961520fc11e6b4eadbc38
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> of: incrementally update /aliases lookup on reconfig notifications
>
> This commit registers an OF reconfig notifier to mirror changes in the
> /aliases node into the global aliases_lookup list. It extracts the alias
> parsing logic into a common function and tracks whether entries are statically
> or dynamically allocated. This ensures aliases defined in overlays are
> correctly resolved by drivers.
>
> Link: https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/
> Link: https://lore.kernel.org/lkml/20150630172131.D4E6CC4041A@trevor.secretlab.ca/
> Link: https://lore.kernel.org/lkml/1435675876-2159-2-git-send-email-geert+renesas@glider.be/
> Link: https://lore.kernel.org/lkml/1435675876-2159-3-git-send-email-geert+renesas@glider.be/
> Link: https://lore.kernel.org/lkml/1435675876-2159-4-git-send-email-geert+renesas@glider.be/
>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 6e7a42dedad3f..d516523a71d5e 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>
> [ ... ]
>
>> @@ -1915,6 +1915,152 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
>> +/*
>> + * Build an alias_prop for @pp using @dt_alloc as the storage allocator
>> + * and add it to aliases_lookup. @owned is stored on the entry so the
>> + * matching destroy path knows whether the alias_prop is a kmalloc'd
>> + * struct that must be kfree()d (with a paired of_node_put on the
>> + * target) or a memblock/dt_alloc'd struct that must be left alone.
>> + *
>> + * Pseudo-properties (name, phandle, ...) and alias names not ending in
>> + * a numeric id are silently skipped.
>> + */
>> +static void of_alias_create(const struct property *pp,
>> + void *(*dt_alloc)(u64 size, u64 align),
>> + bool owned)
>> +{
>> + const char *start = pp->name;
>> + const char *end = start + strlen(start);
>> + struct device_node *np;
>> + struct alias_prop *ap;
>> + int id, len;
>> +
>> + if (is_pseudo_property(pp->name))
>> + return;
>> +
>> + np = of_find_node_by_path(pp->value);
>
> [Severity: Critical]
> This is a pre-existing issue, but does this cause a NULL pointer
> dereference if the property is empty?
>
> If an overlay or boot DT defines a property in /aliases with no value,
> pp->value is NULL. Since of_alias_create() passes pp->value directly to
> of_find_node_by_path(), which passes it to strchr(), will this crash?
>
>> + if (!np)
>> + return;
>> +
>> + while (isdigit(*(end - 1)) && end > start)
>> + end--;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does the loop condition
> cause an out-of-bounds read if the property name is an empty string?
>
> If start is empty, end equals start. The loop evaluates isdigit(*(end - 1))
> before verifying end > start, causing an immediate read of the memory byte
> preceding the buffer.
>
>> + len = end - start;
>
> [ ... ]
>
>> +static void of_alias_destroy(const char *name)
>> +{
>> + struct alias_prop *ap, *tmp;
>> +
>> + list_for_each_entry_safe(ap, tmp, &aliases_lookup, link) {
>> + if (!ap->owned || strcmp(ap->alias, name) != 0)
>> + continue;
>
> [Severity: High]
> If an overlay modifies the target of an existing boot-time alias, will
> skipping the destruction of boot-time entries leave duplicate entries in
> the list?
>
> Since this skips entries where owned is false, the old boot-time alias
> remains in aliases_lookup alongside the newly created overlay alias.
> When of_alias_get_id() searches the list, could it return the same ID for
> both devices?
>
>> + list_del(&ap->link);
>> + of_node_put(ap->np);
>> + kfree(ap);
>> + return;
>> + }
>> +}
>
> [ ... ]
>
>> +static int of_aliases_reconfig_notifier(struct notifier_block *nb,
>> + unsigned long action, void *arg)
>> +{
>> + struct of_reconfig_data *rd = arg;
>> +
>> + /*
>> + * Match /aliases structurally (name + root-parent) rather than by
>> + * pointer against the of_aliases global — a system with no
>> + * boot-time /aliases (of_aliases == NULL) can still acquire one
>> + * from an overlay, and we must track its properties from the
>> + * first ATTACH_NODE onward.
>> + */
>> + if (!rd->dn || !rd->dn->parent ||
>> + !of_node_is_root(rd->dn->parent) ||
>> + !of_node_name_eq(rd->dn, "aliases"))
>> + return NOTIFY_DONE;
>> +
>> + switch (action) {
>> + case OF_RECONFIG_ATTACH_NODE:
>> + if (!of_aliases)
>> + of_aliases = rd->dn;
>
> [Severity: Medium]
> Does assigning of_aliases here create a weak reference?
>
> During boot-time scanning, of_alias_scan() acquires of_aliases with an
> incremented reference count. Here, it is assigned without calling
> of_node_get().
>
> [Severity: Medium]
> Also, if the system started without a boot-time /aliases node, does
> attaching one fail to parse its properties?
>
> Since this handler updates the of_aliases pointer but misses the
> for_each_property_of_node() loop to actually create the aliases, it seems
> the aliases in a newly attached node are silently ignored.
>
>> + break;
>> + case OF_RECONFIG_DETACH_NODE:
>> + if (of_aliases == rd->dn)
>> + of_aliases = NULL;
>
> [Severity: Medium]
> Does clearing the pointer here cause a reference leak since of_node_put()
> is omitted?
>
> [Severity: Critical]
> Additionally, does skipping the removal of the dynamic aliases cause a
> memory leak and use-after-free?
>
> The DETACH_NODE handler clears the of_aliases pointer but skips removing
> the corresponding alias_prop entries from aliases_lookup. When the detached
> node and its properties are later freed, dangling entries will remain in
> the global list. A subsequent alias update could iterate the list and call
> strcmp() on freed pointer strings.
>
>> + break;
>> + case OF_RECONFIG_ADD_PROPERTY:
>> + of_alias_create(rd->prop, alias_alloc, true);
>
> [Severity: High]
> Is it safe to modify aliases_lookup locklessly here?
>
> The notifier processes dynamic changes and calls into of_alias_create()
> which uses list_add_tail() on the global aliases_lookup list. OF reconfig
> notifiers are intentionally executed after of_mutex has been unlocked, such
> as in __of_changeset_apply_notify().
>
> Could this race against other writers, or against driver probes calling
> of_alias_get_id() which iterates the list expecting it to be protected by
> of_mutex?
>
>> + break;
>> + case OF_RECONFIG_REMOVE_PROPERTY:
>> + of_alias_destroy(rd->prop->name);
>> + break;
>> + case OF_RECONFIG_UPDATE_PROPERTY:
>> + of_alias_destroy(rd->old_prop->name);
>
> [Severity: Critical]
> Can rd->old_prop be NULL here?
>
> The of_update_property() API allows adding a property if it does not
> already exist, and emits an OF_RECONFIG_UPDATE_PROPERTY event with
> old_prop set to NULL. Would unconditionally dereferencing
> rd->old_prop->name cause a panic in this scenario?
>
>> + of_alias_create(rd->prop, alias_alloc, true);
>> + break;
>> + default:
>> + break;
>> + }
>> + return NOTIFY_OK;
>> +}
>
> [ ... ]
>
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index 0ae16da066e2c..9d16765ae2c3f 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -17,6 +17,12 @@
>> * @alias: Alias property name
>> * @np: Pointer to device_node that the alias stands for
>> * @id: Index value from end of alias name
>> + * @owned: True if @alias was kstrdup'd and @np was of_node_get'd on
>> + * insertion (overlay-time entries). False for entries built
>> + * by of_alias_scan() at boot, where @alias points into the
>> + * FDT and @np is an unreferenced pointer. The removal path
>> + * uses this flag to decide whether it must kfree(@alias),
>> + * of_node_put(@np), and kfree(the struct itself).
>
> [Severity: Low]
> Does this documentation accurately reflect the implementation?
>
> The comment claims @alias is kstrdup'd on insertion and kfree'd on
> removal. However, the code merely stores the pp->name pointer directly
> and never calls kfree() on the alias string. It also mentions np is
> unreferenced at boot time, but of_find_node_by_path() does take a
> reference.
Good feedback! Will address in v2.
Thanks,
Abdurrahman
^ permalink raw reply
* Re: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths
From: Abdurrahman Hussain @ 2026-07-21 2:51 UTC (permalink / raw)
To: sashiko-reviews, Abdurrahman Hussain; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260720072014.E40481F000E9@smtp.kernel.org>
On Mon Jul 20, 2026 at 12:20 AM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Fallback to `__of_prop_dup()` for `/aliases` masks ENOMEM errors and bypasses null-termination safety checks, causing silent corruption and potential OOB reads.
>
> Pre-existing issues:
> - [Critical] Dangling pointer in target->np->deadprops leads to use-after-free and double-free if of_changeset_add_property() fails.
> --
>
> --- Patch [3]: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths ---
> commit 8741f5ffd2983da599c7be77cbf8c1af8325d4be
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> of/overlay: rewrite /aliases path values to live-tree paths
>
> /aliases entries added by an overlay reference labeled nodes inside
> the overlay via '&label' in the .dtso. dtc renders those references
> as string paths at compile time, but the paths encode the overlay's
> internal fragment layout (e.g. "/fragment@1/__overlay__/fpga@0/i2c@40000")
> rather than the location where the node will live after apply.
>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 654a70d5cb075..338786cef17ab 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -350,6 +350,19 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>> if (prop)
>> return -EINVAL;
>> new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
>> + } else if (target->np->parent &&
>> + of_node_is_root(target->np->parent) &&
>> + of_node_name_eq(target->np, "aliases")) {
>> + /*
>> + * /aliases property values are string paths in the same
>> + * "/fragment@N/__overlay__/..." form as /__symbols__.
>> + * Rewrite them to live-tree paths so of_alias_get_id() can
>> + * resolve them; fall back to a raw dup for non-fragment
>> + * values (e.g. legacy string aliases like "ttyS0").
>> + */
>> + new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
>> + if (!new_prop)
>> + new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>
> [Severity: High]
> If dup_and_fixup_symbol_prop() fails because the string is not
> null-terminated, does this fallback to __of_prop_dup() end up copying the
> malformed string?
>
> Because dup_and_fixup_symbol_prop() can return NULL for both memory
> allocation failures and structural errors like non-null-terminated strings,
> falling back to a raw duplicate here preserves the non-null-terminated
> string.
>
> Later, the reconfig notifier can trigger of_alias_create() which attempts to
> read the property:
>
> drivers/of/base.c:of_alias_create() {
> ...
> if (owned)
> np = of_find_node_by_path(pp->value);
> ...
> }
>
> Could this lead to an out-of-bounds read when of_find_node_by_path() treats
> the non-null-terminated property value as a C string?
>
> Additionally, does this mask memory allocation failures from
> dup_and_fixup_symbol_prop() by falling back to a raw duplicate, injecting
> unresolved paths directly into the live tree instead of failing the overlay
> application?
>
>> } else {
>> new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>> }
>
> [Severity: Critical]
> This is a pre-existing issue, but while reviewing add_changeset_property(), I
> noticed a problem further down in the same function.
>
> When a property is added to a newly created node, it is linked into the
> node's deadprops list before the changeset operation:
>
> drivers/of/overlay.c:add_changeset_property() {
> ...
> if (!prop) {
> if (!target->in_livetree) {
> new_prop->next = target->np->deadprops;
> target->np->deadprops = new_prop;
> }
> ret = of_changeset_add_property(&ovcs->cset, target->np,
> new_prop);
> ...
> if (ret)
> __of_prop_free(new_prop);
> return ret;
> }
>
> If of_changeset_add_property() fails, perhaps due to an allocation error,
> __of_prop_free() is called on new_prop, but new_prop is not unlinked from
> target->np->deadprops.
>
> Will this leave a dangling pointer in the deadprops list, causing a double
> free later when the changeset is aborted and of_node_release() calls
> property_list_free() on node->deadprops?
Will address in v2
^ permalink raw reply
* Re: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert
From: Abdurrahman Hussain @ 2026-07-21 2:51 UTC (permalink / raw)
To: sashiko-reviews, Abdurrahman Hussain; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260720071020.4E1C61F000E9@smtp.kernel.org>
On Mon Jul 20, 2026 at 12:10 AM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The test claims to exercise patch 2's logic but bypasses it by passing NULL for target_base.
> - [Medium] The test claims to exercise patch 3's logic but bypasses it by using an absolute live-tree path.
> --
>
> --- Patch [4]: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert ---
> commit 1237920f0bd1d803a084dc2f03f9e63aaf5cd6a2
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> of: unittest: cover /aliases updates from overlay apply/revert
>
> This commit adds an overlay and a test runner to verify alias creation and
> removal dynamically during device tree overlay apply and revert operations.
> It asserts that the alias is correctly added and then removed.
>
>> exercises all three functional patches earlier in this series
>> together: patch 1's reconfig notifier is what mutates aliases_lookup
>> on apply/revert, patch 2 is what lets target-path="/aliases" resolve
>> to the DT root when target_base is non-NULL, and patch 3 is what
>> rewrites the fragment-internal path in the alias value so
>> of_find_node_by_path() finds the live-tree target. Any one of the
>> three missing turns the middle assertion (get_id -> 99) into
>> -ENODEV.
>
> [Severity: Medium]
> Is this description of the test coverage accurate?
>
> As detailed below, the test appears to bypass the logic for both patch 2 and
> patch 3. The overlay uses an absolute path instead of an intra-overlay
> reference (bypassing patch 3), and overlay_data_apply() passes NULL for
> target_base (bypassing patch 2).
>
>> diff --git a/drivers/of/unittest-data/overlay_alias.dtso b/drivers/of/unittest-data/overlay_alias.dtso
>> new file mode 100644
>> index 0000000000000..32532c80505a7
>> --- /dev/null
>> +++ b/drivers/of/unittest-data/overlay_alias.dtso
>> @@ -0,0 +1,9 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +/* overlay_alias - declare an alias inside an overlay */
>> +
>> +&{/aliases} {
>> + testcase-alias99 = "/testcase-data/overlay-node/test-bus/test-unittest100";
>
> [Severity: Medium]
> Does this overlay actually test the path rewrite logic from patch 3?
>
> The testcase-alias99 property is defined using an absolute live-tree path
> rather than an intra-overlay fragment reference. Because this is already a
> valid absolute path, it seems of_alias_create() will successfully resolve it
> via of_find_node_by_path() even if the rewrite logic from patch 3 is
> missing.
>
>> +};
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index e255f54f4d760..a9f4a0bb30872 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
> [ ... ]
>> @@ -3475,6 +3475,53 @@ static struct notifier_block of_nb = {
>> .notifier_call = of_notify,
>> };
>>
>> +static void __init of_unittest_overlay_alias(void)
>> +{
>> + const char *path = "/testcase-data/overlay-node/test-bus/test-unittest100";
>> + struct device_node *np;
>> + int ovcs_id = 0;
>> + int id, ret;
>> +
>> + np = of_find_node_by_path(path);
>> + if (!np) {
>> + unittest(0, "could not find %s for alias test\n", path);
>> + return;
>> + }
>> +
>> + id = of_alias_get_id(np, "testcase-alias");
>> + if (id != -ENODEV) {
>> + unittest(0,
>> + "of_alias_get_id() = %d before overlay, expected -ENODEV\n",
>> + id);
>> + goto out;
>> + }
>> +
>> + ret = overlay_data_apply("overlay_alias", &ovcs_id);
>
> [Severity: Medium]
> Does this test actually exercise the target_base logic from patch 2?
>
> Looking at overlay_data_apply(), it calls of_overlay_fdt_apply() with a
> hardcoded NULL for target_base:
>
> drivers/of/unittest.c:overlay_data_apply() {
> ...
> ret = of_overlay_fdt_apply(info->dtbo_begin, size, &info->ovcs_id,
> NULL);
> ...
> }
>
> When target_base is NULL, the pre-patch-2 code already resolves
> target-path="/aliases" correctly via of_find_node_by_path(). This means the
> test might pass and bypass patch 2 entirely, missing the intended test
> coverage.
>
>> + if (!ret) {
>> + unittest(0, "overlay_alias apply failed\n");
>> + goto out;
>> + }
> [ ... ]
Will address in v2.
^ permalink raw reply
* [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync
From: Abdurrahman Hussain @ 2026-07-21 2:52 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
/aliases entries added by a device-tree overlay are stored in the live
tree but never enter the global aliases_lookup list that of_alias_scan()
builds at boot. As a result, of_alias_get_id() returns -ENODEV for
aliases declared inside overlays, and any driver that relies on
alias-based numbering (i2c-xiic, spi, tty, mmc, ...) silently loses its
pinned id and falls back to auto-assignment.
The gap has been public since 2015 [1] and reproduces trivially: apply
an overlay that declares e.g. `i2c99 = &foo;`, ask
of_alias_get_id(foo, "i2c") -> -ENODEV. Bootlin's ELCE 2025 talk on
PCI DT overlays [2] enumerates "i2c muxes" as one of the subsystems
broken by dynamic overlays; alias pinning is the underlying cause.
The core fix (patch 1) is a reconfig notifier that mirrors /aliases
property changes into aliases_lookup. Two smaller overlay-code fixes
(patches 2 and 3) fall out of the same use case: without them, the
notifier alone can't actually resolve overlay-declared aliases.
Prior art
---------
Geert Uytterhoeven posted a 3-patch RFC in June 2015 [1] with the same
alias-tracking design shape. Grant Likely reviewed positively; merge
was gated on missing unittests and an object-lifetime concern the
author self-flagged, and the series was never reposted as non-RFC. Ten
years later, drivers/of/overlay.c still contains zero references to
aliases, of_alias_scan, or aliases_lookup.
Series contents
---------------
Patch 1 adds a reconfig notifier that mirrors /aliases property
changes into aliases_lookup. It also refactors of_alias_scan()'s
per-property body into a helper of_alias_create() shared by the
boot-time scan and the runtime notifier. A one-bit `owned` flag on
struct alias_prop distinguishes kmalloc'd (overlay-time) entries
(kstrdup'd alias name, of_node_get'd target) from memblock-backed
(boot-time) ones so the remove path can't kfree the wrong storage.
The notifier keys off structural properties of the target node
(name + root-parent) rather than the of_aliases global, so overlays
that create /aliases from scratch on a system without a boot-time
aliases node are covered too. All aliases_lookup readers and writers
serialize on a dedicated mutex.
Patch 2 fixes find_target() so an overlay applied with a non-NULL
target base can still reach the DT root via target-path="/foo". The
current code unconditionally concatenates base + target-path via
"%pOF%s", so target-path="/aliases" resolves to "<base>/aliases" and
target-path="/" produces "<base>/" (never a valid node). After this
patch, an empty target-path continues to mean "the target base
itself" — preserving the shape used by drivers/misc/lan966x_pci.c,
the only in-tree caller of of_overlay_fdt_apply() that passes a
non-NULL base — while any non-empty target-path is looked up
absolutely.
Patch 3 rewrites /aliases property values from the overlay's internal
fragment path ("/fragment@N/__overlay__/...") to the live-tree path
that the node will occupy after apply. The overlay code already does
this for /__symbols__ via dup_and_fixup_symbol_prop(); /aliases uses
the same textual convention and can reuse the same helper. Without
this, patch 1's notifier receives paths that never resolve in the
live tree, so of_alias_get_id() still returns -ENODEV.
Patch 4 adds a unittest — overlay_alias.dtso plus
of_unittest_overlay_alias() — that applies an overlay with a
non-NULL target_base, declaring a labeled node under the base and an
`testcase-alias99 = &that-label` entry in DT-root /aliases, then
asserts of_alias_get_id() flips from -ENODEV to 99 across the apply,
and back to -ENODEV across the revert. Missing any one of the three
functional patches (notifier / absolute-target-paths / alias path
rewrite) causes the assertion to fail.
Changes in v2
-------------
Addresses the automated review of the RFC posting.
Patch 1:
- Guard rd->old_prop before dereferencing in the UPDATE_PROPERTY
handler; some notifier producers pass NULL.
- Handle OF_RECONFIG_ATTACH_NODE and OF_RECONFIG_DETACH_NODE for
/aliases nodes attached or detached with pre-populated properties;
scan (or purge) each property so per-property ADD/REMOVE events
aren't required.
- Serialize aliases_lookup with a dedicated aliases_mutex; readers
(of_alias_get_id, of_alias_get_highest_id) and the reconfig
notifier both hold it. Boot-time of_alias_scan() stays lockless
(single-threaded during init).
- Destroy path unlinks matching entries irrespective of ownership
(freeing storage only for owned ones), so an overlay UPDATE
against a boot-time alias no longer leaves duplicate stem+id
entries in aliases_lookup.
- Owned entries kstrdup the alias name and hold an of_node_get()
reference on the target — released in the destroy path — so a
property freed by overlay revert can't dangle into aliases_lookup.
- of_aliases is refcounted lazily on ATTACH/DETACH.
- Validate pp->value is non-empty and null-terminated within
pp->length before feeding it to of_find_node_by_path() — prevents
a stray malformed alias entry from causing an OOB read.
Patch 3:
- Only route /aliases properties through dup_and_fixup_symbol_prop()
when the value looks like a fragment-internal path (non-empty,
null-terminated, "/fragment@" prefix). Other alias values pass
through the raw duplicator unchanged. This removes the previous
unconditional fallback that would silently propagate a raw dup on
ENOMEM, and also stops the notifier from ever seeing malformed
values through this path.
Patch 4:
- Rewrite the .dtso to actually exercise all three functional
patches: a labeled node under a fragment with target-path="" plus
an /aliases fragment with target-path="/aliases" (absolute) and
a value referencing the label via `&`. The RFC test bypassed
patches 2 and 3.
- Rewrite the runner to call of_overlay_fdt_apply() directly with a
non-NULL target_base, look up the grafted node by its post-apply
live-tree path, and assert against it.
Open items
----------
None outstanding. Pre-existing issues surfaced by the automated review
(overlay-changeset leak paths, of_node_put()-under-devtree_lock, the
lan966x_pci_probe cleanup gap) are unrelated to this series and are
being tracked separately.
Verification
------------
Series was applied to v7.2-rc3 and boot-tested on an x86 platform
with two PCI-attached Xilinx FPGAs whose i2c-xiic controllers come
from driver-embedded DT overlays. Before this series, i2c bus
numbers auto-assigned regardless of what /aliases said and shifted
across boots depending on which FPGA won the probe race. After: 89
adapters, `/dev/i2c-N` numbering matches the overlay aliases exactly
and is stable across reboots, `sensors` reads live telemetry through
mux channels, and dmesg is free of WARN/BUG/Oops. Patch 4's unittest
passes.
[1] https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/
Geert Uytterhoeven, "[PATCH/RFC 0/3] of/overlay: Update aliases
when added or removed", 2015-06-30.
[2] Hervé Codina, "Using Device Tree Overlays to Support Complex PCI
Devices in Linux", ELCE 2025.
https://bootlin.com/pub/conferences/2025/elce/codina-pcie-dt-overlay.pdf
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Abdurrahman Hussain (4):
of: incrementally update /aliases lookup on reconfig notifications
of/overlay: look up absolute target-paths absolutely
of/overlay: rewrite /aliases path values to live-tree paths
of: unittest: cover /aliases updates from overlay apply/revert
drivers/of/base.c | 271 ++++++++++++++++++++++++----
drivers/of/of_private.h | 7 +
drivers/of/overlay.c | 61 +++++--
drivers/of/unittest-data/Makefile | 2 +
drivers/of/unittest-data/overlay_alias.dtso | 36 ++++
drivers/of/unittest.c | 74 ++++++++
6 files changed, 393 insertions(+), 58 deletions(-)
---
base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260719-nh-of-alias-overlay-6e5e0d56212b
Best regards,
--
Abdurrahman Hussain <abdurrahman@nexthop.ai>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox