Re: dvb usb issues since kernel 4.9
On Mon, 8 Jan 2018, Linus Torvalds wrote: > Can somebody tell which softirq it is that dvb/usb cares about? I don't know about the DVB part. The USB part is a little difficult to analyze, mostly because the bug reports I've seen are mostly from people running non-vanilla kernels. For example, Josef is using a Raspberry Pi 3B with a non-standard USB host controller driver: dwc_otg_hcd is built into raspbian in place of the normal dwc2_hsotg driver. Both dwc2_hsotg and ehci-hcd use the tasklets embedded in the giveback_urb_bh member of struct usb_hcd. See usb_hcd_giveback_urb() in drivers/usb/core/hcd.c; the calls are else if (high_prio_bh) tasklet_hi_schedule(>bh); else tasklet_schedule(>bh); As it turns out, high_prio_bh gets set for interrupt and isochronous URBs but not for bulk and control URBs. The DVB driver in question uses bulk transfers. xhci-hcd, on the other hand, does not use these tasklets (it doesn't set the HCD_BH bit in the hc_driver's .flags member). Alan Stern
Re: Aw: Re: Re: dvb usb issues since kernel 4.9
On Mon, 8 Jan 2018, Josef Griebichler wrote: > No I can't sorry. There's no sat connection near to my workstation. Can we ask the person who made this post: https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/?postID=75965#post75965 to run the test? The post says that the testing was done on an x86_64 machine. > Gesendet: Montag, 08. Januar 2018 um 17:31 Uhr > Von: "Alan Stern" <st...@rowland.harvard.edu> > An: "Josef Griebichler" <griebichler.jo...@gmx.at> > Cc: "Mauro Carvalho Chehab" <mche...@s-opensource.com>, "Greg Kroah-Hartman" > <gre...@linuxfoundation.org>, linux-...@vger.kernel.org, "Eric Dumazet" > <eduma...@google.com>, "Rik van Riel" <r...@redhat.com>, "Paolo Abeni" > <pab...@redhat.com>, "Hannes Frederic Sowa" <han...@redhat.com>, "Jesper > Dangaard Brouer" <jbro...@redhat.com>, linux-kernel > <linux-ker...@vger.kernel.org>, netdev <net...@vger.kernel.org>, "Jonathan > Corbet" <cor...@lwn.net>, LMML <linux-media@vger.kernel.org>, "Peter > Zijlstra" <pet...@infradead.org>, "David Miller" <da...@davemloft.net>, > torva...@linux-foundation.org > Betreff: Re: Aw: Re: dvb usb issues since kernel 4.9 > On Mon, 8 Jan 2018, Josef Griebichler wrote: > Hi Maro, > > I tried your > mentioned patch but unfortunately no real improvement for me. > dmesg > http://ix.io/DOg > tvheadend service log http://ix.io/DOi[http://ix.io/DOi] > > Errors during recording are still there. > Errors increase if there is > additional tcp load on raspberry. > > Unfortunately there's no usbmon or > tshark on libreelec so I can't provide further logs. Can you try running the > same test on an x86_64 system? Alan Stern It appears that you are using a non-standard kernel. The vanilla kernel does not include any "dwc_otg_hcd" driver. Alan Stern
Re: Aw: Re: dvb usb issues since kernel 4.9
On Mon, 8 Jan 2018, Josef Griebichler wrote: > Hi Maro, > > I tried your mentioned patch but unfortunately no real improvement for me. > dmesg http://ix.io/DOg > tvheadend service log http://ix.io/DOi > Errors during recording are still there. > Errors increase if there is additional tcp load on raspberry. > > Unfortunately there's no usbmon or tshark on libreelec so I can't provide > further logs. Can you try running the same test on an x86_64 system? Alan Stern
Re: dvb usb issues since kernel 4.9
On Mon, 8 Jan 2018, Mauro Carvalho Chehab wrote:
> > Let find the root-cause of this before reverting, as this will hurt the
> > networking use-case.
> >
> > I want to see if the increase buffer will solve the issue (the current
> > buffer of 0.63 ms seem too small).
>
> For TV, high latency has mainly two practical consequences:
>
> 1) it increases the time to switch channels. MPEG-TS based transmissions
>usually takes some time to start showing the channel contents. Adding
>more buffers make it worse;
>
> 2) specially when watching sports, a higher latency means that you'll know
>that your favorite team made a score when your neighbors start
>celebrating... seeing the actual event only after them.
>
> So, the lower, the merrier, but I think that 5 ms would be acceptable.
That value 65 for the number of buffers was calculated based on a
misunderstanding of the actual bandwidth requirement. Still increasing
the number of buffers shouldn't hurt, and it's worth trying.
But there is another misunderstanding here which needs to be cleared
up. Adding more buffers does _not_ increase latency; it increases
capacity. Making each buffer larger _would_ increase latency, but
that's not what I proposed.
Going through this more explicitly... Suppose you receive 8 KB of data
every ms, and suppose you have four 8-KB buffers. Then the latency is
1 ms, because that's how long you have to wait for the first buffer to
be filled up after you submit an I/O request. (The driver does _not_
need to wait for all four buffers to be filled before it can start
displaying the data in the first buffer.) The capacity would be 4 ms,
because that's how much data your buffers can store. If you end up
waiting longer than 4 ms before ksoftirqd gets around to processing any
of the data, then some data will inevitably get lost.
That's why the way to deal with the delays caused by deferring softirqs
to ksoftirqd is to add more buffers (and not make the buffers larger
than they already are).
> > I would also like to see experiments with adjusting adjust the sched
> > priority of the kthread's and/or the userspace prog. (e.g use command
> > like 'sudo chrt --fifo -p 10 $(pgrep udp_sink)' ).
>
> If this fixes the issue, we'll need to do something inside the Kernel
> to change the priority, as TV userspace apps should not run as root. Not
> sure where such change should be done (USB? media?).
It would be interesting to try this, but I agree that it's not likely
to be a practical solution. Anyway, shouldn't ksoftirqd already be
running with very high priority?
> > Are we really sure that the regression is cause by 4cd13c21b207
> > ("softirq: Let ksoftirqd do its job"), the forum thread also report
> > that the problem is almost gone after commit 34f41c0316ed ("timers: Fix
> > overflow in get_next_timer_interrupt")
> > https://git.kernel.org/torvalds/c/34f41c0316ed
That is a good point. It's hard to see how the issues in the two
commits could be related, but who knows?
> I'll see if I can mount a test scenario here in order to try reproduce
> the reported bug. I suspect that I won't be able to reproduce it on my
> "standard" i7core-based test machine, even with KPTI enabled.
If you're using the same sort of hardware as Josef, under similar
circumstances, the buggy bahavior should be the same. If not, there
must be something else going on that we're not aware of.
> > It makes me suspicious that this fix changes things...
> > After this fix, I suspect that changing the sched priorities, will fix
> > the remaining glitches.
> >
> >
> > > It is hard to foresee the consequences of the softirq changes for other
> > > devices, though.
> >
> > Yes, it is hard to foresee, I can only cover networking.
> >
> > For networking, if reverting this, we will (again) open the kernel for
> > an easy DDoS vector with UDP packets. As mentioned in the commit desc,
> > before you could easily cause softirq to take all the CPU time from the
> > application, resulting in very low "good-put" in the UDP-app. (That's why
> > it was so easy to DDoS DNS servers before...)
> >
> > With the softirqd patch in place, ksoftirqd is scheduled fairly between
> > other applications running on the same CPU. But in some cases this is
> > not what you want, so as the also commit mentions, the admin can now
> > more easily tune process scheduling parameters if needed, to adjust for
> > such use-cases (it was not really an admin choice before).
>
> Can't the ksoftirq patch be modified to only apply to the networking
> IRQ handling? That sounds less risky of affecting unrelated subsystems[1].
That might work. Or
Re: dvb usb issues since kernel 4.9
On Mon, 8 Jan 2018, Mauro Carvalho Chehab wrote: > Em Sun, 7 Jan 2018 10:41:37 -0500 (EST) > Alan Stern <st...@rowland.harvard.edu> escreveu: > > > On Sun, 7 Jan 2018, Mauro Carvalho Chehab wrote: > > > > > > > It seems that the original patch were designed to solve some IRQ > > > > > issues > > > > > with network cards with causes data losses on high traffic. However, > > > > > it is also causing bad effects on sustained high bandwidth demands > > > > > required by DVB cards, at least on some USB host drivers. > > > > > > > > > > Alan/Greg/Eric/David: > > > > > > > > > > Any ideas about how to fix it without causing regressions to > > > > > network? > > > > > > > > It would be good to know what hardware was involved on the x86 system > > > > and to have some timing data. Can we see the output from lsusb and > > > > usbmon, running on a vanilla kernel that gets plenty of video glitches? > > > > > > > > > > From Josef's report, and from the BZ, the affected hardware seems > > > to be based on Montage Technology M88DS3103/M88TS2022 chipset. > > > > What type of USB host controller does the x86_64 system use? EHCI or > > xHCI? > > I'll let Josef answer this. > > > > > > The driver it uses is at drivers/media/usb/dvb-usb-v2/dvbsky.c, > > > with shares a USB implementation that is used by a lot more drivers. > > > The URB handling code is at: > > > > > > drivers/media/usb/dvb-usb-v2/usb_urb.c > > > > > > This particular driver allocates 8 buffers with 4096 bytes each > > > for bulk transfers, using transfer_flags = URB_NO_TRANSFER_DMA_MAP. > > > > > > This become a popular USB hardware nowadays. I have one S960c > > > myself, so I can send you the lsusb from it. You should notice, however, > > > that a DVB-C/DVB-S2 channel can easily provide very high sustained bit > > > rates. Here, on my DVB-S2 provider, a typical transponder produces 58 Mpps > > > of payload after removing URB headers. > > > > You mentioned earlier that the driver uses bulk transfers. In USB-2.0, > > the maximum possible payload data transfer rate using bulk transfers is > > 53248 bytes/ms, which is 53.248 MB/s (i.e., lower than 58 MB/s). And > > even this is possible only if almost nothing else is using the bus at > > the same time. > > No, I said 58 Mbits/s (not bytes). Well, what you actually _wrote_ was "58 Mpps of payload" (see above), and I couldn't tell how to interpret that. :-) 58 Mb/s is obviously almost 8 times less than the full USB bus bandwidth. > On DVB-C and DVB-S2 specs, AFAIKT, there's no hard limit for the maximum > payload data rate, although industry seems to limit it to be around > 60 Mbits/s. On those standards, the maximal bit rate is defined by the > modulation type and by the channel symbol rate. > > To give you a practical example, my DVB-S2 provider modulates each > transponder with 8/PSK (3 bits/symbol), and define channels with a > symbol rate of 30 Mbauds/s. So, it could, theoretically, transport > a MPEG-TS stream up to 90 Mbits/s (minus headers and guard intervals). > In practice, the streams there are transmitted with 58,026.5 Kbits/s. Okay. This is 58 Kb/ms or 7.25 KB/ms. So your scheme of eight 4-KB buffers gives a latency of 0.57 ms with a total capacity of 4.5 ms, which is a lot better than what I was thinking. > > In any case, you might be able to attack the problem simply by using > > more than 8 buffers. With just eight 4096-byte buffers, the total > > pipeline capacity is only about 0.62 ms (at the maximum possible > > transfer rate). Increasing the number of buffers to 65 would give a > > capacity of 5 ms, which is probably a lot better suited for situations > > where completions are handled by the ksoftirqd thread. > > Increasing it to 65 shouldn't be hard. Not sure, however, if the hardware > will actually fill the 65 buffers, but it is worth to try. Given the new information, 65 would be overkill. But going from 8 to 16 might help. > > > Perhaps media drivers could pass some quirk similar to URB_ISO_ASAP, > > > in order to revert the kernel logic to prioritize latency instead of > > > throughput. > > > > It can't be done without pervasive changes to the USB subsystem, which > > I would greatly prefer to avoid. Besides, this wouldn't really solve > > the problem. Decreasing the latency for one device will cause it to be > > increased for others. > > If there is a TV streaming traffic at a USB bus, it means that the > user wants to either watch and/or record a TV program. On such > usecase scenario, a low latency is highly desired for the TV capture > (and display, if the GPU is USB), even it means a higher latency for > other traffic. Not if the other traffic is also a TV capture. :-) It might make sense to classify softirq sources as "high priority" or "low priority", and only defer the "low priority" work to ksoftirqd. Alan Stern
Re: dvb usb issues since kernel 4.9
On Sun, 7 Jan 2018, Mauro Carvalho Chehab wrote: > > > It seems that the original patch were designed to solve some IRQ issues > > > with network cards with causes data losses on high traffic. However, > > > it is also causing bad effects on sustained high bandwidth demands > > > required by DVB cards, at least on some USB host drivers. > > > > > > Alan/Greg/Eric/David: > > > > > > Any ideas about how to fix it without causing regressions to > > > network? > > > > It would be good to know what hardware was involved on the x86 system > > and to have some timing data. Can we see the output from lsusb and > > usbmon, running on a vanilla kernel that gets plenty of video glitches? > > From Josef's report, and from the BZ, the affected hardware seems > to be based on Montage Technology M88DS3103/M88TS2022 chipset. What type of USB host controller does the x86_64 system use? EHCI or xHCI? > The driver it uses is at drivers/media/usb/dvb-usb-v2/dvbsky.c, > with shares a USB implementation that is used by a lot more drivers. > The URB handling code is at: > > drivers/media/usb/dvb-usb-v2/usb_urb.c > > This particular driver allocates 8 buffers with 4096 bytes each > for bulk transfers, using transfer_flags = URB_NO_TRANSFER_DMA_MAP. > > This become a popular USB hardware nowadays. I have one S960c > myself, so I can send you the lsusb from it. You should notice, however, > that a DVB-C/DVB-S2 channel can easily provide very high sustained bit > rates. Here, on my DVB-S2 provider, a typical transponder produces 58 Mpps > of payload after removing URB headers. You mentioned earlier that the driver uses bulk transfers. In USB-2.0, the maximum possible payload data transfer rate using bulk transfers is 53248 bytes/ms, which is 53.248 MB/s (i.e., lower than 58 MB/s). And even this is possible only if almost nothing else is using the bus at the same time. > A 10 minutes record with the > entire data (with typically contains 5-10 channels) can easily go > above 4 GB, just to reproduce 1-2 glitches. So, I'm not sure if > a usbmon dump would be useful. It might not be helpful at all. However, I'm not interested in the payload data (which would be unintelligible to me anyway) but rather the timing of URB submissions and completions. A usbmon trace which didn't keep much of the payload data would only require on the order of 50 MB per minute -- and Josef said that glitches usually would show up within a minute or so. > I'm enclosing the lsusb from a S960C device, with is based on those > Montage chipsets: What I wanted to see was the output from "lsusb" on the affected system, not the output from "lsusb -v -s B:D" on your system. > > Overall, this may be a very difficult problem to solve. The > > 4cd13c21b207 commit was intended to improve throughput at the cost of > > increased latency. But then what do you do when the latency becomes > > too high for the video subsystem to handle? > > Latency can't be too high, otherwise frames will be dropped. Yes, that's the whole point. > Even if the Kernel itself doesn't drop, if the delay goes higher > than a certain threshold, userspace will need to drop, as it > should be presenting audio and video on real time. Yet, typically, > userspace will delay it by one or two seconds, with would mean > 1500-3500 buffers, with I suspect it is a lot more than the hardware > limits. So I suspect that the hardware starves free buffers a way > before userspace, as media hardware don't have unlimited buffers > inside them, as they assume that the Kernel/userspace will be fast > enough to sustain bit rates up to 66 Mbps of payload. The timing information would tell us how large the latency is. In any case, you might be able to attack the problem simply by using more than 8 buffers. With just eight 4096-byte buffers, the total pipeline capacity is only about 0.62 ms (at the maximum possible transfer rate). Increasing the number of buffers to 65 would give a capacity of 5 ms, which is probably a lot better suited for situations where completions are handled by the ksoftirqd thread. > Perhaps media drivers could pass some quirk similar to URB_ISO_ASAP, > in order to revert the kernel logic to prioritize latency instead of > throughput. It can't be done without pervasive changes to the USB subsystem, which I would greatly prefer to avoid. Besides, this wouldn't really solve the problem. Decreasing the latency for one device will cause it to be increased for others. Alan Stern
Re: dvb usb issues since kernel 4.9
On Sat, 6 Jan 2018, Mauro Carvalho Chehab wrote: > Hi Josef, > > Em Sat, 6 Jan 2018 16:04:16 +0100 > "Josef Griebichler" <griebichler.jo...@gmx.at> escreveu: > > > Hi, > > > > the causing commit has been identified. > > After reverting commit > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cd13c21b207e80ddb1144c576500098f2d5f882 > > its working again. > > Just replying to me won't magically fix this. The ones that were involved on > this patch should also be c/c, plus USB people. Just added them. > > > Please have a look into the thread > > https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/?pageNo=13 > > here are several users aknowledging the revert solves their issues with usb > > dvb cards. > > I read the entire (long) thread there. In order to make easier for the > others, from what I understand, the problem happens on both x86 and arm, > although almost all comments there are mentioning tests with raspbian > Kernel (with uses a different USB host driver than the upstream one). > > It happens when watching digital TV DVB-C channels, with usually means > a sustained bit rate of 11 MBps to 54 MBps. > > The reports mention the dvbsky, with uses USB URB bulk transfers. > On every several minutes (5 to 10 mins), the stream suffer "glitches" > caused by frame losses. > > The part of the thread that contains the bisect is at: > > https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/?postID=75965#post75965 > > It indirectly mentions another comment on the thread with points > to: > https://github.com/raspberrypi/linux/issues/2134 > > There, it says that this fix part of the issues: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=34f41c0316ed52b0b44542491d89278efdaa70e4 > > but it affects URB packet losses on a lesser extend. > > The main issue is really the logic changes a the core softirq logic. > > Using Kernel 4.14.10 on a Raspberry Pi 3 with 4cd13c2 commit reverted > fixed the issue. > > Joseph, is the above right? Anything else to mention? Does the > same issue affect also on x86 with vanilla Kernel 4.14.10? > > - > > It seems that the original patch were designed to solve some IRQ issues > with network cards with causes data losses on high traffic. However, > it is also causing bad effects on sustained high bandwidth demands > required by DVB cards, at least on some USB host drivers. > > Alan/Greg/Eric/David: > > Any ideas about how to fix it without causing regressions to > network? It would be good to know what hardware was involved on the x86 system and to have some timing data. Can we see the output from lsusb and usbmon, running on a vanilla kernel that gets plenty of video glitches? Overall, this may be a very difficult problem to solve. The 4cd13c21b207 commit was intended to improve throughput at the cost of increased latency. But then what do you do when the latency becomes too high for the video subsystem to handle? Alan Stern
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
On Thu, 30 Mar 2017, Oliver Neukum wrote: > Am Donnerstag, den 30.03.2017, 11:55 -0400 schrieb Alan Stern: > > > > I'm pretty sure that usb-storage does not do this, at least, not when > > operating in its normal Bulk-Only-Transport mode. It never tries to > > read the results of an earlier transfer after carrying out a later > > transfer to any part of the same buffer. > > The storage driver takes buffers as the block layer (or sg) provide > them, does it not? Yes. But it does not read the data from an earlier transfer after carrying out a later transfer on the same buffer. The only possible example would be if the sense buffer for a command occupied part of the same block as the data buffer. But this can't happen, because the sense buffer is allocated separately by its own kzalloc_node() call in scsi_init_request(). Alan Stern
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
On Thu, 30 Mar 2017, Mauro Carvalho Chehab wrote:
> Em Thu, 30 Mar 2017 10:26:32 -0400 (EDT)
> Alan Stern <st...@rowland.harvard.edu> escreveu:
>
> > On Thu, 30 Mar 2017, Oliver Neukum wrote:
> >
> > > > Btw, I'm a lot more concerned about USB storage drivers. When I was
> > > > discussing about this issue at the #raspberrypi-devel IRC channel,
> > > > someone complained that, after switching from the RPi downstream Kernel
> > > > to upstream, his USB data storage got corrupted. Well, if the USB
> > > > storage drivers also assume that the buffer can be continuous,
> > > > that can corrupt data.
> >
> > >
> > > They do assume that.
> >
> > Wait a minute. Where does that assumption occur?
> >
> > And exactly what is the assumption? Mauro wrote "the buffer can be
> > continuous", but that is certainly not what he meant.
>
> What I meant to say is that drivers like the uvcdriver (and maybe network and
> usb-storage drivers) may allocate a big buffer and get data there on some
> random order, e. g.:
>
> int get_from_buf_pos(char *buf, int pos, int size)
> {
> /* or an equivalent call to usb_submit_urb() */
> usb_control_msg(..., buf + pos, size, ...);
> }
>
> some_function ()
> {
> ...
>
> chr *buf = kzalloc(4, GFP_KERNEL);
>
> /*
>* Access the bytes at the array on a random order, with random size,
>* Like:
>*/
> get_from_buf_pos(buf, 2, 2);/* should read 0x56, 0x78 */
> get_from_buf_pos(buf, 0, 2);/* should read 0x12, 0x34 */
>
> /*
>* the expected value for the buffer would be:
>* { 0x12, 0x34, 0x56, 0x78 }
>*/
>
> E. g. they assume that the transfer URB can work with any arbitrary
> pointer and size, without needing of pre-align them.
>
> This doesn't work with HCD drivers like dwc2, as each USB_IN operation will
> actually write 4 bytes to the buffer.
>
> So, what happens, instead, is that each data transfer will get four
> bytes. Due to a hack inside dwc2, with checks if the transfer_buffer
> is DWORD aligned. So, the first transfer will do what's expected: it will
> read 4 bytes to a temporary buffer, allocated inside the driver,
> copying just two bytes to buf. So, after the first read, the
> buffer content will be:
>
> buf = { 0x00, x00, 0x56, 0x78 }
>
> But, on the second read, it won't be using any temporary
> buffer. So, instead of reading a 16-bits word (0x5678),
> it will actually read 32 bits, with 16-bits with some random value,
> causing a buffer overflow. E. g. buffer content will now be:
>
> buf = { 0x12, x34, 0xde, 0xad }
>
> In other words, the second transfer corrupted the data from the
> first transfer.
I'm pretty sure that usb-storage does not do this, at least, not when
operating in its normal Bulk-Only-Transport mode. It never tries to
read the results of an earlier transfer after carrying out a later
transfer to any part of the same buffer.
Alan Stern
Re: [PATCH 22/22] usb: document that URB transfer_buffer should be aligned
On Thu, 30 Mar 2017, Oliver Neukum wrote: > > Btw, I'm a lot more concerned about USB storage drivers. When I was > > discussing about this issue at the #raspberrypi-devel IRC channel, > > someone complained that, after switching from the RPi downstream Kernel > > to upstream, his USB data storage got corrupted. Well, if the USB > > storage drivers also assume that the buffer can be continuous, > > that can corrupt data. > > They do assume that. Wait a minute. Where does that assumption occur? And exactly what is the assumption? Mauro wrote "the buffer can be continuous", but that is certainly not what he meant. Alan Stern
Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
On Mon, 28 Nov 2016, Laurent Pinchart wrote: > > Well, I admit it would be nicer if drivers didn't have to worry about > > whether or not CONFIG_PM was enabled. A slightly cleaner approach > > from the one outlined above would have the probe routine do this: > > > > my_power_up(dev); > > pm_runtime_set_active(dev); > > pm_runtime_get_noresume(dev); > > pm_runtime_enable(dev); > > > > and have the runtime-resume callback routine call my_power_up() to do > > its work. (Or make my_power_up() actually be the runtime-resume > > callback routine.) That's pretty straightforward and hard to mess up. > > You'd be surprised how easy drivers can mess simple things up ;-) No -- I wouldn't! :-) > We'd still > have to get the message out there, that would be the most difficult part. Agreed. > > In theory, we could have pm_runtime_enable() invoke the runtime-resume > > callback when CONFIG_PM is disabled. In practice, it would be rather > > awkward. drivers/base/power/runtime.c, which is where > > pm_runtime_enable() is defined and the runtime-PM callbacks are > > invoked, doesn't even get compiled if CONFIG_PM is off. > > Sure, but that can easily be fixed. > > > (Also, it would run against the grain. CONFIG_PM=n means the kernel > > ignores runtime PM, so pm_runtime_enable() shouldn't do anything.) > > I'd argue that CONFIG_PM=n should mean that the runtime PM API doesn't > perform > runtime PM, not that it should do absolutely nothing. If semantics is the > biggest concern, we could introduce a helper (whose name is TBD) that would > enable runtime PM when CONFIG_PM=y or power on the device when CONFIG_PM=n Or have the driver call _both_ the helper routine and pm_runtime_enable() -- the helper would do nothing if CONFIG_PM=y, and it would invoke the runtime-resume callback if CONFIG_PM=n. Either way would be a good approach. Having pm_runtime_enable() call the runtime-resume handler wouldn't work well if the driver has already powered-up the device or the device starts out in the power-on state (which is often the case). > I want to make it as easy as possible for drivers to make sure they won't get > this wrong, which in my opinion requires a simple and straightforward API > with > no code in the driver that would depend on the value of CONFIG_PM. Well, the approach I outlined above is pretty simple and it doesn't depend on the value of CONFIG_PM. Your proposal is just as simple, but it does require drivers to remember to call the new helper routine. > > There's a corollary aspect to this. If you depend on runtime PM for > > powering up your device during probe, does that mean you also depend on > > runtime PM for powering down the device during remove? That is likely > > not to work, because the user can prevent runtime suspends by writing > > to /sys/.../power/control. > > Yes, I do, and I expect most runtime PM-enabled driver to do the same. When > runtime suspend is disabled through /sys/.../power/control does > pm_runtime_disable() invoke the runtime PM suspend handler if the device is > powered on ? No, it doesn't, and neither does pm_runtime_put(). After all, if the user has told the system not to do runtime PM on that device, it doesn't make sense to call the runtime-suspend handler. But you can always blame the user when this happens. :-) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
On Fri, 25 Nov 2016, Laurent Pinchart wrote: > Hi Alan, Hello. > On Friday 25 Nov 2016 10:21:21 Alan Stern wrote: > > On Fri, 25 Nov 2016, Sakari Ailus wrote: > > > On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote: > > >> On Fri, 25 Nov 2016, Laurent Pinchart wrote: > > >>> Dear linux-pm developers, what's the suggested way to ensure that a > > >>> runtime- pm-enabled driver can run fine on a system with CONFIG_PM > > >>> disabled ? > > >> > > >> The exact point of your question isn't entirely clear. In the most > > >> literal sense, the best ways to ensure this are (1) audit the code, and > > >> (2) actually try it. > > >> > > >> I have a feeling this doesn't quite answer your question, however. :-) > > > > > > The question is related to devices that require certain power-up and > > > power-down sequences that are now implemented as PM runtime hooks that, > > > without CONFIG_PM defined, will not be executed. Is there a better way > > > than to handle this than have an implementation in the driver for the PM > > > runtime and non-PM runtime case separately? > > > > Yes, there is a better way. For the initial power-up and final > > power-down sequences, don't rely on the PM core to invoke the > > callbacks. Just call them directly, yourself. > > > > For example, as part of the probe routine, instead of doing this: > > > > pm_runtime_set_suspended(dev); > > pm_runtime_enable(dev); > > pm_runtime_get_sync(dev); > > > > Do this: > > > > pm_runtime_set_active(dev); > > pm_runtime_get_noresume(dev); > > pm_runtime_enable(dev); > > /* > > * In case CONFIG_PM is disabled, invoke the runtime-resume > > * callback directly. > > */ > > my_runtime_resume(dev); > > Wouldn't it be cleaner for drivers not to have to handle this manually (which > gives an opportunity to get it wrong) but instead have pm_runtime_enable() > call the runtime resume callback when CONFIG_PM is disabled ? Well, I admit it would be nicer if drivers didn't have to worry about whether or not CONFIG_PM was enabled. A slightly cleaner approach from the one outlined above would have the probe routine do this: my_power_up(dev); pm_runtime_set_active(dev); pm_runtime_get_noresume(dev); pm_runtime_enable(dev); and have the runtime-resume callback routine call my_power_up() to do its work. (Or make my_power_up() actually be the runtime-resume callback routine.) That's pretty straightforward and hard to mess up. In theory, we could have pm_runtime_enable() invoke the runtime-resume callback when CONFIG_PM is disabled. In practice, it would be rather awkward. drivers/base/power/runtime.c, which is where pm_runtime_enable() is defined and the runtime-PM callbacks are invoked, doesn't even get compiled if CONFIG_PM is off. (Also, it would run against the grain. CONFIG_PM=n means the kernel ignores runtime PM, so pm_runtime_enable() shouldn't do anything.) There's a corollary aspect to this. If you depend on runtime PM for powering up your device during probe, does that mean you also depend on runtime PM for powering down the device during remove? That is likely not to work, because the user can prevent runtime suspends by writing to /sys/.../power/control. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
On Fri, 25 Nov 2016, Sakari Ailus wrote: > Hi Alan and others, > > On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote: > > On Fri, 25 Nov 2016, Laurent Pinchart wrote: > > > > > Dear linux-pm developers, what's the suggested way to ensure that a > > > runtime- > > > pm-enabled driver can run fine on a system with CONFIG_PM disabled ? > > > > The exact point of your question isn't entirely clear. In the most > > literal sense, the best ways to ensure this are (1) audit the code, and > > (2) actually try it. > > > > I have a feeling this doesn't quite answer your question, however. :-) > > The question is related to devices that require certain power-up and > power-down sequences that are now implemented as PM runtime hooks that, > without CONFIG_PM defined, will not be executed. Is there a better way than > to handle this than have an implementation in the driver for the PM runtime > and non-PM runtime case separately? Yes, there is a better way. For the initial power-up and final power-down sequences, don't rely on the PM core to invoke the callbacks. Just call them directly, yourself. For example, as part of the probe routine, instead of doing this: pm_runtime_set_suspended(dev); pm_runtime_enable(dev); pm_runtime_get_sync(dev); Do this: pm_runtime_set_active(dev); pm_runtime_get_noresume(dev); pm_runtime_enable(dev); /* * In case CONFIG_PM is disabled, invoke the runtime-resume * callback directly. */ my_runtime_resume(dev); Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
On Fri, 25 Nov 2016, Laurent Pinchart wrote: > Dear linux-pm developers, what's the suggested way to ensure that a runtime- > pm-enabled driver can run fine on a system with CONFIG_PM disabled ? The exact point of your question isn't entirely clear. In the most literal sense, the best ways to ensure this are (1) audit the code, and (2) actually try it. I have a feeling this doesn't quite answer your question, however. :-) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mceusb xhci issue?
On Thu, 15 Sep 2016, Wade Berrier wrote: > On Thu Sep 15 15:13, Alan Stern wrote: > > On Sat, 10 Sep 2016, Wade Berrier wrote: > > > > > On Thu Aug 11 16:18, Alan Stern wrote: > > > > I never received any replies to this message. Should the patch I > > > > suggested be merged? > > > > > > > > > > Hello, > > > > > > I applied this updated patch to the fedora23 4.7.2 kernel and the mceusb > > > transceiver works as expected. > > > > Thank you for testing. Can you provide the "lsusb -v" output for the > > troublesome IR transceiver? > > > > Here's the output: > > Bus 001 Device 006: ID 1784:0006 TopSeed Technology Corp. eHome Infrared > Transceiver > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass0 > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize0 8 > idVendor 0x1784 TopSeed Technology Corp. > idProduct 0x0006 eHome Infrared Transceiver > bcdDevice1.02 > iManufacturer 1 TopSeed Technology Corp. > iProduct2 eHome Infrared Transceiver > iSerial 3 TS004RrP > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 32 > bNumInterfaces 1 > bConfigurationValue 1 > iConfiguration 0 > bmAttributes 0xa0 > (Bus Powered) > Remote Wakeup > MaxPower 100mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass255 Vendor Specific Subclass > bInterfaceProtocol255 Vendor Specific Protocol > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x01 EP 1 OUT > bmAttributes3 > Transfer TypeInterrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x0020 1x 32 bytes > bInterval 0 And there's the problem. 0 is an invalid bInterval value for an Interrupt endpoint. > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x81 EP 1 IN > bmAttributes3 > Transfer Type Interrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x0020 1x 32 bytes > bInterval 0 Here too. > Device Status: 0x0001 > Self Powered > > Wade Thank you. The patch has been submitted. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: change bInterval default to 10 ms
Some full-speed mceusb infrared transceivers contain invalid endpoint
descriptors for their interrupt endpoints, with bInterval set to 0.
In the past they have worked out okay with the mceusb driver, because
the driver sets the bInterval field in the descriptor to 1,
overwriting whatever value may have been there before. However, this
approach was never sanctioned by the USB core, and in fact it does not
work with xHCI controllers, because they use the bInterval value that
was present when the configuration was installed.
Currently usbcore uses 32 ms as the default interval if the value in
the endpoint descriptor is invalid. It turns out that these IR
transceivers don't work properly unless the interval is set to 10 ms
or below. To work around this mceusb problem, this patch changes the
endpoint-descriptor parsing routine, making the default interval value
be 10 ms rather than 32 ms.
Signed-off-by: Alan Stern <st...@rowland.harvard.edu>
Tested-by: Wade Berrier <wberr...@gmail.com>
CC: <sta...@vger.kernel.org>
---
[as1812]
drivers/usb/core/config.c | 28 +---
1 file changed, 17 insertions(+), 11 deletions(-)
Index: usb-4.x/drivers/usb/core/config.c
===
--- usb-4.x.orig/drivers/usb/core/config.c
+++ usb-4.x/drivers/usb/core/config.c
@@ -240,8 +240,10 @@ static int usb_parse_endpoint(struct dev
memcpy(>desc, d, n);
INIT_LIST_HEAD(>urb_list);
- /* Fix up bInterval values outside the legal range. Use 32 ms if no
-* proper value can be guessed. */
+ /*
+* Fix up bInterval values outside the legal range.
+* Use 10 or 8 ms if no proper value can be guessed.
+*/
i = 0; /* i = min, j = max, n = default */
j = 255;
if (usb_endpoint_xfer_int(d)) {
@@ -250,13 +252,15 @@ static int usb_parse_endpoint(struct dev
case USB_SPEED_SUPER_PLUS:
case USB_SPEED_SUPER:
case USB_SPEED_HIGH:
- /* Many device manufacturers are using full-speed
+ /*
+* Many device manufacturers are using full-speed
* bInterval values in high-speed interrupt endpoint
-* descriptors. Try to fix those and fall back to a
-* 32 ms default value otherwise. */
+* descriptors. Try to fix those and fall back to an
+* 8-ms default value otherwise.
+*/
n = fls(d->bInterval*8);
if (n == 0)
- n = 9; /* 32 ms = 2^(9-1) uframes */
+ n = 7; /* 8 ms = 2^(7-1) uframes */
j = 16;
/*
@@ -271,10 +275,12 @@ static int usb_parse_endpoint(struct dev
}
break;
default:/* USB_SPEED_FULL or _LOW */
- /* For low-speed, 10 ms is the official minimum.
+ /*
+* For low-speed, 10 ms is the official minimum.
* But some "overclocked" devices might want faster
-* polling so we'll allow it. */
- n = 32;
+* polling so we'll allow it.
+*/
+ n = 10;
break;
}
} else if (usb_endpoint_xfer_isoc(d)) {
@@ -282,10 +288,10 @@ static int usb_parse_endpoint(struct dev
j = 16;
switch (to_usb_device(ddev)->speed) {
case USB_SPEED_HIGH:
- n = 9; /* 32 ms = 2^(9-1) uframes */
+ n = 7; /* 8 ms = 2^(7-1) uframes */
break;
default:/* USB_SPEED_FULL */
- n = 6; /* 32 ms = 2^(6-1) frames */
+ n = 4; /* 8 ms = 2^(4-1) frames */
break;
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mceusb xhci issue?
Mauro: I just took a look at the mceusb.c source file under drivers/media/rc/. The probe routine checks that ep_in != NULL, but it doesn't check ep_out. This can lead to a NULL-pointer dereference later on, crashing the driver. Such a crash was reported here: http://marc.info/?l=mythtv-users=144131333703197=2 You should a check for ep_out to the probe routine. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mceusb xhci issue?
On Sat, 10 Sep 2016, Wade Berrier wrote: > On Thu Aug 11 16:18, Alan Stern wrote: > > I never received any replies to this message. Should the patch I > > suggested be merged? > > > > Hello, > > I applied this updated patch to the fedora23 4.7.2 kernel and the mceusb > transceiver works as expected. Thank you for testing. Can you provide the "lsusb -v" output for the troublesome IR transceiver? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: pwc over musb: 100% frame drop (lost) on high resolution stream
On Sun, 21 Aug 2016, Matwey V. Kornilov wrote: > In both cases (with or without HCD_BH), usb_hcd_giveback_urb is called > every 0.01 sec. It is not clear why behavior is so different. What behavior are you asking about? The difference between HCD_BH set and not set? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mceusb xhci issue?
I never received any replies to this message. Should the patch I
suggested be merged?
Alan Stern
On Tue, 12 Jul 2016, Alan Stern wrote:
> On Sat, 9 Jul 2016, Mauro Carvalho Chehab wrote:
>
> > C/C linux-usb Mailing list:
> >
> >
> > Em Wed, 18 May 2016 08:52:28 -0600
> > Wade Berrier <wberr...@gmail.com> escreveu:
>
> ...
>
> > > > That message above links to some other threads describing the issue.
> > > > Here's a post with a patch that supposedly works:
> > > >
> > > > http://www.gossamer-threads.com/lists/mythtv/users/587930
> > > >
> > > > No idea if that's the "correct" way to fix this.
> > > >
> > > > I'll be trying that out and then report back...
> > >
> > > Indeed, this patch does fix the issue:
> > >
> > > --
> > >
> > > diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> > > index 31ccdcc..03321d4 100644
> > > --- a/drivers/usb/core/config.c
> > > +++ b/drivers/usb/core/config.c
> > > @@ -247,7 +247,7 @@ static int usb_parse_endpoint(struct device *ddev,
> > > int cfgno, int inum,
> > > /* For low-speed, 10 ms is the official minimum.
> > >* But some "overclocked" devices might want faster
> > >* polling so we'll allow it. */
> > > - n = 32;
> > > + n = 10;
> > > break;
> > > }
> > > } else if (usb_endpoint_xfer_isoc(d)) {
> > >
> > >
> > > --
> > >
> > > Is this change appropriate to be pushed upstream? Where to go from
> > > here?
> >
> > This issue is at the USB core. So, it should be reported to the
> > linux-usb mailing list.
> >
> > The people there should help about how to proceed to get this
> > fixed upstream.
>
> Here's a proper version of that patch. If this is okay, it can be
> merged.
>
> Alan Stern
>
>
>
> Index: usb-4.x/drivers/usb/core/config.c
> ===
> --- usb-4.x.orig/drivers/usb/core/config.c
> +++ usb-4.x/drivers/usb/core/config.c
> @@ -213,8 +213,10 @@ static int usb_parse_endpoint(struct dev
> memcpy(>desc, d, n);
> INIT_LIST_HEAD(>urb_list);
>
> - /* Fix up bInterval values outside the legal range. Use 32 ms if no
> - * proper value can be guessed. */
> + /*
> + * Fix up bInterval values outside the legal range.
> + * Use 10 or 8 ms if no proper value can be guessed.
> + */
> i = 0; /* i = min, j = max, n = default */
> j = 255;
> if (usb_endpoint_xfer_int(d)) {
> @@ -223,13 +225,15 @@ static int usb_parse_endpoint(struct dev
> case USB_SPEED_SUPER_PLUS:
> case USB_SPEED_SUPER:
> case USB_SPEED_HIGH:
> - /* Many device manufacturers are using full-speed
> + /*
> + * Many device manufacturers are using full-speed
>* bInterval values in high-speed interrupt endpoint
>* descriptors. Try to fix those and fall back to a
> - * 32 ms default value otherwise. */
> + * 8 ms default value otherwise.
> + */
> n = fls(d->bInterval*8);
> if (n == 0)
> - n = 9; /* 32 ms = 2^(9-1) uframes */
> + n = 7; /* 8 ms = 2^(7-1) uframes */
> j = 16;
>
> /*
> @@ -247,7 +251,7 @@ static int usb_parse_endpoint(struct dev
> /* For low-speed, 10 ms is the official minimum.
>* But some "overclocked" devices might want faster
>* polling so we'll allow it. */
> - n = 32;
> + n = 10;
> break;
> }
> } else if (usb_endpoint_xfer_isoc(d)) {
> @@ -255,10 +259,10 @@ static int usb_parse_endpoint(struct dev
> j = 16;
> switch (to_usb_device(ddev)->speed) {
> case USB_SPEED_HIGH:
> - n = 9; /* 32 ms = 2^(9-1) uframes */
> + n = 7; /* 8 ms = 2^(7-1) uframes */
> break;
> default:/* USB_SPEED_FULL */
> - n = 6; /* 32 ms = 2^(6-1) frames */
> + n = 4; /* 8 ms = 2^(4-1) frames */
> break;
> }
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mceusb xhci issue?
On Sat, 9 Jul 2016, Mauro Carvalho Chehab wrote:
> C/C linux-usb Mailing list:
>
>
> Em Wed, 18 May 2016 08:52:28 -0600
> Wade Berrier <wberr...@gmail.com> escreveu:
...
> > > That message above links to some other threads describing the issue.
> > > Here's a post with a patch that supposedly works:
> > >
> > > http://www.gossamer-threads.com/lists/mythtv/users/587930
> > >
> > > No idea if that's the "correct" way to fix this.
> > >
> > > I'll be trying that out and then report back...
> >
> > Indeed, this patch does fix the issue:
> >
> > --
> >
> > diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> > index 31ccdcc..03321d4 100644
> > --- a/drivers/usb/core/config.c
> > +++ b/drivers/usb/core/config.c
> > @@ -247,7 +247,7 @@ static int usb_parse_endpoint(struct device *ddev, int
> > cfgno, int inum,
> > /* For low-speed, 10 ms is the official minimum.
> > * But some "overclocked" devices might want faster
> > * polling so we'll allow it. */
> > - n = 32;
> > + n = 10;
> > break;
> > }
> > } else if (usb_endpoint_xfer_isoc(d)) {
> >
> >
> > --
> >
> > Is this change appropriate to be pushed upstream? Where to go from
> > here?
>
> This issue is at the USB core. So, it should be reported to the
> linux-usb mailing list.
>
> The people there should help about how to proceed to get this
> fixed upstream.
Here's a proper version of that patch. If this is okay, it can be
merged.
Alan Stern
Index: usb-4.x/drivers/usb/core/config.c
===
--- usb-4.x.orig/drivers/usb/core/config.c
+++ usb-4.x/drivers/usb/core/config.c
@@ -213,8 +213,10 @@ static int usb_parse_endpoint(struct dev
memcpy(>desc, d, n);
INIT_LIST_HEAD(>urb_list);
- /* Fix up bInterval values outside the legal range. Use 32 ms if no
-* proper value can be guessed. */
+ /*
+* Fix up bInterval values outside the legal range.
+* Use 10 or 8 ms if no proper value can be guessed.
+*/
i = 0; /* i = min, j = max, n = default */
j = 255;
if (usb_endpoint_xfer_int(d)) {
@@ -223,13 +225,15 @@ static int usb_parse_endpoint(struct dev
case USB_SPEED_SUPER_PLUS:
case USB_SPEED_SUPER:
case USB_SPEED_HIGH:
- /* Many device manufacturers are using full-speed
+ /*
+* Many device manufacturers are using full-speed
* bInterval values in high-speed interrupt endpoint
* descriptors. Try to fix those and fall back to a
-* 32 ms default value otherwise. */
+* 8 ms default value otherwise.
+*/
n = fls(d->bInterval*8);
if (n == 0)
- n = 9; /* 32 ms = 2^(9-1) uframes */
+ n = 7; /* 8 ms = 2^(7-1) uframes */
j = 16;
/*
@@ -247,7 +251,7 @@ static int usb_parse_endpoint(struct dev
/* For low-speed, 10 ms is the official minimum.
* But some "overclocked" devices might want faster
* polling so we'll allow it. */
- n = 32;
+ n = 10;
break;
}
} else if (usb_endpoint_xfer_isoc(d)) {
@@ -255,10 +259,10 @@ static int usb_parse_endpoint(struct dev
j = 16;
switch (to_usb_device(ddev)->speed) {
case USB_SPEED_HIGH:
- n = 9; /* 32 ms = 2^(9-1) uframes */
+ n = 7; /* 8 ms = 2^(7-1) uframes */
break;
default:/* USB_SPEED_FULL */
- n = 6; /* 32 ms = 2^(6-1) frames */
+ n = 4; /* 8 ms = 2^(4-1) frames */
break;
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue
On Wed, 9 Sep 2015, Laurent Pinchart wrote: > On Wednesday 09 September 2015 10:30:12 Hans de Goede wrote: > > On 08-09-15 16:36, Alan Stern wrote: > > > On Tue, 8 Sep 2015, Hans de Goede wrote: > > >> On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote: > > >>> urb completion callback is executed in host controllers interrupt > > >>> context. To keep preempt disable time short, add urbs to a list on > > >>> completion and schedule work to process the list. > > >>> > > >>> Moreover, save timestamp and sof number in the urb completion callback > > >>> to avoid any delays. > > >> > > >> Erm, I thought that we had already moved to using threaded interrupt > > >> handling for the urb completion a while (1-2 years ?) back. Is this then > > >> still needed ? > > > > > > We moved to handling URB completions in a tasklet, not a threaded > > > handler. > > > > Right. > > > > > (Similar idea, though.) And the change was made in only one > > > or two HCDs, not in all of them. > > > > Ah, I was under the impression this was a core change, not a per > > hcd change. > > Instead of fixing the issue in the uvcvideo driver, would it then make more > sense to fix it in the remaining hcd drivers ? Unfortunately that's not so easy. It involves some subtle changes related to the way isochronous endpoints are handled. I wouldn't know what to change in any of the HCDs, except the ones that I maintain. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue
On Wed, 9 Sep 2015, Laurent Pinchart wrote: > > > Instead of fixing the issue in the uvcvideo driver, would it then make > > > more sense to fix it in the remaining hcd drivers ? > > > > Unfortunately that's not so easy. It involves some subtle changes > > related to the way isochronous endpoints are handled. I wouldn't know > > what to change in any of the HCDs, except the ones that I maintain. > > I'm not saying it would be easy, but I'm wondering whether it makes change to > move individual USB device drivers to work queues when the long term goal is > to use tasklets for URB completion anyway. I'm not sure that this is a long-term goal for every HCD. For instance, there probably isn't much incentive to convert a driver if its host controllers can only run at low speed or full speed. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue
On Wed, 9 Sep 2015, Hans de Goede wrote: > Hi, > > On 08-09-15 16:36, Alan Stern wrote: > > On Tue, 8 Sep 2015, Hans de Goede wrote: > > > >> Hi, > >> > >> On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote: > >>> urb completion callback is executed in host controllers interrupt > >>> context. To keep preempt disable time short, add urbs to a list on > >>> completion and schedule work to process the list. > >>> > >>> Moreover, save timestamp and sof number in the urb completion callback > >>> to avoid any delays. > >> > >> Erm, I thought that we had already moved to using threaded interrupt > >> handling for the urb completion a while (1-2 years ?) back. Is this then > >> still needed ? > > > > We moved to handling URB completions in a tasklet, not a threaded > > handler. > > Right. > > > (Similar idea, though.) And the change was made in only one > > or two HCDs, not in all of them. > > Ah, I was under the impression this was a core change, not a per > hcd change. In fact, both the core and the HCD needed to be changed. For example, see commits 94dfd7edfd5c (core) and 9118f9eb4f1e (ehci-hcd). (There was more to it than just those two commits, of course.) In one respect the change still isn't complete: Since the completion callback occurs in a tasklet, we should allow interrupts to remain enabled while the callback runs. But some class drivers still expect interrupts to be disabled at that time, so the core has to disable interrupts before invoking the callback and then re-enable them afterward. There was a proposal to fix up a number of drivers so that we could leave interrupts enabled the whole time. But I don't think it ever got merged. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] media: uvcvideo: handle urb completion in a work queue
On Tue, 8 Sep 2015, Hans de Goede wrote: > Hi, > > On 09/07/2015 06:23 PM, Mian Yousaf Kaukab wrote: > > urb completion callback is executed in host controllers interrupt > > context. To keep preempt disable time short, add urbs to a list on > > completion and schedule work to process the list. > > > > Moreover, save timestamp and sof number in the urb completion callback > > to avoid any delays. > > Erm, I thought that we had already moved to using threaded interrupt > handling for the urb completion a while (1-2 years ?) back. Is this then > still needed ? We moved to handling URB completions in a tasklet, not a threaded handler. (Similar idea, though.) And the change was made in only one or two HCDs, not in all of them. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 0/4] add devm_of_phy_get_by_index and update platform drivers
On Wed, 22 Apr 2015, Arun Ramamurthy wrote: This patch set adds a new API to get phy by index when multiple phys are present. This patch is based on discussion with Arnd Bergmann about dt bindings for multiple phys. History: v1: - Removed null pointers on Dmitry's suggestion - Improved documentation in commit messages - Exported new phy api v2: - EHCI and OHCI platform Kconfigs select Generic Phy to fix build errors in certain configs. v3: - Made GENERIC_PHY an invisible option so that other configs can select it - Added stubs for devm_of_phy_get_by_index - Reformated code Arun Ramamurthy (4): phy: phy-core: Make GENERIC_PHY an invisible option phy: core: Add devm_of_phy_get_by_index to phy-core usb: ehci-platform: Use devm_of_phy_get_by_index usb: ohci-platform: Use devm_of_phy_get_by_index For patches 3 and 4: Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] [media] uvcvideo: Remain runtime-suspended at sleeps
On Fri, 17 Apr 2015, Tomeu Vizoso wrote: When the system goes to sleep and afterwards resumes, a significant amount of time is spent suspending and resuming devices that were already runtime-suspended. By setting the power.force_direct_complete flag, the PM core will ignore the state of descendant devices and the device will be let in runtime-suspend. Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com --- drivers/media/usb/uvc/uvc_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 5970dd6..ae75a70 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1945,6 +1945,8 @@ static int uvc_probe(struct usb_interface *intf, supported.\n, ret); } + intf-dev.parent-power.force_direct_complete = true; This seems wrong. The uvc driver is bound to intf, not to intf's parent. So it would be okay for the driver to set intf-dev.power.force_direct_complete, but it's wrong to set intf-dev.parent-power.force_direct_complete. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: HP EC372S (Yuan DVB ExpressCard) crash in 3.18.3
On Mon, 2 Feb 2015, Jonas Jonsson wrote: Hi, I posted a bug on kernel.org (https://bugzilla.kernel.org/show_bug.cgi?id=92301 ) and was asked to sent it to this mail-address. Since this bug involves the dvb-usb driver, it should also be posted to the linux-media mailing list (CC-ed). Alan Stern Jan 29 21:26:51 plattpcn kernel: [ 17.322493] input: UVC Camera (05ca:1812) as /devices/pci:00/:00:1d.7/usb2/2-4/2-4:1.0/input/input10 Jan 29 21:26:51 plattpcn kernel: [ 17.322621] usbcore: registered new interface driver uvcvideo Jan 29 21:26:51 plattpcn kernel: [ 17.322623] USB Video Class driver (1.1.1) Jan 29 21:26:51 plattpcn kernel: [ 17.583002] input: HP WMI hotkeys as /devices/virtual/input/input11 Jan 29 21:26:51 plattpcn kernel: [ 18.108106] iwl4965 :02:00.0: loaded firmware version 228.61.2.24 Jan 29 21:26:51 plattpcn kernel: [ 18.360154] ieee80211 phy0: Selected rate control algorithm 'iwl-4965-rs' Jan 29 21:26:51 plattpcn kernel: [ 18.620404] dvb-usb: found a 'Yuan EC372S' in cold state, will try to load a firmware Jan 29 21:26:51 plattpcn kernel: [ 18.993039] dvb-usb: downloading firmware from file 'dvb-usb-dib0700-1.20.fw' Jan 29 21:26:51 plattpcn kernel: [ 19.194634] dib0700: firmware started successfully. Jan 29 21:26:51 plattpcn kernel: [ 19.695174] dvb-usb: found a 'Yuan EC372S' in warm state. Jan 29 21:26:51 plattpcn kernel: [ 19.695448] dvb-usb: will pass the complete MPEG2 transport stream to the software demuxer. Jan 29 21:26:51 plattpcn kernel: [ 19.695527] DVB: registering new adapter (Yuan EC372S) Jan 29 21:26:51 plattpcn kernel: [ 20.090809] BUG: unable to handle kernel NULL pointer dereference at 0080 Jan 29 21:26:51 plattpcn kernel: [ 20.090987] IP: [a057b061] dib7000p_attach+0x11/0xa0 [dib7000p] Jan 29 21:26:51 plattpcn kernel: [ 20.091007] PGD 36893067 PUD b95b0067 PMD 0 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] Oops: 0002 [#1] SMP Jan 29 21:26:51 plattpcn kernel: [ 20.091007] Modules linked in: dib7000p(E) dvb_usb_dib0700(E+) dib7000m(E) arc4(E) dib0090(E) dib0070(E) dib3000mc(E) dibx000_common(E) dvb_usb(E) dvb_core(E) coretemp(E) hp_wmi(E) rc_core(E) sparse_keymap(E) uvcvideo(E) iwl4965(E) videobuf2_vmalloc(E) snd_hda_codec_si3054(E) kvm(E) iwlegacy(E) snd_hda_codec_realtek(E) videobuf2_memops(E) mac80211(E) videobuf2_core(E) snd_hda_codec_generic(E) v4l2_common(E) videodev(E) snd_hda_intel(E) joydev(E) snd_hda_controller(E) serio_raw(E) snd_hda_codec(E) snd_hwdep(E) r852(E) cfg80211(E) snd_pcm(E) sm_common(E) btusb(E) nand(E) snd_seq_midi(E) nand_ecc(E) snd_seq_midi_event(E) bluetooth(E) snd_rawmidi(E) nand_bch(E) snd_seq(E) bch(E) r592(E) snd_seq_device(E) nand_ids(E) snd_timer(E) mtd(E) memstick(E) drm(E) snd(E) soundcore(E) lpc_ich(E) wmi(E) video(E) mac_hid(E) parport_pc(E) ppdev(E) lp(E) parport(E) psmouse(E) ahci(E) libahci(E) firewire_ohci(E) firewire_core(E) sdhci_pci(E) crc_itu_t(E) sdhci(E) r8169(E) mii(E) Jan 29 21:26:51 plattpcn kernel: [ 20.091007] CPU: 0 PID: 442 Comm: systemd-udevd Tainted: GE 3.18.3jonas #1 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] Hardware name: Hewlett-Packard HP Pavilion dv9700 Notebook PC/30CB, BIOS F.59 11/25/2008 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] task: 8800b8f68000 ti: 8800b9148000 task.ti: 8800b9148000 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] RIP: 0010:[a057b061] [a057b061] dib7000p_attach+0x11/0xa0 [dib7000p] Jan 29 21:26:51 plattpcn kernel: [ 20.091007] RSP: 0018:8800b914ba88 EFLAGS: 00010202 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] RAX: 0010 RBX: 8800ba9d1278 RCX: a0581040 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] RDX: a0581040 RSI: a0581c2b RDI: 0010 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] RBP: 8800b914ba88 R08: 810e47a0 R09: 0001802a0029 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] R10: ea0002ed9fc0 R11: 8107cf84 R12: Jan 29 21:26:51 plattpcn kernel: [ 20.091007] R13: 0010 R14: 8800ba9d1278 R15: 8800ba9d1398 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] FS: 7fd441492880() GS:88013fc0() knlGS: Jan 29 21:26:51 plattpcn kernel: [ 20.091007] CS: 0010 DS: ES: CR0: 8005003b Jan 29 21:26:51 plattpcn kernel: [ 20.091007] CR2: 0080 CR3: 36892000 CR4: 07f0 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] Stack: Jan 29 21:26:51 plattpcn kernel: [ 20.091007] 8800b914bab8 a055adab 8800ba9d1278 8800ba9d1278 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] 8800ba9d1278 8800b914baf8 a04776b8 Jan 29 21:26:51 plattpcn kernel: [ 20.091007] 8800ba9d
Re: [PATCH] media: stk1160: Avoid stack-allocated buffer for control URBs
On Mon, 14 Apr 2014, Ezequiel Garcia wrote: On Apr 14, Alan Stern wrote: On Mon, 14 Apr 2014, Ezequiel Garcia wrote: Currently stk1160_read_reg() uses a stack-allocated char to get the read control value. This is wrong because usb_control_msg() requires a kmalloc-ed buffer, and a DMA-API warning is produced: WARNING: CPU: 0 PID: 1376 at lib/dma-debug.c:1153 check_for_stack+0xa0/0x100() ehci-pci :00:0a.0: DMA-API: device driver maps memory fromstack [addr=88003d0b56bf] This commit fixes such issue by using a 'usb_ctrl_read' field embedded in the device's struct to pass the value. In addition, we introduce a mutex to protect the value. This isn't right either. The buffer must be allocated in its own cache line; it must not be part of a larger structure. In that case, we can simply allocate 1 byte using kmalloc(). We won't be needing the mutex and it'll ensure proper cache alignment, right? Yes, that will work fine. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: stk1160: Avoid stack-allocated buffer for control URBs
On Mon, 14 Apr 2014, Ezequiel Garcia wrote: Currently stk1160_read_reg() uses a stack-allocated char to get the read control value. This is wrong because usb_control_msg() requires a kmalloc-ed buffer, and a DMA-API warning is produced: WARNING: CPU: 0 PID: 1376 at lib/dma-debug.c:1153 check_for_stack+0xa0/0x100() ehci-pci :00:0a.0: DMA-API: device driver maps memory fromstack [addr=88003d0b56bf] This commit fixes such issue by using a 'usb_ctrl_read' field embedded in the device's struct to pass the value. In addition, we introduce a mutex to protect the value. This isn't right either. The buffer must be allocated in its own cache line; it must not be part of a larger structure. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stk1160 / ehci-pci 0000:00:0a.0: DMA-API: device driver maps memory fromstack [addr=ffff88003d0b56bf]
On Sun, 13 Apr 2014, Sander Eikelenboom wrote: Hi, I'm hitting this warning on boot with a syntek usb video grabber, it's not clear to me if it's a driver issue of the stk1160 or a generic ehci issue. It is a bug in the stk1160 driver. This is with a 3.15-mergewindow kernel (pull of Linus his tree of today). -- Sander [5.587759] [ cut here ] [5.591210] WARNING: CPU: 0 PID: 1376 at lib/dma-debug.c:1153 check_for_stack+0xa0/0x100() [5.596612] ehci-pci :00:0a.0: DMA-API: device driver maps memory fromstack [addr=88003d0b56bf] [5.602548] Modules linked in: [5.605380] CPU: 0 PID: 1376 Comm: khubd Not tainted 3.14.0-security-20140413-v4lall+ #1 [5.611042] Hardware name: Xen HVM domU, BIOS 4.5-unstable 04/10/2014 [5.615314] 0009 88003d0b5348 81c516fe 88003d0b5390 [5.622147] 88003d0b5380 810e4aa3 88003ceae898 88003cf14070 [5.628926] 88003d0b56bf 88003ceae898 8241ab40 88003d0b53e0 [5.635536] Call Trace: [5.638001] [81c516fe] dump_stack+0x45/0x56 [5.641633] [810e4aa3] warn_slowpath_common+0x73/0x90 [5.645688] [810e4b07] warn_slowpath_fmt+0x47/0x50 [5.649843] [81468820] check_for_stack+0xa0/0x100 [5.652420] systemd-udevd[2143]: starting version 204 [5.657709] [814699fc] debug_dma_map_page+0xfc/0x150 [5.661836] [8172109c] usb_hcd_map_urb_for_dma+0x5fc/0x710 [5.666132] [817214c5] usb_hcd_submit_urb+0x315/0xa30 [5.670247] [810ef85a] ? del_timer_sync+0x4a/0x60 [5.674072] [81c6291d] ? schedule_timeout+0x14d/0x1f0 [5.678083] [810ef2a0] ? migrate_timer_list+0x60/0x60 [5.682167] [81722fac] usb_submit_urb+0x30c/0x580 [5.685989] [81c63f4b] ? wait_for_common+0x16b/0x240 [5.689919] [817236fa] usb_start_wait_urb+0x5a/0xe0 [5.693830] [811b] ? mpol_rebind_policy+0x30/0xa0 [5.697638] [81723838] usb_control_msg+0xb8/0x100 [5.701468] [81984ab2] stk1160_read_reg+0x52/0x80 [5.705358] [8198690c] stk1160_i2c_busy_wait+0x6c/0x90 The bug is here. stk1160_i2c_busy_wait() passes the address of a variable on the stack to stk1160_read_reg(), and that routine passes the address to usb_control_msg(). But usb_control_msg() requires the buffer to be allocated by kmalloc, not on the stack. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: Isochronous transfer error on USB3
On Wed, 8 Jan 2014, Mauro Carvalho Chehab wrote:
Hi Hans/Takashi,
I'm getting an weird behavior with em28xx, especially when the device
is connected into an audio port.
I'm using, on my tests, an em28xx HVR-950 device, using this tree:
http://git.linuxtv.org/mchehab/experimental.git/shortlog/refs/heads/em28xx-v4l2-v6
Where the alsa driver is at:
http://git.linuxtv.org/mchehab/experimental.git/blob/refs/heads/em28xx-v4l2-v6:/drivers/media/usb/em28xx/em28xx-audio.c
I'm testing it with xawtv3 (http://git.linuxtv.org/xawtv3.git). The
ALSA userspace code there is at:
http://git.linuxtv.org/xawtv3.git/blob/HEAD:/common/alsa_stream.c
What happens is that, when I require xawtv3 to use any latency lower
than 65 ms, the audio doesn't work, as it gets lots of underruns per
second.
FYI, em28xx works at a 48000 KHz sampling rate, and its PM capture Hw
is described as:
static struct snd_pcm_hardware snd_em28xx_hw_capture = {
.info = SNDRV_PCM_INFO_BLOCK_TRANSFER |
SNDRV_PCM_INFO_MMAP |
SNDRV_PCM_INFO_INTERLEAVED|
SNDRV_PCM_INFO_BATCH |
SNDRV_PCM_INFO_MMAP_VALID,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
.rates = SNDRV_PCM_RATE_CONTINUOUS | SNDRV_PCM_RATE_KNOT,
.rate_min = 48000,
.rate_max = 48000,
.channels_min = 2,
.channels_max = 2,
.buffer_bytes_max = 62720 * 8, /* just about the value in usbaudio.c */
.period_bytes_min = 64, /* 12544/2, */
.period_bytes_max = 12544,
.periods_min = 2,
.periods_max = 98, /* 12544, */
};
On my tests, I experimentally discovered that the minimal latency to
avoid ALSA library underruns is:
- 65ms when using xHCI;
- 25ms when using EHCI.
Any latency lower than that causes lots of overruns. Very high
latency also causes overruns (but on a lower rate, as the period
is bigger).
I'm wandering if is there anything that could be done either at Kernel
side or at userspace side to automatically get some configuration that
works as-is, without requiring the user to play with the latency parameter
by hand.
The alsa-info data is enclosed.
Thank you!
Mauro
PS.: I'm still trying to understand why the minimal allowed latency is
different when using xHCI, but I suspect that it is because it uses a
different urb-interval than EHCI.
You may be able to answer some of these questions by collecting usbmon
traces (see Documentation/usb/usbmon.txt). That would help pinpoint
sources of latency and tell you the actual URB intervals.
25 ms to avoid underruns seems pretty large. Other people, using audio
only (no video), find that EHCI can work well with latencies as low as
2 ms or so. (That's using 3.13-rc, which includes some changes in the
snd-usb-audio driver.)
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New USB core API to change interval and max packet size
On Tue, 1 Oct 2013, Sarah Sharp wrote: When you say a new API, what do you mean? New functions in usbcore to be used by usb device drivers? Yes. You would export the function in the USB core, and put a prototype in a USB include file (probably in include/linux/usb.h). Let's say that function is called usb_change_ep_bandwidth. Drivers could call into that function when they needed to change either the bInterval or wMaxPacketSize of a particular endpoint. This could be during the driver's probe function, or before switching alternate interface settings, or even after the alt setting is in place, but userspace dictates the driver use a different bandwidth. Drivers should pass usb_change_ep_bandwidth a pointer to the endpoint they need to change, along with the bInterval and wMaxPacketSize values they would like the endpoint to have. Those values could be stored as new values in struct usb_host_endpoint. usb_change_ep_bandwidth would then call into the xHCI driver to drop the endpoint, re-add it, and then issue a bandwidth change. The xHCI driver would have to be changed to look at the new fields in usb_host_endpoint, and set up the endpoint contexts with the interval and packet size from those fields, instead of the endpoint descriptor. We should probably set the new values in usb_host_endpoint to zero after the driver unbinds from the device. Not sure if they should be reset after the driver switches interfaces. I would have to see the use cases in the driver. We should consider this before rushing into a new API. In particular, do we want to go around changing single endpoints, one at a time? Or do we want to change all the endpoints in an interface at once? Given that a change to one endpoint may require the entire schedule to be recomputed, it seems to make more sense to do all of them at once. For example, drivers could call a routine to set the desired endpoint parameters, and then the new parameters could take effect when the driver calls usb_set_interface(). In any case, the question about what to do when the interface setting gets switched never really arises. Each usb_host_endpoint structure is referenced from only one altsetting. If the driver wants the new parameters applied to an endpoint in several altsettings, it will have to change each altsetting separately. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New USB core API to change interval and max packet size
On Wed, 2 Oct 2013, Mauro Carvalho Chehab wrote: Let me see if I understand the changes at the media drivers. So, please correct me if I got it wrong. I'm yet to get any USB 3.0 media device, although it is common to connect an USB 1.1 or USB 2.0 device on a USB 3.0 host port. So, for example, on this device: ... Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes3 Transfer TypeInterrupt Synch Type None Usage Type Data wMaxPacketSize 0x0004 1x 4 bytes bInterval 1 ... connected via this BUS device: ... In such situation, and assuming that the USB tables are correct, there's nothing that needs to be done there, as bInterval/wMaxPacketSize are correct for USB 2.0. So, there's no need to call usb_change_ep_bandwidth(). That's right. If so, then usb_change_ep_bandwidth() as a quirk, if bInterval or wMaxPacketSize were improperly filled. Right? Or if the values are correct, but the driver wants to use something different for its own reasons (for example, to get lower latency or because it knows that it will never use packets as large as the descriptor allows). Right. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New USB core API to change interval and max packet size
On Wed, 2 Oct 2013, Mauro Carvalho Chehab wrote: So, there's no need to call usb_change_ep_bandwidth(). That's right. If so, then usb_change_ep_bandwidth() as a quirk, if bInterval or wMaxPacketSize were improperly filled. Right? Or if the values are correct, but the driver wants to use something different for its own reasons (for example, to get lower latency or because it knows that it will never use packets as large as the descriptor allows). Right. Ok, so, in this case, usb_change_ep_bandwidth() could be called just before usb_alloc_urb(), in order to make it to use the packet size that would be expected for that kind of ISOC traffic that userspace indirectly selected, by adjusting the streaming video resolution selected, right? We haven't decided on the final API yet. However, note that usb_alloc_urb() doesn't depend on the packet size. It requires you to specify only the number of packets, not their sizes. Therefore it doesn't matter whether you call usb_change_ep_bandwidth() before or after usb_alloc_urb(). Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New USB core API to change interval and max packet size
On Wed, 2 Oct 2013, Sarah Sharp wrote: In particular, do we want to go around changing single endpoints, one at a time? Or do we want to change all the endpoints in an interface at once? Given that a change to one endpoint may require the entire schedule to be recomputed, it seems to make more sense to do all of them at once. For example, drivers could call a routine to set the desired endpoint parameters, and then the new parameters could take effect when the driver calls usb_set_interface(). I think it makes sense to change several endpoints, and then ask the host to recompute the schedule. Perhaps the driver could issue several calls to usb_change_ep_bandwidth and then ask the USB core to update the host's schedule? That's what I had in mind. Note that usb_set_interface() already updates the schedule. I'm not sure that usb_set_interface() is the right function for the new parameters to take effect. What if the driver is trying to modify the current alt setting? Would they need to call usb_set_interface() again? Yes. I can see two disadvantages: You don't want to call usb_set_interface() if there are any transfers in progress for endpoints in that interface -- even endpoints whose bandwidth you aren't trying to change. The transfers would get shut down. usb_set_interface() communicates with the device, which isn't necessary if you're merely updating the host's schedule. The alternatives are either a separate function to do the schedule update, or else pass usb_change_ep_bandwidth() an array or list of endpoint info and have it do all the updates at once. I think a separate function would be easier for drivers to use. In any case, the question about what to do when the interface setting gets switched never really arises. Each usb_host_endpoint structure is referenced from only one altsetting. If the driver wants the new parameters applied to an endpoint in several altsettings, it will have to change each altsetting separately. Ok, so it sounds like we want to keep the changes the endpoints across alt setting changes. No, just the opposite. That was the point I was trying to make. Any changes to ep5 in altsetting 0 (for example) will have no effect on ep1 in altsetting 1, because the two altsettings reference the endpoint by two separate usb_host_endpoint structures. Furthermore, it generally doesn't make sense to apply a single change across multiple altsettings. An isochronous endpoint, for example, might have maxpacket = 500 in one altsetting and 900 in another. When you reduce the 500 to 400, that doesn't mean you also want to reduce the 900 to 400. But we still want to reset the values after the driver unbinds, correct? Absolutely. When the next driver binds, it should get the default values (i.e., the ones stored in the descriptors). This would be analogous to the way we currently set each interface back to altsetting 0 when a driver unbinds. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New USB core API to change interval and max packet size
On Wed, 2 Oct 2013, Alan Stern wrote: Ok, so it sounds like we want to keep the changes the endpoints across alt setting changes. No, just the opposite. That was the point I was trying to make. Any changes to ep5 in altsetting 0 (for example) will have no effect on ep1 --^^^ Typo: this should have been ep5 as well. in altsetting 1, because the two altsettings reference the endpoint by two separate usb_host_endpoint structures. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Cannot shutdown power use from built in webcam in thinkpad T530 questions]
On Wed, 18 Sep 2013, Marc MERLIN wrote: Howdy, I have a T530 with a built in webcam that uses the uvcvideo driver. Kernel 3.10.6, but the problem has been there for many kernel versions. From time to time (not always) it shows up at the top of powertop with an unexplained high power use while I'm not using the camera. Powertop says autosuspend is active: Note that tunables says: Good Autosuspend for USB device Integrated Camera [Chicony Electronics Co., Ltd.] Unloading uvcvideo makes no difference. However if I never load uvcvideo and never use the camera after booting the laptop, then the device doesn't use any power. This is better than nothing, but not very desirable since I do use the camera from time to google for video conferencing. Here's the module info gandalfthegreat:/sys/bus/usb/drivers/uvcvideo# l total 0 drwxr-xr-x 2 root root0 Sep 18 17:48 ./ drwxr-xr-x 12 root root0 Sep 17 15:10 ../ lrwxrwxrwx 1 root root0 Sep 18 21:30 3-1.6:1.0 - ../../../../devices/pci:00/:00:1a.0/usb3/3-1/3-1.6/3-1.6:1.0/ lrwxrwxrwx 1 root root0 Sep 18 21:30 3-1.6:1.1 - ../../../../devices/pci:00/:00:1a.0/usb3/3-1/3-1.6/3-1.6:1.1/ --w--- 1 root root 4096 Sep 18 21:30 bind lrwxrwxrwx 1 root root0 Sep 18 21:30 module - ../../../../module/uvcvideo/ -rw-r--r-- 1 root root 4096 Sep 18 21:30 new_id -rw-r--r-- 1 root root 4096 Sep 18 21:30 remove_id --w--- 1 root root 4096 Sep 18 17:48 uevent --w--- 1 root root 4096 Sep 18 21:30 unbind gandalfthegreat:/sys/bus/usb/devices/3-1.6/power# grep . * active_duration:61227648 async:enabled autosuspend:2 autosuspend_delay_ms:2000 connected_duration:66830880 control:auto level:auto persist:1 runtime_active_kids:0 runtime_active_time:18870052 runtime_enabled:enabled runtime_status:active runtime_suspended_time:5324088 runtime_usage:0 This all looks correct. Any ideas? You might get more information from a kernel with CONFIG_USB_DEBUG enabled. Especially if you add #define VERBOSE_DEBUG to drivers/usb/core/driver.c before the first #include line. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Cannot shutdown power use from built in webcam in thinkpad T530 questions]
On Sun, 22 Sep 2013, Marc MERLIN wrote: On Sun, Sep 22, 2013 at 12:38:56PM -0400, Alan Stern wrote: gandalfthegreat:/sys/bus/usb/devices/3-1.6/power# grep . * active_duration:61227648 async:enabled autosuspend:2 autosuspend_delay_ms:2000 connected_duration:66830880 control:auto level:auto persist:1 runtime_active_kids:0 runtime_active_time:18870052 runtime_enabled:enabled runtime_status:active runtime_suspended_time:5324088 runtime_usage:0 This all looks correct. Since then, I've confirmed that I don't have the problem some time after reboot. It may be that the device doesn't seem to sleep well after I've used it once. What's interesting, is that I see this when power is plugged in: Power est. Events/sCategory Description 8.18 W100.0% Device USB device: Yubico Yubikey II (Yubico) 8.13 W100.0% Device USB device: Integrated Camera (Chicony Electronics Co., Ltd.) Somehow I know that my Yubikey isn't using 8W, so powertop numbers need to be taken with a grain of salt. I don't know where powertop gets its numbers from. Perhaps it uses the value reported by the device (bMaxPower). That value is only a maximum; it doesn't change to reflect the actual usage. Once I go to batteries, I see this: Summary: 760.1 wakeups/second, 718.9 GPU ops/seconds, 0.0 VFS ops/sec and 6.9% CPU use Power est. Usage Events/sCategory Description 8.32 W100.0% Device USB device: Yubico Yubikey II (Yubico) 2.52 W 73.3% Device Display backlight So at least for now, the camera does sleep ok, until later when it probably won't again. I'm somehow thinking there is a driver or hardware problem when the device does get stuck in a mode where it won't sleep properly again until the next reboot (just unloading/loading the driver does not fix this). That's quite possible. But if it is a driver problem, wouldn't unloading and reloading the driver fix it? You might get more information from a kernel with CONFIG_USB_DEBUG enabled. Especially if you add #define VERBOSE_DEBUG to drivers/usb/core/driver.c before the first #include line. Do you think thaty would help debug the problem above, or not really? I'm There's no way to know in advance. starting to think that the USB layer is not at fault, although I could be wrong I suppose. You asked for advice on things to try, and I suggested something. That's the best I can do. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: USB host support should depend on HAS_DMA
On Sun, 18 Aug 2013, Geert Uytterhoeven wrote:
On Thu, Jul 11, 2013 at 1:12 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 10 July 2013, Alan Stern wrote:
This isn't right. There are USB host controllers that use PIO, not
DMA. The HAS_DMA dependency should go with the controller driver, not
the USB core.
On the other hand, the USB core does call various routines like
dma_unmap_single. It ought to be possible to compile these calls even
when DMA isn't enabled. That is, they should be defined as do-nothing
stubs.
The asm-generic/dma-mapping-broken.h file intentionally causes link
errors, but that could be changed.
The better approach in my mind would be to replace code like
if (hcd-self.uses_dma)
with
if (IS_ENABLED(CONFIG_HAS_DMA) hcd-self.uses_dma) {
which will reliably cause that reference to be omitted from object code,
but not stop giving link errors for drivers that actually require
DMA.
This can be done for drivers/usb/core/hcd.c.
But I'm a bit puzzled by drivers/usb/core/buffer.c. E.g.
void *hcd_buffer_alloc(...)
{
/* some USB hosts just use PIO */
if (!bus-controller-dma_mask
!(hcd-driver-flags HCD_LOCAL_MEM)) {
I don't remember the full story. You should check with the person who
added the HCD_LOCAL_MEM flag originally.
So if DMA is not used (!hcd-self.uses_dma, i.e. bus-controller-dma_mask
is zero), and HCD_LOCAL_MEM is set, we still end up calling dma_pool_alloc()?
(Naively, I'm not so familiar with the USB code) I'd expect it to use
kmalloc() instead?
Maybe what happens in this case is that some sort of hook causes the
allocation to be made from a special memory-mapped region on board the
controller.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, 23 Jul 2013, Tomasz Figa wrote:
On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
Hi Alan,
Thanks for helping to clarify the issues here.
Okay. Are PHYs _always_ platform devices?
They can be i2c, spi or any other device types as well.
In those other cases, presumably there is no platform data associated
with the PHY since it isn't a platform device. Then how does the
kernel know which controller is attached to the PHY? Is this spelled
out in platform data associated with the PHY's i2c/spi/whatever parent?
PHY. Currently this information is represented by name or
ID
strings embedded in platform data.
right. It's embedded in the platform data of the controller.
It must also be embedded in the PHY's platform data somehow.
Otherwise, how would the kernel know which PHY to use?
By using a PHY lookup as Stephen and I suggested in our previous
replies. Without any extra data in platform data. (I have even posted a
code example.)
I don't understand, because I don't know what a PHY lookup does.
In this case, it doesn't matter where the platform_device structures
are created or where the driver source code is. Let's take a simple
example. Suppose the system design includes a PHY named foo. Then
the board file could contain:
struct phy_info { ... } phy_foo;
EXPORT_SYMBOL_GPL(phy_foo);
and a header file would contain:
extern struct phy_info phy_foo;
The PHY supplier could then call phy_create(phy_foo), and the PHY
client could call phy_find(phy_foo). Or something like that; make up
your own structure tags and function names.
It's still possible to have conflicts, but now two PHYs with the same
name (or a misspelled name somewhere) will cause an error at link
time.
This is incorrect, sorry. First of all it's a layering violation - you
export random driver-specific symbols from one driver to another. Then
No, that's not what I said. Neither the PHY driver nor the controller
driver exports anything to the other. Instead, both drivers use data
exported by the board file.
imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2 and
there are two types of consumer drivers (e.g. USB host controllers). Now
consider following mapping:
SoC PHY consumer
A PHY1HOST1
B PHY1HOST2
C PHY2HOST1
D PHY2HOST2
So we have to be able to use any of the PHYs with any of the host
drivers. This means you would have to export symbol with the same name
from both PHY drivers, which obviously would not work in this case,
because having both drivers enabled (in a multiplatform aware
configuration) would lead to linking conflict.
You're right; the scheme was too simple. Instead, the board file must
export two types of data structures, one for PHYs and one for
controllers. Like this:
struct phy_info {
/* Info for the controller attached to this PHY */
struct controller_info *hinfo;
};
struct controller_info {
/* Info for the PHY which this controller is attached to */
struct phy_info *pinfo;
};
The board file for SoC A would contain:
struct phy_info phy1 = {host1);
EXPORT_SYMBOL(phy1);
struct controller_info host1 = {phy1};
EXPORT_SYMBOL(host1);
The board file for SoC B would contain:
struct phy_info phy1 = {host2);
EXPORT_SYMBOL(phy1);
struct controller_info host2 = {phy1};
EXPORT_SYMBOL(host2);
And so on. This explicitly gives the connection between PHYs and
controllers. The PHY providers would use phy1 or phy2, and the PHY
consumers would use host1 or host2.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, 23 Jul 2013, Tomasz Figa wrote:
IMHO it would be better if you provided some code example, but let's try to
check if I understood you correctly.
8
[Board file]
static struct phy my_phy;
static struct platform_device phy_pdev = {
/* ... */
.platform_data = my_phy;
/* ... */
};
static struct platform_device phy_pdev = {
This should be controller_pdev, not phy_pdev, yes?
/* ... */
.platform_data = my_phy;
/* ... */
};
[Provider driver]
struct phy *phy = pdev-dev.platform_data;
ret = phy_create(phy);
[Consumer driver]
struct phy *phy = pdev-dev.platform_data;
ret = phy_get(pdev-dev, phy);
Or even just phy_get(pdev-dev), because phy_get() could be smart
enough to to set phy = dev-platform_data.
8
Is this what you mean?
That's what I was going to suggest too. The struct phy is defined in
the board file, which already knows about all the PHYs that exist in
the system. (Or perhaps it is allocated dynamically, so that when many
board files are present in the same kernel, only the entries listed in
the board file for the current system get created.) Then the
structure's address is stored in the platform data and made available
to both the provider and the consumer.
Even though the struct phy is defined (or allocated) in the board file,
its contents don't get filled in until the PHY driver provides the
details.
It's technically correct, but quality of this solution isn't really nice,
because it's a layering violation (at least if I understood what you mean).
This is because you need to have full definition of struct phy in board file
and a structure that is used as private data in PHY core comes from
platform code.
You don't have to have a full definition in the board file. Just a
partial definition -- most of the contents can be filled in later, when
the PHY driver is ready to store the private data.
It's not a layering violation for one region of the kernel to store
private data in a structure defined by another part of the kernel.
This happens all the time (e.g., dev_set_drvdata).
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, 23 Jul 2013, Tomasz Figa wrote: That's what I was going to suggest too. The struct phy is defined in the board file, which already knows about all the PHYs that exist in the system. (Or perhaps it is allocated dynamically, so that when many board files are present in the same kernel, only the entries listed in the board file for the current system get created.) Well, such dynamic allocation is a must. We don't accept non-multiplatform aware code anymore, not even saying about multiboard. Then the structure's address is stored in the platform data and made available to both the provider and the consumer. Yes, technically this can work. You would still have to perform some kind of synchronization to make sure that the PHY bound to this structure is actually present. This is again technically doable (e.g. a list of registered struct phys inside PHY core). The synchronization takes place inside phy_get. If phy_create hasn't been called for this structure by the time phy_get runs, phy_get will return an error. Even though the struct phy is defined (or allocated) in the board file, its contents don't get filled in until the PHY driver provides the details. You can't assure this. Board file is free to do whatever it wants with this struct. A clean solution would prevent this. I'm not sure what you mean here. Of course I can't prevent a board file from messing up a data structure. I can't prevent it from causing memory access violations either; in fact, I can't prevent any bugs in other people's code. Besides, why do you say the board file is free to do whatever it wants with the struct phy? Currently the struct phy is created by the PHY provider and the PHY core, right? It's not even mentioned in the board file. It's technically correct, but quality of this solution isn't really nice, because it's a layering violation (at least if I understood what you mean). This is because you need to have full definition of struct phy in board file and a structure that is used as private data in PHY core comes from platform code. You don't have to have a full definition in the board file. Just a partial definition -- most of the contents can be filled in later, when the PHY driver is ready to store the private data. It's not a layering violation for one region of the kernel to store private data in a structure defined by another part of the kernel. This happens all the time (e.g., dev_set_drvdata). Not really. The phy struct is something that _is_ private data of PHY subsystem, not something that can store private data of PHY subsystem (sure it can store private data of particular PHY driver, but that's another story) and only PHY subsystem should have access to its contents. If you want to keep the phy struct completely separate from the board file, there's an easy way to do it. Let's say the board file knows about N different PHYs in the system. Then you define an array of N pointers to phys: struct phy *(phy_address[N]); In the platform data for both PHY j and its controller, store phy_address[j]. The PHY provider passes this cookie to phy_create: cookie = pdev-dev.platform_data; ret = phy_create(phy, cookie); and phy_create simply stores: *cookie = phy. The PHY consumer does much the same the same thing: cookie = pdev-dev.platform_data; phy = phy_get(cookie); phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise. By the way, we need to consider other cases here as well, for example it would be nice to have a single phy_get() function that works for both non- DT and DT cases to make the consumer driver not have to worry whether it's being probed from DT or not. You ought to be able to adapt this scheme to work with DT. Maybe by having multiple phy_address arrays. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, 23 Jul 2013, Tomasz Figa wrote: If you want to keep the phy struct completely separate from the board file, there's an easy way to do it. Let's say the board file knows about N different PHYs in the system. Then you define an array of N pointers to phys: struct phy *(phy_address[N]); In the platform data for both PHY j and its controller, store phy_address[j]. The PHY provider passes this cookie to phy_create: cookie = pdev-dev.platform_data; ret = phy_create(phy, cookie); and phy_create simply stores: *cookie = phy. The PHY consumer does much the same the same thing: cookie = pdev-dev.platform_data; phy = phy_get(cookie); phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise. OK, this can work. Again, just technically, because it's rather ugly. There's no reason the phy_address things have to be arrays. A separate individual pointer for each PHY would work just as well. Where would you want to have those phy_address arrays stored? There are no board files when booting with DT. Not even saying that you don't need to use any hacky schemes like this when you have DT that nicely specifies relations between devices. If everybody agrees DT has a nice scheme for specifying relations between devices, why not use that same scheme in the PHY core? Anyway, board file should not be considered as a method to exchange data between drivers. It should be used only to pass data from it to drivers, not the other way. Ideally all data in a board file should be marked as const and __init and dropped after system initialization. The phy_address things don't have to be defined or allocated in the board file; they could be set up along with the platform data. In any case, this was simply meant to be a suggestion to show that it is relatively easy to do what you need without using name or ID strings. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Mon, 22 Jul 2013, Kishon Vijay Abraham I wrote:
The PHY and the controller it is attached to are both physical
devices.
The connection between them is hardwired by the system
manufacturer and cannot be changed by software.
PHYs are generally described by fixed system-specific board
files or by Device Tree information. Are they ever discovered
dynamically?
No. They are created just like any other platform devices are created.
Okay. Are PHYs _always_ platform devices?
Is the same true for the controllers attached to the PHYs?
If not -- if both a PHY and a controller are discovered
dynamically -- how does the kernel know whether they are
connected to each other?
No differences here. Both PHY and controller will have dt information or hwmod
data using which platform devices will be created.
The kernel needs to know which controller is attached to which
PHY. Currently this information is represented by name or ID
strings embedded in platform data.
right. It's embedded in the platform data of the controller.
It must also be embedded in the PHY's platform data somehow.
Otherwise, how would the kernel know which PHY to use?
The PHY's driver (the supplier) uses the platform data to
construct a platform_device structure that represents the PHY.
Currently the driver assigns static labels (corresponding to the label used in
the platform data of the controller).
Until this is done, the controller's driver (the client) cannot
use the PHY.
right.
Since there is no parent-child relation between the PHY and the
controller, there is no guarantee that the PHY's driver will be
ready when the controller's driver wants to use it. A deferred
probe may be needed.
right.
The issue (or one of the issues) in this discussion is that
Greg does not like the idea of using names or IDs to associate
PHYs with controllers, because they are too prone to
duplications or other errors. Pointers are more reliable.
But pointers to what? Since the only data known to be
available to both the PHY driver and controller driver is the
platform data, the obvious answer is a pointer to platform data
(either for the PHY or for the controller, or maybe both).
hmm.. it's not going to be simple though as the platform device for the PHY
and
controller can be created in entirely different places. e.g., in some cases
the
PHY device is a child of some mfd core device (the device will be created in
drivers/mfd) and the controller driver (usually) is created in board file. I
guess then we have to come up with something to share a pointer in two
different files.
The ability for two different source files to share a pointer to a data
item defined in a third source file has been around since long before
the C language was invented. :-)
In this case, it doesn't matter where the platform_device structures
are created or where the driver source code is. Let's take a simple
example. Suppose the system design includes a PHY named foo. Then
the board file could contain:
struct phy_info { ... } phy_foo;
EXPORT_SYMBOL_GPL(phy_foo);
and a header file would contain:
extern struct phy_info phy_foo;
The PHY supplier could then call phy_create(phy_foo), and the PHY
client could call phy_find(phy_foo). Or something like that; make up
your own structure tags and function names.
It's still possible to have conflicts, but now two PHYs with the same
name (or a misspelled name somewhere) will cause an error at link time.
Probably some of the details above are wrong; please indicate where I
have gone astray. Also, I'm not clear about the role played by various
APIs. For example, where does phy_create() fit into this picture?
phy_create is the API by which the PHY's driver (the supplier) hook into the
PHY framework. It's like the controller driver will always interact with the
PHY driver through the PHY framework.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Sun, 21 Jul 2013, Sylwester Nawrocki wrote: What's wrong with the platform_data structure, why can't that be used for this? At the point the platform data of some driver is initialized, e.g. in board setup code the PHY pointer is not known, since the PHY supplier driver has not initialized yet. Even though we wanted to pass pointer to a PHY through some notifier call, it would have been not clear which PHY user driver should match on such notifier. A single PHY supplier driver can create M PHY objects and this needs to be mapped to N PHY user drivers. This mapping needs to be defined somewhere by the system integrator. It works well with device tree, but except that there seems to be no other reliable infrastructure in the kernel to define links/dependencies between devices, since device identifiers are not known in advance in all cases. What Tomasz proposed seems currently most reasonable to me for non-dt. Or, if not, we can always add pointers to the platform device structure, or even the main 'struct device' as well, that's what it is there for. Still we would need to solve a problem which platform device structure gets which PHY pointer. Can you explain the issues in more detail? I don't fully understand the situation. Here's what I think I know: The PHY and the controller it is attached to are both physical devices. The connection between them is hardwired by the system manufacturer and cannot be changed by software. PHYs are generally described by fixed system-specific board files or by Device Tree information. Are they ever discovered dynamically? Is the same true for the controllers attached to the PHYs? If not -- if both a PHY and a controller are discovered dynamically -- how does the kernel know whether they are connected to each other? The kernel needs to know which controller is attached to which PHY. Currently this information is represented by name or ID strings embedded in platform data. The PHY's driver (the supplier) uses the platform data to construct a platform_device structure that represents the PHY. Until this is done, the controller's driver (the client) cannot use the PHY. Since there is no parent-child relation between the PHY and the controller, there is no guarantee that the PHY's driver will be ready when the controller's driver wants to use it. A deferred probe may be needed. The issue (or one of the issues) in this discussion is that Greg does not like the idea of using names or IDs to associate PHYs with controllers, because they are too prone to duplications or other errors. Pointers are more reliable. But pointers to what? Since the only data known to be available to both the PHY driver and controller driver is the platform data, the obvious answer is a pointer to platform data (either for the PHY or for the controller, or maybe both). Probably some of the details above are wrong; please indicate where I have gone astray. Also, I'm not clear about the role played by various APIs. For example, where does phy_create() fit into this picture? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Sat, 20 Jul 2013, Greg KH wrote: That should be passed using platform data. Ick, don't pass strings around, pass pointers. If you have platform data you can get to, then put the pointer there, don't use a name. I don't think I understood you here :-s We wont have phy pointer when we create the device for the controller no?(it'll be done in board file). Probably I'm missing something. Why will you not have that pointer? You can't rely on the name as the device id will not match up, so you should be able to rely on the pointer being in the structure that the board sets up, right? Don't use names, especially as ids can, and will, change, that is going to cause big problems. Use pointers, this is C, we are supposed to be doing that :) Kishon, I think what Greg means is this: The name you are using must be stored somewhere in a data structure constructed by the board file, right? Or at least, associated with some data structure somehow. Otherwise the platform code wouldn't know which PHY hardware corresponded to a particular name. Greg's suggestion is that you store the address of that data structure in the platform data instead of storing the name string. Have the consumer pass the data structure's address when it calls phy_create, instead of passing the name. Then you don't have to worry about two PHYs accidentally ending up with the same name or any other similar problems. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: USB host support should depend on HAS_DMA
On Thu, 11 Jul 2013, Geert Uytterhoeven wrote:
On Thu, Jul 11, 2013 at 3:01 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Thu, 11 Jul 2013, Arnd Bergmann wrote:
On Wednesday 10 July 2013, Alan Stern wrote:
This isn't right. There are USB host controllers that use PIO, not
DMA. The HAS_DMA dependency should go with the controller driver, not
the USB core.
On the other hand, the USB core does call various routines like
dma_unmap_single. It ought to be possible to compile these calls even
when DMA isn't enabled. That is, they should be defined as do-nothing
stubs.
The asm-generic/dma-mapping-broken.h file intentionally causes link
errors, but that could be changed.
The better approach in my mind would be to replace code like
if (hcd-self.uses_dma)
with
if (IS_ENABLED(CONFIG_HAS_DMA) hcd-self.uses_dma) {
which will reliably cause that reference to be omitted from object code,
but not stop giving link errors for drivers that actually require
DMA.
How will it give link errors for drivers that require DMA?
It won't. Unless the host driver itself calls into the DMA API, too
(are there any that don't?).
To my knowledge, all the host controller drivers which use DMA _do_
call functions in the DMA API. So they would still get link errors,
even though the USB core wouldn't.
Therefore adding the appropriate HAS_DMA dependencies should be
straightforward: Try to build all the drivers and see which ones fail
to link.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: USB host support should depend on HAS_DMA
On Wed, 10 Jul 2013, Geert Uytterhoeven wrote: If NO_DMA=y: drivers/built-in.o: In function `usb_hcd_unmap_urb_setup_for_dma': drivers/usb/core/hcd.c:1361: undefined reference to `dma_unmap_single' ,,, Commit d9ea21a779278da06d0cbe989594bf542ed213d7 (usb: host: make USB_ARCH_HAS_?HCI obsolete) allowed to enable USB on platforms with NO_DMA=y, and exposed several input and media USB drivers that just select USB if USB_ARCH_HAS_HCD, without checking HAS_DMA. Fix the former by making USB depend on HAS_DMA. This isn't right. There are USB host controllers that use PIO, not DMA. The HAS_DMA dependency should go with the controller driver, not the USB core. On the other hand, the USB core does call various routines like dma_unmap_single. It ought to be possible to compile these calls even when DMA isn't enabled. That is, they should be defined as do-nothing stubs. To fix the latter, instead of adding lots of depends on HAS_DMA, make those drivers depend on USB, instead of depending on USB_ARCH_HAS_HCD and selecting USB. Drivers for other busses (e.g. MOUSE_SYNAPTICS_I2C) already handle this in a similar way. That seems reasonable. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: USB host support should depend on HAS_DMA
On Thu, 11 Jul 2013, Arnd Bergmann wrote:
On Wednesday 10 July 2013, Alan Stern wrote:
This isn't right. There are USB host controllers that use PIO, not
DMA. The HAS_DMA dependency should go with the controller driver, not
the USB core.
On the other hand, the USB core does call various routines like
dma_unmap_single. It ought to be possible to compile these calls even
when DMA isn't enabled. That is, they should be defined as do-nothing
stubs.
The asm-generic/dma-mapping-broken.h file intentionally causes link
errors, but that could be changed.
The better approach in my mind would be to replace code like
if (hcd-self.uses_dma)
with
if (IS_ENABLED(CONFIG_HAS_DMA) hcd-self.uses_dma) {
which will reliably cause that reference to be omitted from object code,
but not stop giving link errors for drivers that actually require
DMA.
How will it give link errors for drivers that require DMA?
Besides, wouldn't it be better to get an error at config time rather
than at link time? Or even better still, not to be allowed to
configure drivers that depend on DMA if DMA isn't available?
If we add an explicit dependency for HAS_DMA to the Kconfig entries for
these drivers, then your suggestion would be a good way to allow
usbcore to be built independently of DMA support.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] uvcvideo: Disable USB autosuspend for Creative Live! Cam Optia AF
On Thu, 28 Mar 2013, Laurent Pinchart wrote: The camera fails to start video streaming after having been autosuspend. Add a new quirk to selectively disable autosuspend for devices that don't support it. Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com --- drivers/media/usb/uvc/uvc_driver.c | 14 +- drivers/media/usb/uvc/uvcvideo.h | 1 + 2 files changed, 14 insertions(+), 1 deletion(-) I've tried to set the reset resume quirk for this device in the USB core but the camera still failed to start video streaming after having been autosuspended. Regardless of whether the reset resume quirk was set, it would respond to control messages but wouldn't send video data. Presumably the camera won't work after a system suspend, either. This solution below is a hack, but I'm not sure what else I can try. Crazy ideas are welcome. It's not a hack; it's a normal use for a quirk flag. Of course, if you can figure out what's wrong with the camera and see how to fix it, that would be best. How does the camera perform on a Windows system after being put to sleep and then woken up? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] saa7134: Add pm_qos_request to fix video corruption
On Mon, 29 Oct 2012, Simon Farnsworth wrote: On Monday 29 October 2012 13:44:45 Mauro Carvalho Chehab wrote: Thanks for digging into it and getting more data. Do you know if this change it also needed with USB devices that do DMA (isoc and/or bulk)? Or the USB core already handles that? I'm not a huge expert - the linux-pm list (cc'd) will have people around who know more. If I've understood correctly, though, the USB core should take care of pm_qos requests if they're needed for the hardware; remember that if the HCD has enough buffering, there's no need for a pm_qos request. The USB core is not PM-QOS aware. It relies on the PM core to tell it when devices may safely be runtime-suspended. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: Re: Re: Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website
On Sun, 21 Oct 2012, Artem S. Tashkinov wrote:
dmesg messages up to a crash can be seen here:
https://bugzilla.kernel.org/attachment.cgi?id=84221
The first problem in the log is endpoint list corruption. Here's a
debugging patch which should provide a little more information.
Alan Stern
drivers/usb/core/hcd.c | 36
1 file changed, 36 insertions(+)
Index: usb-3.6/drivers/usb/core/hcd.c
===
--- usb-3.6.orig/drivers/usb/core/hcd.c
+++ usb-3.6/drivers/usb/core/hcd.c
@@ -1083,6 +1083,8 @@ EXPORT_SYMBOL_GPL(usb_calc_bus_time);
/*-*/
+static bool list_error;
+
/**
* usb_hcd_link_urb_to_ep - add an URB to its endpoint queue
* @hcd: host controller to which @urb was submitted
@@ -1126,6 +1128,20 @@ int usb_hcd_link_urb_to_ep(struct usb_hc
*/
if (HCD_RH_RUNNING(hcd)) {
urb-unlinked = 0;
+
+ {
+ struct list_head *cur = urb-ep-urb_list;
+ struct list_head *prev = cur-prev;
+
+ if (prev-next != cur !list_error) {
+ list_error = true;
+ dev_err(urb-dev-dev,
+ ep %x list add corruption: %p %p %p\n,
+ urb-ep-desc.bEndpointAddress,
+ cur, prev, prev-next);
+ }
+ }
+
list_add_tail(urb-urb_list, urb-ep-urb_list);
} else {
rc = -ESHUTDOWN;
@@ -1193,6 +1209,26 @@ void usb_hcd_unlink_urb_from_ep(struct u
{
/* clear all state linking urb to this dev (and hcd) */
spin_lock(hcd_urb_list_lock);
+ {
+ struct list_head *cur = urb-urb_list;
+ struct list_head *prev = cur-prev;
+ struct list_head *next = cur-next;
+
+ if (prev-next != cur !list_error) {
+ list_error = true;
+ dev_err(urb-dev-dev,
+ ep %x list del corruption prev: %p %p %p\n,
+ urb-ep-desc.bEndpointAddress,
+ cur, prev, prev-next);
+ }
+ if (next-prev != cur !list_error) {
+ list_error = true;
+ dev_err(urb-dev-dev,
+ ep %x list del corruption next: %p %p %p\n,
+ urb-ep-desc.bEndpointAddress,
+ cur, next, next-prev);
+ }
+ }
list_del_init(urb-urb_list);
spin_unlock(hcd_urb_list_lock);
}
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website
On Mon, 22 Oct 2012, Daniel Mack wrote: On 22.10.2012 17:17, Alan Stern wrote: On Sun, 21 Oct 2012, Artem S. Tashkinov wrote: dmesg messages up to a crash can be seen here: https://bugzilla.kernel.org/attachment.cgi?id=84221 The first problem in the log is endpoint list corruption. Here's a debugging patch which should provide a little more information. Maybe add a BUG() after each of these dev_err() so we stop at the first occurance and also see where we're coming from? A BUG() at these points would crash the machine hard. And where we came from doesn't matter; what matters is the values in the pointers. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website
On Mon, 22 Oct 2012, Artem S. Tashkinov wrote:
OK, here's what the kernel prints with your patch:
usb 6.1.4: ep 86 list del corruption prev: e5103b54 e5103a94 e51039d4
A small delay before I got thousands of list_del corruption messages would
have been nice, but I managed to catch the message anyway.
All right. Here's a new patch, which will print more information and
will provide a 10-second delay.
For this to be useful, you should capture a usbmon trace at the same
time. The relevant entries will show up in the trace shortly before
_and_ shortly after the error message appears.
Alan Stern
P.S.: It will help if you unplug as many of the other USB devices as
possible before running this test.
Index: usb-3.6/drivers/usb/core/hcd.c
===
--- usb-3.6.orig/drivers/usb/core/hcd.c
+++ usb-3.6/drivers/usb/core/hcd.c
@@ -1083,6 +1083,8 @@ EXPORT_SYMBOL_GPL(usb_calc_bus_time);
/*-*/
+static bool list_error;
+
/**
* usb_hcd_link_urb_to_ep - add an URB to its endpoint queue
* @hcd: host controller to which @urb was submitted
@@ -1193,6 +1195,25 @@ void usb_hcd_unlink_urb_from_ep(struct u
{
/* clear all state linking urb to this dev (and hcd) */
spin_lock(hcd_urb_list_lock);
+ {
+ struct list_head *cur = urb-urb_list;
+ struct list_head *prev = cur-prev;
+ struct list_head *next = cur-next;
+
+ if (prev-next != cur !list_error) {
+ list_error = true;
+ dev_err(urb-dev-dev,
+ ep %x list del corruption prev: %p %p %p %p
%p\n,
+ urb-ep-desc.bEndpointAddress,
+ cur, prev, prev-next, next, next-prev);
+ dev_err(urb-dev-dev,
+ head %p urb %p urbprev %p urbnext %p\n,
+ urb-ep-urb_list, urb,
+ list_entry(prev, struct urb, urb_list),
+ list_entry(next, struct urb, urb_list));
+ mdelay(1);
+ }
+ }
list_del_init(urb-urb_list);
spin_unlock(hcd_urb_list_lock);
}
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: Re: Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website
On Sun, 21 Oct 2012, Artem S. Tashkinov wrote: What I've found out is that my system crashes *only* when I try to enable usb-audio (from the same webcam) - I still have no idea how to capture a panic message, but I ran while :; do dmesg -c; done in xterm, then I got like thousands of messages and I photographed my monitor: http://imageshack.us/a/img685/9452/panicz.jpg list_del corruption. prev-next should be ... but was ... I cannot show you more as I have no serial console to use :( and the kernel doesn't have enough time to push error messages to rsyslog and fsync /var/log/messages Is it possible to use netconsole? The screenshot above appears to be the end of a long series of error messages, which isn't too useful. The most important information is in the first error. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: was: Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website
On Sun, 21 Oct 2012, Daniel Mack wrote: As the usb list is still in Cc: - Artem's lcpci dump shows that his machine features XHCI controllers. Can anyone think of a relation to this problem? And Artem, is there any way you boot your system on an older machine that only has EHCI ports? Thinking about it, I wonder whether the freeze in VBox and the crashes on native hardware have the same root cause. In that case, would it be possible to share that VBox image? Don't grasp at straws. All of the kernel logs Artem has posted show ehci-hcd; none of them show xhci-hcd. Therefore the xHCI controller is highly unlikely to be involved. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: A reliable kernel panic (3.6.2) and system crash when visiting a particular website
On Sat, 20 Oct 2012, Artem S. Tashkinov wrote: You don't get me - I have *no* VirtualBox (or any proprietary) modules running - but I can reproduce this problem using *the same system running under* VirtualBox in Windows 7 64. It's almost definitely either a USB driver bug or video4linux driver bug: Does the same thing happen with earlier kernel versions? What about if you unload snd-usb-audio or ehci-hcd? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] Get rid of get_driver() and put_driver()
Greg: This patch series removes the get_driver() and put_driver() routines from the kernel. Those routines don't do anything useful. Their comments say that they increment and decrement the driver's reference count, just like get_device()/put_device() and a lot of other utility routines. But a struct driver is _not_ like a struct device! It resembles a piece of code more than a piece of data -- it acts as an encapsulation of a driver. Incrementing its refcount doesn't have much meaning because a driver's lifetime isn't determined by the structure's refcount; it's determined by when the driver's module gets unloaded. What really matters for a driver is whether or not it is registered. Drivers expect, for example, that none of their methods will be called after driver_unregister() returns. It doesn't matter if some other thread still holds a reference to the driver structure; that reference mustn't be used for accessing the driver code after unregistration. get_driver() does not do any checking for this. People may have been misled by the kerneldoc into thinking that the references obtained by get_driver() do somehow pin the driver structure in memory. This simply isn't true; all it pins is the associated private structure. Code that needs to pin a driver must do it some other way (probably by calling try_module_get()). In short, these routines don't do anything useful and they can actively mislead people. Removing them won't introduce any bugs that aren't already present. There is no reason to keep them. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] Driver core: driver_find() drops reference before returning
As part of the removal of get_driver()/put_driver(), this patch
(as1510) changes driver_find(); it now drops the reference it acquires
before returning. The patch also adjusts all the callers of
driver_find() to remove the now unnecessary calls to put_driver().
In addition, the patch adds a warning to driver_find(): Callers must
make sure the driver they are searching for does not get unloaded
while they are using it. This has always been the case; driver_find()
has never prevented a driver from being unregistered or unloaded.
Hence the patch will not introduce any new bugs. The existing callers
all seem to be okay in this respect, however I don't understand the
video drivers well enough to be certain about them.
Signed-off-by: Alan Stern st...@rowland.harvard.edu
CC: Dmitry Torokhov dmitry.torok...@gmail.com
CC: Kyungmin Park kyungmin.p...@samsung.com
CC: Andy Walls awa...@md.metrocast.net
CC: Martin Schwidefsky schwidef...@de.ibm.com
---
drivers/base/driver.c |7 +--
drivers/input/gameport/gameport.c |1 -
drivers/input/serio/serio.c |1 -
drivers/media/video/cx18/cx18-alsa-main.c |1 -
drivers/media/video/ivtv/ivtvfb.c |2 --
drivers/media/video/s5p-fimc/fimc-mdevice.c |5 +
drivers/media/video/s5p-tv/mixer_video.c|1 -
drivers/s390/net/smsgiucv_app.c |9 -
8 files changed, 10 insertions(+), 17 deletions(-)
Index: usb-3.3/drivers/base/driver.c
===
--- usb-3.3.orig/drivers/base/driver.c
+++ usb-3.3/drivers/base/driver.c
@@ -234,7 +234,6 @@ int driver_register(struct device_driver
other = driver_find(drv-name, drv-bus);
if (other) {
- put_driver(other);
printk(KERN_ERR Error: Driver '%s' is already registered,
aborting...\n, drv-name);
return -EBUSY;
@@ -275,7 +274,9 @@ EXPORT_SYMBOL_GPL(driver_unregister);
* Call kset_find_obj() to iterate over list of drivers on
* a bus to find driver by name. Return driver if found.
*
- * Note that kset_find_obj increments driver's reference count.
+ * This routine provides no locking to prevent the driver it returns
+ * from being unregistered or unloaded while the caller is using it.
+ * The caller is responsible for preventing this.
*/
struct device_driver *driver_find(const char *name, struct bus_type *bus)
{
@@ -283,6 +284,8 @@ struct device_driver *driver_find(const
struct driver_private *priv;
if (k) {
+ /* Drop reference added by kset_find_obj() */
+ kobject_put(k);
priv = to_driver(k);
return priv-driver;
}
Index: usb-3.3/drivers/input/gameport/gameport.c
===
--- usb-3.3.orig/drivers/input/gameport/gameport.c
+++ usb-3.3/drivers/input/gameport/gameport.c
@@ -449,7 +449,6 @@ static ssize_t gameport_rebind_driver(st
} else if ((drv = driver_find(buf, gameport_bus)) != NULL) {
gameport_disconnect_port(gameport);
error = gameport_bind_driver(gameport, to_gameport_driver(drv));
- put_driver(drv);
} else {
error = -EINVAL;
}
Index: usb-3.3/drivers/input/serio/serio.c
===
--- usb-3.3.orig/drivers/input/serio/serio.c
+++ usb-3.3/drivers/input/serio/serio.c
@@ -441,7 +441,6 @@ static ssize_t serio_rebind_driver(struc
} else if ((drv = driver_find(buf, serio_bus)) != NULL) {
serio_disconnect_port(serio);
error = serio_bind_driver(serio, to_serio_driver(drv));
- put_driver(drv);
serio_remove_duplicate_events(serio, SERIO_RESCAN_PORT);
} else {
error = -EINVAL;
Index: usb-3.3/drivers/media/video/cx18/cx18-alsa-main.c
===
--- usb-3.3.orig/drivers/media/video/cx18/cx18-alsa-main.c
+++ usb-3.3/drivers/media/video/cx18/cx18-alsa-main.c
@@ -285,7 +285,6 @@ static void __exit cx18_alsa_exit(void)
drv = driver_find(cx18, pci_bus_type);
ret = driver_for_each_device(drv, NULL, NULL, cx18_alsa_exit_callback);
- put_driver(drv);
cx18_ext_init = NULL;
printk(KERN_INFO cx18-alsa: module unload complete\n);
Index: usb-3.3/drivers/media/video/ivtv/ivtvfb.c
===
--- usb-3.3.orig/drivers/media/video/ivtv/ivtvfb.c
+++ usb-3.3/drivers/media/video/ivtv/ivtvfb.c
@@ -1293,7 +1293,6 @@ static int __init ivtvfb_init(void)
drv = driver_find(ivtv, pci_bus_type);
err = driver_for_each_device(drv, NULL, registered,
ivtvfb_callback_init);
- put_driver(drv);
if (!registered) {
printk(KERN_ERR ivtvfb: no cards
Re: PROBLEM: EHCI disconnects DVB HDD
On Sat, 26 Nov 2011, Johann Klammer wrote: Alan Stern wrote: This is probably a low-level hardware error. Interference between the two ports of some kind. This is quite possible. Have been able to produce a more verbose logfile snippet. The log shows that your EHCI controller reports disconnects on both ports, after which it is unable to re-enumerate the disk drive. Following that, a bug in the DVB driver prevents it from unbinding properly, causing a hang. This could be the result of an electrical glitch or power fluctuation. Are these devices bus-powered? Or it could simply be something wrong with your EHCI controller. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PROBLEM: EHCI disconnects DVB HDD
On Fri, 25 Nov 2011, Johann Klammer wrote: When using a DVB-T Dongle and an external HDD simultaneously, EHCI almost always disconnects. When tuning, it works for some time, but after a while, probably triggered by disk activity the dmesg log fills up with disk errors and failed i2c writes. The devices disconnect and the mountpoint gets lost. Attempting a simple 'ls /mnt/rec' fails with 'input/ouput error'. The kernel log sometimes reports the khubd task hanging. As soon as the dvb software gets shut down(at that point DVB doesn't work either), the devices get rediscovered as low speed devices and work again after umount/mount and restart of software. But it's rather slow from there on. This is probably a low-level hardware error. Interference between the two ports of some kind. usbdump.mon: On request. usbmon output for the whole host controller during the event. I can mail this, but it's rather large and the mailing list won't accept the message. Can you put the usbmon trace on a web server like pastebin.com? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Fri, 12 Aug 2011, Hans de Goede wrote: I'm not claiming that this is a better solution than putting everything in the kernel. Just that it is a workable alternative which would involve a lot less coding. This is definitely an interesting proposal, something to think about ... I have 2 concerns wrt this approach: 1) It feels less clean then just having a single driver; and Agreed. 2) I agree it will be less coding, but I doubt it will really be that much less work. It will likely need less new code (but a lot can be more or less copy pasted), but it will need changes across a wider array of subsystems / userspace components, requiring a lot of coordinating, getting patches merged in different projects, etc. So in the end I think it too will be quite a bit of work. I guess that what I'm trying to say here is, that if we are going to spend a significant amount of time on this, we might just as well go for the best solution we can come up with even if that is some more work. Okay, go ahead. I have no objection. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Thu, 11 Aug 2011, Hans de Goede wrote: The alternative seems to be to define a device-sharing protocol for USB drivers. Kernel drivers would implement a new callback (asking them to give up control of the device), and usbfs would implement new ioctls by which a program could ask for and relinquish control of a device. The amount of rewriting needed would be relatively small. A few loose ends would remain, such as how to handle suspends, resumes, resets, and disconnects. Assuming usbfs is the only driver that will want to share a device in this way, we could handle them. Hans, what do you think? First of all thanks for the constructive input! When you say: device-sharing protocol, do you mean 2 drivers been attached, but only 1 being active. Or just some additional glue to make hand-over between them work better? I was thinking that the webcam driver would always be attached, but from time to time usbfs would ask to use the device. When the webcam driver gives away control, it remains bound to the device but does not send any URBs. If it needs to send an URB, first it has to ask usbfs to give control back. I've 2 concerns with this approach: 1) Assuming we are going for the just make hand over work better solution we will still have the whole disappear / reappear act of the /dev/video# node, which I don't like at all. That will not happen any more, because the webcam driver will always be bound to the device. If for example skype gets started it will say the user has no camera. If it were to say the device is busy, the user just might make a link to some application using the device in stillcam mode still being open. 2) The whole notion of the device being in use is rather vague when it comes to the userspace parts of this. Simply leaving say F-Spot running, or having a gvfs libgphoto share mounted, should not lead to not being able to use the device in webcam mode. But currently it will. That's true -- but it's true no matter what solution we adopt. The various drivers (whether in the kernel or in userspace) will have to decide for themselves when they can give up control. Fixing all users of libgphoto2 wrt this is unlikely to happen, and even if we do that now, more broken ones will likely come along later. I estimate 98% of all cameras are not dual mode cameras, so the average stillcam application developer will not test for this. Not all users of libgphoto2 have to be changed; only those which manage dual-mode cameras. Adding a few ioctls to ask for and give up control at the appropriate times must be a lot easier than porting the entire driver into the kernel. That leaves us with fixing the busy notion inside libgphoto2, iow, release the device as soon as an operation has completed. This will be quite slow, since both drivers don't know anything about each other, they will just know there is some $random_other_driver. So they need to assume the device state is unclean and re-init the device from scratch each time. Well, a user program can assume that the kernel driver left the device in a clean state. The reverse isn't always true, however -- it's one of the drawbacks of using a userspace driver. Besides, even though drivers don't always have to re-init the device from scratch every time, they do send all the current settings each time they use the device. So maybe the extra overhead is tolerable. Where as if we have both functions in one driver, that can remember the actual device state and only make changes if needed, so downloading + deleting 10 photos will lead to setting it to stillcam mode once, rather then 20 times. Depends how the ask-for-control ioctl is implemented. It might return a value indicating whether or not the webcam driver took control during the interval when the user program wasn't using the device. If usbfs retained control the entire time, the program should be able to assume the device's state hasn't changed. I'm not claiming that this is a better solution than putting everything in the kernel. Just that it is a workable alternative which would involve a lot less coding. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
Alan Cox raised a bunch of good points. I'm not going to respond to them; they pretty much speak for themselves. On Thu, 11 Aug 2011, Mauro Carvalho Chehab wrote: Between two or more kernel drivers, a resource locking mechanism like the one you've proposed works fine, It's not a locking mechanism. More like cooperative multi-access. but, when the driver is on userspace, there's one additional issue that needs to be addressed: What happens if, for example, if a camera application using libgphoto2 crashes? The lock will be enabled, and the V4L driver will be locked forever. Also, if the lock is made generic enough to protect between two different userspace applications, re-starting the camera application won't get the control back. You're wrong, because what I proposed isn't a lock. If the user program dies, the usbfs device file will automatically be closed and then usbfs will freely give control back to the webcam driver. If the program hangs at the wrong time, then the webcam driver won't be able to regain control. At least the user will have the option of killing the program when this happens; a similar hang in a kernel driver tends to be much uglier. Two different user programs trying to drive the device at the same time doesn't necessarily have to cause a problem. At any particular moment, only one of them would be in control of the device. To avoid such risk, kernel might need to implement some ugly hacks to detect when the application was killed, and put the device into a sane state, if this happens. No ugly hack is needed. usbfs can directly inform the webcam driver whether or not the device file was closed while the user program retained control. If that didn't happen, it's safe to assume that the program gave up control voluntarily. Not all users of libgphoto2 have to be changed; only those which manage dual-mode cameras. Adding a few ioctls to ask for and give up control at the appropriate times must be a lot easier than porting the entire driver into the kernel. Again, applications that won't implement this control will take the lock forever. No, because there is no lock. Programs that haven't been changed will continue to behave as they do today -- they will unbind the webcam driver and assume full control of the device. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Thu, 11 Aug 2011, Alan Cox wrote: Actually there are more issues than that - you've also got to worry about a security/permission model, and that is hard to get right, especially if you are not very careful that anything that can be retrieved which might violate the security model (eg the last frame on the capture) has been blanked before handover etc. As far as I can tell, these same security issues exist today. I don't see them getting any worse than they are now. And applications that are touching both video (even indirectly) and still camera may get surprise deadlocks if they accidentally reference both the still and video device even via some library or service. No, not deadlocks. Just -EBUSY errors. Well, a user program can assume that the kernel driver left the device in a clean state. The reverse isn't always true, however -- it's one Not it cannot - the user program doesn't know a) if the kernel driver has ever been loaded b) the values the kernel driver leaves set (and those will change no doubt at times) c) if the camera has been plugged and unplugged and not yet had the kernel driver loaded That's true. The program can't assume that a kernel driver was ever bound to the device; all it can assume is that _if_ a kernel driver was bound then it left the device in a sane state -- whatever sane might mean in this context. To me it sounds like a recipe for disaster. For those tiny number of devices involved just use V4L and if need be some small V4L tweaks to handle still mode. In most cases the interface is basically identical and I'd bet much of the code is identical too. I'm not against moving the whole thing into the kernel. I'm just pointing out that an easier-to-code-up solution will accomplish much the same result. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Thu, 11 Aug 2011, Theodore Kilgore wrote: Alan, As I said, I am agnostic, though leaning in the direction that Hans de Goede is pointing. What he says about a single control mechanism seems to make a lot of sense. If you can come up with an outline of the easier to code solution, that would be interesting, though. That approach has been outlined in the other emails I have posted recently. I assume you are also going to be in Vancouver? If you will be there on Monday, then Hans and I are already planning to meet and discuss. No, I'm not going to Vancouver. However I will attend the Linux Plumbers conference in Santa Rosa. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Tue, 9 Aug 2011, Hans de Goede wrote: Hi, On 08/09/2011 04:19 PM, Alan Stern wrote: On Tue, 9 Aug 2011, Hans de Goede wrote: According to Theodore, we have developed 5 drivers for them because the stillcam modes in different devices use four different vendor-specific drivers. Yes, but so the the webcam modes of the different devices, so for the 5 (not sure if that is the right number) dual-cam mode chipsets we support there will be 5 drivers, each supporting both the webcam and the access to pictures stored in memory of the chipset they support. So 5 chipsets - 5 drivers each supporting 1 chipset, and both functions of the single logical device that chipset represents. Does it really make sense to combine 5 drivers into one? Right, that is not the plan. The plan is to simply stop having 2 drivers for 1 logical (and physical) block. So we go from 10 drivers, 5 stillcam + 5 webcam, to just 5 drivers. We will also likely be able to share code between the code for the 2 functionalities for things like generic set / get register functions, initialization, etc. Okay, I didn't realize that the different cameras used different webcam drivers as well as different stillcam drivers. As far as I can see, there's nothing to stop anybody from adding the stillcam functionality into the webcam drivers right now. If some common code can be abstracted out into a shared source file, so much the better. That would solve the problem, right? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Wed, 10 Aug 2011, Theodore Kilgore wrote: Okay, I didn't realize that the different cameras used different webcam drivers as well as different stillcam drivers. Oh, yes. They are Proprietary devices. And that means what it says. :-) And all different from each other, too. As far as I can see, there's nothing to stop anybody from adding the stillcam functionality into the webcam drivers right now. If some common code can be abstracted out into a shared source file, so much the better. That would solve the problem, right? I think everyone involved believes that it would solve the problem. The question has been all along whether or not there is any other way which would work. Also the question of what, exactly, belongs in the kernel and what does not. For, if something has been historically supported in userspace (stillcam support, in this case) and has worked well there, I would think it is kind of too bad to have to move said support into the kernel just because the same hardware requires kernel support for another functionality and the two sides clash. I mean, the kernel is already big enough, no? But the logic that Hans has set forth seems rather compelling. The alternative seems to be to define a device-sharing protocol for USB drivers. Kernel drivers would implement a new callback (asking them to give up control of the device), and usbfs would implement new ioctls by which a program could ask for and relinquish control of a device. The amount of rewriting needed would be relatively small. A few loose ends would remain, such as how to handle suspends, resumes, resets, and disconnects. Assuming usbfs is the only driver that will want to share a device in this way, we could handle them. Hans, what do you think? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB mini-summit at LinuxCon Vancouver
On Tue, 9 Aug 2011, Hans de Goede wrote: I would really like to see the dual mode camera and TV tuner discussion separated. They are 2 different issues AFAIK. 1) Dual mode cameras: In the case of the dual mode camera we have 1 single device (both at the hardware level and at the logical block level), which can do 2 things, but not at the same time. It can stream live video data from a sensor, or it can retrieve earlier taken pictures from some picture memory. Unfortunately even though these 2 functions live in a single logical block, historically we've developed 2 drivers for them. This leads to fighting over device ownership (surprise surprise), and to me the solution is very clear, 1 logical block == 1 driver. According to Theodore, we have developed 5 drivers for them because the stillcam modes in different devices use four different vendor-specific drivers. Does it really make sense to combine 5 drivers into one? I think some sort of sharing protocol would work out better. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Workshop-2011] Media Subsystem Workshop 2011
On Mon, 8 Aug 2011, Adam Baker wrote: Further testing reveals the situation is more complex than I first thought - the behaviour I get depends upon whether what gets plugged in is a full speed or a high speed device. After I've run the test of running gphoto whilst streaming from a supported dual mode camera, lsusb fails to recognise a high speed device plugged into the port the camera was plugged into (it works fine if plugged in elsewhere) and lsusb hangs if I plug in a new low speed or full speed device. When I get some time I'll see if I can recreate the problem using libusb with a totally different device. Looking around my pile of USB bits for something full speed with a kernel driver I've got a PL2303 serial port. Would that be a good choice to test with? I have no idea. But the symptoms you describe are indicative of a hardware problem, not a driver bug. Just for reference with a full speed device I see the messages below in dmesg with the second one only appearing when I do lsusb [10832.128039] usb 3-2: new full speed USB device using uhci_hcd and address 34 [10847.240031] usb 3-2: device descriptor read/64, error -110 and with a high speed device I see a continuous stream of [11079.820097] usb 1-4: new high speed USB device using ehci_hcd and address 103 [11079.888355] hub 1-0:1.0: unable to enumerate USB device on port 4 [11080.072377] hub 1-0:1.0: unable to enumerate USB device on port 4 [11080.312053] usb 1-4: new high speed USB device using ehci_hcd and address 105 [11080.380418] hub 1-0:1.0: unable to enumerate USB device on port 4 [11080.620030] usb 1-4: new high speed USB device using ehci_hcd and address 106 [11080.688322] hub 1-0:1.0: unable to enumerate USB device on port 4 The dmesg log is relatively uninformative unless you enable CONFIG_USB_DEBUG in the kernel build. Have you tried running these tests on a different computer, preferably one using a different chipset? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Workshop-2011] Media Subsystem Workshop 2011
On Sun, 7 Aug 2011, Theodore Kilgore wrote: This indirectly answers my question, above, about whatever device there may or may not be. What I get from this, and also from a bit of snooping around, is that there is not any dev that gets created in order to be accessed by libusb. Just an entry under /proc/bus/usb, which AFAICT is at most a pseudo-device. Thanks. Nowadays, most distributions create device nodes under /dev/bus/usb. A few also support the old /proc/bus/usb files. So, Alan, what do you think is the best way to go about the problem? The camera can act as a stillcam or as a webcam. The problem is to provide access to both, with equal facility (and of course to lock out access to whichever action is not currently in use, if the other one is). The current situation with libusb does not cut it, as among other things it currently does only half the job and seemingly cannot address the locking problem. Hans suggests to create two explicit devices, /dev/video (as already done and something like /dev/cam. Then access webcam function as now and stillcam function with libgphoto2, as now, but through /dev/cam instead of through libusb. This would seem to me to solve all the problems, but at the expense of some work. Can you think of something more clever? I'm not familiar with the MTP protocol used in the stillcam mode, or how feasible it would be to implement that protocol in a kernel driver. Maybe a good compromise would be to create a kind of stub driver that could negotiate the device access while still delegating most of the real work to userspace. This could become a bigger problem if this kind of design becomes an ongoing trend. To do what Hans was suggesting, today we have to merge two separate drivers... then tomorrow we would have to merge two others and then later on even more. Before you know it, we would end up with a single gigantic kernel driver to manage every USB device! Obviously not a sustainable approach in the long run. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Workshop-2011] Media Subsystem Workshop 2011
On Mon, 8 Aug 2011, Theodore Kilgore wrote: On Mon, 8 Aug 2011, Alan Stern wrote: On Sun, 7 Aug 2011, Theodore Kilgore wrote: This indirectly answers my question, above, about whatever device there may or may not be. What I get from this, and also from a bit of snooping around, is that there is not any dev that gets created in order to be accessed by libusb. Just an entry under /proc/bus/usb, which AFAICT is at most a pseudo-device. Thanks. Nowadays, most distributions create device nodes under /dev/bus/usb. A few also support the old /proc/bus/usb files. What does this mean, exactly, in practice? You are right that I have the /dev/bus/usb/ files but does everybody have them, these days? Pretty much everybody using udev should have them, which means pretty much every desktop system. ... Maybe a good compromise would be to create a kind of stub driver that could negotiate the device access while still delegating most of the real work to userspace. Hooray. This appears to me to be a very good solution. I'm not so sure. It would require vast changes to the userspace program, for example. I agree approximately 120% with this. Let's think of a more clever way. If we get the basic idea right, it really ought not to be too terribly difficult. The method Hans suggested was rather clunky. It also required drivers to know when the device was in use, which may be okay for a video driver but is not so practical for usb-storage (although to be fair, I suspect usb-storage wouldn't need to be involved). And it required kernel drivers to inform user programs somehow when they want to get control of the device back, which is not the sort of thing drivers normally have to do. Even if we could come up with a way to let the video driver somehow share ownership of the device with usbfs, we'd still have to set up a protocol for deciding who was in charge at any given time. Would it be okay for the userspace program simply to say I want control now and I'm done, you can have control back? For that matter, what should the video driver do if the user program crashes or hangs while in charge of the device? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Workshop-2011] Media Subsystem Workshop 2011
On Mon, 8 Aug 2011, Theodore Kilgore wrote: Maybe a good compromise would be to create a kind of stub driver that could negotiate the device access while still delegating most of the real work to userspace. Hooray. This appears to me to be a very good solution. I'm not so sure. It would require vast changes to the userspace program, for example. Such as? Such as completely rewriting the USB interface. You wouldn't be able to use libusb, for example. The method Hans suggested was rather clunky. If it involves moving practically all of the gory details of the support of stillcam mode for individual dual-mode cameras into the kernel, then it certainly appears clunky to me, too. It also required drivers to know when the device was in use, which may be okay for a video driver but is not so practical for usb-storage (although to be fair, I suspect usb-storage wouldn't need to be involved). Yes, I can see that. Usb-storage is, essentially, in use while the device is attached, and that has to be true because the device is a storage device. And alas, not all storage devices even get mounted, so one cannot decide whether the device is in use just by checking whether or not something on it is mounted ... And it required kernel drivers to inform user programs somehow when they want to get control of the device back, Why, exactly? Don't ask me, ask Hans! :-) I mean, fundamentally we have two functionalities of the device which are accessed, at the user level, by two userspace programs. One of them gets the still photos off the camera, and the other one gets the video stream. Perhaps we just need a method for saying No! to either one of those apps if the other one is using the camera? That's basically what I suggested below. which is not the sort of thing drivers normally have to do. Even if we could come up with a way to let the video driver somehow share ownership of the device with usbfs, we'd still have to set up a protocol for deciding who was in charge at any given time. Would it be okay for the userspace program simply to say I want control now and I'm done, you can have control back? Actually, I would expect that if one program is accessing the device then the other one can't, and this works the same in both directions. Unless you think that what you described is better? When a program uses libgphoto2, how is the kernel supposed to know when the program is busy accessing the device? The kernel can't just ask the program. Incidentally, I think that in some respects the fact that webcam support is in the kernel and stillcam support is in userspace is a red herring. No, this has some significant implications. In particular, there's no good way for the kernel driver to ask the userspace driver if it is busy. If both drivers were in the kernel, this would be easy to arrange. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Workshop-2011] Media Subsystem Workshop 2011
On Sun, 7 Aug 2011, Adam Baker wrote: I've addec Hans de Geode and linux-usb to the CC as this response picks up on a related discussion about the usb mini summit. On Friday 05 August 2011, Theodore Kilgore wrote: If you can solve the locking problem between devices in the kernel then it shouldn't matter if one of the kernel devices is the generic device that is used to support libusb. Hmmm. Perhaps not. While we are on the topic, what exactly do you mean by the generic device that is used to support libusb. Which device is that, exactly? The file drivers/usb/core/devio.c registers itself as a driver called usb_device which is used to provide all of the device drivers that live under /proc/bus/usb Let's get things correct. The driver is called usbfs, not usb_device. The things that live under /proc/bus/usb are files representing USB devices, not device drivers. If you look in there for the code to handle the ioctl() USBDEVFS_DISCONNECT then you will find the code that is called when you make a usb_detach_kernel_driver_np() call through libusb. That code, according to the documentation and my testing needs to acquire a lock before it calls usb_driver_release_interface(). Based on my testing to date (using cheese to start a camera streaming and then gphoto2 -L to trigger the disconnect ioctl) I would suggest that the fact it doesn't is a kernel bug that needs fixing What makes you think the code doesn't acquire the lock? (Hint: Look at usbdev_do_ioctl() instead of proc_ioctl().) regardless of whether there is any user space solution to camera mode switching because that code could potentially get called on any in use USB device and if it does even thing like lsusb don't work correctly afterwards and completely unrelated devices don't work if they are later plugged into the same USB port. That's a rather incomprehensible run-on sentence, but as near as I can tell, it is wrong. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera
On Fri, 15 Jul 2011, Ming Lei wrote:
Hi,
On Thu, Jul 14, 2011 at 11:03 PM, Alan Stern st...@rowland.harvard.edu
wrote:
More likely, the reset erases some device setting that uvcvideo
installed while binding. �Evidently uvcvideo does not re-install the
setting during reset-resume; this is probably a bug in the driver.
Alan, you are right.
I think I have found the root cause. Given many devices can't
handle set_interface(0) if the interfaces were already in altsetting 0,
usb_reset_and_verify_device does not run set_interface(0). So we
need to do it in .reset_resume handler of uvc driver and it is always
safe for uvc devices.
I have tested the below patch, and it can make the uvc device work
well after rpm resume and system resume(reset resume), both in
streaming on and off case.
Alan, Laurent, if you have no objections, I will submit a formal one.
diff --git a/drivers/media/video/uvc/uvc_driver.c
b/drivers/media/video/uvc/uvc_driver.c
index b6eae48..4055dfc 100644
--- a/drivers/media/video/uvc/uvc_driver.c
+++ b/drivers/media/video/uvc/uvc_driver.c
@@ -1959,8 +1959,12 @@ static int __uvc_resume(struct usb_interface
*intf, int reset)
}
list_for_each_entry(stream, dev-streams, list) {
- if (stream-intf == intf)
+ if (stream-intf == intf) {
+ if (reset)
+ usb_set_interface(stream-dev-udev,
+ stream-intfnum, 0);
return uvc_video_resume(stream);
+ }
}
uvc_trace(UVC_TRACE_SUSPEND, Resume: video streaming USB interface
This is fine with me. However, it is strange that the Set-Interface
request is necessary. After being reset, the device should
automatically be in altsetting 0 for all interfaces.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera
On Fri, 15 Jul 2011, Ming Lei wrote: Hi, On Fri, Jul 15, 2011 at 10:27 PM, Alan Stern st...@rowland.harvard.edu wrote: This is fine with me. �However, it is strange that the Set-Interface request is necessary. �After being reset, the device should automatically be in altsetting 0 for all interfaces. For uvc devices, seems it is not strange, see uvc_video_init(), which is called in .probe path and executes Set-Interface 0 first, then starts to get/set video control. I see what you mean. Apparently other UVC devices also need to receive a Set-Interface(0) request before they will work right. (It's still strange, though...) Anyway, since the driver does this during probe, it makes sense to do it during reset-resume as well. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera
On Thu, 14 Jul 2011, Ming Lei wrote:
Hi,
On Wed, Jul 13, 2011 at 11:59 PM, Alan Stern st...@rowland.harvard.edu
wrote:
On Wed, 13 Jul 2011, Ming Lei wrote:
Almost same.
Come on. �Almost same means they are different. �That difference is
clearly the important thing you need to track down.
I didn't say entirely same because we can't trace the packets via usbmon
during system resume, but we can do it during runtime resume.
In fact, except for above, the packets captured from interrupt ep and control
ep
are completely same. Also all functions in uvc (rpm, system)resume path
return
successfully.
All right; this tends to confirm your guess that the BIOS messes up the
device by resetting it during system resume.
�If I add USB_QUIRK_RESET_RESUME quirk for the device,
the stream data will not be received from the device in runtime pm case,
same with that in system suspend.
uvcvideo should be able to reinitialize the device so that it works
correctly following a reset. �If the device doesn't work then uvcvideo
has a bug in its reset_resume handler.
This also indicates the usb reset during resume does make the uvc device
broken.
Resetting the device doesn't actually _break_ it -- if it did then the
device would _never_ work because the first thing that usbcore does to
initialize a new device is reset it!
More likely, the reset erases some device setting that uvcvideo
installed while binding. Evidently uvcvideo does not re-install the
setting during reset-resume; this is probably a bug in the driver.
The below quirk fixes the issue now.
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 81ce6a8..93c6fa2 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -82,6 +82,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* Broadcom BCM92035DGROM BT dongle */
{ USB_DEVICE(0x0a5c, 0x2021), .driver_info = USB_QUIRK_RESET_RESUME },
+ /* Microdia uvc camera */
+ { USB_DEVICE(0x0c45, 0x6437), .driver_info = USB_QUIRK_RESET_MORPHS },
+
/* Action Semiconductor flash disk */
{ USB_DEVICE(0x10d6, 0x2200), .driver_info =
USB_QUIRK_STRING_FETCH_255 },
It would be better to fix uvcvideo, if you could figure out what it
needs to do differently. This quirk is only a workaround, because the
device doesn't really morph.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera
On Wed, 13 Jul 2011, Ming Lei wrote: Hi, On Tue, Jul 12, 2011 at 11:44 PM, Alan Stern st...@rowland.harvard.edu wrote: On Tue, 12 Jul 2011, Ming Lei wrote: Hi Laurent, After resume from sleep, �all the ISO packets from camera are like below: 880122d9f400 3527230728 S Zi:1:004:1 -115:1:2568 32 -18:0:1600 -18:1600:1600 -18:3200:1600 -18:4800:1600 -18:6400:1600 51200 880122d9d400 3527234708 C Zi:1:004:1 0:1:2600:0 32 0:0:12 0:1600:12 0:3200:12 0:4800:12 0:6400:12 51200 = 0c8c fa7e 012f1b05 All are headed with 0c8c, see attached usbmon captures. Maybe this device needs a USB_QUIRK_RESET_RESUME entry in quirks.c. I will try it, but seems unbindbind driver don't produce extra usb reset signal to the device. Also, the problem didn't happen in runtime pm case, just happen in wakeup from system suspend case. uvcvideo has enabled auto suspend already at default. Why should system suspend be different from runtime suspend? Have you compared usbmon traces for the two types of suspend? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera
On Wed, 13 Jul 2011, Ming Lei wrote: Hi, On Wed, Jul 13, 2011 at 11:20 PM, Alan Stern st...@rowland.harvard.edu wrote: Why should system suspend be different from runtime suspend? �Have you This is also my puzzle, :-) compared usbmon traces for the two types of suspend? Almost same. Come on. Almost same means they are different. That difference is clearly the important thing you need to track down. If I add USB_QUIRK_RESET_RESUME quirk for the device, the stream data will not be received from the device in runtime pm case, same with that in system suspend. uvcvideo should be able to reinitialize the device so that it works correctly following a reset. If the device doesn't work then uvcvideo has a bug in its reset_resume handler. Maybe buggy BIOS makes root hub send reset signal to the device during system suspend time, not sure... That's entirely possible. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] uvcvideo: add fix suspend/resume quirk for Microdia camera
On Tue, 12 Jul 2011, Ming Lei wrote: Hi Laurent, After resume from sleep, all the ISO packets from camera are like below: 880122d9f400 3527230728 S Zi:1:004:1 -115:1:2568 32 -18:0:1600 -18:1600:1600 -18:3200:1600 -18:4800:1600 -18:6400:1600 51200 880122d9d400 3527234708 C Zi:1:004:1 0:1:2600:0 32 0:0:12 0:1600:12 0:3200:12 0:4800:12 0:6400:12 51200 = 0c8c fa7e 012f1b05 All are headed with 0c8c, see attached usbmon captures. Maybe this device needs a USB_QUIRK_RESET_RESUME entry in quirks.c. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC, PATCH] USB: EHCI: Allow users to override 80% max periodic bandwidth
On Thu, 23 Jun 2011, Kirill Smelkov wrote: At 480 Mb/s, each microframe holds 7500 bytes (less if you count bit-stuffing). 4% of that is 300 bytes, which is not enough for a 512-byte bulk packet. I think you'd run into trouble trying to do any serious bulk transfers on such a tight schedule. Yes, you seem to be right. I still think 4% is maybe enough for control traffic. It should be. @@ -571,6 +579,14 @@ static int ehci_init(struct usb_hcd *hcd) hcc_params = ehci_readl(ehci, ehci-caps-hcc_params); /* + * tell user, if using non-standard (80% == 100 usec/uframe) bandwidth + */ + if (uframe_periodic_max != 100) + ehci_info(ehci, using non-standard max periodic bandwith + (%u%% == %u usec/uframe), + 100*uframe_periodic_max/125, uframe_periodic_max); + + /* Check for invalid values. This should never be less than 100 or greater than 125. Ok. By the way, why should we limit it to be not less than 100? Likewise, a user who knows exactly what he/she is doing could limit periodic bandwidth to be less than 80% required by USB specification. What's the point? If you want to use less than 80% of your bandwidth for periodic transfers, go ahead and do so. You don't need to change the limit. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: uvcvideo failure under xHCI
On Thu, 16 Jun 2011, Laurent Pinchart wrote: Hi Sarah, On Thursday 16 June 2011 04:59:57 Sarah Sharp wrote: On Wed, Jun 15, 2011 at 06:39:57PM -0700, Sarah Sharp wrote: When I plug in a webcam under an xHCI host controller in 3.0-rc3+ (basically top of Greg's usb-linus branch) with xHCI debugging turned on, the host controller occasionally cannot keep up with the isochronous transfers, and it tells the xHCI driver that it had to skip several microframes of transfers. These Missed Service Intervals aren't supposed to be fatal errors, just an indication that something was hogging the PCI memory bandwidth. The xHCI driver then sets the URB's status to -EXDEV, to indicate that some of the iso_frame_desc transferred, and sets at least one frame's status to -EXDEV: ... The urb-status causes uvcvideo code in uvc_status.c:uvc_status_complete() to fail with the message: Jun 15 17:37:11 talon kernel: [ 117.987769] uvcvideo: Non-zero status (-18) in video completion handler. ... I've grepped through drivers/media/video, and it seems like none of the drivers handle the -EXDEV status. What should the xHCI driver be setting the URB's status and frame status to when the xHCI host controller skips over transfers? -EREMOTEIO? Or does it need to set the URB's status to zero, but only set the individual frame status to -EXDEV? Ok, looking at both EHCI and UHCI, they seem to set the urb-status to zero, regardless of what they set the frame descriptor field to. Alan, does that seem correct? The description of the behavior of ehci-hcd and uhci-hcd is correct. ohci-hcd behaves the same way too. And they all agree with the behavior described in the kerneldoc for struct urb in include/linux/usb.h. According to Documentation/usb/error-codes.txt, host controller drivers should set the status to -EXDEV. However, no device drivers seem to handle that, probably because the EHCI/UHCI drivers don't use that error code. Drivers are clearly out of sync with the documentation, so we should fix one of them. Under the circumstances, the documentation file should be changed. Sarah, can you do that along with the change to xhci-hcd? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: uvcvideo failure under xHCI
On Thu, 16 Jun 2011, Sarah Sharp wrote: Alan, does that seem correct? The description of the behavior of ehci-hcd and uhci-hcd is correct. ohci-hcd behaves the same way too. And they all agree with the behavior described in the kerneldoc for struct urb in include/linux/usb.h. Ah, you mean this bit? * @status: This is read in non-iso completion functions to get the * status of the particular request. ISO requests only use it * to tell whether the URB was unlinked; detailed status for * each frame is in the fields of the iso_frame-desc. Right. There's also some more near the end: * Completion Callbacks: * * The completion callback is made in_interrupt(), and one of the first * things that a completion handler should do is check the status field. * The status field is provided for all URBs. It is used to report * unlinked URBs, and status for all non-ISO transfers. It should not * be examined before the URB is returned to the completion handler. Under the circumstances, the documentation file should be changed. Sarah, can you do that along with the change to xhci-hcd? Sure. It feels like there should be a note about which values isochronous URBs might have in the urb-status field. The USB core is the only one that would be setting those, so which values would it set? uvcvideo tests for these error codes: case -ENOENT: /* usb_kill_urb() called. */ case -ECONNRESET: /* usb_unlink_urb() called. */ case -ESHUTDOWN:/* The endpoint is being disabled. */ case -EPROTO: /* Device is disconnected (reported by some * host controller). */ Are there any others. -EREMOTEIO, in the unlikely event that URB_SHORT_NOT_OK is set, but no others. And I wasn't aware of that last one... Host controller drivers should report -ESHUTDOWN to mean the device has been disconnected, not -EPROTO. But usually HCD don't take these events into account when determining URB status codes. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: uvcvideo failure under xHCI
On Thu, 16 Jun 2011, Sarah Sharp wrote: Sure. It feels like there should be a note about which values isochronous URBs might have in the urb-status field. The USB core is the only one that would be setting those, so which values would it set? uvcvideo tests for these error codes: case -ENOENT: /* usb_kill_urb() called. */ case -ECONNRESET: /* usb_unlink_urb() called. */ case -ESHUTDOWN:/* The endpoint is being disabled. */ case -EPROTO: /* Device is disconnected (reported by some * host controller). */ Are there any others. -EREMOTEIO, in the unlikely event that URB_SHORT_NOT_OK is set, but no others. Are you saying that the USB core will only set -EREMOTEIO for isochronous URBs? Or do you mean that in addition to the status values that uvcvideo checks, the USB core can also set -EREMOTEIO? The latter. However, if uvcvideo never sets the URB_SHORT_NOT_OK flag then usbcore will never set urb-status to -EREMOTEIO. And I wasn't aware of that last one... Host controller drivers should report -ESHUTDOWN to mean the device has been disconnected, not -EPROTO. But usually HCD don't take these events into account when determining URB status codes. The xHCI driver will return -ESHUTDOWN as a status for URBs when the host controller is dying. That's appropriate. But nobody should ever set an isochronous URB's status field to -EPROTO, no matter whether the device is connected or not and no matter whether the host controller is alive or not. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: uvcvideo failure under xHCI
On Thu, 16 Jun 2011, Sarah Sharp wrote: On Thu, Jun 16, 2011 at 03:39:11PM -0400, Alan Stern wrote: That's appropriate. But nobody should ever set an isochronous URB's status field to -EPROTO, no matter whether the device is connected or not and no matter whether the host controller is alive or not. But the individual frame status be set to -EPROTO, correct? That's what Alex was told to do when an isochronous TD had a completion code of Incompatible Device Error. Right. -EPROTO is a perfectly reasonable code for a frame's status. But not for an isochronous URB's status. There's no reason for uvcvideo to test for it. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Improving kernel - userspace (usbfs) usb device hand off
On Sat, 11 Jun 2011, Hans de Goede wrote: So what do we need to make this situation better: 1) A usb_driver callback alternative to the disconnect callback, I propose to call this soft_disconnect. This serves 2 purposes a) It will allow the driver to tell the caller that that is not a good idea by returning an error code (think usb mass storage driver and mounted filesystem Not feasible. usb-storage has no idea whether or not a device it controls has a mounted filesystem. (All it does is send SCSI commands to a device and get back the results.) Since that's the main use case you're interested in, this part of the proposal seems destined to fail. This is not completely true, I cannot rmmod usb-storage as long as disks using it are mounted. I know this is done through the global module usage count, so this is not per usb-storage device. But extending the ref counting to be per usb-storage device should not be hard. All the accounting is already done for this. It would be harder than you think. All the accounting is _not_ already being done. What you're talking about would amount to a significant change in the driver model core and the SCSI core. It isn't just a USB thing. But userspace _does_ know where the mounted filesystems are. Therefore userspace should be responsible for avoiding programs that want to take control of devices holding these filesystems. That's the reason why usbfs device nodes are owned by root and have 0644 mode; there're can be written to only by programs with superuser privileges -- and such programs are supposed to be careful about what they do. Yes, and what I'm asking for is for an easy way for these programs to be careful. A way for them to ask the kernel, which in general is responsible for things like this and traditionally does resource management and things which come with that like refcounting: unbind the driver from this device unless the device is currently in use. Sure. At the moment the kernel does not keep track of whether a device is currently in use -- at least, not in the way you mean. I'm not saying this can't be done. But it would be a bigger job than you think, and this isn't the appropriate thread to discuss it. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Improving kernel - userspace (usbfs) usb device hand off
want to turn control of devices over to specific drivers. That's why this problem needs to be handled in userspace. 4) A IOCTL_USBFS_SOFT_DISCONNECT ioctl which will call the drivers soft_disconnect if it has one, and otherwise fall back to the regular disconnect. 5) A method for a usbfs fd owning app to know the device driver would like the device back. I suggest using poll with POLLIN to signal this. It seems as if you're trying to implement some notion of allowing a device to have more than one driver at the same time. This is so far out from the way the kernel behaves now, adopting it would be very difficult if not impossible. Certainly the USB stack isn't the place to start. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Improving kernel - userspace (usbfs) usb device hand off
On Fri, 10 Jun 2011, Mauro Carvalho Chehab wrote: Em 10-06-2011 11:48, Alan Stern escreveu: On Fri, 10 Jun 2011, Hans de Goede wrote: As Felipe has mentioned, this sounds like the sort of problem that can better be solved in userspace. A dual-mode device like the one you describe really is either a still-cam or a webcam, never both at the same time. Hence what users need is a utility program to switch modes (by loading/unloading the appropriate programs or drivers). Unloading a driver in order to access the hardware via userspace? This sounds a very bad idea do me. What happens if another hardware is using the same driver? A kernel driver wouldn't have to be unloaded. It could simply be unbound from the device via sysfs or usbfs. Alan STern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Improving kernel - userspace (usbfs) usb device hand off
On Fri, 10 Jun 2011, Felipe Balbi wrote: I don't see any problems in this situation. If, for that particular product, webcam and still image functionality are mutually exclusive, then that's how the product (and their drivers) have to work. If the linux community decided to put webcam functionality in kernel and still image functionality on a completely separate driver, that's entirely our problem. And the problem is how to coordinate the two of them. 2. Until recently in the history of Linux, there was an irreconcilable conflict. If a kernel driver for the video streaming mode was present and installed, it was not possible to use the camera in stillcam mode at all. Thus the only solution to the problem was to blacklist the kernel module so that it would not get loaded automatically and only to install said module by hand if the camera were to be used in video streaming mode, then to rmmod it immediately afterwards. Very cumbersome, obviously. true... but why couldn't we combine both in kernel or in userspace altogether ? Why do we have this split ? (words from a newbie in V4L2, go easy :-p) I think the problem may be that the PTP protocol used in the still-cam mode isn't suitable for a kernel driver. Or if it is suitable, it would have to be something like a shared-filesystem driver -- nothing like a video driver. You certainly wouldn't want to put it in V4L2. Or, to move the libgphoto2 driver to kernel, combine it in the same driver that handles streaming. No ? No. Something else is needed. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: was: af9015, af9013 DVB-T problems. now: Intermittent USB disconnects with many (2.0) high speed devices
On Sun, 13 Jun 2010, thomas schorpp wrote: ehci-hcd is broken and halts silently or disconnects after hours or a few days, with the wlan usb adapter How do you know the bug is in ehci-hcd and not in the hardware? I was able to catch a dmesg err message like ehci...force halt... handshake failed once only. Can you please post the error message? The disconnects with dvb-usb need reboot cause driver cannot be removed with modprobe. That sounds like it might be a bug in dvb-usb driver. It always should be possible to remove the driver. This long standing bug is really nasty and makes permanent high speed usb connections unusable on Linux, at least with this VIA hardware. No debug parms in modules, we need to ask linux-usb how to debug this. You can start by building a kernel with CONFIG_USB_DEBUG enabled. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: was: af9015, af9013 DVB-T problems. now: Intermittent USB disconnects with many (2.0) high speed devices
On Sun, 13 Jun 2010, thomas schorpp wrote: Am 13.06.2010 15:57, schrieb Alan Stern: On Sun, 13 Jun 2010, thomas schorpp wrote: ehci-hcd is broken and halts silently or disconnects after hours or a few days, with the wlan usb adapter How do you know the bug is in ehci-hcd and not in the hardware? All 3 usb devices and 2 different series VIA usb hosts and Hemsing's and many other broken i2c comms reporter's on linux-media are broken instead? It's certainly possible and has been known to happen. Well, if we get that confirmed, I'll buy 2 of those with NEC chipset: http://cgi.ebay.de/ws/eBayISAPI.dll?ViewItemitem=190318779935 I was able to catch a dmesg err message like ehci...force halt... handshake failed once only. Can you please post the error message? Jun 3 08:38:29 tom3 kernel: [75071.004062] ehci_hcd :00:0e.2: force halt; handhake cc9c0814 c000 - -110 Jun 3 08:45:13 tom3 kernel: [75475.004061] ehci_hcd :00:0e.2: force halt; handhake cc9c0814 c000 - -110 Previous debian testing version of Linux tom3 2.6.32-5-686 #1 SMP Tue Jun 1 04:59:47 UTC 2010 i686 GNU/Linux, not yet reproduced with current version. You may need to copy the broken periodic workaround code from the PCI_VENDOR_ID_INTEL case in ehci_pci_setup(), drivers/usb/host/ehci-pci.c into the PCI_VENDOR_ID_VIA case. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: xHCI bandwidth error with USB webcam
On Mon, 12 Apr 2010, Sarah Sharp wrote: I've been trying out the patches to enable isochronous transfers under xHCI, and they work fine on my USB speaker. However, I've been having trouble getting my high speed USB webcam to work. The NEC Express Card I have rejects the first alternate setting that the uvcvideo driver tries to install (altsetting 11), saying that it takes too much bandwidth. This happens even when I plug the device directly into the roothub with no other devices plugged in. I would like to know if this is correct behavior for the host, as I can't believe a device would advertise an alternate setting that took up too much bandwidth by itself. Device descriptors and a log snippet are attached. The webcam is a high-speed device, right? The descriptors call for an isochronous-IN endpoint with 3x1020 bytes (period 1 uframe) and an isochronous-IN endpoint with 36 bytes (period 8 uframes). That combination does not exceed the high-speed bandwidth limit, so the controller should accept it. Broadly speaking, at 480 Mb/s you get 60 KB/ms = 7500 B/uframe. The periodic bandwidth limit is 80% of the total, or 6000 B/uframe. Taking bitstuffing into account (6/7) leaves 5142 B/uframe available for data, and overhead eats up some of that. It's still much more than the 3*1020 + 36 = 3096 bytes needed by the webcam. The other problem is that uvcvideo then gives up on the device when installing the alt setting fails, rather than trying the next less resource-intensive alternate setting. The past, submit_urb() might fail if there wasn't enough bandwidth for the isochronous transfers, but under an xHCI host controller, it will fail sooner, when usb_set_interface() is called. That needs to be fixed in all the USB video drivers. I figured out how to patch the gspca driver, but not uvcvideo. The patch looks a bit hackish; can with experience with that driver look it over? Can anyone tell me where to look for the usb_set_interface() in uvcvideo? Not at the moment, sorry. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH/RESEND] soc-camera: add runtime pm support for subdevices
On Mon, 8 Feb 2010, Guennadi Liakhovetski wrote: To save power soc-camera powers subdevices down, when they are not in use, if this is supported by the platform. However, the V4L standard dictates, that video nodes shall preserve configuration between uses. This requires runtime power management, which is implemented by this patch. It allows subdevice drivers to specify their runtime power-management methods, by assigning a type to the video device. Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- I've posted this patch to linux-media earlier, but I'd also like to get comments on linux-pm, sorry to linux-media falks for a duplicate. To explain a bit - soc_camera.c is a management module, that binds video interfaces on SoCs and sensor drivers. The calls, that I am adding to soc_camera.c shall save and restore sensor registers before they are powered down and after they are powered up. This patch is not correct as it stands. If you use runtime PM then the system PM resume method has to be changed. See the discussion in section 6 of Documentation/power/runtime_pm.txt. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH/RESEND] soc-camera: add runtime pm support for subdevices
On Tue, 9 Feb 2010, Guennadi Liakhovetski wrote: On Tue, 9 Feb 2010, Alan Stern wrote: On Mon, 8 Feb 2010, Guennadi Liakhovetski wrote: To save power soc-camera powers subdevices down, when they are not in use, if this is supported by the platform. However, the V4L standard dictates, that video nodes shall preserve configuration between uses. This requires runtime power management, which is implemented by this patch. It allows subdevice drivers to specify their runtime power-management methods, by assigning a type to the video device. Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- I've posted this patch to linux-media earlier, but I'd also like to get comments on linux-pm, sorry to linux-media falks for a duplicate. To explain a bit - soc_camera.c is a management module, that binds video interfaces on SoCs and sensor drivers. The calls, that I am adding to soc_camera.c shall save and restore sensor registers before they are powered down and after they are powered up. This patch is not correct as it stands. If you use runtime PM then the system PM resume method has to be changed. See the discussion in section 6 of Documentation/power/runtime_pm.txt. ...changed - for the same device, right? And I don't see any system PM methods implemented for these devices yet. Oh, if these new routines are just bus glue then you're right, it doesn't matter. The changes would have to be made in the lower-level drivers, when they implement runtime PM. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 14564] New: capture-example sleeping function called from invalid context at arch/x86/mm/fault.c
On Mon, 4 Jan 2010, Sean wrote: Alan Stern wrote: Um, when you say it does the job, what do you mean? It traps the error and prevents the kernel from crashing. As did some of the earlier patches, right? The job it was _intended_ to do was to prove that your problems are caused by hardware errors rather than software bugs. If the patch causes the problems to stop, without printing any error messages in the log, then it does indeed prove this. After all, the only places the patch changes any persistent values are after it prints an error message. It did print out error messages: .ohci_hcd :00:0b.0: Circular hash: 36 c669f900 c677b900 c677b900 ...ohci_hcd :00:0b.0: Circular hash: 36 c669f900 c677b900 c677b900 .ohci_hcd :00:0b.0: Circular hash: 32 c669f800 c677b800 c677b800 Ooh, that's odd. The Circular hash message occurs in the same spot as the Circular pointer #2a message in the previous patch -- and that message never got printed! I noticed that your CPU is a Cyrix. Perhaps it is the culprit. Have you tried running the program on a different computer? Yes, on other computers I don't get this error. Same os image. Though I haven't found a computer with an ohci controller yet. So that's not a real test, unfortunately. Still, at this point I'm not sure it's worthwhile to pursue this any farther. I'm convinced it's a hardware problem. Do you want to continue, or are you happy to switch computers and forget about it? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 14564] New: capture-example sleeping function called from invalid context at arch/x86/mm/fault.c
On Tue, 5 Jan 2010, Sean wrote: Thanks so much for your help Alan. I also think this is definitely a bug in the hardware. Let's leave it at that, I'm happy. Okay. You should mark the Bugzilla report as REJECTED and close it out. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 14564] New: capture-example sleeping function called from invalid context at arch/x86/mm/fault.c
On Mon, 4 Jan 2010, Sean wrote:
Alan Stern wrote:
Try inserting a line saying:
td_check(ohci, hash, #2c);
two lines above the #2b line, i.e., just after the wmb(). That'll help
narrow down the search for the bug.
Alan,
I put the extra line in and ran capture-example twice. This is what I got:
ohci_hcd :00:0b.0: Circular pointer #2c: 32 c6782800 c66a4800 c6782800
ohci_hcd :00:0b.0: Circular pointer #2c: 1 c6782040 c66a4040 c6782040
...
All right. Let's try this patch in place of all the others, then.
Alan Stern
Index: usb-2.6/drivers/usb/host/ohci-q.c
===
--- usb-2.6.orig/drivers/usb/host/ohci-q.c
+++ usb-2.6/drivers/usb/host/ohci-q.c
@@ -505,6 +505,7 @@ td_fill (struct ohci_hcd *ohci, u32 info
struct urb_priv *urb_priv = urb-hcpriv;
int is_iso = info TD_ISO;
int hash;
+ volatile struct td * volatile td1, * volatile td2;
// ASSERT (index urb_priv-length);
@@ -558,11 +559,30 @@ td_fill (struct ohci_hcd *ohci, u32 info
/* hash it for later reverse mapping */
hash = TD_HASH_FUNC (td-td_dma);
+
+ td1 = ohci-td_hash[hash];
+ td2 = NULL;
+ if (td1) {
+ td2 = td1-td_hash;
+ if (td2 == td1 || td2 == td) {
+ ohci_err(ohci, Circular hash: %d %p %p %p\n,
+ hash, td1, td2, td);
+ td2 = td1-td_hash = NULL;
+ }
+ }
+
td-td_hash = ohci-td_hash [hash];
ohci-td_hash [hash] = td;
/* HC might read the TD (or cachelines) right away ... */
wmb ();
+
+ if (td1 td1-td_hash != td2) {
+ ohci_err(ohci, Hash value changed: %d %p %p %p\n,
+ hash, td1, td2, td);
+ td1-td_hash = (struct td *) td2;
+ }
+
td-ed-hwTailP = td-hwNextTD;
}
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bugme-new] [Bug 14564] New: capture-example sleeping function called from invalid context at arch/x86/mm/fault.c
On Mon, 4 Jan 2010, Sean wrote: Alan, This last patch seems to do the job. Thanks so much for your help! Where do I donate/send beer? Um, when you say it does the job, what do you mean? The job it was _intended_ to do was to prove that your problems are caused by hardware errors rather than software bugs. If the patch causes the problems to stop, without printing any error messages in the log, then it does indeed prove this. After all, the only places the patch changes any persistent values are after it prints an error message. (Admittedly, I didn't expect the problem to stop; I expected to get a bunch of messages from the second ohci_err(). Just out of curiosity, does it make any difference if you remove all those volatiles in the declaration line for td1 and td2?) I noticed that your CPU is a Cyrix. Perhaps it is the culprit. Have you tried running the program on a different computer? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bug 14564] capture-example sleeping function called from invalid context at arch/x86/mm/fault.c
On Mon, 4 Jan 2010, Sean wrote: Jef, I upgraded to the latest v4l-dvb from http://linuxtv.org/hg/v4l-dvb, made the kernel modules, made the v4l libraries, and recompiled capture-example.c. Gspca now shows 2.8.0. The error still persists. Alan Stern's latest patch to ohci-q.c traps the error. I think it is an issue with the cpu or usb controller on the Vortex86SX SoC. The CPU is more likely than the USB controller. The erroneous pointer values we see are virtual addresses, whereas the USB controller would probably write a physical (or DMA) address if it was malfunctioning. Or it could be some bizarre timing problem with the memory bus, or something else equally obscure. You didn't mention before that this was a SoC rather than an ordinary PC. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
