Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Pavel Skripkin <paskripkin@gmail.com>
CC:     Claudiu Manoil <claudiu.manoil@nxp.com>,
        "alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "clement.leger@bootlin.com" <clement.leger@bootlin.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH v3] net: ocelot: fix wrong time_after usage
Thread-Topic: [PATCH v3] net: ocelot: fix wrong time_after usage
Thread-Index: AQHYkSZB/F5QO/XCnE66pe7u42qQSK1xUe4A
Date:   Wed, 6 Jul 2022 13:13:01 +0000
Message-ID: <20220706131300.uontjopbdf72pwxy@skbuf>
References: <20220706105044.8071-1-paskripkin@gmail.com>
In-Reply-To: <20220706105044.8071-1-paskripkin@gmail.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Z5eu70nnFNgxbbwqZWNVGbEKGnxXBC/mrHT68HSv6O2z8BauWBa7GOOmZKen?=
 =?us-ascii?Q?TMu7tmrH6rHfaSC891fBF+w385gKHFX0/M+lnjz/Oy/jYLD/JwBqFZ173MWw?=
 =?us-ascii?Q?TBRUydpQZY2JuDbZzmdEr0u4Ikj0ly8703P/fG73mw/cfyNUCtlI6Ane8rbH?=
 =?us-ascii?Q?YNbUBXZJmyfPVd1t4qGPSxll1u9y2BywSZ9YMwoDJ0rwEfRlRD+NNeaXk/Mn?=
 =?us-ascii?Q?WmDiK7AmzNmz1+1kk1gn8DKjfPLkidjZU8iFN/Ov8sIer3vVcgHWs6smia4s?=
 =?us-ascii?Q?eWmIxV/TLfl2iwM/gvg85A8AO5P+9NLzEuPWWx5jXeyrrcMEBZ8a8R1ktpPu?=
 =?us-ascii?Q?ndwQ9CYtnT6xxXIuZsdZ4JEaK1YCN7pluzFLUrD2MyQaFsYhNYTix8mNfyd0?=
 =?us-ascii?Q?M6ZEQkyLErXYAZ1lEYWt6MutX/3C+KZVWJwGy46Oco+kNKmF/1X3NiUmZJ2z?=
 =?us-ascii?Q?wfG4UZ1Fq9hmvoMGP2Z0omzsiRQeNnHaK/f5YzWBdYZtSl0/yhJghRb8OgQd?=
 =?us-ascii?Q?9b81RVtIXHzbwAmsbdrKeB3M8yZmXVDXUfuIN93OSewDaJyz0avO+3vVhO0v?=
 =?us-ascii?Q?jrNcm7CCs9XVEFpmaOPaeuDYLcZ5saNTEScq8aMjhWkni/CPGRajqd46BK8r?=
 =?us-ascii?Q?vnX55ge0MTxX3tF4SVe6BoM6pE4bsomR/Rf5YSRLBtQw/W6SvMF/HVzqLvCb?=
 =?us-ascii?Q?9xhloqDSOid4TS7cF/WMuureAj6cJ/Vu1WSotQMhNLwlIs5uXLtxq6/MjKre?=
 =?us-ascii?Q?OpDZL7ev9R2AuvQhyqMFa7F5bSeE9cWIjERBLWTdFOvY71ukuJ7+B2rUtzbq?=
 =?us-ascii?Q?7D/VCrIgAAPm8tg849ggItvbaUbPSE2W7LYSMvxd5FkjUVMH5OxM06Nc/YSe?=
 =?us-ascii?Q?ObNe+KqTwY50dMJdJIdIj1DR30QAbTMaIfsjSBPb+vUs40gaocCRSy5lGzm6?=
 =?us-ascii?Q?jdWpvSqBpUw0w0KJSXo8pHf+v0AZJ+NeIdyLzvRP6FQUX+xQNUZ1JONnLudS?=
 =?us-ascii?Q?SQoj4aVLqIXkM4zC/cHZ1qdxghEAlnym1i0VVM7XMwqXshLPCBUm7L9eFZkP?=
 =?us-ascii?Q?BzP9JiRaFKInkqKT1mjZ31lEFOKkm7BeU7QwNZkGbhs9SOg8dBsQEe4FpToi?=
 =?us-ascii?Q?/3BEcbPO4v/Ft4cq6bHLArH7FxLRydKurgeQgSN3QGiiqSBTZ22NUuAVqOQz?=
 =?us-ascii?Q?TgLBM5bntFdg+yfK4AlkNxkzfXklPKDtGzDAX/L7bBJuzBvp82hTAknbHrB9?=
 =?us-ascii?Q?nHBmPnFoa4xeyPDrRd0WSW4V2KOeNi1sMgwjSgIhK7ow+uG5gbsl/m3S/sos?=
 =?us-ascii?Q?5SNuxpqnXPhMs0ficpGjuBCMcUhwfCVcFwb1utK2gJb2edT6GZOeyPj5X7pf?=
 =?us-ascii?Q?yZ9/lo72QYSArGyvEOym5bGrzfrUAh3RcYvZ775L1LpYgms2NSy+5efT9g/w?=
 =?us-ascii?Q?e7L1t1gGw1aUT8m0xDVwyD/064m5jxfdw4umwdQNXLXFUHz8IRheO0ytU9eu?=
 =?us-ascii?Q?yOancX/56NsfGuks2MsJ4HJeHYB9yq6MSnsoODjV2MN0zbtxkQ/wCuR9NP4Q?=
 =?us-ascii?Q?DPpc+wSDPV0LZXf9o8aShGH9C8+4i9UHnXVUCSihJRHFK82498CUF7h6rTTB?=
 =?us-ascii?Q?Tw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2B10E08D4A27824BB1241F8E704D7C89@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 87a29c08-4173-4d76-46e8-08da5f514136
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jul 2022 13:13:01.6943
 (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: IIQYbq3hkZqZwgs6Vfou76kcxz/dVwXPZGxw8MTHxTtAlSSjGvXKpfc75QzL6SHRvKsl8PJVefSrhMWG6KYpvQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR04MB8732
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Pavel,

On Wed, Jul 06, 2022 at 01:50:44PM +0300, Pavel Skripkin wrote:
> Accidentally noticed, that this driver is the only user of
> while (time_after(jiffies...)).
>=20
> It looks like typo, because likely this while loop will finish after 1st
> iteration, because time_after() returns true when 1st argument _is after_
> 2nd one.
>=20
> There is one possible problem with this poll loop: the scheduler could pu=
t
> the thread to sleep, and it does not get woken up for
> OCELOT_FDMA_CH_SAFE_TIMEOUT_US. During that time, the hardware has done
> its thing, but you exit the while loop and return -ETIMEDOUT.
>=20
> Fix it by using sane poll API that avoids all problems described above
>=20
> Fixes: 753a026cfec1 ("net: ocelot: add FDMA support")
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>=20
> Changes since v2:
> 	- Use _atomic variant of readx_poll_timeout
>=20
> Changes since v1:
> 	- Fixed typos in title and commit message
> 	- Remove while loop and use readx_poll_timeout as suggested by
> 	  Andrew
>=20
> ---
>  drivers/net/ethernet/mscc/ocelot_fdma.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/mscc/ocelot_fdma.c b/drivers/net/ethern=
et/mscc/ocelot_fdma.c
> index 083fddd263ec..c93fba0a2a7d 100644
> --- a/drivers/net/ethernet/mscc/ocelot_fdma.c
> +++ b/drivers/net/ethernet/mscc/ocelot_fdma.c
> @@ -94,19 +94,18 @@ static void ocelot_fdma_activate_chan(struct ocelot *=
ocelot, dma_addr_t dma,
>  	ocelot_fdma_writel(ocelot, MSCC_FDMA_CH_ACTIVATE, BIT(chan));
>  }
> =20
> +static u32 ocelot_fdma_read_ch_safe(struct ocelot *ocelot)
> +{
> +	return ocelot_fdma_readl(ocelot, MSCC_FDMA_CH_SAFE);
> +}
> +
>  static int ocelot_fdma_wait_chan_safe(struct ocelot *ocelot, int chan)
>  {
> -	unsigned long timeout;
>  	u32 safe;
> =20
> -	timeout =3D jiffies + usecs_to_jiffies(OCELOT_FDMA_CH_SAFE_TIMEOUT_US);
> -	do {
> -		safe =3D ocelot_fdma_readl(ocelot, MSCC_FDMA_CH_SAFE);
> -		if (safe & BIT(chan))
> -			return 0;
> -	} while (time_after(jiffies, timeout));
> -
> -	return -ETIMEDOUT;
> +	return readx_poll_timeout_atomic(ocelot_fdma_read_ch_safe, ocelot, safe=
,
> +				  safe & BIT(chan), 0,
> +				  OCELOT_FDMA_CH_SAFE_TIMEOUT_US);

Can you please indent the arguments to the open bracket?

>  }
> =20
>  static void ocelot_fdma_dcb_set_data(struct ocelot_fdma_dcb *dcb,
> --=20
> 2.35.1
>=