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.
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.
Overly optimistic:
Acked-by: Saravana Kannan <saravanak@xxxxxxxxxx>
-Saravana
>
> Thanks,
> Andrew
> ---
> drivers/of/property.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 11b922fde7af..4a2fca75e1c6 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1220,6 +1220,7 @@ DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
> DEFINE_SIMPLE_PROP(extcon, "extcon", NULL)
> DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", "#nvmem-cell-cells")
> DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
> +DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
> DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
> DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
> DEFINE_SIMPLE_PROP(pinctrl1, "pinctrl-1", NULL)
> @@ -1366,6 +1367,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> { .parse_prop = parse_extcon, },
> { .parse_prop = parse_nvmem_cells, },
> { .parse_prop = parse_phys, },
> + { .parse_prop = parse_phy_handle, },
> { .parse_prop = parse_wakeup_parent, },
> { .parse_prop = parse_pinctrl0, },
> { .parse_prop = parse_pinctrl1, },
>
> ---
> base-commit: cea5425829f77e476b03702426f6b3701299b925
> change-id: 20240829-phy-handle-fw-devlink-b829622200ae
>
> Best regards,
> --
> Andrew Halaney <ahalaney@xxxxxxxxxx>
>
