Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

[Quentin Schulz]

Hi Russel,

Adding you to the discussion as you're the author and commiter of the
patch adding support for all the paged access in the phy core.

On Fri, Sep 14, 2018 at 03:29:59PM +0200, Andrew Lunn wrote:
> > When you change a page, you basically can access only the registers in
> > this page so if there are two functions requesting different pages at
> > the same time or registers of different pages, it won't work well
> > indeed.
> > 
> > > phy_read_page() and phy_write_page() will do the needed locking if
> > > this is an issue.
> > > 
> > 
> > That's awesome! Didn't know it existed. Thanks a ton!
> > 
> > Well, that means I should migrate the whole driver to use
> > phy_read/write_paged instead of the phy_read/write that is currently in
> > use.
> > 
> > That's impacting performance though as per phy_read/write_paged we read
> > the current page, set the desired page, read/write the register, set the
> > old page back. That's 4 times more operations.
> 
> You can use the lower level locking primatives. See m88e1318_set_wol()
> for example.
> 

I'm converting the drivers/net/phy/mscc.c driver to make use of the
paged accesses but I'm hitting something confusing to me.

Firstly, just to be sure, I should use paged accesses only for read/write
outside of the standard page, right? I'm guessing that because we need
to be able to use the genphy functions which are using phy_write/read
and not __phy_write/read, thus we assume the mdio lock is not taken
(which is taken by phy_select/restore_page) and those functions
read/write to the standard page.

Secondly, I should refactor the driver to do the following:

oldpage = phy_select_page();
if (oldpage < 0) {
	phy_restore_page();
	error_path;
}

[...]
/* paged accesses */
__phy_write/read();
[...]

phy_restore_page();

I assume this is the correct way to handle paged accesses. Let me know
if it's not clear enough or wrong. (depending on the function, we could
of course put phy_restore_page in the error_path).

Now, I saw that phy_restore_page takes the phydev, the oldpage and a ret
parameters[1].

The (ret >= 0 && r < 0) condition of [2] seems counterintuitive to me.

ret being the argument passed to the function and r being the return of
__phy_write_page (which is phydev->drv->phy_write_page()).

In my understanding of C best practices, any return value equal to zero
marks a successful call to the function.

That would mean that with:
if (ret >= 0 && r < 0)
	ret = r;

If ret is greather than 0, if __phy_write_page is successful (r == 0),
ret will be > 0, which would result in phy_restore_page not returning 0
thus signaling (IMO) an error occured in phy_restore_page.

One example is the following:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage);

If phy_select_page is successful, return phy_restore_page(phydev,
oldpage, oldpage) would return the value of oldpage which can be
different from 0.

This code could (I think) be working with `if (ret >= 0 && r <= 0)` (or
even `if (ret >= 0)`).

Now to have the same behaviour, I need to do:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage > 0 ? 0 : oldpage);

Another example is:
oldpage = phy_select_page(phydev, new_page);
ret = `any function returning a value > 0 in case of success and < 0 in
case of failure`().
return phy_restore_page(phydev, oldpage, ret);

Is there any reason for not wanting to overwrite the ret value when
__phy_write_page is successful in phy_restore_page?

I'd say that it could be more readable without the ternary condition in
the argument of phy_restore_page.

Let me know your thoughts on this.

Thanks,
Quentin

[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L444
[2] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L454

Attachment: signature.asc
Description: PGP signature