Re: crash by cdc_acm driver in kernels 4.8-rc1/5

[Bjørn Mork]

On November 23, 2016 1:54:57 AM CET, Wim Osterholt  wrote:
>On Tue, Nov 22, 2016 at 07:08:30PM +0100, Bjørn Mork wrote:
>> > On kernel 4.8.8  this crashes hard and produces over a serial link:
>> 
>> Huh?  That device shouldn't ever enter that code path AFAICS.
>> Unless you wouldn't happen to add a dynamic entry for this
>device,
>
>No idea of what you mean here.
>
>> would you?  What's the output of
>> 
>>  cat /sys/bus/usb/drivers/cdc_acm/new_id
>
>Just empty.

Shit. Back to not understanding how you could possibly enter the debugging code 
at all.

Bjørn

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] fsl/usb: Add FSL USB Gadget entry in platform device id table

[Changming Huang]

Add FSL USB Gadget entry in platform device id table

Signed-off-by: Changming Huang 
Signed-off-by: Suresh Gupta 
---
 drivers/usb/gadget/udc/fsl_udc_core.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c 
b/drivers/usb/gadget/udc/fsl_udc_core.c
index aab5221..b24b1c9 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -2671,6 +2671,8 @@ static int fsl_udc_otg_resume(struct device *dev)
}, {
.name = "imx-udc-mx51",
}, {
+   .name = "fsl-usb2-udc",
+   }, {
/* sentinel */
}
 };
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] arm/dts: ls1021a: Add dma-coherent property to usb3 node

[Changming Huang]

This sets dma ops as coherent for usb 3.0 platform device

Signed-off-by: Changming Huang 
Signed-off-by: Rajesh Bhagat 
---
 arch/arm/boot/dts/ls1021a.dtsi |1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 368e219..81fb4d9 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -627,6 +627,7 @@
dr_mode = "host";
snps,quirk-frame-length-adjustment = <0x20>;
snps,dis_rxdet_inp3_quirk;
+   dma-coherent;
};
 
pcie@340 {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC][PATCH 3/3] usb: dwc2: Avoid suspending if we're in gadget mode

[John Stultz]

I've found when booting HiKey with the usb gadget cable attached
if I then try to connect via adb, I get an infinite spew of:

dwc2 f72c.usb: dwc2_hsotg_ep_sethalt(ep ffc0790ecb18 ep1out, 0)
dwc2 f72c.usb: dwc2_hsotg_ep_sethalt(ep ffc0790eca18 ep1in, 0)

It seems that the usb autosuspend is suspending the bus shortly
after bootup when the gadget cable is attached. So when adbd
then tries to use the device, it doesn't work and it then tries
to restart it over and over via the ep_sethalt calls (via
FUNCTIONFS_CLEAR_HALT ioctl).

Chen Yu suggested this patch to avoid suspending if we're
in device mode, and it avoids the problem.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Suggested-by: Chen Yu 
Signed-off-by: John Stultz 
---
 drivers/usb/dwc2/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index df5a065..619ccfe 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4365,6 +4365,9 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
if (!HCD_HW_ACCESSIBLE(hcd))
goto unlock;
 
+   if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
+   goto unlock;
+
if (!hsotg->core_params->hibernation)
goto skip_power_saving;
 
-- 
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

[RFC][PATCH 1/3] phy: phy-hi6220-usb: Wire up extconn support to hikey's phy driver

[John Stultz]

This wires extconn support to hikey's phy driver, and
connects it to the usb UDC layer via a usb_phy structure.

Not sure if this is the right way to connect phy -> UDC,
but I'm lacking a clear example.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Signed-off-by: John Stultz 
---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  11 +++
 drivers/phy/Kconfig   |   2 +
 drivers/phy/phy-hi6220-usb.c  | 139 ++
 3 files changed, 152 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 17839db..171fbb2 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -732,10 +732,21 @@
regulator-always-on;
};
 
+   usb_vbus: usb-vbus {
+   compatible = "linux,extcon-usb-gpio";
+   id-gpio = < 6 1>;
+   };
+
+   usb_id: usb-id {
+   compatible = "linux,extcon-usb-gpio";
+   id-gpio = < 5 1>;
+   };
+
usb_phy: usbphy {
compatible = "hisilicon,hi6220-usb-phy";
#phy-cells = <0>;
phy-supply = <_5v_hub>;
+   extcon = <_vbus>, <_id>;
hisilicon,peripheral-syscon = <_ctrl>;
};
 
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index fe00f91..76f4f17 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -254,8 +254,10 @@ config PHY_MT65XX_USB3
 config PHY_HI6220_USB
tristate "hi6220 USB PHY support"
depends on (ARCH_HISI && ARM64) || COMPILE_TEST
+   depends on EXTCON
select GENERIC_PHY
select MFD_SYSCON
+   select USB_PHY
help
  Enable this to support the HISILICON HI6220 USB PHY.
 
diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
index b2141cb..89d8475 100644
--- a/drivers/phy/phy-hi6220-usb.c
+++ b/drivers/phy/phy-hi6220-usb.c
@@ -12,7 +12,12 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
 #include 
+#include 
 
 #define SC_PERIPH_CTRL40x00c
 
@@ -44,9 +49,21 @@
 
 #define EYE_PATTERN_PARA   0x7053348c
 
+
+struct hi6220_usb_cable {
+   struct notifier_block   nb;
+   struct extcon_dev   *extcon;
+   int state;
+};
+
 struct hi6220_priv {
struct regmap *reg;
struct device *dev;
+   struct usb_phy phy;
+
+   struct delayed_work work;
+   struct hi6220_usb_cable vbus;
+   struct hi6220_usb_cable id;
 };
 
 static void hi6220_phy_init(struct hi6220_priv *priv)
@@ -112,23 +129,85 @@ static int hi6220_phy_exit(struct phy *phy)
return hi6220_phy_setup(priv, false);
 }
 
+
 static struct phy_ops hi6220_phy_ops = {
.init   = hi6220_phy_start,
.exit   = hi6220_phy_exit,
.owner  = THIS_MODULE,
 };
 
+static void hi6220_detect_work(struct work_struct *work)
+{
+   struct hi6220_priv *priv =
+   container_of(to_delayed_work(work), struct hi6220_priv, work);
+   struct usb_otg *otg = priv->phy.otg;
+
+   if (!IS_ERR(priv->vbus.extcon))
+   priv->vbus.state = extcon_get_cable_state_(priv->vbus.extcon,
+EXTCON_USB);
+   if (!IS_ERR(priv->id.extcon))
+   priv->id.state = extcon_get_cable_state_(priv->id.extcon,
+EXTCON_USB_HOST);
+   if (otg->gadget) {
+   if (priv->id.state)
+   usb_gadget_vbus_connect(otg->gadget);
+   else
+   usb_gadget_vbus_disconnect(otg->gadget);
+   }
+}
+
+static int hi6220_otg_vbus_notifier(struct notifier_block *nb,
+   unsigned long event, void *ptr)
+{
+   struct hi6220_usb_cable *vbus = container_of(nb,
+   struct hi6220_usb_cable, nb);
+   struct hi6220_priv *priv = container_of(vbus,
+   struct hi6220_priv, vbus);
+
+   schedule_delayed_work(>work, msecs_to_jiffies(100));
+   return NOTIFY_DONE;
+}
+
+static int hi6220_otg_id_notifier(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+   struct hi6220_usb_cable *id = 

[RFC][PATCH 0/3] Try to connect hikey's usb phy to dwc2 driver

[John Stultz]

After earlier attempts[1] at submitting somewhat hackish fixes
to the dwc2 driver, I realized the core issue seemed to be the
overly simplistic phy driver.

I've connected the phy-hi6220-usb.c driver to extcon so it now
gets connection and disconnection signals on the usb gadget
cable. And I modified the driver so it registers a usb-phy and
calls usb_gadget_vbus_connect/disconnect() appropriately.

Unfortunately this doesn't quite work with the dwc2 driver,
so I've hacked that driver to allow it to function.

With these changes, while likely not correct, things function
well, and I was able to drop two of the hackish fixups from the
earlier set. I still needed one patch to keep the usb bus from
suspending while in gadget mode, so I've included that in this
series.

[1]: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1272880.html

Feedback and guidance would be greatly appreciated!

thanks
-john


Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org

John Stultz (3):
  phy: phy-hi6220-usb: Wire up extconn support to hikey's phy driver
  HACK: dwc2: force dual use of uphy and phy
  usb: dwc2: Avoid suspending if we're in gadget mode

 arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  11 +++
 drivers/phy/Kconfig   |   2 +
 drivers/phy/phy-hi6220-usb.c  | 139 ++
 drivers/usb/dwc2/hcd.c|   3 +
 drivers/usb/dwc2/platform.c   |   4 +-
 5 files changed, 157 insertions(+), 2 deletions(-)

-- 
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: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[Hayes Wang]

Mark Lord [mailto:ml...@pobox.com]
> Sent: Friday, November 18, 2016 8:03 PM
[..]
> How does the RTL8152 know that the limit is 16KB,
> rather than some other number?  Is this a hardwired number
> in the hardware, or is it a parameter that the software
> sends to the chip during initialization?

It is the limitation of the hardware.

> I have a USB analyzer, but it is difficult to figure out how
> to program an appropriate trigger point for the capture,
> since the problem (with 16KB URBs) takes minutes to hours
> or even days to trigger.

It is good. Our hw engineers real want it. Maybe you could send
a specific packet, and trigger it. You could allocate a skb and
fill the data which you prefer, and call

skb_queue_tail(>tx_queue, skb);

[...]
> The first issue is that a packet sometimes begins in one URB,
> and completes in the next URB, without an rx_desc at the start
> of the second URB.  This I have already reported earlier.

However, our hw engineer says it wouldn't happen. Our hw always
sends rx_desc + packet + padding. The hw wouldn't split it to
two or more transmission. That is why I wonder who does it.

> But the driver, as written, sometimes accesses bytes outside
> of the 16KB URB buffer, because it trusts the non-existent
> rx_desc in these cases, and also because it accesses bytes
> from the rx_desc without first checking whether there is
> sufficient remaining space in the URB to hold an rx_desc.

I think I check them. According to the followning code,

list_for_each_safe(cursor, next, _queue) {
struct rx_desc *rx_desc;
struct rx_agg *agg;
int len_used = 0;
struct urb *urb;
u8 *rx_data;

...

rx_desc = agg->head;
rx_data = agg->head;
len_used += sizeof(struct rx_desc); //<-- add the size of next 
rx_desc

while (urb->actual_length > len_used) {
struct net_device *netdev = tp->netdev;
struct net_device_stats *stats = >stats;
unsigned int pkt_len;
struct sk_buff *skb;

pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
if (pkt_len < ETH_ZLEN)
break;

len_used += pkt_len;
if (urb->actual_length < len_used)
break;

pkt_len -= CRC_SIZE;
rx_data += sizeof(struct rx_desc);

...

find_next_rx:
rx_data = rx_agg_align(rx_data + pkt_len + CRC_SIZE);
rx_desc = (struct rx_desc *)rx_data;
len_used = (int)(rx_data - (u8 *)agg->head);
len_used += sizeof(struct rx_desc); //<-- add the size 
of next rx_desc
}

submit:
...
}

The while loop would check if the next rx_desc is inside the urb
buffer, because the len_used includes the size of the next rx_desc.
Then, in the while loop, the len_used adds the packet size and check
with urb->actual_length again. These make sure the rx_desc and the
packet are inside the urb buffer. Except the urb->actual_length
is more than agg_buf_sz. However, I don't think it would happen.

Best Regards,
Hayes

[RFC][PATCH 2/3] HACK: dwc2: force dual use of uphy and phy

[John Stultz]

I can't seem to figure out how to connect a generic phy device
to the usb UDC logic, as the dwc2 code seems to exclusively work
with either generic phys or usb-phys.

So to try to make this work with the phy-hi6220-usb driver, tweak
the dwc2 logic to support call hooks to both phy and uphy devices.

I realize this is likely wrong, but without a good pointer as to
how this should be done, I'm a bit wandering around in the dark.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Kishon Vijay Abraham I 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Signed-off-by: John Stultz 
---
 drivers/usb/dwc2/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 8e1728b..8fb2f4d 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -297,7 +297,7 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg 
*hsotg)
 
if (hsotg->uphy)
ret = usb_phy_init(hsotg->uphy);
-   else if (hsotg->plat && hsotg->plat->phy_init)
+   if (hsotg->plat && hsotg->plat->phy_init)
ret = hsotg->plat->phy_init(pdev, hsotg->plat->phy_type);
else {
ret = phy_power_on(hsotg->phy);
@@ -411,7 +411,7 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
}
}
 
-   if (!hsotg->phy) {
+   if (1 || !hsotg->phy) {
hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);
if (IS_ERR(hsotg->uphy)) {
ret = PTR_ERR(hsotg->uphy);
-- 
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: crash by cdc_acm driver in kernels 4.8-rc1/5

[Wim Osterholt]

On Tue, Nov 22, 2016 at 07:08:30PM +0100, Bjørn Mork wrote:
> > On kernel 4.8.8  this crashes hard and produces over a serial link:
> 
> Huh?  That device shouldn't ever enter that code path AFAICS.
> Unless you wouldn't happen to add a dynamic entry for this device,

No idea of what you mean here.

> would you?  What's the output of
> 
>  cat /sys/bus/usb/drivers/cdc_acm/new_id

Just empty.

Wim.
--
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: crash by cdc_acm driver in kernels 4.8-rc1/5

[Wim Osterholt]

On Tue, Nov 22, 2016 at 06:50:28PM +0100, Bjørn Mork wrote:
> > iCountryCodeRelDate4 04052004
> > wCountryCode  0x4803
> 
> No excuse for crashing of course, but that's one of the sickets
> descriptor sets I've seen today. Who got the bright idea to put the
> communication class functional descriptors on the data class interfaces?

Whell, the chinese of coarse. It's all chinese to me. But maybe they made 
this time an exact copy of their example from Conexant. Not that they are
that careful usually.

>...
> Don't understand how it could crash.

The oops does normally not immediately lead to a crash. Only with debugging
on it will halt immediately and the log will tell you that a reboot will
be necessairy. 

Wim.
--
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/4] usb: dwc2: Add binding for AHB burst

[Rob Herring]

On Tue, Nov 22, 2016 at 2:51 PM, Christian Lamparter
 wrote:
> On Monday, November 21, 2016 7:32:30 PM CET John Youn wrote:
>> On 11/21/2016 1:10 PM, Christian Lamparter wrote:
>> > On Monday, November 21, 2016 12:16:31 PM CET John Youn wrote:
>> >> On 11/18/2016 12:18 PM, Christian Lamparter wrote:
>> >>> On Friday, November 18, 2016 8:16:08 AM CET Rob Herring wrote:
>>  Also, perhaps you should allow that the compatible string can define the
>>  default.
>> 
>> >>> I hoped you would say that :).
>> >>>
>> >>> I've attached a patch (on top of John Youn changes) [...]
>> >>> ---
>> >>> Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for 
>> >>> amcc,dwc-otg
>> >>> [...]
>> >>> @@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
>> >>> +/* [...] */
>> >>> +static const struct of_device_id dwc2_compat_ahb_bursts[] = {
>> >>> + {
>> >>> + .compatible = "amcc,dwc-otg",
>> >>> + .data = (void *) GAHBCFG_HBSTLEN_INCR16,
>> >>> + },
>> >>> +};
>> [...]
>> > >>> @@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct 
>> > >>> dwc2_hsotg *hsotg)
>> >>>   ret = device_property_read_string(hsotg->dev, "snps,ahb-burst", );
>> >>>   if (ret < 0) {
>> >>> + const struct of_device_id *match;
>> >>> +
>> >>> + match = of_match_node(dwc2_compat_ahb_bursts, node);
>> >>> + if (match)
>> >>> + ret = (int)match->data;
>> >>> +
>> [...]
>> >> I'd prefer if you use the binding which requires no extra code in
>> >> dwc2.
>> > I'm fine with either option. However it think that this would require
>> > that either Mark or Rob would allow an exception to the "keep existing
>> > dts the way they are) and ack the following change to the canyonlands.dts.
>>
>> I don't know about that. Under what circumstance can the dts change?
> As far as I know, the justification for not changing the DTS is that a
> compiled DTB might be stored in an read-only ROM on a board. So it would
> be impossible to update it. Hence, the driver have work with the existing
> (and sometimes buggy or incomplete) information to stay compatible.
>
> (Note: Thankfully, the canyonlands dtb is stored in flash, it's possible
> to update it. But it is an extra step that's not done automatically
> with make install).
>
>> The canyonlands dts was binding to an external vendor driver. So it
>> wasn't documented nor expected to work with dwc2 until your recent
>> patch adding the compatible string.
>
> Oh, no that's not what happend. Let me explain why there was no "external
> vendor driver": AMCC/APM were planing to upstream their hole platform. And
> in fact, the devs tried very hard to include their driver back in 2011 [0].
> But this driver was denied inclusion back then due to:
>
> "[...]
> I would also like to point out that the same Synopsys USB controller
> is used in a number of other SoCs (especially ARM chips), and
> supported by other drivers, some of these even in mainline.
>
> See http://thread.gmane.org/gmane.linux.usb.general/61714/focus=62139
> for a related thread.
>
> Instead of trying to add a completely new driver to mainline (and one
> which has been repeatedly been rejected), I vote for focusing on the
> existing driver code that is already in mainline, and testing and
> improving this so we can use a single implementation of this driver
> code for all SoCs that use the same IP block." [1]
>
> Of course: The listed link goes the "USB Host driver for i.MX28" driver.
> And this is an ehci-hcd like driver... Which is as you are well aware not
> that similar to the dwc2 OTG. And as far as I can tell: AMCC abandoned
> the patch series right there.
>
> Note: AMCC did however succeed in pushing your employer's Synopsys
> DesignWare SATA and DMA drivers to the kernel back then. And I'm happy
> to report that both drivers are still around and working fine for the 460EX
> (sata_dwc_460ex.c[2] and the DW AHB DMA [3]). (The drivers also work for
> different platforms than the original PPC. I know that because I helped
> Andy Shevchenko with testing and pushing some fixes to it when he was
> adding support for the Intel Quark SoC, which uses the DWC SATA and DMA).
>
> So Please?
>> Systems that use the vendor driver will still work with the dts. If
>> you remove the vendor driver and configure it to use dwc2, it won't
>> work due to a quirk of the canyonlands hardware, for which you need to
>> add a dts property.
> Sadly, there is no up to date vendor driver. The canyonlands.dts binding
> is still in place and the hardware works fine. I'm interested in this
> platform since it is a cheap BigEndian system which is useful for usb
> driver development (carl9170 and rtl8192su)... and I would like to
> have out-of-the-box support.
>
>> I think this is reasonable. Rob or Mark, any feedback?
> I recall that Rob has already voiced his opinion about the ahb-burst setting:
> "Also, perhaps you should allow that the compatible string can define 

Re: [PATCH v2 2/4] usb: dwc2: Add binding for AHB burst

[Christian Lamparter]

On Monday, November 21, 2016 7:32:30 PM CET John Youn wrote:
> On 11/21/2016 1:10 PM, Christian Lamparter wrote:
> > On Monday, November 21, 2016 12:16:31 PM CET John Youn wrote:
> >> On 11/18/2016 12:18 PM, Christian Lamparter wrote:
> >>> On Friday, November 18, 2016 8:16:08 AM CET Rob Herring wrote:
>  Also, perhaps you should allow that the compatible string can define the 
>  default.
> 
> >>> I hoped you would say that :).
> >>>
> >>> I've attached a patch (on top of John Youn changes) [...]
> >>> ---
> >>> Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for 
> >>> amcc,dwc-otg
> >>> [...]
> >>> @@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
> >>> +/* [...] */
> >>> +static const struct of_device_id dwc2_compat_ahb_bursts[] = {
> >>> + {
> >>> + .compatible = "amcc,dwc-otg",
> >>> + .data = (void *) GAHBCFG_HBSTLEN_INCR16,
> >>> + },
> >>> +};
> [...]
> > >>> @@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct 
> > >>> dwc2_hsotg *hsotg)
> >>>   ret = device_property_read_string(hsotg->dev, "snps,ahb-burst", );
> >>>   if (ret < 0) {
> >>> + const struct of_device_id *match;
> >>> +
> >>> + match = of_match_node(dwc2_compat_ahb_bursts, node);
> >>> + if (match)
> >>> + ret = (int)match->data;
> >>> +
> [...]
> >> I'd prefer if you use the binding which requires no extra code in
> >> dwc2.
> > I'm fine with either option. However it think that this would require
> > that either Mark or Rob would allow an exception to the "keep existing
> > dts the way they are) and ack the following change to the canyonlands.dts. 
> 
> I don't know about that. Under what circumstance can the dts change?
As far as I know, the justification for not changing the DTS is that a
compiled DTB might be stored in an read-only ROM on a board. So it would
be impossible to update it. Hence, the driver have work with the existing
(and sometimes buggy or incomplete) information to stay compatible.

(Note: Thankfully, the canyonlands dtb is stored in flash, it's possible
to update it. But it is an extra step that's not done automatically
with make install). 

> The canyonlands dts was binding to an external vendor driver. So it
> wasn't documented nor expected to work with dwc2 until your recent
> patch adding the compatible string.

Oh, no that's not what happend. Let me explain why there was no "external 
vendor driver": AMCC/APM were planing to upstream their hole platform. And
in fact, the devs tried very hard to include their driver back in 2011 [0].
But this driver was denied inclusion back then due to:

"[...]
I would also like to point out that the same Synopsys USB controller
is used in a number of other SoCs (especially ARM chips), and
supported by other drivers, some of these even in mainline.

See http://thread.gmane.org/gmane.linux.usb.general/61714/focus=62139
for a related thread.

Instead of trying to add a completely new driver to mainline (and one
which has been repeatedly been rejected), I vote for focusing on the
existing driver code that is already in mainline, and testing and
improving this so we can use a single implementation of this driver
code for all SoCs that use the same IP block." [1]

Of course: The listed link goes the "USB Host driver for i.MX28" driver.
And this is an ehci-hcd like driver... Which is as you are well aware not
that similar to the dwc2 OTG. And as far as I can tell: AMCC abandoned
the patch series right there. 

Note: AMCC did however succeed in pushing your employer's Synopsys
DesignWare SATA and DMA drivers to the kernel back then. And I'm happy
to report that both drivers are still around and working fine for the 460EX 
(sata_dwc_460ex.c[2] and the DW AHB DMA [3]). (The drivers also work for
different platforms than the original PPC. I know that because I helped
Andy Shevchenko with testing and pushing some fixes to it when he was
adding support for the Intel Quark SoC, which uses the DWC SATA and DMA).

So Please?
> Systems that use the vendor driver will still work with the dts. If
> you remove the vendor driver and configure it to use dwc2, it won't
> work due to a quirk of the canyonlands hardware, for which you need to
> add a dts property.
Sadly, there is no up to date vendor driver. The canyonlands.dts binding
is still in place and the hardware works fine. I'm interested in this
platform since it is a cheap BigEndian system which is useful for usb
driver development (carl9170 and rtl8192su)... and I would like to
have out-of-the-box support.

> I think this is reasonable. Rob or Mark, any feedback?
I recall that Rob has already voiced his opinion about the ahb-burst setting: 
"Also, perhaps you should allow that the compatible string can define the 
default."

And based on that, I made the "add a default ahb-burst setting for amcc,dwc-otg"
patch above. Of course, it would be nice to have any feedback too. But unless I
hear otherwise, I'll continue with posting 

Re: [PATCH v6 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

[David Lechner]

On 11/22/2016 02:46 PM, Axel Haslam wrote:

On Tue, Nov 22, 2016 at 9:37 PM, David Lechner  wrote:

On 11/21/2016 10:30 AM, Axel Haslam wrote:


Using a regulator to handle VBUS will eliminate the need for
platform data and callbacks, and make the driver more generic
allowing different types of regulators to handle VBUS.

The regulator equivalents to the platform callbacks are:
set_power   ->  regulator_enable/regulator_disable
get_power   ->  regulator_is_enabled
get_oci ->  regulator_get_error_flags
ocic_notify ->  regulator event notification

Signed-off-by: Axel Haslam 
---
 drivers/usb/host/ohci-da8xx.c | 97
+--
 1 file changed, 94 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 90763ad..d0eb754 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd
*hcd, u16 typeReq,
 static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);

 struct da8xx_ohci_hcd {
+   struct usb_hcd *hcd;
struct clk *usb11_clk;
struct phy *usb11_phy;
+   struct regulator *vbus_reg;
+   struct notifier_block nb;
+   unsigned int reg_enabled;
 };

 #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd
*)(hcd_to_ohci(hcd)->priv)
@@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)

 static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
+   int ret;

if (hub && hub->set_power)
return hub->set_power(1, on);

+   if (!da8xx_ohci->vbus_reg)
+   return 0;
+
+   if (on && !da8xx_ohci->reg_enabled) {
+   ret = regulator_enable(da8xx_ohci->vbus_reg);
+   if (ret) {
+   dev_err(dev, "Failed to enable regulator: %d\n",
ret);
+   return ret;
+   }
+   da8xx_ohci->reg_enabled = 1;
+
+   } else if (!on && da8xx_ohci->reg_enabled) {
+   ret = regulator_disable(da8xx_ohci->vbus_reg);
+   if (ret) {
+   dev_err(dev, "Failed  to disable regulator: %d\n",
ret);
+   return ret;
+   }
+   da8xx_ohci->reg_enabled = 0;
+   }
+
return 0;
 }

 static int ohci_da8xx_get_power(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->get_power)
return hub->get_power(1);

+   if (da8xx_ohci->vbus_reg)
+   return regulator_is_enabled(da8xx_ohci->vbus_reg);
+
return 1;
 }

 static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
+   unsigned int flags;
+   int ret;

if (hub && hub->get_oci)
return hub->get_oci(1);

+   if (!da8xx_ohci->vbus_reg)
+   return 0;
+
+   ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, );
+   if (ret)
+   return ret;
+
+   if (flags & REGULATOR_ERROR_OVER_CURRENT)
+   return 1;
+
return 0;
 }

 static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->set_power)
return 1;

+   if (da8xx_ohci->vbus_reg)
+   return 1;
+
return 0;
 }

 static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->get_oci)
return 1;

+   if (da8xx_ohci->vbus_reg)
+   return 1;
+
return 0;
 }

@@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct
da8xx_ohci_root_hub *hub,
hub->set_power(port, 0);
 }

+static int ohci_da8xx_regulator_event(struct notifier_block *nb,
+   unsigned long event, void *data)
+{
+   struct da8xx_ohci_hcd *da8xx_ohci =
+   container_of(nb, struct da8xx_ohci_hcd,
nb);
+   struct device *dev = 

Re: [PATCH v6 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

[Axel Haslam]

On Tue, Nov 22, 2016 at 9:37 PM, David Lechner  wrote:
> On 11/21/2016 10:30 AM, Axel Haslam wrote:
>>
>> Using a regulator to handle VBUS will eliminate the need for
>> platform data and callbacks, and make the driver more generic
>> allowing different types of regulators to handle VBUS.
>>
>> The regulator equivalents to the platform callbacks are:
>> set_power   ->  regulator_enable/regulator_disable
>> get_power   ->  regulator_is_enabled
>> get_oci ->  regulator_get_error_flags
>> ocic_notify ->  regulator event notification
>>
>> Signed-off-by: Axel Haslam 
>> ---
>>  drivers/usb/host/ohci-da8xx.c | 97
>> +--
>>  1 file changed, 94 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>> index 90763ad..d0eb754 100644
>> --- a/drivers/usb/host/ohci-da8xx.c
>> +++ b/drivers/usb/host/ohci-da8xx.c
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd
>> *hcd, u16 typeReq,
>>  static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
>>
>>  struct da8xx_ohci_hcd {
>> +   struct usb_hcd *hcd;
>> struct clk *usb11_clk;
>> struct phy *usb11_phy;
>> +   struct regulator *vbus_reg;
>> +   struct notifier_block nb;
>> +   unsigned int reg_enabled;
>>  };
>>
>>  #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd
>> *)(hcd_to_ohci(hcd)->priv)
>> @@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)
>>
>>  static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> +   int ret;
>>
>> if (hub && hub->set_power)
>> return hub->set_power(1, on);
>>
>> +   if (!da8xx_ohci->vbus_reg)
>> +   return 0;
>> +
>> +   if (on && !da8xx_ohci->reg_enabled) {
>> +   ret = regulator_enable(da8xx_ohci->vbus_reg);
>> +   if (ret) {
>> +   dev_err(dev, "Failed to enable regulator: %d\n",
>> ret);
>> +   return ret;
>> +   }
>> +   da8xx_ohci->reg_enabled = 1;
>> +
>> +   } else if (!on && da8xx_ohci->reg_enabled) {
>> +   ret = regulator_disable(da8xx_ohci->vbus_reg);
>> +   if (ret) {
>> +   dev_err(dev, "Failed  to disable regulator: %d\n",
>> ret);
>> +   return ret;
>> +   }
>> +   da8xx_ohci->reg_enabled = 0;
>> +   }
>> +
>> return 0;
>>  }
>>
>>  static int ohci_da8xx_get_power(struct usb_hcd *hcd)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>
>> if (hub && hub->get_power)
>> return hub->get_power(1);
>>
>> +   if (da8xx_ohci->vbus_reg)
>> +   return regulator_is_enabled(da8xx_ohci->vbus_reg);
>> +
>> return 1;
>>  }
>>
>>  static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> +   unsigned int flags;
>> +   int ret;
>>
>> if (hub && hub->get_oci)
>> return hub->get_oci(1);
>>
>> +   if (!da8xx_ohci->vbus_reg)
>> +   return 0;
>> +
>> +   ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, );
>> +   if (ret)
>> +   return ret;
>> +
>> +   if (flags & REGULATOR_ERROR_OVER_CURRENT)
>> +   return 1;
>> +
>> return 0;
>>  }
>>
>>  static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>
>> if (hub && hub->set_power)
>> return 1;
>>
>> +   if (da8xx_ohci->vbus_reg)
>> +   return 1;
>> +
>> return 0;
>>  }
>>
>>  static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
>>  {
>> +   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
>> struct device *dev  = hcd->self.controller;
>> struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>>
>> if (hub && hub->get_oci)
>> return 1;
>>
>> +   if (da8xx_ohci->vbus_reg)
>> +   return 1;
>> +
>> return 0;
>>  }
>>
>> @@ -160,15 +212,41 @@ static void 

Re: [PATCH v6 1/5] USB: ohci: da8xx: use ohci priv data instead of globals

[David Lechner]

On 11/21/2016 10:30 AM, Axel Haslam wrote:

Instead of global variables, use the extra_priv_size of
the ohci driver.

We cannot yet move the ocic mask because this is used on
the interrupt handler which is registerded through platform


s/registerded/registered/


data and does not have an hcd pointer. This will be moved
on a later patch.

Signed-off-by: Axel Haslam 
---


Looks good to me (other than the spelling error above).

Tested-by: David Lechner 

--
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 v6 3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

[David Lechner]

On 11/21/2016 10:30 AM, Axel Haslam wrote:

Using a regulator to handle VBUS will eliminate the need for
platform data and callbacks, and make the driver more generic
allowing different types of regulators to handle VBUS.

The regulator equivalents to the platform callbacks are:
set_power   ->  regulator_enable/regulator_disable
get_power   ->  regulator_is_enabled
get_oci ->  regulator_get_error_flags
ocic_notify ->  regulator event notification

Signed-off-by: Axel Haslam 
---
 drivers/usb/host/ohci-da8xx.c | 97 +--
 1 file changed, 94 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 90763ad..d0eb754 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -36,8 +37,12 @@ static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, 
u16 typeReq,
 static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);

 struct da8xx_ohci_hcd {
+   struct usb_hcd *hcd;
struct clk *usb11_clk;
struct phy *usb11_phy;
+   struct regulator *vbus_reg;
+   struct notifier_block nb;
+   unsigned int reg_enabled;
 };

 #define to_da8xx_ohci(hcd) (struct da8xx_ohci_hcd *)(hcd_to_ohci(hcd)->priv)
@@ -83,56 +88,103 @@ static void ohci_da8xx_disable(struct usb_hcd *hcd)

 static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
+   int ret;

if (hub && hub->set_power)
return hub->set_power(1, on);

+   if (!da8xx_ohci->vbus_reg)
+   return 0;
+
+   if (on && !da8xx_ohci->reg_enabled) {
+   ret = regulator_enable(da8xx_ohci->vbus_reg);
+   if (ret) {
+   dev_err(dev, "Failed to enable regulator: %d\n", ret);
+   return ret;
+   }
+   da8xx_ohci->reg_enabled = 1;
+
+   } else if (!on && da8xx_ohci->reg_enabled) {
+   ret = regulator_disable(da8xx_ohci->vbus_reg);
+   if (ret) {
+   dev_err(dev, "Failed  to disable regulator: %d\n", ret);
+   return ret;
+   }
+   da8xx_ohci->reg_enabled = 0;
+   }
+
return 0;
 }

 static int ohci_da8xx_get_power(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->get_power)
return hub->get_power(1);

+   if (da8xx_ohci->vbus_reg)
+   return regulator_is_enabled(da8xx_ohci->vbus_reg);
+
return 1;
 }

 static int ohci_da8xx_get_oci(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
+   unsigned int flags;
+   int ret;

if (hub && hub->get_oci)
return hub->get_oci(1);

+   if (!da8xx_ohci->vbus_reg)
+   return 0;
+
+   ret = regulator_get_error_flags(da8xx_ohci->vbus_reg, );
+   if (ret)
+   return ret;
+
+   if (flags & REGULATOR_ERROR_OVER_CURRENT)
+   return 1;
+
return 0;
 }

 static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->set_power)
return 1;

+   if (da8xx_ohci->vbus_reg)
+   return 1;
+
return 0;
 }

 static int ohci_da8xx_has_oci(struct usb_hcd *hcd)
 {
+   struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
struct device *dev  = hcd->self.controller;
struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);

if (hub && hub->get_oci)
return 1;

+   if (da8xx_ohci->vbus_reg)
+   return 1;
+
return 0;
 }

@@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct 
da8xx_ohci_root_hub *hub,
hub->set_power(port, 0);
 }

+static int ohci_da8xx_regulator_event(struct notifier_block *nb,
+   unsigned long event, void *data)
+{
+   struct da8xx_ohci_hcd *da8xx_ohci =
+   container_of(nb, struct da8xx_ohci_hcd, nb);
+   struct device *dev = da8xx_ohci->hcd->self.controller;
+
+   if (event & REGULATOR_EVENT_OVER_CURRENT) {
+   dev_warn(dev, 

Re: crash by cdc_acm driver in kernels 4.8-rc1/5

[Bjørn Mork]

Wim Osterholt  writes:

> On Mon, Nov 21, 2016 at 02:19:32PM +0100, Oliver Neukum wrote:
>
>> I don't understand it, bit please test the attached patch
>> with dynamic debugging for cdc-acm and the kernel log level
>> at maximum.
>
>> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
>> index 6895f9e..f03b5db 100644
>> --- a/drivers/usb/class/cdc-acm.c
>> +++ b/drivers/usb/class/cdc-acm.c
>> @@ -1188,6 +1188,12 @@ static int acm_probe(struct usb_interface *intf,
>>  
>>  cdc_parse_cdc_header(, intf, buffer, buflen);
>>  union_header = h.usb_cdc_union_desc;
>> +
>> +dev_dbg(>dev, "Parsed device header\n");
>> +dev_dbg(>dev, "Union descriptor %p\n", h.usb_cdc_union_desc);
>> +dev_dbg(>dev, "ACM descriptor %p\n", h.usb_cdc_acm_descriptor);
>> +dev_dbg(>dev, "Country descriptor %p\n", 
>> h.usb_cdc_country_functional_desc);
>> +
>>  cmgmd = h.usb_cdc_call_mgmt_descriptor;
>>  if (cmgmd)
>>  call_intf_num = cmgmd->bDataInterface;
>
>
> On kernel 4.8.8  this crashes hard and produces over a serial link:

Huh?  That device shouldn't ever enter that code path AFAICS.
Unless you wouldn't happen to add a dynamic entry for this device,
would you?  What's the output of

 cat /sys/bus/usb/drivers/cdc_acm/new_id

?

We should probably survive it, but I think the current acm_probe() is
going to barf hard on the last data interface, if probed without the
default NO_UNION_NORMAL quirk.  cdc_parse_cdc_header() will happily
parse all the functional descriptors, including the union pointing to
interfaces 0 and 1.  I might be missing it, but I cannot see any sanity
check verifying that the currently probed interface is actually part of
the set of interfaces pointed to by the union.  There is a sanity check
for the availability of the data interface, but there is none for the
control interface (the assumption of course that that's the interface
we're probing).

I think we need a bit more sanity checking of the union.  This is likely
a generic problem for any CDC driver, so it is worth considering adding
a shared function for that.

And all this fails to explain anything if you didn't add the device
dynamically, of course...



Bjørn


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: crash by cdc_acm driver in kernels 4.8-rc1/5

[Bjørn Mork]

Wim Osterholt  writes:

> On Mon, Nov 21, 2016 at 02:19:32PM +0100, Oliver Neukum wrote:
>> On Thu, 2016-11-17 at 17:11 +0100, Wim Osterholt wrote:
>> 
>> > Nov 17 15:07:51 localhost kernel: Check point  10
>> > Nov 17 15:07:51 localhost kernel: BUG: unable to handle kernel NULL 
>> > pointer dereference at 0249
>> > Nov 17 15:07:51 localhost kernel: IP: [] acm_probe+0x559/0xe53 
>> > [cdc_acm]
>> > Nov 17 15:07:51 localhost kernel: *pde =  
>> > Nov 17 15:07:51 localhost kernel: Oops:  [#1] SMP
>> 
>> I don't understand it, bit please test the attached patch
>> with dynamic debugging for cdc-acm and the kernel log level
>> at maximum. And please repost "lsusb -v" for your device.
>
> I didn't find traces of kernel-4.9-rc5 being ran on any of my laptops, so I
> can't have seen a crash on rc5. It seems rc5 and rc6 is safe now.
>
> I assume you want this on a crashing kernel, but I already removed the
> sources. (Lack of space).
> 4.8.10 is now compiling, that was the fastest option. If that one doesn't
> crash anymore I'll dig up 4.8.8 again.
>
> lsusb -v:
>
> Bus 004 Device 002: ID 0572:1340 Conexant Systems (Rockwell), Inc. 
> Device Descriptor:
>   bLength18
>   bDescriptorType 1
>   bcdUSB   1.10
>   bDeviceClass2 Communications
>   bDeviceSubClass 0 
>   bDeviceProtocol 0 
>   bMaxPacketSize064
>   idVendor   0x0572 Conexant Systems (Rockwell), Inc.
>   idProduct  0x1340 
>   bcdDevice1.00
>   iManufacturer   1 Conexant
>   iProduct2 USB Modem
>   iSerial 3 12345678
>   bNumConfigurations  2
>   Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength   73
> bNumInterfaces  2
> bConfigurationValue 1
> iConfiguration  0 
> bmAttributes 0x80
>   (Bus Powered)
> MaxPower  100mA
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber0
>   bAlternateSetting   0
>   bNumEndpoints   1
>   bInterfaceClass 2 Communications
>   bInterfaceSubClass  2 Abstract (modem)
>   bInterfaceProtocol  1 AT-commands (v.25ter)
>   iInterface  0 
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x81  EP 1 IN
> bmAttributes3
>   Transfer TypeInterrupt
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0040  1x 64 bytes
> bInterval 128
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber1
>   bAlternateSetting   0
>   bNumEndpoints   2
>   bInterfaceClass10 CDC Data
>   bInterfaceSubClass  0 Unused
>   bInterfaceProtocol  0 
>   iInterface  0 
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x82  EP 2 IN
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0040  1x 64 bytes
> bInterval   1
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x02  EP 2 OUT
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0040  1x 64 bytes
> bInterval   1
>   CDC Header:
> bcdCDC   1.10
>   CDC Call Management:
> bmCapabilities   0x03
>   call management
>   use DataInterface
> bDataInterface  1
>   CDC ACM:
> bmCapabilities   0x07
>   sends break
>   line coding and serial state
>   get/set/clear comm features
>   CDC Union:
> bMasterInterface0
> bSlaveInterface 1 
>   Country Selection:
> iCountryCodeRelDate4 04052004
> wCountryCode  0x4803
>   Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength   96
> bNumInterfaces  3
> bConfigurationValue 2
> iConfiguration  0 
> bmAttributes 0x80
>   (Bus Powered)
> MaxPower  100mA
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber0
>   bAlternateSetting   0
> 

Re: [PATCH] usb: gadget: uvc: fix returnvar.cocci warnings

[Laurent Pinchart]

Hi Andrzej and Julia,

Could one of you please submit a patch to fix this ?

On Thursday 17 Sep 2015 13:18:04 Andrzej Pietrasiewicz wrote:
> Hi Julia,
> 
> W dniu 17.09.2015 o 10:57, Julia Lawall pisze:
> > Coccinelle suggests the following patch.  But the code is curious.  Is the
> > function expected to always return a failure value?
> 
> Thank you for catching this. The function is not expected to always
> return a failure value. Fortunately it does not matter anyway because
> the return value of the drop_link() operation is silently ignored by
> its caller in fs/configfs/symlink.c, functions configfs_symlink()
> and configfs_unlink(). For my comments see inline.
> 
> > thanks,
> > julia
> > 
> > On Thu, 17 Sep 2015, kbuild test robot wrote:
> >> TO: Andrzej Pietrasiewicz 
> >> CC: kbuild-...@01.org
> >> CC: Felipe Balbi 
> >> CC: Laurent Pinchart 
> >> CC: "Greg Kroah-Hartman" 
> >> CC: linux-usb@vger.kernel.org
> >> CC: linux-ker...@vger.kernel.org
> >> 
> >> drivers/usb/gadget/function/uvc_configfs.c:866:5-8: Unneeded variable:
> >> "ret". Return "- EINVAL" on line 891>> 
> >>   Remove unneeded variable used to store return value.
> >> 
> >> Generated by: scripts/coccinelle/misc/returnvar.cocci
> >> 
> >> CC: Andrzej Pietrasiewicz 
> >> Signed-off-by: Fengguang Wu 
> >> ---
> >> 
> >> Please take the patch only if it's a positive warning. Thanks!
> >> 
> >>   uvc_configfs.c |3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >> 
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -863,7 +863,6 @@ static int uvcg_streaming_header_drop_li
> >>struct uvcg_streaming_header *src_hdr;
> >>struct uvcg_format *target_fmt = NULL;
> >>struct uvcg_format_ptr *format_ptr, *tmp;
> >> -  int ret = -EINVAL;
> >> 
> >>src_hdr = to_uvcg_streaming_header(src);
> >>mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> >> @@ -888,7 +887,7 @@ static int uvcg_streaming_header_drop_li
> >>   out:
> >>mutex_unlock(>lock);
> >>mutex_unlock(su_mutex);
> >> 
> >> -  return ret;
> >> +  return -EINVAL;
> 
> return 0;

-- 
Regards,

Laurent Pinchart

--
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: crash by cdc_acm driver in kernels 4.8-rc1/5

[Wim Osterholt]

On Mon, Nov 21, 2016 at 02:19:32PM +0100, Oliver Neukum wrote:

> I don't understand it, bit please test the attached patch
> with dynamic debugging for cdc-acm and the kernel log level
> at maximum.

> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 6895f9e..f03b5db 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -1188,6 +1188,12 @@ static int acm_probe(struct usb_interface *intf,
>  
>   cdc_parse_cdc_header(, intf, buffer, buflen);
>   union_header = h.usb_cdc_union_desc;
> +
> + dev_dbg(>dev, "Parsed device header\n");
> + dev_dbg(>dev, "Union descriptor %p\n", h.usb_cdc_union_desc);
> + dev_dbg(>dev, "ACM descriptor %p\n", h.usb_cdc_acm_descriptor);
> + dev_dbg(>dev, "Country descriptor %p\n", 
> h.usb_cdc_country_functional_desc);
> +
>   cmgmd = h.usb_cdc_call_mgmt_descriptor;
>   if (cmgmd)
>   call_intf_num = cmgmd->bDataInterface;


On kernel 4.8.8  this crashes hard and produces over a serial link:

[  156.842106] sysrq: SysRq : Changing Loglevel
[  156.842110] sysrq: Loglevel set to 9
[  156.947852] usbcore: registered new interface driver cdc_acm
[  156.947854] cdc_acm: USB Abstract Control Model driver for USB modems and 
ISDN adapters
[  161.176701] usb 4-1: new full-speed USB device number 2 using uhci_hcd
[  161.383608] usb 4-1: New USB device found, idVendor=0572, idProduct=1340
[  161.384707] usb 4-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[  161.388722] usb 4-1: Product: USB Modem
[  161.392711] usb 4-1: Manufacturer: Conexant
[  161.392714] usb 4-1: SerialNumber: 12345678
[  161.397703] cdc_acm:acm_probe: cdc_acm 4-1:1.0: interfaces are valid
[  161.397731] BUG: unable to handle kernel NULL pointer dereference at 0249
[  161.397740] IP: [] acm_probe+0x580/0xd1e [cdc_acm]
[  161.397742] *pde =  
[  161.397745] Oops:  [#1] SMP
[  161.397786] Modules linked in: cdc_acm radeon drm_kms_helper syscopyarea 
sysfillrect sysimgblt fb_sys_fops ttm drm agpgart i2c_algo_bit fbcon bitblit 
softcursor font tileblit binfmt_misc snd_pcm_oss snd_mixer_oss usb_storage 
usbhid ipw2200 libipw lib80211 snd_intel8x0 cfg80211 snd_ac97_codec ac97_bus 
uhci_hcd snd_pcm ehci_pci snd_timer snd ehci_hcd rfkill usbcore soundcore 
via_rhine firmware_class ppdev pcspkr parport_pc mii lpc_ich parport fan 
usb_common acpi_cpufreq thermal mfd_core floppy button processor
[  161.397790] CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 4.8.8 #2
[  161.397792] Hardware name: MEDIONPC MS-7048/MS-7048, BIOS 6.00 PG 02/12/2004
[  161.397805] Workqueue: usb_hub_wq hub_event [usbcore]
[  161.397807] task: df4c9500 task.stack: df4da000
[  161.397810] EIP: 0060:[] EFLAGS: 00010202 CPU: 0
[  161.397813] EIP is at acm_probe+0x580/0xd1e [cdc_acm]
[  161.397815] EAX: 0246 EBX: dc27b000 ECX: e086c934 EDX: 
[  161.397817] ESI: 0100 EDI:  EBP: df4dbc18 ESP: df4dbb80
[  161.397819]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[  161.397821] CR0: 80050033 CR2: 0249 CR3: 1c45f000 CR4: 0690
[  161.397822] Stack:
[  161.397828]  3640 3662 000e df491d50   0010 
0040
[  161.397835]  0080 0246 dd1fc540 decf5a00 dc468c70 0001 df583a00 
df583a38
[  161.397841]  dc468c00 decf5800 decf5a00  dc452ab0 0004 0246 
df4dbc00
[  161.397842] Call Trace:
[  161.397853]  [] ? __mutex_unlock_slowpath+0xf4/0xfc
[  161.397862]  [] ? usb_probe_interface+0x17b/0x1f6 [usbcore]
[  161.397870]  [] ? usb_probe_interface+0x17b/0x1f6 [usbcore]
[  161.397877]  [] ? driver_probe_device+0x17b/0x30e
[  161.397880]  [] ? driver_probe_device+0x17b/0x30e
[  161.397883]  [] ? bus_for_each_drv+0x59/0x68
[  161.397886]  [] ? bus_for_each_drv+0x59/0x68
[  161.397890]  [] ? __device_attach+0x91/0x105
[  161.397893]  [] ? driver_allows_async_probing+0x2f/0x2f
[  161.397896]  [] ? bus_probe_device+0x27/0x6b
[  161.397899]  [] ? bus_probe_device+0x27/0x6b
[  161.397902]  [] ? device_add+0x289/0x4be
[  161.397911]  [] ? usb_set_configuration+0x5a6/0x5e9 [usbcore]
[  161.397919]  [] ? usb_set_configuration+0x5a6/0x5e9 [usbcore]
[  161.397928]  [] ? generic_probe+0x3b/0x67 [usbcore]
[  161.397937]  [] ? generic_probe+0x3b/0x67 [usbcore]
[  161.397945]  [] ? usb_probe_device+0x49/0x62 [usbcore]
[  161.397953]  [] ? usb_suspend+0xcd/0xcd [usbcore]
[  161.397957]  [] ? driver_probe_device+0x17b/0x30e
[  161.397960]  [] ? driver_probe_device+0x17b/0x30e
[  161.397963]  [] ? bus_for_each_drv+0x59/0x68
[  161.397966]  [] ? bus_for_each_drv+0x59/0x68
[  161.397969]  [] ? __device_attach+0x91/0x105
[  161.397972]  [] ? driver_allows_async_probing+0x2f/0x2f
[  161.397976]  [] ? bus_probe_device+0x27/0x6b
[  161.397979]  [] ? bus_probe_device+0x27/0x6b
[  161.397982]  [] ? device_add+0x289/0x4be
[  161.397985]  [] ? add_device_randomness+0x84/0x9c
[  161.397993]  [] ? usb_new_device+0x29d/0x3b5 [usbcore]
[  161.398001]  [] ? usb_new_device+0x29d/0x3b5 [usbcore]

Re: [PATCH v3 3/5] net: asix: Fix AX88772x resume failures

[Jon Hunter]

Hi Allan,

On 18/11/16 15:09, Jon Hunter wrote:
> Hi Allan,
> 
> On 14/11/16 09:45, ASIX_Allan [Office] wrote:
>> Hi Jon,
>>
>> Please help to double check if the USB host controller of your Terga
>> platform had been powered OFF while running the ax88772_suspend() routine or
>> not? 
> 
> Sorry for the delay. Today I set up a local board to reproduce this on
> and was able to recreate the same problem. The Tegra xhci driver does
> not power off during suspend and simply calls xhci_suspend(). I also
> checked vbus to see if it was turning off but it is not. Furthermore I
> don't see a new USB device detected after the error and so I don't see
> any evidence that it ever disconnects.

In an attempt to isolate if this is a Tegra issue or not, I recompiled 
v4.9-rc6 for x86 and I was able to reproduce the problem on my desktop ...

[  256.030060] PM: Syncing filesystems ... done.
[  256.113925] PM: Preparing system for sleep (mem)
[  256.114119] Freezing user space processes ... (elapsed 0.002 seconds) done.
[  256.116701] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) 
done.
[  256.118041] PM: Suspending system (mem)
[  256.118058] Suspending console(s) (use no_console_suspend to debug)
[  256.118324] asix 1-1.2:1.0 eth2: Failed to read reg index 0x: -19
[  256.118327] asix 1-1.2:1.0 eth2: Error reading Medium Status register: 
ffed
[  256.118329] asix 1-1.2:1.0 eth2: Failed to write reg index 0x: -19
[  256.118332] asix 1-1.2:1.0 eth2: Failed to write Medium Mode mode to 0xfeed: 
ffed
[  256.118374] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[  256.118471] sd 0:0:0:0: [sda] Stopping disk
[  256.152992] hpet1: lost 1 rtc interrupts
[  256.153893] serial 00:06: disabled
[  256.153899] serial 00:06: System wakeup disabled by ACPI
[  256.154068] e1000e: EEE TX LPI TIMER: 0011
[  256.628281] PM: suspend of devices complete after 509.782 msecs
[  256.628620] PM: late suspend of devices complete after 0.336 msecs
[  256.629366] ehci-pci :00:1d.0: System wakeup enabled by ACPI
[  256.629595] tg3 :03:00.0: System wakeup enabled by ACPI
[  256.629601] ehci-pci :00:1a.0: System wakeup enabled by ACPI
[  256.629652] e1000e :00:19.0: System wakeup enabled by ACPI
[  256.629812] xhci_hcd :00:14.0: System wakeup enabled by ACPI
[  256.648347] PM: noirq suspend of devices complete after 19.713 msecs
[  256.648685] ACPI: Preparing to enter system sleep state S3
[  256.668275] PM: Saving platform NVS memory
[  256.668283] Disabling non-boot CPUs ...

To reproduce this, I did the following:

1. Connect the asix device and noted the net interface (ie. eth2)
2. Disabled the interface (ie. sudo ifconfig eth2 down)
3. Ran a suspend-resume cycle using rtcwake (eg. sudo rtcwake -d rtc0 -m mem -s 
5)

Cheers
Jon

-- 
nvpublic
--
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: musb: mark PM functions as __maybe_unused

[Arnd Bergmann]

Building without CONFIG_PM causes a harmless warning:

drivers/usb/musb/musb_core.c:2041:12: error: ‘musb_run_resume_work’ defined but 
not used [-Werror=unused-function]

Removing the #ifdef around the PM code and instead marking the suspend/resume
functions as __maybe_unused will do the right thing without warning.

Fixes: ea2f35c01d5e ("usb: musb: Fix sleeping function called from invalid 
context for hdrc glue")
Signed-off-by: Arnd Bergmann 
---
 drivers/usb/musb/musb_core.c | 17 +
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index c3e172e15ec3..cc8192f1f2af 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2485,8 +2485,6 @@ static int musb_remove(struct platform_device *pdev)
return 0;
 }
 
-#ifdef CONFIG_PM
-
 static void musb_save_context(struct musb *musb)
 {
int i;
@@ -2640,7 +2638,7 @@ static void musb_restore_context(struct musb *musb)
musb_writeb(musb_base, MUSB_INDEX, musb->context.index);
 }
 
-static int musb_suspend(struct device *dev)
+static int __maybe_unused musb_suspend(struct device *dev)
 {
struct musb *musb = dev_to_musb(dev);
unsigned long   flags;
@@ -2667,7 +2665,7 @@ static int musb_suspend(struct device *dev)
return 0;
 }
 
-static int musb_resume(struct device *dev)
+static int __maybe_unused musb_resume(struct device *dev)
 {
struct musb *musb = dev_to_musb(dev);
unsigned long flags;
@@ -2717,7 +2715,7 @@ static int musb_resume(struct device *dev)
return 0;
 }
 
-static int musb_runtime_suspend(struct device *dev)
+static int __maybe_unused musb_runtime_suspend(struct device *dev)
 {
struct musb *musb = dev_to_musb(dev);
 
@@ -2727,7 +2725,7 @@ static int musb_runtime_suspend(struct device *dev)
return 0;
 }
 
-static int musb_runtime_resume(struct device *dev)
+static int __maybe_unused musb_runtime_resume(struct device *dev)
 {
struct musb *musb = dev_to_musb(dev);
unsigned long flags;
@@ -2771,16 +2769,11 @@ static const struct dev_pm_ops musb_dev_pm_ops = {
.runtime_resume = musb_runtime_resume,
 };
 
-#define MUSB_DEV_PM_OPS (_dev_pm_ops)
-#else
-#defineMUSB_DEV_PM_OPS NULL
-#endif
-
 static struct platform_driver musb_driver = {
.driver = {
.name   = (char *)musb_driver_name,
.bus= _bus_type,
-   .pm = MUSB_DEV_PM_OPS,
+   .pm = _dev_pm_ops,
},
.probe  = musb_probe,
.remove = musb_remove,
-- 
2.9.0

--
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: [v5,3/5] USB: ohci: da8xx: Allow a regulator to handle VBUS

[Axel Haslam]

On Mon, Nov 21, 2016 at 5:29 PM, David Lechner  wrote:
> On 11/21/2016 04:22 AM, Axel Haslam wrote:
>>
>> Hi David,
>>
>> Thanks for the review,
>>
>
> You're welcome.
>

 @@ -160,15 +212,41 @@ static void ohci_da8xx_ocic_handler(struct
 da8xx_ohci_root_hub *hub,
 hub->set_power(port, 0);
  }

 +static int ohci_da8xx_regulator_event(struct notifier_block *nb,
 +   unsigned long event, void *data)
 +{
 +   struct da8xx_ohci_hcd *da8xx_ohci =
 +   container_of(nb, struct da8xx_ohci_hcd,
 nb);
 +   struct device *dev = da8xx_ohci->hcd->self.controller;
 +
 +   if (event & REGULATOR_EVENT_OVER_CURRENT) {
 +   dev_warn(dev, "over current event\n");
>>>
>>>
>>>
>>> Won't this result in duplicate overcurrent warnings in the kernel log? It
>>> seems like in previous version of this patch series, we would get an
>>> overcurrent error from the core usb driver.
>>
>>
>> you mean in the regulator driver? i did not make changes to core usb.
>> but, no,  i did not add a print in the fixed regulator driver itself.
>> Since
>> the regulator is  a separate driver, and could be implemented with or
>> without
>> a trace, i think its better to leave this print. It shows that the usb
>> driver
>> has well received the notification.
>>
>
> No, I mean in drivers/usb/core/hub.c. There is
>
> if (status & USB_PORT_STAT_OVERCURRENT)
> dev_err(_dev->dev, "over-current condition\n");
>
> and
>
> if (status & HUB_STATUS_OVERCURRENT)
> dev_err(hub_dev, "over-current condition\n");
>
> In ohci_da8xx_hub_control(), we are setting RH_PS_POCI and RH_PS_OCIC, so
> these messages will be printed via the core hub driver. We don't need to
> print another message from the same event.
>
>

Hi David

I did a couple of tests, and i don't get those prints do you get them?.
What i understand is that they happen when we get a hub event that is
reporting the over current. Which is what the root hub of the davinci system
does not have, and why we use gpios instead).

Regards,
Axel.

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

[PATCHv12 0/3] USB Type-C Connector class

[Heikki Krogerus]

The USB Type-C class is meant to provide unified interface to the
userspace to present the USB Type-C ports in a system.

Changes since v11:
- The port drivers are responsible of removing the alternate
  modes (just like the documentation already said).

Changes since v10:
- Using ATTRIBUTE_GROUPS and DEVICE_ATTR marcos everywhere
- Moved sysfs_match_string to lib/string.c
- Rationalized uevents
- Calling ida_destroy

Changes since v9:
- Minor typec_wcove.c cleanup as proposed by Guenter Roeck. No
  function affect.

Changes since v8:
- checking sysfs_streq() result correctly in sysfs_strmatch
- fixed accessory check in supported_accessory_mode
- using "none" as the only string that can clear the preferred role

Changes since v7:
- Removed "type" attribute from partners
- Added supports_usb_power_delivery attribute for partner and cable

Changes since v6:
- current_vconn_role attr renamed to vconn_source (no API changes)
- Small documentation improvements proposed by Vincent Palatin

Changes since v5:
- Only updating the roles based on driver notifications
- Added MODULE_ALIAS for the WhiskeyCove module
- Including the patch that creates the actual platform device for the
  WhiskeyCove Type-C PHY in this series.

Changes since v4:
- Remove the port lock completely

Changes since v3:
- Documentation cleanup as proposed by Roger Quadros
- Setting partner altmodes member to NULL on removal and fixing a
  warning, as proposed by Guenter Roeck
- Added the following attributes for partners and cables:
  * supports_usb_power_delivery
  * id_header_vdo
- "id_header_vdo" is visible only when the partner or cable supports
  USB Power Delivery communication.
- Partner attribute "accessory" is hidden when the partner type is not
  "Accessory".

Changes since v2:
- Notification on role and alternate mode changes
- cleanups

Changes since v1:
- Completely rewrote alternate mode support
- Patners, cables and cable plugs presented as devices.


Heikki Krogerus (3):
  lib/string: add sysfs_match_string helper
  usb: USB Type-C connector class
  usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

 Documentation/ABI/testing/sysfs-class-typec |  222 ++
 Documentation/usb/typec.txt |  103 +++
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |   21 +
 drivers/usb/typec/Makefile  |2 +
 drivers/usb/typec/typec.c   | 1013 +++
 drivers/usb/typec/typec_wcove.c |  372 ++
 include/linux/string.h  |   10 +
 include/linux/usb/typec.h   |  252 +++
 lib/string.c|   26 +
 12 files changed, 2034 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 drivers/usb/typec/typec_wcove.c
 create mode 100644 include/linux/usb/typec.h

-- 
2.10.2

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

[PATCHv12 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

The purpose of USB Type-C connector class is to provide
unified interface for the user space to get the status and
basic information about USB Type-C connectors on a system,
control over data role swapping, and when the port supports
USB Power Delivery, also control over power role swapping
and Alternate Modes.

Signed-off-by: Heikki Krogerus 
---
 Documentation/ABI/testing/sysfs-class-typec |  222 ++
 Documentation/usb/typec.txt |  103 +++
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |7 +
 drivers/usb/typec/Makefile  |1 +
 drivers/usb/typec/typec.c   | 1013 +++
 include/linux/usb/typec.h   |  252 +++
 9 files changed, 1611 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 include/linux/usb/typec.h

diff --git a/Documentation/ABI/testing/sysfs-class-typec 
b/Documentation/ABI/testing/sysfs-class-typec
new file mode 100644
index 000..4fac77c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -0,0 +1,222 @@
+USB Type-C port devices (eg. /sys/class/typec/usbc0/)
+
+What:  /sys/class/typec//current_data_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   The current USB data role the port is operating in. This
+   attribute can be used for requesting data role swapping on the
+   port. Swapping is supported as synchronous operation, so
+   write(2) to the attribute will not return until the operation
+   has finished. The attribute is notified about role changes so
+   that poll(2) on the attribute wakes up. Change on the role will
+   also generate uevent KOBJ_CHANGE on the port.
+
+   Valid values:
+   - host
+   - device
+
+What:  /sys/class/typec//current_power_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   The current power role of the port. This attribute can be used
+   to request power role swap on the port when the port supports
+   USB Power Delivery. Swapping is supported as synchronous
+   operation, so write(2) to the attribute will not return until
+   the operation has finished. The attribute is notified about role
+   changes so that poll(2) on the attribute wakes up. Change on the
+   role will also generate uevent KOBJ_CHANGE.
+
+   Valid values:
+   - source
+   - sink
+
+What:  /sys/class/typec//vconn_source
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows is the port VCONN Source. This attribute can be used to
+   request VCONN swap to change the VCONN Source during connection
+   when both the port and the partner support USB Power Delivery.
+   Swapping is supported as synchronous operation, so write(2) to
+   the attribute will not return until the operation has finished.
+   The attribute is notified about VCONN source changes so that
+   poll(2) on the attribute wakes up. Change on VCONN source also
+   generates uevent KOBJ_CHANGE.
+
+   Valid values are:
+   - 0 when the port is not the VCONN Source
+   - 1 when the port is the VCONN Source
+
+What:  /sys/class/typec//power_operation_mode
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows the current power operational mode the port is in.
+
+   Valid values:
+   - USB - Normal power levels defined in USB specifications
+   - BC1.2 - Power levels defined in Battery Charging Specification
+ v1.2
+   - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
+   specification.
+   - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
+   specification.
+- USB Power Delivery - The voltages and currents defined in USB
+  Power Delivery specification
+
+What:  /sys/class/typec//preferred_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:

[PATCHv12 1/3] lib/string: add sysfs_match_string helper

[Heikki Krogerus]

Make a simple helper for matching strings with sysfs
attribute files. In most parts the same as match_string(),
except sysfs_match_string() uses sysfs_streq() instead of
strcmp() for matching. This is more convenient when used
with sysfs attributes.

Signed-off-by: Heikki Krogerus 
---
 include/linux/string.h | 10 ++
 lib/string.c   | 26 ++
 2 files changed, 36 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 26b6f6a..c4011b2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
 }
 
 int match_string(const char * const *array, size_t n, const char *string);
+int __sysfs_match_string(const char * const *array, size_t n, const char *s);
+
+/**
+ * sysfs_match_string - matches given string in an array
+ * @_a: array of strings
+ * @_s: string to match with
+ *
+ * Helper for __sysfs_match_string(). Calculates the size of @a automatically.
+ */
+#define sysfs_match_string(_a, _s) __sysfs_match_string(_a, ARRAY_SIZE(_a), _s)
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/string.c b/lib/string.c
index ed83562..c7a20cb 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -656,6 +656,32 @@ int match_string(const char * const *array, size_t n, 
const char *string)
 }
 EXPORT_SYMBOL(match_string);
 
+/**
+ * __sysfs_match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq instead of strcmp for matching.
+ */
+int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+{
+   const char *item;
+   int index;
+
+   for (index = 0; index < n; index++) {
+   item = array[index];
+   if (!item)
+   break;
+   if (!sysfs_streq(item, str))
+   return index;
+   }
+
+   return -EINVAL;
+}
+EXPORT_SYMBOL(__sysfs_match_string);
+
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value
-- 
2.10.2

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

[PATCHv12 3/3] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

[Heikki Krogerus]

This adds driver for the USB Type-C PHY on Intel WhiskeyCove
PMIC which is available on some of the Intel Broxton SoC
based platforms.

Signed-off-by: Heikki Krogerus 
---
 drivers/usb/typec/Kconfig   |  14 ++
 drivers/usb/typec/Makefile  |   1 +
 drivers/usb/typec/typec_wcove.c | 372 
 3 files changed, 387 insertions(+)
 create mode 100644 drivers/usb/typec/typec_wcove.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 17792f9..2abbcb0 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,4 +4,18 @@ menu "USB Power Delivery and Type-C drivers"
 config TYPEC
tristate
 
+config TYPEC_WCOVE
+   tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
+   depends on ACPI
+   depends on INTEL_SOC_PMIC
+   depends on INTEL_PMC_IPC
+   select TYPEC
+   help
+ This driver adds support for USB Type-C detection on Intel Broxton
+ platforms that have Intel Whiskey Cove PMIC. The driver can detect the
+ role and cable orientation.
+
+ To compile this driver as module, choose M here: the module will be
+ called typec_wcove
+
 endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 1012a8b..b9cb862 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_TYPEC)+= typec.o
+obj-$(CONFIG_TYPEC_WCOVE)  += typec_wcove.o
diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
new file mode 100644
index 000..953d59b
--- /dev/null
+++ b/drivers/usb/typec/typec_wcove.c
@@ -0,0 +1,372 @@
+/**
+ * typec_wcove.c - WhiskeyCove PMIC USB Type-C PHY driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ * Author: Heikki Krogerus 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Register offsets */
+#define WCOVE_CHGRIRQ0 0x4e09
+#define WCOVE_PHYCTRL  0x5e07
+
+#define USBC_CONTROL1  0x7001
+#define USBC_CONTROL2  0x7002
+#define USBC_CONTROL3  0x7003
+#define USBC_CC1_CTRL  0x7004
+#define USBC_CC2_CTRL  0x7005
+#define USBC_STATUS1   0x7007
+#define USBC_STATUS2   0x7008
+#define USBC_STATUS3   0x7009
+#define USBC_IRQ1  0x7015
+#define USBC_IRQ2  0x7016
+#define USBC_IRQMASK1  0x7017
+#define USBC_IRQMASK2  0x7018
+
+/* Register bits */
+
+#define USBC_CONTROL1_MODE_DRP(r)  (((r) & ~0x7) | 4)
+
+#define USBC_CONTROL2_UNATT_SNKBIT(0)
+#define USBC_CONTROL2_UNATT_SRCBIT(1)
+#define USBC_CONTROL2_DIS_ST   BIT(2)
+
+#define USBC_CONTROL3_PD_DIS   BIT(1)
+
+#define USBC_CC_CTRL_VCONN_EN  BIT(1)
+
+#define USBC_STATUS1_DET_ONGOING   BIT(6)
+#define USBC_STATUS1_RSLT(r)   ((r) & 0xf)
+#define USBC_RSLT_NOTHING  0
+#define USBC_RSLT_SRC_DEFAULT  1
+#define USBC_RSLT_SRC_1_5A 2
+#define USBC_RSLT_SRC_3_0A 3
+#define USBC_RSLT_SNK  4
+#define USBC_RSLT_DEBUG_ACC5
+#define USBC_RSLT_AUDIO_ACC6
+#define USBC_RSLT_UNDEF15
+#define USBC_STATUS1_ORIENT(r) (((r) >> 4) & 0x3)
+#define USBC_ORIENT_NORMAL 1
+#define USBC_ORIENT_REVERSE2
+
+#define USBC_STATUS2_VBUS_REQ  BIT(5)
+
+#define USBC_IRQ1_ADCDONE1 BIT(2)
+#define USBC_IRQ1_OVERTEMP BIT(1)
+#define USBC_IRQ1_SHORTBIT(0)
+
+#define USBC_IRQ2_CC_CHANGEBIT(7)
+#define USBC_IRQ2_RX_PDBIT(6)
+#define USBC_IRQ2_RX_HRBIT(5)
+#define USBC_IRQ2_RX_CRBIT(4)
+#define USBC_IRQ2_TX_SUCCESS   BIT(3)
+#define USBC_IRQ2_TX_FAIL  BIT(2)
+
+#define USBC_IRQMASK1_ALL  (USBC_IRQ1_ADCDONE1 | USBC_IRQ1_OVERTEMP | \
+USBC_IRQ1_SHORT)
+
+#define USBC_IRQMASK2_ALL  (USBC_IRQ2_CC_CHANGE | USBC_IRQ2_RX_PD | \
+USBC_IRQ2_RX_HR | USBC_IRQ2_RX_CR | \
+USBC_IRQ2_TX_SUCCESS | USBC_IRQ2_TX_FAIL)
+
+struct wcove_typec {
+   struct mutex lock; /* device lock */
+   struct device *dev;
+   struct regmap *regmap;
+   struct typec_port *port;
+   struct typec_capability cap;
+   struct typec_connection con;
+   struct typec_partner partner;
+};
+
+enum wcove_typec_func {
+   WCOVE_FUNC_DRIVE_VBUS = 1,
+   WCOVE_FUNC_ORIENTATION,
+   WCOVE_FUNC_ROLE,
+   WCOVE_FUNC_DRIVE_VCONN,
+};
+
+enum wcove_typec_orientation {
+   

Re: [PATCH 5/5] usb: dwc2: fix kernel-doc for dwc2_set_param

[Felipe Balbi]

Hi,

Stefan Wahren  writes:
> Hi Felipe,
>
> Am 22.11.2016 um 13:23 schrieb Felipe Balbi:
>> Hi,
>>
>> Stefan Wahren  writes:
>>> Since there is no parameter @value replace it with @legacy.
>>>
>>> Fixes: 05ee799f202 ("usb: dwc2: Move gadget settings into core_params")
>>> Signed-off-by: Stefan Wahren 
>>> ---
>>>  drivers/usb/dwc2/params.c |2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>>> index 11fe68a..10407cb 100644
>>> --- a/drivers/usb/dwc2/params.c
>>> +++ b/drivers/usb/dwc2/params.c
>>> @@ -320,7 +320,7 @@ static void dwc2_set_core_param(void *param, u8 size, 
>>> u64 value)
>>>   * @size: The size of the core parameter in bytes, or 0 for bool.
>>>   *
>>>   * This function looks up @property and sets the @param to that value.
>>> - * If the property doesn't exist it uses the passed-in @value. It will
>>> + * If the property doesn't exist it uses the passed-in @legacy value. It 
>>> will
>> This doesn't fix any bugs. 
>
> you are right. I found this documentation bug while fixing the issues
> before.
>
> Since this is the last patch of the series, you could ignore it. And i
> resend it without fixes tag after the merge window.

fine by me :-)

>> Also, is @legacy a parameter?
>>
>
> |/** * dwc2_set_param() - Set a core parameter * * @hsotg: Programming
> view of the DWC_otg controller * @param: Pointer to the parameter to set
> * @lookup: True if the property should be looked up * @property: The
> device property to read * @legacy: The param value to set if @property
> is not available. This * will typically be the legacy value set in the
> static * params structure. * @def: The default value * @min: The minimum
> value * @max: The maximum value * @size: The size of the core parameter
> in bytes, or 0 for bool. * * This function looks up @property and sets
> the @param to that value. * If the property doesn't exist it uses the
> passed-in @value. It will * verify that the value falls between @min and
> @max. If it doesn't, * it will output an error and set the parameter to
> either @def or, * failing that, to @min. * * The @size is used to write
> to @param and to query the device * properties so that this same
> function can be used with different * types of parameters. */ static
> void dwc2_set_param(struct dwc2_hsotg *hsotg, void *param, bool lookup,
> char *property, u64 legacy, u64 def, u64 min, u64 max, u8 size)|

heh, a little difficult to read, but got your point ;-) thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 5/5] usb: dwc2: fix kernel-doc for dwc2_set_param

[Stefan Wahren]

Hi Felipe,

Am 22.11.2016 um 13:23 schrieb Felipe Balbi:
> Hi,
>
> Stefan Wahren  writes:
>> Since there is no parameter @value replace it with @legacy.
>>
>> Fixes: 05ee799f202 ("usb: dwc2: Move gadget settings into core_params")
>> Signed-off-by: Stefan Wahren 
>> ---
>>  drivers/usb/dwc2/params.c |2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>> index 11fe68a..10407cb 100644
>> --- a/drivers/usb/dwc2/params.c
>> +++ b/drivers/usb/dwc2/params.c
>> @@ -320,7 +320,7 @@ static void dwc2_set_core_param(void *param, u8 size, 
>> u64 value)
>>   * @size: The size of the core parameter in bytes, or 0 for bool.
>>   *
>>   * This function looks up @property and sets the @param to that value.
>> - * If the property doesn't exist it uses the passed-in @value. It will
>> + * If the property doesn't exist it uses the passed-in @legacy value. It 
>> will
> This doesn't fix any bugs. 

you are right. I found this documentation bug while fixing the issues
before.

Since this is the last patch of the series, you could ignore it. And i
resend it without fixes tag after the merge window.

> Also, is @legacy a parameter?
>

|/** * dwc2_set_param() - Set a core parameter * * @hsotg: Programming
view of the DWC_otg controller * @param: Pointer to the parameter to set
* @lookup: True if the property should be looked up * @property: The
device property to read * @legacy: The param value to set if @property
is not available. This * will typically be the legacy value set in the
static * params structure. * @def: The default value * @min: The minimum
value * @max: The maximum value * @size: The size of the core parameter
in bytes, or 0 for bool. * * This function looks up @property and sets
the @param to that value. * If the property doesn't exist it uses the
passed-in @value. It will * verify that the value falls between @min and
@max. If it doesn't, * it will output an error and set the parameter to
either @def or, * failing that, to @min. * * The @size is used to write
to @param and to query the device * properties so that this same
function can be used with different * types of parameters. */ static
void dwc2_set_param(struct dwc2_hsotg *hsotg, void *param, bool lookup,
char *property, u64 legacy, u64 def, u64 min, u64 max, u8 size)|



--
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: [PATCHv11 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

On Mon, Nov 21, 2016 at 03:45:06PM +0100, Greg KH wrote:
> Again, free the device for which this release function is being called
> for, that is why it is there.

I will.

Thanks,

-- 
heikki
--
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: [PATCHv11 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

On Tue, Nov 22, 2016 at 12:51:11PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 21, 2016 at 03:45:06PM +0100, Greg KH wrote:
> > > We could allocate an extra structure for the partner when
> > > typec_connect() is called, but we would do that just for the sake of
> > > having something to free in the release hook. It would not be useful
> > > for anything. It would not help us increase/decrease the reference
> > > count of the device, and the port driver would still have to provide
> > > details about the partner capabilities the moment it tells us the
> > > partner was connected.
> > 
> > Again, free the device for which this release function is being called
> > for, that is why it is there.
> 
> The struct device is now member of struct typec_partner. This is what
> a typical port driver would have (I hope it's readable):
> 
> struct my_port {
> /* This structure is provided by the class */
> struct typec_port *port;
> 
> /*
>  * Don't forget, there can only be one partner at a time
>  */
> struct typec_partner partner; /* NOTE: this is not a pointer */
> };
> 
> int my_interrupt(...)
> {
> ...
> /*
>  * Connection happened (I'm skipping the typec_connection
>  * wrapper in this example)
>  */
> my_port->partner.usb_pd = ...
> ...
> ret = typec_connect(my_port->port, _port->partner);
> ...
> /*
>  * Disconnect
>  */
> typec_disconnect(my_port->port);
> memset(_port->partner, 0, sizeof(struct typec_partner));
> ...
> }
> 
> int my_probe(...)
> {
> struct my_port *my_port;
> ...
> my_port = devm_kzalloc(...
> ...
> my_port->port = typec_register_port(...
> ...
> }
> 
> To have something to free when the partner device's reference counter
> goes to zero and release is called (this happens after all the
> alternate modes, so the children, and the device are unregistered), we
> will need an extra structure, just for the fun of having something to
> free in release.
> 
> struct internal_partner_structure {
> struct device dev;
> struct typec_partner *partner_capabilities; /* port driver provides */
> };
> 
> Why is this necessary in this case? It is just something extra we have
> to do, just so we can allocate that when connection happens and the
> partner device is generated, and so we can then free that thing when
> release gets called? It does not give us anything. It does not affect
> anything.

Blah, ignore that message. I'm talking about the wrong thing here.

Sorry about that.

-- 
heikki
--
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 net 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-18 07:03 AM, Mark Lord wrote:

On 16-11-18 02:57 AM, Hayes Wang wrote:
..

Besides, the maximum data length which the RTL8152 would send to
the host is 16KB. That is, if the agg_buf_sz is 16KB, the host
wouldn't split it. However, you still see problems for it.


How does the RTL8152 know that the limit is 16KB,
rather than some other number?  Is this a hardwired number
in the hardware, or is it a parameter that the software
sends to the chip during initialization?

..

The first issue is that a packet sometimes begins in one URB,
and completes in the next URB, without an rx_desc at the start
of the second URB.  This I have already reported earlier.


Long run tests over the weekend, with the invalidate_dcache_range() call
before the inner loop of r8152_rx_bottom(), turned up a few instances
where packets were truncated inside a 16384 byte URB buffer, without filling 
the URB.

[10.293228] r8152_rx_bottom: 4278 corrupted urb: head=9d21 
urb_offset=2856/3376 pkt_len(1518) exceeds remainder(496)
[10.304523] r8152_dump_rx_desc: 044805ee 4008 006005dc 0602  
 rx_len=1518
..
[   16.660431] r8152_rx_bottom: 7802 corrupted urb: head=9d1f8000 
urb_offset=1544/2064 pkt_len(1518) exceeds remainder(496)
[   16.671719] r8152_dump_rx_desc: 044805ee 4048 004005dc 46020006  
 rx_len=1518

The r8152.c driver attempted to build skb's for the entire packet size,
even though the 1518-byte packets had only 496-bytes of data in the URB.
It is not clear what the chip did with the rest of the packets in question,
but the next URBs in each case began with a new/real rx_desc and new packet.

There were also unconnected events during the test runs where the
test code noticed totally invalid rx_desc structs in the middles of URBs.
The stock driver would again have attempted to treat those as "valid" (ugh).

..
[   10.273906] r8152_check_rx_desc: rx_desc looks bad.
[   10.279012] r8152_rx_bottom: 4338 corrupted urb. head=9d21 
urb_offset=2856/3376 len_used=2880
[   10.288196] r8152_dump_rx_desc: 312e3239 382e3836 0a20382e 3d435253 3034336d 
202f3a30 rx_len=12857

..
[7.184565] r8152_check_rx_desc: rx_desc looks bad.
[7.189657] r8152_rx_bottom: 1678 corrupted urb. head=9d21 
urb_offset=2856/3376 len_used=2880
[7.198852] r8152_dump_rx_desc: a1388402 803c9001 84380810 a67c5c4c a77c782b 
c64c782b rx_len=1026
..
[   10.351251] r8152_check_rx_desc: rx_desc looks bad.
[   10.356356] r8152_rx_bottom: 4397 corrupted urb. head=9d20c000 
urb_offset=4400/7984 len_used=4424
[   10.365543] r8152_dump_rx_desc: 312e3239 382e3836 0a20382e 3d435253 3034336d 
202f3a30 rx_len=12857
..
[   10.518119] r8152_check_rx_desc: rx_desc looks bad.
[   10.523204] r8152_rx_bottom: 4458 corrupted urb. head=9d21 
urb_offset=4400/7984 len_used=4424
[   10.532416] r8152_dump_rx_desc: 54544120 6e3d5352 636f6c6f 65762c6b 343d7372 
6464612c rx_len=16672
..


But the driver, as written, sometimes accesses bytes outside
of the 16KB URB buffer, because it trusts the non-existent
rx_desc in these cases, and also because it accesses bytes
from the rx_desc without first checking whether there is
sufficient remaining space in the URB to hold an rx_desc.

These incorrect accesses sometimes touch memory outside
of the URB buffer.  Since the driver allocates all of its
rx URB buffers at once, they are highly likely to be
physically (and therefore virtually) adjacent in memory.

So mistakenly accessing beyond the end of one buffer will
often result in a read from memory of the next URB buffer.
Which causes a portion of it to be loaded in the the D-cache.

When that URB is subsequently filled by DMA, there then exists
a data-consistency issue:  the D-cache contains stale information
from before the latest DMA cycle.

So this explains the strange memory behaviour observed earlier on.
When I add a call to invalidate_dcache_range() to the driver
just before it begins examining a new rx URB, the problems go away.
So this confirms the observations.

Using non-cacheable RAM also makes the problem go away.
But neither is a fix for the real buffer overrun accesses in the driver.

Fix the "packet spans URBs" bug, and fix the driver to ALWAYS
test lengths/ranges before accessing the actual buffer,
and everything should begin working reliably.

--
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 5/5] usb: dwc2: fix kernel-doc for dwc2_set_param

[Felipe Balbi]

Hi,

Stefan Wahren  writes:
> Since there is no parameter @value replace it with @legacy.
>
> Fixes: 05ee799f202 ("usb: dwc2: Move gadget settings into core_params")
> Signed-off-by: Stefan Wahren 
> ---
>  drivers/usb/dwc2/params.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 11fe68a..10407cb 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -320,7 +320,7 @@ static void dwc2_set_core_param(void *param, u8 size, u64 
> value)
>   * @size: The size of the core parameter in bytes, or 0 for bool.
>   *
>   * This function looks up @property and sets the @param to that value.
> - * If the property doesn't exist it uses the passed-in @value. It will
> + * If the property doesn't exist it uses the passed-in @legacy value. It will

This doesn't fix any bugs. Also, is @legacy a parameter?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 0/5] usb: dwc2: fix parameter handling

[Felipe Balbi]

Hi,

John Youn  writes:
> On 11/20/2016 1:26 PM, Stefan Wahren wrote:
>> This patch series fixes several parameter handling issues
>> found on bcm2835 in gadget mode. It's based on Felipe's USB next.
>> 
>> Stefan Wahren (5):
>>   usb: dwc2: Do not set host parameter in peripheral mode
>>   usb: dwc2: fix dwc2_get_device_property for u8 and u16
>>   usb: dwc2: fix default value for DMA support
>>   usb: dwc2: gadget: fix default value for gadget-dma-desc
>>   usb: dwc2: fix kernel-doc for dwc2_set_param
>> 
>>  drivers/usb/dwc2/params.c |   32 ++--
>>  1 file changed, 10 insertions(+), 22 deletions(-)
>> 
>
> For this series:
>
> Acked-by: John Youn 
>
>
> Felipe,
>
> This is too late for 4.10-rc1 right?
>
> Can you queue for 4.10 fixes. I can remind you after 4.10-rc1 if it's
> too early for that.

I can keep it in testing/fixes rebased on 'the next of the day' until
v4.10-rc1 is tagged. No problems.

-- 
balbi


signature.asc
Description: PGP signature

Re: Flood of hub_ext_port_status failed (err = -71) messages

[Mathias Nyman]

On 22.11.2016 11:52, Simon Arlott wrote:

On 22/11/16 08:37, Mathias Nyman wrote:

On 21.11.2016 23:52, Simon Arlott wrote:

2016-06-17T09:56:09.219+01:00  kernel: xhci_hcd :00:14.0: URB 
transfer length is wrong, xHC issue? req. len = 4, act. len = 4294967292


Your broken hardware triggered another interesting issue.
The len value 4294967292 looks like a u32 wrapped around.

What kernelversion was this?


These versions:


   1 Linux version 4.4.0-24-generic (buildd@lgw01-12) (gcc version 5.3.1 
20160413 (Ubuntu 5.3.1-14ubuntu2.1) ) #43-Ubuntu SMP Wed Jun 8 19:27:37 UTC 
2016 (Ubuntu 4.4.0-24.43-generic 4.4.10)
  134694 xhci_hcd :00:14.0: URB transfer length is wrong, xHC issue? req. 
len = 4, act. len = 4294967292



Ok, there are no fixes in the control transfers pde since 4.4-0-24, so whatever 
is causing this is still there.
Probably related to how we calculate the length of these erronous control 
transfers.

one clear fault is that we overwrite the previous error code with a zero if the 
length mismatch.

Can you take a log with xhci debugging enabled, this can be done by:
echo -n 'module xhci_hcd =p' > /sys/kernel/debug/dynamic_debug/control
before plugging in the faulty hub (cable)

A usbmon trace would also help, see Documentation/usb/usbmon.txt

How about If I write some test patches, do you have the time to apply them and 
try them out?

-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: [PATCHv11 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

Hi Greg,

On Mon, Nov 21, 2016 at 03:45:06PM +0100, Greg KH wrote:
> > We could allocate an extra structure for the partner when
> > typec_connect() is called, but we would do that just for the sake of
> > having something to free in the release hook. It would not be useful
> > for anything. It would not help us increase/decrease the reference
> > count of the device, and the port driver would still have to provide
> > details about the partner capabilities the moment it tells us the
> > partner was connected.
> 
> Again, free the device for which this release function is being called
> for, that is why it is there.

The struct device is now member of struct typec_partner. This is what
a typical port driver would have (I hope it's readable):

struct my_port {
/* This structure is provided by the class */
struct typec_port *port;

/*
 * Don't forget, there can only be one partner at a time
 */
struct typec_partner partner; /* NOTE: this is not a pointer */
};

int my_interrupt(...)
{
...
/*
 * Connection happened (I'm skipping the typec_connection
 * wrapper in this example)
 */
my_port->partner.usb_pd = ...
...
ret = typec_connect(my_port->port, _port->partner);
...
/*
 * Disconnect
 */
typec_disconnect(my_port->port);
memset(_port->partner, 0, sizeof(struct typec_partner));
...
}

int my_probe(...)
{
struct my_port *my_port;
...
my_port = devm_kzalloc(...
...
my_port->port = typec_register_port(...
...
}

To have something to free when the partner device's reference counter
goes to zero and release is called (this happens after all the
alternate modes, so the children, and the device are unregistered), we
will need an extra structure, just for the fun of having something to
free in release.

struct internal_partner_structure {
struct device dev;
struct typec_partner *partner_capabilities; /* port driver provides */
};

Why is this necessary in this case? It is just something extra we have
to do, just so we can allocate that when connection happens and the
partner device is generated, and so we can then free that thing when
release gets called? It does not give us anything. It does not affect
anything.


Thanks,

-- 
heikki
--
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: Flood of hub_ext_port_status failed (err = -71) messages

[Simon Arlott]

On 22/11/16 08:37, Mathias Nyman wrote:
> On 21.11.2016 23:52, Simon Arlott wrote:
>> 2016-06-17T09:56:09.219+01:00  kernel: xhci_hcd :00:14.0: URB 
>> transfer length is wrong, xHC issue? req. len = 4, act. len = 4294967292
> 
> Your broken hardware triggered another interesting issue.
> The len value 4294967292 looks like a u32 wrapped around.
> 
> What kernelversion was this?

These versions:

  4 Linux version 4.2.0-34-generic (buildd@lgw01-54) (gcc version 5.2.1 
20151010 (Ubuntu 5.2.1-22ubuntu2) ) #39-Ubuntu SMP Thu Mar 10 22:13:01 UTC 2016 
(Ubuntu 4.2.0-34.39-generic 4.2.8-ckt4)
  1 xhci_hcd :00:14.0: URB transfer length is wrong, xHC issue? req. 
len = 0, act. len = 4294967288

  1 Linux version 4.2.0-35-generic (buildd@lgw01-43) (gcc version 5.2.1 
20151010 (Ubuntu 5.2.1-22ubuntu2) ) #40-Ubuntu SMP Tue Mar 15 22:15:45 UTC 2016 
(Ubuntu 4.2.0-35.40-generic 4.2.8-ckt5)
 11 xhci_hcd :00:14.0: URB transfer length is wrong, xHC issue? req. 
len = 0, act. len = 4294967288

  1 Linux version 4.4.0-22-generic (buildd@lgw01-41) (gcc version 5.3.1 
20160413 (Ubuntu 5.3.1-14ubuntu2) ) #40-Ubuntu SMP Thu May 12 22:03:46 UTC 2016 
(Ubuntu 4.4.0-22.40-generic 4.4.8)
  1 xhci_hcd :00:14.0: URB transfer length is wrong, xHC issue? req. 
len = 0, act. len = 4294967288
 82 xhci_hcd :00:14.0: URB transfer length is wrong, xHC issue? req. 
len = 4, act. len = 4294967292
  2 xhci_hcd :00:14.0: URB transfer length is wrong, xHC issue? req. 
len = 0, act. len = 4294967288

  1 Linux version 4.4.0-24-generic (buildd@lgw01-12) (gcc version 5.3.1 
20160413 (Ubuntu 5.3.1-14ubuntu2.1) ) #43-Ubuntu SMP Wed Jun 8 19:27:37 UTC 
2016 (Ubuntu 4.4.0-24.43-generic 4.4.10)
 134694 xhci_hcd :00:14.0: URB transfer length is wrong, xHC issue? req. 
len = 4, act. len = 4294967292

-- 
Simon Arlott
--
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: [PATCHv11 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

On Mon, Nov 21, 2016 at 07:33:45AM -0800, Guenter Roeck wrote:
> On 11/21/2016 06:23 AM, Heikki Krogerus wrote:
> > On Mon, Nov 21, 2016 at 03:11:03PM +0200, Heikki Krogerus wrote:
> > > Hi Greg,
> > > 
> > > On Mon, Nov 21, 2016 at 11:35:28AM +0100, Greg KH wrote:
> > > > > +static void typec_partner_release(struct device *dev)
> > > > > +{
> > > > > + struct typec_port *port = to_typec_port(dev->parent);
> > > > > +
> > > > > + typec_unregister_altmodes(dev);
> > > > > + port->partner = NULL;
> > > > > +}
> > > > 
> > > > This doesn't feel right, why are you both exporting
> > > > typec_unregister_altmodes() and also calling it from release callbacks?
> > > > That implies there is two way to clean stuff up, so what is a driver
> > > > writer to use?  Please simplify and force it to be one way or the other.
> > > 
> > > OK.
> > 
> > Guenter did you need to also remove the alternate modes in drivers, or
> > can we just do it here?
> > 
> 
> It is currently called explicitly on a data role change, when executing
> a hard reset, on detach, and during error recovery. Most of those would
> also unregister the partner, so I should be able to drop those calls
> (or maybe I'll have to - I will see when testing), but I am not sure
> how to handle data role changes.

Let's keep this in the drivers. Actually, we can't unregister them
here. The partner is the parent of the alternate modes devices. Sorry
about the hassle.


Thanks,

-- 
heikki
--
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: Flood of hub_ext_port_status failed (err = -71) messages

[Mathias Nyman]

On 21.11.2016 23:52, Simon Arlott wrote:

I have a 5-port USB 2.0 hub attached to a USB 3.0 host by a long length
of Cat 5e cable (via a pair of cheap USB<->Cat5e adapters from eBay)
that only works at full speed and doesn't stay connected all the time.

This wouldn't be a problem except that the kernel can get stuck in a
loop with the "URB transfer length is wrong, xHC issue?" error.

The message "hub_ext_port_status failed (err = -71)" in particular can
occur 70+ times for every instance of this error. There's no guarantee
that it will ever disconnect the device and stop doing this, it has
previously suddenly started generating 30MB/minute of log messages for
30 minutes until I unplugged the device.

Would it be ok to rate limit both of these messages to reduce the impact
of poor connections?

2016-06-17T09:56:09.219+01:00  kernel: xhci_hcd :00:14.0: URB 
transfer length is wrong, xHC issue? req. len = 4, act. len = 4294967292


Your broken hardware triggered another interesting issue.
The len value 4294967292 looks like a u32 wrapped around.

What kernelversion was this?

-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: [PATCHv11 2/3] usb: USB Type-C connector class

[Greg KH]

On Tue, Nov 22, 2016 at 09:58:13AM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 21, 2016 at 03:46:08PM +0100, Greg KH wrote:
> > > > > +
> > > > > +config TYPEC
> > > > > + tristate
> > > > 
> > > > Hah, that says NOTHING about what this code is at all.
> > > 
> > > Alone the class driver does nothing. Why would the user need to be
> > > aware of if it when selecting the Type-C drivers, and what can the
> > > user use that information for?
> > 
> > If you see a blank Kconfig option, what are you supposed to do with it?
> > How do you know if you need to enable it or not?  Are you just supposed
> > to guess?
> 
> But you don't see anything when you are selecting the drivers and that
> is the point. You now can't select this separately. There is now no
> option for it.
> 
> Why should we bother the user with this? The user is most likely only
> interested in the drivers and by selecting those the user will get the
> interface. The drivers will need to have the dependency to the class
> set correctly in any case.

So with this patch, no code gets built or asked about in Kconfig?

Odd, if so, sorry for the noise...

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: Flood of hub_ext_port_status failed (err = -71) messages

[Greg KH]

On Mon, Nov 21, 2016 at 09:52:56PM +, Simon Arlott wrote:
> I have a 5-port USB 2.0 hub attached to a USB 3.0 host by a long length
> of Cat 5e cable (via a pair of cheap USB<->Cat5e adapters from eBay)
> that only works at full speed and doesn't stay connected all the time.

Sounds like some crazy broken hardware :)

> This wouldn't be a problem except that the kernel can get stuck in a
> loop with the "URB transfer length is wrong, xHC issue?" error.
> 
> The message "hub_ext_port_status failed (err = -71)" in particular can
> occur 70+ times for every instance of this error. There's no guarantee
> that it will ever disconnect the device and stop doing this, it has
> previously suddenly started generating 30MB/minute of log messages for
> 30 minutes until I unplugged the device.
> 
> Would it be ok to rate limit both of these messages to reduce the impact
> of poor connections?

Is it causing any problems?  Your system logger should be able to
rate-limit, or merge, these messages, right?

You really have broken hardware, why should the kernel work around that?
:)

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: [PATCHv11 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

Hi Greg,

On Mon, Nov 21, 2016 at 03:46:08PM +0100, Greg KH wrote:
> > > > +
> > > > +config TYPEC
> > > > +   tristate
> > > 
> > > Hah, that says NOTHING about what this code is at all.
> > 
> > Alone the class driver does nothing. Why would the user need to be
> > aware of if it when selecting the Type-C drivers, and what can the
> > user use that information for?
> 
> If you see a blank Kconfig option, what are you supposed to do with it?
> How do you know if you need to enable it or not?  Are you just supposed
> to guess?

But you don't see anything when you are selecting the drivers and that
is the point. You now can't select this separately. There is now no
option for it.

Why should we bother the user with this? The user is most likely only
interested in the drivers and by selecting those the user will get the
interface. The drivers will need to have the dependency to the class
set correctly in any case.


Thanks,

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