Re: [PATCH v9 2/3] of: factor arguments passed to of_map_id() into a struct

[Marc Zyngier <maz@xxxxxxxxxx>]

On Sun, 01 Mar 2026 10:46:57 +0000,
Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx> wrote:
> 
> On Sun, Mar 01, 2026 at 10:02:49AM +0000, Marc Zyngier wrote:
> > On Sun, 01 Mar 2026 08:34:20 +0000,
> > Vijayanand Jitta <vijayanand.jitta@xxxxxxxxxxxxxxxx> wrote:
> > > 
> > > From: Charan Teja Kalla <charan.kalla@xxxxxxxxxxxxxxxx>
> > > 
> > > Change of_map_id() to take a pointer to struct of_phandle_args
> > > instead of passing target device node and translated IDs separately.
> > > Update all callers accordingly.
> > > 
> > > Subsequent patch will make use of the args_count field in
> > > struct of_phandle_args.
> > > 
> > > Suggested-by: Rob Herring (Arm) <robh@xxxxxxxxxx>
> > > Signed-off-by: Charan Teja Kalla <charan.kalla@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Vijayanand Jitta <vijayanand.jitta@xxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/iommu/of_iommu.c              |  2 +-
> > >  drivers/of/base.c                     | 37 
> > > +++++++++++++++++------------------
> > >  drivers/pci/controller/dwc/pci-imx6.c |  8 +++++++-
> > >  drivers/pci/controller/pcie-apple.c   |  4 +++-
> > >  drivers/xen/grant-dma-ops.c           |  2 +-
> > >  include/linux/of.h                    | 21 +++++++++++++-------
> > >  6 files changed, 44 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > > index a511ecf21fcd..d255d0f58e8c 100644
> > > --- a/drivers/iommu/of_iommu.c
> > > +++ b/drivers/iommu/of_iommu.c
> > > @@ -48,7 +48,7 @@ static int of_iommu_configure_dev_id(struct device_node 
> > > *master_np,
> > >   struct of_phandle_args iommu_spec = { .args_count = 1 };
> > >   int err;
> > >  
> > > - err = of_map_iommu_id(master_np, *id, &iommu_spec.np, iommu_spec.args);
> > > + err = of_map_iommu_id(master_np, *id, &iommu_spec);
> > >   if (err)
> > >           return err;
> > >  
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index 57420806c1a2..6c3628255908 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -2102,8 +2102,11 @@ int of_find_last_cache_level(unsigned int cpu)
> > >   * @id: device ID to map.
> > >   * @map_name: property name of the map to use.
> > >   * @map_mask_name: optional property name of the mask to use.
> > > - * @target: optional pointer to a target device node.
> > > - * @id_out: optional pointer to receive the translated ID.
> > > + * @arg: of_phandle_args structure,
> > > + *       which includes:
> > > + *       np: pointer to the target device node
> > > + *       args_count: number of arguments
> > 
> > Number of arguments *to what*? Isn't that the size of args[] instead?
> 
> It is a number of values corresponding to the phandle in the DT
> property.

No. It is what the *caller* expects. Not what is is in the DT, which
could be (and generally is) a pile of random crap. If the two don't
match, return an error. But don't randomly overwrite data that is not
yours.

[...]

> It might be not obvious here for iommu-maps, but the struct is
> idiomatic in OF world. Let me quote a (trimmed) example from
> qcom/sm8650.dtsi (for a different property, but it explains the meaning
> of the values here):
> 
> gem_noc: interconnect@24100000 {
>       #interconnect-cells = <2>;
> };
> 
> epss_l3: interconnect@17d90000 {
>       #interconnect-cells = <1>;
> };
> 
> interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
>                &gem_noc SLAVE_LLCC QCOM_ICC_TAG_ACTIVE_ONLY>,
>               <&epss_l3 MASTER_EPSS_L3_APPS
>                &epss_l3 SLAVE_EPSS_L3_SHARED>;
> /* I skipped the second pair, it adds nothing here */
> 
> Here the parsing function for this property (of_icc_get_by_index()) will
> call of_parse_phandle_with_args() 4 times and it expects to return the
> following values in the of_phandle_args:
> 
> 1. { .np = gem_noc, .args_count = 2, .args = [MASTER_APPSS_PROC,
>                                               QCOM_ICC_TAG_ACTIVE_ONLY] }
> 2. { .np = gem_noc, .args_count = 2, .args = [SLAVE_LLCC,
>                                               QCOM_ICC_TAG_ACTIVE_ONLY] }
> 3. { .np = epss_l3, .args_count = 1, .args = [MASTER_EPSS_L3_APPS] }
> 4. { .np = epss_l3, .args_count = 1, .args = [SLAVE_EPSS_L3_SHARED] }
> 
> The whole of_phandle_args is then typically passed to the corresponding
> xlate function, specific to the paricular .np ('provider'), which will
> use #args_count values from the #args array to return the object from
> the provider.
> 
> Now let's see iommu-maps (again, qcom/sm8650.dtsi):
> 
> apps_smmu: iommu@15000000 {
>       #iommu-cells = <2>;
> };
> 
> iommu-map = <0     &apps_smmu 0x1400 0x1>,
>           <0x100 &apps_smmu 0x1401 0x1>;
> 
> The property matches current definition at [1], however this spec
> doesn't match the DT practice. It forces that the property should use 1
> cell for identifying the "object" in the IOMMU provider, even if the
> provider expects to use 2 cells (two args).
> 
> The correct property should look like:
> 
> iommu-map = <0     &apps_smmu 0x1400 0x0 0x1>,
>           <0x100 &apps_smmu 0x1401 0x0 0x1>;
> 
> [1] 
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-iommu.yaml
> 
> > 
> > > + *       args[]: array to receive the translated ID(s).
> > >   *
> > >   * Given a device ID, look up the appropriate implementation-defined
> > >   * platform ID and/or the target device which receives transactions on 
> > > that
> > > @@ -2117,21 +2120,21 @@ int of_find_last_cache_level(unsigned int cpu)
> > >   */
> > >  int of_map_id(const struct device_node *np, u32 id,
> > >          const char *map_name, const char *map_mask_name,
> > > -        struct device_node **target, u32 *id_out)
> > > +        struct of_phandle_args *arg)
> > >  {
> > >   u32 map_mask, masked_id;
> > >   int map_len;
> > >   const __be32 *map = NULL;
> > >  
> > > - if (!np || !map_name || (!target && !id_out))
> > > + if (!np || !map_name || !arg)
> > >           return -EINVAL;
> > >  
> > >   map = of_get_property(np, map_name, &map_len);
> > >   if (!map) {
> > > -         if (target)
> > > +         if (arg->np)
> > >                   return -ENODEV;
> > >           /* Otherwise, no map implies no translation */
> > > -         *id_out = id;
> > > +         arg->args[0] = id;
> > 
> > What if args_count is 0? Given that you place no restriction on the
> > way this can be called, that'd be entirely legitimate, and you'd
> > corrupt something you're not supposed to touch.
> 
> args is an array (not a pointer) in of_phandle_args. As such we know
> that args[0] is legit.

Again, no. The caller is telling you what it expects. This is strictly
equivalent to:

        void func(void *blah[], int sz);

func() is not supposed to look beyond sz. As it stands, this change in
not acceptable.

        M.

-- 
Without deviation from the norm, progress is not possible.