On Mon, Sep 30, 2024 at 05:12:42PM GMT, Saravana Kannan wrote:
> On Mon, Sep 30, 2024 at 2:28 PM Andrew Halaney <ahalaney@xxxxxxxxxx> wrote:
> >
> > Add support for parsing the phy-handle binding so that fw_devlink can
> > enforce the dependency. This prevents MACs (that use this binding to
> > claim they're using the corresponding phy) from probing prior to the
> > phy, unless the phy is a child of the MAC (which results in a
> > dependency cycle) or similar.
> >
> > For some motivation, imagine a device topology like so:
> >
> > ðernet0 {
> > phy-mode = "sgmii";
> > phy-handle = <&sgmii_phy0>;
> >
> > mdio {
> > compatible = "snps,dwmac-mdio";
> > sgmii_phy0: phy@8 {
> > compatible = "ethernet-phy-id0141.0dd4";
> > reg = <0x8>;
> > device_type = "ethernet-phy";
> > };
> >
> > sgmii_phy1: phy@a {
> > compatible = "ethernet-phy-id0141.0dd4";
> > reg = <0xa>;
> > device_type = "ethernet-phy";
> > };
> > };
> > };
> >
> > ðernet1 {
> > phy-mode = "sgmii";
> > phy-handle = <&sgmii_phy1>;
> > };
> >
> > Here ethernet1 depends on sgmii_phy1 to function properly. In the below
> > link an issue is reported where ethernet1 is probed and used prior to
> > sgmii_phy1, resulting in a failure to get things working for ethernet1.
> > With this change in place ethernet1 doesn't probe until sgmii_phy1 is
> > ready, resulting in ethernet1 functioning properly.
> >
> > ethernet0 consumes sgmii_phy0, but this dependency isn't enforced
> > via the device_links backing fw_devlink since ethernet0 is the parent of
> > sgmii_phy0. Here's a log showing that in action:
> >
> > [ 7.000432] qcom-ethqos 23040000.ethernet: Fixed dependency cycle(s) with /soc@0/ethernet@23040000/mdio/phy@8
> >
> > With this change in place ethernet1's dependency is properly described,
> > and it doesn't probe prior to sgmii_phy1 being available.
> >
> > Link: https://lore.kernel.org/netdev/7723d4l2kqgrez3yfauvp2ueu6awbizkrq4otqpsqpytzp45q2@rju2nxmqu4ew/
> > Suggested-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > Signed-off-by: Andrew Halaney <ahalaney@xxxxxxxxxx>
> > ---
> > I've marked this as an RFT because when looking through old mailing
> > list discusssions and kernel tech talks on this subject, I was unable
> > to really understand why in the past phy-handle had been left out. There
> > were some loose references to circular dependencies (which seem more or
> > less handled by fw_devlink to me), and the fact that a lot of behavior
> > happens in ndo_open() (but I couldn't quite grok the concern there).
> >
> > I'd appreciate more testing by others and some feedback from those who
> > know this a bit better to indicate whether fw_devlink is ready to handle
> > this or not.
> >
> > At least in my narrow point of view, it's working well for me.
>
> I do want this to land and I'm fairly certain it'll break something.
> But it's been so long that I don't remember what it was. I think it
> has to do with the generic phy driver not working well with fw_devlink
> because it doesn't go through the device driver model.
Let me see if I can hack something up on this board (which has a decent
dependency tree for testing this stuff) to use the generic phy driver
instead of the marvell one that it needs and see how that goes. It won't
*actually* work from a phy perspective, but it will at least test out
the driver core bits here I think.
>
> But like you said, it's been a while and fw_devlink has improved since
> then (I think). So please go ahead and give this a shot. If you can
> help fix any issues this highlights, I'd really appreciate it and I'd
> be happy to guide you through what I think needs to happen. But I
> don't think I have the time to fix it myself.
Sure, I tend to agree. Let me check the generic phy driver path for any
issues and if that test seems to go okay I too am of the opinion that
without any solid reasoning against this we enable it and battle through
(revert and fix after the fact if necessary) any newly identified issues
that prevent phy-handle and fw_devlink have with each other.
>
> Overly optimistic:
> Acked-by: Saravana Kannan <saravanak@xxxxxxxxxx>
>
> -Saravana
>
