[PATCH net] net: fec: avoid lock evasion when reading pps_enable

[PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Wei Fang]

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) {
+		spin_unlock_irqrestore(&fep->tmreg_lock, flags);
+		return 0;
+	}
+
 	if (enable) {
 		/* clear capture or output compare interrupt status if have.
 		 */
-- 
2.34.1

Re: [PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Eric Dumazet]

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
>

RE: [PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Wei Fang]

> -----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
> >

Re: [PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Eric Dumazet]

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.

[PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Hariprasad Kelam]

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
> 

RE: [PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Wei Fang]

> -----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.

RE: [PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Wei Fang]

> -----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
> >

Re: [PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Simon Horman]

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>

Re: [PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Simon Horman]

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.

RE: [PATCH net] net: fec: avoid lock evasion when reading pps_enable

[Hariprasad Kelam]

> > -----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
> > >