linux-kernel - Re: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 4 Sep 2015 06:52:51 +0000
From:	John Youn <John.Youn@...opsys.com>
To:	Scott Branden <sbranden@...adcom.com>,
	John Youn <John.Youn@...opsys.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	Roman Bacik <rbacik@...adcom.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"bcm-kernel-feedback-list@...adcom.com" 
	<bcm-kernel-feedback-list@...adcom.com>
Subject: Re: [PATCH v2 1/1] usb: dwc2: gadget: parity fix in isochronous mode

On 8/31/2015 9:17 AM, Scott Branden wrote:
> From: Roman Bacik <rbacik@...adcom.com>
> 
> USB OTG driver in isochronous mode has to set the parity of the receiving
> microframe. The parity is set to even by default. This causes problems for
> an audio gadget, if the host starts transmitting on odd microframes.
> 
> This fix uses Incomplete Periodic Transfer interrupt to toggle between
> even and odd parity until the Transfer Complete interrupt is received.
> 
> Signed-off-by: Roman Bacik <rbacik@...adcom.com>
> Reviewed-by: Abhinav Ratna <aratna@...adcom.com>
> Reviewed-by: Srinath Mannam <srinath.mannam@...adcom.com>
> Signed-off-by: Scott Branden <sbranden@...adcom.com>
> ---
>  drivers/usb/dwc2/core.h   |  1 +
>  drivers/usb/dwc2/gadget.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/dwc2/hw.h     |  1 +
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 0ed87620..a5634fd 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -150,6 +150,7 @@ struct s3c_hsotg_ep {
>  	unsigned int            periodic:1;
>  	unsigned int            isochronous:1;
>  	unsigned int            send_zlp:1;
> +	unsigned int            has_correct_parity:1;
>  
>  	char                    name[10];
>  };
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 4d47b7c..fac3e2f 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1954,6 +1954,7 @@ static void s3c_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx,
>  		ints &= ~DXEPINT_XFERCOMPL;
>  
>  	if (ints & DXEPINT_XFERCOMPL) {
> +		hs_ep->has_correct_parity = 1;
>  		if (hs_ep->isochronous && hs_ep->interval == 1) {
>  			if (ctrl & DXEPCTL_EOFRNUM)
>  				ctrl |= DXEPCTL_SETEVENFR;
> @@ -2316,7 +2317,8 @@ void s3c_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>  		GINTSTS_CONIDSTSCHNG | GINTSTS_USBRST |
>  		GINTSTS_RESETDET | GINTSTS_ENUMDONE |
>  		GINTSTS_OTGINT | GINTSTS_USBSUSP |
> -		GINTSTS_WKUPINT,
> +		GINTSTS_WKUPINT |
> +		GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT,
>  		hsotg->regs + GINTMSK);
>  
>  	if (using_dma(hsotg))
> @@ -2581,6 +2583,52 @@ irq_retry:
>  		s3c_hsotg_dump(hsotg);
>  	}
>  
> +	if (gintsts & GINTSTS_INCOMPL_SOIN) {
> +		u32 idx, epctl_reg, ctrl;
> +		struct s3c_hsotg_ep *hs_ep;
> +
> +		dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n", __func__);
> +		for (idx = 1; idx < MAX_EPS_CHANNELS; idx++) {

Valid endpoints are only up to hsotg->num_of_eps so this might
crash on certain configurations.

Also, have you tried to find the endpoint which caused the
incomplete interrupt by reading the control registers as
described in the databook?

> +			hs_ep = hsotg->eps_in[idx];
> +
> +			if (!hs_ep->isochronous || hs_ep->has_correct_parity)
> +				continue;
> +
> +			epctl_reg = DIEPCTL(idx);
> +			ctrl = readl(hsotg->regs + epctl_reg);
> +
> +			if (ctrl & DXEPCTL_EOFRNUM)
> +				ctrl |= DXEPCTL_SETEVENFR;
> +			else
> +				ctrl |= DXEPCTL_SETODDFR;
> +			writel(ctrl, hsotg->regs + epctl_reg);

This toggling code could be moved out to a helper function to
reduce redundancy as it is in three places now.

Regards,
John

--
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/