Re: [PATCH v1 1/2] driver core: fw_devlink: Add support for FWNODE_FLAG_BROKEN_PARENT

[Saravana Kannan <saravanak@google.com>]

On Sat, Aug 28, 2021 at 10:01 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Aug 27, 2021 at 02:33:02PM -0700, Saravana Kannan wrote:
> > On Fri, Aug 27, 2021 at 1:11 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > > I've not yet looked at plain Ethernet drivers. This pattern could also
> > > > > exist there. And i wonder about other complex structures, i2c bus
> > > > > multiplexors, you can have interrupt controllers as i2c devices,
> > > > > etc. So the general case could exist in other places.
> > > >
> > > > I haven't seen any generic issues like this reported so far. It's only
> > > > after adding phy-handle that we are hitting these issues with DSA
> > > > switches.
> > >
> > > Can you run your parser over the 2250 DTB blobs and see how many
> > > children have dependencies on a parent? That could give us an idea how
> > > many moles need whacking. And maybe, where in the tree they are
> > > hiding?
> >
> > You are only responding to part of my email. As I said in my previous
> > email: "There are plenty of cases where it's better to delay the child
> > device's probe until the parent finishes. You even gave an example[7]
> > where it would help avoid unnecessary deferred probes." Can you please
> > give your thoughts on the rest of the points I made too?
>
> I must admit, my main problem at the moment is -rc1 in two weeks
> time. It seems like a number of board with Ethernet switches will be
> broken, that worked before. phy-handle is not limited to switch
> drivers, it is also used for Ethernet drivers. So it could be, a
> number of Ethernet drivers are also going to be broken in -rc1?

Again, in those cases, based on your FEC example, fw_devlink=on
actually improves things.

> But the issues sounds not to be specific to phy-handle, but any
> phandle that points back to a parent.

I feel like I'm going in circles here. This statement is not true.
Please read my previous explanations.

> So it could be drivers outside
> of networking are also going to be broken with -rc1?
> You have been very focused on one or two drivers. I would much rather
> see you getting an idea of how wide spread this problem is, and what
> should we do for -rc1?

Again, it's not a widespread problem as I explained before.
fw_devlink=on has been the default for 2 kernel versions now. With no
unfixed reported issues.

> Even if modifying DSA drivers to component drivers is possible, while
> not breaking backwards compatibility with DT,

It should be possible without needing any DT changes.

> it is not going to
> happen over night. That is something for the next merge window, not
> this merge window.

Right, I wasn't suggesting the component driver option be implemented
right away. We were talking about what the longer term proper fix
would be for DSA (and Ethernet if we actually find issues there) and
who would do it. That's what I hope this discussion could be.

Also, if we replace Patch 2/2 in this series with the patch below, it
will work as a generic quick fix for DSA that we could use for -rc1.
And if we still have issues reported on the phy-handle patch by -rc5
or so, we could revert the phy-handle patch then so that v5.15 isn't
broken.

-Saravana

--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1286,6 +1286,17 @@ static int dsa_switch_parse_of(struct
dsa_switch *ds, struct device_node *dn)
 {
        int err;

+       /* A lot of switch devices have their PHYs as child devices and have
+        * the PHYs depend on the switch as a supplier (Eg: interrupt
+        * controller). With fw_devlink=on, that means the PHYs will defer
+        * probe until the probe() of the switch completes. However, the way
+        * the DSA framework is designed, the PHYs are expected to be probed
+        * successfully before the probe() of the switch completes.
+        *
+        * So, mark the switch devices as a "broken parent" so that fw_devlink
+        * knows not to create device links between PHYs and the parent switch.
+        */
+       np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT;
        err = dsa_switch_parse_member_of(ds, dn);
        if (err)
                return err;