Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

[Felipe Ferreri Tonello]

Hi Clemens

On 13/11/15 08:55, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This patch introduces pre-allocation of IN endpoint USB requests. This
>> improves on latency (requires no usb request allocation on transmit) and avoid
>> several potential probles on allocating too many usb requests (which involves
>> DMA pool allocation problems).
>>
>> This implementation also handles better multiple MIDI Gadget ports, always
>> processing the last processed MIDI substream if the last USB request wasn't
>> enought to handle the whole stream.
> 
>> ...
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -88,6 +89,9 @@ struct f_midi {
>>  	int index;
>>  	char *id;
>>  	unsigned int buflen, qlen;
>> +	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>> +	unsigned int in_req_num;
>> +	unsigned int in_last_port;
> 
> As far as I can see, in_req_num must always have the same value as qlen.

Yes, I've removed in_req_num.

> 
>> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
>> +	while (!kfifo_is_empty(&midi->in_req_fifo)) {
>> +		...
>> +		len = kfifo_get(&midi->in_req_fifo, &req);
>> +		if (len == 1)
>> +			free_ep_req(midi->in_ep, req);
>> +		else
>> +			ERROR(midi, "%s couldn't free usb request: something went wrong with kfifo\n",
>> +			      midi->in_ep->name);
>> +	}
> 
> kfifo_get() already checks for the FIFO being empty, so you can just
> drop kfifi_is_empty().

Ok. I've simplified this code. I wasn't really happy with it either.

> 
>> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request *req,
>> ...
>> +static void f_midi_transmit(struct f_midi *midi)
>> +{
>> +...
>> +		len = kfifo_peek(&midi->in_req_fifo, &req);
>> +		...
>> +		if (req->length > 0) {
>> +			WARNING(midi, "%s: All USB requests have been used. Current queue size "
>> +				"is %u, consider increasing it.\n", __func__, midi->in_req_num);
>> +			goto drop_out;
>> +		}
> 
> There are two cases where the in_req FIFO might overflow:
> 1) the gadget is trying to send too much data at once; or
> 2) the host does not bother to read any of the data.
> 
> In case 1), the appropriate action would be to do nothing, so that the
> remaining data is sent after some currently queued packets have been
> transmitted.  In case 2), the appropriate action would be to drop the
> data (even better, the _oldest_ data), and spamming the log with error
> messages would not help.

True. In this case the log will be spammed.

How would you suggest to drop the oldest data? That doesn't really seem
to be feasible.

> 
> This code shows the error message for case 1), but does the action for
> case 2).
> 
> I'm not quite sure if trying to detect which of these cases we have is
> possible, or worthwhile.  Anyway, with a packet size of 64, the queue
> size would be 32*64 = 2KB, which should be enough for everyone.  So I
> propose to ignore case 1), and to drop the error message.

Agree. It would be useful for users to know about case 1), but like you
said it is probably not worthwhile to do to so.

> 
>> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>> +	if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
>> +		goto setup_fail;
> 
> The setup_fail code expects an error code in the status variable.

Done.

Felipe

Attachment: 0x92698E6A.asc
Description: application/pgp-keys