Re: [PATCH 2/2] of: property: fw_devlink: Fix links to supplier when created from phandles

[Saravana Kannan]

On Wed, Feb 21, 2024 at 12:51 AM Herve Codina <herve.codina@bootlincom> wrote:
>
> Hi Saravana,
>
> On Tue, 20 Feb 2024 18:40:40 -0800
> Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> > On Tue, Feb 20, 2024 at 3:10 AM Herve Codina <herve.codina@xxxxxxxxxxx> wrote:
> > >
> > > Since commit 1a50d9403fb9 ("treewide: Fix probing of devices in DT
> > > overlays"), when using device-tree overlays, the FWNODE_FLAG_NOT_DEVICE
> > > is set on each overlay nodes. This flag is cleared when a struct device
> > > is actually created for the DT node.
> > > Also, when a device is created, the device DT node is parsed for known
> > > phandle and devlinks consumer/supplier links are created between the
> > > device (consumer) and the devices referenced by phandles (suppliers).
> > > As these supplier device can have a struct device not already created,
> > > the FWNODE_FLAG_NOT_DEVICE can be set for suppliers and leads the
> > > devlink supplier point to the device's parent instead of the device
> > > itself.
> > >
> > > Avoid this situation clearing the supplier FWNODE_FLAG_NOT_DEVICE just
> > > before the devlink creation if a device is supposed to be created and
> > > handled later in the process.
> > >
> > > Fixes: 1a50d9403fb9 ("treewide: Fix probing of devices in DT overlays")
> > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>
> > > ---
> > >  drivers/of/property.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 641a40cf5cf3..ff5cac477dbe 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1097,6 +1097,7 @@ static void of_link_to_phandle(struct device_node *con_np,
> > >                               struct device_node *sup_np)
> > >  {
> > >         struct device_node *tmp_np = of_node_get(sup_np);
> > > +       struct fwnode_handle *sup_fwnode;
> > >
> > >         /* Check that sup_np and its ancestors are available. */
> > >         while (tmp_np) {
> > > @@ -1113,7 +1114,20 @@ static void of_link_to_phandle(struct device_node *con_np,
> > >                 tmp_np = of_get_next_parent(tmp_np);
> > >         }
> > >
> > > -       fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> > > +       /*
> > > +        * In case of overlays, the fwnode are added with FWNODE_FLAG_NOT_DEVICE
> > > +        * flag set. A node can have a phandle that references an other node
> > > +        * added by the overlay.
> > > +        * Clear the supplier's FWNODE_FLAG_NOT_DEVICE so that fw_devlink links
> > > +        * to this supplier instead of linking to its parent.
> > > +        */
> > > +       sup_fwnode = of_fwnode_handle(sup_np);
> > > +       if (sup_fwnode->flags & FWNODE_FLAG_NOT_DEVICE) {
> > > +               if (of_property_present(sup_np, "compatible") &&
> > > +                   of_device_is_available(sup_np))
> > > +                       sup_fwnode->flags &= ~FWNODE_FLAG_NOT_DEVICE;
> > > +       }
> > > +       fwnode_link_add(of_fwnode_handle(con_np), sup_fwnode);
> >
> > Nack.
> >
> > of_link_to_phandle() doesn't care about any of the fwnode flags. It
> > just creates links between the consumer and supplier nodes. Don't add
> > more intelligence into it please. Also, "compatible" doesn't really
> > guarantee device creation and you can have devices created out of
> > nodes with no compatible property. I finally managed to get away from
> > looking for the "compatible" property. So, let's not add back a
> > dependency on that property please.
> >
> > Can you please give a real example where you are hitting this? I have
> > some thoughts on solutions, but I want to understand the issue fully
> > before I make suggestions.
> >
>
> I detected the issue with this overlay:
> --- 8< ---
> &{/}
> {
>         reg_dock_sys_3v3: regulator-dock-sys-3v3 {
>                 compatible = "regulator-fixed";
>                 regulator-name = "DOCK_SYS_3V3";
>                 regulator-min-microvolt = <3300000>;
>                 regulator-max-microvolt = <3300000>;
>                 gpios = <&tca6424_dock_1 5 GPIO_ACTIVE_HIGH>; // DOCK_SYS3V3_EN
>                 enable-active-high;
>                 regulator-always-on;
>         };
> };
>
> &i2c5 {
>         tca6424_dock_1: gpio@22 {
>                 compatible = "ti,tca6424";
>                 reg = <0x22>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>                 interrupt-parent = <&gpio4>;
>                 interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>                 vcc-supply = <&reg_dock_ctrl_3v3>;
>         };
> };
> --- 8< ---
>
> The regulator uses a gpio.
> The supplier for the regulator was not the gpio chip (gpio@22) but the i2c bus.

Thanks for the example. Let me think about this a bit on how we could
fix this and get back to you.

Please do ping me if I don't get back in a week or two.

-Saravana

>
> I first tried to clear always the flag in of_link_to_phandle() without any check
> to a "compatible" string and in that case, I broke pinctrl.
>
> All devices were waiting for the pinctrl they used (child of pinctrl device
> node) even if the pinctrl driver was bound to the device.
>
> For pinctrl, the DT structure looks like the following:
> --- 8< ---
> {
>         ...
>         pinctrl@1234 {
>                 reg = <1234>;
>                 compatible = "vendor,chip";
>
>                 pinctrl_some_device: grp {
>                         fsl,pins = < ... >;
>                 };
>         };
>
>         some_device@4567 {
>                 compablile = "foo,bar";
>                 reg = <4567>;
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_some_device>;
>                 ...
>         };
> };
> --- 8< ---
>
> In that case the link related to pinctrl for some_device needs to be to the
> 'pinctrl_some_device' node parent (i.e. the pinctrl@1234 node).
>
>
> Best regards,
> Hervé