Message-ID: <e86ad436-cb35-d059-6ce6-f88f50b4b676@seco.com>
Date:   Mon, 22 Nov 2021 16:06:58 -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/22/21 2:15 PM, Russell King (Oracle) wrote:
> Hi Sean,
>
> Today's update...
>
> On Fri, Nov 19, 2021 at 06:56:09PM +0000, Russell King (Oracle) wrote:
>> > Say that you are a MAC with an integrated PCS (so you handle everything
>> > in the MAC driver). You support GMII full and half duplex, but your PCS
>> > only supports 1000BASE-X with full duplex. The naïve bitmap is
>> >
>> > supported_interfaces = PHY_INTERFACE_GMII | PHY_INTERFACE_1000BASEX;
>> > mac_capabilities = MAC_10 | MAC_100 | MAC_1000;
>> >
>> > but this will report 1000BASE-X as supporting both full and half duplex.
>> > So you still need a custom validate() in order to report the correct link
>> > modes.
>>
>> First, let me say that I've now been through all phylink users in the
>> kernel, converting them, and there are seven cases where the generic
>> helper can't be used. The validate() method isn't going away (at least
>> not just yet) which allows unexpected situations to still be handled.
>> The current state of this conversion is as follows.
>>
>> In terms of network drivers:
>>
>> mvneta and mvpp2: the generic helper is used but needs to be wrapped
>>   since these specify that negotiation must not be disabled in
>>   "1000BASE-X" mode, which includes the up-clocked 2500BASE-X.
>
> Over the weekend, I've been able to eliminate these two with the
> mac_select_pcs() method, and adding a pcs_validate() callback to the
> PCS operations. The updated patches are in the net-queue branch.

That looks good. It looks effectively the same as what I'd prototyped.

>> > > > Right now, "no pcs" is really not an option I'm afraid. The presence
>> > > > of a PCS changes the phylink behaviour slightly . This is one of my
>> > > > bug-bears. The evolution of phylink has meant that we need to keep
>> > > > compatibility with how phylink used to work before we split the PCS
>> > > > support - and we detect that by whether there is a PCS to determine
>> > > > whether we need to operate with that compatibility. It probably was
>> > > > a mistake to do that in hind sight.
>> >
>> > Of course it's an option :)
>>
>> I'm saying that with phylink as it is, phylink does _not_ support there
>> being no PCS in a non-legacy driver, since the presence of a PCS is a
>> flag that the driver is non-legacy. It changes the circumstances in
>> which the mac_config() method is called.
>>
>> If we want the PCS to become optional, then we need phylink to deal with
>> detecting non-legacy drivers some other way (e.g. a legacy-mode flag in
>> struct phylink_config), or we need to eliminate the legacy drivers by
>> either converting them (which is always my preference) or deleting them.
>
> ... and over the weekend, I've implemented a flag "legacy_pre_march2020"
> which is used to flag which drivers predate the changes to add PCS
> support, allowing phylink to know whether they are legacy drivers or
> not irrespective of whether they use a PCS.

Thanks for clarifying this. It was always a little unclear to me exactly
what the implications of unregistering a PCS were.

> So, I've updated the "mac_select_pcs" patch to allow NULL as a valid
> return, and use error-pointers to indicate failure.
>
> Then there's a following patch that adds a "pcs_validate" method which
> gives the PCS an opportunity to have its say in the MAC validation
> without the MAC having code to call the PCS - this change allowed mvneta
> and mvpp2 to use phylink_generic_validate().
>
> So, this is another step towards the end goal.
>
> I'm planning to get lkp chew on the "legacy_pre_march2020" before I send
> those out to netdev (which will be the next batch of patches), and then
> will be the patches that add the ground work to allow DSA drivers to
> become properly non-legacy.
>
> Please have a look at my net-queue - are you happier with the
> mac_select_pcs method now?

Yes. This looks like it can be implemented reasonably. I'm going to hold
off on working on Xilinx PCS for now and wait until your changes have
made their way upstream.

> Once we've got this settled, and remaining drivers converted, my plan is
> to kill off the old .validate method entirely. At that point,
> "mac_capabilities" then becomes exactly that - the capabilities of the
> MAC block irrespective of anything else.
>
> Given bcm_sf2, I may need to add a method which allows the MAC to modify
> those capabilities depending on the interface - bcm_sf2's pause frame
> capabilities appear to be dependent on the interface mode. I'm tempted
> to reverse the logic I have in bcm_sf2 in these patches - set
> MAC_ASYM_PAUSE | MAC_SYM_PAUSE in mac_capabilities, and exclude them for
> interface types that don't support pause frames.

Yeah. I like how phylink_get_linkmodes allows for modifying the
capabilities if necessary.

Thanks for reworking this stuff. I think this will make it a lot easier
to get my changes upstreamed :)

--Sean

Powered by blists - more mailing lists