Hi Russel,

On Tue, Oct 02, 2018 at 03:51:11PM +0200, Quentin Schulz wrote:

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:

quoted

quoted

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.

quoted

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);

The whole point was there. We're trying to propagate return values
through phy_restore_page and only overwrite it if it's 0. However, there
are some functions that return something different from 0 (e.g. size of
something that is handled or returned) and are still valid and wanted to
be propagated. If we were to overwrite the return value with 0 if
__phy_write_page is returning 0, we would need to use a temporary
variable to not overwrite the return value before calling
phy_restore_page.

With my suggestion, we would need to use a temporary variable to keep a

0 return values while calling phy_restore_page but not when we want

phy_restore_page to return 0 even when the return value before calling
phy_restore_page is > 0.

With the current behaviour, we would need to use a temporary value (or a
ternary condition as given as an example in original mail) when we want
to return 0 only when no error happens in phy_restore_page and the
return value before calling phy_restore_page was >= 0. We would not need
to use a temporary variable when phy_restore_page finishes without error
and we want to keep the return value before calling phy_restore_page if
it's >=0.

So basically, that's down to a technical choice and none is perfect.
Sorry for bothering.

Thanks,
Quentin