* [PATCH 5/8] clk: clocking-wizard: Move clk_in1 acquisition before static-config check
From: Shubhrajyoti Datta @ 2026-06-15 3:48 UTC (permalink / raw)
To: linux-clk, linux-kernel
Cc: git, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek,
Shubhrajyoti Datta, devicetree, linux-arm-kernel
In-Reply-To: <20260615034845.3320286-1-shubhrajyoti.datta@amd.com>
The clk_in1 is the input clock for both the dynamic reconfig and the
static-config paths. Acquire clk_in1 for static-config as well. Output
clocks are registered as fixed-factor children of clk_in1.
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index e082051221be..ffc78c90bee6 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1178,6 +1178,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
return -EINVAL;
}
+ clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
+ if (IS_ERR(clk_wzrd->clk_in1))
+ return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
+ "failed to get clk_in1\n");
+
if (!of_property_present(np, "xlnx,static-config")) {
clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(clk_wzrd->base))
@@ -1192,11 +1197,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
}
}
- clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
- if (IS_ERR(clk_wzrd->clk_in1))
- return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
- "clk_in1 not found\n");
-
ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs);
if (ret)
return ret;
--
2.49.1
^ permalink raw reply related
* [PATCH 6/8] clk: clocking-wizard: Add static-config clock provider support
From: Shubhrajyoti Datta @ 2026-06-15 3:48 UTC (permalink / raw)
To: linux-clk, linux-kernel
Cc: git, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek,
Shubhrajyoti Datta, devicetree, linux-arm-kernel
In-Reply-To: <20260615034845.3320286-1-shubhrajyoti.datta@amd.com>
When xlnx,static-config is present the divider/multiplier path is
synthesized inside the Wizard without exposing runtime MMIO
reconfiguration, so omit the AXI register mapping and advertise each
routed output clock as a clk_fixed_factor child of clk_in1 using the
synthesized ratios exported through xlnx,clk-mul-div.
However the parent clock of clk_in1 can still be gated, disabled, or
(re-)enabled after FPGA programming (typical FPGA Manager flows,
CCF-managed parents, PLL fabric power sequencing).
Add a 10 us delay in each output clock's .enable() hook so consumers
wait for reference settling before first access.
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 137 ++++++++++++++++++++-
1 file changed, 136 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index ffc78c90bee6..5470a717fccc 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -12,6 +12,7 @@
#include <linux/platform_device.h>
#include <linux/clk.h>
#include <linux/clk-provider.h>
+#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/io.h>
#include <linux/of.h>
@@ -107,6 +108,7 @@
#define VER_WZRD_O_MAX 511
#define WZRD_MIN_ERR 20000
#define WZRD_FRAC_POINTS 1000
+#define WZRD_ENABLE_DELAY_US 10
/* Get the mask from width */
#define div_mask(width) ((1 << (width)) - 1)
@@ -697,6 +699,13 @@ static int clk_wzrd_ver_determine_rate_all(struct clk_hw *hw,
return 0;
}
+static int clk_wzrd_enable(struct clk_hw *hw)
+{
+ /* Allow the output clock to settle after enable */
+ udelay(WZRD_ENABLE_DELAY_US);
+ return 0;
+}
+
static const struct clk_ops clk_wzrd_ver_divider_ops = {
.determine_rate = clk_wzrd_determine_rate,
.set_rate = clk_wzrd_ver_dynamic_reconfig,
@@ -790,6 +799,85 @@ static const struct clk_ops clk_wzrd_clk_divider_ops_f = {
.recalc_rate = clk_wzrd_recalc_ratef,
};
+static unsigned long
+clk_wzrd_static_factor_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+ struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
+ unsigned long long rate;
+
+ rate = (unsigned long long)parent_rate * fix->mult;
+ do_div(rate, fix->div);
+ return (unsigned long)rate;
+}
+
+static int clk_wzrd_static_factor_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req)
+{
+ struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
+
+ if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+ unsigned long best_parent;
+
+ best_parent = (req->rate / fix->mult) * fix->div;
+ req->best_parent_rate =
+ clk_hw_round_rate(clk_hw_get_parent(hw), best_parent);
+ }
+
+ req->rate = (req->best_parent_rate / fix->div) * fix->mult;
+
+ return 0;
+}
+
+static int clk_wzrd_static_factor_set_rate(struct clk_hw *hw,
+ unsigned long rate,
+ unsigned long parent_rate)
+{
+ return 0;
+}
+
+static const struct clk_ops clk_wzrd_static_fixed_factor_ops = {
+ .enable = clk_wzrd_enable,
+ .determine_rate = clk_wzrd_static_factor_determine_rate,
+ .set_rate = clk_wzrd_static_factor_set_rate,
+ .recalc_rate = clk_wzrd_static_factor_recalc_rate,
+};
+
+static struct clk_hw *
+clk_wzrd_devm_register_static_fixed_factor(struct device *dev,
+ const char *name,
+ const struct clk_parent_data *parent_data,
+ unsigned long flags,
+ unsigned int mult,
+ unsigned int div)
+{
+ struct clk_init_data init = {};
+ struct clk_fixed_factor *fix;
+ struct clk_hw *hw;
+ int ret;
+
+ fix = devm_kzalloc(dev, sizeof(*fix), GFP_KERNEL);
+ if (!fix)
+ return ERR_PTR(-ENOMEM);
+
+ fix->mult = mult;
+ fix->div = div;
+
+ init.name = name;
+ init.ops = &clk_wzrd_static_fixed_factor_ops;
+ init.flags = flags;
+ init.parent_data = parent_data;
+ init.num_parents = 1;
+
+ fix->hw.init = &init;
+
+ hw = &fix->hw;
+ ret = devm_clk_hw_register(dev, hw);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return hw;
+}
+
static struct clk_hw *clk_wzrd_register_divf(struct device *dev,
const char *name,
const char *parent_name,
@@ -1154,9 +1242,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct clk_wzrd *clk_wzrd;
+ const char *clk_name;
unsigned long rate;
+ struct clk_hw *hw;
int nr_outputs;
- int ret;
+ int ret, i;
ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs);
if (ret || nr_outputs > WZRD_NUM_OUTPUTS)
@@ -1224,6 +1314,51 @@ static int clk_wzrd_probe(struct platform_device *pdev)
dev_warn(&pdev->dev,
"unable to register clock notifier\n");
}
+ } else {
+ u32 mul_div[WZRD_NUM_OUTPUTS * 2];
+ const struct clk_parent_data parent_data = { .fw_name = "clk_in1" };
+ int num_elems = nr_outputs * 2;
+
+ /*
+ * xlnx,clk-mul-div is a uint32-matrix of <mul div> pairs;
+ * FDT encodes it as a flat u32 array so we can read it directly.
+ */
+ ret = of_property_read_u32_array(np, "xlnx,clk-mul-div",
+ mul_div, num_elems);
+ if (ret) {
+ dev_err(&pdev->dev, "xlnx,clk-mul-div missing or invalid\n");
+ return ret;
+ }
+
+ for (i = 0; i < nr_outputs; i++) {
+ u32 mul = mul_div[2 * i];
+ u32 div = mul_div[2 * i + 1];
+
+ if (!mul || !div)
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "invalid mul/div for clkout%d\n", i);
+
+ clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+ "%s_out%d", dev_name(&pdev->dev), i);
+ if (!clk_name)
+ return -ENOMEM;
+
+ hw = clk_wzrd_devm_register_static_fixed_factor(&pdev->dev, clk_name,
+ &parent_data,
+ CLK_SET_RATE_PARENT,
+ mul, div);
+ if (IS_ERR(hw))
+ return PTR_ERR(hw);
+ clk_wzrd->clk_data.hws[i] = hw;
+ }
+
+ clk_wzrd->clk_data.num = nr_outputs;
+
+ ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
+ &clk_wzrd->clk_data);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "unable to register clock provider\n");
}
return 0;
--
2.49.1
^ permalink raw reply related
* [PATCH 7/8] clk: clocking-wizard: Skip s_axi_aclk for static-config
From: Shubhrajyoti Datta @ 2026-06-15 3:48 UTC (permalink / raw)
To: linux-clk, linux-kernel
Cc: git, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek,
Shubhrajyoti Datta, devicetree, linux-arm-kernel
In-Reply-To: <20260615034845.3320286-1-shubhrajyoti.datta@amd.com>
For static-config mode the AXI bus interface is not used, so there is
no need to get and enable s_axi_aclk. Move the axi_clk setup inside
the non-static-config branch.
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 23 +++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index 5470a717fccc..fe73ee02b54e 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1243,7 +1243,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
struct device_node *np = pdev->dev.of_node;
struct clk_wzrd *clk_wzrd;
const char *clk_name;
- unsigned long rate;
struct clk_hw *hw;
int nr_outputs;
int ret, i;
@@ -1258,22 +1257,24 @@ static int clk_wzrd_probe(struct platform_device *pdev)
return -ENOMEM;
platform_set_drvdata(pdev, clk_wzrd);
- clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
- if (IS_ERR(clk_wzrd->axi_clk))
- return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
- "s_axi_aclk not found\n");
- rate = clk_get_rate(clk_wzrd->axi_clk);
- if (rate > WZRD_ACLK_MAX_FREQ) {
- dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", rate);
- return -EINVAL;
- }
-
clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
if (IS_ERR(clk_wzrd->clk_in1))
return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
"failed to get clk_in1\n");
if (!of_property_present(np, "xlnx,static-config")) {
+ unsigned long rate;
+
+ clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
+ if (IS_ERR(clk_wzrd->axi_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
+ "s_axi_aclk not found\n");
+ rate = clk_get_rate(clk_wzrd->axi_clk);
+ if (rate > WZRD_ACLK_MAX_FREQ) {
+ dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", rate);
+ return -EINVAL;
+ }
+
clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(clk_wzrd->base))
return PTR_ERR(clk_wzrd->base);
--
2.49.1
^ permalink raw reply related
* [PATCH 8/8] clk: clocking-wizard: Use dev_err_probe() when mapping registers
From: Shubhrajyoti Datta @ 2026-06-15 3:48 UTC (permalink / raw)
To: linux-clk, linux-kernel
Cc: git, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek,
Shubhrajyoti Datta, devicetree, linux-arm-kernel
In-Reply-To: <20260615034845.3320286-1-shubhrajyoti.datta@amd.com>
Align the devm_platform_ioremap_resource() error path with clk_in1 and
s_axi_aclk handling for consistent logging and deferred-probe behavior.
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index fe73ee02b54e..381e396aef0e 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1277,7 +1277,8 @@ static int clk_wzrd_probe(struct platform_device *pdev)
clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(clk_wzrd->base))
- return PTR_ERR(clk_wzrd->base);
+ return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->base),
+ "failed to map registers\n");
ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
if (!ret) {
--
2.49.1
^ permalink raw reply related
* Re: [PATCH RFC 3/9] net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass
From: Mohd Ayaan Anwar @ 2026-06-15 3:54 UTC (permalink / raw)
To: Andrew Lunn
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Richard Cochran, Bjorn Andersson, Konrad Dybcio, Maxime Coquelin,
Alexandre Torgue, Russell King, linux-arm-msm, netdev, devicetree,
linux-kernel, linux-stm32, linux-arm-kernel
In-Reply-To: <42355330-c22a-4fce-98ab-dc22b321ff16@lunn.ch>
Hello Andrew,
On Thu, Jun 11, 2026 at 10:54:37PM +0200, Andrew Lunn wrote:
> On Fri, Jun 12, 2026 at 12:06:59AM +0530, Mohd Ayaan Anwar wrote:
> > When "rgmii-id" is selected the PHY supplies both TX and RX delays, so
> > the MAC must not add its own. The driver currently falls through to the
> > generic DLL initialisation path which programs it to add a delay.
> >
> > Power down the DLL and set DDR bypass mode for RGMII_ID, then program
> > the IO_MACRO via a new ethqos_rgmii_id_macro_init() helper. Also fix
> > ethqos_set_clk_tx_rate() to not double the clock rate in bypass mode at
> > 100M/10M, and remove RGMII_ID from the phase-shift suppression in
> > ethqos_rgmii_macro_init() since RGMII_ID no longer reaches that path.
>
> I'm curious how this works at the moment? Do no boards make use of
> RGMII ID? Are all current boards broken?
Searching through the DTS, I found that we have two boards using "rgmii"
(qcs404-evb-4000.dts and sa8155-adp.dts) and another board using
"rgmii-txid" (sa8540p-ride.dts). No board which uses RGMII ID.
I don't think any of these boards have extra long wires which would add
PCB level delay. They are against the netdev definitions for "rgmii" and
"rgmii-txid".
But the first two boards should still be working fine since the current
driver programs the IO_MACRO to add the delay when operating in RGMII
mode. I am not sure about the last board. I went through the different
versions of the ETHQOS programming guide, and it should reliably support
either only MAC side Rx/Tx delay -or- bypass mode (no MAC side delay),
with each having different clock requirements.
Ayaan
^ permalink raw reply
* Re: [PATCH 8/8] clk: clocking-wizard: Use dev_err_probe() when mapping registers
From: sashiko-bot @ 2026-06-15 3:54 UTC (permalink / raw)
To: Shubhrajyoti Datta; +Cc: devicetree, conor+dt, robh
In-Reply-To: <20260615034845.3320286-9-shubhrajyoti.datta@amd.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Redundant error logging and incorrect commit justification for devm_platform_ioremap_resource()
--
commit fc6668cc718e0e8c82476eab0f48f985072a67de
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
clk: clocking-wizard: Use dev_err_probe() when mapping registers
This commit wraps the error return of devm_platform_ioremap_resource() in
dev_err_probe() to align with other error paths. The commit message states
this is for consistent logging and deferred-probe behavior.
> Align the devm_platform_ioremap_resource() error path with clk_in1 and
> s_axi_aclk handling for consistent logging and deferred-probe behavior.
[Severity: Medium]
Is the deferred-probe justification accurate for this specific function?
Since devm_platform_ioremap_resource() handles memory resources, it does
not return -EPROBE_DEFER.
> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index fe73ee02b54ee..381e396aef0e7 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -1277,7 +1277,8 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>
> clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(clk_wzrd->base))
> - return PTR_ERR(clk_wzrd->base);
> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->base),
> + "failed to map registers\n");
[Severity: Medium]
Does this result in duplicate error messages in the kernel log?
The devm_platform_ioremap_resource() helper internally uses dev_err_probe()
to log its own detailed error messages for missing resources, request
failures, and ioremap failures. Wrapping the return value with an additional
dev_err_probe() in the driver might emit a redundant error string.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615034845.3320286-1-shubhrajyoti.datta@amd.com?part=8
^ permalink raw reply
* Re: [PATCH RFC 7/9] arm64: dts: qcom: shikra-cqm-evk: Enable ethernet0
From: Mohd Ayaan Anwar @ 2026-06-15 3:55 UTC (permalink / raw)
To: Andrew Lunn
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Richard Cochran, Bjorn Andersson, Konrad Dybcio, Maxime Coquelin,
Alexandre Torgue, Russell King, linux-arm-msm, netdev, devicetree,
linux-kernel, linux-stm32, linux-arm-kernel
In-Reply-To: <6fde35ce-52dd-4679-9952-728b6553b843@lunn.ch>
On Thu, Jun 11, 2026 at 10:58:39PM +0200, Andrew Lunn wrote:
> > + ethphy0: ethernet-phy@7 {
> > + compatible = "ethernet-phy-ieee802.3-c22";
> > + reg = <7>;
> > + reset-gpios = <&tlmm 135 GPIO_ACTIVE_LOW>;
> > + reset-assert-us = <10000>;
> > + reset-deassert-us = <50000>;
> > + ti,tx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> > + ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
>
> Are these two needed? It should default to 2ns, since that is what the
> RGMII standard says the delay should be.
>
That is true, I will remove these in v2.
Ayaan
^ permalink raw reply
* Re: [PATCH 3/8] dt-bindings: clock: clocking-wizard: Make s_axi_aclk optional for static-config
From: sashiko-bot @ 2026-06-15 3:58 UTC (permalink / raw)
To: Shubhrajyoti Datta; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260615034845.3320286-4-shubhrajyoti.datta@amd.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Restricting `clocks` and `clock-names` to `maxItems: 1` in static-config mode breaks backward compatibility with existing device trees.
--
commit e42f118e6d12d9aebf43f9b7f7b451d1e25632ab
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
dt-bindings: clock: clocking-wizard: Make s_axi_aclk optional for static-config
This commit updates the device tree binding for the Xilinx clocking
wizard to allow a single clock input when configured in static-config
mode. It modifies the top-level clock properties to have minItems: 1
and enforces maxItems: 1 in the static-config conditional block.
> diff --git a/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
> index aa397550d107c..0daefe89ea89f 100644
> --- a/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
> +++ b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
[ ... ]
> @@ -87,9 +89,19 @@ allOf:
> then:
> required:
> - xlnx,clk-mul-div
> + properties:
> + clocks:
> + maxItems: 1
> + clock-names:
> + maxItems: 1
[Severity: High]
The commit message states the intention is to make the s_axi_aclk optional
for static-config. However, by setting maxItems to 1 here, does this strictly
forbid the second clock instead?
Because the previous schema required s_axi_aclk, existing device trees that
specify both xlnx,static-config and s_axi_aclk will now fail dtbs_check
validation.
If the goal is to make the clock optional while maintaining backward
compatibility with older device trees, would it be better to leave maxItems
unrestricted in this block? This would allow older device trees with both
clocks to continue passing validation while new ones can safely omit it.
> else:
> required:
> - reg
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615034845.3320286-1-shubhrajyoti.datta@amd.com?part=3
^ permalink raw reply
* Re: [PATCH 4/8] clk: clocking-wizard: Do not map the memory for static-config
From: sashiko-bot @ 2026-06-15 3:58 UTC (permalink / raw)
To: Shubhrajyoti Datta; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260615034845.3320286-5-shubhrajyoti.datta@amd.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Registering the same struct notifier_block to two different clock notifiers corrupts the intrusive linked lists.
--
commit 8b915ba31e389cdcf5a743f96eb5c3695bc448c3
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
clk: clocking-wizard: Do not map the memory for static-config
This patch prevents the memory mapping of the AXI register block if the
device is configured with xlnx,static-config. It achieves this by moving
devm_platform_ioremap_resource() under the non-static-config conditional
branch, since only the dynamic path requires the AXI register block.
> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 4a0136349f71a..e082051221bef 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
[ ... ]
> @@ -1183,6 +1179,10 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> }
>
> if (!of_property_present(np, "xlnx,static-config")) {
> + clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(clk_wzrd->base))
> + return PTR_ERR(clk_wzrd->base);
> +
> ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
> if (!ret) {
> if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
[ ... ]
> if (clk_wzrd->speed_grade) {
> clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
>
> ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1,
> &clk_wzrd->nb);
> if (ret)
> dev_warn(&pdev->dev,
> "unable to register clock notifier\n");
>
> ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
> &clk_wzrd->nb);
[Severity: High]
This isn't a bug introduced by this patch, but it is a pre-existing issue.
Does registering the same struct notifier_block (&clk_wzrd->nb) to two
different clocks in clk_wzrd_probe() corrupt the SRCU notifier chains?
The clock notifier framework uses srcu_notifier_chain_register(), which
links the provided struct notifier_block into an intrusive linked list by
modifying its next pointer. Because clk_in1 and axi_clk are distinct clocks
with separate notifier chains, adding the exact same notifier_block instance
to the second chain overwrites its next pointer.
Could this merge the two chains, causing events from the first clock to
traverse into the second clock's chain? Furthermore, when the notifiers are
unregistered, would the unregistration logic leave the first clock's chain
pointing to elements from the second clock's chain, leading to a potential
use-after-free?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615034845.3320286-1-shubhrajyoti.datta@amd.com?part=4
^ permalink raw reply
* Re: [PATCH 7/8] clk: clocking-wizard: Skip s_axi_aclk for static-config
From: sashiko-bot @ 2026-06-15 3:58 UTC (permalink / raw)
To: Shubhrajyoti Datta; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260615034845.3320286-8-shubhrajyoti.datta@amd.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The driver registers the same `struct notifier_block` instance (`clk_wzrd->nb`) to two different clock notification chains (`clk_in1` and `axi_clk`), causing notifier list corruption.
--
commit 2cde73f9cf8529ad3e29f28db978947c4f80fd0e
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
clk: clocking-wizard: Skip s_axi_aclk for static-config
For static-config mode the AXI bus interface is not used, so there is
no need to get and enable s_axi_aclk. Move the axi_clk setup inside
the non-static-config branch.
> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 5470a717fccc8..fe73ee02b54ee 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
[ ... ]
> @@ -1258,22 +1257,24 @@ static int clk_wzrd_probe(struct platform_device *pdev)
> return -ENOMEM;
> platform_set_drvdata(pdev, clk_wzrd);
>
> - clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
> - if (IS_ERR(clk_wzrd->axi_clk))
> - return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
> - "s_axi_aclk not found\n");
> - rate = clk_get_rate(clk_wzrd->axi_clk);
> - if (rate > WZRD_ACLK_MAX_FREQ) {
> - dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", rate);
> - return -EINVAL;
> - }
> -
> clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
> if (IS_ERR(clk_wzrd->clk_in1))
> return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
> "failed to get clk_in1\n");
>
> if (!of_property_present(np, "xlnx,static-config")) {
> + unsigned long rate;
> +
> + clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
> + if (IS_ERR(clk_wzrd->axi_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
> + "s_axi_aclk not found\n");
> + rate = clk_get_rate(clk_wzrd->axi_clk);
> + if (rate > WZRD_ACLK_MAX_FREQ) {
> + dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", rate);
> + return -EINVAL;
> + }
> +
[Severity: High]
This is a pre-existing issue, but does using the same struct notifier_block
instance for multiple clock notifiers corrupt the notification chain?
Looking further down in clk_wzrd_probe(), the same &clk_wzrd->nb instance
is registered to two different clocks:
ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1,
&clk_wzrd->nb);
...
ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
&clk_wzrd->nb);
Since a struct notifier_block has a single next pointer, registering the
same instance to a second chain overwrites its next pointer. Doesn't this
merge the two clock notification chains and corrupt the first notifier list?
> clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(clk_wzrd->base))
> return PTR_ERR(clk_wzrd->base);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615034845.3320286-1-shubhrajyoti.datta@amd.com?part=7
^ permalink raw reply
* Re: [PATCH 6/8] clk: clocking-wizard: Add static-config clock provider support
From: sashiko-bot @ 2026-06-15 4:02 UTC (permalink / raw)
To: Shubhrajyoti Datta; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260615034845.3320286-7-shubhrajyoti.datta@amd.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Division by zero in `clk_wzrd_determine_rate` when the requested rate is significantly larger than the parent rate.
- [High] Logic bug with ignored `min_t` return value leads to unbounded register writes.
- [High] Division by zero in `clk_wzrd_get_divisors` and `clk_wzrd_get_divisors_ver` when the target rate is 0.
--
commit b31cd102641055f0a34c6e79498f21e726bb2a91
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
clk: clocking-wizard: Add static-config clock provider support
When xlnx,static-config is present the divider/multiplier path is
synthesized inside the Wizard without exposing runtime MMIO
reconfiguration, so omit the AXI register mapping and advertise each
routed output clock as a clk_fixed_factor child of clk_in1 using the
synthesized ratios exported through xlnx,clk-mul-div.
> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index ffc78c90bee6f..5470a717fccc8 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
[ ... ]
This isn't a bug introduced by this patch, but I noticed a potential division
by zero in the unpatched code:
drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_determine_rate() {
...
div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
req->rate = req->best_parent_rate / div;
...
}
[Severity: High]
If the requested rate is significantly larger than the best parent rate,
could div evaluate to 0 and trigger a division by zero exception here?
This is a pre-existing issue, but the capping logic appears to be unused
in the following code:
drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_dynamic_reconfig() {
...
/* Cap the value to max */
min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
/* Set divisor and clear phase offset */
writel(value, div_addr);
...
}
[Severity: High]
Does this code leave value unbounded before the register write since the
return value of min_t() is not assigned to anything?
This isn't a bug introduced by this patch, but there might be another division
by zero risk in the divisor calculations:
drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_get_divisors_ver() {
...
o = DIV_ROUND_CLOSEST_ULL(vco_freq, rate);
...
}
And similarly in clk_wzrd_get_divisors():
drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_get_divisors() {
...
o = DIV_ROUND_CLOSEST_ULL(vco_freq, rate);
...
}
[Severity: High]
If rate is exactly 0, can this cause a kernel panic due to a division
by zero?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260615034845.3320286-1-shubhrajyoti.datta@amd.com?part=6
^ permalink raw reply
* Re: [PATCH v3 2/4] spi: qcom-geni: Use geni_se_resources_init() for resource initialization
From: Praveen Talari @ 2026-06-15 4:08 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Konrad Dybcio,
bjorn.andersson, linux-arm-msm, linux-spi, devicetree,
linux-kernel, mukesh.savaliya, aniket.randive, chandana.chiluveru,
jyothi.seerapu, chiluka.harish
In-Reply-To: <aixBtqPq3MNgYOkk@sirena.co.uk>
HI Mark,
On 12-06-2026 22:58, Mark Brown wrote:
> On Thu, Jun 04, 2026 at 12:20:36PM +0530, Praveen Talari wrote:
>
>> - ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
>> + ret = geni_se_resources_init(&mas->se);
>> if (ret)
>> return ret;
> Doesn't this still make qup-memory mandatory, breaking existing DTs that
> might not supply it?
No. "qup-memory" is optional only.
Ref
https://lore.kernel.org/all/20260227061544.1785978-2-praveen.talari@oss.qualcomm.com/
Thanks,
Praveen Talari
^ permalink raw reply
* Re: [PATCH net-next v5 14/15] dt-bindings: net: add onsemi's S2500
From: Rob Herring @ 2026-06-15 4:10 UTC (permalink / raw)
To: Selvamani Rajagopal
Cc: Andrew Lunn, Piergiorgio Beruto, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, Parthiban Veerasooran, Richard Cochran,
Krzysztof Kozlowski, Conor Dooley, Simon Horman, Jonathan Corbet,
Shuah Khan, netdev, linux-kernel, devicetree, linux-doc,
Jerry Ray
In-Reply-To: <20260614-s2500-mac-phy-support-v5-14-89874b72f725@onsemi.com>
On Sun, Jun 14, 2026 at 10:00:30AM -0700, Selvamani Rajagopal wrote:
> Add YAML device tree binding for the onsemi S2500 IEEE 802.3cg
> compliant Ethernet transceiver device.
>
> We use IRQF_TRIGGER_FALLING, though OPEN Alliance 10BASE-T1x
> Serial Interface specification calls for IRQF_TRIGGER_LOW.
>
> This is to match IRQF_TRIGGER_FALLING used by OA TC6 framework code.
> This bug fix requires changes to the stable branch. At that time,
> this will be changed to IRQF_TRIGGER_LOW.
>
> ---
Everything after this is dropped from the commit message when applying.
Your Sob needs to be above it.
And you are missing tags from prior versions. It is your responsibility
to add them.
> changes in v5
> - no changes
> changes in v4:
> - added spi-max-frequency as suggested by AI review
> - changed interrupt to IRQ_TYPE_EDGE_FALLING as it is
> being taken care in net (stable) branch
> changes in v3
> - Removed URL link that failed verification
> changes in v2
> - removed spi-max-frequency entry
> - changed the compatible string to s2500
> changes in v1
> - Added the first version of YAML file for onsemi MAC-PHY
>
> Signed-off-by: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
> ---
> .../devicetree/bindings/net/onnn,s2500.yaml | 67 ++++++++++++++++++++++
> 1 file changed, 67 insertions(+)
^ permalink raw reply
* Re: [PATCH 2/2] spi: qcom-geni: Add property to force GSI mode
From: Pengyu Luo @ 2026-06-15 4:25 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Andy Gross, Bjorn Andersson, linux-arm-msm, linux-spi, devicetree,
linux-kernel
In-Reply-To: <qwbda435on6rhsbf5o4jqijakanjmnmswnc6g6qsubuqbyvbok@fuoclv6u7tq5>
On Mon, Jun 15, 2026 at 5:18 AM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Sun, Jun 14, 2026 at 04:34:24PM +0800, Pengyu Luo wrote:
> > Some devices (such as gaokun3) do not disable FIFO mode, causing the
> > driver to fallback to FIFO mode by default. However, these platforms
> > also support GSI mode, which is highly preferred for certain
> > peripherals like SPI touchscreens to improve performance.
> >
> > Introduce the "qcom,force-gsi-mode" device property to hint and force
> > the controller into GSI mode during initialization.
>
> Ideally, this should be decided by the SPI controller based on the
> requirements. Another option would be to prefer GSI for all transfers if
> it is available, ignoring the FIFO even if it is not disabled.
>
I have read reviews. Can we check if the compatible
"qcom,sm6350-gpi-dma" is in DT? If the gpi dma controller is
presented, then we enable GSI mode.
Best wishes,
Pengyu
> >
> > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > ---
> > drivers/spi/spi-geni-qcom.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
>
> --
> With best wishes
> Dmitry
^ permalink raw reply
* Re: [PATCH RFC 6/9] arm64: dts: qcom: shikra: Add ethernet nodes
From: Mohd Ayaan Anwar @ 2026-06-15 4:26 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Richard Cochran, Bjorn Andersson, Konrad Dybcio, Maxime Coquelin,
Alexandre Torgue, Russell King
Cc: linux-arm-msm, netdev, devicetree, linux-kernel, linux-stm32,
linux-arm-kernel
In-Reply-To: <20260612-shikra_ethernet-v1-6-f0f4a1d19929@oss.qualcomm.com>
On Fri, Jun 12, 2026 at 12:07:02AM +0530, Mohd Ayaan Anwar wrote:
> + clocks = <&gcc GCC_EMAC0_AXI_CLK>,
> + <&gcc GCC_EMAC0_AHB_CLK>,
> + <&gcc GCC_EMAC0_PTP_CLK>,
> + <&gcc GCC_EMAC0_RGMII_CLK>,
> + <&gcc GCC_EMAC0_AXI_CLK>,
> + <&gcc GCC_EMAC0_AXI_SYS_NOC_CLK>,
> + <&gcc GCC_PCIE_TILE_AXI_SYS_NOC_CLK>;
> + clock-names = "stmmaceth", "pclk", "ptp_ref", "rgmii",
> + "axi", "axi-noc", "pcie-tile-axi-noc";
> +
I now realize that having GCC_EMAC0_AXI_CLK for both "stmmaceth" and
"axi" clocks is probably wrong. I will remove "axi" and have the glue
driver enable and set rates for "stmmaceth", "axi-noc" and
"pcie-tile-axi-noc" to ungate DDR access.
Ayaan
^ permalink raw reply
* Re: [PATCH v4 4/4] iio: flow: add Sensirion SLF3S liquid flow sensor driver
From: Krzysztof Kozlowski @ 2026-06-15 4:27 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Wadim Mueller, Krzysztof Kozlowski, Rob Herring, Conor Dooley,
David Lechner, Nuno Sá, Andy Shevchenko, Maxwell Doose,
linux-iio, devicetree, linux-kernel, Marcelo Schmitt,
Rodrigo Alencar
In-Reply-To: <20260614161011.7f7946f1@jic23-huawei>
On 14/06/2026 17:10, Jonathan Cameron wrote:
> On Sat, 13 Jun 2026 09:51:13 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
>> On 12/06/2026 19:47, Jonathan Cameron wrote:
>>> On Thu, 11 Jun 2026 16:01:12 +0200
>>> Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>
>>>> On 11/06/2026 15:27, Wadim Mueller wrote:
>>>>> +
>>>>> +static const struct of_device_id slf3s_of_match[] = {
>>>>> + { .compatible = "sensirion,slf3s-0600f", .data = &slf3s_variants[0] },
>>>>> + { .compatible = "sensirion,slf3s-1300f", .data = &slf3s_variants[1] },
>>>>> + { .compatible = "sensirion,slf3s-4000b", .data = &slf3s_variants[2] },
>>>>
>>>> You should have only 1300f here and detect the variants. That was my
>>>> point when I suggested to use the fallback.
>>>>
>>>
>>> I'm lost. How does that work? They cannot fallback to that part because
>>> it relies on in driver detection of the fact that they are incompatible.
>>>
>>
>> I am lost too. Then why were they made compatible in the binding?
>>
>> Entire discussion was that these are FULLY compatible due to variant
>> detection. That was the entire point of discussing more generic
>> fallback. Using specific fallback does not change that - it is the same
>> concept.
>>
>> If devices are not detectable, why were we discussing any compatibility?
>
> They are detectable, but the feature set is not, so to me there is zero valid
> in a generic fallback, we have to update the driver every time a new part comes
> along. (i.e. I agree with Conor's reply to the previous version thread).
> A specific fallback to a completely compatible part would be fine as there
> would be sufficient info to not need the ID lookup.
This patch has specific fallback and we discuss this now.
>
> The case in the binding for this version is the worst of all options because
> it implies it is valid to fallback to something that gives a false impression
> of being specific when it's relying on ID matching to say actually it's something
> else.
>
> So definitely not
> + compatible:
> + oneOf:
> + - const: sensirion,slf3s-1300f
> + - items:
> + - enum:
> + - sensirion,slf3s-0600f
> + - sensirion,slf3s-4000b
> + - const: sensirion,slf3s-1300f
> +
>
> Falling back to slf3s is better than this, but I'd rather not have a fallback
> at all, thus allowing correct fallback to the parts listed here in future.
Why? These devices are fully detectable thus fully compatible.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 7/8] dt-bindings: display: allwinner: Split H616 DE33 layer reg space
From: Krzysztof Kozlowski @ 2026-06-15 4:28 UTC (permalink / raw)
To: Jernej Škrabec, wens
Cc: samuel, mripard, maarten.lankhorst, tzimmermann, airlied, simona,
robh, krzk+dt, conor+dt, mturquette, sboyd, dri-devel, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel, linux-clk
In-Reply-To: <nIKN_benRn2Bk8SDZrkMCA@gmail.com>
On 14/06/2026 16:08, Jernej Škrabec wrote:
> Dne ponedeljek, 25. maj 2026 ob 14:10:38 Srednjeevropski poletni čas je Krzysztof Kozlowski napisal(a):
>> On 24/05/2026 23:33, Chen-Yu Tsai wrote:
>>> Hi,
>>>
>>> (resent from new email)
>>>
>>> On Thu, May 14, 2026 at 2:04 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>
>>>> On Sat, May 09, 2026 at 09:00:14PM +0200, Jernej Skrabec wrote:
>>>>> From: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>>>
>>>>> As it turns out, current H616 DE33 binding was written based on
>>>>> incomplete understanding of DE33 design. Namely, planes are shared
>>>>> resource and not tied to specific mixer, which was the case for previous
>>>>> generations of Display Engine (DE3 and earlier).
>>>>>
>>>>> This means that current DE33 binding doesn't properly reflect HW and
>>>>> using it would mean that second mixer (used for second display output)
>>>>> can't be supported.
>>>>>
>>>>> Remove layer register space, which will be represented with additional
>>>>> node, and replace it with phandle, which will point to that new, shared
>>>>> node. That way, all mixers can share same layers.
>>>>>
>>>>> There is no user of this binding yet, so changes can be made safely,
>>>>> without breaking any backward compatibility.
>>>>
>>>> There is user. git grep gives me:
>>>> drivers/gpu/drm/sun4i/sun8i_mixer.c
>>>>
>>>> which means this is a released ABI. As I understood, the old code was
>>>
>>> We held off on merging the DT changes so that we could rework this.
>>> I can't find the actual request though. It was probably over IRC.
>>>
>>>> working fine but just did not support all use cases. Why this cannot be
>>>> kept backwards compatible?
>>>
>>> AFAIK the "planes" block is shared between two display mixers. As the
>>> commit message explains, this prevents using the second mixer, since
>>> only one of them can claim and map the register space. And on the H700
>>> (which is the same die as the H616 discussed here but with more exposed
>>> interfaces), there could actually be a use case for the second mixer.
>>
>> It explains why you want to make the changes but not why you cannot keep
>> it backwards compatible.
>
> I guess it can be backward compatible, but I don't think it makes sense.
> Yes, original driver implemented original DT bindings, but there is no node
> which uses that binding. If there is no user of that, why would driver
Did you check all out of tree users of the ABI? All vendor kernels,
forks and all of them for which the ABI was made for?
If there is no single downstream/out of tree kernel using this ABI, then
of course you do not need to consider it. I don't know how would you
prove that but I am open for suggestions.
> need to support it nevertheless? Supporting only actually used DT binding
> allows for better code architecture, as there is no need to support second,
> unused path. It also simplifies testing, since developer doesn't need to
> test both paths if code is changed in that area.
>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 2/5] iio: adc: Add ti-ads1262 driver
From: Krzysztof Kozlowski @ 2026-06-15 4:30 UTC (permalink / raw)
To: Kurt Borja
Cc: Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, David Lechner, Nuno Sá,
Andy Shevchenko, linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <DJ92M0ZMSI2C.2I39LHFRNQS7W@gmail.com>
On 14/06/2026 22:56, Kurt Borja wrote:
> On Sat Jun 13, 2026 at 1:59 PM -05, Krzysztof Kozlowski wrote:
>
> [...]
>
>> Functions used by probe() should be before probe(), not somewhere in the
>> middle of the code. IOW, entire probe is together.
>
> I they all are, it's just that regmap stuff takes a huge chunk. I'll
> check how to reorganize.
>
> [...]
>
>>> +static const struct of_device_id ads1262_of_match[] = {
>>> + { .compatible = "ti,ads1262" },
>>> + { .compatible = "ti,ads1263" },
>>
>> So devices are fully compatible? Then it should be expressed in the
>> binding and drop one entry here.
>
> Not fully compatible as Jonathan said. One is a subset of the other.
This is THE meaning of compatible!
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 2/5] iio: adc: Add ti-ads1262 driver
From: Krzysztof Kozlowski @ 2026-06-15 4:33 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Kurt Borja, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, David Lechner, Nuno Sá,
Andy Shevchenko, linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <20260614143918.35503c5a@jic23-huawei>
On 14/06/2026 15:39, Jonathan Cameron wrote:
>
>>> +
>>> +DEFINE_RUNTIME_DEV_PM_OPS(ads1262_runtime_pm, ads1262_runtime_suspend,
>>> + ads1262_runtime_resume, NULL);
>>> +
>>> +static const struct of_device_id ads1262_of_match[] = {
>>> + { .compatible = "ti,ads1262" },
>>> + { .compatible = "ti,ads1263" },
>>
>> So devices are fully compatible? Then it should be expressed in the
>> binding and drop one entry here.
>
> They aren't. It's relying on one of them having a subnode that spins up an
I don't see anything in the this patch that would be using the other
compatible, so driver looks like handling it fully compatible.
> auxdev for the hardware block they don't share. A fallback would be fine
Patch #5 adding auxdev still does it uncoditionally, thus driver
clearly treats them as 100% compatible.
Or I missed piece of code - please point me where is any incompatible
behavior coded.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: iio: adc: Add TI ADS126x ADC family
From: Krzysztof Kozlowski @ 2026-06-15 4:34 UTC (permalink / raw)
To: Kurt Borja
Cc: Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, David Lechner, Nuno Sá,
Andy Shevchenko, linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <DJ92JT0CPSXJ.1113K3KLSRHH4@gmail.com>
On 14/06/2026 22:53, Kurt Borja wrote:
> Hi Krzysztof,
>
> On Sat Jun 13, 2026 at 1:54 PM -05, Krzysztof Kozlowski wrote:
>> On Fri, Jun 12, 2026 at 05:46:19PM -0500, Kurt Borja wrote:
>>> + ti,neg-refmux:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: |
>>> + Selects the negative voltage reference input:
>>> + 0: Internal 2.5 V reference
>>> + 1: AIN1 pin
>>> + 2: AIN3 pin
>>> + 3: AIN5 pin
>>> + 4: AVSS pin
>>> + minimum: 0
>>> + maximum: 4
>>> + default: 0
>>> +
>>> + ti,vbias:
>>> + $ref: /schemas/types.yaml#/definitions/flag
>>> + description: Enables the level-shift voltage on the AINCOM pin.
>>> + default: false
>>
>> There is no such syntax, drop.
>
> The "default: false" syntax? Sure I'll drop.
>
>>
>>> +
>>> + ti,idac1-pin:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: |
>>> + Selects the analog input pin to connect IDAC1:
>>> + 0: AIN0
>>> + 1: AIN1
>>> + 2: AIN2
>>> + 3: AIN3
>>> + 4: AIN4
>>> + 5: AIN5
>>> + 6: AIN6
>>> + 7: AIN7
>>> + 8: AIN8
>>> + 9: AIN9
>>> + 10: AINCOM
>>> + 11: No Connection
>>> + minimum: 0
>>> + maximum: 11
>>> + default: 11
>>> +
>>> + ti,idac1-microamp:
>>> + description: Selects the current values of IDAC1.
>>> + enum: [0, 50, 100, 250, 500, 750, 1000, 1500, 2000, 2500, 3000]
>>> + default: 0
>>> +
>>> + ti,idac2-pin:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: |
>>> + Selects the analog input pin to connect IDAC2:
>>> + 0: AIN0
>>> + 1: AIN1
>>> + 2: AIN2
>>> + 3: AIN3
>>> + 4: AIN4
>>> + 5: AIN5
>>> + 6: AIN6
>>> + 7: AIN7
>>> + 8: AIN8
>>> + 9: AIN9
>>> + 10: AINCOM
>>> + 11: No Connection
>>> + minimum: 0
>>> + maximum: 11
>>> + default: 11
>>> +
>>> + ti,idac2-microamp:
>>> + description: Selects the current values of IDAC2.
>>> + enum: [0, 50, 100, 250, 500, 750, 1000, 1500, 2000, 2500, 3000]
>>> + default: 0
>>> +
>>> + clocks:
>>> + maxItems: 1
>>> +
>>> + '#io-channel-cells':
>>> + const: 1
>>> +
>>> + '#gpio-cells':
>>> + const: 2
>>> +
>>> + gpio-controller: true
>>> +
>>> + adc:
>>> + $ref: /schemas/iio/adc/ti,ads1263-adc2.yaml#
>>
>> Not a separate device node. Fold into the parent... or explain in
>> commit msg. You have entire commit msg to explain odd things.
>>
>> In that binding description you call it "independent", so it should have
>> its own SPI chip select? Why "independent" and part of this binding?
>> Maybe not independent, so basically part of this device?
>
> It's independent in the sense that it is a proper subdevice on the same
You cannot use DT syntax as argument why you use DT syntax like that.
> chip. It shares the serial interface but operates completely in
> parallel.
How completely in parallel? If the interface is the same, then it does
not operate in parallel. It's impossible.
>
> I decided to add a subnode because other devices might request their
> io-channels and most importantly a different voltage reference might be
> connected to it.
>
> I'll clarify this in the commmit message on the next version. Although
> after seeing this submitted bindings [1], I wonder if it's a better
> approach to do something like
>
> spi@0 {
> mydevice@0 {
> ...
> adc@0 { ... };
> adc@1 { ... };
> };
> };
>
> Any thoughts?
Does not look like separate subnode. You still did not provide arguments
why this is independent.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: iio: adc: Add TI ADS126x ADC family
From: Kurt Borja @ 2026-06-15 4:40 UTC (permalink / raw)
To: Krzysztof Kozlowski, Kurt Borja
Cc: Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, David Lechner, Nuno Sá,
Andy Shevchenko, linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <73d85fad-d39a-4c34-90c2-819998656f7a@kernel.org>
On Sun Jun 14, 2026 at 11:34 PM -05, Krzysztof Kozlowski wrote:
> On 14/06/2026 22:53, Kurt Borja wrote:
>> Hi Krzysztof,
>>
>> On Sat Jun 13, 2026 at 1:54 PM -05, Krzysztof Kozlowski wrote:
>>> On Fri, Jun 12, 2026 at 05:46:19PM -0500, Kurt Borja wrote:
>>>> + ti,neg-refmux:
>>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>>> + description: |
>>>> + Selects the negative voltage reference input:
>>>> + 0: Internal 2.5 V reference
>>>> + 1: AIN1 pin
>>>> + 2: AIN3 pin
>>>> + 3: AIN5 pin
>>>> + 4: AVSS pin
>>>> + minimum: 0
>>>> + maximum: 4
>>>> + default: 0
>>>> +
>>>> + ti,vbias:
>>>> + $ref: /schemas/types.yaml#/definitions/flag
>>>> + description: Enables the level-shift voltage on the AINCOM pin.
>>>> + default: false
>>>
>>> There is no such syntax, drop.
>>
>> The "default: false" syntax? Sure I'll drop.
>>
>>>
>>>> +
>>>> + ti,idac1-pin:
>>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>>> + description: |
>>>> + Selects the analog input pin to connect IDAC1:
>>>> + 0: AIN0
>>>> + 1: AIN1
>>>> + 2: AIN2
>>>> + 3: AIN3
>>>> + 4: AIN4
>>>> + 5: AIN5
>>>> + 6: AIN6
>>>> + 7: AIN7
>>>> + 8: AIN8
>>>> + 9: AIN9
>>>> + 10: AINCOM
>>>> + 11: No Connection
>>>> + minimum: 0
>>>> + maximum: 11
>>>> + default: 11
>>>> +
>>>> + ti,idac1-microamp:
>>>> + description: Selects the current values of IDAC1.
>>>> + enum: [0, 50, 100, 250, 500, 750, 1000, 1500, 2000, 2500, 3000]
>>>> + default: 0
>>>> +
>>>> + ti,idac2-pin:
>>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>>> + description: |
>>>> + Selects the analog input pin to connect IDAC2:
>>>> + 0: AIN0
>>>> + 1: AIN1
>>>> + 2: AIN2
>>>> + 3: AIN3
>>>> + 4: AIN4
>>>> + 5: AIN5
>>>> + 6: AIN6
>>>> + 7: AIN7
>>>> + 8: AIN8
>>>> + 9: AIN9
>>>> + 10: AINCOM
>>>> + 11: No Connection
>>>> + minimum: 0
>>>> + maximum: 11
>>>> + default: 11
>>>> +
>>>> + ti,idac2-microamp:
>>>> + description: Selects the current values of IDAC2.
>>>> + enum: [0, 50, 100, 250, 500, 750, 1000, 1500, 2000, 2500, 3000]
>>>> + default: 0
>>>> +
>>>> + clocks:
>>>> + maxItems: 1
>>>> +
>>>> + '#io-channel-cells':
>>>> + const: 1
>>>> +
>>>> + '#gpio-cells':
>>>> + const: 2
>>>> +
>>>> + gpio-controller: true
>>>> +
>>>> + adc:
>>>> + $ref: /schemas/iio/adc/ti,ads1263-adc2.yaml#
>>>
>>> Not a separate device node. Fold into the parent... or explain in
>>> commit msg. You have entire commit msg to explain odd things.
>>>
>>> In that binding description you call it "independent", so it should have
>>> its own SPI chip select? Why "independent" and part of this binding?
>>> Maybe not independent, so basically part of this device?
>>
>> It's independent in the sense that it is a proper subdevice on the same
>
> You cannot use DT syntax as argument why you use DT syntax like that.
I'm not saying subdevice in the DT sense, I'm saying subdevice in the
actual secondary ADC inside the chip sense.
>
>
>> chip. It shares the serial interface but operates completely in
>> parallel.
>
> How completely in parallel? If the interface is the same, then it does
> not operate in parallel. It's impossible.
It does conversions in parallel, communication is of course still
serial.
>
>>
>> I decided to add a subnode because other devices might request their
>> io-channels and most importantly a different voltage reference might be
>> connected to it.
>>
>> I'll clarify this in the commmit message on the next version. Although
>> after seeing this submitted bindings [1], I wonder if it's a better
>> approach to do something like
>>
>> spi@0 {
>> mydevice@0 {
>> ...
>> adc@0 { ... };
>> adc@1 { ... };
>> };
>> };
>>
>> Any thoughts?
>
> Does not look like separate subnode. You still did not provide arguments
> why this is independent.
Well, there's not more arguments than this [1].
Anyway, I'll go for David's #io-channels-cells = <2> approach and drop
the subnode.
>
> Best regards,
> Krzysztof
[1] https://lore.kernel.org/linux-iio/DJ93WSYC3HTT.3NXQW390CLQ82@gmail.com/
--
Thanks,
~ Kurt
^ permalink raw reply
* Re: [PATCH 2/5] iio: adc: Add ti-ads1262 driver
From: Kurt Borja @ 2026-06-15 4:42 UTC (permalink / raw)
To: Krzysztof Kozlowski, Jonathan Cameron
Cc: Kurt Borja, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, David Lechner, Nuno Sá,
Andy Shevchenko, linux-iio, devicetree, linux-kernel, linux-gpio
In-Reply-To: <08a9a68a-f0f3-45eb-b018-43007a0acfdf@kernel.org>
On Sun Jun 14, 2026 at 11:33 PM -05, Krzysztof Kozlowski wrote:
> On 14/06/2026 15:39, Jonathan Cameron wrote:
>>
>>>> +
>>>> +DEFINE_RUNTIME_DEV_PM_OPS(ads1262_runtime_pm, ads1262_runtime_suspend,
>>>> + ads1262_runtime_resume, NULL);
>>>> +
>>>> +static const struct of_device_id ads1262_of_match[] = {
>>>> + { .compatible = "ti,ads1262" },
>>>> + { .compatible = "ti,ads1263" },
>>>
>>> So devices are fully compatible? Then it should be expressed in the
>>> binding and drop one entry here.
>>
>> They aren't. It's relying on one of them having a subnode that spins up an
>
> I don't see anything in the this patch that would be using the other
> compatible, so driver looks like handling it fully compatible.
>
>
>
>> auxdev for the hardware block they don't share. A fallback would be fine
>
> Patch #5 adding auxdev still does it uncoditionally, thus driver
> clearly treats them as 100% compatible.
>
> Or I missed piece of code - please point me where is any incompatible
> behavior coded.
You're right, Jonathan mentioned the same thing. This was the only check
when creating the auxiliary device.
node = device_get_named_child_node(dev, "adc");
if (!node)
return 0;
But I should definitely check if chip is actually ads1263. I just
assumed user would know better, but I'm dropping the subnode anyway so
I'll check the model.
>
>
>
> Best regards,
> Krzysztof
--
Thanks,
~ Kurt
^ permalink raw reply
* Re: [PATCH v7 1/3] dt-bindings: mmc: sdhci-msm: Add ICE phandle
From: Krzysztof Kozlowski @ 2026-06-15 4:45 UTC (permalink / raw)
To: Neeraj Soni, Kuldeep Singh, ulf.hansson, robh, krzk+dt, conor+dt,
andersson, konradybcio
Cc: linux-mmc, devicetree, linux-kernel, Abel Vesa, Abhinaba Rakshit
In-Reply-To: <91a90c37-f93a-17c8-b4fb-290759645977@oss.qualcomm.com>
On 13/06/2026 07:21, Neeraj Soni wrote:
>
>
> On 6/9/2026 5:19 PM, Krzysztof Kozlowski wrote:
>> On 09/06/2026 10:18, Neeraj Soni wrote:
>>>
>>>
>>> On 6/8/2026 11:40 AM, Kuldeep Singh wrote:
>>>> On 08-06-2026 09:46, Neeraj Soni wrote:
>>>>> Starting with sc7280(kodiak), the ICE will have its own device-tree node.
>>>>> So add the qcom,ice property to reference it.
>>>>>
>>>>> To avoid double-modeling, when qcom,ice is present, disallow an embedded
>>>>> ICE register region in the SDHCI node. Older SoCs without ICE remain
>>>>> valid as no additional requirement is imposed.
>>>>>
>>>>> Co-developed-by: Abel Vesa <abel.vesa@linaro.org>
>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>>>> Co-developed-by: Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>
>>>>> Signed-off-by: Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>
>>>>> Reviewed-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>>>>> Link: https://lore.kernel.org/r/20260310113557.348502-2-neeraj.soni@oss.qualcomm.com
>>>>> Signed-off-by: Neeraj Soni <neeraj.soni@oss.qualcomm.com>
>>>>
>>>> Link should go below "---" to specify previous discussions.
>>>>
>>>> Usually maintainers add link of patchset being merged here while merging
>>>> changes in their tree but authors don't add it here.
>>>>
>>> Do not see any such restrictions here https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>>
>> What do you provide that Link for? What does it bring?
>>
> Kuleep highlighted a concern with the usage of "Link:" tag. The link i
> provided guides on posting patches to upstream linux. Wanted to highlight
> what i folloed and I did not find any specific comment/line in the guide
> which prohibits author to use "Link:" tag in trailer section. If there
This is reversed logic. You need to provide reasons WHY it is worth, not
just claim it is not disallowed. Adding 100 Links to whatever website is
not disallowed, so you are going to add them?
> are guidelines otherwise please let me know and i will correct and post
> new patch.
The "Link:", not the URL itself. What does it bring? Linus made clear
statement that you should not add links UNLESS you have a reason.
So please share the reason. If you do not have, DO NOT ADD any "Link:".
Best regards,
Krzysztof
^ permalink raw reply
* Re: [RFC PATCH net-next 1/7] xfrm: allow packet offload drivers to own transmit
From: sashiko-bot @ 2026-06-15 4:58 UTC (permalink / raw)
To: Jihong Min; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260614040032.1567994-2-hurryman2212@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Bypassing `xfrm_dev_offload_ok()` allows packets to be transmitted without applying mandatory outer IPsec transformations for nested SAs.
- [High] Unchecked dereference of `x->xso.dev` races with asynchronous device unregistration, leading to a deterministic kernel panic.
--
commit c38a10fca161e28cf54507d8d670ee75e47687df
Author: Jihong Min <hurryman2212@gmail.com>
xfrm: allow packet offload drivers to own transmit
Packet offload drivers can currently program state and validate whether an
skb can be offloaded, but they cannot take ownership of a packet that needs
driver-specific transmit preparation before the regular XFRM output path
continues. This commit adds an optional xdo_dev_packet_xmit callback for
drivers to consume the skb and return the final transmit status.
[ ... ]
> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> index cc35c2fcbbe09..9f11559b02210 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -770,6 +770,17 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> }
>
> if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
> +#ifdef CONFIG_XFRM_OFFLOAD
> + const struct xfrmdev_ops *ops;
> +#endif
> +
> +#ifdef CONFIG_XFRM_OFFLOAD
> + ops = x->xso.dev->xfrmdev_ops;
[Severity: High]
Can this dereference of x->xso.dev crash if the device is concurrently
unregistered?
When a device is unregistered, xfrm_dev_state_flush() calls
xfrm_dev_state_free(), which clears x->xso.dev to null before updating
x->xso.type.
If xfrm_output() sees x->xso.type == XFRM_DEV_OFFLOAD_PACKET but reads
x->xso.dev after it was cleared, won't this result in a null pointer
dereference for in-flight packets?
> + /* Callback validates, consumes skb and returns final TX status. */
> + if (ops && ops->xdo_dev_packet_xmit)
> + return ops->xdo_dev_packet_xmit(skb, x);
[Severity: Critical]
Does this early return bypass outer IPsec transformations for nested
associations?
By returning early here, the code skips the call to xfrm_dev_offload_ok(),
which performs validation including checking !xdst->child->xfrm. This check
normally ensures there are no nested IPsec transformations pending.
If a packet has nested IPsec associations, such as transport over tunnel,
would the hardware driver transmit the packet out of the physical port
applying only the innermost transformation, leaving the outer transformations
completely bypassed?
> +#endif
> +
> if (!xfrm_dev_offload_ok(skb, x)) {
> XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> kfree_skb(skb);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260614040032.1567994-1-hurryman2212@gmail.com?part=1
^ permalink raw reply
* Re: [RFC PATCH net-next 5/7] net: airoha: add QDMA support for SOE packets
From: sashiko-bot @ 2026-06-15 4:58 UTC (permalink / raw)
To: Jihong Min; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260614040032.1567994-6-hurryman2212@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Accessing `skb` after handing it off to the QDMA hardware TX ring causes a Use-After-Free vulnerability.
Pre-existing issues:
- [High] Using `dma_map_single()` with `skb_frag_address()` instead of `skb_frag_dma_map()` causes mapping failures or kernel panics on systems with high memory.
- [High] Unnecessarily re-evaluating `dev` and `netdev` from scattered RX descriptors causes invalid metadata extraction, resulting in misrouted or dropped packets.
--
commit a3fa3b0477739d5fa5e7176d3210e1a5b7b0f8df
Author: Jihong Min <hurryman2212@gmail.com>
net: airoha: add QDMA support for SOE packets
Add QDMA RX/TX plumbing for SOE packets, including the SOE RX ring,
coherent RX slots, SOE completion decoding, and the private QDMA submit
helper used by the SOE provider. Wire the Ethernet netdev feature and
lifetime hooks through the SOE stubs.
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -822,92 +822,210 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
[ ... ]
> len = FIELD_GET(QDMA_DESC_LEN_MASK, desc_ctrl);
> - data_len = q->skb ? AIROHA_RX_LEN(q->buf_size) : e->dma_len;
> + data_len = q->skb && !q->rx_coherent ?
> + AIROHA_RX_LEN(q->buf_size) :
> + e->dma_len;
> if (!len || data_len < len)
> goto free_frag;
>
> - dev = airoha_qdma_get_gdm_dev(eth, desc);
> - if (IS_ERR(dev))
> - goto free_frag;
> + msg0 = le32_to_cpu(READ_ONCE(desc->msg0));
> + msg1 = le32_to_cpu(READ_ONCE(desc->msg1));
> + msg2 = le32_to_cpu(READ_ONCE(desc->msg2));
> +#if IS_REACHABLE(CONFIG_NET_AIROHA_SOE)
> + soe_pkt = partial_soe || airoha_qdma_rx_is_soe(msg0);
> +#endif
> + if (!soe_pkt) {
> + dev = airoha_qdma_get_gdm_dev(eth, desc);
> + if (IS_ERR(dev))
> + goto free_frag;
> + netdev = netdev_from_priv(dev);
> + rx_dev = netdev;
> + }
[Severity: High]
This is a pre-existing issue, but does re-evaluating dev and netdev on every
descriptor in a scattered chain overwrite the valid metadata from the first
descriptor?
If the hardware only writes msg0 and msg1 to the first descriptor and
zeroes them out in subsequent ones, re-evaluating here might corrupt DSA
routing and flow hashing by using zeroed data.
[ ... ]
> @@ -2176,6 +2552,110 @@ int airoha_get_fe_port(struct airoha_gdm_dev *dev)
[ ... ]
> +int airoha_qdma_xmit_skb(struct airoha_gdm_dev *dev, struct sk_buff *skb,
> + u32 msg0, u32 msg1, u32 msg2)
> +{
[ ... ]
> + while (true) {
> + struct airoha_qdma_desc *desc = &q->desc[index];
> + skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> + dma_addr_t addr;
> + u32 val;
> +
> + addr = dma_map_single(netdev->dev.parent, data, len,
> + DMA_TO_DEVICE);
[Severity: High]
This is a pre-existing issue copied into this new function (and also present
in airoha_dev_xmit), but does using dma_map_single() with skb_frag_address()
risk mapping failures or kernel panics on systems with high memory?
On architectures with HIGHMEM, page_address() returns NULL for pages not
mapped into the kernel's virtual address space. Calling dma_map_single() on
a NULL address could map garbage or trigger a kernel oops. Should this use
skb_frag_dma_map() instead?
[ ... ]
> @@ -2293,13 +2779,24 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
> q->txq_stopped = true;
> }
>
> - if (netif_xmit_stopped(txq) || !netdev_xmit_more())
> + if (netif_xmit_stopped(txq) || !netdev_xmit_more() ||
> + soe_decrypt_skb)
> airoha_qdma_rmw(qdma, REG_TX_CPU_IDX(qid),
> TX_RING_CPU_IDX_MASK,
> FIELD_PREP(TX_RING_CPU_IDX_MASK, index));
>
> spin_unlock_bh(&q->lock);
>
> + /* SOE decrypt flow binding needs the final egress netdev and QDMA
> + * descriptor context. The SOE RX path marks only candidate packets; bind
> + * only after the current plaintext packet has been queued and kicked so
> + * the newly bound decrypt entry cannot race the CPU-transmitted packet.
> + */
> +#if IS_REACHABLE(CONFIG_NET_AIROHA_SOE)
> + if (soe_decrypt_skb)
> + airoha_ppe_soe_xmit_skb(&dev->eth->ppe->dev, skb, netdev);
> +#endif
> +
> return NETDEV_TX_OK;
[Severity: High]
Could this cause a use-after-free by accessing the skb after it has been
handed off to the hardware?
Once the transmission queue spinlock is released and the hardware doorbell
is kicked via airoha_qdma_rmw(), the skb ownership is fully transferred.
The transmission could complete concurrently on another CPU, which would
invoke dev_kfree_skb_any(skb) in the TX completion handler before
airoha_ppe_soe_xmit_skb() is able to safely access skb->mark.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260614040032.1567994-1-hurryman2212@gmail.com?part=5
^ 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