| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <1994765.6nlskCcuVy@amdc1032>
Date: Mon, 23 Sep 2013 15:59:05 +0200
From: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
To: Robert Baldyga <r.baldyga@...sung.com>
Cc: balbi@...com, gregkh@...uxfoundation.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
m.szyprowski@...sung.com, Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [PATCH] USB: gadget: s3c-hsotg: add isochronous transfers support
Hi Robert,
On Monday, September 23, 2013 10:07:12 AM Robert Baldyga wrote:
> This patch adds isochronous transfer support. It adds few modifications:
> - Modify s3c_hsotg_write_fifo() function. It actually calculates transfer
> size, taking into account Multi Count value, which indicates number of
> transactions per microframe.
> - Fix s3c_hsotg_start_req() function by setting number of packets to Multi
> Count field in DIEPTSIZ register for isochronous endpoints.
> - Fix s3c_hsotg_set_ep_maxpacket() function. Field wMaxPacketSize of endpoint
> descriptor is now splitted into maximum packet size value and number of
> additional transaction per microframe.
> - Modify s3c_hsotg_epint() function. Some interrupts are ignored for
> isochronous endpoints, (e.g. INTknTXFEmpMsk) becouse isochronous request is
> always transfered in single transaction, which ends with XferCompl interrupt.
> Add Odd/Even microframe toggle to allow data transfering in each microframe.
> - Fix s3c_hsotg_ep_enable() function by supporting isochronous endpoint type.
>
> Signed-off-by: Robert Baldyga <r.baldyga@...sung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@...sung.com>
> ---
> drivers/usb/gadget/s3c-hsotg.c | 74 +++++++++++++++++++++++++++++++---------
> 1 file changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
> index d5d951d..d737452 100644
> --- a/drivers/usb/gadget/s3c-hsotg.c
> +++ b/drivers/usb/gadget/s3c-hsotg.c
> @@ -82,9 +82,12 @@ struct s3c_hsotg_req;
> * @dir_in: Set to true if this endpoint is of the IN direction, which
> * means that it is sending data to the Host.
> * @index: The index for the endpoint registers.
> + * @mc: Multi Count - number of transactions per microframe
> + * @interval - Interval for periodic endpoints
> * @name: The name array passed to the USB core.
> * @halted: Set if the endpoint has been halted.
> * @periodic: Set if this is a periodic ep, such as Interrupt
> + * @insochronous: Set if this is a isochronous ep
s/insochronous/isochronous/
> * @sent_zlp: Set if we've sent a zero-length packet.
> * @total_data: The total number of data bytes done.
> * @fifo_size: The size of the FIFO (for periodic IN endpoints)
> @@ -120,9 +123,12 @@ struct s3c_hsotg_ep {
>
> unsigned char dir_in;
> unsigned char index;
> + unsigned char mc;
> + unsigned char interval;
>
> unsigned int halted:1;
> unsigned int periodic:1;
> + unsigned int isochronous:1;
> unsigned int sent_zlp:1;
>
> char name[10];
> @@ -467,6 +473,7 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
> void *data;
> int can_write;
> int pkt_round;
> + int max_transfer;
>
> to_write -= (buf_pos - hs_ep->last_load);
>
> @@ -534,15 +541,17 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
> can_write *= 4; /* fifo size is in 32bit quantities. */
> }
>
> - dev_dbg(hsotg->dev, "%s: GNPTXSTS=%08x, can=%d, to=%d, mps %d\n",
> - __func__, gnptxsts, can_write, to_write, hs_ep->ep.maxpacket);
> + max_transfer = hs_ep->ep.maxpacket * hs_ep->mc;
> +
> + dev_dbg(hsotg->dev, "%s: GNPTXSTS=%08x, can=%d, to=%d, max_transfer %d\n",
> + __func__, gnptxsts, can_write, to_write, max_transfer);
>
> /*
> * limit to 512 bytes of data, it seems at least on the non-periodic
> * FIFO, requests of >512 cause the endpoint to get stuck with a
> * fragment of the end of the transfer in it.
> */
> - if (can_write > 512)
> + if (can_write > 512 && !periodic)
> can_write = 512;
Doesn't it also affect non-isochronous transfers?
If so this change should be in a separate preparatory patch.
> /*
> @@ -550,8 +559,8 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
> * the transfer to return that it did not run out of fifo space
> * doing it.
> */
> - if (to_write > hs_ep->ep.maxpacket) {
> - to_write = hs_ep->ep.maxpacket;
> + if (to_write > max_transfer) {
> + to_write = max_transfer;
>
> /* it's needed only when we do not use dedicated fifos */
> if (!hsotg->dedicated_fifos)
> @@ -564,7 +573,7 @@ static int s3c_hsotg_write_fifo(struct s3c_hsotg *hsotg,
>
> if (to_write > can_write) {
> to_write = can_write;
> - pkt_round = to_write % hs_ep->ep.maxpacket;
> + pkt_round = to_write % max_transfer;
>
> /*
> * Round the write down to an
> @@ -730,8 +739,16 @@ static void s3c_hsotg_start_req(struct s3c_hsotg *hsotg,
> else
> packets = 1; /* send one packet if length is zero. */
>
> + if (length > (hs_ep->mc * hs_ep->ep.maxpacket) && hs_ep->isochronous) {
Wouldn't checking hs_ep->isonchronous first be better?
> + dev_err(hsotg->dev, "req length > maxpacket*mc\n");
> + return;
> + }
The rest of the patch looks good to me.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists