[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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