Hi,
Peter Chen <hzpeterchen@xxxxxxxxx> writes:
> On Tue, Nov 01, 2016 at 01:29:59PM +0200, Felipe Balbi wrote:
>> According to Dave Miller "the networking stack has a
>> hard requirement that all SKBs which are transmitted
>> must have their completion signalled in a fininte
>> amount of time. This is because, until the SKB is
>> freed by the driver, it holds onto socket,
>> netfilter, and other subsystem resources."
>>
>> In summary, this means that using TX IRQ throttling
>> for the networking gadgets is, at least, complex and
>> we should avoid it for the time being.
>>
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Suggested-by: David Miller <davem@xxxxxxxxxxxxx>
>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>> ---
>> drivers/usb/gadget/function/u_ether.c | 8 --------
>> 1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
>> index f4a640216913..119a2e5848e8 100644
>> --- a/drivers/usb/gadget/function/u_ether.c
>> +++ b/drivers/usb/gadget/function/u_ether.c
>> @@ -589,14 +589,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
>>
>> req->length = length;
>>
>> - /* throttle high/super speed IRQ rate back slightly */
>> - if (gadget_is_dualspeed(dev->gadget))
>> - req->no_interrupt = (((dev->gadget->speed == USB_SPEED_HIGH ||
>> - dev->gadget->speed == USB_SPEED_SUPER)) &&
>> - !list_empty(&dev->tx_reqs))
>> - ? ((atomic_read(&dev->tx_qlen) % dev->qmult) != 0)
>> - : 0;
>> -
>> retval = usb_ep_queue(in, req, GFP_ATOMIC);
>> switch (retval) {
>> default:
>> --
>
> Felipe, it may increase cpu utilization since more interrupts will be there,
> it may affect the SoC which has lower cpu frequency. This code existed
> many years, why this problem has only reported at dwc3 recently?
No idea, but at least for networking gadgets we shouldn't throttle. This
has been a bug since the beginning. Read Dave Miller's explanation at
[1]
moreover, dwc3 seems to be the only one actually throttling IRQ. Here's
a rundown of a few of the UDCs:
- chipidea: uses TD_IOC conditionally, but always sets TD_TERMINATE
lastnode->ptr->next = cpu_to_le32(TD_TERMINATE);
if (!hwreq->req.no_interrupt)
lastnode->ptr->token |= cpu_to_le32(TD_IOC);
I'm guessing TD_TERMINATE works similar to dwc3's LST bit. If
it's set, it will force an interrupt.
- musb: no_interrupt only used for tracing
- atmel_usba_udc: no_interrupt only used for tracing
- mv_u3d_core: probably throttles interrupt, but probably exhibits same
behavior. It's just that it hasn't been reported.
- fsl_udc_core: probably throttles interrupt, but probably exhibits same
behavior. It's just that it hasn't been reported.
and there are the only uses for the no_interrupt flag.
[1] https://marc.info/?l=linux-usb&m=147793992922064&w=2
--
balbi
Attachment:
signature.asc
Description: PGP signature
