Re: [PATCH v2 05/14] USB: ch341: fix USB buffer allocations
On Sat, 2016-04-02 at 19:07 +0200, Grigori Goronzy wrote:
> Use the correct types and sizes.
>
> Signed-off-by: Grigori Goronzy
> ---
> drivers/usb/serial/ch341.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 25c5d8d..6781911 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -116,7 +116,7 @@ static int ch341_control_out(struct usb_device *dev, u8
> request,
>
> static int ch341_control_in(struct usb_device *dev,
> u8 request, u16 value, u16 index,
> - char *buf, unsigned bufsize)
> + unsigned char *buf, unsigned bufsize)
If you do that, you can just use u8 *
> {
> int r;
>
> @@ -169,9 +169,9 @@ static int ch341_set_handshake(struct usb_device *dev, u8
> control)
>
> static int ch341_get_status(struct usb_device *dev, struct ch341_private
> *priv)
> {
> - char *buffer;
> + unsigned char *buffer;
> int r;
> - const unsigned size = 8;
> + const unsigned size = 2;
> unsigned long flags;
>
> buffer = kmalloc(size, GFP_KERNEL);
> @@ -199,9 +199,9 @@ out: kfree(buffer);
>
> static int ch341_configure(struct usb_device *dev, struct ch341_private
> *priv)
> {
> - char *buffer;
> + unsigned char *buffer;
> int r;
> - const unsigned size = 8;
> + const unsigned size = 2;
Are you sure only 2 are used?
For the amount of space needed it makes no difference.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Create an audit record of USB specific details
Oliver Neukum writes: > On Mon, 2016-04-04 at 00:02 -0400, wmealing wrote: > >> I'm looking to create an audit trail for when devices are added or removed >> from the system. >> >> The audit subsystem is a logging subsystem in kernel space that can be >> used to create advanced filters on generated events. It has partnered >> userspace >> utilities ausearch, auditd, aureport, auditctl which work exclusively on >> audit >> records. >> >> These tools are able to set filters to "trigger" on specific in-kernel events >> specified by privileged users. While the userspace tools can create audit >> events these are not able to be handled intelligently (decoded,filtered or >> ignored) as kernel generated audit events are. > > That is a goal that should be debated in general. Yes. And I think it would make this proposal appear a lot less fishy if it included links and summaries of previous discussions on the subject. Is there an assumption that people on this list remember every discussion for weeks? Or the opposite maybe? AFAICS, Greg has already asked the obvious questions and made the obvious "do this in userspace using the existing uevents" proposal. I did not see any followup to his last message, so I assumed this audit thing would return to the drawing board with a userspace implementation: http://www.spinics.net/lists/linux-usb/msg137671.html It was quite suprising to instead see a USB specific kernel implemenation duplicating exisiting device add/remove functionality. Why? The provided reason makes absolutely no sense at all. Userspace tools are as intelligent as you make them. And "decoded,filtered or ignored" implies policy, which IMHO has no place in the kernel in any case. >> I have this working at the moment with the USB subsystem (as an example). >> Its been suggested that I use systemd-udev however this means that the audit >> tools (ausearch) will not be able to index these records. > > Chaining this so tightly to the USB subsystem makes no sense. > If you do this, then please hook into the generic layer, that > is add_device(), and provide a method in the generic device structure > for providing information to the audit subsystem. I think the generic layer implementation is already there. The proposed USB specific solution adds nothing, as pointed out by Greg the last time this was discussed. 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 1/2] usb: dwc3: core: Introduce dwc3_device_reinit()
+Abhishek, Ravi, Felipe, On 31/03/16 17:26, Felipe Balbi wrote: > > Hi, > > Roger Quadros writes: >>> We will need this function for a workaround. >>> The function issues a softreset only to the device >>> controller and performs minimal re-initialization >>> so that the device controller can be usable. >>> >>> As some code is similar to dwc3_core_init() take out >>> common code into dwc3_get_gctl_quirks(). >>> >>> We add a new member (prtcap_mode) to struct dwc3 to >>> keep track of the current mode in the PRTCAPDIR register. >>> >>> Signed-off-by: Roger Quadros >> >> I must say, I don't like this at all :-p There's ONE known silicon >> which >> needs this because of a poor silicon integration which took an IP >> with a >> known erratum where it can't be made to work on lower speeds and >> STILL >> was integrated without a superspeed PHY. >> >> There's a reason why I never tried to push this upstream myself ;-) >> >> I'm really thinking we might be better off adding a quirk flag to >> skip >> the metastability workaround and allow this ONE silicon to set the >> controller to lower speed. >> >> John, can you check with your colleagues if we would ever fall into >> STAR#9000525659 if we set maximum speed to high speed during driver >> probe and never touch it again ? I would assume we don't really fall >> into the metastability workaround, right ? We're not doing any sort >> of >> PM for dwc3... >> >>> >>> Hi Felipe, >>> >>> Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS? >>> I don't see an issue with that as long as we always ignore >>> dwc->maximum_speed when programming DCFG.speed for all affected >>> versions of the core. As long as the DCFG.speed = SS, you should not >>> hit the STAR. >> >> I actually mean changing DCFG.speed during driver probe and never >> touching it again. Would that still cause problems ? >> > > In that case I'm not sure. The engineer who would know is off until > next week so I'll get back to you as soon as I can talk to him about > it. >>> >>> So the engineers said that this issue can occur while set to HS and >>> the run/stop bit is changed so it seems that won't work. >> >> Thanks John. >> >> Felipe, >> >> any suggestion how we can fix this upstream? > > no idea, I don't have a lot of memory about this problem. I really don't > remember the details about this, is there an openly available errata > document which I could read ? /me goes search for it. > > I found [1] which tells me, the following: > > > | i819| A Device Control Bit Meta-Stability for USB3.0 Controller in > USB2.0 Mode | > |-+| > | Criticality | Medium > | > | | > | > | Descritiion | When USB3.0 controller core is programmed to be a USB > 2.0-only device | > | | hardware meta-stability on USB_DCTL[31]RUNSTOP bit causing > the core to | > | | attempt high speed as well as SuperSpeed connection or > completely miss | > | | the attach request. > | > | | > | > | Workaround | If the requirement is to always function in USB 2.0 mode, > there is no | > | | workaround. > | > | | Otherwise, you can always program the USB controller core to > be SuperSpeed | > | | 3.0 capable (USB_DCFG[2:0]DEVSPD = 0x4) > | > | | > | > | Revisions | SR 1.1, 2.0 > | > | Impacted| > | > |-+| > > So, TI's own documentation says that there is _no_ workaround. My We are working on updating that document. Apparently Synopsis has suggested this workaround. pasting verbatim " - As last step of device configuration we set the RUNSTOP bit in DCTL. - Once we set the RUNSTOP bit, we need to monitor GDBGLTSSM for 100 ms until one of the two below happens:. o We see the GDBGLTSSM.LTDB_LINK_STATE changing from 4 o We receive the
Re: musb: kernel 3.18.29 build issue
Hi Bin, On Thu, Mar 31, 2016 at 9:58 PM, Bin Liu wrote: > On Thu, Mar 31, 2016 at 12:48:39PM -0700, Greg KH wrote: >> On Thu, Mar 31, 2016 at 02:10:59PM -0500, Bin Liu wrote: >> > Hi Greg, >> > >> > On Thu, Mar 31, 2016 at 09:06:10AM -0700, Greg KH wrote: >> > > On Thu, Mar 31, 2016 at 07:29:21AM +0200, Yegor Yefremov wrote: >> > > > Hello Greg, >> > > > >> > > > 3.18.x is missing this patch [1] in order to perform a successful >> > > > build. The reason: following patch [2] in 3.18.x uses a macro name >> > > > introduced in the missing patch [1]. >> > > > >> > > > Could you queue it for 3.18.30? >> > > >> > > Nope, I have nothing to do with 3.18-stable, sorry. >> > > >> > > Please read Documentation/stable_kernel_rules.txt for how to get patches >> > > into stable kernel releases. >> > >> > The related discussion on v3.12 is [1], it seems for some reason commit >> > cb83df7 has been back ported to stable, but the dependent patch 0149b07 >> > is missed, which causes build error. >> >> Again, that's nice, but I have nothing to do with 3.18-stable, so >> there's nothing I can do here, please read that file for how to get this >> resolved. > > Sorry, I should have checked first - you are not maintaining > 3.18-stable. I've looked at Documentation/stable_kernel_rules.txt and it seems like we should use "Option 2". Could you please send an e-mail to sta...@vger.kernel.org? Thanks. Yegor -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb 3.0 stopped working with 4.6.0 rc1
On 02.04.2016 02:29, Paulo Dias wrote: Would you mind sending me the patches , so i could test with my external HD? even with today git build im getting all but errors with this controller and the driver. with today build i even have one shutdown usb 3.0 port! Regression fix is in Gregs usb-linus tree: 59b9023 usb: fix regression in SuperSpeed endpoint descriptor parsing It's not yet in any rc kernel. I hope it will be in rc-3 Does Gregs usb-linus branch work for you? -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/2] usb: dwc3: core: Introduce dwc3_device_reinit()
Hi, Roger Quadros writes: We will need this function for a workaround. The function issues a softreset only to the device controller and performs minimal re-initialization so that the device controller can be usable. As some code is similar to dwc3_core_init() take out common code into dwc3_get_gctl_quirks(). We add a new member (prtcap_mode) to struct dwc3 to keep track of the current mode in the PRTCAPDIR register. Signed-off-by: Roger Quadros >>> >>> I must say, I don't like this at all :-p There's ONE known silicon >>> which >>> needs this because of a poor silicon integration which took an IP >>> with a >>> known erratum where it can't be made to work on lower speeds and >>> STILL >>> was integrated without a superspeed PHY. >>> >>> There's a reason why I never tried to push this upstream myself ;-) >>> >>> I'm really thinking we might be better off adding a quirk flag to >>> skip >>> the metastability workaround and allow this ONE silicon to set the >>> controller to lower speed. >>> >>> John, can you check with your colleagues if we would ever fall into >>> STAR#9000525659 if we set maximum speed to high speed during driver >>> probe and never touch it again ? I would assume we don't really fall >>> into the metastability workaround, right ? We're not doing any sort >>> of >>> PM for dwc3... >>> Hi Felipe, Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS? I don't see an issue with that as long as we always ignore dwc->maximum_speed when programming DCFG.speed for all affected versions of the core. As long as the DCFG.speed = SS, you should not hit the STAR. >>> >>> I actually mean changing DCFG.speed during driver probe and never >>> touching it again. Would that still cause problems ? >>> >> >> In that case I'm not sure. The engineer who would know is off until >> next week so I'll get back to you as soon as I can talk to him about >> it. > So the engineers said that this issue can occur while set to HS and the run/stop bit is changed so it seems that won't work. >>> >>> Thanks John. >>> >>> Felipe, >>> >>> any suggestion how we can fix this upstream? >> >> no idea, I don't have a lot of memory about this problem. I really don't >> remember the details about this, is there an openly available errata >> document which I could read ? /me goes search for it. >> >> I found [1] which tells me, the following: >> >> >> | i819| A Device Control Bit Meta-Stability for USB3.0 Controller in >> USB2.0 Mode | >> |-+| >> | Criticality | Medium >> | >> | | >> | >> | Descritiion | When USB3.0 controller core is programmed to be a USB >> 2.0-only device | >> | | hardware meta-stability on USB_DCTL[31]RUNSTOP bit causing >> the core to | >> | | attempt high speed as well as SuperSpeed connection or >> completely miss | >> | | the attach request. >> | >> | | >> | >> | Workaround | If the requirement is to always function in USB 2.0 mode, >> there is no | >> | | workaround. >> | >> | | Otherwise, you can always program the USB controller core to >> be SuperSpeed | >> | | 3.0 capable (USB_DCFG[2:0]DEVSPD = 0x4) >> | >> | | >> | >> | Revisions | SR 1.1, 2.0 >> | >> | Impacted| >> | >> |-+| >> >> So, TI's own documentation says that there is _no_ workaround. My > > We are working on updating that document. Apparently Synopsis has suggested > this workaround. > pasting verbatim > > " > - As last step of device configuration we set the RUNSTOP bit in > DCTL. > > - Once we set the RUNSTOP bit, we need to monitor GDBGLTSSM for 100 > ms until one of the two below happens:. > > o We see the GDBGLTSSM.LTDB_LINK
Re: [PATCH 1/5 v10] dt/bindings: Add binding for the DA8xx MUSB driver
On 15.03.2016 03:07, David Lechner wrote: On 03/14/2016 02:00 AM, Felipe Balbi wrote: Hi, why isn't this an actual clock phandle ? Driver could, then, use clk_get_rate() to figure this one out. You could just use a fixed clock here to satisfy the clock API. I've actually been working on getting the da8xx ohci driver working with device tree. It has similar clock issues (in fact, the ohci clock is a child of the musb clock). Petr, give me a day or two and will have post some patches. It will have a clock that can be used here for clk_get_rate(). Hi David, I'm trying to move on with my USB2.0 patch set because my projects is blocked waiting for it. I've seen your PHY patch but it contains only on/off functions and does not handle the clocks, which IMHO should be moved to the PHY driver as well if we decided to do it properly. How shall we proceed if both of us are working on the same piece of code? Thanks Petr -- 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
XHCI is slow during boot (bios/efi) and leaves many dmesg messages
Hi list, I have a Apple Inc. MacBookPro11,1 (with the most recent 'bios': BIOS MBP111.88Z.0138.B16.1509081438 09/08/2015). At the beginning, USB worked normally. After a while (and after newer kernel versions released by debian?) things started to act strangely. For one, the bios/efi boot takes a very long time (probably due to the same reason I describe later) just to get to the bootloader/grub. Likley resetting and probing for USB ports/mass storage. When grub finally pops up, I can use the (internal USB based keyboard) normally to select a grub entry etc. Booting the kernel then works reasonably fine, until it loads the xhci module. It spews some messages in dmesg (taking some 15 seconds) and only then, the keyboard starts to work again. The log is filled with messages like: [7.248479] xhci_hcd :00:14.0: Command completion event does not match command [7.248495] xhci_hcd :00:14.0: Timeout while waiting for setup device command [ 12.256347] xhci_hcd :00:14.0: Timeout while waiting for setup device command [ 12.256363] usb 1-2: hub failed to enable device, error -62 [ 17.264166] xhci_hcd :00:14.0: Timeout while waiting for setup device command (followed by USB hub/device enumeration) I've tried several combinations and quirks, updating to the latest rc kernels since 3.16 (am on 4.5.0 right now) and it only seems to get worse. Last year, on the 3.x series of kernels occasionally after a reboot the 'bios' would go through quickly and fine and also no problems loading the module and logging in. But now it always fails. Additionally it (may or may not) seems to cause the internal usb card reader to not even show up almost all of the time, though under OSX it works fine. There is/was a known issue with this cardreader where it would disappear after a suspend. Adding various seemingly related intel usb3 quirks I had no change, as I think all of them are already applied to this chipset. I'm guessing that somehow the usb chipset has some configuration option miss-set (which persists over reboots/power down) and the driver doesn't quite understand it. Unfortunately it seems that this chipset does not work in pure USB2.0 (ehci) mode and needs the xhci module to work at all, so even falling to USB2 is no option. Also disconnecting all USB perhipials is nearly impossible as the touchpad, bluetooth cardreader and keyboard are internally all wired to USB. I'm attaching 3 dmesg logs with various kernels and levels of debugging information. I tried to google for errors from these logs, but to no avail. I have attached some log files on the bugzilla issue tracker [0] (they are to big for the ML I think). [0] https://bugzilla.kernel.org/show_bug.cgi?id=115741 Olliver -- 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
probably missing patch to stable?
Hi Greg,
This commit [1] mentions that it affects certain stable versions but
I didn't see cc: stable in it nor could find it in any mailing list.
Just wanted to bring to your attention. Thanks.
cheers,
-roger
[1]
commit e5bdfd50d6f76077bf8441d130c606229e100d40
Author: Greg Kroah-Hartman
Revert "usb: hub: do not clear BOS field during reset device"
This reverts commit d8f00cd685f5c8e0def8593e520a7fef12c22407.
Tony writes:
This upstream commit is causing an oops:
d8f00cd685f5 ("usb: hub: do not clear BOS field during reset device")
This patch has already been included in several -stable kernels. Here
are the affected kernels:
4.5.0-rc4 (current git)
4.4.2
4.3.6 (currently in review)
4.1.18
3.18.27
3.14.61
How to reproduce the problem:
Boot kernel with slub debugging enabled (otherwise memory corruption
will cause random oopses later instead of immediately)
Plug in USB 3.0 disk to xhci USB 3.0 port
dd if=/dev/sdc of=/dev/null bs=65536
(where /dev/sdc is the USB 3.0 disk)
Unplug USB cable while dd is still going
Oops is immediate:
Reported-by: Tony Battersby
Cc: Du, Changbin
Signed-off-by: Greg Kroah-Hartman
--
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: gadget: f_midi: unlock on error
Hi Dan,
On 02/04/16 05:51, Dan Carpenter wrote:
> We added some new locking here, but missed an error path where we need
> to unlock.
>
> Fixes: 9acdf4df2fc4 ('usb: gadget: f_midi: added spinlock on transmit
> function')
> Signed-off-by: Dan Carpenter
Acked-by: Felipe F. Tonello
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde..2c0616c 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -609,8 +609,10 @@ static void f_midi_transmit(struct f_midi *midi)
>
> do {
> ret = f_midi_do_transmit(midi, ep);
> - if (ret < 0)
> + if (ret < 0) {
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> goto drop_out;
> + }
> } while (ret);
>
> spin_unlock_irqrestore(&midi->transmit_lock, flags);
>
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
[PATCH 2/2] usb: dwc3: gadget: put link to U0 before Start Transfer
Synopsys Databook says we should move link to U0
before issuing a Start Transfer command. We could
require the gadget driver to call
usb_gadget_wakeup() however I feel that changing all
gagdget drivers to keep track of Link State and
conditionally call usb_gadget_wakeup() would be far
too much work. For now we will handle this at the
UDC level, but at some point composite.c should be
one handling this.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 552ebdf972b4..404573fcb3c9 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -221,6 +221,8 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
unsigned cmd, u32 param)
} while (1);
}
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
+
int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
unsigned cmd, struct dwc3_gadget_ep_cmd_params *params)
{
@@ -242,12 +244,24 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
* by the same section on Synopsys databook.
*/
if (cmd == DWC3_DEPCMD_STARTTRANSFER) {
+ int needs_wakeup;
+
reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
susphy = true;
reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
}
+
+ needs_wakeup = (dwc->link_state == DWC3_LINK_STATE_U1 ||
+ dwc->link_state == DWC3_LINK_STATE_U2 ||
+ dwc->link_state == DWC3_LINK_STATE_U3);
+
+ if (unlikely(needs_wakeup)) {
+ ret = __dwc3_gadget_wakeup(dwc);
+ dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
+ ret);
+ }
}
dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
--
2.8.0.rc2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: dwc3: gadget: extract unlocked dwc3_gadget_wakeup()
we will need this from StartTransfer to make sure
link is in U0 before starting a transfer.
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 32 +---
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8b3a676db346..552ebdf972b4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1363,22 +1363,16 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
return DWC3_DSTS_SOFFN(reg);
}
-static int dwc3_gadget_wakeup(struct usb_gadget *g)
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
{
- struct dwc3 *dwc = gadget_to_dwc(g);
-
unsigned long timeout;
- unsigned long flags;
+ int ret;
u32 reg;
- int ret = 0;
-
u8 link_state;
u8 speed;
- spin_lock_irqsave(&dwc->lock, flags);
-
/*
* According to the Databook Remote wakeup request should
* be issued only when the device is in early suspend state.
@@ -1391,8 +1385,7 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
if ((speed == DWC3_DSTS_SUPERSPEED) ||
(speed == DWC3_DSTS_SUPERSPEED_PLUS)) {
dwc3_trace(trace_dwc3_gadget, "no wakeup on SuperSpeed\n");
- ret = -EINVAL;
- goto out;
+ return -EINVAL;
}
link_state = DWC3_DSTS_USBLNKST(reg);
@@ -1405,14 +1398,13 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
dwc3_trace(trace_dwc3_gadget,
"can't wakeup from '%s'\n",
dwc3_gadget_link_string(link_state));
- ret = -EINVAL;
- goto out;
+ return -EINVAL;
}
ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
if (ret < 0) {
dev_err(dwc->dev, "failed to put link in Recovery\n");
- goto out;
+ return ret;
}
/* Recent versions do this automatically */
@@ -1436,10 +1428,20 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
dev_err(dwc->dev, "failed to send remote wakeup\n");
- ret = -EINVAL;
+ return -EINVAL;
}
-out:
+ return 0;
+}
+
+static int dwc3_gadget_wakeup(struct usb_gadget *g)
+{
+ struct dwc3 *dwc = gadget_to_dwc(g);
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&dwc->lock, flags);
+ ret = __dwc3_gadget_wakeup(dwc);
spin_unlock_irqrestore(&dwc->lock, flags);
return ret;
--
2.8.0.rc2
--
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
dwc3: wakeup request details (was: Re: [PATCH 2/2] usb: dwc3: gadget: put link to U0 before Start Transfer)
Hi John, Felipe Balbi writes: > Synopsys Databook says we should move link to U0 > before issuing a Start Transfer command. We could > require the gadget driver to call > usb_gadget_wakeup() however I feel that changing all > gagdget drivers to keep track of Link State and > conditionally call usb_gadget_wakeup() would be far > too much work. For now we will handle this at the > UDC level, but at some point composite.c should be > one handling this. > > Signed-off-by: Felipe Balbi I have been looking at this for a while but I have two question which I can't seem to answer using Synopsys databook (I have here 2.60a). i) When is it legal to start Wakeup request ? ii) What is the proper way to start wakeup request ? IIRC current code was very much extracted from a BSD reference code which was sent to me (while back at TI) years ago and now I have been wondering if current code is actually correct or not. I can't find answers in the databook. Everytime I look for 'wakeup' on the databook it's always along the lines of "SW can issue wakeup request" but there's never a "here's how you start wakeup request and here's when is it legal to do so" section. Would you be able to request Synopsys to clarify this in the Databook ? (I know I'm using an older version of the databook, but I really don't have anything else right now, sorry). -- balbi signature.asc Description: PGP signature
cdc-acm: v4.6-rc1: Kernel panic in acm_start_wb
Hello When updating the kernel to a post v4.6-rc1 version (based on c05c2ec Merge branch 'parisc-4.6-2' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux) I got the following kernel panic on a T440p system with a 0bdb:193e Ericsson Business Mobile Networks BV while probing with the ModemManager: [ 31.819040] BUG: unable to handle kernel NULL pointer dereference at 0018 [ 31.821612] IP: [] acm_start_wb+0x18/0xb0 [cdc_acm] The complete backtrace is in the attached file dmesg.log. I could track down to the problem to the following commit: commit a81cf9799ad7299b03a4dff020d9685f9ac5f3e0 Author: Oliver Neukum Date: Wed Feb 10 10:39:49 2016 +0100 cdc-acm: implement put_char() and flush_chars() This should cut down latencies and waste if the tty layer writes single bytes. Signed-off-by: Oliver Neukum >oneu...@suse.com> Signed-off-by: Greg Kroah-Hartman Reverting it let the problem disappear. Please let me known if you need additional information, Torsten Please CC me on answers on the list. [ 31.819040] BUG: unable to handle kernel NULL pointer dereference at 0018 [ 31.821612] IP: [] acm_start_wb+0x18/0xb0 [cdc_acm] [ 31.823520] PGD 0 [ 31.824120] Oops: [#1] SMP [ 31.825068] Modules linked in: cdc_mbim snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi cdc_ncm usbnet cdc_acm cdc_wdm mii iwlmvm mac80211 iwlwifi cfg80211 snd_hda_intel snd_hda_codec snd_hda_core e1000e snd_hwdep psmouse snd_pcm thinkpad_acpi snd_timer snd soundcore nvram ipsec ipsec_cp(P) ipsec_eroutes(P) libfreeswan cipher_SHA1(PO) cipher_RMD(PO) cipher_AES(PO) cryptdev nls_iso8859_1 nls_cp437 vfat fat dm_mod sd_mod ahci i2c_i801 libahci i915 drm_kms_helper xhci_pci drm xhci_hcd fb_sys_fops sysimgblt sysfillrect ehci_pci syscopyarea i2c_algo_bit i2c_core video loop usb_storage ehci_hcd [ 31.841846] CPU: 3 PID: 1568 Comm: kworker/u16:6 Tainted: P O4.6.0-devel+ #1 [ 31.844280] Hardware name: LENOVO 20AWA00V00/20AWA00V00, BIOS GLET69WW (2.23 ) 04/25/2014 [ 31.847068] Workqueue: events_unbound flush_to_ldisc [ 31.848524] task: 8804381a1a00 ti: 880438acc000 task.ti: 880438acc000 [ 31.850900] RIP: 0010:[] [] acm_start_wb+0x18/0xb0 [cdc_acm] [ 31.853513] RSP: 0018:880438acfcb0 EFLAGS: 00010002 [ 31.855091] RAX: RBX: 880427412000 RCX: 0001 [ 31.857557] RDX: 0001 RSI: RDI: 880427412000 [ 31.859689] RBP: 880438acfcc8 R08: R09: c90003694000 [ 31.862031] R10: R11: R12: 880427412000 [ 31.864155] R13: 0246 R14: R15: [ 31.866439] FS: () GS:88044e2c() knlGS: [ 31.868829] CS: 0010 DS: ES: CR0: 80050033 [ 31.872201] CR2: 0018 CR3: 01a07000 CR4: 001406a0 [ 31.875916] DR0: DR1: DR2: [ 31.879703] DR3: DR6: fffe0ff0 DR7: 0400 [ 31.883236] Stack: [ 31.885242] 880427412000 880427412744 0246 880438acfd00 [ 31.889147] a078ebd3 88043840e000 c90003694000 c90003694000 [ 31.892905] 0001 880097dc3021 880438acfdb0 8136e900 [ 31.896524] Call Trace: [ 31.898603] [] acm_tty_flush_chars+0x63/0xa0 [cdc_acm] [ 31.902128] [] n_tty_receive_buf_common+0x5d0/0xc00 [ 31.905567] [] ? set_next_entity+0x3f4/0x880 [ 31.908681] [] n_tty_receive_buf2+0x14/0x20 [ 31.912026] [] tty_ldisc_receive_buf+0x22/0x50 [ 31.915169] [] flush_to_ldisc+0xc7/0x130 [ 31.918163] [] process_one_work+0x14d/0x410 [ 31.921461] [] worker_thread+0x69/0x4a0 [ 31.924669] [] ? rescuer_thread+0x340/0x340 [ 31.927720] [] kthread+0xdb/0x100 [ 31.930685] [] ret_from_fork+0x22/0x40 [ 31.933602] [] ? kthread_park+0x60/0x60 [ 31.936783] Code: c8 f7 da 83 e2 40 09 f0 09 d0 c3 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 41 55 41 54 53 83 87 40 07 00 00 01 49 89 fc <48> 8b 46 18 48 8b 16 48 89 f3 48 89 50 68 48 8b 46 18 48 8b 56 [ 31.943788] RIP [] acm_start_wb+0x18/0xb0 [cdc_acm] [ 31.947155] RSP [ 31.949611] CR2: 0018 [ 31.952011] ---[ end trace b34d9c31926cfa72 ]--- [ 31.954772] [ cut here ] [ 31.957550] WARNING: CPU: 3 PID: 1568 at kernel/softirq.c:150 __local_bh_enable_ip+0x8c/0xc0 [ 31.961667] Modules linked in: cdc_mbim snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi cdc_ncm usbnet cdc_acm cdc_wdm mii iwlmvm mac80211 iwlwifi cfg80211 snd_hda_intel snd_hda_codec snd_hda_core e1000e snd_hwdep psmouse snd_pcm thinkpad_acpi snd_timer snd soundcore nvram ipsec ipsec_cp(P) ipsec_eroutes(P) libfreeswan cipher_SHA1(PO) cipher_RMD(PO) cipher_AES(PO) cryptdev nls_iso8859_1 nls_cp437 vfat fat dm_mod sd_mod ahci
Re: function ehci_hub_control in ehci-hub.c
On Sun, Apr 03, 2016 at 02:25:05PM -0400, Alan Stern wrote:
> On Sun, 3 Apr 2016, Navin P.S wrote:
>
> > On Sat, Apr 2, 2016 at 8:00 PM, Alan Stern
> > wrote:
> > > On Sat, 2 Apr 2016, Navin P.S wrote:
> > >>regs->hostpc[(wIndex & 0xff) - 1];
> > >
> > > You're asking the question backwards. wIndex is allowed to be 0
> > > because the USB spec says so. You can't argue with that.
> > >
> > > You should be asking why we initialize status_reg and hostpc_reg as
> > > above.
> > >.
> >
> > Can i initialize them to NULL and only use them if wIndex is not zero
> > and wIndex <= ports.
>
> You can't use a NULL pointer! You have to set it to a non-NULL value
> before you can dereference it.
>
> > I assign them goto error case statement.
>
> I don't understand that sentence.
>
> > That would be a cleaner solution.
>
> It would not be cleaner than leaving the code the way it is.
I was proposing something like the attached.
When you have an oppurtunity to fix or be complaint let us use that so.
I finally leave this here for you to accept or reject.
>
> > >> This is only valid when we have regs->port_status and regs->hostpc and
> > >> 1 more into the actual array but gcc catches this.
> > >
> > > No, it's always valid. The C spec might not agree with me, but the
> > > kernel doesn't use standard C. It uses gcc, which is different from
> > > the standard in quite a few ways.
> > >
> >
> >
> > If you had told me the size of port_status and hostpc is 65536
> > atleast i wouldn't have a problem
>
> I don't see why you have a problem anyway. There's nothing wrong with
> assigning a nonsense value to a pointer, as long as you don't try to
> use it. For example, your compiler might not like this program, but
> the program won't cause an error when you run it:
>
> int a[5];
>
> int main()
> {
> int *x = &a[-1];
>
> return 0;
> }
>
I understand you are initializing a variable x with some value like you
do int x=5 or int x=5-1; I guess the C standard doesn't allow that
(like you said wIndex can be 0 as per the EHCI spec and specs cannot
be argued with).
gcc devels wouldn't even accept a patch because the C standard says
it is undefined behaviour. Maybe due to trap representation . Again
gcc doesn't catch 2nd level access. So even gcc/ubsan is at fault here.
int main()
{
/*This program returns 0 on gcc -fsanitize=undefined tt.c */
int arr[4]={1,2,3,4};
int *ptr=arr;
ptr--;
if(*ptr==ptr[0]) return 0;// should have aborted
return 1;
}
Clang catches the above but when you change it to
if(ptr ==&ptr[0]) it passes instead of aborting if it were
complaint to the spec.
Maybe to speed up things they don't actually track boundaries of
stack variables. Maybe like when you have 2 arrays what happens
if one pointer subtracts or adds and offset that goes exactly
into the address that second array lies. So we have to track
each pointer with valid address and you cannot assign anything
beyond that or use aliases to do that.
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index ffc9029..8e7e4a7 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -872,9 +872,8 @@ int ehci_hub_control(
) {
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
int ports = HCS_N_PORTS (ehci->hcs_params);
- u32 __iomem *status_reg = &ehci->regs->port_status[
- (wIndex & 0xff) - 1];
- u32 __iomem *hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
+ u32 __iomem *status_reg = NULL;
+ u32 __iomem *hostpc_reg = NULL;
u32 temp, temp1, status;
unsigned long flags;
int retval = 0;
@@ -902,6 +901,9 @@ int ehci_hub_control(
case ClearPortFeature:
if (!wIndex || wIndex > ports)
goto error;
+
+ status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
+ hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
wIndex--;
temp = ehci_readl(ehci, status_reg);
temp &= ~PORT_RWC_BITS;
@@ -990,6 +992,8 @@ int ehci_hub_control(
case GetPortStatus:
if (!wIndex || wIndex > ports)
goto error;
+ status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
+ hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
wIndex--;
status = 0;
temp = ehci_readl(ehci, status_reg);
@@ -1159,6 +1163,8 @@ int ehci_hub_control(
}
if (!wIndex || wIndex > ports)
goto error;
+ status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
+ hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
wIndex--;
temp = ehci_readl(ehci, status_reg);
if (tem
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
Hi, Felipe Ferreri Tonello writes: >>> On 30/03/16 13:33, Michal Nazarewicz wrote: On Wed, Mar 30 2016, Felipe Balbi wrote: > a USB packet, right. that's correct. But a struct usb_request can > point to whatever size buffer it wants and UDC is required to split > that into wMaxPacketSize transfers. D’oh. Of course. Disregard all my comments on the patch (except for Ack). >>> >>> I didn't really get it. Does that mean that if buflen is multiple of >>> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into >>> one usb_request and call complete() or it will always call complete() on >>> each [DATA] packet, thus not requiring buflen at all? >>> >>> Does that mean that we can still use buflen and this patch is still >>> valid? (besides the endianess issue that was addressed on v2) >> >> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is >> e.g. 256 bytes, the UDC controller is required to do whatever it needs >> to do to transfer 2048 bytes (or less if there's a short packet). >> >> You don't need to break these 2048 bytes into several requests yourself, >> the UDC is required to do that for the gadget. > > Right, what about OUT endpoints? also applicable -- balbi signature.asc Description: PGP signature
Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework
Hi, Mark Brown writes: > On Fri, Apr 01, 2016 at 08:43:10AM +0300, Felipe Balbi wrote: >> Mark Brown writes: > >> > IIRC Greg didn't want new classes? > >> good point. Still, this doesn't seem to fit a but_type IMO. > > It does in this new world order. IIRC on an earlier round of review > there was some code that didn't use a bus but that got complaints that > it was trying to reimplement the bus functionality. fair enough, I'll wait for Greg to have some time to comment on this. Bottomline is that there is *no* real bus. Charger ICs will use SPI or I2C and that's a real bus, $subject is not. -- balbi signature.asc Description: PGP signature
Re: [RFT PATCH 0/4] usb: dwc2: Fix core reset and force mode delay problems
Hi John, John Youn writes: > The following patch series addresses the core reset and force mode > delay problems we have been seeing on dwc2 for some platforms. > > I think I have identified the source of the inconsistencies between > platforms and this series attempts to address them. > > Basically everything stems from the IDDIG debounce filter delay, which > is a function of the PHY clock speed and can range from 5-50 ms if > enabled. This delay must be taken into account on core reset and force > modes. A full explanation is provided in the patch commit log and code > comments. > > The first two patches are prerequisites to the force mode fixes, > including one patch that was sent separately by Przemek Rudy. I have > resubmitted it with this series for convenience. > > Please help by reviewing and testing on your platforms. > > Patches were tested on: > * Synopsys HAPS platform IP 3.20a OTG, dr_mode=OTG,HOST,PERIPHERAL I'll drop this from my queue. Once someone tests, I'm hoping you can re-send without 'RFT' on subject line. best -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc2: gadget: avoid null dereference on incomplete transfer
Hi,
John Youn writes:
> On 3/30/2016 6:22 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> John Keeping writes:
>>> Setting up a gadget with the uac2 function results in:
>>>
>>> Unable to handle kernel NULL pointer dereference at virtual address
>>> 0058
>>> ...
>>> PC is at dwc2_hsotg_irq+0x7f0/0x908
>>> LR is at dwc2_hsotg_irq+0x4c/0x908
>>> Backtrace:
>>> [] (dwc2_hsotg_irq) from []
>>> (handle_irq_event_percpu+0x130/0x3ec)
>>> [] (handle_irq_event_percpu) from []
>>> (handle_irq_event+0x48/0x6c)
>>>
>>> In all other loops we already skip endpoints that are null, so do so
>>> here as well.
>>>
>>> Signed-off-by: John Keeping
>>> ---
>>> drivers/usb/dwc2/gadget.c | 8 ++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index 0abf73c..df43ec0 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -2606,7 +2606,9 @@ irq_retry:
>>> for (idx = 1; idx < hsotg->num_of_eps; idx++) {
>>> hs_ep = hsotg->eps_in[idx];
>>>
>>> - if (!hs_ep->isochronous || hs_ep->has_correct_parity)
>>> + if (!hs_ep ||
>>> + !hs_ep->isochronous ||
>>> + hs_ep->has_correct_parity)
>>
>> this is fine (even though choice of where to break line is a bit odd),
>> but I have a question about how the rest of the code works (a bit
>> off-topic, sorry)
>>
>>> continue;
>>>
>>> epctl_reg = DIEPCTL(idx);
>>
>> So, this means that the first ISO endpoint without correct parity will
>> be used. Isn't this a bit fragile ? What happens when you use a device
>> with several different interfaces using several different endpoints ?
>>
>> Isn't there a register where we can check which physical endpoint
>> generated the IRQ ? Seems like you really wanna check what:
>>
>
> We discussed this back when the patch was first submitted and
> determined it should work fine like this. I don't remember exactly why
> though.
>
> But this ISOC parity stuff is a workaround and we have a series of
> patches to correctly set up ISOC allowing us to remove it. We're doing
> some final tests before we send them.
fair enough, thanks
--
balbi
signature.asc
Description: PGP signature
Re: cdc-acm: v4.6-rc1: Kernel panic in acm_start_wb
On Mon, 2016-04-04 at 12:31 +0200, Torsten Hilbrich wrote:
> commit a81cf9799ad7299b03a4dff020d9685f9ac5f3e0
> Author: Oliver Neukum
> Date: Wed Feb 10 10:39:49 2016 +0100
>
> cdc-acm: implement put_char() and flush_chars()
Hi,
does this fix the issue?
Regards
Oliver
From e533cede8ccb9f5778f7894ee039f63ac083a334 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Mon, 4 Apr 2016 13:04:44 +0200
Subject: [PATCH] cdc-acm: fix crash if flushed with nothing buffered
Under some circumstances acm_tty_flush_chars() is called
with no buffer to flush. We simply need to do nothing.
Signed-off-by: Oliver Neukum
Reported-by: Torsten Hilbrich
---
drivers/usb/class/cdc-acm.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 83fd30b..a6c4a1b 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -744,11 +744,15 @@ static void acm_tty_flush_chars(struct tty_struct *tty)
int err;
unsigned long flags;
+ if (!cur) /* nothing to do */
+ return;
+
acm->putbuffer = NULL;
err = usb_autopm_get_interface_async(acm->control);
spin_lock_irqsave(&acm->write_lock, flags);
if (err < 0) {
cur->use = 0;
+ acm->putbuffer = cur;
goto out;
}
--
2.1.4
Re: XHCI is slow during boot (bios/efi) and leaves many dmesg messages
On 04.04.2016 12:06, Olliver Schinagl wrote: Hi list, I have a Apple Inc. MacBookPro11,1 (with the most recent 'bios': BIOS MBP111.88Z.0138.B16.1509081438 09/08/2015). At the beginning, USB worked normally. After a while (and after newer kernel versions released by debian?) things started to act strangely. For one, the bios/efi boot takes a very long time (probably due to the same reason I describe later) just to get to the bootloader/grub. Likley resetting and probing for USB ports/mass storage. When grub finally pops up, I can use the (internal USB based keyboard) normally to select a grub entry etc. Booting the kernel then works reasonably fine, until it loads the xhci module. It spews some messages in dmesg (taking some 15 seconds) and only then, the keyboard starts to work again. The log is filled with messages like: [7.248479] xhci_hcd :00:14.0: Command completion event does not match command [7.248495] xhci_hcd :00:14.0: Timeout while waiting for setup device command [ 12.256347] xhci_hcd :00:14.0: Timeout while waiting for setup device command [ 12.256363] usb 1-2: hub failed to enable device, error -62 [ 17.264166] xhci_hcd :00:14.0: Timeout while waiting for setup device command (followed by USB hub/device enumeration) Could be related to the usb wide -> per bus locking changes. xHCI handles both a usb2 and usb3 bus. There's a possible fix here: http://marc.info/?l=linux-usb&m=145493945408601&w=2 Does it help? -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 v4] usb: xhci: applying XHCI_PME_STUCK_QUIRK to Intel BXT B0 host
Hi Mathias,
Robert Dobrowolski writes:
> From: Rafal Redzimski
>
> Broxton B0 also requires XHCI_PME_STUCK_QUIRK.
> Adding PCI device ID for Broxton B and adding to quirk.
>
> Cc:
> Signed-off-by: Rafal Redzimski
> Signed-off-by: Robert Dobrowolski
Any comments to this patch ? Seems like it could go in during the -rc
itself.
> ---
> Changes in v4:
> - Updating commit message
> Changes in v3:
> - Corrected cc list
> Changes in v2:
> - Fixed commit message
>
> drivers/usb/host/xhci-pci.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index f0640b7..071b34a 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -48,6 +48,7 @@
> #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_XHCI 0xa12f
> #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI 0x9d2f
> #define PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI 0x0aa8
> +#define PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI 0x1aa8
>
> static const char hcd_name[] = "xhci_hcd";
>
> @@ -155,7 +156,8 @@ static void xhci_pci_quirks(struct device *dev, struct
> xhci_hcd *xhci)
> (pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI ||
>pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_XHCI ||
>pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
> - pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI)) {
> + pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI ||
> + pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI)) {
> xhci->quirks |= XHCI_PME_STUCK_QUIRK;
> }
> if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
> --
> 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
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
Hi Balbi, On 04/04/16 11:46, Felipe Balbi wrote: > > Hi, > > Felipe Ferreri Tonello writes: On 30/03/16 13:33, Michal Nazarewicz wrote: > On Wed, Mar 30 2016, Felipe Balbi wrote: >> a USB packet, right. that's correct. But a struct usb_request can >> point to whatever size buffer it wants and UDC is required to split >> that into wMaxPacketSize transfers. > > D’oh. Of course. Disregard all my comments on the patch (except for > Ack). > I didn't really get it. Does that mean that if buflen is multiple of wMaxPacketSize, the UDC driver should fit as many [DATA] packets into one usb_request and call complete() or it will always call complete() on each [DATA] packet, thus not requiring buflen at all? Does that mean that we can still use buflen and this patch is still valid? (besides the endianess issue that was addressed on v2) >>> >>> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is >>> e.g. 256 bytes, the UDC controller is required to do whatever it needs >>> to do to transfer 2048 bytes (or less if there's a short packet). >>> >>> You don't need to break these 2048 bytes into several requests yourself, >>> the UDC is required to do that for the gadget. >> >> Right, what about OUT endpoints? > > also applicable > Ok, so I will make few tests here and resend a v3 probably with buflen still. -- Felipe 0x92698E6A.asc Description: application/pgp-keys
[PATCH] usb: gadget: udc-core: remove manual dma configuration
Since commit 7ace8fc8219e ("usb: gadget: udc: core: Fix argument of
dma_map_single for IOMMU") it is not necessary to configure DMA for
usb_gadget device manually, because all DMA operation are performed
using parent/controller device.
Cc: Yoshihiro Shimoda
Signed-off-by: Grygorii Strashko
---
drivers/usb/gadget/udc/udc-core.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index 4151597..e4e70e1 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -371,12 +371,6 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
INIT_WORK(&gadget->work, usb_gadget_state_work);
gadget->dev.parent = parent;
-#ifdef CONFIG_HAS_DMA
- dma_set_coherent_mask(&gadget->dev, parent->coherent_dma_mask);
- gadget->dev.dma_parms = parent->dma_parms;
- gadget->dev.dma_mask = parent->dma_mask;
-#endif
-
if (release)
gadget->dev.release = release;
else
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: dwc3: keystone: drop dma_mask configuration
The Keystone 2 supports DT-boot only, as result dma_mask will be
always configured properly from DT -
of_platform_device_create_pdata()->of_dma_configure(). More over,
dwc3-keystone.c can be built as module and in this case it's unsafe to
assign local variable as dma_mask.
Hence, remove dma_mask configuration code.
Cc: Murali Karicheri
Signed-off-by: Grygorii Strashko
---
drivers/usb/dwc3/dwc3-keystone.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 2be268d..7266470 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -39,8 +39,6 @@
#define USBSS_IRQ_COREIRQ_EN BIT(0)
#define USBSS_IRQ_COREIRQ_CLR BIT(0)
-static u64 kdwc3_dma_mask;
-
struct dwc3_keystone {
struct device *dev;
struct clk *clk;
@@ -108,9 +106,6 @@ static int kdwc3_probe(struct platform_device *pdev)
if (IS_ERR(kdwc->usbss))
return PTR_ERR(kdwc->usbss);
- kdwc3_dma_mask = dma_get_mask(dev);
- dev->dma_mask = &kdwc3_dma_mask;
-
kdwc->clk = devm_clk_get(kdwc->dev, "usb");
error = clk_prepare_enable(kdwc->clk);
--
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: renesas_usbhs: fix to avoid using a disabled ep in usbhsg_queue_done()
This patch fixes an issue that usbhsg_queue_done() may cause kernel
panic when dma callback is running and usb_ep_disable() is called
by interrupt handler. (Especially, we can reproduce this issue using
g_audio with usb-dmac driver.)
For example of a flow:
usbhsf_dma_complete (on tasklet)
--> usbhsf_pkt_handler (on tasklet)
--> usbhsg_queue_done (on tasklet)
*** interrupt happened and usb_ep_disable() is called ***
--> usbhsg_queue_pop (on tasklet)
Then, oops happened.
Fixes: e73a989 ("usb: renesas_usbhs: add DMAEngine support")
Cc: # v3.1+
Signed-off-by: Yoshihiro Shimoda
---
drivers/usb/renesas_usbhs/mod_gadget.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
b/drivers/usb/renesas_usbhs/mod_gadget.c
index 664b263..53d104b 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -158,10 +158,14 @@ static void usbhsg_queue_done(struct usbhs_priv *priv,
struct usbhs_pkt *pkt)
struct usbhs_pipe *pipe = pkt->pipe;
struct usbhsg_uep *uep = usbhsg_pipe_to_uep(pipe);
struct usbhsg_request *ureq = usbhsg_pkt_to_ureq(pkt);
+ unsigned long flags;
ureq->req.actual = pkt->actual;
- usbhsg_queue_pop(uep, ureq, 0);
+ usbhs_lock(priv, flags);
+ if (uep)
+ __usbhsg_queue_pop(uep, ureq, 0);
+ usbhs_unlock(priv, flags);
}
static void usbhsg_queue_push(struct usbhsg_uep *uep,
--
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: cdc-acm: v4.6-rc1: Kernel panic in acm_start_wb
On 04.04.2016 13:07, Oliver Neukum wrote: > On Mon, 2016-04-04 at 12:31 +0200, Torsten Hilbrich wrote: > >> commit a81cf9799ad7299b03a4dff020d9685f9ac5f3e0 >> Author: Oliver Neukum >> Date: Wed Feb 10 10:39:49 2016 +0100 >> >> cdc-acm: implement put_char() and flush_chars() > > Hi, > > does this fix the issue? Yes, this patch solves the problem. The panic was previously well reproducible and went away. Tested-by: Torsten Hilbrich Regards, Torsten -- 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: keystone: drop dma_mask configuration
Hi, Grygorii Strashko writes: > The Keystone 2 supports DT-boot only, as result dma_mask will be > always configured properly from DT - > of_platform_device_create_pdata()->of_dma_configure(). More over, > dwc3-keystone.c can be built as module and in this case it's unsafe to > assign local variable as dma_mask. > > Hence, remove dma_mask configuration code. > > Cc: Murali Karicheri > Signed-off-by: Grygorii Strashko with these two patches from you, does USB Peripheral work on k2 devices ? I'll drop my k2 changes from my series which I sent on saturday. -- balbi signature.asc Description: PGP signature
Re: [patch] usb: gadget: f_midi: unlock on error
On Sat, Apr 02 2016, Dan Carpenter wrote:
> We added some new locking here, but missed an error path where we need
> to unlock.
>
> Fixes: 9acdf4df2fc4 ('usb: gadget: f_midi: added spinlock on transmit
> function')
> Signed-off-by: Dan Carpenter
>
Acked-by: Michal Nazarewicz
But perhaps this would be cleaner:
diff --git a/drivers/usb/gadget/function/f_midi.c
b/drivers/usb/gadget/function/f_midi.c
index 56e2dde..c04436f 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -606,19 +606,14 @@ static void f_midi_transmit(struct f_midi *midi)
goto drop_out;
spin_lock_irqsave(&midi->transmit_lock, flags);
-
- do {
- ret = f_midi_do_transmit(midi, ep);
- if (ret < 0)
- goto drop_out;
- } while (ret);
-
+ while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
+ /* nop */
+ }
spin_unlock_irqrestore(&midi->transmit_lock, flags);
- return;
-
+ if (ret < 0)
drop_out:
- f_midi_drop_out_substreams(midi);
+ f_midi_drop_out_substreams(midi);
}
static void f_midi_in_tasklet(unsigned long data)
Or maybe even this which gets away with gotos all together:
diff --git a/drivers/usb/gadget/function/f_midi.c
b/drivers/usb/gadget/function/f_midi.c
index 56e2dde..91cae60 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -598,27 +598,20 @@ done:
static void f_midi_transmit(struct f_midi *midi)
{
struct usb_ep *ep = midi->in_ep;
- int ret;
unsigned long flags;
+ int ret = -1;
/* We only care about USB requests if IN endpoint is enabled */
- if (!ep || !ep->enabled)
- goto drop_out;
-
- spin_lock_irqsave(&midi->transmit_lock, flags);
-
- do {
- ret = f_midi_do_transmit(midi, ep);
- if (ret < 0)
- goto drop_out;
- } while (ret);
-
- spin_unlock_irqrestore(&midi->transmit_lock, flags);
-
- return;
+ if (ep && ep->enabled) {
+ spin_lock_irqsave(&midi->transmit_lock, flags);
+ while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
+ /* nop */
+ }
+ spin_unlock_irqrestore(&midi->transmit_lock, flags);
+ }
-drop_out:
- f_midi_drop_out_substreams(midi);
+ if (ret < 0)
+ f_midi_drop_out_substreams(midi);
}
static void f_midi_in_tasklet(unsigned long data)
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde..2c0616c 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -609,8 +609,10 @@ static void f_midi_transmit(struct f_midi *midi)
>
> do {
> ret = f_midi_do_transmit(midi, ep);
> - if (ret < 0)
> + if (ret < 0) {
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> goto drop_out;
> + }
> } while (ret);
>
> spin_unlock_irqrestore(&midi->transmit_lock, flags);
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
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: gadget: f_midi: unlock on error
Michal Nazarewicz writes:
> On Sat, Apr 02 2016, Dan Carpenter wrote:
>> We added some new locking here, but missed an error path where we need
>> to unlock.
>>
>> Fixes: 9acdf4df2fc4 ('usb: gadget: f_midi: added spinlock on transmit
>> function')
>> Signed-off-by: Dan Carpenter
>>
>
> Acked-by: Michal Nazarewicz
>
> But perhaps this would be cleaner:
both options below are not real fixes and, as such, as not subject to
the -rc cycle.
thanks
--
balbi
signature.asc
Description: PGP signature
[PATCH] cdc-acm: fix crash if flushed with nothing buffered
Under some circumstances acm_tty_flush_chars() is called
with no buffer to flush. We simply need to do nothing.
Signed-off-by: Oliver Neukum
Reported-by: Torsten Hilbrich
---
drivers/usb/class/cdc-acm.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 83fd30b..a6c4a1b 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -744,11 +744,15 @@ static void acm_tty_flush_chars(struct tty_struct *tty)
int err;
unsigned long flags;
+ if (!cur) /* nothing to do */
+ return;
+
acm->putbuffer = NULL;
err = usb_autopm_get_interface_async(acm->control);
spin_lock_irqsave(&acm->write_lock, flags);
if (err < 0) {
cur->use = 0;
+ acm->putbuffer = cur;
goto out;
}
--
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 2/2] usb: dwc3: gadget: put link to U0 before Start Transfer
On 4/4/2016 12:49 PM, Felipe Balbi wrote:
Synopsys Databook says we should move link to U0
Why "databook" is capitalized? :-)
before issuing a Start Transfer command. We could
require the gadget driver to call
usb_gadget_wakeup() however I feel that changing all
gagdget drivers to keep track of Link State and
Gadget.
conditionally call usb_gadget_wakeup() would be far
too much work. For now we will handle this at the
UDC level, but at some point composite.c should be
one handling this.
The one?
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 552ebdf972b4..404573fcb3c9 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
[...]
@@ -242,12 +244,24 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
* by the same section on Synopsys databook.
*/
if (cmd == DWC3_DEPCMD_STARTTRANSFER) {
+ int needs_wakeup;
+
reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
susphy = true;
reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
}
+
+ needs_wakeup = (dwc->link_state == DWC3_LINK_STATE_U1 ||
+ dwc->link_state == DWC3_LINK_STATE_U2 ||
+ dwc->link_state == DWC3_LINK_STATE_U3);
Parens not needed.
+
Empty line here is hardly needed as well...
+ if (unlikely(needs_wakeup)) {
+ ret = __dwc3_gadget_wakeup(dwc);
+ dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
+ ret);
+ }
}
dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cdc-acm: fix crash if flushed with nothing buffered
Under some circumstances acm_tty_flush_chars() is called
with no buffer to flush. We simply need to do nothing.
Signed-off-by: Oliver Neukum
Reported-by: Torsten Hilbrich
---
drivers/usb/class/cdc-acm.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 83fd30b..a6c4a1b 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -744,11 +744,15 @@ static void acm_tty_flush_chars(struct tty_struct *tty)
int err;
unsigned long flags;
+ if (!cur) /* nothing to do */
+ return;
+
acm->putbuffer = NULL;
err = usb_autopm_get_interface_async(acm->control);
spin_lock_irqsave(&acm->write_lock, flags);
if (err < 0) {
cur->use = 0;
+ acm->putbuffer = cur;
goto out;
}
--
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 2/2] usb: dwc3: gadget: put link to U0 before Start Transfer
Sergei Shtylyov writes:
> On 4/4/2016 12:49 PM, Felipe Balbi wrote:
>
>> Synopsys Databook says we should move link to U0
>
> Why "databook" is capitalized? :-)
because I like capital leters
>> before issuing a Start Transfer command. We could
>> require the gadget driver to call
>> usb_gadget_wakeup() however I feel that changing all
>> gagdget drivers to keep track of Link State and
>
> Gadget.
will fix
>
>> conditionally call usb_gadget_wakeup() would be far
>> too much work. For now we will handle this at the
>> UDC level, but at some point composite.c should be
>> one handling this.
>
> The one?
works either way
>
>> Signed-off-by: Felipe Balbi
>> ---
>> drivers/usb/dwc3/gadget.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 552ebdf972b4..404573fcb3c9 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
> [...]
>> @@ -242,12 +244,24 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned
>> ep,
>> * by the same section on Synopsys databook.
>> */
>> if (cmd == DWC3_DEPCMD_STARTTRANSFER) {
>> +int needs_wakeup;
>> +
>> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
>> susphy = true;
>> reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> }
>> +
>> +needs_wakeup = (dwc->link_state == DWC3_LINK_STATE_U1 ||
>> +dwc->link_state == DWC3_LINK_STATE_U2 ||
>> +dwc->link_state == DWC3_LINK_STATE_U3);
>
> Parens not needed.
I like the clarity of parens
>> +
>
> Empty line here is hardly needed as well...
I like this empty line
--
balbi
signature.asc
Description: PGP signature
Re: [RFC] Create an audit record of USB specific details
On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > From: Wade Mealing > > Gday, > > I'm looking to create an audit trail for when devices are added or removed > from the system. Then please do it in userspace, as I suggested before, that way you catch all types of devices, not just USB ones. Also I don't think you realize that USB interfaces are what are bound to drivers, not USB devices, so that is going to mess with any attempted audit trails here. How are you going to distinguish between the 5 different devices that just got plugged in that all have / as vid/pid for them because they are "cheap" devices from China, yet do totally different things because they are different _types_ of devices? Again, do this in userspace please, that is where it belongs. 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: USB gadgets with configfs hang reboot
On Sat, Apr 02 2016, Alan Stern wrote:
> On Sat, 2 Apr 2016, Michal Nazarewicz wrote:
>> At the same time, mass storage should work fine if it’s bound to
>> multiple configurations. Only one configuration can be active at any
>> given time so interfaces on different configurations cannot interfere
>> with each other.
> Yes, it _should_. But it doesn't with the nokia legacy driver.
> I don't know if this has any connection with configfs; it could be
> a problem with the way f_mass_storage interacts with the composite
> core.
I believe the failure is related to the thread being started twice where
it indeed shouldn’t.
>> The problem we are having is that when mass storage is added to
>> a configuration, fsg_bind is called and it starts the thread.
> This is what I'm not sure about. Which callbacks does the composite
> core invoke when a config is installed or uninstalled?
usb_add_config is what is called to create a usb_configuration. It
initialises the structure and passes it to a callback bind function
(most code removed for brevity):
int usb_add_config(struct usb_composite_dev *cdev,
struct usb_configuration *config,
int (*bind)(struct usb_configuration *))
{
usb_add_config_only(cdev, config);
bind(config);
/* set_alt(), or next bind(), sets up ep->claimed as needed */
usb_ep_autoconfig_reset(cdev->gadget);
return 0;
}
The bind callback calls usb_add_function to add usb_function to a newly
created usb_configuration (again, most code removed for brevity):
int usb_add_function(struct usb_configuration *config,
struct usb_function *function)
{
function->config = config;
value = function->bind(config, function);
return 0;
}
For mass_storage, function->bind is fsg_bind (ditto):
static struct usb_function *fsg_alloc(struct usb_function_instance *fi)
{
struct fsg_dev *fsg kzalloc(sizeof(*fsg), GFP_KERNEL);
fsg->function.name = FSG_DRIVER_DESC;
fsg->function.bind = fsg_bind; /* !!! */
fsg->function.unbind= fsg_unbind;
fsg->function.setup = fsg_setup;
fsg->function.set_alt = fsg_set_alt;
fsg->function.disable = fsg_disable;
fsg->function.free_func = fsg_free;
fsg->common = common;
return &fsg->function;
}
> Those callbacks should be where the thread is started and stopped.
Starting thread in that callback is what is happening right now and
because single usb_function_instance can have multiple usb_function
structures each binding to separate usb_configuration, this causes
problem where the thread is started multiple times.
This is also why the thread is not stopped in fsg_unbind but only once
fsg_common structure is released.
Conceptually, the thread should be started when fsg_common structure is
created (which is at the same time when usb_function_instance is
created) and stopped when fsg_common is released.
At this point, I’m not entirely sure if there is a reason why this isn’t
the case. The only reason I can think of is that starting the thread
right away may be considered wasteful since the thread won’t have
anything to do until the function is bound to a configuration. In the
current code, there may also be issues where perhaps the thread would
not get stopped if fsg_bind has never been called.
Because of all that, I think the best course of action is to just check
whether the thread is running and conditionally start it in fsg_bind,
i.e.:
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2979,20 +2979,7 @@ EXPORT_SYMBOL_GPL(fsg_common_set_inquiry_string);
int fsg_common_run_thread(struct fsg_common *common)
{
- common->state = FSG_STATE_IDLE;
- /* Tell the thread to start working */
- common->thread_task =
- kthread_create(fsg_main_thread, common, "file-storage");
- if (IS_ERR(common->thread_task)) {
- common->state = FSG_STATE_TERMINATED;
- return PTR_ERR(common->thread_task);
- }
-
- DBG(common, "I/O thread pid: %d\n", task_pid_nr(common->thread_task));
-
- wake_up_process(common->thread_task);
-
- return 0;
+ /* kill this function and all call sites */
}
EXPORT_SYMBOL_GPL(fsg_common_run_thread);
@@ -3005,6 +2992,7 @@ static void fsg_common_release(struct kref *ref)
if (common->state != FSG_STATE_TERMINATED) {
raise_exception(common, FSG_STATE_EXIT);
wait_for_completion(&common->thread_notifier);
+ common->thread_task = NULL;
}
for (i = 0; i < ARRAY_SIZE(common->luns); ++i) {
@@ -3050,9 +3038,21 @@ static int fsg_bind(struct usb_configuration *c, struct
usb_function *f)
if (ret)
return ret;
fsg_common_set_inquiry_string(fsg->common, NULL, NULL);
- ret = fsg_
Re: [patch] usb: gadget: f_midi: unlock on error
Hi Michal,
On 04/04/16 13:11, Michal Nazarewicz wrote:
> On Sat, Apr 02 2016, Dan Carpenter wrote:
>> We added some new locking here, but missed an error path where we need
>> to unlock.
>>
>> Fixes: 9acdf4df2fc4 ('usb: gadget: f_midi: added spinlock on transmit
>> function')
>> Signed-off-by: Dan Carpenter
>>
>
> Acked-by: Michal Nazarewicz
>
> But perhaps this would be cleaner:
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde..c04436f 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -606,19 +606,14 @@ static void f_midi_transmit(struct f_midi *midi)
> goto drop_out;
>
> spin_lock_irqsave(&midi->transmit_lock, flags);
> -
> - do {
> - ret = f_midi_do_transmit(midi, ep);
> - if (ret < 0)
> - goto drop_out;
> - } while (ret);
> -
> + while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
> + /* nop */
> + }
> spin_unlock_irqrestore(&midi->transmit_lock, flags);
>
> - return;
> -
> + if (ret < 0)
> drop_out:
> - f_midi_drop_out_substreams(midi);
> + f_midi_drop_out_substreams(midi);
> }
>
> static void f_midi_in_tasklet(unsigned long data)
>
> Or maybe even this which gets away with gotos all together:
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde..91cae60 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -598,27 +598,20 @@ done:
> static void f_midi_transmit(struct f_midi *midi)
> {
> struct usb_ep *ep = midi->in_ep;
> - int ret;
> unsigned long flags;
> + int ret = -1;
>
> /* We only care about USB requests if IN endpoint is enabled */
> - if (!ep || !ep->enabled)
> - goto drop_out;
> -
> - spin_lock_irqsave(&midi->transmit_lock, flags);
> -
> - do {
> - ret = f_midi_do_transmit(midi, ep);
> - if (ret < 0)
> - goto drop_out;
> - } while (ret);
> -
> - spin_unlock_irqrestore(&midi->transmit_lock, flags);
> -
> - return;
> + if (ep && ep->enabled) {
> + spin_lock_irqsave(&midi->transmit_lock, flags);
> + while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
> + /* nop */
> + }
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> + }
>
> -drop_out:
> - f_midi_drop_out_substreams(midi);
> + if (ret < 0)
> + f_midi_drop_out_substreams(midi);
> }
>
> static void f_midi_in_tasklet(unsigned long data)
I am fine with these options, probably the second, but I don't think
they are any clearer than before, so I don't see any real benefits with
the changes.
In fact, I think f_midi_do_transmit should be documented, since there
are 3 possible return condition:
<0 breaks the loop and drop out substreams
0 breaks the loop
>0 continues the loop
>
>
>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>> b/drivers/usb/gadget/function/f_midi.c
>> index 56e2dde..2c0616c 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -609,8 +609,10 @@ static void f_midi_transmit(struct f_midi *midi)
>>
>> do {
>> ret = f_midi_do_transmit(midi, ep);
>> -if (ret < 0)
>> +if (ret < 0) {
>> +spin_unlock_irqrestore(&midi->transmit_lock, flags);
>> goto drop_out;
>> +}
>> } while (ret);
>>
>> spin_unlock_irqrestore(&midi->transmit_lock, flags);
>
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH] usb: dwc3: keystone: drop dma_mask configuration
On 04/04/2016 02:45 PM, Felipe Balbi wrote: Hi, Grygorii Strashko writes: The Keystone 2 supports DT-boot only, as result dma_mask will be always configured properly from DT - of_platform_device_create_pdata()->of_dma_configure(). More over, dwc3-keystone.c can be built as module and in this case it's unsafe to assign local variable as dma_mask. Hence, remove dma_mask configuration code. Cc: Murali Karicheri Signed-off-by: Grygorii Strashko with these two patches from you, does USB Peripheral work on k2 devices ? I've tried CONFIG_USB_DWC3_GADGET=y + g_zero and k2e was detected as gzero dev from Host PC I'll drop my k2 changes from my series which I sent on saturday. Yes, please. -- 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: [PATCH 2/6] usb: dwc3: omap: don't access DMA bits directly
On 04/02/2016 11:28 AM, Felipe Balbi wrote:
> Instead of having a static global just for
> initializing dma_mask directly, let's use
> dma_coerce_mask_and_coherent() for that.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/dwc3/dwc3-omap.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index 22e9606d8e08..c219118bfda0 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -331,8 +331,6 @@ static void dwc3_omap_disable_irqs(struct dwc3_omap *omap)
> dwc3_omap_write_irqmisc_clr(omap, reg);
> }
>
> -static u64 dwc3_omap_dma_mask = DMA_BIT_MASK(32);
> -
> static int dwc3_omap_id_notifier(struct notifier_block *nb,
> unsigned long event, void *ptr)
> {
> @@ -490,7 +488,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
> omap->irq = irq;
> omap->base = base;
> omap->vbus_reg = vbus_reg;
> - dev->dma_mask = &dwc3_omap_dma_mask;
> + dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
I think, It'll be better to just remove DMA configuration code
from this driver and other drivers which support DT-boot mode only.
--
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: [PATCH 5/6] usb: dwc3: core: don't access DMA bits directly
On 04/02/2016 11:28 AM, Felipe Balbi wrote:
> instead of manually copying DMA bits from parent
> device, we should let DMA API do its job.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/dwc3/core.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 17fd81447c9f..d601de20e1cd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -981,11 +981,7 @@ static int dwc3_probe(struct platform_device *pdev)
>
> spin_lock_init(&dwc->lock);
>
> - if (!dev->dma_mask) {
> - dev->dma_mask = dev->parent->dma_mask;
> - dev->dma_parms = dev->parent->dma_parms;
Here, and in most of other patches you've dropped dma_parms copying -
Is it expected?
> - dma_set_coherent_mask(dev, dev->parent->coherent_dma_mask);
> - }
> + dma_coerce_mask_and_coherent(dev, dma_get_mask(dev->parent));
No. Above if case should stay, otherwise, already valid, DMA configuration
might be overwritten:
commit 19bacdc925055f020ad36da04bd72dc8b28637b8
Author: Heikki Krogerus
Date: Wed Sep 24 11:00:38 2014 +0300
usb: dwc3: core: only setting the dma_mask when needed
If the probe drivers have already set the dma_mask, not
replacing the value.
Signed-off-by: Heikki Krogerus
Signed-off-by: Felipe Balbi
--
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: [PATCH 2/2] usb: dwc3: gadget: put link to U0 before Start Transfer
On 4/4/2016 3:38 PM, Felipe Balbi wrote: On 4/4/2016 12:49 PM, Felipe Balbi wrote: Synopsys Databook says we should move link to U0 Why "databook" is capitalized? :-) because I like capital leters before issuing a Start Transfer command. We could require the gadget driver to call usb_gadget_wakeup() however I feel that changing all gagdget drivers to keep track of Link State and Gadget. will fix Thanks and sorry for nitpicking. I sometimes forget that people on this list don't tolerate some of my nits. MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: probably missing patch to stable?
Just to recap:
The description of the original patch d8f00cd685f5 ("usb: hub: do not
clear BOS field during reset device") indicates that it fixes an oops,
but it also had a bug that introduced a different oops (reported by
me). That patch has now been reverted in mainline, fixing the new oops
that I reported but AFAIK re-introducing the original oops. Changbin
has also posted an updated patch that fixes both the original oops and
the new oops ("usb: hub: fix panic caused by NULL bos pointer during
reset device"), but that patch has not yet been merged into mainline.
So perhaps it would be better to merge Changbin's new patch into
mainline and backport that to -stable also, so that both oopses get fixed.
As far as testing goes, Changbin posted a small patch in the thread "Re:
USB oops regression caused by -stable patch", which I tested and it
fixed the oops that I found. But that small patch was before the
original patch d8f00cd685f5 was reverted. Changbin's new patch ("usb:
hub: fix panic caused by NULL bos pointer during reset device") is
equivalent to un-reverting d8f00cd685f5 and applying the small patch
that I already tested. So my testing also applies to Changbin's new patch.
Tony Battersby
Cybernetics
On 04/04/2016 05:26 AM, Roger Quadros wrote:
> Hi Greg,
>
> This commit [1] mentions that it affects certain stable versions but
> I didn't see cc: stable in it nor could find it in any mailing list.
>
> Just wanted to bring to your attention. Thanks.
>
> cheers,
> -roger
>
> [1]
> commit e5bdfd50d6f76077bf8441d130c606229e100d40
> Author: Greg Kroah-Hartman
>
> Revert "usb: hub: do not clear BOS field during reset device"
>
> This reverts commit d8f00cd685f5c8e0def8593e520a7fef12c22407.
>
> Tony writes:
>
> This upstream commit is causing an oops:
> d8f00cd685f5 ("usb: hub: do not clear BOS field during reset device")
>
> This patch has already been included in several -stable kernels. Here
> are the affected kernels:
> 4.5.0-rc4 (current git)
> 4.4.2
> 4.3.6 (currently in review)
> 4.1.18
> 3.18.27
> 3.14.61
>
> How to reproduce the problem:
> Boot kernel with slub debugging enabled (otherwise memory corruption
> will cause random oopses later instead of immediately)
> Plug in USB 3.0 disk to xhci USB 3.0 port
> dd if=/dev/sdc of=/dev/null bs=65536
> (where /dev/sdc is the USB 3.0 disk)
> Unplug USB cable while dd is still going
> Oops is immediate:
>
> Reported-by: Tony Battersby
> Cc: Du, Changbin
> Signed-off-by: Greg Kroah-Hartman
>
--
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: probably missing patch to stable?
On 04/04/16 17:07, Tony Battersby wrote:
> Just to recap:
>
> The description of the original patch d8f00cd685f5 ("usb: hub: do not
> clear BOS field during reset device") indicates that it fixes an oops,
> but it also had a bug that introduced a different oops (reported by
> me). That patch has now been reverted in mainline, fixing the new oops
> that I reported but AFAIK re-introducing the original oops. Changbin
> has also posted an updated patch that fixes both the original oops and
> the new oops ("usb: hub: fix panic caused by NULL bos pointer during
> reset device"), but that patch has not yet been merged into mainline.
> So perhaps it would be better to merge Changbin's new patch into
> mainline and backport that to -stable also, so that both oopses get fixed.
>
> As far as testing goes, Changbin posted a small patch in the thread "Re:
> USB oops regression caused by -stable patch", which I tested and it
> fixed the oops that I found. But that small patch was before the
> original patch d8f00cd685f5 was reverted. Changbin's new patch ("usb:
> hub: fix panic caused by NULL bos pointer during reset device") is
> equivalent to un-reverting d8f00cd685f5 and applying the small patch
> that I already tested. So my testing also applies to Changbin's new patch.
That explains. Thanks :).
cheers,
-roger
>
> Tony Battersby
> Cybernetics
>
> On 04/04/2016 05:26 AM, Roger Quadros wrote:
>> Hi Greg,
>>
>> This commit [1] mentions that it affects certain stable versions but
>> I didn't see cc: stable in it nor could find it in any mailing list.
>>
>> Just wanted to bring to your attention. Thanks.
>>
>> cheers,
>> -roger
>>
>> [1]
>> commit e5bdfd50d6f76077bf8441d130c606229e100d40
>> Author: Greg Kroah-Hartman
>>
>> Revert "usb: hub: do not clear BOS field during reset device"
>>
>> This reverts commit d8f00cd685f5c8e0def8593e520a7fef12c22407.
>>
>> Tony writes:
>>
>> This upstream commit is causing an oops:
>> d8f00cd685f5 ("usb: hub: do not clear BOS field during reset device")
>>
>> This patch has already been included in several -stable kernels. Here
>> are the affected kernels:
>> 4.5.0-rc4 (current git)
>> 4.4.2
>> 4.3.6 (currently in review)
>> 4.1.18
>> 3.18.27
>> 3.14.61
>>
>> How to reproduce the problem:
>> Boot kernel with slub debugging enabled (otherwise memory corruption
>> will cause random oopses later instead of immediately)
>> Plug in USB 3.0 disk to xhci USB 3.0 port
>> dd if=/dev/sdc of=/dev/null bs=65536
>> (where /dev/sdc is the USB 3.0 disk)
>> Unplug USB cable while dd is still going
>> Oops is immediate:
>>
>> Reported-by: Tony Battersby
>> Cc: Du, Changbin
>> Signed-off-by: Greg Kroah-Hartman
>>
>
--
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: function ehci_hub_control in ehci-hub.c
On Mon, 4 Apr 2016, Navin P.S wrote:
> I was proposing something like the attached.
> When you have an oppurtunity to fix or be complaint let us use that so.
>
> I finally leave this here for you to accept or reject.
It's hard to comment on attachments. You should put your patches in
the body of the email.
Here's your patch:
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index ffc9029..8e7e4a7 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -872,9 +872,8 @@ int ehci_hub_control(
) {
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
int ports = HCS_N_PORTS (ehci->hcs_params);
- u32 __iomem *status_reg = &ehci->regs->port_status[
- (wIndex & 0xff) - 1];
- u32 __iomem *hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
+ u32 __iomem *status_reg = NULL;
+ u32 __iomem *hostpc_reg = NULL;
u32 temp, temp1, status;
unsigned long flags;
int retval = 0;
@@ -902,6 +901,9 @@ int ehci_hub_control(
case ClearPortFeature:
if (!wIndex || wIndex > ports)
goto error;
+
+ status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
+ hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
wIndex--;
temp = ehci_readl(ehci, status_reg);
temp &= ~PORT_RWC_BITS;
@@ -990,6 +992,8 @@ int ehci_hub_control(
case GetPortStatus:
if (!wIndex || wIndex > ports)
goto error;
+ status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
+ hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
wIndex--;
status = 0;
temp = ehci_readl(ehci, status_reg);
@@ -1159,6 +1163,8 @@ int ehci_hub_control(
}
if (!wIndex || wIndex > ports)
goto error;
+ status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
+ hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
wIndex--;
temp = ehci_readl(ehci, status_reg);
if (temp & PORT_OWNER)
You want to replace 2 statements with 8 statements that do exactly
the same thing? This does not seem like an improvement.
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: USB gadgets with configfs hang reboot
On Mon, 4 Apr 2016, Michal Nazarewicz wrote:
> On Sat, Apr 02 2016, Alan Stern wrote:
> > On Sat, 2 Apr 2016, Michal Nazarewicz wrote:
> >> At the same time, mass storage should work fine if it’s bound to
> >> multiple configurations. Only one configuration can be active at any
> >> given time so interfaces on different configurations cannot interfere
> >> with each other.
>
> > Yes, it _should_. But it doesn't with the nokia legacy driver.
> > I don't know if this has any connection with configfs; it could be
> > a problem with the way f_mass_storage interacts with the composite
> > core.
>
> I believe the failure is related to the thread being started twice where
> it indeed shouldn’t.
Yes, of course. The questions we need to answer are: Why is the thread
being started twice, and how can we fix it?
> >> The problem we are having is that when mass storage is added to
> >> a configuration, fsg_bind is called and it starts the thread.
>
> > This is what I'm not sure about. Which callbacks does the composite
> > core invoke when a config is installed or uninstalled?
>
> usb_add_config is what is called to create a usb_configuration. It
> initialises the structure and passes it to a callback bind function
> (most code removed for brevity):
Note: I did not ask what happens when a configuration is _created_; I
asked what happens when a configuration is _installed_ -- that is, when
the host sends a Set-Config request.
> int usb_add_config(struct usb_composite_dev *cdev,
> struct usb_configuration *config,
> int (*bind)(struct usb_configuration *))
> {
> usb_add_config_only(cdev, config);
> bind(config);
> /* set_alt(), or next bind(), sets up ep->claimed as needed */
> usb_ep_autoconfig_reset(cdev->gadget);
> return 0;
> }
>
> The bind callback calls usb_add_function to add usb_function to a newly
> created usb_configuration (again, most code removed for brevity):
>
> int usb_add_function(struct usb_configuration *config,
> struct usb_function *function)
> {
> function->config = config;
> value = function->bind(config, function);
> return 0;
> }
So there is no way to add a single function to several configurations?
> For mass_storage, function->bind is fsg_bind (ditto):
>
> static struct usb_function *fsg_alloc(struct usb_function_instance *fi)
> {
> struct fsg_dev *fsg kzalloc(sizeof(*fsg), GFP_KERNEL);
> fsg->function.name = FSG_DRIVER_DESC;
> fsg->function.bind = fsg_bind; /* !!! */
> fsg->function.unbind= fsg_unbind;
> fsg->function.setup = fsg_setup;
> fsg->function.set_alt = fsg_set_alt;
> fsg->function.disable = fsg_disable;
> fsg->function.free_func = fsg_free;
> fsg->common = common;
> return &fsg->function;
> }
>
> > Those callbacks should be where the thread is started and stopped.
>
> Starting thread in that callback is what is happening right now and
> because single usb_function_instance can have multiple usb_function
> structures each binding to separate usb_configuration, this causes
> problem where the thread is started multiple times.
It sounds like there are two problems then. The first problem is that
struct usb_function has a ->disable() callback but no ->enable()
callback. The set_config() routine in composite.c should invoke the
->enable() callback for each function in the config when the config is
installed.
The second problem is that f_mass_storage should start the thread in
the enable callback and stop the thread in the disable callback, rather
than in fsg_bind() and fsg_free_inst() (or wherever).
> This is also why the thread is not stopped in fsg_unbind but only once
> fsg_common structure is released.
This design is a holdover from the days before the composite core
existed and g_file_storage was a standalone gadget driver. It could
stand to be updated.
> Conceptually, the thread should be started when fsg_common structure is
> created (which is at the same time when usb_function_instance is
> created) and stopped when fsg_common is released.
>
> At this point, I’m not entirely sure if there is a reason why this isn’t
> the case. The only reason I can think of is that starting the thread
> right away may be considered wasteful since the thread won’t have
> anything to do until the function is bound to a configuration. In the
> current code, there may also be issues where perhaps the thread would
> not get stopped if fsg_bind has never been called.
Even after the function is bound to a configuration, the thread won't
have anything to do until the configuration is installed by the host.
> Because of all that, I think the best course of action is to just check
> whether the thread is running and conditionally start it in fsg_bind,
> i.e.:
> This should get rid of all the confusion and just do the right thing.
Except that you'll have a bunch of threads hanging
Re: function ehci_hub_control in ehci-hub.c
On Mon, Apr 04, 2016 at 10:38:37AM -0400, Alan Stern wrote:
> On Mon, 4 Apr 2016, Navin P.S wrote:
>
> > I was proposing something like the attached.
> > When you have an oppurtunity to fix or be complaint let us use that so.
> >
> > I finally leave this here for you to accept or reject.
>
> It's hard to comment on attachments. You should put your patches in
> the body of the email.
>
> Here's your patch:
>
> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index ffc9029..8e7e4a7 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -872,9 +872,8 @@ int ehci_hub_control(
> ) {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> int ports = HCS_N_PORTS (ehci->hcs_params);
> - u32 __iomem *status_reg = &ehci->regs->port_status[
> - (wIndex & 0xff) - 1];
> - u32 __iomem *hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
> + u32 __iomem *status_reg = NULL;
> + u32 __iomem *hostpc_reg = NULL;
> u32 temp, temp1, status;
> unsigned long flags;
> int retval = 0;
> @@ -902,6 +901,9 @@ int ehci_hub_control(
> case ClearPortFeature:
> if (!wIndex || wIndex > ports)
> goto error;
> +
> + status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
> + hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
> wIndex--;
> temp = ehci_readl(ehci, status_reg);
> temp &= ~PORT_RWC_BITS;
> @@ -990,6 +992,8 @@ int ehci_hub_control(
> case GetPortStatus:
> if (!wIndex || wIndex > ports)
> goto error;
> + status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
> + hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
> wIndex--;
> status = 0;
> temp = ehci_readl(ehci, status_reg);
> @@ -1159,6 +1163,8 @@ int ehci_hub_control(
> }
> if (!wIndex || wIndex > ports)
> goto error;
> + status_reg = &ehci->regs->port_status[ (wIndex & 0xff) - 1];
> + hostpc_reg = &ehci->regs->hostpc[(wIndex & 0xff) - 1];
> wIndex--;
> temp = ehci_readl(ehci, status_reg);
> if (temp & PORT_OWNER)
>
> You want to replace 2 statements with 8 statements that do exactly
> the same thing? This does not seem like an improvement.
>
I think Yes i have replaced 2 incorrect statements with 8 correct ones
due to below reason.
Ubsan is a feature that is enabled .config , it was supposed to
fix things that were caught at runtime. If ubsan produces an error at
runtime, it gives us an oppurtunity to fix . Ignoring it would defeat
the purpose of ubsan=y in .config.
--
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: XHCI is slow during boot (bios/efi) and leaves many dmesg messages
Hey Mathias, On 04-04-16 13:30, Mathias Nyman wrote: On 04.04.2016 12:06, Olliver Schinagl wrote: Hi list, I have a Apple Inc. MacBookPro11,1 (with the most recent 'bios': BIOS MBP111.88Z.0138.B16.1509081438 09/08/2015). At the beginning, USB worked normally. After a while (and after newer kernel versions released by debian?) things started to act strangely. For one, the bios/efi boot takes a very long time (probably due to the same reason I describe later) just to get to the bootloader/grub. Likley resetting and probing for USB ports/mass storage. When grub finally pops up, I can use the (internal USB based keyboard) normally to select a grub entry etc. Booting the kernel then works reasonably fine, until it loads the xhci module. It spews some messages in dmesg (taking some 15 seconds) and only then, the keyboard starts to work again. The log is filled with messages like: [7.248479] xhci_hcd :00:14.0: Command completion event does not match command [7.248495] xhci_hcd :00:14.0: Timeout while waiting for setup device command [ 12.256347] xhci_hcd :00:14.0: Timeout while waiting for setup device command [ 12.256363] usb 1-2: hub failed to enable device, error -62 [ 17.264166] xhci_hcd :00:14.0: Timeout while waiting for setup device command (followed by USB hub/device enumeration) Could be related to the usb wide -> per bus locking changes. xHCI handles both a usb2 and usb3 bus. There's a possible fix here: http://marc.info/?l=linux-usb&m=145493945408601&w=2 I will try this fix and report on its success, the only things I can say now is that you mention a 6% failure rate of all boots, but what I am seeing is 99.9% failure rate :) But the kernel is compiling so i'll report back when I find anything interesting. Olliver Does it help? -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: function ehci_hub_control in ehci-hub.c
On Mon, 4 Apr 2016, Navin P.S wrote: > > You want to replace 2 statements with 8 statements that do exactly > > the same thing? This does not seem like an improvement. > > > > I think Yes i have replaced 2 incorrect statements with 8 correct ones > due to below reason. The original statements are not incorrect. > Ubsan is a feature that is enabled .config , it was supposed to > fix things that were caught at runtime. If ubsan produces an error at > runtime, it gives us an oppurtunity to fix . Ignoring it would defeat > the purpose of ubsan=y in .config. UBSAN is not always right. This is a case where it is wrong. 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 v7 1/4] gadget: Introduce the usb charger framework
On Mon, Apr 04, 2016 at 01:47:50PM +0300, Felipe Balbi wrote: > Mark Brown writes: > > It does in this new world order. IIRC on an earlier round of review > > there was some code that didn't use a bus but that got complaints that > > it was trying to reimplement the bus functionality. > fair enough, I'll wait for Greg to have some time to comment on > this. Bottomline is that there is *no* real bus. Charger ICs will use I've got a feeling Greg is zoning this out by now... > SPI or I2C and that's a real bus, $subject is not. Well, there's the physical connection between the system power supply and the USB port if you're very keen on looking for hardware. signature.asc Description: PGP signature
Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child
On 4/3/2016 11:28 PM, Felipe Balbi wrote: santosh shilimkar writes: +Arnd, RMK, On 4/1/2016 4:57 AM, Felipe Balbi wrote: Hi, Grygorii Strashko writes: On 04/01/2016 01:20 PM, Felipe Balbi wrote: [...] commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf Author: Yoshihiro Shimoda Date: Mon Jul 13 18:10:05 2015 +0900 usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU The dma_map_single and dma_unmap_single should set "gadget->dev.parent" instead of "&gadget->dev" in the first argument because the parent has a udc controller's device pointer. Otherwise, iommu functions are not called in ARM environment. Signed-off-by: Yoshihiro Shimoda Signed-off-by: Felipe Balbi Above actually means that DMA configuration code can be dropped from usb_add_gadget_udc_release() completely. Right?: true, but now I'm not sure what's better: copy all necessary bits from parent or just pass the parent device to all DMA API. Anybody to shed a light here ? The expectation is drivers should pass the proper dev pointers and let core DMA code deal with it since it knows the per device dma properties. okay, so how do you get proper DMA pointers with something like this: kdwc3_dma_mask = dma_get_mask(dev); dev->dma_mask = &kdwc3_dma_mask; This doesn't anything. Drivers actually needs to touch dma_mask(s) only if the core DMA code hasn't populated it it. I see Grygorii pointed out couple of things already. Reagrds, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB gadgets with configfs hang reboot
Hi,
On 4.04.2016 15:57, Michal Nazarewicz wrote:
On Sat, Apr 02 2016, Alan Stern wrote:
On Sat, 2 Apr 2016, Michal Nazarewicz wrote:
Because of all that, I think the best course of action is to just check
whether the thread is running and conditionally start it in fsg_bind,
i.e.:
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2979,20 +2979,7 @@ EXPORT_SYMBOL_GPL(fsg_common_set_inquiry_string);
int fsg_common_run_thread(struct fsg_common *common)
{
- common->state = FSG_STATE_IDLE;
- /* Tell the thread to start working */
- common->thread_task =
- kthread_create(fsg_main_thread, common, "file-storage");
- if (IS_ERR(common->thread_task)) {
- common->state = FSG_STATE_TERMINATED;
- return PTR_ERR(common->thread_task);
- }
-
- DBG(common, "I/O thread pid: %d\n", task_pid_nr(common->thread_task));
-
- wake_up_process(common->thread_task);
-
- return 0;
+ /* kill this function and all call sites */
}
EXPORT_SYMBOL_GPL(fsg_common_run_thread);
@@ -3005,6 +2992,7 @@ static void fsg_common_release(struct kref *ref)
if (common->state != FSG_STATE_TERMINATED) {
raise_exception(common, FSG_STATE_EXIT);
wait_for_completion(&common->thread_notifier);
+ common->thread_task = NULL;
}
for (i = 0; i < ARRAY_SIZE(common->luns); ++i) {
@@ -3050,9 +3038,21 @@ static int fsg_bind(struct usb_configuration *c, struct
usb_function *f)
if (ret)
return ret;
fsg_common_set_inquiry_string(fsg->common, NULL, NULL);
- ret = fsg_common_run_thread(fsg->common);
- if (ret)
+ }
+
+ if (!common->thread_task) {
+ common->state = FSG_STATE_IDLE;
+ common->thread_task =
+ kthread_create(fsg_main_thread, common, "file-storage");
+ if (IS_ERR(common->thread_task)) {
+ int ret = PTR_ERR(common->thread_task);
+ common->thread_task = NULL;
+ common->state = FSG_STATE_TERMINATED;
return ret;
+ }
+ DBG(common, "I/O thread pid: %d\n",
+ task_pid_nr(common->thread_task));
+ wake_up_process(common->thread_task);
}
fsg->gadget = gadget;
This should get rid of all the confusion and just do the right thing.
Who and when is going to destroy the thread if one does
"/sys/bus/platform/drivers/musb-hdrc# echo musb-hdrc.0.auto > unbind"?
Wouldn't some kind of refcounting make sense here?
Regards,
Ivo
--
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/5 v10] dt/bindings: Add binding for the DA8xx MUSB driver
On 04/04/2016 03:45 AM, Petr Kulhavy wrote: I'm trying to move on with my USB2.0 patch set because my projects is blocked waiting for it. I've seen your PHY patch but it contains only on/off functions and does not handle the clocks, which IMHO should be moved to the PHY driver as well if we decided to do it properly. Have a closer look at my (v3) patchset. It does handle clocks in the phy driver. The clocks themselves are implemented in mach-davinici similar to the rest of the SoC clocks and the phy driver uses clk_get(), clk_prepare_enable(), etc. How shall we proceed if both of us are working on the same piece of code? I have never submitted patches for the Linux kernel before last month, so I have not yet experienced the whole process of getting a patch accepted. So, I don't know an answer to this question. Perhaps someone with more experience can offer advice? -- 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: function ehci_hub_control in ehci-hub.c
On Mon, Apr 04, 2016 at 11:57:27AM -0400, Alan Stern wrote: > On Mon, 4 Apr 2016, Navin P.S wrote: > > > > You want to replace 2 statements with 8 statements that do exactly > > > the same thing? This does not seem like an improvement. > > > > > > > I think Yes i have replaced 2 incorrect statements with 8 correct ones > > due to below reason. > > The original statements are not incorrect. > > > Ubsan is a feature that is enabled .config , it was supposed to > > fix things that were caught at runtime. If ubsan produces an error at > > runtime, it gives us an oppurtunity to fix . Ignoring it would defeat > > the purpose of ubsan=y in .config. > > UBSAN is not always right. This is a case where it is wrong. Can you please provide me with bug id for ubsan/gcc since you say that UBSAN is wrong here ? -- 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: function ehci_hub_control in ehci-hub.c
On Mon, 4 Apr 2016, Navin P.S wrote: > On Mon, Apr 04, 2016 at 11:57:27AM -0400, Alan Stern wrote: > > On Mon, 4 Apr 2016, Navin P.S wrote: > > > > > > You want to replace 2 statements with 8 statements that do exactly > > > > the same thing? This does not seem like an improvement. > > > > > > > > > > I think Yes i have replaced 2 incorrect statements with 8 correct ones > > > due to below reason. > > > > The original statements are not incorrect. > > > > > Ubsan is a feature that is enabled .config , it was supposed to > > > fix things that were caught at runtime. If ubsan produces an error at > > > runtime, it gives us an oppurtunity to fix . Ignoring it would defeat > > > the purpose of ubsan=y in .config. > > > > UBSAN is not always right. This is a case where it is wrong. > > Can you please provide me with bug id for ubsan/gcc since you say > that UBSAN is wrong here ? There is no bug ID as far as I know. 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: USB gadgets with configfs hang reboot
On Mon, Apr 04 2016, Alan Stern wrote: > So there is no way to add a single function to several configurations? There is. f_mass_storage (and any other usb_function_instance) simply has to have multiple usb_function structures. > It sounds like there are two problems then. The first problem is that > struct usb_function has a ->disable() callback but no ->enable() > callback. The set_config() routine in composite.c should invoke the > ->enable() callback for each function in the config when the config is > installed. Well, there is set_alt which is called when configuration is chosen (as well as when interface’s alternative is chosen). It’s not ideal but if we had to we could work with that. > The second problem is that f_mass_storage should start the thread in > the enable callback and stop the thread in the disable callback, > rather than in fsg_bind() and fsg_free_inst() (or wherever). […] > Even after the function is bound to a configuration, the thread won't > have anything to do until the configuration is installed by the host. […] > Except that you'll have a bunch of threads hanging around with nothing > to do. They shouldn't be created until it is known that they will be > needed, and they should be destroyed when it is known that they won't > have any more work to do. I’m not sure how big of a deal that is. The flip side is that equating thread’s lifetime to the lifetime of the function instance is conceptually easier. Even with thread started in fsg_bind this is still less complex than having the thread pop in and out of existence. Furthermore, having it start and stop may lead to unnecessary delays when host switches configurations. This may be an esoteric problem though. I’m not married to any either idea, but right now my patch is a better course of action because it brings a quick fix to the bug. More dramatic changes to thread’s lifetime should be handled by separate patches. -- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving» -- 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: XHCI is slow during boot (bios/efi) and leaves many dmesg messages
Hey Mathias, On 04-04-16 13:30, Mathias Nyman wrote: On 04.04.2016 12:06, Olliver Schinagl wrote: Hi list, I have a Apple Inc. MacBookPro11,1 (with the most recent 'bios': BIOS MBP111.88Z.0138.B16.1509081438 09/08/2015). At the beginning, USB worked normally. After a while (and after newer kernel versions released by debian?) things started to act strangely. For one, the bios/efi boot takes a very long time (probably due to the same reason I describe later) just to get to the bootloader/grub. Likley resetting and probing for USB ports/mass storage. When grub finally pops up, I can use the (internal USB based keyboard) normally to select a grub entry etc. Booting the kernel then works reasonably fine, until it loads the xhci module. It spews some messages in dmesg (taking some 15 seconds) and only then, the keyboard starts to work again. The log is filled with messages like: [7.248479] xhci_hcd :00:14.0: Command completion event does not match command [7.248495] xhci_hcd :00:14.0: Timeout while waiting for setup device command [ 12.256347] xhci_hcd :00:14.0: Timeout while waiting for setup device command [ 12.256363] usb 1-2: hub failed to enable device, error -62 [ 17.264166] xhci_hcd :00:14.0: Timeout while waiting for setup device command (followed by USB hub/device enumeration) Could be related to the usb wide -> per bus locking changes. xHCI handles both a usb2 and usb3 bus. There's a possible fix here: http://marc.info/?l=linux-usb&m=145493945408601&w=2 Does it help? The patch makes no difference. Find attached the dmesg output of a 4.6.0-rc2 with the patch. If there's anything else I should test, feel free to send me a patch. -Mathias [ 47.773043] xhci_hcd :00:14.0: @880037267064 (virt) @37267064 (dma) 0x00 - ep_info2 [ 47.773045] xhci_hcd :00:14.0: @880037267068 (virt) @37267068 (dma) 0x00 - deq [ 47.773046] xhci_hcd :00:14.0: @880037267070 (virt) @37267070 (dma) 0x00 - tx_info [ 47.773047] xhci_hcd :00:14.0: @880037267074 (virt) @37267074 (dma) 0x00 - rsvd[0] [ 47.773049] xhci_hcd :00:14.0: @880037267078 (virt) @37267078 (dma) 0x00 - rsvd[1] [ 47.773050] xhci_hcd :00:14.0: @88003726707c (virt) @3726707c (dma) 0x00 - rsvd[2] [ 47.773051] xhci_hcd :00:14.0: IN Endpoint 01 Context (ep_index 02): [ 47.773053] xhci_hcd :00:14.0: @880037267080 (virt) @37267080 (dma) 0x00 - ep_info [ 47.773054] xhci_hcd :00:14.0: @880037267084 (virt) @37267084 (dma) 0x00 - ep_info2 [ 47.773055] xhci_hcd :00:14.0: @880037267088 (virt) @37267088 (dma) 0x00 - deq [ 47.773056] xhci_hcd :00:14.0: @880037267090 (virt) @37267090 (dma) 0x00 - tx_info [ 47.773058] xhci_hcd :00:14.0: @880037267094 (virt) @37267094 (dma) 0x00 - rsvd[0] [ 47.773059] xhci_hcd :00:14.0: @880037267098 (virt) @37267098 (dma) 0x00 - rsvd[1] [ 47.773061] xhci_hcd :00:14.0: @88003726709c (virt) @3726709c (dma) 0x00 - rsvd[2] [ 47.773062] xhci_hcd :00:14.0: Slot ID 10 Output Context: [ 47.773063] xhci_hcd :00:14.0: Slot Context: [ 47.773064] xhci_hcd :00:14.0: @880083541000 (virt) @83541000 (dma) 0x813 - dev_info [ 47.773065] xhci_hcd :00:14.0: @880083541004 (virt) @83541004 (dma) 0x03 - dev_info2 [ 47.773067] xhci_hcd :00:14.0: @880083541008 (virt) @83541008 (dma) 0x00 - tt_info [ 47.773068] xhci_hcd :00:14.0: @88008354100c (virt) @8354100c (dma) 0x100a - dev_state [ 47.773069] xhci_hcd :00:14.0: @880083541010 (virt) @83541010 (dma) 0x00 - rsvd[0] [ 47.773071] xhci_hcd :00:14.0: @880083541014 (virt) @83541014 (dma) 0x00 - rsvd[1] [ 47.773072] xhci_hcd :00:14.0: @880083541018 (virt) @83541018 (dma) 0x00 - rsvd[2] [ 47.773074] xhci_hcd :00:14.0: @88008354101c (virt) @8354101c (dma) 0x00 - rsvd[3] [ 47.773075] xhci_hcd :00:14.0: IN Endpoint 00 Context (ep_index 00): [ 47.773076] xhci_hcd :00:14.0: @880083541020 (virt) @83541020 (dma) 0x01 - ep_info [ 47.773077] xhci_hcd :00:14.0: @880083541024 (virt) @83541024 (dma) 0x400026 - ep_info2 [ 47.773079] xhci_hcd :00:14.0: @880083541028 (virt) @83541028 (dma) 0x8361e031 - deq [ 47.773080] xhci_hcd :00:14.0: @880083541030 (virt) @83541030 (dma) 0x00 - tx_info [ 47.773081] xhci_hcd :00:14.0: @880083541034 (virt) @83541034 (dma) 0x00 - rsvd[0] [ 47.773083] xhci_hcd :00:14.0: @880083541038 (virt) @83541038 (dma) 0x00 - rsvd[1] [ 47.773084] xhci_hcd :00:14.0: @88008354103c (virt) @8354103c (dma) 0x00 - rsvd[2] [ 47.773085] xhci_hcd :00:14.0: OUT Endpoint 01 Context (ep_index 01): [ 47.773087] xhci_hcd :00:14.0: @880083541040 (virt) @83541040 (dma) 0x00 - ep_info [ 47.773088] xhci_hcd :00:14.0: @880083541044 (virt) @8354104
Re: [PATCH v7 1/4] gadget: Introduce the usb charger framework
On Mon, Apr 04, 2016 at 09:04:48AM -0700, Mark Brown wrote: > On Mon, Apr 04, 2016 at 01:47:50PM +0300, Felipe Balbi wrote: > > Mark Brown writes: > > > > It does in this new world order. IIRC on an earlier round of review > > > there was some code that didn't use a bus but that got complaints that > > > it was trying to reimplement the bus functionality. > > > fair enough, I'll wait for Greg to have some time to comment on > > this. Bottomline is that there is *no* real bus. Charger ICs will use > > I've got a feeling Greg is zoning this out by now... Very true :) I have a lot of other patches to go through this week, it will be a while before I get to these... 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: USB gadgets with configfs hang reboot
On Mon, Apr 04 2016, Ivaylo Dimitrov wrote: > Who and when is going to destroy the thread if one does > "/sys/bus/platform/drivers/musb-hdrc# echo musb-hdrc.0.auto > unbind"? > Wouldn't some kind of refcounting make sense here? Currently the thread is killed when fsg_common structure is released and that structure has a reference counter. -- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving» -- 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: gadget: f_midi: unlock on error
> On 04/04/16 13:11, Michal Nazarewicz wrote:
>> Or maybe even this which gets away with gotos all together:
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>> b/drivers/usb/gadget/function/f_midi.c
>> index 56e2dde..91cae60 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -598,27 +598,20 @@ done:
>> static void f_midi_transmit(struct f_midi *midi)
>> {
>> struct usb_ep *ep = midi->in_ep;
>> - int ret;
>> unsigned long flags;
>> + int ret = -1;
>>
>> /* We only care about USB requests if IN endpoint is enabled */
>> - if (!ep || !ep->enabled)
>> - goto drop_out;
>> -
>> - spin_lock_irqsave(&midi->transmit_lock, flags);
>> -
>> - do {
>> - ret = f_midi_do_transmit(midi, ep);
>> - if (ret < 0)
>> - goto drop_out;
>> - } while (ret);
>> -
>> - spin_unlock_irqrestore(&midi->transmit_lock, flags);
>> -
>> - return;
>> + if (ep && ep->enabled) {
>> + spin_lock_irqsave(&midi->transmit_lock, flags);
>> + while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
>> + /* nop */
>> + }
>> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
>> + }
>>
>> -drop_out:
>> - f_midi_drop_out_substreams(midi);
>> + if (ret < 0)
>> + f_midi_drop_out_substreams(midi);
>> }
>>
>> static void f_midi_in_tasklet(unsigned long data)
On Mon, Apr 04 2016, Felipe Ferreri Tonello wrote:
> I am fine with these options, probably the second, but I don't think
> they are any clearer than before, so I don't see any real benefits with
> the changes.
The spin_lock_irqsave is paired with exactly one spin_unlock_irqrestore
which is cleaner in my book. I don’t have strong feelings about this
though.
> In fact, I think f_midi_do_transmit should be documented, since there
> are 3 possible return condition:
> <0 breaks the loop and drop out substreams
> 0 breaks the loop
> >0 continues the loop
That’s of course separate issue.
--
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB gadgets with configfs hang reboot
On Mon, 4 Apr 2016, Michal Nazarewicz wrote:
> On Mon, Apr 04 2016, Alan Stern wrote:
> > So there is no way to add a single function to several configurations?
>
> There is. f_mass_storage (and any other usb_function_instance) simply
> has to have multiple usb_function structures.
>
> > It sounds like there are two problems then. The first problem is that
> > struct usb_function has a ->disable() callback but no ->enable()
> > callback. The set_config() routine in composite.c should invoke the
> > ->enable() callback for each function in the config when the config is
> > installed.
>
> Well, there is set_alt which is called when configuration is chosen (as
> well as when interface’s alternative is chosen). It’s not ideal but if
> we had to we could work with that.
I suppose so.
> > Except that you'll have a bunch of threads hanging around with nothing
> > to do. They shouldn't be created until it is known that they will be
> > needed, and they should be destroyed when it is known that they won't
> > have any more work to do.
>
> I’m not sure how big of a deal that is. The flip side is that equating
> thread’s lifetime to the lifetime of the function instance is
> conceptually easier. Even with thread started in fsg_bind this is still
> less complex than having the thread pop in and out of existence.
True. (Provided one understands that a function instance corresponds
to the usb_function structure, not the usb_function_instance
structure.)
> Furthermore, having it start and stop may lead to unnecessary delays
> when host switches configurations. This may be an esoteric problem
> though.
>
> I’m not married to any either idea, but right now my patch is a better
> course of action because it brings a quick fix to the bug. More
> dramatic changes to thread’s lifetime should be handled by separate
> patches.
Okay. However, I noticed your patch does:
> + if (!common->thread_task) {
> + common->state = FSG_STATE_IDLE;
> + common->thread_task =
> + kthread_create(fsg_main_thread, common,
in fsg_bind(). How could thread_task ever be non-NULL at this point?
Wouldn't that mean the usb_function structure was registered twice?
Which brings us back to the nokia driver. Apparently it _does_ manage
to create the thread twice. How can this happen? The function that
nokia_bind_config() registers is created dynamically:
f_msg = usb_get_function(fi_msg);
which goes through fsg_alloc().
Aha, and now I see the problem. fsg_alloc() shares opts->common among
all the fsg structures it creates. This means all the function
instances will share common->thread_task, because they all share
common. The same goes for common->state and a bunch of other fields.
It looks like a lot of stuff has to move out of fsg->common and into
fsg.
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/2] usb: dwc3: core: Introduce dwc3_device_reinit()
Roger I am sorry but this mail chain is unreadable!! I am not able to filter out the >>>... Please send me a clean version of the mail if possible...i will be unable to respond without that.. ---Abhishek -Original Message- From: Felipe Balbi [mailto:ba...@kernel.org] Sent: Monday, April 04, 2016 3:11 AM To: Quadros, Roger; John Youn Cc: Nori, Sekhar; linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Shankar, Abhishek; B, Ravi Subject: Re: [PATCH 1/2] usb: dwc3: core: Introduce dwc3_device_reinit() Hi, Roger Quadros writes: We will need this function for a workaround. The function issues a softreset only to the device controller and performs minimal re-initialization so that the device controller can be usable. As some code is similar to dwc3_core_init() take out common code into dwc3_get_gctl_quirks(). We add a new member (prtcap_mode) to struct dwc3 to keep track of the current mode in the PRTCAPDIR register. Signed-off-by: Roger Quadros >>> >>> I must say, I don't like this at all :-p There's ONE known >>> silicon which needs this because of a poor silicon >>> integration which took an IP with a known erratum where it >>> can't be made to work on lower speeds and STILL was integrated >>> without a superspeed PHY. >>> >>> There's a reason why I never tried to push this upstream >>> myself ;-) >>> >>> I'm really thinking we might be better off adding a quirk >>> flag to skip the metastability workaround and allow this ONE >>> silicon to set the controller to lower speed. >>> >>> John, can you check with your colleagues if we would ever >>> fall into >>> STAR#9000525659 if we set maximum speed to high speed during >>> driver probe and never touch it again ? I would assume we >>> don't really fall into the metastability workaround, right ? >>> We're not doing any sort of PM for dwc3... >>> Hi Felipe, Do you mean to keep DCFG.speed to SS and set dwc->maximum_speed to HS? I don't see an issue with that as long as we always ignore dwc->maximum_speed when programming DCFG.speed for all affected versions of the core. As long as the DCFG.speed = SS, you should not hit the STAR. >>> >>> I actually mean changing DCFG.speed during driver probe and >>> never touching it again. Would that still cause problems ? >>> >> >> In that case I'm not sure. The engineer who would know is off >> until next week so I'll get back to you as soon as I can talk to >> him about it. > So the engineers said that this issue can occur while set to HS and the run/stop bit is changed so it seems that won't work. >>> >>> Thanks John. >>> >>> Felipe, >>> >>> any suggestion how we can fix this upstream? >> >> no idea, I don't have a lot of memory about this problem. I really >> don't remember the details about this, is there an openly available >> errata document which I could read ? /me goes search for it. >> >> I found [1] which tells me, the following: >> >> >> | i819| A Device Control Bit Meta-Stability for USB3.0 Controller in >> USB2.0 Mode | >> |-+| >> | Criticality | Medium >> | >> | | >> | >> | Descritiion | When USB3.0 controller core is programmed to be a USB >> 2.0-only device | >> | | hardware meta-stability on USB_DCTL[31]RUNSTOP bit causing >> the core to | >> | | attempt high speed as well as SuperSpeed connection or >> completely miss | >> | | the attach request. >> | >> | | >> | >> | Workaround | If the requirement is to always function in USB 2.0 mode, >> there is no | >> | | workaround. >> | >> | | Otherwise, you can always program the USB controller core to >> be SuperSpeed | >> | | 3.0 capable (USB_DCFG[2:0]DEVSPD = 0x4) >> | >> | | >> | >> | Revisions | SR 1.1, 2.0 >> | >> | Impacted| >> | >> |-+---
Re: usb 3.0 stopped working with 4.6.0 rc1
ok, i applied 59b9023 usb: fix regression in SuperSpeed endpoint descriptor parsing to linus rc2. everything is working again :) Abr 04 18:07:55 hydra systemd[1]: Starting Stop ureadahead data collection... Abr 04 18:07:55 hydra systemd[1]: Stopped Read required files in advance. Abr 04 18:07:55 hydra systemd[1]: Started Stop ureadahead data collection. Abr 04 18:07:59 hydra kernel: usb 3-1: new SuperSpeed USB device number 2 using xhci_hcd Abr 04 18:07:59 hydra kernel: usb 3-1: New USB device found, idVendor=0bc2, idProduct=2312 Abr 04 18:07:59 hydra kernel: usb 3-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 Abr 04 18:07:59 hydra kernel: usb 3-1: Product: Expansion Abr 04 18:07:59 hydra kernel: usb 3-1: Manufacturer: Seagate Abr 04 18:07:59 hydra kernel: usb 3-1: SerialNumber: 2HC015KJ Abr 04 18:07:59 hydra mtp-probe[2138]: checking bus 3, device 2: "/sys/devices/pci:00/:00:14.0/usb3/3-1" Abr 04 18:07:59 hydra mtp-probe[2138]: bus: 3, device: 2 was not an MTP device Abr 04 18:07:59 hydra kernel: usbcore: registered new interface driver usb-storage Abr 04 18:07:59 hydra kernel: scsi host3: uas Abr 04 18:07:59 hydra kernel: scsi 3:0:0:0: Direct-Access Seagate Expansion0636 PQ: 0 ANSI: 6 Abr 04 18:07:59 hydra kernel: usbcore: registered new interface driver uas Abr 04 18:07:59 hydra kernel: sd 3:0:0:0: Attached scsi generic sg2 type 0 Abr 04 18:07:59 hydra kernel: sd 3:0:0:0: [sdb] Spinning up disk... Abr 04 18:08:03 hydra kernel: .ready Abr 04 18:08:03 hydra kernel: sd 3:0:0:0: [sdb] 1465149167 512-byte logical blocks: (750 GB/699 GiB) Abr 04 18:08:03 hydra kernel: sd 3:0:0:0: [sdb] Write Protect is off Abr 04 18:08:03 hydra kernel: sd 3:0:0:0: [sdb] Mode Sense: 2b 00 10 08 Abr 04 18:08:03 hydra kernel: sd 3:0:0:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA Abr 04 18:08:03 hydra kernel: sdb: sdb1 sdb2 Abr 04 18:08:03 hydra kernel: sd 3:0:0:0: [sdb] Attached SCSI disk | Paulo Dias | paulo.miguel.d...@gmail.com Tempora mutantur, nos et mutamur in illis. On Mon, Apr 4, 2016 at 5:11 AM, Mathias Nyman wrote: > On 02.04.2016 02:29, Paulo Dias wrote: >> >> Would you mind sending me the patches , so i could test with my >> external HD? even with today git build im getting all but errors with >> this controller and the driver. with today build i even have one >> shutdown usb 3.0 port! >> > > Regression fix is in Gregs usb-linus tree: > > 59b9023 usb: fix regression in SuperSpeed endpoint descriptor parsing > > It's not yet in any rc kernel. I hope it will be in rc-3 > Does Gregs usb-linus branch work for you? > > -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: [RFC] Create an audit record of USB specific details
On Monday, April 04, 2016 05:56:26 AM Greg KH wrote: > On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > > From: Wade Mealing > > > > Gday, > > > > I'm looking to create an audit trail for when devices are added or removed > > from the system. > > Then please do it in userspace, as I suggested before, that way you > catch all types of devices, not just USB ones. > > Also I don't think you realize that USB interfaces are what are bound to > drivers, not USB devices, so that is going to mess with any attempted > audit trails here. How are you going to distinguish between the 5 > different devices that just got plugged in that all have / as > vid/pid for them because they are "cheap" devices from China, yet do > totally different things because they are different _types_ of devices? This sounds like vid/pid should be captured in the event. > Again, do this in userspace please, that is where it belongs. There is one issue that may need some clarification. The audit system has to do everything possible to make sure that an event is captured and logged. Does the uevent netlink protocol ever drop events because the user space queue is full? If the uevent interface drops events, then its not audit quality in terms of doing everything possible to prevent the loss of a record. If this were to happen, how would user space find out when a uevent gets dropped? I may have to panic the machine if that happens depending on the configured policy. So, we need to know when it happens. If on the otherhand it doesn't ever drop events, then it might be usable. -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 v5 1/4] leds: core: add generic support for RGB Color LED's
Hi Pavel, On 04/01/2016 11:18 PM, Pavel Machek wrote: Hi! It would have the same downsides as in case of having r, g and b in separate attributes, i.e. - problems with setting LED colour in a consistent way. This way LED blinking in whatever colour couldn't be supported reliably. It was one of your primary rationale standing behind this design, if I remember correctly. Second - what about triggers? We've had a long discussion about it and this design turned out to be most fitting. Are on/off triggers really that useful for a LED that can produce 16 million colors? I believe we should support patterns for RGB LEDs. Something like [ (time, r, g, b), ... ] . Ok, what about this one? Lets say we have /sys/class/pattern/lp5533::0 /sys/class/pattern/software::0 /sys/class/led/n900::red ; default trigger "lp5533::0:0" /sys/class/led/n900::green ; default trigger "lp5533::0:1" /sys/class/led/n900::blue ; default trigger "lp5533::0:2" Normally, pattern would correspond to one RGB LED. We could have attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for this pattern. Could you give an example on how to set a color for RGB LED using this interface? Would it be compatible with LED triggers? Where the "pattern" class would be implemented? This involves the same issue you were opposed to: three values per sysfs attribute. And solves a lot of other things. Like actually being backwards compatible. And yes, it involves three values in a file, but now it is array of led brightnesses, and that might actually be acceptable. (At least the values have uniform meaning). > Plus, it is not "issue you were opposed to" it is "something that is not permitted by sysfs maintainers". -- Best regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Create an audit record of USB specific details
On Monday, April 04, 2016 12:02:42 AM wmealing wrote: > I'm looking to create an audit trail for when devices are added or removed > from the system. > > The audit subsystem is a logging subsystem in kernel space that can be > used to create advanced filters on generated events. It has partnered > userspace utilities ausearch, auditd, aureport, auditctl which work > exclusively on audit records. > > These tools are able to set filters to "trigger" on specific in-kernel > events specified by privileged users. While the userspace tools can create > audit events these are not able to be handled intelligently > (decoded,filtered or ignored) as kernel generated audit events are. > > I have this working at the moment with the USB subsystem (as an example). > Its been suggested that I use systemd-udev however this means that the audit > tools (ausearch) will not be able to index these records. > > Here is an example of picking out the AUDIT_DEVICE record type for example. > > > # ausearch -l -i -ts today -m AUDIT_DEVICE > > > > type=AUDIT_DEVICE msg=audit(31/03/16 16:37:15.642:2) : action=add > > manufacturer=Linux 4.4.0-ktest ehci_hcd product=EHCI Host Controller > > serial=:00:06.7 major=189 minor=0 bus="usb" About this event's format...we can't have any spaces in the value side of the name=value fields unless its encoded as an untrusted string. You can replace spaces with an underscore or dash for readability. So, manufacturer and product would need this treatment. -Steve > Admittedly this is only the USB device type at the moment, but I'd like to > break this out into other bus types at some time in the future, gotta start > somewhere. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Create an audit record of USB specific details
On Monday, April 04, 2016 05:56:26 AM Greg KH wrote: > On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > > From: Wade Mealing > > > > Gday, > > > > I'm looking to create an audit trail for when devices are added or removed > > from the system. > > Then please do it in userspace, as I suggested before, that way you > catch all types of devices, not just USB ones. Audit has some odd requirements placed on it by some of its users. I think most notable in this particular case is the need to take specific actions, including panicking the system, when audit records can't be sent to userspace and are "lost". Granted, it's an odd requirement, definitely not the norm/default configuration, but supporting weird stuff like this has allowed Linux to be used on some pretty interesting systems that wouldn't have been possible otherwise. Looking quickly at some of the kobject/uvent code, it doesn't appear that the uevent/netlink channel has this capability. It also just noticed that it looks like userspace can send fake uevent messages; I haven't looked at it closely enough yet, but that may be a concern for users which restrict/subdivide root using a LSM ... although it is possible that the LSM policy could help here. I'm thinking aloud a bit right now, but for SELinux the netlink controls aren't very granular and sysfs can be tricky so I can't say for certain about blocking fake events from userspace using LSMs/SELinux. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Create an audit record of USB specific details
On Mon, 2016-04-04 at 17:37 -0400, Steve Grubb wrote: > On Monday, April 04, 2016 12:02:42 AM wmealing wrote: > > I'm looking to create an audit trail for when devices are added or removed > > from the system. > > > > The audit subsystem is a logging subsystem in kernel space that can be > > used to create advanced filters on generated events. It has partnered > > userspace utilities ausearch, auditd, aureport, auditctl which work > > exclusively on audit records. > > > > These tools are able to set filters to "trigger" on specific in-kernel > > events specified by privileged users. While the userspace tools can create > > audit events these are not able to be handled intelligently > > (decoded,filtered or ignored) as kernel generated audit events are. > > > > I have this working at the moment with the USB subsystem (as an example). > > Its been suggested that I use systemd-udev however this means that the audit > > tools (ausearch) will not be able to index these records. > > > > Here is an example of picking out the AUDIT_DEVICE record type for example. > > > > > # ausearch -l -i -ts today -m AUDIT_DEVICE > > > > > > type=AUDIT_DEVICE msg=audit(31/03/16 16:37:15.642:2) : action=add > > > manufacturer=Linux 4.4.0-ktest ehci_hcd product=EHCI Host Controller > > > serial=:00:06.7 major=189 minor=0 bus="usb" > > About this event's format...we can't have any spaces in the value side of the > name=value fields unless its encoded as an untrusted string. You can replace > spaces with an underscore or dash for readability. So, manufacturer and > product would need this treatment. > > -Steve I think you'll find the original event has properly encoded strings ... note the '-i' on the ausearch. > > > Admittedly this is only the USB device type at the moment, but I'd like to > > break this out into other bus types at some time in the future, gotta start > > somewhere. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Create an audit record of USB specific details
On Mon, Apr 04, 2016 at 05:37:01PM -0400, Steve Grubb wrote: > On Monday, April 04, 2016 12:02:42 AM wmealing wrote: > > I'm looking to create an audit trail for when devices are added or removed > > from the system. > > > > The audit subsystem is a logging subsystem in kernel space that can be > > used to create advanced filters on generated events. It has partnered > > userspace utilities ausearch, auditd, aureport, auditctl which work > > exclusively on audit records. > > > > These tools are able to set filters to "trigger" on specific in-kernel > > events specified by privileged users. While the userspace tools can create > > audit events these are not able to be handled intelligently > > (decoded,filtered or ignored) as kernel generated audit events are. > > > > I have this working at the moment with the USB subsystem (as an example). > > Its been suggested that I use systemd-udev however this means that the audit > > tools (ausearch) will not be able to index these records. > > > > Here is an example of picking out the AUDIT_DEVICE record type for example. > > > > > # ausearch -l -i -ts today -m AUDIT_DEVICE > > > > > > type=AUDIT_DEVICE msg=audit(31/03/16 16:37:15.642:2) : action=add > > > manufacturer=Linux 4.4.0-ktest ehci_hcd product=EHCI Host Controller > > > serial=:00:06.7 major=189 minor=0 bus="usb" > > About this event's format...we can't have any spaces in the value side of the > name=value fields unless its encoded as an untrusted string. You can replace > spaces with an underscore or dash for readability. So, manufacturer and > product would need this treatment. What is the character encoding that audit messages can accept? Does it match up with the character encoding that USB strings are in? 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: [RFC] Create an audit record of USB specific details
On Mon, Apr 04, 2016 at 05:33:10PM -0400, Steve Grubb wrote: > On Monday, April 04, 2016 05:56:26 AM Greg KH wrote: > > On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > > > From: Wade Mealing > > > > > > Gday, > > > > > > I'm looking to create an audit trail for when devices are added or removed > > > from the system. > > > > Then please do it in userspace, as I suggested before, that way you > > catch all types of devices, not just USB ones. > > > > Also I don't think you realize that USB interfaces are what are bound to > > drivers, not USB devices, so that is going to mess with any attempted > > audit trails here. How are you going to distinguish between the 5 > > different devices that just got plugged in that all have / as > > vid/pid for them because they are "cheap" devices from China, yet do > > totally different things because they are different _types_ of devices? > > This sounds like vid/pid should be captured in the event. The code did that, the point is, vid/pid means nothing in the real world. So why are you going to audit anything based on it? :) > > Again, do this in userspace please, that is where it belongs. > > There is one issue that may need some clarification. The audit system has to > do > everything possible to make sure that an event is captured and logged. Does > the uevent netlink protocol ever drop events because the user space queue is > full? If the uevent interface drops events, then its not audit quality in > terms of doing everything possible to prevent the loss of a record. If this > were to happen, how would user space find out when a uevent gets dropped? I > may > have to panic the machine if that happens depending on the configured policy. > So, we need to know when it happens. If on the otherhand it doesn't ever drop > events, then it might be usable. I have never seen it drop events, have you? It's been pretty reliable for the past 10+ years :) 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: [RFC] Create an audit record of USB specific details
On Mon, Apr 04, 2016 at 05:37:58PM -0400, Paul Moore wrote: > On Monday, April 04, 2016 05:56:26 AM Greg KH wrote: > > On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > > > From: Wade Mealing > > > > > > Gday, > > > > > > I'm looking to create an audit trail for when devices are added or removed > > > from the system. > > > > Then please do it in userspace, as I suggested before, that way you > > catch all types of devices, not just USB ones. > > Audit has some odd requirements placed on it by some of its users. I think > most notable in this particular case is the need to take specific actions, > including panicking the system, when audit records can't be sent to userspace > and are "lost". Granted, it's an odd requirement, definitely not the > norm/default configuration, but supporting weird stuff like this has allowed > Linux to be used on some pretty interesting systems that wouldn't have been > possible otherwise. Looking quickly at some of the kobject/uvent code, it > doesn't appear that the uevent/netlink channel has this capability. Are you sure you can loose netlink messages? If you do, you know you lost them, so isn't that good enough? > It also just noticed that it looks like userspace can send fake uevent > messages; That's how your machine boots properly :) > I haven't looked at it closely enough yet, but that may be a concern > for users which restrict/subdivide root using a LSM ... although it is > possible that the LSM policy could help here. I'm thinking aloud a bit right > now, but for SELinux the netlink controls aren't very granular and sysfs can > be tricky so I can't say for certain about blocking fake events from > userspace > using LSMs/SELinux. uevents are not tied into LSMs from what I can tell, so I don't understand wht you are talking about here, sorry. 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: [RFC] Create an audit record of USB specific details
On Mon, Apr 04, 2016 at 02:48:43PM -0700, Greg KH wrote: > On Mon, Apr 04, 2016 at 05:33:10PM -0400, Steve Grubb wrote: > > On Monday, April 04, 2016 05:56:26 AM Greg KH wrote: > > > On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > > > > From: Wade Mealing > > > > > > > > Gday, > > > > > > > > I'm looking to create an audit trail for when devices are added or > > > > removed > > > > from the system. > > > > > > Then please do it in userspace, as I suggested before, that way you > > > catch all types of devices, not just USB ones. > > > > > > Also I don't think you realize that USB interfaces are what are bound to > > > drivers, not USB devices, so that is going to mess with any attempted > > > audit trails here. How are you going to distinguish between the 5 > > > different devices that just got plugged in that all have / as > > > vid/pid for them because they are "cheap" devices from China, yet do > > > totally different things because they are different _types_ of devices? > > > > This sounds like vid/pid should be captured in the event. > > The code did that, the point is, vid/pid means nothing in the real > world. So why are you going to audit anything based on it? :) Oh wait, it's worse, it is logging strings, which are even more unreliable than vid/pid values. It's pretty obvious this has not been tested on any large batch of real-world devices, or thought through as to why any of this is even needed at all. So why is this being added? Who needs/wants this? What are their requirements here? From what I recall, the author is just messing around with the USB subsystem and audit as something fun to do, which is great, but don't expect it to be mergable :) 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: [RFC] Create an audit record of USB specific details
That is a good question, maybe I've been lucky in the devices that I have been testing with. Most of them seem to be ascii, my assumption was that shouldn't be a problem. The same encoding function used by the path audit_log_d_path, definitely audits UTF8 named files: # ausearch -i -f /tmp/test/권성주.txt type=PATH msg=audit(04/04/16 21:05:00.521:1638) : item=0 name=/tmp/권성주.txt inode=627534 dev=fd:00 mode=file,664 ouid=wmealing ogid=wmealing rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 objtype=NORMAL # ausearch -f /tmp/test/권성주.txt type=PATH msg=audit(1459818300.521:1638): item=0 name=2F746D702FEAB68CEC84B1ECA3BC2E747874 inode=627534 dev=fd:00 mode=0100664 ouid=1000 ogid=1000 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 objtype=NORMAL Thanks, Wade Mealing. On Tue, Apr 5, 2016 at 11:51 AM, Wade Mealing wrote: > That is a good question, maybe I've been lucky in the devices that I have > been testing with. Most of them seem to be ascii, my assumption was that > shouldn't be a problem. The same encoding function used by the path > audit_log_d_path, definitely audits UTF8 named files: > > # ausearch -i -f /tmp/test/권성주.txt > > type=PATH msg=audit(04/04/16 21:05:00.521:1638) : item=0 name=/tmp/권성주.txt > inode=627534 dev=fd:00 mode=file,664 ouid=wmealing ogid=wmealing rdev=00:00 > obj=unconfined_u:object_r:user_tmp_t:s0 objtype=NORMAL > > # ausearch -f /tmp/test/권성주.txt > > type=PATH msg=audit(1459818300.521:1638): item=0 > name=2F746D702FEAB68CEC84B1ECA3BC2E747874 inode=627534 dev=fd:00 > mode=0100664 ouid=1000 ogid=1000 rdev=00:00 > obj=unconfined_u:object_r:user_tmp_t:s0 objtype=NORMAL > > Thanks, > > Wade Mealing. > > On Tue, Apr 5, 2016 at 7:54 AM, Greg KH wrote: >> >> On Mon, Apr 04, 2016 at 05:37:01PM -0400, Steve Grubb wrote: >> > On Monday, April 04, 2016 12:02:42 AM wmealing wrote: >> > > I'm looking to create an audit trail for when devices are added or >> > > removed >> > > from the system. >> > > >> > > The audit subsystem is a logging subsystem in kernel space that can be >> > > used to create advanced filters on generated events. It has partnered >> > > userspace utilities ausearch, auditd, aureport, auditctl which work >> > > exclusively on audit records. >> > > >> > > These tools are able to set filters to "trigger" on specific in-kernel >> > > events specified by privileged users. While the userspace tools can >> > > create >> > > audit events these are not able to be handled intelligently >> > > (decoded,filtered or ignored) as kernel generated audit events are. >> > > >> > > I have this working at the moment with the USB subsystem (as an >> > > example). >> > > Its been suggested that I use systemd-udev however this means that the >> > > audit >> > > tools (ausearch) will not be able to index these records. >> > > >> > > Here is an example of picking out the AUDIT_DEVICE record type for >> > > example. >> > > >> > > > # ausearch -l -i -ts today -m AUDIT_DEVICE >> > > > >> > > > type=AUDIT_DEVICE msg=audit(31/03/16 16:37:15.642:2) : action=add >> > > > manufacturer=Linux 4.4.0-ktest ehci_hcd product=EHCI Host Controller >> > > > serial=:00:06.7 major=189 minor=0 bus="usb" >> > >> > About this event's format...we can't have any spaces in the value side >> > of the >> > name=value fields unless its encoded as an untrusted string. You can >> > replace >> > spaces with an underscore or dash for readability. So, manufacturer and >> > product would need this treatment. >> >> What is the character encoding that audit messages can accept? Does it >> match up with the character encoding that USB strings are in? >> >> 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: [RFC] Create an audit record of USB specific details
A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Tue, Apr 05, 2016 at 11:54:07AM +1000, Wade Mealing wrote: > That is a good question What is? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Create an audit record of USB specific details
On Tue, Apr 05, 2016 at 11:54:07AM +1000, Wade Mealing wrote: > That is a good question, maybe I've been lucky in the devices that I have > been testing with. Most of them seem to be ascii, my assumption was that > shouldn't be a problem. The same encoding function used by the path > audit_log_d_path, definitely audits UTF8 named files: > > # ausearch -i -f /tmp/test/권성주.txt Please look at the USB spec to see the encoding that USB strings are in. They are in UTF-16LE, but we do some manipulation of them in the call to usb_string() to make them semi-readable by the kernel. But, as we aren't doing anything important with these, except printing them out for people to lovingly gaze at, that's just fine. But if you need to do policy decisions based on them, well, you better use the "real" version of the string, otherwise you could run into major problems. But again, please step back, what are the requirements here that you are doing this work for? If it's just for fun, wonderful, but please say so when you post the patches so we don't take them seriously. Well, I'm not taking them seriously now as obviously they will not work, so I guess all is fine :) 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: [RFC] Create an audit record of USB specific details
On April 4, 2016 6:17:23 PM Greg KH wrote: On Mon, Apr 04, 2016 at 05:37:58PM -0400, Paul Moore wrote: On Monday, April 04, 2016 05:56:26 AM Greg KH wrote: > On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > > From: Wade Mealing > > > > Gday, > > > > I'm looking to create an audit trail for when devices are added or removed > > from the system. > > Then please do it in userspace, as I suggested before, that way you > catch all types of devices, not just USB ones. Audit has some odd requirements placed on it by some of its users. I think most notable in this particular case is the need to take specific actions, including panicking the system, when audit records can't be sent to userspace and are "lost". Granted, it's an odd requirement, definitely not the norm/default configuration, but supporting weird stuff like this has allowed Linux to be used on some pretty interesting systems that wouldn't have been possible otherwise. Looking quickly at some of the kobject/uvent code, it doesn't appear that the uevent/netlink channel has this capability. Are you sure you can loose netlink messages? If you do, you know you lost them, so isn't that good enough? Last I checked netlink didn't have a provision for panicking the system, so no :) It also just noticed that it looks like userspace can send fake uevent messages; That's how your machine boots properly :) Yes, it looks like that is how the initial devices are handled, right? Allowing something like that is probably okay for a variety of reasons, but I expect users would want to restrict access beyond this single trusted process. The good news is that I think you should be able to do that with a combination of DAC and MAC. I haven't looked at it closely enough yet, but that may be a concern for users which restrict/subdivide root using a LSM ... although it is possible that the LSM policy could help here. I'm thinking aloud a bit right now, but for SELinux the netlink controls aren't very granular and sysfs can be tricky so I can't say for certain about blocking fake events from userspace using LSMs/SELinux. uevents are not tied into LSMs from what I can tell, so I don't understand wht you are talking about here, sorry. Perhaps I'm mistaken, but uevents are sent to userspace via netlink which does have LSM controls. There also appears to be a file I/O mechanism via sysfs which also has LSM controls. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Create an audit record of USB specific details
On Mon, Apr 04, 2016 at 10:54:56PM -0400, Paul Moore wrote: > On April 4, 2016 6:17:23 PM Greg KH wrote: > > On Mon, Apr 04, 2016 at 05:37:58PM -0400, Paul Moore wrote: > > > On Monday, April 04, 2016 05:56:26 AM Greg KH wrote: > > > > On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > > > > > From: Wade Mealing > > > > > > > > > > Gday, > > > > > > > > > > I'm looking to create an audit trail for when devices are added or > > > > > removed > > > > > from the system. > > > > > > > > Then please do it in userspace, as I suggested before, that way you > > > > catch all types of devices, not just USB ones. > > > > > > Audit has some odd requirements placed on it by some of its users. I > > > think > > > most notable in this particular case is the need to take specific actions, > > > including panicking the system, when audit records can't be sent to > > > userspace > > > and are "lost". Granted, it's an odd requirement, definitely not the > > > norm/default configuration, but supporting weird stuff like this has > > > allowed > > > Linux to be used on some pretty interesting systems that wouldn't have > > > been > > > possible otherwise. Looking quickly at some of the kobject/uvent code, it > > > doesn't appear that the uevent/netlink channel has this capability. > > > > Are you sure you can loose netlink messages? If you do, you know you > > lost them, so isn't that good enough? > > Last I checked netlink didn't have a provision for panicking the system, so > no :) Userspace can panic the system if it detects this, so why not just do that? > > > It also just noticed that it looks like userspace can send fake uevent > > > messages; > > > > That's how your machine boots properly :) > > Yes, it looks like that is how the initial devices are handled, right? > Allowing something like that is probably okay for a variety of reasons, but > I expect users would want to restrict access beyond this single trusted > process. The good news is that I think you should be able to do that with a > combination of DAC and MAC. Again, please step back. What exactly are you trying to do here? What is the requirement? > > > I haven't looked at it closely enough yet, but that may be a concern > > > for users which restrict/subdivide root using a LSM ... although it is > > > possible that the LSM policy could help here. I'm thinking aloud a bit > > > right > > > now, but for SELinux the netlink controls aren't very granular and sysfs > > > can > > > be tricky so I can't say for certain about blocking fake events from > > > userspace > > > using LSMs/SELinux. > > > > uevents are not tied into LSMs from what I can tell, so I don't > > understand wht you are talking about here, sorry. > > Perhaps I'm mistaken, but uevents are sent to userspace via netlink which > does have LSM controls. There also appears to be a file I/O mechanism via > sysfs which also has LSM controls. And do any of them look at uevents through these mechanisms? I doubt they care... 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 1/2] phy: Group vendor specific phy drivers
Hi Krzysztof, On Sat, Apr 2, 2016 at 11:05 PM, Krzysztof Kozlowski wrote: > On Fri, Apr 01, 2016 at 04:59:15PM +0530, Vivek Gautam wrote: >> Adding vendor specific directories in phy to group >> phy drivers under their respective vendor umbrella. >> >> Signed-off-by: Vivek Gautam >> --- >> >> With growing number of phy drivers, it makes sense to >> group these drivers under their respective vendor/platform >> umbrella directory. >> >> Build-tested 'multi_v7_defconfig'. > > Please update also the MAINTAINERS file. For many entires the path won't > match anymore. Thanks for pointing out. I will update the MAINTAINERS file as well. > > Best regards, > Krzysztof > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards Vivek Gautam Samsung, India -- 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/2] phy: Group vendor specific phy drivers
On Fri, Apr 1, 2016 at 8:31 AM, kbuild test robot wrote: > Hi Vivek, > > [auto build test ERROR on rockchip/for-next] > [also build test ERROR on v4.6-rc1 next-20160401] > [if your patch is applied to the wrong git tree, please drop us a note to > help improving the system] > > url: > https://github.com/0day-ci/linux/commits/Vivek-Gautam/phy-Group-vendor-specific-phy-drivers/20160401-192920 > base: > https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git > for-next > config: arm-allyesconfig (attached as .config) > reproduce: > wget > https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross > -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=arm > > All errors (new ones prefixed by >>): > >>> make[4]: *** No rule to make target 'drivers/phy/qcom/+=', needed by >>> 'drivers/phy/qcom/built-in.o'. >make[4]: Target '__build' not remade because of errors. will fix this. > > --- > 0-DAY kernel test infrastructureOpen Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation -- Best Regards Vivek Gautam Samsung, India -- 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 2/2] usb: dwc3: gadget: clear SUSPHY bit before StartTransfer
Hi,
Felipe Balbi writes:
> Synopsys Databook 2.60a has a note that if we're
> sending a Start Transfer command we _must_ make sure
> that DWC3_GUSB2PHY(n).SUSPHY bit is cleared.
>
> This patch implements that particular detail.
>
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/dwc3/gadget.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 0967b5d8fb75..8b3a676db346 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -227,10 +227,29 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned
> ep,
> struct dwc3_ep *dep = dwc->eps[ep];
> u32 timeout = 500;
> u32 reg;
> +
> + int susphy = false;
> int ret = -EINVAL;
>
> trace_dwc3_gadget_ep_cmd(dep, cmd, params);
>
> + /*
> + * Synopsys Databook 2.60a states, on section 6.3.2.5.5, that if we're
> + * issuing a Start Transfer command, we must check if GUSB2PHYCFG.SUSPHY
> + * bit is set. If it is, then we need to clear it.
> + *
> + * We will also set SUSPHY bit to what it was before returning as stated
> + * by the same section on Synopsys databook.
> + */
> + if (cmd == DWC3_DEPCMD_STARTTRANSFER) {
After digging a little deeper, every command needs this, not only Start
Transfer. I'll remove this check.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 2/2] usb: dwc3: gadget: clear SUSPHY bit before StartTransfer
Hi,
Felipe Balbi writes:
> Felipe Balbi writes:
>> Synopsys Databook 2.60a has a note that if we're
>> sending a Start Transfer command we _must_ make sure
>> that DWC3_GUSB2PHY(n).SUSPHY bit is cleared.
>>
>> This patch implements that particular detail.
>>
>> Signed-off-by: Felipe Balbi
>> ---
>> drivers/usb/dwc3/gadget.c | 25 +
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 0967b5d8fb75..8b3a676db346 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -227,10 +227,29 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned
>> ep,
>> struct dwc3_ep *dep = dwc->eps[ep];
>> u32 timeout = 500;
>> u32 reg;
>> +
>> +int susphy = false;
>> int ret = -EINVAL;
>>
>> trace_dwc3_gadget_ep_cmd(dep, cmd, params);
>>
>> +/*
>> + * Synopsys Databook 2.60a states, on section 6.3.2.5.5, that if we're
>> + * issuing a Start Transfer command, we must check if GUSB2PHYCFG.SUSPHY
>> + * bit is set. If it is, then we need to clear it.
>> + *
>> + * We will also set SUSPHY bit to what it was before returning as stated
>> + * by the same section on Synopsys databook.
>> + */
>> +if (cmd == DWC3_DEPCMD_STARTTRANSFER) {
>
> After digging a little deeper, every command needs this, not only Start
> Transfer. I'll remove this check.
Here's v2, I've pushed it to testing/next:
8<
From 81b72307545522bb962f2ebbb49be8fef1ffa547 Mon Sep 17 00:00:00 2001
From: Felipe Balbi
Date: Mon, 4 Apr 2016 09:19:17 +0300
Subject: [PATCH v2] usb: dwc3: gadget: clear SUSPHY bit before ep cmds
Synopsys Databook 2.60a has a note that if we're
sending an endpoint command we _must_ make sure that
DWC3_GUSB2PHY(n).SUSPHY bit is cleared.
This patch implements that particular detail.
Signed-off-by: Felipe Balbi
---
changes since v1:
- clear SUSPHY for all endpoint commands
drivers/usb/dwc3/gadget.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0967b5d8fb75..7b8c1c8574f9 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -227,10 +227,27 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
struct dwc3_ep *dep = dwc->eps[ep];
u32 timeout = 500;
u32 reg;
+
+ int susphy = false;
int ret = -EINVAL;
trace_dwc3_gadget_ep_cmd(dep, cmd, params);
+ /*
+* Synopsys Databook 2.60a states, on section 6.3.2.5.[1-8], that if
+* we're issuing an endpoint command, we must check if
+* GUSB2PHYCFG.SUSPHY bit is set. If it is, then we need to clear it.
+*
+* We will also set SUSPHY bit to what it was before returning as stated
+* by the same section on Synopsys databook.
+*/
+ reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+ if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
+ susphy = true;
+ reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+ dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+ }
+
dwc3_writel(dwc->regs, DWC3_DEPCMDPAR0(ep), params->param0);
dwc3_writel(dwc->regs, DWC3_DEPCMDPAR1(ep), params->param1);
dwc3_writel(dwc->regs, DWC3_DEPCMDPAR2(ep), params->param2);
@@ -263,6 +280,12 @@ int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
udelay(1);
} while (1);
+ if (unlikely(susphy)) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+ reg |= DWC3_GUSB2PHYCFG_SUSPHY;
+ dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+ }
+
return ret;
}
--
2.8.0.rc2
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child
santosh shilimkar writes: > On 4/3/2016 11:28 PM, Felipe Balbi wrote: >> santosh shilimkar writes: >>> +Arnd, RMK, >>> >>> On 4/1/2016 4:57 AM, Felipe Balbi wrote: Hi, Grygorii Strashko writes: > On 04/01/2016 01:20 PM, Felipe Balbi wrote: >>> >>> [...] >>> > commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf > Author: Yoshihiro Shimoda > Date: Mon Jul 13 18:10:05 2015 +0900 > > usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU > > The dma_map_single and dma_unmap_single should set > "gadget->dev.parent" > instead of "&gadget->dev" in the first argument because the parent > has > a udc controller's device pointer. > Otherwise, iommu functions are not called in ARM environment. > > Signed-off-by: Yoshihiro Shimoda > Signed-off-by: Felipe Balbi > > Above actually means that DMA configuration code can be dropped from > usb_add_gadget_udc_release() completely. Right?: true, but now I'm not sure what's better: copy all necessary bits from parent or just pass the parent device to all DMA API. Anybody to shed a light here ? >>> The expectation is drivers should pass the proper dev pointers and let >>> core DMA code deal with it since it knows the per device dma properties. >> >> okay, so how do you get proper DMA pointers with something like this: >> >> kdwc3_dma_mask = dma_get_mask(dev); >> dev->dma_mask = &kdwc3_dma_mask; >> >> This doesn't anything. >> > Drivers actually needs to touch dma_mask(s) only if the core DMA > code hasn't populated it it. that's fair, but when driver _do_ touch it, I'd rather it be useful ;-) > I see Grygorii pointed out couple of things already. yeah -- balbi signature.asc Description: PGP signature
Re: [PATCH 2/2] usb: dwc3: gadget: put link to U0 before Start Transfer
Sergei Shtylyov writes: > On 4/4/2016 3:38 PM, Felipe Balbi wrote: > >>> On 4/4/2016 12:49 PM, Felipe Balbi wrote: >>> Synopsys Databook says we should move link to U0 >>> >>> Why "databook" is capitalized? :-) >> >> because I like capital leters >> before issuing a Start Transfer command. We could require the gadget driver to call usb_gadget_wakeup() however I feel that changing all gagdget drivers to keep track of Link State and >>> >>> Gadget. >> >> will fix > > Thanks and sorry for nitpicking. I sometimes forget that people on > this list don't tolerate some of my nits. totally tolerable, just remember not all of them might be considered. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: keystone: drop dma_mask configuration
Hi, Grygorii Strashko writes: > On 04/04/2016 02:45 PM, Felipe Balbi wrote: >> >> Hi, >> >> Grygorii Strashko writes: >>> The Keystone 2 supports DT-boot only, as result dma_mask will be >>> always configured properly from DT - >>> of_platform_device_create_pdata()->of_dma_configure(). More over, >>> dwc3-keystone.c can be built as module and in this case it's unsafe to >>> assign local variable as dma_mask. >>> >>> Hence, remove dma_mask configuration code. >>> >>> Cc: Murali Karicheri >>> Signed-off-by: Grygorii Strashko >> >> with these two patches from you, does USB Peripheral work on k2 devices ? > > I've tried CONFIG_USB_DWC3_GADGET=y + g_zero and k2e was detected > as gzero dev from Host PC > >> >> I'll drop my k2 changes from my series which I sent on saturday. >> > > Yes, please. can you test a similar patch to dwc3-omap.c, then ? -- balbi signature.asc Description: PGP signature
Re: [PATCH 2/6] usb: dwc3: omap: don't access DMA bits directly
Grygorii Strashko writes:
> On 04/02/2016 11:28 AM, Felipe Balbi wrote:
>> Instead of having a static global just for
>> initializing dma_mask directly, let's use
>> dma_coerce_mask_and_coherent() for that.
>>
>> Signed-off-by: Felipe Balbi
>> ---
>> drivers/usb/dwc3/dwc3-omap.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
>> index 22e9606d8e08..c219118bfda0 100644
>> --- a/drivers/usb/dwc3/dwc3-omap.c
>> +++ b/drivers/usb/dwc3/dwc3-omap.c
>> @@ -331,8 +331,6 @@ static void dwc3_omap_disable_irqs(struct dwc3_omap
>> *omap)
>> dwc3_omap_write_irqmisc_clr(omap, reg);
>> }
>>
>> -static u64 dwc3_omap_dma_mask = DMA_BIT_MASK(32);
>> -
>> static int dwc3_omap_id_notifier(struct notifier_block *nb,
>> unsigned long event, void *ptr)
>> {
>> @@ -490,7 +488,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>> omap->irq = irq;
>> omap->base = base;
>> omap->vbus_reg = vbus_reg;
>> -dev->dma_mask = &dwc3_omap_dma_mask;
>> +dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
>
> I think, It'll be better to just remove DMA configuration code
> from this driver and other drivers which support DT-boot mode only.
I don't have HW, can you test that on AM57x and/or AM437x ?
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 5/6] usb: dwc3: core: don't access DMA bits directly
Grygorii Strashko writes:
> On 04/02/2016 11:28 AM, Felipe Balbi wrote:
>> instead of manually copying DMA bits from parent
>> device, we should let DMA API do its job.
>>
>> Signed-off-by: Felipe Balbi
>> ---
>> drivers/usb/dwc3/core.c | 6 +-
>> 1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 17fd81447c9f..d601de20e1cd 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -981,11 +981,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>
>> spin_lock_init(&dwc->lock);
>>
>> -if (!dev->dma_mask) {
>> -dev->dma_mask = dev->parent->dma_mask;
>> -dev->dma_parms = dev->parent->dma_parms;
>
> Here, and in most of other patches you've dropped dma_parms copying -
> Is it expected?
I mentioned in cover letter that I don't know exactly what's the proper
way of dealing with dma_parms.
>> -dma_set_coherent_mask(dev, dev->parent->coherent_dma_mask);
>> -}
>> +dma_coerce_mask_and_coherent(dev, dma_get_mask(dev->parent));
>
>
> No. Above if case should stay, otherwise, already valid, DMA configuration
> might be overwritten:
okay.
--
balbi
signature.asc
Description: PGP signature
RE: [PATCH] usb: gadget: udc-core: remove manual dma configuration
Hi,
> From: Grygorii Strashko [mailto:grygorii.stras...@ti.com]
> Sent: Monday, April 04, 2016 8:32 PM
>
> Since commit 7ace8fc8219e ("usb: gadget: udc: core: Fix argument of
> dma_map_single for IOMMU") it is not necessary to configure DMA for
> usb_gadget device manually, because all DMA operation are performed
> using parent/controller device.
>
> Cc: Yoshihiro Shimoda
> Signed-off-by: Grygorii Strashko
My environment could work correctly after I applied this patch.
So,
Tested-by: Yoshihiro Shimoda
I realized that the commit 7ace8fc8219e is not suitable for my environment.
(Especially I realized it could use all of DMAC channels when IOMMU was
enabled.)
So, I will submit RFC patch set for udc-core.c / renesas_usbhs later :)
Best regards,
Yoshihiro Shimoda
--
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 v9 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Fri, Apr 01, 2016 at 03:21:48PM +0800, 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. > > Changes since v8: > - Remove the bus type things. > - Introduce one charger_detect() callback to detect charger type if it is > needed. > - Add some sysfs attributes for showing the charger type and state. > - Change the charger current attributes to read only. > - Move charger state and type definition to include/uapi. > - Some other optimiazations. > > Baolin Wang (4): > gadget: Introduce the usb charger framework > gadget: Support for the usb charger framework > gadget: Integrate with the usb gadget supporting for usb charger > power: wm831x_power: Support USB charger current limit management > We are thinking USB charger framework for Freescale i.mx SoC series, since our internal framework is not good enough. So I have more questions for your framework since there are many different USB charger designs, and I hope it is universal. - I would like to see your all code to let the charger work, eg you have said the charger detection is done by PMIC automatically, but I did not find your PMIC code to read charger type. Besides, how you can make sure the charger detection has finished before the framework handles USB_CHARGER_PRESENT event? - I commented the current limit at different situations for USB charger last time, but I have not seen your further comments. I would like give it again. For DCP, you can notify charger IC once you get the charger type. But for CDP/SDP, you need to notify charger IC after set configuration has finished, since the host may still not be ready to give high current. -- 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
