Message-ID: <20181004141734.6bnkj6zh3dbqefeo@qschulz>
Date:   Thu, 4 Oct 2018 16:17:34 +0200
From:   Quentin Schulz <quentin.schulz@...tlin.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, f.fainelli@...il.com,
        allan.nielsen@...rochip.com, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, thomas.petazzoni@...tlin.com,
        Raju Lakkaraju <Raju.Lakkaraju@...rosemi.com>,
        rmk+kernel@...linux.org.uk
Subject: Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics
 counters

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

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists