| Message ID | 20240513015127.961360-1-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: fec: avoid lock evasion when reading pps_enable | expand |
On Mon, May 13, 2024 at 4:02 AM Wei Fang <wei.fang@nxp.com> wrote: > > The assignment of pps_enable is protected by tmreg_lock, but the read > operation of pps_enable is not. So the Coverity tool reports a lock > evasion warning which may cause data race to occur when running in a > multithread environment. Although this issue is almost impossible to > occur, we'd better fix it, at least it seems more logically reasonable, > and it also prevents Coverity from continuing to issue warnings. > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp clock") > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c > index 181d9bfbee22..8d37274a3fb0 100644 > --- a/drivers/net/ethernet/freescale/fec_ptp.c > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable) > struct timespec64 ts; > u64 ns; > > - if (fep->pps_enable == enable) > - return 0; > - > fep->pps_channel = DEFAULT_PPS_CHANNEL; > fep->reload_period = PPS_OUPUT_RELOAD_PERIOD; Why are these writes left without the spinlock protection ? > > spin_lock_irqsave(&fep->tmreg_lock, flags); > > + if (fep->pps_enable == enable) { > + spin_unlock_irqrestore(&fep->tmreg_lock, flags); > + return 0; > + } > + > if (enable) { > /* clear capture or output compare interrupt status if have. > */ > -- > 2.34.1 >
> -----Original Message----- > From: Eric Dumazet <edumazet@google.com> > Sent: 2024年5月13日 15:29 > To: Wei Fang <wei.fang@nxp.com> > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; Shenwei > Wang <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > richardcochran@gmail.com; andrew@lunn.ch; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; imx@lists.linux.dev > Subject: Re: [PATCH net] net: fec: avoid lock evasion when reading pps_enable > > On Mon, May 13, 2024 at 4:02 AM Wei Fang <wei.fang@nxp.com> wrote: > > > > The assignment of pps_enable is protected by tmreg_lock, but the read > > operation of pps_enable is not. So the Coverity tool reports a lock > > evasion warning which may cause data race to occur when running in a > > multithread environment. Although this issue is almost impossible to > > occur, we'd better fix it, at least it seems more logically > > reasonable, and it also prevents Coverity from continuing to issue warnings. > > > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp > > clock") > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > --- > > drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c > > b/drivers/net/ethernet/freescale/fec_ptp.c > > index 181d9bfbee22..8d37274a3fb0 100644 > > --- a/drivers/net/ethernet/freescale/fec_ptp.c > > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > > @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct > fec_enet_private *fep, uint enable) > > struct timespec64 ts; > > u64 ns; > > > > - if (fep->pps_enable == enable) > > - return 0; > > - > > fep->pps_channel = DEFAULT_PPS_CHANNEL; > > fep->reload_period = PPS_OUPUT_RELOAD_PERIOD; > > Why are these writes left without the spinlock protection ? For fec driver, the pps_channel and the reload_period of PPS request are always fixed, and they were also not protected by the lock in the original code. > > > > > > spin_lock_irqsave(&fep->tmreg_lock, flags); > > > > + if (fep->pps_enable == enable) { > > + spin_unlock_irqrestore(&fep->tmreg_lock, flags); > > + return 0; > > + } > > + > > if (enable) { > > /* clear capture or output compare interrupt status if > have. > > */ > > -- > > 2.34.1 > >
On Mon, May 13, 2024 at 9:53 AM Wei Fang <wei.fang@nxp.com> wrote: > > > -----Original Message----- > > From: Eric Dumazet <edumazet@google.com> > > Sent: 2024年5月13日 15:29 > > To: Wei Fang <wei.fang@nxp.com> > > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; Shenwei > > Wang <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > richardcochran@gmail.com; andrew@lunn.ch; netdev@vger.kernel.org; > > linux-kernel@vger.kernel.org; imx@lists.linux.dev > > Subject: Re: [PATCH net] net: fec: avoid lock evasion when reading pps_enable > > > > On Mon, May 13, 2024 at 4:02 AM Wei Fang <wei.fang@nxp.com> wrote: > > > > > > The assignment of pps_enable is protected by tmreg_lock, but the read > > > operation of pps_enable is not. So the Coverity tool reports a lock > > > evasion warning which may cause data race to occur when running in a > > > multithread environment. Although this issue is almost impossible to > > > occur, we'd better fix it, at least it seems more logically > > > reasonable, and it also prevents Coverity from continuing to issue warnings. > > > > > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp > > > clock") > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > --- > > > drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c > > > b/drivers/net/ethernet/freescale/fec_ptp.c > > > index 181d9bfbee22..8d37274a3fb0 100644 > > > --- a/drivers/net/ethernet/freescale/fec_ptp.c > > > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > > > @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct > > fec_enet_private *fep, uint enable) > > > struct timespec64 ts; > > > u64 ns; > > > > > > - if (fep->pps_enable == enable) > > > - return 0; > > > - > > > fep->pps_channel = DEFAULT_PPS_CHANNEL; > > > fep->reload_period = PPS_OUPUT_RELOAD_PERIOD; > > > > Why are these writes left without the spinlock protection ? > For fec driver, the pps_channel and the reload_period of PPS request > are always fixed, and they were also not protected by the lock in the > original code. If this is the case, please move this initialization elsewhere, so that we can be absolutely sure of the claim. I see fep->reload_period being overwritten in this file, I do not see clear evidence this is all safe.
See inline, > The assignment of pps_enable is protected by tmreg_lock, but the read > operation of pps_enable is not. So the Coverity tool reports a lock evasion > warning which may cause data race to occur when running in a multithread > environment. Although this issue is almost impossible to occur, we'd better fix > it, at least it seems more logically reasonable, and it also prevents Coverity > from continuing to issue warnings. > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp clock") > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c > b/drivers/net/ethernet/freescale/fec_ptp.c > index 181d9bfbee22..8d37274a3fb0 100644 > --- a/drivers/net/ethernet/freescale/fec_ptp.c > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct > fec_enet_private *fep, uint enable) > struct timespec64 ts; > u64 ns; > > - if (fep->pps_enable == enable) > - return 0; > - > fep->pps_channel = DEFAULT_PPS_CHANNEL; > fep->reload_period = PPS_OUPUT_RELOAD_PERIOD; > > spin_lock_irqsave(&fep->tmreg_lock, flags); > > + if (fep->pps_enable == enable) { Can we atomic_set/get instead of spin_lock here. Thanks, Hariprasad k > + spin_unlock_irqrestore(&fep->tmreg_lock, flags); > + return 0; > + } > + > if (enable) { > /* clear capture or output compare interrupt status if have. > */ > -- > 2.34.1 >
> -----Original Message----- > From: Eric Dumazet <edumazet@google.com> > Sent: 2024年5月13日 16:41 > To: Wei Fang <wei.fang@nxp.com> > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; Shenwei > Wang <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > richardcochran@gmail.com; andrew@lunn.ch; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; imx@lists.linux.dev > Subject: Re: [PATCH net] net: fec: avoid lock evasion when reading > pps_enable > > On Mon, May 13, 2024 at 9:53 AM Wei Fang <wei.fang@nxp.com> wrote: > > > > > -----Original Message----- > > > From: Eric Dumazet <edumazet@google.com> > > > Sent: 2024年5月13日 15:29 > > > To: Wei Fang <wei.fang@nxp.com> > > > Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com; > Shenwei > > > Wang <shenwei.wang@nxp.com>; Clark Wang > <xiaoning.wang@nxp.com>; > > > richardcochran@gmail.com; andrew@lunn.ch; netdev@vger.kernel.org; > > > linux-kernel@vger.kernel.org; imx@lists.linux.dev > > > Subject: Re: [PATCH net] net: fec: avoid lock evasion when reading > > > pps_enable > > > > > > On Mon, May 13, 2024 at 4:02 AM Wei Fang <wei.fang@nxp.com> > wrote: > > > > > > > > The assignment of pps_enable is protected by tmreg_lock, but the > > > > read operation of pps_enable is not. So the Coverity tool reports > > > > a lock evasion warning which may cause data race to occur when > > > > running in a multithread environment. Although this issue is > > > > almost impossible to occur, we'd better fix it, at least it seems > > > > more logically reasonable, and it also prevents Coverity from continuing > to issue warnings. > > > > > > > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on > > > > ptp > > > > clock") > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > > --- > > > > drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c > > > > b/drivers/net/ethernet/freescale/fec_ptp.c > > > > index 181d9bfbee22..8d37274a3fb0 100644 > > > > --- a/drivers/net/ethernet/freescale/fec_ptp.c > > > > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > > > > @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct > > > fec_enet_private *fep, uint enable) > > > > struct timespec64 ts; > > > > u64 ns; > > > > > > > > - if (fep->pps_enable == enable) > > > > - return 0; > > > > - > > > > fep->pps_channel = DEFAULT_PPS_CHANNEL; > > > > fep->reload_period = PPS_OUPUT_RELOAD_PERIOD; > > > > > > Why are these writes left without the spinlock protection ? > > For fec driver, the pps_channel and the reload_period of PPS request > > are always fixed, and they were also not protected by the lock in the > > original code. > > If this is the case, please move this initialization elsewhere, so that we can be > absolutely sure of the claim. > Accept, thanks > I see fep->reload_period being overwritten in this file, I do not see clear > evidence this is all safe.
> -----Original Message----- > From: Hariprasad Kelam <hkelam@marvell.com> > Sent: 2024年5月13日 17:27 > To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Shenwei > Wang <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > richardcochran@gmail.com; andrew@lunn.ch; netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; imx@lists.linux.dev > Subject: [PATCH net] net: fec: avoid lock evasion when reading pps_enable > > See inline, > > > The assignment of pps_enable is protected by tmreg_lock, but the read > > operation of pps_enable is not. So the Coverity tool reports a lock > > evasion warning which may cause data race to occur when running in a > > multithread environment. Although this issue is almost impossible to > > occur, we'd better fix it, at least it seems more logically > > reasonable, and it also prevents Coverity from continuing to issue warnings. > > > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp > > clock") > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > --- > > drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c > > b/drivers/net/ethernet/freescale/fec_ptp.c > > index 181d9bfbee22..8d37274a3fb0 100644 > > --- a/drivers/net/ethernet/freescale/fec_ptp.c > > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > > @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct > > fec_enet_private *fep, uint enable) > > struct timespec64 ts; > > u64 ns; > > > > - if (fep->pps_enable == enable) > > - return 0; > > - > > fep->pps_channel = DEFAULT_PPS_CHANNEL; > > fep->reload_period = PPS_OUPUT_RELOAD_PERIOD; > > > > spin_lock_irqsave(&fep->tmreg_lock, flags); > > > > + if (fep->pps_enable == enable) { > > Can we atomic_set/get instead of spin_lock here. > I'm afraid that cannot eliminate the lock evasion warning, because it's still possible that multithreads take the false branch of "if (fep->pps_enable == enable)" before pps_enable is updated. > Thanks, > Hariprasad k > > + spin_unlock_irqrestore(&fep->tmreg_lock, flags); > > + return 0; > > + } > > + > > if (enable) { > > /* clear capture or output compare interrupt status if have. > > */ > > -- > > 2.34.1 > >
On Mon, May 13, 2024 at 09:51:26AM +0800, Wei Fang wrote: > The assignment of pps_enable is protected by tmreg_lock, but the read > operation of pps_enable is not. So the Coverity tool reports a lock > evasion warning which may cause data race to occur when running in a > multithread environment. Although this issue is almost impossible to > occur, we'd better fix it, at least it seems more logically reasonable, > and it also prevents Coverity from continuing to issue warnings. > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp clock") > Signed-off-by: Wei Fang <wei.fang@nxp.com> Reviewed-by: Simon Horman <horms@kernel.org>
On Mon, May 13, 2024 at 08:54:59PM +0100, Simon Horman wrote: > On Mon, May 13, 2024 at 09:51:26AM +0800, Wei Fang wrote: > > The assignment of pps_enable is protected by tmreg_lock, but the read > > operation of pps_enable is not. So the Coverity tool reports a lock > > evasion warning which may cause data race to occur when running in a > > multithread environment. Although this issue is almost impossible to > > occur, we'd better fix it, at least it seems more logically reasonable, > > and it also prevents Coverity from continuing to issue warnings. > > > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp clock") > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > Reviewed-by: Simon Horman <horms@kernel.org> Sorry, I convinced myself this is correct. But I now see that questions have been raised by others. So please ignore the above.
> > -----Original Message----- > > From: Hariprasad Kelam <hkelam@marvell.com> > > Sent: 2024年5月13日 17:27 > > To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Shenwei > Wang > > <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > richardcochran@gmail.com; andrew@lunn.ch; netdev@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org; imx@lists.linux.dev > > Subject: [PATCH net] net: fec: avoid lock evasion when reading > > pps_enable > > > > See inline, > > > > > The assignment of pps_enable is protected by tmreg_lock, but the > > > read operation of pps_enable is not. So the Coverity tool reports a > > > lock evasion warning which may cause data race to occur when running > > > in a multithread environment. Although this issue is almost > > > impossible to occur, we'd better fix it, at least it seems more > > > logically reasonable, and it also prevents Coverity from continuing to issue > warnings. > > > > > > Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp > > > clock") > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > --- > > > drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_ptp.c > > > b/drivers/net/ethernet/freescale/fec_ptp.c > > > index 181d9bfbee22..8d37274a3fb0 100644 > > > --- a/drivers/net/ethernet/freescale/fec_ptp.c > > > +++ b/drivers/net/ethernet/freescale/fec_ptp.c > > > @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct > > > fec_enet_private *fep, uint enable) > > > struct timespec64 ts; > > > u64 ns; > > > > > > - if (fep->pps_enable == enable) > > > - return 0; > > > - > > > fep->pps_channel = DEFAULT_PPS_CHANNEL; > > > fep->reload_period = PPS_OUPUT_RELOAD_PERIOD; > > > > > > spin_lock_irqsave(&fep->tmreg_lock, flags); > > > > > > + if (fep->pps_enable == enable) { > > > > Can we atomic_set/get instead of spin_lock here. > > > I'm afraid that cannot eliminate the lock evasion warning, because it's still > possible that multithreads take the false branch of "if (fep->pps_enable == > enable)" before pps_enable is updated. > > Since in fec_ptp_enable_pps(), value of pps_enable is checked before entering the actual code changes, Better approach is to use atomic_read/write. This is not only for reading but for assigning the values as well. This way covertity wont complain. > > Thanks, > > Hariprasad k > > > + spin_unlock_irqrestore(&fep->tmreg_lock, flags); > > > + return 0; > > > + } > > > + > > > if (enable) { > > > /* clear capture or output compare interrupt status if have. > > > */ > > > -- > > > 2.34.1 > > >
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c index 181d9bfbee22..8d37274a3fb0 100644 --- a/drivers/net/ethernet/freescale/fec_ptp.c +++ b/drivers/net/ethernet/freescale/fec_ptp.c @@ -104,14 +104,16 @@ static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable) struct timespec64 ts; u64 ns; - if (fep->pps_enable == enable) - return 0; - fep->pps_channel = DEFAULT_PPS_CHANNEL; fep->reload_period = PPS_OUPUT_RELOAD_PERIOD; spin_lock_irqsave(&fep->tmreg_lock, flags); + if (fep->pps_enable == enable) { + spin_unlock_irqrestore(&fep->tmreg_lock, flags); + return 0; + } + if (enable) { /* clear capture or output compare interrupt status if have. */
The assignment of pps_enable is protected by tmreg_lock, but the read operation of pps_enable is not. So the Coverity tool reports a lock evasion warning which may cause data race to occur when running in a multithread environment. Although this issue is almost impossible to occur, we'd better fix it, at least it seems more logically reasonable, and it also prevents Coverity from continuing to issue warnings. Fixes: 278d24047891 ("net: fec: ptp: Enable PPS output based on ptp clock") Signed-off-by: Wei Fang <wei.fang@nxp.com> --- drivers/net/ethernet/freescale/fec_ptp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)