| Message ID | 1484775745-14050-1-git-send-email-timur@codeaurora.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Timur Tabi <timur@codeaurora.org> Date: Wed, 18 Jan 2017 15:42:25 -0600 > During reset, functions emac_mac_down() and emac_mac_up() are called, > so we don't want to free and claim the IRQ unnecessarily. Move those > operations to open/close. > > Signed-off-by: Timur Tabi <timur@codeaurora.org> If you don't tell me what tree this is against in your Subject line, I have to guess. I don't like guessing.
On 01/20/2017 11:19 AM, David Miller wrote: > If you don't tell me what tree this is against in your Subject line, > I have to guess. > > I don't like guessing. Sorry, this is for net-next. I will specify the target tree in all future patches.
Hi, On 18.01.2017 22:42, Timur Tabi wrote: > @@ -1029,8 +1017,6 @@ void emac_mac_down(struct emac_adapter *adpt) > */ > writel(DIS_INT, adpt->base + EMAC_INT_STATUS); > writel(0, adpt->base + EMAC_INT_MASK); > - synchronize_irq(adpt->irq.irq); There is no reason to remove the irq synchronization, is it? Note that the desriptors are freed after that so we must be sure that the irq handler is not running any more. Regards, Lino
On 01/20/2017 02:44 PM, Lino Sanfilippo wrote: > > > On 18.01.2017 22:42, Timur Tabi wrote: >> @@ -1029,8 +1017,6 @@ void emac_mac_down(struct emac_adapter *adpt) >> */ >> writel(DIS_INT, adpt->base + EMAC_INT_STATUS); >> writel(0, adpt->base + EMAC_INT_MASK); >> - synchronize_irq(adpt->irq.irq); > > There is no reason to remove the irq synchronization, is it? > Note that the desriptors are freed after that so we must be sure that > the irq handler is not running any more. I'm moving it to stay with the free_irq(). @@ -283,6 +292,9 @@ static int emac_close(struct net_device *netdev) mutex_lock(&adpt->reset_lock); + synchronize_irq(adpt->irq.irq); + free_irq(adpt->irq.irq, &adpt->irq); + However, I'll admit that I don't know why we call synchronize_irq() at all.
On 20.01.2017 22:05, Timur Tabi wrote: > On 01/20/2017 02:44 PM, Lino Sanfilippo wrote: >> >> >> On 18.01.2017 22:42, Timur Tabi wrote: >>> @@ -1029,8 +1017,6 @@ void emac_mac_down(struct emac_adapter *adpt) >>> */ >>> writel(DIS_INT, adpt->base + EMAC_INT_STATUS); >>> writel(0, adpt->base + EMAC_INT_MASK); >>> - synchronize_irq(adpt->irq.irq); >> >> There is no reason to remove the irq synchronization, is it? >> Note that the desriptors are freed after that so we must be sure that >> the irq handler is not running any more. > > I'm moving it to stay with the free_irq(). > > @@ -283,6 +292,9 @@ static int emac_close(struct net_device *netdev) > > mutex_lock(&adpt->reset_lock); > > + synchronize_irq(adpt->irq.irq); > + free_irq(adpt->irq.irq, &adpt->irq); > + > > However, I'll admit that I don't know why we call synchronize_irq() at > all. > free_irq() will call synchronize_irq() if necessary, so it is pointless to call synchronize_irq() right before free_irq(). In emac_mac_down() however we need synchronize_irq(), since it ensures that the irq handler is not running any more when it (synchronize_irq) returns. Regards, Lino
On 01/20/2017 03:31 PM, Lino Sanfilippo wrote: > > In emac_mac_down() however we need synchronize_irq(), since it ensures > that the irq > handler is not running any more when it (synchronize_irq) returns. So in general, if a driver disables a interrupt but does not free it, it should call synchronize_irq()?
On 20.01.2017 22:36, Timur Tabi wrote: > On 01/20/2017 03:31 PM, Lino Sanfilippo wrote: >> >> In emac_mac_down() however we need synchronize_irq(), since it ensures >> that the irq >> handler is not running any more when it (synchronize_irq) returns. > > So in general, if a driver disables a interrupt but does not free it, > it should call synchronize_irq()? > Yes, thats right. Regards, Lino
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c index 384e1be..ab3a3c0 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c +++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c @@ -314,8 +314,6 @@ struct emac_skb_cb { RX_PKT_INT2 |\ RX_PKT_INT3) -#define EMAC_MAC_IRQ_RES "core0" - void emac_mac_multicast_addr_set(struct emac_adapter *adpt, u8 *addr) { u32 crc32, bit, reg, mta; @@ -977,26 +975,16 @@ static void emac_adjust_link(struct net_device *netdev) int emac_mac_up(struct emac_adapter *adpt) { struct net_device *netdev = adpt->netdev; - struct emac_irq *irq = &adpt->irq; int ret; emac_mac_rx_tx_ring_reset_all(adpt); emac_mac_config(adpt); - - ret = request_irq(irq->irq, emac_isr, 0, EMAC_MAC_IRQ_RES, irq); - if (ret) { - netdev_err(adpt->netdev, "could not request %s irq\n", - EMAC_MAC_IRQ_RES); - return ret; - } - emac_mac_rx_descs_refill(adpt, &adpt->rx_q); ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link, PHY_INTERFACE_MODE_SGMII); if (ret) { netdev_err(adpt->netdev, "could not connect phy\n"); - free_irq(irq->irq, irq); return ret; } @@ -1029,8 +1017,6 @@ void emac_mac_down(struct emac_adapter *adpt) */ writel(DIS_INT, adpt->base + EMAC_INT_STATUS); writel(0, adpt->base + EMAC_INT_MASK); - synchronize_irq(adpt->irq.irq); - free_irq(adpt->irq.irq, &adpt->irq); phy_disconnect(adpt->phydev); diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c index b74ec7f..5926e54 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac.c +++ b/drivers/net/ethernet/qualcomm/emac/emac.c @@ -256,18 +256,27 @@ static int emac_change_mtu(struct net_device *netdev, int new_mtu) static int emac_open(struct net_device *netdev) { struct emac_adapter *adpt = netdev_priv(netdev); + struct emac_irq *irq = &adpt->irq; int ret; + ret = request_irq(irq->irq, emac_isr, 0, "emac-core0", irq); + if (ret) { + netdev_err(adpt->netdev, "could not request emac-core0 irq\n"); + return ret; + } + /* allocate rx/tx dma buffer & descriptors */ ret = emac_mac_rx_tx_rings_alloc_all(adpt); if (ret) { netdev_err(adpt->netdev, "error allocating rx/tx rings\n"); + free_irq(irq->irq, irq); return ret; } ret = emac_mac_up(adpt); if (ret) { emac_mac_rx_tx_rings_free_all(adpt); + free_irq(irq->irq, irq); return ret; } @@ -283,6 +292,9 @@ static int emac_close(struct net_device *netdev) mutex_lock(&adpt->reset_lock); + synchronize_irq(adpt->irq.irq); + free_irq(adpt->irq.irq, &adpt->irq); + emac_mac_down(adpt); emac_mac_rx_tx_rings_free_all(adpt); diff --git a/drivers/net/ethernet/qualcomm/emac/emac.h b/drivers/net/ethernet/qualcomm/emac/emac.h index 1368440..2725507 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac.h +++ b/drivers/net/ethernet/qualcomm/emac/emac.h @@ -331,7 +331,6 @@ struct emac_adapter { int emac_reinit_locked(struct emac_adapter *adpt); void emac_reg_update32(void __iomem *addr, u32 mask, u32 val); -irqreturn_t emac_isr(int irq, void *data); void emac_set_ethtool_ops(struct net_device *netdev); void emac_update_hw_stats(struct emac_adapter *adpt);
During reset, functions emac_mac_down() and emac_mac_up() are called, so we don't want to free and claim the IRQ unnecessarily. Move those operations to open/close. Signed-off-by: Timur Tabi <timur@codeaurora.org> --- drivers/net/ethernet/qualcomm/emac/emac-mac.c | 14 -------------- drivers/net/ethernet/qualcomm/emac/emac.c | 12 ++++++++++++ drivers/net/ethernet/qualcomm/emac/emac.h | 1 - 3 files changed, 12 insertions(+), 15 deletions(-)