[PATCH -next] usb: musb: cppi41: fix missing unlock on error in cppi41_dma_callback()
From: Wei Yongjun yongjun_...@trendmicro.com.cn
Add the missing unlock before return from function cppi41_dma_callback()
in the error handling case.
Signed-off-by: Wei Yongjun yongjun_...@trendmicro.com.cn
---
drivers/usb/musb/musb_cppi41.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index a74259e..6c12c86 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -150,8 +150,10 @@ static void cppi41_dma_callback(void *private_data)
remain_bytes,
direction,
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
- if (WARN_ON(!dma_desc))
+ if (WARN_ON(!dma_desc)) {
+ spin_unlock_irqrestore(musb-lock, flags);
return;
+ }
dma_desc-callback = cppi41_dma_callback;
dma_desc-callback_param = channel;
--
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 v1 02/49] USB: sg: prepare for enabling irq in complete()
On Sun, Aug 18, 2013 at 9:01 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Sun, 18 Aug 2013, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/message.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 82927e1..8bba734 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -266,8 +266,9 @@ static void sg_complete(struct urb *urb)
{
struct usb_sg_request *io = urb-context;
int status = urb-status;
+ unsigned long flags;
- spin_lock(io-lock);
+ spin_lock_irqsave(io-lock, flags);
/* In 2.5 we require hcds' endpoint queues not to progress after fault
* reports, until the completion callback (this!) returns. That lets
@@ -326,7 +327,7 @@ static void sg_complete(struct urb *urb)
if (!io-count)
complete(io-complete);
- spin_unlock(io-lock);
+ spin_unlock_irqrestore(io-lock, flags);
}
As far as I can see, these don't need to disable interrupts. All they
protect against is the code in usb_sg_wait() and usb_sg_cancel(), which
both run in process context.
Yes.
But will lockdep complain if they don't disable interrupts?
Looks lockdep won't complain because the lock can't be held in
another hardirq context.
As I mentioned in 00/50, the patchset is basically a mechanical
change, so one patch can be dropped if anyone reviews and
concludes it isn't needed.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
USB configfs questions and help
Hi, Last week I discovered the new configfs for the USB gadget support in the kernel. I found the following threads on the mailing list: https://lkml.org/lkml/2012/6/21/154 https://lkml.org/lkml/2012/11/26/38 Now there is some discussion about having a userpace library to handle the creation of all the necessary files easily. But I cannot find a trace of it. Does it exist already? If not I will need to write code for it anyway to support it in the USB gadget handler daemon I maintain to support this. It loads/unloads modules, sets up usb gadget modes, can start programs needed etc ... (See usb_moded: https://gitorious.org/meego-middleware/usb_moded ) So if there is no code out there do handle the creation of those configfs structures I do not mind doing the work. Also I have some questions about the documentation. In the section about the gadget creation it states, I quote: A gadget also needs its serial number, manufacturer and product strings. In order to have a place to store them, a strings subdirectory must be created for each language, e.g.: $ mkdir strings/0x409 What is meant with language here, and why does the example have a hex string as identifier. Are we rather talking about one of the USB identifiers here? The rest seems to be pretty clear so I'll start playing around with it. Thanks, Philippe -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb:gadget Fix comment for pointer to configfs
The documentation for the USB gadget fs is actually in Documentation/usb/gadget_configfs.txt. Signed-off-by: Philippe De Swert philippe.desw...@jollamobile.com --- drivers/usb/gadget/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 8e93683..1c5c7de 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -572,7 +572,7 @@ config USB_CONFIGFS specified simply by creating appropriate directories in configfs. Associating functions with configurations is done by creating appropriate symbolic links. - For more information see Documentation/usb/gadget-configfs.txt. + For more information see Documentation/usb/gadget_configfs.txt. config USB_CONFIGFS_SERIAL boolean Generic serial bulk in/out -- 1.8.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: octeon-usb and dwc2 in staging are for the same hw
Hi, On Sat, Aug 17, 2013 at 08:44:18PM +, Paul Zimmerman wrote: It doesn't get very far: External DMA Mode not supported dwc2_hcd_init() FAILED, returning -22 Hi Greg, all, After taking a look at the Octeon driver, it looks like that controller uses a customized version of the DWC2 core - it has a different DMA engine than the one provided by the standard hardware. So in fact these two drivers are actually not for the same hw. FWIW, I also tried forcing the DWC2 to non-DMA/FIFO mode, but could not get that working either. Do you support any HW running in slave mode? It seems the RX direction fails. I noticed the dwc2_read_packet() is hard-coded to channel 0 which looks odd, but changing that didn't change anything. A. -- 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 v1 26/49] input: cm109: prepare for enabling irq in complete()
Hi Dmitry, On Sun, Aug 18, 2013 at 11:37 AM, Dmitry Torokhov dmitry.torok...@gmail.com wrote: Hi Ming, On Sun, Aug 18, 2013 at 12:24:51AM +0800, Ming Lei wrote: Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). I think cm109 needs some love in it's URB handling, but this patch does not change anything, so: Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com Thank you. Or do you want me to pick it up for my tree? IMO, it might be easier to merge these patches via one tree, so Greg, would you like to manage all these patches via your tree? If not, I have to push these patches on each subsystem, then you need to pick it up for your input tree... Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 02/49] USB: sg: prepare for enabling irq in complete()
On Sun, 18 Aug 2013, Ming Lei wrote: As far as I can see, these don't need to disable interrupts. All they protect against is the code in usb_sg_wait() and usb_sg_cancel(), which both run in process context. Yes. But will lockdep complain if they don't disable interrupts? Looks lockdep won't complain because the lock can't be held in another hardirq context. Don't be so sure. Suppose you have two mass-storage devices, one connected by EHCI and one connected by UHCI. The one using UHCI _will_ invoke the completion handler in hardirq context, because uhci-hcd doesn't support tasklets. Have you tested this? As I mentioned in 00/50, the patchset is basically a mechanical change, so one patch can be dropped if anyone reviews and concludes it isn't needed. I'm afraid that it might be needed to keep lockdep happy, not to prevent real problems. 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 configfs questions and help
On Sun, 18 Aug 2013, Philippe wrote: Hi, Last week I discovered the new configfs for the USB gadget support in the kernel. I found the following threads on the mailing list: https://lkml.org/lkml/2012/6/21/154 https://lkml.org/lkml/2012/11/26/38 Now there is some discussion about having a userpace library to handle the creation of all the necessary files easily. But I cannot find a trace of it. Does it exist already? No mention of it has appeared on the mailing list. If not I will need to write code for it anyway to support it in the USB gadget handler daemon I maintain to support this. It loads/unloads modules, sets up usb gadget modes, can start programs needed etc ... (See usb_moded: https://gitorious.org/meego-middleware/usb_moded ) So if there is no code out there do handle the creation of those configfs structures I do not mind doing the work. Also I have some questions about the documentation. In the section about the gadget creation it states, I quote: A gadget also needs its serial number, manufacturer and product strings. In order to have a place to store them, a strings subdirectory must be created for each language, e.g.: $ mkdir strings/0x409 What is meant with language here, and why does the example have a hex string as identifier. Are we rather talking about one of the USB identifiers here? The word language refers to human languages, like English, French, Mandarin, and so on. The hex number is a language identifier, as specified by the USB Interface Forum: http://www.usb.org/developers/docs/ (see the paragraph near the end about LANGID codes). The strings are not USB identifiers; they are USB descriptors. The rest seems to be pretty clear so I'll start playing around with it. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: musb: Avoid null pointer dereference in debug logging
Since commit 511f3c53 usb_gadget_remove_driver will pass NULL for the driver argument. Signed-off-by: Maarten ter Huurne maar...@treewalker.org --- drivers/usb/musb/musb_gadget.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c index ba70923..82e5386 100644 --- a/drivers/usb/musb/musb_gadget.c +++ b/drivers/usb/musb/musb_gadget.c @@ -1935,7 +1935,8 @@ static int musb_gadget_stop(struct usb_gadget *g, stop_activity(musb, driver); otg_set_peripheral(musb-xceiv-otg, NULL); - dev_dbg(musb-controller, unregistering driver %s\n, driver-function); + dev_dbg(musb-controller, unregistering driver %s\n, + driver ? driver-function : (removed)); musb-is_active = 0; musb-gadget_driver = NULL; -- 1.8.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: [alsa-devel] Buffer size for ALSA USB PCM audio
Alan Stern wrote: On Tue, 13 Aug 2013, Clemens Ladisch wrote: The difference is that this version tries always to keep a period's worth of bytes in the USB hardware queue. Having truncated URBs is possible only when URBs are shorter than one period, No. URBs are truncated when a full-sized URB would extend at least one packet beyond the end of a frame. This thought was in the context of avoiding too-short queues. When there are multiple URBs per period, the queue is long enough anyway. so I think that a queue length of at least two periods, together with a minimum period size, should ensure the isochrounous scheduling threshold. This depends on how long a period is. In that patch, a period is guaranteed to be no smaller than the IST. And while we're at it: the default number of packets per URB was chosen before the driver had the ability to estimate the delay from the current frame number; it should now be quite safe to increase it. I don't understand how this delay estimation is supposed to work, or what it is meant to accomplish. Can you explain? The delay is the difference between the time when a sample is written by the application and when that sample is actually played, and is important for things like A/V synchronization. It depends on 1) the amount of samples already in the ring buffer; 2) any processing done by the driver; and 3) any processing done by the hardware. Most drivers don't do any processing, and most of the hardware has very low delays, so it is common for drivers to pretend 2) and 3) are zero. snd-usb-audio computes 2) from the current number of packets in the queue, with the progress estimated based on the current frame. Without this computation, it was desirable to have short URBs because the delay would jump by a large amount whenever a URB was completed. Regards, Clemens -- 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 v1 26/49] input: cm109: prepare for enabling irq in complete()
On Sun, Aug 18, 2013 at 10:10:15PM +0800, Ming Lei wrote: Hi Dmitry, On Sun, Aug 18, 2013 at 11:37 AM, Dmitry Torokhov dmitry.torok...@gmail.com wrote: Hi Ming, On Sun, Aug 18, 2013 at 12:24:51AM +0800, Ming Lei wrote: Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). I think cm109 needs some love in it's URB handling, but this patch does not change anything, so: Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com Thank you. Or do you want me to pick it up for my tree? IMO, it might be easier to merge these patches via one tree, so Greg, would you like to manage all these patches via your tree? Yes, I can take them all. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: serial: fix stringify operator in usb-serial-simple
From: Yann Droneaud ydrone...@opteya.com
usb-serial-simple uses an unknown stringify macro that make
all drivers being named stringify(vendor).
This can be a problem when two drivers have the same (wrong) name:
kernel: usbcore: registered new interface driver usb_serial_simple
kernel: usbserial: USB Serial support registered for stringify(vendor)
kernel Error: Driver 'stringify(vendor)' is already registered, aborting...
kernel: usbserial: problem -16 when registering driver stringify(vendor)
kernel: usbserial: USB Serial deregistering driver stringify(vendor)
kernel: usbcore: deregistering interface driver usb_serial_simple
Before the fix:
$ strings drivers/usb/serial/usb-serial-simple.o
usb_serial_simple
stringify(vendor)
After the fix:
$ strings drivers/usb/serial/usb-serial-simple.o
usb_serial_simple
funsoft
flashloader
vivopay
moto_modem
hp4x
suunto
siemens_mpi
This patch makes usb-serial-simple use the correct stringify operator.
Signed-off-by: Yann Droneaud ydrone...@opteya.com
---
drivers/usb/serial/usb-serial-simple.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/serial/usb-serial-simple.c
b/drivers/usb/serial/usb-serial-simple.c
index 6a06131..677c08c 100644
--- a/drivers/usb/serial/usb-serial-simple.c
+++ b/drivers/usb/serial/usb-serial-simple.c
@@ -29,7 +29,7 @@ static const struct usb_device_id vendor##_id_table[] = {
\
static struct usb_serial_driver vendor##_device = {\
.driver = { \
.owner =THIS_MODULE,\
- .name = stringify(vendor),\
+ .name = #vendor,\
}, \
.id_table = vendor##_id_table, \
.num_ports =1, \
--
1.8.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: fix stringify operator in usb-serial-simple
On Sun, Aug 18, 2013 at 09:29:00PM +0200, Yann Droneaud wrote: From: Yann Droneaud ydrone...@opteya.com usb-serial-simple uses an unknown stringify macro that make all drivers being named stringify(vendor). This can be a problem when two drivers have the same (wrong) name: kernel: usbcore: registered new interface driver usb_serial_simple kernel: usbserial: USB Serial support registered for stringify(vendor) kernel Error: Driver 'stringify(vendor)' is already registered, aborting... kernel: usbserial: problem -16 when registering driver stringify(vendor) kernel: usbserial: USB Serial deregistering driver stringify(vendor) kernel: usbcore: deregistering interface driver usb_serial_simple Ugh, sorry about that, I thought there used to be a stringify() macro that used to do this. Nice patch, I'll queue it up. 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: octeon-usb and dwc2 in staging are for the same hw
From: Aaro Koskinen [mailto:aaro.koski...@iki.fi] Sent: Sunday, August 18, 2013 4:41 AM On Sat, Aug 17, 2013 at 08:44:18PM +, Paul Zimmerman wrote: It doesn't get very far: External DMA Mode not supported dwc2_hcd_init() FAILED, returning -22 Hi Greg, all, After taking a look at the Octeon driver, it looks like that controller uses a customized version of the DWC2 core - it has a different DMA engine than the one provided by the standard hardware. So in fact these two drivers are actually not for the same hw. FWIW, I also tried forcing the DWC2 to non-DMA/FIFO mode, but could not get that working either. Do you support any HW running in slave mode? It seems the RX direction fails. I noticed the dwc2_read_packet() is hard-coded to channel 0 which looks odd, but changing that didn't change anything. I have tested FIFO mode on x86 only. It looks like there may be a problem with unaligned RX transfers on archs like ARM that have strict alignment requirements. See this comment in dwc2_read_packet(): * Todo: Account for the case where dest is not dword aligned. Hard-coding to FIFO 0 is correct for RX, since there is only one RX FIFO. -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: fix stringify operator in usb-serial-simple
Le 18.08.2013 21:40, Greg Kroah-Hartman a écrit : On Sun, Aug 18, 2013 at 09:29:00PM +0200, Yann Droneaud wrote: Ugh, sorry about that, I thought there used to be a stringify() macro that used to do this. Nice patch, I'll queue it up. That's __stringify() which is defined in linux/stringify.h but: 1) inside a string (eg __stringify(vendor)) it's gonna never work; 2) it's not required here, __stringify(vendor) would be needed if vendor was itself a macro and the macro content was to be converted to a string. Regards. -- Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] usb: gadget: USB_FUSB300 should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `fusb300_set_idma': drivers/usb/gadget/fusb300_udc.c:946: undefined reference to `usb_gadget_map_request' drivers/usb/gadget/fusb300_udc.c:958: undefined reference to `usb_gadget_unmap_request' Signed-off-by: Geert Uytterhoeven ge...@linux-m68k.org --- drivers/usb/gadget/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 8e93683..d2e6f4f 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -188,7 +188,7 @@ config USB_FSL_USB2 config USB_FUSB300 tristate Faraday FUSB300 USB Peripheral Controller - depends on !PHYS_ADDR_T_64BIT + depends on !PHYS_ADDR_T_64BIT HAS_DMA help Faraday usb device controller FUSB300 driver -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] usb: gadget: USB_R8A66597 should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `sudmac_free_channel': drivers/usb/gadget/r8a66597-udc.c:676: undefined reference to `usb_gadget_unmap_request' drivers/built-in.o: In function `sudmac_alloc_channel': drivers/usb/gadget/r8a66597-udc.c:666: undefined reference to `usb_gadget_map_request' Signed-off-by: Geert Uytterhoeven ge...@linux-m68k.org --- drivers/usb/gadget/Kconfig |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index d2e6f4f..3c1cadd 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -246,6 +246,7 @@ config USB_PXA25X_SMALL config USB_R8A66597 tristate Renesas R8A66597 USB Peripheral Controller + depends on HAS_DMA help R8A66597 is a discrete USB host and peripheral controller chip that supports both full and high speed USB 2.0 data transfers. -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] usb: chipidea: USB_CHIPIDEA should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `dma_set_coherent_mask': include/linux/dma-mapping.h:93: undefined reference to `dma_supported' Signed-off-by: Geert Uytterhoeven ge...@linux-m68k.org --- drivers/usb/chipidea/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig index d1bd8ef..dbd5232 100644 --- a/drivers/usb/chipidea/Kconfig +++ b/drivers/usb/chipidea/Kconfig @@ -1,6 +1,6 @@ config USB_CHIPIDEA tristate ChipIdea Highspeed Dual Role Controller - depends on USB || USB_GADGET + depends on (USB || USB_GADGET) HAS_DMA help Say Y here if your system has a dual role high speed USB controller based on ChipIdea silicon IP. Currently, only the -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] usb: gadget: USB_NET2272_DMA should depend on HAS_DMA
If NO_DMA=y: drivers/built-in.o: In function `net2272_done': drivers/usb/gadget/net2272.c:386: undefined reference to `usb_gadget_unmap_request' drivers/built-in.o: In function `net2272_queue': drivers/usb/gadget/net2272.c:848: undefined reference to `usb_gadget_map_request' Signed-off-by: Geert Uytterhoeven ge...@linux-m68k.org --- drivers/usb/gadget/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 3c1cadd..324dc61 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -403,7 +403,7 @@ config USB_NET2272 config USB_NET2272_DMA boolean Support external DMA controller - depends on USB_NET2272 + depends on USB_NET2272 HAS_DMA help The NET2272 part can optionally support an external DMA controller, but your board has to have support in the -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: USB host support should depend on HAS_DMA
On Thu, Jul 11, 2013 at 1:12 AM, Arnd Bergmann a...@arndb.de wrote:
On Wednesday 10 July 2013, Alan Stern wrote:
This isn't right. There are USB host controllers that use PIO, not
DMA. The HAS_DMA dependency should go with the controller driver, not
the USB core.
On the other hand, the USB core does call various routines like
dma_unmap_single. It ought to be possible to compile these calls even
when DMA isn't enabled. That is, they should be defined as do-nothing
stubs.
The asm-generic/dma-mapping-broken.h file intentionally causes link
errors, but that could be changed.
The better approach in my mind would be to replace code like
if (hcd-self.uses_dma)
with
if (IS_ENABLED(CONFIG_HAS_DMA) hcd-self.uses_dma) {
which will reliably cause that reference to be omitted from object code,
but not stop giving link errors for drivers that actually require
DMA.
This can be done for drivers/usb/core/hcd.c.
But I'm a bit puzzled by drivers/usb/core/buffer.c. E.g.
void *hcd_buffer_alloc(...)
{
/* some USB hosts just use PIO */
if (!bus-controller-dma_mask
!(hcd-driver-flags HCD_LOCAL_MEM)) {
*dma = ~(dma_addr_t) 0;
return kmalloc(size, mem_flags);
}
for (i = 0; i HCD_BUFFER_POOLS; i++) {
if (size = pool_max[i])
return dma_pool_alloc(hcd-pool[i], mem_flags, dma);
}
return dma_alloc_coherent(hcd-self.controller, size, dma, mem_flags);
}
which is called from usb_hcd_map_urb_for_dma():
if (hcd-self.uses_dma) {
} else if (hcd-driver-flags HCD_LOCAL_MEM) {
ret = hcd_alloc_coherent(
urb-dev-bus, mem_flags,
urb-setup_dma,
(void **)urb-setup_packet,
sizeof(struct usb_ctrlrequest),
DMA_TO_DEVICE);
...
}
So if DMA is not used (!hcd-self.uses_dma, i.e. bus-controller-dma_mask
is zero), and HCD_LOCAL_MEM is set, we still end up calling dma_pool_alloc()?
(Naively, I'm not so familiar with the USB code) I'd expect it to use
kmalloc() instead?
So I would change it to
if (!IS_ENABLED(CONFIG_HAS_DMA) ||
(!bus-controller-dma_mask
!(hcd-driver-flags HCD_LOCAL_MEM))) {
*dma = ~(dma_addr_t) 0;
return kmalloc(size, mem_flags);
}
Thanks for your clarification!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: 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 configfs questions and help
Hi, On 18/08/13 17:52, Alan Stern wrote: Now there is some discussion about having a userpace library to handle the creation of all the necessary files easily. But I cannot find a trace of it. Does it exist already? No mention of it has appeared on the mailing list. From: https://lkml.org/lkml/2012/11/27/341 Couldn't we have a tool to manage all this? target has such a thing, you have just select a few things via a CLI tool and the tool creates the directories for you _and_ removes them later on. I don't want to push python on anyone but the removal magic is simply straight forward: unlink the disk ports, rmdir luns, tpgt,… Or http://marc.info/?l=linux-usbm=132431126927355w=2 I understand that the echo/mkdir like interface is not perfect for everybody. For target there is a python tool that handles it so you/the user does not see configfs internals. The problem with python is that small users which ship just a busybox as their RFS might consider python as too big. Therefore I would suggest a small C program/library and Yeah, a library is the way to go. It also standardizes the usage, allows for lots of language bindings - so if you want to use python/ruby/perl/C#/etc you can - and allows the whole thing to go into products. Or maybe I misunderstood your answer and it means nothing like that made yet? And does somebody know where that python tool might be available? Unfortunately I did not have the time today to read all the code, but some mails regarding configfs suggest some paths are created by configfs itself. I did not immediately see anything backing that up, but any input is welcome. It is a bit hard to follow all the different threads as the search mixes things up. $ mkdir strings/0x409 What is meant with language here, and why does the example have a hex string as identifier. Are we rather talking about one of the USB identifiers here? The word language refers to human languages, like English, French, Mandarin, and so on. The hex number is a language identifier, as specified by the USB Interface Forum: http://www.usb.org/developers/docs/ (see the paragraph near the end about LANGID codes). Of course! I just never looked deep enough into that. The strings are not USB identifiers; they are USB descriptors. True. I always mix the exact terms up. Thanks Alan! Philippe -- 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: musb: Avoid null pointer dereference in debug logging
Hello. On 18-08-2013 20:21, Maarten ter Huurne wrote: Since commit 511f3c53 usb_gadget_remove_driver will pass NULL for the Please also specify that commit's summary line in parens. driver argument. Signed-off-by: Maarten ter Huurne maar...@treewalker.org --- drivers/usb/musb/musb_gadget.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) WBR, Sergei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 02/49] USB: sg: prepare for enabling irq in complete()
On Sun, Aug 18, 2013 at 10:44 PM, Alan Stern st...@rowland.harvard.edu wrote: On Sun, 18 Aug 2013, Ming Lei wrote: As far as I can see, these don't need to disable interrupts. All they protect against is the code in usb_sg_wait() and usb_sg_cancel(), which both run in process context. Yes. But will lockdep complain if they don't disable interrupts? Looks lockdep won't complain because the lock can't be held in another hardirq context. Don't be so sure. Suppose you have two mass-storage devices, one connected by EHCI and one connected by UHCI. The one using UHCI _will_ invoke the completion handler in hardirq context, because uhci-hcd doesn't support tasklets. Have you tested this? It can't be triggered since we don't enable local interrupts yet before calling completion handler. Also uhci-hcd/ohci-hcd should support tasklet later, even for xhci-hcd, there is only little performance loss with tasklet. As I mentioned in 00/50, the patchset is basically a mechanical change, so one patch can be dropped if anyone reviews and concludes it isn't needed. I'm afraid that it might be needed to keep lockdep happy, not to prevent real problems. Right, so the patch should be kept. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB configfs questions and help
On Mon, 19 Aug 2013, Philippe De Swert wrote: Hi, On 18/08/13 17:52, Alan Stern wrote: Now there is some discussion about having a userpace library to handle the creation of all the necessary files easily. But I cannot find a trace of it. Does it exist already? No mention of it has appeared on the mailing list. From: https://lkml.org/lkml/2012/11/27/341 Couldn't we have a tool to manage all this? target has such a thing, you have just select a few things via a CLI tool and the tool creates the directories for you _and_ removes them later on. I don't want to push python on anyone but the removal magic is simply straight forward: unlink the disk ports, rmdir luns, tpgt,� Or http://marc.info/?l=linux-usbm=132431126927355w=2 I understand that the echo/mkdir like interface is not perfect for everybody. For target there is a python tool that handles it so you/the user does not see configfs internals. The problem with python is that small users which ship just a busybox as their RFS might consider python as too big. Therefore I would suggest a small C program/library and Yeah, a library is the way to go. It also standardizes the usage, allows for lots of language bindings - so if you want to use python/ruby/perl/C#/etc you can - and allows the whole thing to go into products. Or maybe I misunderstood your answer and it means nothing like that made yet? Right -- nobody has mentioned building such a tool yet. And does somebody know where that python tool might be available? Not me. 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] usb: phy: Cleanup error code in **_usb_get_phy_**() APIs
Hi,
On Thu, Aug 8, 2013 at 12:05 AM, Julius Werner jwer...@chromium.org wrote:
@@ -94,11 +94,11 @@ static int devm_usb_phy_match(struct device *dev, void
*res, void *match_data)
*/
struct usb_phy *devm_usb_get_phy(struct device *dev, enum usb_phy_type type)
{
- struct usb_phy **ptr, *phy;
+ struct usb_phy *phy = ERR_PTR(-ENOMEM), **ptr;
This looks a little roundabout, don't you think? Why don't you just
directly have 'return ERR_PTR(-ENOMEM)' down there where you put 'goto
err0'?
Ok, will change this.
ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
if (!ptr)
- return NULL;
+ goto err0;
phy = usb_get_phy(type);
if (!IS_ERR(phy)) {
@@ -107,6 +107,7 @@ struct usb_phy *devm_usb_get_phy(struct device *dev,
enum usb_phy_type type)
} else
devres_free(ptr);
+err0:
return phy;
}
EXPORT_SYMBOL_GPL(devm_usb_get_phy);
struct usb_phy *devm_usb_get_phy_dev(struct device *dev, u8 index)
{
- struct usb_phy **ptr, *phy;
+ struct usb_phy *phy = ERR_PTR(-ENOMEM), **ptr;
Same here
will change this too.
ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
if (!ptr)
- return NULL;
+ goto err0;
phy = usb_get_phy_dev(dev, index);
if (!IS_ERR(phy)) {
@@ -267,6 +268,7 @@ struct usb_phy *devm_usb_get_phy_dev(struct device *dev,
u8 index)
} else
devres_free(ptr);
+err0:
return phy;
}
EXPORT_SYMBOL_GPL(devm_usb_get_phy_dev);
@@ -142,7 +142,7 @@ extern void usb_remove_phy(struct usb_phy *);
/* helpers for direct access thru low-level io interface */
static inline int usb_phy_io_read(struct usb_phy *x, u32 reg)
{
- if (x-io_ops x-io_ops-read)
+ if (!IS_ERR(x) x-io_ops x-io_ops-read)
I liked the ones where we had IS_ERR_OR_NULL() here (and in all the
ones below)... you sometimes have to handle PHYs in
platform-independent code where you don't want to worry about if this
platform actually has a PHY driver there or not. Any reason you
changed that?
The **get_phy_*() APIs never return a NULL pointer now, do we still
need to handle that in that case.
Or are we assuming that code will use these phy operations without
getting a phy in the first place ?
return x-io_ops-read(x, reg);
return -EINVAL;
@@ -150,7 +150,7 @@ static inline int usb_phy_io_read(struct usb_phy *x, u32
reg)
static inline int usb_phy_io_write(struct usb_phy *x, u32 val, u32 reg)
{
- if (x-io_ops x-io_ops-write)
+ if (!IS_ERR(x) x-io_ops x-io_ops-write)
return x-io_ops-write(x, val, reg);
return -EINVAL;
@@ -159,7 +159,7 @@ static inline int usb_phy_io_write(struct usb_phy *x,
u32 val, u32 reg)
static inline int
usb_phy_init(struct usb_phy *x)
{
- if (x-init)
+ if (!IS_ERR(x) x-init)
return x-init(x);
return 0;
@@ -168,26 +168,27 @@ usb_phy_init(struct usb_phy *x)
static inline void
usb_phy_shutdown(struct usb_phy *x)
{
- if (x-shutdown)
+ if (!IS_ERR(x) x-shutdown)
x-shutdown(x);
}
static inline int
usb_phy_vbus_on(struct usb_phy *x)
{
- if (!x-set_vbus)
- return 0;
+ if (!IS_ERR(x) x-set_vbus)
+ return x-set_vbus(x, true);
- return x-set_vbus(x, true);
+ return 0;
}
static inline int
usb_phy_vbus_off(struct usb_phy *x)
{
- if (!x-set_vbus)
- return 0;
+ if (!IS_ERR(x) x-set_vbus)
+ return x-set_vbus(x, false);
+
+ return 0;
- return x-set_vbus(x, false);
}
/* for usb host and peripheral controller drivers */
@@ -249,8 +250,9 @@ static inline int usb_bind_phy(const char *dev_name, u8
index,
static inline int
usb_phy_set_power(struct usb_phy *x, unsigned mA)
{
- if (x x-set_power)
+ if (!IS_ERR(x) x-set_power)
return x-set_power(x, mA);
+
return 0;
}
@@ -258,28 +260,28 @@ usb_phy_set_power(struct usb_phy *x, unsigned mA)
static inline int
usb_phy_set_suspend(struct usb_phy *x, int suspend)
{
- if (x-set_suspend != NULL)
+ if (!IS_ERR(x) x-set_suspend != NULL)
return x-set_suspend(x, suspend);
- else
- return 0;
+
+ return 0;
}
static inline int
usb_phy_notify_connect(struct usb_phy *x, enum usb_device_speed speed)
{
- if (x-notify_connect)
+ if (!IS_ERR(x) x-notify_connect)
return x-notify_connect(x, speed);
- else
- return 0;
+
+ return 0;
}
static inline int
usb_phy_notify_disconnect(struct usb_phy *x, enum usb_device_speed speed)
{
- if (x-notify_disconnect)
+ if (!IS_ERR(x) x-notify_disconnect)
Re: [PATCH 4/4] usb: chipidea: USB_CHIPIDEA should depend on HAS_DMA
On Sun, Aug 18, 2013 at 10:20:44PM +0200, Geert Uytterhoeven wrote: If NO_DMA=y: drivers/built-in.o: In function `dma_set_coherent_mask': include/linux/dma-mapping.h:93: undefined reference to `dma_supported' Signed-off-by: Geert Uytterhoeven ge...@linux-m68k.org --- drivers/usb/chipidea/Kconfig |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig index d1bd8ef..dbd5232 100644 --- a/drivers/usb/chipidea/Kconfig +++ b/drivers/usb/chipidea/Kconfig @@ -1,6 +1,6 @@ config USB_CHIPIDEA tristate ChipIdea Highspeed Dual Role Controller - depends on USB || USB_GADGET + depends on (USB || USB_GADGET) HAS_DMA help Say Y here if your system has a dual role high speed USB controller based on ChipIdea silicon IP. Currently, only the I can't understand why the DMA can't be changed to fix this instead of changing every driver? -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
Felipe,
ping..
On Wednesday 14 August 2013 08:35 PM, Kishon Vijay Abraham I wrote:
Hi,
On Wednesday 14 August 2013 04:34 AM, Tomasz Figa wrote:
On Wednesday 14 of August 2013 00:19:28 Sylwester Nawrocki wrote:
W dniu 2013-08-13 14:05, Kishon Vijay Abraham I pisze:
On Tuesday 13 August 2013 05:07 PM, Tomasz Figa wrote:
On Tuesday 13 of August 2013 16:14:44 Kishon Vijay Abraham I wrote:
On Wednesday 31 July 2013 11:45 AM, Felipe Balbi wrote:
On Wed, Jul 31, 2013 at 11:14:32AM +0530, Kishon Vijay Abraham I
wrote:
IMHO we need a lookup method for PHYs, just like for clocks,
regulators, PWMs or even i2c busses because there are complex
cases
when passing just a name using platform data will not work. I
would
second what Stephen said [1] and define a structure doing
things
in a
DT-like way.
Example;
[platform code]
static const struct phy_lookup my_phy_lookup[] = {
PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1,
phy.2),
The only problem here is that if *PLATFORM_DEVID_AUTO* is used
while
creating the device, the ids in the device name would change
and
PHY_LOOKUP wont be useful.
I don't think this is a problem. All the existing lookup
methods
already
use ID to identify devices (see regulators, clkdev, PWMs, i2c,
...). You
can simply add a requirement that the ID must be assigned
manually,
without using PLATFORM_DEVID_AUTO to use PHY lookup.
And I'm saying that this idea, of using a specific name and id,
is
frought with fragility and will break in the future in various
ways
when
devices get added to systems, making these strings constantly
have
to be
kept up to date with different board configurations.
People, NEVER, hardcode something like an id. The fact that
this
happens today with the clock code, doesn't make it right, it
makes
the
clock code wrong. Others have already said that this is wrong
there
as
well, as systems change and dynamic ids get used more and more.
Let's not repeat the same mistakes of the past just because we
refuse to
learn from them...
So again, the find a phy by a string functions should be
removed,
the
device id should be automatically created by the phy core just
to
make
things unique in sysfs, and no driver code should _ever_ be
reliant
on
the number that is being created, and the pointer to the phy
structure
should be used everywhere instead.
With those types of changes, I will consider merging this
subsystem,
but
without them, sorry, I will not.
I'll agree with Greg here, the very fact that we see people
trying to
add a requirement of *NOT* using PLATFORM_DEVID_AUTO already
points
to a big problem in the framework.
The fact is that if we don't allow PLATFORM_DEVID_AUTO we will
end up
adding similar infrastructure to the driver themselves to make
sure
we
don't end up with duplicate names in sysfs in case we have
multiple
instances of the same IP in the SoC (or several of the same PCIe
card).
I really don't want to go back to that.
If we are using PLATFORM_DEVID_AUTO, then I dont see any way we
can
give the correct binding information to the PHY framework. I think
we
can drop having this non-dt support in PHY framework? I see only
one
platform (OMAP3) going to be needing this non-dt support and we
can
use the USB PHY library for it.
you shouldn't drop support for non-DT platform, in any case we
lived
without DT (and still do) for years. Gotta find a better way ;-)
hmm..
how about passing the device names of PHY in platform data of the
controller? It should be deterministic as the PHY framework assigns
its
own id and we *don't* want to add any requirement that the ID must
be
assigned manually without using PLATFORM_DEVID_AUTO. We can get rid
of
*phy_init_data* in the v10 patch series.
OK, so the PHY device name would have a fixed part, passed as
platform data of the controller and a variable part appended
by the PHY core, depending on the number of registered PHYs ?
Then same PHY names would be passed as the PHY provider driver's
platform data ?
Then if there are 2 instances of the above (same names in platform
data) how would be determined which PHY controller is linked to
which PHY supplier ?
I guess you want each device instance to have different PHY device
names already in platform data ? That might work. We probably will
be focused mostly on DT anyway. It seem without DT we are trying
to find some layer that would allow us to couple relevant devices
and overcome driver core inconvenience that it provides to means
to identify specific devices in advance. :) Your proposal sounds
reasonable, however I might be missing some details or corner cases.
What about slightly altering the concept of v9 to pass a pointer to
struct device instead of device name inside phy_init_data?
As Felipe said, we don't want to pass pointers in platform_data
to/from random subsystems. We pass data, passing pointers would
be a total mess IMHO.
Well,
