Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Florinel Iordache <florinel.iordache@nxp.com>
To:     Andrew Lunn <andrew@lunn.ch>
CC:     "davem@davemloft.net" <davem@davemloft.net>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
        "hkallweit1@gmail.com" <hkallweit1@gmail.com>,
        "linux@armlinux.org.uk" <linux@armlinux.org.uk>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "corbet@lwn.net" <corbet@lwn.net>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        Leo Li <leoyang.li@nxp.com>,
        "Madalin Bucur (OSS)" <madalin.bucur@oss.nxp.com>,
        Ioana Ciornei <ioana.ciornei@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Florinel Iordache <florinel.iordache@nxp.com>
Subject: RE: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver
 support
Thread-Topic: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver
 support
Thread-Index: AdYchlLdOcGMcLhKS+m3uu2Z+Q+2Zw==
Date:   Mon, 27 Apr 2020 12:40:37 +0000
Message-ID: <AM0PR04MB54432C98E0CD7FF5B155F446FBAF0@AM0PR04MB5443.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 6ebcaccd-c0c3-4b14-9c7d-08d7eaa83023
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Apr 2020 12:40:37.7710
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: +zgwpDVkCj1kKlfABq50fvhlSZDXytNOBJAkC9eUZviYcodjumIYpd1rd38js+FtfKZGV7r1rFVf8p4VQrDFZ2XIxTqdWywoUm9KC90c2sU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB7027
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > +/* Backplane mutex between all KR PHY threads */ static struct mutex
> > +backplane_lock;
>=20
>=20
> > +/* Read AN Link Status */
> > +static int is_an_link_up(struct phy_device *phydev) {
> > +     struct backplane_device *bpdev =3D phydev->priv;
> > +     int ret, val =3D 0;
> > +
> > +     mutex_lock(&bpdev->bpphy_lock);
>=20
> Last time i asked the question about how this mutex and the phy mutex int=
eract.
> I don't remember seeing an answer.
>=20
>           Andrew

Hi Andrew,
Yes, your question was:
<<How does this mutex interact with phydev->lock? It appears both are tryin=
g to do the same thing, serialise access to the PHY hardware.>>
The answer is: yes, you are right, they both are protecting the critical se=
ction 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 p=
hydev->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 e=
ach 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 dur=
ation will increase because only one single lane can enter the training pro=
cedure 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 w=
anted 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 la=
nes 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 differentia=
ting between the calls coming from phy_driver callbacks where the phydev->l=
ock is already acquired for this phy_device by the phy framework so the mut=
ex 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 vers=
ion 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 callbac=
ks and will be used to skip the locks on phydev->lock used at phy_read/writ=
e_mmd calls in these cases.

I'm sorry I have not answered this question the first time when you asked i=
t.
Florin.