Message-ID: <20200427125156.GD25745@shell.armlinux.org.uk>
Date:   Mon, 27 Apr 2020 13:51:56 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Florinel Iordache <florinel.iordache@....com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "corbet@....net" <corbet@....net>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        Leo Li <leoyang.li@....com>,
        "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver
 support

On Mon, Apr 27, 2020 at 12:40:37PM +0000, Florinel Iordache wrote:
> > > +/* Backplane mutex between all KR PHY threads */ static struct mutex
> > > +backplane_lock;
> > 
> > 
> > > +/* Read AN Link Status */
> > > +static int is_an_link_up(struct phy_device *phydev) {
> > > +     struct backplane_device *bpdev = phydev->priv;
> > > +     int ret, val = 0;
> > > +
> > > +     mutex_lock(&bpdev->bpphy_lock);
> > 
> > Last time i asked the question about how this mutex and the phy mutex interact.
> > I don't remember seeing an answer.
> > 
> >           Andrew
> 
> Hi Andrew,
> Yes, your question was:
> <<How does this mutex interact with phydev->lock? It appears both are trying to do the same thing, serialise access to the PHY hardware.>>
> The answer is: yes, you are right, they both are protecting the critical section related to accessing the PHY hardware for a particular PHY.
> As you can see the backplane device (bpdev) has associated one phy_device (phydev) so  bpdev->bpphy_lock and phydev->lock are equivalent.
> Normally your assumption is correct: backplane driver should use the same phydev->lock but there is the following problem:
> Backplane driver needs to protect all accesses to a PHY hardware including the ones coming from backplane scheduled workqueues for all lanes within a PHY.
> But phydev->lock is already acquired for a phy_device (from phy.c) before each phy_driver callback is called (e.g.: config_aneg, suspend, ...)
> So if I would use phydev->lock instead of bpdev->bpphy_lock then this would result in a deadlock when it is called from phy_driver callbacks.
> However a possible solution would be to remove all these locks using bpphy_lock and use instead only one phydev->lock in backplane kr state machine: (bp_kr_state_machine).
> But this solution will result in poorer performance, the training total duration will increase because only one single lane can enter the training procedure at a time therefore it would be possible for multi-lane phy training to ultimately fail because training is not finished in under 500ms. So I wanted to avoid this loss of training performance.
> Yet another possible solution would be to keep the locks where they are, at the lowest level exactly at phy_read/write_mmd calls, in order to allow lanes training running in parallel, but use instead the phydev->lock as would be normal to be and according to your suggestion.
> But in this case I must avoid the deadlock I mentioned above by differentiating between the calls coming from phy_driver callbacks where the phydev->lock is already acquired for this phy_device by the phy framework so the mutex should be skipped in this case and the calls coming from anywhere else (for example from backplane kr state machine) when the phydev->lock was not already acquired for this phy_device and the mutex must be used.
> If you agree with this latest solution then I can implement it in next version by using a flag in backplane_device called: 'phy_mutex_already_acquired' or 'skip_phy_mutex' which must be set in all backplane phy_driver callbacks and will be used to skip the locks on phydev->lock used at phy_read/write_mmd calls in these cases.

I think you have a rather big misunderstanding of the locking in phylib
from what you said above.

The register accessors do not use phydev->lock.

Follow the code.

	phy_read_mmd() uses phy_lock_mdio_bus().

	phy_lock_mdio_bus() locks the phydev->mdio.bus->mdio_lock mutex.

This is the _bus_ level lock, and is entirely different from
phydev->lock.

It is entirely safe to call phy_read_mmd() from any region of code
which is holding phydev->lock - indeed, we have many PHY drivers that
already do this.

So, I think you need to rewrite your entire locking strategy, because
it seems that you've misunderstood the locking here.

However, it's actually way worse, because of the abuse in your driver
of a single phy_device struct, which you use to access multiple PHYs,
randomly changing phydev->mdio.addr according to which PHY needs to be
accessed - you need to _carefully_ consider how your locking is done
for that.  I regard this as a big abuse, and I'm very tempted to NAK
your patches on this abuse alone.

I think you need to take onboard my comments about the (ab)use of
phy_device here.

An alternative solution to this is to push the phy_* accessors up a
level to the mdiobus level (we already have some, and I've already
been converting others) so you don't have to mess with
phydev->mdio.addr at all.

However, I would still consider your use of struct phy_device to be
an abuse.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Powered by blists - more mailing lists