Re: [PATCH v2 1/2] net: macb: Clean up macb_validate

On 10/25/21 6:35 AM, Russell King (Oracle) wrote:

On Fri, Oct 22, 2021 at 01:37:34PM -0400, Sean Anderson wrote:

quoted

Hi Russell,

For "net: phy: add phy_interface_t bitmap support", phylink_or would be
nice as well. I use it when implementing NA support for PCSs.

I think you actually mean phy_interface_or(). Given that we will need
MAC drivers to provide the union of their PCS support, I think that
would be a sensible addition. Thanks.

quoted

For "net: sfp: augment SFP parsing with phy_interface_t bitmap",
drivers/net/phy/marvell.c also needs to be converted. This is due to
b697d9d38a5a ("net: phy: marvell: add SFP support for 88E1510") being
added to net-next/master.

(I think you have fixed this in your latest revision)

I haven't - but when I move the patch series onto net-next, that will
need to be updated.

quoted

"net: phylink: use supported_interfaces for phylink validation" looks
good. Though the documentation should be updated. Perhaps something
like

Yes, I haven't bothered with the doc updates yet... they will need to
be done before the patches are ready. Thanks for the suggestions though.

quoted

I think "net: macb: populate supported_interfaces member" is wrong.
Gigabit modes should be predicated on GIGABIT_MODE_AVAILABLE.

It is a conversion of what macb_validate() does - if the conversion is
incorrect, then macb_validate() is incorrect.

If MACB_CAPS_GIGABIT_MODE_AVAILABLE isn't set, but MACB_CAPS_HIGH_SPEED
and MACB_CAPS_PCS are both set, macb_validate() will not zero the
supported mask if e.g. PHY_INTERFACE_MODE_10GBASER is requested - it
will indicate 10baseT and 100baseT speeds are supported. So the current
macb_validate() code basically tells phylink that
PHY_INTERFACE_MODE_10GBASER supports 10baseT and 100baseT speeds!

This probably is not what is intended, but this is what the code does,
and I'm maintaining bug-compatibility with the current macb_validate()
implementation. Any changes to the behaviour should be a separate
patch - either fixing it before this patch, or fixing it afterwards.
As the series is currently based on v5.14, it may be that this has
already been fixed.

Ugh. This sort of thing is what I wanted to address in the first place.
The current logic lends itself well to these sorts of errors.

--Sean