Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

Hi,

On Fri, 2019-02-15 at 10:53 -0800, Florian Fainelli wrote:
> On 2/15/19 10:34 AM, Paul Kocialkowski wrote:
> > As I was mentionning to Andrew in the initial submission of this patch,
> > this driver is a bit unusual since it represents a GMII to RGMII
> > bridge, so it's not actually a PHY driver on its own -- it just sticks
> > itself in between the actual PHY and the MAC.
> 
> Yes, my bad, you should still consider checking priv->phy_drv though, if
> someone unbinds the PHY driver on either side, you are toast.

Thanks for the suggestion! So I had a closer look at that driver to try
and see what could go wrong and it looks like I found a few things
there.

First, we have:
> priv->phy_dev->priv = priv;

Which basically overwrites that target PHY driver's priv with the
gmii2rgmii driver's. It looks like most PHY drivers don't use their
priv data so much so it kind of works in practice. But that's still a
receipe for disaster. I don't really see an immediate easy fix for that
one.

It might help to do things the other way round and bind the gmii2rgmii
PHY to the MAC, which itself would bind the actual PHY. That way we can
just have the gmii2rgmii redirect all ops to the actual PHY, except for
read_status. Maybe there was a reason I'm not seeing for doing things
the way they are done now though?

Then, it looks like there is no way for the gmii2rgmii driver to know
whether the PHY driver is still alive. It gets a pointer to the initial
priv->phy_dev->drv and then overwrites it. So when the target driver is
removed, the pointer will still be alive. Perhaps the memory backing it
will have been freed too.

How realistic does this scneario sound? I guess there are not many
cases where the PHY driver will be unregistered once it was picked up
by the gmii2rgmii driver, but I'm pretty new to the subsystem.

Cheers,

Paul

> > > >  drivers/net/phy/xilinx_gmii2rgmii.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > index 74a8782313cf..bd6084e315de 100644
> > > > --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> > > > @@ -44,7 +44,10 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
> > > >  	u16 val = 0;
> > > >  	int err;
> > > >  
> > > > -	err = priv->phy_drv->read_status(phydev);
> > > > +	if (priv->phy_drv->read_status)
> > > > +		err = priv->phy_drv->read_status(phydev);
> > > > +	else
> > > > +		err = genphy_read_status(phydev);
> > > >  	if (err < 0)
> > > >  		return err;
> > > >  
> > > > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com