* Re: [PATCH v15 2/3] of: Factor arguments passed to of_map_id() into a struct
From: Vijayanand Jitta @ 2026-05-21 13:10 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Frank.Li, devicetree, imx, linux-pci, robh, conor+dt
In-Reply-To: <20260520085613.9052F1F000E9@smtp.kernel.org>
On 5/20/2026 2:26 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Unconditional dereference of `msi_node` in `of_check_msi_parent` when it can be NULL.
> --
>
> commit d17879206927f3ee695759bc3559fd04a6a551ab
> Author: Charan Teja Kalla <charan.kalla@oss.qualcomm.com>
> Subject: of: Factor arguments passed to of_map_id() into a struct
>
> This patch updates of_map_id() to take a struct of_phandle_args instead of
> passing the target device node and translated IDs separately. It explicitly
> separates the input filter node from the output arguments and transfers
> ownership of the matched node reference to the caller.
>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index e37c1b3f87362..e63a43be6c4a3 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -796,14 +796,15 @@ static int of_check_msi_parent(struct device_node *dev_node, struct device_node
>> /**
>> * of_msi_xlate - map a MSI ID and find relevant MSI controller node
>> * @dev: device for which the mapping is to be done.
>> - * @msi_np: Pointer to target MSI controller node
>> + * @msi_np: Pointer to target MSI controller node, or NULL if the caller
>> + * only needs the translated ID without receiving the controller node.
>> + * If non-NULL and pointing to a non-NULL node, only entries targeting
>> + * that node will be matched. If non-NULL and pointing to NULL, it will
>> + * receive the first matching target node with a reference held.
>> * @id_in: Device ID.
>> *
>> * Walk up the device hierarchy looking for devices with a "msi-map"
>> * or "msi-parent" property. If found, apply the mapping to @id_in.
>> - * If @msi_np points to a non-NULL device node pointer, only entries targeting
>> - * that node will be matched; if it points to a NULL value, it will receive the
>> - * device node of the first matching target phandle, with a reference held.
>> *
>> * Returns: The mapped MSI id.
>> */
>> @@ -817,8 +818,21 @@ u32 of_msi_xlate(struct device *dev, struct device_node **msi_np, u32 id_in)
>> * "msi-map" or an "msi-parent" property.
>> */
>> for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
>> - if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &id_out))
>> + struct of_phandle_args msi_spec = {};
>> +
>> + if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &msi_spec)) {
>> + if (msi_spec.np) {
>> + /* msi-map matched: use the translated ID and target node */
>> + if (msi_spec.args_count > 0)
>> + id_out = msi_spec.args[0];
>> + if (msi_np && !*msi_np)
>> + *msi_np = of_node_get(msi_spec.np);
>> + of_node_put(msi_spec.np);
>> + }
>> + /* msi-map present but no match → stop walking */
>> break;
>> + }
>> + /* -ENODEV: msi-map absent → check for msi-parent */
>> if (!of_check_msi_parent(parent_dev->of_node, msi_np))
>
> [Severity: High]
> This is a pre-existing issue, but does of_check_msi_parent() handle a NULL
> msi_np pointer correctly here?
>
> The documentation added above explicitly states that msi_np can be NULL if
> the caller only needs the translated ID without receiving the controller node.
> However, if msi-map is absent, execution falls through to
> of_check_msi_parent() with that potentially NULL pointer.
>
> Looking at drivers/of/irq.c:of_check_msi_parent(), if the msi-parent
> property is found, it evaluates:
>
> if ((*msi_node && *msi_node != msi_spec.np) || msi_spec.args_count != 0)
> ret = -EINVAL;
>
> if (!ret) {
> /* Return with a node reference held */
> *msi_node = msi_spec.np;
> return 0;
> }
>
> If msi_np is NULL, wouldn't dereferencing it via *msi_node trigger a NULL
> pointer dereference? Should this be fixed to safely handle a NULL pointer?
>
I see this is indeed a pre-existing latent bug .
fsl_mc_get_msi_id() in drivers/bus/fsl-mc/fsl-mc-msi.c already passes NULL for msi_np:
return (root_dprc_dev->parent->of_node ?
of_msi_xlate(dev, NULL, mc_dev->icid) :
iort_msi_map_id(dev, mc_dev->icid));
In practice it never triggered because FSL MC devices use msi-map, so of_map_msi_id()
succeeds and of_check_msi_parent() is never reached.
I'll will fix this in v16 by checking for msi_np aswell before calling
of_check_msi_parent() in of_msi_xlate().
if (msi_np && !of_check_msi_parent(parent_dev->of_node, msi_np))
break;
Thanks,
Vijay
>> break;
>> }
>
^ permalink raw reply
* Re: [PATCH v12 13/22] media: i2c: add Maxim GMSL2/3 serializer and deserializer framework
From: sashiko-bot @ 2026-05-21 13:12 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260521-gmsl2-3_serdes-v12-13-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] max_serdes_get_streams_masks inadvertently clears unaffected pad masks and the source pad mask due to incorrect array initialization.
- [High] max_serdes_get_streams_masks uses devm_kcalloc for dynamic stream mask allocation in a runtime operation, leading to a memory leak.
- [High] Tautological pointer check in max_serdes_xlate_enable_disable_streams leads to a potential NULL pointer dereference.
--
commit 3a9623717272197fc2bda2f941c128e58eebdcaa
Author: Cosmin Tanislav <demonsingur@gmail.com>
Subject: media: i2c: add Maxim GMSL2/3 serializer and deserializer framework
This commit adds a common framework for Maxim GMSL2/3 serializers and
deserializers, enabling support for Streams API, GMSL link negotiation,
and other shared functionality across the chips.
> diff --git a/drivers/media/i2c/maxim-serdes/max_serdes.c b/drivers/media/i2c/maxim-serdes/max_serdes.c
> new file mode 100644
> index 0000000000000..bed70b8ce99a4
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_serdes.c
[ ... ]
> +int max_serdes_xlate_enable_disable_streams(struct max_serdes_source *sources,
> + u32 source_sink_pad_offset,
> + const struct v4l2_subdev_state *state,
> + u32 pad, u64 updated_streams_mask,
> + u32 sink_pad_start, u32 num_sink_pads,
> + bool enable)
> +{
> + u32 failed_sink_pad;
> + int ret;
> + u32 i;
> +
> + for (i = sink_pad_start; i < sink_pad_start + num_sink_pads; i++) {
> + u64 matched_streams_mask = updated_streams_mask;
> + u64 updated_sink_streams_mask;
> + struct max_serdes_source *source;
> +
> + updated_sink_streams_mask =
> + v4l2_subdev_state_xlate_streams(state, pad, i,
> + &matched_streams_mask);
> + if (!updated_sink_streams_mask)
> + continue;
> +
> + source = &sources[i + source_sink_pad_offset];
> + if (!source)
> + continue;
> +
> + if (enable)
> + ret = v4l2_subdev_enable_streams(source->sd, source->pad,
> + updated_sink_streams_mask);
[Severity: High]
Will this check actually prevent a NULL pointer dereference?
Since source is assigned the address of an element in the sources
array, the pointer itself will never be NULL. If the corresponding
sink pad is not linked to a remote subdevice, the unpopulated element
might have source->sd set to NULL.
If userspace configures an active route to an unconnected sink pad
and starts the stream, will v4l2_subdev_enable_streams() dereference
the NULL source->sd pointer and panic the kernel?
[ ... ]
> +int max_serdes_get_streams_masks(struct device *dev,
> + const struct v4l2_subdev_state *state,
> + u32 pad, u64 updated_streams_mask,
> + u32 num_pads, u64 *old_streams_masks,
> + u64 **new_streams_masks, bool enable)
> +{
> + u64 *streams_masks;
> + unsigned int i;
> +
> + streams_masks = devm_kcalloc(dev, num_pads, sizeof(*streams_masks), GFP_KERNEL);
[Severity: High]
Could this lead to a memory leak during runtime operations?
This function appears to be called during stream enable and disable
operations, which userspace can trigger repeatedly via V4L2 ioctls.
Because devm_kcalloc allocations are bound to the device's lifetime,
will continuous stream toggling exhaust memory and lead to an OOM
condition since the allocations are only freed on driver detach?
> + if (!streams_masks)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_pads; i++) {
> + u64 matched_streams_mask = updated_streams_mask;
> + u64 updated_sink_streams_mask;
> +
> + updated_sink_streams_mask =
> + v4l2_subdev_state_xlate_streams(state, pad, i,
> + &matched_streams_mask);
> + if (!updated_sink_streams_mask)
> + continue;
> +
> + streams_masks[i] = old_streams_masks[i];
> + if (enable)
> + streams_masks[i] |= updated_sink_streams_mask;
> + else
> + streams_masks[i] &= ~updated_sink_streams_mask;
> + }
> +
> + if (enable)
> + streams_masks[pad] |= updated_streams_mask;
> + else
> + streams_masks[pad] &= ~updated_streams_mask;
[Severity: High]
Does this logic accidentally clear the state of unaffected pads and the
source pad?
The streams_masks array is zero-initialized by devm_kcalloc. Inside the
loop, if updated_sink_streams_mask is 0, the continue statement skips
the assignment of streams_masks[i] = old_streams_masks[i]. This leaves
unaffected pads with a cleared state of 0.
Additionally, when updating the target pad itself after the loop, the
operation directly applies updated_streams_mask without inheriting its
prior state from old_streams_masks[pad].
Will this cause the tracking to forget previously enabled streams and
break stream state management?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=13
^ permalink raw reply
* Re: [PATCH v2 1/1] riscv: dts: spacemit: enable USB3 on OrangePi R2S
From: Yixun Lan @ 2026-05-21 13:13 UTC (permalink / raw)
To: Chukun Pan
Cc: Rob Herring, Paul Walmsley, Alexandre Ghiti, Albert Ou,
Palmer Dabbelt, Conor Dooley, Krzysztof Kozlowski, linux-riscv,
linux-kernel, devicetree, spacemit
In-Reply-To: <20260518100030.2354606-1-amadeus@jmu.edu.cn>
Hi Chukun,
On 18:00 Mon 18 May , Chukun Pan wrote:
> Enable the DWC3 USB3.0 controller and its associated PHY on the
> OrangePi R2S. The USB regulator provides VBUS for USB2 and USB3
> ports, but the USB2 ports are handled by a separate controller.
>
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
Applied, thanks!
[1/1] riscv: dts: spacemit: enable USB3 on OrangePi R2S
https://github.com/spacemit-com/linux/commit/c5ad6737df88bd3c3e83145edb25737cb19f9a1f
--
Yixun Lan (dlan)
^ permalink raw reply
* Re: [PATCH v12 19/22] arm64: defconfig: disable deprecated MAX96712 driver
From: sashiko-bot @ 2026-05-21 13:14 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260521-gmsl2-3_serdes-v12-19-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removal of MAX96712 from defconfig without enabling its replacement (MAX96724) drops hardware support from the default arm64 build.
--
commit ddbcc79406c11acc7140054025e3af8a12bcaf19
Author: Cosmin Tanislav <demonsingur@gmail.com>
arm64: defconfig: disable deprecated MAX96712 driver
This commit removes the staging MAX96712 driver from the arm64 defconfig.
The functionality has been moved to the new MAX96724 driver.
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index d905a0777f939..5a4b75612c054 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1422,7 +1422,6 @@ CONFIG_GREYBUS=m
> CONFIG_GREYBUS_BEAGLEPLAY=m
> CONFIG_STAGING=y
> CONFIG_STAGING_MEDIA=y
> -CONFIG_VIDEO_MAX96712=m
[Severity: Medium]
Should the new CONFIG_VIDEO_MAX96724 be enabled here in place of the removed
driver?
The commit message notes that the functionality has moved to the MAX96724
driver, but without adding CONFIG_VIDEO_MAX96724=m to the defconfig, this
hardware support will be silently disabled out of the box for the default
arm64 build.
> CONFIG_VIDEO_MESON_VDEC=m
> CONFIG_SND_BCM2835=m
> CONFIG_CHROME_PLATFORMS=y
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=19
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: media: Add bindings for qcom,x1p42100-camss
From: Vikram Sharma @ 2026-05-21 13:16 UTC (permalink / raw)
To: Wenmeng Liu, Robert Foss, Todor Tomov, Bryan O'Donoghue,
Vladimir Zapolskiy, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
Cc: linux-media, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260511-purwa_camss-v2-1-22608ab9126c@oss.qualcomm.com>
On 5/11/2026 2:29 PM, Wenmeng Liu wrote:
> + camss: isp@acb7000 {
> + compatible = "qcom,x1p42100-camss";
> +
> + reg = <0 0x0acb7000 0 0x2000>,
> + <0 0x0acb9000 0 0x2000>,
> + <0 0x0acbb000 0 0x2000>,
> + <0 0x0acc6000 0 0x1000>,
> + <0 0x0acca000 0 0x1000>,
> + <0 0x0acb6000 0 0x1000>,
> + <0 0x0ace4000 0 0x1000>,
> + <0 0x0acec000 0 0x4000>,
Please cross check sizes for csiphy.
> + <0 0x0acf6000 0 0x1000>,
> + <0 0x0acf7000 0 0x1000>,
> + <0 0x0acf8000 0 0x1000>,
> + <0 0x0ac62000 0 0xf000>,
> + <0 0x0acc7000 0 0x2000>,
> + <0 0x0accb000 0 0x2000>;
Regards,
Vikram
^ permalink raw reply
* Re: [PATCH v15 3/3] of: Respect #{iommu,msi}-cells in maps
From: Vijayanand Jitta @ 2026-05-21 13:17 UTC (permalink / raw)
To: sashiko-reviews; +Cc: imx, conor+dt, devicetree, robh, linux-pci, Frank.Li
In-Reply-To: <20260520092923.0EDD11F000E9@smtp.kernel.org>
On 5/20/2026 2:59 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Strict requirement for `#iommu-cells` or `#msi-cells` breaks MSI allocations on standard-compliant controllers.
> - [High] Array desynchronization when parsing legacy 0-cell maps padded to 4 words per entry.
> --
>
> commit 23cece64ecb1dcea6290dbab34facbdac53ff293
> Author: Robin Murphy <robin.murphy@arm.com>
> Date: Wed May 20 08:02:42 2026 +0000
>
> of: Respect #{iommu,msi}-cells in maps
>
> This commit adds support for parsing target nodes' #cells values for
> iommu-map and msi-map properties, instead of assuming output specifiers
> always have exactly 1 cell. It updates of_map_id() to parse multi-cell and
> 0-cell output specifiers and reflect the actual number of output specifier
> cells.
>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index d658c26201353..f436e26763818 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>
> [ ... ]
>
>> @@ -2176,39 +2206,93 @@ int of_map_id(const struct device_node *np, u32 id,
>
> [ ... ]
>
>> + phandle = be32_to_cpup(map + offset + 1);
>> phandle_node = of_find_node_by_phandle(phandle);
>> if (!phandle_node)
>> return -ENODEV;
>>
>> + if (bad_map) {
>> + cells = 1;
>> + } else if (of_property_read_u32(phandle_node, cells_name, &cells)) {
>> + pr_err("%pOF: missing %s property\n", phandle_node, cells_name);
>> + of_node_put(phandle_node);
>> + return -EINVAL;
>> + }
>
> [Severity: High]
> Does this strict requirement for #iommu-cells or #msi-cells break MSI
> allocations on standard-compliant controllers?
>
> Several MSI controllers validly lack a #msi-cells property. For instance,
> arm,gic-v2m-frame nodes do not require it. In
> arch/arm64/boot/dts/qcom/ipq5018.dtsi, the pcie0 node defines:
>
> msi-map = <0x0 &v2m0 0x0 0xff8>;
>
> but v2m0 does not have #msi-cells.
>
> Prior to this patch, of_map_id() implicitly consumed 4 words per entry and
> succeeded. Will it now fail to find #msi-cells, print an error, and
> return -EINVAL, propagating the error through of_msi_xlate() and breaking
> MSI initialization on affected platforms?
>
I see both arch/arm64/boot/dts/qcom/ipq5018.dtsi and ipq5332.dtsi both have
msi-map pointing to arm,gic-v2m-frame nodes without #msi-cells.
The new strict pr_err + -EINVAL breaks MSI on these platforms. I'll fix this
in v16 by defaulting cells to '1' when #msi-cells is absent.
>> +
>> + if (cells > MAX_PHANDLE_ARGS) {
>
> [ ... ]
>
>> + if (map_len - offset < 3 + cells) {
>> + of_node_put(phandle_node);
>> + goto err_map_len;
>> + }
>> +
>> + out_base = map + offset + 2;
>> + offset += 3 + cells;
>
> [Severity: High]
> Could this cause array desynchronization when parsing legacy 0-cell maps
> that were padded to 4 words per entry?
>
> Prior to this patch, of_map_id() mandated that the map property length must
> be a multiple of 16 bytes:
>
> if (!map_len || map_len % (4 * sizeof(*map)))
> return -EINVAL;
>
> Consequently, if a legacy device tree targeted an MSI controller that
> explicitly defined #msi-cells = <0> (such as RISC-V IMSIC nodes), it was
> structurally forced to pad the 3-word map entry with a dummy 4th word to
> satisfy the kernel's length check.
>
> Since the new logic computes the entry stride dynamically as 3 + cells,
> when parsing a legacy padded map with cells=0, does the code advance by 3
> and incorrectly interpret the 4th dummy word of the first entry as the
> id_base of the second entry?
>
I see there is no real in-tree DT combines #msi-cells = <0> or #iommu-cells = <0>
with msi-map/iommu-map. All in-tree #msi-cells = <0> nodes (RISC-V IMSIC) use msi-parent.
If such DTs appear in the future, they would need to use the new 3-word entry format
(without the old padding), which is correct per the binding spec.
So, No changes are needed here.
Thanks,
Vijay
^ permalink raw reply
* [PATCH 0/5] Shikra: Add DT support for ice, rng and qce
From: Kuldeep Singh @ 2026-05-21 13:17 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Vinod Koul, Thara Gopinath,
Konrad Dybcio, Frank Li, Konrad Dybcio, Andy Gross
Cc: Harshal Dev, linux-arm-msm, linux-crypto, devicetree,
linux-kernel, dmaengine, Kuldeep Singh
This patchseries attempt to enable sdhc-ice, rng and qce on shikra
platform similar to other platforms.
Previously, the 3 dt-bindigs/DT changes were sent as individual series
and with feedback received, clubbed them together as all belong to same
crypto subsystem.
Here's link to old patchsets.
QCE: https://lore.kernel.org/lkml/20260515-shikra_qcrypto-v1-0-80f07b345c29@oss.qualcomm.com/
RNG: https://lore.kernel.org/lkml/20260514-shikra_rng-v1-0-4ea721a1429a@oss.qualcomm.com/
ICE: https://lore.kernel.org/lkml/20260515-shikra_ice_ufs-v2-0-2724a54339db@oss.qualcomm.com/
The patchseries require below as prerequisite:
https://lore.kernel.org/all/20260512-shikra-dt-v1-0-716438330dd0@oss.qualcomm.com/
Validations:
- Ice: driver probe on bootup.
- Qce: kcapi test alongwith driver probe
- Rng: validated with rngutils package.
- Validate shikra-cqs-evk.dtb with dt_bindings_check and CHECK_DTBS=y
with all yamls.
Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
Kuldeep Singh (5):
dt-bindings: crypto: qcom,inline-crypto-engine: Document Shikra ICE
dt-bindings: crypto: qcom,prng: Document Shikra TRNG
dt-bindings: crypto: qcom-qce: Document the Shikra crypto engine
dt-bindings: dma: qcom,bam-dma: Increase iommus maxItems to seven
arm64: dts: qcom: shikra: Add ICE, TRNG and QCE nodes
.../bindings/crypto/qcom,inline-crypto-engine.yaml | 1 +
.../devicetree/bindings/crypto/qcom,prng.yaml | 1 +
.../devicetree/bindings/crypto/qcom-qce.yaml | 1 +
.../devicetree/bindings/dma/qcom,bam-dma.yaml | 2 +-
arch/arm64/boot/dts/qcom/shikra.dtsi | 52 ++++++++++++++++++++++
5 files changed, 56 insertions(+), 1 deletion(-)
---
base-commit: bbf247cdb7ea162afb0c9548e2cd1354abb6462f
change-id: 20260521-shikra_crypto_changse-0c976c17763b
prerequisite-change-id: 20260511-shikra-dt-d75d97454646:v2
prerequisite-patch-id: e6edea1ef76fc1f3a9ad944684bee330c9ad7e59
prerequisite-patch-id: acae1549863061f0fdd7221f0a924b0892f15acf
prerequisite-patch-id: d145c1bd01622f50c69e5c1f13dee469ee4d1fe4
prerequisite-patch-id: 1b0da59c3859cba8c5f4f1e6bf08429988134c46
prerequisite-patch-id: fac2b512a679e1507debab6fc279b292fe4048cd
Best regards,
--
Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
^ permalink raw reply
* [PATCH 1/5] dt-bindings: crypto: qcom,inline-crypto-engine: Document Shikra ICE
From: Kuldeep Singh @ 2026-05-21 13:17 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Vinod Koul, Thara Gopinath,
Konrad Dybcio, Frank Li, Konrad Dybcio, Andy Gross
Cc: Harshal Dev, linux-arm-msm, linux-crypto, devicetree,
linux-kernel, dmaengine, Kuldeep Singh
In-Reply-To: <20260521-shikra_crypto_changse-v1-0-0154cc9cc0de@oss.qualcomm.com>
Document the Inline Crypto Engine (ICE) on the Qualcomm Shikra platform.
Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
index ccb6b8dd8e11..c0b083da78bf 100644
--- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
+++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
@@ -20,6 +20,7 @@ properties:
- qcom,sa8775p-inline-crypto-engine
- qcom,sc7180-inline-crypto-engine
- qcom,sc7280-inline-crypto-engine
+ - qcom,shikra-inline-crypto-engine
- qcom,sm8450-inline-crypto-engine
- qcom,sm8550-inline-crypto-engine
- qcom,sm8650-inline-crypto-engine
--
2.34.1
^ permalink raw reply related
* [PATCH 2/5] dt-bindings: crypto: qcom,prng: Document Shikra TRNG
From: Kuldeep Singh @ 2026-05-21 13:17 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Vinod Koul, Thara Gopinath,
Konrad Dybcio, Frank Li, Konrad Dybcio, Andy Gross
Cc: Harshal Dev, linux-arm-msm, linux-crypto, devicetree,
linux-kernel, dmaengine, Kuldeep Singh
In-Reply-To: <20260521-shikra_crypto_changse-v1-0-0154cc9cc0de@oss.qualcomm.com>
Document shikra compatible for the True Random Number Generator.
Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
Documentation/devicetree/bindings/crypto/qcom,prng.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/crypto/qcom,prng.yaml b/Documentation/devicetree/bindings/crypto/qcom,prng.yaml
index d054cc114707..3698525d3857 100644
--- a/Documentation/devicetree/bindings/crypto/qcom,prng.yaml
+++ b/Documentation/devicetree/bindings/crypto/qcom,prng.yaml
@@ -29,6 +29,7 @@ properties:
- qcom,sa8255p-trng
- qcom,sa8775p-trng
- qcom,sc7280-trng
+ - qcom,shikra-trng
- qcom,sm8450-trng
- qcom,sm8550-trng
- qcom,sm8650-trng
--
2.34.1
^ permalink raw reply related
* [PATCH 3/5] dt-bindings: crypto: qcom-qce: Document the Shikra crypto engine
From: Kuldeep Singh @ 2026-05-21 13:17 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Vinod Koul, Thara Gopinath,
Konrad Dybcio, Frank Li, Konrad Dybcio, Andy Gross
Cc: Harshal Dev, linux-arm-msm, linux-crypto, devicetree,
linux-kernel, dmaengine, Kuldeep Singh
In-Reply-To: <20260521-shikra_crypto_changse-v1-0-0154cc9cc0de@oss.qualcomm.com>
Document the crypto engine on the Qualcomm Shikra platform.
Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
Documentation/devicetree/bindings/crypto/qcom-qce.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/crypto/qcom-qce.yaml b/Documentation/devicetree/bindings/crypto/qcom-qce.yaml
index 08febd66c22b..5a653757ee75 100644
--- a/Documentation/devicetree/bindings/crypto/qcom-qce.yaml
+++ b/Documentation/devicetree/bindings/crypto/qcom-qce.yaml
@@ -54,6 +54,7 @@ properties:
- qcom,qcs8300-qce
- qcom,sa8775p-qce
- qcom,sc7280-qce
+ - qcom,shikra-qce
- qcom,sm6350-qce
- qcom,sm8250-qce
- qcom,sm8350-qce
--
2.34.1
^ permalink raw reply related
* [PATCH 4/5] dt-bindings: dma: qcom,bam-dma: Increase iommus maxItems to seven
From: Kuldeep Singh @ 2026-05-21 13:17 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Vinod Koul, Thara Gopinath,
Konrad Dybcio, Frank Li, Konrad Dybcio, Andy Gross
Cc: Harshal Dev, linux-arm-msm, linux-crypto, devicetree,
linux-kernel, dmaengine, Kuldeep Singh
In-Reply-To: <20260521-shikra_crypto_changse-v1-0-0154cc9cc0de@oss.qualcomm.com>
Shikra bam dma engine support 7 iommu entries and not 6.
Increase maxItems property for iommus to pass dtbs_check errors.
Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
index 0923fb189ada..e72adc172af1 100644
--- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
+++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
@@ -48,7 +48,7 @@ properties:
iommus:
minItems: 1
- maxItems: 6
+ maxItems: 7
num-channels:
$ref: /schemas/types.yaml#/definitions/uint32
--
2.34.1
^ permalink raw reply related
* [PATCH 5/5] arm64: dts: qcom: shikra: Add ICE, TRNG and QCE nodes
From: Kuldeep Singh @ 2026-05-21 13:17 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Vinod Koul, Thara Gopinath,
Konrad Dybcio, Frank Li, Konrad Dybcio, Andy Gross
Cc: Harshal Dev, linux-arm-msm, linux-crypto, devicetree,
linux-kernel, dmaengine, Kuldeep Singh
In-Reply-To: <20260521-shikra_crypto_changse-v1-0-0154cc9cc0de@oss.qualcomm.com>
Add device tree nodes describing the crypto hardware blocks present
on the Qualcomm Shikra platform:
- BAM DMA controller used by the Qualcomm crypto engine
- QCE (crypto) engine with DMA support
- TRNG hardware random number generator
- Inline crypto engine (ICE)
Also connect the SDHC controller to ICE via "qcom,ice" property to
support inline encryption.
Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
arch/arm64/boot/dts/qcom/shikra.dtsi | 52 ++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
index 31d0126e5b3e..b617735650ac 100644
--- a/arch/arm64/boot/dts/qcom/shikra.dtsi
+++ b/arch/arm64/boot/dts/qcom/shikra.dtsi
@@ -546,6 +546,41 @@ config_noc: interconnect@1900000 {
#interconnect-cells = <2>;
};
+ cryptobam: dma-controller@1b04000 {
+ compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0";
+ reg = <0x0 0x01b04000 0x0 0x24000>;
+ interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH 0>;
+ #dma-cells = <1>;
+ iommus = <&apps_smmu 0x84 0x0011>,
+ <&apps_smmu 0x86 0x0011>,
+ <&apps_smmu 0x92 0x0>,
+ <&apps_smmu 0x94 0x0011>,
+ <&apps_smmu 0x96 0x0011>,
+ <&apps_smmu 0x98 0x0001>,
+ <&apps_smmu 0x9f 0x0>;
+ qcom,ee = <0>;
+ qcom,controlled-remotely;
+ num-channels = <16>;
+ qcom,num-ees = <4>;
+ };
+
+ crypto: crypto@1b3a000 {
+ compatible = "qcom,shikra-qce", "qcom,sm8150-qce", "qcom,qce";
+ reg = <0x0 0x01b3a000 0x0 0x6000>;
+ dmas = <&cryptobam 4>, <&cryptobam 5>;
+ dma-names = "rx", "tx";
+ iommus = <&apps_smmu 0x84 0x0011>,
+ <&apps_smmu 0x86 0x0011>,
+ <&apps_smmu 0x92 0x0>,
+ <&apps_smmu 0x94 0x0011>,
+ <&apps_smmu 0x96 0x0011>,
+ <&apps_smmu 0x98 0x0001>,
+ <&apps_smmu 0x9f 0x0>;
+ interconnects = <&system_noc MASTER_CRYPTO_CORE0 0
+ &mc_virt SLAVE_EBI_CH0 0>;
+ interconnect-names = "memory";
+ };
+
qfprom: efuse@1b44000 {
compatible = "qcom,shikra-qfprom", "qcom,qfprom";
reg = <0x0 0x01b44000 0x0 0x3000>;
@@ -585,6 +620,11 @@ spmi_bus: spmi@1c40000 {
qcom,ee = <0>;
};
+ rng: rng@4454000 {
+ compatible = "qcom,shikra-trng", "qcom,trng";
+ reg = <0x0 0x04454000 0x0 0x1000>;
+ };
+
rpm_msg_ram: sram@45f0000 {
compatible = "qcom,rpm-msg-ram", "mmio-sram";
reg = <0x0 0x045f0000 0x0 0x7000>;
@@ -646,6 +686,7 @@ &mc_virt SLAVE_EBI_CH0 RPM_ALWAYS_TAG>,
mmc-hs400-enhanced-strobe;
resets = <&gcc GCC_SDCC1_BCR>;
+ qcom,ice = <&sdhc_ice>;
status = "disabled";
@@ -668,6 +709,17 @@ opp-384000000 {
};
};
+ sdhc_ice: crypto@4748000 {
+ compatible = "qcom,shikra-inline-crypto-engine",
+ "qcom,inline-crypto-engine";
+ reg = <0x0 0x04748000 0x0 0x18000>;
+ clocks = <&gcc GCC_SDCC1_ICE_CORE_CLK>,
+ <&gcc GCC_SDCC1_AHB_CLK>;
+ clock-names = "core",
+ "iface";
+ power-domains = <&rpmpd RPMHPD_CX>;
+ };
+
qupv3_0: geniqup@4ac0000 {
compatible = "qcom,geni-se-qup";
reg = <0x0 0x04ac0000 0x0 0x2000>;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v12 01/22] media: mc: Add INTERNAL pad flag
From: sashiko-bot @ 2026-05-21 13:19 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260521-gmsl2-3_serdes-v12-1-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] The fallback logic in `media_entity_get_fwnode_pad()` was not updated to ignore internal pads, mapping external endpoints to internal pads.
Pre-existing issues:
- [High] The error path in `media_create_pad_link()` causes `num_links` counters for both the source and sink pads to underflow.
--
commit fcec98f088400370f507804a947df270add985d6
Author: Sakari Ailus <sakari.ailus@linux.intel.com>
media: mc: Add INTERNAL pad flag
Internal sink pads will be used as routing endpoints in V4L2 [GS]_ROUTING
IOCTLs, to indicate that the stream begins in the entity. Internal sink
pads are pads that have both SINK and INTERNAL flags set.
Also prevent creating links to pads that have been flagged as internal and
initialising SOURCE pads with INTERNAL flag set.
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 3fa0bc687851b..7938f785e2e3c 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
[ ... ]
> @@ -1148,6 +1154,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
> return -EINVAL;
> if (WARN_ON(!(sink->pads[sink_pad].flags & MEDIA_PAD_FL_SINK)))
> return -EINVAL;
> + if (WARN_ON(source->pads[source_pad].flags & MEDIA_PAD_FL_INTERNAL) ||
> + WARN_ON(sink->pads[sink_pad].flags & MEDIA_PAD_FL_INTERNAL))
> + return -EINVAL;
>
> link = media_add_link(&source->links);
> if (link == NULL)
[Severity: High]
This is a pre-existing issue, but does the error path in media_create_pad_link()
cause the num_links counters for both the source and sink pads to underflow?
If the forward link is allocated but the backlink allocation fails, the error
path calls __media_entity_remove_link(source, link):
drivers/media/mc/mc-entity.c:media_create_pad_link() {
...
backlink = media_add_link(&sink->links);
if (backlink == NULL) {
__media_entity_remove_link(source, link);
return -ENOMEM;
}
...
}
Inside __media_entity_remove_link(), it unconditionally decrements
link->source->num_links and link->sink->num_links for data links:
drivers/media/mc/mc-entity.c:__media_entity_remove_link() {
...
if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) == MEDIA_LNK_FL_DATA_LINK) {
link->source->num_links--;
link->sink->num_links--;
...
}
Because these struct media_pad link counters are only incremented at the very
end of media_create_pad_link(), they haven't been incremented yet. Will these
decrements cause the unsigned 16-bit integers to underflow to 65535, corrupting
the internal state of the media graph?
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 1c80b1d6bbaf3..80cfd12a43fc1 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -208,6 +208,7 @@ struct media_entity_desc {
> #define MEDIA_PAD_FL_SINK (1U << 0)
> #define MEDIA_PAD_FL_SOURCE (1U << 1)
> #define MEDIA_PAD_FL_MUST_CONNECT (1U << 2)
> +#define MEDIA_PAD_FL_INTERNAL (1U << 3)
>
> struct media_pad_desc {
> __u32 entity; /* entity ID */
[Severity: Medium]
Was the fallback logic in media_entity_get_fwnode_pad() updated to ignore
internal pads?
Since internal sink pads also have the MEDIA_PAD_FL_SINK flag set, the fallback
logic (used when a driver lacks a get_fwnode_pad operation) searches for the
first pad matching the requested direction flags:
drivers/media/mc/mc-entity.c:media_entity_get_fwnode_pad() {
...
if (!entity->ops || !entity->ops->get_fwnode_pad) {
for (i = 0; i < entity->num_pads; i++) {
if (entity->pads[i].flags & direction_flags)
return i;
}
return -ENXIO;
}
...
}
If an internal pad appears before external pads in the array, won't it
incorrectly match and return the internal pad?
Any subsequent attempt to establish a link to this fwnode pad (e.g., via
v4l2_create_fwnode_links_to_pad()) would then trigger the new WARN_ON in
media_create_pad_link() and fail with -EINVAL, breaking initialization for
new drivers utilizing internal pads.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=1
^ permalink raw reply
* Re: [PATCH v12 07/22] dt-bindings: media: i2c: max96712: add support for I2C ATR
From: sashiko-bot @ 2026-05-21 13:19 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260521-gmsl2-3_serdes-v12-7-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The `i2c-atr` node allows child nodes (`^i2c@[0-3]$`) with a `reg` property, but fails to make `#address-cells` and `#size-cells` required in the parent node.
- [Low] The `reg` property under the `i2c-atr`'s `^i2c@[0-3]$` child nodes applies invalid numeric constraints and lacks a `maxItems: 1` constraint.
--
commit 8c55c178a378c597e61e804da4b186def25c5a4c
Author: Cosmin Tanislav <demonsingur@gmail.com>
dt-bindings: media: i2c: max96712: add support for I2C ATR
This commit adds I2C ATR support for the MAX96712 and MAX96724 deserializers.
It documents the ability to change I2C addresses for connected serializers to
avoid conflicts when they share the same CFG pin configuration.
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> index 583bbd60157c6..abacc3c874a9f 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> @@ -36,6 +36,30 @@ properties:
>
> enable-gpios: true
>
> + i2c-alias-pool:
> + maxItems: 4
> +
> + i2c-atr:
> + type: object
> + additionalProperties: false
> +
> + properties:
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
[Severity: Low]
Since the i2c-atr node allows child nodes (^i2c@[0-3]$) with a reg property,
shouldn't #address-cells and #size-cells be explicitly listed in a required
block under i2c-atr?
Omitting them allows an incomplete device tree missing these parsing
properties to silently pass schema validation.
> + patternProperties:
> + '^i2c@[0-3]$':
> + $ref: /schemas/i2c/i2c-controller.yaml#
> + unevaluatedProperties: false
> + properties:
> + reg:
> + items:
> + minimum: 0
> + maximum: 3
[Severity: Low]
Does this correctly constrain the reg property?
In dt-schema, the reg property is parsed as an array of arrays. Applying
items constraints to minimum and maximum attempts to enforce numeric limits on
the nested elements, which standard JSON Schema ignores for arrays, making it
a silent no-op.
Should this instead use maxItems: 1 to constrain the matrix length, while
relying on the unit-address regex pattern (^i2c@[0-3]$) to natively enforce
the bounds?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=7
^ permalink raw reply
* Re: [PATCH v12 02/22] dt-bindings: media: i2c: max96717: add support for I2C ATR
From: sashiko-bot @ 2026-05-21 13:20 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260521-gmsl2-3_serdes-v12-2-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `reg` property constraint inside the `^i2c@[01]$` pattern is missing `maxItems: 1` and incorrectly applies numeric constraints directly via `items`, making the validation ineffective.
- [Low] The `anyOf` logic used to enforce mutual exclusion between `i2c-atr` and `i2c-gate` is overly complex and redundant.
--
commit 38012c69a8fca6511a921f6751ee84fefe0e14a1
Author: Cosmin Tanislav <demonsingur@gmail.com>
dt-bindings: media: i2c: max96717: add support for I2C ATR
This commit adds support for I2C address translation (ATR) to the MAX96717
device tree bindings. It introduces the i2c-atr property and associated
address pools while maintaining support for the legacy i2c-gate property.
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> index d1e8ba6e368ec..976f6dd2c164c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
[ ... ]
> + patternProperties:
> + '^i2c@[01]$':
> + $ref: /schemas/i2c/i2c-controller.yaml#
> + unevaluatedProperties: false
> + properties:
> + reg:
> + items:
> + minimum: 0
> + maximum: 1
[Severity: Medium]
Does this properly validate the reg property? Since dt-schema parses reg as
a matrix (an array of tuples), applying minimum and maximum directly to
items causes JSON schema to silently ignore the numeric constraints, as they
are being applied to arrays instead of numbers.
Additionally, the lack of maxItems: 1 allows multiple register regions to
incorrectly pass validation.
[ ... ]
> +allOf:
> + - $ref: /schemas/i2c/i2c-atr.yaml#
> +
> + - anyOf:
> + - oneOf:
> + - required: [i2c-atr]
> + - required: [i2c-gate]
> +
> + - not:
> + required: [i2c-atr, i2c-gate]
[Severity: Low]
Is this anyOf block more complex than necessary?
The not branch evaluates to true when only one is present or when neither
is present, which completely subsumes the oneOf branch. Could this be
mathematically simplified to just use the not condition without changing
the validation logic?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=2
^ permalink raw reply
* [PATCH v3 0/2] remoteproc: qcom: Add Shikra remoteproc support
From: Komal Bajaj @ 2026-05-21 13:21 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel,
Bibek Kumar Patro, Komal Bajaj, Dmitry Baryshkov
Add Peripheral Authentication Service (PAS) support for the Qualcomm
Shikra SoC. This series introduces the device tree bindings and the
driver resource configurations for the three remoteproc cores present
on Shikra: CDSP, LPAICP and MPSS.
Signed-off-by: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
Signed-off-by: Komal Bajaj <komal.bajaj@oss.qualcomm.com>
---
Changes in v3:
- Fix dtschema/dtc warnings/errors (Rob's bot)
- Added firmware-name property in example (Krzysztof)
- Added power-domains and power-domains-names to false for
qcom,shikra-lpaicp-pas (Krzysztof)
- Collected Reviewed-By tag from Dmitry
- Link to v2: https://lore.kernel.org/r/20260520-shikra-rproc-v2-0-583a2e413a3a@oss.qualcomm.com
Changes in v2:
- Add glink-edge, qcom,smem-states and qcom,smem-state-names properties
in bindings
- Drop second interconnect path from the example node (sashiko-bot)
- Update auto_boot to true for CDSP resource (Dmitry)
- Drop region_assign_vmid from CDSP and MPSS resources (Dmitry)
- Drop decrypt_shutdown from MPSS resource (Dmitry)
- Link to v1: https://lore.kernel.org/r/20260514-shikra-rproc-v1-0-9afdedeee002@oss.qualcomm.com
---
Bibek Kumar Patro (1):
remoteproc: qcom: pas: Add Shikra remoteproc support
Komal Bajaj (1):
dt-bindings: remoteproc: qcom,shikra-pas: Document Shikra PAS remoteprocs
.../bindings/remoteproc/qcom,shikra-pas.yaml | 167 +++++++++++++++++++++
drivers/remoteproc/qcom_q6v5_pas.c | 48 ++++++
2 files changed, 215 insertions(+)
---
base-commit: 687da68900cd1a46549f7d9430c7d40346cb86a0
change-id: 20260513-shikra-rproc-0da355c56c69
Best regards,
--
Komal Bajaj <komal.bajaj@oss.qualcomm.com>
^ permalink raw reply
* [PATCH v3 1/2] dt-bindings: remoteproc: qcom,shikra-pas: Document Shikra PAS remoteprocs
From: Komal Bajaj @ 2026-05-21 13:21 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel,
Bibek Kumar Patro, Komal Bajaj
In-Reply-To: <20260521-shikra-rproc-v3-0-2fca0bbe1ad7@oss.qualcomm.com>
Document the bindings for the CDSP, LPAICP and MPSS PAS on
the Shikra SoC.
Signed-off-by: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
Signed-off-by: Komal Bajaj <komal.bajaj@oss.qualcomm.com>
---
.../bindings/remoteproc/qcom,shikra-pas.yaml | 167 +++++++++++++++++++++
1 file changed, 167 insertions(+)
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,shikra-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,shikra-pas.yaml
new file mode 100644
index 000000000000..253b14eb2b59
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,shikra-pas.yaml
@@ -0,0 +1,167 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,shikra-pas.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Shikra SoC Peripheral Authentication Service
+
+maintainers:
+ - Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
+ - Komal Bajaj <komal.bajaj@oss.qualcomm.com>
+
+description:
+ Qualcomm Shikra SoC Peripheral Authentication Service loads and boots
+ firmware on the Qualcomm DSP Hexagon cores.
+
+properties:
+ compatible:
+ enum:
+ - qcom,shikra-cdsp-pas
+ - qcom,shikra-lpaicp-pas
+ - qcom,shikra-mpss-pas
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: XO clock
+
+ clock-names:
+ items:
+ - const: xo
+
+ memory-region:
+ minItems: 1
+ maxItems: 2
+
+ smd-edge: false
+
+ firmware-name:
+ minItems: 1
+ items:
+ - description: Firmware name of the Hexagon core
+ - description: Firmware name of the Hexagon Devicetree
+
+ glink-edge:
+ $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
+ description:
+ Qualcomm G-Link subnode which represents communication edge, channels
+ and devices related to the remoteproc core.
+ unevaluatedProperties: false
+
+ qcom,smem-states:
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+ description: States used by the AP to signal the Hexagon core
+ items:
+ - description: Stop the remote processor
+ items:
+ - description: Phandle to the Shared Memory Point 2 Point device
+ handling the communication with a remote processor
+ - description: Single bit index to toggle in the value sent to
+ the remote processor
+ maximum: 32
+
+ qcom,smem-state-names:
+ description: The names of the state bits used for SMP2P output
+ items:
+ - const: stop
+
+required:
+ - compatible
+ - reg
+ - memory-region
+
+allOf:
+ - $ref: /schemas/remoteproc/qcom,pas-common.yaml#
+
+ - if:
+ properties:
+ compatible:
+ enum:
+ - qcom,shikra-cdsp-pas
+ - qcom,shikra-mpss-pas
+ then:
+ properties:
+ interrupts:
+ minItems: 6
+ interrupt-names:
+ minItems: 6
+ memory-region:
+ maxItems: 1
+ firmware-name:
+ maxItems: 1
+ power-domains:
+ items:
+ - description: CX power domain
+ power-domain-names:
+ items:
+ - const: cx
+
+ - if:
+ properties:
+ compatible:
+ enum:
+ - qcom,shikra-lpaicp-pas
+ then:
+ properties:
+ interrupts:
+ maxItems: 5
+ interrupt-names:
+ maxItems: 5
+ memory-region:
+ minItems: 2
+ firmware-name:
+ minItems: 2
+ power-domains: false
+ power-domain-names: false
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,rpmcc.h>
+ #include <dt-bindings/interconnect/qcom,icc.h>
+ #include <dt-bindings/interconnect/qcom,rpm-icc.h>
+ #include <dt-bindings/interconnect/qcom,shikra.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/power/qcom-rpmpd.h>
+
+ remoteproc@b300000 {
+ compatible = "qcom,shikra-cdsp-pas";
+ reg = <0x0b300000 0x100000>;
+
+ interrupts-extended = <&intc GIC_SPI 265 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "wdog", "fatal", "ready",
+ "handover", "stop-ack", "shutdown-ack";
+
+ clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>;
+ clock-names = "xo";
+
+ interconnects = <&mem_noc MASTER_AMPSS_M0 RPM_ALWAYS_TAG
+ &mc_virt SLAVE_EBI_CH0 RPM_ALWAYS_TAG>;
+
+ power-domains = <&rpmpd RPMHPD_CX>;
+ power-domain-names = "cx";
+
+ memory-region = <&cdsp_mem>;
+
+ qcom,smem-states = <&cdsp_smp2p_out 0>;
+ qcom,smem-state-names = "stop";
+
+ firmware-name = "qcom/shikra/cdsp.mbn";
+
+ glink-edge {
+ interrupts = <GIC_SPI 261 IRQ_TYPE_EDGE_RISING>;
+ mboxes = <&apcs_glb 4>;
+ qcom,remote-pid = <5>;
+ label = "cdsp";
+ };
+ };
--
2.34.1
^ permalink raw reply related
* [PATCH v3 2/2] remoteproc: qcom: pas: Add Shikra remoteproc support
From: Komal Bajaj @ 2026-05-21 13:21 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-remoteproc, devicetree, linux-kernel,
Bibek Kumar Patro, Komal Bajaj, Dmitry Baryshkov
In-Reply-To: <20260521-shikra-rproc-v3-0-2fca0bbe1ad7@oss.qualcomm.com>
From: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
Add the CDSP, LPAICP and MPSS Peripheral Authentication Service support
for the Qualcomm Shikra SoC.
Signed-off-by: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Komal Bajaj <komal.bajaj@oss.qualcomm.com>
---
drivers/remoteproc/qcom_q6v5_pas.c | 48 ++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index da27d1d3c9da..0101f1db6458 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -1457,6 +1457,51 @@ static const struct qcom_pas_data sc7280_wpss_resource = {
.ssctl_id = 0x19,
};
+static const struct qcom_pas_data shikra_cdsp_resource = {
+ .crash_reason_smem = 601,
+ .firmware_name = "cdsp.mbn",
+ .pas_id = 18,
+ .minidump_id = 7,
+ .auto_boot = true,
+ .proxy_pd_names = (char *[]){
+ "cx",
+ NULL
+ },
+ .load_state = "cdsp",
+ .ssr_name = "cdsp",
+ .sysmon_name = "cdsp",
+ .ssctl_id = 0x17,
+ .smem_host_id = 5,
+};
+
+static const struct qcom_pas_data shikra_lpaicp_resource = {
+ .crash_reason_smem = 682,
+ .firmware_name = "lpaicp.mbn",
+ .dtb_firmware_name = "lpaicp_dtb.mbn",
+ .pas_id = 0x56,
+ .dtb_pas_id = 0x57,
+ .minidump_id = 0,
+ .auto_boot = true,
+ .ssr_name = "lpaicp",
+ .sysmon_name = "lpaicp",
+};
+
+static const struct qcom_pas_data shikra_mpss_resource = {
+ .crash_reason_smem = 421,
+ .firmware_name = "qdsp6sw.mbn",
+ .pas_id = 4,
+ .minidump_id = 3,
+ .auto_boot = false,
+ .proxy_pd_names = (char *[]){
+ "cx",
+ NULL
+ },
+ .load_state = "modem",
+ .ssr_name = "mpss",
+ .sysmon_name = "modem",
+ .ssctl_id = 0x12,
+};
+
static const struct qcom_pas_data sm8650_cdsp_resource = {
.crash_reason_smem = 601,
.firmware_name = "cdsp.mdt",
@@ -1571,6 +1616,9 @@ static const struct of_device_id qcom_pas_of_match[] = {
{ .compatible = "qcom,sdm845-slpi-pas", .data = &sdm845_slpi_resource_init },
{ .compatible = "qcom,sdx55-mpss-pas", .data = &sdx55_mpss_resource },
{ .compatible = "qcom,sdx75-mpss-pas", .data = &sm8650_mpss_resource },
+ { .compatible = "qcom,shikra-cdsp-pas", .data = &shikra_cdsp_resource },
+ { .compatible = "qcom,shikra-lpaicp-pas", .data = &shikra_lpaicp_resource },
+ { .compatible = "qcom,shikra-mpss-pas", .data = &shikra_mpss_resource },
{ .compatible = "qcom,sm6115-adsp-pas", .data = &adsp_resource_init },
{ .compatible = "qcom,sm6115-cdsp-pas", .data = &cdsp_resource_init },
{ .compatible = "qcom,sm6115-mpss-pas", .data = &sc8180x_mpss_resource },
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 3/3] arm64: dts: qcom: purwa: Add camss node
From: Vikram Sharma @ 2026-05-21 13:22 UTC (permalink / raw)
To: Wenmeng Liu, Robert Foss, Todor Tomov, Bryan O'Donoghue,
Vladimir Zapolskiy, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio
Cc: linux-media, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20260511-purwa_camss-v2-3-22608ab9126c@oss.qualcomm.com>
On 5/11/2026 2:29 PM, Wenmeng Liu wrote:
> +&camss {
> + compatible = "qcom,x1p42100-camss";
> +
> + reg = <0 0x0acb7000 0 0x2000>,
> + <0 0x0acb9000 0 0x2000>,
> + <0 0x0acbb000 0 0x2000>,
> + <0 0x0acc6000 0 0x1000>,
> + <0 0x0acca000 0 0x1000>,
> + <0 0x0acb6000 0 0x1000>,
> + <0 0x0ace4000 0 0x2000>,
> + <0 0x0acec000 0 0x4000>,
Size not matching with bindings, Please cross check with SWI.
> + <0 0x0acf6000 0 0x1000>,
> + <0 0x0acf7000 0 0x1000>,
> + <0 0x0acf8000 0 0x1000>,
> + <0 0x0ac62000 0 0xf000>,
> + <0 0x0acc7000 0 0x2000>,
> + <0 0x0accb000 0 0x2000>;
> +
Regards,
Vikram
^ permalink raw reply
* Re: [PATCH v12 14/22] media: i2c: add Maxim GMSL2/3 serializer framework
From: sashiko-bot @ 2026-05-21 13:23 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: devicetree, conor+dt, robh
In-Reply-To: <20260521-gmsl2-3_serdes-v12-14-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] Uninitialized stack memory usage and stack buffer overflow with `v4l2_mbus_frame_desc`.
- [High] Heap buffer overflow in `max_ser_get_pipe_vcs_dts()` when storing unique Data Types.
- [High] Array out-of-bounds write in `max_ser_i2c_atr_detach_addr()`.
- [High] Use of uninitialized stack variable and Undefined Behavior via `__ffs(0)` / `__fls(0)`.
- [High] Lockless iteration in `max_ser_log_status` leads to Use-After-Free.
- [High] Error rollback paths rely on infallible dynamic memory allocations.
- [High] Use of stack-allocated buffers for `i2c_transfer()` violates DMA safety.
- [Medium] Multiple fwnode reference count leaks.
- [Medium] V4L2 control handler memory is leaked upon driver unregistration.
- [Medium] Memory leak of the I2C ATR structure on adapter initialization failure.
- [Medium] Missing `.get_mbus_config()` API implementation despite commit message claims.
- [Medium] Inappropriate use of devres functions in runtime hot paths.
--
commit 210e31826495bb37471b675b425310f6af76310b
Author: Cosmin Tanislav <demonsingur@gmail.com>
Date: Thu May 21 12:44:20 2026 +0000
media: i2c: add Maxim GMSL2/3 serializer framework
These drivers are meant to be used as a common framework for Maxim
GMSL2/3 serializers.
This framework enables support for the following new features across
all the chips:
* Full Streams API support
* .get_frame_desc()
* .get_mbus_config()
* I2C ATR
[Severity: Medium]
The commit message states that .get_mbus_config() is supported. However, it
does not appear to be implemented or linked in the max_ser_pad_ops structure.
Was this feature intentionally removed during development, or did it get missed?
[ ... ]
diff --git a/drivers/media/i2c/maxim-serdes/max_ser.c b/drivers/media/i2c/maxim-serdes/max_ser.c
new file mode 100644
index 0000000000000..9eb33cf83c076
--- /dev/null
+++ b/drivers/media/i2c/maxim-serdes/max_ser.c
@@ -0,0 +1,2155 @@
[ ... ]
> +static int max_ser_route_to_hw(struct max_ser_priv *priv,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_route *route,
> + struct max_ser_route_hw *hw)
> +{
> + struct max_ser *ser = priv->ser;
> + struct v4l2_mbus_frame_desc fd;
> + struct max_ser_phy *phy;
[Severity: Critical]
Does this leave the num_entries field of fd uninitialized?
Since fd is declared on the stack, it may contain random garbage. When passed
to the source subdev's get_frame_desc pad op, the target function frequently
executes fd->entry[fd->num_entries++] = hw.entry; which could result in
out-of-bounds writes on the stack buffer. Would it be better to
zero-initialize it here?
> + unsigned int i;
> + int ret;
> +
> + memset(hw, 0, sizeof(*hw));
[ ... ]
> +static void max_ser_i2c_atr_detach_addr(struct i2c_atr *atr, u32 chan_id, u16 addr)
> +{
> + struct max_ser_priv *priv = i2c_atr_get_driver_data(atr);
> + struct max_ser *ser = priv->ser;
> + struct max_serdes_i2c_xlate xlate = { 0 };
> + unsigned int i;
> +
> + /* Find index of matching I2C translation. */
> + for (i = 0; i < ser->ops->num_i2c_xlates; i++)
> + if (ser->i2c_xlates[i].dst == addr)
> + break;
> +
> + WARN_ON(i == ser->ops->num_i2c_xlates);
> +
> + ser->ops->set_i2c_xlate(ser, i, &xlate);
> + ser->i2c_xlates[i] = xlate;
> +}
[Severity: High]
If the loop finishes without finding a match, i will be exactly equal to
ser->ops->num_i2c_xlates. While the WARN_ON flags this case, execution continues
and accesses ser->i2c_xlates[i]. Can this cause an out-of-bounds write past
the end of the i2c_xlates array? Should the function return early if the
address is not found?
[ ... ]
> +static int max_ser_i2c_atr_init(struct max_ser_priv *priv)
> +{
> + struct i2c_atr_adap_desc desc = {
> + .chan_id = 0,
> + };
> +
> + if (!i2c_check_functionality(priv->client->adapter,
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> + return -ENODEV;
> +
> + priv->atr = i2c_atr_new(priv->client->adapter, priv->dev,
> + &max_ser_i2c_atr_ops, 1, 0);
> + if (IS_ERR(priv->atr))
> + return PTR_ERR(priv->atr);
> +
> + i2c_atr_set_driver_data(priv->atr, priv);
> +
> + return i2c_atr_add_adapter(priv->atr, &desc);
> +}
[Severity: Medium]
If i2c_atr_add_adapter() fails, does this permanently leak the memory
allocated by i2c_atr_new()?
[ ... ]
> +static int max_ser_i2c_adapter_init(struct max_ser_priv *priv)
> +{
> + if (device_get_named_child_node(priv->dev, "i2c-gate"))
> + return max_ser_i2c_mux_init(priv);
> + else
> + return max_ser_i2c_atr_init(priv);
> +}
[Severity: Medium]
Does device_get_named_child_node() return a reference-counted fwnode
pointer? It appears the pointer is used here only for a truthiness check and
is immediately discarded without calling fwnode_handle_put(). Can this lead
to a reference count leak during driver probe?
[ ... ]
> +static int max_ser_log_status(struct v4l2_subdev *sd)
> +{
> + struct max_ser_priv *priv = sd_to_priv(sd);
> + struct max_ser *ser = priv->ser;
> + unsigned int i, j;
> + int ret;
[ ... ]
> + for (i = 0; i < ser->ops->num_pipes; i++) {
> + struct max_ser_pipe *pipe = &ser->pipes[i];
> +
> + v4l2_info(sd, "pipe: %u\n", pipe->index);
> + v4l2_info(sd, "\tenabled: %u\n", pipe->enabled);
> +
> + if (!pipe->enabled) {
> + v4l2_info(sd, "\n");
> + continue;
> + }
> +
> + v4l2_info(sd, "\tphy_id: %u\n", pipe->phy_id);
> + v4l2_info(sd, "\tstream_id: %u\n", pipe->stream_id);
> + if (ser->ops->set_pipe_phy)
> + v4l2_info(sd, "\tphy_id: %u\n", pipe->phy_id);
> + if (ser->ops->set_pipe_dt) {
> + v4l2_info(sd, "\tdts: %u\n", pipe->num_dts);
> + for (j = 0; j < pipe->num_dts; j++)
> + v4l2_info(sd, "\t\tdt: 0x%02x\n", pipe->dts[j]);
> + }
[Severity: High]
Is this iteration over pipe->dts safe from data races?
It looks like max_ser_log_status() iterates locklessly here, but
max_ser_update_pipe() dynamically frees and reallocates pipe->dts
using devm_kfree(priv->dev, pipe->dts) during runtime stream configuration.
Could concurrent execution of these two code paths trigger a use-after-free?
[ ... ]
> +static int max_ser_get_pipe_vcs_dts(struct max_ser_priv *priv,
> + struct v4l2_subdev_state *state,
> + struct max_ser_pipe *pipe,
> + unsigned int *vcs,
> + unsigned int *dts, unsigned int *num_dts,
> + u64 *streams_masks)
> +{
> + struct v4l2_subdev_route *route;
> + struct max_ser *ser = priv->ser;
> + unsigned int i;
> + int ret;
> +
> + *vcs = 0;
> + *num_dts = 0;
[ ... ]
> + /* Skip already added DT. */
> + for (i = 0; i < *num_dts; i++)
> + if (dts[i] == dt)
> + break;
> +
> + if (i < *num_dts)
> + continue;
> +
> + dts[*num_dts] = dt;
> + (*num_dts)++;
> + }
[Severity: High]
Is there a risk of an out-of-bounds array write here?
The dts array is dynamically allocated with a fixed capacity of
ser->ops->num_dts_per_pipe. If a userspace-configured routing table supplies
more unique streams than this limit, it seems *num_dts could exceed the
bounds of the array. Should we add a bounds check?
[ ... ]
> +static int max_ser_get_pipe_mode(struct max_ser_priv *priv,
> + struct v4l2_subdev_state *state,
> + struct max_ser_pipe *pipe,
> + struct max_ser_pipe_mode *mode)
> +{
> + struct v4l2_subdev_route *route;
> + struct max_ser *ser = priv->ser;
> + bool force_set_bpp = false;
> + unsigned int doubled_bpp;
> + unsigned int min_bpp;
> + unsigned int max_bpp;
> + u32 bpps = 0;
> + int ret;
> +
> + if (ser->mode != MAX_SERDES_GMSL_PIXEL_MODE)
> + return 0;
> +
> + for_each_active_route(&state->routing, route) {
[ ... ]
> + }
> +
> + ret = max_serdes_process_bpps(priv->dev, bpps, priv->double_bpps, &doubled_bpp);
> + if (ret)
> + return ret;
> +
> + if (doubled_bpp == 8)
> + mode->dbl8 = true;
> + else if (doubled_bpp == 10)
> + mode->dbl10 = true;
> + else if (doubled_bpp == 12)
> + mode->dbl12 = true;
> +
> + if (doubled_bpp) {
> + bpps &= ~BIT(doubled_bpp);
> + bpps |= BIT(doubled_bpp * 2);
> + }
> +
> + min_bpp = __ffs(bpps);
> + max_bpp = __fls(bpps);
[Severity: High]
If the pipe has no active routes, bpps will remain 0. Calling
max_serdes_process_bpps() with bpps = 0 appears to return 0 immediately
without setting doubled_bpp, leaving it uninitialized for later use.
Furthermore, if bpps is 0, does calling __ffs(bpps) and __fls(bpps)
result in undefined behavior? These typically map to __builtin_ctzl(), which
may cause faults or unpredictable behavior when called with zero.
[ ... ]
> +static int max_ser_update_pipe(struct max_ser_priv *priv,
> + struct max_ser_pipe *pipe,
> + struct v4l2_subdev_state *state,
> + u64 *streams_masks)
> +{
> + struct max_ser *ser = priv->ser;
> + struct max_ser_pipe_mode mode = { 0 };
> + unsigned int num_dts;
> + unsigned int *dts;
> + unsigned int vcs;
> + int ret;
> +
> + if (!ser->ops->num_dts_per_pipe)
> + return 0;
> +
> + dts = devm_kcalloc(priv->dev, ser->ops->num_dts_per_pipe, sizeof(*dts),
> + GFP_KERNEL);
[Severity: Medium]
Is devm_kcalloc() suitable for use in a runtime hot path like stream
enablement/disablement?
Using devres functions here requires taking a spinlock and performing an O(N)
list traversal for every allocation and free, which can degrade performance
and fragment the list.
[Severity: High]
Additionally, error rollback paths like max_ser_update_phys() call
max_ser_update_pipe() to restore previous states. If this devm_kcalloc()
fails during a rollback, the error is ignored, leaving the hardware and
software states out of sync. Could this be mitigated by pre-allocating the
arrays during probe?
[ ... ]
> +static int max_ser_update_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + u32 pad, u64 updated_streams_mask, bool enable)
> +{
> + struct max_ser_priv *priv = v4l2_get_subdevdata(sd);
> + struct max_ser *ser = priv->ser;
> + unsigned int num_pads = max_ser_num_pads(ser);
> + u64 *streams_masks;
> + int ret;
> +
> + ret = max_serdes_get_streams_masks(priv->dev, state, pad, updated_streams_mask,
> + num_pads, priv->streams_masks, &streams_masks,
> + enable);
[ ... ]
> + devm_kfree(priv->dev, priv->streams_masks);
> + priv->streams_masks = streams_masks;
> + ser->active = !!streams_masks[pad];
> +
> + return 0;
[Severity: Medium]
Similar to the dts array allocation above, is it acceptable to continually
allocate and free streams_masks via devres APIs during stream start/stop
operations?
[ ... ]
> +static void max_ser_v4l2_unregister(struct max_ser_priv *priv)
> +{
> + struct v4l2_subdev *sd = &priv->sd;
> +
> + max_ser_v4l2_notifier_unregister(priv);
> + v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> + media_entity_cleanup(&sd->entity);
> +}
[Severity: Medium]
The V4L2 control handler is initialized during probe with
v4l2_ctrl_handler_init(), but it doesn't seem to be freed here. Should we
call v4l2_ctrl_handler_free(&priv->ctrl_handler) to prevent memory leaks
upon module unregistration?
[ ... ]
> +static int max_ser_parse_sink_dt_endpoint(struct max_ser_priv *priv,
> + struct max_ser_phy *phy,
> + struct max_serdes_source *source,
> + struct fwnode_handle *fwnode)
> +{
> + struct max_ser *ser = priv->ser;
> + u32 pad = max_ser_phy_to_pad(ser, phy);
> + struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
> + struct fwnode_handle *ep;
> + int ret;
> +
> + ep = fwnode_graph_get_endpoint_by_id(fwnode, pad, 0, 0);
> + if (!ep)
> + return 0;
> +
> + source->ep_fwnode = fwnode_graph_get_remote_endpoint(ep);
> + if (!source->ep_fwnode) {
> + dev_err(priv->dev,
> + "Failed to get remote endpoint on port %u\n", pad);
> + return -EINVAL;
> + }
[Severity: Medium]
If fwnode_graph_get_remote_endpoint() fails, the function returns -EINVAL
without putting the ep fwnode pointer. Could this cause a fwnode reference
count leak?
[ ... ]
> +static int max_ser_read_reg(struct i2c_adapter *adapter, u8 addr,
> + u16 reg, u8 *val)
> +{
> + u8 buf[2] = { reg >> 8, reg & 0xff };
> + struct i2c_msg msg[2] = {
> + {
> + .addr = addr,
> + .flags = 0,
> + .buf = buf,
> + .len = sizeof(buf),
> + },
> + {
> + .addr = addr,
> + .flags = I2C_M_RD,
> + .buf = buf,
> + .len = 1,
> + },
> + };
> + int ret;
> +
> + ret = i2c_transfer(adapter, msg, ARRAY_SIZE(msg));
> + if (ret < 0)
> + return ret;
> +
> + *val = buf[0];
> +
> + return 0;
> +}
[Severity: High]
Is it safe to pass a stack-allocated buffer to i2c_transfer()?
Because many underlying I2C controller drivers utilize DMA for data transfers,
passing stack-allocated memory can be unsafe and trigger panics or data
corruption, especially with CONFIG_VMAP_STACK enabled.
> +static int max_ser_write_reg(struct i2c_adapter *adapter, u8 addr,
> + u16 reg, u8 val)
> +{
> + u8 buf[3] = { reg >> 8, reg & 0xff, val };
> + struct i2c_msg msg[1] = {
> + {
> + .addr = addr,
> + .flags = 0,
> + .buf = buf,
> + .len = sizeof(buf),
> + },
> + };
> + int ret;
> +
> + ret = i2c_transfer(adapter, msg, ARRAY_SIZE(msg));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
[Severity: High]
Similarly, does this stack-allocated buffer face the same DMA safety issues
when passed to i2c_transfer()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=14
^ permalink raw reply related
* Re: [PATCH v22 08/13] mfd: core: Add firmware-node support to MFD cells
From: Lee Jones @ 2026-05-21 13:24 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Shivendra Pratap, Sebastian Reichel, Mark Rutland,
Lorenzo Pieralisi, Rafael J. Wysocki, Daniel Lezcano,
Christian Loehle, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Konrad Dybcio, Arnd Bergmann,
Souvik Chakravarty, Andy Yan, Matthias Brugger, John Stultz,
Moritz Fischer, Sudeep Holla, linux-pm, linux-kernel,
linux-arm-msm, linux-arm-kernel, devicetree, Florian Fainelli,
Krzysztof Kozlowski, Dmitry Baryshkov, Mukesh Ojha, Andre Draszik,
Greg Kroah-Hartman, Kathiravan Thirumoorthy, Srinivas Kandagatla,
Bartosz Golaszewski
In-Reply-To: <CAMRc=MfqaCjiALZyVBHQs=Taft1M9xmNTFvQHWPrd5PgcTfJDQ@mail.gmail.com>
On Thu, 21 May 2026, Bartosz Golaszewski wrote:
> On Thu, May 21, 2026 at 1:26 PM Lee Jones <lee@kernel.org> wrote:
> >
> > On Thu, 14 May 2026, Shivendra Pratap wrote:
> >
> > > MFD core has no way to register a child device using an explicit firmware
> > > node. This prevents drivers from registering child nodes when those nodes
> > > do not define a compatible string. One such example is the PSCI
> > > "reboot-mode" node, which omits a compatible string as it describes
> > > boot-states provided by the underlying firmware.
> > >
> > > Extend struct mfd_cell with a callback that allows drivers to provide an
> > > explicit firmware node. The node is added to the MFD child device during
> > > registration when none is assigned by device tree, ACPI, or software
> > > matching.
> > >
> > > Suggested-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > > Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
> > > ---
> > > drivers/mfd/mfd-core.c | 30 ++++++++++++++++++++++++++++++
> > > include/linux/mfd/core.h | 14 ++++++++++++++
> > > 2 files changed, 44 insertions(+)
> > >
> > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > index 7aa32b90cf1eb7fa0a05bf3dc506e60a262c9850..cc2a2a924d6d3044e29a9f864b536ee325ed797b 100644
> > > --- a/drivers/mfd/mfd-core.c
> > > +++ b/drivers/mfd/mfd-core.c
> > > @@ -10,6 +10,7 @@
> > > #include <linux/kernel.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/acpi.h>
> > > +#include <linux/fwnode.h>
> > > #include <linux/list.h>
> > > #include <linux/property.h>
> > > #include <linux/mfd/core.h>
> > > @@ -148,6 +149,11 @@ static int mfd_match_of_node_to_dev(struct platform_device *pdev,
> > > return 0;
> > > }
> > >
> > > +static void mfd_child_fwnode_put(void *data)
> > > +{
> > > + fwnode_handle_put(data);
> > > +}
> > > +
> > > static int mfd_add_device(struct device *parent, int id,
> > > const struct mfd_cell *cell,
> > > struct resource *mem_base,
> > > @@ -156,6 +162,7 @@ static int mfd_add_device(struct device *parent, int id,
> > > struct resource *res;
> > > struct platform_device *pdev;
> > > struct mfd_of_node_entry *of_entry, *tmp;
> > > + struct fwnode_handle *fwnode;
> > > bool disabled = false;
> > > int ret = -ENOMEM;
> > > int platform_id;
> > > @@ -224,6 +231,29 @@ static int mfd_add_device(struct device *parent, int id,
> > >
> > > mfd_acpi_add_device(cell, pdev);
> > >
> > > + if (!pdev->dev.fwnode && cell->get_child_fwnode) {
> > > + fwnode = cell->get_child_fwnode(parent);
> > > + if (fwnode) {
> > > + device_set_node(&pdev->dev, fwnode);
> > > +
> > > + /*
> > > + * platform_device_release() drops only of_node refs.
> > > + * Track non-OF fwnodes explicitly so they are put on
> > > + * all teardown paths.
> > > + */
> > > + if (!to_of_node(fwnode)) {
> > > + ret = devm_add_action(&pdev->dev,
> > > + mfd_child_fwnode_put,
> > > + fwnode);
> > > + if (ret) {
> > > + device_set_node(&pdev->dev, NULL);
> > > + fwnode_handle_put(fwnode);
> > > + goto fail_of_entry;
> > > + }
> > > + }
> > > + }
> > > + }
> >
> > mfd_add_device() is getting very busy now with support for all of these
> > different registration APIs. Suggest that we start breaking them out.
> >
> > > +
> > > if (cell->pdata_size) {
> > > ret = platform_device_add_data(pdev,
> > > cell->platform_data, cell->pdata_size);
> > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > > index faeea7abd688f223fb0b31cde0a9b69dfe2a61ff..abfc26c057d6ee46947ba2b6f2e99f420e74b127 100644
> > > --- a/include/linux/mfd/core.h
> > > +++ b/include/linux/mfd/core.h
> > > @@ -50,6 +50,7 @@
> > > #define MFD_DEP_LEVEL_HIGH 1
> > >
> > > struct irq_domain;
> > > +struct fwnode_handle;
> > > struct software_node;
> > >
> > > /* Matches ACPI PNP id, either _HID or _CID, or ACPI _ADR */
> > > @@ -80,6 +81,19 @@ struct mfd_cell {
> > >
> > > /* Software node for the device. */
> > > const struct software_node *swnode;
> > > + /*
> > > + * Callback to return an explicit firmware node.
> > > + * @parent: MFD parent device passed to mfd_add_devices().
> > > + *
> > > + * Called only if OF/ACPI matching did not assign a fwnode.
> > > + * Ownership of the returned reference is transferred to MFD core.
> > > + *
> > > + * Return a referenced fwnode or NULL if none is available.
> > > + *
> > > + * mfd_cell must be zero-initialized or get_child_fwnode must be NULL
> > > + * when unused.
> > > + */
> > > + struct fwnode_handle *(*get_child_fwnode)(struct device *parent);
> >
> > I'm very much against pointers to functions if they can be avoided. Why
> > does fwnode need this and none of the other APIs do?
> >
>
> I suggested it because of its flexibility. The alternative I had in
> mind is something like a new field in mfd_cell:
>
> const char *cell_node_name;
>
> Which - if set - would tell MFD to look up an fwnode that's a child of
> the parent device's node by name - as it may not have a compatible.
Remind me why the chlid device can't look-up its own fwnode?
--
Lee Jones
^ permalink raw reply
* Re: [PATCH v12 15/22] media: i2c: add Maxim GMSL2/3 deserializer framework
From: sashiko-bot @ 2026-05-21 13:27 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260521-gmsl2-3_serdes-v12-15-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] Missing initialization and bounds check for `fd->num_entries` in `max_des_get_frame_desc_state`.
- [High] Stack buffer overflow in `max_des_set_pipes_stream_id` via unvalidated `stream_id`.
- [High] Checking `pipe->num_vc_remaps` instead of `pipe->vc_remaps` causes sequential devm memory leaks.
- [High] I2C bus notifier relies on an adapter array that is not yet populated during synchronous driver binding.
- [High] Uninitialized `struct v4l2_mbus_frame_desc fd` is passed to the serializer subdev.
- [High] Output struct fields are used as search constraints in `max_des_enum_frame_interval`.
- [High] Unbalanced POC regulator unwinding causes warnings and halts cleanup.
- [High] Unprotected access to `source->sd` leads to TOCTOU NULL pointer dereferences and dangling pointers.
- [High] Lockless array iteration in `max_des_log_status` causes a Use-After-Free (UAF) vulnerability.
- [High] Hardware state corruption due to an I2C multiplexing race condition during serializer binding.
- [High] Incorrect `chan_id` mapping in `max_des_i2c_mux_bus_notifier_call`.
- [Medium] Fwnode reference leak in `max_des_parse_sink_dt_endpoint`.
- [Medium] Memory leak of `streams_masks` on the error path in `max_des_update_streams`.
- [Medium] I2C ATR instances and adapters are leaked if link selection fails in `max_des_i2c_atr_init`.
--
commit 83b14a77624d98c746a5672b763eea7d8ac99877
Author: Cosmin Tanislav <demonsingur@gmail.com>
Subject: media: i2c: add Maxim GMSL2/3 deserializer framework
This commit introduces a common framework for Maxim GMSL2/3 deserializers.
It adds support for features like the full Streams API, I2C ATR, automatic
GMSL link version negotiation, and automatic stream ID selection.
> diff --git a/drivers/media/i2c/maxim-serdes/max_des.c b/drivers/media/i2c/maxim-serdes/max_des.c
> new file mode 100644
> index 0000000000000..a03101678360f
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_des.c
> @@ -0,0 +1,3205 @@
[ ... ]
> +static int max_des_route_to_hw(struct max_des_priv *priv,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_route *route,
> + struct max_des_route_hw *hw)
> +{
> + struct max_des *des = priv->des;
> + struct v4l2_mbus_frame_desc fd;
[Severity: High]
Could the uninitialized fd here lead to issues?
If the subdev expects fd.num_entries to start at 0, or reads other fields,
it might operate on garbage stack data.
> + struct max_des_link *link;
> + unsigned int i;
> + int ret;
[ ... ]
> +static int max_des_set_pipes_stream_id(struct max_des_priv *priv)
> +{
> + bool stream_id_usage[MAX_SERDES_STREAMS_NUM] = { 0 };
> + struct max_des *des = priv->des;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < des->info->num_links; i++) {
> + struct max_des_link_hw hw;
> + unsigned int stream_id;
[ ... ]
> + if (ret == -EOPNOTSUPP) {
> + /*
> + * Serializer does not support setting the stream id,
> + * retrieve its hardcoded stream id.
> + */
> + ret = max_ser_get_stream_id(hw.source->sd, &stream_id);
> + }
> +
> + if (ret)
> + return ret;
> +
> + if (stream_id_usage[stream_id] && des->info->needs_unique_stream_id) {
[Severity: High]
Is a bounds check needed for stream_id before using it to index the
stream_id_usage stack array?
A malformed or unexpectedly large value from the serializer could cause an
out-of-bounds stack write here.
> + dev_err(priv->dev, "Duplicate stream id %u\n", stream_id);
> + return -EINVAL;
> + }
[ ... ]
> +static int max_des_update_pipe_vc_remaps(struct max_des_priv *priv,
> + struct max_des_remap_context *context,
> + struct max_des_pipe *pipe,
> + struct v4l2_subdev_state *state,
> + u64 *streams_masks)
> +{
[ ... ]
> + ret = max_des_set_pipe_vc_remaps(priv, pipe, vc_remaps, num_vc_remaps);
> + if (ret)
> + goto err_free_new_vc_remaps;
> +
> + if (pipe->num_vc_remaps)
> + devm_kfree(priv->dev, pipe->vc_remaps);
[Severity: High]
Is there a potential memory leak if the previous state allocated memory but
populated 0 remaps?
Checking pipe->num_vc_remaps instead of the pointer pipe->vc_remaps might
bypass the free on stream updates, leaving the device-managed memory leaked.
> +
> + pipe->vc_remaps = vc_remaps;
> + pipe->num_vc_remaps = num_vc_remaps;
> +
> + return 0;
[ ... ]
> +static int max_des_init_link_ser_xlate(struct max_des_priv *priv,
> + struct max_des_link *link,
> + struct i2c_adapter *adapter,
> + u8 power_up_addr, u8 new_addr)
> +{
> + struct max_des *des = priv->des;
> + u8 addrs[] = { power_up_addr, new_addr };
> + u8 current_addr;
> + int ret;
> +
> + ret = des->ops->select_links(des, BIT(link->index));
> + if (ret)
> + return ret;
> +
> + ret = max_ser_wait_for_multiple(adapter, addrs, ARRAY_SIZE(addrs),
> + ¤t_addr);
[Severity: High]
Can setting the hardware mux directly via select_links and performing I2C
transactions bypass the i2c_mux core locks?
Since the bus notifier can execute concurrently while different clients probe,
this sequence could potentially overwrite the hardware mux state and misdirect
I2C traffic to the wrong serializers.
> + if (ret) {
> + dev_err(priv->dev,
[ ... ]
> +static int max_des_i2c_atr_init(struct max_des_priv *priv)
> +{
[ ... ]
> + for (i = 0; i < des->info->num_links; i++) {
> + struct max_des_link *link = &des->links[i];
> +
> + if (!link->enabled)
> + continue;
> +
> + mask |= BIT(link->index);
> + }
> +
> + return des->ops->select_links(des, mask);
[Severity: Medium]
If des->ops->select_links(des, mask) fails here, should it clean up the
allocated resources?
Returning the error code immediately bypasses the err_add_adapters label,
which seems like it would leak the ATR object and its adapters.
> +
> +err_add_adapters:
> + max_des_i2c_atr_deinit(priv);
[ ... ]
> +static int max_des_i2c_mux_bus_notifier_call(struct notifier_block *nb,
> + unsigned long event, void *device)
> +{
[ ... ]
> + client = i2c_verify_client(dev);
> + if (!client)
> + return NOTIFY_DONE;
> +
> + for (chan_id = 0; chan_id < priv->mux->max_adapters; ++chan_id) {
> + if (client->adapter == priv->mux->adapter[chan_id])
> + break;
> + }
[Severity: High]
Is it guaranteed that the priv->mux->adapter array is populated at this
point?
If a serializer binds synchronously during adapter registration, this notifier
might fire before i2c_mux_add_adapter() populates the array, causing it to
silently skip link version negotiation.
> +
> + if (chan_id == priv->mux->max_adapters)
> + return NOTIFY_DONE;
> +
> + max_des_ser_attach_addr(priv, chan_id, client->addr, client->addr);
[Severity: High]
Additionally, does this chan_id indexing correctly map to the hardware links?
Because i2c_mux_add_adapter() packs adapters sequentially for enabled links,
if link 0 is disabled, link 1 might sit at array index 0. This could cause
the driver to initialize link 0's data structures for a device connected to
link 1.
> +
> + return NOTIFY_DONE;
> +}
[ ... ]
> +static int max_des_enum_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_frame_interval_enum *fie)
> +{
> + struct max_des_priv *priv = v4l2_get_subdevdata(sd);
> + struct max_des *des = priv->des;
> + const struct max_serdes_tpg_entry *entry;
> +
> + if (!max_des_pad_is_tpg(des, fie->pad) ||
> + fie->stream != MAX_SERDES_TPG_STREAM)
> + return -ENOTTY;
> +
> + entry = max_des_find_tpg_entry(des, fie->index, fie->width, fie->height,
> + fie->code, fie->interval.denominator,
> + fie->interval.numerator);
[Severity: High]
Is it safe to use fie->interval.numerator and denominator as search
constraints here?
Since interval is an output parameter during
VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL, userspace might provide uninitialized
memory, which would cause the search to fail with garbage data.
> + if (!entry)
> + return -EINVAL;
[ ... ]
> +static int max_des_log_status(struct v4l2_subdev *sd)
> +{
[ ... ]
> + if (des->ops->set_pipe_remap) {
> + v4l2_info(sd, "\tremaps: %u\n", pipe->num_remaps);
> + for (j = 0; j < pipe->num_remaps; j++) {
> + struct max_des_remap *remap = &pipe->remaps[j];
[Severity: High]
Does this function need to hold the subdev active state lock while iterating
over pipe->remaps and pipe->vc_remaps?
Concurrent stream updates might free and reallocate these arrays under the
lock, which could lead to a use-after-free here.
> +
> + v4l2_info(sd, "\t\tremap: from: vc: %u, dt: 0x%02x\n",
> + remap->from_vc, remap->from_dt);
[ ... ]
> +static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_mbus_frame_desc *fd,
> + unsigned int pad)
> +{
> + struct max_des_remap_context context = { 0 };
> + struct max_des_priv *priv = sd_to_priv(sd);
> + struct v4l2_subdev_route *route;
> + int ret;
> +
> + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
[Severity: High]
Should fd->num_entries be explicitly initialized to 0 here before appending
new entries?
Since it appears this might be called with an uninitialized stack variable
from other places (like max_des_route_to_hw), it could result in an
out-of-bounds write to the array.
> +
> + ret = max_des_populate_remap_context(priv, &context, state);
> + if (ret)
[ ... ]
> +static int max_des_update_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + u32 pad, u64 updated_streams_mask, bool enable)
> +{
[ ... ]
> + ret = max_des_set_modes(priv, &mode_context);
> + if (ret)
> + goto err_free_streams_masks;
> +
> + ret = max_des_set_vc_remaps(priv, &context, state, streams_masks);
> + if (ret)
> + return ret;
[Severity: Medium]
If max_des_set_vc_remaps() fails, should this go to
err_free_streams_masks?
Returning the error code directly seems to skip freeing the dynamically
allocated streams_masks.
> +
> + ret = max_des_set_pipes_stream_id(priv);
[ ... ]
> +static int max_des_notify_bound(struct v4l2_async_notifier *nf,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_connection *base_asc)
> +{
[ ... ]
> + source->sd = subdev;
> + source->pad = ret;
> +
> + ret = media_create_pad_link(&source->sd->entity, source->pad,
> + &priv->sd.entity, pad,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> + if (ret) {
> + dev_err(priv->dev, "Unable to link %s:%u -> %s:%u\n",
> + source->sd->name, source->pad, priv->sd.name, pad);
[Severity: High]
If media_create_pad_link() fails here, should source->sd be set back to
NULL?
Leaving it populated on failure might result in a dangling pointer.
> + return ret;
> + }
> +
> + return 0;
> +}
[ ... ]
> +static void max_des_notify_unbind(struct v4l2_async_notifier *nf,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_connection *base_asc)
> +{
> + struct max_serdes_asc *asc = asc_to_max(base_asc);
> + struct max_serdes_source *source = asc->source;
> +
> + source->sd = NULL;
[Severity: High]
Could setting source->sd = NULL here without locking race against streaming
paths like max_des_set_tunnel()?
This looks like it could lead to a time-of-check to time-of-use NULL pointer
dereference.
> +}
[ ... ]
> +static int max_des_update_pocs(struct max_des_priv *priv, bool enable)
> +{
[ ... ]
> + if (enable)
> + ret = regulator_enable(priv->pocs[i]);
> + else
> + ret = regulator_disable(priv->pocs[i]);
> +
> + if (ret) {
> + dev_err(priv->dev,
> + "Failed to set POC supply to %u: %u\n",
> + enable, ret);
[Severity: High]
If regulator_enable() fails here, the function returns without disabling
the already-enabled links.
During the error path cleanup, this function is called again with
enable = false, which tries to disable the regulator that originally failed.
Could this trigger a kernel warning and halt further unwinding, leaving
genuine regulators permanently enabled?
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
[ ... ]
> +static int max_des_parse_sink_dt_endpoint(struct max_des_priv *priv,
> + struct max_des_link *link,
> + struct max_serdes_source *source,
> + struct fwnode_handle *fwnode)
> +{
[ ... ]
> + ep = fwnode_graph_get_endpoint_by_id(fwnode, pad, 0, 0);
> + if (!ep)
> + return 0;
> +
> + source->ep_fwnode = fwnode_graph_get_remote_endpoint(ep);
> + fwnode_handle_put(ep);
> + if (!source->ep_fwnode) {
[Severity: Medium]
Does this code leak the fwnode handle acquired for source->ep_fwnode?
It doesn't seem to be released via fwnode_handle_put() anywhere in the
driver's teardown path or error cleanup.
> + dev_err(priv->dev,
> + "Failed to get remote endpoint on port %u\n", pad);
> + return -ENODEV;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=15
^ permalink raw reply
* Re: [PATCH v12 16/22] media: i2c: maxim-serdes: add MAX96717 driver
From: sashiko-bot @ 2026-05-21 13:33 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260521-gmsl2-3_serdes-v12-16-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect array mapping logic for Slew Rate PIN configuration
- [High] Unconditional hardcoding of rclkout pin mux bypasses pinctrl framework
- [High] Race condition exposing GPIOs and Clock before hardware initialization
- [High] gpio_get_direction returns an error for input pins
- [Medium] Kconfig dependency propagation issue for VIDEO_MAX96717
- [Medium] Log spam in clock determine_rate callback
--
commit e3bc10c21430d0bc98ab74141ac3217a5001c9ab
Author: Cosmin Tanislav <demonsingur@gmail.com>
media: i2c: maxim-serdes: add MAX96717 driver
Add a new MAX96717 driver that also supports MAX9295A, MAX96717F and
MAX96793.
Integrate it with the common serializer framework, while keeping
compatibility with existing usecases, avoiding code duplication, and
also enabling more features across all chips.
> diff --git a/drivers/media/i2c/maxim-serdes/Kconfig b/drivers/media/i2c/maxim-serdes/Kconfig
> index f5a4ca80a263b..ddbb5791e9349 100644
> --- a/drivers/media/i2c/maxim-serdes/Kconfig
> +++ b/drivers/media/i2c/maxim-serdes/Kconfig
> @@ -15,3 +15,21 @@ config VIDEO_MAXIM_SERDES
[ ... ]
> +config VIDEO_MAX96717
> + tristate "Maxim MAX96717 Serializer support"
> + depends on COMMON_CLK
> + depends on I2C
> + depends on PINCTRL
> + select VIDEO_MAXIM_SERDES
[Severity: Medium]
Does this symbol need to inherit the dependencies of VIDEO_MAXIM_SERDES? If
VIDEO_MAXIM_SERDES has dependencies that are not met, could selecting it here
cause unmet direct dependency warnings or silent build failures on platforms
lacking the required infrastructure?
> diff --git a/drivers/media/i2c/maxim-serdes/max96717.c b/drivers/media/i2c/maxim-serdes/max96717.c
> new file mode 100644
> index 0000000000000..afca6ba51851f
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max96717.c
[ ... ]
> +static int max96717_get_pin_config_reg(unsigned int offset, u32 param,
> + unsigned int *reg, unsigned int *mask,
> + unsigned int *val)
> +{
[ ... ]
> + switch (param) {
> + case PIN_CONFIG_SLEW_RATE:
> + if (offset < 3) {
> + *reg = MAX96717_PIO_SLEW_0;
> + if (offset == 0)
> + *mask = MAX96717_PIO_SLEW_0_PIO00_SLEW;
> + else if (offset == 1)
> + *mask = MAX96717_PIO_SLEW_0_PIO01_SLEW;
> + else
> + *mask = MAX96717_PIO_SLEW_0_PIO02_SLEW;
> + } else if (offset < 5) {
> + *reg = MAX96717_PIO_SLEW_1;
> + if (offset == 3)
> + *mask = MAX96717_PIO_SLEW_1_PIO05_SLEW;
> + else
> + *mask = MAX96717_PIO_SLEW_1_PIO06_SLEW;
> + } else if (offset < 7) {
> + return -EINVAL;
[Severity: High]
Since the offset parameter represents the absolute hardware GPIO pin number
(0 to 10), does this logic incorrectly treat it as a contiguous index?
For example, if offset is 3 (GPIO 3), will it write to PIO05_SLEW instead of
returning -EINVAL?
[ ... ]
> +static int max96717_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + unsigned long config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
> + struct max96717_priv *priv = gpiochip_get_data(gc);
> + int ret;
> +
> + ret = max96717_conf_pin_config_get(priv->pctldev, offset, &config);
> + if (ret)
> + return ret;
> +
> + return pinconf_to_config_argument(config) ? GPIO_LINE_DIRECTION_OUT
> + : GPIO_LINE_DIRECTION_IN;
> +}
[Severity: High]
Because max96717_conf_pin_config_get() correctly returns -EINVAL when output
enable is false, does returning the error here break direction querying for
all input pins instead of interpreting it as GPIO_LINE_DIRECTION_IN?
[ ... ]
> +static int max96717_clk_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + struct max96717_priv *priv = clk_hw_to_priv(hw);
> + struct device *dev = &priv->client->dev;
> + unsigned int idx;
> +
> + idx = max96717_clk_find_best_index(priv, req->rate);
> +
> + if (req->rate != max96717_predef_freqs[idx].freq) {
> + dev_warn(dev, "Request CLK freq:%lu, found CLK freq:%lu\n",
> + req->rate, max96717_predef_freqs[idx].freq);
> + }
[Severity: Medium]
Since the clock framework routinely queries determine_rate to find the
closest available frequency, will this dev_warn() flood the kernel log during
normal clock rate negotiations?
[ ... ]
> +static int max96717_register_clkout(struct max96717_priv *priv)
> +{
> + struct device *dev = &priv->client->dev;
> + struct clk_init_data init = { .ops = &max96717_clk_ops };
> + int ret;
> +
> + ret = max96717_mux_set_rclkout(priv, MAX96717_RCLK_MFP);
> + if (ret)
> + return ret;
[Severity: High]
Does hardcoding MAX96717_RCLK_MFP (pin 4) here bypass the pinctrl framework
and override device tree configurations if a board uses the alternate
supported pin (MFP 2) for rclkout?
[ ... ]
> +static int max96717_probe(struct i2c_client *client)
> +{
[ ... ]
> + ret = max96717_wait_for_device(priv);
> + if (ret)
> + return ret;
> +
> + ret = max96717_gpiochip_probe(priv);
> + if (ret)
> + return ret;
> +
> + ret = max96717_register_clkout(priv);
> + if (ret)
> + return ret;
> +
> + return max_ser_probe(client, &priv->ser);
> +}
[Severity: High]
Can exposing the GPIOs and clock to userspace and other subsystems before
max_ser_probe() completes lead to erratic hardware behavior?
Mandatory hardware initialization (like setting CMU2 PFDDIV to 1.1V for
correct functionality) happens later inside max_ser_init() which is triggered
by max_ser_probe().
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=16
^ permalink raw reply
* Re: [PATCH v2 1/5] dt-bindings: arm: qcom: Document Shikra and its EVK boards
From: Komal Bajaj @ 2026-05-21 13:33 UTC (permalink / raw)
To: sashiko-reviews
Cc: robh, conor+dt, olteanv, devicetree, linux-phy, neil.armstrong,
vkoul
In-Reply-To: <20260519113234.BA75FC2BCB3@smtp.kernel.org>
On 5/19/2026 5:02 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Device tree compatible strings use generic abbreviations instead of specific hardware part numbers.
> - [Low] Alphabetical sorting violation in qcom.yaml.
> --
>
> commit d5de85f1dcdd73430dd4bed54ebebe0837ba7f5e
> Author: Komal Bajaj <komal.bajaj@oss.qualcomm.com>
>
> dt-bindings: arm: qcom: Document Shikra and its EVK boards
>
> This commit introduces compatible strings for the Qualcomm Shikra SoC in
> System-on-Module (SoM) form factors and their corresponding EVK carrier
> boards.
>
>> Add compatible strings for the CQ2390M, CQ2390S, IQ2390S SoM variant and its
>> corresponding EVK boards.
> Should these compatible strings use the specific hardware part numbers
> (CQ2390M, CQ2390S, IQ2390S) instead of generic abbreviations like
> qcom,shikra-cqm-som?
>
> If the generic abbreviations are kept, can the schema map these
> abbreviations to the specific part numbers to document the ABI?
SoC number 2390 corresponds to Shikra, so using shikra-cqm-som style
compatibles looks appropriate. Same applies to shikra-cqs-som and
shikra-iqs-som.
>
>> diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
>> index af266d584fae6..7c5c5f4fc30f7 100644
>> --- a/Documentation/devicetree/bindings/arm/qcom.yaml
>> +++ b/Documentation/devicetree/bindings/arm/qcom.yaml
>> @@ -968,6 +968,24 @@ properties:
>> - const: qcom,qcs9100
>> - const: qcom,sa8775p
>>
>> + - items:
>> + - enum:
>> + - qcom,shikra-cqm-evk
>> + - const: qcom,shikra-cqm-som
>> + - const: qcom,shikra
> This isn't a bug, but should these new blocks be sorted alphabetically
> based on the root SoC compatible string?
>
> Since 'h' comes after 'd', could qcom,shikra be placed after qcom,sdm845
> instead of after qcom,sa8775p?
Yeah, I missed it, will correct in next series.
Thanks
Komal
>
^ permalink raw reply
* Re: [PATCH v12 18/22] media: i2c: maxim-serdes: add MAX9296A driver
From: sashiko-bot @ 2026-05-21 13:34 UTC (permalink / raw)
To: Dumitru Ceclan via B4 Relay; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260521-gmsl2-3_serdes-v12-18-b26d92931196@analog.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] max96792a_ops is missing the .set_pipe_link callback, preventing proper link routing.
- [High] max9296a_set_pipe_stream_id ignores the new stream_id argument and programs the old state.
- [High] Unvalidated data lane values of 0 can trigger an integer underflow and undefined bitwise shift behavior.
- [Medium] The device remains powered on (resource state leak) if probe fails after toggling the powerdown GPIO.
- [Low] The Kconfig symbol VIDEO_MAX9296A selects VIDEO_MAXIM_SERDES but does not inherit its dependency on VIDEO_DEV.
--
commit dd3188793bbc20f9b99b6950a6fcec1fa7e23c5d
Author: Cosmin Tanislav <demonsingur@gmail.com>
media: i2c: maxim-serdes: add MAX9296A driver
Add a new MAX9296A driver that also supports MAX96714, MAX96714F,
MAX96714R, MAX96716A and MAX96792A.
Integrate it with the common Deserializer framework.
> diff --git a/drivers/media/i2c/maxim-serdes/Kconfig b/drivers/media/i2c/maxim-serdes/Kconfig
> --- a/drivers/media/i2c/maxim-serdes/Kconfig
> +++ b/drivers/media/i2c/maxim-serdes/Kconfig
[ ... ]
> @@ -45,3 +45,16 @@ config VIDEO_MAX96724
>
> To compile this driver as a module, choose M here: the module
> will be called max96724.
> +
> +config VIDEO_MAX9296A
> + tristate "Maxim MAX9296A Dual Deserializer support"
> + depends on I2C
> + select VIDEO_MAXIM_SERDES
[Severity: Low]
Is there a missing dependency here? VIDEO_MAXIM_SERDES depends on VIDEO_DEV,
but this config only depends on I2C. Selecting a symbol without meeting its
dependencies can lead to unmet direct dependency warnings and silent build
failures.
> diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c b/drivers/media/i2c/maxim-serdes/max9296a.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max9296a.c
[ ... ]
> +static int max9296a_init_phy(struct max_des *des, struct max_des_phy *phy)
> +{
[ ... ]
> + for (i = 0; i < num_hw_data_lanes ; i++) {
> + unsigned int map;
> +
> + if (i < num_data_lanes)
> + map = phy->mipi.data_lanes[i] - 1;
> + else
> + map = ffz(used_data_lanes);
> +
> + val |= map << (i * 2);
> + used_data_lanes |= BIT(map);
[Severity: High]
What happens if a malformed Device Tree provides a lane value of 0?
It appears the core framework only validates if the value exceeds
num_data_lanes. If the data lane is 0, the subtraction will underflow to
UINT_MAX, which could trigger undefined behavior when shifting bits in the
BIT(map) macro.
[ ... ]
> +static int max9296a_set_pipe_stream_id(struct max_des *des, struct max_des_pipe *pipe,
> + unsigned int stream_id)
> +{
> + struct max9296a_priv *priv = des_to_priv(des);
> + unsigned int index = max9296a_pipe_id(priv, pipe);
> +
> + return regmap_update_bits(priv->regmap, MAX9296A_RX50(index), MAX9296A_RX50_STR_SEL,
> + FIELD_PREP(MAX9296A_RX50_STR_SEL, pipe->stream_id));
[Severity: High]
Should this use the passed stream_id argument instead of pipe->stream_id?
If the framework hasn't updated the pipe state yet, this might program
the hardware with the old stream ID, potentially breaking video stream
routing.
[ ... ]
> +static int max9296a_probe(struct i2c_client *client)
> +{
[ ... ]
> + if (priv->gpiod_pwdn) {
> + /* PWDN must be held for 1us for reset */
> + udelay(1);
> +
> + gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> + /* Maximum power-up time (tLOCK) 4ms */
> + usleep_range(4000, 5000);
> + }
> +
> + priv->des.info = priv->info->info;
> + priv->des.ops = priv->info->ops;
> +
> + ret = max9296a_reset(priv);
> + if (ret)
> + return ret;
[Severity: Medium]
If max9296a_reset() or max_des_probe() fails below, does the device remain
powered on?
The powerdown GPIO is driven to 0 here to wake up the device, but it
doesn't appear to be reverted to 1 on the error paths, potentially leaving
the hardware drawing power.
> +
> + return max_des_probe(client, &priv->des);
> +}
[ ... ]
> +static const struct max_des_ops max96792a_ops = {
> + MAX9296A_COMMON_OPS,
> + .set_pipe_stream_id = max96714_set_pipe_stream_id,
> + .set_pipe_enable = max96714_set_pipe_enable,
> + .set_pipe_tunnel_phy = max96716a_set_pipe_tunnel_phy,
> + .set_pipe_tunnel_enable = max96714_set_pipe_tunnel_enable,
> +};
[Severity: High]
Should this struct include the .set_pipe_link callback?
The MAX96792A chip supports 2 pipes and 2 links, and it looks like the closely
related max96716a_ops uses max96716a_set_pipe_link for proper link routing.
Without it, could the core framework skip configuring the pipe-to-link mapping,
leaving the hardware with an incomplete configuration?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-gmsl2-3_serdes-v12-0-b26d92931196@analog.com?part=18
^ 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