From: Quentin Schulz <quentin.schulz@bootlin.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: davem@davemloft.net, f.fainelli@gmail.com,
allan.nielsen@microchip.com, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, thomas.petazzoni@bootlin.com,
Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>,
rmk+kernel@armlinux.org.uk
Subject: Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters
Date: Thu, 4 Oct 2018 16:17:34 +0200 [thread overview]
Message-ID: <20181004141734.6bnkj6zh3dbqefeo@qschulz> (raw)
In-Reply-To: <20181002135111.b4ykicst5ipp4o74@qschulz>
[-- Attachment #1: Type: text/plain, Size: 5101 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2018-10-04 14:17 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-14 8:33 [PATCH net-next 0/5] Various improvements to Microsemi PHY driver Quentin Schulz
2018-09-14 8:33 ` [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters Quentin Schulz
2018-09-14 13:01 ` Andrew Lunn
2018-09-14 13:16 ` Quentin Schulz
2018-09-14 13:29 ` Andrew Lunn
2018-10-02 13:51 ` Quentin Schulz
2018-10-04 14:17 ` Quentin Schulz [this message]
2018-09-14 8:33 ` [PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence Quentin Schulz
2018-09-15 2:21 ` Florian Fainelli
2018-10-01 8:51 ` Quentin Schulz
2018-10-01 16:27 ` Florian Fainelli
2018-09-14 8:33 ` [PATCH net-next 3/5] net: phy: mscc: remove unneeded parenthesis Quentin Schulz
2018-09-14 13:04 ` Andrew Lunn
2018-09-15 20:52 ` Florian Fainelli
2018-09-14 8:33 ` [PATCH net-next 4/5] net: phy: mscc: shorten `x != 0` condition to `x` Quentin Schulz
2018-09-14 13:05 ` Andrew Lunn
2018-09-15 20:53 ` Florian Fainelli
2018-09-14 8:33 ` [PATCH net-next 5/5] net: phy: mscc: remove unneeded temporary variable Quentin Schulz
2018-09-14 13:06 ` Andrew Lunn
2018-09-15 2:19 ` Florian Fainelli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181004141734.6bnkj6zh3dbqefeo@qschulz \
--to=quentin.schulz@bootlin.com \
--cc=Raju.Lakkaraju@microsemi.com \
--cc=allan.nielsen@microchip.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=rmk+kernel@armlinux.org.uk \
--cc=thomas.petazzoni@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).