[PATCH v2 1/1] usb: dwc3: fix Clear Stall EP command failure

[Lu Baolu]

Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
Stall EP command") sets ClearPendIN bit for all IN endpoints of
v2.60a+ cores. This causes ClearStall command fails on 2.60+ cores
operating in HighSpeed mode.

In page 539 of 2.60a specification:

"When issuing Clear Stall command for IN endpoints in SuperSpeed
mode, the software must set the "ClearPendIN" bit to '1' to
clear any pending IN transcations, so that the device does not
expect any ACK TP from the host for the data sent earlier."

It's obvious that we only need to apply this rule to those IN
endpoints that currently operating in SuperSpeed mode.

Fixes: 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear Stall EP 
command")
Cc:  # v4.7+
Signed-off-by: Lu Baolu 
---
 v1->v2:
 - check current instead of maximum speed
 - improve the commit message

 drivers/usb/dwc3/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 7a8d3d8..4fd9fda 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -348,7 +348,8 @@ static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep *dep)
 * IN transfers due to a mishandled error condition. Synopsys
 * STAR 9000614252.
 */
-   if (dep->direction && (dwc->revision >= DWC3_REVISION_260A))
+   if (dep->direction && (dwc->revision >= DWC3_REVISION_260A) &&
+   (dwc->gadget.speed >= USB_SPEED_SUPER))
cmd |= DWC3_DEPCMD_CLEARPENDIN;
 
memset(, 0, sizeof(params));
-- 
2.1.4

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

[PATCH v3 1/1] usb: xhci: fix return value of xhci_setup_device()

[Lu Baolu]

xhci_setup_device() should return failure with correct error number
when xhci host has died, removed or halted.

During usb device enumeration, if usb host is not accessible (died,
removed or halted), the hc_driver->address_device() should return
a corresponding error code to usb core. But current xhci driver just
returns success. This misleads usb core to continue the enumeration
by reading the device descriptor, which will result in failure, and
users will get a misleading message like "device descriptor read/8,
error -110".

Cc:  # v4.3+
Signed-off-by: Lu Baolu 
---
 v2->v3:
 - improve the commit description
 v1->v2:
 - fix email mismatch issue

 drivers/usb/host/xhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 01d96c9..3e66e73 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3785,8 +3785,10 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct 
usb_device *udev,
 
mutex_lock(>mutex);
 
-   if (xhci->xhc_state)/* dying, removing or halted */
+   if (xhci->xhc_state) {  /* dying, removing or halted */
+   ret = -ESHUTDOWN;
goto out;
+   }
 
if (!udev->slot_id) {
xhci_dbg_trace(xhci, trace_xhci_dbg_address,
-- 
2.1.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] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 03:59:19PM +0300, Grygorii Strashko wrote:
> On 09/08/2016 03:28 PM, Peter Chen wrote:
> > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> >> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >>> Arnd Bergmann  writes:
>  On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> >> If we do that, we have to put child devices of the dwc3 devices into
> >> the platform glue, and it also breaks those dwc3 devices that don't
> >> have a parent driver.
> >
> > Well, this is easy to fix:
> >
> > if (dwc->dev->parent) {
> > dwc->sysdev = dwc->dev->parent;
> > } else {
> > dev_info(dwc->dev, "Please provide a glue layer!\n");
> > dwc->sysdev = dwc->dev;
> > }
> 
>  I don't understand. Do you mean we should have an extra level of
>  stacking and splitting "static struct platform_driver dwc3_driver"
>  in two so instead of
> 
>    "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> 
>  we do this?
> 
>    "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> 
>  "xhci" (usb_bus.dev)
> >>>
> >>> no 
> >>>
> >>> If we have a parent device, use that as sysdev, otherwise use self as
> >>> sysdev.
> >>
> >> But there is often a parent device in DT, as the xhci device is
> >> attached to some internal bus that gets turned into a platform_device
> >> as well, so checking whether there is a parent will get the wrong
> >> device node.
> > 
> > From my point, all platform and firmware information at dwc3 are
> > correct, so we don't need to change dwc3/core.c, only changing for
> > xhci-plat.c is ok.
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index ed56bf9..fd57c0d 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > struct clk  *clk;
> > int ret;
> > int irq;
> > +   struct device *dev = >dev, *sysdev;
> >  
> > if (usb_disabled())
> > return -ENODEV;
> > @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device 
> > *pdev)
> > if (irq < 0)
> > return -ENODEV;
> >  
> > +   if (dev->parent) {
> > +   sysdev = dev->parent;
> > +   } else {
> > +   sysdev = dev;
> > +   }
> > +
> 
> Shouldn't we be more careful with that?
> 

Above code does not consider pci device case, Arnd's patch covers
all cases.

> armada-375.dtsi
> 
>   soc {
>   compatible = "marvell,armada375-mbus", "simple-bus";
> 
>   internal-regs {
>   compatible = "simple-bus";
> 
>   usb3@58000 {
>   compatible = "marvell,armada-375-xhci";
>   reg = <0x58000 0x2>,<0x5b880 0x80>;
>   interrupts = ;
>   clocks = < 16>;
>   phys = < PHY_TYPE_USB3>;
>   phy-names = "usb";
>   status = "disabled";
>   };
> 
> 
> What will be the parent dev in above case?
> 

In this case, no parent dev for above case, it will use itself as sysdev
since it has of_node at dts.

-- 

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 02:52:29PM +0200, Arnd Bergmann wrote:
> On Thursday, September 8, 2016 8:28:10 PM CEST Peter Chen wrote:
> > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> > > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > > > Arnd Bergmann  writes:
> > > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > > > If we have a parent device, use that as sysdev, otherwise use self as
> > > > sysdev.
> > > 
> > > But there is often a parent device in DT, as the xhci device is
> > > attached to some internal bus that gets turned into a platform_device
> > > as well, so checking whether there is a parent will get the wrong
> > > device node.
> > 
> > From my point, all platform and firmware information at dwc3 are
> > correct, so we don't need to change dwc3/core.c, only changing for
> > xhci-plat.c is ok.
> 
> Ok, thanks. That leaves the PCI glue, right?

If pci's firmware information can only get from dwc3-pci, I was wrong.
I am almost sure your patch covers all 3 cases. dwc3->sysdev covers
dwc3 core and gadget side, hcd->self.sysdev cover host side. The only
possible improvement may be how to detect pci device.

> 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index d2e3f65..563600b 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
> > /* Did the HC die before the root hub was registered? */
> > if (HCD_DEAD(hcd))
> > usb_hc_died (hcd);  /* This time clean up */
> > -   usb_dev->dev.of_node = parent_dev->of_node;
> > +   usb_dev->dev.of_node = parent_dev->sysdev->of_node;
> > }
> > mutex_unlock(_bus_idr_lock);
> > 
> > At above changes, the root hub's of_node equals to xhci-hcd sysdev's
> > of_node, which is from firmware or from its parent (it is dwc3 core
> > device).
> 
> Just to make sure I understand you right:
> 
> in the qcom,dwc3 -> dwc3 -> xhci hierarchy, this would be the
> dwc3 device, not the qcom,dwc3 device.
> 

Yes, since there is a DT node for dwc3, and firmware information is there,
that's why the original patch (Grygorii Strashko's) can work.

-- 

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

Re: [PATCH v4 00/22] Support qcom's HSIC USB and rewrite USB2 HS support

[Peter Chen]

On Thu, Sep 08, 2016 at 02:13:28PM -0700, Stephen Boyd wrote:
> Quoting Peter Chen (2016-09-07 19:06:45)
> > On Thu, Sep 8, 2016 at 5:34 AM, Stephen Boyd  
> > wrote:
> > > The state of USB ChipIdea support on Qualcomm's platforms is not great.
> > > The DT description of these devices requires up to three different nodes
> > > for what amounts to be the same hardware block, when there should really
> > > only be one. Furthermore, the "phy" driver that is in mainline 
> > > (phy-msm-usb.c)
> > > duplicates the OTG state machine and touches the ci controller wrapper
> > > registers when it should really be focused on the phy and the ULPI 
> > > accesses
> > > needed to get the phy working. There's also a slimmed down phy driver for
> > > the msm8916 platform, but really the phy hardware is the same as other 
> > > MSMs,
> > > so we have two drivers doing pretty much the same thing. This leads to a
> > > situtaion where we have the chipidea core driver, the "phy" driver, and
> > > sometimes the ehci-msm.c driver operating the same device all at the same
> > > time with very little coordination. This just isn't very safe and is
> > > confusing from a driver perspective when trying to figure out who does 
> > > what.
> > > Finally, there isn't any HSIC support on platforms like apq8074 so we
> > > should add that.
> > >
> > > This patch series updates the ChipIdea driver and the MSM wrapper
> > > (ci_hdrc_msm.c) to properly handle the PHY and wrapper bits at the right
> > > times in the right places. To get there, we update the ChipIdea core to
> > > have support for the ULPI phy bus introduced by Heikki. Along the way
> > > we fix bugs with the extcon handling for peripheral and OTG mode 
> > > controllers
> > > and move the parts of phy-usb-msm.c that are touching the CI controller
> > > wrapper into the wrapper driver (ci_hdrc_msm.c). Finally we add support
> > > for the HSIC phy based on the ULPI bus and rewrite the HS phy driver
> > > (phy-usb-msm.c) as a standard ULPI phy driver.
> > >
> > > Once this series is accepted, we should be able to delete the 
> > > phy-usb-msm.c,
> > > phy-qcom-8x16-usb.c, and ehci-msm.c drivers from the tree and use the ULPI
> > > based phy driver (which also lives in drivers/phy/ instead of 
> > > drivers/usb/phy/)
> > > and the chipidea host core instead.
> > >
> > > I've also sent separate patches for other minor pieces to make this
> > > all work. The full tree can be found here[3], hacks and all to get
> > > things working. I've tested this on the db410c, apq8074 dragonboard,
> > > and ifc6410 with configfs gadgets and otg cables.
> > >
> > > Patches based on v4.8-rc1
> > >
> > > Changes from v3:
> > >
> > >  * Picked up Acks from Peter
> > >
> > >  * Updated extcon consolidation patch per Peter's comments
> > >
> > >  * Folded in simplification from Heikki for ULPI DT matching
> > >
> > 
> > I find the kbuild dependency error when compile your series, I remembered
> > someone met it before, would you have a solution:
> 
> Yes. Should I send it through the drm maintainers?
> 

Yes, please add me at cc list, thanks.

Peter
> ---8<---
> From: Stephen Boyd 
> Subject: [PATCH] gpu: Remove depends on RESET_CONTROLLER when not a provider
> 
> These GPU drivers only depend on the RESET_CONTROLLER config
> option to fix build issues that existed when there weren't stub
> reset APIs for reset controller consumers. Given that these
> drivers aren't providing any reset controllers themselves, they
> don't actually depend on the API to build (just to function) so
> they don't need to depend on it. Remove the dependency to fix
> recursive build errors like the following:
> 
> drivers/usb/Kconfig:39:error: recursive dependency detected!
> drivers/usb/Kconfig:39: symbol USB is selected by MOUSE_APPLETOUCH
> drivers/input/mouse/Kconfig:187:symbol MOUSE_APPLETOUCH depends on 
> INPUT
> drivers/input/Kconfig:8:symbol INPUT is selected by VT
> drivers/tty/Kconfig:12: symbol VT is selected by FB_STI
> drivers/video/fbdev/Kconfig:674:symbol FB_STI depends on FB
> drivers/video/fbdev/Kconfig:5:  symbol FB is selected by DRM_KMS_FB_HELPER
> drivers/gpu/drm/Kconfig:42: symbol DRM_KMS_FB_HELPER is selected by 
> DRM_KMS_CMA_HELPER
> drivers/gpu/drm/Kconfig:98: symbol DRM_KMS_CMA_HELPER is selected by 
> DRM_IMX
> drivers/gpu/drm/imx/Kconfig:1:  symbol DRM_IMX depends on IMX_IPUV3_CORE
> drivers/gpu/ipu-v3/Kconfig:1:   symbol IMX_IPUV3_CORE depends on 
> RESET_CONTROLLER
> drivers/reset/Kconfig:4:symbol RESET_CONTROLLER is selected by 
> USB_CHIPIDEA
> drivers/usb/chipidea/Kconfig:1: symbol USB_CHIPIDEA depends on USB_EHCI_HCD
> drivers/usb/host/Kconfig:84:symbol USB_EHCI_HCD depends on USB
> 
> Cc: Arnd Bergmann 
> Cc: David Airlie 
> Cc: 
> Cc: Heiko Stuebner 
> Cc: Mark Yao 
> Cc: Philipp Zabel 

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[NeilBrown]

On Thu, Sep 08 2016, Baolin Wang wrote:

> On 8 September 2016 at 15:31, NeilBrown  wrote:
>> On Thu, Sep 08 2016, Baolin Wang wrote:
>>>
>>> Now the usb charger will not get charger type from 'extcon' subsystem,
>>> we get the charger type from 'power_supply' and calllback
>>> 'get_charger_type' for users.
>>
>> I understand this.  I think it is wrong because, in general, the
>> power_supply doesn't know what the charger_type is (it might know it is
>> USB, but most don't know which sort of USB).  The PHY knows that, not
>> the power_supply.
>
> I don't think so. Now many platforms will detect the charger type by
> PMIC hardware, and we can get the charger type by PMIC hardware
> register. Then power supply driver can access the PMIC register to get
> the charger type. Here USB charger just considers if the accessing the
> PMIC register to get charger type is implemented in power supply, it
> is optional depending on what your platform designed.
>

In practice, the USB PHY and the Power manager will often be in the same
IC (the PMIC) so the driver for one could look at the registers for the
other.
But there is no guarantee that the hardware works like that.  It is
best to create a generally design.
Conceptually, the PHY is separate from the power manager and a solution
which recognises that will be more universal.

If the power manager can always just look at that phy registers to know
what sort of charger is connected, why does you framework need to work
with charger types at all?

>>>
>>> Yes, but you must think about some special cases on some platforms.
>>> Users may need to change the current in some situations, thus we
>>> should export one API for users to change the current. (I think you
>>> misunderstand the current limit here, that is the current for power
>>> driver  to draw).
>>
>> Can you be specific about these "special cases" please?
>> I cannot think of any.
>
> Suppose the USB configuration requests 100mA, then we should set the
> USB charger current is 100mA by __usb_charger_set_cur_limit_by_type()
> funtion, then notify this to power driver.

ahh I had missed something there.  It's a while since I looked
closely at these patches.

Only this usage of usb_charger_set_cur_limit_by_type() is really
nonsensical.

If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then
the number negotiated with the USB configuration is not relevant and
should be ignored.  There is a guaranteed minimum which is at least the
maximum that *can* be negotiated.

It is only when the cable appears to be a SDP (standard downstream
port) that the usb-config negotiation is relevant.  That is because the
minimum guaranteed for SDP is only 100mA.

NeilBrown


signature.asc
Description: PGP signature

Re: unusual_devs addition for Kingston DT100G3/32GB 0951:1666

[Wolfgang Breyha]

On 08/09/16 17:47, David Laight wrote:
> Try writing with:
> dd if=768MB.test of=/dev/sdb oflag=direct bs=1M seek_bytes seek=$((n*512))
> for several small values of 'n' so that the writes are misaligned.
> 
> It may also be worth finding out what the USB transfers look like.

I was wrong when I wrote that the stick fails on 4.7.2 as well. I confused
it with the last 4.6.7 update for Fedora. And I found the difference. 4.7.2
uses max_sectors 2048 with this stick and this seems to work flawlessly.
4.6.x used 240. If I set max_sectors to 2048 on 4.6.x it works as well. So
at least my unusual_devs addition seems obsolete (at least for 4.7.x)

Most interestingly reading with n*512 skip=...
dd if=/dev/sdh iflag=skip_bytes,direct of=sdh2g bs=1M skip=1024 count=2048
status=progress
... for 512,1024,1536,2048 leads to hangs followed by
usb 2-4: reset SuperSpeed USB device number 4 using xhci_hcd
every 90-110MB. The read hangs for several seconds until the reset and then
the next chunk is read. This happens on my homesystem with Intel Z170
chipset (USB PCI ID 8086:a12f (rev 31)) and i5-6500 on kernel
4.7.2-201.fc24.x86_64.

sha1sum is correct afterwards for 512.
writing with seek=512,1024,1536 worked with usual speed without hangs.

Reading with offset 1536 large chunks of the file are broken. I've usbmon
dump available of both write and read if of interest.

Some broken regions:
0810 3A00-0810 3DFF
0810 7A00-081F 
1020 3A00-1020 3DFF
1020 7A00-102F 
1830 3A00-1830 3DFF
1830 7A00-183F 
2040 3A00-2040 3DFF
2040 7A00-

But these are read errors. If I read without skip and strip the first 1536
afterwards the file is identical again.

With kind regards,
Wolfgang
-- 
Wolfgang Breyha  | http://www.blafasel.at/
Vienna University Computer Center | Austria
--
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 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Martin Blumenstingl]

On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks  wrote:
> On 08/09/16 21:42, Kevin Hilman wrote:
>>
>> Ben Dooks  writes:
>>
>>> On 08/09/16 20:52, Martin Blumenstingl wrote:

 On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman 
 wrote:
>>
>> + phy = devm_phy_create(>dev, NULL, _meson_usb2_ops);
>> + if (IS_ERR(phy)) {
>> + dev_err(>dev, "failed to create PHY\n");
>> + return PTR_ERR(phy);
>> + }
>> +
>> + if (usb_reset_refcnt++ == 0) {
>> + ret = device_reset(>dev);
>> + if (ret) {
>> + dev_err(>dev, "Failed to reset USB PHY\n");
>> + return ret;
>> + }
>> + }
>
>
> The ref count + reset here looks like something that could/should be
> handled in a runtime PM callback.

 Unfortunately that doesn't work (as Jerome found out) because both
 PHYs are sharing the same reset line.
 So if the second PHY would call device_reset then it would also reset
 the first PHY!

 There's a comment above the declaration of usb_reset_refcnt which
 tries to explain this:
 "The PHYs are sharing a common reset line -> we are only allowed to
 reset once for all PHYs."
 Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0)
 {" line to make it easier to see?

>>>
>>> pm-runtime has refcounting in it. When one of the nodes turns on,
>>> the pm-runtime will call your driver to say there is a user when
>>> this first use turns up.
>>>
>>> If all the sub-phys turn off and drop their refcount then the driver
>>> is called to say there are no more users and you can go to sleep.
>>
>>
>> After a chat w/Martin on IRC, It turns out runtime PM wont help here.
>>
>> The reason is because there are physically two PHY devices[1].  Those 2
>> devices will be treated independely by runtime PM, and have separate
>> use-counting, which means doing what I proposed would cause a reset to
>> happen when either device was probed.
>>
>> So, I think it's OK as it is.
>
>
> Surely you can do pm_runtime_get/put on the phy's parent platform
> device and do it that way?
could you please be more specific with that (do you mean pdev->dev.parent)?
so we would use pm_runtime_{get_sync,put} with the parent, while we
would still define the runtime_resume in our driver.

I'd be happy if that works and we can remove that refcounting hack
--
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 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Ben Dooks]

On 08/09/16 21:42, Kevin Hilman wrote:

Ben Dooks  writes:


On 08/09/16 20:52, Martin Blumenstingl wrote:

On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman  wrote:

+ phy = devm_phy_create(>dev, NULL, _meson_usb2_ops);
+ if (IS_ERR(phy)) {
+ dev_err(>dev, "failed to create PHY\n");
+ return PTR_ERR(phy);
+ }
+
+ if (usb_reset_refcnt++ == 0) {
+ ret = device_reset(>dev);
+ if (ret) {
+ dev_err(>dev, "Failed to reset USB PHY\n");
+ return ret;
+ }
+ }


The ref count + reset here looks like something that could/should be
handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both
PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset
the first PHY!

There's a comment above the declaration of usb_reset_refcnt which
tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to
reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0)
{" line to make it easier to see?



pm-runtime has refcounting in it. When one of the nodes turns on,
the pm-runtime will call your driver to say there is a user when
this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver
is called to say there are no more users and you can go to sleep.


After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1].  Those 2
devices will be treated independely by runtime PM, and have separate
use-counting, which means doing what I proposed would cause a reset to
happen when either device was probed.

So, I think it's OK as it is.


Surely you can do pm_runtime_get/put on the phy's parent platform
device and do it that way?

--
Ben Dooks   http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
--
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: TCPCI driver issue

[Steve Schefter]

Hi Guenter.


I would be interested in seeing the test driver, either on the list or
privately if you are not comfortable with a general release.


Please try to clone
https://chromium.googlesource.com/chromiumos/third_party/kernel and
fetch refs/sandbox/groeck/typec-4.4-cros-090816 (sha
a3fb79db5e901ce265176702c5769666ecb6c679). That should give you
everything I have. It should also fix the problem you have seen.


Thanks, I'll take a look.




I'm currently working on support for the Analogix AN7688.


Is that the HDMI to USB Type-C Bridge ? I would have thought this
would be used in a Type-C dock ?


It converts HDMI (native to the SoC) to DisplayPort rather than the 
other way around as you'd need in a dock.


Regards,
Steve

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

Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Kevin Hilman]

Ben Dooks  writes:

> On 08/09/16 20:52, Martin Blumenstingl wrote:
>> On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman  wrote:
 + phy = devm_phy_create(>dev, NULL, _meson_usb2_ops);
 + if (IS_ERR(phy)) {
 + dev_err(>dev, "failed to create PHY\n");
 + return PTR_ERR(phy);
 + }
 +
 + if (usb_reset_refcnt++ == 0) {
 + ret = device_reset(>dev);
 + if (ret) {
 + dev_err(>dev, "Failed to reset USB PHY\n");
 + return ret;
 + }
 + }
>>>
>>> The ref count + reset here looks like something that could/should be
>>> handled in a runtime PM callback.
>> Unfortunately that doesn't work (as Jerome found out) because both
>> PHYs are sharing the same reset line.
>> So if the second PHY would call device_reset then it would also reset
>> the first PHY!
>>
>> There's a comment above the declaration of usb_reset_refcnt which
>> tries to explain this:
>> "The PHYs are sharing a common reset line -> we are only allowed to
>> reset once for all PHYs."
>> Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0)
>> {" line to make it easier to see?
>>
>
> pm-runtime has refcounting in it. When one of the nodes turns on,
> the pm-runtime will call your driver to say there is a user when
> this first use turns up.
>
> If all the sub-phys turn off and drop their refcount then the driver
> is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1].  Those 2
devices will be treated independely by runtime PM, and have separate
use-counting, which means doing what I proposed would cause a reset to
happen when either device was probed.

So, I think it's OK as it is.

Kevin

[1] from the DT patch:

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi 
b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 2e8a3d9..02dfc54 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -151,6 +151,34 @@
#size-cells = <2>;
ranges;
 
+   usb-phys@c000 {
+   compatible = "simple-bus";
+   reg = <0x0 0xc000 0x0 0x40>;
+   #address-cells = <2>;
+   #size-cells = <2>;
+   ranges = <0x0 0x0 0x0 0xc000 0x0 0x40>;
+
+   usb0_phy: usb_phy@0 {
+   compatible = "amlogic,meson-gxbb-usb2-phy";
+   #phy-cells = <0>;
+   reg = <0x0 0x0 0x0 0x20>;
+   resets = < 34>;
+   clocks = < CLKID_USB  CLKID_USB0>;
+   clock-names = "usb_general", "usb";
+   status = "disabled";
+   };
+
+   usb1_phy: usb_phy@20 {
+   compatible = "amlogic,meson-gxbb-usb2-phy";
+   #phy-cells = <0>;
+   reg = <0x0 0x20 0x0 0x20>;
+   resets = < 34>;
+   clocks = < CLKID_USB  CLKID_USB1>;
+   clock-names = "usb_general", "usb";
+   status = "disabled";
+   };
+   };
+
--
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 1/3] usb: gadgetfs: introduce feature control mechanism

[Alan Stern]

On Thu, 8 Sep 2016, Binyamin Sharet (bsharet) wrote:

> 
> > On 8 Sep 2016, at 22:20, Alan Stern  wrote:
> > 
> > On Thu, 8 Sep 2016, Binyamin Sharet (bsharet) wrote:
> > 
> >>> I was thinking more like:
> >>> 
> >>> struct usb_gadgetfs_ioctl_arg {
> >>>   uint16_t length;
> >>>   uint8_t reserved[2];
> >>> 
> >>>   uint8_t data[0];
> >>> }
> >>> 
> >>> but the principle is pretty much the same.
> >>> 
> >>> Alan Stern
> >>> 
> >> 
> >> Won’t the user lose the relevant information (e.g. feature
> >> structure) by using this model?
> > 
> > What feature structure?  Aren't your feature lists just vectors of 64 
> > bits?  They can be stored in the .data field above.
> > 
> > Alan Stern
> > 
> 
> Not “just” - they are platform-dependant uint64_t. which means they
> won’t look the same on systems with different endianness. If the
> user is unaware of this, it can cause confusion w/r/t which bit is which.
> 
> We can use 8-bit vectors instead and skip the endianness issue,
> but why define a generic usb_gadgetfs_ioctl_args structure instead
> of “features struct” for feature-related ioctls and different structs for
> other types of ioctl (if we’ll have such in the future)?

Good point.  Yes, you can have different interfaces for different 
ioctls.

This whole question arose because I complained that the features API
looked too extravagant, and Felipe said that he didn't want to run out
of available fields in case any features in the future need them.

But if you're worried about that, why limit yourself to 64 features and 
24 bytes of extra data?  It doesn't make sense to think that some fixed 
limit might end up being too small, but then restrict yourself to a 
different fixed limit.

Alan Stern

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

Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Ben Dooks]

On 08/09/16 20:52, Martin Blumenstingl wrote:

On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman  wrote:

+ phy = devm_phy_create(>dev, NULL, _meson_usb2_ops);
+ if (IS_ERR(phy)) {
+ dev_err(>dev, "failed to create PHY\n");
+ return PTR_ERR(phy);
+ }
+
+ if (usb_reset_refcnt++ == 0) {
+ ret = device_reset(>dev);
+ if (ret) {
+ dev_err(>dev, "Failed to reset USB PHY\n");
+ return ret;
+ }
+ }


The ref count + reset here looks like something that could/should be
handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both
PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset
the first PHY!

There's a comment above the declaration of usb_reset_refcnt which
tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to
reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0)
{" line to make it easier to see?



pm-runtime has refcounting in it. When one of the nodes turns on,
the pm-runtime will call your driver to say there is a user when
this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver
is called to say there are no more users and you can go to sleep.

So, in phy_meson_usb2_power_on() you could do:

pm_runtime_get_sync(pdev);

and in phy_meson_usb2_power_off

pm_runtime_put(pdev);

https://www.kernel.org/doc/Documentation/power/runtime_pm.txt

--
Ben Dooks   http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
--
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: gadget: remove variable ret and remove unnecessary if statement

[Colin King]

From: Colin Ian King 

the if statement in lb_modinit is unnecessary so we can totally
remove the variable ret and just return the return value from
the call to usb_function_register.

Signed-off-by: Colin Ian King 
---
 drivers/usb/gadget/function/f_loopback.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/f_loopback.c 
b/drivers/usb/gadget/function/f_loopback.c
index ddb8c89..e700938 100644
--- a/drivers/usb/gadget/function/f_loopback.c
+++ b/drivers/usb/gadget/function/f_loopback.c
@@ -591,13 +591,9 @@ DECLARE_USB_FUNCTION(Loopback, loopback_alloc_instance, 
loopback_alloc);
 
 int __init lb_modinit(void)
 {
-   int ret;
-
-   ret = usb_function_register(_func);
-   if (ret)
-   return ret;
-   return ret;
+   return usb_function_register(_func);
 }
+
 void __exit lb_modexit(void)
 {
usb_function_unregister(_func);
-- 
2.9.3

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

Re: TCPCI driver issue

[Guenter Roeck]

Hi Steve,

On Thu, Sep 8, 2016 at 12:42 PM, Steve Schefter  wrote:
> Hi Guenter.
>
>>> I would be interested in seeing the test driver, either on the list or
>>> privately if you are not comfortable with a general release.
>>>
>> Please try to clone
>> https://chromium.googlesource.com/chromiumos/third_party/kernel and
>> fetch refs/sandbox/groeck/typec-4.4-cros-090816 (sha
>> a3fb79db5e901ce265176702c5769666ecb6c679). That should give you
>> everything I have. It should also fix the problem you have seen.
>
>
> Thanks, I'll take a look.
>
>>
>>> I'm currently working on support for the Analogix AN7688.
>>
>>
>> Is that the HDMI to USB Type-C Bridge ? I would have thought this
>> would be used in a Type-C dock ?
>
>
> It converts HDMI (native to the SoC) to DisplayPort rather than the other
> way around as you'd need in a dock.
>

Sounds like you'll need DisplayPort support in tcpm ... which isn't
implemented yet since I did not have a good test case (nor the time to
really think about it). We'll need a clean API between the DP chip
driver and tcpm to control if and when DP Alternate Mode should be
selected.

Thanks,
Guenter
--
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: [PATCHv8 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Thu, Sep 01, 2016 at 02:49:47PM +0300, Heikki Krogerus wrote:
> 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 
> ---
... 

> +
> +static int sysfs_strmatch(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))

This doesn't work ... sysfs_streq() returns true if there is a match,
so the "!" is wrong.

> + return index;
> + }
> +
> + return -EINVAL;
> +}
> +
[ ... ]
> +
> +static ssize_t
> +preferred_role_store(struct device *dev, struct device_attribute *attr,
> +  const char *buf, size_t size)
> +{
> + struct typec_port *port = to_typec_port(dev);
> + enum typec_role role;
> + int ret;
> +
> + if (port->cap->type != TYPEC_PORT_DRP) {
> + dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (!port->cap->try_role) {
> + dev_dbg(dev, "Setting preferred role not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + ret = sysfs_strmatch(typec_roles, ARRAY_SIZE(typec_roles), buf);
> + if (ret < 0) {
> + port->prefer_role = -1;
> + return size;

Are you sure about that ? It is kind of unusual to accept "bad" strings.
Why not return -EINVAL ?

> + }
> +
> + role = ret;
> +
> + ret = port->cap->try_role(port->cap, role);
> + if (ret)
> + return ret;
> +
> + port->prefer_role = role;
> + return size;
> +}
> +
[ ... ]
> +
> +static ssize_t supported_accessory_modes_show(struct device *dev,
> +   struct device_attribute *attr,
> +   char *buf)
> +{
> + struct typec_port *port = to_typec_port(dev);
> + ssize_t ret = 0;
> + int i;
> +
> + if (!port->cap->accessory[0])

You probably want 
if (!port->cap->accessory)
here. Otherwise the check is quite pointless (and crashes if the pointer
is NULL).

> + return 0;
> +
> + for (i = 0; port->cap->accessory[i]; i++)
> + ret += sprintf(buf + ret, "%s\n",
> +typec_accessory_modes[port->cap->accessory[i]]);
> + return ret;
> +}
> +static DEVICE_ATTR_RO(supported_accessory_modes);
> +

Guenter
--
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: TCPCI driver issue

[Guenter Roeck]

On Thu, Sep 8, 2016 at 12:35 PM, Guenter Roeck  wrote:
> On Thu, Sep 8, 2016 at 12:21 PM, Guenter Roeck  wrote:
>> Hi Steve,
>>
>> On Thu, Sep 8, 2016 at 9:32 AM, Steve Schefter  wrote:
>>> Hi Guenter.
>>>
 I think (hope) I did mention that the tcpci patch was compile tested
 only. Apologies if not. I'll try to get to it today and send a fix,
 though it will obviously only be a workaround (config data is platform
 specific). I also hope I can send a Reviewed-by: for Heikki's series
 today. Sorry for being late on that one.
>>>
>>>
>>> I did see in the patch header that it is a WIP, but didn't realize it was
>>> compile tested only.  I understand I'm on the bleeding edge here.
>>>
> But we probable need to talk about how to extract the platform
> specific details needed for the config in any case (and also what
> exactly is needed) at one point. I think we can get all of them with
> device properties regardless of the system (ACPI/DT/whatever), so
> we would just need to agree on the what the properties are.
>
 Yes, we'll definitely need that. I've used platform data and DMI
 internally for x86, but that seems so out of date. I'll also see if it
 makes sense to attach the test driver I used to test the tcpm code.
 Note that we also have a driver for fusb302 as client of tcpm in the
 works, plus an extcon based driver connected to the EC on chromebooks
 (and ties directly into the Type-C infrastructure).
>>>
>>>
>>> I would be interested in seeing the test driver, either on the list or
>>> privately if you are not comfortable with a general release.
>>>
>> Please try to clone
>> https://chromium.googlesource.com/chromiumos/third_party/kernel and
>> fetch refs/sandbox/groeck/typec-4.4-cros-090816 (sha
>> a3fb79db5e901ce265176702c5769666ecb6c679). That should give you
>> everything I have. It should also fix the problem you have seen.
>>
> Wait a minute; I messed it up. I'll try again.
>

Ok, it is there again. I had to fix a couple of bugs in the infra code.

Guenter

> Guenter
>
>>> I'm currently working on support for the Analogix AN7688.
>>
>> Is that the HDMI to USB Type-C Bridge ? I would have thought this
>> would be used in a Type-C dock ?
>>
>> Thanks,
>> Guenter
--
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 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Martin Blumenstingl]

On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman  wrote:
>> + phy = devm_phy_create(>dev, NULL, _meson_usb2_ops);
>> + if (IS_ERR(phy)) {
>> + dev_err(>dev, "failed to create PHY\n");
>> + return PTR_ERR(phy);
>> + }
>> +
>> + if (usb_reset_refcnt++ == 0) {
>> + ret = device_reset(>dev);
>> + if (ret) {
>> + dev_err(>dev, "Failed to reset USB PHY\n");
>> + return ret;
>> + }
>> + }
>
> The ref count + reset here looks like something that could/should be
> handled in a runtime PM callback.
Unfortunately that doesn't work (as Jerome found out) because both
PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset
the first PHY!

There's a comment above the declaration of usb_reset_refcnt which
tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to
reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0)
{" line to make it easier to see?


Regards,
Martin
--
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 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Ben Dooks]

On 08/09/16 20:35, Kevin Hilman wrote:

Martin Blumenstingl  writes:


This is a new driver for the USB PHY found in Meson8b and GXBB SoCs.

Signed-off-by: Martin Blumenstingl 
Signed-off-by: Jerome Brunet 


I tested this on meson-gxbb-p200 with USB host and a mass storage
device.

Tested-by: Kevin Hilman 

A minor question/comment below, for you and for Kishon...

[...]


+static int phy_meson_usb2_probe(struct platform_device *pdev)
+{
+   struct phy_meson_usb2_priv *priv;
+   struct resource *res;
+   struct phy *phy;
+   struct phy_provider *phy_provider;
+   int ret;
+
+   priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv)
+   return -ENOMEM;
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   priv->regs = devm_ioremap_resource(>dev, res);
+   if (IS_ERR(priv->regs))
+   return PTR_ERR(priv->regs);
+
+   priv->clk_usb_general = devm_clk_get(>dev, "usb_general");
+   if (IS_ERR(priv->clk_usb_general))
+   return PTR_ERR(priv->clk_usb_general);
+
+   priv->clk_usb = devm_clk_get(>dev, "usb");
+   if (IS_ERR(priv->clk_usb))
+   return PTR_ERR(priv->clk_usb);
+
+   priv->dr_mode = of_usb_get_dr_mode_by_phy(pdev->dev.of_node, -1);
+   if (priv->dr_mode == USB_DR_MODE_UNKNOWN) {
+   dev_err(>dev,
+   "missing dual role configuration of the controller\n");
+   return -EINVAL;
+   }
+
+   phy = devm_phy_create(>dev, NULL, _meson_usb2_ops);
+   if (IS_ERR(phy)) {
+   dev_err(>dev, "failed to create PHY\n");
+   return PTR_ERR(phy);
+   }
+
+   if (usb_reset_refcnt++ == 0) {
+   ret = device_reset(>dev);
+   if (ret) {
+   dev_err(>dev, "Failed to reset USB PHY\n");
+   return ret;
+   }
+   }


The ref count + reset here looks like something that could/should be
handled in a runtime PM callback.

IOW, there should be a pm_runtime_get_sync() here, and in the
->runtime_resume() callback, the device_reset() would be called.
Runtime PM does the refcounting, and only calls ->runtime_resume() on
the 0 -> 1 transition.

This isn't a big deal for now, so I'll let Kishon decide, but using
runtime PM from the start will help enabling other PM features later.


I agree, pm_runtime would probably be the best place to handle >1
device with shared items such as reset.

The version I wrote, I simply enabled the clocks and reset the device
when probed to work around the shared reset.


--
Ben Dooks   http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
--
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 1/3] usb: gadgetfs: introduce feature control mechanism

[Binyamin Sharet (bsharet)]

> On 8 Sep 2016, at 22:20, Alan Stern  wrote:
> 
> On Thu, 8 Sep 2016, Binyamin Sharet (bsharet) wrote:
> 
>>> I was thinking more like:
>>> 
>>> struct usb_gadgetfs_ioctl_arg {
>>> uint16_t length;
>>> uint8_t reserved[2];
>>> 
>>> uint8_t data[0];
>>> }
>>> 
>>> but the principle is pretty much the same.
>>> 
>>> Alan Stern
>>> 
>> 
>> Won’t the user lose the relevant information (e.g. feature
>> structure) by using this model?
> 
> What feature structure?  Aren't your feature lists just vectors of 64 
> bits?  They can be stored in the .data field above.
> 
> Alan Stern
> 

Not “just” - they are platform-dependant uint64_t. which means they
won’t look the same on systems with different endianness. If the
user is unaware of this, it can cause confusion w/r/t which bit is which.

We can use 8-bit vectors instead and skip the endianness issue,
but why define a generic usb_gadgetfs_ioctl_args structure instead
of “features struct” for feature-related ioctls and different structs for
other types of ioctl (if we’ll have such in the future)?

Binyamin Sharet
Cisco, STARE-C
N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

Re: TCPCI driver issue

[Guenter Roeck]

On Thu, Sep 8, 2016 at 12:21 PM, Guenter Roeck  wrote:
> Hi Steve,
>
> On Thu, Sep 8, 2016 at 9:32 AM, Steve Schefter  wrote:
>> Hi Guenter.
>>
>>> I think (hope) I did mention that the tcpci patch was compile tested
>>> only. Apologies if not. I'll try to get to it today and send a fix,
>>> though it will obviously only be a workaround (config data is platform
>>> specific). I also hope I can send a Reviewed-by: for Heikki's series
>>> today. Sorry for being late on that one.
>>
>>
>> I did see in the patch header that it is a WIP, but didn't realize it was
>> compile tested only.  I understand I'm on the bleeding edge here.
>>
 But we probable need to talk about how to extract the platform
 specific details needed for the config in any case (and also what
 exactly is needed) at one point. I think we can get all of them with
 device properties regardless of the system (ACPI/DT/whatever), so
 we would just need to agree on the what the properties are.

>>> Yes, we'll definitely need that. I've used platform data and DMI
>>> internally for x86, but that seems so out of date. I'll also see if it
>>> makes sense to attach the test driver I used to test the tcpm code.
>>> Note that we also have a driver for fusb302 as client of tcpm in the
>>> works, plus an extcon based driver connected to the EC on chromebooks
>>> (and ties directly into the Type-C infrastructure).
>>
>>
>> I would be interested in seeing the test driver, either on the list or
>> privately if you are not comfortable with a general release.
>>
> Please try to clone
> https://chromium.googlesource.com/chromiumos/third_party/kernel and
> fetch refs/sandbox/groeck/typec-4.4-cros-090816 (sha
> a3fb79db5e901ce265176702c5769666ecb6c679). That should give you
> everything I have. It should also fix the problem you have seen.
>
Wait a minute; I messed it up. I'll try again.

Guenter

>> I'm currently working on support for the Analogix AN7688.
>
> Is that the HDMI to USB Type-C Bridge ? I would have thought this
> would be used in a Type-C dock ?
>
> Thanks,
> Guenter
--
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 1/7] clk: gxbb: expose USB clocks

[Kevin Hilman]

Stephen Boyd  writes:

> On 09/04, Martin Blumenstingl wrote:
>> USB0_DDR_BRIDGE and USB1_DDR_BRIDGE1 are needed for the related
>> dwc2 usb controller. USB, USB0 and USB1 are needed for the PHYs.
>> Expose these clocks to DT and comment out in clk driver.
>> 
>> Signed-off-by: Jerome Brunet 
>> Signed-off-by: Martin Blumenstingl 
>> ---
>
> Assuming authorship is resolved:
>
> Acked-by: Stephen Boyd 

After some clarification from Jerome (Martin is the primary author),
I've applied this, with the SoB order swapped.

Kevin
--
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: TCPCI driver issue

[Guenter Roeck]

Hi Steve,

On Thu, Sep 8, 2016 at 9:32 AM, Steve Schefter  wrote:
> Hi Guenter.
>
>> I think (hope) I did mention that the tcpci patch was compile tested
>> only. Apologies if not. I'll try to get to it today and send a fix,
>> though it will obviously only be a workaround (config data is platform
>> specific). I also hope I can send a Reviewed-by: for Heikki's series
>> today. Sorry for being late on that one.
>
>
> I did see in the patch header that it is a WIP, but didn't realize it was
> compile tested only.  I understand I'm on the bleeding edge here.
>
>>> But we probable need to talk about how to extract the platform
>>> specific details needed for the config in any case (and also what
>>> exactly is needed) at one point. I think we can get all of them with
>>> device properties regardless of the system (ACPI/DT/whatever), so
>>> we would just need to agree on the what the properties are.
>>>
>> Yes, we'll definitely need that. I've used platform data and DMI
>> internally for x86, but that seems so out of date. I'll also see if it
>> makes sense to attach the test driver I used to test the tcpm code.
>> Note that we also have a driver for fusb302 as client of tcpm in the
>> works, plus an extcon based driver connected to the EC on chromebooks
>> (and ties directly into the Type-C infrastructure).
>
>
> I would be interested in seeing the test driver, either on the list or
> privately if you are not comfortable with a general release.
>
Please try to clone
https://chromium.googlesource.com/chromiumos/third_party/kernel and
fetch refs/sandbox/groeck/typec-4.4-cros-090816 (sha
a3fb79db5e901ce265176702c5769666ecb6c679). That should give you
everything I have. It should also fix the problem you have seen.

> I'm currently working on support for the Analogix AN7688.

Is that the HDMI to USB Type-C Bridge ? I would have thought this
would be used in a Type-C dock ?

Thanks,
Guenter
--
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 1/3] usb: gadgetfs: introduce feature control mechanism

[Alan Stern]

On Thu, 8 Sep 2016, Binyamin Sharet (bsharet) wrote:

> > I was thinking more like:
> > 
> > struct usb_gadgetfs_ioctl_arg {
> > uint16_t length;
> > uint8_t reserved[2];
> > 
> > uint8_t data[0];
> > }
> > 
> > but the principle is pretty much the same.
> > 
> > Alan Stern
> > 
> 
> Won’t the user lose the relevant information (e.g. feature
> structure) by using this model?

What feature structure?  Aren't your feature lists just vectors of 64 
bits?  They can be stored in the .data field above.

Alan Stern

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

Re: [PATCH 1/3] usb: gadgetfs: introduce feature control mechanism

[Binyamin Sharet (bsharet)]

> On 8 Sep 2016, at 17:34, Alan Stern  wrote:
> 
> On Thu, 8 Sep 2016, Felipe Balbi wrote:
> 
>>> +struct usb_gadgetfs_features {
>>> + uint64_t bitmap[4];
>> 
>> this should really be:
>> 
>>  uint64_t features0;
>> 
>>/* Keep 24 bytes reserved for possible future usage */
>>uint8_t RESERVED[24];
> 
> Isn't this really extreme overkill?  I would be surprised if gadgetfs 
> ever supports more than 10 features.
 
 I'm hoping to be extreme overkill :-) I really don't wanna get into a
 situation where we run out of bits. What if some features require
 arguments in the future? We could just pass them here, right?
 
 Frankly, I'm afraid of what happened to i2c-dev :-) We can't change
 (easily) i2c subsystem's write() to return the amount of bytes written
 because its userspace facing ABI has no reserved bits we could use to
 return that value. Changing that would be a huge amount of work.
 
 What do you think?
>>> 
>>> Well, I don't care about it all that much, especially since I don't 
>>> think you'll ever need to use it.  :-)
>>> 
>>> Still, there are approaches that have been used in the past when people
>>> wanted to create an extensible interface.  Passing a variable-length
>>> structure that has a length field at a reserved spot near the beginning
>>> (perhaps along with one or two other reserved entries), for example.  
>> 
>> something along the like below?
>> 
>> 
>> /* better names appreciated */
>> struct usb_gadgetfs_packet {
>>  uint16_t length;
>>uint8_t reserved[2];
>> 
>>uint8_t *data; /* dynamically allocated by userspace */
>> }
>> 
>> I can work with that no problem.
> 
> I was thinking more like:
> 
> struct usb_gadgetfs_ioctl_arg {
>   uint16_t length;
>   uint8_t reserved[2];
> 
>   uint8_t data[0];
> }
> 
> but the principle is pretty much the same.
> 
> Alan Stern
> 

Won’t the user lose the relevant information (e.g. feature structure) by using 
this model?

Binyamin Sharet
Cisco, STARE-C

Re: [PATCH 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform

[John Youn]

On 9/8/2016 7:27 AM, Randy Li wrote:
> On the rk3288 USB host-only port (the one that's not the OTG-enabled
> port) the PHY can get into a bad state when a wakeup is asserted (not
> just a wakeup from full system suspend but also a wakeup from
> autosuspend).
> 
> We can get the PHY out of its bad state by asserting its "port reset",
> but unfortunately that seems to assert a reset onto the USB bus so it
> could confuse things if we don't actually deenumerate / reenumerate the
> device.
> 
> We can also get the PHY out of its bad state by fully resetting it using
> the reset from the CRU (clock reset unit) in chip, which does a more full
> reset.  The CRU-based reset appears to actually cause devices on the bus
> to be removed and reinserted, which fixes the problem (albeit in a hacky
> way).
> 
> It's unfortunate that we need to do a full re-enumeration of devices at
> wakeup time, but this is better than alternative of letting the bus get
> wedged.
> 
> Signed-off-by: Randy Li 
> ---
>  drivers/usb/dwc2/core_intr.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index d85c5c9..5b9b671 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct 
> dwc2_hsotg *hsotg)
>  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  {
>   int ret;
> + struct device_node *np = hsotg->dev->of_node;
>  
>   /* Clear interrupt */
>   dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct 
> dwc2_hsotg *hsotg)
>   /* Restart the Phy Clock */
>   pcgcctl &= ~PCGCTL_STOPPCLK;
>   dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
> +
> + /*
> +  * It is a quirk in Rockchip RK3288, causing by
> +  * a hardware bug. This will propagate out and
> +  * eventually we'll re-enumerate the device. 
> +  * Not great but the best we can do 

Hi Randy,

Please run ./scripts/checkpatch.pl on your patches and fix trailing
whitespace in above two lines.

Thanks,
John

--
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: TCPCI driver issue

[Steve Schefter]

Hi Guenter.


I think (hope) I did mention that the tcpci patch was compile tested
only. Apologies if not. I'll try to get to it today and send a fix,
though it will obviously only be a workaround (config data is platform
specific). I also hope I can send a Reviewed-by: for Heikki's series
today. Sorry for being late on that one.


I did see in the patch header that it is a WIP, but didn't realize it 
was compile tested only.  I understand I'm on the bleeding edge here.



But we probable need to talk about how to extract the platform
specific details needed for the config in any case (and also what
exactly is needed) at one point. I think we can get all of them with
device properties regardless of the system (ACPI/DT/whatever), so
we would just need to agree on the what the properties are.


Yes, we'll definitely need that. I've used platform data and DMI
internally for x86, but that seems so out of date. I'll also see if it
makes sense to attach the test driver I used to test the tcpm code.
Note that we also have a driver for fusb302 as client of tcpm in the
works, plus an extcon based driver connected to the EC on chromebooks
(and ties directly into the Type-C infrastructure).


I would be interested in seeing the test driver, either on the list or 
privately if you are not comfortable with a general release.


I'm currently working on support for the Analogix AN7688.

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

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

[Wim Osterholt]

On Thu, Sep 08, 2016 at 03:05:15PM +0200, Oliver Neukum wrote:
> > Sep  6 19:12:38 localhost kernel: Call Trace:
> > Sep  6 19:12:38 localhost kernel:  [] ? 0xc01f4347
> 
> Hi,
> 
> your stack trace is broken. Did you fail to install the System.map file?

Source is available under /usr/src/linux --> /usr/src/linux-4.8-rc5
System.map is available there, also as System.map-4.8-rc5.
System.map and System.map-4.8-rc5 is also available in /boot.
But the call trace still shows no symbols.
>From reading I understood that symbols from modules will not be available in
System.map. So what else  should I do?



[   46.133212] usb 4-1: new full-speed USB device number 2 using uhci_hcd
[   46.334136] usb 4-1: New USB device found, idVendor=0572, idProduct=1340
[   46.334140] usb 4-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[   46.334142] usb 4-1: Product: USB Modem
[   46.334145] usb 4-1: Manufacturer: Conexant
[   46.334147] usb 4-1: SerialNumber: 12345678
[   46.388110] BUG: unable to handle kernel NULL pointer dereference at 0246
[   46.391243] IP: [] 0xe12d0a35
[   46.391243] *pde =  
[   46.391243] Oops:  [#1] SMP
[   46.391243] 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 lirc_serial(C) lirc_dev(O) binfmt_misc 
svgalib_helper(O) snd_pcm_oss snd_mixer_oss usbhid usb_storage ipw2200 
snd_intel8x0 snd_ac97_codec libipw lib80211 ac97_bus cfg80211 snd_pcm snd_timer 
rfkill firmware_class snd via_rhine ppdev soundcore pcspkr uhci_hcd mii 
ehci_pci ehci_hcd usbcore floppy parport_pc lpc_ich usb_common fan parport 
acpi_cpufreq thermal mfd_core processor button
[   46.391243] CPU: 1 PID: 1868 Comm: udevd Tainted: G C O4.8.0-rc5 
#1
[   46.391243] Hardware name: MEDIONPC MS-7048/MS-7048, BIOS 6.00 PG 02/12/2004
[   46.391243] task: df6adb00 task.stack: dc74
[   46.391243] EIP: 0060:[] EFLAGS: 00010202 CPU: 1
[   46.391243] EAX:  EBX: dec6f000 ECX:  EDX: 0124
[   46.391243] ESI: dec6f27c EDI: dc7a5800 EBP: 0246 ESP: dc741ce8
[   46.391243]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[   46.391243] CR0: 80050033 CR2: 0246 CR3: 1c714000 CR4: 0690
[   46.391243] Stack:
[   46.391243]  df788000 df788000 df788400 dc7a5800 df4b5780 df4b57b8 0001 
dc7a5870
[   46.391243]  dc4409c0 df78801c 0040 0010  dec6f00c dec6f27c 

[   46.391243]  dc79e800  df6adb00 da4a5f30 c01f5243 0246 0246 
da4a5f30
[   46.391243] Call Trace:
[   46.391243]  [] ? 0xc01f5243
[   46.391243]  [] ? 0xc043c68a
[   46.391243]  [] ? 0xe09fc22f
[   46.391243]  [] ? 0xc03087a8
[   46.391243]  [] ? 0xc0308907
[   46.391243]  [] ? 0xc0307444
[   46.391243]  [] ? 0xc03083c3
[   46.391243]  [] ? 0xc03088b2
[   46.391243]  [] ? 0xc0308097
[   46.391243]  [] ? 0xc0308ebe
[   46.391243]  [] ? 0xe09fb525
[   46.391243]  [] ? 0xe12d30a9
[   46.391243]  [] ? 0xe12d3000
[   46.391243]  [] ? 0xc01003eb
[   46.391243]  [] ? 0xc043c68a
[   46.391243]  [] ? 0xc027d355
[   46.391243]  [] ? 0xc01af013
[   46.391243]  [] ? 0xc0184275
[   46.391243]  [] ? 0xc01842a4
[   46.391243]  [] ? 0xc017546b
[   46.391243]  [] ? 0xc01756c3
[   46.391243]  [] ? 0xc0100ed1
[   46.391243]  [] ? 0xc043dd25
[   46.391243] Code: 44 24 04 ba 20 1c 2d e1 89 58 74 83 c0 1c 89 44 24 24 e8 
35 4e 03 df 85 c0 0f 88 ed fe ff ff 8b 6c 24 54 85 ed 0f 84 91 00 00 00 <0f> b6 
45 00 ba c0 00 40 02 83 e8 04 e8 b4 e4 ed de 85 c0 89 83
[   46.391243] EIP: []  SS:ESP 0068:dc741ce8
[   46.391243] CR2: 0246
[   46.802809] ---[ end trace 3cd7f784cc67fa66 ]---
[   46.811156] udevd[884]: worker [1868] terminated by signal 9 (Killed)
[   46.811164] udevd[884]: worker [1868] failed while handling 
'/devices/pci:00/:00:1d.2/usb4/4-1/4-1:1.1'


Regards, Wim.
y
--
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 V5] leds: trigger: Introduce a USB port trigger

[Rafał Miłecki]

From: Rafał Miłecki 

This commit adds a new trigger responsible for turning on LED when USB
device gets connected to the selected USB port. This can can useful for
various home routers that have USB port(s) and a proper LED telling user
a device is connected.

The trigger gets its documentation file but basically it just requires
enabling it and selecting USB ports (e.g. echo 1 > ports/usb1-1).

There was a long discussion on design of this driver. Its current state
is a result of picking them most adjustable solution as others couldn't
handle all cases.

1) It wasn't possible for the driver to register separated trigger for
   each USB port. Some physical USB ports are handled by more than one
   controller and so by more than one USB port. E.g. USB 2.0 physical
   port may be handled by OHCI's port and EHCI's port.
   It's also not possible to assign more than 1 trigger to a single LED
   and implementing such feature would be tricky due to syncing triggers
   and sysfs conflicts with old triggers.

2) Another idea was to register trigger per USB hub. This wouldn't allow
   handling devices with multiple USB LEDs and controllers (hubs)
   controlling more than 1 physical port. It's common for hubs to have
   few ports and each may have its own LED.

This final trigger is highly flexible. It allows selecting any USB ports
for any LED. It was also modified (compared to the initial version) to
allow choosing ports rather than having user /guess/ proper names. It
was successfully tested on SmartRG SR400ac which has 3 USB LEDs,
2 physical ports and 3 controllers.

Another planned feature is support for LED reacting to the USB activity.
This can be implemented with another sysfs file for setting mode. The
default mode wouldn't change so there won't be ABI breakage and such
feature can be safely implemented later.

There was also an idea of supporting other devices (PCI, SDIO, etc.) but
as this driver already contains some USB specific code (and will get
more) these should be probably separated drivers (triggers).

Signed-off-by: Rafał Miłecki 
---
V2: Trying to add DT support, idea postponed as it will take more time
to discuss the bindings.
V3: Fix typos in commit and Documentation (thanks Jacek!)
Use "ports" sysfs file for adding and removing USB ports (thx Jacek)
Check if there is USB device connected after adding new USB port
Fix memory leak or two
V3.5: Fix e-mail address (thanks Matthias)
  Simplify conditions in usbport_trig_notify (thx Matthias)
  Make "ports" a subdirectory with file per port, to match one value
  per file sysfs rule (thanks Greg)
  As "ports" couldn't be used for adding and removing ports anymore,
  there are now "new_port" and "remove_port". Having them makes this
  API also common with e.g. pci and usb buses.
V4: Add Documentation/ABI/testing/sysfs-class-led-trigger-usbport and
reference it in Documentation/leds/ledtrig-usbport.txt to avoid doc
duplication.
V5: Update commit message to explain driver design & Documentation
Avoid specifying ports manually and dummy files (chmod )
Don't use kobject_create_and_add but a struct attribute_group
Fix typo, improve comments, update Date
---
 .../ABI/testing/sysfs-class-led-trigger-usbport|  11 +
 Documentation/leds/ledtrig-usbport.txt |  41 +++
 drivers/leds/trigger/Kconfig   |   8 +
 drivers/leds/trigger/Makefile  |   1 +
 drivers/leds/trigger/ledtrig-usbport.c | 316 +
 5 files changed, 377 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-usbport
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-usbport 
b/Documentation/ABI/testing/sysfs-class-led-trigger-usbport
new file mode 100644
index 000..a9b7e92
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-usbport
@@ -0,0 +1,11 @@
+What:  /sys/class/leds//ports/
+Date:  September 2016
+KernelVersion: 4.9
+Contact:   linux-l...@vger.kernel.org
+Description:
+   Every dir entry represents a single USB port that can be
+   selected for the USB port trigger. Selecting ports makes trigger
+   observing them for any connected devices and lighting on LED if
+   there are any.
+   Echoing "1" value selects USB port. Echoing "0" unselects it.
+   Current state can be also read.
diff --git a/Documentation/leds/ledtrig-usbport.txt 
b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 000..69f54bf
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,41 @@
+USB port LED trigger
+
+
+This LED trigger can be used for signalling to the user a presence of USB 
device
+in a given port. It 

Re: TCPCI driver issue

[Guenter Roeck]

On Thu, Sep 8, 2016 at 3:29 AM, Heikki Krogerus
 wrote:
> Hi Steve,
>
> On Wed, Sep 07, 2016 at 10:00:28PM -0400, Steve Schefter wrote:
>> Hi Heikki.
>>
>> I'm seeing an issue with the USB-C TCPCI driver.  On startup, I get a panic
>> with the following error and stack dump.
>>
>> Unable to handle kernel NULL pointer dereference at virtual address 002c
>>
>> [] tcpm_register_port+0x228/0x3b0
>> [] tcpci_probe+0x1c8/0x1fc
>> [] i2c_device_probe+0x12c/0x160
>>
>> The problem seems to stem from the fact that tcpci->tcpc.config is not
>> initialized in the TCPCI driver.  It is NULL due to tcpci being allocated
>> using devm_kzalloc().  This causes a problem when tcpm_register_port()
>> attempts to access port->tcpc->config->type.
>>
>> Am I missing something?  I understand that this is a work in progress, but
>> it looks like this should happen for everyone.  Some I'm wondering if I'm
>> missing something dumb.
>
> I believe you are trying Guenter's (CCd) Type-C Port Manager series.
> He is the correct best person to answer questions about that. But that
> series is RFC, so you can't really expect everything to be in place.
> It is more like proof of concept at this point.
>
I think (hope) I did mention that the tcpci patch was compile tested
only. Apologies if not. I'll try to get to it today and send a fix,
though it will obviously only be a workaround (config data is platform
specific). I also hope I can send a Reviewed-by: for Heikki's series
today. Sorry for being late on that one.

> But we probable need to talk about how to extract the platform
> specific details needed for the config in any case (and also what
> exactly is needed) at one point. I think we can get all of them with
> device properties regardless of the system (ACPI/DT/whatever), so
> we would just need to agree on the what the properties are.
>
Yes, we'll definitely need that. I've used platform data and DMI
internally for x86, but that seems so out of date. I'll also see if it
makes sense to attach the test driver I used to test the tcpm code.
Note that we also have a driver for fusb302 as client of tcpm in the
works, plus an extcon based driver connected to the EC on chromebooks
(and ties directly into the Type-C infrastructure).

Guenter
--
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: unusual_devs addition for Kingston DT100G3/32GB 0951:1666

[David Laight]

From: Wolfgang Breyha
> Sent: 08 September 2016 16:32
> David Laight wrote on 08/09/16 15:30:
> > Try with raw writes to the disk, rather than going through the filesystem.
> > That will remove effects caused by the alignment of the filesystem and the
> > size of the filesystem blocks.
> 
> Ok, I did
> dd if=768MB.test of=/dev/sdb oflag=direct bs=1M
> dd if=/dev/sdb bs=1M count=768 iflag=direct|sha1sum
> with the differently sized files from before (and a 2GB file) now...
> 
> I wrote them all once and the larger files (768MB, 2GB) multiple times, but
> only the 768MB and the 2GB file failed once yet:

Try writing with:
dd if=768MB.test of=/dev/sdb oflag=direct bs=1M seek_bytes seek=$((n*512))
for several small values of 'n' so that the writes are misaligned.

It may also be worth finding out what the USB transfers look like.

David

--
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: unusual_devs addition for Kingston DT100G3/32GB 0951:1666

[Wolfgang Breyha]

David Laight wrote on 08/09/16 15:30:
> Try with raw writes to the disk, rather than going through the filesystem.
> That will remove effects caused by the alignment of the filesystem and the
> size of the filesystem blocks.

Ok, I did
dd if=768MB.test of=/dev/sdb oflag=direct bs=1M
dd if=/dev/sdb bs=1M count=768 iflag=direct|sha1sum
with the differently sized files from before (and a 2GB file) now...

I wrote them all once and the larger files (768MB, 2GB) multiple times, but
only the 768MB and the 2GB file failed once yet:
768MB.test:
0x2F58B000-0x2F58

This one occured with "oflag=nonblock,seek_bytes seek=4G"
2GB.test:
0x55A01000-0x55A07FFF

With kind regards,
Wolfgang
-- 
Wolfgang Breyha  | http://www.blafasel.at/
Vienna University Computer Center | Austria

--
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 1/4] phy: Add reset callback

[Heiko Stuebner]

Am Donnerstag, 8. September 2016, 22:27:13 CEST schrieb Randy Li:
> The only use for this is for solving a hardware design problem in
> usb of Rockchip RK3288.
> 
> Signed-off-by: Randy Li 

to me this looks good.

Although Kishon suggested earlier to have the init callback do the reset and 
simply call it again in the reset-case, the whole refcounting done in phy_init 
and phy_exit (phy->init_count) really shows that init and exit should be 
called pairwise, so that extra reset callback seems justified, so from my phy-
noob-pov

Reviewed-by: Heiko Stuebner 

with one minor style nitpick below

> ---
>  drivers/phy/phy-core.c  | 14 ++
>  include/linux/phy/phy.h |  3 +++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 8eca906..32e838d 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -357,6 +357,20 @@ int phy_set_mode(struct phy *phy, enum phy_mode mode)
>  }
>  EXPORT_SYMBOL_GPL(phy_set_mode);
> 
> +int phy_reset(struct phy *phy)
> +{
> + int ret;

I think a blank line between declarations and code is customary.

> + if (!phy || !phy->ops->reset)
> + return 0;
> +
> + mutex_lock(>mutex);
> + ret = phy->ops->reset(phy);
> + mutex_unlock(>mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_reset);
> +
>  /**
>   * _of_phy_get() - lookup and obtain a reference to a phy by phandle
>   * @np: device_node for which to get the phy

Heiko
--
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 Thu, Sep 08, 2016 at 03:05:15PM +0200, Oliver Neukum wrote:
> > Sep  6 19:12:38 localhost kernel: Call Trace:
> > Sep  6 19:12:38 localhost kernel:  [] ? 0xc01f4347
> 
> Hi,
> 
> your stack trace is broken. Did you fail to install the System.map file?

Never needed that for anything the last 20 years or so.
I don't have the device at hand here, so new logs will be available
tomorrow.

Regards, 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 in usb_hc_died+0x16 when unplugging usb-c dock

[Alan Stern]

On Thu, 8 Sep 2016, Mathias Nyman wrote:

> > ehci-hcd includes checks in several places for ehci->rh_state ==
> > RH_STATE_RUNNING.  The removal pathway sets ehci->rh_state to
> > RH_STATE_HALTED.  As a result, the driver avoids waiting for things
> > that will never happen.
> >
> 
> Yes, seems that there are two things that need to be done for xhci here.
> 
> First part is doing the similar thing to xhci_urb_dequeue as ehci does, make 
> sure
> host is alive before queuing any stop endpoint commands. It does check if PCI 
> reads return
> 0x or host is XHCI_STATE_DYING, but we could detect a remove a lot 
> earlier.

There's a big difference between a removal and the host controller
dying, because removal can be clean.  So as long as the controller is
still running okay, you shouldn't skip any steps -- just refuse to
accept new URB submissions.  But if the controller is gone or dead,
then absolutely avoid queuing any new commands.

Alan Stern

> Second part is to make sure that the canceled URB is given back if the stop 
> endpoint command
> times out.
> Currently the xhci_stop_endpoint_command_watchdog() function may return 
> without
> giving back canceled urbs, causing usb_kill_urb() to wait on 
> wait_event(usb_kill_urb_queue, ..) forever with
> locks held, blocking the pci remove thread.
> 
> I'll start writing a patch
> 
> -Mathias
> 
> 

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

Re: [PATCH 1/3] usb: gadgetfs: introduce feature control mechanism

[Alan Stern]

On Thu, 8 Sep 2016, Felipe Balbi wrote:

> >> >> > +struct usb_gadgetfs_features {
> >> >> > + uint64_t bitmap[4];
> >> >> 
> >> >> this should really be:
> >> >> 
> >> >> uint64_t features0;
> >> >> 
> >> >> /* Keep 24 bytes reserved for possible future usage */
> >> >> uint8_t RESERVED[24];
> >> >
> >> > Isn't this really extreme overkill?  I would be surprised if gadgetfs 
> >> > ever supports more than 10 features.
> >> 
> >> I'm hoping to be extreme overkill :-) I really don't wanna get into a
> >> situation where we run out of bits. What if some features require
> >> arguments in the future? We could just pass them here, right?
> >> 
> >> Frankly, I'm afraid of what happened to i2c-dev :-) We can't change
> >> (easily) i2c subsystem's write() to return the amount of bytes written
> >> because its userspace facing ABI has no reserved bits we could use to
> >> return that value. Changing that would be a huge amount of work.
> >> 
> >> What do you think?
> >
> > Well, I don't care about it all that much, especially since I don't 
> > think you'll ever need to use it.  :-)
> >
> > Still, there are approaches that have been used in the past when people
> > wanted to create an extensible interface.  Passing a variable-length
> > structure that has a length field at a reserved spot near the beginning
> > (perhaps along with one or two other reserved entries), for example.  
> 
> something along the like below?
> 
> 
> /* better names appreciated */
> struct usb_gadgetfs_packet {
>   uint16_t length;
> uint8_t reserved[2];
> 
> uint8_t *data; /* dynamically allocated by userspace */
> }
> 
> I can work with that no problem.

I was thinking more like:

struct usb_gadgetfs_ioctl_arg {
uint16_t length;
uint8_t reserved[2];

uint8_t data[0];
}

but the principle is pretty much the same.

Alan Stern

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

[PATCH 2/4] phy: rockchip-usb: use rockchip_usb_phy_reset to reset phy during wakeup

[Randy Li]

It is a hardware bug in RK3288, the only way to solve it is to
reset the phy.

Signed-off-by: Randy Li 
---
 .../devicetree/bindings/phy/rockchip-usb-phy.txt |  3 +++
 drivers/phy/phy-rockchip-usb.c   | 20 
 2 files changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt 
b/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
index cc6be96..57dc388 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
@@ -27,6 +27,9 @@ Optional Properties:
 - clocks : phandle + clock specifier for the phy clocks
 - clock-names: string, clock name, must be "phyclk"
 - #clock-cells: for users of the phy-pll, should be 0
+- reset-names: Only allow the following entries:
+ - phy-reset
+- resets: Must contain an entry for each entry in reset-names.
 
 Example:
 
diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
index 2a7381f..734987f 100644
--- a/drivers/phy/phy-rockchip-usb.c
+++ b/drivers/phy/phy-rockchip-usb.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static int enable_usb_uart;
 
@@ -64,6 +65,7 @@ struct rockchip_usb_phy {
struct clk_hw   clk480m_hw;
struct phy  *phy;
booluart_enabled;
+   struct reset_control *reset;
 };
 
 static int rockchip_usb_phy_power(struct rockchip_usb_phy *phy,
@@ -144,9 +146,23 @@ static int rockchip_usb_phy_power_on(struct phy *_phy)
return clk_prepare_enable(phy->clk480m);
 }
 
+static int rockchip_usb_phy_reset(struct phy *_phy)
+{
+   struct rockchip_usb_phy *phy = phy_get_drvdata(_phy);
+
+   if (phy->reset) {
+   reset_control_assert(phy->reset);
+   udelay(10);
+   reset_control_deassert(phy->reset);
+   }
+
+   return 0;
+}
+
 static const struct phy_ops ops = {
.power_on   = rockchip_usb_phy_power_on,
.power_off  = rockchip_usb_phy_power_off,
+   .reset  = rockchip_usb_phy_reset,
.owner  = THIS_MODULE,
 };
 
@@ -185,6 +201,10 @@ static int rockchip_usb_phy_init(struct 
rockchip_usb_phy_base *base,
return -EINVAL;
}
 
+   rk_phy->reset = of_reset_control_get(child, "phy-reset");
+   if (IS_ERR(rk_phy->reset))
+   rk_phy->reset = NULL;
+
rk_phy->reg_offset = reg_offset;
 
rk_phy->clk = of_clk_get_by_name(child, "phyclk");
-- 
2.7.4

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

[PATCH 0/4] the fix for the USB HOST1 at rk3288 platform

[Randy Li]

  At this stage it is the only "full features" and well verified 
USB EHCI controller in this platform. More review is always necessary.

Changelog:
 - v7:
adding a wrapper for the reset operation for phy
using that wrapper
 - v6:
move pwms pinctrl to pwms node
fix the order of the dtb file in Makefile
 - v5:
   - correct the mail format
 - v4:
   - re-order some nodes in alphabetical order
   - fix some minor bugs
   - add a entry in vendor list
 - v3:
   - fixing the rtc clock, using clock source from PMIC
   - enable the tmu
   - enable the fimc for elite board
   - suuport the audio codec at elite board
   - fixing minor bugs in the last commit
 - v2:
   - removing rtc node
 the clock source driver is not done yet.
   - adding exynos-bus
   - fixing the MFC

Randy Li (4):
  phy: Add reset callback
  phy: rockchip-usb: use rockchip_usb_phy_reset to reset phy during
wakeup
  usb: dwc2: assert phy reset when waking up in rk3288 platform
  ARM: dts: rockchip: Point rk3288 dwc2 usb at the full PHY reset

 .../devicetree/bindings/phy/rockchip-usb-phy.txt |  3 +++
 arch/arm/boot/dts/rk3288.dtsi|  4 
 drivers/phy/phy-core.c   | 14 ++
 drivers/phy/phy-rockchip-usb.c   | 20 
 drivers/usb/dwc2/core_intr.c | 11 +++
 include/linux/phy/phy.h  |  3 +++
 6 files changed, 55 insertions(+)

-- 
2.7.4

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

[PATCH 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform

[Randy Li]

On the rk3288 USB host-only port (the one that's not the OTG-enabled
port) the PHY can get into a bad state when a wakeup is asserted (not
just a wakeup from full system suspend but also a wakeup from
autosuspend).

We can get the PHY out of its bad state by asserting its "port reset",
but unfortunately that seems to assert a reset onto the USB bus so it
could confuse things if we don't actually deenumerate / reenumerate the
device.

We can also get the PHY out of its bad state by fully resetting it using
the reset from the CRU (clock reset unit) in chip, which does a more full
reset.  The CRU-based reset appears to actually cause devices on the bus
to be removed and reinserted, which fixes the problem (albeit in a hacky
way).

It's unfortunate that we need to do a full re-enumeration of devices at
wakeup time, but this is better than alternative of letting the bus get
wedged.

Signed-off-by: Randy Li 
---
 drivers/usb/dwc2/core_intr.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index d85c5c9..5b9b671 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg 
*hsotg)
 static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 {
int ret;
+   struct device_node *np = hsotg->dev->of_node;
 
/* Clear interrupt */
dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
@@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct 
dwc2_hsotg *hsotg)
/* Restart the Phy Clock */
pcgcctl &= ~PCGCTL_STOPPCLK;
dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
+
+   /*
+* It is a quirk in Rockchip RK3288, causing by
+* a hardware bug. This will propagate out and
+* eventually we'll re-enumerate the device. 
+* Not great but the best we can do 
+*/
+   if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
+   phy_reset(hsotg->phy);
+
mod_timer(>wkp_timer,
  jiffies + msecs_to_jiffies(71));
} else {
-- 
2.7.4

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

[PATCH 4/4] ARM: dts: rockchip: Point rk3288 dwc2 usb at the full PHY reset

[Randy Li]

The "host1" port (AKA the dwc2 port that isn't the OTG port) on rk3288
has a hardware errata that causes everything to get confused when we get
a remote wakeup.  We'll use the reset that's in the CRU to reset the
port when it's in a bad state.

Note that we add the reset to both dwc2 controllers even though only one
has the errata in case we find some other use for this reset that's
unrelated to the current hardware errata.  Only the host port gets the
quirk property, though.

Signed-off-by: Randy Li 
---
 arch/arm/boot/dts/rk3288.dtsi | 4 
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 17ec2e2..34de803 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -857,6 +857,8 @@
clocks = < SCLK_OTGPHY0>;
clock-names = "phyclk";
#clock-cells = <0>;
+   resets = < SRST_USBOTG_PHY>;
+   reset-names = "phy-reset";
};
 
usbphy1: usb-phy@334 {
@@ -873,6 +875,8 @@
clocks = < SCLK_OTGPHY2>;
clock-names = "phyclk";
#clock-cells = <0>;
+   resets = < SRST_USBHOST1_PHY>;
+   reset-names = "phy-reset";
};
};
};
-- 
2.7.4

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

[PATCH 1/4] phy: Add reset callback

[Randy Li]

The only use for this is for solving a hardware design problem in
usb of Rockchip RK3288.

Signed-off-by: Randy Li 
---
 drivers/phy/phy-core.c  | 14 ++
 include/linux/phy/phy.h |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 8eca906..32e838d 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -357,6 +357,20 @@ int phy_set_mode(struct phy *phy, enum phy_mode mode)
 }
 EXPORT_SYMBOL_GPL(phy_set_mode);
 
+int phy_reset(struct phy *phy)
+{
+   int ret;
+   if (!phy || !phy->ops->reset)
+   return 0;
+
+   mutex_lock(>mutex);
+   ret = phy->ops->reset(phy);
+   mutex_unlock(>mutex);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(phy_reset);
+
 /**
  * _of_phy_get() - lookup and obtain a reference to a phy by phandle
  * @np: device_node for which to get the phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index f08b672..7978df6 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -36,6 +36,7 @@ enum phy_mode {
  * @power_on: powering on the phy
  * @power_off: powering off the phy
  * @set_mode: set the mode of the phy
+ * @reset: reseting the phy
  * @owner: the module owner containing the ops
  */
 struct phy_ops {
@@ -44,6 +45,7 @@ struct phy_ops {
int (*power_on)(struct phy *phy);
int (*power_off)(struct phy *phy);
int (*set_mode)(struct phy *phy, enum phy_mode mode);
+   int (*reset)(struct phy *phy);
struct module *owner;
 };
 
@@ -136,6 +138,7 @@ int phy_exit(struct phy *phy);
 int phy_power_on(struct phy *phy);
 int phy_power_off(struct phy *phy);
 int phy_set_mode(struct phy *phy, enum phy_mode mode);
+int phy_reset(struct phy *phy);
 static inline int phy_get_bus_width(struct phy *phy)
 {
return phy->attrs.bus_width;
-- 
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 in usb_hc_died+0x16 when unplugging usb-c dock

[Mathias Nyman]

On 07.09.2016 17:53, Alan Stern wrote:

On Wed, 7 Sep 2016, Mathias Nyman wrote:


I'm still seeing occasional problems. For example, when I unplugged the dock 
last night, it seems to have wedged some things, and then plugging it back in 
didn't work. See some logs below.


I ran a show-blocked-tasks after plugging the dock back in:



Looks like there is the usb_hub_wq that tries to handle the disconnect event
at the same time as the pci remove code is removing xhci hosts (and connected 
devices)


Sep  7 09:03:30 fred kernel: [83879.383356] Workqueue: usb_hub_wq hub_event
Sep  7 09:03:30 fred kernel: [83879.383395] Call Trace:
Sep  7 09:03:30 fred kernel: [83879.383416]  [] 
schedule+0x35/0x80
Sep  7 09:03:30 fred kernel: [83879.383427]  [] 
usb_kill_urb+0x8d/0xc0
Sep  7 09:03:30 fred kernel: [83879.383444]  [] ? 
wake_atomic_t_function+0x60/0x60
Sep  7 09:03:30 fred kernel: [83879.383454]  [] 
usb_hcd_flush_endpoint+0x126/0x190
Sep  7 09:03:30 fred kernel: [83879.383465]  [] 
usb_disable_endpoint+0x9b/0xb0




Sep  7 09:03:30 fred kernel: [83879.383686] Workqueue: kacpi_hotplug 
acpi_hotplug_work_fn
Sep  7 09:03:30 fred kernel: [83879.383717] Call Trace:
Sep  7 09:03:30 fred kernel: [83879.383728]  [] 
schedule+0x35/0x80
Sep  7 09:03:30 fred kernel: [83879.383738]  [] 
schedule_preempt_disabled+0xe/0x10
Sep  7 09:03:30 fred kernel: [83879.383748]  [] 
__mutex_lock_slowpath+0xb9/0x130
Sep  7 09:03:30 fred kernel: [83879.383758]  [] 
mutex_lock+0x1f/0x30
Sep  7 09:03:30 fred kernel: [83879.383766]  [] 
usb_disconnect+0x51/0x280
Sep  7 09:03:30 fred kernel: [83879.383776]  [] 
usb_remove_hcd+0xd0/0x240


First guess would be there is something wrong with killing the urb.
usb_hub_wq takes the roothub device lock first, and then ends up waiting for 
usb_kill_urb forever.


I agree.  Probably xhci-hcd is waiting for the controller to do
something before it will give back the cancelled URB.  But since the
controller has been removed, it never does anything.


This would block the pci remove path when usb_remove_hcd calls usb_disconnect, 
which
tries to take the roothub lock as well.

Doing a usbfs read on a usb device also takes the roothub device lock, which 
could explain
why lsusb is blocked.

Just an idea, need to check the code in more detail to see if it's a possible 
cause


ehci-hcd includes checks in several places for ehci->rh_state ==
RH_STATE_RUNNING.  The removal pathway sets ehci->rh_state to
RH_STATE_HALTED.  As a result, the driver avoids waiting for things
that will never happen.



Yes, seems that there are two things that need to be done for xhci here.

First part is doing the similar thing to xhci_urb_dequeue as ehci does, make 
sure
host is alive before queuing any stop endpoint commands. It does check if PCI 
reads return
0x or host is XHCI_STATE_DYING, but we could detect a remove a lot 
earlier.
 
Second part is to make sure that the canceled URB is given back if the stop endpoint command

times out.
Currently the xhci_stop_endpoint_command_watchdog() function may return without
giving back canceled urbs, causing usb_kill_urb() to wait on 
wait_event(usb_kill_urb_queue, ..) forever with
locks held, blocking the pci remove thread.

I'll start writing a patch

-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: unusual_devs addition for Kingston DT100G3/32GB 0951:1666

[David Laight]

From: Wolfgang Breyha [mailto:wbre...@gmx.net]
> Sent: 08 September 2016 14:27
> David Laight wrote on 07/09/16 18:25:
> > I'd expect files much smaller than 500MB to fail if the problem was
> > actually to do with a 64 sector limit.
> >
> > So I'm guessing that this isn't the fix.
> >
> > Much more likely is that limiting the sector count to 64 stops transfers
> > crossing some sector boundary (maybe at 500MB ?).
> 
> I did some more tests. The test system for these tests runs Fedora 24 with
> kernel 4.6.4-301.fc24.x86_64, 8GB RAM, i5-4670. The problems also occurs on
> the most recent 4.7.2-201.fc24.x86_64 on two other machines (HP EB 820 G1,
> ASUS Z170/i5-6500).
> 
> I removed the quirks option and generated random files with different sizes
> with eg:
> dd if=/dev/urandom iflag=fullblock of=256MB.test bs=1M count=256
...

Try with raw writes to the disk, rather than going through the filesystem.
That will remove effects caused by the alignment of the filesystem and the
size of the filesystem blocks.

David

--
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: unusual_devs addition for Kingston DT100G3/32GB 0951:1666

[Wolfgang Breyha]

David Laight wrote on 07/09/16 18:25:
> I'd expect files much smaller than 500MB to fail if the problem was
> actually to do with a 64 sector limit.
> 
> So I'm guessing that this isn't the fix.
> 
> Much more likely is that limiting the sector count to 64 stops transfers
> crossing some sector boundary (maybe at 500MB ?).

I did some more tests. The test system for these tests runs Fedora 24 with
kernel 4.6.4-301.fc24.x86_64, 8GB RAM, i5-4670. The problems also occurs on
the most recent 4.7.2-201.fc24.x86_64 on two other machines (HP EB 820 G1,
ASUS Z170/i5-6500).

I removed the quirks option and generated random files with different sizes
with eg:
dd if=/dev/urandom iflag=fullblock of=256MB.test bs=1M count=256

I tried: 256,320,352,368,376,380,382,383,384,512,640,768

I copied them multiple times to the stick with a small script doing
cp 
sync
sleep 20
and until now the smallest file with damage was the 352MB file. First I
thought it's arround 384MB, but then smaller files failed sometimes as well.

I checked some files with vbindiff and search for the damaged regions.
They always start on clean 0x1000 borders and end with 0x3FFF, 0x7FFF, 0xAFFF
or 0x. Most of the time 0x7FFF and 0x. 0x3FFF and 0xAFFF seem to occur
only if it starts exactly at 0x or 0x8000 and size is 0x4000. Sizes seen
go from 0x1000 up to 0x7000.

The larger the file gets it's more likely it's damaged. Up to 320MB all test
files were ok. Starting at 352MB up to 384MB the likelyness raises and from
512MB up all files were damaged.

I tried to find the first 8 hex bytes of the wrong sectors in the rest of the
file, but they do not occure anywhere else, neither in the original file nor
in the copy. So it's not a leftover from some other buffer of the same file.

damaged regions:
352MB.test2:
0x1353B000-0x1353
368MB.test1:
0x15B4B000-0x15B4
376MB.test2:
0x13AB3000-0x13AB7FFF
380MB.test1:
0x138FC000-0x138F
0x14907000-0x14907FFF
0x14931000-0x14937FFF
0x1694C000-0x1694
381MB.test1:
0x14011000-0x14017FFF
382MB.test1:
0x135F8000-0x135FAFFF
383MB.test3:
0x0EC0A000-0x0EC0
0x15D0B000-0x15D0
384MB.test:
0x087FB000-0x087F
0x13959000-0x1395
384MB.test2:
0x159B2000-0x159B7FFF
0x169EC000-0x169E
384MB.test3:
0x151C3000-0x151C7FFF
512MB.test:
0x0FBA7000-0x0FBA7FFF
0x11BFC000-0x11BF
0x16C95000-0x16C97FFF
0x18CD4000-0x18CD7FFF
0x1ED7D000-0x1ED7
0x1FDAF000-0x1FDA
512MB.test2:
0x0705D000-0x0705
0x16263000-0x16267FFF
0x192AC000-0x192A
0x192E6000-0x192E7FFF
0x1F36-0x1F363FFF
0x1F372000-0x1F377FFF

If I can do anything else to find the source of this corruptions I will try to
help.

With kind regards,
Wolfgang Breyha
-- 
Wolfgang Breyha  | http://www.blafasel.at/
Vienna University Computer Center | Austria

--
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-next 0/3] r8152: configuration setting

[Bjørn Mork]

Hayes Wang  writes:

> Bjørn Mork [mailto:bj...@mork.no]
>> Sent: Thursday, September 08, 2016 3:55 PM
> [...]
>> Yes, I see that.  But is that strictly necessary? Couldn't you just say:
>> "CDC ECM is supported by cdc_ether and therefore limited to the features
>> implemented by cdc_ether.  If you want feature X, then please use our
>> vendor specific mode with the r8152 driver?"
>
> My customs have a case that they must force the speed to 100M
> for some reasons. I also wish to implement the driver as simple
> as possible, but I don't think I could determine this. I accept
> you reject my patches. However, I couldn't deny the requests
> from the boss or customs without doing anything. I must prove
> the way is unacceptable.

That's an odd combination of requirements, but I know how customers can
be :)

Just to make it clear:  I provide comments, but I am in no position to
reject any of your patches.  Or have any wish to do so.  You maintain
r8152.  Oliver maintains cdc_ether.  I am confident that whatever you
two decide is going to be fine.

> [...]
>> Each USB configuation comes with a set of descriptors identifying the
>> functions, and USB interface drivers attach to the functions they
>> support.  The user can dynamically switch the device from e.g. cfg #1 to
>> cfg #3 by writing "3" to /sys/bus/usb/devices//bConfigurationValue
>> This will cause the ECM and ACM USB interfaces to disappear, and the
>> associated class drivers will unbind, and new vendor specific USB
>> interfaces appear instead, causing a matching vendor specific driver to
>> load and bind.
>> 
>> Naturally, end users will not switch configurations all the time.  They
>> will select the configuration providing the set of functions they want.
>> If this is different from the default configuration  selected by the
>> Linux USB core, then that's a simple udev rule to update the
>> bConfigurationValue sysfs attribute on device disceovery.
>
> I tested above method before. And I found that the cdc_ether
> was loaded before switching the configuration. The behavior
> of loading one driver and changing to another driver has
> opportunity to let our some previous chips become abnormal.
> To switch configuration is fine. However, it may have problem
> to switch driver. That is why the current kernel only supports
> vendor mode. If the method works fine, I have no trouble now.

Yes, many firmwares/devices are not prepared to do "late" config
switching and can end up in a strange limbo if they are initialized
before switching. An udev rule should still run early enough to prevent
this problem I believe.

But if that doesn't work, then I agree that a blacklist makes more
sense. Just make it runtime configurable so that distros can do
something sane with it.



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: [PATCH net-next 0/3] r8152: configuration setting

[Oliver Neukum]

On Thu, 2016-09-08 at 13:02 +, Hayes Wang wrote:
> I tested above method before. And I found that the cdc_ether 
> was loaded before switching the configuration. The behavior 
> of loading one driver and changing to another driver has 
> opportunity to let our some previous chips become abnormal. 
> To switch configuration is fine. However, it may have problem 
> to switch driver. That is why the current kernel only supports 
> vendor mode. If the method works fine, I have no trouble now.

We may talk about creating extensions to cdc-ether, if they
are needed for the phy. What is unacceptable is to override
the configuration mechanism in the kernel.

Regards
Oliver



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

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

[Oliver Neukum]

On Thu, 2016-09-08 at 14:58 +0200, Wim Osterholt wrote:
> On Thu, Sep 08, 2016 at 02:20:38PM +0200, Oliver Neukum wrote:
> > > 
> > > The oops tells things that I didn't all write down, but it says
> > > null pointer dereference at 0246
> > 
> > That is the important part. I am sorry, but without the oops
> > nobody can help you. Please capture it
> 
> Sep  6 19:12:37 localhost kernel: usb 7-1: new full-speed USB device number 2 
> using uhci_hcd
> Sep  6 19:12:37 localhost kernel: usb 7-1: New USB device found, 
> idVendor=0572, idProduct=1340
> Sep  6 19:12:37 localhost kernel: usb 7-1: New USB device strings: Mfr=1, 
> Product=2, SerialNumber=3
> Sep  6 19:12:37 localhost kernel: usb 7-1: Product: USB Modem
> Sep  6 19:12:37 localhost kernel: usb 7-1: Manufacturer: Conexant
> Sep  6 19:12:37 localhost kernel: usb 7-1: SerialNumber: 12345678
> Sep  6 19:12:38 localhost mtp-probe[13126]: checking bus 7, device 2: 
> "/sys/devices/pci:00/:00:1d.3/usb7/7-1"
> Sep  6 19:12:38 localhost mtp-probe[13126]: bus: 7, device: 2 was not an MTP 
> device
> Sep  6 19:12:38 localhost kernel: BUG: unable to handle kernel NULL pointer 
> dereference at 0246
> Sep  6 19:12:38 localhost kernel: IP: [] 0xe0a81a35
> Sep  6 19:12:38 localhost kernel: *pde =  
> Sep  6 19:12:38 localhost kernel: Oops:  [#1] SMP
> Sep  6 19:12:38 localhost kernel: Modules linked in: cdc_acm(+) nouveau video 
> drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm agpgart 
> i2c_algo_bit lirc_serial(C) lirc_dev(O) cfg80211 rfkill binfmt_misc 
> svgalib_helper(O) snd_pcm_oss snd_mixer_oss fbcon bitblit softcursor font 
> tileblit sr9700 dm9601 usbnet usb_storage mii snd_hda_codec_generic tg3 
> snd_hda_intel snd_hda_codec ptp snd_hwdep pps_core gpio_ich snd_hda_core 
> libphy ppdev firmware_class uhci_hcd pcspkr snd_pcm ohci_pci ohci_hcd lpc_ich 
> ehci_pci snd_timer ehci_hcd mfd_core snd usbcore floppy soundcore wmi 
> parport_pc usb_common parport acpi_cpufreq processor button
> Sep  6 19:12:38 localhost kernel: CPU: 0 PID: 13127 Comm: udevd Tainted: G
>  C O4.8.0-rc5 #1
> Sep  6 19:12:38 localhost kernel: Hardware name: Hewlett-Packard HP xw4300 
> Workstation/0A00h, BIOS 786D3 v01.08 03/10/2006
> Sep  6 19:12:38 localhost kernel: task: df639c00 task.stack: df4d6000
> Sep  6 19:12:38 localhost kernel: EIP: 0060:[] EFLAGS: 00010202 
> CPU: 0
> Sep  6 19:12:38 localhost kernel: EAX:  EBX: def22000 ECX:  
> EDX: 0124
> Sep  6 19:12:38 localhost kernel: ESI: def2227c EDI: dee04000 EBP: 0246 
> ESP: df4d7ce8
> Sep  6 19:12:38 localhost kernel:  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 
> 0068
> Sep  6 19:12:38 localhost kernel: CR0: 80050033 CR2: 0246 CR3: 19b4d000 
> CR4: 0690
> Sep  6 19:12:38 localhost kernel: Stack:
> Sep  6 19:12:38 localhost kernel:  dee8ba00 dee8ba00 dee8b400 dee04000 
> df4b6380 df4b63b8 0001 dee04070
> Sep  6 19:12:38 localhost kernel:  daca2ec0 dee8ba1c 0040 0010 
>  def2200c def2227c 
> Sep  6 19:12:38 localhost kernel:  dee20800  df639c00 dcdc37e0 
> c01f4347 0246 0246 dcdc37e0
> Sep  6 19:12:38 localhost kernel: Call Trace:
> Sep  6 19:12:38 localhost kernel:  [] ? 0xc01f4347

Hi,

your stack trace is broken. Did you fail to install the System.map file?

Regards
Oliver


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

RE: [PATCH net-next 0/3] r8152: configuration setting

[Hayes Wang]

Bjørn Mork [mailto:bj...@mork.no]
> Sent: Thursday, September 08, 2016 3:55 PM
[...]
> Yes, I see that.  But is that strictly necessary? Couldn't you just say:
> "CDC ECM is supported by cdc_ether and therefore limited to the features
> implemented by cdc_ether.  If you want feature X, then please use our
> vendor specific mode with the r8152 driver?"

My customs have a case that they must force the speed to 100M
for some reasons. I also wish to implement the driver as simple
as possible, but I don't think I could determine this. I accept
you reject my patches. However, I couldn't deny the requests
from the boss or customs without doing anything. I must prove
the way is unacceptable.

[...]
> Each USB configuation comes with a set of descriptors identifying the
> functions, and USB interface drivers attach to the functions they
> support.  The user can dynamically switch the device from e.g. cfg #1 to
> cfg #3 by writing "3" to /sys/bus/usb/devices//bConfigurationValue
> This will cause the ECM and ACM USB interfaces to disappear, and the
> associated class drivers will unbind, and new vendor specific USB
> interfaces appear instead, causing a matching vendor specific driver to
> load and bind.
> 
> Naturally, end users will not switch configurations all the time.  They
> will select the configuration providing the set of functions they want.
> If this is different from the default configuration  selected by the
> Linux USB core, then that's a simple udev rule to update the
> bConfigurationValue sysfs attribute on device disceovery.

I tested above method before. And I found that the cdc_ether
was loaded before switching the configuration. The behavior
of loading one driver and changing to another driver has
opportunity to let our some previous chips become abnormal.
To switch configuration is fine. However, it may have problem
to switch driver. That is why the current kernel only supports
vendor mode. If the method works fine, I have no trouble now.

Best Regards,
Hayes

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Grygorii Strashko]

On 09/08/2016 03:28 PM, Peter Chen wrote:
> On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
>> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>>> Arnd Bergmann  writes:
 On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> If we do that, we have to put child devices of the dwc3 devices into
>> the platform glue, and it also breaks those dwc3 devices that don't
>> have a parent driver.
>
> Well, this is easy to fix:
>
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

 I don't understand. Do you mean we should have an extra level of
 stacking and splitting "static struct platform_driver dwc3_driver"
 in two so instead of

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

 we do this?

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
 (usb_bus.dev)
>>>
>>> no 
>>>
>>> If we have a parent device, use that as sysdev, otherwise use self as
>>> sysdev.
>>
>> But there is often a parent device in DT, as the xhci device is
>> attached to some internal bus that gets turned into a platform_device
>> as well, so checking whether there is a parent will get the wrong
>> device node.
> 
> From my point, all platform and firmware information at dwc3 are
> correct, so we don't need to change dwc3/core.c, only changing for
> xhci-plat.c is ok.
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index ed56bf9..fd57c0d 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   struct clk  *clk;
>   int ret;
>   int irq;
> + struct device *dev = >dev, *sysdev;
>  
>   if (usb_disabled())
>   return -ENODEV;
> @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   if (irq < 0)
>   return -ENODEV;
>  
> + if (dev->parent) {
> + sysdev = dev->parent;
> + } else {
> + sysdev = dev;
> + }
> +

Shouldn't we be more careful with that?

armada-375.dtsi

soc {
compatible = "marvell,armada375-mbus", "simple-bus";

internal-regs {
compatible = "simple-bus";

usb3@58000 {
compatible = "marvell,armada-375-xhci";
reg = <0x58000 0x2>,<0x5b880 0x80>;
interrupts = ;
clocks = < 16>;
phys = < PHY_TYPE_USB3>;
phy-names = "usb";
status = "disabled";
};


What will be the parent dev in above case?

-- 
regards,
-grygorii
--
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 Thu, Sep 08, 2016 at 02:20:38PM +0200, Oliver Neukum wrote:
> > 
> > The oops tells things that I didn't all write down, but it says
> > null pointer dereference at 0246
> 
> That is the important part. I am sorry, but without the oops
> nobody can help you. Please capture it

Sep  6 19:12:37 localhost kernel: usb 7-1: new full-speed USB device number 2 
using uhci_hcd
Sep  6 19:12:37 localhost kernel: usb 7-1: New USB device found, idVendor=0572, 
idProduct=1340
Sep  6 19:12:37 localhost kernel: usb 7-1: New USB device strings: Mfr=1, 
Product=2, SerialNumber=3
Sep  6 19:12:37 localhost kernel: usb 7-1: Product: USB Modem
Sep  6 19:12:37 localhost kernel: usb 7-1: Manufacturer: Conexant
Sep  6 19:12:37 localhost kernel: usb 7-1: SerialNumber: 12345678
Sep  6 19:12:38 localhost mtp-probe[13126]: checking bus 7, device 2: 
"/sys/devices/pci:00/:00:1d.3/usb7/7-1"
Sep  6 19:12:38 localhost mtp-probe[13126]: bus: 7, device: 2 was not an MTP 
device
Sep  6 19:12:38 localhost kernel: BUG: unable to handle kernel NULL pointer 
dereference at 0246
Sep  6 19:12:38 localhost kernel: IP: [] 0xe0a81a35
Sep  6 19:12:38 localhost kernel: *pde =  
Sep  6 19:12:38 localhost kernel: Oops:  [#1] SMP
Sep  6 19:12:38 localhost kernel: Modules linked in: cdc_acm(+) nouveau video 
drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm agpgart 
i2c_algo_bit lirc_serial(C) lirc_dev(O) cfg80211 rfkill binfmt_misc 
svgalib_helper(O) snd_pcm_oss snd_mixer_oss fbcon bitblit softcursor font 
tileblit sr9700 dm9601 usbnet usb_storage mii snd_hda_codec_generic tg3 
snd_hda_intel snd_hda_codec ptp snd_hwdep pps_core gpio_ich snd_hda_core libphy 
ppdev firmware_class uhci_hcd pcspkr snd_pcm ohci_pci ohci_hcd lpc_ich ehci_pci 
snd_timer ehci_hcd mfd_core snd usbcore floppy soundcore wmi parport_pc 
usb_common parport acpi_cpufreq processor button
Sep  6 19:12:38 localhost kernel: CPU: 0 PID: 13127 Comm: udevd Tainted: G  
   C O4.8.0-rc5 #1
Sep  6 19:12:38 localhost kernel: Hardware name: Hewlett-Packard HP xw4300 
Workstation/0A00h, BIOS 786D3 v01.08 03/10/2006
Sep  6 19:12:38 localhost kernel: task: df639c00 task.stack: df4d6000
Sep  6 19:12:38 localhost kernel: EIP: 0060:[] EFLAGS: 00010202 CPU: 0
Sep  6 19:12:38 localhost kernel: EAX:  EBX: def22000 ECX:  
EDX: 0124
Sep  6 19:12:38 localhost kernel: ESI: def2227c EDI: dee04000 EBP: 0246 
ESP: df4d7ce8
Sep  6 19:12:38 localhost kernel:  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Sep  6 19:12:38 localhost kernel: CR0: 80050033 CR2: 0246 CR3: 19b4d000 
CR4: 0690
Sep  6 19:12:38 localhost kernel: Stack:
Sep  6 19:12:38 localhost kernel:  dee8ba00 dee8ba00 dee8b400 dee04000 df4b6380 
df4b63b8 0001 dee04070
Sep  6 19:12:38 localhost kernel:  daca2ec0 dee8ba1c 0040 0010  
def2200c def2227c 
Sep  6 19:12:38 localhost kernel:  dee20800  df639c00 dcdc37e0 c01f4347 
0246 0246 dcdc37e0
Sep  6 19:12:38 localhost kernel: Call Trace:
Sep  6 19:12:38 localhost kernel:  [] ? 0xc01f4347
Sep  6 19:12:38 localhost kernel:  [] ? 0xc04357ca
Sep  6 19:12:38 localhost kernel:  [] ? 0xe0ac422f
Sep  6 19:12:38 localhost kernel:  [] ? 0xc030276f
Sep  6 19:12:38 localhost kernel:  [] ? 0xc03028ce
Sep  6 19:12:38 localhost kernel:  [] ? 0xc030140b
Sep  6 19:12:38 localhost kernel:  [] ? 0xc030238a
Sep  6 19:12:38 localhost kernel:  [] ? 0xc0302879
Sep  6 19:12:38 localhost kernel:  [] ? 0xc030205e
Sep  6 19:12:38 localhost kernel:  [] ? 0xc0302e85
Sep  6 19:12:38 localhost kernel:  [] ? 0xe0ac3525
Sep  6 19:12:38 localhost kernel:  [] ? 0xe08620a9
Sep  6 19:12:38 localhost kernel:  [] ? 0xe0862000
Sep  6 19:12:38 localhost kernel:  [] ? 0xc01003eb
Sep  6 19:12:38 localhost kernel:  [] ? 0xc04357ca
Sep  6 19:12:38 localhost kernel:  [] ? 0xc027c45c
Sep  6 19:12:38 localhost kernel:  [] ? 0xc01ae14a
Sep  6 19:12:38 localhost kernel:  [] ? 0xc0183bf5
Sep  6 19:12:38 localhost kernel:  [] ? 0xc0183c24
Sep  6 19:12:38 localhost kernel:  [] ? 0xc0174deb
Sep  6 19:12:38 localhost kernel:  [] ? 0xc0175043
Sep  6 19:12:38 localhost kernel:  [] ? 0xc0100ed1
Sep  6 19:12:38 localhost kernel:  [] ? 0xc0436e65
Sep  6 19:12:38 localhost kernel: Code: 44 24 04 ba 20 2c a8 e0 89 58 74 83 c0 
1c 89 44 24 24 e8 0f de 87 df 85 c0 0f 88 ed fe ff ff 8b 6c 24 54 85 ed 0f 84 
91 00 00 00 <0f> b6 45 00 ba c0 00 40 02 83 e8 04 e8 eb c5 72 df 85 c0 89 83
Sep  6 19:12:38 localhost kernel: EIP: []  SS:ESP 0068:df4d7ce8
Sep  6 19:12:38 localhost kernel: CR2: 0246
Sep  6 19:12:38 localhost kernel: ---[ end trace 64919c4014c0aa1d ]---
Sep  6 19:12:38 localhost kernel: udevd[919]: worker [13127] terminated by 
signal 9 (Killed)
Sep  6 19:12:38 localhost kernel: udevd[919]: worker [13127] failed while 
handling '/devices/pci:00/:00:1d.3/usb7/7-1/7-1:1.1'
Sep  6 19:12:38 localhost kernel: clocksource: timekeeping watchdog on CPU1: 
Marking clocksource 'tsc' as unstable because the skew is too 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 8:28:10 PM CEST Peter Chen wrote:
> On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > > Arnd Bergmann  writes:
> > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > > If we have a parent device, use that as sysdev, otherwise use self as
> > > sysdev.
> > 
> > But there is often a parent device in DT, as the xhci device is
> > attached to some internal bus that gets turned into a platform_device
> > as well, so checking whether there is a parent will get the wrong
> > device node.
> 
> From my point, all platform and firmware information at dwc3 are
> correct, so we don't need to change dwc3/core.c, only changing for
> xhci-plat.c is ok.

Ok, thanks. That leaves the PCI glue, right?

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index d2e3f65..563600b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
>   /* Did the HC die before the root hub was registered? */
>   if (HCD_DEAD(hcd))
>   usb_hc_died (hcd);  /* This time clean up */
> - usb_dev->dev.of_node = parent_dev->of_node;
> + usb_dev->dev.of_node = parent_dev->sysdev->of_node;
>   }
>   mutex_unlock(_bus_idr_lock);
> 
> At above changes, the root hub's of_node equals to xhci-hcd sysdev's
> of_node, which is from firmware or from its parent (it is dwc3 core
> device).

Just to make sure I understand you right:

in the qcom,dwc3 -> dwc3 -> xhci hierarchy, this would be the
dwc3 device, not the qcom,dwc3 device.

> > > > That sounds a bit clumsy for the sake of consistency with PCI.
> > > > The advantage is that xhci can always use the grandparent device
> > > > as sysdev whenever it isn't probed through PCI or firmware
> > > > itself, but the purpose of the dwc3-glue is otherwise questionable.
> > > >
> > > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > > > device when that is created from the PCI driver and checking for that
> > > > with the device property interface instead? If it's "snps,dwc3"
> > > > we use the device itself while for "snps,dwc3-pci", we use the parent?
> > > 
> 
> For pci glue device, it is always the parent for dwc3 core device.
> In your patch, you may not need to split pci or non-pci, just using
> if (dev->parent).

Here we have the pci-dwc3 -> dwc3 -> xhci hierarchy, and we want
sysdev to point to pci-dwc3, not dwc3!

The point is that the pci_dev is where we have the dma settings
and (optionally) additional DT or ACPI data for that device.

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> >> If we make dwc3.ko a library which glue calls directly then all these
> >> problems are solved but we break all current DTs and fall into the trap
> >> of having another MUSB.
> >
> > I don't see how we'd break the current DTs, I'm fairly sure we could turn 
> > dwc3
> 
> well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to
> look at possible children for their own quirks and properties.
> 
> > into a library without changing the DT representation. However the parts
> > that I think would change are
> >
> > - The sysfs representation for dwc3-pci, as we would no longer have
> >   a parent-child relationship there.
> 
> that's a no-brainer, I think
> 
> > - The power management handling might need a rework, since you currently
> >   rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
> >   power on and off
> 
> simple enough to do as well.
> 
> > - turning dwc3 into a library probably implies also turning xhci into
> >   a library, in part for consistency.
> 
> yeah, I considered that too. We could still do it in parts, though.
> 
> > - if we don't do the whole usb_bus->sysdev thing, we need to not just
> >   do this for dwc3 but also chipidea and maybe a couple of others.
> 
> MUSB comes to mind

Right.

> > There should not be any show-stoppers here, but it's a lot of work.
> 
> I think the biggest work will making sure people don't abuse functions
> just because they're now part of a single binary. Having them as
> separate modules helped a lot reducing the maintenance overhead. There
> was only one occasion where someone sent a glue layer which iterated
> over its children to find struct dwc3 * from child's drvdata.

This is where it get a bit philosophical ;-)

I understand that you like the strict separation that the current model
provides, and I agree that can be an advantage.

Changing the abstraction model to a set of library modules the way that
other drivers (e.g. ehci, sdhci, or libata) work to me means changing
this separation model into a different model and once we do that I would
not consider it a mistake for the platform specific driver to take
advantage of that. You still get a bit of separation since the drivers
would be in separate modules that can only access exported symbols,
and the library can still hide its data structures (to some degree).

I still think that turning xhci (and dwc3) into a library would be
an overall win, but if we solve the problems of DMA settings and
usb_device DT properties without it, I'd prefer not to fight over
that with you again ;-)

> >> If we try to pass DMA bits from parent to child, then we have the fact
> >> that DT ends up, in practice, always having a parent device.
> >
> > I don't understand what you mean here, but I agree that the various ways
> 
> well, we can't simply use what I pointed out a few emails back:
> 
> if (dwc->dev->parent)
>   dwc->sysdev = dwc->dev->parent
> else
>   dwc->sysdev = dwc->dev

Ok, I see.

> > we discussed for copying the DMA flags from one 'struct device' to another
> > all turned out to be flawed in at least one way.
> >
> > Do you see any problems with the patch I posted other than the ugliness
> > of the dwc3 and xhci drivers finding out which pointer to use for
> > usb_bus->sysdev? If we can solve this, we shouldn't need any new
> > of_dma_configure/acpi_dma_configure calls and we won't have to
> > turn the drivers into a library, so maybe let's try to come up with
> > better ideas for that sub-problem.
> 
> No big problems with that, no. Just the ifdef looking for a PCI bus in
> the parent. How about passing a flag via device_properties? I don't
> wanna change dwc3 core's device name with a platform_device_id because
> there probably already are scripts relying on the names to enable
> pm_runtime for example.

Sounds ok to me. Grygorii's solution might a be a bit more elegant,
but also a bit more error-prone:
If we get a platform that mistakenly sets the dma_mask pointer of
the child device, or a platform that does not set the dma_mask
pointer of the parent, things break.

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

[PATCH 3/3] usb: chipidea: udc: Use the preferred form for passing a size of a struct

[Fabio Estevam]

From: Fabio Estevam 

According to Documentation/CodingStyle:

"The preferred form for passing a size of a struct is the following:

p = kmalloc(sizeof(*p), ...);
"
, so do as suggested to improve readability.

Signed-off-by: Fabio Estevam 
---
 drivers/usb/chipidea/udc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 336793f..16c251a 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1984,7 +1984,7 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
return -ENXIO;
 
-   rdrv = devm_kzalloc(ci->dev, sizeof(struct ci_role_driver), GFP_KERNEL);
+   rdrv = devm_kzalloc(ci->dev, sizeof(*rdrv), GFP_KERNEL);
if (!rdrv)
return -ENOMEM;
 
-- 
1.9.1

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

[PATCH 2/3] usb: chipidea: udc: Fit into a single line

[Fabio Estevam]

From: Fabio Estevam 

No need to split the dma_pool_zalloc() line into two as it can
perfectly fit into a single line.

Signed-off-by: Fabio Estevam 
---
 drivers/usb/chipidea/udc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index f24d922..336793f 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -350,8 +350,7 @@ static int add_td_to_list(struct ci_hw_ep *hwep, struct 
ci_hw_req *hwreq,
if (node == NULL)
return -ENOMEM;
 
-   node->ptr = dma_pool_zalloc(hwep->td_pool, GFP_ATOMIC,
-  >dma);
+   node->ptr = dma_pool_zalloc(hwep->td_pool, GFP_ATOMIC, >dma);
if (node->ptr == NULL) {
kfree(node);
return -ENOMEM;
-- 
1.9.1

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

[PATCH 1/3] usb: chipidea: udc: Use dma_pool_zalloc()

[Fabio Estevam]

From: Fabio Estevam 

We can make the code simpler by using dma_pool_zalloc() instead
of calling dma_pool_zalloc() and then a memset().

Signed-off-by: Fabio Estevam 
---
 drivers/usb/chipidea/udc.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 0f692fc..f24d922 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1668,12 +1668,10 @@ static int init_eps(struct ci_hdrc *ci)
usb_ep_set_maxpacket_limit(>ep, (unsigned 
short)~0);
 
INIT_LIST_HEAD(>qh.queue);
-   hwep->qh.ptr = dma_pool_alloc(ci->qh_pool, GFP_KERNEL,
->qh.dma);
+   hwep->qh.ptr = dma_pool_zalloc(ci->qh_pool, GFP_KERNEL,
+  >qh.dma);
if (hwep->qh.ptr == NULL)
retval = -ENOMEM;
-   else
-   memset(hwep->qh.ptr, 0, sizeof(*hwep->qh.ptr));
 
/*
 * set up shorthands for ep0 out and in endpoints,
-- 
1.9.1

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > Arnd Bergmann  writes:
> > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > >> > If we do that, we have to put child devices of the dwc3 devices into
> > >> > the platform glue, and it also breaks those dwc3 devices that don't
> > >> > have a parent driver.
> > >> 
> > >> Well, this is easy to fix:
> > >> 
> > >> if (dwc->dev->parent) {
> > >> dwc->sysdev = dwc->dev->parent;
> > >> } else {
> > >> dev_info(dwc->dev, "Please provide a glue layer!\n");
> > >> dwc->sysdev = dwc->dev;
> > >> }
> > >
> > > I don't understand. Do you mean we should have an extra level of
> > > stacking and splitting "static struct platform_driver dwc3_driver"
> > > in two so instead of
> > >
> > >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> > >
> > > we do this?
> > >
> > >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> 
> > > "xhci" (usb_bus.dev)
> > 
> > no 
> > 
> > If we have a parent device, use that as sysdev, otherwise use self as
> > sysdev.
> 
> But there is often a parent device in DT, as the xhci device is
> attached to some internal bus that gets turned into a platform_device
> as well, so checking whether there is a parent will get the wrong
> device node.

>From my point, all platform and firmware information at dwc3 are
correct, so we don't need to change dwc3/core.c, only changing for
xhci-plat.c is ok.

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ed56bf9..fd57c0d 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
struct clk  *clk;
int ret;
int irq;
+   struct device *dev = >dev, *sysdev;
 
if (usb_disabled())
return -ENODEV;
@@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (irq < 0)
return -ENODEV;
 
+   if (dev->parent) {
+   sysdev = dev->parent;
+   } else {
+   sysdev = dev;
+   }
+
/* Try to set 64-bit DMA first */
if (WARN_ON(!pdev->dev.dma_mask))
/* Platform did not initialize dma_mask */
@@ -170,7 +177,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
return ret;
}
 
-   hcd = usb_create_hcd(driver, >dev, dev_name(>dev));
+   hcd = __usb_create_hcd(driver, sysdev, >dev,
+   dev_name(>dev), NULL);
if (!hcd)
return -ENOMEM;
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d2e3f65..563600b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
/* Did the HC die before the root hub was registered? */
if (HCD_DEAD(hcd))
usb_hc_died (hcd);  /* This time clean up */
-   usb_dev->dev.of_node = parent_dev->of_node;
+   usb_dev->dev.of_node = parent_dev->sysdev->of_node;
}
mutex_unlock(_bus_idr_lock);

At above changes, the root hub's of_node equals to xhci-hcd sysdev's
of_node, which is from firmware or from its parent (it is dwc3 core
device).

> 
> > > That sounds a bit clumsy for the sake of consistency with PCI.
> > > The advantage is that xhci can always use the grandparent device
> > > as sysdev whenever it isn't probed through PCI or firmware
> > > itself, but the purpose of the dwc3-glue is otherwise questionable.
> > >
> > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > > device when that is created from the PCI driver and checking for that
> > > with the device property interface instead? If it's "snps,dwc3"
> > > we use the device itself while for "snps,dwc3-pci", we use the parent?
> > 

For pci glue device, it is always the parent for dwc3 core device.
In your patch, you may not need to split pci or non-pci, just using
if (dev->parent).

> > Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
> 
> That would be incompatible with the USB binding, as the sysdev
> is assumed to be a USB host controller with #address-cells=<1>
> and #size-cells=<0> in order to hold the child devices, for
> example:
> 
> / {
>  omap_dwc3_1: omap_dwc3_1@4888 {
> compatible = "ti,dwc3";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> usb1: usb@4889 {
> compatible = "snps,dwc3";
> reg = <0x4889 0x17000>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = ,
>  ,

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

[Oliver Neukum]

On Thu, 2016-09-08 at 13:58 +0200, Wim Osterholt wrote:
> L.S.
> 
> up to vanilla kernel 4.7.3 I've seen no problems.
> On two different dekstops running vanilla kernel-4.8 I can force a
> crash by
> inserting an USB-modem that requires module cdc_acm.ko .
> (Strangely enough that doesn't happen on my laptop.)
> 
> The moment that cdc_acm loads (manually or automatically - the
> hardware needs
> to be present) a kernel oops occurs.
> The system remains responsive for a while, but a reboot is necessairy.
> If you try a neat shutdown, the system will hang forever after
> 'remounting fs
> read-only'. You'll need a power-down.
> 
> The oops tells things that I didn't all write down, but it says
> null pointer dereference at 0246

That is the important part. I am sorry, but without the oops
nobody can help you. Please capture it

dmesg > /root/oops.txt

and report it.

Regards
Oliver


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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 3:02:56 PM CEST Grygorii Strashko wrote:
> dwc3: probe()
> if (!>dev->of_node)
>  legacy case - hard-code DMA props
> dwc->sysdev = >dev;

The PCI case will fall into this too, as we almost never have an
->of_node pointer for a PCI device.

Do we actually have any legacy dwc3 users in Linux that are neither DT
nor PCI based? Maybe we can just skip that.

> else
> dev = >dev;
> do {
> if (is_device_dma_capable(dev)) {
> dwc->sysdev = dev;
> break;
> }
>dev = dev->parent;
> while (dev);
> ^this cycle can be limited in depth (2 for PCI)

Right, this could work by itself and looks generic enough.

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Grygorii Strashko]

On 09/08/2016 02:00 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Arnd Bergmann  writes:
>> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>>> Arnd Bergmann  writes:
 On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> If we do that, we have to put child devices of the dwc3 devices into
>> the platform glue, and it also breaks those dwc3 devices that don't
>> have a parent driver.
>
> Well, this is easy to fix:
>
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

 I don't understand. Do you mean we should have an extra level of
 stacking and splitting "static struct platform_driver dwc3_driver"
 in two so instead of

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

 we do this?

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
 (usb_bus.dev)
>>>
>>> no 
>>>
>>> If we have a parent device, use that as sysdev, otherwise use self as
>>> sysdev.
>>
>> But there is often a parent device in DT, as the xhci device is
>> attached to some internal bus that gets turned into a platform_device
>> as well, so checking whether there is a parent will get the wrong
>> device node.
> 
> oh, that makes things more interesting :-s
> 
 That sounds a bit clumsy for the sake of consistency with PCI.
 The advantage is that xhci can always use the grandparent device
 as sysdev whenever it isn't probed through PCI or firmware
 itself, but the purpose of the dwc3-glue is otherwise questionable.

 How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
 device when that is created from the PCI driver and checking for that
 with the device property interface instead? If it's "snps,dwc3"
 we use the device itself while for "snps,dwc3-pci", we use the parent?
>>>
>>> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>>
>> That would be incompatible with the USB binding, as the sysdev
>> is assumed to be a USB host controller with #address-cells=<1>
>> and #size-cells=<0> in order to hold the child devices, for
>> example:
>>
>> / {
>>  omap_dwc3_1: omap_dwc3_1@4888 {
>> compatible = "ti,dwc3";
>> #address-cells = <1>;
>> #size-cells = <1>;
>> ranges;
>> usb1: usb@4889 {
>> compatible = "snps,dwc3";
>> reg = <0x4889 0x17000>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> interrupts = ,
>>  ,
>>  ;
>> interrupt-names = "peripheral",
>>   "host",
>>   "otg";
>> phys = <_phy1>, <_phy1>;
>> phy-names = "usb2-phy", "usb3-phy";
>>
>> hub@1 {
>> compatible = "usb5e3,608";
>> reg = <1>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> ethernet@1 {
>> compatible = "usb424,ec00";
>> mac-address = [00 11 22 33 44 55];
>> reg = <1>;
>> };
>> };
>> };
>> };
>>
>> It's also the node that contains the "phys" properties and
>> presumably other properties like "otg-rev", "maximum-speed"
>> etc.
>>
>> If we make the sysdev point to the parent, then we can no longer
>> look up those properties and child devices from the USB core code
>> by looking at "sysdev->of_node".
> 
> this also makes things more interesting. I can't of anything other than
> having some type of flag passed via e.g. device_properties by dwc3-pci.c
> :-s
> 
> It's quite a hack, though. I still think that inheriting DMA (or
> manually initializing a child with parent's DMA bits and pieces) is the
> best way to go. So we're back to of_dma_configure() and
> acpi_dma_configure(), right?
> 
> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> that's easy, but for DT devices, seems like it should be in of
> core. Below is, clearly, not enough but should show the idea:
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad7c403..a54610198946 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> device_node *np)
>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>  * setup the correct supported mask.
>  */
> -   if (!dev->coherent_dma_mask)
> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);

crash by cdc_acm driver in kernels 4.8-rc1/5

[Wim Osterholt]

L.S.

up to vanilla kernel 4.7.3 I've seen no problems.
On two different dekstops running vanilla kernel-4.8 I can force a crash by
inserting an USB-modem that requires module cdc_acm.ko .
(Strangely enough that doesn't happen on my laptop.)

The moment that cdc_acm loads (manually or automatically - the hardware needs
to be present) a kernel oops occurs.
The system remains responsive for a while, but a reboot is necessairy.
If you try a neat shutdown, the system will hang forever after 'remounting fs
read-only'. You'll need a power-down.

The oops tells things that I didn't all write down, but it says
null pointer dereference at 0246
...
failed while handling devices/pci:00/:00:1d.3/usb7/7-1/7-1:1d  etc.
...
udevd .. is taking too long..

Could someone please explain and repair the magic that is happening here?

Thanks in advance, Wim Osterholt.


- w...@djo.tudelft.nl -

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

Re: [PATCH] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> When we change the USB function with configfs dynamically, we possibly met 
> this
> situation: one core is doing the control transfer, another core is trying to
> unregister the USB gadget from userspace, we must wait for completing this
> control tranfer, or it will hang the controller to set the DEVCTRLHLT flag.
>
> Also adding some disconnect checking to avoid queuing any requests when the
> gadget is stopped.
>
> Signed-off-by: Baolin Wang 
> ---
>  drivers/usb/dwc3/ep0.c|8 
>  drivers/usb/dwc3/gadget.c |   32 +++-
>  2 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index fe79d77..11519d7 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -228,6 +228,14 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct 
> usb_request *request,
>   int ret;
>  
>   spin_lock_irqsave(>lock, flags);
> + if (dwc->pullups_connected == false) {
> + dwc3_trace(trace_dwc3_ep0,
> + "queuing request %p to %s when gadget is disconnected",
> + request, dep->name);
> + ret = -ESHUTDOWN;
> + goto out;
> + }

this could go on its own patch, however we can use if
(!dwc->pullups_connected) instead.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1783406..bbac8f5 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1040,6 +1040,13 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
> struct dwc3_request *req)
>   struct dwc3 *dwc = dep->dwc;
>   int ret;
>  
> + if (dwc->pullups_connected == false) {
> + dwc3_trace(trace_dwc3_gadget,
> + "queuing request %p to %s when gadget is disconnected",
> + >request, dep->endpoint.name);
> + return -ESHUTDOWN;
> + }

ditto

> @@ -1434,6 +1441,13 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int 
> is_on, int suspend)
>   if (pm_runtime_suspended(dwc->dev))
>   return 0;
>  
> + /*
> +  * Per databook, when we want to stop the gadget, if a control transfer
> +  * is still in process, complete it and get the core into setup phase.
> +  */

Do you have the section reference for this? Which databook version are
you reading?

> + if (!is_on && dwc->ep0state != EP0_SETUP_PHASE)
> + return -EBUSY;
> +
>   reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>   if (is_on) {
>   if (dwc->revision <= DWC3_REVISION_187A) {
> @@ -1481,12 +1495,20 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, 
> int is_on)
>   struct dwc3 *dwc = gadget_to_dwc(g);
>   unsigned long   flags;
>   int ret;
> + int timeout = 500;
>  
>   is_on = !!is_on;
>  
> - spin_lock_irqsave(>lock, flags);
> - ret = dwc3_gadget_run_stop(dwc, is_on, false);
> - spin_unlock_irqrestore(>lock, flags);
> + do {
> + spin_lock_irqsave(>lock, flags);
> + ret = dwc3_gadget_run_stop(dwc, is_on, false);
> + spin_unlock_irqrestore(>lock, flags);
> +
> + if (ret != -EBUSY)
> + break;
> +
> + udelay(10);
> + } while (--timeout);

no, this is not a good idea at all. I'd rather see:

wait_event_timeout(dwc->wq, dwc->ep0state == EP0_SETUP_PHASE,
jiffies + msecs_to_jiffies(500));

or, perhaps, a wait_for_completion_timeout(). Then, ep0.c needs to call
complete() everytime Status Phase completes. That's probably a better
idea ;-)

> @@ -1990,7 +2012,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, 
> struct dwc3_ep *dep,
>* dwc3_gadget_giveback(). If that happens, we're just gonna return 1
>* early on so DWC3_EP_BUSY flag gets cleared
>*/
> - if (!dep->endpoint.desc)
> + if (!dep->endpoint.desc || dwc->pullups_connected == false)

is this really necessary? Can we really get pullups_connect set to false
with dep->endpoint.desc still valid?

> @@ -2064,7 +2086,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 
> *dwc,
>* dwc3_gadget_giveback(). If that happens, we're just gonna return 1
>* early on so DWC3_EP_BUSY flag gets cleared
>*/
> - if (!dep->endpoint.desc)
> + if (!dep->endpoint.desc || dwc->pullups_connected == false)

ditto

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> >> But this needs to be done before dwc3_probe() executes. For dwc3-pci
>> >> that's easy, but for DT devices, seems like it should be in of
>> >> core. Below is, clearly, not enough but should show the idea:
>> >> 
>> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> >> index fd5cfad7c403..a54610198946 100644
>> >> --- a/drivers/of/device.c
>> >> +++ b/drivers/of/device.c
>> >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
>> >> device_node *np)
>> >>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected 
>> >> to
>> >>  * setup the correct supported mask.
>> >>  */
>> >> -   if (!dev->coherent_dma_mask)
>> >> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> >> +   if (!dev->coherent_dma_mask) {
>> >> +   if (!dev->parent->coherent_dma_mask)
>> >> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> >> +   else
>> >> +   dev->coherent_dma_mask = 
>> >> dev->parent->coherent_dma_mask;
>> >> +   }
>> >>  
>> >
>> > As the comment above that code says, the default 32-bit mask is 
>> > intentional,
>> > and you need the driver to ask for the mask it wants using
>> > dma_set_mask_and_coherent(), while the platform code should be able to use
>> > dev->of_node to figure out whether that mask is supported.
>> >
>> > Just setting the initial mask to something else based on what the parent
>> > supports will not do the right thing elsewhere.
>> 
>> oh man, it gets more and more complex. Seems like either path we take
>> will cause problems somewhere 
>> 
>> If we make dwc3.ko a library which glue calls directly then all these
>> problems are solved but we break all current DTs and fall into the trap
>> of having another MUSB.
>
> I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3

well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to
look at possible children for their own quirks and properties.

> into a library without changing the DT representation. However the parts
> that I think would change are
>
> - The sysfs representation for dwc3-pci, as we would no longer have
>   a parent-child relationship there.

that's a no-brainer, I think

> - The power management handling might need a rework, since you currently
>   rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
>   power on and off

simple enough to do as well.

> - turning dwc3 into a library probably implies also turning xhci into
>   a library, in part for consistency.

yeah, I considered that too. We could still do it in parts, though.

> - if we don't do the whole usb_bus->sysdev thing, we need to not just
>   do this for dwc3 but also chipidea and maybe a couple of others.

MUSB comes to mind

> There should not be any show-stoppers here, but it's a lot of work.

I think the biggest work will making sure people don't abuse functions
just because they're now part of a single binary. Having them as
separate modules helped a lot reducing the maintenance overhead. There
was only one occasion where someone sent a glue layer which iterated
over its children to find struct dwc3 * from child's drvdata.

>> If we try to pass DMA bits from parent to child, then we have the fact
>> that DT ends up, in practice, always having a parent device.
>
> I don't understand what you mean here, but I agree that the various ways

well, we can't simply use what I pointed out a few emails back:

if (dwc->dev->parent)
dwc->sysdev = dwc->dev->parent
else
dwc->sysdev = dwc->dev

> we discussed for copying the DMA flags from one 'struct device' to another
> all turned out to be flawed in at least one way.
>
> Do you see any problems with the patch I posted other than the ugliness
> of the dwc3 and xhci drivers finding out which pointer to use for
> usb_bus->sysdev? If we can solve this, we shouldn't need any new
> of_dma_configure/acpi_dma_configure calls and we won't have to
> turn the drivers into a library, so maybe let's try to come up with
> better ideas for that sub-problem.

No big problems with that, no. Just the ifdef looking for a PCI bus in
the parent. How about passing a flag via device_properties? I don't
wanna change dwc3 core's device name with a platform_device_id because
there probably already are scripts relying on the names to enable
pm_runtime for example.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:20:58 PM CEST Felipe Balbi wrote:
> >> It's quite a hack, though. I still think that inheriting DMA (or
> >> manually initializing a child with parent's DMA bits and pieces) is the
> >> best way to go. So we're back to of_dma_configure() and
> >> acpi_dma_configure(), right?
> >
> > That won't solve the problems with the DT properties or the
> > dma configuration for PCI devices though.
> 
> acpi_dma_configure() is supposed to pass along DMA bits from PCI to
> child devices, no?

I don't know, haven't looked at that code.

> >> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> >> that's easy, but for DT devices, seems like it should be in of
> >> core. Below is, clearly, not enough but should show the idea:
> >> 
> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >> index fd5cfad7c403..a54610198946 100644
> >> --- a/drivers/of/device.c
> >> +++ b/drivers/of/device.c
> >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> >> device_node *np)
> >>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected 
> >> to
> >>  * setup the correct supported mask.
> >>  */
> >> -   if (!dev->coherent_dma_mask)
> >> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> >> +   if (!dev->coherent_dma_mask) {
> >> +   if (!dev->parent->coherent_dma_mask)
> >> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> >> +   else
> >> +   dev->coherent_dma_mask = 
> >> dev->parent->coherent_dma_mask;
> >> +   }
> >>  
> >
> > As the comment above that code says, the default 32-bit mask is intentional,
> > and you need the driver to ask for the mask it wants using
> > dma_set_mask_and_coherent(), while the platform code should be able to use
> > dev->of_node to figure out whether that mask is supported.
> >
> > Just setting the initial mask to something else based on what the parent
> > supports will not do the right thing elsewhere.
> 
> oh man, it gets more and more complex. Seems like either path we take
> will cause problems somewhere 
> 
> If we make dwc3.ko a library which glue calls directly then all these
> problems are solved but we break all current DTs and fall into the trap
> of having another MUSB.

I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3
into a library without changing the DT representation. However the parts
that I think would change are

- The sysfs representation for dwc3-pci, as we would no longer have
  a parent-child relationship there.
- The power management handling might need a rework, since you currently
  rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
  power on and off
- turning dwc3 into a library probably implies also turning xhci into
  a library, in part for consistency.
- if we don't do the whole usb_bus->sysdev thing, we need to not just
  do this for dwc3 but also chipidea and maybe a couple of others.

There should not be any show-stoppers here, but it's a lot of work.

> If we try to pass DMA bits from parent to child, then we have the fact
> that DT ends up, in practice, always having a parent device.

I don't understand what you mean here, but I agree that the various ways
we discussed for copying the DMA flags from one 'struct device' to another
all turned out to be flawed in at least one way.

Do you see any problems with the patch I posted other than the ugliness
of the dwc3 and xhci drivers finding out which pointer to use for
usb_bus->sysdev? If we can solve this, we shouldn't need any new
of_dma_configure/acpi_dma_configure calls and we won't have to
turn the drivers into a library, so maybe let's try to come up with
better ideas for that sub-problem.

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

[PATCH] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

[Baolin Wang]

When we change the USB function with configfs dynamically, we possibly met this
situation: one core is doing the control transfer, another core is trying to
unregister the USB gadget from userspace, we must wait for completing this
control tranfer, or it will hang the controller to set the DEVCTRLHLT flag.

Also adding some disconnect checking to avoid queuing any requests when the
gadget is stopped.

Signed-off-by: Baolin Wang 
---
 drivers/usb/dwc3/ep0.c|8 
 drivers/usb/dwc3/gadget.c |   32 +++-
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index fe79d77..11519d7 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -228,6 +228,14 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct 
usb_request *request,
int ret;
 
spin_lock_irqsave(>lock, flags);
+   if (dwc->pullups_connected == false) {
+   dwc3_trace(trace_dwc3_ep0,
+   "queuing request %p to %s when gadget is disconnected",
+   request, dep->name);
+   ret = -ESHUTDOWN;
+   goto out;
+   }
+
if (!dep->endpoint.desc) {
dwc3_trace(trace_dwc3_ep0,
"trying to queue request %p to disabled %s",
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1783406..bbac8f5 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1040,6 +1040,13 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
struct dwc3_request *req)
struct dwc3 *dwc = dep->dwc;
int ret;
 
+   if (dwc->pullups_connected == false) {
+   dwc3_trace(trace_dwc3_gadget,
+   "queuing request %p to %s when gadget is disconnected",
+   >request, dep->endpoint.name);
+   return -ESHUTDOWN;
+   }
+
if (!dep->endpoint.desc) {
dwc3_trace(trace_dwc3_gadget,
"trying to queue request %p to disabled %s",
@@ -1434,6 +1441,13 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int 
is_on, int suspend)
if (pm_runtime_suspended(dwc->dev))
return 0;
 
+   /*
+* Per databook, when we want to stop the gadget, if a control transfer
+* is still in process, complete it and get the core into setup phase.
+*/
+   if (!is_on && dwc->ep0state != EP0_SETUP_PHASE)
+   return -EBUSY;
+
reg = dwc3_readl(dwc->regs, DWC3_DCTL);
if (is_on) {
if (dwc->revision <= DWC3_REVISION_187A) {
@@ -1481,12 +1495,20 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int 
is_on)
struct dwc3 *dwc = gadget_to_dwc(g);
unsigned long   flags;
int ret;
+   int timeout = 500;
 
is_on = !!is_on;
 
-   spin_lock_irqsave(>lock, flags);
-   ret = dwc3_gadget_run_stop(dwc, is_on, false);
-   spin_unlock_irqrestore(>lock, flags);
+   do {
+   spin_lock_irqsave(>lock, flags);
+   ret = dwc3_gadget_run_stop(dwc, is_on, false);
+   spin_unlock_irqrestore(>lock, flags);
+
+   if (ret != -EBUSY)
+   break;
+
+   udelay(10);
+   } while (--timeout);
 
return ret;
 }
@@ -1990,7 +2012,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, 
struct dwc3_ep *dep,
 * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
 * early on so DWC3_EP_BUSY flag gets cleared
 */
-   if (!dep->endpoint.desc)
+   if (!dep->endpoint.desc || dwc->pullups_connected == false)
return 1;
 
if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
@@ -2064,7 +2086,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 
*dwc,
 * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
 * early on so DWC3_EP_BUSY flag gets cleared
 */
-   if (!dep->endpoint.desc)
+   if (!dep->endpoint.desc || dwc->pullups_connected == false)
return;
 
if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
-- 
1.7.9.5

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote:
>> Arnd Bergmann  writes:
>> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>> >> Arnd Bergmann  writes:
>> >> > That sounds a bit clumsy for the sake of consistency with PCI.
>> >> > The advantage is that xhci can always use the grandparent device
>> >> > as sysdev whenever it isn't probed through PCI or firmware
>> >> > itself, but the purpose of the dwc3-glue is otherwise questionable.
>> >> >
>> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
>> >> > device when that is created from the PCI driver and checking for that
>> >> > with the device property interface instead? If it's "snps,dwc3"
>> >> > we use the device itself while for "snps,dwc3-pci", we use the parent?
>> >> 
>> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>> >
>> > That would be incompatible with the USB binding, as the sysdev
>> > is assumed to be a USB host controller with #address-cells=<1>
>> > and #size-cells=<0> in order to hold the child devices, for
>> > example:
>> >
>> > / {
>> >  omap_dwc3_1: omap_dwc3_1@4888 {
>> > compatible = "ti,dwc3";
>> > #address-cells = <1>;
>> > #size-cells = <1>;
>> > ranges;
>> > usb1: usb@4889 {
>> > compatible = "snps,dwc3";
>> > reg = <0x4889 0x17000>;
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>> > interrupts = ,
>> >  ,
>> >  ;
>> > interrupt-names = "peripheral",
>> >   "host",
>> >   "otg";
>> > phys = <_phy1>, <_phy1>;
>> > phy-names = "usb2-phy", "usb3-phy";
>> >
>> > hub@1 {
>> > compatible = "usb5e3,608";
>> > reg = <1>;
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>> >
>> > ethernet@1 {
>> > compatible = "usb424,ec00";
>> > mac-address = [00 11 22 33 44 55];
>> > reg = <1>;
>> > };
>> > };
>> > };
>> > };
>> >
>> > It's also the node that contains the "phys" properties and
>> > presumably other properties like "otg-rev", "maximum-speed"
>> > etc.
>> >
>> > If we make the sysdev point to the parent, then we can no longer
>> > look up those properties and child devices from the USB core code
>> > by looking at "sysdev->of_node".
>> 
>> this also makes things more interesting. I can't of anything other than
>> having some type of flag passed via e.g. device_properties by dwc3-pci.c
>> :-s
>
> Ok.

man, I have been skipping words rather frequently when typing lately. I
meant "I can't THINK of anything other "

>> It's quite a hack, though. I still think that inheriting DMA (or
>> manually initializing a child with parent's DMA bits and pieces) is the
>> best way to go. So we're back to of_dma_configure() and
>> acpi_dma_configure(), right?
>
> That won't solve the problems with the DT properties or the
> dma configuration for PCI devices though.

acpi_dma_configure() is supposed to pass along DMA bits from PCI to
child devices, no?

>> But this needs to be done before dwc3_probe() executes. For dwc3-pci
>> that's easy, but for DT devices, seems like it should be in of
>> core. Below is, clearly, not enough but should show the idea:
>> 
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index fd5cfad7c403..a54610198946 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
>> device_node *np)
>>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>>  * setup the correct supported mask.
>>  */
>> -   if (!dev->coherent_dma_mask)
>> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +   if (!dev->coherent_dma_mask) {
>> +   if (!dev->parent->coherent_dma_mask)
>> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +   else
>> +   dev->coherent_dma_mask = 
>> dev->parent->coherent_dma_mask;
>> +   }
>>  
>
> As the comment above that code says, the default 32-bit mask is intentional,
> and you need the driver to ask for the mask it wants using
> dma_set_mask_and_coherent(), while the platform code should be able to use
> dev->of_node to figure out whether that mask is supported.
>
> Just setting the initial mask to something else based on what the parent
> supports will not do the right thing elsewhere.

oh man, it gets more and more complex. Seems like either path we take
will 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >> Arnd Bergmann  writes:
> >> > That sounds a bit clumsy for the sake of consistency with PCI.
> >> > The advantage is that xhci can always use the grandparent device
> >> > as sysdev whenever it isn't probed through PCI or firmware
> >> > itself, but the purpose of the dwc3-glue is otherwise questionable.
> >> >
> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> >> > device when that is created from the PCI driver and checking for that
> >> > with the device property interface instead? If it's "snps,dwc3"
> >> > we use the device itself while for "snps,dwc3-pci", we use the parent?
> >> 
> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
> >
> > That would be incompatible with the USB binding, as the sysdev
> > is assumed to be a USB host controller with #address-cells=<1>
> > and #size-cells=<0> in order to hold the child devices, for
> > example:
> >
> > / {
> >  omap_dwc3_1: omap_dwc3_1@4888 {
> > compatible = "ti,dwc3";
> > #address-cells = <1>;
> > #size-cells = <1>;
> > ranges;
> > usb1: usb@4889 {
> > compatible = "snps,dwc3";
> > reg = <0x4889 0x17000>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> > interrupts = ,
> >  ,
> >  ;
> > interrupt-names = "peripheral",
> >   "host",
> >   "otg";
> > phys = <_phy1>, <_phy1>;
> > phy-names = "usb2-phy", "usb3-phy";
> >
> > hub@1 {
> > compatible = "usb5e3,608";
> > reg = <1>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > ethernet@1 {
> > compatible = "usb424,ec00";
> > mac-address = [00 11 22 33 44 55];
> > reg = <1>;
> > };
> > };
> > };
> > };
> >
> > It's also the node that contains the "phys" properties and
> > presumably other properties like "otg-rev", "maximum-speed"
> > etc.
> >
> > If we make the sysdev point to the parent, then we can no longer
> > look up those properties and child devices from the USB core code
> > by looking at "sysdev->of_node".
> 
> this also makes things more interesting. I can't of anything other than
> having some type of flag passed via e.g. device_properties by dwc3-pci.c
> :-s

Ok.

> It's quite a hack, though. I still think that inheriting DMA (or
> manually initializing a child with parent's DMA bits and pieces) is the
> best way to go. So we're back to of_dma_configure() and
> acpi_dma_configure(), right?

That won't solve the problems with the DT properties or the
dma configuration for PCI devices though.

> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> that's easy, but for DT devices, seems like it should be in of
> core. Below is, clearly, not enough but should show the idea:
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad7c403..a54610198946 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> device_node *np)
>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>  * setup the correct supported mask.
>  */
> -   if (!dev->coherent_dma_mask)
> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +   if (!dev->coherent_dma_mask) {
> +   if (!dev->parent->coherent_dma_mask)
> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +   else
> +   dev->coherent_dma_mask = 
> dev->parent->coherent_dma_mask;
> +   }
>  

As the comment above that code says, the default 32-bit mask is intentional,
and you need the driver to ask for the mask it wants using
dma_set_mask_and_coherent(), while the platform code should be able to use
dev->of_node to figure out whether that mask is supported.

Just setting the initial mask to something else based on what the parent
supports will not do the right thing elsewhere.

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

Re: [PATCH 1/3] usb: gadgetfs: introduce feature control mechanism

[Felipe Balbi]

Hi,

Alan Stern  writes:
>> Alan Stern  writes:
>> > On Wed, 7 Sep 2016, Felipe Balbi wrote:
>> >
>> >> > --- a/include/uapi/linux/usb/gadgetfs.h
>> >> > +++ b/include/uapi/linux/usb/gadgetfs.h
>> >> > @@ -85,4 +85,33 @@ struct usb_gadgetfs_event {
>> >> >   */
>> >> >  #defineGADGETFS_CLEAR_HALT_IO('g', 3)
>> >> >  
>> >> > +
>> >> > +
>> >> > +struct usb_gadgetfs_features {
>> >> > + uint64_t bitmap[4];
>> >> 
>> >> this should really be:
>> >> 
>> >>   uint64_t features0;
>> >> 
>> >> /* Keep 24 bytes reserved for possible future usage */
>> >> uint8_t RESERVED[24];
>> >
>> > Isn't this really extreme overkill?  I would be surprised if gadgetfs 
>> > ever supports more than 10 features.
>> 
>> I'm hoping to be extreme overkill :-) I really don't wanna get into a
>> situation where we run out of bits. What if some features require
>> arguments in the future? We could just pass them here, right?
>> 
>> Frankly, I'm afraid of what happened to i2c-dev :-) We can't change
>> (easily) i2c subsystem's write() to return the amount of bytes written
>> because its userspace facing ABI has no reserved bits we could use to
>> return that value. Changing that would be a huge amount of work.
>> 
>> What do you think?
>
> Well, I don't care about it all that much, especially since I don't 
> think you'll ever need to use it.  :-)
>
> Still, there are approaches that have been used in the past when people
> wanted to create an extensible interface.  Passing a variable-length
> structure that has a length field at a reserved spot near the beginning
> (perhaps along with one or two other reserved entries), for example.  

something along the like below?


/* better names appreciated */
struct usb_gadgetfs_packet {
uint16_t length;
uint8_t reserved[2];

uint8_t *data; /* dynamically allocated by userspace */
}

I can work with that no problem.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/1] usb: dwc3: fix Clear Stall EP command failure

[Felipe Balbi]

Hi,

Lu Baolu  writes:
>> Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
>> Stall EP command") sets ClearPendIN bit for all IN endpoints in
>> v2.60a+ cores no matter which speed mode they're in. This causes
>> Clear Stall EP command failing on some Intel devices.
>>
>> In page 539 of 2.60a specification, it says:
>>
>> "When issuing Clear Stall command for IN endpoints in SuperSpeed
>> mode, the software must set the "ClearPendIN" bit to '1' to
>> clear any pending IN transcations, so that the device does not
>> expect any ACK TP from the host for the data sent earlier."
>>
>> It's obviously that we only need to apply this rule to those IN
>> endpoints in SuperSpeed mode.
>>
>> Fixes: 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear Stall EP 
>> command")
>> Cc: sta...@vger.kernel.org # 4.7+
>> Signed-off-by: Lu Baolu 
>> ---
>>  drivers/usb/dwc3/gadget.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 7a8d3d8..f1858d6 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -348,7 +348,8 @@ static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep 
>> *dep)
>>   * IN transfers due to a mishandled error condition. Synopsys
>>   * STAR 9000614252.
>>   */
>> -if (dep->direction && (dwc->revision >= DWC3_REVISION_260A))
>> +if (dep->direction && (dwc->revision >= DWC3_REVISION_260A) &&
>> +(dwc->maximum_speed >= USB_SPEED_SUPER))
>>  cmd |= DWC3_DEPCMD_CLEARPENDIN;
>>  
>>  memset(, 0, sizeof(params));
>
> Oh, my bad.
>
> I'm sending you a patch which hasn't been verified on the real
> hardware yet.
>
> My mind goes blank after a whole day's work. Poor me. :-(

happens to all of us ;-)

> I will send you a refreshed one tomorrow. Really sorry about it.

no problem, thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>> Arnd Bergmann  writes:
>> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> >> > If we do that, we have to put child devices of the dwc3 devices into
>> >> > the platform glue, and it also breaks those dwc3 devices that don't
>> >> > have a parent driver.
>> >> 
>> >> Well, this is easy to fix:
>> >> 
>> >> if (dwc->dev->parent) {
>> >> dwc->sysdev = dwc->dev->parent;
>> >> } else {
>> >> dev_info(dwc->dev, "Please provide a glue layer!\n");
>> >> dwc->sysdev = dwc->dev;
>> >> }
>> >
>> > I don't understand. Do you mean we should have an extra level of
>> > stacking and splitting "static struct platform_driver dwc3_driver"
>> > in two so instead of
>> >
>> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
>> >
>> > we do this?
>> >
>> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
>> > (usb_bus.dev)
>> 
>> no 
>> 
>> If we have a parent device, use that as sysdev, otherwise use self as
>> sysdev.
>
> But there is often a parent device in DT, as the xhci device is
> attached to some internal bus that gets turned into a platform_device
> as well, so checking whether there is a parent will get the wrong
> device node.

oh, that makes things more interesting :-s

>> > That sounds a bit clumsy for the sake of consistency with PCI.
>> > The advantage is that xhci can always use the grandparent device
>> > as sysdev whenever it isn't probed through PCI or firmware
>> > itself, but the purpose of the dwc3-glue is otherwise questionable.
>> >
>> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
>> > device when that is created from the PCI driver and checking for that
>> > with the device property interface instead? If it's "snps,dwc3"
>> > we use the device itself while for "snps,dwc3-pci", we use the parent?
>> 
>> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>
> That would be incompatible with the USB binding, as the sysdev
> is assumed to be a USB host controller with #address-cells=<1>
> and #size-cells=<0> in order to hold the child devices, for
> example:
>
> / {
>  omap_dwc3_1: omap_dwc3_1@4888 {
> compatible = "ti,dwc3";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> usb1: usb@4889 {
> compatible = "snps,dwc3";
> reg = <0x4889 0x17000>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = ,
>  ,
>  ;
> interrupt-names = "peripheral",
>   "host",
>   "otg";
> phys = <_phy1>, <_phy1>;
> phy-names = "usb2-phy", "usb3-phy";
>
> hub@1 {
> compatible = "usb5e3,608";
> reg = <1>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> ethernet@1 {
> compatible = "usb424,ec00";
> mac-address = [00 11 22 33 44 55];
> reg = <1>;
> };
> };
> };
> };
>
> It's also the node that contains the "phys" properties and
> presumably other properties like "otg-rev", "maximum-speed"
> etc.
>
> If we make the sysdev point to the parent, then we can no longer
> look up those properties and child devices from the USB core code
> by looking at "sysdev->of_node".

this also makes things more interesting. I can't of anything other than
having some type of flag passed via e.g. device_properties by dwc3-pci.c
:-s

It's quite a hack, though. I still think that inheriting DMA (or
manually initializing a child with parent's DMA bits and pieces) is the
best way to go. So we're back to of_dma_configure() and
acpi_dma_configure(), right?

But this needs to be done before dwc3_probe() executes. For dwc3-pci
that's easy, but for DT devices, seems like it should be in of
core. Below is, clearly, not enough but should show the idea:

diff --git a/drivers/of/device.c b/drivers/of/device.c
index fd5cfad7c403..a54610198946 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct device_node 
*np)
 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
 * setup the correct supported mask.
 */
-   if (!dev->coherent_dma_mask)
-   dev->coherent_dma_mask = DMA_BIT_MASK(32);
+   if (!dev->coherent_dma_mask) {
+   if (!dev->parent->coherent_dma_mask)
+   dev->coherent_dma_mask = DMA_BIT_MASK(32);
+  

Re: [PATCH] scsi: introduce a quirk for false cache reporting

[Oliver Neukum]

On Thu, 2016-08-18 at 22:19 -0400, Martin K. Petersen wrote:
> > "Oliver" == Oliver Neukum  writes:
> 
> Oliver> Some SATA to USB bridges fail to cooperate with some drives
> Oliver> resulting in no cache being present being reported to the
> Oliver> host. That causes the host to skip sending a command to
> Oliver> synchronize caches. That causes data loss when the drive is
> Oliver> powered down.
> 
> Reviewed-by: Martin K. Petersen 
> 

Hi,

may I ask about the fate of this patch? Has it been lost?
Should I resubmit? Are any further changes necessary?

Regards
Oliver


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

Re: TCPCI driver issue

[Heikki Krogerus]

Hi Steve,

On Wed, Sep 07, 2016 at 10:00:28PM -0400, Steve Schefter wrote:
> Hi Heikki.
> 
> I'm seeing an issue with the USB-C TCPCI driver.  On startup, I get a panic
> with the following error and stack dump.
> 
> Unable to handle kernel NULL pointer dereference at virtual address 002c
> 
> [] tcpm_register_port+0x228/0x3b0
> [] tcpci_probe+0x1c8/0x1fc
> [] i2c_device_probe+0x12c/0x160
> 
> The problem seems to stem from the fact that tcpci->tcpc.config is not
> initialized in the TCPCI driver.  It is NULL due to tcpci being allocated
> using devm_kzalloc().  This causes a problem when tcpm_register_port()
> attempts to access port->tcpc->config->type.
> 
> Am I missing something?  I understand that this is a work in progress, but
> it looks like this should happen for everyone.  Some I'm wondering if I'm
> missing something dumb.

I believe you are trying Guenter's (CCd) Type-C Port Manager series.
He is the correct best person to answer questions about that. But that
series is RFC, so you can't really expect everything to be in place.
It is more like proof of concept at this point.

But we probable need to talk about how to extract the platform
specific details needed for the config in any case (and also what
exactly is needed) at one point. I think we can get all of them with
device properties regardless of the system (ACPI/DT/whatever), so
we would just need to agree on the what the properties are.

Also adding the list.


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: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> >> > If we do that, we have to put child devices of the dwc3 devices into
> >> > the platform glue, and it also breaks those dwc3 devices that don't
> >> > have a parent driver.
> >> 
> >> Well, this is easy to fix:
> >> 
> >> if (dwc->dev->parent) {
> >> dwc->sysdev = dwc->dev->parent;
> >> } else {
> >> dev_info(dwc->dev, "Please provide a glue layer!\n");
> >> dwc->sysdev = dwc->dev;
> >> }
> >
> > I don't understand. Do you mean we should have an extra level of
> > stacking and splitting "static struct platform_driver dwc3_driver"
> > in two so instead of
> >
> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> >
> > we do this?
> >
> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
> > (usb_bus.dev)
> 
> no 
> 
> If we have a parent device, use that as sysdev, otherwise use self as
> sysdev.

But there is often a parent device in DT, as the xhci device is
attached to some internal bus that gets turned into a platform_device
as well, so checking whether there is a parent will get the wrong
device node.

> > That sounds a bit clumsy for the sake of consistency with PCI.
> > The advantage is that xhci can always use the grandparent device
> > as sysdev whenever it isn't probed through PCI or firmware
> > itself, but the purpose of the dwc3-glue is otherwise questionable.
> >
> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > device when that is created from the PCI driver and checking for that
> > with the device property interface instead? If it's "snps,dwc3"
> > we use the device itself while for "snps,dwc3-pci", we use the parent?
> 
> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?

That would be incompatible with the USB binding, as the sysdev
is assumed to be a USB host controller with #address-cells=<1>
and #size-cells=<0> in order to hold the child devices, for
example:

/ {
 omap_dwc3_1: omap_dwc3_1@4888 {
compatible = "ti,dwc3";
#address-cells = <1>;
#size-cells = <1>;
ranges;
usb1: usb@4889 {
compatible = "snps,dwc3";
reg = <0x4889 0x17000>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = ,
 ,
 ;
interrupt-names = "peripheral",
  "host",
  "otg";
phys = <_phy1>, <_phy1>;
phy-names = "usb2-phy", "usb3-phy";

hub@1 {
compatible = "usb5e3,608";
reg = <1>;
#address-cells = <1>;
#size-cells = <0>;

ethernet@1 {
compatible = "usb424,ec00";
mac-address = [00 11 22 33 44 55];
reg = <1>;
};
};
};
};

It's also the node that contains the "phys" properties and
presumably other properties like "otg-rev", "maximum-speed"
etc.

If we make the sysdev point to the parent, then we can no longer
look up those properties and child devices from the USB core code
by looking at "sysdev->of_node".

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

*** MICROSOFT FINAL WARNING ***

[Muhammad Fadzli Bin Ali]

Outlook
A few of your incoming mails were placed on pending status due to the recent 
upgrade on our database. In order to receive your messages,
click on the link below to login and wait for response from Webmail.
CLICK HERE
We apologise for any inconvenience and appreciate your understanding.
Thank You.
Copyright © 2016 Webmail .Inc . All rights reserved.
--
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 1/1] usb: dwc3: fix Clear Stall EP command failure

[Felipe Balbi]

Hi,

Lu Baolu  writes:
> Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
> Stall EP command") sets ClearPendIN bit for all IN endpoints in
> v2.60a+ cores no matter which speed mode they're in. This causes
> Clear Stall EP command failing on some Intel devices.

Well, the fact that is fails on some Intel device is, IMO, a
symptom. You should have said that ClearStall command fails on 2.60+
operating in HighSpeed mode. That's the real failure. It doesn't matter
if it's Intel or not, what matters is the *current* speed.

> In page 539 of 2.60a specification, it says:
>
> "When issuing Clear Stall command for IN endpoints in SuperSpeed
> mode, the software must set the "ClearPendIN" bit to '1' to
> clear any pending IN transcations, so that the device does not
> expect any ACK TP from the host for the data sent earlier."
>
> It's obviously that we only need to apply this rule to those IN

s/obviously/obvious/

> endpoints in SuperSpeed mode.

endpoints currently operating in SuperSpeed mode.

> Fixes: 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear Stall EP 
> command")
> Cc: sta...@vger.kernel.org # 4.7+

please use proper email format:

Cc:  # v4.7+

> Signed-off-by: Lu Baolu 
> ---
>  drivers/usb/dwc3/gadget.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 7a8d3d8..f1858d6 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -348,7 +348,8 @@ static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep 
> *dep)
>* IN transfers due to a mishandled error condition. Synopsys
>* STAR 9000614252.
>*/
> - if (dep->direction && (dwc->revision >= DWC3_REVISION_260A))
> + if (dep->direction && (dwc->revision >= DWC3_REVISION_260A) &&
> + (dwc->maximum_speed >= USB_SPEED_SUPER))

not sure this is correct. Based on text quoted above, it seems to be me
we should check *current* speed, not maximum speed. We hold current
speed in dwc->gadget.speed.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/1] usb: dwc3: fix Clear Stall EP command failure

[Lu Baolu]

Hi Felipe,

On 09/08/2016 04:41 PM, Lu Baolu wrote:
> Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
> Stall EP command") sets ClearPendIN bit for all IN endpoints in
> v2.60a+ cores no matter which speed mode they're in. This causes
> Clear Stall EP command failing on some Intel devices.
>
> In page 539 of 2.60a specification, it says:
>
> "When issuing Clear Stall command for IN endpoints in SuperSpeed
> mode, the software must set the "ClearPendIN" bit to '1' to
> clear any pending IN transcations, so that the device does not
> expect any ACK TP from the host for the data sent earlier."
>
> It's obviously that we only need to apply this rule to those IN
> endpoints in SuperSpeed mode.
>
> Fixes: 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear Stall EP 
> command")
> Cc: sta...@vger.kernel.org # 4.7+
> Signed-off-by: Lu Baolu 
> ---
>  drivers/usb/dwc3/gadget.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 7a8d3d8..f1858d6 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -348,7 +348,8 @@ static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep 
> *dep)
>* IN transfers due to a mishandled error condition. Synopsys
>* STAR 9000614252.
>*/
> - if (dep->direction && (dwc->revision >= DWC3_REVISION_260A))
> + if (dep->direction && (dwc->revision >= DWC3_REVISION_260A) &&
> + (dwc->maximum_speed >= USB_SPEED_SUPER))
>   cmd |= DWC3_DEPCMD_CLEARPENDIN;
>  
>   memset(, 0, sizeof(params));

Oh, my bad.

I'm sending you a patch which hasn't been verified on the real
hardware yet.

My mind goes blank after a whole day's work. Poor me. :-(

I will send you a refreshed one tomorrow. Really sorry about it.

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> > If we do that, we have to put child devices of the dwc3 devices into
>> > the platform glue, and it also breaks those dwc3 devices that don't
>> > have a parent driver.
>> 
>> Well, this is easy to fix:
>> 
>> if (dwc->dev->parent) {
>> dwc->sysdev = dwc->dev->parent;
>> } else {
>> dev_info(dwc->dev, "Please provide a glue layer!\n");
>> dwc->sysdev = dwc->dev;
>> }
>
> I don't understand. Do you mean we should have an extra level of
> stacking and splitting "static struct platform_driver dwc3_driver"
> in two so instead of
>
>   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
>
> we do this?
>
>   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
> (usb_bus.dev)

no :-)

If we have a parent device, use that as sysdev, otherwise use self as
sysdev.

> That sounds a bit clumsy for the sake of consistency with PCI.
> The advantage is that xhci can always use the grandparent device
> as sysdev whenever it isn't probed through PCI or firmware
> itself, but the purpose of the dwc3-glue is otherwise questionable.
>
> How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> device when that is created from the PCI driver and checking for that
> with the device property interface instead? If it's "snps,dwc3"
> we use the device itself while for "snps,dwc3-pci", we use the parent?

Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?

-- 
balbi


signature.asc
Description: PGP signature

[PATCH v1 0/1] usb: gadget: f_fs: Stop ffs_closed NULL pointers

[Harish Jenny K N]

This patchset fixes the crash in ffs_closed during a disconnect 
of a USB composite FFS session. The issue was caused by the use 
of an outdated pointer in ffs_closed which had been deleted and 
not Nulled.

Note: Testing has only been done on an ARM i.MX6 based platform.

Jim Baxter (1):
  usb: gadget: f_fs: Stop ffs_closed NULL pointers.

 drivers/usb/gadget/function/f_fs.c |5 +
 1 file changed, 5 insertions(+)

-- 
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 v1 1/1] usb: gadget: f_fs: Stop ffs_closed NULL pointers.

[Harish Jenny K N]

From: Jim Baxter 

The struct ffs_data::private_data has a pointer to
ffs_dev stored in it during the ffs_fs_mount() function
however it is not cleared when the ffs_dev is freed
later which causes the ffs_closed function to crash
with "Unable to handle kernel NULL pointer dereference"
error when using the data in ffs_data::private_data.

This clears this pointer during the ffs_free_dev clean
up function.

Signed-off-by: Jim Baxter 
Signed-off-by: Jiada Wang 
Signed-off-by: Harish Jenny K N 
---
 drivers/usb/gadget/function/f_fs.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/function/f_fs.c 
b/drivers/usb/gadget/function/f_fs.c
index 5c8429f..b309650 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -3470,6 +3470,11 @@ static void _ffs_free_dev(struct ffs_dev *dev)
list_del(>entry);
if (dev->name_allocated)
kfree(dev->name);
+
+   /* Clear the private_data pointer to stop incorrect dev access */
+   if (dev->ffs_data)
+   dev->ffs_data->private_data = NULL;
+
kfree(dev);
if (list_empty(_devices))
functionfs_cleanup();
-- 
1.7.9.5


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

Re: [PATCH v2 1/1] usb: xhci: fix return value of xhci_setup_device()

[Lu Baolu]

Hi Greg,

On 09/08/2016 04:58 PM, Greg KH wrote:
> On Thu, Sep 08, 2016 at 03:29:25PM +0800, Lu Baolu wrote:
>> Hi Greg,
>>
>> On 09/08/2016 02:38 PM, Greg KH wrote:
>>> On Thu, Sep 08, 2016 at 08:41:02AM +0800, Lu Baolu wrote:
 xhci_setup_device() should return failure with correct error number
 when xhci host has died, removed or halted.

 Cc: sta...@vger.kernel.org # 4.3+
>>> Why is this a stable kernel issue?  What bug does it fix that affects
>>> users?
>> During usb device enumeration, if xhci host is not accessible (died,
>> removed or halted), the hc_driver->address_device() should return
>> a corresponding error code to usb core. But current xhci driver just
>> returns success. This will mislead usb core to continue enumeration:
>> reading device descriptor, which will result in failure, and users will
>> get a misleading message like "device descriptor read/8, error -110".
> Why didn't you include this in the changelog text so that it is obvious
> why this patch is needed?
>
> Don't just describe what the patch does, we can read C code, describe
> _why_ it is needed.

Yes. Thank you for the guidance. I will do it in a v3 patch.

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

Re: [PATCH v2 1/1] usb: xhci: fix return value of xhci_setup_device()

[Greg KH]

On Thu, Sep 08, 2016 at 03:29:25PM +0800, Lu Baolu wrote:
> Hi Greg,
> 
> On 09/08/2016 02:38 PM, Greg KH wrote:
> > On Thu, Sep 08, 2016 at 08:41:02AM +0800, Lu Baolu wrote:
> >> xhci_setup_device() should return failure with correct error number
> >> when xhci host has died, removed or halted.
> >>
> >> Cc: sta...@vger.kernel.org # 4.3+
> > Why is this a stable kernel issue?  What bug does it fix that affects
> > users?
> 
> During usb device enumeration, if xhci host is not accessible (died,
> removed or halted), the hc_driver->address_device() should return
> a corresponding error code to usb core. But current xhci driver just
> returns success. This will mislead usb core to continue enumeration:
> reading device descriptor, which will result in failure, and users will
> get a misleading message like "device descriptor read/8, error -110".

Why didn't you include this in the changelog text so that it is obvious
why this patch is needed?

Don't just describe what the patch does, we can read C code, describe
_why_ it is needed.

thanks,

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > If we do that, we have to put child devices of the dwc3 devices into
> > the platform glue, and it also breaks those dwc3 devices that don't
> > have a parent driver.
> 
> Well, this is easy to fix:
> 
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

I don't understand. Do you mean we should have an extra level of
stacking and splitting "static struct platform_driver dwc3_driver"
in two so instead of

"qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

we do this?

"qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
(usb_bus.dev)

That sounds a bit clumsy for the sake of consistency with PCI.
The advantage is that xhci can always use the grandparent device
as sysdev whenever it isn't probed through PCI or firmware
itself, but the purpose of the dwc3-glue is otherwise questionable.

How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
device when that is created from the PCI driver and checking for that
with the device property interface instead? If it's "snps,dwc3"
we use the device itself while for "snps,dwc3-pci", we use the parent?

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

[PATCH 1/1] usb: dwc3: fix Clear Stall EP command failure

[Lu Baolu]

Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
Stall EP command") sets ClearPendIN bit for all IN endpoints in
v2.60a+ cores no matter which speed mode they're in. This causes
Clear Stall EP command failing on some Intel devices.

In page 539 of 2.60a specification, it says:

"When issuing Clear Stall command for IN endpoints in SuperSpeed
mode, the software must set the "ClearPendIN" bit to '1' to
clear any pending IN transcations, so that the device does not
expect any ACK TP from the host for the data sent earlier."

It's obviously that we only need to apply this rule to those IN
endpoints in SuperSpeed mode.

Fixes: 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear Stall EP 
command")
Cc: sta...@vger.kernel.org # 4.7+
Signed-off-by: Lu Baolu 
---
 drivers/usb/dwc3/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 7a8d3d8..f1858d6 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -348,7 +348,8 @@ static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep *dep)
 * IN transfers due to a mishandled error condition. Synopsys
 * STAR 9000614252.
 */
-   if (dep->direction && (dwc->revision >= DWC3_REVISION_260A))
+   if (dep->direction && (dwc->revision >= DWC3_REVISION_260A) &&
+   (dwc->maximum_speed >= USB_SPEED_SUPER))
cmd |= DWC3_DEPCMD_CLEARPENDIN;
 
memset(, 0, sizeof(params));
-- 
2.1.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 1/1] usb: dwc3: avoid setting ClearPendIn for Intel Cherry Trail devices

[Felipe Balbi]

Hi Baolu,

Lu Baolu  writes:
> Hi Felipe,
>
> On 09/07/2016 11:05 AM, Lu Baolu wrote:
>> Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
>> Stall EP command") causes Clear Stall EP command failure on Intel
>> Cherry Trail devices. This patch add a quirk to avoid setting this
>> bit for those Intel devices.
>>
>> Cc: sta...@vger.kernel.org # 4.7+
>> Signed-off-by: Lu Baolu 
>
> Please ignore this patch. It's not a quirk, but a bug.
> I will send you a fix soon. Sorry for disturbing.

that's okay, thanks for letting me know.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 
>> > *dwc)
>> >  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
>> >struct dwc3_event_buffer *evt)
>> >  {
>> > -  dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
>> > +  dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);
>> 
>> how about "dma_dev" instead? Is this used for anything other than DMA?
>
> The two other things we have discussed in this thread are:
>
> - connecting of_node pointers to usb_device structures for children
>   of sysdev->of_node. Note that this can happen even for PCI devices
>   in case you have a USB ethernet device hardwired to a PCI-USB bridge
>   and put the mac address in DT.
>
> - finding the PHY device for a HCD
>
> There might be others. Basically sysdev here is what the USB core code
> can use for looking up any kind of properties provided by the firmware.

fair enough

>> > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
>> >dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
>> >dwc->mem = mem;
>> >dwc->dev = dev;
>> > +#ifdef CONFIG_PCI
>> > +  /* TODO: or some other way of detecting this? */
>> > +  if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
>> > +  dwc->sysdev = dwc->dev->parent;
>> > +  else
>> > +#endif
>> > +  dwc->sysdev = dwc->dev;
>> 
>> Well, we can remove this ifdef and *always* use the parent. We will just
>> require that dwc3 users provide a glue layer. In that case, your check
>> becomes:
>> 
>>  if (dwc->dev->parent)
>>  dwc->sysdev = dwc->dev->parent;
>>  else
>>  dev_info(dwc->dev, "Please provide a glue layer!\n");
>
> If we do that, we have to put child devices of the dwc3 devices into
> the platform glue, and it also breaks those dwc3 devices that don't
> have a parent driver.

Well, this is easy to fix:

if (dwc->dev->parent) {
dwc->sysdev = dwc->dev->parent;
} else {
dev_info(dwc->dev, "Please provide a glue layer!\n");
dwc->sysdev = dwc->dev;
}

>> > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
>> > index 89a2f712fdfe..4d7439cb8cd8 100644
>> > --- a/drivers/usb/dwc3/dwc3-st.c
>> > +++ b/drivers/usb/dwc3/dwc3-st.c
>> > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev)
>> >if (IS_ERR(regmap))
>> >return PTR_ERR(regmap);
>> >  
>> > -  dma_set_coherent_mask(dev, dev->coherent_dma_mask);
>> 
>> so is this.
>> 
>> All in all, I like where you're going with this, we just need a matching
>> acpi_dma_configure() and problems will be sorted out.
>
> With this patch, I don't think we even need that any more, as the device
> that we use the dma-mapping API is the one that already gets configured
> correctly by the platform code for all cases: PCI, OF, ACPI and combinations
> of those.

sounds good to me

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 11:03:10 AM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> >> Arnd Bergmann  writes:
> just look at the history of the file, you'll see that an Intel employee
> was a maintainer of chipidea driver. Also:
> 
> $ git ls-files drivers/usb/chipidea/ | grep pci
> drivers/usb/chipidea/ci_hdrc_pci.c

Right, Peter pointed that one out too.

> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 35d092456bec..08db66c64c66 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -25,6 +25,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> 
> actually, we don't want the core to know what it's attached to.

Agreed. This was just a first draft and I couldn't come up with
a better way to detect the case in which the parent device is
probed from another HW bus and the child is not known to the
firmware.

> >  #include 
> >  #include 
> >  #include 
> > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 
> > *dwc)
> >  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
> > struct dwc3_event_buffer *evt)
> >  {
> > -   dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
> > +   dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);
> 
> how about "dma_dev" instead? Is this used for anything other than DMA?

The two other things we have discussed in this thread are:

- connecting of_node pointers to usb_device structures for children
  of sysdev->of_node. Note that this can happen even for PCI devices
  in case you have a USB ethernet device hardwired to a PCI-USB bridge
  and put the mac address in DT.

- finding the PHY device for a HCD

There might be others. Basically sysdev here is what the USB core code
can use for looking up any kind of properties provided by the firmware.

> > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
> > dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
> > dwc->mem = mem;
> > dwc->dev = dev;
> > +#ifdef CONFIG_PCI
> > +   /* TODO: or some other way of detecting this? */
> > +   if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
> > +   dwc->sysdev = dwc->dev->parent;
> > +   else
> > +#endif
> > +   dwc->sysdev = dwc->dev;
> 
> Well, we can remove this ifdef and *always* use the parent. We will just
> require that dwc3 users provide a glue layer. In that case, your check
> becomes:
> 
>   if (dwc->dev->parent)
>   dwc->sysdev = dwc->dev->parent;
>   else
>   dev_info(dwc->dev, "Please provide a glue layer!\n");

If we do that, we have to put child devices of the dwc3 devices into
the platform glue, and it also breaks those dwc3 devices that don't
have a parent driver.

> > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> > index 2f1fb7e7aa54..e27899bb5706 100644
> > --- a/drivers/usb/dwc3/dwc3-exynos.c
> > +++ b/drivers/usb/dwc3/dwc3-exynos.c
> > @@ -20,7 +20,6 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device 
> > *pdev)
> > if (!exynos)
> > return -ENOMEM;
> >  
> > -   /*
> > -* Right now device-tree probed devices don't get dma_mask set.
> > -* Since shared usb code relies on it, set it here for now.
> > -* Once we move to full device tree support this will vanish off.
> > -*/
> > -   ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > -   if (ret)
> > -   return ret;
> 
> this is a separate patch, right?

Yes, this is probably just a cleanup that we can apply regardless.
We have not needed this in a long time.

> > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
> > index 89a2f712fdfe..4d7439cb8cd8 100644
> > --- a/drivers/usb/dwc3/dwc3-st.c
> > +++ b/drivers/usb/dwc3/dwc3-st.c
> > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev)
> > if (IS_ERR(regmap))
> > return PTR_ERR(regmap);
> >  
> > -   dma_set_coherent_mask(dev, dev->coherent_dma_mask);
> 
> so is this.
> 
> All in all, I like where you're going with this, we just need a matching
> acpi_dma_configure() and problems will be sorted out.

With this patch, I don't think we even need that any more, as the device
that we use the dma-mapping API is the one that already gets configured
correctly by the platform code for all cases: PCI, OF, ACPI and combinations
of those.

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

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

On 8 September 2016 at 15:31, NeilBrown  wrote:
> On Thu, Sep 08 2016, Baolin Wang wrote:
>
>> Hi Neil,
>>
>> On 6 September 2016 at 13:40, NeilBrown  wrote:
>>> On Mon, Aug 29 2016, Baolin Wang wrote:
>>>
 Hi Felipe,

 On 11 August 2016 at 11:14, Baolin Wang  wrote:
> Hi Felipe,
>
> On 1 August 2016 at 15:09, Baolin Wang  wrote:
>> Currently the Linux kernel does not provide any standard integration of 
>> this
>> feature that integrates the USB subsystem with the system power 
>> regulation
>> provided by PMICs meaning that either vendors must add this in their 
>> kernels
>> or USB gadget devices based on Linux (such as mobile phones) may not 
>> behave
>> as they should. Thus provide a standard framework for doing this in 
>> kernel.
>>
>> Now introduce one user with wm831x_power to support and test the usb 
>> charger,
>> which is pending testing. Moreover there may be other potential users 
>> will use
>> it in future.
>
> Could you please apply this patchset into your 'next' branch if you
> have no comments about it? Thank you.

 Since there are no other comments about this patchset for a long time,
 could you please apply this patchset? Thanks.
>>>
>>
>> Sorry for the late reply.
>>
>>> Sorry, I should have replied earlier.  Tim Bird mentioned on the
>>> ksummit-discuss list that there was a frustration with this not making
>>> progress so I decided to contribute what I could now.
>>>
>>> I think this patch set is attempting to address an important problem
>>> that needs solving.  However I think it gets some key aspects wrong.
>>> Maybe they can get fixed up after the patchset is upstream, maybe they
>>> should be fixed first - I have no strong opinion on that.
>>>
>>> My main complaints involve the detection and handling of the different
>>> charger types - DCP, CDP, ACA etc.
>>> The big-picture requirement here that the PHY will detect the physical
>>> properties of the cable (e.g. resistance to ground on ID) and determine
>>> the type of charger expected.  This information must be communicated to
>>> the PMIC "power_supply" device so it can regulate the power being drawn
>>> through the cable.
>>>
>>> The first problem is that there are two different ways that the
>>> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
>>> are cable types in the 'extcon' subsystem, and they are power_supply
>>> types in the 'power_supply' subsystem.  This duplication is confusing.
>>> It is not caused by your patch set, but I believe your patchset needs to
>>> work with the duplication and I think it does so poorly.
>>
>> Now the usb charger will not get charger type from 'extcon' subsystem,
>> we get the charger type from 'power_supply' and calllback
>> 'get_charger_type' for users.
>
> I understand this.  I think it is wrong because, in general, the
> power_supply doesn't know what the charger_type is (it might know it is
> USB, but most don't know which sort of USB).  The PHY knows that, not
> the power_supply.

I don't think so. Now many platforms will detect the charger type by
PMIC hardware, and we can get the charger type by PMIC hardware
register. Then power supply driver can access the PMIC register to get
the charger type. Here USB charger just considers if the accessing the
PMIC register to get charger type is implemented in power supply, it
is optional depending on what your platform designed.

>>
>>>
>>> In my mind, the power_supply should *not* know about this distinction at
>>> all (except possibly as an advisor attribute simiarly to the current
>>> battery technology attribute).  The other types it knows of are "AC",
>>> "USB", and "BATTERY".  The contrast between these is quite different
>>> From the contrast between DCP, CDP, ACA, which, from the perspective of
>>> the power supply, are almost irrelevant.  Your patchset effectively
>>> examines the power_supply_type of one power_supply, and communicates it
>>> to another.  It isn't clear to me how the first power_supply gets the
>>> information, or what the relationship between the two power_supplies is
>>> meant to be.
>>
>> We just get the charger type from the power supply which can get the
>> charger type from register or something else,
>
> But that register would be part of the PHY, not part of the charger.
> Having the power_supply driver reading the PHY register might work for
> some hardware, but is not a general solution.

Not only PHY. Like I said, the charger type detection can be done by
PMIC hardware, power supply can get the charger type by accessing PMIC
registers.

>>and the usb charger did
>> nothing for this power supply. In some platform, the charger type is
>> get in power supply driver, thus we should link this power supply to
>> get the charger type when USB 

Re: [PATCH 1/1] usb: dwc3: avoid setting ClearPendIn for Intel Cherry Trail devices

[Lu Baolu]

Hi Felipe,

On 09/07/2016 11:05 AM, Lu Baolu wrote:
> Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
> Stall EP command") causes Clear Stall EP command failure on Intel
> Cherry Trail devices. This patch add a quirk to avoid setting this
> bit for those Intel devices.
>
> Cc: sta...@vger.kernel.org # 4.7+
> Signed-off-by: Lu Baolu 

Please ignore this patch. It's not a quirk, but a bug.
I will send you a fix soon. Sorry for disturbing.

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 9:15:36 AM CEST Peter Chen wrote:
> > 
> > Right, I was specifically talking about the code in chipidea here,
> > which I think is never used on the PCI bus, and how the current
> > code is broken. We can probably do better than of_dma_configure()
> > (see below), but it would be an improvement.
> 
> Chipidea is also used at PCI bus too, see drivers/usb/chipidea/ci_hdrc_pci.c
> 

Ok, I see.

The experimental patch I posted should actually handle this just fine,
as it simply assumes that dev->parent is the device used for the DMA
API in chipidea, and I think this holds true for both the PCI and the
DT based uses of this driver.

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> Arnd Bergmann  writes:
>> 
>> [...]
>> 
>> > Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
>> > I think we should replace 
>> >
>> > pdev->dev.dma_mask = dev->dma_mask;
>> > pdev->dev.dma_parms = dev->dma_parms;
>> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>> >
>> > with of_dma_configure(), which has the chance to configure more than
>> > just those three, as the dma API might look into different aspects:
>> >
>> > - iommu specific configuration
>> > - cache coherency information
>> > - bus type
>> > - dma offset
>> > - dma_map_ops pointer
>> >
>> > We try to handle everything in of_dma_configure() at configuration
>> > time, and that would be the place to add anything else that we might
>> > need in the future.
>> 
>> There are a couple problems with this:
>> 
>> 1) won't work for PCI-based systems.
>> 
>> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
>> platform (FPGA that appears like a PCI card to host PC)
>
> Right, I was specifically talking about the code in chipidea here,
> which I think is never used on the PCI bus, and how the current

just look at the history of the file, you'll see that an Intel employee
was a maintainer of chipidea driver. Also:

$ git ls-files drivers/usb/chipidea/ | grep pci
drivers/usb/chipidea/ci_hdrc_pci.c

> code is broken. We can probably do better than of_dma_configure()
> (see below), but it would be an improvement.
>
>> 2) not very robust solution
>> 
>> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because
>> that's not created by DT. The only reason why this works at all is
>> because of the default 32-bit dma mask thing :-) So, how is it any
>> different than copying 32-bit dma mask from parent?
>
> The idea here is that you pass in the parent of_node along with the child
> device pointer, so it would behave exactly like the parent already does.
> The difference is that it also handles all the other attributes besides the 
> mask.

Now we're talking :-) I like that. We just need a matching API for
ACPI/PCI-based systems.

> However, to summarize the discussion so far, I agree that
> of_dma_configure() is not the solution to these problems, and I think
> we can do much better:
>
> Splitting the usb_bus->controller field into the Linux-internal device
> (used for the sysfs hierarchy, for printks and for power management)
> and a new pointer (used for DMA, DT enumeration and phy lookup) probably
> covers all that we really need.
>
> I've prototyped it below, with the dwc3, xhci and chipidea changes
> together with the core changes. I've surely made mistakes there and
> don't expect it to work out of the box, but this should give an
> idea of how I think this can all be solved in the least invasive
> way.
>
> I noticed that the gadget interface already has a way to handle the
> DMA allocation by device, so I added that in as well.

yeah, I wanna use that :-)

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 35d092456bec..08db66c64c66 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

actually, we don't want the core to know what it's attached to.

>  #include 
>  #include 
>  #include 
> @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
>   struct dwc3_event_buffer *evt)
>  {
> - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
> + dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);

how about "dma_dev" instead? Is this used for anything other than DMA?

> @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
>   dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
>   dwc->mem = mem;
>   dwc->dev = dev;
> +#ifdef CONFIG_PCI
> + /* TODO: or some other way of detecting this? */
> + if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
> + dwc->sysdev = dwc->dev->parent;
> + else
> +#endif
> + dwc->sysdev = dwc->dev;

Well, we can remove this ifdef and *always* use the parent. We will just
require that dwc3 users provide a glue layer. In that case, your check
becomes:

if (dwc->dev->parent)
dwc->sysdev = dwc->dev->parent;
else
dev_info(dwc->dev, "Please provide a glue layer!\n");

> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index 2f1fb7e7aa54..e27899bb5706 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -20,7 +20,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device 
> *pdev)
>   if (!exynos)
>   return -ENOMEM;
>  
> - /*
> -  * 

Re: [PATCH net-next 0/3] r8152: configuration setting

[Bjørn Mork]

Hayes Wang  writes:

> Bjørn Mork [mailto:bj...@mork.no]
>> Sent: Wednesday, September 07, 2016 9:51 PM
> [...]
>> So this adds a lot of code to work around the issues you introduced by
>> unnecessarily blacklisting the CDC ECM configuration earlier, and still
>> makes the r8152 driver handle the device even in ECM mode.
>
> I suggest to use vendor mode only, but some people ask me to
> submit such patches. If these patches are rejected, I have
> enough reasons to tell them it is unacceptable rather than
> I don't do it.
>
>> Just remove the completely unnecessary blacklist, and let the cdc_ether
>> driver handle the device if the user selects the ECM configuration.
>> That't how the USB system works.  There is no need for any code in r8152
>> to do that.
>
> The pure cdc_ether driver couldn't change the speed of the
> ethernet, because it doesn't know how to access the PHY of
> the device. Therefore, I add relative code in r8152 driver.

Yes, I see that.  But is that strictly necessary? Couldn't you just say:
"CDC ECM is supported by cdc_ether and therefore limited to the features
implemented by cdc_ether.  If you want feature X, then please use our
vendor specific mode with the r8152 driver?"

As for the dynamic switching of multi-configuration USB devices:  This
is not special for your device.  It is very common for e.g. LTE modems
to provide 2 or more configurations, allowing the user to select between
for example

 1: ECM class + ACM class functions
 2: MBIM class function
 3: vendor specific functions

Each USB configuation comes with a set of descriptors identifying the
functions, and USB interface drivers attach to the functions they
support.  The user can dynamically switch the device from e.g. cfg #1 to
cfg #3 by writing "3" to /sys/bus/usb/devices//bConfigurationValue
This will cause the ECM and ACM USB interfaces to disappear, and the
associated class drivers will unbind, and new vendor specific USB
interfaces appear instead, causing a matching vendor specific driver to
load and bind.

Naturally, end users will not switch configurations all the time.  They
will select the configuration providing the set of functions they want.
If this is different from the default configuration  selected by the
Linux USB core, then that's a simple udev rule to update the
bConfigurationValue sysfs attribute on device disceovery.

This is how the USB core works by *default*, as long as you do not
deliberately break it by blacklisting any functions or forcing a
specific configuration.

You are overdesigning to solve a non-existing problem.



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: [PATCH] usb: dwc3: OCTEON: add support for device tree

[Felipe Balbi]

Hi Steven,

"Steven J. Hill"  writes:
> This patch adds support to parse probe data for
> the dwc3-octeon driver using device tree. The
> DWC3 IP core is found on OCTEON III processors.
>
> Signed-off-by: Steven J. Hill 
> ---
>  drivers/usb/dwc3/Kconfig   | 10 +
>  drivers/usb/dwc3/Makefile  |  1 +
>  drivers/usb/dwc3/dwc3-octeon.c | 96 
> ++
>  3 files changed, 107 insertions(+)
>  create mode 100644 drivers/usb/dwc3/dwc3-octeon.c
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index a64ce1c..99db6008 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -105,4 +105,14 @@ config USB_DWC3_ST
> inside (i.e. STiH407).
> Say 'Y' or 'M' if you have one such device.
>  
> +config USB_DWC3_OCTEON
> + tristate "Cavium OCTEON III Platforms"
> + depends on CAVIUM_OCTEON_SOC

we really don't want SoC dependencies. At a minimum, you should have:

depends on CAVIUM_OCTEON_SOC || COMPILE_TEST

> +static int dwc3_octeon_probe(struct platform_device *pdev)
> +{
> + struct device   *dev = >dev;
> + struct resource *res;
> + struct dwc3_octeon  *octeon;
> + int ret;
> +
> + octeon = devm_kzalloc(dev, sizeof(*octeon), GFP_KERNEL);
> + if (!octeon)
> + return - ENOMEM;
> +
> + /*
> +  * Right now device-tree probed devices don't get dma_mask set.
> +  * Since shared usb code relies on it, set it here for now.
> +  */

this doesn't look correct to me. Are you, perhaps, just missing
dma-ranges and dma-coherent properties?

> +static int dwc3_octeon_remove(struct platform_device *pdev)
> +{
> + struct dwc3_octeon *octeon = platform_get_drvdata(pdev);
> +
> + octeon->usbctl = NULL;
> + octeon->index = -1;

octeon is going to be freed when ->remove() gets executed. You really
don't need to do these. In fact, setting usbctl to NULL will break
iounmap(). It seems to be me you don't need a remove at all.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[NeilBrown]

On Thu, Sep 08 2016, Baolin Wang wrote:

> Hi Neil,
>
> On 6 September 2016 at 13:40, NeilBrown  wrote:
>> On Mon, Aug 29 2016, Baolin Wang wrote:
>>
>>> Hi Felipe,
>>>
>>> On 11 August 2016 at 11:14, Baolin Wang  wrote:
 Hi Felipe,

 On 1 August 2016 at 15:09, Baolin Wang  wrote:
> Currently the Linux kernel does not provide any standard integration of 
> this
> feature that integrates the USB subsystem with the system power regulation
> provided by PMICs meaning that either vendors must add this in their 
> kernels
> or USB gadget devices based on Linux (such as mobile phones) may not 
> behave
> as they should. Thus provide a standard framework for doing this in 
> kernel.
>
> Now introduce one user with wm831x_power to support and test the usb 
> charger,
> which is pending testing. Moreover there may be other potential users 
> will use
> it in future.

 Could you please apply this patchset into your 'next' branch if you
 have no comments about it? Thank you.
>>>
>>> Since there are no other comments about this patchset for a long time,
>>> could you please apply this patchset? Thanks.
>>
>
> Sorry for the late reply.
>
>> Sorry, I should have replied earlier.  Tim Bird mentioned on the
>> ksummit-discuss list that there was a frustration with this not making
>> progress so I decided to contribute what I could now.
>>
>> I think this patch set is attempting to address an important problem
>> that needs solving.  However I think it gets some key aspects wrong.
>> Maybe they can get fixed up after the patchset is upstream, maybe they
>> should be fixed first - I have no strong opinion on that.
>>
>> My main complaints involve the detection and handling of the different
>> charger types - DCP, CDP, ACA etc.
>> The big-picture requirement here that the PHY will detect the physical
>> properties of the cable (e.g. resistance to ground on ID) and determine
>> the type of charger expected.  This information must be communicated to
>> the PMIC "power_supply" device so it can regulate the power being drawn
>> through the cable.
>>
>> The first problem is that there are two different ways that the
>> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
>> are cable types in the 'extcon' subsystem, and they are power_supply
>> types in the 'power_supply' subsystem.  This duplication is confusing.
>> It is not caused by your patch set, but I believe your patchset needs to
>> work with the duplication and I think it does so poorly.
>
> Now the usb charger will not get charger type from 'extcon' subsystem,
> we get the charger type from 'power_supply' and calllback
> 'get_charger_type' for users.

I understand this.  I think it is wrong because, in general, the
power_supply doesn't know what the charger_type is (it might know it is
USB, but most don't know which sort of USB).  The PHY knows that, not
the power_supply.


>
>>
>> In my mind, the power_supply should *not* know about this distinction at
>> all (except possibly as an advisor attribute simiarly to the current
>> battery technology attribute).  The other types it knows of are "AC",
>> "USB", and "BATTERY".  The contrast between these is quite different
>> From the contrast between DCP, CDP, ACA, which, from the perspective of
>> the power supply, are almost irrelevant.  Your patchset effectively
>> examines the power_supply_type of one power_supply, and communicates it
>> to another.  It isn't clear to me how the first power_supply gets the
>> information, or what the relationship between the two power_supplies is
>> meant to be.
>
> We just get the charger type from the power supply which can get the
> charger type from register or something else,

But that register would be part of the PHY, not part of the charger.
Having the power_supply driver reading the PHY register might work for
some hardware, but is not a general solution.


>and the usb charger did
> nothing for this power supply. In some platform, the charger type is
> get in power supply driver, thus we should link this power supply to
> get the charger type when USB cable is plugin. If you don't get
> charger type from power supply driver, then it does not need to link
> this power supply phandle.
>
>>
>> It makes much more sense, to me, to utilized the knowledge of this
>> distinction that extcon provides.  A usb PHY can register an extcon,
>> declare the sorts of cables that it can detect, and tell the extcon as
>> cables appear or disappear.  The PMIC power_supply can then register with
>> that extcon for events and can find out when a cable is attached, and
>> what sort of cable.
>> Your usb-charging framework would be well placed to help the
>> power_supply to find the correct extcon, and possibly even to handle the
>> registration for events.
>>
>> Your framework does 

Re: [PATCH v2 1/1] usb: xhci: fix return value of xhci_setup_device()

[Lu Baolu]

Hi Greg,

On 09/08/2016 02:38 PM, Greg KH wrote:
> On Thu, Sep 08, 2016 at 08:41:02AM +0800, Lu Baolu wrote:
>> xhci_setup_device() should return failure with correct error number
>> when xhci host has died, removed or halted.
>>
>> Cc: sta...@vger.kernel.org # 4.3+
> Why is this a stable kernel issue?  What bug does it fix that affects
> users?

During usb device enumeration, if xhci host is not accessible (died,
removed or halted), the hc_driver->address_device() should return
a corresponding error code to usb core. But current xhci driver just
returns success. This will mislead usb core to continue enumeration:
reading device descriptor, which will result in failure, and users will
get a misleading message like "device descriptor read/8, error -110".

Best regards,
Lu Baolu
--
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 v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Baolin Wang]

Hi Neil,

On 6 September 2016 at 13:40, NeilBrown  wrote:
> On Mon, Aug 29 2016, Baolin Wang wrote:
>
>> Hi Felipe,
>>
>> On 11 August 2016 at 11:14, Baolin Wang  wrote:
>>> Hi Felipe,
>>>
>>> On 1 August 2016 at 15:09, Baolin Wang  wrote:
 Currently the Linux kernel does not provide any standard integration of 
 this
 feature that integrates the USB subsystem with the system power regulation
 provided by PMICs meaning that either vendors must add this in their 
 kernels
 or USB gadget devices based on Linux (such as mobile phones) may not behave
 as they should. Thus provide a standard framework for doing this in kernel.

 Now introduce one user with wm831x_power to support and test the usb 
 charger,
 which is pending testing. Moreover there may be other potential users will 
 use
 it in future.
>>>
>>> Could you please apply this patchset into your 'next' branch if you
>>> have no comments about it? Thank you.
>>
>> Since there are no other comments about this patchset for a long time,
>> could you please apply this patchset? Thanks.
>

Sorry for the late reply.

> Sorry, I should have replied earlier.  Tim Bird mentioned on the
> ksummit-discuss list that there was a frustration with this not making
> progress so I decided to contribute what I could now.
>
> I think this patch set is attempting to address an important problem
> that needs solving.  However I think it gets some key aspects wrong.
> Maybe they can get fixed up after the patchset is upstream, maybe they
> should be fixed first - I have no strong opinion on that.
>
> My main complaints involve the detection and handling of the different
> charger types - DCP, CDP, ACA etc.
> The big-picture requirement here that the PHY will detect the physical
> properties of the cable (e.g. resistance to ground on ID) and determine
> the type of charger expected.  This information must be communicated to
> the PMIC "power_supply" device so it can regulate the power being drawn
> through the cable.
>
> The first problem is that there are two different ways that the
> distinction between DCP, CDP, ACA etc can be represented in Linux.  They
> are cable types in the 'extcon' subsystem, and they are power_supply
> types in the 'power_supply' subsystem.  This duplication is confusing.
> It is not caused by your patch set, but I believe your patchset needs to
> work with the duplication and I think it does so poorly.

Now the usb charger will not get charger type from 'extcon' subsystem,
we get the charger type from 'power_supply' and calllback
'get_charger_type' for users.

>
> In my mind, the power_supply should *not* know about this distinction at
> all (except possibly as an advisor attribute simiarly to the current
> battery technology attribute).  The other types it knows of are "AC",
> "USB", and "BATTERY".  The contrast between these is quite different
> From the contrast between DCP, CDP, ACA, which, from the perspective of
> the power supply, are almost irrelevant.  Your patchset effectively
> examines the power_supply_type of one power_supply, and communicates it
> to another.  It isn't clear to me how the first power_supply gets the
> information, or what the relationship between the two power_supplies is
> meant to be.

We just get the charger type from the power supply which can get the
charger type from register or something else, and the usb charger did
nothing for this power supply. In some platform, the charger type is
get in power supply driver, thus we should link this power supply to
get the charger type when USB cable is plugin. If you don't get
charger type from power supply driver, then it does not need to link
this power supply phandle.

>
> It makes much more sense, to me, to utilized the knowledge of this
> distinction that extcon provides.  A usb PHY can register an extcon,
> declare the sorts of cables that it can detect, and tell the extcon as
> cables appear or disappear.  The PMIC power_supply can then register with
> that extcon for events and can find out when a cable is attached, and
> what sort of cable.
> Your usb-charging framework would be well placed to help the
> power_supply to find the correct extcon, and possibly even to handle the
> registration for events.
>
> Your framework does currently register with extcon, but only listens for
> EXTCON_USB cables.  I don't think that cable type is (reliably) reported
> when a DCP (for example) is plugged in.

Here we just listen the plugin/out events from extcon, if one cable
plugin it will report to usb charger.

>
> Here there is another problem that is not of your making, but still
> needs fixing.  Extcon declares a number of cable types like:
>
> /* USB external connector */
> #define EXTCON_USB  1
> #define EXTCON_USB_HOST 2
>
> /* Charging external connector */
> #define EXTCON_CHG_USB_SDP  5   /* Standard Downstream Port */

[PATCH v2 2/2] usb: gadgetfs: feature - delegate get descriptor

[Binyamin Sharet]

When this feature is enabled, all GET_DESCRIPTOR control requests
will not be handled by gadgetfs, but delegated to the user mode
driver for handling.

The feature is disabled when ep0 file is opened, and can be set
via ioctl, since when enabled, it breaks compatibility with older
versions, and will break user mode drivers that expect gedgetfs
to handle GET_DESCRIPTOR requests.

The feature availability is protected by Kconfig
USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTOR (depends on EXPERT) so
that a user mode driver will not enable it by mistake.

Signed-off-by: Binyamin Sharet 
---
 drivers/usb/gadget/legacy/Kconfig | 12 
 drivers/usb/gadget/legacy/inode.c | 17 +
 include/uapi/linux/usb/gadgetfs.h |  5 +
 3 files changed, 34 insertions(+)

diff --git a/drivers/usb/gadget/legacy/Kconfig
b/drivers/usb/gadget/legacy/Kconfig
index 0b36878..bbedead 100644
--- a/drivers/usb/gadget/legacy/Kconfig
+++ b/drivers/usb/gadget/legacy/Kconfig
@@ -184,6 +184,18 @@ config USB_GADGETFS
   Say "y" to link the driver statically, or "m" to build a
   dynamically linked module called "gadgetfs".
 
+config USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS
+bool "Delegate get descriptor requests to user"
+depends on USB_GADGETFS && EXPERT
+default n
+help
+  This feature allows to enable delegation of GET_DESCRIPTOR
+  requests in GadgetFS.
+  Enabling the request delegation is done through ioctl calls on
+  the control endpoint file descriptor. Once enabled, all
+  GET_DESCRIPTOR requests will be delegated to user mode, and will
+  not be handled by GadgetFS.
+
 config USB_FUNCTIONFS
 tristate "Function Filesystem"
 select USB_LIBCOMPOSITE
diff --git a/drivers/usb/gadget/legacy/inode.c
b/drivers/usb/gadget/legacy/inode.c
index ba2f6f4..b5ef210 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -182,6 +182,11 @@ static struct dev_data *dev_new (void)
 feature_set(>supported_features,
 GADGETFS_FEATURE_FEATURES0_SUPPORTED);
 
+#ifdef CONFIG_USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS
+feature_set(>supported_features,
+GADGETFS_FEATURE_DELEGATE_DESCRIPTORS);
+#endif
+
 return dev;
 }
 
@@ -1461,6 +1466,13 @@ gadgetfs_setup (struct usb_gadget *gadget, const
struct usb_ctrlrequest *ctrl)
 case USB_REQ_GET_DESCRIPTOR:
 if (ctrl->bRequestType != USB_DIR_IN)
 goto unrecognized;
+
+#ifdef CONFIG_USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS
+if (feature_is_set(>enabled_features,
+GADGETFS_FEATURE_DELEGATE_DESCRIPTORS))
+goto delegate;
+#endif
+
 switch (w_value >> 8) {
 
 case USB_DT_DEVICE:
@@ -1975,6 +1987,11 @@ dev_open (struct inode *inode, struct file *fd)
 feature_set(>enabled_features,
 GADGETFS_FEATURE_FEATURES0_SUPPORTED);
 
+#ifdef CONFIG_USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS
+feature_clear(>enabled_features,
+GADGETFS_FEATURE_DELEGATE_DESCRIPTORS);
+#endif
+
 get_dev (dev);
 value = 0;
 }
diff --git a/include/uapi/linux/usb/gadgetfs.h
b/include/uapi/linux/usb/gadgetfs.h
index 3abe3d3..b72c13a 100644
--- a/include/uapi/linux/usb/gadgetfs.h
+++ b/include/uapi/linux/usb/gadgetfs.h
@@ -93,6 +93,11 @@ enum usb_gadgetfs_feature_bits {
  * This should always be on
  */
 GADGETFS_FEATURE_FEATURES0_SUPPORTED,
+/* delegate all GET_DESCRIPTOR requests to user mode driver.
+ * enabling this feature means that the user mode driver must
+ * handle all GET_DEACRIPTOR requests.
+ */
+GADGETFS_FEATURE_DELEGATE_DESCRIPTORS,
 /* reserved features.
  * any new feature should be added BEFORE this one.
  */
-- 
2.5.0

-- 
Binyamin Sharet,
Cisco, STARE-C

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

[PATCH v2 1/2] usb: gadgetfs: introduce feature control mechanism

[Binyamin Sharet]

Feature control mechanism allows addition of dynamic features to
gadgetfs.

It provides a user-mode driver the ability to control those
features, by querying the supported and enabled features and
enable/disable features in runtime via ioctl on ep0 fd.

All user operations are done on the entire set of features.
e.g. in order to check a specific feature, a user needs to read
the supported and enabled features, set the desired bitmap on
the read bitmap and call the "set enabled features" ioctl with
the modified bitmap.

Signed-off-by: Binyamin Sharet 
---
 drivers/usb/gadget/legacy/inode.c | 120
+-
 include/uapi/linux/usb/gadgetfs.h |  42 +
 2 files changed, 160 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c
b/drivers/usb/gadget/legacy/inode.c
index 16104b5e..ba2f6f4 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -77,6 +77,8 @@ MODULE_LICENSE ("GPL");
 
 static int ep_open(struct inode *, struct file *);
 
+static bool feature_set(struct usb_gadgetfs_features *,
+enum usb_gadgetfs_feature_bits feature);
 
 /*--*/
 
@@ -144,6 +146,8 @@ struct dev_data {
 wait_queue_head_twait;
 struct super_block*sb;
 struct dentry*dentry;
+struct usb_gadgetfs_featuresenabled_features;
+struct usb_gadgetfs_featuressupported_features;
 
 /* except this scratch i/o buffer for ep0 */
 u8rbuf [256];
@@ -175,6 +179,9 @@ static struct dev_data *dev_new (void)
 spin_lock_init (>lock);
 INIT_LIST_HEAD (>epfiles);
 init_waitqueue_head (>wait);
+feature_set(>supported_features,
+GADGETFS_FEATURE_FEATURES0_SUPPORTED);
+
 return dev;
 }
 
@@ -1235,14 +1242,120 @@ out:
return mask;
 }
 
+/* feature control */
+
+/* get a features unit, e.g. one u64 bitmap */
+static u64 *
+features_get_unit(struct usb_gadgetfs_features *features,
+enum usb_gadgetfs_feature_bits feature_bit)
+{
+if (feature_bit < GADGETFS_FEATURE_RESERVED)
+if ((feature_bit / 64) == 0)
+return >features0;
+return NULL;
+}
+
+#ifdef CONFIG_USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS
+/* check if a feature is set in a feature bitmap */
+static bool
+feature_is_set(struct usb_gadgetfs_features *features,
+enum usb_gadgetfs_feature_bits feature_bit)
+{
+u64 *p_unit;
+u64 feature_mask;
+
+p_unit = features_get_unit(features, feature_bit);
+if (p_unit == NULL)
+return false;
+feature_mask = 1 << (feature_bit % 64);
+return (*p_unit & feature_mask) != 0;
+}
+#endif
+
+#ifdef CONFIG_USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS
+/* clear a feature in a feature bitmap */
+static bool
+feature_clear(struct usb_gadgetfs_features *features,
+enum usb_gadgetfs_feature_bits feature_bit)
+{
+u64 *p_unit;
+u64 feature_mask;
+
+p_unit = features_get_unit(features, feature_bit);
+if (p_unit == NULL)
+return false;
+if (feature_bit == GADGETFS_FEATURE_FEATURES0_SUPPORTED)
+return false;
+feature_mask = 1 << (feature_bit % 64);
+*p_unit &= ~feature_mask;
+return true;
+}
+#endif
+
+/* set a feature in a feature bitmap */
+static bool
+feature_set(struct usb_gadgetfs_features *features,
+enum usb_gadgetfs_feature_bits feature_bit)
+{
+u64 *p_unit;
+u64 feature_mask;
+
+p_unit = features_get_unit(features, feature_bit);
+if (p_unit == NULL)
+return false;
+feature_mask = 1 << (feature_bit % 64);
+*p_unit |= feature_mask;
+return true;
+}
+
+/* set features based on user input */
+static void
+set_user_features(struct dev_data *dev, struct usb_gadgetfs_features *src)
+{
+/* only set supported features */
+src->features0 &= dev->supported_features.features0;
+/* always set FEATURES0 */
+src->features0 |= (1 << GADGETFS_FEATURE_FEATURES0_SUPPORTED);
+dev->enabled_features.features0 = src->features0;
+}
+
 static long dev_ioctl (struct file *fd, unsigned code, unsigned long value)
 {
 struct dev_data*dev = fd->private_data;
 struct usb_gadget*gadget = dev->gadget;
 long ret = -ENOTTY;
 
-if (gadget->ops->ioctl)
-ret = gadget->ops->ioctl (gadget, code, value);
+switch (code) {
+case GADGETFS_GET_SUPPORTED_FEATURES:
+ret = copy_to_user((void *)value, >supported_features,
+sizeof(dev->supported_features));
+if (ret != 0)
+ret = -EFAULT;
+break;
+case GADGETFS_GET_ENABLED_FEATURES:
+ret = copy_to_user((void *)value, >enabled_features,
+sizeof(dev->enabled_features));
+if (ret != 0)
+ret = -EFAULT;
+break;
+case GADGETFS_SET_ENABLED_FEATURES:
+{
+struct usb_gadgetfs_features user_features;
+
+ret = 

Re: [PATCH v2 1/1] usb: xhci: fix return value of xhci_setup_device()

[Greg KH]

On Thu, Sep 08, 2016 at 08:41:02AM +0800, Lu Baolu wrote:
> xhci_setup_device() should return failure with correct error number
> when xhci host has died, removed or halted.
> 
> Cc: sta...@vger.kernel.org # 4.3+

Why is this a stable kernel issue?  What bug does it fix that affects
users?

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

[PATCH v2 0/2] usb: gadgetfs: support delegation of GET_DESCRIPTOR

[Binyamin Sharet]

This patch series adds support for delegation of GET_DESCRIPTOR requests
from GadgetFS to user mode driver. This will allow deeper testing of USB
hosts using GadgetFS, since it allows a user mode driver to control the
responses to to GET_DESCRIPTOR requests, and respond with malformed
descriptors.

Enabling this feature will break compatibility, since GadgetFS will not
handle GET_DESCRIPTOR requests, although every user-mode driver so far
expects it to.

So we add three protection mechanisms to avoid this:

- enabling this feature will only be possible by enabling
  USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTOR in Kconfig, which depends
  enabling EXPERT as well.
- even when enabled in Kconfig, delegation will be disabled until an
  appropriate ioctl on the control endpoint file descriptor is issued.
- enabling this feature is done per file descriptor. if a user enables
  this feature and then close and re-open the control endpoint file,
  the feature will be disabled until the ioctl will be issued on the new
  file descriptor.

-- 
Binyamin Sharet,
Cisco, STARE-C


--
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 1/1] subsystem:usb

[Greg KH]

On Wed, Sep 07, 2016 at 04:09:36PM -0700, Kees Cook wrote:
> On Thu, Sep 1, 2016 at 11:07 PM, Greg KH  wrote:
> > On Fri, Sep 02, 2016 at 01:20:21AM +0100, James wrote:
> >> This patch fixes a NULL pointer dereference caused by a race codition in 
> >> the probe
> >> function of the legousbtower driver. The probe function does not 
> >> deregister the
> >> usb interface after an error receiving the devices firmware ID. The device 
> >> file
> >> registered (/dev/usb/legousbtower%d) may be read/written globally before 
> >> the probe
> >> function returns; then when tower_delete is called in the probe function 
> >> (after an
> >> r/w has been initiated), core dev structures are deleted while the file 
> >> operation
> >> functions are still running. If the 0 address is mappable on the machine, 
> >> this
> >> vulnerability can be used to create a Local Priviege Escalation exploit 
> >> via a
> >> write-what-where condition by remapping dev->interrupt_out_buffer in 
> >> tower_write.
> >> A forged USB device and local program execution would be required for LPE.
> >> The USB device would have to delay the control message in tower_probe and
> >> accept the control urb in tower_open whilst guest code initiated a write to
> >> the device file as tower_delete is called from the error in tower_probe.
> >
> > How about this simpler patch, that moves the registering for the minor
> > number as the last thing that happens in the probe function (which is
> > what it should be).
> >
> > Can you test this to verify it prevents the above problem?
> >
> > And this driver really needs an overhaul one of these days...
> >
> > thanks,
> >
> > greg k-h
> >
> > diff --git a/drivers/usb/misc/legousbtower.c 
> > b/drivers/usb/misc/legousbtower.c
> > index 7771be3ac178..4dd531ac5a7f 100644
> > --- a/drivers/usb/misc/legousbtower.c
> > +++ b/drivers/usb/misc/legousbtower.c
> > @@ -898,24 +898,6 @@ static int tower_probe (struct usb_interface 
> > *interface, const struct usb_device
> > dev->interrupt_in_interval = interrupt_in_interval ? 
> > interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
> > dev->interrupt_out_interval = interrupt_out_interval ? 
> > interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
> >
> > -   /* we can register the device now, as it is ready */
> > -   usb_set_intfdata (interface, dev);
> > -
> > -   retval = usb_register_dev (interface, _class);
> > -
> > -   if (retval) {
> > -   /* something prevented us from registering this driver */
> > -   dev_err(idev, "Not able to get a minor for this device.\n");
> > -   usb_set_intfdata (interface, NULL);
> > -   goto error;
> > -   }
> > -   dev->minor = interface->minor;
> > -
> > -   /* let the user know what node this device is now attached to */
> > -   dev_info(>dev, "LEGO USB Tower #%d now attached to major 
> > "
> > -"%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
> > -USB_MAJOR, dev->minor);
> > -
> > /* get the firmware version and log it */
> > result = usb_control_msg (udev,
> >   usb_rcvctrlpipe(udev, 0),
> > @@ -936,6 +918,23 @@ static int tower_probe (struct usb_interface 
> > *interface, const struct usb_device
> >  get_version_reply.minor,
> >  le16_to_cpu(get_version_reply.build_no));
> >
> > +   /* we can register the device now, as it is ready */
> > +   usb_set_intfdata (interface, dev);
> > +
> > +   retval = usb_register_dev (interface, _class);
> > +
> > +   if (retval) {
> > +   /* something prevented us from registering this driver */
> > +   dev_err(idev, "Not able to get a minor for this device.\n");
> > +   usb_set_intfdata (interface, NULL);
> > +   goto error;
> > +   }
> > +   dev->minor = interface->minor;
> > +
> > +   /* let the user know what node this device is now attached to */
> > +   dev_info(>dev, "LEGO USB Tower #%d now attached to major 
> > "
> > +"%d minor %d\n", (dev->minor - LEGO_USB_TOWER_MINOR_BASE),
> > +USB_MAJOR, dev->minor);
> >
> >  exit:
> > return retval;
> >
> 
> Did this get any testing?

I have not heard anything back from the original poster :(

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