[RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Sean Anderson]

For a long time, PCSs have been tightly coupled with their MACs. For
this reason, the MAC creates the "phy" or mdio device, and then passes
it to the PCS to initialize. This has a few disadvantages:

- Each MAC must re-implement the same steps to look up/create a PCS
- The PCS cannot use functions tied to device lifetime, such as devm_*.
- Generally, the PCS does not have easy access to its device tree node

I'm not sure if these are terribly large disadvantages. In fact, I'm not
sure if this series provides any benefit which could not be achieved
with judicious use of helper functions. In any case, here it is.

NB: Several (later) patches in this series should not be applied. See
the notes in each commit for details on when they can be applied.


Sean Anderson (9):
  dt-bindings: net: Add lynx PCS
  dt-bindings: net: Expand pcs-handle to an array
  net: pcs: Add helpers for registering and finding PCSs
  net: pcs: lynx: Convert to an mdio driver
  net: pcs: lynx: Use pcs_get_by_provider to get PCS
  net: pcs: lynx: Remove lynx_get_mdio_device and lynx_pcs_destroy
  arm64: dts: Add compatible strings for Lynx PCSs
  powerpc: dts: Add compatible strings for Lynx PCSs
  net: pcs: lynx: Remove remaining users of lynx_pcs_create

 .../bindings/net/ethernet-controller.yaml     |   7 +-
 .../devicetree/bindings/net/fsl,lynx-pcs.yaml |  47 ++++
 MAINTAINERS                                   |   1 +
 .../arm64/boot/dts/freescale/fsl-ls1088a.dtsi |  30 ++-
 .../arm64/boot/dts/freescale/fsl-ls208xa.dtsi |  48 ++--
 .../arm64/boot/dts/freescale/fsl-lx2160a.dtsi |  54 +++--
 .../dts/freescale/qoriq-fman3-0-10g-0.dtsi    |   3 +-
 .../dts/freescale/qoriq-fman3-0-10g-1.dtsi    |   3 +-
 .../dts/freescale/qoriq-fman3-0-1g-0.dtsi     |   3 +-
 .../dts/freescale/qoriq-fman3-0-1g-1.dtsi     |   3 +-
 .../dts/freescale/qoriq-fman3-0-1g-2.dtsi     |   3 +-
 .../dts/freescale/qoriq-fman3-0-1g-3.dtsi     |   3 +-
 .../dts/freescale/qoriq-fman3-0-1g-4.dtsi     |   3 +-
 .../dts/freescale/qoriq-fman3-0-1g-5.dtsi     |   3 +-
 .../fsl/qoriq-fman3-0-10g-0-best-effort.dtsi  |   3 +-
 .../boot/dts/fsl/qoriq-fman3-0-10g-0.dtsi     |   3 +-
 .../fsl/qoriq-fman3-0-10g-1-best-effort.dtsi  |   3 +-
 .../boot/dts/fsl/qoriq-fman3-0-10g-1.dtsi     |   3 +-
 .../boot/dts/fsl/qoriq-fman3-0-1g-0.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-0-1g-1.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-0-1g-2.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-0-1g-3.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-0-1g-4.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-0-1g-5.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-1-10g-0.dtsi     |   3 +-
 .../boot/dts/fsl/qoriq-fman3-1-10g-1.dtsi     |   3 +-
 .../boot/dts/fsl/qoriq-fman3-1-1g-0.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-1-1g-1.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-1-1g-2.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-1-1g-3.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-1-1g-4.dtsi      |   3 +-
 .../boot/dts/fsl/qoriq-fman3-1-1g-5.dtsi      |   3 +-
 drivers/net/dsa/ocelot/Kconfig                |   2 +
 drivers/net/dsa/ocelot/felix_vsc9959.c        |  26 +-
 drivers/net/dsa/ocelot/seville_vsc9953.c      |  26 +-
 drivers/net/ethernet/freescale/dpaa2/Kconfig  |   1 +
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  |  43 +---
 drivers/net/ethernet/freescale/enetc/Kconfig  |   1 +
 .../net/ethernet/freescale/enetc/enetc_pf.c   |  25 +-
 drivers/net/pcs/Kconfig                       |  23 +-
 drivers/net/pcs/Makefile                      |   2 +
 drivers/net/pcs/core.c                        | 226 ++++++++++++++++++
 drivers/net/pcs/pcs-lynx.c                    |  76 ++++--
 drivers/of/property.c                         |   2 +
 include/linux/pcs-lynx.h                      |   6 +-
 include/linux/pcs.h                           |  33 +++
 include/linux/phylink.h                       |   6 +
 47 files changed, 566 insertions(+), 197 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
 create mode 100644 drivers/net/pcs/core.c
 create mode 100644 include/linux/pcs.h

-- 
2.35.1.1320.gc452695387.dirty

[RFC PATCH net-next 1/9] dt-bindings: net: Add lynx PCS

[Sean Anderson]

This adds bindings for the PCS half of the Lynx 10g/28g SerDes drivers.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 .../devicetree/bindings/net/fsl,lynx-pcs.yaml | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml

diff --git a/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
new file mode 100644
index 000000000000..49dee66ab679
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/fsl,lynx-pcs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP Lynx 10g/28g PCS
+
+maintainers:
+  - Ioana Ciornei <ioana.ciornei@nxp.com>
+
+description: |
+  Lynx SerDes devices may contain several Ethernet protocol controllers. These
+  controllers convert between (X)GMII and a variety of high-speed interfaces
+  (SGMII, 10GBase-R, QSGMII, etc). Unlike the SerDes itself, the PCSs are
+  accessed over an internal MDIO bus.
+
+properties:
+  compatible:
+    const: fsl,lynx-pcs
+
+  reg:
+    maxItems: 1
+
+  phys:
+    description: A reference to the SerDes lane(s)
+    maxItems: 1
+
+  phy-names:
+    const: serdes
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio-bus {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      ethernet-pcs@1 {
+        compatible = "fsl,lynx-pcs";
+        reg = <0x1>;
+      };
+    };
-- 
2.35.1.1320.gc452695387.dirty

Re: [RFC PATCH net-next 1/9] dt-bindings: net: Add lynx PCS

[Krzysztof Kozlowski]

On 11/07/2022 18:05, Sean Anderson wrote:
> This adds bindings for the PCS half of the Lynx 10g/28g SerDes drivers.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  .../devicetree/bindings/net/fsl,lynx-pcs.yaml | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
> new file mode 100644
> index 000000000000..49dee66ab679
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml

Shouldn't this be under net/pcs?

Rest looks good to me:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [RFC PATCH net-next 1/9] dt-bindings: net: Add lynx PCS

[Sean Anderson]

Hi Krzysztof,

On 7/12/22 4:47 AM, Krzysztof Kozlowski wrote:
> On 11/07/2022 18:05, Sean Anderson wrote:
>> This adds bindings for the PCS half of the Lynx 10g/28g SerDes drivers.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>>  .../devicetree/bindings/net/fsl,lynx-pcs.yaml | 47 +++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
>> new file mode 100644
>> index 000000000000..49dee66ab679
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
> 
> Shouldn't this be under net/pcs?

There's no net/pcs, since this is the first of its kind. There's no net/phy
either, so I didn't bother creating one.

--Sean

Re: [RFC PATCH net-next 1/9] dt-bindings: net: Add lynx PCS

[Krzysztof Kozlowski]

On 12/07/2022 16:57, Sean Anderson wrote:
> Hi Krzysztof,
> 
> On 7/12/22 4:47 AM, Krzysztof Kozlowski wrote:
>> On 11/07/2022 18:05, Sean Anderson wrote:
>>> This adds bindings for the PCS half of the Lynx 10g/28g SerDes drivers.
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>>
>>>  .../devicetree/bindings/net/fsl,lynx-pcs.yaml | 47 +++++++++++++++++++
>>>  1 file changed, 47 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
>>> new file mode 100644
>>> index 000000000000..49dee66ab679
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
>>
>> Shouldn't this be under net/pcs?
> 
> There's no net/pcs, since this is the first of its kind. 

There is, coming via Renesas tree.

> There's no net/phy
> either, so I didn't bother creating one.


Best regards,
Krzysztof

Re: [RFC PATCH net-next 1/9] dt-bindings: net: Add lynx PCS

[Sean Anderson]

On 7/12/22 11:00 AM, Krzysztof Kozlowski wrote:
> On 12/07/2022 16:57, Sean Anderson wrote:
>> Hi Krzysztof,
>> 
>> On 7/12/22 4:47 AM, Krzysztof Kozlowski wrote:
>>> On 11/07/2022 18:05, Sean Anderson wrote:
>>>> This adds bindings for the PCS half of the Lynx 10g/28g SerDes drivers.
>>>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>> ---
>>>>
>>>>  .../devicetree/bindings/net/fsl,lynx-pcs.yaml | 47 +++++++++++++++++++
>>>>  1 file changed, 47 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
>>>> new file mode 100644
>>>> index 000000000000..49dee66ab679
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/fsl,lynx-pcs.yaml
>>>
>>> Shouldn't this be under net/pcs?
>> 
>> There's no net/pcs, since this is the first of its kind. 
> 
> There is, coming via Renesas tree.

Ah, I will move this then.

--Sean

[RFC PATCH net-next 2/9] dt-bindings: net: Expand pcs-handle to an array

[Sean Anderson]

This allows multiple phandles to be specified for pcs-handle, such as
when multiple PCSs are present for a single MAC. To differentiate
between them, also add a pcs-names property.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 .../devicetree/bindings/net/ethernet-controller.yaml       | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 4f15463611f8..c033e536f869 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -107,11 +107,16 @@ properties:
     $ref: "#/properties/phy-connection-type"
 
   pcs-handle:
-    $ref: /schemas/types.yaml#/definitions/phandle
+    $ref: /schemas/types.yaml#/definitions/phandle-array
     description:
       Specifies a reference to a node representing a PCS PHY device on a MDIO
       bus to link with an external PHY (phy-handle) if exists.
 
+  pcs-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description:
+      The name of each PCS in pcs-handle.
+
   phy-handle:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
-- 
2.35.1.1320.gc452695387.dirty

Re: [RFC PATCH net-next 2/9] dt-bindings: net: Expand pcs-handle to an array

[Krzysztof Kozlowski]

On 11/07/2022 18:05, Sean Anderson wrote:
> This allows multiple phandles to be specified for pcs-handle, such as
> when multiple PCSs are present for a single MAC. To differentiate
> between them, also add a pcs-names property.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  .../devicetree/bindings/net/ethernet-controller.yaml       | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 4f15463611f8..c033e536f869 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -107,11 +107,16 @@ properties:
>      $ref: "#/properties/phy-connection-type"
>  
>    pcs-handle:
> -    $ref: /schemas/types.yaml#/definitions/phandle
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>      description:
>        Specifies a reference to a node representing a PCS PHY device on a MDIO
>        bus to link with an external PHY (phy-handle) if exists.

You need to update all existing bindings and add maxItems:1.

>  
> +  pcs-names:

To be consistent with other properties this should be "pcs-handle-names"
and the other "pcs-handles"... and then actually drop the "handle".


> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description:
> +      The name of each PCS in pcs-handle.

You also need:
dependencies:
  pcs-names: [ pcs-handle ]

> +
>    phy-handle:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:


Best regards,
Krzysztof

Re: [RFC PATCH net-next 2/9] dt-bindings: net: Expand pcs-handle to an array

[Sean Anderson]

Hi Krzysztof,

On 7/12/22 4:51 AM, Krzysztof Kozlowski wrote:
> On 11/07/2022 18:05, Sean Anderson wrote:
>> This allows multiple phandles to be specified for pcs-handle, such as
>> when multiple PCSs are present for a single MAC. To differentiate
>> between them, also add a pcs-names property.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>>  .../devicetree/bindings/net/ethernet-controller.yaml       | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>> index 4f15463611f8..c033e536f869 100644
>> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>> @@ -107,11 +107,16 @@ properties:
>>      $ref: "#/properties/phy-connection-type"
>>  
>>    pcs-handle:
>> -    $ref: /schemas/types.yaml#/definitions/phandle
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>      description:
>>        Specifies a reference to a node representing a PCS PHY device on a MDIO
>>        bus to link with an external PHY (phy-handle) if exists.
> 
> You need to update all existing bindings and add maxItems:1.
> 
>>  
>> +  pcs-names:
> 
> To be consistent with other properties this should be "pcs-handle-names"
> and the other "pcs-handles"... and then actually drop the "handle".

Sorry, I'm not sure what you're recommending in the second half here.

>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +    description:
>> +      The name of each PCS in pcs-handle.
> 
> You also need:
> dependencies:
>   pcs-names: [ pcs-handle ]
> 

OK

--Sean

Re: [RFC PATCH net-next 2/9] dt-bindings: net: Expand pcs-handle to an array

[Krzysztof Kozlowski]

On 12/07/2022 17:06, Sean Anderson wrote:
> Hi Krzysztof,
> 
> On 7/12/22 4:51 AM, Krzysztof Kozlowski wrote:
>> On 11/07/2022 18:05, Sean Anderson wrote:
>>> This allows multiple phandles to be specified for pcs-handle, such as
>>> when multiple PCSs are present for a single MAC. To differentiate
>>> between them, also add a pcs-names property.
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>>
>>>  .../devicetree/bindings/net/ethernet-controller.yaml       | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>> index 4f15463611f8..c033e536f869 100644
>>> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>> @@ -107,11 +107,16 @@ properties:
>>>      $ref: "#/properties/phy-connection-type"
>>>  
>>>    pcs-handle:
>>> -    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>>      description:
>>>        Specifies a reference to a node representing a PCS PHY device on a MDIO
>>>        bus to link with an external PHY (phy-handle) if exists.
>>
>> You need to update all existing bindings and add maxItems:1.
>>
>>>  
>>> +  pcs-names:
>>
>> To be consistent with other properties this should be "pcs-handle-names"
>> and the other "pcs-handles"... and then actually drop the "handle".
> 
> Sorry, I'm not sure what you're recommending in the second half here.

I would be happy to see consistent naming with other xxxs/xxx-names
properties, therefore I recommend to:
1. deprecate pcs-handle because anyway the naming is encoding DT spec
into the name ("handle"),
2. add new property 'pcs' or 'pcss' (the 's' at the end like clocks but
maybe that's too much) with pcs-names.

However before implementing this, please wait for more feedback. Maybe
Rob or net folks will have different opinions.

Best regards,
Krzysztof

Re: [RFC PATCH net-next 2/9] dt-bindings: net: Expand pcs-handle to an array

[Sean Anderson]

On 7/12/22 11:18 AM, Krzysztof Kozlowski wrote:
> On 12/07/2022 17:06, Sean Anderson wrote:
>> Hi Krzysztof,
>> 
>> On 7/12/22 4:51 AM, Krzysztof Kozlowski wrote:
>>> On 11/07/2022 18:05, Sean Anderson wrote:
>>>> This allows multiple phandles to be specified for pcs-handle, such as
>>>> when multiple PCSs are present for a single MAC. To differentiate
>>>> between them, also add a pcs-names property.
>>>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>> ---
>>>>
>>>>  .../devicetree/bindings/net/ethernet-controller.yaml       | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>>> index 4f15463611f8..c033e536f869 100644
>>>> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>>> @@ -107,11 +107,16 @@ properties:
>>>>      $ref: "#/properties/phy-connection-type"
>>>>  
>>>>    pcs-handle:
>>>> -    $ref: /schemas/types.yaml#/definitions/phandle
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>      description:
>>>>        Specifies a reference to a node representing a PCS PHY device on a MDIO
>>>>        bus to link with an external PHY (phy-handle) if exists.
>>>
>>> You need to update all existing bindings and add maxItems:1.
>>>
>>>>  
>>>> +  pcs-names:
>>>
>>> To be consistent with other properties this should be "pcs-handle-names"
>>> and the other "pcs-handles"... and then actually drop the "handle".
>> 
>> Sorry, I'm not sure what you're recommending in the second half here.
> 
> I would be happy to see consistent naming with other xxxs/xxx-names
> properties, therefore I recommend to:
> 1. deprecate pcs-handle because anyway the naming is encoding DT spec
> into the name ("handle"),

I agree with you here.

> 2. add new property 'pcs' or 'pcss' (the 's' at the end like clocks but
> maybe that's too much) with pcs-names.
> 
> However before implementing this, please wait for more feedback. Maybe
> Rob or net folks will have different opinions.

For some context:

https://lore.kernel.org/netdev/20211004191527.1610759-2-sean.anderson@seco.com/
https://lore.kernel.org/netdev/20220321152515.287119-3-andy.chiu@sifive.com/

--Sean

Re: [RFC PATCH net-next 2/9] dt-bindings: net: Expand pcs-handle to an array

[Russell King (Oracle)]

On Tue, Jul 12, 2022 at 05:18:05PM +0200, Krzysztof Kozlowski wrote:
> On 12/07/2022 17:06, Sean Anderson wrote:
> > Hi Krzysztof,
> > 
> > On 7/12/22 4:51 AM, Krzysztof Kozlowski wrote:
> >> On 11/07/2022 18:05, Sean Anderson wrote:
> >>> This allows multiple phandles to be specified for pcs-handle, such as
> >>> when multiple PCSs are present for a single MAC. To differentiate
> >>> between them, also add a pcs-names property.
> >>>
> >>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >>> ---
> >>>
> >>>  .../devicetree/bindings/net/ethernet-controller.yaml       | 7 ++++++-
> >>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> >>> index 4f15463611f8..c033e536f869 100644
> >>> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> >>> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> >>> @@ -107,11 +107,16 @@ properties:
> >>>      $ref: "#/properties/phy-connection-type"
> >>>  
> >>>    pcs-handle:
> >>> -    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>      description:
> >>>        Specifies a reference to a node representing a PCS PHY device on a MDIO
> >>>        bus to link with an external PHY (phy-handle) if exists.
> >>
> >> You need to update all existing bindings and add maxItems:1.
> >>
> >>>  
> >>> +  pcs-names:
> >>
> >> To be consistent with other properties this should be "pcs-handle-names"
> >> and the other "pcs-handles"... and then actually drop the "handle".
> > 
> > Sorry, I'm not sure what you're recommending in the second half here.
> 
> I would be happy to see consistent naming with other xxxs/xxx-names
> properties, therefore I recommend to:
> 1. deprecate pcs-handle because anyway the naming is encoding DT spec
> into the name ("handle"),
> 2. add new property 'pcs' or 'pcss' (the 's' at the end like clocks but
> maybe that's too much) with pcs-names.
> 
> However before implementing this, please wait for more feedback. Maybe
> Rob or net folks will have different opinions.

We decided on "pcs-handle" for PCS for several drivers, to be consistent
with the situation for network PHYs (which are "phy-handle", settled on
after we also had "phy" and "phy-device" and decided to deprecate these
two.

Surely we should have consistency within the net code - so either "phy"
and "pcs" or "phy-handle" and "pcs-handle" but not a mixture of both?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 2/9] dt-bindings: net: Expand pcs-handle to an array

[Krzysztof Kozlowski]

On 12/07/2022 17:59, Russell King (Oracle) wrote:
>> However before implementing this, please wait for more feedback. Maybe
>> Rob or net folks will have different opinions.
> 
> We decided on "pcs-handle" for PCS for several drivers, to be consistent
> with the situation for network PHYs (which are "phy-handle", settled on
> after we also had "phy" and "phy-device" and decided to deprecate these
> two.
> 
> Surely we should have consistency within the net code - so either "phy"
> and "pcs" or "phy-handle" and "pcs-handle" but not a mixture of both?

True. Then the new property should be "pcs-handle-names"?

Best regards,
Krzysztof

Re: [RFC PATCH net-next 2/9] dt-bindings: net: Expand pcs-handle to an array

[Rob Herring]

On Thu, Jul 14, 2022 at 12:45:54PM +0200, Krzysztof Kozlowski wrote:
> On 12/07/2022 17:59, Russell King (Oracle) wrote:
> >> However before implementing this, please wait for more feedback. Maybe
> >> Rob or net folks will have different opinions.
> > 
> > We decided on "pcs-handle" for PCS for several drivers, to be consistent
> > with the situation for network PHYs (which are "phy-handle", settled on
> > after we also had "phy" and "phy-device" and decided to deprecate these
> > two.
> > 
> > Surely we should have consistency within the net code - so either "phy"
> > and "pcs" or "phy-handle" and "pcs-handle" but not a mixture of both?
> 
> True. Then the new property should be "pcs-handle-names"?

IMO, just keep "pcs-handle" and then "pcs-handle-names". We never seem 
to get free of the deprecated versions (-gpio).

While just add/remove 's' would be nice, we have to deal with things 
like 'mboxes' and I think some other inconsistencies. 

Rob

[RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs

[Sean Anderson]

This adds support for getting PCS devices from the device tree. PCS
drivers must first register with phylink_register_pcs. After that, MAC
drivers may look up their PCS using phylink_get_pcs.

To prevent the PCS driver from leaving suddenly, we use try_module_get. To
provide some ordering during probing/removal, we use device links managed
by of_fwnode_add_links. This will reduce the number of probe failures due
to deferral. It will not prevent this for non-standard properties (aka
pcsphy-handle), but the worst that happens is that we re-probe a few times.

At the moment there is no support for specifying the interface used to
talk to the PCS. The MAC driver is expected to know how to talk to the
PCS. This is not a change, but it is perhaps an area for improvement.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This is adapted from [1], primarily incorporating the changes discussed
there.

[1] https://lore.kernel.org/netdev/9f73bc4f-5f99-95f5-78fa-dac96f9e0146@seco.com/

 MAINTAINERS              |   1 +
 drivers/net/pcs/Kconfig  |  12 +++
 drivers/net/pcs/Makefile |   2 +
 drivers/net/pcs/core.c   | 226 +++++++++++++++++++++++++++++++++++++++
 drivers/of/property.c    |   2 +
 include/linux/pcs.h      |  33 ++++++
 include/linux/phylink.h  |   6 ++
 7 files changed, 282 insertions(+)
 create mode 100644 drivers/net/pcs/core.c
 create mode 100644 include/linux/pcs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ca95b1833b97..3965d49753d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7450,6 +7450,7 @@ F:	include/linux/*mdio*.h
 F:	include/linux/mdio/*.h
 F:	include/linux/mii.h
 F:	include/linux/of_net.h
+F:	include/linux/pcs.h
 F:	include/linux/phy.h
 F:	include/linux/phy_fixed.h
 F:	include/linux/platform_data/mdio-bcm-unimac.h
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 22ba7b0b476d..fed6264fdf33 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -5,6 +5,18 @@
 
 menu "PCS device drivers"
 
+config PCS
+	bool "PCS subsystem"
+	help
+	  This provides common helper functions for registering and looking up
+	  Physical Coding Sublayer (PCS) devices. PCS devices translate between
+	  different interface types. In some use cases, they may either
+	  translate between different types of Medium-Independent Interfaces
+	  (MIIs), such as translating GMII to SGMII. This allows using a fast
+	  serial interface to talk to the phy which translates the MII to the
+	  Medium-Dependent Interface. Alternatively, they may translate a MII
+	  directly to an MDI, such as translating GMII to 1000Base-X.
+
 config PCS_XPCS
 	tristate "Synopsys DesignWare XPCS controller"
 	depends on MDIO_DEVICE && MDIO_BUS
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 0603d469bd57..1fd21a1619d4 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux PCS drivers
 
+obj-$(CONFIG_PCS)		+= core.o
+
 pcs_xpcs-$(CONFIG_PCS_XPCS)	:= pcs-xpcs.o pcs-xpcs-nxp.o
 
 obj-$(CONFIG_PCS_XPCS)		+= pcs_xpcs.o
diff --git a/drivers/net/pcs/core.c b/drivers/net/pcs/core.c
new file mode 100644
index 000000000000..b39ff1ccdb34
--- /dev/null
+++ b/drivers/net/pcs/core.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
+ */
+
+#include <linux/fwnode.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/pcs.h>
+#include <linux/phylink.h>
+#include <linux/property.h>
+
+static LIST_HEAD(pcs_devices);
+static DEFINE_MUTEX(pcs_mutex);
+
+/**
+ * pcs_register() - register a new PCS
+ * @pcs: the PCS to register
+ *
+ * Registers a new PCS which can be automatically attached to a phylink.
+ *
+ * Return: 0 on success, or -errno on error
+ */
+int pcs_register(struct phylink_pcs *pcs)
+{
+	if (!pcs->dev || !pcs->ops)
+		return -EINVAL;
+	if (!pcs->ops->pcs_an_restart || !pcs->ops->pcs_config ||
+	    !pcs->ops->pcs_get_state)
+		return -EINVAL;
+
+	INIT_LIST_HEAD(&pcs->list);
+	mutex_lock(&pcs_mutex);
+	list_add(&pcs->list, &pcs_devices);
+	mutex_unlock(&pcs_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pcs_register);
+
+/**
+ * pcs_unregister() - unregister a PCS
+ * @pcs: a PCS previously registered with pcs_register()
+ */
+void pcs_unregister(struct phylink_pcs *pcs)
+{
+	mutex_lock(&pcs_mutex);
+	list_del(&pcs->list);
+	mutex_unlock(&pcs_mutex);
+}
+EXPORT_SYMBOL_GPL(pcs_unregister);
+
+static void devm_pcs_release(struct device *dev, void *res)
+{
+	pcs_unregister(*(struct phylink_pcs **)res);
+}
+
+/**
+ * devm_pcs_register - resource managed pcs_register()
+ * @dev: device that is registering this PCS
+ * @pcs: the PCS to register
+ *
+ * Managed pcs_register(). For PCSs registered by this function,
+ * pcs_unregister() is automatically called on driver detach. See
+ * pcs_register() for more information.
+ *
+ * Return: 0 on success, or -errno on failure
+ */
+int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs)
+{
+	struct phylink_pcs **pcsp;
+	int ret;
+
+	pcsp = devres_alloc(devm_pcs_release, sizeof(*pcsp),
+			    GFP_KERNEL);
+	if (!pcsp)
+		return -ENOMEM;
+
+	ret = pcs_register(pcs);
+	if (ret) {
+		devres_free(pcsp);
+		return ret;
+	}
+
+	*pcsp = pcs;
+	devres_add(dev, pcsp);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_pcs_register);
+
+/**
+ * pcs_find() - Find the PCS associated with a fwnode or device
+ * @fwnode: The PCS's fwnode
+ * @dev: The PCS's device
+ *
+ * Search PCSs registered with pcs_register() for one with a matching
+ * fwnode or device. Either @fwnode or @dev may be %NULL if matching against a
+ * fwnode or device is not desired (respectively).
+ *
+ * Return: a matching PCS, or %NULL if not found
+ */
+static struct phylink_pcs *pcs_find(const struct fwnode_handle *fwnode,
+				    const struct device *dev)
+{
+	struct phylink_pcs *pcs;
+
+	mutex_lock(&pcs_mutex);
+	list_for_each_entry(pcs, &pcs_devices, list) {
+		if (dev && pcs->dev == dev)
+			goto out;
+		if (fwnode && pcs->dev->fwnode == fwnode)
+			goto out;
+	}
+	pcs = NULL;
+
+out:
+	mutex_unlock(&pcs_mutex);
+	pr_devel("%s: looking for %pfwf or %s %s...%s found\n", __func__,
+		 fwnode, dev ? dev_driver_string(dev) : "(null)",
+		 dev ? dev_name(dev) : "(null)", pcs ? " not" : "");
+	return pcs;
+}
+
+/**
+ * pcs_get_tail() - Finish getting a PCS
+ * @pcs: The PCS to get, or %NULL if one could not be found
+ *
+ * This performs common operations necessary when getting a PCS (chiefly
+ * incrementing reference counts)
+ *
+ * Return: @pcs, or an error pointer on failure
+ */
+static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
+{
+	if (!pcs)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (!try_module_get(pcs->ops->owner))
+		return ERR_PTR(-ENODEV);
+	get_device(pcs->dev);
+
+	return pcs;
+}
+
+/**
+ * _pcs_get_by_fwnode() - Get a PCS from a fwnode property
+ * @fwnode: The fwnode to get an associated PCS of
+ * @id: The name of the PCS to get. May be %NULL to get the first PCS.
+ * @optional: Whether the PCS is optional or not
+ *
+ * Look up a PCS associated with @fwnode and return a reference to it. Every
+ * call to pcs_get_by_fwnode() must be balanced with one to pcs_put().
+ *
+ * If @optional is true, and @id is non-%NULL, then if @id cannot be found in
+ * pcs-names, %NULL is returned (instead of an error). If @optional is true and
+ * @id is %NULL, then no error is returned if pcs-handle is absent.
+ *
+ * Return: a PCS if found, or an error pointer on failure
+ */
+struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
+				       const char *id, bool optional)
+{
+	int index;
+	struct phylink_pcs *pcs;
+	struct fwnode_handle *pcs_fwnode;
+
+	if (id)
+		index = fwnode_property_match_string(fwnode, "pcs-names", id);
+	else
+		index = 0;
+	if (index < 0) {
+		if (optional && (index == -EINVAL || index == -ENODATA))
+			return NULL;
+		return ERR_PTR(index);
+	}
+
+	/* First try pcs-handle, and if that doesn't work fall back to the
+	 * (legacy) pcsphy-handle.
+	 */
+	pcs_fwnode = fwnode_find_reference(fwnode, "pcs-handle", index);
+	if (PTR_ERR(pcs_fwnode) == -ENOENT)
+		pcs_fwnode = fwnode_find_reference(fwnode, "pcsphy-handle",
+						   index);
+	if (optional && !id && PTR_ERR(pcs_fwnode) == -ENOENT)
+		return NULL;
+	else if (IS_ERR(pcs_fwnode))
+		return ERR_CAST(pcs_fwnode);
+
+	pcs = pcs_find(pcs_fwnode, NULL);
+	fwnode_handle_put(pcs_fwnode);
+	return pcs_get_tail(pcs);
+}
+EXPORT_SYMBOL_GPL(pcs_get_by_fwnode);
+
+/**
+ * pcs_get_by_provider() - Get a PCS from an existing provider
+ * @dev: The device providing the PCS
+ *
+ * This finds the first PCS registersed by @dev and returns a reference to it.
+ * Every call to pcs_get_by_provider() must be balanced with one to
+ * pcs_put().
+ *
+ * Return: a PCS if found, or an error pointer on failure
+ */
+struct phylink_pcs *pcs_get_by_provider(const struct device *dev)
+{
+	return pcs_get_tail(pcs_find(NULL, dev));
+}
+EXPORT_SYMBOL_GPL(pcs_get_by_provider);
+
+/**
+ * pcs_put() - Release a previously-acquired PCS
+ * @pcs: The PCS to put
+ *
+ * This frees resources associated with the PCS which were acquired when it was
+ * gotten.
+ */
+void pcs_put(struct phylink_pcs *pcs)
+{
+	if (!pcs)
+		return;
+
+	put_device(pcs->dev);
+	module_put(pcs->ops->owner);
+}
+EXPORT_SYMBOL_GPL(pcs_put);
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 967f79b59016..860d35bde5e9 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1318,6 +1318,7 @@ DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
 DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
 DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
 DEFINE_SIMPLE_PROP(remote_endpoint, "remote-endpoint", NULL)
+DEFINE_SIMPLE_PROP(pcs_handle, "pcs-handle", NULL)
 DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
@@ -1406,6 +1407,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_pinctrl7, },
 	{ .parse_prop = parse_pinctrl8, },
 	{ .parse_prop = parse_remote_endpoint, .node_not_dev = true, },
+	{ .parse_prop = parse_pcs_handle, },
 	{ .parse_prop = parse_pwms, },
 	{ .parse_prop = parse_resets, },
 	{ .parse_prop = parse_leds, },
diff --git a/include/linux/pcs.h b/include/linux/pcs.h
new file mode 100644
index 000000000000..00e76594e03c
--- /dev/null
+++ b/include/linux/pcs.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
+ */
+
+#ifndef _PCS_H
+#define _PCS_H
+
+struct phylink_pcs;
+struct fwnode;
+
+int pcs_register(struct phylink_pcs *pcs);
+void pcs_unregister(struct phylink_pcs *pcs);
+int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs);
+struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
+				       const char *id, bool optional);
+struct phylink_pcs *pcs_get_by_provider(const struct device *dev);
+void pcs_put(struct phylink_pcs *pcs);
+
+static inline struct phylink_pcs
+*pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
+		   const char *id)
+{
+	return _pcs_get_by_fwnode(fwnode, id, false);
+}
+
+static inline struct phylink_pcs
+*pcs_get_by_fwnode_optional(const struct fwnode_handle *fwnode, const char *id)
+{
+	return _pcs_get_by_fwnode(fwnode, id, true);
+}
+
+#endif /* PCS_H */
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6d06896fc20d..a713e70108a1 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -396,19 +396,24 @@ struct phylink_pcs_ops;
 
 /**
  * struct phylink_pcs - PHYLINK PCS instance
+ * @dev: the device associated with this PCS
  * @ops: a pointer to the &struct phylink_pcs_ops structure
+ * @list: internal list of PCS devices
  * @poll: poll the PCS for link changes
  *
  * This structure is designed to be embedded within the PCS private data,
  * and will be passed between phylink and the PCS.
  */
 struct phylink_pcs {
+	struct device *dev;
 	const struct phylink_pcs_ops *ops;
+	struct list_head list;
 	bool poll;
 };
 
 /**
  * struct phylink_pcs_ops - MAC PCS operations structure.
+ * @owner: the module which implements this PCS.
  * @pcs_validate: validate the link configuration.
  * @pcs_get_state: read the current MAC PCS link state from the hardware.
  * @pcs_config: configure the MAC PCS for the selected mode and state.
@@ -417,6 +422,7 @@ struct phylink_pcs {
  *               (where necessary).
  */
 struct phylink_pcs_ops {
+	struct module *owner;
 	int (*pcs_validate)(struct phylink_pcs *pcs, unsigned long *supported,
 			    const struct phylink_link_state *state);
 	void (*pcs_get_state)(struct phylink_pcs *pcs,
-- 
2.35.1.1320.gc452695387.dirty

Re: [RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs

[Saravana Kannan]

On Mon, Jul 11, 2022 at 9:05 AM Sean Anderson <sean.anderson@seco.com> wrote:
>
> This adds support for getting PCS devices from the device tree. PCS
> drivers must first register with phylink_register_pcs. After that, MAC
> drivers may look up their PCS using phylink_get_pcs.
>
> To prevent the PCS driver from leaving suddenly, we use try_module_get. To
> provide some ordering during probing/removal, we use device links managed
> by of_fwnode_add_links. This will reduce the number of probe failures due
> to deferral. It will not prevent this for non-standard properties (aka
> pcsphy-handle), but the worst that happens is that we re-probe a few times.
>
> At the moment there is no support for specifying the interface used to
> talk to the PCS. The MAC driver is expected to know how to talk to the
> PCS. This is not a change, but it is perhaps an area for improvement.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> This is adapted from [1], primarily incorporating the changes discussed
> there.
>
> [1] https://lore.kernel.org/netdev/9f73bc4f-5f99-95f5-78fa-dac96f9e0146@seco.com/
>
>  MAINTAINERS              |   1 +
>  drivers/net/pcs/Kconfig  |  12 +++
>  drivers/net/pcs/Makefile |   2 +
>  drivers/net/pcs/core.c   | 226 +++++++++++++++++++++++++++++++++++++++
>  drivers/of/property.c    |   2 +
>  include/linux/pcs.h      |  33 ++++++
>  include/linux/phylink.h  |   6 ++
>  7 files changed, 282 insertions(+)
>  create mode 100644 drivers/net/pcs/core.c
>  create mode 100644 include/linux/pcs.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ca95b1833b97..3965d49753d3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7450,6 +7450,7 @@ F:        include/linux/*mdio*.h
>  F:     include/linux/mdio/*.h
>  F:     include/linux/mii.h
>  F:     include/linux/of_net.h
> +F:     include/linux/pcs.h
>  F:     include/linux/phy.h
>  F:     include/linux/phy_fixed.h
>  F:     include/linux/platform_data/mdio-bcm-unimac.h
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index 22ba7b0b476d..fed6264fdf33 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -5,6 +5,18 @@
>
>  menu "PCS device drivers"
>
> +config PCS
> +       bool "PCS subsystem"
> +       help
> +         This provides common helper functions for registering and looking up
> +         Physical Coding Sublayer (PCS) devices. PCS devices translate between
> +         different interface types. In some use cases, they may either
> +         translate between different types of Medium-Independent Interfaces
> +         (MIIs), such as translating GMII to SGMII. This allows using a fast
> +         serial interface to talk to the phy which translates the MII to the
> +         Medium-Dependent Interface. Alternatively, they may translate a MII
> +         directly to an MDI, such as translating GMII to 1000Base-X.
> +
>  config PCS_XPCS
>         tristate "Synopsys DesignWare XPCS controller"
>         depends on MDIO_DEVICE && MDIO_BUS
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 0603d469bd57..1fd21a1619d4 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Makefile for Linux PCS drivers
>
> +obj-$(CONFIG_PCS)              += core.o
> +
>  pcs_xpcs-$(CONFIG_PCS_XPCS)    := pcs-xpcs.o pcs-xpcs-nxp.o
>
>  obj-$(CONFIG_PCS_XPCS)         += pcs_xpcs.o
> diff --git a/drivers/net/pcs/core.c b/drivers/net/pcs/core.c
> new file mode 100644
> index 000000000000..b39ff1ccdb34
> --- /dev/null
> +++ b/drivers/net/pcs/core.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
> + */
> +
> +#include <linux/fwnode.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/pcs.h>
> +#include <linux/phylink.h>
> +#include <linux/property.h>
> +
> +static LIST_HEAD(pcs_devices);
> +static DEFINE_MUTEX(pcs_mutex);
> +
> +/**
> + * pcs_register() - register a new PCS
> + * @pcs: the PCS to register
> + *
> + * Registers a new PCS which can be automatically attached to a phylink.
> + *
> + * Return: 0 on success, or -errno on error
> + */
> +int pcs_register(struct phylink_pcs *pcs)
> +{
> +       if (!pcs->dev || !pcs->ops)
> +               return -EINVAL;
> +       if (!pcs->ops->pcs_an_restart || !pcs->ops->pcs_config ||
> +           !pcs->ops->pcs_get_state)
> +               return -EINVAL;
> +
> +       INIT_LIST_HEAD(&pcs->list);
> +       mutex_lock(&pcs_mutex);
> +       list_add(&pcs->list, &pcs_devices);
> +       mutex_unlock(&pcs_mutex);
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(pcs_register);
> +
> +/**
> + * pcs_unregister() - unregister a PCS
> + * @pcs: a PCS previously registered with pcs_register()
> + */
> +void pcs_unregister(struct phylink_pcs *pcs)
> +{
> +       mutex_lock(&pcs_mutex);
> +       list_del(&pcs->list);
> +       mutex_unlock(&pcs_mutex);
> +}
> +EXPORT_SYMBOL_GPL(pcs_unregister);
> +
> +static void devm_pcs_release(struct device *dev, void *res)
> +{
> +       pcs_unregister(*(struct phylink_pcs **)res);
> +}
> +
> +/**
> + * devm_pcs_register - resource managed pcs_register()
> + * @dev: device that is registering this PCS
> + * @pcs: the PCS to register
> + *
> + * Managed pcs_register(). For PCSs registered by this function,
> + * pcs_unregister() is automatically called on driver detach. See
> + * pcs_register() for more information.
> + *
> + * Return: 0 on success, or -errno on failure
> + */
> +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs)
> +{
> +       struct phylink_pcs **pcsp;
> +       int ret;
> +
> +       pcsp = devres_alloc(devm_pcs_release, sizeof(*pcsp),
> +                           GFP_KERNEL);
> +       if (!pcsp)
> +               return -ENOMEM;
> +
> +       ret = pcs_register(pcs);
> +       if (ret) {
> +               devres_free(pcsp);
> +               return ret;
> +       }
> +
> +       *pcsp = pcs;
> +       devres_add(dev, pcsp);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_pcs_register);
> +
> +/**
> + * pcs_find() - Find the PCS associated with a fwnode or device
> + * @fwnode: The PCS's fwnode
> + * @dev: The PCS's device
> + *
> + * Search PCSs registered with pcs_register() for one with a matching
> + * fwnode or device. Either @fwnode or @dev may be %NULL if matching against a
> + * fwnode or device is not desired (respectively).
> + *
> + * Return: a matching PCS, or %NULL if not found
> + */
> +static struct phylink_pcs *pcs_find(const struct fwnode_handle *fwnode,
> +                                   const struct device *dev)
> +{
> +       struct phylink_pcs *pcs;
> +
> +       mutex_lock(&pcs_mutex);
> +       list_for_each_entry(pcs, &pcs_devices, list) {
> +               if (dev && pcs->dev == dev)
> +                       goto out;
> +               if (fwnode && pcs->dev->fwnode == fwnode)
> +                       goto out;
> +       }
> +       pcs = NULL;
> +
> +out:
> +       mutex_unlock(&pcs_mutex);
> +       pr_devel("%s: looking for %pfwf or %s %s...%s found\n", __func__,
> +                fwnode, dev ? dev_driver_string(dev) : "(null)",
> +                dev ? dev_name(dev) : "(null)", pcs ? " not" : "");
> +       return pcs;
> +}
> +
> +/**
> + * pcs_get_tail() - Finish getting a PCS
> + * @pcs: The PCS to get, or %NULL if one could not be found
> + *
> + * This performs common operations necessary when getting a PCS (chiefly
> + * incrementing reference counts)
> + *
> + * Return: @pcs, or an error pointer on failure
> + */
> +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
> +{
> +       if (!pcs)
> +               return ERR_PTR(-EPROBE_DEFER);
> +
> +       if (!try_module_get(pcs->ops->owner))
> +               return ERR_PTR(-ENODEV);
> +       get_device(pcs->dev);
> +
> +       return pcs;
> +}
> +
> +/**
> + * _pcs_get_by_fwnode() - Get a PCS from a fwnode property
> + * @fwnode: The fwnode to get an associated PCS of
> + * @id: The name of the PCS to get. May be %NULL to get the first PCS.
> + * @optional: Whether the PCS is optional or not
> + *
> + * Look up a PCS associated with @fwnode and return a reference to it. Every
> + * call to pcs_get_by_fwnode() must be balanced with one to pcs_put().
> + *
> + * If @optional is true, and @id is non-%NULL, then if @id cannot be found in
> + * pcs-names, %NULL is returned (instead of an error). If @optional is true and
> + * @id is %NULL, then no error is returned if pcs-handle is absent.
> + *
> + * Return: a PCS if found, or an error pointer on failure
> + */
> +struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
> +                                      const char *id, bool optional)
> +{
> +       int index;
> +       struct phylink_pcs *pcs;
> +       struct fwnode_handle *pcs_fwnode;
> +
> +       if (id)
> +               index = fwnode_property_match_string(fwnode, "pcs-names", id);
> +       else
> +               index = 0;
> +       if (index < 0) {
> +               if (optional && (index == -EINVAL || index == -ENODATA))
> +                       return NULL;
> +               return ERR_PTR(index);
> +       }
> +
> +       /* First try pcs-handle, and if that doesn't work fall back to the
> +        * (legacy) pcsphy-handle.
> +        */
> +       pcs_fwnode = fwnode_find_reference(fwnode, "pcs-handle", index);
> +       if (PTR_ERR(pcs_fwnode) == -ENOENT)
> +               pcs_fwnode = fwnode_find_reference(fwnode, "pcsphy-handle",
> +                                                  index);
> +       if (optional && !id && PTR_ERR(pcs_fwnode) == -ENOENT)
> +               return NULL;
> +       else if (IS_ERR(pcs_fwnode))
> +               return ERR_CAST(pcs_fwnode);
> +
> +       pcs = pcs_find(pcs_fwnode, NULL);
> +       fwnode_handle_put(pcs_fwnode);
> +       return pcs_get_tail(pcs);
> +}
> +EXPORT_SYMBOL_GPL(pcs_get_by_fwnode);
> +
> +/**
> + * pcs_get_by_provider() - Get a PCS from an existing provider
> + * @dev: The device providing the PCS
> + *
> + * This finds the first PCS registersed by @dev and returns a reference to it.
> + * Every call to pcs_get_by_provider() must be balanced with one to
> + * pcs_put().
> + *
> + * Return: a PCS if found, or an error pointer on failure
> + */
> +struct phylink_pcs *pcs_get_by_provider(const struct device *dev)
> +{
> +       return pcs_get_tail(pcs_find(NULL, dev));
> +}
> +EXPORT_SYMBOL_GPL(pcs_get_by_provider);
> +
> +/**
> + * pcs_put() - Release a previously-acquired PCS
> + * @pcs: The PCS to put
> + *
> + * This frees resources associated with the PCS which were acquired when it was
> + * gotten.
> + */
> +void pcs_put(struct phylink_pcs *pcs)
> +{
> +       if (!pcs)
> +               return;
> +
> +       put_device(pcs->dev);
> +       module_put(pcs->ops->owner);
> +}
> +EXPORT_SYMBOL_GPL(pcs_put);
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 967f79b59016..860d35bde5e9 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1318,6 +1318,7 @@ DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
>  DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
>  DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
>  DEFINE_SIMPLE_PROP(remote_endpoint, "remote-endpoint", NULL)
> +DEFINE_SIMPLE_PROP(pcs_handle, "pcs-handle", NULL)
>  DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
>  DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
>  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
> @@ -1406,6 +1407,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_pinctrl7, },
>         { .parse_prop = parse_pinctrl8, },
>         { .parse_prop = parse_remote_endpoint, .node_not_dev = true, },
> +       { .parse_prop = parse_pcs_handle, },
>         { .parse_prop = parse_pwms, },
>         { .parse_prop = parse_resets, },
>         { .parse_prop = parse_leds, },

Can you break the changes to this file into a separate patch please?
That'll clarify that this doesn't depend on any of the other changes
in this patch to work and it can stand on its own.

Also, I don't know how the pcs-handle is used, but it's likely that
this probe ordering enforcement could cause issues. So, if we need to
revert it, having it as a separate patch would help too.

And put this at the end of the series maybe?

Thanks,
Saravana

>
> diff --git a/include/linux/pcs.h b/include/linux/pcs.h
> new file mode 100644
> index 000000000000..00e76594e03c
> --- /dev/null
> +++ b/include/linux/pcs.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
> + */
> +
> +#ifndef _PCS_H
> +#define _PCS_H
> +
> +struct phylink_pcs;
> +struct fwnode;
> +
> +int pcs_register(struct phylink_pcs *pcs);
> +void pcs_unregister(struct phylink_pcs *pcs);
> +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs);
> +struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
> +                                      const char *id, bool optional);
> +struct phylink_pcs *pcs_get_by_provider(const struct device *dev);
> +void pcs_put(struct phylink_pcs *pcs);
> +
> +static inline struct phylink_pcs
> +*pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
> +                  const char *id)
> +{
> +       return _pcs_get_by_fwnode(fwnode, id, false);
> +}
> +
> +static inline struct phylink_pcs
> +*pcs_get_by_fwnode_optional(const struct fwnode_handle *fwnode, const char *id)
> +{
> +       return _pcs_get_by_fwnode(fwnode, id, true);
> +}
> +
> +#endif /* PCS_H */
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 6d06896fc20d..a713e70108a1 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -396,19 +396,24 @@ struct phylink_pcs_ops;
>
>  /**
>   * struct phylink_pcs - PHYLINK PCS instance
> + * @dev: the device associated with this PCS
>   * @ops: a pointer to the &struct phylink_pcs_ops structure
> + * @list: internal list of PCS devices
>   * @poll: poll the PCS for link changes
>   *
>   * This structure is designed to be embedded within the PCS private data,
>   * and will be passed between phylink and the PCS.
>   */
>  struct phylink_pcs {
> +       struct device *dev;
>         const struct phylink_pcs_ops *ops;
> +       struct list_head list;
>         bool poll;
>  };
>
>  /**
>   * struct phylink_pcs_ops - MAC PCS operations structure.
> + * @owner: the module which implements this PCS.
>   * @pcs_validate: validate the link configuration.
>   * @pcs_get_state: read the current MAC PCS link state from the hardware.
>   * @pcs_config: configure the MAC PCS for the selected mode and state.
> @@ -417,6 +422,7 @@ struct phylink_pcs {
>   *               (where necessary).
>   */
>  struct phylink_pcs_ops {
> +       struct module *owner;
>         int (*pcs_validate)(struct phylink_pcs *pcs, unsigned long *supported,
>                             const struct phylink_link_state *state);
>         void (*pcs_get_state)(struct phylink_pcs *pcs,
> --
> 2.35.1.1320.gc452695387.dirty
>

Re: [RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs

[Sean Anderson]

On 7/11/22 3:42 PM, Saravana Kannan wrote:
> On Mon, Jul 11, 2022 at 9:05 AM Sean Anderson <sean.anderson@seco.com> wrote:
>>
>> This adds support for getting PCS devices from the device tree. PCS
>> drivers must first register with phylink_register_pcs. After that, MAC
>> drivers may look up their PCS using phylink_get_pcs.
>>
>> To prevent the PCS driver from leaving suddenly, we use try_module_get. To
>> provide some ordering during probing/removal, we use device links managed
>> by of_fwnode_add_links. This will reduce the number of probe failures due
>> to deferral. It will not prevent this for non-standard properties (aka
>> pcsphy-handle), but the worst that happens is that we re-probe a few times.
>>
>> At the moment there is no support for specifying the interface used to
>> talk to the PCS. The MAC driver is expected to know how to talk to the
>> PCS. This is not a change, but it is perhaps an area for improvement.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> This is adapted from [1], primarily incorporating the changes discussed
>> there.
>>
>> [1] https://lore.kernel.org/netdev/9f73bc4f-5f99-95f5-78fa-dac96f9e0146@seco.com/
>>
>>  MAINTAINERS              |   1 +
>>  drivers/net/pcs/Kconfig  |  12 +++
>>  drivers/net/pcs/Makefile |   2 +
>>  drivers/net/pcs/core.c   | 226 +++++++++++++++++++++++++++++++++++++++
>>  drivers/of/property.c    |   2 +
>>  include/linux/pcs.h      |  33 ++++++
>>  include/linux/phylink.h  |   6 ++
>>  7 files changed, 282 insertions(+)
>>  create mode 100644 drivers/net/pcs/core.c
>>  create mode 100644 include/linux/pcs.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ca95b1833b97..3965d49753d3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7450,6 +7450,7 @@ F:        include/linux/*mdio*.h
>>  F:     include/linux/mdio/*.h
>>  F:     include/linux/mii.h
>>  F:     include/linux/of_net.h
>> +F:     include/linux/pcs.h
>>  F:     include/linux/phy.h
>>  F:     include/linux/phy_fixed.h
>>  F:     include/linux/platform_data/mdio-bcm-unimac.h
>> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
>> index 22ba7b0b476d..fed6264fdf33 100644
>> --- a/drivers/net/pcs/Kconfig
>> +++ b/drivers/net/pcs/Kconfig
>> @@ -5,6 +5,18 @@
>>
>>  menu "PCS device drivers"
>>
>> +config PCS
>> +       bool "PCS subsystem"
>> +       help
>> +         This provides common helper functions for registering and looking up
>> +         Physical Coding Sublayer (PCS) devices. PCS devices translate between
>> +         different interface types. In some use cases, they may either
>> +         translate between different types of Medium-Independent Interfaces
>> +         (MIIs), such as translating GMII to SGMII. This allows using a fast
>> +         serial interface to talk to the phy which translates the MII to the
>> +         Medium-Dependent Interface. Alternatively, they may translate a MII
>> +         directly to an MDI, such as translating GMII to 1000Base-X.
>> +
>>  config PCS_XPCS
>>         tristate "Synopsys DesignWare XPCS controller"
>>         depends on MDIO_DEVICE && MDIO_BUS
>> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
>> index 0603d469bd57..1fd21a1619d4 100644
>> --- a/drivers/net/pcs/Makefile
>> +++ b/drivers/net/pcs/Makefile
>> @@ -1,6 +1,8 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  # Makefile for Linux PCS drivers
>>
>> +obj-$(CONFIG_PCS)              += core.o
>> +
>>  pcs_xpcs-$(CONFIG_PCS_XPCS)    := pcs-xpcs.o pcs-xpcs-nxp.o
>>
>>  obj-$(CONFIG_PCS_XPCS)         += pcs_xpcs.o
>> diff --git a/drivers/net/pcs/core.c b/drivers/net/pcs/core.c
>> new file mode 100644
>> index 000000000000..b39ff1ccdb34
>> --- /dev/null
>> +++ b/drivers/net/pcs/core.c
>> @@ -0,0 +1,226 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
>> + */
>> +
>> +#include <linux/fwnode.h>
>> +#include <linux/list.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pcs.h>
>> +#include <linux/phylink.h>
>> +#include <linux/property.h>
>> +
>> +static LIST_HEAD(pcs_devices);
>> +static DEFINE_MUTEX(pcs_mutex);
>> +
>> +/**
>> + * pcs_register() - register a new PCS
>> + * @pcs: the PCS to register
>> + *
>> + * Registers a new PCS which can be automatically attached to a phylink.
>> + *
>> + * Return: 0 on success, or -errno on error
>> + */
>> +int pcs_register(struct phylink_pcs *pcs)
>> +{
>> +       if (!pcs->dev || !pcs->ops)
>> +               return -EINVAL;
>> +       if (!pcs->ops->pcs_an_restart || !pcs->ops->pcs_config ||
>> +           !pcs->ops->pcs_get_state)
>> +               return -EINVAL;
>> +
>> +       INIT_LIST_HEAD(&pcs->list);
>> +       mutex_lock(&pcs_mutex);
>> +       list_add(&pcs->list, &pcs_devices);
>> +       mutex_unlock(&pcs_mutex);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pcs_register);
>> +
>> +/**
>> + * pcs_unregister() - unregister a PCS
>> + * @pcs: a PCS previously registered with pcs_register()
>> + */
>> +void pcs_unregister(struct phylink_pcs *pcs)
>> +{
>> +       mutex_lock(&pcs_mutex);
>> +       list_del(&pcs->list);
>> +       mutex_unlock(&pcs_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(pcs_unregister);
>> +
>> +static void devm_pcs_release(struct device *dev, void *res)
>> +{
>> +       pcs_unregister(*(struct phylink_pcs **)res);
>> +}
>> +
>> +/**
>> + * devm_pcs_register - resource managed pcs_register()
>> + * @dev: device that is registering this PCS
>> + * @pcs: the PCS to register
>> + *
>> + * Managed pcs_register(). For PCSs registered by this function,
>> + * pcs_unregister() is automatically called on driver detach. See
>> + * pcs_register() for more information.
>> + *
>> + * Return: 0 on success, or -errno on failure
>> + */
>> +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs)
>> +{
>> +       struct phylink_pcs **pcsp;
>> +       int ret;
>> +
>> +       pcsp = devres_alloc(devm_pcs_release, sizeof(*pcsp),
>> +                           GFP_KERNEL);
>> +       if (!pcsp)
>> +               return -ENOMEM;
>> +
>> +       ret = pcs_register(pcs);
>> +       if (ret) {
>> +               devres_free(pcsp);
>> +               return ret;
>> +       }
>> +
>> +       *pcsp = pcs;
>> +       devres_add(dev, pcsp);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_pcs_register);
>> +
>> +/**
>> + * pcs_find() - Find the PCS associated with a fwnode or device
>> + * @fwnode: The PCS's fwnode
>> + * @dev: The PCS's device
>> + *
>> + * Search PCSs registered with pcs_register() for one with a matching
>> + * fwnode or device. Either @fwnode or @dev may be %NULL if matching against a
>> + * fwnode or device is not desired (respectively).
>> + *
>> + * Return: a matching PCS, or %NULL if not found
>> + */
>> +static struct phylink_pcs *pcs_find(const struct fwnode_handle *fwnode,
>> +                                   const struct device *dev)
>> +{
>> +       struct phylink_pcs *pcs;
>> +
>> +       mutex_lock(&pcs_mutex);
>> +       list_for_each_entry(pcs, &pcs_devices, list) {
>> +               if (dev && pcs->dev == dev)
>> +                       goto out;
>> +               if (fwnode && pcs->dev->fwnode == fwnode)
>> +                       goto out;
>> +       }
>> +       pcs = NULL;
>> +
>> +out:
>> +       mutex_unlock(&pcs_mutex);
>> +       pr_devel("%s: looking for %pfwf or %s %s...%s found\n", __func__,
>> +                fwnode, dev ? dev_driver_string(dev) : "(null)",
>> +                dev ? dev_name(dev) : "(null)", pcs ? " not" : "");
>> +       return pcs;
>> +}
>> +
>> +/**
>> + * pcs_get_tail() - Finish getting a PCS
>> + * @pcs: The PCS to get, or %NULL if one could not be found
>> + *
>> + * This performs common operations necessary when getting a PCS (chiefly
>> + * incrementing reference counts)
>> + *
>> + * Return: @pcs, or an error pointer on failure
>> + */
>> +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
>> +{
>> +       if (!pcs)
>> +               return ERR_PTR(-EPROBE_DEFER);
>> +
>> +       if (!try_module_get(pcs->ops->owner))
>> +               return ERR_PTR(-ENODEV);
>> +       get_device(pcs->dev);
>> +
>> +       return pcs;
>> +}
>> +
>> +/**
>> + * _pcs_get_by_fwnode() - Get a PCS from a fwnode property
>> + * @fwnode: The fwnode to get an associated PCS of
>> + * @id: The name of the PCS to get. May be %NULL to get the first PCS.
>> + * @optional: Whether the PCS is optional or not
>> + *
>> + * Look up a PCS associated with @fwnode and return a reference to it. Every
>> + * call to pcs_get_by_fwnode() must be balanced with one to pcs_put().
>> + *
>> + * If @optional is true, and @id is non-%NULL, then if @id cannot be found in
>> + * pcs-names, %NULL is returned (instead of an error). If @optional is true and
>> + * @id is %NULL, then no error is returned if pcs-handle is absent.
>> + *
>> + * Return: a PCS if found, or an error pointer on failure
>> + */
>> +struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
>> +                                      const char *id, bool optional)
>> +{
>> +       int index;
>> +       struct phylink_pcs *pcs;
>> +       struct fwnode_handle *pcs_fwnode;
>> +
>> +       if (id)
>> +               index = fwnode_property_match_string(fwnode, "pcs-names", id);
>> +       else
>> +               index = 0;
>> +       if (index < 0) {
>> +               if (optional && (index == -EINVAL || index == -ENODATA))
>> +                       return NULL;
>> +               return ERR_PTR(index);
>> +       }
>> +
>> +       /* First try pcs-handle, and if that doesn't work fall back to the
>> +        * (legacy) pcsphy-handle.
>> +        */
>> +       pcs_fwnode = fwnode_find_reference(fwnode, "pcs-handle", index);
>> +       if (PTR_ERR(pcs_fwnode) == -ENOENT)
>> +               pcs_fwnode = fwnode_find_reference(fwnode, "pcsphy-handle",
>> +                                                  index);
>> +       if (optional && !id && PTR_ERR(pcs_fwnode) == -ENOENT)
>> +               return NULL;
>> +       else if (IS_ERR(pcs_fwnode))
>> +               return ERR_CAST(pcs_fwnode);
>> +
>> +       pcs = pcs_find(pcs_fwnode, NULL);
>> +       fwnode_handle_put(pcs_fwnode);
>> +       return pcs_get_tail(pcs);
>> +}
>> +EXPORT_SYMBOL_GPL(pcs_get_by_fwnode);
>> +
>> +/**
>> + * pcs_get_by_provider() - Get a PCS from an existing provider
>> + * @dev: The device providing the PCS
>> + *
>> + * This finds the first PCS registersed by @dev and returns a reference to it.
>> + * Every call to pcs_get_by_provider() must be balanced with one to
>> + * pcs_put().
>> + *
>> + * Return: a PCS if found, or an error pointer on failure
>> + */
>> +struct phylink_pcs *pcs_get_by_provider(const struct device *dev)
>> +{
>> +       return pcs_get_tail(pcs_find(NULL, dev));
>> +}
>> +EXPORT_SYMBOL_GPL(pcs_get_by_provider);
>> +
>> +/**
>> + * pcs_put() - Release a previously-acquired PCS
>> + * @pcs: The PCS to put
>> + *
>> + * This frees resources associated with the PCS which were acquired when it was
>> + * gotten.
>> + */
>> +void pcs_put(struct phylink_pcs *pcs)
>> +{
>> +       if (!pcs)
>> +               return;
>> +
>> +       put_device(pcs->dev);
>> +       module_put(pcs->ops->owner);
>> +}
>> +EXPORT_SYMBOL_GPL(pcs_put);
>> diff --git a/drivers/of/property.c b/drivers/of/property.c
>> index 967f79b59016..860d35bde5e9 100644
>> --- a/drivers/of/property.c
>> +++ b/drivers/of/property.c
>> @@ -1318,6 +1318,7 @@ DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
>>  DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
>>  DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
>>  DEFINE_SIMPLE_PROP(remote_endpoint, "remote-endpoint", NULL)
>> +DEFINE_SIMPLE_PROP(pcs_handle, "pcs-handle", NULL)
>>  DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
>>  DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
>>  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
>> @@ -1406,6 +1407,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>>         { .parse_prop = parse_pinctrl7, },
>>         { .parse_prop = parse_pinctrl8, },
>>         { .parse_prop = parse_remote_endpoint, .node_not_dev = true, },
>> +       { .parse_prop = parse_pcs_handle, },
>>         { .parse_prop = parse_pwms, },
>>         { .parse_prop = parse_resets, },
>>         { .parse_prop = parse_leds, },
> 
> Can you break the changes to this file into a separate patch please?
> That'll clarify that this doesn't depend on any of the other changes
> in this patch to work and it can stand on its own.

OK

> Also, I don't know how the pcs-handle is used, but it's likely that
> this probe ordering enforcement could cause issues. So, if we need to
> revert it, having it as a separate patch would help too.
> 
> And put this at the end of the series maybe?
OK, I'll put it before patch 9/9 (which will likely need to be applied
much after the rest of this series.

--Sean

> Thanks,
> Saravana
> 
>>
>> diff --git a/include/linux/pcs.h b/include/linux/pcs.h
>> new file mode 100644
>> index 000000000000..00e76594e03c
>> --- /dev/null
>> +++ b/include/linux/pcs.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
>> + */
>> +
>> +#ifndef _PCS_H
>> +#define _PCS_H
>> +
>> +struct phylink_pcs;
>> +struct fwnode;
>> +
>> +int pcs_register(struct phylink_pcs *pcs);
>> +void pcs_unregister(struct phylink_pcs *pcs);
>> +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs);
>> +struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
>> +                                      const char *id, bool optional);
>> +struct phylink_pcs *pcs_get_by_provider(const struct device *dev);
>> +void pcs_put(struct phylink_pcs *pcs);
>> +
>> +static inline struct phylink_pcs
>> +*pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
>> +                  const char *id)
>> +{
>> +       return _pcs_get_by_fwnode(fwnode, id, false);
>> +}
>> +
>> +static inline struct phylink_pcs
>> +*pcs_get_by_fwnode_optional(const struct fwnode_handle *fwnode, const char *id)
>> +{
>> +       return _pcs_get_by_fwnode(fwnode, id, true);
>> +}
>> +
>> +#endif /* PCS_H */
>> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
>> index 6d06896fc20d..a713e70108a1 100644
>> --- a/include/linux/phylink.h
>> +++ b/include/linux/phylink.h
>> @@ -396,19 +396,24 @@ struct phylink_pcs_ops;
>>
>>  /**
>>   * struct phylink_pcs - PHYLINK PCS instance
>> + * @dev: the device associated with this PCS
>>   * @ops: a pointer to the &struct phylink_pcs_ops structure
>> + * @list: internal list of PCS devices
>>   * @poll: poll the PCS for link changes
>>   *
>>   * This structure is designed to be embedded within the PCS private data,
>>   * and will be passed between phylink and the PCS.
>>   */
>>  struct phylink_pcs {
>> +       struct device *dev;
>>         const struct phylink_pcs_ops *ops;
>> +       struct list_head list;
>>         bool poll;
>>  };
>>
>>  /**
>>   * struct phylink_pcs_ops - MAC PCS operations structure.
>> + * @owner: the module which implements this PCS.
>>   * @pcs_validate: validate the link configuration.
>>   * @pcs_get_state: read the current MAC PCS link state from the hardware.
>>   * @pcs_config: configure the MAC PCS for the selected mode and state.
>> @@ -417,6 +422,7 @@ struct phylink_pcs {
>>   *               (where necessary).
>>   */
>>  struct phylink_pcs_ops {
>> +       struct module *owner;
>>         int (*pcs_validate)(struct phylink_pcs *pcs, unsigned long *supported,
>>                             const struct phylink_link_state *state);
>>         void (*pcs_get_state)(struct phylink_pcs *pcs,
>> --
>> 2.35.1.1320.gc452695387.dirty
>>
> 

Re: [RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs

[Russell King (Oracle)]

Hi Sean,

It's a good attempt and may be nice to have, but I'm afraid the
implementation has a flaw to do with the lifetime of data structures
which always becomes a problem when we have multiple devices being
used in aggregate.

On Mon, Jul 11, 2022 at 12:05:13PM -0400, Sean Anderson wrote:
> +/**
> + * pcs_get_tail() - Finish getting a PCS
> + * @pcs: The PCS to get, or %NULL if one could not be found
> + *
> + * This performs common operations necessary when getting a PCS (chiefly
> + * incrementing reference counts)
> + *
> + * Return: @pcs, or an error pointer on failure
> + */
> +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
> +{
> +	if (!pcs)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	if (!try_module_get(pcs->ops->owner))
> +		return ERR_PTR(-ENODEV);

What you're trying to prevent here is the PCS going away - but holding a
reference to the module doesn't prevent that with the driver model. The
driver model design is such that a device can be unbound from its driver
at any moment. Taking a reference to the module doesn't prevent that,
all it does is ensure that the user can't remove the module. It doesn't
mean that the "pcs" structure will remain allocated.

The second issue that this creates is if a MAC driver creates the PCS
and then "gets" it through this interface, then the MAC driver module
ends up being locked in until the MAC driver devices are all unbound,
which isn't friendly at all.

So, anything that proposes to create a new subsystem where we have
multiple devices that make up an aggregate device needs to nicely cope
with any of those devices going away. For that to happen in this
instance, phylink would need to know that its in-use PCS for a
particular MAC is going away, then it could force the link down before
removing all references to the PCS device.

Another solution would be devlinks, but I am really not a fan of that
when there may be a single struct device backing multiple network
interfaces, where some of them may require PCS and others do not. One
wouldn't want the network interface with nfs-root to suddenly go away
because a PCS was unbound from its driver!

> +	get_device(pcs->dev);

This helps, but not enough. All it means is the struct device won't
go away, the "pcs" can still go away if the device is unbound from the
driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs

[Sean Anderson]

Hi Russell,

On 7/11/22 4:59 PM, Russell King (Oracle) wrote:
> Hi Sean,
> 
> It's a good attempt and may be nice to have, but I'm afraid the
> implementation has a flaw to do with the lifetime of data structures
> which always becomes a problem when we have multiple devices being
> used in aggregate.
> 
> On Mon, Jul 11, 2022 at 12:05:13PM -0400, Sean Anderson wrote:
>> +/**
>> + * pcs_get_tail() - Finish getting a PCS
>> + * @pcs: The PCS to get, or %NULL if one could not be found
>> + *
>> + * This performs common operations necessary when getting a PCS (chiefly
>> + * incrementing reference counts)
>> + *
>> + * Return: @pcs, or an error pointer on failure
>> + */
>> +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
>> +{
>> +	if (!pcs)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	if (!try_module_get(pcs->ops->owner))
>> +		return ERR_PTR(-ENODEV);
> 
> What you're trying to prevent here is the PCS going away - but holding a
> reference to the module doesn't prevent that with the driver model. The
> driver model design is such that a device can be unbound from its driver
> at any moment. Taking a reference to the module doesn't prevent that,
> all it does is ensure that the user can't remove the module. It doesn't
> mean that the "pcs" structure will remain allocated.

So how do things like (serdes) phys work? Presumably the same hazard
occurs any time a MAC uses a phy, because the phy can disappear at any time.

As it happens I can easily trigger an Oops by unbinding my serdes driver
and the plugging in an ethernet cable. Presumably this means that the phy
subsystem needs the devlink treatment? There are already several in-tree
MAC drivers using phys...

> The second issue that this creates is if a MAC driver creates the PCS
> and then "gets" it through this interface, then the MAC driver module
> ends up being locked in until the MAC driver devices are all unbound,
> which isn't friendly at all.

The intention here is not to use this for "internal" PCSs, but only for
external ones. I suppose you're referring to 

> So, anything that proposes to create a new subsystem where we have
> multiple devices that make up an aggregate device needs to nicely cope
> with any of those devices going away. For that to happen in this
> instance, phylink would need to know that its in-use PCS for a
> particular MAC is going away, then it could force the link down before
> removing all references to the PCS device.
> 
> Another solution would be devlinks, but I am really not a fan of that
> when there may be a single struct device backing multiple network
> interfaces, where some of them may require PCS and others do not. One
> wouldn't want the network interface with nfs-root to suddenly go away
> because a PCS was unbound from its driver!

Well, you can also do

echo "mmc0:0001" > /sys/bus/mmc/drivers/mmcblk/unbind

which will (depending on your system) have the same effect.

If being able to unbind any driver at any time is intended,
then I don't think we can save userspace from itself.

>> +	get_device(pcs->dev);
> 
> This helps, but not enough. All it means is the struct device won't
> go away, the "pcs" can still go away if the device is unbound from the
> driver.
> 

--Sean

Re: [RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs

[Sean Anderson]

On 7/11/22 5:47 PM, Sean Anderson wrote:
> Hi Russell,
> 
> On 7/11/22 4:59 PM, Russell King (Oracle) wrote:
>> Hi Sean,
>> 
>> It's a good attempt and may be nice to have, but I'm afraid the
>> implementation has a flaw to do with the lifetime of data structures
>> which always becomes a problem when we have multiple devices being
>> used in aggregate.
>> 
>> On Mon, Jul 11, 2022 at 12:05:13PM -0400, Sean Anderson wrote:
>>> +/**
>>> + * pcs_get_tail() - Finish getting a PCS
>>> + * @pcs: The PCS to get, or %NULL if one could not be found
>>> + *
>>> + * This performs common operations necessary when getting a PCS (chiefly
>>> + * incrementing reference counts)
>>> + *
>>> + * Return: @pcs, or an error pointer on failure
>>> + */
>>> +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
>>> +{
>>> +	if (!pcs)
>>> +		return ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +	if (!try_module_get(pcs->ops->owner))
>>> +		return ERR_PTR(-ENODEV);
>> 
>> What you're trying to prevent here is the PCS going away - but holding a
>> reference to the module doesn't prevent that with the driver model. The
>> driver model design is such that a device can be unbound from its driver
>> at any moment. Taking a reference to the module doesn't prevent that,
>> all it does is ensure that the user can't remove the module. It doesn't
>> mean that the "pcs" structure will remain allocated.
> 
> So how do things like (serdes) phys work? Presumably the same hazard
> occurs any time a MAC uses a phy, because the phy can disappear at any time.
> 
> As it happens I can easily trigger an Oops by unbinding my serdes driver
> and the plugging in an ethernet cable. Presumably this means that the phy
> subsystem needs the devlink treatment? There are already several in-tree
> MAC drivers using phys...
> 
>> The second issue that this creates is if a MAC driver creates the PCS
>> and then "gets" it through this interface, then the MAC driver module
>> ends up being locked in until the MAC driver devices are all unbound,
>> which isn't friendly at all.
> 
> The intention here is not to use this for "internal" PCSs, but only for
> external ones. I suppose you're referring to 

(looks like I forgot a sentence here)

...things like MAC->MDIO->PCS. I'll have to investigate whether this can
be removed properly.

>> So, anything that proposes to create a new subsystem where we have
>> multiple devices that make up an aggregate device needs to nicely cope
>> with any of those devices going away. For that to happen in this
>> instance, phylink would need to know that its in-use PCS for a
>> particular MAC is going away, then it could force the link down before
>> removing all references to the PCS device.
>>
>> Another solution would be devlinks, but I am really not a fan of that
>> when there may be a single struct device backing multiple network
>> interfaces, where some of them may require PCS and others do not.

I wonder if we could add an enable/disable callback of some kind, and
remove the devlink when the PCS was not in use. Not ideal, but then all
you have to do is set the interface correctly before removing the PCS.

>> One
>> wouldn't want the network interface with nfs-root to suddenly go away
>> because a PCS was unbound from its driver!
> 
> Well, you can also do
> 
> echo "mmc0:0001" > /sys/bus/mmc/drivers/mmcblk/unbind
> 
> which will (depending on your system) have the same effect.
> 
> If being able to unbind any driver at any time is intended,
> then I don't think we can save userspace from itself.
> 
>>> +	get_device(pcs->dev);
>> 
>> This helps, but not enough. All it means is the struct device won't
>> go away, the "pcs" can still go away if the device is unbound from the
>> driver.
>> 
> 
> --Sean
> 

Re: [RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs

[Russell King (Oracle)]

On Mon, Jul 11, 2022 at 05:47:26PM -0400, Sean Anderson wrote:
> Hi Russell,
> 
> On 7/11/22 4:59 PM, Russell King (Oracle) wrote:
> > Hi Sean,
> > 
> > It's a good attempt and may be nice to have, but I'm afraid the
> > implementation has a flaw to do with the lifetime of data structures
> > which always becomes a problem when we have multiple devices being
> > used in aggregate.
> > 
> > On Mon, Jul 11, 2022 at 12:05:13PM -0400, Sean Anderson wrote:
> >> +/**
> >> + * pcs_get_tail() - Finish getting a PCS
> >> + * @pcs: The PCS to get, or %NULL if one could not be found
> >> + *
> >> + * This performs common operations necessary when getting a PCS (chiefly
> >> + * incrementing reference counts)
> >> + *
> >> + * Return: @pcs, or an error pointer on failure
> >> + */
> >> +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
> >> +{
> >> +	if (!pcs)
> >> +		return ERR_PTR(-EPROBE_DEFER);
> >> +
> >> +	if (!try_module_get(pcs->ops->owner))
> >> +		return ERR_PTR(-ENODEV);
> > 
> > What you're trying to prevent here is the PCS going away - but holding a
> > reference to the module doesn't prevent that with the driver model. The
> > driver model design is such that a device can be unbound from its driver
> > at any moment. Taking a reference to the module doesn't prevent that,
> > all it does is ensure that the user can't remove the module. It doesn't
> > mean that the "pcs" structure will remain allocated.
> 
> So how do things like (serdes) phys work? Presumably the same hazard
> occurs any time a MAC uses a phy, because the phy can disappear at any time.
> 
> As it happens I can easily trigger an Oops by unbinding my serdes driver
> and the plugging in an ethernet cable. Presumably this means that the phy
> subsystem needs the devlink treatment? There are already several in-tree
> MAC drivers using phys...

It's sadly another example of this kind of thing. When you consider
that the system should operate in a safe manner with as few "gotchas"
as possible, then being able to "easily trigger an Oops" is something
that we should be avoiding. It's not hard to avoid - we have multiple
mechanisms in the kernel now to deal with it. We have the component
helper. We have devlinks. We can come up with other solutions such
as what I mentioned in my previous reply (which I consider to be the
superior solution in this case - because it doesn't mess up cases
where a single struct device is associated with multiple network
devices (such as on Armada 8040 based systems.)

It's really about "Quality of Implementation" - and I expect high
quality. I don't want my systems crashing because I've tried to
temporarily unbind some device.

> > The second issue that this creates is if a MAC driver creates the PCS
> > and then "gets" it through this interface, then the MAC driver module
> > ends up being locked in until the MAC driver devices are all unbound,
> > which isn't friendly at all.
> 
> The intention here is not to use this for "internal" PCSs, but only for
> external ones. I suppose you're referring to 

I wish I could say that intentions for use bear the test of time, but
sadly I can not.

> > So, anything that proposes to create a new subsystem where we have
> > multiple devices that make up an aggregate device needs to nicely cope
> > with any of those devices going away. For that to happen in this
> > instance, phylink would need to know that its in-use PCS for a
> > particular MAC is going away, then it could force the link down before
> > removing all references to the PCS device.
> > 
> > Another solution would be devlinks, but I am really not a fan of that
> > when there may be a single struct device backing multiple network
> > interfaces, where some of them may require PCS and others do not. One
> > wouldn't want the network interface with nfs-root to suddenly go away
> > because a PCS was unbound from its driver!
> 
> Well, you can also do
> 
> echo "mmc0:0001" > /sys/bus/mmc/drivers/mmcblk/unbind
> 
> which will (depending on your system) have the same effect.
> 
> If being able to unbind any driver at any time is intended,
> then I don't think we can save userspace from itself.

If you unbind the device that contains your rootfs, you are absolutely
correct. It's the same as taking down the network interface that gives
you access to your NFS root.

However, neither of these cause the kernel to crash - they make
userspace unusable.

So, let's say that it is acceptable that the kernel crashes if one
unbinds a device. Why then bother with try_module_get() - if the user
is silly enough to remove the module containing the PCS code, doesn't
the same argument apply? "Shouldn't have done that then."

I don't see the logic.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs

[Sean Anderson]

On 7/12/22 11:51 AM, Russell King (Oracle) wrote:
> On Mon, Jul 11, 2022 at 05:47:26PM -0400, Sean Anderson wrote:
>> Hi Russell,
>> 
>> On 7/11/22 4:59 PM, Russell King (Oracle) wrote:
>> > Hi Sean,
>> > 
>> > It's a good attempt and may be nice to have, but I'm afraid the
>> > implementation has a flaw to do with the lifetime of data structures
>> > which always becomes a problem when we have multiple devices being
>> > used in aggregate.
>> > 
>> > On Mon, Jul 11, 2022 at 12:05:13PM -0400, Sean Anderson wrote:
>> >> +/**
>> >> + * pcs_get_tail() - Finish getting a PCS
>> >> + * @pcs: The PCS to get, or %NULL if one could not be found
>> >> + *
>> >> + * This performs common operations necessary when getting a PCS (chiefly
>> >> + * incrementing reference counts)
>> >> + *
>> >> + * Return: @pcs, or an error pointer on failure
>> >> + */
>> >> +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
>> >> +{
>> >> +	if (!pcs)
>> >> +		return ERR_PTR(-EPROBE_DEFER);
>> >> +
>> >> +	if (!try_module_get(pcs->ops->owner))
>> >> +		return ERR_PTR(-ENODEV);
>> > 
>> > What you're trying to prevent here is the PCS going away - but holding a
>> > reference to the module doesn't prevent that with the driver model. The
>> > driver model design is such that a device can be unbound from its driver
>> > at any moment. Taking a reference to the module doesn't prevent that,
>> > all it does is ensure that the user can't remove the module. It doesn't
>> > mean that the "pcs" structure will remain allocated.
>> 
>> So how do things like (serdes) phys work? Presumably the same hazard
>> occurs any time a MAC uses a phy, because the phy can disappear at any time.
>> 
>> As it happens I can easily trigger an Oops by unbinding my serdes driver
>> and the plugging in an ethernet cable. Presumably this means that the phy
>> subsystem needs the devlink treatment? There are already several in-tree
>> MAC drivers using phys...
> 
> It's sadly another example of this kind of thing. When you consider
> that the system should operate in a safe manner with as few "gotchas"
> as possible, then being able to "easily trigger an Oops" is something
> that we should be avoiding. It's not hard to avoid - we have multiple
> mechanisms in the kernel now to deal with it. 

OK, so as mentioned above this exists in several MAC drivers already. How do
you propose to fix this?

> We have the component
> helper. We have devlinks. We can come up with other solutions such
> as what I mentioned in my previous reply (which I consider to be the
> superior solution in this case - because it doesn't mess up cases
> where a single struct device is associated with multiple network
> devices (such as on Armada 8040 based systems.)
> 
> It's really about "Quality of Implementation" - and I expect high
> quality. I don't want my systems crashing because I've tried to
> temporarily unbind some device.
> 
>> > The second issue that this creates is if a MAC driver creates the PCS
>> > and then "gets" it through this interface, then the MAC driver module
>> > ends up being locked in until the MAC driver devices are all unbound,
>> > which isn't friendly at all.
>> 
>> The intention here is not to use this for "internal" PCSs, but only for
>> external ones. I suppose you're referring to 
> 
> I wish I could say that intentions for use bear the test of time, but
> sadly I can not.

Well, we can burn that bridge when we come to it. For now, yes if you call
pcs_get_by_* from the same device where you call pcs_register then the device
will be "locked in".

>> > So, anything that proposes to create a new subsystem where we have
>> > multiple devices that make up an aggregate device needs to nicely cope
>> > with any of those devices going away. For that to happen in this
>> > instance, phylink would need to know that its in-use PCS for a
>> > particular MAC is going away, then it could force the link down before
>> > removing all references to the PCS device.
>> > 
>> > Another solution would be devlinks, but I am really not a fan of that
>> > when there may be a single struct device backing multiple network
>> > interfaces, where some of them may require PCS and others do not. One
>> > wouldn't want the network interface with nfs-root to suddenly go away
>> > because a PCS was unbound from its driver!
>> 
>> Well, you can also do
>> 
>> echo "mmc0:0001" > /sys/bus/mmc/drivers/mmcblk/unbind
>> 
>> which will (depending on your system) have the same effect.
>> 
>> If being able to unbind any driver at any time is intended,
>> then I don't think we can save userspace from itself.
> 
> If you unbind the device that contains your rootfs, you are absolutely
> correct. It's the same as taking down the network interface that gives
> you access to your NFS root.
> 
> However, neither of these cause the kernel to crash - they make
> userspace unusable.
> 
> So, let's say that it is acceptable that the kernel crashes if one
> unbinds a device. Why then bother with try_module_get() - if the user
> is silly enough to remove the module containing the PCS code, doesn't
> the same argument apply? "Shouldn't have done that then."
> 
> I don't see the logic.
> 

This was in response to your opposition to using devlink to manage the
PCS, since it would unbind the MAC as well. So what would happen here is
that someone would unbind the PCS, which would in turn unbind the MAC,
having the same effect as if the user manually unbound the MAC directly.

If you really want to avoid this, we'd need some kind of callback from
devlink to allow the MAC to say "well, I wasn't using that PCS anyway,"
or at the very least "let me clean up this (soon-to-be) dangling pointer."

--Sean

[RFC PATCH net-next 4/9] net: pcs: lynx: Convert to an mdio driver

[Sean Anderson]

This converts the lynx PCS driver to a proper MDIO driver. This allows
using a more conventional driver lifecycle (e.g. with a probe and
remove). For compatibility with existing device trees lacking a
compatible property, we bind the driver in lynx_pcs_create. This is
intended only as a transitional method. After compatible properties are
added to all existing device trees (and a reasonable amount of time has
passed), then lynx_pcs_create can be removed, and users can be converted
to pcs_get_fwnode.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/net/dsa/ocelot/Kconfig                |  2 +
 drivers/net/dsa/ocelot/felix_vsc9959.c        |  2 +-
 drivers/net/dsa/ocelot/seville_vsc9953.c      |  2 +-
 drivers/net/ethernet/freescale/dpaa2/Kconfig  |  1 +
 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  |  4 +-
 drivers/net/ethernet/freescale/enetc/Kconfig  |  1 +
 .../net/ethernet/freescale/enetc/enetc_pf.c   |  4 +-
 drivers/net/pcs/Kconfig                       | 11 +++-
 drivers/net/pcs/pcs-lynx.c                    | 65 +++++++++++++++----
 9 files changed, 72 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig
index 220b0b027b55..cbb0ced3f37d 100644
--- a/drivers/net/dsa/ocelot/Kconfig
+++ b/drivers/net/dsa/ocelot/Kconfig
@@ -10,6 +10,7 @@ config NET_DSA_MSCC_FELIX
 	select NET_DSA_TAG_OCELOT_8021Q
 	select NET_DSA_TAG_OCELOT
 	select FSL_ENETC_MDIO
+	select PCS
 	select PCS_LYNX
 	help
 	  This driver supports the VSC9959 (Felix) switch, which is embedded as
@@ -25,6 +26,7 @@ config NET_DSA_MSCC_SEVILLE
 	select MSCC_OCELOT_SWITCH_LIB
 	select NET_DSA_TAG_OCELOT_8021Q
 	select NET_DSA_TAG_OCELOT
+	select PCS
 	select PCS_LYNX
 	help
 	  This driver supports the VSC9953 (Seville) switch, which is embedded
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 570d0204b7be..57634e2296c0 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1094,7 +1094,7 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 			continue;
 
 		phylink_pcs = lynx_pcs_create(mdio_device);
-		if (!phylink_pcs) {
+		if (IS_ERR(phylink_pcs)) {
 			mdio_device_free(mdio_device);
 			continue;
 		}
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index ea0649211356..8c52de5d0b02 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1049,7 +1049,7 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 			continue;
 
 		phylink_pcs = lynx_pcs_create(mdio_device);
-		if (!phylink_pcs) {
+		if (IS_ERR(phylink_pcs)) {
 			mdio_device_free(mdio_device);
 			continue;
 		}
diff --git a/drivers/net/ethernet/freescale/dpaa2/Kconfig b/drivers/net/ethernet/freescale/dpaa2/Kconfig
index d029b69c3f18..2648e9fb6e13 100644
--- a/drivers/net/ethernet/freescale/dpaa2/Kconfig
+++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
@@ -3,6 +3,7 @@ config FSL_DPAA2_ETH
 	tristate "Freescale DPAA2 Ethernet"
 	depends on FSL_MC_BUS && FSL_MC_DPIO
 	select PHYLINK
+	select PCS
 	select PCS_LYNX
 	select FSL_XGMAC_MDIO
 	select NET_DEVLINK
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index c9bee9a0c9b2..e82c0d23eeb5 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -268,10 +268,10 @@ static int dpaa2_pcs_create(struct dpaa2_mac *mac,
 		return -EPROBE_DEFER;
 
 	mac->pcs = lynx_pcs_create(mdiodev);
-	if (!mac->pcs) {
+	if (IS_ERR(mac->pcs)) {
 		netdev_err(mac->net_dev, "lynx_pcs_create() failed\n");
 		put_device(&mdiodev->dev);
-		return -ENOMEM;
+		return PTR_ERR(mac->pcs);
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig
index cdc0ff89388a..c7dcdeb9a333 100644
--- a/drivers/net/ethernet/freescale/enetc/Kconfig
+++ b/drivers/net/ethernet/freescale/enetc/Kconfig
@@ -5,6 +5,7 @@ config FSL_ENETC
 	select FSL_ENETC_IERB
 	select FSL_ENETC_MDIO
 	select PHYLINK
+	select PCS
 	select PCS_LYNX
 	select DIMLIB
 	help
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index c4a0e836d4f0..8c923a93da88 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -859,9 +859,9 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 	}
 
 	phylink_pcs = lynx_pcs_create(mdio_device);
-	if (!phylink_pcs) {
+	if (IS_ERR(phylink_pcs)) {
 		mdio_device_free(mdio_device);
-		err = -ENOMEM;
+		err = PTR_ERR(phylink_pcs);
 		dev_err(dev, "cannot create lynx pcs (%d)\n", err);
 		goto unregister_mdiobus;
 	}
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index fed6264fdf33..a225176f92e8 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -25,9 +25,14 @@ config PCS_XPCS
 	  controllers.
 
 config PCS_LYNX
-	tristate
+	tristate "NXP Lynx PCS driver"
+	depends on PCS && MDIO_DEVICE
 	help
-	  This module provides helpers to phylink for managing the Lynx PCS
-	  which is part of the Layerscape and QorIQ Ethernet SERDES.
+	  This module provides driver support for the PCSs in Lynx 10g and 28g
+	  SerDes devices. These devices are present in NXP QorIQ SoCs,
+	  including the Layerscape series.
+
+	  If you want to use Ethernet on a QorIQ SoC, say "Y". If compiled as a
+	  module, it will be called "pcs-lynx".
 
 endmenu
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index fd3445374955..8272072698e4 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -1,11 +1,14 @@
-// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
-/* Copyright 2020 NXP
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (C) 2022 Sean Anderson <seanga2@gmail.com>
+ * Copyright 2020 NXP
  * Lynx PCS MDIO helpers
  */
 
 #include <linux/mdio.h>
-#include <linux/phylink.h>
+#include <linux/of.h>
+#include <linux/pcs.h>
 #include <linux/pcs-lynx.h>
+#include <linux/phylink.h>
 
 #define SGMII_CLOCK_PERIOD_NS		8 /* PCS is clocked at 125 MHz */
 #define LINK_TIMER_VAL(ns)		((u32)((ns) / SGMII_CLOCK_PERIOD_NS))
@@ -337,34 +340,74 @@ static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 }
 
 static const struct phylink_pcs_ops lynx_pcs_phylink_ops = {
+	.owner = THIS_MODULE,
 	.pcs_get_state = lynx_pcs_get_state,
 	.pcs_config = lynx_pcs_config,
 	.pcs_an_restart = lynx_pcs_an_restart,
 	.pcs_link_up = lynx_pcs_link_up,
 };
 
-struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
+static int lynx_pcs_probe(struct mdio_device *mdio)
 {
+	struct device *dev = &mdio->dev;
 	struct lynx_pcs *lynx;
+	int ret;
 
-	lynx = kzalloc(sizeof(*lynx), GFP_KERNEL);
+	lynx = devm_kzalloc(dev, sizeof(*lynx), GFP_KERNEL);
 	if (!lynx)
-		return NULL;
+		return -ENOMEM;
 
 	lynx->mdio = mdio;
+	lynx->pcs.dev = dev;
 	lynx->pcs.ops = &lynx_pcs_phylink_ops;
 	lynx->pcs.poll = true;
 
-	return lynx_to_phylink_pcs(lynx);
+	ret = devm_pcs_register(dev, &lynx->pcs);
+	if (ret)
+		return dev_err_probe(dev, ret, "could not register PCS\n");
+	dev_info(dev, "probed\n");
+	return 0;
+}
+
+static const struct of_device_id lynx_pcs_of_match[] = {
+	{ .compatible = "fsl,lynx-pcs" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, lynx_pcs_of_match);
+
+static struct mdio_driver lynx_pcs_driver = {
+	.probe = lynx_pcs_probe,
+	.mdiodrv.driver = {
+		.name = "lynx-pcs",
+		.of_match_table = of_match_ptr(lynx_pcs_of_match),
+	},
+};
+mdio_module_driver(lynx_pcs_driver);
+
+struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
+{
+	struct device *dev = &mdio->dev;
+	int err;
+
+	/* For compatibility with device trees lacking compatible strings, we
+	 * bind the device manually here.
+	 */
+	err = device_driver_attach(&lynx_pcs_driver.mdiodrv.driver, dev);
+	if (err && err != -EBUSY) {
+		if (err == -EAGAIN)
+			err = -EPROBE_DEFER;
+		return ERR_PTR(err);
+	}
+
+	return pcs_get_by_provider(&mdio->dev);
 }
 EXPORT_SYMBOL(lynx_pcs_create);
 
 void lynx_pcs_destroy(struct phylink_pcs *pcs)
 {
-	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
-
-	kfree(lynx);
+	pcs_put(pcs);
 }
 EXPORT_SYMBOL(lynx_pcs_destroy);
 
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_DESCRIPTION("NXP Lynx 10G/28G PCS driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1.1320.gc452695387.dirty

Re: [RFC PATCH net-next 4/9] net: pcs: lynx: Convert to an mdio driver

[Vladimir Oltean]

On Mon, Jul 11, 2022 at 12:05:14PM -0400, Sean Anderson wrote:
> This converts the lynx PCS driver to a proper MDIO driver. This allows
> using a more conventional driver lifecycle (e.g. with a probe and
> remove). For compatibility with existing device trees lacking a
> compatible property, we bind the driver in lynx_pcs_create. This is
> intended only as a transitional method. After compatible properties are
> added to all existing device trees (and a reasonable amount of time has
> passed), then lynx_pcs_create can be removed, and users can be converted
> to pcs_get_fwnode.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---

I'm compiling and testing patch by patch now. Here's how things go on
LS1028A at this stage:

[    6.317357] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000110
[    6.326219] Mem abort info:
[    6.329027]   ESR = 0x0000000096000004
[    6.332815]   EC = 0x25: DABT (current EL), IL = 32 bits
[    6.338182]   SET = 0, FnV = 0
[    6.341252]   EA = 0, S1PTW = 0
[    6.344436]   FSC = 0x04: level 0 translation fault
[    6.349378] Data abort info:
[    6.352273]   ISV = 0, ISS = 0x00000004
[    6.356154]   CM = 0, WnR = 0
[    6.359164] [0000000000000110] user address but active_mm is swapper
[    6.365629] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[    6.371221] Modules linked in:
[    6.374284] CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3317
[    6.383364] Hardware name: LS1028A RDB Board (DT)
[    6.388081] Workqueue: events_unbound deferred_probe_work_func
[    6.393939] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    6.400926] pc : __driver_probe_device+0x1c/0x150
[    6.405646] lr : device_driver_attach+0x58/0xc0
[    6.410190] sp : ffff8000085639c0
[    6.413510] x29: ffff8000085639c0 x28: ffffb1a2587dae50 x27: ffff2b6943304bc0
[    6.420676] x26: ffff2b694330c000 x25: ffff2b69433010a0 x24: ffff2b69bf719898
[    6.427840] x23: ffff2b6941074000 x22: ffff2b6943304000 x21: ffff2b6943301880
[    6.435004] x20: ffff2b6943301800 x19: ffffb1a259faf3d0 x18: ffffffffffffffff
[    6.442168] x17: 000000002b64f81b x16: 000000006d50a0b2 x15: ffff2b6943307196
[    6.449332] x14: 0000000000000002 x13: ffff2b6943307194 x12: 0000000000000003
[    6.456497] x11: ffff2b69433018f0 x10: 0000000000000003 x9 : ffffb1a2578b1e08
[    6.463662] x8 : ffff2b6940b36200 x7 : ffffb1a25a0da000 x6 : 000000003225858e
[    6.470826] x5 : 0000000000000000 x4 : ffff79c76227a000 x3 : 0000000000000000
[    6.477989] x2 : 0000000000000000 x1 : ffff2b6943301800 x0 : ffffb1a259faf3d0
[    6.485153] Call trace:
[    6.487601]  __driver_probe_device+0x1c/0x150
[    6.491971]  device_driver_attach+0x58/0xc0
[    6.496167]  lynx_pcs_create+0x30/0x7c
[    6.499927]  enetc_pf_probe+0x984/0xeb0
[    6.503775]  local_pci_probe+0x4c/0xc0
[    6.507536]  pci_device_probe+0xb8/0x210
[    6.511470]  really_probe.part.0+0xa4/0x2b0
[    6.515665]  __driver_probe_device+0xa0/0x150
[    6.520033]  driver_probe_device+0xb4/0x150
[    6.524228]  __device_attach_driver+0xc4/0x130
[    6.528684]  bus_for_each_drv+0x84/0xe0
[    6.532529]  __device_attach+0xb0/0x1d0
[    6.536375]  device_initial_probe+0x20/0x2c
[    6.540569]  bus_probe_device+0xac/0xb4
[    6.544414]  deferred_probe_work_func+0x98/0xd4
[    6.548956]  process_one_work+0x294/0x6d0
[    6.552979]  worker_thread+0x80/0x460
[    6.556651]  kthread+0x124/0x130
[    6.559887]  ret_from_fork+0x10/0x20
[    6.563475] Code: a9bd7bfd 910003fd a90153f3 f9402422 (39444042)

Disassembly of drivers/base/dd.c shows that dev->p is a NULL pointer,
and dev->p->dead goes right through it. How did we even get here...
device_private_init() should be called by device_add().

Curiously enough, mdio_device_create() only calls device_initialize().
It's mdio_device_register() that calls device_add(). So after this
patch, we cannot call lynx_pcs_create() without calling
mdio_device_register().

Re: [RFC PATCH net-next 4/9] net: pcs: lynx: Convert to an mdio driver

[Sean Anderson]

On 7/19/22 12:01 PM, Vladimir Oltean wrote:
> On Mon, Jul 11, 2022 at 12:05:14PM -0400, Sean Anderson wrote:
>> This converts the lynx PCS driver to a proper MDIO driver. This allows
>> using a more conventional driver lifecycle (e.g. with a probe and
>> remove). For compatibility with existing device trees lacking a
>> compatible property, we bind the driver in lynx_pcs_create. This is
>> intended only as a transitional method. After compatible properties are
>> added to all existing device trees (and a reasonable amount of time has
>> passed), then lynx_pcs_create can be removed, and users can be converted
>> to pcs_get_fwnode.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
> 
> I'm compiling and testing patch by patch now. Here's how things go on
> LS1028A at this stage:
> 
> [    6.317357] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000110
> [    6.326219] Mem abort info:
> [    6.329027]   ESR = 0x0000000096000004
> [    6.332815]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    6.338182]   SET = 0, FnV = 0
> [    6.341252]   EA = 0, S1PTW = 0
> [    6.344436]   FSC = 0x04: level 0 translation fault
> [    6.349378] Data abort info:
> [    6.352273]   ISV = 0, ISS = 0x00000004
> [    6.356154]   CM = 0, WnR = 0
> [    6.359164] [0000000000000110] user address but active_mm is swapper
> [    6.365629] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [    6.371221] Modules linked in:
> [    6.374284] CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3317
> [    6.383364] Hardware name: LS1028A RDB Board (DT)
> [    6.388081] Workqueue: events_unbound deferred_probe_work_func
> [    6.393939] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    6.400926] pc : __driver_probe_device+0x1c/0x150
> [    6.405646] lr : device_driver_attach+0x58/0xc0
> [    6.410190] sp : ffff8000085639c0
> [    6.413510] x29: ffff8000085639c0 x28: ffffb1a2587dae50 x27: ffff2b6943304bc0
> [    6.420676] x26: ffff2b694330c000 x25: ffff2b69433010a0 x24: ffff2b69bf719898
> [    6.427840] x23: ffff2b6941074000 x22: ffff2b6943304000 x21: ffff2b6943301880
> [    6.435004] x20: ffff2b6943301800 x19: ffffb1a259faf3d0 x18: ffffffffffffffff
> [    6.442168] x17: 000000002b64f81b x16: 000000006d50a0b2 x15: ffff2b6943307196
> [    6.449332] x14: 0000000000000002 x13: ffff2b6943307194 x12: 0000000000000003
> [    6.456497] x11: ffff2b69433018f0 x10: 0000000000000003 x9 : ffffb1a2578b1e08
> [    6.463662] x8 : ffff2b6940b36200 x7 : ffffb1a25a0da000 x6 : 000000003225858e
> [    6.470826] x5 : 0000000000000000 x4 : ffff79c76227a000 x3 : 0000000000000000
> [    6.477989] x2 : 0000000000000000 x1 : ffff2b6943301800 x0 : ffffb1a259faf3d0
> [    6.485153] Call trace:
> [    6.487601]  __driver_probe_device+0x1c/0x150
> [    6.491971]  device_driver_attach+0x58/0xc0
> [    6.496167]  lynx_pcs_create+0x30/0x7c
> [    6.499927]  enetc_pf_probe+0x984/0xeb0
> [    6.503775]  local_pci_probe+0x4c/0xc0
> [    6.507536]  pci_device_probe+0xb8/0x210
> [    6.511470]  really_probe.part.0+0xa4/0x2b0
> [    6.515665]  __driver_probe_device+0xa0/0x150
> [    6.520033]  driver_probe_device+0xb4/0x150
> [    6.524228]  __device_attach_driver+0xc4/0x130
> [    6.528684]  bus_for_each_drv+0x84/0xe0
> [    6.532529]  __device_attach+0xb0/0x1d0
> [    6.536375]  device_initial_probe+0x20/0x2c
> [    6.540569]  bus_probe_device+0xac/0xb4
> [    6.544414]  deferred_probe_work_func+0x98/0xd4
> [    6.548956]  process_one_work+0x294/0x6d0
> [    6.552979]  worker_thread+0x80/0x460
> [    6.556651]  kthread+0x124/0x130
> [    6.559887]  ret_from_fork+0x10/0x20
> [    6.563475] Code: a9bd7bfd 910003fd a90153f3 f9402422 (39444042)
> 
> Disassembly of drivers/base/dd.c shows that dev->p is a NULL pointer,
> and dev->p->dead goes right through it. How did we even get here...
> device_private_init() should be called by device_add().
> 
> Curiously enough, mdio_device_create() only calls device_initialize().
> It's mdio_device_register() that calls device_add(). So after this
> patch, we cannot call lynx_pcs_create() without calling
> mdio_device_register().

OK, so presumably we need to call mdio_device_register after mdio_device_create.
I suppose I should have caught this because patch 5 does exactly this.

--Sean

Re: [RFC PATCH net-next 4/9] net: pcs: lynx: Convert to an mdio driver

[Vladimir Oltean]

On Tue, Jul 19, 2022 at 12:16:17PM -0400, Sean Anderson wrote:
> > Curiously enough, mdio_device_create() only calls device_initialize().
> > It's mdio_device_register() that calls device_add(). So after this
> > patch, we cannot call lynx_pcs_create() without calling
> > mdio_device_register().
> 
> OK, so presumably we need to call mdio_device_register after mdio_device_create.
> I suppose I should have caught this because patch 5 does exactly this.

And that's only to not crash during boot, mind you. Networking is broken
at this stage. I'll proceed to patch 5 to see what happens next.

[RFC PATCH net-next 5/9] net: pcs: lynx: Use pcs_get_by_provider to get PCS

[Sean Anderson]

There is a common flow in several drivers where a lynx PCS is created
without a corresponding firmware node. Consolidate these into one helper
function. Because we control when the mdiodev is registered, we can add
a custom match function which will automatically bind our driver
(instead of using device_driver_attach).

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/net/dsa/ocelot/felix_vsc9959.c        | 25 ++++---------------
 drivers/net/dsa/ocelot/seville_vsc9953.c      | 25 ++++---------------
 .../net/ethernet/freescale/enetc/enetc_pf.c   | 21 +++-------------
 drivers/net/pcs/pcs-lynx.c                    | 24 ++++++++++++++++++
 include/linux/pcs-lynx.h                      |  1 +
 5 files changed, 39 insertions(+), 57 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 57634e2296c0..0a756c25d5e8 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -11,6 +11,7 @@
 #include <net/tc_act/tc_gate.h>
 #include <soc/mscc/ocelot.h>
 #include <linux/dsa/ocelot.h>
+#include <linux/pcs.h>
 #include <linux/pcs-lynx.h>
 #include <net/pkt_sched.h>
 #include <linux/iopoll.h>
@@ -1089,16 +1090,9 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
 			continue;
 
-		mdio_device = mdio_device_create(felix->imdio, port);
-		if (IS_ERR(mdio_device))
+		phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, port);
+		if (IS_ERR(phylink_pcs))
 			continue;
-
-		phylink_pcs = lynx_pcs_create(mdio_device);
-		if (IS_ERR(phylink_pcs)) {
-			mdio_device_free(mdio_device);
-			continue;
-		}
-
 		felix->pcs[port] = phylink_pcs;
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", port);
@@ -1112,17 +1106,8 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
 	struct felix *felix = ocelot_to_felix(ocelot);
 	int port;
 
-	for (port = 0; port < ocelot->num_phys_ports; port++) {
-		struct phylink_pcs *phylink_pcs = felix->pcs[port];
-		struct mdio_device *mdio_device;
-
-		if (!phylink_pcs)
-			continue;
-
-		mdio_device = lynx_get_mdio_device(phylink_pcs);
-		mdio_device_free(mdio_device);
-		lynx_pcs_destroy(phylink_pcs);
-	}
+	for (port = 0; port < ocelot->num_phys_ports; port++)
+		pcs_put(felix->pcs[port]);
 	mdiobus_unregister(felix->imdio);
 	mdiobus_free(felix->imdio);
 }
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 8c52de5d0b02..9006dec85ef0 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -9,6 +9,7 @@
 #include <linux/mdio/mdio-mscc-miim.h>
 #include <linux/of_mdio.h>
 #include <linux/of_platform.h>
+#include <linux/pcs.h>
 #include <linux/pcs-lynx.h>
 #include <linux/dsa/ocelot.h>
 #include <linux/iopoll.h>
@@ -1044,16 +1045,9 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
 			continue;
 
-		mdio_device = mdio_device_create(felix->imdio, addr);
-		if (IS_ERR(mdio_device))
+		phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, addr);
+		if (IS_ERR(phylink_pcs))
 			continue;
-
-		phylink_pcs = lynx_pcs_create(mdio_device);
-		if (IS_ERR(phylink_pcs)) {
-			mdio_device_free(mdio_device);
-			continue;
-		}
-
 		felix->pcs[port] = phylink_pcs;
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", addr);
@@ -1067,17 +1061,8 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
 	struct felix *felix = ocelot_to_felix(ocelot);
 	int port;
 
-	for (port = 0; port < ocelot->num_phys_ports; port++) {
-		struct phylink_pcs *phylink_pcs = felix->pcs[port];
-		struct mdio_device *mdio_device;
-
-		if (!phylink_pcs)
-			continue;
-
-		mdio_device = lynx_get_mdio_device(phylink_pcs);
-		mdio_device_free(mdio_device);
-		lynx_pcs_destroy(phylink_pcs);
-	}
+	for (port = 0; port < ocelot->num_phys_ports; port++)
+		pcs_put(felix->pcs[port]);
 
 	/* mdiobus_unregister and mdiobus_free handled by devres */
 }
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
index 8c923a93da88..8da7c8644e44 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
@@ -8,6 +8,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
+#include <linux/pcs.h>
 #include <linux/pcs-lynx.h>
 #include "enetc_ierb.h"
 #include "enetc_pf.h"
@@ -827,7 +828,6 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 	struct device *dev = &pf->si->pdev->dev;
 	struct enetc_mdio_priv *mdio_priv;
 	struct phylink_pcs *phylink_pcs;
-	struct mdio_device *mdio_device;
 	struct mii_bus *bus;
 	int err;
 
@@ -851,16 +851,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 		goto free_mdio_bus;
 	}
 
-	mdio_device = mdio_device_create(bus, 0);
-	if (IS_ERR(mdio_device)) {
-		err = PTR_ERR(mdio_device);
-		dev_err(dev, "cannot create mdio device (%d)\n", err);
-		goto unregister_mdiobus;
-	}
-
-	phylink_pcs = lynx_pcs_create(mdio_device);
+	phylink_pcs = lynx_pcs_create_on_bus(bus, 0);
 	if (IS_ERR(phylink_pcs)) {
-		mdio_device_free(mdio_device);
 		err = PTR_ERR(phylink_pcs);
 		dev_err(dev, "cannot create lynx pcs (%d)\n", err);
 		goto unregister_mdiobus;
@@ -880,13 +872,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
 
 static void enetc_imdio_remove(struct enetc_pf *pf)
 {
-	struct mdio_device *mdio_device;
-
-	if (pf->pcs) {
-		mdio_device = lynx_get_mdio_device(pf->pcs);
-		mdio_device_free(mdio_device);
-		lynx_pcs_destroy(pf->pcs);
-	}
+	if (pf->pcs)
+		pcs_put(pf->pcs);
 	if (pf->imdio) {
 		mdiobus_unregister(pf->imdio);
 		mdiobus_free(pf->imdio);
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index 8272072698e4..adb9fd5ce72e 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -403,6 +403,30 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
 }
 EXPORT_SYMBOL(lynx_pcs_create);
 
+struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr)
+{
+	struct mdio_device *mdio;
+	struct phylink_pcs *pcs;
+	int err;
+
+	mdio = mdio_device_create(bus, addr);
+	if (IS_ERR(mdio))
+		return ERR_CAST(mdio);
+
+	mdio->bus_match = mdio_device_bus_match;
+	strncpy(mdio->modalias, "lynx-pcs", sizeof(mdio->modalias));
+	err = mdio_device_register(mdio);
+	if (err) {
+		mdio_device_free(mdio);
+		return ERR_PTR(err);
+	}
+
+	pcs = pcs_get_by_provider(&mdio->dev);
+	mdio_device_free(mdio);
+	return pcs;
+}
+EXPORT_SYMBOL(lynx_pcs_create_on_bus);
+
 void lynx_pcs_destroy(struct phylink_pcs *pcs)
 {
 	pcs_put(pcs);
diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h
index 5712cc2ce775..1c14342bb8c4 100644
--- a/include/linux/pcs-lynx.h
+++ b/include/linux/pcs-lynx.h
@@ -12,6 +12,7 @@
 struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs);
 
 struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio);
+struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr);
 
 void lynx_pcs_destroy(struct phylink_pcs *pcs);
 
-- 
2.35.1.1320.gc452695387.dirty

Re: [RFC PATCH net-next 5/9] net: pcs: lynx: Use pcs_get_by_provider to get PCS

[Vladimir Oltean]

On Mon, Jul 11, 2022 at 12:05:15PM -0400, Sean Anderson wrote:
> There is a common flow in several drivers where a lynx PCS is created
> without a corresponding firmware node. Consolidate these into one helper
> function. Because we control when the mdiodev is registered, we can add
> a custom match function which will automatically bind our driver
> (instead of using device_driver_attach).
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  drivers/net/dsa/ocelot/felix_vsc9959.c        | 25 ++++---------------
>  drivers/net/dsa/ocelot/seville_vsc9953.c      | 25 ++++---------------
>  .../net/ethernet/freescale/enetc/enetc_pf.c   | 21 +++-------------
>  drivers/net/pcs/pcs-lynx.c                    | 24 ++++++++++++++++++
>  include/linux/pcs-lynx.h                      |  1 +
>  5 files changed, 39 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index 57634e2296c0..0a756c25d5e8 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -11,6 +11,7 @@
>  #include <net/tc_act/tc_gate.h>
>  #include <soc/mscc/ocelot.h>
>  #include <linux/dsa/ocelot.h>
> +#include <linux/pcs.h>
>  #include <linux/pcs-lynx.h>
>  #include <net/pkt_sched.h>
>  #include <linux/iopoll.h>
> @@ -1089,16 +1090,9 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
>  		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
>  			continue;
>  
> -		mdio_device = mdio_device_create(felix->imdio, port);
> -		if (IS_ERR(mdio_device))
> +		phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, port);
> +		if (IS_ERR(phylink_pcs))
>  			continue;
> -
> -		phylink_pcs = lynx_pcs_create(mdio_device);
> -		if (IS_ERR(phylink_pcs)) {
> -			mdio_device_free(mdio_device);
> -			continue;
> -		}
> -
>  		felix->pcs[port] = phylink_pcs;
>  
>  		dev_info(dev, "Found PCS at internal MDIO address %d\n", port);
> @@ -1112,17 +1106,8 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
>  	struct felix *felix = ocelot_to_felix(ocelot);
>  	int port;
>  
> -	for (port = 0; port < ocelot->num_phys_ports; port++) {
> -		struct phylink_pcs *phylink_pcs = felix->pcs[port];
> -		struct mdio_device *mdio_device;
> -
> -		if (!phylink_pcs)
> -			continue;
> -
> -		mdio_device = lynx_get_mdio_device(phylink_pcs);
> -		mdio_device_free(mdio_device);
> -		lynx_pcs_destroy(phylink_pcs);
> -	}
> +	for (port = 0; port < ocelot->num_phys_ports; port++)
> +		pcs_put(felix->pcs[port]);
>  	mdiobus_unregister(felix->imdio);
>  	mdiobus_free(felix->imdio);
>  }
> diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
> index 8c52de5d0b02..9006dec85ef0 100644
> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> @@ -9,6 +9,7 @@
>  #include <linux/mdio/mdio-mscc-miim.h>
>  #include <linux/of_mdio.h>
>  #include <linux/of_platform.h>
> +#include <linux/pcs.h>
>  #include <linux/pcs-lynx.h>
>  #include <linux/dsa/ocelot.h>
>  #include <linux/iopoll.h>
> @@ -1044,16 +1045,9 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
>  		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
>  			continue;
>  
> -		mdio_device = mdio_device_create(felix->imdio, addr);
> -		if (IS_ERR(mdio_device))
> +		phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, addr);
> +		if (IS_ERR(phylink_pcs))
>  			continue;
> -
> -		phylink_pcs = lynx_pcs_create(mdio_device);
> -		if (IS_ERR(phylink_pcs)) {
> -			mdio_device_free(mdio_device);
> -			continue;
> -		}
> -
>  		felix->pcs[port] = phylink_pcs;
>  
>  		dev_info(dev, "Found PCS at internal MDIO address %d\n", addr);
> @@ -1067,17 +1061,8 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
>  	struct felix *felix = ocelot_to_felix(ocelot);
>  	int port;
>  
> -	for (port = 0; port < ocelot->num_phys_ports; port++) {
> -		struct phylink_pcs *phylink_pcs = felix->pcs[port];
> -		struct mdio_device *mdio_device;
> -
> -		if (!phylink_pcs)
> -			continue;
> -
> -		mdio_device = lynx_get_mdio_device(phylink_pcs);
> -		mdio_device_free(mdio_device);
> -		lynx_pcs_destroy(phylink_pcs);
> -	}
> +	for (port = 0; port < ocelot->num_phys_ports; port++)
> +		pcs_put(felix->pcs[port]);
>  
>  	/* mdiobus_unregister and mdiobus_free handled by devres */
>  }
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> index 8c923a93da88..8da7c8644e44 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -8,6 +8,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
> +#include <linux/pcs.h>
>  #include <linux/pcs-lynx.h>
>  #include "enetc_ierb.h"
>  #include "enetc_pf.h"
> @@ -827,7 +828,6 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>  	struct device *dev = &pf->si->pdev->dev;
>  	struct enetc_mdio_priv *mdio_priv;
>  	struct phylink_pcs *phylink_pcs;
> -	struct mdio_device *mdio_device;
>  	struct mii_bus *bus;
>  	int err;
>  
> @@ -851,16 +851,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>  		goto free_mdio_bus;
>  	}
>  
> -	mdio_device = mdio_device_create(bus, 0);
> -	if (IS_ERR(mdio_device)) {
> -		err = PTR_ERR(mdio_device);
> -		dev_err(dev, "cannot create mdio device (%d)\n", err);
> -		goto unregister_mdiobus;
> -	}
> -
> -	phylink_pcs = lynx_pcs_create(mdio_device);
> +	phylink_pcs = lynx_pcs_create_on_bus(bus, 0);
>  	if (IS_ERR(phylink_pcs)) {
> -		mdio_device_free(mdio_device);
>  		err = PTR_ERR(phylink_pcs);
>  		dev_err(dev, "cannot create lynx pcs (%d)\n", err);
>  		goto unregister_mdiobus;
> @@ -880,13 +872,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>  
>  static void enetc_imdio_remove(struct enetc_pf *pf)
>  {
> -	struct mdio_device *mdio_device;
> -
> -	if (pf->pcs) {
> -		mdio_device = lynx_get_mdio_device(pf->pcs);
> -		mdio_device_free(mdio_device);
> -		lynx_pcs_destroy(pf->pcs);
> -	}
> +	if (pf->pcs)
> +		pcs_put(pf->pcs);
>  	if (pf->imdio) {
>  		mdiobus_unregister(pf->imdio);
>  		mdiobus_free(pf->imdio);
> diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
> index 8272072698e4..adb9fd5ce72e 100644
> --- a/drivers/net/pcs/pcs-lynx.c
> +++ b/drivers/net/pcs/pcs-lynx.c
> @@ -403,6 +403,30 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
>  }
>  EXPORT_SYMBOL(lynx_pcs_create);
>  
> +struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr)
> +{
> +	struct mdio_device *mdio;
> +	struct phylink_pcs *pcs;
> +	int err;
> +
> +	mdio = mdio_device_create(bus, addr);
> +	if (IS_ERR(mdio))
> +		return ERR_CAST(mdio);
> +
> +	mdio->bus_match = mdio_device_bus_match;
> +	strncpy(mdio->modalias, "lynx-pcs", sizeof(mdio->modalias));
> +	err = mdio_device_register(mdio);

Yeah, so the reason why mdio_device_register() fails with -EBUSY for the
PCS devices created by felix_vsc9959.c is this:

int mdiobus_register_device(struct mdio_device *mdiodev)
{
	int err;

	if (mdiodev->bus->mdio_map[mdiodev->addr])
		return -EBUSY;

In other words, we already have an existing mdiodev on the bus at
address mdiodev->addr. Funnily enough, that device is actually us.
It was created at MDIO bus creation time, a dummy phydev that no one
connects to, found by mdiobus_scan(). I knew this was taking place,
but forgot/didn't realize the connection with this patch set, and that
dummy phy_device was completely harmless until now.

I can suppress its creation like this:

From b1d1cd1625a27a62fd02598c7015b8ff0afdd28a Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 19 Jul 2022 20:15:52 +0300
Subject: [PATCH] net: dsa: ocelot: suppress PHY device scanning on the
 internal MDIO bus

This bus contains Lynx PCS devices, and if the lynx-pcs driver ever
decided to call mdio_device_register(), it would fail due to
mdiobus_scan() having created a dummy phydev for the same address
(the PCS responds to standard clause 22 PHY ID registers and can
therefore by autodetected by phylib which thinks it's a PHY).

On the Seville driver, things are a bit more complicated, since bus
creation is handled by mscc_miim_setup() and that is shared with the
dedicated mscc-miim driver. Suppress PHY scanning only for the Seville
internal MDIO bus rather than for the whole mscc-miim driver, since we
know that on NXP T1040, this bus only contains Lynx PCS devices.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c   | 4 ++++
 drivers/net/dsa/ocelot/seville_vsc9953.c | 6 +++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 927225e51f05..1ff71f1df316 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1062,6 +1062,10 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 	bus->read = enetc_mdio_read;
 	bus->write = enetc_mdio_write;
 	bus->parent = dev;
+	/* Suppress PHY device creation in mdiobus_scan(),
+	 * we have Lynx PCSs
+	 */
+	bus->phy_mask = ~0;
 	mdio_priv = bus->priv;
 	mdio_priv->hw = hw;
 	/* This gets added to imdio_regs, which already maps addresses
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 9006dec85ef0..9f400867fce3 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1018,12 +1018,16 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 	rc = mscc_miim_setup(dev, &bus, "VSC9953 internal MDIO bus",
 			     ocelot->targets[GCB],
 			     ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK]);
-
 	if (rc) {
 		dev_err(dev, "failed to setup MDIO bus\n");
 		return rc;
 	}
 
+	/* Suppress PHY device creation in mdiobus_scan(),
+	 * we have Lynx PCSs
+	 */
+	bus->phy_mask = ~0;
+
 	/* Needed in order to initialize the bus mutex lock */
 	rc = devm_of_mdiobus_register(dev, bus, NULL);
 	if (rc < 0) {
-- 
2.34.1

and then things start working (including traffic).

> +	if (err) {
> +		mdio_device_free(mdio);
> +		return ERR_PTR(err);
> +	}
> +
> +	pcs = pcs_get_by_provider(&mdio->dev);
> +	mdio_device_free(mdio);
> +	return pcs;
> +}
> +EXPORT_SYMBOL(lynx_pcs_create_on_bus);
> +
>  void lynx_pcs_destroy(struct phylink_pcs *pcs)
>  {
>  	pcs_put(pcs);
> diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h
> index 5712cc2ce775..1c14342bb8c4 100644
> --- a/include/linux/pcs-lynx.h
> +++ b/include/linux/pcs-lynx.h
> @@ -12,6 +12,7 @@
>  struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs);
>  
>  struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio);
> +struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr);
>  
>  void lynx_pcs_destroy(struct phylink_pcs *pcs);
>  
> -- 
> 2.35.1.1320.gc452695387.dirty
> 

Re: [RFC PATCH net-next 5/9] net: pcs: lynx: Use pcs_get_by_provider to get PCS

[Sean Anderson]

On 7/19/22 1:26 PM, Vladimir Oltean wrote:
> On Mon, Jul 11, 2022 at 12:05:15PM -0400, Sean Anderson wrote:
>> There is a common flow in several drivers where a lynx PCS is created
>> without a corresponding firmware node. Consolidate these into one helper
>> function. Because we control when the mdiodev is registered, we can add
>> a custom match function which will automatically bind our driver
>> (instead of using device_driver_attach).
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>>  drivers/net/dsa/ocelot/felix_vsc9959.c        | 25 ++++---------------
>>  drivers/net/dsa/ocelot/seville_vsc9953.c      | 25 ++++---------------
>>  .../net/ethernet/freescale/enetc/enetc_pf.c   | 21 +++-------------
>>  drivers/net/pcs/pcs-lynx.c                    | 24 ++++++++++++++++++
>>  include/linux/pcs-lynx.h                      |  1 +
>>  5 files changed, 39 insertions(+), 57 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
>> index 57634e2296c0..0a756c25d5e8 100644
>> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
>> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
>> @@ -11,6 +11,7 @@
>>  #include <net/tc_act/tc_gate.h>
>>  #include <soc/mscc/ocelot.h>
>>  #include <linux/dsa/ocelot.h>
>> +#include <linux/pcs.h>
>>  #include <linux/pcs-lynx.h>
>>  #include <net/pkt_sched.h>
>>  #include <linux/iopoll.h>
>> @@ -1089,16 +1090,9 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
>>  		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
>>  			continue;
>>  
>> -		mdio_device = mdio_device_create(felix->imdio, port);
>> -		if (IS_ERR(mdio_device))
>> +		phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, port);
>> +		if (IS_ERR(phylink_pcs))
>>  			continue;
>> -
>> -		phylink_pcs = lynx_pcs_create(mdio_device);
>> -		if (IS_ERR(phylink_pcs)) {
>> -			mdio_device_free(mdio_device);
>> -			continue;
>> -		}
>> -
>>  		felix->pcs[port] = phylink_pcs;
>>  
>>  		dev_info(dev, "Found PCS at internal MDIO address %d\n", port);
>> @@ -1112,17 +1106,8 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
>>  	struct felix *felix = ocelot_to_felix(ocelot);
>>  	int port;
>>  
>> -	for (port = 0; port < ocelot->num_phys_ports; port++) {
>> -		struct phylink_pcs *phylink_pcs = felix->pcs[port];
>> -		struct mdio_device *mdio_device;
>> -
>> -		if (!phylink_pcs)
>> -			continue;
>> -
>> -		mdio_device = lynx_get_mdio_device(phylink_pcs);
>> -		mdio_device_free(mdio_device);
>> -		lynx_pcs_destroy(phylink_pcs);
>> -	}
>> +	for (port = 0; port < ocelot->num_phys_ports; port++)
>> +		pcs_put(felix->pcs[port]);
>>  	mdiobus_unregister(felix->imdio);
>>  	mdiobus_free(felix->imdio);
>>  }
>> diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
>> index 8c52de5d0b02..9006dec85ef0 100644
>> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
>> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/mdio/mdio-mscc-miim.h>
>>  #include <linux/of_mdio.h>
>>  #include <linux/of_platform.h>
>> +#include <linux/pcs.h>
>>  #include <linux/pcs-lynx.h>
>>  #include <linux/dsa/ocelot.h>
>>  #include <linux/iopoll.h>
>> @@ -1044,16 +1045,9 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
>>  		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
>>  			continue;
>>  
>> -		mdio_device = mdio_device_create(felix->imdio, addr);
>> -		if (IS_ERR(mdio_device))
>> +		phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, addr);
>> +		if (IS_ERR(phylink_pcs))
>>  			continue;
>> -
>> -		phylink_pcs = lynx_pcs_create(mdio_device);
>> -		if (IS_ERR(phylink_pcs)) {
>> -			mdio_device_free(mdio_device);
>> -			continue;
>> -		}
>> -
>>  		felix->pcs[port] = phylink_pcs;
>>  
>>  		dev_info(dev, "Found PCS at internal MDIO address %d\n", addr);
>> @@ -1067,17 +1061,8 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
>>  	struct felix *felix = ocelot_to_felix(ocelot);
>>  	int port;
>>  
>> -	for (port = 0; port < ocelot->num_phys_ports; port++) {
>> -		struct phylink_pcs *phylink_pcs = felix->pcs[port];
>> -		struct mdio_device *mdio_device;
>> -
>> -		if (!phylink_pcs)
>> -			continue;
>> -
>> -		mdio_device = lynx_get_mdio_device(phylink_pcs);
>> -		mdio_device_free(mdio_device);
>> -		lynx_pcs_destroy(phylink_pcs);
>> -	}
>> +	for (port = 0; port < ocelot->num_phys_ports; port++)
>> +		pcs_put(felix->pcs[port]);
>>  
>>  	/* mdiobus_unregister and mdiobus_free handled by devres */
>>  }
>> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
>> index 8c923a93da88..8da7c8644e44 100644
>> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
>> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
>> @@ -8,6 +8,7 @@
>>  #include <linux/of_platform.h>
>>  #include <linux/of_mdio.h>
>>  #include <linux/of_net.h>
>> +#include <linux/pcs.h>
>>  #include <linux/pcs-lynx.h>
>>  #include "enetc_ierb.h"
>>  #include "enetc_pf.h"
>> @@ -827,7 +828,6 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>>  	struct device *dev = &pf->si->pdev->dev;
>>  	struct enetc_mdio_priv *mdio_priv;
>>  	struct phylink_pcs *phylink_pcs;
>> -	struct mdio_device *mdio_device;
>>  	struct mii_bus *bus;
>>  	int err;
>>  
>> @@ -851,16 +851,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>>  		goto free_mdio_bus;
>>  	}
>>  
>> -	mdio_device = mdio_device_create(bus, 0);
>> -	if (IS_ERR(mdio_device)) {
>> -		err = PTR_ERR(mdio_device);
>> -		dev_err(dev, "cannot create mdio device (%d)\n", err);
>> -		goto unregister_mdiobus;
>> -	}
>> -
>> -	phylink_pcs = lynx_pcs_create(mdio_device);
>> +	phylink_pcs = lynx_pcs_create_on_bus(bus, 0);
>>  	if (IS_ERR(phylink_pcs)) {
>> -		mdio_device_free(mdio_device);
>>  		err = PTR_ERR(phylink_pcs);
>>  		dev_err(dev, "cannot create lynx pcs (%d)\n", err);
>>  		goto unregister_mdiobus;
>> @@ -880,13 +872,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>>  
>>  static void enetc_imdio_remove(struct enetc_pf *pf)
>>  {
>> -	struct mdio_device *mdio_device;
>> -
>> -	if (pf->pcs) {
>> -		mdio_device = lynx_get_mdio_device(pf->pcs);
>> -		mdio_device_free(mdio_device);
>> -		lynx_pcs_destroy(pf->pcs);
>> -	}
>> +	if (pf->pcs)
>> +		pcs_put(pf->pcs);
>>  	if (pf->imdio) {
>>  		mdiobus_unregister(pf->imdio);
>>  		mdiobus_free(pf->imdio);
>> diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
>> index 8272072698e4..adb9fd5ce72e 100644
>> --- a/drivers/net/pcs/pcs-lynx.c
>> +++ b/drivers/net/pcs/pcs-lynx.c
>> @@ -403,6 +403,30 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
>>  }
>>  EXPORT_SYMBOL(lynx_pcs_create);
>>  
>> +struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr)
>> +{
>> +	struct mdio_device *mdio;
>> +	struct phylink_pcs *pcs;
>> +	int err;
>> +
>> +	mdio = mdio_device_create(bus, addr);
>> +	if (IS_ERR(mdio))
>> +		return ERR_CAST(mdio);
>> +
>> +	mdio->bus_match = mdio_device_bus_match;
>> +	strncpy(mdio->modalias, "lynx-pcs", sizeof(mdio->modalias));
>> +	err = mdio_device_register(mdio);
> 
> Yeah, so the reason why mdio_device_register() fails with -EBUSY for the
> PCS devices created by felix_vsc9959.c is this:
> 
> int mdiobus_register_device(struct mdio_device *mdiodev)
> {
> 	int err;
> 
> 	if (mdiodev->bus->mdio_map[mdiodev->addr])
> 		return -EBUSY;
> 
> In other words, we already have an existing mdiodev on the bus at
> address mdiodev->addr. Funnily enough, that device is actually us.
> It was created at MDIO bus creation time, a dummy phydev that no one
> connects to, found by mdiobus_scan(). I knew this was taking place,
> but forgot/didn't realize the connection with this patch set, and that
> dummy phy_device was completely harmless until now.
> 
> I can suppress its creation like this:
> 
> From b1d1cd1625a27a62fd02598c7015b8ff0afdd28a Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Tue, 19 Jul 2022 20:15:52 +0300
> Subject: [PATCH] net: dsa: ocelot: suppress PHY device scanning on the
>  internal MDIO bus
> 
> This bus contains Lynx PCS devices, and if the lynx-pcs driver ever
> decided to call mdio_device_register(), it would fail due to
> mdiobus_scan() having created a dummy phydev for the same address
> (the PCS responds to standard clause 22 PHY ID registers and can
> therefore by autodetected by phylib which thinks it's a PHY).
> 
> On the Seville driver, things are a bit more complicated, since bus
> creation is handled by mscc_miim_setup() and that is shared with the
> dedicated mscc-miim driver. Suppress PHY scanning only for the Seville
> internal MDIO bus rather than for the whole mscc-miim driver, since we
> know that on NXP T1040, this bus only contains Lynx PCS devices.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/ocelot/felix_vsc9959.c   | 4 ++++
>  drivers/net/dsa/ocelot/seville_vsc9953.c | 6 +++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)

Thanks! I'll pick this up for v2.

--Sean

[RFC PATCH net-next 6/9] net: pcs: lynx: Remove lynx_get_mdio_device and lynx_pcs_destroy

[Sean Anderson]

The PCS subsystem now manages getting/putting the PCS device. We can
convert our manual cleanup with a call to pcs_put. This removes the last
users of lynx_get_mdio_device lynx_pcs_destroy, so they can be removed.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/net/dsa/ocelot/felix_vsc9959.c           |  1 -
 drivers/net/dsa/ocelot/seville_vsc9953.c         |  1 -
 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 14 ++++----------
 drivers/net/pcs/pcs-lynx.c                       | 14 --------------
 include/linux/pcs-lynx.h                         |  4 ----
 5 files changed, 4 insertions(+), 30 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 0a756c25d5e8..16ff0052a8bf 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1082,7 +1082,6 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 	for (port = 0; port < felix->info->num_ports; port++) {
 		struct ocelot_port *ocelot_port = ocelot->ports[port];
 		struct phylink_pcs *phylink_pcs;
-		struct mdio_device *mdio_device;
 
 		if (dsa_is_unused_port(felix->ds, port))
 			continue;
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 9006dec85ef0..669af83c9611 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1036,7 +1036,6 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 	for (port = 0; port < felix->info->num_ports; port++) {
 		struct ocelot_port *ocelot_port = ocelot->ports[port];
 		struct phylink_pcs *phylink_pcs;
-		struct mdio_device *mdio_device;
 		int addr = port + 4;
 
 		if (dsa_is_unused_port(felix->ds, port))
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index e82c0d23eeb5..d8b491ffa4db 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -2,6 +2,7 @@
 /* Copyright 2019 NXP */
 
 #include <linux/acpi.h>
+#include <linux/pcs.h>
 #include <linux/pcs-lynx.h>
 #include <linux/phy/phy.h>
 #include <linux/property.h>
@@ -268,6 +269,7 @@ static int dpaa2_pcs_create(struct dpaa2_mac *mac,
 		return -EPROBE_DEFER;
 
 	mac->pcs = lynx_pcs_create(mdiodev);
+	mdio_device_free(mdiodev);
 	if (IS_ERR(mac->pcs)) {
 		netdev_err(mac->net_dev, "lynx_pcs_create() failed\n");
 		put_device(&mdiodev->dev);
@@ -279,16 +281,8 @@ static int dpaa2_pcs_create(struct dpaa2_mac *mac,
 
 static void dpaa2_pcs_destroy(struct dpaa2_mac *mac)
 {
-	struct phylink_pcs *phylink_pcs = mac->pcs;
-
-	if (phylink_pcs) {
-		struct mdio_device *mdio = lynx_get_mdio_device(phylink_pcs);
-		struct device *dev = &mdio->dev;
-
-		lynx_pcs_destroy(phylink_pcs);
-		put_device(dev);
-		mac->pcs = NULL;
-	}
+	pcs_put(mac->pcs);
+	mac->pcs = NULL;
 }
 
 static void dpaa2_mac_set_supported_interfaces(struct dpaa2_mac *mac)
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index adb9fd5ce72e..bfa72d9cbcf9 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -40,14 +40,6 @@ enum sgmii_speed {
 #define phylink_pcs_to_lynx(pl_pcs) container_of((pl_pcs), struct lynx_pcs, pcs)
 #define lynx_to_phylink_pcs(lynx) (&(lynx)->pcs)
 
-struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs)
-{
-	struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
-
-	return lynx->mdio;
-}
-EXPORT_SYMBOL(lynx_get_mdio_device);
-
 static void lynx_pcs_get_state_usxgmii(struct mdio_device *pcs,
 				       struct phylink_link_state *state)
 {
@@ -427,11 +419,5 @@ struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr)
 }
 EXPORT_SYMBOL(lynx_pcs_create_on_bus);
 
-void lynx_pcs_destroy(struct phylink_pcs *pcs)
-{
-	pcs_put(pcs);
-}
-EXPORT_SYMBOL(lynx_pcs_destroy);
-
 MODULE_DESCRIPTION("NXP Lynx 10G/28G PCS driver");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h
index 1c14342bb8c4..61caa59a069c 100644
--- a/include/linux/pcs-lynx.h
+++ b/include/linux/pcs-lynx.h
@@ -9,11 +9,7 @@
 #include <linux/mdio.h>
 #include <linux/phylink.h>
 
-struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs);
-
 struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio);
 struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr);
 
-void lynx_pcs_destroy(struct phylink_pcs *pcs);
-
 #endif /* __LINUX_PCS_LYNX_H */
-- 
2.35.1.1320.gc452695387.dirty

[RFC PATCH net-next 7/9] arm64: dts: Add compatible strings for Lynx PCSs

[Sean Anderson]

This adds appropriate compatible strings for Lynx PCSs on arm64 QorIQ
platforms. This also changes the node name to avoid warnings from
ethernet-phy.yaml.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This will break mEMACs. The DPAA driver needs at least something like
[1] before this is applied. This is because adding a non-phy compatible
string to something on an MDIO bus makes it a regular MDIO device and
not a phy. This will break the existing code, which expects a phy and
not an MDIO device.

[1] https://lore.kernel.org/netdev/20220628221404.1444200-1-sean.anderson@seco.com/T/#md2dee008cbb0962bc9e943426b2c02d2e64b6e3b

 .../arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 30 +++++++----
 .../arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 48 +++++++++++------
 .../arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 54 ++++++++++++-------
 .../dts/freescale/qoriq-fman3-0-10g-0.dtsi    |  3 +-
 .../dts/freescale/qoriq-fman3-0-10g-1.dtsi    |  3 +-
 .../dts/freescale/qoriq-fman3-0-1g-0.dtsi     |  3 +-
 .../dts/freescale/qoriq-fman3-0-1g-1.dtsi     |  3 +-
 .../dts/freescale/qoriq-fman3-0-1g-2.dtsi     |  3 +-
 .../dts/freescale/qoriq-fman3-0-1g-3.dtsi     |  3 +-
 .../dts/freescale/qoriq-fman3-0-1g-4.dtsi     |  3 +-
 .../dts/freescale/qoriq-fman3-0-1g-5.dtsi     |  3 +-
 11 files changed, 104 insertions(+), 52 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
index f476b7d8b056..259ca8f3f44d 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
@@ -791,7 +791,8 @@ pcs_mdio1: mdio@8c07000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs1: ethernet-phy@0 {
+			pcs1: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -804,7 +805,8 @@ pcs_mdio2: mdio@8c0b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs2: ethernet-phy@0 {
+			pcs2: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -817,19 +819,23 @@ pcs_mdio3: mdio@8c0f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs3_0: ethernet-phy@0 {
+			pcs3_0: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 
-			pcs3_1: ethernet-phy@1 {
+			pcs3_1: ethernet-pcs@1 {
+				compatible = "fsl,lynx-pcs";
 				reg = <1>;
 			};
 
-			pcs3_2: ethernet-phy@2 {
+			pcs3_2: ethernet-pcs@2 {
+				compatible = "fsl,lynx-pcs";
 				reg = <2>;
 			};
 
-			pcs3_3: ethernet-phy@3 {
+			pcs3_3: ethernet-pcs@3 {
+				compatible = "fsl,lynx-pcs";
 				reg = <3>;
 			};
 		};
@@ -842,19 +848,23 @@ pcs_mdio7: mdio@8c1f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs7_0: ethernet-phy@0 {
+			pcs7_0: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 
-			pcs7_1: ethernet-phy@1 {
+			pcs7_1: ethernet-pcs@1 {
+				compatible = "fsl,lynx-pcs";
 				reg = <1>;
 			};
 
-			pcs7_2: ethernet-phy@2 {
+			pcs7_2: ethernet-pcs@2 {
+				compatible = "fsl,lynx-pcs";
 				reg = <2>;
 			};
 
-			pcs7_3: ethernet-phy@3 {
+			pcs7_3: ethernet-pcs@3 {
+				compatible = "fsl,lynx-pcs";
 				reg = <3>;
 			};
 		};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
index 4ba1e0499dfd..130a96054ff9 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
@@ -545,7 +545,8 @@ pcs_mdio1: mdio@8c07000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs1: ethernet-phy@0 {
+			pcs1: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -558,7 +559,8 @@ pcs_mdio2: mdio@8c0b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs2: ethernet-phy@0 {
+			pcs2: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -571,7 +573,8 @@ pcs_mdio3: mdio@8c0f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs3: ethernet-phy@0 {
+			pcs3: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -584,7 +587,8 @@ pcs_mdio4: mdio@8c13000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs4: ethernet-phy@0 {
+			pcs4: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -597,7 +601,8 @@ pcs_mdio5: mdio@8c17000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs5: ethernet-phy@0 {
+			pcs5: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -610,7 +615,8 @@ pcs_mdio6: mdio@8c1b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs6: ethernet-phy@0 {
+			pcs6: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -623,7 +629,8 @@ pcs_mdio7: mdio@8c1f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs7: ethernet-phy@0 {
+			pcs7: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -636,7 +643,8 @@ pcs_mdio8: mdio@8c23000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs8: ethernet-phy@0 {
+			pcs8: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -649,7 +657,8 @@ pcs_mdio9: mdio@8c27000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs9: ethernet-phy@0 {
+			pcs9: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -662,7 +671,8 @@ pcs_mdio10: mdio@8c2b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs10: ethernet-phy@0 {
+			pcs10: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -675,7 +685,8 @@ pcs_mdio11: mdio@8c2f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs11: ethernet-phy@0 {
+			pcs11: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -688,7 +699,8 @@ pcs_mdio12: mdio@8c33000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs12: ethernet-phy@0 {
+			pcs12: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -701,7 +713,8 @@ pcs_mdio13: mdio@8c37000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs13: ethernet-phy@0 {
+			pcs13: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -714,7 +727,8 @@ pcs_mdio14: mdio@8c3b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs14: ethernet-phy@0 {
+			pcs14: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -727,7 +741,8 @@ pcs_mdio15: mdio@8c3f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs15: ethernet-phy@0 {
+			pcs15: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -740,7 +755,8 @@ pcs_mdio16: mdio@8c43000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs16: ethernet-phy@0 {
+			pcs16: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
index 47ea854720ce..b48b2d6acd3d 100644
--- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
@@ -1398,7 +1398,8 @@ pcs_mdio1: mdio@8c07000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs1: ethernet-phy@0 {
+			pcs1: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1411,7 +1412,8 @@ pcs_mdio2: mdio@8c0b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs2: ethernet-phy@0 {
+			pcs2: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1424,7 +1426,8 @@ pcs_mdio3: mdio@8c0f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs3: ethernet-phy@0 {
+			pcs3: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1437,7 +1440,8 @@ pcs_mdio4: mdio@8c13000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs4: ethernet-phy@0 {
+			pcs4: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1450,7 +1454,8 @@ pcs_mdio5: mdio@8c17000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs5: ethernet-phy@0 {
+			pcs5: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1463,7 +1468,8 @@ pcs_mdio6: mdio@8c1b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs6: ethernet-phy@0 {
+			pcs6: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1476,7 +1482,8 @@ pcs_mdio7: mdio@8c1f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs7: ethernet-phy@0 {
+			pcs7: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1489,7 +1496,8 @@ pcs_mdio8: mdio@8c23000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs8: ethernet-phy@0 {
+			pcs8: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1502,7 +1510,8 @@ pcs_mdio9: mdio@8c27000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs9: ethernet-phy@0 {
+			pcs9: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1515,7 +1524,8 @@ pcs_mdio10: mdio@8c2b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs10: ethernet-phy@0 {
+			pcs10: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1528,7 +1538,8 @@ pcs_mdio11: mdio@8c2f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs11: ethernet-phy@0 {
+			pcs11: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1541,7 +1552,8 @@ pcs_mdio12: mdio@8c33000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs12: ethernet-phy@0 {
+			pcs12: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1554,7 +1566,8 @@ pcs_mdio13: mdio@8c37000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs13: ethernet-phy@0 {
+			pcs13: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1567,7 +1580,8 @@ pcs_mdio14: mdio@8c3b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs14: ethernet-phy@0 {
+			pcs14: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1580,7 +1594,8 @@ pcs_mdio15: mdio@8c3f000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs15: ethernet-phy@0 {
+			pcs15: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1593,7 +1608,8 @@ pcs_mdio16: mdio@8c43000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs16: ethernet-phy@0 {
+			pcs16: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1606,7 +1622,8 @@ pcs_mdio17: mdio@8c47000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs17: ethernet-phy@0 {
+			pcs17: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
@@ -1619,7 +1636,8 @@ pcs_mdio18: mdio@8c4b000 {
 			#size-cells = <0>;
 			status = "disabled";
 
-			pcs18: ethernet-phy@0 {
+			pcs18: ethernet-pcs@0 {
+				compatible = "fsl,lynx-pcs";
 				reg = <0>;
 			};
 		};
diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi
index dbd2fc3ba790..4cf65e40126f 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi
@@ -35,7 +35,8 @@ mdio@f1000 {
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xf1000 0x1000>;
 
-		pcsphy6: ethernet-phy@0 {
+		pcsphy6: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi
index 6fc5d2560057..de483c7e9ae0 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi
@@ -35,7 +35,8 @@ mdio@f3000 {
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xf3000 0x1000>;
 
-		pcsphy7: ethernet-phy@0 {
+		pcsphy7: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-0.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-0.dtsi
index 4e02276fcf99..9c31b3b2292d 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-0.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-0.dtsi
@@ -34,7 +34,8 @@ mdio@e1000 {
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xe1000 0x1000>;
 
-		pcsphy0: ethernet-phy@0 {
+		pcsphy0: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-1.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-1.dtsi
index 0312fa43fa77..72dbb26c7fd4 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-1.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-1.dtsi
@@ -34,7 +34,8 @@ mdio@e3000 {
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xe3000 0x1000>;
 
-		pcsphy1: ethernet-phy@0 {
+		pcsphy1: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-2.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-2.dtsi
index af2df07971dd..e7aa07964d1c 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-2.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-2.dtsi
@@ -34,7 +34,8 @@ mdio@e5000 {
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xe5000 0x1000>;
 
-		pcsphy2: ethernet-phy@0 {
+		pcsphy2: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-3.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-3.dtsi
index 4ac98dc8b227..fb6b8d4eb786 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-3.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-3.dtsi
@@ -34,7 +34,8 @@ mdio@e7000 {
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xe7000 0x1000>;
 
-		pcsphy3: ethernet-phy@0 {
+		pcsphy3: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-4.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-4.dtsi
index bd932d8b0160..1d9cc79bf7e2 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-4.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-4.dtsi
@@ -34,7 +34,8 @@ mdio@e9000 {
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xe9000 0x1000>;
 
-		pcsphy4: ethernet-phy@0 {
+		pcsphy4: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-5.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-5.dtsi
index 7de1c5203f3e..b6151d6f6859 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-5.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-1g-5.dtsi
@@ -34,7 +34,8 @@ mdio@eb000 {
 		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
 		reg = <0xeb000 0x1000>;
 
-		pcsphy5: ethernet-phy@0 {
+		pcsphy5: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
-- 
2.35.1.1320.gc452695387.dirty

[RFC PATCH net-next 8/9] powerpc: dts: Add compatible strings for Lynx PCSs

[Sean Anderson]

This adds appropriate compatible strings for Lynx PCSs on PowerPC QorIQ
platforms. This also changes the node name to avoid warnings from
ethernet-phy.yaml.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0-best-effort.dtsi | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0.dtsi             | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1-best-effort.dtsi | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1.dtsi             | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-0.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-1.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-2.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-3.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-4.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-5.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-0.dtsi             | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-1.dtsi             | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-0.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-1.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-2.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-3.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-4.dtsi              | 3 ++-
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-5.dtsi              | 3 ++-
 18 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0-best-effort.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0-best-effort.dtsi
index baa0c503e741..b0bb58121440 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0-best-effort.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0-best-effort.dtsi
@@ -65,7 +65,8 @@ mdio@e1000 {
 		reg = <0xe1000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy0: ethernet-phy@0 {
+		pcsphy0: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0.dtsi
index 93095600e808..67765c49c6dd 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-0.dtsi
@@ -62,7 +62,8 @@ mdio@f1000 {
 		reg = <0xf1000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy6: ethernet-phy@0 {
+		pcsphy6: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1-best-effort.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1-best-effort.dtsi
index ff4bd38f0645..5b5f1768507f 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1-best-effort.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1-best-effort.dtsi
@@ -65,7 +65,8 @@ mdio@e3000 {
 		reg = <0xe3000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy1: ethernet-phy@0 {
+		pcsphy1: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1.dtsi
index 1fa38ed6f59e..c1b4a9cf8435 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-10g-1.dtsi
@@ -62,7 +62,8 @@ mdio@f3000 {
 		reg = <0xf3000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy7: ethernet-phy@0 {
+		pcsphy7: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-0.dtsi
index a8cc9780c0c4..f4f7445a261c 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-0.dtsi
@@ -61,7 +61,8 @@ mdio@e1000 {
 		reg = <0xe1000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy0: ethernet-phy@0 {
+		pcsphy0: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-1.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-1.dtsi
index 8b8bd70c9382..78bf1c1e09c2 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-1.dtsi
@@ -61,7 +61,8 @@ mdio@e3000 {
 		reg = <0xe3000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy1: ethernet-phy@0 {
+		pcsphy1: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-2.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-2.dtsi
index 619c880b54d8..432e3da63584 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-2.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-2.dtsi
@@ -61,7 +61,8 @@ mdio@e5000 {
 		reg = <0xe5000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy2: ethernet-phy@0 {
+		pcsphy2: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-3.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-3.dtsi
index d7ebb73a400d..a92d88685b91 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-3.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-3.dtsi
@@ -61,7 +61,8 @@ mdio@e7000 {
 		reg = <0xe7000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy3: ethernet-phy@0 {
+		pcsphy3: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-4.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-4.dtsi
index b151d696a069..af70c159d030 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-4.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-4.dtsi
@@ -61,7 +61,8 @@ mdio@e9000 {
 		reg = <0xe9000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy4: ethernet-phy@0 {
+		pcsphy4: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-5.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-5.dtsi
index adc0ae0013a3..da46fd9aab2e 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-5.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0-1g-5.dtsi
@@ -61,7 +61,8 @@ mdio@eb000 {
 		reg = <0xeb000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy5: ethernet-phy@0 {
+		pcsphy5: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-0.dtsi
index 435047e0e250..50c1b75c073f 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-0.dtsi
@@ -62,7 +62,8 @@ mdio@f1000 {
 		reg = <0xf1000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy14: ethernet-phy@0 {
+		pcsphy14: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-1.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-1.dtsi
index c098657cca0a..03ed8d83adde 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-10g-1.dtsi
@@ -62,7 +62,8 @@ mdio@f3000 {
 		reg = <0xf3000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy15: ethernet-phy@0 {
+		pcsphy15: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-0.dtsi
index 9d06824815f3..ced05a914e36 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-0.dtsi
@@ -61,7 +61,8 @@ mdio@e1000 {
 		reg = <0xe1000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy8: ethernet-phy@0 {
+		pcsphy8: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-1.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-1.dtsi
index 70e947730c4b..de6be3e6db36 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-1.dtsi
@@ -61,7 +61,8 @@ mdio@e3000 {
 		reg = <0xe3000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy9: ethernet-phy@0 {
+		pcsphy9: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-2.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-2.dtsi
index ad96e6529595..053cb568529e 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-2.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-2.dtsi
@@ -61,7 +61,8 @@ mdio@e5000 {
 		reg = <0xe5000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy10: ethernet-phy@0 {
+		pcsphy10: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-3.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-3.dtsi
index 034bc4b71f7a..448a73c24d1c 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-3.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-3.dtsi
@@ -61,7 +61,8 @@ mdio@e7000 {
 		reg = <0xe7000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy11: ethernet-phy@0 {
+		pcsphy11: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-4.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-4.dtsi
index 93ca23d82b39..5d388ab4268b 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-4.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-4.dtsi
@@ -61,7 +61,8 @@ mdio@e9000 {
 		reg = <0xe9000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy12: ethernet-phy@0 {
+		pcsphy12: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-5.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-5.dtsi
index 23b3117a2fd2..fb262d9e0c01 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-5.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1-1g-5.dtsi
@@ -61,7 +61,8 @@ mdio@eb000 {
 		reg = <0xeb000 0x1000>;
 		fsl,erratum-a011043; /* must ignore read errors */
 
-		pcsphy13: ethernet-phy@0 {
+		pcsphy13: ethernet-pcs@0 {
+			compatible = "fsl,lynx-pcs";
 			reg = <0x0>;
 		};
 	};
-- 
2.35.1.1320.gc452695387.dirty

[RFC PATCH net-next 9/9] net: pcs: lynx: Remove remaining users of lynx_pcs_create

[Sean Anderson]

Now that PCS devices have a compatible string, we no longer have to bind
the driver manually in lynx_pcs_create. Remove it, and convert the
remaining users to pcs_get_by_fwnode.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This requires that all PCSs have a compatible string. For a reasonable
window of compatibility, this should be applied one major release after
all compatible strings are added.

 .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 27 ++-----------------
 drivers/net/pcs/pcs-lynx.c                    | 19 -------------
 include/linux/pcs-lynx.h                      |  1 -
 3 files changed, 2 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index d8b491ffa4db..1c1cd935ec1d 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -247,32 +247,9 @@ static int dpaa2_pcs_create(struct dpaa2_mac *mac,
 			    struct fwnode_handle *dpmac_node,
 			    int id)
 {
-	struct mdio_device *mdiodev;
-	struct fwnode_handle *node;
-
-	node = fwnode_find_reference(dpmac_node, "pcs-handle", 0);
-	if (IS_ERR(node)) {
-		/* do not error out on old DTS files */
-		netdev_warn(mac->net_dev, "pcs-handle node not found\n");
-		return 0;
-	}
-
-	if (!fwnode_device_is_available(node)) {
-		netdev_err(mac->net_dev, "pcs-handle node not available\n");
-		fwnode_handle_put(node);
-		return -ENODEV;
-	}
-
-	mdiodev = fwnode_mdio_find_device(node);
-	fwnode_handle_put(node);
-	if (!mdiodev)
-		return -EPROBE_DEFER;
-
-	mac->pcs = lynx_pcs_create(mdiodev);
-	mdio_device_free(mdiodev);
+	mac->pcs = pcs_get_by_fwnode(dpmac_node, NULL);
 	if (IS_ERR(mac->pcs)) {
-		netdev_err(mac->net_dev, "lynx_pcs_create() failed\n");
-		put_device(&mdiodev->dev);
+		netdev_err(mac->net_dev, "pcs_get_by_fwnode() failed\n");
 		return PTR_ERR(mac->pcs);
 	}
 
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index bfa72d9cbcf9..f4a70a1a73fe 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -376,25 +376,6 @@ static struct mdio_driver lynx_pcs_driver = {
 };
 mdio_module_driver(lynx_pcs_driver);
 
-struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
-{
-	struct device *dev = &mdio->dev;
-	int err;
-
-	/* For compatibility with device trees lacking compatible strings, we
-	 * bind the device manually here.
-	 */
-	err = device_driver_attach(&lynx_pcs_driver.mdiodrv.driver, dev);
-	if (err && err != -EBUSY) {
-		if (err == -EAGAIN)
-			err = -EPROBE_DEFER;
-		return ERR_PTR(err);
-	}
-
-	return pcs_get_by_provider(&mdio->dev);
-}
-EXPORT_SYMBOL(lynx_pcs_create);
-
 struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr)
 {
 	struct mdio_device *mdio;
diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h
index 61caa59a069c..54dacabc0884 100644
--- a/include/linux/pcs-lynx.h
+++ b/include/linux/pcs-lynx.h
@@ -9,7 +9,6 @@
 #include <linux/mdio.h>
 #include <linux/phylink.h>
 
-struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio);
 struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr);
 
 #endif /* __LINUX_PCS_LYNX_H */
-- 
2.35.1.1320.gc452695387.dirty

Re: [RFC PATCH net-next 9/9] net: pcs: lynx: Remove remaining users of lynx_pcs_create

[Rob Herring]

On Mon, Jul 11, 2022 at 12:05:19PM -0400, Sean Anderson wrote:
> Now that PCS devices have a compatible string, we no longer have to bind
> the driver manually in lynx_pcs_create. Remove it, and convert the
> remaining users to pcs_get_by_fwnode.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> This requires that all PCSs have a compatible string. For a reasonable
> window of compatibility, this should be applied one major release after
> all compatible strings are added.

These platforms are pretty stable. I don't think a 1 release window is 
sufficient. Maybe a 1 LTS release.

> 
>  .../net/ethernet/freescale/dpaa2/dpaa2-mac.c  | 27 ++-----------------
>  drivers/net/pcs/pcs-lynx.c                    | 19 -------------
>  include/linux/pcs-lynx.h                      |  1 -
>  3 files changed, 2 insertions(+), 45 deletions(-)

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Vladimir Oltean]

Hi Sean,

On Mon, Jul 11, 2022 at 12:05:10PM -0400, Sean Anderson wrote:
> For a long time, PCSs have been tightly coupled with their MACs. For
> this reason, the MAC creates the "phy" or mdio device, and then passes
> it to the PCS to initialize. This has a few disadvantages:
> 
> - Each MAC must re-implement the same steps to look up/create a PCS
> - The PCS cannot use functions tied to device lifetime, such as devm_*.
> - Generally, the PCS does not have easy access to its device tree node
> 
> I'm not sure if these are terribly large disadvantages. In fact, I'm not
> sure if this series provides any benefit which could not be achieved
> with judicious use of helper functions. In any case, here it is.
> 
> NB: Several (later) patches in this series should not be applied. See
> the notes in each commit for details on when they can be applied.

Sorry to burst your bubble, but the networking drivers on NXP LS1028A
(device tree at arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, drivers
at drivers/net/ethernet/freescale/enetc/ and drivers/net/dsa/ocelot/)
do not use the Lynx PCS through a pcs-handle, because the Lynx PCS in
fact has no backing OF node there, nor do the internal MDIO buses of the
ENETC and of the switch.

It seems that I need to point this out explicitly: you need to provide
at least a working migration path to your PCS driver model. Currently
there isn't one, and as a result, networking is broken on the LS1028A
with this patch set.

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Sean Anderson]

Hi Vladimir,

On 7/19/22 11:25 AM, Vladimir Oltean wrote:
> Hi Sean,
> 
> On Mon, Jul 11, 2022 at 12:05:10PM -0400, Sean Anderson wrote:
>> For a long time, PCSs have been tightly coupled with their MACs. For
>> this reason, the MAC creates the "phy" or mdio device, and then passes
>> it to the PCS to initialize. This has a few disadvantages:
>> 
>> - Each MAC must re-implement the same steps to look up/create a PCS
>> - The PCS cannot use functions tied to device lifetime, such as devm_*.
>> - Generally, the PCS does not have easy access to its device tree node
>> 
>> I'm not sure if these are terribly large disadvantages. In fact, I'm not
>> sure if this series provides any benefit which could not be achieved
>> with judicious use of helper functions. In any case, here it is.
>> 
>> NB: Several (later) patches in this series should not be applied. See
>> the notes in each commit for details on when they can be applied.
> 
> Sorry to burst your bubble, but the networking drivers on NXP LS1028A
> (device tree at arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, drivers
> at drivers/net/ethernet/freescale/enetc/ and drivers/net/dsa/ocelot/)
> do not use the Lynx PCS through a pcs-handle, because the Lynx PCS in
> fact has no backing OF node there, nor do the internal MDIO buses of the
> ENETC and of the switch.
> 
> It seems that I need to point this out explicitly: you need to provide
> at least a working migration path to your PCS driver model. Currently
> there isn't one, and as a result, networking is broken on the LS1028A
> with this patch set.
> 

Please refer to patches 4, 5, and 6.

--Sean

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Vladimir Oltean]

On Tue, Jul 19, 2022 at 11:28:42AM -0400, Sean Anderson wrote:
> Hi Vladimir,
> 
> On 7/19/22 11:25 AM, Vladimir Oltean wrote:
> > Hi Sean,
> > 
> > On Mon, Jul 11, 2022 at 12:05:10PM -0400, Sean Anderson wrote:
> >> For a long time, PCSs have been tightly coupled with their MACs. For
> >> this reason, the MAC creates the "phy" or mdio device, and then passes
> >> it to the PCS to initialize. This has a few disadvantages:
> >> 
> >> - Each MAC must re-implement the same steps to look up/create a PCS
> >> - The PCS cannot use functions tied to device lifetime, such as devm_*.
> >> - Generally, the PCS does not have easy access to its device tree node
> >> 
> >> I'm not sure if these are terribly large disadvantages. In fact, I'm not
> >> sure if this series provides any benefit which could not be achieved
> >> with judicious use of helper functions. In any case, here it is.
> >> 
> >> NB: Several (later) patches in this series should not be applied. See
> >> the notes in each commit for details on when they can be applied.
> > 
> > Sorry to burst your bubble, but the networking drivers on NXP LS1028A
> > (device tree at arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, drivers
> > at drivers/net/ethernet/freescale/enetc/ and drivers/net/dsa/ocelot/)
> > do not use the Lynx PCS through a pcs-handle, because the Lynx PCS in
> > fact has no backing OF node there, nor do the internal MDIO buses of the
> > ENETC and of the switch.
> > 
> > It seems that I need to point this out explicitly: you need to provide
> > at least a working migration path to your PCS driver model. Currently
> > there isn't one, and as a result, networking is broken on the LS1028A
> > with this patch set.
> > 
> 
> Please refer to patches 4, 5, and 6.

I don't understand, could you be more clear? Are you saying that I
shouldn't have applied patch 9 while testing? When would be a good
moment to apply patch 9?

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Sean Anderson]

On 7/19/22 11:38 AM, Vladimir Oltean wrote:
> On Tue, Jul 19, 2022 at 11:28:42AM -0400, Sean Anderson wrote:
>> Hi Vladimir,
>> 
>> On 7/19/22 11:25 AM, Vladimir Oltean wrote:
>> > Hi Sean,
>> > 
>> > On Mon, Jul 11, 2022 at 12:05:10PM -0400, Sean Anderson wrote:
>> >> For a long time, PCSs have been tightly coupled with their MACs. For
>> >> this reason, the MAC creates the "phy" or mdio device, and then passes
>> >> it to the PCS to initialize. This has a few disadvantages:
>> >> 
>> >> - Each MAC must re-implement the same steps to look up/create a PCS
>> >> - The PCS cannot use functions tied to device lifetime, such as devm_*.
>> >> - Generally, the PCS does not have easy access to its device tree node
>> >> 
>> >> I'm not sure if these are terribly large disadvantages. In fact, I'm not
>> >> sure if this series provides any benefit which could not be achieved
>> >> with judicious use of helper functions. In any case, here it is.
>> >> 
>> >> NB: Several (later) patches in this series should not be applied. See
>> >> the notes in each commit for details on when they can be applied.
>> > 
>> > Sorry to burst your bubble, but the networking drivers on NXP LS1028A
>> > (device tree at arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, drivers
>> > at drivers/net/ethernet/freescale/enetc/ and drivers/net/dsa/ocelot/)
>> > do not use the Lynx PCS through a pcs-handle, because the Lynx PCS in
>> > fact has no backing OF node there, nor do the internal MDIO buses of the
>> > ENETC and of the switch.
>> > 
>> > It seems that I need to point this out explicitly: you need to provide
>> > at least a working migration path to your PCS driver model. Currently
>> > there isn't one, and as a result, networking is broken on the LS1028A
>> > with this patch set.
>> > 
>> 
>> Please refer to patches 4, 5, and 6.
> 
> I don't understand, could you be more clear? Are you saying that I
> shouldn't have applied patch 9 while testing? When would be a good
> moment to apply patch 9?

I'm saying that patches 4 and 5 [1] provide "...a working migration
path to [my] PCS driver model." Since enetc/ocelot do not use
devicetree for the PCS, patch 9 should have no effect.

That said, if you've tested this on actual hardware, I'm interested
in your results. I do not have access to enetc/ocelot hardware, so
I was unable to test whether my proposed migration would work.

--Sean

[1] I listed 6 but it seems like it just has some small hunks which should have been in 5 instead

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Vladimir Oltean]

On Tue, Jul 19, 2022 at 11:46:23AM -0400, Sean Anderson wrote:
> I'm saying that patches 4 and 5 [1] provide "...a working migration
> path to [my] PCS driver model." Since enetc/ocelot do not use
> devicetree for the PCS, patch 9 should have no effect.
> 
> That said, if you've tested this on actual hardware, I'm interested
> in your results. I do not have access to enetc/ocelot hardware, so
> I was unable to test whether my proposed migration would work.
> 
> --Sean
> 
> [1] I listed 6 but it seems like it just has some small hunks which should have been in 5 instead

Got it, thanks. So things actually work up until the end, after fixing
the compilation errors and warnings and applying my phy_mask patch first.
However, as mentioned by Russell King, this patch set now gives us the
possibility of doing this, which happily kills the system:

echo "0000:00:00.5-imdio:03" > /sys/bus/mdio_bus/drivers/lynx-pcs/unbind

For your information, pcs-rzn1-miic.c already has a device_link_add()
call to its consumer, and it does avoid the unbinding problem. It is a
bit of a heavy hammer as Russell points out (a DSA switch is a single
struct device, but has multiple net_devices and phylink instances, and
the switch device would be unregistered in its entirety), but on the
other hand, this is one of the simpler things we can do, until we have
something more fine-grained. I, for one, am perfectly happy with a
device link. The alternative would be reworking phylink to react on PCS
devices coming and going. I don't even know what the implications are
upon mac_select_pcs() and such...

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Sean Anderson]

On 7/19/22 2:11 PM, Vladimir Oltean wrote:
> On Tue, Jul 19, 2022 at 11:46:23AM -0400, Sean Anderson wrote:
>> I'm saying that patches 4 and 5 [1] provide "...a working migration
>> path to [my] PCS driver model." Since enetc/ocelot do not use
>> devicetree for the PCS, patch 9 should have no effect.
>> 
>> That said, if you've tested this on actual hardware, I'm interested
>> in your results. I do not have access to enetc/ocelot hardware, so
>> I was unable to test whether my proposed migration would work.
>> 
>> --Sean
>> 
>> [1] I listed 6 but it seems like it just has some small hunks which should have been in 5 instead
> 
> Got it, thanks. So things actually work up until the end, after fixing
> the compilation errors and warnings and applying my phy_mask patch first.
> However, as mentioned by Russell King, this patch set now gives us the
> possibility of doing this, which happily kills the system:
> 
> echo "0000:00:00.5-imdio:03" > /sys/bus/mdio_bus/drivers/lynx-pcs/unbind
> 
> For your information, pcs-rzn1-miic.c already has a device_link_add()
> call to its consumer, and it does avoid the unbinding problem. It is a
> bit of a heavy hammer as Russell points out (a DSA switch is a single
> struct device, but has multiple net_devices and phylink instances, and
> the switch device would be unregistered in its entirety), but on the
> other hand, this is one of the simpler things we can do, until we have
> something more fine-grained. I, for one, am perfectly happy with a
> device link. The alternative would be reworking phylink to react on PCS
> devices coming and going. I don't even know what the implications are
> upon mac_select_pcs() and such...

We could do it, but it'd be a pretty big hack. Something like the
following. Phylink would need to be modified to grab the lock before
every op and check if the PCS is dead or not. This is of course still
not optimal, since there's no way to re-attach a PCS once it goes away.

IMO a better solution is to use devlink and submit a patch to add
notifications which the MAC driver can register for. That way it can
find out when the PCS goes away and potentially do something about it
(or just let itself get removed).

---
 drivers/net/pcs/core.c     | 115 +++++++++++++++++++++++++++----------
 drivers/net/pcs/pcs-lynx.c |  20 +++----
 include/linux/pcs.h        |  23 +++++++-
 include/linux/phylink.h    |  19 +-----
 4 files changed, 117 insertions(+), 60 deletions(-)

diff --git a/drivers/net/pcs/core.c b/drivers/net/pcs/core.c
index 782a4cdd19b2..46e4168802db 100644
--- a/drivers/net/pcs/core.c
+++ b/drivers/net/pcs/core.c
@@ -10,42 +10,83 @@
 #include <linux/phylink.h>
 #include <linux/property.h>
 
+struct phylink_pcs {
+	struct mutex lock;
+	struct list_head list;
+	struct device *dev;
+	void *priv;
+	const struct phylink_pcs_ops *ops;
+	int refs;
+	bool dead;
+	bool poll;
+};
+
 static LIST_HEAD(pcs_devices);
 static DEFINE_MUTEX(pcs_mutex);
 
 /**
  * pcs_register() - register a new PCS
- * @pcs: the PCS to register
+ * @init: Initialization data for a new PCS
  *
  * Registers a new PCS which can be automatically attached to a phylink.
  *
- * Return: 0 on success, or -errno on error
+ * Return: A new PCS, or an error pointer
  */
-int pcs_register(struct phylink_pcs *pcs)
+struct phylink_pcs *pcs_register(struct device *dev,
+				 struct pcs_init *init)
 {
-	if (!pcs->dev || !pcs->ops)
-		return -EINVAL;
-	if (!pcs->ops->pcs_an_restart || !pcs->ops->pcs_config ||
-	    !pcs->ops->pcs_get_state)
-		return -EINVAL;
+	struct phylink_pcs *pcs;
 
+	if (!init->ops)
+		return ERR_PTR(-EINVAL);
+	if (!init->ops->pcs_an_restart || !init->ops->pcs_config ||
+	    !init->ops->pcs_get_state)
+		return ERR_PTR(-EINVAL);
+
+	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
+	if (!pcs)
+		return ERR_PTR(-ENOMEM);
+
+	pcs->dev = dev;
+	pcs->priv = init->priv;
+	pcs->ops = init->ops;
+	pcs->poll = init->poll;
+	pcs->refs = 1;
+	mutex_init(&pcs->lock);
 	INIT_LIST_HEAD(&pcs->list);
+
 	mutex_lock(&pcs_mutex);
 	list_add(&pcs->list, &pcs_devices);
 	mutex_unlock(&pcs_mutex);
-	return 0;
+	return pcs;
 }
 EXPORT_SYMBOL_GPL(pcs_register);
 
+static void pcs_free(struct phylink_pcs *pcs)
+{
+	int refs;
+
+	refs = --pcs->refs;
+	mutex_unlock(&pcs->lock);
+	if (refs)
+		return;
+
+	WARN_ON(!pcs->dead);
+	mutex_lock(&pcs_mutex);
+	list_del(&pcs->list);
+	mutex_unlock(&pcs_mutex);
+	kfree(pcs);
+}
+
 /**
  * pcs_unregister() - unregister a PCS
  * @pcs: a PCS previously registered with pcs_register()
  */
 void pcs_unregister(struct phylink_pcs *pcs)
 {
-	mutex_lock(&pcs_mutex);
-	list_del(&pcs->list);
-	mutex_unlock(&pcs_mutex);
+	mutex_lock(&pcs->lock);
+	pcs->dead = true;
+	pcs_free(pcs);
 }
 EXPORT_SYMBOL_GPL(pcs_unregister);
 
@@ -65,26 +106,25 @@ static void devm_pcs_release(struct device *dev, void *res)
  *
  * Return: 0 on success, or -errno on failure
  */
-int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs)
+struct phylink_pcs *devm_pcs_register(struct device *dev,
+				      struct pcs_init *init)
 {
 	struct phylink_pcs **pcsp;
-	int ret;
+	struct phylink_pcs *pcs;
 
 	pcsp = devres_alloc(devm_pcs_release, sizeof(*pcsp),
 			    GFP_KERNEL);
 	if (!pcsp)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
-	ret = pcs_register(pcs);
-	if (ret) {
+	pcs = pcs_register(dev, init);
+	if (IS_ERR(pcs)) {
 		devres_free(pcsp);
-		return ret;
+	} else {
+		*pcsp = pcs;
+		devres_add(dev, pcsp);
 	}
-
-	*pcsp = pcs;
-	devres_add(dev, pcsp);
-
-	return ret;
+	return pcs;
 }
 EXPORT_SYMBOL_GPL(devm_pcs_register);
 
@@ -106,16 +146,20 @@ static struct phylink_pcs *pcs_find(const struct fwnode_handle *fwnode,
 
 	mutex_lock(&pcs_mutex);
 	list_for_each_entry(pcs, &pcs_devices, list) {
-		if (dev && pcs->dev == dev)
-			goto out;
-		if (fwnode && pcs->dev->fwnode == fwnode)
-			goto out;
+		mutex_lock(&pcs->lock);
+		if (!pcs->dead) {
+			if (dev && pcs->dev == dev)
+				goto out;
+			if (fwnode && pcs->dev->fwnode == fwnode)
+				goto out;
+		}
+		mutex_unlock(&pcs->lock);
 	}
 	pcs = NULL;
 
 out:
 	mutex_unlock(&pcs_mutex);
-	pr_devel("%s: looking for %pfwf or %s %s...%s found\n", __func__,
+	pr_debug("%s: looking for %pfwf or %s %s...%s found\n", __func__,
 		 fwnode, dev ? dev_driver_string(dev) : "(null)",
 		 dev ? dev_name(dev) : "(null)", pcs ? " not" : "");
 	return pcs;
@@ -132,10 +176,15 @@ static struct phylink_pcs *pcs_find(const struct fwnode_handle *fwnode,
  */
 static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs)
 {
+	bool got_module;
+
 	if (!pcs)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	if (!try_module_get(pcs->ops->owner))
+	got_module = try_module_get(pcs->ops->owner);
+	pcs->refs += got_module;
+	mutex_unlock(&pcs->lock);
+	if (!got_module)
 		return ERR_PTR(-ENODEV);
 	get_device(pcs->dev);
 
@@ -222,5 +271,13 @@ void pcs_put(struct phylink_pcs *pcs)
 
 	put_device(pcs->dev);
 	module_put(pcs->ops->owner);
+	mutex_lock(&pcs->lock);
+	pcs_free(pcs);
 }
 EXPORT_SYMBOL_GPL(pcs_put);
+
+void *pcs_get_priv(struct phylink_pcs *pcs)
+{
+	return pcs->priv;
+}
+EXPORT_SYMBOL_GPL(pcs_get_priv);
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index c3e2c4a6fab6..f792f2a7cdf2 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -26,7 +26,6 @@
 #define IF_MODE_HALF_DUPLEX		BIT(4)
 
 struct lynx_pcs {
-	struct phylink_pcs pcs;
 	struct mdio_device *mdio;
 };
 
@@ -37,8 +36,7 @@ enum sgmii_speed {
 	SGMII_SPEED_2500	= 2,
 };
 
-#define phylink_pcs_to_lynx(pl_pcs) container_of((pl_pcs), struct lynx_pcs, pcs)
-#define lynx_to_phylink_pcs(lynx) (&(lynx)->pcs)
+#define phylink_pcs_to_lynx(pl_pcs) pcs_get_priv(pl_pcs)
 
 static void lynx_pcs_get_state_usxgmii(struct mdio_device *pcs,
 				       struct phylink_link_state *state)
@@ -318,21 +316,23 @@ static const struct phylink_pcs_ops lynx_pcs_phylink_ops = {
 static int lynx_pcs_probe(struct mdio_device *mdio)
 {
 	struct device *dev = &mdio->dev;
+	struct phylink_pcs *pcs;
 	struct lynx_pcs *lynx;
-	int ret;
+	struct pcs_init init;
 
 	lynx = devm_kzalloc(dev, sizeof(*lynx), GFP_KERNEL);
 	if (!lynx)
 		return -ENOMEM;
 
 	lynx->mdio = mdio;
-	lynx->pcs.dev = dev;
-	lynx->pcs.ops = &lynx_pcs_phylink_ops;
-	lynx->pcs.poll = true;
+	init.priv = lynx;
+	init.ops = &lynx_pcs_phylink_ops;
+	init.poll = true;
 
-	ret = devm_pcs_register(dev, &lynx->pcs);
-	if (ret)
-		return dev_err_probe(dev, ret, "could not register PCS\n");
+	pcs = devm_pcs_register(dev, &init);
+	if (IS_ERR(pcs))
+		return dev_err_probe(dev, PTR_ERR(pcs),
+				     "could not register PCS\n");
 	dev_info(dev, "probed\n");
 	return 0;
 }
diff --git a/include/linux/pcs.h b/include/linux/pcs.h
index 00e76594e03c..2605603149ec 100644
--- a/include/linux/pcs.h
+++ b/include/linux/pcs.h
@@ -6,12 +6,27 @@
 #ifndef _PCS_H
 #define _PCS_H
 
-struct phylink_pcs;
 struct fwnode;
+struct phylink_pcs;
+struct phylink_pcs_ops;
 
-int pcs_register(struct phylink_pcs *pcs);
+/**
+ * struct pcs_init - PCS initialization data
+ * @priv: the device's private data
+ * @ops: a pointer to the &struct phylink_pcs_ops structure
+ * @poll: poll the PCS for link changes
+ */
+struct pcs_init {
+	void *priv;
+	const struct phylink_pcs_ops *ops;
+	bool poll;
+};
+
+struct phylink_pcs *pcs_register(struct device *dev,
+				 struct pcs_init *init);
 void pcs_unregister(struct phylink_pcs *pcs);
-int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs);
+struct phylink_pcs *devm_pcs_register(struct device *dev,
+				      struct pcs_init *init);
 struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode,
 				       const char *id, bool optional);
 struct phylink_pcs *pcs_get_by_provider(const struct device *dev);
@@ -30,4 +45,6 @@ static inline struct phylink_pcs
 	return _pcs_get_by_fwnode(fwnode, id, true);
 }
 
+void *pcs_get_priv(struct phylink_pcs *pcs);
+
 #endif /* PCS_H */
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index a713e70108a1..864536d1b293 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -392,24 +392,7 @@ void mac_link_up(struct phylink_config *config, struct phy_device *phy,
 		 int speed, int duplex, bool tx_pause, bool rx_pause);
 #endif
 
-struct phylink_pcs_ops;
-
-/**
- * struct phylink_pcs - PHYLINK PCS instance
- * @dev: the device associated with this PCS
- * @ops: a pointer to the &struct phylink_pcs_ops structure
- * @list: internal list of PCS devices
- * @poll: poll the PCS for link changes
- *
- * This structure is designed to be embedded within the PCS private data,
- * and will be passed between phylink and the PCS.
- */
-struct phylink_pcs {
-	struct device *dev;
-	const struct phylink_pcs_ops *ops;
-	struct list_head list;
-	bool poll;
-};
+struct phylink_pcs;
 
 /**
  * struct phylink_pcs_ops - MAC PCS operations structure.
-- 
2.35.1.1320.gc452695387.dirty

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Vladimir Oltean]

On Tue, Jul 19, 2022 at 03:34:45PM -0400, Sean Anderson wrote:
> We could do it, but it'd be a pretty big hack. Something like the
> following. Phylink would need to be modified to grab the lock before
> every op and check if the PCS is dead or not. This is of course still
> not optimal, since there's no way to re-attach a PCS once it goes away.

You assume it's just phylink who operates on a PCS structure, but if you
include your search pool to also cover include/linux/pcs/pcs-xpcs.h,
you'll see a bunch of exported functions which are called directly by
the client drivers (stmmac, sja1105). At this stage it gets pretty hard
to validate that drivers won't attempt from any code path to do
something stupid with a dead PCS. All in all it creates an environment
with insanely weak guarantees; that's pretty hard to get behind IMO.

> IMO a better solution is to use devlink and submit a patch to add
> notifications which the MAC driver can register for. That way it can
> find out when the PCS goes away and potentially do something about it
> (or just let itself get removed).

Not sure I understand what connection there is between devlink (device
links) and PCS {de}registration notifications. We could probably add those
notifications without any intervention from the device core: we would
just need to make this new PCS "core" to register an blocking_notifier_call_chain
to which interested drivers could add their notifier blocks. How a
certain phylink user is going to determine that "hey, this PCS is
definitely mine and I can use it" is an open question. In any case, my
expectation is that we have a notifier chain, we can at least continue
operating (avoid unbinding the struct device), but essentially move our
phylink_create/phylink_destroy calls to within those notifier blocks.

Again, retrofitting this model to existing drivers, phylink API (and
maybe even its internal structure) is something that's hard to hop on
board of; I think it's a solution waiting for a problem, and I don't
have an interest to develop or even review it.

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Sean Anderson]

On 7/20/22 9:53 AM, Vladimir Oltean wrote:
> On Tue, Jul 19, 2022 at 03:34:45PM -0400, Sean Anderson wrote:
>> We could do it, but it'd be a pretty big hack. Something like the
>> following. Phylink would need to be modified to grab the lock before
>> every op and check if the PCS is dead or not. This is of course still
>> not optimal, since there's no way to re-attach a PCS once it goes away.
> 
> You assume it's just phylink who operates on a PCS structure, but if you
> include your search pool to also cover include/linux/pcs/pcs-xpcs.h,
> you'll see a bunch of exported functions which are called directly by
> the client drivers (stmmac, sja1105). At this stage it gets pretty hard
> to validate that drivers won't attempt from any code path to do
> something stupid with a dead PCS. All in all it creates an environment
> with insanely weak guarantees; that's pretty hard to get behind IMO.

Right. To do this properly, we'd need wrapper functions for all the PCS
operations. And the super-weak guarantees is why I referred to this as a
"hack". But we could certainly make it so that removing a PCS might not
bring down the MAC.

>> IMO a better solution is to use devlink and submit a patch to add
>> notifications which the MAC driver can register for. That way it can
>> find out when the PCS goes away and potentially do something about it
>> (or just let itself get removed).
> 
> Not sure I understand what connection there is between devlink (device
> links) and PCS {de}registration notifications. 

The default action when a supplier is going to be removed is to remove
the consumers. However, it'd be nice to notify the consumer beforehand.
If we used device links, this would need to be integrated (since otherwise
we'd only find out that a PCS was gone after the MAC was gone too).

> We could probably add those
> notifications without any intervention from the device core: we would
> just need to make this new PCS "core" to register an blocking_notifier_call_chain
> to which interested drivers could add their notifier blocks. How a> certain phylink user is going to determine that "hey, this PCS is
> definitely mine and I can use it" is an open question. In any case, my
> expectation is that we have a notifier chain, we can at least continue
> operating (avoid unbinding the struct device), but essentially move our
> phylink_create/phylink_destroy calls to within those notifier blocks.
> Again, retrofitting this model to existing drivers, phylink API (and
> maybe even its internal structure) is something that's hard to hop on
> board of; I think it's a solution waiting for a problem, and I don't
> have an interest to develop or even review it.

I don't either. I'd much rather just bring down the whole MAC when any
PCS gets removed. Whatever we decide on doing here should also be done
for (serdes) phys as well, since they have all the same pitfalls. For
that reason I'd rather use a generic, non-intrusive solution like device
links. I know Russell mentioned composite devices, but I think those
would have similar advantages/drawbacks as a device-link-based solution
(unbinding of one device unbinds the rest).

--Sean

Re: [RFC PATCH net-next 0/9] net: pcs: Add support for devices probed in the "usual" manner

[Sean Anderson]

On 7/21/22 5:41 PM, Sean Anderson wrote:
> On 7/20/22 9:53 AM, Vladimir Oltean wrote:
>> On Tue, Jul 19, 2022 at 03:34:45PM -0400, Sean Anderson wrote:
>>> We could do it, but it'd be a pretty big hack. Something like the
>>> following. Phylink would need to be modified to grab the lock before
>>> every op and check if the PCS is dead or not. This is of course still
>>> not optimal, since there's no way to re-attach a PCS once it goes away.
>> 
>> You assume it's just phylink who operates on a PCS structure, but if you
>> include your search pool to also cover include/linux/pcs/pcs-xpcs.h,
>> you'll see a bunch of exported functions which are called directly by
>> the client drivers (stmmac, sja1105). At this stage it gets pretty hard
>> to validate that drivers won't attempt from any code path to do
>> something stupid with a dead PCS. All in all it creates an environment
>> with insanely weak guarantees; that's pretty hard to get behind IMO.
> 
> Right. To do this properly, we'd need wrapper functions for all the PCS
> operations. And the super-weak guarantees is why I referred to this as a
> "hack". But we could certainly make it so that removing a PCS might not
> bring down the MAC.
> 
>>> IMO a better solution is to use devlink and submit a patch to add
>>> notifications which the MAC driver can register for. That way it can
>>> find out when the PCS goes away and potentially do something about it
>>> (or just let itself get removed).
>> 
>> Not sure I understand what connection there is between devlink (device
>> links) and PCS {de}registration notifications. 
> 
> The default action when a supplier is going to be removed is to remove
> the consumers. However, it'd be nice to notify the consumer beforehand.
> If we used device links, this would need to be integrated (since otherwise
> we'd only find out that a PCS was gone after the MAC was gone too).
> 
>> We could probably add those
>> notifications without any intervention from the device core: we would
>> just need to make this new PCS "core" to register an blocking_notifier_call_chain
>> to which interested drivers could add their notifier blocks. How a> certain phylink user is going to determine that "hey, this PCS is
>> definitely mine and I can use it" is an open question. In any case, my
>> expectation is that we have a notifier chain, we can at least continue
>> operating (avoid unbinding the struct device), but essentially move our
>> phylink_create/phylink_destroy calls to within those notifier blocks.
>> Again, retrofitting this model to existing drivers, phylink API (and
>> maybe even its internal structure) is something that's hard to hop on
>> board of; I think it's a solution waiting for a problem, and I don't
>> have an interest to develop or even review it.
> 
> I don't either. I'd much rather just bring down the whole MAC when any
> PCS gets removed. Whatever we decide on doing here should also be done
> for (serdes) phys as well, since they have all the same pitfalls. For
> that reason I'd rather use a generic, non-intrusive solution like device
> links. I know Russell mentioned composite devices, but I think those
> would have similar advantages/drawbacks as a device-link-based solution
> (unbinding of one device unbinds the rest).

OK, I've thought about this a bit more. Right now, you can crash the
kernel by unbinding a phy (either serdes or networking) and waiting for
a state change. The serdes problem could probably be solved by
strengthening the existing device_link_add calls. This will of course
unbind the netdev if you unbind the serdes. I think this is not a common
use case; if a user does this they probably know what they're doing (or
not).

The problem with ethernet phys is a bit worse. It's very common to have
something like

	+ netdev
	|
	+-+ mdiobus
	  |
	  +-- phy

which poses a problem for device links. The phy is a child of the
netdev, so it should be unbound first. device_link_add will see this and
refuse to create a link, since such a link implies that netdev should be
unbound before phy.

One solution might be something like:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a74b320f5b27..05894e1c3e59 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -27,6 +27,7 @@
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
 #include <linux/property.h>
+#include <linux/rtnetlink.h>
 #include <linux/sfp.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
@@ -3111,6 +3112,13 @@ static int phy_remove(struct device *dev)
 {
        struct phy_device *phydev = to_phy_device(dev);
 
+	// I'm pretty sure this races with multiple unbinds...
+       rtnl_lock();
+       device_unlock(dev);
+       dev_close(phydev->attached_dev);
+       device_lock(dev);
+       rtnl_unlock();
+       WARN_ON(phydev->attached_dev);
+
        cancel_delayed_work_sync(&phydev->state_queue);
 
        mutex_lock(&phydev->lock);

which is probably the least intrusive we can get. But this isn't very
nice for PCSs.

First, PCSs are not always used by netdevs. So there's no generic way to
ask the user "please clean up this PCS." Additionally, most PCS users
expect the PCS to be around for the lifetime of the driver (or at least
until they're done using it).

Maybe the best solution is to provide some helper functions to use with
bus_register_notifier and just let the drivers fend for themselves. Or
possibly default to a devlink, but allow registering a notifier instead.

--Sean