Re: [RFC PATCH net-next 5/8] net: phy: realtek: use phy_read_paged instead of open coding

[Daniel Golle]

On Sat, Apr 22, 2023 at 05:11:57PM +0200, Heiner Kallweit wrote:
> On 22.04.2023 13:48, Daniel Golle wrote:
> > Instead of open coding a paged read, use the phy_read_paged function
> > in rtlgen_supports_2_5gbps.
> > 
> > Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> > ---
> >  drivers/net/phy/realtek.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index f97b5e49fae58..62fb965b6d338 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -735,9 +735,7 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
> >  {
> >  	int val;
> >  
> > -	phy_write(phydev, RTL821x_PAGE_SELECT, 0xa61);
> > -	val = phy_read(phydev, 0x13);
> > -	phy_write(phydev, RTL821x_PAGE_SELECT, 0);
> > +	val = phy_read_paged(phydev, 0xa61, 0x13);
> >  
> >  	return val >= 0 && val & RTL_SUPPORTS_2500FULL;
> >  }
> 
> I remember I had a reason to open-code it, it took me some minutes
> to recall it.
> phy_read_paged() calls __phy_read_page() that relies on phydev->drv
> being set. phydev->drv is set in phy_probe(). And probing is done
> after matching. __phy_read_paged() should have given you a warning.
> Did you test this patch? If yes and you didn't get the warning,
> then apparently I miss something.
>

Yes, you are right, this change was a bit too naive and causes a
NULL pointer dereference e.g. for the r8169 driver which also uses
the RealTek Ethernet PHY driver.
My main concern and original motivation was the lack of mutex protection
for the paged read operation. I suggest to rather make this change
instead: