Date:   Tue, 16 Nov 2021 17:56:43 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Parshuram Thombare <pthombar@...ence.com>,
        Antoine Tenart <atenart@...nel.org>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Milind Parab <mparab@...ence.com>
Subject: Re: [net-next PATCH v6] net: macb: Fix several edge cases in validate

Hi Russell,

On 11/15/21 11:44 AM, Russell King (Oracle) wrote:
>
> Thanks - this looks good to me.
>
> Reviewed-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
>
> I've added it to my tree as I have patches that follow on which
> entirely eliminate macb_validate(), replacing it with the generic
> implementation that was merged into net-next today.

I have a few questions/comments about your tree (and pl in general).
This is not particularly relevant to the above patch, but this is as
good a place as any to ask.

What is the intent for the supported link modes in validate()? The docs
say

> Note that the PHY may be able to transform from one connection
> technology to another, so, eg, don't clear 1000BaseX just
> because the MAC is unable to BaseX mode. This is more about
> clearing unsupported speeds and duplex settings. The port modes
> should not be cleared; phylink_set_port_modes() will help with this.

But this is not how validate() has been/is currently implemented in many
drivers. In 34ae2c09d46a ("net: phylink: add generic validate
implementation"), it appears you are hewing closer to the documented
purpose (e.g. MAC_1000FD selects all the full-duplex 1G link modes).
Should new code try to stick to the above documentation?

Of course, the above leaves me quite confused about where the best place
is to let the PCS have a say about what things are supported, and (as
discussed below) whether it can support such a thing. The general
perspective taken in existing drivers seems to be that PCSs are
integrated with the MAC. This is in contrast to the IEEE specs, which
take the pespective that the PCS is a part of the PHY. It's unclear to
me what stance the above documentation takes.

Consider the Xilinx 1G PCS. This PCS supports 1000BASE-X and SGMII, but
only at full duplex. This caveat already rules out a completely
bitmap-based solution (as phylink_get_linkmodes thinks that both of
those interfaces are always full-duplex). There are also config options
which (either as a feature or a consequence) disable SPEED_10 SGMII or
autonegotiation (although I don't plan on supporting either of those).
The "easiest" solution is simply to provide two callbacks like

	void pcs_set_modes(struct phylink_pcs *pcs, ulong *supported,
			   phy_interface_t interface);
	bool pcs_mode_supported(struct phylink_pcs *pcs,
				phy_interface_t interface, int speed,
				int duplex);

perhaps with some generic substitutes. The former would need to be
called from mac_validate, and the latter from mac_select_pcs/
mac_prepare. This design is rather averse to genericization, so perhaps
you have some suggestion?

On the subject of PCS selection, mac_select_pcs should supply the whole
state. This is because the interface alone is insufficient to determine
which PCS to select. For example, a PCS which supports full duplex but
not half duplex should not be selected if the config specifies half
duplex. Additionally, it should also support a selection of "no pcs".
Otherwise MACs which (optionally!) have PCSs will fail to configure. We
should not fail when no PCS has yet been selected or when there is no
PCS at all in some hardware configuration.  Further, why do we have this
callback in the first place? Why don't we have drivers just do this in
prepare()?

--Sean

Powered by blists - more mailing lists