Subject: Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests
To: "Felipe F. Tonello" <eu@felipetonello.com>, linux-usb@vger.kernel.org
References: <1447177929-22252-1-git-send-email-eu@felipetonello.com>
 <1447177929-22252-8-git-send-email-eu@felipetonello.com>
Cc: linux-kernel@vger.kernel.org, Felipe Balbi <balbi@ti.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Robert Baldyga <r.baldyga@samsung.com>
From: Clemens Ladisch <clemens@ladisch.de>
Message-ID: <5645A58F.9030902@ladisch.de>
Date: Fri, 13 Nov 2015 09:55:43 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <1447177929-22252-8-git-send-email-eu@felipetonello.com>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3057
Lines: 81

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.

> @@ -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().

> @@ -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.

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.

> @@ -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.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/