Date: Wed, 7 Dec 2016 21:55:42 +0100
From: Pavel Machek <pavel@ucw.cz>
To: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Cc: bh74.an@samsung.com, ks.giri@samsung.com, vipul.pandya@samsung.com,
        peppe.cavallaro@st.com, alexandre.torgue@st.com, davem@davemloft.net,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH 2/2] net: ethernet: stmmac: remove private tx queue lock
Message-ID: <20161207205542.GA2250@amd>
References: <1481141138-19466-1-git-send-email-LinoSanfilippo@gmx.de>
 <1481141138-19466-3-git-send-email-LinoSanfilippo@gmx.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
        protocol="application/pgp-signature"; boundary="45Z9DzgjV8m4Oswq"
Content-Disposition: inline
In-Reply-To: <1481141138-19466-3-git-send-email-LinoSanfilippo@gmx.de>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2159
Lines: 68


--45Z9DzgjV8m4Oswq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi!

> The driver uses a private lock for synchronization between the xmit
> function and the xmit completion handler, but since the NETIF_F_LLTX flag
> is not set, the xmit function is also called with the xmit_lock held.
>=20
> On the other hand the xmit completion handler first takes the private lock
> and (in case that the tx queue has been stopped) the xmit_lock, leading to
> a reverse locking order and the potential danger of a deadlock.
>=20
> Fix this by removing the private lock completely and synchronizing the xm=
it
> function and completion handler solely by means of the xmit_lock. By doing
> this remove also the now unnecessary double check for a stopped tx queue.
>=20
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Does not seem to apply to net-next based on
adc176c5472214971d77c1a61c83db9b01e9cdc7. Aha, that's the printk()
changes, probably would apply to mainline.

> index caf069a..db46ec4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1307,7 +1307,7 @@ static void stmmac_tx_clean(struct stmmac_priv *pri=
v)
>  	unsigned int bytes_compl =3D 0, pkts_compl =3D 0;
>  	unsigned int entry =3D priv->dirty_tx;
> =20
> -	spin_lock(&priv->tx_lock);
> +	netif_tx_lock(priv->dev);
> =20
>  	priv->xstats.tx_clean++;
>

Should it use "netif_tx_lock_bh"?

I could not reproduce the deadlock without this patch, nor can I
detect anything wrong with this patch, so I guess that is:

Tested-by: Pavel Machek <pavel@denx.de>

Thanks,
									Pavel
--=20
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blo=
g.html

--45Z9DzgjV8m4Oswq
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iEYEARECAAYFAlhId04ACgkQMOfwapXb+vItjACgj6IbYrvZOGMYRYEdrQS0ypnN
sgMAoMPsAFvrY3g6CIcOOZzXkikVH/Un
=h25n
-----END PGP SIGNATURE-----

--45Z9DzgjV8m4Oswq--