Re: [PATCH net-next 10/16] net: pcs: xpcs: Add generic DW XPCS MDIO-device support

[Serge Semin]

Hi Vladimir

On Tue, Dec 05, 2023 at 02:23:16PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 05, 2023 at 02:35:46PM +0300, Serge Semin wrote:
> > Omg, thank you very much for testing the series straight away and
> > sorry for the immediate trouble it caused. I'll need some more time
> > for investigation. I'll get back to this topic a bit later on this
> > week.
> 
> Don't worry, I got suspicious when I was CCed to review only a one-line
> change in patch 11/16. It's never about that one line, is it?)

Right. I should have added you to the list of recipients of the entire
series since the patchset changes more than that. The bug you caught
brightly highlights my mistake. I'll make sure you are in the list.
I'll add Jiawen and Mengyuan there too since the driver under their
maintenance is also affected. Hopefully they'll get to test the series
too.

> 
> Anyway, the NULL dev->p is a symptom of device_add() not having been
> called, most likely from mdio_device_register().

Absolutely right. I thought that mdio_device_create() having the
device_initialize() method called was enough for the device_attach()
function being happy. It turns out it wasn't and I missed that the
device_private instance is allocated only on the device registration.
So I'll need to call mdio_device_register() after all, if I get to
preserve the current design of the solution.

> 
> I'll be honest and say that I still don't quite understand what you're
> trying to achieve. You're trying to bind the hardcoded mdio_devices
> created by xpcs_create() to a driver? 

My idea was to reuse the mdio_device which has already been created
either by means of the MDIO-bus OF-subnode or by means of the MDIO-bus
board_info infrastructure (can be utilized in the SJA1105 or Wangxun
Tx GBE). The xpcs_create() method then either probes the device on the MDIO
bus and gets ID from there, or just uses the custom IDs based on the
OF compatible match table or on the platform_data. If no MDIO-device
was created my patchset is supposed to preserve the previous
semantics: create MDIO-device, probe the device on the MDIO-bus, get
device IDs from there. See the next patch for more details:
https://lore.kernel.org/netdev/20231205103559.9605-11-fancer.lancer@xxxxxxxxx/

> That was attempted a while ago by
> Sean Anderson with the Lynx PCS. Are you aware of the fact that even in
> the good case in which binding the driver actually works, the user can
> then come along and unbind it from the PCS device, and phylink isn't
> prepared to handle that, so it will crash the kernel upon the next
> phylink_pcs call?

To be honest I didn't consider the driver bind/unbind option. But my
case a bit different. DW XPCS MDIO-device is supposed to be created
automatically by means of the DW XPCS MI driver from the DT-nodes
hierarchy like this:
mdio@1f05d000 {
	compatible = "snps,dw-xpcs-mi";
	reg = <0 0x1f05d000 0 0x1000>;

	xgmac_pcs: ethernet-pcs@0 {
		compatible = "snps,dw-xpcs";
		reg = <0>;
	};
};
The platform-device is created for the mdio@1f05d000 node for which
the DW XPCS MI driver is loaded, which calls the
devm_of_mdiobus_register() in the probe() method which registers the
MDIO-bus and then creates the MDIO-device from the ethernet-pcs@0
node. The DW XPCS MDIO-device driver is attached to that MDIO-device
then. In such model the PCS can be supplied to the DW *MAC via the
"pcs-handle = &xgmac_pcs" property.

Regarding the current semantics it's preserved in the framework of the
xpcs_create_byaddr() method (former xpcs_create_mdiodev()) by means of
the next code snippet:
        if (mdiobus_is_registered_device(bus, addr)) {
                mdiodev = bus->mdio_map[addr];
                mdio_device_get(mdiodev);
        } else {
                mdiodev = mdio_device_create(bus, addr);
                if (IS_ERR(mdiodev))
                        return ERR_CAST(mdiodev);
        }
Device can be automatically created if before registering the MDIO-bus
the xpcs_create_byaddr() caller registered the MDIO-device board info
by means of the mdiobus_register_board_info() method. In addition to
that it's now possible to supply some custom data (custom device IDs
in my implementation) to the XPCS driver by means of the
mdio_board_info.platform_data field. See the next patch for
reference:
https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@xxxxxxxxx

So what the difference with the Lynx PCS is that in my case the
MDIO-device is created automatically as a result of the DW XPCS MI
MDIO-bus registration. Additionally I implemented the MDIO-device
creation based on the MDIO-board-info, thus there won't be need in the
calling mdio_device_create() on each xpcs_create_mdiodev() invocation.
The later part isn't that important in the framework of this
conversation, but just so you be aware.

Regarding the driver bind/unbind. As I said I didn't actually consider
that option. On the other hand my DW XPCS MDIO-device driver doesn't
do actual probe() or remove(). The only implemented thing is the
of_device_id table, which is used to assign PCS and PMA IDs if
required based on the DT compatible property. So I can easily drop any
MDIO device-driver part and parse the of_device_id table right in the
xpcs_create_bynode(). From that perspective my implementation won't
differ much from the Lynx PCS design. The only difference will be is
the way the MDIO-bus is created and registered. In case of Lynx PCS
the bus is created by the MAC-driver itself. In my case DW XPCS MI is
currently created in the framework of the separate platform driver. Do
you think it would be better to follow the Lynx design pattern in
order to get rid from the possibility of the DW XPCS MI driver being
unbound behind the STMMAC+XPCS couple back?

In this case the Dw MAC DT-node hierarchy would look like this:

xgmac: ethernet@1f054000 {
	compatible = "snps,dwxgmac";
	reg = <0 0x1f054000 0 0x4000>;
	reg-names = "stmmaceth";
	ranges;

	...

	pcs-handle = &xgmac_pcs;

	// DW XPCS MI to access the DW XPCS attached to the device
	mdio@1f05d000 {
		compatible = "snps,dwmac-mi";
		reg = <0 0x1f05d000 0 0x1000>;

		xgmac_pcs: ethernet-pcs@0 {
			compatible = "snps,dw-xpcs";
			reg = <0>;
		};
	};

	// Normal MDIO-bus to access external PHYs (it's also called
	// as SMA - Station Management Agent - by Synopsys)
	mdio {
		compatible = "snps,dwmac-mdio";
		#address-cells = <1>;
		#size-cells = <0>;
	};
};

I actually thought to use that hardware description pattern instead,
but after some meditation around that I decided that having the DW
XPCS device defined separately from the DW MAC node seemed better at
least from the code separation point of view. Now I think that it
wasn't the best decision. DW XPCS is always attached to the DW XGMAC
controller. So it would be more correct having it defined as a
sub-node. It would also helped to avoid the platform device driver
bind/unbind problem.

What do you think? Should I re-design my patchset to be supporting the
design above? (After having conversion with you I am more inclined to
do that now than to stick with the currently implemented solution.)

> 
> The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least
> tear down the whole thing when the PCS is unbound, which is saner than
> crashing the kernel. I don't see the equivalent protection mechanism here?

You are right. I don't have any equivalent protection here. Thanks for
suggesting a solution.

> 
> Can't the xpcs continue to live without a bound driver? Having a
> compatible string in the OF description is perfectly fine though,
> and should absolutely not preclude that.

As I explained above Dw XPCS device can live without a bound driver
because the DW XPCS MDIO-driver doesn't do much but merely gets to be
bound based on the of_device_id table. In my case the problem is in
the DW XPCS MI driver which indeed can be detached. Please see my
long-read text above.

-Serge(y)