There is no need to disable interrupts with a spin_lock_irqsave() in
bcm_sysport_tx_poll() since we are in softIRQ context already. Leave
interrupts enabled, thus giving a chance for the RX interrupts to be
processed.
This now makes bcm_sysport_tx_reclaim() equivalent to
bcm_sysport_tx_clean(), thus remove the former, and make
bcm_sysport_tx_reclaim_all() to use the latter.
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/net/ethernet/broadcom/bcmsysport.c | 30 ++++++----------------
1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index f07ac0e0af59..dfff0657ce8f 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -925,26 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
return pkts_compl;
}
-/* Locked version of the per-ring TX reclaim routine */
-static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
- struct bcm_sysport_tx_ring *ring)
-{
- struct netdev_queue *txq;
- unsigned int released;
- unsigned long flags;
-
- txq = netdev_get_tx_queue(priv->netdev, ring->index);
-
- spin_lock_irqsave(&ring->lock, flags);
- released = __bcm_sysport_tx_reclaim(priv, ring);
- if (released)
- netif_tx_wake_queue(txq);
-
- spin_unlock_irqrestore(&ring->lock, flags);
-
- return released;
-}
-
/* Locked version of the per-ring TX reclaim, but does not wake the queue */
static void bcm_sysport_tx_clean(struct bcm_sysport_priv *priv,
struct bcm_sysport_tx_ring *ring)
@@ -960,9 +940,15 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
{
struct bcm_sysport_tx_ring *ring =
container_of(napi, struct bcm_sysport_tx_ring, napi);
+ struct bcm_sysport_priv *priv = ring->priv;
unsigned int work_done = 0;
- work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
+ spin_lock(&ring->lock);
+ work_done = __bcm_sysport_tx_reclaim(priv, ring);
+ if (work_done)
+ netif_tx_wake_queue(netdev_get_tx_queue(priv->netdev,
+ ring->index));
+ spin_unlock(&ring->lock);
if (work_done == 0) {
napi_complete(napi);
@@ -984,7 +970,7 @@ static void bcm_sysport_tx_reclaim_all(struct bcm_sysport_priv *priv)
unsigned int q;
for (q = 0; q < priv->netdev->num_tx_queues; q++)
- bcm_sysport_tx_reclaim(priv, &priv->tx_rings[q]);
+ bcm_sysport_tx_clean(priv, &priv->tx_rings[q]);
}
static int bcm_sysport_poll(struct napi_struct *napi, int budget)
--
2.17.1
On 1/24/20 3:59 PM, Florian Fainelli wrote:
> There is no need to disable interrupts with a spin_lock_irqsave() in
> bcm_sysport_tx_poll() since we are in softIRQ context already. Leave
> interrupts enabled, thus giving a chance for the RX interrupts to be
> processed.
>
> This now makes bcm_sysport_tx_reclaim() equivalent to
> bcm_sysport_tx_clean(), thus remove the former, and make
> bcm_sysport_tx_reclaim_all() to use the latter.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> drivers/net/ethernet/broadcom/bcmsysport.c | 30 ++++++----------------
> 1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index f07ac0e0af59..dfff0657ce8f 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -925,26 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
> return pkts_compl;
> }
>
> -/* Locked version of the per-ring TX reclaim routine */
> -static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
> - struct bcm_sysport_tx_ring *ring)
> -{
> - struct netdev_queue *txq;
> - unsigned int released;
> - unsigned long flags;
> -
> - txq = netdev_get_tx_queue(priv->netdev, ring->index);
> -
> - spin_lock_irqsave(&ring->lock, flags);
> - released = __bcm_sysport_tx_reclaim(priv, ring);
> - if (released)
> - netif_tx_wake_queue(txq);
> -
> - spin_unlock_irqrestore(&ring->lock, flags);
> -
> - return released;
> -}
> -
> /* Locked version of the per-ring TX reclaim, but does not wake the queue */
> static void bcm_sysport_tx_clean(struct bcm_sysport_priv *priv,
> struct bcm_sysport_tx_ring *ring)
> @@ -960,9 +940,15 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
> {
> struct bcm_sysport_tx_ring *ring =
> container_of(napi, struct bcm_sysport_tx_ring, napi);
> + struct bcm_sysport_priv *priv = ring->priv;
> unsigned int work_done = 0;
>
> - work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
> + spin_lock(&ring->lock);
> + work_done = __bcm_sysport_tx_reclaim(priv, ring);
> + if (work_done)
> + netif_tx_wake_queue(netdev_get_tx_queue(priv->netdev,
> + ring->index));
> + spin_unlock(&ring->lock);
>
> if (work_done == 0) {
> napi_complete(napi);
> @@ -984,7 +970,7 @@ static void bcm_sysport_tx_reclaim_all(struct bcm_sysport_priv *priv)
> unsigned int q;
>
> for (q = 0; q < priv->netdev->num_tx_queues; q++)
> - bcm_sysport_tx_reclaim(priv, &priv->tx_rings[q]);
> + bcm_sysport_tx_clean(priv, &priv->tx_rings[q]);
> }
>
> static int bcm_sysport_poll(struct napi_struct *napi, int budget)
>
I am a bit confused by this patch, the changelog mixes hard and soft irqs.
This driver seems to call bcm_sysport_tx_reclaim_all() from hard irq handler
(INTRL2_0_TX_RING_FULL condition)
So it looks you need to acquire ring->lock with some _irqsave() variant when
bcm_sysport_tx_poll() is running (from BH context)
On 1/24/2020 5:35 PM, Eric Dumazet wrote:
>
>
> On 1/24/20 3:59 PM, Florian Fainelli wrote:
>> There is no need to disable interrupts with a spin_lock_irqsave() in
>> bcm_sysport_tx_poll() since we are in softIRQ context already. Leave
>> interrupts enabled, thus giving a chance for the RX interrupts to be
>> processed.
>>
>> This now makes bcm_sysport_tx_reclaim() equivalent to
>> bcm_sysport_tx_clean(), thus remove the former, and make
>> bcm_sysport_tx_reclaim_all() to use the latter.
>>
>> Signed-off-by: Florian Fainelli <[email protected]>
>> ---
>> drivers/net/ethernet/broadcom/bcmsysport.c | 30 ++++++----------------
>> 1 file changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index f07ac0e0af59..dfff0657ce8f 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -925,26 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>> return pkts_compl;
>> }
>>
>> -/* Locked version of the per-ring TX reclaim routine */
>> -static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>> - struct bcm_sysport_tx_ring *ring)
>> -{
>> - struct netdev_queue *txq;
>> - unsigned int released;
>> - unsigned long flags;
>> -
>> - txq = netdev_get_tx_queue(priv->netdev, ring->index);
>> -
>> - spin_lock_irqsave(&ring->lock, flags);
>> - released = __bcm_sysport_tx_reclaim(priv, ring);
>> - if (released)
>> - netif_tx_wake_queue(txq);
>> -
>> - spin_unlock_irqrestore(&ring->lock, flags);
>> -
>> - return released;
>> -}
>> -
>> /* Locked version of the per-ring TX reclaim, but does not wake the queue */
>> static void bcm_sysport_tx_clean(struct bcm_sysport_priv *priv,
>> struct bcm_sysport_tx_ring *ring)
>> @@ -960,9 +940,15 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
>> {
>> struct bcm_sysport_tx_ring *ring =
>> container_of(napi, struct bcm_sysport_tx_ring, napi);
>> + struct bcm_sysport_priv *priv = ring->priv;
>> unsigned int work_done = 0;
>>
>> - work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
>> + spin_lock(&ring->lock);
>> + work_done = __bcm_sysport_tx_reclaim(priv, ring);
>> + if (work_done)
>> + netif_tx_wake_queue(netdev_get_tx_queue(priv->netdev,
>> + ring->index));
>> + spin_unlock(&ring->lock);
>>
>> if (work_done == 0) {
>> napi_complete(napi);
>> @@ -984,7 +970,7 @@ static void bcm_sysport_tx_reclaim_all(struct bcm_sysport_priv *priv)
>> unsigned int q;
>>
>> for (q = 0; q < priv->netdev->num_tx_queues; q++)
>> - bcm_sysport_tx_reclaim(priv, &priv->tx_rings[q]);
>> + bcm_sysport_tx_clean(priv, &priv->tx_rings[q]);
>> }
>>
>> static int bcm_sysport_poll(struct napi_struct *napi, int budget)
>>
>
> I am a bit confused by this patch, the changelog mixes hard and soft irqs.
>
> This driver seems to call bcm_sysport_tx_reclaim_all() from hard irq handler
> (INTRL2_0_TX_RING_FULL condition)
>
> So it looks you need to acquire ring->lock with some _irqsave() variant when
> bcm_sysport_tx_poll() is running (from BH context)
You are right, I completely missed that path and the very reason why
this is spin_lock_irqsave() in the first place... time to get some sleep.
Thanks!
--
Florian