[PATCH] net: macb: Restart tx only if queue pointer is lagging

[PATCH] net: macb: Restart tx only if queue pointer is lagging

[Tomas Melin]

commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
added support for restarting transmission. Restarting tx does not work
in case controller asserts TXUBR interrupt and TQBP is already at the end
of the tx queue. In that situation, restarting tx will immediately cause
assertion of another TXUBR interrupt. The driver will end up in an infinite
interrupt loop which it cannot break out of.

For cases where TQBP is at the end of the tx queue, instead
only clear TXUBR interrupt. As more data gets pushed to the queue,
transmission will resume.

This issue was observed on a Xilinx Zynq based board. During stress test of
the network interface, driver would get stuck on interrupt loop
within seconds or minutes causing CPU to stall.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 800d5ced5800..e475be29845c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue *queue)
 	unsigned int head = queue->tx_head;
 	unsigned int tail = queue->tx_tail;
 	struct macb *bp = queue->bp;
+	unsigned int head_idx, tbqp;
 
 	if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
 		queue_writel(queue, ISR, MACB_BIT(TXUBR));
@@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue *queue)
 	if (head == tail)
 		return;
 
+	tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
+	tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
+	head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, head));
+
+	if (tbqp == head_idx)
+		return;
+
 	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 }
 
-- 
2.35.1

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Claudiu.Beznea]

On 25.03.2022 08:50, Tomas Melin wrote:
> [Some people who received this message don't often get email from tomas.melin@vaisala.com. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
> added support for restarting transmission. Restarting tx does not work
> in case controller asserts TXUBR interrupt and TQBP is already at the end
> of the tx queue. In that situation, restarting tx will immediately cause
> assertion of another TXUBR interrupt. The driver will end up in an infinite
> interrupt loop which it cannot break out of.
> 
> For cases where TQBP is at the end of the tx queue, instead
> only clear TXUBR interrupt. As more data gets pushed to the queue,
> transmission will resume.
> 
> This issue was observed on a Xilinx Zynq based board. During stress test of
> the network interface, driver would get stuck on interrupt loop
> within seconds or minutes causing CPU to stall.
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 800d5ced5800..e475be29845c 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue *queue)
>         unsigned int head = queue->tx_head;
>         unsigned int tail = queue->tx_tail;
>         struct macb *bp = queue->bp;
> +       unsigned int head_idx, tbqp;
> 
>         if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>                 queue_writel(queue, ISR, MACB_BIT(TXUBR));
> @@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue *queue)
>         if (head == tail)
>                 return;
> 
> +       tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
> +       tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
> +       head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, head));
> +
> +       if (tbqp == head_idx)
> +               return;
> +

This looks like TBQP is not advancing though there are packets in the
software queues (head != tail). Packets are added in the software queues on
TX path and removed when TX was done for them.

Maybe TX_WRAP is missing on one TX descriptor? Few months ago while
investigating some other issues on this I found that this might be missed
on one descriptor [1] but haven't managed to make it break at that point
anyhow.

Could you check on your side if this is solving your issue?

	/* Set 'TX_USED' bit in buffer descriptor at tx_head position
	 * to set the end of TX queue
	 */
	i = tx_head;
	entry = macb_tx_ring_wrap(bp, i);
	ctrl = MACB_BIT(TX_USED);
+	if (entry == bp->tx_ring_size - 1)
+		ctrl |= MACB_BIT(TX_WRAP);
	desc = macb_tx_desc(queue, entry);
	desc->ctrl = ctrl;

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/cadence/macb_main.c#n1958

>         macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>  }
> 
> --
> 2.35.1
> 

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Tomas Melin]

Hi,

On 25/03/2022 10:57, Claudiu.Beznea@microchip.com wrote:
> On 25.03.2022 08:50, Tomas Melin wrote:
>> [Some people who received this message don't often get email from tomas.melin@vaisala.com. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
>> added support for restarting transmission. Restarting tx does not work
>> in case controller asserts TXUBR interrupt and TQBP is already at the end
>> of the tx queue. In that situation, restarting tx will immediately cause
>> assertion of another TXUBR interrupt. The driver will end up in an infinite
>> interrupt loop which it cannot break out of.
>>
>> For cases where TQBP is at the end of the tx queue, instead
>> only clear TXUBR interrupt. As more data gets pushed to the queue,
>> transmission will resume.
>>
>> This issue was observed on a Xilinx Zynq based board. During stress test of
>> the network interface, driver would get stuck on interrupt loop
>> within seconds or minutes causing CPU to stall.
>>
>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>> ---
>>   drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 800d5ced5800..e475be29845c 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue *queue)
>>          unsigned int head = queue->tx_head;
>>          unsigned int tail = queue->tx_tail;
>>          struct macb *bp = queue->bp;
>> +       unsigned int head_idx, tbqp;
>>
>>          if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>>                  queue_writel(queue, ISR, MACB_BIT(TXUBR));
>> @@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue *queue)
>>          if (head == tail)
>>                  return;
>>
>> +       tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
>> +       tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
>> +       head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, head));
>> +
>> +       if (tbqp == head_idx)
>> +               return;
>> +
> 
> This looks like TBQP is not advancing though there are packets in the
> software queues (head != tail). Packets are added in the software queues on
> TX path and removed when TX was done for them.

TBQP is at the end of the queue, and that matches with tx_head 
maintained by driver. So seems controller is happily at end marker,
and when restarted immediately sees that end marker used tag and 
triggers an interrupt again.

Also when looking at the buffer descriptor memory it shows that all 
frames between tx_tail and tx_head have been marked as used.

GEM documentation says "transmission is restarted from
the first buffer descriptor of the frame being transmitted when the 
transmit start bit is rewritten" but since all frames are already marked
as transmitted, restarting wont help. Adding this additional check will 
help for the issue we have.


> 
> Maybe TX_WRAP is missing on one TX descriptor? Few months ago while
> investigating some other issues on this I found that this might be missed
> on one descriptor [1] but haven't managed to make it break at that point
> anyhow.
> 
> Could you check on your side if this is solving your issue?

I have seen that we can get stuck at any location in the ring buffer, so 
this does not seem to be the case here. I can try though if it would 
have any effect.

thanks,
Tomas


> 
> 	/* Set 'TX_USED' bit in buffer descriptor at tx_head position
> 	 * to set the end of TX queue
> 	 */
> 	i = tx_head;
> 	entry = macb_tx_ring_wrap(bp, i);
> 	ctrl = MACB_BIT(TX_USED);
> +	if (entry == bp->tx_ring_size - 1)
> +		ctrl |= MACB_BIT(TX_WRAP);
> 	desc = macb_tx_desc(queue, entry);
> 	desc->ctrl = ctrl;
> 
> [1]
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fnet%2Fethernet%2Fcadence%2Fmacb_main.c%23n1958&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C2fe72e2a6a874b5279a708da0e3d7852%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637837954434714462%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=hijcfj3TnOxj12dhG0Q8d0AJNFNBJSxtEjOTkCoZThI%3D&amp;reserved=0
> 
>>          macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>>   }
>>
>> --
>> 2.35.1
>>
> 

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Claudiu.Beznea]

On 25.03.2022 11:35, Tomas Melin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi,
> 
> On 25/03/2022 10:57, Claudiu.Beznea@microchip.com wrote:
>> On 25.03.2022 08:50, Tomas Melin wrote:
>>> [Some people who received this message don't often get email from
>>> tomas.melin@vaisala.com. Learn why this is important at
>>> http://aka.ms/LearnAboutSenderIdentification.]
>>>
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
>>> added support for restarting transmission. Restarting tx does not work
>>> in case controller asserts TXUBR interrupt and TQBP is already at the end
>>> of the tx queue. In that situation, restarting tx will immediately cause
>>> assertion of another TXUBR interrupt. The driver will end up in an infinite
>>> interrupt loop which it cannot break out of.
>>>
>>> For cases where TQBP is at the end of the tx queue, instead
>>> only clear TXUBR interrupt. As more data gets pushed to the queue,
>>> transmission will resume.
>>>
>>> This issue was observed on a Xilinx Zynq based board. During stress test of
>>> the network interface, driver would get stuck on interrupt loop
>>> within seconds or minutes causing CPU to stall.
>>>
>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>> ---
>>>   drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>>> b/drivers/net/ethernet/cadence/macb_main.c
>>> index 800d5ced5800..e475be29845c 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>> @@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue *queue)
>>>          unsigned int head = queue->tx_head;
>>>          unsigned int tail = queue->tx_tail;
>>>          struct macb *bp = queue->bp;
>>> +       unsigned int head_idx, tbqp;
>>>
>>>          if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>>>                  queue_writel(queue, ISR, MACB_BIT(TXUBR));
>>> @@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue
>>> *queue)
>>>          if (head == tail)
>>>                  return;
>>>
>>> +       tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
>>> +       tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
>>> +       head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, head));
>>> +
>>> +       if (tbqp == head_idx)
>>> +               return;
>>> +
>>
>> This looks like TBQP is not advancing though there are packets in the
>> software queues (head != tail). Packets are added in the software queues on
>> TX path and removed when TX was done for them.
> 
> TBQP is at the end of the queue, and that matches with tx_head
> maintained by driver. So seems controller is happily at end marker,
> and when restarted immediately sees that end marker used tag and
> triggers an interrupt again.
> 
> Also when looking at the buffer descriptor memory it shows that all
> frames between tx_tail and tx_head have been marked as used.

I see. Controller sets TX_USED on the 1st descriptor of the transmitted
frame. If there were packets with one descriptor enqueued that should mean
controller did its job.

head != tail on software data structures when receiving TXUBR interrupt and
all descriptors in queue have TX_USED bit set might signal that  a
descriptor is not updated to CPU on TCOMP interrupt when CPU uses it and
thus driver doesn't treat a TCOMP interrupt. See the above code on
macb_tx_interrupt():

desc = macb_tx_desc(queue, tail);

/* Make hw descriptor updates visible to CPU */
rmb();

ctrl = desc->ctrl;

/* TX_USED bit is only set by hardware on the very first buffer
* descriptor of the transmitted frame.
*/

if (!(ctrl & MACB_BIT(TX_USED)))
	break;


> 
> GEM documentation says "transmission is restarted from
> the first buffer descriptor of the frame being transmitted when the
> transmit start bit is rewritten" but since all frames are already marked
> as transmitted, restarting wont help. Adding this additional check will
> help for the issue we have.
> 

I see but according to your description (all descriptors treated by
controller) if no packets are enqueued for TX after:

+       if (tbqp == head_idx)
+               return;
+

there are some SKBs that were correctly treated by controller but not freed
by software (they are freed on macb_tx_unmap() called from
macb_tx_interrupt()). They will be freed on next TCOMP interrupt for other
packets being transmitted.

> 
>>
>> Maybe TX_WRAP is missing on one TX descriptor? Few months ago while
>> investigating some other issues on this I found that this might be missed
>> on one descriptor [1] but haven't managed to make it break at that point
>> anyhow.
>>
>> Could you check on your side if this is solving your issue?
> 
> I have seen that we can get stuck at any location in the ring buffer, so
> this does not seem to be the case here. I can try though if it would
> have any effect.

I was thinking that having small packets there is high chance that TBQP to
not reach a descriptor with wrap bit set due to the code pointed in my
previous email.

Thank you,
Claudiu Beznea

> 
> thanks,
> Tomas
> 
> 
>>
>>       /* Set 'TX_USED' bit in buffer descriptor at tx_head position
>>        * to set the end of TX queue
>>        */
>>       i = tx_head;
>>       entry = macb_tx_ring_wrap(bp, i);
>>       ctrl = MACB_BIT(TX_USED);
>> +     if (entry == bp->tx_ring_size - 1)
>> +             ctrl |= MACB_BIT(TX_WRAP);
>>       desc = macb_tx_desc(queue, entry);
>>       desc->ctrl = ctrl;
>>
>> [1]
>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fnet%2Fethernet%2Fcadence%2Fmacb_main.c%23n1958&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C2fe72e2a6a874b5279a708da0e3d7852%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637837954434714462%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=hijcfj3TnOxj12dhG0Q8d0AJNFNBJSxtEjOTkCoZThI%3D&amp;reserved=0
>>
>>
>>>          macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>>>   }
>>>
>>> -- 
>>> 2.35.1
>>>
>>

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Tomas Melin]

Hi,

On 25/03/2022 15:41, Claudiu.Beznea@microchip.com wrote:
> On 25.03.2022 11:35, Tomas Melin wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> Hi,
>>
>> On 25/03/2022 10:57, Claudiu.Beznea@microchip.com wrote:
>>> On 25.03.2022 08:50, Tomas Melin wrote:
>>>> [Some people who received this message don't often get email from
>>>> tomas.melin@vaisala.com. Learn why this is important at
>>>> http://aka.ms/LearnAboutSenderIdentification.]
>>>>
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>>> the content is safe
>>>>
>>>> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
>>>> added support for restarting transmission. Restarting tx does not work
>>>> in case controller asserts TXUBR interrupt and TQBP is already at the end
>>>> of the tx queue. In that situation, restarting tx will immediately cause
>>>> assertion of another TXUBR interrupt. The driver will end up in an infinite
>>>> interrupt loop which it cannot break out of.
>>>>
>>>> For cases where TQBP is at the end of the tx queue, instead
>>>> only clear TXUBR interrupt. As more data gets pushed to the queue,
>>>> transmission will resume.
>>>>
>>>> This issue was observed on a Xilinx Zynq based board. During stress test of
>>>> the network interface, driver would get stuck on interrupt loop
>>>> within seconds or minutes causing CPU to stall.
>>>>
>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>> ---
>>>>    drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>>>> b/drivers/net/ethernet/cadence/macb_main.c
>>>> index 800d5ced5800..e475be29845c 100644
>>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>>> @@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue *queue)
>>>>           unsigned int head = queue->tx_head;
>>>>           unsigned int tail = queue->tx_tail;
>>>>           struct macb *bp = queue->bp;
>>>> +       unsigned int head_idx, tbqp;
>>>>
>>>>           if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>>>>                   queue_writel(queue, ISR, MACB_BIT(TXUBR));
>>>> @@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue
>>>> *queue)
>>>>           if (head == tail)
>>>>                   return;
>>>>
>>>> +       tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
>>>> +       tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
>>>> +       head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, head));
>>>> +
>>>> +       if (tbqp == head_idx)
>>>> +               return;
>>>> +
>>>
>>> This looks like TBQP is not advancing though there are packets in the
>>> software queues (head != tail). Packets are added in the software queues on
>>> TX path and removed when TX was done for them.
>>
>> TBQP is at the end of the queue, and that matches with tx_head
>> maintained by driver. So seems controller is happily at end marker,
>> and when restarted immediately sees that end marker used tag and
>> triggers an interrupt again.
>>
>> Also when looking at the buffer descriptor memory it shows that all
>> frames between tx_tail and tx_head have been marked as used.
> 
> I see. Controller sets TX_USED on the 1st descriptor of the transmitted
> frame. If there were packets with one descriptor enqueued that should mean
> controller did its job >
> head != tail on software data structures when receiving TXUBR interrupt and
> all descriptors in queue have TX_USED bit set might signal that  a
> descriptor is not updated to CPU on TCOMP interrupt when CPU uses it and
> thus driver doesn't treat a TCOMP interrupt. See the above code on

Both TX_USED and last buffer (bit 15) indicator looks ok from
memory, so controller seems to be up to date. If we were to get a TCOMP
interrupt things would be rolling again.
Ofcourse this is speculation, but perhaps there could also be some
corner cases where the controller fails to generate TCOMP as expected?

> macb_tx_interrupt():
> 
> desc = macb_tx_desc(queue, tail);
> 
> /* Make hw descriptor updates visible to CPU */
> rmb();
> 
> ctrl = desc->ctrl;
> 
> /* TX_USED bit is only set by hardware on the very first buffer
> * descriptor of the transmitted frame.
> */
> 
> if (!(ctrl & MACB_BIT(TX_USED)))
> 	break;
> 
> 
>>
>> GEM documentation says "transmission is restarted from
>> the first buffer descriptor of the frame being transmitted when the
>> transmit start bit is rewritten" but since all frames are already marked
>> as transmitted, restarting wont help. Adding this additional check will
>> help for the issue we have.
>>
> 
> I see but according to your description (all descriptors treated by
> controller) if no packets are enqueued for TX after:
> 
> +       if (tbqp == head_idx)
> +               return;
> +
> 
> there are some SKBs that were correctly treated by controller but not freed
> by software (they are freed on macb_tx_unmap() called from
> macb_tx_interrupt()). They will be freed on next TCOMP interrupt for other
> packets being transmitted.
Yes, that is idea. We cannot restart since it triggers new irq,
but instead need to break out. When more data arrives, the controller
continues operation again.


> 
>>
>>>
>>> Maybe TX_WRAP is missing on one TX descriptor? Few months ago while
>>> investigating some other issues on this I found that this might be missed
>>> on one descriptor [1] but haven't managed to make it break at that point
>>> anyhow.
>>>
>>> Could you check on your side if this is solving your issue?
>>
>> I have seen that we can get stuck at any location in the ring buffer, so
>> this does not seem to be the case here. I can try though if it would
>> have any effect.
> 
> I was thinking that having small packets there is high chance that TBQP to
> not reach a descriptor with wrap bit set due to the code pointed in my
> previous email.

I tested with the additions suggested below, but with no change.

Thanks,
Tomas


> 
> Thank you,
> Claudiu Beznea
> 
>>
>> thanks,
>> Tomas
>>
>>
>>>
>>>        /* Set 'TX_USED' bit in buffer descriptor at tx_head position
>>>         * to set the end of TX queue
>>>         */
>>>        i = tx_head;
>>>        entry = macb_tx_ring_wrap(bp, i);
>>>        ctrl = MACB_BIT(TX_USED);
>>> +     if (entry == bp->tx_ring_size - 1)
>>> +             ctrl |= MACB_BIT(TX_WRAP);
>>>        desc = macb_tx_desc(queue, entry);
>>>        desc->ctrl = ctrl;
>>>
>>> [1]
>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fnet%2Fethernet%2Fcadence%2Fmacb_main.c%23n1958&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C4d5ef5a2a27247697b0b08da0e6539ff%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637838125183624670%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=v%2F%2FdE1Y8BxHWsmtn3nX70OFN5oCb%2Bzlb881UXuYtoMs%3D&amp;reserved=0
>>>
>>>
>>>>           macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>>>>    }
>>>>
>>>> -- 
>>>> 2.35.1
>>>>
>>>
> 

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Claudiu.Beznea]

Hi,

On 25.03.2022 16:41, Tomas Melin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi,
> 
> On 25/03/2022 15:41, Claudiu.Beznea@microchip.com wrote:
>> On 25.03.2022 11:35, Tomas Melin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>> content is safe
>>>
>>> Hi,
>>>
>>> On 25/03/2022 10:57, Claudiu.Beznea@microchip.com wrote:
>>>> On 25.03.2022 08:50, Tomas Melin wrote:
>>>>> [Some people who received this message don't often get email from
>>>>> tomas.melin@vaisala.com. Learn why this is important at
>>>>> http://aka.ms/LearnAboutSenderIdentification.]
>>>>>
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>>>> the content is safe
>>>>>
>>>>> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
>>>>> added support for restarting transmission. Restarting tx does not work
>>>>> in case controller asserts TXUBR interrupt and TQBP is already at the end
>>>>> of the tx queue. In that situation, restarting tx will immediately cause
>>>>> assertion of another TXUBR interrupt. The driver will end up in an
>>>>> infinite
>>>>> interrupt loop which it cannot break out of.
>>>>>
>>>>> For cases where TQBP is at the end of the tx queue, instead
>>>>> only clear TXUBR interrupt. As more data gets pushed to the queue,
>>>>> transmission will resume.
>>>>>
>>>>> This issue was observed on a Xilinx Zynq based board. During stress
>>>>> test of
>>>>> the network interface, driver would get stuck on interrupt loop
>>>>> within seconds or minutes causing CPU to stall.
>>>>>
>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>> ---
>>>>>    drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
>>>>>    1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>>>>> b/drivers/net/ethernet/cadence/macb_main.c
>>>>> index 800d5ced5800..e475be29845c 100644
>>>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>>>> @@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue
>>>>> *queue)
>>>>>           unsigned int head = queue->tx_head;
>>>>>           unsigned int tail = queue->tx_tail;
>>>>>           struct macb *bp = queue->bp;
>>>>> +       unsigned int head_idx, tbqp;
>>>>>
>>>>>           if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>>>>>                   queue_writel(queue, ISR, MACB_BIT(TXUBR));
>>>>> @@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue
>>>>> *queue)
>>>>>           if (head == tail)
>>>>>                   return;
>>>>>
>>>>> +       tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
>>>>> +       tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
>>>>> +       head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp,
>>>>> head));
>>>>> +
>>>>> +       if (tbqp == head_idx)
>>>>> +               return;
>>>>> +
>>>>
>>>> This looks like TBQP is not advancing though there are packets in the
>>>> software queues (head != tail). Packets are added in the software
>>>> queues on
>>>> TX path and removed when TX was done for them.
>>>
>>> TBQP is at the end of the queue, and that matches with tx_head
>>> maintained by driver. So seems controller is happily at end marker,
>>> and when restarted immediately sees that end marker used tag and
>>> triggers an interrupt again.
>>>
>>> Also when looking at the buffer descriptor memory it shows that all
>>> frames between tx_tail and tx_head have been marked as used.
>>
>> I see. Controller sets TX_USED on the 1st descriptor of the transmitted
>> frame. If there were packets with one descriptor enqueued that should mean
>> controller did its job >
>> head != tail on software data structures when receiving TXUBR interrupt and
>> all descriptors in queue have TX_USED bit set might signal that  a
>> descriptor is not updated to CPU on TCOMP interrupt when CPU uses it and
>> thus driver doesn't treat a TCOMP interrupt. See the above code on
> 
> Both TX_USED and last buffer (bit 15) indicator looks ok from
> memory, so controller seems to be up to date. If we were to get a TCOMP
> interrupt things would be rolling again.

My current supposition is that controller fires TCOMP but the descriptor is
not updated to CPU when it uses. Of course is just supposition. Maybe check
that rmb() expand to your system needs, maybe use dma_rmb(), or check the
ctrl part of the descriptor when head and tail points to the values that
makes the driver fail. Just wanted to be sure the concurrency is not from here.

> Ofcourse this is speculation, but perhaps there could also be some
> corner cases where the controller fails to generate TCOMP as expected?

I suppose this has to be checked with Cadence.

Thank you,
Claudiu Beznea

> 
>> macb_tx_interrupt():
>>
>> desc = macb_tx_desc(queue, tail);
>>
>> /* Make hw descriptor updates visible to CPU */
>> rmb();
>>
>> ctrl = desc->ctrl;
>>
>> /* TX_USED bit is only set by hardware on the very first buffer
>> * descriptor of the transmitted frame.
>> */
>>
>> if (!(ctrl & MACB_BIT(TX_USED)))
>>       break;
>>
>>
>>>
>>> GEM documentation says "transmission is restarted from
>>> the first buffer descriptor of the frame being transmitted when the
>>> transmit start bit is rewritten" but since all frames are already marked
>>> as transmitted, restarting wont help. Adding this additional check will
>>> help for the issue we have.
>>>
>>
>> I see but according to your description (all descriptors treated by
>> controller) if no packets are enqueued for TX after:
>>
>> +       if (tbqp == head_idx)
>> +               return;
>> +
>>
>> there are some SKBs that were correctly treated by controller but not freed
>> by software (they are freed on macb_tx_unmap() called from
>> macb_tx_interrupt()). They will be freed on next TCOMP interrupt for other
>> packets being transmitted.
> Yes, that is idea. We cannot restart since it triggers new irq,
> but instead need to break out. When more data arrives, the controller
> continues operation again.
> 
> 
>>
>>>
>>>>
>>>> Maybe TX_WRAP is missing on one TX descriptor? Few months ago while
>>>> investigating some other issues on this I found that this might be missed
>>>> on one descriptor [1] but haven't managed to make it break at that point
>>>> anyhow.
>>>>
>>>> Could you check on your side if this is solving your issue?
>>>
>>> I have seen that we can get stuck at any location in the ring buffer, so
>>> this does not seem to be the case here. I can try though if it would
>>> have any effect.
>>
>> I was thinking that having small packets there is high chance that TBQP to
>> not reach a descriptor with wrap bit set due to the code pointed in my
>> previous email.
> 
> I tested with the additions suggested below, but with no change.
> 
> Thanks,
> Tomas
> 
> 
>>
>> Thank you,
>> Claudiu Beznea
>>
>>>
>>> thanks,
>>> Tomas
>>>
>>>
>>>>
>>>>        /* Set 'TX_USED' bit in buffer descriptor at tx_head position
>>>>         * to set the end of TX queue
>>>>         */
>>>>        i = tx_head;
>>>>        entry = macb_tx_ring_wrap(bp, i);
>>>>        ctrl = MACB_BIT(TX_USED);
>>>> +     if (entry == bp->tx_ring_size - 1)
>>>> +             ctrl |= MACB_BIT(TX_WRAP);
>>>>        desc = macb_tx_desc(queue, entry);
>>>>        desc->ctrl = ctrl;
>>>>
>>>> [1]
>>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fnet%2Fethernet%2Fcadence%2Fmacb_main.c%23n1958&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C4d5ef5a2a27247697b0b08da0e6539ff%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637838125183624670%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=v%2F%2FdE1Y8BxHWsmtn3nX70OFN5oCb%2Bzlb881UXuYtoMs%3D&amp;reserved=0
>>>>
>>>>
>>>>
>>>>>           macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>>>>>    }
>>>>>
>>>>> -- 
>>>>> 2.35.1
>>>>>
>>>>
>>

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Melin Tomas]

Hi,

On 25/03/2022 17:19, Claudiu.Beznea@microchip.com wrote:
> Hi,
> 
> On 25.03.2022 16:41, Tomas Melin wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> Hi,
>>
>> On 25/03/2022 15:41, Claudiu.Beznea@microchip.com wrote:
>>> On 25.03.2022 11:35, Tomas Melin wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>>> content is safe
>>>>
>>>> Hi,
>>>>
>>>> On 25/03/2022 10:57, Claudiu.Beznea@microchip.com wrote:
>>>>> On 25.03.2022 08:50, Tomas Melin wrote:
>>>>>> [Some people who received this message don't often get email from
>>>>>> tomas.melin@vaisala.com. Learn why this is important at
>>>>>> http://aka.ms/LearnAboutSenderIdentification.]
>>>>>>
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>>>>> the content is safe
>>>>>>
>>>>>> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
>>>>>> added support for restarting transmission. Restarting tx does not work
>>>>>> in case controller asserts TXUBR interrupt and TQBP is already at the end
>>>>>> of the tx queue. In that situation, restarting tx will immediately cause
>>>>>> assertion of another TXUBR interrupt. The driver will end up in an
>>>>>> infinite
>>>>>> interrupt loop which it cannot break out of.
>>>>>>
>>>>>> For cases where TQBP is at the end of the tx queue, instead
>>>>>> only clear TXUBR interrupt. As more data gets pushed to the queue,
>>>>>> transmission will resume.
>>>>>>
>>>>>> This issue was observed on a Xilinx Zynq based board. During stress
>>>>>> test of
>>>>>> the network interface, driver would get stuck on interrupt loop
>>>>>> within seconds or minutes causing CPU to stall.
>>>>>>
>>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>>> ---
>>>>>>     drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
>>>>>>     1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>>>>>> b/drivers/net/ethernet/cadence/macb_main.c
>>>>>> index 800d5ced5800..e475be29845c 100644
>>>>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>>>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>>>>> @@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue
>>>>>> *queue)
>>>>>>            unsigned int head = queue->tx_head;
>>>>>>            unsigned int tail = queue->tx_tail;
>>>>>>            struct macb *bp = queue->bp;
>>>>>> +       unsigned int head_idx, tbqp;
>>>>>>
>>>>>>            if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>>>>>>                    queue_writel(queue, ISR, MACB_BIT(TXUBR));
>>>>>> @@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue
>>>>>> *queue)
>>>>>>            if (head == tail)
>>>>>>                    return;
>>>>>>
>>>>>> +       tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
>>>>>> +       tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
>>>>>> +       head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp,
>>>>>> head));
>>>>>> +
>>>>>> +       if (tbqp == head_idx)
>>>>>> +               return;
>>>>>> +
>>>>>
>>>>> This looks like TBQP is not advancing though there are packets in the
>>>>> software queues (head != tail). Packets are added in the software
>>>>> queues on
>>>>> TX path and removed when TX was done for them.
>>>>
>>>> TBQP is at the end of the queue, and that matches with tx_head
>>>> maintained by driver. So seems controller is happily at end marker,
>>>> and when restarted immediately sees that end marker used tag and
>>>> triggers an interrupt again.
>>>>
>>>> Also when looking at the buffer descriptor memory it shows that all
>>>> frames between tx_tail and tx_head have been marked as used.
>>>
>>> I see. Controller sets TX_USED on the 1st descriptor of the transmitted
>>> frame. If there were packets with one descriptor enqueued that should mean
>>> controller did its job >
>>> head != tail on software data structures when receiving TXUBR interrupt and
>>> all descriptors in queue have TX_USED bit set might signal that  a
>>> descriptor is not updated to CPU on TCOMP interrupt when CPU uses it and
>>> thus driver doesn't treat a TCOMP interrupt. See the above code on
>>
>> Both TX_USED and last buffer (bit 15) indicator looks ok from
>> memory, so controller seems to be up to date. If we were to get a TCOMP
>> interrupt things would be rolling again.
> 
> My current supposition is that controller fires TCOMP but the descriptor is
> not updated to CPU when it uses. Of course is just supposition. Maybe check
> that rmb() expand to your system needs, maybe use dma_rmb(), or check the
> ctrl part of the descriptor when head and tail points to the values that
> makes the driver fail. Just wanted to be sure the concurrency is not from here.

Thank you for the suggestion. I have tried dma_rmb(), and the behaviour
is the same. AFAIU dma_rmb() is a somewhat more relaxed version of rmb()
(atleast on ARM), so I guess using those should be ok in the first
place. Or do you see differently?

Ctrl part bits of descriptor looks normal also for the case when this 
happens.

Basically how I understand this is that if we get a TX_USED interrupt
without eventually receiving a TXCOMP that indicates some kind of error
situation for the controller. This patch builds on the same approach
that added tx_restart() in the first place, adding support for case
where driver will otherwise loop indefinitely on interrupt.

Claudiu, would it be possible for you to test this patch on the system
that you originally had TX_USED issues with on your side?

Thanks,
Tomas

> 
>> Ofcourse this is speculation, but perhaps there could also be some
>> corner cases where the controller fails to generate TCOMP as expected?
> 
> I suppose this has to be checked with Cadence.
> 
> Thank you,
> Claudiu Beznea
> 
>>
>>> macb_tx_interrupt():
>>>
>>> desc = macb_tx_desc(queue, tail);
>>>
>>> /* Make hw descriptor updates visible to CPU */
>>> rmb();
>>>
>>> ctrl = desc->ctrl;
>>>
>>> /* TX_USED bit is only set by hardware on the very first buffer
>>> * descriptor of the transmitted frame.
>>> */
>>>
>>> if (!(ctrl & MACB_BIT(TX_USED)))
>>>        break;
>>>
>>>
>>>>
>>>> GEM documentation says "transmission is restarted from
>>>> the first buffer descriptor of the frame being transmitted when the
>>>> transmit start bit is rewritten" but since all frames are already marked
>>>> as transmitted, restarting wont help. Adding this additional check will
>>>> help for the issue we have.
>>>>
>>>
>>> I see but according to your description (all descriptors treated by
>>> controller) if no packets are enqueued for TX after:
>>>
>>> +       if (tbqp == head_idx)
>>> +               return;
>>> +
>>>
>>> there are some SKBs that were correctly treated by controller but not freed
>>> by software (they are freed on macb_tx_unmap() called from
>>> macb_tx_interrupt()). They will be freed on next TCOMP interrupt for other
>>> packets being transmitted.
>> Yes, that is idea. We cannot restart since it triggers new irq,
>> but instead need to break out. When more data arrives, the controller
>> continues operation again.
>>
>>
>>>
>>>>
>>>>>
>>>>> Maybe TX_WRAP is missing on one TX descriptor? Few months ago while
>>>>> investigating some other issues on this I found that this might be missed
>>>>> on one descriptor [1] but haven't managed to make it break at that point
>>>>> anyhow.
>>>>>
>>>>> Could you check on your side if this is solving your issue?
>>>>
>>>> I have seen that we can get stuck at any location in the ring buffer, so
>>>> this does not seem to be the case here. I can try though if it would
>>>> have any effect.
>>>
>>> I was thinking that having small packets there is high chance that TBQP to
>>> not reach a descriptor with wrap bit set due to the code pointed in my
>>> previous email.
>>
>> I tested with the additions suggested below, but with no change.
>>
>> Thanks,
>> Tomas
>>
>>
>>>
>>> Thank you,
>>> Claudiu Beznea
>>>
>>>>
>>>> thanks,
>>>> Tomas
>>>>
>>>>
>>>>>
>>>>>         /* Set 'TX_USED' bit in buffer descriptor at tx_head position
>>>>>          * to set the end of TX queue
>>>>>          */
>>>>>         i = tx_head;
>>>>>         entry = macb_tx_ring_wrap(bp, i);
>>>>>         ctrl = MACB_BIT(TX_USED);
>>>>> +     if (entry == bp->tx_ring_size - 1)
>>>>> +             ctrl |= MACB_BIT(TX_WRAP);
>>>>>         desc = macb_tx_desc(queue, entry);
>>>>>         desc->ctrl = ctrl;
>>>>>
>>>>> [1]
>>>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fnet%2Fethernet%2Fcadence%2Fmacb_main.c%23n1958&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7Ca5a0fc448b8145b5dfca08da0e72e903%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637838183979496853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=hdSHlXI%2BgMKgqqxk%2B3xLtgmd2rfSmqiQ9gaFk2V2j3Y%3D&amp;reserved=0
>>>>>
>>>>>
>>>>>
>>>>>>            macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>>>>>>     }
>>>>>>
>>>>>> -- 
>>>>>> 2.35.1
>>>>>>
>>>>>
>>>
> 

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Claudiu.Beznea]

On 28.03.2022 07:16, Melin Tomas wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi,
> 
> On 25/03/2022 17:19, Claudiu.Beznea@microchip.com wrote:
>> Hi,
>>
>> On 25.03.2022 16:41, Tomas Melin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>> content is safe
>>>
>>> Hi,
>>>
>>> On 25/03/2022 15:41, Claudiu.Beznea@microchip.com wrote:
>>>> On 25.03.2022 11:35, Tomas Melin wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>>>> content is safe
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 25/03/2022 10:57, Claudiu.Beznea@microchip.com wrote:
>>>>>> On 25.03.2022 08:50, Tomas Melin wrote:
>>>>>>> [Some people who received this message don't often get email from
>>>>>>> tomas.melin@vaisala.com. Learn why this is important at
>>>>>>> http://aka.ms/LearnAboutSenderIdentification.]
>>>>>>>
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>>>>>> the content is safe
>>>>>>>
>>>>>>> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
>>>>>>> added support for restarting transmission. Restarting tx does not work
>>>>>>> in case controller asserts TXUBR interrupt and TQBP is already at the end
>>>>>>> of the tx queue. In that situation, restarting tx will immediately cause
>>>>>>> assertion of another TXUBR interrupt. The driver will end up in an
>>>>>>> infinite
>>>>>>> interrupt loop which it cannot break out of.
>>>>>>>
>>>>>>> For cases where TQBP is at the end of the tx queue, instead
>>>>>>> only clear TXUBR interrupt. As more data gets pushed to the queue,
>>>>>>> transmission will resume.
>>>>>>>
>>>>>>> This issue was observed on a Xilinx Zynq based board. During stress
>>>>>>> test of
>>>>>>> the network interface, driver would get stuck on interrupt loop
>>>>>>> within seconds or minutes causing CPU to stall.
>>>>>>>
>>>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>>>> ---
>>>>>>>     drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
>>>>>>>     1 file changed, 8 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>>>>>>> b/drivers/net/ethernet/cadence/macb_main.c
>>>>>>> index 800d5ced5800..e475be29845c 100644
>>>>>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>>>>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>>>>>> @@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue
>>>>>>> *queue)
>>>>>>>            unsigned int head = queue->tx_head;
>>>>>>>            unsigned int tail = queue->tx_tail;
>>>>>>>            struct macb *bp = queue->bp;
>>>>>>> +       unsigned int head_idx, tbqp;
>>>>>>>
>>>>>>>            if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>>>>>>>                    queue_writel(queue, ISR, MACB_BIT(TXUBR));
>>>>>>> @@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue
>>>>>>> *queue)
>>>>>>>            if (head == tail)
>>>>>>>                    return;
>>>>>>>
>>>>>>> +       tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
>>>>>>> +       tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
>>>>>>> +       head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp,
>>>>>>> head));
>>>>>>> +
>>>>>>> +       if (tbqp == head_idx)
>>>>>>> +               return;
>>>>>>> +
>>>>>>
>>>>>> This looks like TBQP is not advancing though there are packets in the
>>>>>> software queues (head != tail). Packets are added in the software
>>>>>> queues on
>>>>>> TX path and removed when TX was done for them.
>>>>>
>>>>> TBQP is at the end of the queue, and that matches with tx_head
>>>>> maintained by driver. So seems controller is happily at end marker,
>>>>> and when restarted immediately sees that end marker used tag and
>>>>> triggers an interrupt again.
>>>>>
>>>>> Also when looking at the buffer descriptor memory it shows that all
>>>>> frames between tx_tail and tx_head have been marked as used.
>>>>
>>>> I see. Controller sets TX_USED on the 1st descriptor of the transmitted
>>>> frame. If there were packets with one descriptor enqueued that should mean
>>>> controller did its job >
>>>> head != tail on software data structures when receiving TXUBR interrupt and
>>>> all descriptors in queue have TX_USED bit set might signal that  a
>>>> descriptor is not updated to CPU on TCOMP interrupt when CPU uses it and
>>>> thus driver doesn't treat a TCOMP interrupt. See the above code on
>>>
>>> Both TX_USED and last buffer (bit 15) indicator looks ok from
>>> memory, so controller seems to be up to date. If we were to get a TCOMP
>>> interrupt things would be rolling again.
>>
>> My current supposition is that controller fires TCOMP but the descriptor is
>> not updated to CPU when it uses. Of course is just supposition. Maybe check
>> that rmb() expand to your system needs, maybe use dma_rmb(), or check the
>> ctrl part of the descriptor when head and tail points to the values that
>> makes the driver fail. Just wanted to be sure the concurrency is not from here.
> 
> Thank you for the suggestion. I have tried dma_rmb(), and the behaviour
> is the same. AFAIU dma_rmb() is a somewhat more relaxed version of rmb()
> (atleast on ARM), 
> so I guess using those should be ok in the first
> place. Or do you see differently?

Yes, should be good. Reviewing the thread I see you are talking about Zynq
which is also ARM based.

> 
> Ctrl part bits of descriptor looks normal also for the case when this
> happens.>
> Basically how I understand this is that if we get a TX_USED interrupt
> without eventually receiving a TXCOMP that indicates some kind of error
> situation for the controller. 

Yes, that should be the case. What I asked you in the previous email was to
be sure that TCOMP interrupt has been received and CPU didn't miss updating
software data structure due to descriptor not updated to CPU due to
improper usage of the barrier, as follows:

IRQ handler
     |
     |  TCOMP
     +----------->macb_tx_interrupt()
     |                     |
     |                     v
     |               read descriptor
     |                     |
     |                     |
     |                     v
     |<------------!(desc->ctrl & TX_USED) // make sure at this point
     |                     |               // reflects HW state
     |                     v
     |                 advance tail and
     |                 tail = head
     |                     |
     |                     |
     |<--------------------+
     |
     |

If the desc update is not done when a TCOMP is received then head != tail
in macb_tx_restart() and TSTART is issued again but since HW did its job
and TBQP points to a descriptor with TX_USED set it will fire TX_USED irq
in a loop.

Now, if you verified this and all good with the descriptor then it may mean
HW does something buggy.

> This patch builds on the same approach
> that added tx_restart() in the first place, adding support for case
> where driver will otherwise loop indefinitely on interrupt.

The assumption on macb_tx_restart() was that TX stopped due to concurrency
b/w software updating a descriptor and HW reading the same descriptor, thus
TBQP didn't advance. On your situation the TX didn't stopped but it seems
it succeeded.

One thing I fear about this patch is that freeing data structures
associated with a skb relies on sending another packet and that being
successfully transmitted (w/o the issue being reproduced again). If issue
is reproduced in a loop then in the end system will eventually stuck again.

> 
> Claudiu, would it be possible for you to test this patch on the system
> that you originally had TX_USED issues with on your side?

I don't have one with me but I'll go to the office and grab it. I need to
look further for emails to see the proper setup (the initial issue
reproduces only with a particular web server and a particular setup for
it). It might be difficult to replicate it.

MACB support for Zynq is enabled for quite some time and you said that you
reproduce it pretty easy without anything fancy. Maybe Xilinx people know
something about this behavior. Harini, do you have an idea about this
behavior on Zynq?

Thank you,
Claudiu Beznea

> 
> Thanks,
> Tomas
> 
>>
>>> Ofcourse this is speculation, but perhaps there could also be some
>>> corner cases where the controller fails to generate TCOMP as expected?
>>
>> I suppose this has to be checked with Cadence.
>>
>> Thank you,
>> Claudiu Beznea
>>
>>>
>>>> macb_tx_interrupt():
>>>>
>>>> desc = macb_tx_desc(queue, tail);
>>>>
>>>> /* Make hw descriptor updates visible to CPU */
>>>> rmb();
>>>>
>>>> ctrl = desc->ctrl;
>>>>
>>>> /* TX_USED bit is only set by hardware on the very first buffer
>>>> * descriptor of the transmitted frame.
>>>> */
>>>>
>>>> if (!(ctrl & MACB_BIT(TX_USED)))
>>>>        break;
>>>>
>>>>
>>>>>
>>>>> GEM documentation says "transmission is restarted from
>>>>> the first buffer descriptor of the frame being transmitted when the
>>>>> transmit start bit is rewritten" but since all frames are already marked
>>>>> as transmitted, restarting wont help. Adding this additional check will
>>>>> help for the issue we have.
>>>>>
>>>>
>>>> I see but according to your description (all descriptors treated by
>>>> controller) if no packets are enqueued for TX after:
>>>>
>>>> +       if (tbqp == head_idx)
>>>> +               return;
>>>> +
>>>>
>>>> there are some SKBs that were correctly treated by controller but not freed
>>>> by software (they are freed on macb_tx_unmap() called from
>>>> macb_tx_interrupt()). They will be freed on next TCOMP interrupt for other
>>>> packets being transmitted.
>>> Yes, that is idea. We cannot restart since it triggers new irq,
>>> but instead need to break out. When more data arrives, the controller
>>> continues operation again.
>>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Maybe TX_WRAP is missing on one TX descriptor? Few months ago while
>>>>>> investigating some other issues on this I found that this might be missed
>>>>>> on one descriptor [1] but haven't managed to make it break at that point
>>>>>> anyhow.
>>>>>>
>>>>>> Could you check on your side if this is solving your issue?
>>>>>
>>>>> I have seen that we can get stuck at any location in the ring buffer, so
>>>>> this does not seem to be the case here. I can try though if it would
>>>>> have any effect.
>>>>
>>>> I was thinking that having small packets there is high chance that TBQP to
>>>> not reach a descriptor with wrap bit set due to the code pointed in my
>>>> previous email.
>>>
>>> I tested with the additions suggested below, but with no change.
>>>
>>> Thanks,
>>> Tomas
>>>
>>>
>>>>
>>>> Thank you,
>>>> Claudiu Beznea
>>>>
>>>>>
>>>>> thanks,
>>>>> Tomas
>>>>>
>>>>>
>>>>>>
>>>>>>         /* Set 'TX_USED' bit in buffer descriptor at tx_head position
>>>>>>          * to set the end of TX queue
>>>>>>          */
>>>>>>         i = tx_head;
>>>>>>         entry = macb_tx_ring_wrap(bp, i);
>>>>>>         ctrl = MACB_BIT(TX_USED);
>>>>>> +     if (entry == bp->tx_ring_size - 1)
>>>>>> +             ctrl |= MACB_BIT(TX_WRAP);
>>>>>>         desc = macb_tx_desc(queue, entry);
>>>>>>         desc->ctrl = ctrl;
>>>>>>
>>>>>> [1]
>>>>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fnet%2Fethernet%2Fcadence%2Fmacb_main.c%23n1958&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7Ca5a0fc448b8145b5dfca08da0e72e903%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637838183979496853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=hdSHlXI%2BgMKgqqxk%2B3xLtgmd2rfSmqiQ9gaFk2V2j3Y%3D&amp;reserved=0
>>>>>>
>>>>>>
>>>>>>
>>>>>>>            macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>>>>>>>     }
>>>>>>>
>>>>>>> --
>>>>>>> 2.35.1
>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Tomas Melin]

Hi,

On 30/03/2022 17:27, Claudiu.Beznea@microchip.com wrote:
> On 28.03.2022 07:16, Melin Tomas wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi,
>>
>> On 25/03/2022 17:19, Claudiu.Beznea@microchip.com wrote:
>>> Hi,
>>>
>>> On 25.03.2022 16:41, Tomas Melin wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>>> content is safe
>>>>
>>>> Hi,
>>>>
>>>> On 25/03/2022 15:41, Claudiu.Beznea@microchip.com wrote:
>>>>> On 25.03.2022 11:35, Tomas Melin wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>>>>> content is safe
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 25/03/2022 10:57, Claudiu.Beznea@microchip.com wrote:
>>>>>>> On 25.03.2022 08:50, Tomas Melin wrote:
>>>>>>>> [Some people who received this message don't often get email from
>>>>>>>> tomas.melin@vaisala.com. Learn why this is important at
>>>>>>>> http://aka.ms/LearnAboutSenderIdentification.]
>>>>>>>>
>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>>>>>>> the content is safe
>>>>>>>>
>>>>>>>> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")
>>>>>>>> added support for restarting transmission. Restarting tx does not work
>>>>>>>> in case controller asserts TXUBR interrupt and TQBP is already at the end
>>>>>>>> of the tx queue. In that situation, restarting tx will immediately cause
>>>>>>>> assertion of another TXUBR interrupt. The driver will end up in an
>>>>>>>> infinite
>>>>>>>> interrupt loop which it cannot break out of.
>>>>>>>>
>>>>>>>> For cases where TQBP is at the end of the tx queue, instead
>>>>>>>> only clear TXUBR interrupt. As more data gets pushed to the queue,
>>>>>>>> transmission will resume.
>>>>>>>>
>>>>>>>> This issue was observed on a Xilinx Zynq based board. During stress
>>>>>>>> test of
>>>>>>>> the network interface, driver would get stuck on interrupt loop
>>>>>>>> within seconds or minutes causing CPU to stall.
>>>>>>>>
>>>>>>>> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
>>>>>>>> ---
>>>>>>>>      drivers/net/ethernet/cadence/macb_main.c | 8 ++++++++
>>>>>>>>      1 file changed, 8 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>>>>>>>> b/drivers/net/ethernet/cadence/macb_main.c
>>>>>>>> index 800d5ced5800..e475be29845c 100644
>>>>>>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>>>>>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>>>>>>> @@ -1658,6 +1658,7 @@ static void macb_tx_restart(struct macb_queue
>>>>>>>> *queue)
>>>>>>>>             unsigned int head = queue->tx_head;
>>>>>>>>             unsigned int tail = queue->tx_tail;
>>>>>>>>             struct macb *bp = queue->bp;
>>>>>>>> +       unsigned int head_idx, tbqp;
>>>>>>>>
>>>>>>>>             if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>>>>>>>>                     queue_writel(queue, ISR, MACB_BIT(TXUBR));
>>>>>>>> @@ -1665,6 +1666,13 @@ static void macb_tx_restart(struct macb_queue
>>>>>>>> *queue)
>>>>>>>>             if (head == tail)
>>>>>>>>                     return;
>>>>>>>>
>>>>>>>> +       tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
>>>>>>>> +       tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
>>>>>>>> +       head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp,
>>>>>>>> head));
>>>>>>>> +
>>>>>>>> +       if (tbqp == head_idx)
>>>>>>>> +               return;
>>>>>>>> +
>>>>>>>
>>>>>>> This looks like TBQP is not advancing though there are packets in the
>>>>>>> software queues (head != tail). Packets are added in the software
>>>>>>> queues on
>>>>>>> TX path and removed when TX was done for them.
>>>>>>
>>>>>> TBQP is at the end of the queue, and that matches with tx_head
>>>>>> maintained by driver. So seems controller is happily at end marker,
>>>>>> and when restarted immediately sees that end marker used tag and
>>>>>> triggers an interrupt again.
>>>>>>
>>>>>> Also when looking at the buffer descriptor memory it shows that all
>>>>>> frames between tx_tail and tx_head have been marked as used.
>>>>>
>>>>> I see. Controller sets TX_USED on the 1st descriptor of the transmitted
>>>>> frame. If there were packets with one descriptor enqueued that should mean
>>>>> controller did its job >
>>>>> head != tail on software data structures when receiving TXUBR interrupt and
>>>>> all descriptors in queue have TX_USED bit set might signal that  a
>>>>> descriptor is not updated to CPU on TCOMP interrupt when CPU uses it and
>>>>> thus driver doesn't treat a TCOMP interrupt. See the above code on
>>>>
>>>> Both TX_USED and last buffer (bit 15) indicator looks ok from
>>>> memory, so controller seems to be up to date. If we were to get a TCOMP
>>>> interrupt things would be rolling again.
>>>
>>> My current supposition is that controller fires TCOMP but the descriptor is
>>> not updated to CPU when it uses. Of course is just supposition. Maybe check
>>> that rmb() expand to your system needs, maybe use dma_rmb(), or check the
>>> ctrl part of the descriptor when head and tail points to the values that
>>> makes the driver fail. Just wanted to be sure the concurrency is not from here.
>>
>> Thank you for the suggestion. I have tried dma_rmb(), and the behaviour
>> is the same. AFAIU dma_rmb() is a somewhat more relaxed version of rmb()
>> (atleast on ARM),
>> so I guess using those should be ok in the first
>> place. Or do you see differently?
> 
> Yes, should be good. Reviewing the thread I see you are talking about Zynq
> which is also ARM based.
> 
>>
>> Ctrl part bits of descriptor looks normal also for the case when this
>> happens.>
>> Basically how I understand this is that if we get a TX_USED interrupt
>> without eventually receiving a TXCOMP that indicates some kind of error
>> situation for the controller.
> 
> Yes, that should be the case. What I asked you in the previous email was to
> be sure that TCOMP interrupt has been received and CPU didn't miss updating
> software data structure due to descriptor not updated to CPU due to
> improper usage of the barrier, as follows:
> 
> IRQ handler
>       |
>       |  TCOMP
>       +----------->macb_tx_interrupt()
>       |                     |
>       |                     v
>       |               read descriptor
>       |                     |
>       |                     |
>       |                     v
>       |<------------!(desc->ctrl & TX_USED) // make sure at this point
>       |                     |               // reflects HW state
>       |                     v
>       |                 advance tail and
>       |                 tail = head
>       |                     |
>       |                     |
>       |<--------------------+
>       |
>       |
> 
> If the desc update is not done when a TCOMP is received then head != tail
> in macb_tx_restart() and TSTART is issued again but since HW did its job
> and TBQP points to a descriptor with TX_USED set it will fire TX_USED irq
> in a loop >
> Now, if you verified this and all good with the descriptor then it may mean
> HW does something buggy.

I believe we see this the same way and you have a good hypothesis.
That location in the code is potentially a place where things *could* go 
wrong.

As mentioned, ctrl part has correct bits.
Also to me, there is no obvious problem in the code around that point as 
such. And dma and normal version of memory barriers have both been tested.


> 
>> This patch builds on the same approach
>> that added tx_restart() in the first place, adding support for case
>> where driver will otherwise loop indefinitely on interrupt.
> 
> The assumption on macb_tx_restart() was that TX stopped due to concurrency
> b/w software updating a descriptor and HW reading the same descriptor, thus
> TBQP didn't advance. On your situation the TX didn't stopped but it seems
> it succeeded.
> 
> One thing I fear about this patch is that freeing data structures
> associated with a skb relies on sending another packet and that being
> successfully transmitted (w/o the issue being reproduced again). If issue
> is reproduced in a loop then in the end system will eventually stuck again.

I understand, but that does not seem to be the case with this particular 
issue.
The problem unrolls as soon as new packets are pushed to queue.
If we encounter that kind of situation you describe,
that would need to be handled separately.

I propose now to concentrate on a solution for the actual behaviour we 
have seen. The addition of tx_restart() exposed the issue we have seen,
because if we ignore TX_USED interrupts this is never visible.

This is why I believe adding these additional checks to tx_restart is
the correct approach.


> 
>>
>> Claudiu, would it be possible for you to test this patch on the system
>> that you originally had TX_USED issues with on your side?
> 
> I don't have one with me but I'll go to the office and grab it. I need to
> look further for emails to see the proper setup (the initial issue
> reproduces only with a particular web server and a particular setup for
> it). It might be difficult to replicate it.
> 
> MACB support for Zynq is enabled for quite some time and you said that you
> reproduce it pretty easy without anything fancy. Maybe Xilinx people know
> something about this behavior. Harini, do you have an idea about this
> behavior on Zynq?

My comment on this is that I was also surprised not to find other 
reports about this. But also for us, this issue was not visible for long 
time even with identical kernel version. As load and other parameters 
have changed, the situation has become more easy to trigger. So it's 
likely the conditions need to be correct.

Thanks,
Tomas


> 
> Thank you,
> Claudiu Beznea
> 
>>
>> Thanks,
>> Tomas
>>
>>>
>>>> Ofcourse this is speculation, but perhaps there could also be some
>>>> corner cases where the controller fails to generate TCOMP as expected?
>>>
>>> I suppose this has to be checked with Cadence.
>>>
>>> Thank you,
>>> Claudiu Beznea
>>>
>>>>
>>>>> macb_tx_interrupt():
>>>>>
>>>>> desc = macb_tx_desc(queue, tail);
>>>>>
>>>>> /* Make hw descriptor updates visible to CPU */
>>>>> rmb();
>>>>>
>>>>> ctrl = desc->ctrl;
>>>>>
>>>>> /* TX_USED bit is only set by hardware on the very first buffer
>>>>> * descriptor of the transmitted frame.
>>>>> */
>>>>>
>>>>> if (!(ctrl & MACB_BIT(TX_USED)))
>>>>>         break;
>>>>>
>>>>>
>>>>>>
>>>>>> GEM documentation says "transmission is restarted from
>>>>>> the first buffer descriptor of the frame being transmitted when the
>>>>>> transmit start bit is rewritten" but since all frames are already marked
>>>>>> as transmitted, restarting wont help. Adding this additional check will
>>>>>> help for the issue we have.
>>>>>>
>>>>>
>>>>> I see but according to your description (all descriptors treated by
>>>>> controller) if no packets are enqueued for TX after:
>>>>>
>>>>> +       if (tbqp == head_idx)
>>>>> +               return;
>>>>> +
>>>>>
>>>>> there are some SKBs that were correctly treated by controller but not freed
>>>>> by software (they are freed on macb_tx_unmap() called from
>>>>> macb_tx_interrupt()). They will be freed on next TCOMP interrupt for other
>>>>> packets being transmitted.
>>>> Yes, that is idea. We cannot restart since it triggers new irq,
>>>> but instead need to break out. When more data arrives, the controller
>>>> continues operation again.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Maybe TX_WRAP is missing on one TX descriptor? Few months ago while
>>>>>>> investigating some other issues on this I found that this might be missed
>>>>>>> on one descriptor [1] but haven't managed to make it break at that point
>>>>>>> anyhow.
>>>>>>>
>>>>>>> Could you check on your side if this is solving your issue?
>>>>>>
>>>>>> I have seen that we can get stuck at any location in the ring buffer, so
>>>>>> this does not seem to be the case here. I can try though if it would
>>>>>> have any effect.
>>>>>
>>>>> I was thinking that having small packets there is high chance that TBQP to
>>>>> not reach a descriptor with wrap bit set due to the code pointed in my
>>>>> previous email.
>>>>
>>>> I tested with the additions suggested below, but with no change.
>>>>
>>>> Thanks,
>>>> Tomas
>>>>
>>>>
>>>>>
>>>>> Thank you,
>>>>> Claudiu Beznea
>>>>>
>>>>>>
>>>>>> thanks,
>>>>>> Tomas
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>          /* Set 'TX_USED' bit in buffer descriptor at tx_head position
>>>>>>>           * to set the end of TX queue
>>>>>>>           */
>>>>>>>          i = tx_head;
>>>>>>>          entry = macb_tx_ring_wrap(bp, i);
>>>>>>>          ctrl = MACB_BIT(TX_USED);
>>>>>>> +     if (entry == bp->tx_ring_size - 1)
>>>>>>> +             ctrl |= MACB_BIT(TX_WRAP);
>>>>>>>          desc = macb_tx_desc(queue, entry);
>>>>>>>          desc->ctrl = ctrl;
>>>>>>>
>>>>>>> [1]
>>>>>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fnet%2Fethernet%2Fcadence%2Fmacb_main.c%23n1958&amp;data=04%7C01%7Ctomas.melin%40vaisala.com%7C6033e443aec34049f9c308da125966c2%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637842472480310420%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=UEVT3ToG3UOogZDjHbdTG5BTj9PRztUY8%2Ft9HkADS2k%3D&amp;reserved=0
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>             macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>>>>>>>>      }
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.35.1
>>>>>>>>
>>>>>>>
>>>>>
>>>
> 

Re: [PATCH] net: macb: Restart tx only if queue pointer is lagging

[Jakub Kicinski]

On Fri, 25 Mar 2022 08:50:12 +0200 Tomas Melin wrote:
> commit 5ea9c08a8692 ("net: macb: restart tx after tx used bit read")

Ooh, I just noticed the commit ID here is from the stable tree.
Please change it to the original commit ID, so 4298388574da
and repost once the discussion settles.

> added support for restarting transmission. Restarting tx does not work
> in case controller asserts TXUBR interrupt and TQBP is already at the end
> of the tx queue. In that situation, restarting tx will immediately cause
> assertion of another TXUBR interrupt. The driver will end up in an infinite
> interrupt loop which it cannot break out of.
> 
> For cases where TQBP is at the end of the tx queue, instead
> only clear TXUBR interrupt. As more data gets pushed to the queue,
> transmission will resume.
> 
> This issue was observed on a Xilinx Zynq based board. During stress test of
> the network interface, driver would get stuck on interrupt loop
> within seconds or minutes causing CPU to stall.