Re: [PATCH] of: property: add i2c-parent to fw_devlink supplier bindings

On Fri, Aug 14, 2026 at 12:13:45PM -0700, Abdurrahman Hussain wrote:
> On Fri Aug 14, 2026 at 11:15 AM PDT, Rob Herring wrote:
> > On Fri, Aug 14, 2026 at 01:09:49AM +0000, [email protected] wrote:
> >> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >> - [High] fw_devlink incorrectly parses I2C slave addresses as phandles due to 0-cell supplier binding definition for `i2c-parent`.
> >> --
> >> 
> >> --- Patch [1]: [PATCH] of: property: add i2c-parent to fw_devlink supplier bindings ---
> >> commit 85f34a66eac871d9e36bccb6795b94b17ab065d4
> >> Author: Abdurrahman Hussain <[email protected]>
> >> 
> >> of: property: add i2c-parent to fw_devlink supplier bindings
> >> 
> >> This commit teaches fw_devlink about i2c-parent so the core creates the
> >> corresponding device links. This ensures consumers are unbound before the
> >> parent adapter's device, removing the need for -EPROBE_DEFER.
> >> 
> >> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> >> > --- a/drivers/of/property.c
> >> > +++ b/drivers/of/property.c
> >> > @@ -1409,6 +1409,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
> >> >  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
> >> >  DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
> >> >  DEFINE_SIMPLE_PROP(panel, "panel", NULL)
> >> > +DEFINE_SIMPLE_PROP(i2c_parent, "i2c-parent", NULL)
> >> 
> >> [Severity: High]
> >> Will this correctly handle device tree bindings where the i2c-parent
> >> property includes a slave address argument?
> >> 
> >> For example, platforms using devices like the Toshiba TC9563 PCIe switch
> >> might use a property such as: i2c-parent = <&qup_i2c 0x77>;
> >> 
> >> By passing NULL as the cells argument to DEFINE_SIMPLE_PROP(), the parser
> >> assumes a 0-cell list. When fw_devlink iterates over the property, it finds
> >> the first phandle correctly, but won't it erroneously interpret the slave
> >> address argument (0x77 / 119) as a second, independent 0-cell phandle?
> >> 
> >> If another unrelated device tree node happens to match this phandle ID,
> >> could this cause fw_devlink to link the consumer to the wrong device,
> >> potentially leading to spurious probe deferrals or circular dependencies?
> >
> > I think you need custom parsing for this one to read the first cell and 
> > ignore the 2nd cell if present.
> >
> > Rob
> 
> Agreed. One wrinkle with unconditionally ignoring everything after the
> first phandle: i2c-demux-pinctrl holds a list of several bare phandles
> (i2c-parent = <&iic2>, <&i2c2>, <&gpioi2c2>;), and in the flattened
> tree that's indistinguishable from tc9563's <phandle addr> form. So
> always stopping at entry 0 would silently drop the demux's remaining
> supplier links.
> 
> The only reliable disambiguator I found is the consumer's compatible —
> toshiba,tc9563 is the one binding with a trailing slave-address cell.
> I have this queued for v2:
> 
> static struct device_node *parse_i2c_parent(struct device_node *np,
>                                           const char *prop_name, int index)
> {
>       if (strcmp(prop_name, "i2c-parent"))
>               return NULL;
> 
>       /* toshiba,tc9563 is <phandle addr>; every other user is bare phandles */
>       if (of_device_is_compatible(np, "toshiba,tc9563")) {
>               if (index)
>                       return NULL;
>               return of_parse_phandle(np, prop_name, 0);
>       }
> 
>       return of_parse_phandle(np, prop_name, index);
> }
> 
> Is a compatible check in property.c acceptable here, or would you
> rather see something else (e.g. entry 0 only, accepting the lost demux
> links)? If this looks fine I'll send it as v2.

This is fine. Hopefully we can prevent more cases though.

Rob