| 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: <ZT/XkhOvbucxA90V@lizhi-Precision-Tower-5810>
Date: Mon, 30 Oct 2023 12:21:32 -0400
From: Frank Li <Frank.li@....com>
To: Pawel Laszczak <pawell@...ence.com>
Cc: "peter.chen@...nel.org" <peter.chen@...nel.org>,
"rogerq@...nel.org" <rogerq@...nel.org>,
"a-govindraju@...com" <a-govindraju@...com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: Re: cdns3 uvc first ISO parkage lost problem
On Mon, Oct 30, 2023 at 06:34:48AM +0000, Pawel Laszczak wrote:
>
> >
> >hi Pawel Laszczak
> >
> >Recently, I met the problem when use uvc. UVC report jpg header error.
> >
> >Basic reproduce steps.
> >Gadget side:
> >1 -
> > https://urldefense.com/v3/__https://gist.github.com/kbingham/c39c
> >4cc7c20882a104c08df5206e2f9f?permalink_comment_id=3270713__;!!EHscm
> >S1ygiU1lA!H1h8GlLnbS6vqklXm_2qGyinP638O62Kk2eLB9zeMkjAGXUAPYyPXDL
> >FasSqYt16xq0RGT0Ff-cP4A$
> > uvc-gadget.sh start
> >2 -
> > https://urldefense.com/v3/__https://git.ideasonboard.org/uvc-
> >gadget.git__;!!EHscmS1ygiU1lA!H1h8GlLnbS6vqklXm_2qGyinP638O62Kk2eLB9z
> >eMkjAGXUAPYyPXDLFasSqYt16xq0RGT1ogOdRQA$
> > uvc-gadget -i test.jpg
> >
> >
> >Host side:
> > https://urldefense.com/v3/__https://github.com/thekvs/uvccapture2
> >__;!!EHscmS1ygiU1lA!H1h8GlLnbS6vqklXm_2qGyinP638O62Kk2eLB9zeMkjAGX
> >UAPYyPXDLFasSqYt16xq0RGT1MNlKiXA$
> > uvccapture2 --device /dev/video0 --resolution 640x360 --count 1 --
> >result 8qxp.jpeg
> >
> > It will report jpeg header error.
> >
> >
> >After debugs, I found two problem.
> >
> >Problem 1, sg is enabled. so uvc driver will use sg. each package include two
> >trb, trb0 is 8bytes header, trb1 is 1016bytes. total 1024.
> >
> >num_trb here is wrong.
> >it should be
> > num_trb = priv_ep->interval * request->num_mapped_sgs.
> >
> >because priv_ep->interval is 1, I just simple set to request->num_mapped_sg
> >as below patch. USB analyer show one whole 1024 ISO package sent out as
> >expectation although document said only support one TD when use ISO
> >(Maybe my doc is too old).
>
> Support for sg in uvc has been added after upstreaming this driver, so the driver
> needs some improvement.
>
> Calculating of num_trb probably will more complicated change.
>
> You can see how it is implemented in
> https://elixir.bootlin.com/linux/latest/source/drivers/usb/gadget/udc/cdns2/cdns2-gadget.c#L412.
>
> CDNS2 is different controller and support only HS but has borrowed the DMA part from CDNS3.
> It was upsteamed after adding sg to UVC.
>
> Regarding TD, it is true that controller can support only one TD per SOF but this TD can contain many TRBs
Okay, great. I can work a patch if I can resolve problem 2.
>
> >
> >diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-
> >gadget.c
> >index 69a44bd7e5d02..8cc99a885883f 100644
> >--- a/drivers/usb/cdns3/cdns3-gadget.c
> >+++ b/drivers/usb/cdns3/cdns3-gadget.c
> >@@ -1125,10 +1125,7 @@ static int cdns3_ep_run_transfer(struct
> >cdns3_endpoint *priv_ep,
> > struct scatterlist *s = NULL;
> > bool sg_supported = !!(request->num_mapped_sgs);
> >
> >- if (priv_ep->type == USB_ENDPOINT_XFER_ISOC)
> >- num_trb = priv_ep->interval;
> >- else
> >- num_trb = sg_supported ? request->num_mapped_sgs : 1;
> >+ num_trb = sg_supported ? request->num_mapped_sgs : 1;
> >
> > if (num_trb > priv_ep->free_trbs) {
> > priv_ep->flags |= EP_RING_FULL;
> >
> >
> >*** Problem 2 ***
> >
> >According to doc and my observation, looks like hardware fetch data into FIFO
> >when get SOF, then transfer data when get IN token. Each SOF will increase
> >TRB regardless it is ready or not.
>
> Yes, but this fetched data will be sent in next ITP.
>
> >
> >When gadget complete equeue ISO data, so SOF will increase TRB regardless if
> >there are IN token.
> >
> > SOF SOF SOF SOF IN SOF ....
> > TRB0 TRB1 TRB2 TRB3 ...
> >
> >
> >Host may start get data at some time after gadget queue data.
> >
> >So TRB0..2 data will be lost.
> >
> >If it is audio data, it should be okay. But for uvc, it is jpeg header, so host side
> >report error.
> >
> >I checked dwc gadget driver, which start equeue ISO data only get NYET.
> >
> >I check cdns spec, there are ISOERR. But it is never happen. According to
> >document, ISOERR should issue when IN token and FIFO no data.
> >
>
> Current CDNS3 driver has disabled ISOERR. Did you enable it?
Yes, I enabled all IRQ.
+ if (priv_ep->type == USB_ENDPOINT_XFER_ISOC && priv_ep->dir) {
+ priv_ep->flags |= EP_QUIRK_ISO_IN_NOST;
+ reg |= 0xFFFF;
+ }
Supposed ISOERR should happen even DMA is disabled.
But I also tried enable DMA, and using lenght 0 TRB and link to loop.
Still no ISOERR happen. I can see TRBADDR changed, but still no ISOERR
irq/447-5b13000-200 [000] d..1. 78.662729: cdns3_epx_irq: IRQ for ep2in: 00000804 IOC , ep_traddr: c0086018 ep_last_sid: 00000000 use_streams: 0
^^^^^^^
irq/447-5b13000-200 [000] d..1. 78.662851: cdns3_epx_irq: IRQ for ep2in: 00000804 IOC , ep_traddr: c008600c ep_last_sid: 00000000 use_streams: 0
irq/447-5b13000-200 [000] d..1. 78.662975: cdns3_epx_irq: IRQ for ep2in: 00000804 IOC , ep_traddr: c0086018 ep_last_sid: 00000000 use_streams: 0
STS is 0x804, only IOC set.
Frank
>
> >I tried below method
> > 1. Delay queue TRB, but no ISOERR.
> > 2. queue a lenght 0 TRB,but no ISOERR
> >
> >My question is how to delay queue TRB to ISO IN token really happen to avoid
> >lost JPEG header.
> >
> >Frank
> >
> >
> >
> >
> >
>
Powered by blists - more mailing lists