Re: [PATCH v3 00/49] usb: dwc2: fixes, enhancements and new features
On 1/9/2018 11:56 PM, Stefan Wahren wrote: Hi Stefan, > Hi Razmik, > >> Razmik Karapetyan hat am 8. Januar 2018 um >> 13:40 geschrieben: >> >> >> On 12/31/2017 9:19 PM, Stefan Wahren wrote: >> >> Hi Stefan, >> Razmik Karapetyan (10): usb: dwc2: Set AHB burst size to INCR >>> >>> The usage hsotg->params.ahbcfg instead of the defines is a unintended fix >>> for BCM2835. According to the BCM2835 datasheet this register have a >>> different definition. So i like to see this split up. >>> >> >> This patch sets AHB burst size by default to INCR, but it can be >> customized later for each platform by device match table. For BSM you >> have dwc2_set_bcm_params() function. >> >> Patch also uses already calculated value for burst size >> "hsotg->params.ahbcfg" in dwc2_hsotg_core_init_disconnected() >> instead of calculating the same value again. >> >> What kind of problems you see here? > > the problem is there is no AHB burst size on BCM2835. > > Quote from BCM2835 datasheet (p. 204): > >The USB_GAHBCFG register has been adapted. Bits [4:1] which are marked in > the Synopsys >documentation as "Burst Length/Type (HBstLen)" have been used differently. > > Your patch is doing two different things, which should better be split up. So > in case of a revert only one part is affected. > > Stefan > Ok, in that case i will split it up. I will prepare 2 patches, first related to switching from INCR4 to INCR, second related to using ahbcfg parameter in dwc2_hsotg_core_init_disconnected(). Best regards, Razmik >> usb: dwc2: Define PCGCCTL1 register in hw.h usb: dwc2: Define Active Clock Gating support bit in GHWCFG4 >>> >>> I think this one should be merged into the first patch which uses this >>> define. >>> >> >> I am ok with this observation, i will change it. >> >> Best regards, >> Razmik > -- 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] uas: Unblock scsi-requests on failure to alloc streams in post_reset
If we return 1 from our post_reset handler, then our disconnect handler
will be called immediately afterwards. Since pre_reset blocks all scsi
requests our disconnect handler will then hang in the scsi_remove_host
call.
This is esp. bad because our disconnect handler hanging for ever also
stops the USB subsys from enumerating any new USB devices, causes commands
like lsusb to hang, etc.
In practice this happens when unplugging some uas devices because the hub
code may see the device as needing a warm-reset and calls usb_reset_device
before seeing the disconnect. In this case uas_configure_endpoints fails
with -ENODEV. We do not want to print an error for this, so this commit
also silences the shost_printk for -ENODEV.
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1531966
Cc: sta...@vger.kernel.org
Fixes: 8d51444cdd06 ("uas: Not being able to alloc streams ... is an error")
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 5d04c40ee40a..5471422aa1ab 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -1077,9 +1077,13 @@ static int uas_post_reset(struct usb_interface *intf)
err = uas_configure_endpoints(devinfo);
if (err) {
- shost_printk(KERN_ERR, shost,
-"%s: alloc streams error %d after reset",
-__func__, err);
+ if (err != -ENODEV) {
+ shost_printk(KERN_ERR, shost,
+"%s: alloc streams error %d after reset",
+__func__, err);
+ }
+ /* So that scsi_remove_host in uas_disconnect does not hang */
+ scsi_unblock_requests(shost);
return 1;
}
--
2.14.3
--
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] USB TYPEC: RT1711H Type-C Chip Driver
From: ShuFanLee
Richtek RT1711H Type-C chip driver that works with
Type-C Port Controller Manager to provide USB PD and
USB Type-C functionalities.
Signed-off-by: ShuFanLee
---
.../devicetree/bindings/usb/richtek,rt1711h.txt| 38 +
arch/arm64/boot/dts/hisilicon/rt1711h.dtsi | 11 +
drivers/usb/typec/Kconfig |2 +
drivers/usb/typec/Makefile |1 +
drivers/usb/typec/rt1711h/Kconfig |7 +
drivers/usb/typec/rt1711h/Makefile |2 +
drivers/usb/typec/rt1711h/rt1711h.c| 2241
drivers/usb/typec/rt1711h/rt1711h.h| 300 +++
8 files changed, 2602 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
create mode 100644 arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
create mode 100644 drivers/usb/typec/rt1711h/Kconfig
create mode 100644 drivers/usb/typec/rt1711h/Makefile
create mode 100644 drivers/usb/typec/rt1711h/rt1711h.c
create mode 100644 drivers/usb/typec/rt1711h/rt1711h.h
diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
new file mode 100644
index 000..c28299c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
@@ -0,0 +1,38 @@
+Richtek RT1711H Type-C Port Controller.
+
+Required properties:
+- compatible : Must be "richtek,typec_rt1711h";
+- reg : Must be 0x4e, it's default slave address of RT1711H.
+- rt,intr_gpio : IRQ GPIO pin that's connected to RT1711H interrupt.
+
+Optional node:
+- rt,name : Name used for registering IRQ and creating kthread.
+ If this property is not specified, "default" will be applied.
+- rt,def_role : Default port role (TYPEC_SINK(0) or TYPEC_SOURCE(1)).
+ Set to TYPEC_NO_PREFERRED_ROLE(-1) if no default role.
+ If this property is not specified, TYPEC_SINK will be applied.
+- rt,port_type : Port type (TYPEC_PORT_DFP(0), TYPEC_PORT_UFP(1),
+or TYPEC_PORT_DRP(2)). If this property is not specified,
+TYPEC_PORT_DRP will be applied.
+- rt,max_snk_mv : Maximum acceptable sink voltage in mV.
+ If this property is not specified, 5000mV will be applied.
+- rt,max_snk_ma : Maximum sink current in mA.
+ If this property is not specified, 3000mA will be applied.
+- rt,max_snk_mw : Maximum required sink power in mW.
+ If this property is not specified, 15000mW will be applied.
+- rt,operating_snk_mw : Required operating sink power in mW.
+ If this property is not specified,
+ 2500mW will be applied.
+- rt,try_role_hw : True if try.{Src,Snk} is implemented in hardware.
+ If this property is not specified, False will be applied.
+
+Example:
+rt1711h@4e {
+ status = "ok";
+ compatible = "richtek,typec_rt1711h";
+ reg = <0x4e>;
+ rt,intr_gpio = <&gpio26 0 0x0>;
+ rt,name = "rt1711h";
+ rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+ rt,def_role = <0>; /* 0: SNK, 1: SRC, -1: TYPEC_NO_PREFERRED_ROLE */
+};
diff --git a/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
new file mode 100644
index 000..4196cc0
--- /dev/null
+++ b/arch/arm64/boot/dts/hisilicon/rt1711h.dtsi
@@ -0,0 +1,11 @@
+&i2c7 {
+ rt1711h@4e {
+ status = "ok";
+ compatible = "richtek,typec_rt1711h";
+ reg = <0x4e>;
+ rt,intr_gpio = <&gpio26 0 0x0>;
+ rt,name = "rt1711h";
+ rt,port_type = <2>; /* 0: DFP, 1: UFP, 2: DRP */
+ rt,def_role = <0>; /* 0: SNK, 1: SRC */
+ };
+};
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index bcb2744..7bede0b 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -56,6 +56,8 @@ if TYPEC_TCPM
source "drivers/usb/typec/fusb302/Kconfig"
+source "drivers/usb/typec/rt1711h/Kconfig"
+
config TYPEC_WCOVE
tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
depends on ACPI
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index bb3138a..e3aaf3c 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_TYPEC)+= typec.o
obj-$(CONFIG_TYPEC_TCPM) += tcpm.o
obj-y += fusb302/
+obj-$(CONFIG_TYPEC_RT1711H)+= rt1711h/
obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o
obj-$(CONFIG_TYPEC_UCSI) += ucsi/
obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
diff --git a/drivers/usb/typec/rt1711h/Kconfig
b/drivers/usb/typec/rt1711h/Kconfig
new file mode 100644
index 000..2fbfff5
--- /dev/null
+++ b/drivers/usb/typec/rt1711h/Kconfig
@@ -0,0 +1,7 @@
+config TYPEC_RT1711H
+ tristate "Richtek RT1711H Type-C chip driver"
+ depends on I2C && POWER_SUPPLY
+
Re: [PATCH V2 5/5] usb: serial: f81534: fix tx error on some baud rate
Hi Johan, Johan Hovold 於 2018/1/9 下午 07:32 寫道: On Thu, Jan 04, 2018 at 10:29:21AM +0800, Ji-Ze Hong (Peter Hong) wrote: + /* +* We'll make tx frame error when baud rate from 384~500kps. So we'll +* delay all tx data frame with 1bit. +*/ + port_priv->shadow_clk |= F81534_CLK_TX_DELAY_1BIT; You don't wan't to enable this only for the affected rates? This bit will control all transmit TX frame always delay 1 bit on enabled, but It'll transmit TX frame randomly delay 1 bit on disabled. We had tested it with BurnInTest to check the performance, It'll not make the performance regression. So we'll directly add it on all baud rate. Thanks -- With Best Regards, Peter Hong -- 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 V2 1/5] usb: serial: f81534: add high baud rate support
Hi Johan, Johan Hovold 於 2018/1/9 下午 07:08 寫道: On Thu, Jan 04, 2018 at 10:29:17AM +0800, Ji-Ze Hong (Peter Hong) wrote: The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates can be up to 1.5Mbits with 24MHz. This device may generate data overrun when baud rate setting to 921600bps or higher with old UART trigger level setting (8x14=112) with full loading. We'll change trigger level from 8x14=112 to 8x8=64 to avoid data overrun. Also the read/write of EP0 will be affected by this patch. The worst case of responding time is 20s when all serial port are full loading and trying to access EP0, so we change EP0 timeout from 10 to 20s. Surely you meant 1 and 2 seconds respectively here? And if you have indeed measured response times close to 2000 ms then perhaps you want to add even more margin? Normally, the communication with F81534 ep0 will take less than 1 sec (even only some milliseconds), but It maybe take much long time with huge loading with UART functional. We had tested it on BurnInTest, 4 ports with 921600bps + MSR status check to perform huge loading test. The worst case to read MSR register via ep0 will take 15~18 seconds. So We'll still remain the max waiting time for access ep0 with 2x10=20s in high baud rate mode. Thanks -- With Best Regards, Peter Hong -- 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/2] usb: xhci: Remove ep_trb from finish_td()
Function argument ep_trb for finish_td() isn't needed anymore.
Cleanup it.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 642a070..88071c4 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1921,7 +1921,7 @@ static int xhci_td_cleanup(struct xhci_hcd *xhci, struct
xhci_td *td,
}
static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
- union xhci_trb *ep_trb, struct xhci_transfer_event *event,
+ struct xhci_transfer_event *event,
struct xhci_virt_ep *ep, int *status)
{
struct xhci_virt_device *xdev;
@@ -2081,7 +2081,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct
xhci_td *td,
td->urb->actual_length = requested;
finish_td:
- return finish_td(xhci, td, ep_trb, event, ep, status);
+ return finish_td(xhci, td, event, ep, status);
}
/*
@@ -2168,7 +2168,7 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct
xhci_td *td,
td->urb->actual_length += frame->actual_length;
- return finish_td(xhci, td, ep_trb, event, ep, status);
+ return finish_td(xhci, td, event, ep, status);
}
static int skip_isoc_td(struct xhci_hcd *xhci, struct xhci_td *td,
@@ -2258,7 +2258,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci,
struct xhci_td *td,
remaining);
td->urb->actual_length = 0;
}
- return finish_td(xhci, td, ep_trb, event, ep, status);
+ return finish_td(xhci, td, event, ep, status);
}
/*
--
2.7.4
--
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/2] usb: xhci: Remove ep_trb from xhci_cleanup_halted_endpoint()
Function argument ep_trb for xhci_cleanup_halted_endpoint() isn't
needed anymore. Cleanup it.
Signed-off-by: Lu Baolu
---
drivers/usb/host/xhci-ring.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index daa94c3..642a070 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1815,8 +1815,7 @@ struct xhci_segment *trb_in_td(struct xhci_hcd *xhci,
static void xhci_cleanup_halted_endpoint(struct xhci_hcd *xhci,
unsigned int slot_id, unsigned int ep_index,
- unsigned int stream_id,
- struct xhci_td *td, union xhci_trb *ep_trb,
+ unsigned int stream_id, struct xhci_td *td,
enum xhci_ep_reset_type reset_type)
{
struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index];
@@ -1957,8 +1956,7 @@ static int finish_td(struct xhci_hcd *xhci, struct
xhci_td *td,
* The class driver clears the device side halt later.
*/
xhci_cleanup_halted_endpoint(xhci, slot_id, ep_index,
- ep_ring->stream_id, td, ep_trb,
- EP_HARD_RESET);
+ ep_ring->stream_id, td, EP_HARD_RESET);
} else {
/* Update ring dequeue pointer */
while (ep_ring->dequeue != td->last_trb)
@@ -2318,7 +2316,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
case COMP_INVALID_STREAM_TYPE_ERROR:
case COMP_INVALID_STREAM_ID_ERROR:
xhci_cleanup_halted_endpoint(xhci, slot_id, ep_index, 0,
-NULL, NULL, EP_SOFT_RESET);
+NULL, EP_SOFT_RESET);
goto cleanup;
case COMP_RING_UNDERRUN:
case COMP_RING_OVERRUN:
@@ -2584,8 +2582,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
xhci_cleanup_halted_endpoint(xhci, slot_id,
ep_index,
ep_ring->stream_id,
-td, ep_trb,
-EP_HARD_RESET);
+td, EP_HARD_RESET);
goto cleanup;
}
--
2.7.4
--
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: dvb usb issues since kernel 4.9
On Tue, 2018-01-09 at 22:26 +0100, Jesper Dangaard Brouer wrote:
>
> I've previously experienced that you can be affected by the scheduler
> granularity, which is adjustable (with CONFIG_SCHED_DEBUG=y):
>
> $ grep -H . /proc/sys/kernel/sched_*_granularity_ns
> /proc/sys/kernel/sched_min_granularity_ns:225
> /proc/sys/kernel/sched_wakeup_granularity_ns:300
>
> The above numbers were confirmed on the RPi2 (see[2]). With commit
> 4cd13c21b207 ("softirq: Let ksoftirqd do its job"), I expect/assume that
> softirq processing latency is bounded by the sched_wakeup_granularity_ns,
> which with 3 ms is not good enough for their use-case.
Note of caution wrt twiddling sched_wakeup_granularity_ns: it must
remain < sched_latency_ns/2 else you effectively disable wakeup
preemption completely, turning CFS into a tick granularity scheduler.
-Mike
--
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
Attn: Email Owner,
IMF/World Bank Auditors Instructed Western Union to Release Your Sum of $1.5Million dollars, Email lottery Awards Held in Russia fortnight ago for the oncoming World Cup, 2018,So Kindly Contact Western Union Director Harry Morris: +22998220265, Email (emaillottoryfi...@gmail.com) with your Personal Address for your Claims.And follow their instructions because your email award was paid with your email that won the Lottery,Congratulation! From IMF/WORLD BANK AUDITORS -- 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
[GIT PULL] usb: chipidea: updates for v4.16-rc1
The following changes since commit 16e791e7659645e6d8ea6286d210a24ee473d6c2: doc: usb: chipidea: Fix typo in 'enumerate' (2017-12-08 17:43:53 +0100) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git/ tags/usb-ci-v4.16-rc1 for you to fetch changes up to 061e20e9899e2fef170135a5d68f62d2a9514b3b: usb: chipidea: tegra: Use aligned DMA on Tegra30 (2017-12-21 09:32:05 +0800) Just one small update: Use aligned DMA on Tegra30, and USB Ethernet gadget now works on it. Dmitry Osipenko (1): usb: chipidea: tegra: Use aligned DMA on Tegra30 drivers/usb/chipidea/ci_hdrc_tegra.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- Best Regards, Peter Chen -- 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 08/15] usb: dwc3: Make RX/TX threshold configurable
Hi, On 1/8/2018 8:12 PM, Rob Herring wrote: > On Fri, Jan 05, 2018 at 12:14:48PM -0800, Thinh Nguyen wrote: >> DWC_usb31 periodic transfer at 48K+ bytes per interval may need >> modification to the TX/RX packet threshold to achieve optimal result. >> Add properties to make it configurable. > > I tend to think these should all be implied by the SoC specific > compatible string if they need to be tuned. > >> >> Cc: John Youn >> Signed-off-by: Thinh Nguyen >> --- >> Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >> b/Documentation/devicetree/bindings/usb/dwc3.txt >> index 52fb41046b34..02dde83d02fa 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -55,6 +55,12 @@ Optional properties: >>- snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of >> GFLADJ >> register for post-silicon frame length adjustment when the >> fladj_30mhz_sdbnd signal is invalid or incorrect. >> + - snps,rx_thr_sel_prd: set to enable periodic ESS RX packet threshold. > > Isn't the next property being present sufficient to enable this or not? Yes, we can do that. Actually, both settings must be set to enable the periodic TX/RX threshold. > >> + - snps,rx_thr_num_pkt_prd: periodic ESS RX packet threshold count. >> + - snps,rx_max_burst_prd: Max periodic ESS RX burst size. >> + - snps,tx_thr_sel_prd: set to enable periodic ESS TX packet threshold. > > ditto > >> + - snps,tx_thr_num_pkt_prd: periodic ESS TX packet threshold count. >> + - snps,tx_max_burst_prd: Max periodic ESS TX burst size. > > Don't use '_' in property names. I'll make the change. > >> >>- tx-fifo-resize: determines if the FIFO *has* to be >> reallocated. >> >> -- >> 2.11.0 >> > Thanks, Thinh -- 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: Re: dvb usb issues since kernel 4.9
On Tue, Jan 9, 2018 at 10:58 AM, Linus Torvalds wrote: > On Tue, Jan 9, 2018 at 9:57 AM, Eric Dumazet wrote: >> >> Your patch considers TASKLET_SOFTIRQ being a candidate for 'immediate >> handling', but TCP Small queues heavily use TASKLET, >> so as far as I am concerned a revert would have the same effect. > > Does it actually? > > TCP ends up dropping packets outside of the window etc, so flooding a > machine with TCP packets and causing some further processing up the > stack sounds very different from the basic packet flooding thing that > happens with NET_RX_SOFTIRQ. > > Also, honestly, the kinds of people who really worry about flooding > tend to have packet filtering in the receive path etc. > > So I really think "you can use up 90% of CPU time with a UDP packet > flood from the same network" is very very very different - and > honestly not at all as important - as "you want to be able to use a > USB DVB receiver and watch/record TV". > > Because that whole "UDP packet flood from the same network" really is > something you _fundamentally_ have other mitigations for. > > I bet that whole commit was introduced because of a benchmark test, > rather than real life. No? > > In contrast, now people are complaining about real loads not working. > > Linus I said that a revert was fine, maybe I was not clear. Clearly we can not touch anything scheduler related without breaking someone workload/assumptions on how system behaved at some point. Your patch wont solve other workloads that might have been impacted by my patch, so in one year (or next week), we will have to cope with another device driver not using tasklet but still relying on immediate softirq processing. Apparently, we have to live with softirq model forever, or switch to RT kernels. Note that we have no mitigation for something that involve flood of valid packets that no firewall can drop (without dropping legitimate packets). The 'benchmark' here is not really the trigger, only a tool validating an idea/patch. -- 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
Error messages on USB 3.0 device removal: bug or feature?
Hi, I recently started to use an USB 3.0 card to plug in an external drive (using 4.14.x). So xhci does its thing and everything works fine, but I get curious error messages after unmounting & removing a device, which don't appear when unplugging something from my internal USB2 port: [Jan 9 10:46] xhci_hcd :03:00.0: Cannot set link state. [ +0.03] usb usb4-port2: cannot disable (err = -32) [ +0.01] usb 4-2: USB disconnect, device number 2 Are these messages intended or just a hardware hiccup on device removal? thanks, Holger -- 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: dwc2: Fix endless deferral probe
On Tue, Jan 9, 2018 at 8:28 PM, Stefan Wahren wrote: > The dwc2 USB driver tries to find a generic PHY first and then look > for an old style USB PHY. In case of a valid generic PHY node without > a PHY driver, the PHY layer will return -EPROBE_DEFER forever. So dwc2 > will never tries for an USB PHY. > > Fix this issue by finding a generic PHY and an old style USB PHY > at once. This would fix only one of the USB controllers (dwc2), but not the others that are affected. As I wrote in my suggested patch, dwc3 appears to be affected the same way, and all other host drivers that call usb_add_hcd() without first setting hcd->phy would suffer from this as well. If we go down the route of addressing it here in the hcd drivers, we should at least change all three of those, and hope this doesn't regress in another way. Arnd -- 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: dvb usb issues since kernel 4.9
On Tue, 9 Jan 2018 15:42:35 -0200 Mauro Carvalho Chehab
wrote:
> Em Mon, 8 Jan 2018 11:51:04 -0800 Linus Torvalds
> escreveu:
>
[...]
> Patch makes sense to me, although I was not able to test it myself.
The patch also make sense to me. I've done some basic testing with it
on my high-end Broadwell system (that I use for 100Gbit/s testing). As
expected the network overload case still works, as NET_RX_SOFTIRQ is
not matched.
> I set a RPi3 machine here with vanilla Kernel 4.14.11 running a
> standard raspbian distribution (with elevator=deadline).
I found a Raspberry Pi Model B+ (I think, BCM2835), that I loaded the
LibreELEC distro on. One of the guys even created an image for me with
a specific kernel[1] (that I just upgraded the system with).
[1]
https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/?postID=77031#post77031
> My plan is to do more tests along this week, and try to tweak a little
> bit both userspace and kernelspace, in order to see if I can get
> better results.
I've previously experienced that you can be affected by the scheduler
granularity, which is adjustable (with CONFIG_SCHED_DEBUG=y):
$ grep -H . /proc/sys/kernel/sched_*_granularity_ns
/proc/sys/kernel/sched_min_granularity_ns:225
/proc/sys/kernel/sched_wakeup_granularity_ns:300
The above numbers were confirmed on the RPi2 (see[2]). With commit
4cd13c21b207 ("softirq: Let ksoftirqd do its job"), I expect/assume that
softirq processing latency is bounded by the sched_wakeup_granularity_ns,
which with 3 ms is not good enough for their use-case.
Thus, if you manage to reproduce the case, try to see if adjusting this
can mitigate the issue...
Their system have non-preempt kernel, should they use PREEMPT?
LibreELEC:~ # uname -a
Linux LibreELEC 4.14.10 #1 SMP Tue Jan 9 17:35:03 GMT 2018 armv7l GNU/Linux
[2]
https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/?postID=76999#post76999
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
--
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 v3 00/49] usb: dwc2: fixes, enhancements and new features
Hi Razmik, > Razmik Karapetyan hat am 8. Januar 2018 um > 13:40 geschrieben: > > > On 12/31/2017 9:19 PM, Stefan Wahren wrote: > > Hi Stefan, > > >> > >> Razmik Karapetyan (10): > >>usb: dwc2: Set AHB burst size to INCR > > > > The usage hsotg->params.ahbcfg instead of the defines is a unintended fix > > for BCM2835. According to the BCM2835 datasheet this register have a > > different definition. So i like to see this split up. > > > > This patch sets AHB burst size by default to INCR, but it can be > customized later for each platform by device match table. For BSM you > have dwc2_set_bcm_params() function. > > Patch also uses already calculated value for burst size > "hsotg->params.ahbcfg" in dwc2_hsotg_core_init_disconnected() > instead of calculating the same value again. > > What kind of problems you see here? the problem is there is no AHB burst size on BCM2835. Quote from BCM2835 datasheet (p. 204): The USB_GAHBCFG register has been adapted. Bits [4:1] which are marked in the Synopsys documentation as "Burst Length/Type (HBstLen)" have been used differently. Your patch is doing two different things, which should better be split up. So in case of a revert only one part is affected. Stefan > > >>usb: dwc2: Define PCGCCTL1 register in hw.h > >>usb: dwc2: Define Active Clock Gating support bit in GHWCFG4 > > > > I think this one should be merged into the first patch which uses this > > define. > > > > I am ok with this observation, i will change it. > > Best regards, > Razmik -- 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] usb: dwc2: Fix endless deferral probe
The dwc2 USB driver tries to find a generic PHY first and then look
for an old style USB PHY. In case of a valid generic PHY node without
a PHY driver, the PHY layer will return -EPROBE_DEFER forever. So dwc2
will never tries for an USB PHY.
Fix this issue by finding a generic PHY and an old style USB PHY
at once.
Fixes: 6c2dad69163f ("usb: dwc2: Return errors from PHY")
Link: https://marc.info/?l=linux-usb&m=151518314314753&w=2
Signed-off-by: Stefan Wahren
---
drivers/usb/dwc2/platform.c | 42 --
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 3e26550..5279567 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -225,10 +225,11 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
hsotg->phyif = GUSBCFG_PHYIF16;
/*
-* Attempt to find a generic PHY, then look for an old style
-* USB PHY and then fall back to pdata
+* Attempt to find a generic PHY or an old style USB PHY at once
+* otherwise fall back to pdata
*/
hsotg->phy = devm_phy_get(hsotg->dev, "usb2-phy");
+ hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);
if (IS_ERR(hsotg->phy)) {
ret = PTR_ERR(hsotg->phy);
switch (ret) {
@@ -237,29 +238,34 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
hsotg->phy = NULL;
break;
case -EPROBE_DEFER:
- return ret;
+ if (IS_ERR(hsotg->uphy))
+ return ret;
+
+ hsotg->phy = NULL;
+ break;
default:
dev_err(hsotg->dev, "error getting phy %d\n", ret);
return ret;
}
}
- if (!hsotg->phy) {
- hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);
- if (IS_ERR(hsotg->uphy)) {
- ret = PTR_ERR(hsotg->uphy);
- switch (ret) {
- case -ENODEV:
- case -ENXIO:
- hsotg->uphy = NULL;
- break;
- case -EPROBE_DEFER:
- return ret;
- default:
- dev_err(hsotg->dev, "error getting usb phy
%d\n",
- ret);
+ if (IS_ERR(hsotg->uphy)) {
+ ret = PTR_ERR(hsotg->uphy);
+ switch (ret) {
+ case -ENODEV:
+ case -ENXIO:
+ hsotg->uphy = NULL;
+ break;
+ case -EPROBE_DEFER:
+ if (!hsotg->phy)
return ret;
- }
+
+ hsotg->uphy = NULL;
+ break;
+ default:
+ dev_err(hsotg->dev, "error getting usb phy %d\n",
+ ret);
+ return ret;
}
}
--
2.7.4
--
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] Documentation: usb: fix typo in UVC gadgetfs config command
This seems to be a copy&paste error. With the fix the uvc gadget now can be created by following the instrucitons. Signed-off-by: Bin Liu --- Documentation/usb/gadget-testing.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/usb/gadget-testing.txt b/Documentation/usb/gadget-testing.txt index 441a4b9b666f..5908a21fddb6 100644 --- a/Documentation/usb/gadget-testing.txt +++ b/Documentation/usb/gadget-testing.txt @@ -693,7 +693,7 @@ such specification consists of a number of lines with an inverval value in each line. The rules stated above are best illustrated with an example: # mkdir functions/uvc.usb0/control/header/h -# cd functions/uvc.usb0/control/header/h +# cd functions/uvc.usb0/control/ # ln -s header/h class/fs # ln -s header/h class/ss # mkdir -p functions/uvc.usb0/streaming/uncompressed/u/360p -- 1.9.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: Re: dvb usb issues since kernel 4.9
On Tue, Jan 9, 2018 at 9:57 AM, Eric Dumazet wrote: > > Your patch considers TASKLET_SOFTIRQ being a candidate for 'immediate > handling', but TCP Small queues heavily use TASKLET, > so as far as I am concerned a revert would have the same effect. Does it actually? TCP ends up dropping packets outside of the window etc, so flooding a machine with TCP packets and causing some further processing up the stack sounds very different from the basic packet flooding thing that happens with NET_RX_SOFTIRQ. Also, honestly, the kinds of people who really worry about flooding tend to have packet filtering in the receive path etc. So I really think "you can use up 90% of CPU time with a UDP packet flood from the same network" is very very very different - and honestly not at all as important - as "you want to be able to use a USB DVB receiver and watch/record TV". Because that whole "UDP packet flood from the same network" really is something you _fundamentally_ have other mitigations for. I bet that whole commit was introduced because of a benchmark test, rather than real life. No? In contrast, now people are complaining about real loads not working. Linus -- 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: Re: dvb usb issues since kernel 4.9
On Tue, Jan 9, 2018 at 9:48 AM, Linus Torvalds
wrote:
> On Tue, Jan 9, 2018 at 9:27 AM, Eric Dumazet wrote:
>>
>> So yes, commit 4cd13c21b207 ("softirq: Let ksoftirqd do its job") has
>> shown up multiple times in various 'regressions'
>> simply because it could surface the problem more often.
>> But even if you revert it, you can still make the faulty
>> driver/subsystem misbehave by adding more stress to the cpu handling
>> the IRQ.
>
> ..but that's always true. People sometimes live on the edge - often by
> design (ie hardware has been designed/selected to be the crappiest
> possible that still work).
>
> That doesn't change anything. A patch that takes "bad things can
> happen" to "bad things DO happen" is a bad patch.
I was expecting that people could get a chance to fix the root cause,
instead of trying to keep status quo.
Strangely, it took 18 months for someone to complain enough and
'bisect to this commit'
Your patch considers TASKLET_SOFTIRQ being a candidate for 'immediate
handling', but TCP Small queues heavily use TASKLET,
so as far as I am concerned a revert would have the same effect.
--
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: dvb usb issues since kernel 4.9
On Tue, Jan 9, 2018 at 9:42 AM, Mauro Carvalho Chehab wrote: > > On my preliminar tests, writing to a file on an ext4 partition at a > USB stick loses data up to the point to make it useless (1/4 of the data > is lost!). However, writing to a class 10 microSD card is doable. Note that most USB sticks are horrible crap. They can have write latencies counted in _seconds_. You can cause VM issues and various other non-hardware stalls with them, simply because something gets stuck waiting for a page writeout that should take a few ms on any reasonable hardware, but ends up talking half a second or more. For example, even really well-written software that tries to do things like threaded write-behind to smooth out the IO will be _totally_ screwed by the USB stick behavior (where you might write a few MB at high speeds, and then the next write - however small - takes a second because the stupid USB stick does a synchronous garbage collection. Suddenly all that clever software that tried to keep things moving along smoothly without any hiccups, and tried hard to make the USB bus have a nice constant loadm can't do anything at all about the crap hardware. So when testing writes to USB sticks, I'm not convinced you're actually testing any USB bus limitations or even really any other hardware limitations than the USB stick itself. Linus -- 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: Re: dvb usb issues since kernel 4.9
On Tue, Jan 9, 2018 at 9:27 AM, Eric Dumazet wrote:
>
> So yes, commit 4cd13c21b207 ("softirq: Let ksoftirqd do its job") has
> shown up multiple times in various 'regressions'
> simply because it could surface the problem more often.
> But even if you revert it, you can still make the faulty
> driver/subsystem misbehave by adding more stress to the cpu handling
> the IRQ.
..but that's always true. People sometimes live on the edge - often by
design (ie hardware has been designed/selected to be the crappiest
possible that still work).
That doesn't change anything. A patch that takes "bad things can
happen" to "bad things DO happen" is a bad patch.
> Maybe the answer is to tune the kernel for small latencies at the
> price of small throughput (situation before the patch)
Generally we always want to tune for latency. Throughput is "easy",
but almost never interesting.
Sure, people do batch jobs. And yes, people often _benchmark_
throughput, because it's easy to benchmark. It's much harder to
benchmark latency, even when it's often much more important.
A prime example is the SSD benchmarks in the last few years - they
improved _dramatically_ when people noticed that the real problem was
latency, not the idiotic maximum big-block bandwidth numbers that have
almost zero impact on most people.
Put another way: we already have a very strong implicit bias towards
bandwidth just because it's easier to see and measure.
That means that we generally should strive to have a explicit bias
towards optimizing for latency when that choice comes up. Just to
balance things out (and just to not take the easy way out: bandwidth
can often be improved by adding more layers of buffering and bigger
buffers, and that often ends up really hurting latency).
> 1) Revert the patch
Well, we can revert it only partially - limiting it to just networking
for example.
Just saying "act the way you used to for tasklets" already seems to
have fixed the issue in DVB.
> 2) get rid of ksoftirqd since it adds unexpected latencies.
We can't get rid of it entirely, since the synchronous softirq code
can cause problems too. It's why we have that "maximum of ten
synchronous events" in __do_softirq().
And we don't *want* to get rid of it.
We've _always_ had that small-scale "at some point we can't do it
synchronously any more".
That is a small-scale "don't have horrible latency for _other_ things"
protection. So it's about latency too, it's just about protecting
latency of the rest of the system.
The problem with commit 4cd13c21b207 is that it turns the small-scale
latency issues in softirq handling (they get larger latencies for lots
of hardware interrupts or even from non-preemptible kernel code) into
the _huge_ scale latency of scheduling, and does so in a racy way too.
> 3) Let applications that expect to have high throughput make sure to
> pin their threads on cpus that are not processing IRQ.
> (And make sure to not use irqbalance, and setup IRQ cpu affinities)
The only people that really deal in "thoughput only" tend to be the
HPC people, and they already do things like this.
(The other end of the spectrum is the realtime people that have
extreme latency requirements, who do things like that for the reverse
reason: keeping one or more CPU's reserved for the particular
low-latency realtime job).
Linus
--
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 V2] USBIP: return correct port ENABLE status
On 12/18/2017 11:00 PM, pei.zh...@intel.com wrote: > From: Pei Zhang > > USB system will clear port's ENABLE feature for some USB devices when > vdev is already assigned port address. This cause getPortStatus reports > to system that this device is not enabled, client OS will failed to use > this usb device. > > The failure devices include a SAMSUNG SSD storage, Logitech webcam C920. > > V2: send again to all related maintainers. > > Signed-off-by: Pei Zhang Hi Pei Zhang, Can you elaborate on how you can trigger this condition? Can you send me more information on how to recreate the problem and demsg from server and client side when this problem happens. thanks, -- Shuah -- 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: dvb usb issues since kernel 4.9
Em Mon, 8 Jan 2018 11:51:04 -0800
Linus Torvalds escreveu:
> On Mon, Jan 8, 2018 at 11:15 AM, Alan Stern wrote:
> >
> > Both dwc2_hsotg and ehci-hcd use the tasklets embedded in the
> > giveback_urb_bh member of struct usb_hcd. See usb_hcd_giveback_urb()
> > in drivers/usb/core/hcd.c; the calls are
> >
> > else if (high_prio_bh)
> > tasklet_hi_schedule(&bh->bh);
> > else
> > tasklet_schedule(&bh->bh);
> >
> > As it turns out, high_prio_bh gets set for interrupt and isochronous
> > URBs but not for bulk and control URBs. The DVB driver in question
> > uses bulk transfers.
>
> Ok, so we could try out something like the appended?
>
> NOTE! I have not tested this at all. It LooksObvious(tm), but...
>
> Linus
> kernel/softirq.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 2f5e87f1bae2..97b080956fea 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -79,12 +79,16 @@ static void wakeup_softirqd(void)
>
> /*
> * If ksoftirqd is scheduled, we do not want to process pending softirqs
> - * right now. Let ksoftirqd handle this at its own rate, to get fairness.
> + * right now. Let ksoftirqd handle this at its own rate, to get fairness,
> + * unless we're doing some of the synchronous softirqs.
> */
> -static bool ksoftirqd_running(void)
> +#define SOFTIRQ_NOW_MASK ((1 << HI_SOFTIRQ) | (1 << TASKLET_SOFTIRQ))
> +static bool ksoftirqd_running(unsigned long pending)
> {
> struct task_struct *tsk = __this_cpu_read(ksoftirqd);
>
> + if (pending & SOFTIRQ_NOW_MASK)
> + return false;
> return tsk && (tsk->state == TASK_RUNNING);
> }
>
> @@ -325,7 +329,7 @@ asmlinkage __visible void do_softirq(void)
>
> pending = local_softirq_pending();
>
> - if (pending && !ksoftirqd_running())
> + if (pending && !ksoftirqd_running(pending))
> do_softirq_own_stack();
>
> local_irq_restore(flags);
> @@ -352,7 +356,7 @@ void irq_enter(void)
>
> static inline void invoke_softirq(void)
> {
> - if (ksoftirqd_running())
> + if (ksoftirqd_running(local_softirq_pending()))
> return;
>
> if (!force_irqthreads) {
Hi Linus,
Patch makes sense to me, although I was not able to test it myself.
I set a RPi3 machine here with vanilla Kernel 4.14.11 running a standard
raspbian distribution (with elevator=deadline). Right now, I'm trying to
reproduce the bug with dvbv5-zap. I may eventually do more tests on
some other slow machines.
Usually, applications like tvheadend records just one channel. So, instead
of a ~58 Mbits/s payload, it uses, typically, ~11 Mbits/s for a HD channel.
This is usually filtered by hardware. Here, I'm forcing to record the
entire TS, in order to make easier to reproduce the issue. So, I'm forcing
a condition that it is usually worse than real usecases (at last for HD - I
I don't have any DVB stream here with a 4K channel).
>From what I checked so far, with vanila upstream Kernel on RPi3, just
receiving a DVB stream - or receiving it and writing to /dev/null works
with or without your patch.
The problem starts to happen when there are concurrency with writes.
On my preliminar tests, writing to a file on an ext4 partition at a
USB stick loses data up to the point to make it useless (1/4 of the data
is lost!). However, writing to a class 10 microSD card is doable.
If you're curious enough, this is what I'm doing (that are the results
while using class 10 microSD card):
$ FILE=/tmp/out.ts; for i in $(seq 1 6); do echo "step $i"; rm $FILE
2>/dev/null; dvbv5-zap -l universal -c ~/vivo-channels.conf NBR -o $FILE -P
-t60 2>&1|grep -E "(buffer|received)"; du $FILE 2>/dev/null; done
step 1
Setting buffer length to 725
buffer overrun
buffer overrun
buffer overrun
buffer overrun
buffer overrun
buffer overrun
buffer overrun
received 347504652 bytes (5656 Kbytes/sec)
339368 /tmp/out.ts
step 2
Setting buffer length to 725
buffer overrun
received 408995880 bytes (6656 Kbytes/sec)
399416 /tmp/out.ts
step 3
Setting buffer length to 725
received 412999716 bytes (6722 Kbytes/sec)
403328 /tmp/out.ts
step 4
Setting buffer length to 725
buffer overrun
received 415564788 bytes (6763 Kbytes/sec)
405832 /tmp/out.ts
step 5
Setting buffer length to 725
received 412999716 bytes (6722 Kbytes/sec)
403324 /tmp/out.ts
step 6
Setting buffer length to 725
received 408366080 bytes (6646 Kbytes/sec)
398796 /tmp/out.ts
My plan is to do more tests along this week, and try to tweak a little
bit both userspace and kernelspace, in order to see if I can get better
results.
Thanks,
Mauro
--
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: Re: dvb usb issues since kernel 4.9
On Tue, Jan 9, 2018 at 8:51 AM, Josef Griebichler
wrote:
> Hi Linus,
>
> your patch works very good for me and others (please see
> https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/?postID=77006#post77006).
> No errors in recordings any more.
> The patch was also tested on x86_64 (Revo 3700) with positive effect.
> I agree with the forum poster, that there's still an issue when recording and
> watching livetv at same time. I also get audio dropouts and audio is out of
> sync.
> According to user smp kernel 4.9.73 with your patch on rpi and according to
> user jahutchi kernel 4.11.12 on x86_64 have no such issues.
> I don't know if this dropouts are related to this topic.
>
> If of any help I could provide perf output on raspberry with libreelec and
> tvheadend.
>
Sorry to come late to the party.
It seems problem comes from some piece of hardware/driver having some
precise timing prereq, and opportunistic use of softirq/tasklet
(instead maybe of hard irq handlers )
While it is true that softirq might do the job in most cases, we
already have cases where this can be easily defeated,
say if one cpu has suddenly to handle multiple sources of interrupts
for various devices.
NET_RX can easily lock the cpu for 10ms (on HZ=100 builds)
So yes, commit 4cd13c21b207 ("softirq: Let ksoftirqd do its job") has
shown up multiple times in various 'regressions'
simply because it could surface the problem more often.
But even if you revert it, you can still make the faulty
driver/subsystem misbehave by adding more stress to the cpu handling
the IRQ.
Note that networking lacks fine control of its softirq processing.
Some people found/complained that relying more on ksoftirqd was
potentially adding tail latencies.
Maybe the answer is to tune the kernel for small latencies at the
price of small throughput (situation before the patch)
1) Revert the patch
2) get rid of ksoftirqd since it adds unexpected latencies.
3) Let applications that expect to have high throughput make sure to
pin their threads on cpus that are not processing IRQ.
(And make sure to not use irqbalance, and setup IRQ cpu affinities)
--
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
Aw: Re: dvb usb issues since kernel 4.9
Hi Linus, your patch works very good for me and others (please see https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/?postID=77006#post77006). No errors in recordings any more. The patch was also tested on x86_64 (Revo 3700) with positive effect. I agree with the forum poster, that there's still an issue when recording and watching livetv at same time. I also get audio dropouts and audio is out of sync. According to user smp kernel 4.9.73 with your patch on rpi and according to user jahutchi kernel 4.11.12 on x86_64 have no such issues. I don't know if this dropouts are related to this topic. If of any help I could provide perf output on raspberry with libreelec and tvheadend. Regards, Josef Gesendet: Montag, 08. Januar 2018 um 23:16 Uhr Von: "Jesper Dangaard Brouer" An: "Peter Zijlstra" Cc: "Josef Griebichler" , "Mauro Carvalho Chehab" , "Alan Stern" , "Greg Kroah-Hartman" , linux-usb@vger.kernel.org, "Eric Dumazet" , "Rik van Riel" , "Paolo Abeni" , "Hannes Frederic Sowa" , linux-kernel , netdev , "Jonathan Corbet" , LMML , "David Miller" , torva...@linux-foundation.org Betreff: Re: dvb usb issues since kernel 4.9 On Mon, 8 Jan 2018 22:44:27 +0100 Peter Zijlstra wrote: > On Mon, Jan 08, 2018 at 10:31:09PM +0100, Jesper Dangaard Brouer wrote: > > I did expected the issue to get worse, when you load the Pi with > > network traffic, as now the softirq time-budget have to be shared > > between networking and USB/DVB. Thus, I guess you are running TCP and > > USB/mpeg2ts on the same CPU (why when you have 4 CPUs?...) > > Isn't networking also over USB on the Pi ? Darn, that is true. Looking at the dmesg output in http://ix.io/DOg: [ 0.405942] usbcore: registered new interface driver smsc95xx [ 5.821104] smsc95xx 1-1.1:1.0 eth0: link up, 100Mbps, full-duplex, lpa 0x45E1 I don't know enough about USB... is it possible to control which CPU handles the individual USB ports, or on some other level (than ports)? -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer[http://www.linkedin.com/in/brouer] -- 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 v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Tuesday, January 9, 2018 5:03:18 PM CET Rafael J. Wysocki wrote:
> On Tue, Jan 9, 2018 at 4:30 PM, Geert Uytterhoeven
> wrote:
> > Hi Rafael,
> >
> > CC usb
> >
> > On Tue, Jan 9, 2018 at 4:00 PM, Rafael J. Wysocki wrote:
> >> On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven
> >> wrote:
> >>> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki
> >>> wrote:
> From: Rafael J. Wysocki
>
> One of the limitations of pm_runtime_force_suspend/resume() is that
> if a parent driver wants to use these functions, all of its child
> drivers have to do that too because of the parent usage counter
> manipulations necessary to get the correct state of the parent during
> system-wide transitions to the working state (system resume).
> However, that limitation turns out to be artificial, so remove it.
>
> Namely, pm_runtime_force_suspend() only needs to update the children
> counter of its parent (if there's is a parent) when the device can
> stay in suspend after the subsequent system resume transition, as
> that counter is correct already otherwise. Now, if the parent's
> children counter is not updated, it is not necessary to increment
> the parent's usage counter in that case any more, as long as the
> children counters of devices are checked along with their usage
> counters in order to decide whether or not the devices may be left
> in suspend after the subsequent system resume transition.
>
> Accordingly, modify pm_runtime_force_suspend() to only call
> pm_runtime_set_suspended() for devices whose usage and children
> counters are at the "no references" level (the runtime PM status
> of the device needs to be updated to "suspended" anyway in case
> this function is called once again for the same device during the
> transition under way), drop the parent usage counter incrementation
> from it and update pm_runtime_force_resume() to compensate for these
> changes.
>
> Signed-off-by: Rafael J. Wysocki
> >>>
> >>> This patch causes a regression during system resume on Renesas Salvator-XS
> >>> with R-Car H3 ES2.0:
> >>
> >> I have dropped it for now, but we need to address the underlying issue.
> >>
> >>> SError Interrupt on CPU3, code 0xbf02 -- SError
> >>> SError Interrupt on CPU2, code 0xbf02 -- SError
> >>> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
> >>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> >>> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
> >>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
> >>> Hardware name: Renesas Salvator-X 2nd version board based on
> >>> r8a7795 ES2.0+ (DT)
> >>> Hardware name: Renesas Salvator-X 2nd version board based on
> >>> r8a7795 ES2.0+ (DT)
> >>> Workqueue: events_unbound async_run_entry_fn
> >>> Workqueue: events_unbound async_run_entry_fn
> >>> pstate: 6005 (nZCv daif -PAN -UAO)
> >>> pstate: 6005 (nZCv daif -PAN -UAO)
> >>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> >>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
> >>> lr : phy_init+0x64/0xcc
> >>> lr : phy_init+0x64/0xcc
> >>> ...
> >>> Kernel panic - not syncing: Asynchronous SError Interrupt
> >>>
> >>> Note that before, it printed a warning instead:
> >>>
> >>> Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
> >>> active children
> >>> WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
> >>> pm_runtime_enable+0x94/0xd8
> >>>
> >>> Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
> >>> pm_runtime_force_suspend/resume()") fixes the crash.
> >>>
> >>> Note that applying Ulf's "[PATCH v2 0/3] phy: core: Re-work runtime PM
> >>> deployment and fix an issue"
> >>> (https://www.spinics.net/lists/linux-renesas-soc/msg21719.html) instead
> >>> does not fix the crash.
> >>
> >> OK
> >>
> >>> With more debug code added, it seems the EHCI module clocks (701-703) are
> >>> enabled earlier than before. I guess this triggers the workqueue to
> >>> perform
> >>> an operation while another related device (HSUSB 704?) is still disabled?
> >>
> >> Probably.
> >>
> >> Likely a device that wasn't resumed before resumes now and that causes
> >> the issue to appear.
> >>
> >> I'm wondering if adding the ignore_children check to the patch helps.
> >> If not, there clearly is a resume ordering issue that is papered over
> >> by the current code.
> >
> > Something fishy is going on. Status of the USB PHYs differ before/after
> > system suspend, according to /sys/kernel/debug/pm_genpd/pm_genpd_summary:
> >
> > -/devices/platform/soc/ee0a0200.usb-phy active
> > -/devices/platform/soc/ee0c0200.usb-phy active
> > -/devices/platform/soc/ee080200.usb-phy active
> > +/devices/platform/soc/ee0a0200.usb-phy suspended
> > +/devices/platform/soc/ee0c0200.usb-phy
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
On Tue, Jan 9, 2018 at 4:30 PM, Geert Uytterhoeven wrote:
> Hi Rafael,
>
> CC usb
>
> On Tue, Jan 9, 2018 at 4:00 PM, Rafael J. Wysocki wrote:
>> On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven
>> wrote:
>>> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki
>>> wrote:
From: Rafael J. Wysocki
One of the limitations of pm_runtime_force_suspend/resume() is that
if a parent driver wants to use these functions, all of its child
drivers have to do that too because of the parent usage counter
manipulations necessary to get the correct state of the parent during
system-wide transitions to the working state (system resume).
However, that limitation turns out to be artificial, so remove it.
Namely, pm_runtime_force_suspend() only needs to update the children
counter of its parent (if there's is a parent) when the device can
stay in suspend after the subsequent system resume transition, as
that counter is correct already otherwise. Now, if the parent's
children counter is not updated, it is not necessary to increment
the parent's usage counter in that case any more, as long as the
children counters of devices are checked along with their usage
counters in order to decide whether or not the devices may be left
in suspend after the subsequent system resume transition.
Accordingly, modify pm_runtime_force_suspend() to only call
pm_runtime_set_suspended() for devices whose usage and children
counters are at the "no references" level (the runtime PM status
of the device needs to be updated to "suspended" anyway in case
this function is called once again for the same device during the
transition under way), drop the parent usage counter incrementation
from it and update pm_runtime_force_resume() to compensate for these
changes.
Signed-off-by: Rafael J. Wysocki
>>>
>>> This patch causes a regression during system resume on Renesas Salvator-XS
>>> with R-Car H3 ES2.0:
>>
>> I have dropped it for now, but we need to address the underlying issue.
>>
>>> SError Interrupt on CPU3, code 0xbf02 -- SError
>>> SError Interrupt on CPU2, code 0xbf02 -- SError
>>> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
>>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
>>> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
>>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
>>> Hardware name: Renesas Salvator-X 2nd version board based on
>>> r8a7795 ES2.0+ (DT)
>>> Hardware name: Renesas Salvator-X 2nd version board based on
>>> r8a7795 ES2.0+ (DT)
>>> Workqueue: events_unbound async_run_entry_fn
>>> Workqueue: events_unbound async_run_entry_fn
>>> pstate: 6005 (nZCv daif -PAN -UAO)
>>> pstate: 6005 (nZCv daif -PAN -UAO)
>>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
>>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
>>> lr : phy_init+0x64/0xcc
>>> lr : phy_init+0x64/0xcc
>>> ...
>>> Kernel panic - not syncing: Asynchronous SError Interrupt
>>>
>>> Note that before, it printed a warning instead:
>>>
>>> Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
>>> active children
>>> WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
>>> pm_runtime_enable+0x94/0xd8
>>>
>>> Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
>>> pm_runtime_force_suspend/resume()") fixes the crash.
>>>
>>> Note that applying Ulf's "[PATCH v2 0/3] phy: core: Re-work runtime PM
>>> deployment and fix an issue"
>>> (https://www.spinics.net/lists/linux-renesas-soc/msg21719.html) instead
>>> does not fix the crash.
>>
>> OK
>>
>>> With more debug code added, it seems the EHCI module clocks (701-703) are
>>> enabled earlier than before. I guess this triggers the workqueue to perform
>>> an operation while another related device (HSUSB 704?) is still disabled?
>>
>> Probably.
>>
>> Likely a device that wasn't resumed before resumes now and that causes
>> the issue to appear.
>>
>> I'm wondering if adding the ignore_children check to the patch helps.
>> If not, there clearly is a resume ordering issue that is papered over
>> by the current code.
>
> Something fishy is going on. Status of the USB PHYs differ before/after
> system suspend, according to /sys/kernel/debug/pm_genpd/pm_genpd_summary:
>
> -/devices/platform/soc/ee0a0200.usb-phy active
> -/devices/platform/soc/ee0c0200.usb-phy active
> -/devices/platform/soc/ee080200.usb-phy active
> +/devices/platform/soc/ee0a0200.usb-phy suspended
> +/devices/platform/soc/ee0c0200.usb-phy suspended
> +/devices/platform/soc/ee080200.usb-phy suspended
Yeah.
That's because of the pm_runtime_force_suspend/resume() called by
genpd. These functions generally may cause devices active before
system suspend to be left in suspend after it. That generally i
Re: [PATCH v2] PM / runtime: Rework pm_runtime_force_suspend/resume()
Hi Rafael,
CC usb
On Tue, Jan 9, 2018 at 4:00 PM, Rafael J. Wysocki wrote:
> On Tue, Jan 9, 2018 at 2:37 PM, Geert Uytterhoeven
> wrote:
>> On Wed, Jan 3, 2018 at 12:06 PM, Rafael J. Wysocki
>> wrote:
>>> From: Rafael J. Wysocki
>>>
>>> One of the limitations of pm_runtime_force_suspend/resume() is that
>>> if a parent driver wants to use these functions, all of its child
>>> drivers have to do that too because of the parent usage counter
>>> manipulations necessary to get the correct state of the parent during
>>> system-wide transitions to the working state (system resume).
>>> However, that limitation turns out to be artificial, so remove it.
>>>
>>> Namely, pm_runtime_force_suspend() only needs to update the children
>>> counter of its parent (if there's is a parent) when the device can
>>> stay in suspend after the subsequent system resume transition, as
>>> that counter is correct already otherwise. Now, if the parent's
>>> children counter is not updated, it is not necessary to increment
>>> the parent's usage counter in that case any more, as long as the
>>> children counters of devices are checked along with their usage
>>> counters in order to decide whether or not the devices may be left
>>> in suspend after the subsequent system resume transition.
>>>
>>> Accordingly, modify pm_runtime_force_suspend() to only call
>>> pm_runtime_set_suspended() for devices whose usage and children
>>> counters are at the "no references" level (the runtime PM status
>>> of the device needs to be updated to "suspended" anyway in case
>>> this function is called once again for the same device during the
>>> transition under way), drop the parent usage counter incrementation
>>> from it and update pm_runtime_force_resume() to compensate for these
>>> changes.
>>>
>>> Signed-off-by: Rafael J. Wysocki
>>
>> This patch causes a regression during system resume on Renesas Salvator-XS
>> with R-Car H3 ES2.0:
>
> I have dropped it for now, but we need to address the underlying issue.
>
>> SError Interrupt on CPU3, code 0xbf02 -- SError
>> SError Interrupt on CPU2, code 0xbf02 -- SError
>> CPU: 3 PID: 1769 Comm: kworker/u16:13 Not tainted
>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
>> CPU: 2 PID: 1774 Comm: kworker/u16:18 Not tainted
>> 4.15.0-rc7-arm64-renesas-05338-gf14cf570a813c9ca-dirty #97
>> Hardware name: Renesas Salvator-X 2nd version board based on
>> r8a7795 ES2.0+ (DT)
>> Hardware name: Renesas Salvator-X 2nd version board based on
>> r8a7795 ES2.0+ (DT)
>> Workqueue: events_unbound async_run_entry_fn
>> Workqueue: events_unbound async_run_entry_fn
>> pstate: 6005 (nZCv daif -PAN -UAO)
>> pstate: 6005 (nZCv daif -PAN -UAO)
>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
>> pc : rcar_gen3_phy_usb2_init+0x34/0xf8
>> lr : phy_init+0x64/0xcc
>> lr : phy_init+0x64/0xcc
>> ...
>> Kernel panic - not syncing: Asynchronous SError Interrupt
>>
>> Note that before, it printed a warning instead:
>>
>> Enabling runtime PM for inactive device (ee0a0200.usb-phy) with
>> active children
>> WARNING: CPU: 0 PID: 1741 at drivers/base/power/runtime.c:1300
>> pm_runtime_enable+0x94/0xd8
>>
>> Reverting commit 0408584d580d4a2c ("PM / runtime: Rework
>> pm_runtime_force_suspend/resume()") fixes the crash.
>>
>> Note that applying Ulf's "[PATCH v2 0/3] phy: core: Re-work runtime PM
>> deployment and fix an issue"
>> (https://www.spinics.net/lists/linux-renesas-soc/msg21719.html) instead
>> does not fix the crash.
>
> OK
>
>> With more debug code added, it seems the EHCI module clocks (701-703) are
>> enabled earlier than before. I guess this triggers the workqueue to perform
>> an operation while another related device (HSUSB 704?) is still disabled?
>
> Probably.
>
> Likely a device that wasn't resumed before resumes now and that causes
> the issue to appear.
>
> I'm wondering if adding the ignore_children check to the patch helps.
> If not, there clearly is a resume ordering issue that is papered over
> by the current code.
Something fishy is going on. Status of the USB PHYs differ before/after
system suspend, according to /sys/kernel/debug/pm_genpd/pm_genpd_summary:
-/devices/platform/soc/ee0a0200.usb-phy active
-/devices/platform/soc/ee0c0200.usb-phy active
-/devices/platform/soc/ee080200.usb-phy active
+/devices/platform/soc/ee0a0200.usb-phy suspended
+/devices/platform/soc/ee0c0200.usb-phy suspended
+/devices/platform/soc/ee080200.usb-phy suspended
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: s
Re: [PATCH] USB: core: Fix misuse of USB_DT_USB_SSP_CAP_SIZE()
On Wed, Dec 20, 2017 at 03:14:09AM +0900, Masakazu Mokuno wrote:
> As USB_DT_USB_SSP_CAP_SIZE() takes SSAC value as an argument, the low
> bound of the size for struct usb_ssp_cap_descriptor should be described
> by USB_DT_USB_SSP_CAP_SIZE(0), not by USB_DT_USB_SSP_CAP_SIZE(1)
>
> The type-specific length check patch was added to stable and needs to be
> fixed as well.
>
> Fixes: 81cf4a45360f ("USB: core: Add type-specific length check of BOS
> descriptors")
> Cc: linux-stable
> Cc: Mathias Nyman
> Signed-off-by: Masakazu Mokuno
>
> --
>
> drivers/usb/core/config.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 78e92d2..33a3ab7 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -911,7 +911,7 @@ void usb_release_bos_descriptor(struct usb_device *dev)
> [USB_CAP_TYPE_WIRELESS_USB] = USB_DT_USB_WIRELESS_CAP_SIZE,
> [USB_CAP_TYPE_EXT] = USB_DT_USB_EXT_CAP_SIZE,
> [USB_SS_CAP_TYPE] = USB_DT_USB_SS_CAP_SIZE,
> - [USB_SSP_CAP_TYPE] = USB_DT_USB_SSP_CAP_SIZE(1),
> + [USB_SSP_CAP_TYPE] = USB_DT_USB_SSP_CAP_SIZE(0),
> [CONTAINER_ID_TYPE] = USB_DT_USB_SS_CONTN_ID_SIZE,
> [USB_PTM_CAP_TYPE] = USB_DT_USB_PTM_ID_SIZE,
> };
>
> --
> Masakazu Mokuno
Patch is corrupted and can not be applied :(
--
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] The usbmon triggers a BUG in ./include/linux/mm.h
On Mon, Jan 08, 2018 at 03:46:41PM -0600, Pete Zaitcev wrote: > Automated tests triggered this by opening usbmon and accessing the > mmap while simultaneously resizing the buffers. This bug was with > us since 2006, because typically applications only size the buffers > once and thus avoid racing. Reported by Kirill A. Shutemov. > > Signed-off-by: Pete Zaitcev > --- > drivers/usb/mon/mon_bin.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) You forgot a reported-by line :( I'll go add it, it's been a while since you submitted a kernel patch, you must have forgotten :) 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: xhci: Clean up error code in xhci_dbc_tty_register_device()
On 09.01.2018 11:41, Dan Carpenter wrote:
tty_port_register_device() returns error pointers on error, never NULL.
The IS_ERR_OR_NULL() function returns either 1 or 0 so it means we
return 1 on error instead of a proper error code. The caller only
checks for zero vs non-zero so this doesn't affect runtime.
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
index 8d47b6fbf973..6f534b6decef 100644
--- a/drivers/usb/host/xhci-dbgtty.c
+++ b/drivers/usb/host/xhci-dbgtty.c
@@ -443,9 +443,10 @@ int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
xhci_dbc_tty_init_port(xhci, port);
tty_dev = tty_port_register_device(&port->port,
dbc_tty_driver, 0, NULL);
- ret = IS_ERR_OR_NULL(tty_dev);
- if (ret)
+ if (IS_ERR(tty_dev)) {
+ ret = PTR_ERR(tty_dev);
goto register_fail;
+ }
ret = kfifo_alloc(&port->write_fifo, DBC_WRITE_BUF_SIZE, GFP_KERNEL);
if (ret)
--
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
Thanks, adding to queue, I'll send it forward after 4.16-rc1
-Mathias
--
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 v2 2/2] USB: serial: ark3116: Move TIOCGSERIAL ioctl case to function.
On Sat, Jan 06, 2018 at 08:15:22PM +0300, Mikhail Zaytsev wrote:
> The patch moves TIOCGSERIAL ioctl case to get_serial_info function.
>
> Signed-off-by: Mikhail Zaytsev
> static int ark3116_ioctl(struct tty_struct *tty,
>unsigned int cmd, unsigned long arg)
> {
> struct usb_serial_port *port = tty->driver_data;
> - struct serial_struct serstruct;
> - void __user *user_arg = (void __user *)arg;
I prefer keeping this pointer here to avoid adding casts to every helper
function call, so I added it back before applying.
>
> switch (cmd) {
> case TIOCGSERIAL:
> + return ark3116_get_serial_info(port,
> + (struct serial_struct __user *)arg);
Thanks,
Johan
--
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 v2 1/2] USB: serial: ark3116: Remove unused TIOCSSERIAL ioctl case.
On Tue, Jan 09, 2018 at 12:45:30AM +0300, Mikhail Zaytsev wrote:
> On Mon, 8 Jan 2018 16:28:58 +0100 Johan Hovold wrote:
>
> > On Mon, Jan 08, 2018 at 11:33:32AM +0100, Oliver Neukum wrote:
> > > Am Samstag, den 06.01.2018, 20:14 +0300 schrieb Mikhail Zaytsev:
> > > > The patch removes unused TIOCSSERIAL ioctl case and adds the default
> > > > block
> > > > to the switch. This will make the ioctl return -ENOTTY to user space
> > > > (e.g.
> > > > setserial), because TIOCSSERIAL really isn't supported for these devices
> > > > currently.
> > >
> > > Hi,
> > >
> > > this will break software that is now running on these devices,
> > > won't it? Do you know why those devices basically ignore the
> > > ioctl?
> >
> > Yeah, that was my initial reactions as well, but then again, any sane
> > user space cannot rely on these ioctl being implemented for all tty
> > devices.
> >
> > I did some digging now and these (dummy) ioctl implementations where
> > added by commit 2f430b4bbae7 ("USB: ark3116: Add TIOCGSERIAL and
> > TIOCSSERIAL ioctl calls.") back in 2006. This in turn appears to have
> > been triggered by a change in a user space tool, wvdial, which started
> > erroring out if either was missing.
> >
> > I found a couple of bug reports about that through google, and looking
> > at the wvstreams (library) code now, it looks like the issue has indeed
> > been resolved by handling errors more gracefully (e.g. just logging
> > them).
> >
> > So I'm willing to give this a try, and if anyone complains later we add
> > back (or implement) TIOCSSERIAL.
> >
>
> Thanks Johan. I looked the commit 2f430b4bbae7. Author just did a cut'n'paste
> from other USB serial drivers. I think that it would be better remove
> the TIOCGSERIAL implementation too.
I've applied this one now after adding some of the backstory from above
to the commit message.
Thanks,
Johan
--
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 V2 5/5] usb: serial: f81534: fix tx error on some baud rate
On Thu, Jan 04, 2018 at 10:29:21AM +0800, Ji-Ze Hong (Peter Hong) wrote: > The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates > can be up to 1.5Mbits with 24MHz. But on some baud rate (384~500kps), the > TX side will send the data frame too close to treat frame error on RX > side. This patch will force all TX data frame with delay 1bit gap. > > Signed-off-by: Ji-Ze Hong (Peter Hong) > --- > V2: > 1: First introduced in this series patches. > > drivers/usb/serial/f81534.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c > index a4666171239a..513805eeae6a 100644 > --- a/drivers/usb/serial/f81534.c > +++ b/drivers/usb/serial/f81534.c > @@ -130,6 +130,7 @@ > #define F81534_CLK_18_46_MHZ (F81534_UART_EN | BIT(1)) > #define F81534_CLK_24_MHZ(F81534_UART_EN | BIT(2)) > #define F81534_CLK_14_77_MHZ (F81534_UART_EN | BIT(1) | BIT(2)) > +#define F81534_CLK_TX_DELAY_1BIT BIT(3) > > #define F81534_CLK_RS485_MODEBIT(4) > #define F81534_CLK_RS485_INVERT BIT(5) > @@ -1438,6 +1439,11 @@ static int f81534_port_probe(struct usb_serial_port > *port) > break; > } > > + /* > + * We'll make tx frame error when baud rate from 384~500kps. So we'll > + * delay all tx data frame with 1bit. > + */ > + port_priv->shadow_clk |= F81534_CLK_TX_DELAY_1BIT; You don't wan't to enable this only for the affected rates? > return f81534_set_port_output_pin(port); > } Johan -- 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 V2 4/5] usb: serial: f81534: add H/W disable port support
On Thu, Jan 04, 2018 at 10:29:20AM +0800, Ji-Ze Hong (Peter Hong) wrote: > The F81532/534 can be disable port by manufacturer with > following H/W design. > 1: Connect DCD/DSR/CTS/RI pin to ground. > 2: Connect RX pin to ground. > > In driver, we'll implements some detect method likes following: > 1: Read MSR. > 2: Turn MCR LOOP bit on, off and read LSR after delay with 60ms. >It'll contain BREAK status in LSR. > > Signed-off-by: Ji-Ze Hong (Peter Hong) > --- > V2: > 1: f81534_check_port_hw_disabled() change return type from int to bool. > 2: Add help function f81534_set_phy_port_register() / > f81534_get_phy_port_register() for f81534_check_port_hw_disabled() > to read register without port. > 3: Re-write f81534_calc_num_ports() & f81534_attach() to reduce the > f81534_check_port_hw_disabled() repeatedly called. This looks good, but please split up the config-data-readout refactoring and f81534_check_port_hw_disabled() changes in two patches. Johan -- 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 V2 3/5] usb: serial: f81534: add output pin control
On Thu, Jan 04, 2018 at 10:29:19AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 3 output pin (M0/SD, M1, M2) with open-drain mode to
> control transceiver. We'll read it from internal Flash with address
> 0x2f05~0x2f08 for 4 ports. The value is range from 0 to 7. The M0/SD is
> MSB of this value. For a examples, If read value is 6, we'll write M0/SD,
> M1, M2 as 1, 1, 0.
>
> Signed-off-by: Ji-Ze Hong (Peter Hong)
> ---
> V2:
> 1: Fix for space between brace.
> 2: Remain the old pin control method.
>
> drivers/usb/serial/f81534.c | 67
> -
> 1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 8a778bc1d492..7f175f39a171 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -52,6 +52,7 @@
> #define F81534_CUSTOM_NO_CUSTOM_DATA 0xff
> #define F81534_CUSTOM_VALID_TOKEN0xf0
> #define F81534_CONF_OFFSET 1
> +#define F81534_CONF_GPIO_OFFSET 4
>
> #define F81534_MAX_DATA_BLOCK64
> #define F81534_MAX_BUS_RETRY 20
> @@ -164,6 +165,23 @@ struct f81534_port_private {
> u8 phy_num;
> };
>
> +struct f81534_pin_data {
> + const u16 reg_addr;
> + const u16 reg_mask;
I asked in my previous review comments whether this mask should really
be u8?
> +};
> +
> +struct f81534_port_out_pin {
> + struct f81534_pin_data pin[3];
> +};
> +
> +/* Pin output value for M2/M1/M0(SD) */
> +static const struct f81534_port_out_pin f81534_port_out_pins[] = {
> + { { {0x2ae8, BIT(7)}, {0x2a90, BIT(5)}, {0x2a90, BIT(4) } } },
> + { { {0x2ae8, BIT(6)}, {0x2ae8, BIT(0)}, {0x2ae8, BIT(3) } } },
> + { { {0x2a90, BIT(0)}, {0x2ae8, BIT(2)}, {0x2a80, BIT(6) } } },
> + { { {0x2a90, BIT(3)}, {0x2a90, BIT(2)}, {0x2a90, BIT(1) } } },
Nit picking, but you still don't use space consistently around { and }
above.
Johan
--
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 V2 2/5] usb: serial: f81534: add auto RTS direction support
On Thu, Jan 04, 2018 at 10:29:18AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had auto RTS direction support for RS485 mode.
> We'll read it from internal Flash with address 0x2f01~0x2f04 for 4 ports.
> There are 4 conditions below:
> 0: F81534_PORT_CONF_RS232.
> 1: F81534_PORT_CONF_RS485.
> 2: value error, default to F81534_PORT_CONF_RS232.
> 3: F81534_PORT_CONF_RS485_INVERT.
>
> F81532/534 Clock register (offset +08h)
>
> Bit0: UART Enable (always on)
> Bit2-1: Clock source selector
> 00: 1.846MHz.
> 01: 18.46MHz.
> 10: 24MHz.
> 11: 14.77MHz.
> Bit4: Auto direction(RTS) control (RTS pin Low when TX)
> Bit5: Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
>
> Signed-off-by: Ji-Ze Hong (Peter Hong)
> ---
> V2:
> 1: Read the configure data from flash and save it to shadow clock
> register.
>
> drivers/usb/serial/f81534.c | 34 +-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 758ef0424164..8a778bc1d492 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -98,11 +98,16 @@
>
> #define F81534_DEFAULT_BAUD_RATE 9600
>
> +#define F81534_PORT_CONF_RS232 0
> +#define F81534_PORT_CONF_RS485 BIT(0)
> +#define F81534_PORT_CONF_RS485_INVERT(BIT(0) | BIT(1))
> #define F81534_PORT_CONF_DISABLE_PORTBIT(3)
> #define F81534_PORT_CONF_NOT_EXIST_PORT BIT(7)
> #define F81534_PORT_UNAVAILABLE \
> (F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
>
> +#define F81534_UART_MODE_MASK(BIT(0) | BIT(1))
Use GENMASK()?
> +
> #define F81534_1X_RXTRIGGER 0xc3
> #define F81534_8X_RXTRIGGER 0xcf
>
> @@ -115,6 +120,8 @@
> * 01: 18.46MHz.
> * 10: 24MHz.
> * 11: 14.77MHz.
> + * Bit4: Auto direction(RTS) control (RTS pin Low when TX)
> + * Bit5: Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
> */
>
> #define F81534_UART_EN BIT(0)
> @@ -123,6 +130,9 @@
> #define F81534_CLK_24_MHZ(F81534_UART_EN | BIT(2))
> #define F81534_CLK_14_77_MHZ (F81534_UART_EN | BIT(1) | BIT(2))
>
> +#define F81534_CLK_RS485_MODEBIT(4)
> +#define F81534_CLK_RS485_INVERT BIT(5)
> +
> static const struct usb_device_id f81534_id_table[] = {
> { USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
> { USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
> @@ -517,7 +527,8 @@ static int f81534_set_port_config(struct usb_serial_port
> *port,
> }
>
> port_priv->baud_base = baudrate_table[idx];
> - port_priv->shadow_clk = clock_table[idx];
> + port_priv->shadow_clk &= ~F81534_CLK_14_77_MHZ;
Add a dedicated mask define instead of using a clock value here. No need
to clear the enable bit for example (which shouldn't be part of the
clock value as I mentioned earlier).
> + port_priv->shadow_clk |= clock_table[idx];
>
> status = f81534_set_port_register(port, F81534_CLOCK_REG,
> port_priv->shadow_clk);
> @@ -1269,9 +1280,12 @@ static void f81534_lsr_worker(struct work_struct *work)
>
> static int f81534_port_probe(struct usb_serial_port *port)
> {
> + struct f81534_serial_private *serial_priv;
> struct f81534_port_private *port_priv;
> int ret;
> + u8 value;
>
> + serial_priv = usb_get_serial_data(port->serial);
> port_priv = devm_kzalloc(&port->dev, sizeof(*port_priv), GFP_KERNEL);
> if (!port_priv)
> return -ENOMEM;
> @@ -1303,6 +1317,24 @@ static int f81534_port_probe(struct usb_serial_port
> *port)
> if (ret)
> return ret;
>
> + value = serial_priv->conf_data[port_priv->phy_num];
> + switch (value & F81534_UART_MODE_MASK) {
> + case F81534_PORT_CONF_RS485_INVERT:
> + port_priv->shadow_clk = F81534_CLK_RS485_MODE |
> + F81534_CLK_RS485_INVERT;
> + dev_info(&port->dev, "RS485 invert mode.\n");
> + break;
> + case F81534_PORT_CONF_RS485:
> + port_priv->shadow_clk = F81534_CLK_RS485_MODE;
> + dev_info(&port->dev, "RS485 mode.\n");
> + break;
> +
> + default:
> + case F81534_PORT_CONF_RS232:
> + dev_info(&port->dev, "RS232 mode.\n");
Again, I think these dev_info should really be dev_dbg.
> + break;
> + }
> +
> return 0;
> }
Johan
--
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 V2 1/5] usb: serial: f81534: add high baud rate support
On Thu, Jan 04, 2018 at 10:29:17AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had 4 clocksource 1.846/18.46/14.77/24MHz and baud rates
> can be up to 1.5Mbits with 24MHz.
>
> This device may generate data overrun when baud rate setting to 921600bps
> or higher with old UART trigger level setting (8x14=112) with full
> loading. We'll change trigger level from 8x14=112 to 8x8=64 to avoid data
> overrun.
>
> Also the read/write of EP0 will be affected by this patch. The worst case
> of responding time is 20s when all serial port are full loading and trying
> to access EP0, so we change EP0 timeout from 10 to 20s.
Surely you meant 1 and 2 seconds respectively here? And if you have
indeed measured response times close to 2000 ms then perhaps you want to
add even more margin?
> F81532/534 Clock register (offset +08h)
>
> Bit0: UART Enable (always on)
> Bit2-1: Clock source selector
> 00: 1.846MHz.
> 01: 18.46MHz.
> 10: 24MHz.
> 11: 14.77MHz.
>
> Signed-off-by: Ji-Ze Hong (Peter Hong)
> ---
> v2:
> 1: Add commit message for F81534_USB_TIMEOUT from 1000 to 2000 and
> trigger level from UART_FCR_R_TRIG_11 to UART_FCR_TRIGGER_8.
> 2: Separate UART Enable bit from clock sources.
> 3: Add help function "f81534_find_clk()" to calculate baud rate and
> clock source
> 4: Add shadow clock register for future use.
>
> drivers/usb/serial/f81534.c | 87
> -
> 1 file changed, 71 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index e4573b4c8935..758ef0424164 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -41,6 +41,7 @@
> #define F81534_MODEM_CONTROL_REG (0x04 + F81534_UART_BASE_ADDRESS)
> #define F81534_LINE_STATUS_REG (0x05 +
> F81534_UART_BASE_ADDRESS)
> #define F81534_MODEM_STATUS_REG (0x06 +
> F81534_UART_BASE_ADDRESS)
> +#define F81534_CLOCK_REG (0x08 + F81534_UART_BASE_ADDRESS)
> #define F81534_CONFIG1_REG (0x09 + F81534_UART_BASE_ADDRESS)
>
> #define F81534_DEF_CONF_ADDRESS_START0x3000
> @@ -57,7 +58,7 @@
>
> /* Default URB timeout for USB operations */
> #define F81534_USB_MAX_RETRY 10
> -#define F81534_USB_TIMEOUT 1000
> +#define F81534_USB_TIMEOUT 2000
> #define F81534_SET_GET_REGISTER 0xA0
>
> #define F81534_NUM_PORT 4
> @@ -96,7 +97,6 @@
> #define F81534_CMD_READ 0x03
>
> #define F81534_DEFAULT_BAUD_RATE 9600
> -#define F81534_MAX_BAUDRATE 115200
>
> #define F81534_PORT_CONF_DISABLE_PORTBIT(3)
> #define F81534_PORT_CONF_NOT_EXIST_PORT BIT(7)
> @@ -106,6 +106,23 @@
> #define F81534_1X_RXTRIGGER 0xc3
> #define F81534_8X_RXTRIGGER 0xcf
>
> +/*
> + * F81532/534 Clock registers (offset +08h)
> + *
> + * Bit0: UART Enable (always on)
> + * Bit2-1: Clock source selector
> + * 00: 1.846MHz.
> + * 01: 18.46MHz.
> + * 10: 24MHz.
> + * 11: 14.77MHz.
> + */
> +
> +#define F81534_UART_EN BIT(0)
> +#define F81534_CLK_1_846_MHZ F81534_UART_EN
> +#define F81534_CLK_18_46_MHZ (F81534_UART_EN | BIT(1))
> +#define F81534_CLK_24_MHZ(F81534_UART_EN | BIT(2))
> +#define F81534_CLK_14_77_MHZ (F81534_UART_EN | BIT(1) | BIT(2))
I meant that you should keep the UART_EN define separate from the CLK
values and explicitly include it when you update the register (or just
once when you first initialise the shadow register during probe).
> +
> static const struct usb_device_id f81534_id_table[] = {
> { USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
> { USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
> @@ -129,12 +146,18 @@ struct f81534_port_private {
> struct usb_serial_port *port;
> unsigned long tx_empty;
> spinlock_t msr_lock;
> + u32 baud_base;
> u8 shadow_mcr;
> u8 shadow_lcr;
> u8 shadow_msr;
> + u8 shadow_clk;
> u8 phy_num;
> };
>
> +static u32 const baudrate_table[] = {115200, 921600, 1152000, 150};
> +static u8 const clock_table[] = {F81534_CLK_1_846_MHZ, F81534_CLK_14_77_MHZ,
> + F81534_CLK_18_46_MHZ, F81534_CLK_24_MHZ};
> +
> static int f81534_logic_to_phy_port(struct usb_serial *serial,
> struct usb_serial_port *port)
> {
> @@ -460,13 +483,48 @@ static u32 f81534_calc_baud_divisor(u32 baudrate, u32
> clockrate)
> return DIV_ROUND_CLOSEST(clockrate, baudrate);
> }
>
> -static int f81534_set_port_config(struct usb_serial_port *port, u32 baudrate,
> - u8 lcr)
> +static int f81534_find_clk(u32 baudrate)
Re: Help needed debugging Motorola Solutions TETRA PEI interface
On Mon, Jan 08, 2018 at 07:56:51PM +0100, Max Schulze wrote: > Thanks Johan for taking a look. > > > Am 08.01.2018 um 17:30 schrieb Johan Hovold: > > Adding the device ids and a quirk to cdc_acm.c > >>> .driver_info = NO_UNION_NORMAL, > >> does only suppress the "Zero length" message. > > Do you then get a ttyACMn device? Or some other error? > Nothing more. No ttyACM device . Only the > > [ 1745.845561] cdc_acm: probe of 1-1.4:1.1 failed with error -22 > > > > Thus doesn't look like a CDC device. Can you post the full output of > > "lsusb -v"? > ( As mentioned, I interfered the cdc-nature from the windows driver) > Bus 001 Device 007: ID 0cad:9011 Motorola CGISS > Device Descriptor: > bLength 18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass 0 (Defined at Interface level) > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize0 64 > idVendor 0x0cad Motorola CGISS > idProduct 0x9011 > bcdDevice 24.16 > iManufacturer 1 Motorola Solutions Inc. > iProduct 2 Motorola Solutions TETRA PEI interface > iSerial 0 > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 55 > bNumInterfaces 2 > bConfigurationValue 1 > iConfiguration 3 Generic Serial config > bmAttributes 0x80 > (Bus Powered) > MaxPower 500mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber 0 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass 0 > bInterfaceProtocol 0 > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x81 EP 1 IN > bmAttributes 2 > Transfer Type Bulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x01 EP 1 OUT > bmAttributes 2 > Transfer Type Bulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber 1 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass 255 Vendor Specific Class > bInterfaceSubClass 0 > bInterfaceProtocol 0 > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x82 EP 2 IN > bmAttributes 2 > Transfer Type Bulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x02 EP 2 OUT > bmAttributes 2 > Transfer Type Bulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Device Status: 0x > (Bus Powered) Yeah, that's not a CDC device so usb-serial is the right subsystem for this one. > > Yeah, this is expected since you will not be able to do modem control > > when using the generic driver. > > > Strange enough - after said "sudo modprobe usbserial vendor=0x0cad > product=0x9011" > I can connect to the ttyUSB0 and ttyUSB1 with minicom instead of > miniterm.py (which indeed sems broken) - and it works! I get the AT > command set I expect. That's good to hear. Are both ports usable? > So I guess for the time being leave it here - unsupported but somehow > working with the usbserial generic driver? We should still add your device's id to some driver, so you don't have to manually add it every time you want to use it. What kind of device is this? Johan -- 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] usb: xhci: Clean up error code in xhci_dbc_tty_register_device()
tty_port_register_device() returns error pointers on error, never NULL.
The IS_ERR_OR_NULL() function returns either 1 or 0 so it means we
return 1 on error instead of a proper error code. The caller only
checks for zero vs non-zero so this doesn't affect runtime.
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
index 8d47b6fbf973..6f534b6decef 100644
--- a/drivers/usb/host/xhci-dbgtty.c
+++ b/drivers/usb/host/xhci-dbgtty.c
@@ -443,9 +443,10 @@ int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
xhci_dbc_tty_init_port(xhci, port);
tty_dev = tty_port_register_device(&port->port,
dbc_tty_driver, 0, NULL);
- ret = IS_ERR_OR_NULL(tty_dev);
- if (ret)
+ if (IS_ERR(tty_dev)) {
+ ret = PTR_ERR(tty_dev);
goto register_fail;
+ }
ret = kfifo_alloc(&port->write_fifo, DBC_WRITE_BUF_SIZE, GFP_KERNEL);
if (ret)
--
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: [usb_add_gadget_udc_release] BUG: KASAN: double-free or invalid-free in (null)
Hi, Alan Stern writes: > On Sun, 17 Dec 2017, Fengguang Wu wrote: > >> Hello, >> >> FYI this happens in mainline kernel 4.15.0-rc3. >> It looks like a new regression. >> >> It occurs in 23 out of 36 boots. >> >> [ 38.592360] LUN: removable file: (no medium) >> [ 38.593442] no file given for LUN0 >> [ 38.594589] g_mass_storage usbip-vudc.0: failed to start g_mass_storage: >> -22 >> [ 38.600881] udc usbip-vudc.0: releasing 'usbip-vudc.0' >> [ 38.604397] >> == >> [ 38.605034] BUG: KASAN: double-free or invalid-free in (null) >> [ 38.605034] >> [ 38.605034] CPU: 0 PID: 1 Comm: swapper Not tainted 4.15.0-rc3 #468 >> [ 38.605034] Call Trace: >> [ 38.605034] dump_stack+0x2f/0x3e: >> __dump_stack at >> lib/dump_stack.c:17 >> (inlined by) dump_stack at >> lib/dump_stack.c:63 >> [ 38.605034] print_address_description+0xc2/0x3b7: >> print_address_description at >> mm/kasan/report.c:253 >> [ 38.605034] kasan_report_double_free+0x50/0x8c: >> kasan_report_double_free at >> mm/kasan/report.c:334 >> [ 38.605034] kasan_slab_free+0x60/0x1ef: >> kasan_slab_free at >> mm/kasan/kasan.c:514 >> [ 38.605034] ? ftrace_likely_update+0x5c/0xc4: >> ftrace_likely_update at >> kernel/trace/trace_branch.c:223 >> [ 38.605034] ? kobj_kset_leave+0x193/0x1dc: >> kobj_kset_leave at >> lib/kobject.c:184 >> [ 38.605034] ? lock_acquired+0x8d2/0x8d2: >> lock_release at >> kernel/locking/lockdep.c:4013 >> [ 38.605034] ? ftrace_likely_update+0x5c/0xc4: >> ftrace_likely_update at >> kernel/trace/trace_branch.c:223 >> [ 38.605034] ? trace_preempt_on+0x489/0x4d7: >> trace_preempt_enable_rcuidle at >> include/trace/events/preemptirq.h:50 >> (inlined by) trace_preempt_on >> at kernel/trace/trace_irqsoff.c:855 >> [ 38.605034] ? static_obj+0x40/0x40: >> match_held_lock at >> kernel/locking/lockdep.c:3567 >> [ 38.605034] ? kobject_put+0xf5/0x642: >> refcount_dec_and_test at >> arch/x86/include/asm/refcount.h:75 >> (inlined by) kref_put at >> include/linux/kref.h:69 >> (inlined by) kobject_put at >> lib/kobject.c:694 >> [ 38.605034] ? trace_hardirqs_off+0x17/0x1f: >> trace_hardirqs_off at >> kernel/locking/lockdep.c:2984 >> [ 38.605034] ? kfree+0x419/0x5e7: >> slab_free_hook at mm/slub.c:1380 >> (inlined by) >> slab_free_freelist_hook at mm/slub.c:1412 >> (inlined by) slab_free at >> mm/slub.c:2968 >> (inlined by) kfree at >> mm/slub.c:3899 >> [ 38.605034] kfree+0x43c/0x5e7: >> slab_free at mm/slub.c:2973 >> (inlined by) kfree at >> mm/slub.c:3899 >> [ 38.605034] usb_add_gadget_udc_release+0x693/0x6ca: >> usb_add_gadget_udc_release at >> drivers/usb/gadget/udc/core.c:1199 > > Boy, the error handling in that routine is a mess. The patch below > should straighten it out. looks good: Acked-by: Felipe Balbi > Alan Stern > > > > Index: usb-4.x/drivers/usb/gadget/udc/core.c > === > --- usb-4.x.orig/drivers/usb/gadget/udc/core.c > +++ usb-4.x/drivers/usb/gadget/udc/core.c > @@ -1147,11 +1147,7 @@ int usb_add_gadget_udc_release(struct de > > udc = kzalloc(sizeof(*udc), GFP_KERNEL); > if (!udc) > - goto err1; > - > - ret = device_add(&gadget->dev); > - if (ret) > - goto err2; > + goto err_put_gadget; > > device_initialize(&udc->dev); > udc->dev.release = usb_udc_release; > @@ -1160,7 +1156,11 @@ int usb_add_gadget_udc_release(struct de > udc->dev.parent = parent; > ret = dev_set_name(&udc->dev, "%s", kobject_name(&parent->kobj)); > if (ret) > - goto err3; > + goto err_put_udc; > + > + ret = device_add(&gadget->dev); > + if (ret) > + goto err_put_udc; > > udc->gadget = gadget; > gadget->udc = udc; > @@ -1170,7 +1170,7 @@ int usb_add_gadget_udc_release(struct de >
Re: [GIT PULL] USB: changes for v4.16 merge window
On Tue, Jan 09, 2018 at 10:47:02AM +0200, Felipe Balbi wrote: > > Hi, > > Alan Stern writes: > > On Mon, 8 Jan 2018, Felipe Balbi wrote: > > > >> > >> Hi Greg, > >> > >> Here are my changes for this merge window. Let me know if you want > >> anything to be changed. > >> > >> Patches have been on linux-next for quite a while ;-) > >> > >> cheers > >> > >> The following changes since commit > >> 50c4c4e268a2d7a3e58ebb698ac74da0de40ae36: > >> > >> Linux 4.15-rc3 (2017-12-10 17:56:26 -0800) > >> > >> are available in the git repository at: > >> > >> git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git usb-for-v4.16 > >> > >> for you to fetch changes up to 8ada211d0383b72878582bd312b984a9eae62b30: > >> > >> usb: renesas_usbhs: add extcon notifier to set mode for non-otg channel > >> (2017-12-14 09:57:38 +0200) > >> > >> > >> usb: changes for v4.16 merge window > >> > >> Not many changes here, the most important being an improvement for TI's > >> AM57xx and DRA7xx devices which allows them to disable a metastability > >> workaround in situations where we know what's going on. > >> > >> Other than that, we have a set of changes on Renesas UDC to make the > >> code a little easier to read and maintain while also better supporting > >> extcon framework. > >> > >> The u_serial adaptation layer learned to use kfifo instead of cooking > >> its own FIFO implementation. > >> > >> DWC3 learned to decode a few more USB requests on the trace output. > >> > >> > > > > Felipe, have you looked at my patch submitted on Jan. 3 (USB: UDC core: > > fix double-free in usb_add_gadget_udc_release)? > > > > https://marc.info/?l=linux-usb&m=151500192800372&w=2 > > > > Was it just too late to fit into your 4.16 time frame? > > I just missed it because of my 3 weeks out of office. I can pick it up > during the -rc unless Greg wants to take it by hand if it's super > important. I can take it now, can I get an ack? 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 v3 0/3] usb: renesas_usbhs: Add RZ/A1 support
On Mon, Jan 08, 2018 at 01:15:08PM +0100, Geert Uytterhoeven wrote:
> Hi Chris,
>
> On Mon, Jan 8, 2018 at 12:59 PM, Chris Brandt
> wrote:
> > On Monday, January 08, 2018, Geert Uytterhoeven wrote:
> >> Thanks for the update, but I think there has been a misunderstanding.
> >> I didn't mean to drop "renesas,usbhs-r7s72100" everywhere, only from
> >> the matching in the driver.
> >
> > Opps, I was all kinds of confused then.
> >
> > So, before I submit a V4, here is my understanding:
> >
> > drivers/.../common.c
> > * contains -only- '.compatible = "renesas,rza1-usbhs"'
>
> OK.
>
> > Documentation/.../renesas_usbhs.txt
> > * contains both "renesas,usbhs-r7s72100" and "renesas,rza1-usbhs"
>
> OK.
>
> > r7s72100.dtsi
> > * usbhs0: usb@e801 {
> > compatible = "renesas,usbhs-r7s72100", "renesas,rza1-usbhs";
>
> OK.
>
> > Is this correct?
>
> Yes.
Ack.
--
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 v4 2/3] dt-bindings: usb: renesas_usbhs: Add support for RZ/A1
On Mon, Jan 08, 2018 at 07:30:54AM -0500, Chris Brandt wrote: > Document support for RZ/A1 SoCs > > Signed-off-by: Chris Brandt > Reviewed-by: Geert Uytterhoeven Reviewed-by: Simon Horman > --- > v4: > * Re-added "renesas,usbhs-r7s72100" > v3: > * Removed "renesas,usbhs-r7s72100" > v2: > * Added Reviewed-by > --- > Documentation/devicetree/bindings/usb/renesas_usbhs.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt > b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt > index 47394ab788e3..d060172f1529 100644 > --- a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt > +++ b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt > @@ -13,8 +13,10 @@ Required properties: > - "renesas,usbhs-r8a7795" for r8a7795 (R-Car H3) compatible device > - "renesas,usbhs-r8a7796" for r8a7796 (R-Car M3-W) compatible device > - "renesas,usbhs-r8a77995" for r8a77995 (R-Car D3) compatible device > + - "renesas,usbhs-r7s72100" for r7s72100 (RZ/A1) compatible device > - "renesas,rcar-gen2-usbhs" for R-Car Gen2 or RZ/G1 compatible devices > - "renesas,rcar-gen3-usbhs" for R-Car Gen3 compatible device > + - "renesas,rza1-usbhs" for RZ/A1 compatible device > > When compatible with the generic version, nodes must list the > SoC-specific version corresponding to the platform first followed > -- > 2.15.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: [GIT PULL] USB: changes for v4.16 merge window
Hi, Alan Stern writes: > On Mon, 8 Jan 2018, Felipe Balbi wrote: > >> >> Hi Greg, >> >> Here are my changes for this merge window. Let me know if you want >> anything to be changed. >> >> Patches have been on linux-next for quite a while ;-) >> >> cheers >> >> The following changes since commit 50c4c4e268a2d7a3e58ebb698ac74da0de40ae36: >> >> Linux 4.15-rc3 (2017-12-10 17:56:26 -0800) >> >> are available in the git repository at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git usb-for-v4.16 >> >> for you to fetch changes up to 8ada211d0383b72878582bd312b984a9eae62b30: >> >> usb: renesas_usbhs: add extcon notifier to set mode for non-otg channel >> (2017-12-14 09:57:38 +0200) >> >> >> usb: changes for v4.16 merge window >> >> Not many changes here, the most important being an improvement for TI's >> AM57xx and DRA7xx devices which allows them to disable a metastability >> workaround in situations where we know what's going on. >> >> Other than that, we have a set of changes on Renesas UDC to make the >> code a little easier to read and maintain while also better supporting >> extcon framework. >> >> The u_serial adaptation layer learned to use kfifo instead of cooking >> its own FIFO implementation. >> >> DWC3 learned to decode a few more USB requests on the trace output. >> >> > > Felipe, have you looked at my patch submitted on Jan. 3 (USB: UDC core: > fix double-free in usb_add_gadget_udc_release)? > > https://marc.info/?l=linux-usb&m=151500192800372&w=2 > > Was it just too late to fit into your 4.16 time frame? I just missed it because of my 3 weeks out of office. I can pick it up during the -rc unless Greg wants to take it by hand if it's super important. -- balbi signature.asc Description: PGP signature
Re: [PATCH 14/15] usb: dwc3: Add disabling of start_transfer failure quirk
Hi, Rob Herring writes: > On Fri, Jan 05, 2018 at 12:16:02PM -0800, Thinh Nguyen wrote: >> In DWC_usb31 version 1.70a-ea06 and prior needs a SW workaround for isoc >> START TRANSFER command failure. However, some affected versions may have >> RTL patches to fix this without a SW workaround. Add this quirk to >> disable the SW workaround when it is not needed. > > I really think this one should be implied by compatible strings. won't work for dwc3. Core driver has a single compatible string, also this wouldn't work for PCI-based implementations. -- balbi signature.asc Description: PGP signature
Re: [PATCH 10/15] usb: dwc3: gadget: Set maxpacket size for ep0 IN
Hi, Thinh Nguyen writes: > Hi, > > On 1/8/2018 4:06 AM, Felipe Balbi wrote: >> >> Hi, >> >> Thinh Nguyen writes: >>> There are 2 control endpoint structures for DWC3. However, the driver >>> only updates the OUT direction control endpoint structure during >>> ConnectDone event. DWC3 driver needs to update the endpoint max packet >>> size for control IN endpoint as well. If the max packet size is not >>> properly set, then the driver will incorrectly calculate the data >>> transfer size and fail to send ZLP for HS/FS 3-stage control read >>> transfer. >>> >>> The fix is simply to update the max packet size for the ep0 IN direction >>> during ConnectDone event. >>> >>> Signed-off-by: Thinh Nguyen >>> --- >>> drivers/usb/dwc3/gadget.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index bdf2a2014ebd..6ae924d95cbc 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2751,6 +2751,8 @@ static void dwc3_gadget_conndone_interrupt(struct >>> dwc3 *dwc) >>> break; >>> } >>> >>> + dwc->eps[1]->endpoint.maxpacket = dwc->gadget.ep0->maxpacket; >> >> thanks :-) I've had that on my list for a while but never got to it >> since it has no real effects other than reporting properly on >> tracepoints :-) >> > > Just to clarify, this is a bug. We found this issue when we test for > HS/FS 3-stage control read transfer for ZLP. interesting... I have never seen such bug before. Got some tracepoints of the problem? Also, if it's a bug, why wasn't it sent as a separate patch with a Cc stable and a Fixes tag? -- balbi signature.asc Description: PGP signature
