| Message ID | 1471354746-27110-1-git-send-email-andrianov@ispras.ru |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Pavel Andrianov <andrianov@ispras.ru> Date: Tue, 16 Aug 2016 16:39:06 +0300 > smc_reset may be executed in parallel with timer function media_check. > To avoid data race in smc_set_xcvr a spinlock was added. > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Pavel Andrianov <andrianov@ispras.ru> This is not sufficient. You have to block basically the entire function, because both smc_reset and media_check program the bank selection so could corrupt eachother's register accesses.
On Friday 19 August 2016 09:37 AM, David Miller wrote: > From: Pavel Andrianov <andrianov@ispras.ru> > Date: Tue, 16 Aug 2016 16:39:06 +0300 > >> smc_reset may be executed in parallel with timer function media_check. >> To avoid data race in smc_set_xcvr a spinlock was added. >> >> Found by Linux Driver Verification project (linuxtesting.org). >> >> Signed-off-by: Pavel Andrianov <andrianov@ispras.ru> > > This is not sufficient. > > You have to block basically the entire function, because both > smc_reset and media_check program the bank selection so could > corrupt eachother's register accesses. Hmm, but then there is a use of udelay as well. Would it be still fine to acquire a spinlock on whole function? >
From: Vaishali Thakkar <vaishali.thakkar@oracle.com> Date: Fri, 19 Aug 2016 10:11:32 +0530 > > > On Friday 19 August 2016 09:37 AM, David Miller wrote: >> From: Pavel Andrianov <andrianov@ispras.ru> >> Date: Tue, 16 Aug 2016 16:39:06 +0300 >> >>> smc_reset may be executed in parallel with timer function media_check. >>> To avoid data race in smc_set_xcvr a spinlock was added. >>> >>> Found by Linux Driver Verification project (linuxtesting.org). >>> >>> Signed-off-by: Pavel Andrianov <andrianov@ispras.ru> >> >> This is not sufficient. >> >> You have to block basically the entire function, because both >> smc_reset and media_check program the bank selection so could >> corrupt eachother's register accesses. > > Hmm, but then there is a use of udelay as well. Would it be still > fine to acquire a spinlock on whole function? I don't know, but what I'm telling you is that you have to execute register programming these two functions do atomically so that the bank selection doesn't check get changed midstream.
diff --git a/drivers/net/ethernet/smsc/smc91c92_cs.c b/drivers/net/ethernet/smsc/smc91c92_cs.c index db3c696..69d865c 100644 --- a/drivers/net/ethernet/smsc/smc91c92_cs.c +++ b/drivers/net/ethernet/smsc/smc91c92_cs.c @@ -1637,6 +1637,7 @@ static void smc_reset(struct net_device *dev) unsigned int ioaddr = dev->base_addr; struct smc_private *smc = netdev_priv(dev); int i; + unsigned long flags; netdev_dbg(dev, "smc91c92 reset called.\n"); @@ -1647,6 +1648,7 @@ static void smc_reset(struct net_device *dev) outw(RCR_SOFTRESET, ioaddr + RCR); udelay(10); + spin_lock_irqsave(&smc->lock, flags); /* Clear the transmit and receive configuration registers. */ outw(RCR_CLEAR, ioaddr + RCR); outw(TCR_CLEAR, ioaddr + TCR); @@ -1699,6 +1701,7 @@ static void smc_reset(struct net_device *dev) SMC_SELECT_BANK(2); outw((IM_EPH_INT | IM_RX_OVRN_INT | IM_RCV_INT) << 8, ioaddr + INTERRUPT); + spin_unlock_irqrestore(&smc->lock, flags); } /*======================================================================
smc_reset may be executed in parallel with timer function media_check. To avoid data race in smc_set_xcvr a spinlock was added. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Pavel Andrianov <andrianov@ispras.ru> --- drivers/net/ethernet/smsc/smc91c92_cs.c | 3 +++ 1 file changed, 3 insertions(+)