Re: [RFC PATCH 1/4] USB: HCD: support giveback of URB in tasklet context
On Sun, Jun 9, 2013 at 11:58 PM, Alan Stern wrote: > On Sun, 9 Jun 2013, Ming Lei wrote: > >> This patch implements the mechanism of giveback of URB in >> tasklet context, so that hardware interrupt handling time for >> usb host controller can be saved much, and HCD interrupt handling >> can be simplified. > > Why do you want to minimize the hardware interrupt handling time? The > total interrupt handling time will actually be increased by your > change; the only advantage is that much of the work will not be in > hardirq context. Disabling interrupts too long will cause system to respond slowly, for example timer interrupt may be handled very lately. That is why tasklet/ softirq is introduced to split driver's interrupt handler into two parts: the urgent thing is done quickly in hard irq context with IRQs disabled, and the remaining is handled in softirq/tasklet part with IRQs enabled. > > Also, I suspect that this will make HCD interrupt handling _more_ > complicated, not less. If HCD's lock wasn't released before calling usb_hcd_giveback_urb(), HCD interrupt handling would have been a bit simpler. > >> Motivations: >> >> 1), on some arch(such as ARM), DMA mapping/unmapping is a bit >> time-consuming, for example: when accessing usb mass storage >> via EHCI on pandaboard, the common length of transfer buffer is 120KB, >> the time consumed on DMA unmapping may reach hundreds of microseconds; >> even on A15 based box, the time is still about scores of microseconds > > DMA mapping/unmapping will remain just as time-consuming as before. > This patch won't change that. Yes, it is platform dependent, and I have tried to improve it, but not succeed. But, is there any good reason to do time-consuming DMA mapping/ unmapping with all IRQs disabled to make system response slowly? > >> 2), on some arch, reading DMA coherent memoery is very time-consuming, >> the most common example is usb video class driver[1] > > Reading DMA-coherent memory will remain just as time-consuming as > before. Same with above. > >> 3), driver's complete() callback may do much things which is driver >> specific, so the time is consumed unnecessarily in hardware irq context. > > With this patch, the time is consumed in softirq context. What is the > advantage? IRQs are enabled in softirq context so that system can respond events which might require to be handled very quickly. > >> 4), running driver's complete() callback in hardware irq context causes >> that host controller driver has to release its lock in interrupt handler, >> so reacquiring the lock after return may busy wait a while and increase >> interrupt handling time. More seriously, releasing the HCD lock makes >> HCD becoming quite complicated to deal with introduced races. > > There is no choice; the lock _has_ to be released before giving back > completed URBs. Therefore the races are unavoidable, as is the > busy-wait. Could you explain it in a bit detail why there is no choice? IMO, the patchset can make it possible. > >> So the patch proposes to run giveback of URB in tasklet context, then >> time consumed in HCD irq handling doesn't depend on drivers' complete and >> DMA mapping/unmapping any more, also we can simplify HCD since the HCD >> lock isn't needed to be released during irq handling. > > I'm not convinced. In particular, I'm not convinced that this is > better than using a threaded interrupt handler. Threaded interrupt handler should be OK but with more context switch cost, so URB giveback may be handled a bit lately and performance might be affected. Also ISOC transfer for video/audio applicaton should be handled with as less latency as possible to provide good user experience, thread handler may be delayed a bit too long to meet the demand. Also there is no any sleep in usb_hcd_giveback_urb(), so tasklet should be better. > >> The patch should be reasonable and doable: >> >> 1), for drivers, they don't care if the complete() is called in hard irq >> context or softirq context > > True. > >> 2), the biggest change is the situation in which usb_submit_urb() is called >> in complete() callback, so the introduced tasklet schedule delay might be a >> con, but it shouldn't be a big deal: >> >> - control/bulk asynchronous transfer isn't sensitive to schedule >> delay > > Mass-storage device access (which uses bulk async transfers) certainly > _is_ sensitive to scheduling delays. > >> - the patch schedules giveback of periodic URBs using >> tasklet_hi_schedule, so the introduced delay should be very >> small >> >> - use percpu tasklset for each HCD to schedule giveback of URB, >> which are running as much as parallel > > That might be an advantage; it depends on the work load. But if the > tasklet ends up running on a different CPU from the task that submitted > the URB originally, it would be less of an advantage. > >> - for ISOC transfer, generally, drivers submit several URBs >>
Re: [RFC PATCH 1/4] USB: HCD: support giveback of URB in tasklet context
On Sunday 09 June 2013 23:18:28 Ming Lei wrote: > 2), the biggest change is the situation in which usb_submit_urb() is called > in complete() callback, so the introduced tasklet schedule delay might be a > con, but it shouldn't be a big deal: > > - control/bulk asynchronous transfer isn't sensitive to schedule > delay That is debatable.Missing a frame boundary is expensive because the increased latency then translates into lower throughput. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 2/4] USB: EHCI: don't release ehci->lock if URB giveback in tasklet context
On Mon, Jun 10, 2013 at 12:06 AM, Alan Stern wrote: > On Sun, 9 Jun 2013, Ming Lei wrote: > >> If HCD_BH is set for HC driver's flags, URB giveback will be >> done in tasklet context instead of interrupt context, so the >> ehci->lock needn't to be released any more before calling >> usb_hcd_giveback_urb(). > > Ah, this is the reason why you don't need to release the private lock. > I'm not sure that this will save much time overall. I did't mention this 3/4 patch can save much time. In fact, without this patch, handling URB giveback still can work. But releasing the private lock can make hard irq handler become random long, since reacquiring the lock may busy wait quite a while until other CPUs released the lock. > > With the existing code, the main reason for lock contention would be if > (for example) CPU 2 submitted or cancelled an URB while the giveback > was occurring on CPU 1. Then CPU 1 would be forced to wait while CPU 2 > finished its submission or cancellation. > > With this patch, it's the other way around. CPU 2 would be forced to > wait while CPU 1 does all the rest of the work in its interrupt > handler. The total time spent holding the lock will be the same; it > will just be distributed differently among the CPUs. Hence it is not > at all clear that this change will save any time. Generally the interrupt handler without URB giveback consumes not much time, and the time consumed is about 10~20us on fast machine, and <40us on slow machine averagely per my tests in 4/4 commit log. You may ague the situation would become worse if there are many URBs to be handled in ehci_irq() on CPU1, but I think it should be very very rare to happen attributed to this patchset: HCD hard interrupt handler takes much less time than general HCD irq interval(for example, in EHCI, generally there won't have many URBs to be completed in one uFrame, and the URBs completed in next uFrame will interrupt CPUs 125us later) Also we may introduce another lock/flags to let CPU1 handle CPU2's interrupts and let CPU2 return IRQ_HANDLED immediately, but I am not sure if it is necessary. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/39] ARM: ux500: Stop passing MMC's platform data for Device Tree boots
On Wed, 15 May 2013, Linus Walleij wrote: > On Wed, May 15, 2013 at 11:51 AM, Lee Jones wrote: > > > It was required to pass DMA channel configuration information to the > > MMC driver before the new DMA API was in place. Now that it is, and > > is fully compatible with Device Tree we can stop doing that. > > > > Reviewed-by: Linus Walleij > > Signed-off-by: Lee Jones > > So since the use of dma_request_slave_channel() is not upstream, > I guess this will break DMA use (i.e slow down transfers!) on all > device tree boots? > > I'd be happy to apply it once the MMCI patch is in linux-next > indicating there may just be a window in the merge period > where it falls back to IRQ mode, but I don't want to disable > DMA on DT boots for an entire kernel cycle just like that. > > Not applied as of yet. I believe it's now okay to apply this. -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/39] ARM: ux500: Move SDI (MMC) and UART devices under more descriptive heading
On Wed, 15 May 2013, Linus Walleij wrote: > On Wed, May 15, 2013 at 11:51 AM, Lee Jones wrote: > > > Now DMA DT bindings exist and are in use by he MMC and UART drivers, it > > should be possible to remove them from the auxdata structure. However, > > after doing so the drivers fail. Common clk is still reliant on the > > dev_name() call to do device name matching, which will fail due to the > > fact that Device Tree naming differs somewhat do the more traditional > > conventions. > > > > Reviewed-by: Linus Walleij > > Signed-off-by: Lee Jones > > Cannot be applied due to dependency on 5/39. I think this can be applied now (if it hasn't already). -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 1/4] USB: HCD: support giveback of URB in tasklet context
On Mon, Jun 10, 2013 at 4:43 PM, Oliver Neukum wrote: > On Sunday 09 June 2013 23:18:28 Ming Lei wrote: >> 2), the biggest change is the situation in which usb_submit_urb() is called >> in complete() callback, so the introduced tasklet schedule delay might be a >> con, but it shouldn't be a big deal: >> >> - control/bulk asynchronous transfer isn't sensitive to schedule >> delay > > That is debatable.Missing a frame boundary is expensive because the increased > latency then translates into lower throughput. Suppose so, considered that bulk transfer will do large data block transfer, and the extra frame or uFrame doesn't matter over the whole transfer time. Also the tasklet function will be scheduled once the hard interrupt handler completes, and the delay is often several microseconds or smaller, which has a very low probability to miss frame/uframe boundary. Even with submitting URBs in hardware interrupt handler, there is still the interrupt handling delay, isn't there? (So disabling interrupt too long is really very bad, :-)) Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 1/4] USB: HCD: support giveback of URB in tasklet context
On Monday 10 June 2013 17:23:46 Ming Lei wrote: > On Mon, Jun 10, 2013 at 4:43 PM, Oliver Neukum wrote: > > On Sunday 09 June 2013 23:18:28 Ming Lei wrote: > >> 2), the biggest change is the situation in which usb_submit_urb() is called > >> in complete() callback, so the introduced tasklet schedule delay might be a > >> con, but it shouldn't be a big deal: > >> > >> - control/bulk asynchronous transfer isn't sensitive to schedule > >> delay > > > > That is debatable.Missing a frame boundary is expensive because the > > increased > > latency then translates into lower throughput. > > Suppose so, considered that bulk transfer will do large data block transfer, > and > the extra frame or uFrame doesn't matter over the whole transfer time. That is not true for all use cases. Networking looks vulnerable. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 1/4] USB: HCD: support giveback of URB in tasklet context
On Mon, Jun 10, 2013 at 5:31 PM, Oliver Neukum wrote: > On Monday 10 June 2013 17:23:46 Ming Lei wrote: >> On Mon, Jun 10, 2013 at 4:43 PM, Oliver Neukum wrote: >> > On Sunday 09 June 2013 23:18:28 Ming Lei wrote: >> >> 2), the biggest change is the situation in which usb_submit_urb() is >> >> called >> >> in complete() callback, so the introduced tasklet schedule delay might be >> >> a >> >> con, but it shouldn't be a big deal: >> >> >> >> - control/bulk asynchronous transfer isn't sensitive to schedule >> >> delay >> > >> > That is debatable.Missing a frame boundary is expensive because the >> > increased >> > latency then translates into lower throughput. >> >> Suppose so, considered that bulk transfer will do large data block transfer, >> and >> the extra frame or uFrame doesn't matter over the whole transfer time. > > That is not true for all use cases. Networking looks vulnerable. > That is debatable.Missing a frame boundary is expensive because the increased > latency then translates into lower throughput. Missing uframe/frame boundary doesn't cause lower throughput since network devices use bulk transfer, which is scheduled in hw aync queue and there should always have pending transfers in the queue when submitting bulk URB to the queue. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/15] Convert to use devm_ioremap_resource
These are the remaining instances of devm_request_and_ioremap. Convert
them to use devm_ioremap_resource as introduced by commit
75096579c3ac ("lib: devres: Introduce devm_ioremap_resource()).
Patches 1 to 13 remove occurrences of devm_request_and_ioremap.
Patch 14 modifies one comment that speaks about devm_request_and_ioremap.
Patch 15 removes the definition of devm_request_and_ioremap which
should only be applied if all other pathces are merged.
Tushar Behera (15):
sparc,leon: Convert to use devm_ioremap_resource
sudmac: Convert to use devm_ioremap_resource
mmc: mvsdio: Convert to use devm_ioremap_resource
gpio_msm: Convert to use devm_ioremap_resource
gpio-sta2x11: Convert to use devm_ioremap_resource
net: bcm63xx_enet: Convert to use devm_ioremap_resource
net: fec: Convert to use devm_ioremap_resource
net: emaclite: Convert to use devm_ioremap_resource
net: can: Convert to use devm_ioremap_resource
Staging: netlogic: Convert to use devm_ioremap_resource
regulator: ti-abb: Convert to use devm_ioremap_resource
ASoC: spear: Convert to use devm_ioremap_resource
pci: mvebu: Convert to use devm_ioremap_resource
usb: phy: rcar-usb: Fix comment w.r.t. devm_ioremap_resource
lib: devres: Remove deprecated devm_request_and_ioremap
Documentation/driver-model/devres.txt |1 -
arch/sparc/kernel/leon_pci_grpci1.c |6 +++---
drivers/dma/sh/sudmac.c |6 +++---
drivers/gpio/gpio-msm-v1.c| 12 +--
drivers/gpio/gpio-sta2x11.c |4 +++-
drivers/mmc/host/mvsdio.c |6 +++---
drivers/net/can/c_can/c_can_platform.c|4 ++--
drivers/net/ethernet/broadcom/bcm63xx_enet.c | 12 +--
drivers/net/ethernet/freescale/fec_main.c | 12 +--
drivers/net/ethernet/xilinx/xilinx_emaclite.c |6 --
drivers/pci/host/pci-mvebu.c |5 +++--
drivers/regulator/ti-abb-regulator.c | 12 +--
drivers/staging/netlogic/xlr_net.c|8 +++
drivers/usb/phy/phy-rcar-usb.c|2 +-
include/linux/device.h|2 --
lib/devres.c | 28 -
sound/soc/spear/spdif_out.c | 20 +-
17 files changed, 55 insertions(+), 91 deletions(-)
CC: alsa-de...@alsa-project.org
CC: de...@driverdev.osuosl.org
CC: linux-...@vger.kernel.org
CC: linux-...@vger.kernel.org
CC: linux-g...@vger.kernel.org
CC: linux-...@vger.kernel.org
CC: linux-...@vger.kernel.org
CC: linux-usb@vger.kernel.org
CC: net...@vger.kernel.org
CC: sparcli...@vger.kernel.org
CC: Bjorn Helgaas
CC: Chris Ball
CC: Dan Williams
CC: "David S. Miller"
CC: Felipe Balbi
CC: Grant Likely
CC: Greg Kroah-Hartman
CC: Liam Girdwood
CC: Linus Walleij
CC: Marc Kleine-Budde
CC: Mark Brown
CC: Michal Simek
CC: Rob Landley
CC: Vinod Koul
CC: Wolfgang Grandegger
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/15] usb: phy: rcar-usb: Fix comment w.r.t. devm_ioremap_resource
Commit 75096579c3ac ("lib: devres: Introduce devm_ioremap_resource()")
introduced devm_ioremap_resource() and deprecated the use of
devm_request_and_ioremap().
Signed-off-by: Tushar Behera
CC: linux-usb@vger.kernel.org
CC: Felipe Balbi
---
drivers/usb/phy/phy-rcar-usb.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/phy/phy-rcar-usb.c b/drivers/usb/phy/phy-rcar-usb.c
index a35681b..23c3dd3 100644
--- a/drivers/usb/phy/phy-rcar-usb.c
+++ b/drivers/usb/phy/phy-rcar-usb.c
@@ -161,7 +161,7 @@ static int rcar_usb_phy_probe(struct platform_device *pdev)
* CAUTION
*
* Because this phy address is also mapped under OHCI/EHCI address area,
-* this driver can't use devm_request_and_ioremap(dev, res) here
+* this driver can't use devm_ioremap_resource(dev, res) here
*/
reg0 = devm_ioremap_nocache(dev, res0->start, resource_size(res0));
reg1 = devm_ioremap_nocache(dev, res1->start, resource_size(res1));
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] usb: phy: rcar-usb: Fix comment w.r.t. devm_ioremap_resource
Hello.
On 10-06-2013 15:35, Tushar Behera wrote:
Commit 75096579c3ac ("lib: devres: Introduce devm_ioremap_resource()")
introduced devm_ioremap_resource() and deprecated the use of
devm_request_and_ioremap().
Signed-off-by: Tushar Behera
CC: linux-usb@vger.kernel.org
CC: Felipe Balbi
---
drivers/usb/phy/phy-rcar-usb.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/phy/phy-rcar-usb.c b/drivers/usb/phy/phy-rcar-usb.c
index a35681b..23c3dd3 100644
--- a/drivers/usb/phy/phy-rcar-usb.c
+++ b/drivers/usb/phy/phy-rcar-usb.c
@@ -161,7 +161,7 @@ static int rcar_usb_phy_probe(struct platform_device *pdev)
* CAUTION
*
* Because this phy address is also mapped under OHCI/EHCI address area,
-* this driver can't use devm_request_and_ioremap(dev, res) here
+* this driver can't use devm_ioremap_resource(dev, res) here
*/
reg0 = devm_ioremap_nocache(dev, res0->start, resource_size(res0));
reg1 = devm_ioremap_nocache(dev, res1->start, resource_size(res1));
I'm completely removing this comment in my series pushed thru
renesas.git, so I advise this patch to be ignored not to create conflict
in the future.
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: chipidea: udc: add force-full-speed option
Michael Grzeschik writes:
> From: Michael Grzeschik
>
> This patch makes it possible to set the chipidea udc
> into full-speed only mode. It can be set by the oftree
> property "force-full-speed".
>
> Signed-off-by: Michael Grzeschik
> Signed-off-by: Marc Kleine-Budde
> ---
> Fixed the compilation issues on x86.
>
> Thanks to Alex,
> Michael
>
>
> Documentation/devicetree/bindings/usb/ci13xxx-imx.txt | 2 ++
> drivers/usb/chipidea/bits.h | 2 ++
> drivers/usb/chipidea/core.c | 6 ++
> 3 files changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index b4b5b79..1943aef 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -18,6 +18,7 @@ Optional properties:
> - vbus-supply: regulator for vbus
> - disable-over-current: disable over current detect
> - external-vbus-divider: enables off-chip resistor divider for Vbus
> +- force-full-speed: limit the maximum connection speed to full-speed
>
> Examples:
> usb@02184000 { /* USB OTG */
> @@ -28,4 +29,5 @@ usb@02184000 { /* USB OTG */
> fsl,usbmisc = <&usbmisc 0>;
> disable-over-current;
> external-vbus-divider;
> + force-full-speed;
> };
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index 93efe4e..4c14ab7 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -49,11 +49,13 @@
> #define PORTSC_HSPBIT(9)
> #define PORTSC_PTC(0x0FUL << 16)
> /* PTS and PTW for non lpm version only */
> +#define PORTSC_PFSC BIT(24)
> #define PORTSC_PTS(d) d) & 0x3) << 30) | (((d) & 0x4) ? BIT(25)
> : 0))
> #define PORTSC_PTWBIT(28)
> #define PORTSC_STSBIT(29)
>
> /* DEVLC */
> +#define DEVLC_PFSCBIT(23)
> #define DEVLC_PSPD(0x03UL << 25)
> #define DEVLC_PSPD_HS (0x02UL << 25)
> #define DEVLC_PTW BIT(27)
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index ae239c7..cf1c9ee 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -65,6 +65,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "ci.h"
> @@ -240,6 +241,11 @@ static void hw_phymode_configure(struct ci13xxx *ci)
> return;
> }
>
> + if (of_property_read_bool(ci->dev->of_node, "force-full-speed")) {
> + portsc |= PORTSC_PFSC;
> + lpm |= DEVLC_PFSC;
> + }
> +
Actually, now that I'm looking at it, it's inconsistent with the rest of
the code. And it also denies the possibility to force fullspeed for
non-OF platforms.
Can you amend it?
Thanks,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Connection to a 3G USB Module using USB-Serial
Thanks Greg for taking som etime to answer. My kernel is an old 2.6.35.7 and migrating it entirely to 3+ one is a real mess (given the number of platfomr customization it includes). So probably the best is to try back-porting the ZTE usb-serial driver... I guess. Is it "drivers/usb/serial/zte_ev.c" what you're referring to? My device is a ZTE MF-210... does the same driver really apply for this device too? Does the same driver rely on some kernel 3.x-only features? I can always give it a *blind* try, of course... but better knowing in advance, if it won't work. On Sat, Jun 8, 2013 at 8:22 PM, Greg KH wrote: > On Thu, May 30, 2013 at 02:54:40PM +0200, Fabio Fumi wrote: >> I'm struggling with this issue since a while and as the author of the >> USB-Serial module I thought experts here might provide some help... As >> it's a USB-serail issue, I've posted on both USB and Serial lists. >> thanks in advance! >> >> The problem is about connecting with a 3G USB modem, a ZTE MF-210 >> mini-pci board; plugged as a daughter board inside an Android tablet. >> I have posted already on a couple forums, but got no useful feedback >> yet. >> >> As soon as I power up the module I get the expected /dev/ttyUSB0-3 and >> that's fine, so far: >> >> <6>usb 1-1.1: new high speed USB device using emxx-ehci-driver and address 3 >> <6>option 1-1.1:1.0: GSM modem (1-port) converter detected >> <6>usb 1-1.1: GSM modem (1-port) converter now attached to ttyUSB0 >> <6>option 1-1.1:1.1: GSM modem (1-port) converter detected >> <6>usb 1-1.1: GSM modem (1-port) converter now attached to ttyUSB1 >> <6>option 1-1.1:1.2: GSM modem (1-port) converter detected >> <6>usb 1-1.1: GSM modem (1-port) converter now attached to ttyUSB2 >> <6>option 1-1.1:1.3: GSM modem (1-port) converter detected >> <6>usb 1-1.1: GSM modem (1-port) converter now attached to ttyUSB3 >> >> Issue is when I try to communicate with any of these ports. I can >> connect using picocom to all but can't receive any feedback from AT >> commands sent over to them. I made some partial progress only after >> using the "-i" option (i.e. "no port init"), but below you can find >> what I get if I simply type A + T + Z + , for example. >> >> As you can see, characters and commands are repeated several times. >> That's what I can't understand. >> >> Without the -i option, I don't even get any feedback from the modem >> and the same happens using other connection programs, e.g. busybox >> microcom. I'm using picocom as it's very small and I succesfully used >> it already in a similar case, though on a different Android platform >> and kernel. >> >> This Kernel is built including standard usb-serial options below: >> >> CONFIG_USB_SERIAL=y >> CONFIG_USB_SERIAL_WWAN=y >> CONFIG_USB_SERIAL_OPTION=y > > What kernel version are you using? > >> Is the kernel missing anything, or requiring some special ZTE-specific >> customizations in your opinion? > > There is a new zte-specific driver in the 3.9 kernel release that should > work for your device. We just got some patches merged that moved the > device id from the option driver to the zte driver, which should resolve > issues like the one you are seeing here (i.e. the wrong driver > controlling the device.) Can you test the 3.10-rc5 kernel that comes > out in a few days, or the next 3.9-stable release 3.9.6 should have the > proper patches)? > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] chipidea: move to pcim_* functions
Andy Shevchenko writes:
> This patch makes error path cleaner and probe function tidier.
...but does not apply to chipidea tree.
> Signed-off-by: Andy Shevchenko
> ---
> It assumes that removal of PCI_D0 and drvdata(NULL) were done previously.
>
> drivers/usb/chipidea/ci13xxx_pci.c | 18 +-
> 1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci13xxx_pci.c
> b/drivers/usb/chipidea/ci13xxx_pci.c
> index 59fab90..c9e9cb3 100644
> --- a/drivers/usb/chipidea/ci13xxx_pci.c
> +++ b/drivers/usb/chipidea/ci13xxx_pci.c
> @@ -49,7 +49,7 @@ static struct ci13xxx_platform_data penwell_pci_platdata = {
> * invokes the udc_probe() method to start the UDC associated with it
> */
> static int ci13xxx_pci_probe(struct pci_dev *pdev,
> -const struct pci_device_id *id)
> + const struct pci_device_id *id)
This does not seem to be relevant to the patch description.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
[Thomas and Steve, please see the question below.] On Mon, 10 Jun 2013, Ming Lei wrote: > On Sun, Jun 9, 2013 at 11:48 PM, Alan Stern wrote: > > On Sun, 9 Jun 2013, Ming Lei wrote: > > > >> Hi, > >> > >> The patchset supports to run giveback of URB in tasklet context, so that > >> DMA unmapping/mapping on transfer buffer and compelte() callback can be > >> run with interrupt enabled, then time of HCD interrupt handler can be > >> saved much. Also this approach may simplify HCD since HCD lock needn't > >> be released any more before calling usb_hcd_giveback_urb(). > > > > That's a lot of material. However, you haven't specifically said > > _what_ you want to accomplish. It sounds like you're trying to > > All IRQs are disabled(locally) in hard interrupt context, but they are > enabled in softirq context, this patchset can decrease the time a lot. That isn't clear. It is documented that USB completion handlers are called with local interrupts disabled. An IRQ arriving before the tasklet starts up might therefore be serviced during the short interval before the tasklet disables local interrupts, but if that happens it would mean that the completion handler would be delayed. In effect, you are prioritizing other IRQs higher than USB completion handlers. Nevertheless, the total time spent with interrupts disabled will remain the same. (There's one other side effect that perhaps you haven't considered. When multiple URBs are addressed to the same endpoint, their completion handlers are called in order of URB completion, which is the same as the order of URB submission unless an URB is cancelled. By delegating the completion handlers to tasklets, and particularly by using per-CPU tasklets, you may violate this behavior.) > > minimize the amount of time spent in hardirq context, but I can't be > > sure that is really what you want. > > > > If it is, this is not a good way to do it. A better way to minimize > > the time spent in hardirq context is to use a threaded interrupt > > handler. But why do you want to minimize this time in the first place? > > Process context switch may have more cost than switching to softirq, > then USB performance may be effected, that is why I choose to use > tasklet. Also now there is no any sleep in URB giveback, so it isn't > necessary to introduce process to handle the giveback or completion. Thomas and Steve, can you comment on this? I do not have a good understanding of the relative benefits/drawbacks of tasklets vs. threaded interrupt handlers. > > The CPU will be still have to use at least as much time as before; the > > only difference is that it will be in softirq or in a high-priority > > thread instead of in hardirq. > > Yes, as I said above, but we can allow other IRQs to be served in > handling URB giveback in softirq context, that is the value of > softirq/tasklet. > Of course, system may have more important things to do during the time. > Generally speaking, for one driver, hard interrupt handler should always > consume less time as possible. As I understand it, the tradeoff between hardirq and softirq is generally unimportant. It does matter a lot in realtime settings -- but the RT kernel effectively makes _every_ interrupt handler threaded, so it won't benefit from this change. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Monday 10 June 2013 10:03:00 Alan Stern wrote: > [Thomas and Steve, please see the question below.] > > On Mon, 10 Jun 2013, Ming Lei wrote: > > That isn't clear. It is documented that USB completion handlers are > called with local interrupts disabled. An IRQ arriving before the > tasklet starts up might therefore be serviced during the short interval > before the tasklet disables local interrupts, but if that happens it > would mean that the completion handler would be delayed. That is what tasklets do by definition, isn't it? > In effect, you are prioritizing other IRQs higher than USB completion > handlers. Nevertheless, the total time spent with interrupts disabled > will remain the same. It pobably slightly increases. You have colder caches twice. And potentially you swich CPUs. > (There's one other side effect that perhaps you haven't considered. > When multiple URBs are addressed to the same endpoint, their completion > handlers are called in order of URB completion, which is the same as > the order of URB submission unless an URB is cancelled. By delegating > the completion handlers to tasklets, and particularly by using per-CPU > tasklets, you may violate this behavior.) This is quite serious. It mustn't happen. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] chipidea: move to pcim_* functions
This patch makes error path cleaner and probe function tidier.
Signed-off-by: Andy Shevchenko
---
drivers/usb/chipidea/ci13xxx_pci.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/chipidea/ci13xxx_pci.c
b/drivers/usb/chipidea/ci13xxx_pci.c
index 59fab90..7cf5495 100644
--- a/drivers/usb/chipidea/ci13xxx_pci.c
+++ b/drivers/usb/chipidea/ci13xxx_pci.c
@@ -61,14 +61,13 @@ static int ci13xxx_pci_probe(struct pci_dev *pdev,
return -ENODEV;
}
- retval = pci_enable_device(pdev);
+ retval = pcim_enable_device(pdev);
if (retval)
- goto done;
+ return retval;
if (!pdev->irq) {
dev_err(&pdev->dev, "No IRQ, check BIOS/PCI setup!");
- retval = -ENODEV;
- goto disable_device;
+ return -ENODEV;
}
pci_set_master(pdev);
@@ -84,18 +83,12 @@ static int ci13xxx_pci_probe(struct pci_dev *pdev,
plat_ci = ci13xxx_add_device(&pdev->dev, res, nres, platdata);
if (IS_ERR(plat_ci)) {
dev_err(&pdev->dev, "ci13xxx_add_device failed!\n");
- retval = PTR_ERR(plat_ci);
- goto disable_device;
+ return PTR_ERR(plat_ci);
}
pci_set_drvdata(pdev, plat_ci);
return 0;
-
- disable_device:
- pci_disable_device(pdev);
- done:
- return retval;
}
/**
@@ -111,7 +104,6 @@ static void ci13xxx_pci_remove(struct pci_dev *pdev)
struct platform_device *plat_ci = pci_get_drvdata(pdev);
ci13xxx_remove_device(plat_ci);
- pci_disable_device(pdev);
}
/**
--
1.8.2.rc0.22.gb3600c3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] chipidea: move to pcim_* functions
On Mon, 2013-06-10 at 17:01 +0300, Alexander Shishkin wrote: > Andy Shevchenko writes: > > > This patch makes error path cleaner and probe function tidier. > > ...but does not apply to chipidea tree. > > It assumes that removal of PCI_D0 and drvdata(NULL) were done previously. This perhaps explained why. I just sent v2. -- Andy Shevchenko Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] chipidea: remove superfluous pci_set_drvdata(pci, NULL)
As drvdata is cleared to NULL at probe failure or at removal by the driver core, we don't have to call pci_set_drvdata(pci, NULL) any longer in each driver. Signed-off-by: Andy Shevchenko --- drivers/usb/chipidea/ci13xxx_pci.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/usb/chipidea/ci13xxx_pci.c b/drivers/usb/chipidea/ci13xxx_pci.c index a2a6ac8..59fab90 100644 --- a/drivers/usb/chipidea/ci13xxx_pci.c +++ b/drivers/usb/chipidea/ci13xxx_pci.c @@ -111,7 +111,6 @@ static void ci13xxx_pci_remove(struct pci_dev *pdev) struct platform_device *plat_ci = pci_get_drvdata(pdev); ci13xxx_remove_device(plat_ci); - pci_set_drvdata(pdev, NULL); pci_disable_device(pdev); } -- 1.8.2.rc0.22.gb3600c3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] chipidea: move to pcim_* functions
Andy Shevchenko writes: > This patch makes error path cleaner and probe function tidier. > > Signed-off-by: Andy Shevchenko Applied and pushed both, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/7] USB: add devicetree helpers for determining dr_mode and phy_type
Michael Grzeschik writes: > changes since v6: > > - added patch descriptions where missing > - added Felipes Acked-by > - fixed wording in patch descriptions > - moved hw_phymode_configure to hw_device_reset > - added force-full-speed of property > - fixed devm_usb_get_phy_by_phandle result error handling > > Michael Grzeschik (4): > USB: add devicetree helpers for determining dr_mode and phy_type > USB: chipidea: add PTW, PTS and STS handling > USB: chipidea: ci13xxx-imx: move static pdata into probe function > usb: chipidea: udc: add force-full-speed option > > Philipp Zabel (1): > usb: chipidea: usbmisc: use module_platform_driver > > Sascha Hauer (2): > USB chipidea: introduce dual role mode pdata flags > USB chipidea i.MX: use devm_usb_get_phy_by_phandle to get phy Applied and pushed everything except for force-full-speed patch. Fixed a few warnings on the way, nothing tragic. Thanks, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Mon, 10 Jun 2013, Oliver Neukum wrote: > On Monday 10 June 2013 10:03:00 Alan Stern wrote: > > [Thomas and Steve, please see the question below.] > > > > On Mon, 10 Jun 2013, Ming Lei wrote: > > > > > That isn't clear. It is documented that USB completion handlers are > > called with local interrupts disabled. An IRQ arriving before the > > tasklet starts up might therefore be serviced during the short interval > > before the tasklet disables local interrupts, but if that happens it > > would mean that the completion handler would be delayed. > > That is what tasklets do by definition, isn't it? Yes. Although tasklets in general have no reason to leave interrupts disabled. > > In effect, you are prioritizing other IRQs higher than USB completion > > handlers. Nevertheless, the total time spent with interrupts disabled > > will remain the same. > > It pobably slightly increases. You have colder caches twice. > And potentially you swich CPUs. Actually, the situation is a little better in one respect, which was pointed out earlier but not emphasized. The DMA unmapping can be deferred to the tasklet, and it can run with interrupts enabled before the completion hanlder is called. Since the unmapping sometimes takes a while to run, this is an advantage. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Mon, Jun 10, 2013 at 10:03 PM, Alan Stern wrote: > [Thomas and Steve, please see the question below.] > > On Mon, 10 Jun 2013, Ming Lei wrote: > >> On Sun, Jun 9, 2013 at 11:48 PM, Alan Stern >> wrote: >> > On Sun, 9 Jun 2013, Ming Lei wrote: >> > >> >> Hi, >> >> >> >> The patchset supports to run giveback of URB in tasklet context, so that >> >> DMA unmapping/mapping on transfer buffer and compelte() callback can be >> >> run with interrupt enabled, then time of HCD interrupt handler can be >> >> saved much. Also this approach may simplify HCD since HCD lock needn't >> >> be released any more before calling usb_hcd_giveback_urb(). >> > >> > That's a lot of material. However, you haven't specifically said >> > _what_ you want to accomplish. It sounds like you're trying to >> >> All IRQs are disabled(locally) in hard interrupt context, but they are >> enabled in softirq context, this patchset can decrease the time a lot. > > That isn't clear. It is documented that USB completion handlers are > called with local interrupts disabled. An IRQ arriving before the Is there any good reason to do URB giveback with interrupt disabled? IMO, disabling IRQs isn't necessary for completing URB since the complete() of URBs for same endpoint is reentrant. > tasklet starts up might therefore be serviced during the short interval > before the tasklet disables local interrupts, but if that happens it Tasklet doesn't disable local interrupts. > would mean that the completion handler would be delayed. Yes, the tasklet schedule delay is introduced to URB completion, but the delay doesn't have much side effect about completing URB. For async transfer, generally, it should have no effect since there should have URBs queued on the qh queue before submitting URB. For periodic transfer, the patch uses high priority tasklet to schedule it. > > In effect, you are prioritizing other IRQs higher than USB completion > handlers. Both other IRQs and HCD's IRQ are prioritizing the URB completion. > Nevertheless, the total time spent with interrupts disabled > will remain the same. No, the total time spent with interrupts disabled does not remain same, since no local IRQs are disabled during URB giveback anymore. > > (There's one other side effect that perhaps you haven't considered. > When multiple URBs are addressed to the same endpoint, their completion > handlers are called in order of URB completion, which is the same as > the order of URB submission unless an URB is cancelled. By delegating > the completion handlers to tasklets, and particularly by using per-CPU > tasklets, you may violate this behavior.) Even though URB giveback is handled by hard interrupt handler, it still can't guarantee that 100%, please see below: - interrupt for URB A comes in CPU 0, and handled, then HCD private lock is released, and usb_hcd_giveback_urb() is called to complete URB A - interrupt for URB B comes in CPU 1 just before CPU0 releases HCD private lock, so CPU1 holds the private lock when CPU 0 releases the lock and handles the HCD interrupt. So now just like two CPUs are racing, if CPU0 wins, URB A is completed first, otherwise URB B is completed first. With introducing completing URB in percpu tasklet, the situation is just very similar. On ehci hcd, it can make sure that by always letting CPU0 complete all URBs (use ehci->scanning/ehci->need_rescan flag), but the patch can change percpu tasklet to tasklet to meet the demand. > >> > minimize the amount of time spent in hardirq context, but I can't be >> > sure that is really what you want. >> > >> > If it is, this is not a good way to do it. A better way to minimize >> > the time spent in hardirq context is to use a threaded interrupt >> > handler. But why do you want to minimize this time in the first place? >> >> Process context switch may have more cost than switching to softirq, >> then USB performance may be effected, that is why I choose to use >> tasklet. Also now there is no any sleep in URB giveback, so it isn't >> necessary to introduce process to handle the giveback or completion. > > Thomas and Steve, can you comment on this? I do not have a good > understanding of the relative benefits/drawbacks of tasklets vs. > threaded interrupt handlers. > >> > The CPU will be still have to use at least as much time as before; the >> > only difference is that it will be in softirq or in a high-priority >> > thread instead of in hardirq. >> >> Yes, as I said above, but we can allow other IRQs to be served in >> handling URB giveback in softirq context, that is the value of >> softirq/tasklet. >> Of course, system may have more important things to do during the time. >> Generally speaking, for one driver, hard interrupt handler should always >> consume less time as possible. > > As I understand it, the tradeoff between hardirq and softirq is > generally unimportant. It does matter a lot in realtime settings -- I don't think so, and obviously softirq allows IRQs to
Re: [PATCH] usb: chipidea: udc: add force-full-speed option
Hi,
On Mon, Jun 10, 2013 at 04:52:06PM +0300, Alexander Shishkin wrote:
> Michael Grzeschik writes:
>
> > From: Michael Grzeschik
> >
> > This patch makes it possible to set the chipidea udc
> > into full-speed only mode. It can be set by the oftree
> > property "force-full-speed".
> >
> > Signed-off-by: Michael Grzeschik
> > Signed-off-by: Marc Kleine-Budde
> > ---
> > Fixed the compilation issues on x86.
> >
> > Thanks to Alex,
> > Michael
> >
> >
> > Documentation/devicetree/bindings/usb/ci13xxx-imx.txt | 2 ++
> > drivers/usb/chipidea/bits.h | 2 ++
> > drivers/usb/chipidea/core.c | 6 ++
> > 3 files changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > index b4b5b79..1943aef 100644
> > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -18,6 +18,7 @@ Optional properties:
> > - vbus-supply: regulator for vbus
> > - disable-over-current: disable over current detect
> > - external-vbus-divider: enables off-chip resistor divider for Vbus
> > +- force-full-speed: limit the maximum connection speed to full-speed
> >
> > Examples:
> > usb@02184000 { /* USB OTG */
> > @@ -28,4 +29,5 @@ usb@02184000 { /* USB OTG */
> > fsl,usbmisc = <&usbmisc 0>;
> > disable-over-current;
> > external-vbus-divider;
> > + force-full-speed;
> > };
> > diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> > index 93efe4e..4c14ab7 100644
> > --- a/drivers/usb/chipidea/bits.h
> > +++ b/drivers/usb/chipidea/bits.h
> > @@ -49,11 +49,13 @@
> > #define PORTSC_HSPBIT(9)
> > #define PORTSC_PTC(0x0FUL << 16)
> > /* PTS and PTW for non lpm version only */
> > +#define PORTSC_PFSC BIT(24)
> > #define PORTSC_PTS(d) d) & 0x3) << 30) | (((d) & 0x4) ?
> > BIT(25) : 0))
> > #define PORTSC_PTWBIT(28)
> > #define PORTSC_STSBIT(29)
> >
> > /* DEVLC */
> > +#define DEVLC_PFSCBIT(23)
> > #define DEVLC_PSPD(0x03UL << 25)
> > #define DEVLC_PSPD_HS (0x02UL << 25)
> > #define DEVLC_PTW BIT(27)
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index ae239c7..cf1c9ee 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -65,6 +65,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "ci.h"
> > @@ -240,6 +241,11 @@ static void hw_phymode_configure(struct ci13xxx *ci)
> > return;
> > }
> >
> > + if (of_property_read_bool(ci->dev->of_node, "force-full-speed")) {
> > + portsc |= PORTSC_PFSC;
> > + lpm |= DEVLC_PFSC;
> > + }
> > +
>
> Actually, now that I'm looking at it, it's inconsistent with the rest of
> the code. And it also denies the possibility to force fullspeed for
> non-OF platforms.
>
> Can you amend it?
Yes, i will rework it. I somehow also didn't like the way that hunk
looks like. Beside that, thanks for applying this and the multitd series.
Michael
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] USB: pl2303: fix device initialisation at open
Do not use uninitialised termios data to determine when to configure the
device at open.
This also prevents stack data from leaking to userspace in the OOM error
path.
Cc: sta...@vger.kernel.org
Signed-off-by: Johan Hovold
---
drivers/usb/serial/pl2303.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 7151659..048cd44 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -284,7 +284,7 @@ static void pl2303_set_termios(struct tty_struct *tty,
serial settings even to the same values as before. Thus
we actually need to filter in this specific case */
- if (!tty_termios_hw_change(&tty->termios, old_termios))
+ if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
return;
cflag = tty->termios.c_cflag;
@@ -293,7 +293,8 @@ static void pl2303_set_termios(struct tty_struct *tty,
if (!buf) {
dev_err(&port->dev, "%s - out of memory.\n", __func__);
/* Report back no change occurred */
- tty->termios = *old_termios;
+ if (old_termios)
+ tty->termios = *old_termios;
return;
}
@@ -433,7 +434,7 @@ static void pl2303_set_termios(struct tty_struct *tty,
control = priv->line_control;
if ((cflag & CBAUD) == B0)
priv->line_control &= ~(CONTROL_DTR | CONTROL_RTS);
- else if ((old_termios->c_cflag & CBAUD) == B0)
+ else if (old_termios && (old_termios->c_cflag & CBAUD) == B0)
priv->line_control |= (CONTROL_DTR | CONTROL_RTS);
if (control != priv->line_control) {
control = priv->line_control;
@@ -492,7 +493,6 @@ static void pl2303_close(struct usb_serial_port *port)
static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
{
- struct ktermios tmp_termios;
struct usb_serial *serial = port->serial;
struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
int result;
@@ -508,7 +508,7 @@ static int pl2303_open(struct tty_struct *tty, struct
usb_serial_port *port)
/* Setup termios */
if (tty)
- pl2303_set_termios(tty, port, &tmp_termios);
+ pl2303_set_termios(tty, port, NULL);
result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
if (result) {
--
1.8.2.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] USB: f81232: fix device initialisation at open
Do not use uninitialised termios data to determine when to configure the
device at open.
This also prevents stack data from leaking to userspace.
Cc: sta...@vger.kernel.org
Signed-off-by: Johan Hovold
---
drivers/usb/serial/f81232.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 090b411..7d8dd5a 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -165,11 +165,12 @@ static void f81232_set_termios(struct tty_struct *tty,
/* FIXME - Stubbed out for now */
/* Don't change anything if nothing has changed */
- if (!tty_termios_hw_change(&tty->termios, old_termios))
+ if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
return;
/* Do the real work here... */
- tty_termios_copy_hw(&tty->termios, old_termios);
+ if (old_termios)
+ tty_termios_copy_hw(&tty->termios, old_termios);
}
static int f81232_tiocmget(struct tty_struct *tty)
@@ -187,12 +188,11 @@ static int f81232_tiocmset(struct tty_struct *tty,
static int f81232_open(struct tty_struct *tty, struct usb_serial_port *port)
{
- struct ktermios tmp_termios;
int result;
/* Setup termios */
if (tty)
- f81232_set_termios(tty, port, &tmp_termios);
+ f81232_set_termios(tty, port, NULL);
result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
if (result) {
--
1.8.2.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] USB: spcp8x5: fix device initialisation at open
Do not use uninitialised termios data to determine when to configure the
device at open.
Cc: sta...@vger.kernel.org
Signed-off-by: Johan Hovold
---
drivers/usb/serial/spcp8x5.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/spcp8x5.c b/drivers/usb/serial/spcp8x5.c
index cf3df79..ddf6c47 100644
--- a/drivers/usb/serial/spcp8x5.c
+++ b/drivers/usb/serial/spcp8x5.c
@@ -291,7 +291,6 @@ static void spcp8x5_set_termios(struct tty_struct *tty,
struct spcp8x5_private *priv = usb_get_serial_port_data(port);
unsigned long flags;
unsigned int cflag = tty->termios.c_cflag;
- unsigned int old_cflag = old_termios->c_cflag;
unsigned short uartdata;
unsigned char buf[2] = {0, 0};
int baud;
@@ -299,15 +298,15 @@ static void spcp8x5_set_termios(struct tty_struct *tty,
u8 control;
/* check that they really want us to change something */
- if (!tty_termios_hw_change(&tty->termios, old_termios))
+ if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
return;
/* set DTR/RTS active */
spin_lock_irqsave(&priv->lock, flags);
control = priv->line_control;
- if ((old_cflag & CBAUD) == B0) {
+ if (old_termios && (old_termios->c_cflag & CBAUD) == B0) {
priv->line_control |= MCR_DTR;
- if (!(old_cflag & CRTSCTS))
+ if (!(old_termios->c_cflag & CRTSCTS))
priv->line_control |= MCR_RTS;
}
if (control != priv->line_control) {
@@ -394,7 +393,6 @@ static void spcp8x5_set_termios(struct tty_struct *tty,
static int spcp8x5_open(struct tty_struct *tty, struct usb_serial_port *port)
{
- struct ktermios tmp_termios;
struct usb_serial *serial = port->serial;
struct spcp8x5_private *priv = usb_get_serial_port_data(port);
int ret;
@@ -411,7 +409,7 @@ static int spcp8x5_open(struct tty_struct *tty, struct
usb_serial_port *port)
spcp8x5_set_ctrl_line(port, priv->line_control);
if (tty)
- spcp8x5_set_termios(tty, port, &tmp_termios);
+ spcp8x5_set_termios(tty, port, NULL);
port->port.drain_delay = 256;
--
1.8.2.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] USB: serial: more fixes for v3.10
Here's three more fixes for v3.10 that fix set_termios being called with uninitialised data at open. Johan Johan Hovold (3): USB: f81232: fix device initialisation at open USB: pl2303: fix device initialisation at open USB: spcp8x5: fix device initialisation at open drivers/usb/serial/f81232.c | 8 drivers/usb/serial/pl2303.c | 10 +- drivers/usb/serial/spcp8x5.c | 10 -- 3 files changed, 13 insertions(+), 15 deletions(-) -- 1.8.2.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: chipidea: udc: add force-full-speed option
Michael Grzeschik writes:
> Hi,
>
> On Mon, Jun 10, 2013 at 04:52:06PM +0300, Alexander Shishkin wrote:
>> Michael Grzeschik writes:
>>
>> > From: Michael Grzeschik
>> >
>> > This patch makes it possible to set the chipidea udc
>> > into full-speed only mode. It can be set by the oftree
>> > property "force-full-speed".
>> >
>> > Signed-off-by: Michael Grzeschik
>> > Signed-off-by: Marc Kleine-Budde
>> > ---
>> > Fixed the compilation issues on x86.
>> >
>> > Thanks to Alex,
>> > Michael
>> >
>> >
>> > Documentation/devicetree/bindings/usb/ci13xxx-imx.txt | 2 ++
>> > drivers/usb/chipidea/bits.h | 2 ++
>> > drivers/usb/chipidea/core.c | 6 ++
>> > 3 files changed, 10 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> > index b4b5b79..1943aef 100644
>> > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> > @@ -18,6 +18,7 @@ Optional properties:
>> > - vbus-supply: regulator for vbus
>> > - disable-over-current: disable over current detect
>> > - external-vbus-divider: enables off-chip resistor divider for Vbus
>> > +- force-full-speed: limit the maximum connection speed to full-speed
>> >
>> > Examples:
>> > usb@02184000 { /* USB OTG */
>> > @@ -28,4 +29,5 @@ usb@02184000 { /* USB OTG */
>> >fsl,usbmisc = <&usbmisc 0>;
>> >disable-over-current;
>> >external-vbus-divider;
>> > + force-full-speed;
>> > };
>> > diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
>> > index 93efe4e..4c14ab7 100644
>> > --- a/drivers/usb/chipidea/bits.h
>> > +++ b/drivers/usb/chipidea/bits.h
>> > @@ -49,11 +49,13 @@
>> > #define PORTSC_HSPBIT(9)
>> > #define PORTSC_PTC(0x0FUL << 16)
>> > /* PTS and PTW for non lpm version only */
>> > +#define PORTSC_PFSC BIT(24)
>> > #define PORTSC_PTS(d) d) & 0x3) << 30) | (((d) & 0x4) ?
>> > BIT(25) : 0))
>> > #define PORTSC_PTWBIT(28)
>> > #define PORTSC_STSBIT(29)
>> >
>> > /* DEVLC */
>> > +#define DEVLC_PFSCBIT(23)
>> > #define DEVLC_PSPD(0x03UL << 25)
>> > #define DEVLC_PSPD_HS (0x02UL << 25)
>> > #define DEVLC_PTW BIT(27)
>> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> > index ae239c7..cf1c9ee 100644
>> > --- a/drivers/usb/chipidea/core.c
>> > +++ b/drivers/usb/chipidea/core.c
>> > @@ -65,6 +65,7 @@
>> > #include
>> > #include
>> > #include
>> > +#include
>> > #include
>> >
>> > #include "ci.h"
>> > @@ -240,6 +241,11 @@ static void hw_phymode_configure(struct ci13xxx *ci)
>> >return;
>> >}
>> >
>> > + if (of_property_read_bool(ci->dev->of_node, "force-full-speed")) {
>> > + portsc |= PORTSC_PFSC;
>> > + lpm |= DEVLC_PFSC;
>> > + }
>> > +
>>
>> Actually, now that I'm looking at it, it's inconsistent with the rest of
>> the code. And it also denies the possibility to force fullspeed for
>> non-OF platforms.
>>
>> Can you amend it?
>
> Yes, i will rework it. I somehow also didn't like the way that hunk
> looks like. Beside that, thanks for applying this and the multitd series.
Thanks for *making* the improvements!
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Serial ports for Septentrio USB GNSS receiver
Hello Greg, thanks for getting back to me! I'm using a http://www.septentrio.com/products/receivers/asterx2i-oem INS/GNSS receiver. When connected via USB to a windows-host, there's two (or even three, don't remember exactly) virtual serial ports to talk to it. Why do you want to talk to the other ports? What is on those connections? They're all serial ports, used to issue commands to the receiver (in binary or ascii form) and receive data at different intervals (again, binary or ascii). Which messages exactly are accepted or sent on which (virtual) port at which interval can be configured. >> When connected to a 3.8.0-19-generic #30-Ubuntu SMP i686 GNU/Linux >> system, I only see /dev/ttyACM0. I'd really like to get access to >> the second virtual com port, so I tried setting > > Why? These receivers are rather complex systems and use of multiple (virtual) serial ports is often necessary. For example, to use a very precise positioning mode like differential GPS or even RTK (Real-Time-Kinematic GPS), differential correction data has to be streamed to the receiver (from another, stationary GNSS receiver). This MUST be done on a separate port, because otherwise the receiver is unable to discriminate command input from differential correction input. It also is often convenient to access the same receiver from multiple processes using different ports. In essence, I'd simply like to be able to use any or all of /dev/ttyACM* to talk to the device. >> [lsusb output] Even though I don't really understand the interfaces and endpoints, I get the impression there's multiple CDC-Data interfaces. Yes, there are, but the cdc-acm driver binds to them all (you can see this by running 'usbdevices' from the usbutils package. T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 2 Spd=12 MxCh= 0 D: Ver= 1.10 Cls=02(commc) Sub=00 Prot=00 MxPS= 8 #Cfgs= 2 P: Vendor=152a ProdID=8230 Rev=01.10 S: Manufacturer=Septentrio S: Product=Septentrio USB Device C: #Ifs= 2 Cfg#= 1 Atr=c0 MxPwr=2mA I: If#= 0 Alt= 0 #EPs= 1 Cls=02(commc) Sub=02 Prot=01 Driver=cdc_acm I: If#= 1 Alt= 0 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_acm I can confirm :) How can this be changed? How can I use them? The network should connect to the device just fine through the single port, right? I don't understand, which network are you referring to? There is no network connection for this device. Also, is there a way to influence the latency? As I'm using the receiver's data for flight control, I'd prefer to achieve low latency in data transfer from receiver->host on at least one port. I see the Transfer Type for most endpoints is bulk, not interrupt, but maybe there's a few tricks to make bulk-transfers faster? Right now I'm seeing latencies between 5 and 70ms. There is no difference in "speed" or "latency" between the bulk and interrupt endpoints, it just is how the data is transferred from the USB device (they really are the same, just that interrupt is smaller packets more often, and bulk is larger packets whenever there is some data.) USB devices are well known for having latency issues, that's due to the cheap chip in them, plus the usb protocol overhead, plus whatever is on the other side of the USB device sending data. It almost never is the host OS issue here. Thanks for the explanation! Meanwhile I've also realized that 95% of the packets I receive are exactly 60 bytes in length, and sometimes (not always) I see latencies similar to this (in milliseconds): 34 6 44 5 51 8 20 7 So I could also imagine that there is a buffer (probably of 61bytes) that only gets flushed once its full. Is there, by any chance, a 64 byte buffer in the cdc_acm module or linux USB code? cheers, ben -- Ben Adler Universität Hamburg MIN-Fakultät FB Informatik, AB TAMS Vogt-Kölln-Strasse 30 22527 Hamburg Tel +49 40 42883 2504 Fax +49 40 42883 2397 Mob +49 160 858 0660 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Connection to a 3G USB Module using USB-Serial
On Mon, Jun 10, 2013 at 03:58:38PM +0200, Fabio Fumi wrote: > Thanks Greg for taking som etime to answer. > > My kernel is an old 2.6.35.7 and migrating it entirely to 3+ one is a > real mess (given the number of platfomr customization it includes). Ick, really old obsolete kernels like that one are not good for trying to run new hardware with. I suggest contacting the company that is forcing you to stick with that version, and getting support from them to do the needed backporting of this driver. > So probably the best is to try back-porting the ZTE usb-serial > driver... I guess. Is it "drivers/usb/serial/zte_ev.c" what you're > referring to? My device is a ZTE MF-210... does the same driver > really apply for this device too? I think it does, what is the device/vendor id of it? > Does the same driver rely on some kernel 3.x-only features? Yes, all drivers are tied tightly to the specific kernel version they are released with. > I can always give it a *blind* try, of course... but better knowing in > advance, if it won't work. You can try to backport it, it shouldn't be that hard, but your really going to be on your own here, sorry. best of luck, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Serial ports for Septentrio USB GNSS receiver
On Mon, Jun 10, 2013 at 06:56:30PM +0200, Ben Adler wrote: > Hello Greg, > > thanks for getting back to me! > > >>I'm using a > >>http://www.septentrio.com/products/receivers/asterx2i-oem INS/GNSS > >>receiver. When connected via USB to a windows-host, there's two (or > >>even three, don't remember exactly) virtual serial ports to talk to > >>it. > > > >Why do you want to talk to the other ports? What is on those > >connections? > > They're all serial ports, used to issue commands to the receiver (in > binary or ascii form) and receive data at different intervals > (again, binary or ascii). Which messages exactly are accepted or > sent on which (virtual) port at which interval can be configured. That's not what the USB device descriptors you provided below show. They look like a "normal" cdc-acm device, so we are (according to the USB-IF specification), supposed to export to userspace a single device that can be used to communicate with it. > >> When connected to a 3.8.0-19-generic #30-Ubuntu SMP i686 GNU/Linux > >> system, I only see /dev/ttyACM0. I'd really like to get access to > >> the second virtual com port, so I tried setting > > > > Why? > > These receivers are rather complex systems and use of multiple > (virtual) serial ports is often necessary. For example, to use a > very precise positioning mode like differential GPS or even RTK > (Real-Time-Kinematic GPS), differential correction data has to be > streamed to the receiver (from another, stationary GNSS receiver). > This MUST be done on a separate port, because otherwise the receiver > is unable to discriminate command input from differential correction > input. Odd, that's not what a cdc-acm device is supposed to be for, but oh well :) But, again, the device isn't exposing multiple devices to the operating system at all, that must be something Windows is doing with a custom driver for the device. > It also is often convenient to access the same receiver from > multiple processes using different ports. Multiple processes can access the same device port just fine, that's how they should always work (although if you are doing command/response type things, it can get messy with multiple applications.) > In essence, I'd simply like to be able to use any or all of > /dev/ttyACM* to talk to the device. You are getting the "one" port to talk to the device properly already, but it seems it is lying to the OS about what it really is :( > >> [lsusb output] > >>Even though I don't really understand the interfaces and endpoints, > >>I get the impression there's multiple CDC-Data interfaces. > > > >Yes, there are, but the cdc-acm driver binds to them all (you can see > >this by running 'usbdevices' from the usbutils package. > > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 2 Spd=12 MxCh= 0 > D: Ver= 1.10 Cls=02(commc) Sub=00 Prot=00 MxPS= 8 #Cfgs= 2 > P: Vendor=152a ProdID=8230 Rev=01.10 > S: Manufacturer=Septentrio > S: Product=Septentrio USB Device > C: #Ifs= 2 Cfg#= 1 Atr=c0 MxPwr=2mA > I: If#= 0 Alt= 0 #EPs= 1 Cls=02(commc) Sub=02 Prot=01 Driver=cdc_acm > I: If#= 1 Alt= 0 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_acm > > I can confirm :) How can this be changed? > > >>How can I use them? > > > >The network should connect to the device just fine through the single > >port, right? > > I don't understand, which network are you referring to? There is no > network connection for this device. Ok, my mistake, I thought this was a network device. You're going to have to possibly unbind the cdc-acm driver from the device, and either talk directly to it using libusb, or write a custom kernel driver to expose the different "ports" properly. I would suggest contacting the company to get the specs on how to talk to the device in order to be able to do this. > >>Also, is there a way to influence the latency? As I'm using the > >>receiver's data for flight control, I'd prefer to achieve low > >>latency in data transfer from receiver->host on at least one port. I > >>see the Transfer Type for most endpoints is bulk, not interrupt, but > >>maybe there's a few tricks to make bulk-transfers faster? Right now > >>I'm seeing latencies between 5 and 70ms. > > > >There is no difference in "speed" or "latency" between the bulk and > >interrupt endpoints, it just is how the data is transferred from the USB > >device (they really are the same, just that interrupt is smaller packets > >more often, and bulk is larger packets whenever there is some data.) > > > >USB devices are well known for having latency issues, that's due to the > >cheap chip in them, plus the usb protocol overhead, plus whatever is on > >the other side of the USB device sending data. It almost never is the > >host OS issue here. > > Thanks for the explanation! Meanwhile I've also realized that 95% of > the packets I receive are exactly 60 bytes in length, and sometimes > (not always) I see latencies similar to this (in milliseconds): > > 34 6 44 5 51 8 20 7 >
Re: [RFC PATCH 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Mon, 10 Jun 2013, Ming Lei wrote: > > That isn't clear. It is documented that USB completion handlers are > > called with local interrupts disabled. An IRQ arriving before the > > Is there any good reason to do URB giveback with interrupt disabled? That's a good question. Offhand I can't think of any drivers that rely on this -- although there certainly are places where callback routines use spin_lock() rather than spin_lock_irq() or spin_lock_irqsave(), because they know that interrupts are already disabled. However, changing a documented API is not something to be done lightly. You would have to audit _every_ USB driver to make sure no trouble would arise. > IMO, disabling IRQs isn't necessary for completing URB since the > complete() of URBs for same endpoint is reentrant. Whether it is _necessary_ isn't the point. It is _documented_, and therefore it cannot be changed easily. > > tasklet starts up might therefore be serviced during the short interval > > before the tasklet disables local interrupts, but if that happens it > > Tasklet doesn't disable local interrupts. It is documented that interrupts will be disabled while the completion handler runs. Therefore the tasklet _must_ disable local interrupts. > > would mean that the completion handler would be delayed. > > Yes, the tasklet schedule delay is introduced to URB completion, but > the delay doesn't have much side effect about completing URB. What about delays that occur because a separate interrupt is serviced before the URB's completion handler is called? With the current code that can't happen, but with your scheme it can. > For async transfer, generally, it should have no effect since there should > have URBs queued on the qh queue before submitting URB. That's not how the Bulk-Only mass-storage protocol works. There are times when the protocol _requires_ bulk packets not to be submitted until a particular URB has completed. Since mass-storage is an important use case for USB, its requirements should not be ignored. > > In effect, you are prioritizing other IRQs higher than USB completion > > handlers. > > Both other IRQs and HCD's IRQ are prioritizing the URB completion. I don't understand that sentence. "Prioritizing" means "assigning a priority to". IRQs and HCDs don't assign priorities to anything; priorities are assigned by human programmers. > > Nevertheless, the total time spent with interrupts disabled > > will remain the same. > > No, the total time spent with interrupts disabled does not remain same, > since no local IRQs are disabled during URB giveback anymore. That is a bug. Local IRQs must be disabled during URB giveback. > > (There's one other side effect that perhaps you haven't considered. > > When multiple URBs are addressed to the same endpoint, their completion > > handlers are called in order of URB completion, which is the same as > > the order of URB submission unless an URB is cancelled. By delegating > > the completion handlers to tasklets, and particularly by using per-CPU > > tasklets, you may violate this behavior.) > > Even though URB giveback is handled by hard interrupt handler, it still can't > guarantee that 100%, please see below: > > - interrupt for URB A comes in CPU 0, and handled, then HCD private > lock is released, and usb_hcd_giveback_urb() is called to complete URB A > > - interrupt for URB B comes in CPU 1 just before CPU0 releases HCD private > lock, so CPU1 holds the private lock when CPU 0 releases the lock and > handles the HCD interrupt. > > So now just like two CPUs are racing, if CPU0 wins, URB A is completed > first, otherwise URB B is completed first. Although you didn't say so, I assume you mean that A and B are URBs for the same endpoint. This means they use the same host controller and hence they use the same IRQ line. When an interrupt is handled, it is not possible for a second interrupt to arrive on the same IRQ line before the handler for the first interrupt returns. The kernel disables the IRQ line when the first interrupt occurs, and keeps it disabled until all the handlers are finished. Therefore the scenario you described cannot occur. > With introducing completing URB in percpu tasklet, the situation is > just very similar. No, it isn't, for the reason given above. > On ehci hcd, it can make sure that by always letting CPU0 complete all URBs > (use ehci->scanning/ehci->need_rescan flag), but the patch can change percpu > tasklet to tasklet to meet the demand. Or perhaps you could assign each endpoint to a particular tasklet. > > As I understand it, the tradeoff between hardirq and softirq is > > generally unimportant. It does matter a lot in realtime settings -- > > I don't think so, and obviously softirq allows IRQs to be served quickly, > otherwise why is softirq and tasklet introduced? and why so many drivers > are using tasklet/softirq? This is part of what I would like to hear Thomas and Steve comment on. Ala
Re: Serial ports for Septentrio USB GNSS receiver
On Mon, 10 Jun 2013, Greg KH wrote: > > >> [lsusb output] The lsusb output in the original email message showed two configurations. Config 1 exposes a single port, whereas config 2 exposes two ports (although it is vendor-specific, not CDC-ACM). > > >>Even though I don't really understand the interfaces and endpoints, > > >>I get the impression there's multiple CDC-Data interfaces. > > > > > >Yes, there are, but the cdc-acm driver binds to them all (you can see > > >this by running 'usbdevices' from the usbutils package. > > > > T: Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 2 Spd=12 MxCh= 0 > > D: Ver= 1.10 Cls=02(commc) Sub=00 Prot=00 MxPS= 8 #Cfgs= 2 > > P: Vendor=152a ProdID=8230 Rev=01.10 > > S: Manufacturer=Septentrio > > S: Product=Septentrio USB Device > > C: #Ifs= 2 Cfg#= 1 Atr=c0 MxPwr=2mA > > I: If#= 0 Alt= 0 #EPs= 1 Cls=02(commc) Sub=02 Prot=01 Driver=cdc_acm > > I: If#= 1 Alt= 0 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_acm This output confirms that there are two configurations, although only one of them is shown. By default, config 1 is installed. > > I can confirm :) How can this be changed? It is possible to switch to config 2 manually or by a pre-programmed script. The necessary command is very simple: echo 2 >/sys/bus/usb/devices/3-2/bConfigurationValue Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: Fwd: request some hep for usb comminucation
On Mon, Jun 10, 2013 at 10:25:26AM +0430, Mahdi Razavi wrote: > Hi > This is a Digital Radio which can report some data through USB to its > application. The output of lsudb : > Bus 002 Device 004: ID 238b:0a31 > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass 255 Vendor Specific Class Ick, a vendor-specific device :( > bDeviceSubClass 254 > bDeviceProtocol 254 > bMaxPacketSize0 8 > idVendor 0x238b > idProduct 0x0a31 > bcdDevice0.00 > iManufacturer 1 > iProduct2 > iSerial 0 > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 39 > bNumInterfaces 1 > bConfigurationValue 1 > iConfiguration 0 > bmAttributes 0xc0 > Self Powered > MaxPower4mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber2 > bAlternateSetting 0 > bNumEndpoints 3 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass255 Vendor Specific Subclass > bInterfaceProtocol255 Vendor Specific Protocol > iInterface 5 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x84 EP 4 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 255 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x83 EP 3 IN > bmAttributes3 > Transfer TypeInterrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x0008 1x 8 bytes > bInterval 1 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x04 EP 4 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 255 > > -- > output od dmesg: > [26785.637159] usb 2-2.1: new full-speed USB device number 11 using uhci_hcd > [26785.748010] usb 2-2.1: config 1 has an invalid interface number: 2 > but max is 0 > [26785.748018] usb 2-2.1: config 1 has no interface number 0 > [26785.759058] usb 2-2.1: New USB device found, idVendor=238b, idProduct=0a31 > [26785.759064] usb 2-2.1: New USB device strings: Mfr=1, Product=2, > SerialNumber=0 > [26785.759069] usb 2-2.1: Product: Digital Radio > [26785.759073] usb 2-2.1: Manufacturer: RIGOL Communications > > this device has a SDK for developing in windows but there isnt any in > Linux. i know protocol specification of message, and try sniffing on > windows verify protocol. > but when send a same data in Linux(with pyusb) to device dosent > response. i guess there is a initial handshake in driver layer between > device and OS. There really shouldn't be, but you might want to run Windows under kvm or vmware and snoop the data that way on the Linux side to determine the whole protocol being used. Good luck, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Serial ports for Septentrio USB GNSS receiver
Greg KH writes: > On Mon, Jun 10, 2013 at 06:56:30PM +0200, Ben Adler wrote: >> Hello Greg, >> >> thanks for getting back to me! >> >> >>I'm using a >> >>http://www.septentrio.com/products/receivers/asterx2i-oem INS/GNSS >> >>receiver. When connected via USB to a windows-host, there's two (or >> >>even three, don't remember exactly) virtual serial ports to talk to >> >>it. >> > >> >Why do you want to talk to the other ports? What is on those >> >connections? >> >> They're all serial ports, used to issue commands to the receiver (in >> binary or ascii form) and receive data at different intervals >> (again, binary or ascii). Which messages exactly are accepted or >> sent on which (virtual) port at which interval can be configured. > > That's not what the USB device descriptors you provided below show. > They look like a "normal" cdc-acm device, so we are (according to the > USB-IF specification), supposed to export to userspace a single device > that can be used to communicate with it. Note that the lsusb output showed a second configuration with 2 vendor specific ACM functions (only 2 bulk endpoints and protocol = 0xff) instead of the single standard CDC ACM function. This is probably the configuration used by the Windows host. The cdc-acm driver cannot handle those ports, but a more forgiving generic driver can. I don't recommend it for normal use because it abuses the option driver, but Ben could do a simple test like this: echo 2 >/sys/bus/usb/devices//bConfigurationValue modprobe option echo 152a 8230 > /sys/bus/usb-serial/drivers/option1/new_id Unless I missed something, this should result in two /dev/ttyUSBx serial devices. I guess this device is worth a new serial driver of its own in case that works? Or should we create a generic driver for 02/02/ff serial devices (using the inverse of the logic in drivers/net/usb/cdc_ether.c: usbnet_generic_cdc_bind to avoid RNDIS devices)? A few modems with such ports have been added to option, but a generic solution might be better. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Mon, 2013-06-10 at 13:36 -0400, Alan Stern wrote: > > I don't think so, and obviously softirq allows IRQs to be served quickly, > > otherwise why is softirq and tasklet introduced? Because of history. In the old days there were top halves (hard irq) and bottom halves (like a softirq). The thing about a bottom half was, no two could run at the same time. That is, if one CPU was running a bottom half, another CPU would have to wait for that one to complete before it could run any bottom half. The killer here is that it didn't matter if it was the same bottom half or not! Microsoft/Mindcraft was kind enough to point out this inefficiency with some benchmarks showing how horrible Linux ran on a 4 way box. Thanks to them, softirqs and tasklets were born :-) A softirq does the rest of the interrupt work with interrupts enabled. A tasklet is run from softirq context, but the difference between a softirq and tasklet is that the same tasklet can not run on two different CPUs at the same time. It acts similar to a task, as a task can not run on two different CPUs at the same time either, hence the name "tasklet". But tasklets are better than bottom halves because it allows for different tasklets to run on different CPUs. > and why so many drivers > > are using tasklet/softirq? Because it's easy to set up and device driver authors don't know any better ;-). Note, a lot of drivers are now using work queues today, which run as a task. Yes, there's a little more overhead with a task than for a softirq, but the problem with softirqs and tasklets is that they can't be preempted, and they are more important than all tasks on the system. If you have a task that is critical, it must yield for your softirq. Almost! That is, even if you have a softirq, it may not run in irq context at all. If you get too many softirqs at a time (one comes while another one is running), it will defer the processing to the ksoftirq thread. This not only runs as a task, but also runs as SCHED_OTHER, and must yield to other tasks like everyone else too. Thus, adding it as a softirq does not guarantee that it will be processed quickly. It just means that most of the time it will prevent anything else from happening while your "most important handler in the world" is running. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Mon, 10 Jun 2013, Steven Rostedt wrote: > > and why so many drivers > > > are using tasklet/softirq? > > Because it's easy to set up and device driver authors don't know any > better ;-). Note, a lot of drivers are now using work queues today, > which run as a task. > > Yes, there's a little more overhead with a task than for a softirq, but > the problem with softirqs and tasklets is that they can't be preempted, > and they are more important than all tasks on the system. If you have a > task that is critical, it must yield for your softirq. Almost! > > That is, even if you have a softirq, it may not run in irq context at > all. If you get too many softirqs at a time (one comes while another one > is running), it will defer the processing to the ksoftirq thread. This > not only runs as a task, but also runs as SCHED_OTHER, and must yield to > other tasks like everyone else too. > > Thus, adding it as a softirq does not guarantee that it will be > processed quickly. It just means that most of the time it will prevent > anything else from happening while your "most important handler in the > world" is running. >From this, it sounds like you generally advise using threaded interrupt handlers rather than tasklets/softirqs. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Mon, 10 Jun 2013, Alan Stern wrote: > > Tasklet doesn't disable local interrupts. > > It is documented that interrupts will be disabled while the completion > handler runs. Therefore the tasklet _must_ disable local interrupts. You know, it may be that you can get most of the advantages you want by enabling local interrupts around the call to unmap_urb_for_dma() in usb_hcd_giveback_urb(). This may be a little dangerous, though, because it is possible for an URB to be given back at the time it is submitted. Drivers may not expect interrupts to get enabled temporarily when they call usb_submit_urb(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Mon, 2013-06-10 at 16:47 -0400, Alan Stern wrote: > On Mon, 10 Jun 2013, Steven Rostedt wrote: > > > > and why so many drivers > > > > are using tasklet/softirq? > > > > Because it's easy to set up and device driver authors don't know any > > better ;-). Note, a lot of drivers are now using work queues today, > > which run as a task. > > > > Yes, there's a little more overhead with a task than for a softirq, but > > the problem with softirqs and tasklets is that they can't be preempted, > > and they are more important than all tasks on the system. If you have a > > task that is critical, it must yield for your softirq. Almost! > > > > That is, even if you have a softirq, it may not run in irq context at > > all. If you get too many softirqs at a time (one comes while another one > > is running), it will defer the processing to the ksoftirq thread. This > > not only runs as a task, but also runs as SCHED_OTHER, and must yield to > > other tasks like everyone else too. > > > > Thus, adding it as a softirq does not guarantee that it will be > > processed quickly. It just means that most of the time it will prevent > > anything else from happening while your "most important handler in the > > world" is running. > > >From this, it sounds like you generally advise using threaded interrupt > handlers rather than tasklets/softirqs. Yes, there's plenty of benefits for them, and I highly doubt that any USB device would even notice the difference between a softirq and a thread for response time latencies. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] usb: phy: rcar-usb: Fix comment w.r.t. devm_ioremap_resource
On 06/10/2013 05:47 PM, Sergei Shtylyov wrote: > Hello. > > On 10-06-2013 15:35, Tushar Behera wrote: > [ ... ] >>* CAUTION >>* >>* Because this phy address is also mapped under OHCI/EHCI >> address area, >> - * this driver can't use devm_request_and_ioremap(dev, res) here >> + * this driver can't use devm_ioremap_resource(dev, res) here >>*/ >> reg0 = devm_ioremap_nocache(dev, res0->start, resource_size(res0)); >> reg1 = devm_ioremap_nocache(dev, res1->start, resource_size(res1)); > > I'm completely removing this comment in my series pushed thru > renesas.git, so I advise this patch to be ignored not to create conflict > in the future. > Ok. I will drop this from the patchset. > WBR, Sergei > Thanks. -- Tushar Behera -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Tue, Jun 11, 2013 at 1:36 AM, Alan Stern wrote: > On Mon, 10 Jun 2013, Ming Lei wrote: > >> > That isn't clear. It is documented that USB completion handlers are >> > called with local interrupts disabled. An IRQ arriving before the >> >> Is there any good reason to do URB giveback with interrupt disabled? > > That's a good question. Offhand I can't think of any drivers that rely > on this -- although there certainly are places where callback routines > use spin_lock() rather than spin_lock_irq() or spin_lock_irqsave(), > because they know that interrupts are already disabled. Complete() may take long time, for example in UVC's driver, URB's complete() may take more than 3 ms, as reported in below link: http://marc.info/?t=136438111600010&r=1&w=2 Also complete() is provided by interface driver, and it does many driver specific works, so it isn't good to disable interrupt only for one driver. If complete() callback runs in one tasklet context, spin_lock() inside complete() is enough, just like hard irq, the tasklet itself is disabled during complete(), if the percpu tasklet is converted to single tasklet. So no problem when the lock is to protect shared resources between complete(). When the lock is to protect shared resources between complete() and non-IRQ context, currently spin_lock_irqsave() is used in non-IRQ context, which is enough to prevent tasklet from being run on the CPU, so no problem for this situation too. When all HCDs support to run URB giveback in tasklet context, we can change all spin_lock_irq*() to spin_lock() in drivers URB->complete(), and in other places, the spin_lock_irq*() can be changed to spin_lock_bh(). > > However, changing a documented API is not something to be done lightly. > You would have to audit _every_ USB driver to make sure no trouble > would arise. OK, I will write patch to request for changing the API if the improvement on the situation isn't objected. In fact, at least now, the change on API is only about document change, and no drivers' change is required, isn't it? We can describe that URB->complete() might run in hard irq or softirq context, depends on HCD_BH flag of hcd->driver->flags. > >> IMO, disabling IRQs isn't necessary for completing URB since the >> complete() of URBs for same endpoint is reentrant. > > Whether it is _necessary_ isn't the point. It is _documented_, and > therefore it cannot be changed easily. Same with above. > >> > tasklet starts up might therefore be serviced during the short interval >> > before the tasklet disables local interrupts, but if that happens it >> >> Tasklet doesn't disable local interrupts. > > It is documented that interrupts will be disabled while the completion > handler runs. Therefore the tasklet _must_ disable local interrupts. Same with above, we can change the document which itself shouldn't have been the only reason for blocking the change, :-) >> > would mean that the completion handler would be delayed. >> >> Yes, the tasklet schedule delay is introduced to URB completion, but >> the delay doesn't have much side effect about completing URB. > > What about delays that occur because a separate interrupt is serviced > before the URB's completion handler is called? With the current code > that can't happen, but with your scheme it can. Even with current code, one HCD's interrupt handling still may delay the handling for another HCD interrupt handling for quite long, that is the motivation to decrease the interrupt handling time of USB HCD, ;-) And finally, USB HCDs themselves will benefit from the change, won't they? Also it shouldn't be a problem since most of interrupt handlers takes very less time. >> For async transfer, generally, it should have no effect since there should >> have URBs queued on the qh queue before submitting URB. > > That's not how the Bulk-Only mass-storage protocol works. There are > times when the protocol _requires_ bulk packets not to be submitted > until a particular URB has completed. Yes, I agree. But mass-storage driver itself is very fragile, it will perform badly if CPU has a higher load. And it is better to submit DATA/CSW transfer in previous transfer's complete(), IMO. Compared with "usb-storage" task's schedule latency, the tasklet schedule delay should be lower at most of situations since the tasklet is scheduled inside irq handler. > > Since mass-storage is an important use case for USB, its requirements > should not be ignored. That is why I did many tests on mass storage case, and from the results in commit log of patch 4/4, looks no performance loss is involved with the change on usb storage. > >> > In effect, you are prioritizing other IRQs higher than USB completion >> > handlers. >> >> Both other IRQs and HCD's IRQ are prioritizing the URB completion. > > I don't understand that sentence. "Prioritizing" means "assigning a > priority to". IRQs and HCDs don't assign priorities to anything; > priorities are assigned by human pro
Re: [RFC PATCH 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
On Tue, Jun 11, 2013 at 4:51 AM, Alan Stern wrote: > On Mon, 10 Jun 2013, Alan Stern wrote: > >> > Tasklet doesn't disable local interrupts. >> >> It is documented that interrupts will be disabled while the completion >> handler runs. Therefore the tasklet _must_ disable local interrupts. > > You know, it may be that you can get most of the advantages you want by > enabling local interrupts around the call to unmap_urb_for_dma() in > usb_hcd_giveback_urb(). No, please don't enable IRQs inside interrupt handler, and you will get warning dump. Except for unmap_urb_for_dma() in usb_hcd_giveback_urb(), I hope map_urb_for_dma() inside complete() can benefit from the change too, since map_urb_for_dma() is same time-consuming with unmap_urb_for_dma(). Also I hope complete() can be run with interrupt enabled since driver's complete() may take long time on some ARCH, as I mentioned in my last mail. And it isn't good to disable interrupt only for one interface driver, looks I still don't get real good explanation about disabling IRQs here, :-) > > This may be a little dangerous, though, because it is possible for an > URB to be given back at the time it is submitted. Drivers may not > expect interrupts to get enabled temporarily when they call > usb_submit_urb(). Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: serial/ftdi_sio byte loss / performance regression
On Thu, Jun 06, 2013 at 11:21:54AM -0700, Greg KH wrote:
> On Thu, Jun 06, 2013 at 11:58:56AM +0200, Johan Hovold wrote:
> > On Thu, Jun 06, 2013 at 10:50:36AM +0200, Tomaž Šolc wrote:
> > > Hi
> > >
> > > I have noticed that the ftdi_sio serial driver in recent kernel versions
> > > has very bad performance when used through the Python's serial library.
> > >
> > > As a test case I have a custom device that will send a continuous block
> > > of 5k characters once every few seconds over a RS-232 line (115200 baud)
> > > to an Olimex programmer (based on FT2232C, also tried one with FT2232H).
> > >
> > > Programmer is connected to a Linux system where a simple Python script
> > > reads the device:
> > >
> > > import serial
> > > comm = serial.Serial("/dev/ttyUSB0", 115200)
> > > while True:
> > > line = comm.readline()
> > >
> > > With kernels before 3.7.0 the script reads uncorrupted data while using
> > > newer kernels (including 3.9.4) the Python script sees heavy byte loss.
> > > "top" shows an 95% "idle" CPU. Only very slow transmissions (on the
> > > order of tens of bytes per second) will come through uncorrupted.
> > >
> > > Using git-bisect, I have found the commit that introduced this problem:
> > >
> > > 6f602912c9d0c84c2edbd446dd9f72660b701605
> > > usb: serial: ftdi_sio: Add missing chars_in_buffer function
> > >
> > > This might also be related with the unusual way Python serial library
> > > reads the device. It uses select() with no timeout and single byte
> > > read()s in a loop. strace output:
> > >
> > > select(4, [3], [], [], NULL)= 1 (in [3])
> > > read(3, "D", 1) = 1
> > > select(4, [3], [], [], NULL)= 1 (in [3])
> > > read(3, "E", 1) = 1
> > > ...
> > >
> > > With sufficiently large read()s the byte loss can be eliminated.
> > >
> > > With the commit above, each select() now causes an additional round trip
> > > over USB to read the state of the hardware buffer. It's possible that
> > > constant status querying triggers some bug in the hardware or the query
> > > is simply too slow and causes overflows in the hardware buffer.
> >
> > You're absolutely right. This is a known issue (the select overhead)
> > that was just recently fixed by commit a37025b5c7 ("USB: ftdi_sio: fix
> > chars_in_buffer overhead") in v3.10-rc3. Care to give v3.10-rc4 a try?
> >
> > Greg, perhaps we should consider backporting the wait-until-sent
> > patches (i.e. 0693196fe..4746b6c6e)?
>
> Yes, that's a good idea, I'll do that for the next round of stable
> updates, after this next release tomorrow.
I applied these, plus a few others in order to get them all to apply and
build properly.
But the last patch, 4746b6c6e, didn't apply, and I don't think we really
need it for the 3.9 kernel, do we?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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: serial/ftdi_sio.c Fix kernel oops
On Fri, Jun 07, 2013 at 03:14:32PM +0200, Lotfi Manseur wrote: > Handle null termios in ftdi_set_termios(), introduced in > commit 552f6bf1bb0eda0011c0525dd587aa9e7ba5b846 > This has been corrected in the mainline by > commits c515598e0f5769916c31c00392cc2bfe6af74e55 and This commit showed up in 3.3, so it can't go into 3.4 at all. Please be more careful when asking for stable patches to be applied. That is why I want the _exact_ same patch to apply, don't try going and being smart by mushing them together into something else, this would have obviously not been correct for the 3.4 kernel at all. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
