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 <[email protected]>
---
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) {
+ 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 4:02 AM Wei Fang <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> Sent: 2024年5月13日 15:29
> To: Wei Fang <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; Shenwei
> Wang <[email protected]>; Clark Wang <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH net] net: fec: avoid lock evasion when reading pps_enable
>
> On Mon, May 13, 2024 at 4:02 AM Wei Fang <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
>
> > -----Original Message-----
> > From: Eric Dumazet <[email protected]>
> > Sent: 2024年5月13日 15:29
> > To: Wei Fang <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected]; Shenwei
> > Wang <[email protected]>; Clark Wang <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH net] net: fec: avoid lock evasion when reading pps_enable
> >
> > On Mon, May 13, 2024 at 4:02 AM Wei Fang <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> ---
> 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 <[email protected]>
> Sent: 2024年5月13日 16:41
> To: Wei Fang <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; Shenwei
> Wang <[email protected]>; Clark Wang <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH net] net: fec: avoid lock evasion when reading
> pps_enable
>
> On Mon, May 13, 2024 at 9:53 AM Wei Fang <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: Eric Dumazet <[email protected]>
> > > Sent: 2024年5月13日 15:29
> > > To: Wei Fang <[email protected]>
> > > Cc: [email protected]; [email protected]; [email protected];
> Shenwei
> > > Wang <[email protected]>; Clark Wang
> <[email protected]>;
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH net] net: fec: avoid lock evasion when reading
> > > pps_enable
> > >
> > > On Mon, May 13, 2024 at 4:02 AM Wei Fang <[email protected]>
> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> Sent: 2024??5??13?? 17:27
> To: Wei Fang <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Shenwei
> Wang <[email protected]>; Clark Wang <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
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 <[email protected]>
>
> Reviewed-by: Simon Horman <[email protected]>
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 <[email protected]>
> > Sent: 2024$BG/(B5$B7n(B13$BF|(B 17:27
> > To: Wei Fang <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected]; Shenwei
> Wang
> > <[email protected]>; Clark Wang <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > Cc: [email protected]; [email protected]
> > 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 <[email protected]>
> > > ---
> > > 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
> > >