Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Alvin Šipraga]

On Mon, Jun 06, 2022 at 02:31:16PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 06, 2022 at 03:01:30PM +0200, Alvin Šipraga wrote:
> > From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
> > 
> > phylib defaults to GMII when no phy-mode or phy-connection-type property
> > is specified in a DSA port node.
> > 
> > Commit a5dba0f207e5 ("net: dsa: rtl8365mb: add GMII as user port mode")
> > introduced implicit support for GMII mode on ports with internal PHY to
> > allow a PHY connection for device trees where the phy-mode is not
> > explicitly set to "internal".
> > 
> > Commit 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> > then broke this behaviour by discarding the usage of
> > rtl8365mb_phy_mode_supported() - where this GMII support was indicated -
> > while switching to the new .phylink_get_caps API.
> > 
> > With the new API, rtl8365mb_phy_mode_supported() is no longer needed.
> > Remove it altogether and add back the GMII capability - this time to
> > rtl8365mb_phylink_get_caps() - so that the above default behaviour works
> > for ports with internal PHY again.
> 
> Oops - I guess this has been caused by the delay between my patch being
> initially prepared, it sitting around in my tree for many months while
> other patches get merged, and it eventually seeing the light of day.
> 
> Sorry about that.

No problem, it's actually my bad, but I am still getting to grips with the
responsibility of maintaining a Linux driver :)

> 
> > Fixes: 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> > Signed-off-by: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
> > ---
> > 
> > Luiz, Russel:
> > 
> > Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> > claims to have been fixing a regression in the net-next tree - is that
> > right? I seem to have missed both referenced commits when they were
> > posted and never hit this issue personally. I only found things now
> > during some other refactoring and the test for GMII looked weird to me
> > so I went and investigated.
> > 
> > Could you please help me identify that Fixes: tag? Just for my own
> > understanding of what caused this added requirement for GMII on ports
> > with internal PHY.
> 
> I have absolutely no idea. I don't think any "requirement" has ever been
> added - phylib has always defaulted to GMII, so as the driver stood when
> it was first submitted on Oct 18 2021, I don't see how it could have
> worked, unless the DT it was being tested with specified a phy-mode of
> "internal". As you were the one who submitted it, you would have a
> better idea.
> 
> The only suggestion I have is to bisect to find out exactly what caused
> the GMII vs INTERNAL issue to crop up.

Alright, thanks for the quick response. Maybe Luiz has a better idea, otherwise
I will try bisecting if I find the time.

For what it's worth, I believe the patch is correct and applicable in its
current form - it just lacks an explanation as to why this ever worked in the
first place.

> 
> > 
> > ---
> >  drivers/net/dsa/realtek/rtl8365mb.c | 38 +++++++----------------------
> >  1 file changed, 9 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> > index 3bb42a9f236d..769f672e9128 100644
> > --- a/drivers/net/dsa/realtek/rtl8365mb.c
> > +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> > @@ -955,35 +955,21 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
> >  	return 0;
> >  }
> >  
> > -static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
> > -					 phy_interface_t interface)
> > -{
> > -	int ext_int;
> > -
> > -	ext_int = rtl8365mb_extint_port_map[port];
> > -
> > -	if (ext_int < 0 &&
> > -	    (interface == PHY_INTERFACE_MODE_NA ||
> > -	     interface == PHY_INTERFACE_MODE_INTERNAL ||
> > -	     interface == PHY_INTERFACE_MODE_GMII))
> > -		/* Internal PHY */
> > -		return true;
> > -	else if ((ext_int >= 1) &&
> > -		 phy_interface_mode_is_rgmii(interface))
> > -		/* Extension MAC */
> > -		return true;
> > -
> > -	return false;
> > -}
> > -
> >  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
> >  				       struct phylink_config *config)
> >  {
> > -	if (dsa_is_user_port(ds, port))
> > +	if (dsa_is_user_port(ds, port)) {
> 
> Given the updates to rtl8365mb_phy_mode_supported(), this misses out on
> the check of rtl8365mb_extint_port_map[port] introduced in commit
> 6147631c079f ("net: dsa: realtek: rtl8365mb: allow non-cpu extint
> ports").

You are right, but since this is targetting net and not net-next, I figured I
would not introduce any additional changes. I have a separate series lined up
for net-next (just submitted) which fixes this in a much cleaner way.

My rationale here is: yes, this test is incorrect and is a shortcoming of the
driver, but it does not represent a real bug worthy of inclusion in the stable
trees.

> 
> >  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> >  			  config->supported_interfaces);
> > -	else if (dsa_is_cpu_port(ds, port))
> > +
> > +		/* GMII is the default interface mode for phylib, so
> > +		 * we have to support it for ports with integrated PHY.
> > +		 */
> > +		__set_bit(PHY_INTERFACE_MODE_GMII,
> > +			  config->supported_interfaces);
> > +	} else if (dsa_is_cpu_port(ds, port)) {
> 
> This test also needs to be updated.
> 
> Not sure what rtl8365mb_extint_port_map[port] == 0 is supposed to
> signify - maybe port unusable? Looks that way to me.

Check the other series I just sent for a cleaner implementation of this.