Re: [PATCH v2] net: davicom: dm9000: Avoid spinlock recursion during dm9000_timeout routine

[xiang fei]

Hi,
  In this patch, I don't change the routine of dm9000_phy_write().
  I just abstract the real phy operation from dm9000_phy_write()
  to make a new function dm9000_phy_write_reg(),
  which contains no lock.

  It is called in dm9000_phy_write() or dm9000_init_dm9000().
  Since dm9000_timeout() holds the main spinlock through the
  entire routine, dm9000_phy_write_reg() called in this routine is
  protected from another thread. I think there is no need to
  acquire any locks.

  The only problem is dm9000_init_dm9000() may be called
  in dm9000_open(). If another thread calls dm9000_open()
  in timeout may cause race condition.

  dm9000_init_dm9000() needs lock to avoid race condition,
  did you mean that?
  Thanks!

Re: [PATCH v2] net: davicom: dm9000: Avoid spinlock recursion during dm9000_timeout routine

[David Miller]

From: Liu Xiang 
Date: Tue,  6 Jun 2017 22:17:06 +0800

> On the DM9000B, dm9000_phy_write() is called after the main spinlock
> is held, during the dm9000_timeout() routine. Spinlock recursion
> occurs because the main spinlock is requested again in
> dm9000_phy_write(). So spinlock should be avoided in phy operation
> during the dm9000_timeout() routine.
> 
> ---
> v2:
>dm9000_phy_write_reg is extracted from dm9000_phy_write, with no lock,
>do the real phy operation.

I told you during your first submission that this is racy.

Conditional locking in multithreaded driver almost never works
properly.

Once we enter the timeout, another asynchronous thread of control (an
interrupt, timer, whatever) can try to do a PHY operation and skip the
locking because the timeout boolean is set.

That's not right.

You will need to find a more correct solution to this locking problem.

Thanks.

[PATCH v2] net: davicom: dm9000: Avoid spinlock recursion during dm9000_timeout routine

[Liu Xiang]

On the DM9000B, dm9000_phy_write() is called after the main spinlock
is held, during the dm9000_timeout() routine. Spinlock recursion
occurs because the main spinlock is requested again in
dm9000_phy_write(). So spinlock should be avoided in phy operation
during the dm9000_timeout() routine.

---
v2:
   dm9000_phy_write_reg is extracted from dm9000_phy_write, with no lock,
   do the real phy operation.
---

Signed-off-by: Liu Xiang 
---
 drivers/net/ethernet/davicom/dm9000.c | 37 +--
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/davicom/dm9000.c 
b/drivers/net/ethernet/davicom/dm9000.c
index 008dc81..0aa2900 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -327,21 +327,13 @@ static void dm9000_msleep(struct board_info *db, unsigned 
int ms)
return ret;
 }
 
-/* Write a word to phyxcer */
 static void
-dm9000_phy_write(struct net_device *dev,
-int phyaddr_unused, int reg, int value)
+dm9000_phy_write_reg(struct net_device *dev,
+int phyaddr_unused, int reg, int value)
 {
struct board_info *db = netdev_priv(dev);
-   unsigned long flags;
unsigned long reg_save;
 
-   dm9000_dbg(db, 5, "phy_write[%02x] = %04x\n", reg, value);
-   if (!db->in_timeout)
-   mutex_lock(>addr_lock);
-
-   spin_lock_irqsave(>lock, flags);
-
/* Save previous register address */
reg_save = readb(db->io_addr);
 
@@ -356,17 +348,32 @@ static void dm9000_msleep(struct board_info *db, unsigned 
int ms)
iow(db, DM9000_EPCR, EPCR_EPOS | EPCR_ERPRW);
 
writeb(reg_save, db->io_addr);
-   spin_unlock_irqrestore(>lock, flags);
 
-   dm9000_msleep(db, 1);   /* Wait write complete */
+   mdelay(1);  /* Wait write complete */
 
-   spin_lock_irqsave(>lock, flags);
reg_save = readb(db->io_addr);
 
iow(db, DM9000_EPCR, 0x0);  /* Clear phyxcer write command */
 
/* restore the previous address */
writeb(reg_save, db->io_addr);
+}
+
+/* Write a word to phyxcer */
+static void
+dm9000_phy_write(struct net_device *dev,
+int phyaddr_unused, int reg, int value)
+{
+   struct board_info *db = netdev_priv(dev);
+   unsigned long flags;
+
+   dm9000_dbg(db, 5, "phy_write[%02x] = %04x\n", reg, value);
+   if (!db->in_timeout)
+   mutex_lock(>addr_lock);
+
+   spin_lock_irqsave(>lock, flags);
+
+   dm9000_phy_write_reg(dev, phyaddr_unused, reg, value);
 
spin_unlock_irqrestore(>lock, flags);
if (!db->in_timeout)
@@ -933,8 +940,8 @@ static unsigned char dm9000_type_to_char(enum dm9000_type 
type)
 * manual phy reset, and setting init params.
 */
if (db->type == TYPE_DM9000B) {
-   dm9000_phy_write(dev, 0, MII_BMCR, BMCR_RESET);
-   dm9000_phy_write(dev, 0, MII_DM_DSPCR, DSPCR_INIT_PARAM);
+   dm9000_phy_write_reg(dev, 0, MII_BMCR, BMCR_RESET);
+   dm9000_phy_write_reg(dev, 0, MII_DM_DSPCR, DSPCR_INIT_PARAM);
}
 
ncr = (db->flags & DM9000_PLATF_EXT_PHY) ? NCR_EXT_PHY : 0;
-- 
1.9.1