[PATCH] i2c: mux: Fix the crash when the i2c-arbitrator node is present

[Abbarapu, Venkatesh]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi @Simon Glass,
Do you have any feedback on this?

Thanks
Venkatesh
> -----Original Message-----
> From: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>
> Sent: Wednesday, May 28, 2025 3:47 PM
> To: hs at denx.de; u-boot at lists.denx.de; Simon Glass <sjg at chromium.org>
> Cc: Simek, Michal <michal.simek at amd.com>; trini at konsulko.com; git (AMD-Xilinx)
> <git at amd.com>
> Subject: RE: [PATCH] i2c: mux: Fix the crash when the i2c-arbitrator node is present
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Heiko,
>
>
> > -----Original Message-----
> > From: Heiko Schocher <hs at denx.de>
> > Sent: Wednesday, May 28, 2025 3:05 PM
> > To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>;
> > u-boot at lists.denx.de; Simon Glass <sjg at chromium.org>
> > Cc: Simek, Michal <michal.simek at amd.com>; trini at konsulko.com; git
> > (AMD-Xilinx) <git at amd.com>
> > Subject: Re: [PATCH] i2c: mux: Fix the crash when the i2c-arbitrator
> > node is present
> >
> > Hello Venkatesh,
> >
> > On 28.05.25 05:12, Venkatesh Yadav Abbarapu wrote:
> > > Observing the crash when we add the i2c-arbitrator node in the
> > > device
> >
> > Which crash?
>
> Observing crash like below
> U-Boot 2025.01 (May 22 2025 - 12:32:01 +0000)
> CPU:   ZynqMP
> Silicon: v3
> Chip:  xck24
> Model: ZynqMP SC K24 RevA
> Board: Xilinx ZynqMP
> DRAM:  2 GiB
> initcall failed at call 0000000008037364 (err=-22) ### ERROR ### Please RESET
> the board ###
>
>
> >
> > > tree as per the DT bindings. The issue is with the child node of
> > > i2c-arbitrator at 72 i.e., i2c at f1950000->i2c-arbitrator at 72->i2c-arb, as
> > > the arbitrator uses the uclass of mux(UCLASS_I2C_MUX) and the mux
> > > uclass driver checks for the "reg" property using the
> > > i2c_mux_child_post_bind() function, if it won't find the "reg"
> > > property it will return -EINVAL which is leading to the crash.
> > > So, add the logic to check whether the  child node has the "reg"
> > > property, if the "reg" property exists then read the "reg" and update the channel.
> > >
> > > https://www.kernel.org/doc/Documentation/devicetree/bindings/i2c/i2c
> > > -a
> > > rb.txt
> > >
> > > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu at amd.com>
> > > ---
> > >   drivers/i2c/muxes/i2c-mux-uclass.c | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/i2c/muxes/i2c-mux-uclass.c
> > > b/drivers/i2c/muxes/i2c-mux-uclass.c
> > > index d1999d21feb..b18999c1fe3 100644
> > > --- a/drivers/i2c/muxes/i2c-mux-uclass.c
> > > +++ b/drivers/i2c/muxes/i2c-mux-uclass.c
> > > @@ -40,11 +40,14 @@ static int i2c_mux_child_post_bind(struct udevice *dev)
> > >     struct i2c_mux_bus *plat = dev_get_parent_plat(dev);
> > >     int channel;
> > >
> > > -   channel = dev_read_u32_default(dev, "reg", -1);
> >
> > Shouldn;t this function return -1 (the default value), if property "reg" is not present?
> > (And also if dev_ofnode(dev) is not valid?)
> >
> > Seems we should look into this function and may fix the problem there?
> >
> > Aha, this comes from
> >
> > drivers/core/read.c
> > int dev_read_u32_default(const struct udevice *dev, const char *propname,
> >                           int def)
> > {
> >          return ofnode_read_u32_default(dev_ofnode(dev), propname,
> > def); }
> >
> > with drivers/core/ofnode.c
> > u32 ofnode_read_u32_default(ofnode node, const char *propname, u32 def) {
> >          assert(ofnode_valid(node));
> >          ofnode_read_u32_index(node, propname, 0, &def);
> >
> >          return def;
> > }
> >
> > added Simon.
> >
> > @Simon: may proper fix would be to check in ofnode_read_u32_default()
> >          if propname exists, and if not return default value?
> >          May also return with default value if node is not valid?
> >
> >          What do you think?
> >
> >          Thanks!
> >
> >
> > > -   if (channel < 0)
> > > -           return -EINVAL;
> > > -   plat->channel = channel;
> > > +   ofnode node = dev_ofnode(dev);
> > >
> > > +   if (ofnode_has_property(node, "reg")) {
> >
> > If we fix it in this code, you can here simply return immediately if
> > "reg" propertyis missing, so you can save the identation level, and patch looks
> cleaner.
> >
> > > +           channel = dev_read_u32_default(dev, "reg", -1);
> > > +           if (channel < 0)
> > > +                   return -EINVAL;
> > > +           plat->channel = channel;
> > > +   }
> > >     return 0;
> > >   }
> > >
> > >
> >
> > Thanks!
>
> Thanks
> Venkatesh
> >
> > bye,
> > Heiko
> > --
> > DENX Software Engineering GmbH,      Managing Director: Erika Unter
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de