RE: [PATCH net-next] net: fec: Convert fec driver to use lock guards

[Markus Elfring <Markus.Elfring@web.de>]

…
> > > +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> > > @@ -99,18 +99,17 @@
> > >   */
> > >  static int fec_ptp_enable_pps(struct fec_enet_private *fep, uint enable)
> > >  {
> > > -	unsigned long flags;
> > >  	u32 val, tempval;
> > >  	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);
> > > +	guard(spinlock_irqsave)(&fep->tmreg_lock);
> > > +
> > > +	if (fep->pps_enable == enable)
> > > +		return 0;
> >
> > This is not obviously correct. Why has this condition moved?
> >
> As you see, the assignment of ' pps_enable ' is protected by the 'tmreg_lock',
> But the read operation of 'pps_enable' was not protected by the lock, so the
> Coverity tool will complain a LOCK EVASION warning which may cause data
> race to occur when running in a multithreaded environment.

Should such information trigger the addition of any corresponding tags
(like “Fixes” and “Cc”)?


>                                                            Of course, this
> data race issue is almost impossible, so I modified it by the way. Because I don't
> really want to fix it through another patch, unless you insist on doing so.

Would you like to take the known advice “Solve only one problem per patch”
better into account?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc4#n81

Please take another look at further approaches for the presentation of
similar “change combinations”.

Regards,
Markus