Skip to content

Commit

Permalink
net: ks8851-ml: Fix IRQ handling and locking
Browse files Browse the repository at this point in the history
The KS8851 requires that packet RX and TX are mutually exclusive.
Currently, the driver hopes to achieve this by disabling interrupt
from the card by writing the card registers and by disabling the
interrupt on the interrupt controller. This however is racy on SMP.

Replace this approach by expanding the spinlock used around the
ks_start_xmit() TX path to ks_irq() RX path to assure true mutual
exclusion and remove the interrupt enabling/disabling, which is
now not needed anymore. Furthermore, disable interrupts also in
ks_net_stop(), which was missing before.

Note that a massive improvement here would be to re-use the KS8851
driver approach, which is to move the TX path into a worker thread,
interrupt handling to threaded interrupt, and synchronize everything
with mutexes, but that would be a much bigger rework, for a separate
patch.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Stetiar <ynezz@true.cz>
Cc: YueHaibing <yuehaibing@huawei.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Marek Vasut authored and David S. Miller committed Feb 24, 2020
1 parent 52df1e5 commit 4434341
Showing 1 changed file with 8 additions and 6 deletions.
14 changes: 8 additions & 6 deletions drivers/net/ethernet/micrel/ks8851_mll.c
Original file line number Diff line number Diff line change
Expand Up @@ -513,14 +513,17 @@ static irqreturn_t ks_irq(int irq, void *pw)
{
struct net_device *netdev = pw;
struct ks_net *ks = netdev_priv(netdev);
unsigned long flags;
u16 status;

spin_lock_irqsave(&ks->statelock, flags);
/*this should be the first in IRQ handler */
ks_save_cmd_reg(ks);

status = ks_rdreg16(ks, KS_ISR);
if (unlikely(!status)) {
ks_restore_cmd_reg(ks);
spin_unlock_irqrestore(&ks->statelock, flags);
return IRQ_NONE;
}

Expand All @@ -546,6 +549,7 @@ static irqreturn_t ks_irq(int irq, void *pw)
ks->netdev->stats.rx_over_errors++;
/* this should be the last in IRQ handler*/
ks_restore_cmd_reg(ks);
spin_unlock_irqrestore(&ks->statelock, flags);
return IRQ_HANDLED;
}

Expand Down Expand Up @@ -615,6 +619,7 @@ static int ks_net_stop(struct net_device *netdev)

/* shutdown RX/TX QMU */
ks_disable_qmu(ks);
ks_disable_int(ks);

/* set powermode to soft power down to save power */
ks_set_powermode(ks, PMECR_PM_SOFTDOWN);
Expand Down Expand Up @@ -671,10 +676,9 @@ static netdev_tx_t ks_start_xmit(struct sk_buff *skb, struct net_device *netdev)
{
netdev_tx_t retv = NETDEV_TX_OK;
struct ks_net *ks = netdev_priv(netdev);
unsigned long flags;

disable_irq(netdev->irq);
ks_disable_int(ks);
spin_lock(&ks->statelock);
spin_lock_irqsave(&ks->statelock, flags);

/* Extra space are required:
* 4 byte for alignment, 4 for status/length, 4 for CRC
Expand All @@ -688,9 +692,7 @@ static netdev_tx_t ks_start_xmit(struct sk_buff *skb, struct net_device *netdev)
dev_kfree_skb(skb);
} else
retv = NETDEV_TX_BUSY;
spin_unlock(&ks->statelock);
ks_enable_int(ks);
enable_irq(netdev->irq);
spin_unlock_irqrestore(&ks->statelock, flags);
return retv;
}

Expand Down

0 comments on commit 4434341

Please sign in to comment.