Re: [PATCH] smc91c92_cs : add a spinlock to avoid race condition
From: Vaishali ThakkarDate: Fri, 19 Aug 2016 10:11:32 +0530 > > > On Friday 19 August 2016 09:37 AM, David Miller wrote: >> From: Pavel Andrianov >> 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 >> >> 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.
Re: [PATCH] smc91c92_cs : add a spinlock to avoid race condition
From: Vaishali Thakkar Date: Fri, 19 Aug 2016 10:11:32 +0530 > > > On Friday 19 August 2016 09:37 AM, David Miller wrote: >> From: Pavel Andrianov >> 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 >> >> 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.
Re: [PATCH] smc91c92_cs : add a spinlock to avoid race condition
On Friday 19 August 2016 09:37 AM, David Miller wrote: > From: Pavel Andrianov> 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 > > 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? > -- Vaishali
Re: [PATCH] smc91c92_cs : add a spinlock to avoid race condition
On Friday 19 August 2016 09:37 AM, David Miller wrote: > From: Pavel Andrianov > 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 > > 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? > -- Vaishali
Re: [PATCH] smc91c92_cs : add a spinlock to avoid race condition
From: Pavel AndrianovDate: 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 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.
Re: [PATCH] smc91c92_cs : add a spinlock to avoid race condition
From: Pavel Andrianov 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 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.
[PATCH] smc91c92_cs : add a spinlock to avoid race condition
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--- drivers/net/ethernet/smsc/smc91c92_cs.c | 3 +++ 1 file changed, 3 insertions(+) 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(>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(>lock, flags); } /*== -- 2.7.4
[PATCH] smc91c92_cs : add a spinlock to avoid race condition
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 --- drivers/net/ethernet/smsc/smc91c92_cs.c | 3 +++ 1 file changed, 3 insertions(+) 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(>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(>lock, flags); } /*== -- 2.7.4