[PATCH] Fix clk->enabled counter

[Dmitry Baryshkov]

Hi,

isn't it better to extract clk_{enable, disable} from udc_disable()?
this can be treated as two different things, one is for the clock, the
other controls the udc register bit, and better be divided.

Good idea. What about this patch:

Fix the pxa2xx_udc to balance calls to clk_enable/clk_disable

Signed-off-by: Dmitry Baryshkov ***@gmail.com

diff --git a/drivers/usb/gadget/pxa2xx_udc.c b/drivers/usb/gadget/pxa2xx_udc.c
index 2900556..f4484e4 100644
--- a/drivers/usb/gadget/pxa2xx_udc.c
+++ b/drivers/usb/gadget/pxa2xx_udc.c
@@ -934,20 +934,36 @@ static void udc_disable(struct pxa2xx_udc *);
/* We disable the UDC -- and its 48 MHz clock -- whenever it's not
* in active use.
*/
-static int pullup(struct pxa2xx_udc *udc, int is_active)
+static int pullup(struct pxa2xx_udc *udc)
{
- is_active = is_active && udc->vbus && udc->pullup;
+ int is_active = udc->vbus && udc->pullup &&
+ ! udc->suspended;
DMSG("%s\n", is_active ? "active" : "inactive");
- if (is_active)
- udc_enable(udc);
- else {
- if (udc->gadget.speed != USB_SPEED_UNKNOWN) {
- DMSG("disconnect %s\n", udc->driver
- ? udc->driver->driver.name
- : "(no driver)");
- stop_activity(udc, udc->driver);
+ if (is_active) {
+ if (!udc->active) {
+ udc->active = 1;
+#ifdef CONFIG_ARCH_PXA
+ /* Enable clock for USB device */
+ clk_enable(udc->clk);
+#endif
+ udc_enable(udc);
+ }
+ } else {
+ if (udc->active) {
+ if (udc->gadget.speed != USB_SPEED_UNKNOWN) {
+ DMSG("disconnect %s\n", udc->driver
+ ? udc->driver->driver.name
+ : "(no driver)");
+ stop_activity(udc, udc->driver);
+ }
+ udc_disable(udc);
+#ifdef CONFIG_ARCH_PXA
+ /* Disable clock for USB device */
+ clk_disable(udc->clk);
+#endif
+ udc->active = 0;
}
- udc_disable(udc);
+
}
return 0;
}
@@ -958,9 +974,9 @@ static int pxa2xx_udc_vbus_session(struct usb_gadget *_gadget, int is_active)
struct pxa2xx_udc *udc;

udc = container_of(_gadget, struct pxa2xx_udc, gadget);
- udc->vbus = is_active = (is_active != 0);
+ udc->vbus = (is_active != 0);
DMSG("vbus %s\n", is_active ? "supplied" : "inactive");
- pullup(udc, is_active);
+ pullup(udc);
return 0;
}

@@ -975,9 +991,8 @@ static int pxa2xx_udc_pullup(struct usb_gadget *_gadget, int is_active)
if (!udc->mach->gpio_pullup && !udc->mach->udc_command)
return -EOPNOTSUPP;

- is_active = (is_active != 0);
- udc->pullup = is_active;
- pullup(udc, is_active);
+ udc->pullup = (is_active != 0);
+ pullup(udc);
return 0;
}

@@ -1145,11 +1160,6 @@ static void udc_disable(struct pxa2xx_udc *dev)

udc_clear_mask_UDCCR(UDCCR_UDE);

-#ifdef CONFIG_ARCH_PXA
- /* Disable clock for USB device */
- clk_disable(dev->clk);
-#endif
-
ep0_idle (dev);
dev->gadget.speed = USB_SPEED_UNKNOWN;
}
@@ -1190,11 +1200,6 @@ static void udc_enable (struct pxa2xx_udc *dev)
{
udc_clear_mask_UDCCR(UDCCR_UDE);

-#ifdef CONFIG_ARCH_PXA
- /* Enable clock for USB device */
- clk_enable(dev->clk);
-#endif
-
/* try to clear these bits before we enable the udc */
udc_ack_int_UDCCR(UDCCR_SUSIR|/*UDCCR_RSTIR|*/UDCCR_RESIR);

@@ -1285,7 +1290,7 @@ fail:
* for set_configuration as well as eventual disconnect.
*/
DMSG("registered gadget driver '%s'\n", driver->driver.name);
- pullup(dev, 1);
+ pullup(dev);
dump_state(dev);
return 0;
}
@@ -1328,7 +1333,8 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
return -EINVAL;

local_irq_disable();
- pullup(dev, 0);
+ dev->pullup = 0;
+ pullup(dev);
stop_activity(dev, driver);
local_irq_enable();

@@ -2267,7 +2273,9 @@ static int __exit pxa2xx_udc_remove(struct platform_device *pdev)
if (dev->driver)
return -EBUSY;

- udc_disable(dev);
+ dev->pullup = 0;
+ pullup(dev);
+
remove_debug_files(dev);

if (dev->got_irq) {
@@ -2318,7 +2326,8 @@ static int pxa2xx_udc_suspend(struct platform_device *dev, pm_message_t state)

if (!udc->mach->gpio_pullup && !udc->mach->udc_command)
WARN("USB host won't detect disconnect!\n");
- pullup(udc, 0);
+ udc->suspended = 1;
+ pullup(udc);

return 0;
}
@@ -2327,7 +2336,8 @@ static int pxa2xx_udc_resume(struct platform_device *dev)
{
struct pxa2xx_udc *udc = platform_get_drvdata(dev);

- pullup(udc, 1);
+ udc->suspended = 0;
+ pullup(udc);

return 0;
}
diff --git a/drivers/usb/gadget/pxa2xx_udc.h b/drivers/usb/gadget/pxa2xx_udc.h
index c08b1a2..26c0347 100644
--- a/drivers/usb/gadget/pxa2xx_udc.h
+++ b/drivers/usb/gadget/pxa2xx_udc.h
@@ -119,13 +119,17 @@ struct pxa2xx_udc {
has_cfr : 1,
req_pending : 1,
req_std : 1,
- req_config : 1;
+ req_config : 1,
+ suspended : 1,
+ active : 1;

#define start_watchdog(dev) mod_timer(&dev->timer, jiffies + (HZ/200))
struct timer_list timer;

struct device *dev;
+#ifdef CONFIG_ARCH_PXA
struct clk *clk;
+#endif
struct pxa2xx_udc_mach_info *mach;
u64 dma_mask;
struct pxa2xx_ep ep [PXA_UDC_NUM_ENDPOINTS];

Hi,

Fix clk->enabled counter so that drivers with unpaired
clock_disable calls (pxa2xx_udc :) can work.

NAK. It's already been discussed why this kind of approach is a
bad idea.

Then the pxa2xx_udc should be fixed. Because currently it's broken.

Yes. It would be nice if someone who uses it could look into coming
up with a tested fix.

[Dmitry Baryshkov]

Hi,

Dmitry Baryshkov wrote:

What's the status of this patch? Can I submit it to patch-tracker?
Didn't receive any comments.

Post by Dmitry Baryshkov
Hi,

isn't it better to extract clk_{enable, disable} from udc_disable()?
this can be treated as two different things, one is for the clock, the
other controls the udc register bit, and better be divided.

Fix the pxa2xx_udc to balance calls to clk_enable/clk_disable
diff --git a/drivers/usb/gadget/pxa2xx_udc.c
b/drivers/usb/gadget/pxa2xx_udc.c index 2900556..f4484e4 100644
--- a/drivers/usb/gadget/pxa2xx_udc.c +++
udc_disable(struct pxa2xx_udc *);
/* We disable the UDC -- and its 48 MHz clock -- whenever it's not
* in active use.
*/
-static int pullup(struct pxa2xx_udc *udc, int is_active) +static int
pullup(struct pxa2xx_udc *udc)
{
- is_active = is_active && udc->vbus && udc->pullup; + int is_active =
udc->vbus && udc->pullup && + ! udc->suspended;
DMSG("%s\n", is_active ? "active" : "inactive");
- if (is_active)
- udc_enable(udc);
- else {
- if (udc->gadget.speed != USB_SPEED_UNKNOWN) { - DMSG("disconnect
%s\n", udc->driver - ? udc->driver->driver.name
- : "(no driver)");
- stop_activity(udc, udc->driver);
+ if (is_active) {
+ if (!udc->active) {
+ udc->active = 1;
+#ifdef CONFIG_ARCH_PXA
+ /* Enable clock for USB device */ + clk_enable(udc->clk);
+#endif
+ udc_enable(udc);
+ }
+ } else {
+ if (udc->active) {
+ if (udc->gadget.speed != USB_SPEED_UNKNOWN) { + DMSG("disconnect
%s\n", udc->driver + ? udc->driver->driver.name
+ : "(no driver)");
+ stop_activity(udc, udc->driver); + }
+ udc_disable(udc);
+#ifdef CONFIG_ARCH_PXA
+ /* Disable clock for USB device */ + clk_disable(udc->clk);
+#endif
+ udc->active = 0;
}
- udc_disable(udc);
+
}
return 0;
}
@@ -958,9 +974,9 @@ static int pxa2xx_udc_vbus_session(struct usb_gadget
*_gadget, int is_active)
struct pxa2xx_udc *udc;
udc = container_of(_gadget, struct pxa2xx_udc, gadget);
- udc->vbus = is_active = (is_active != 0); + udc->vbus = (is_active !=
0);
DMSG("vbus %s\n", is_active ? "supplied" : "inactive");
- pullup(udc, is_active);
+ pullup(udc);
return 0;
}
@@ -975,9 +991,8 @@ static int pxa2xx_udc_pullup(struct usb_gadget *_gadget, int is_active)
if (!udc->mach->gpio_pullup && !udc->mach->udc_command)
return -EOPNOTSUPP;
- is_active = (is_active != 0);
- udc->pullup = is_active;
- pullup(udc, is_active);
+ udc->pullup = (is_active != 0);
+ pullup(udc);
return 0;
}
@@ -1145,11 +1160,6 @@ static void udc_disable(struct pxa2xx_udc *dev)
udc_clear_mask_UDCCR(UDCCR_UDE);
-#ifdef CONFIG_ARCH_PXA
- /* Disable clock for USB device */ - clk_disable(dev->clk);
-#endif
-
ep0_idle (dev);
dev->gadget.speed = USB_SPEED_UNKNOWN;
}
@@ -1190,11 +1200,6 @@ static void udc_enable (struct pxa2xx_udc *dev)
{
udc_clear_mask_UDCCR(UDCCR_UDE);
-#ifdef CONFIG_ARCH_PXA
- /* Enable clock for USB device */ - clk_enable(dev->clk);
-#endif
-
/* try to clear these bits before we enable the udc */
udc_ack_int_UDCCR(UDCCR_SUSIR|/*UDCCR_RSTIR|*/UDCCR_RESIR);
* for set_configuration as well as eventual disconnect. */
DMSG("registered gadget driver '%s'\n", driver->driver.name);
- pullup(dev, 1);
+ pullup(dev);
dump_state(dev);
return 0;
}
@@ -1328,7 +1333,8 @@ int usb_gadget_unregister_driver(struct
usb_gadget_driver *driver)
return -EINVAL;
local_irq_disable();
- pullup(dev, 0);
+ dev->pullup = 0;
+ pullup(dev);
stop_activity(dev, driver);
local_irq_enable();
@@ -2267,7 +2273,9 @@ static int __exit pxa2xx_udc_remove(struct platform_device *pdev)
if (dev->driver)
return -EBUSY;
- udc_disable(dev);
+ dev->pullup = 0;
+ pullup(dev);
+
remove_debug_files(dev);
if (dev->got_irq) {
@@ -2318,7 +2326,8 @@ static int pxa2xx_udc_suspend(struct
platform_device *dev, pm_message_t state)
if (!udc->mach->gpio_pullup && !udc->mach->udc_command)
WARN("USB host won't detect disconnect!\n");
- pullup(udc, 0);
+ udc->suspended = 1;
+ pullup(udc);
return 0;
}
@@ -2327,7 +2336,8 @@ static int pxa2xx_udc_resume(struct
platform_device *dev)
{
struct pxa2xx_udc *udc = platform_get_drvdata(dev);
- pullup(udc, 1);
+ udc->suspended = 0;
+ pullup(udc);
return 0;
}
diff --git a/drivers/usb/gadget/pxa2xx_udc.h
b/drivers/usb/gadget/pxa2xx_udc.h index c08b1a2..26c0347 100644
--- a/drivers/usb/gadget/pxa2xx_udc.h +++
pxa2xx_udc {
has_cfr : 1,
req_pending : 1,
req_std : 1,
- req_config : 1;
+ req_config : 1,
+ suspended : 1,
+ active : 1;
#define start_watchdog(dev) mod_timer(&dev->timer, jiffies + (HZ/200))
struct timer_list timer;
struct device *dev;
+#ifdef CONFIG_ARCH_PXA
struct clk *clk;
+#endif
struct pxa2xx_udc_mach_info *mach;
u64 dma_mask;
struct pxa2xx_ep ep [PXA_UDC_NUM_ENDPOINTS];

Hi,

Fix clk->enabled counter so that drivers with unpaired
clock_disable calls (pxa2xx_udc :) can work.

NAK. It's already been discussed why this kind of approach is a
bad idea.

Then the pxa2xx_udc should be fixed. Because currently it's broken.

Yes. It would be nice if someone who uses it could look into coming
up with a tested fix.

--
With best wishes
Dmitry



-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php

[David Brownell]

Post by Dmitry Baryshkov
Hi,

isn't it better to extract clk_{enable, disable} from udc_disable()?
this can be treated as two different things, one is for the clock, the
other controls the udc register bit, and better be divided.

Minor issues as noted below. You tested this? With suspend,
resume, remote wakeup, unplug-while-active, and rmmod g_<what>
scenarios to make sure the main code paths behave OK?

Post by Dmitry Baryshkov
Fix the pxa2xx_udc to balance calls to clk_enable/clk_disable
diff --git a/drivers/usb/gadget/pxa2xx_udc.c b/drivers/usb/gadget/pxa2xx_udc.c
index 2900556..f4484e4 100644
--- a/drivers/usb/gadget/pxa2xx_udc.c
+++ b/drivers/usb/gadget/pxa2xx_udc.c
@@ -934,20 +934,36 @@ static void udc_disable(struct pxa2xx_udc *);
/* We disable the UDC -- and its 48 MHz clock -- whenever it's not
* in active use.
*/
-static int pullup(struct pxa2xx_udc *udc, int is_active)
+static int pullup(struct pxa2xx_udc *udc)
{
- is_active = is_active && udc->vbus && udc->pullup;
+ int is_active = udc->vbus && udc->pullup &&
+ ! udc->suspended;

That should fit neatly on one line, with no superfluous whitespace
before the "!"...

Post by Dmitry Baryshkov
DMSG("%s\n", is_active ? "active" : "inactive");
- if (is_active)
- udc_enable(udc);
- else {
- if (udc->gadget.speed != USB_SPEED_UNKNOWN) {
- DMSG("disconnect %s\n", udc->driver
- ? udc->driver->driver.name
- : "(no driver)");
- stop_activity(udc, udc->driver);
+ if (is_active) {
+ if (!udc->active) {
+ udc->active = 1;
+#ifdef CONFIG_ARCH_PXA

Adding #ifdefs ... yeech. I suppose it's unavoidable until
the IXP processors get <linux/clk.h> support. What's the
story on IXP support there?

Post by Dmitry Baryshkov
--- a/drivers/usb/gadget/pxa2xx_udc.h
+++ b/drivers/usb/gadget/pxa2xx_udc.h
@@ -119,13 +119,17 @@ struct pxa2xx_udc {
has_cfr : 1,
req_pending : 1,
req_std : 1,
- req_config : 1;
+ req_config : 1,
+ suspended : 1,
+ active : 1;
#define start_watchdog(dev) mod_timer(&dev->timer, jiffies + (HZ/200))
struct timer_list timer;
struct device *dev;
+#ifdef CONFIG_ARCH_PXA
struct clk *clk;
+#endif

On the principle of not adding needless #ifdefs -- remove that one.
The cost of a pointer here is miniscule.

Post by Dmitry Baryshkov
struct pxa2xx_udc_mach_info *mach;
u64 dma_mask;
struct pxa2xx_ep ep [PXA_UDC_NUM_ENDPOINTS];

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Dmitry Baryshkov]

Hi,

isn't it better to extract clk_{enable, disable} from udc_disable()?
this can be treated as two different things, one is for the clock,
the other controls the udc register bit, and better be divided.

Minor issues as noted below. You tested this? With suspend, resume,
remote wakeup, unplug-while-active, and rmmod g_<what> scenarios to make
sure the main code paths behave OK?

I've tested with g_ether the suspend/resume of gadget (got log messages
as found bellow, but otherwise OK), plug-unplug also works. Can't test
rmmod/modrobe as I don't build modular kernels for my device.
Also what do you mean by remote wakeup?
I'll try making modules to work, fix things you noted and resubmit the patch.
--
With best wishes
Dmitry



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Dmitry Baryshkov]

Post by Dmitry Baryshkov

Hi,

isn't it better to extract clk_{enable, disable} from udc_disable()?
this can be treated as two different things, one is for the clock,
the other controls the udc register bit, and better be divided.

Minor issues as noted below. You tested this? With suspend, resume,
remote wakeup, unplug-while-active, and rmmod g_<what> scenarios to
make sure the main code paths behave OK?

I've tested with g_ether the suspend/resume of gadget (got log messages
as found bellow, but otherwise OK), plug-unplug also works. Can't test
rmmod/modrobe as I don't build modular kernels for my device. Also what
do you mean by remote wakeup? I'll try making modules to work, fix
things you noted and resubmit the patch.

Oops...forgot the log:

pxa2xx-udc pxa2xx-udc: suspend
udc: inactive
udc: disconnect ether

=================================
[ INFO: inconsistent lock state ]
2.6.24-rc8 #110
---------------------------------
inconsistent {in-hardirq-W} -> {hardirq-on-W} usage.
apmd/975 [HC0[0]:SC0[0]:HE1:SE1] takes:
(&dev->req_lock){++..}, at: [<c01a06e8>] rx_complete+0x1b0/0x204
{in-hardirq-W} state was registered at:
[<c0058dc0>] lock_acquire+0x6c/0x84
[<c0245164>] _spin_lock+0x40/0x50
[<c019f038>] set_ether_config+0x1d0/0x210
[<c019f0e0>] eth_set_config+0x68/0x138
[<c01a032c>] eth_setup+0x238/0x444
[<c019e0cc>] pxa2xx_udc_irq+0x368/0x638
[<c00649f0>] handle_IRQ_event+0x2c/0x70
[<c0066498>] handle_level_irq+0xb8/0x120
[<c001c048>] __exception_text_start+0x48/0x64
[<c001ca88>] __irq_svc+0x48/0xd0
[<c001e4e0>] cpu_idle+0x38/0x64
[<c02427b8>] rest_init+0x64/0x74
[<c0008be0>] start_kernel+0x22c/0x288
[<a0008034>] 0xa0008034
irq event stamp: 5342
hardirqs last enabled at (5342): [<c0087f8c>] kmem_cache_free+0xd8/0xe0
hardirqs last disabled at (5341): [<c0087ee0>] kmem_cache_free+0x2c/0xe0
softirqs last enabled at (4739): [<c003bfa4>] __do_softirq+0x100/0x114
softirqs last disabled at (4734): [<c003c52c>] irq_exit+0x5c/0x70

other info that might help us debug this:
2 locks held by apmd/975:
#0: (pm_mutex){--..}, at: [<c0061c5c>] enter_state+0x170/0x1d0
#1: (dpm_mtx){--..}, at: [<c0161534>] device_suspend+0x20/0x2d4

stack backtrace:
[<c0021818>] (dump_stack+0x0/0x14) from [<c005670c>] (print_usage_bug+0x10c/0x144)
[<c0056600>] (print_usage_bug+0x0/0x144) from [<c0057228>] (mark_lock+0x27c/0x530)
[<c0056fac>] (mark_lock+0x0/0x530) from [<c00580cc>] (__lock_acquire+0x4e0/0xcf4)
[<c0057bec>] (__lock_acquire+0x0/0xcf4) from [<c0058dc0>] (lock_acquire+0x6c/0x84)
[<c0058d54>] (lock_acquire+0x0/0x84) from [<c0245164>] (_spin_lock+0x40/0x50)
r7:c030d498 r6:c3e57c30 r5:c01a06e8 r4:c3d71584
[<c0245124>] (_spin_lock+0x0/0x50) from [<c01a06e8>] (rx_complete+0x1b0/0x204)
r5:c3d71540 r4:c3d71584
[<c01a0538>] (rx_complete+0x0/0x204) from [<c019c7f0>] (done+0x5c/0x70)
r6:c030d0e8 r5:c3e57c30 r4:00000001
[<c019c794>] (done+0x0/0x70) from [<c019c820>] (nuke+0x1c/0x50)
r6:00000003 r5:ffffff94 r4:c030d0e8
[<c019c804>] (nuke+0x0/0x50) from [<c019c8a0>] (stop_activity+0x4c/0x80)
r5:c030ce20 r4:c030d0e8
[<c019c854>] (stop_activity+0x0/0x80) from [<c019ca08>] (pullup+0x134/0x190)
r7:00000000 r6:00000002 r5:c02f4e20 r4:c030ce20
[<c019c8d4>] (pullup+0x0/0x190) from [<c019dd58>] (pxa2xx_udc_suspend+0x50/0x5c)
r5:c02f4e20 r4:c030ce20
[<c019dd08>] (pxa2xx_udc_suspend+0x0/0x5c) from [<c015ee60>] (platform_drv_suspend+0x20/0x24)
r4:c02f4f78
[<c015ee40>] (platform_drv_suspend+0x0/0x24) from [<c015eeb4>] (platform_suspend+0x2c/0x38)
[<c015ee88>] (platform_suspend+0x0/0x38) from [<c01616b0>] (device_suspend+0x19c/0x2d4)
[<c0161514>] (device_suspend+0x0/0x2d4) from [<c0061a08>] (suspend_devices_and_enter+0x4c/0x130)
r8:c001d108 r7:00000003 r6:00000003 r5:c05245a0 r4:00002969
[<c00619bc>] (suspend_devices_and_enter+0x0/0x130) from [<c0061c14>] (enter_state+0x128/0x1d0)
r6:c02ac158 r5:c05268bc r4:00002969
[<c0061aec>] (enter_state+0x0/0x1d0) from [<c0061d9c>] (pm_suspend+0x20/0x2c)
r7:00000036 r6:c3f3ace0 r5:00004102 r4:00000000
[<c0061d7c>] (pm_suspend+0x0/0x2c) from [<c0155cb8>] (apm_suspend+0x18/0xac)
[<c0155ca0>] (apm_suspend+0x0/0xac) from [<c015640c>] (apm_ioctl+0x80/0x2b0)
r4:00000000
[<c015638c>] (apm_ioctl+0x0/0x2b0) from [<c00994fc>] (do_ioctl+0x7c/0x98)
r6:00000000 r5:00004102 r4:c3e56bd0
[<c0099480>] (do_ioctl+0x0/0x98) from [<c00997c4>] (vfs_ioctl+0x2ac/0x2dc)
r6:00000000 r5:c3f2d498 r4:c3e56bd0
[<c0099518>] (vfs_ioctl+0x0/0x2dc) from [<c0099834>] (sys_ioctl+0x40/0x64)
r9:c3f62000 r8:c001d108 r6:00004102 r5:fffffff7 r4:c3e56bd0
[<c00997f4>] (sys_ioctl+0x0/0x64) from [<c001cf60>] (ret_fast_syscall+0x0/0x2c)
r6:0000001e r5:0000000a r4:00000000
usb0: eth_reset_config
--
With best wishes
Dmitry



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[David Brownell]

Post by Dmitry Baryshkov

Minor issues as noted below.  You tested this?  With suspend, resume,
remote wakeup, unplug-while-active, and rmmod g_<what> scenarios to make
sure the main code paths behave OK?

I've tested with g_ether the suspend/resume of gadget (got log messages
as found bellow, but otherwise OK),

Hmm, looks like you were testing system suspend/resume (initiated
by writing to /sys/power/state) when that lockdep problem showed up,
not USB suspend/resume (initiated by the host). Actually *BOTH* of
those sequences need to be tested!

The lockdep warning should go away. It's basically saying that
this code predates lockdep, and has an associated bug. The bug
fix looks to be along the lines of ensuring that IRQs are blocked
during pxa2xx_udc_{suspend,resume}() methods ... the easiest way
to fix that is updating the udc_driver struct to make those become
the late_suspend and early_resume methods, which are always called
with IRQs disabled (so you won't need to do it yourself).

(And kudos on running -- or at least testing! -- with lockdep.
That's a practice all developers should follow, and too few of
them do so.)

Post by Dmitry Baryshkov
plug-unplug also works. Can't test
rmmod/modrobe as I don't build modular kernels for my device.

Well, if you can't do that then you can't. The trick is rmmod
while the device is active. That can be problematic, since it
forces a clean shutdown in a different way than disconnect.

Post by Dmitry Baryshkov
Also what do you mean by remote wakeup?

See the USB 2.0 spec for definition. The easy way to test it is
as described in http://www.linux-usb.org/usbtest where it says how
to set up g_zero with "autoresume=5" to ensure the remote wakeup
tests can run with USBCV. There are no easy automated tests to
run on Linux.

In fact, running USBCV is also the easiest way to test USB suspend
and resume too.

Post by Dmitry Baryshkov
I'll try making modules to work, fix things you noted and resubmit the patch.

Thanks. It basically looks OK, but that's code I've learned to
be cautious with. It's easy for subtle bugs to creep in ... as
you may have deduced by noticing that lockdep bug.

- Dave




-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Dmitry Baryshkov]

Hi,

Post by Dmitry Baryshkov

Minor issues as noted below.  You tested this?  With suspend, resume,
remote wakeup, unplug-while-active, and rmmod g_<what> scenarios to
make sure the main code paths behave OK?

I've tested with g_ether the suspend/resume of gadget (got log messages
as found bellow, but otherwise OK),

Hmm, looks like you were testing system suspend/resume (initiated by
writing to /sys/power/state) when that lockdep problem showed up, not
USB suspend/resume (initiated by the host). Actually *BOTH* of those
sequences need to be tested!

Yup.

The lockdep warning should go away. It's basically saying that this
code predates lockdep, and has an associated bug. The bug fix looks to
be along the lines of ensuring that IRQs are blocked during
pxa2xx_udc_{suspend,resume}() methods ... the easiest way to fix that is
updating the udc_driver struct to make those become the late_suspend and
early_resume methods, which are always called with IRQs disabled (so you
won't need to do it yourself).
(And kudos on running -- or at least testing! -- with lockdep. That's a
practice all developers should follow, and too few of them do so.)

:)

After running into pair of locking problems just when I've started touching
kernel development I tend to write & test code with lockdep enabled.

Post by Dmitry Baryshkov
plug-unplug also works. Can't test
rmmod/modrobe as I don't build modular kernels for my device.

Well, if you can't do that then you can't. The trick is rmmod while the
device is active. That can be problematic, since it forces a clean
shutdown in a different way than disconnect.

Yes. I understood it. Actually I've built and tested the kernel with g_ether
built as a module. modprobe/rmmod worked.

Post by Dmitry Baryshkov
Also what do you mean by remote wakeup?

See the USB 2.0 spec for definition. The easy way to test it is as
described in http://www.linux-usb.org/usbtest where it says how to set
up g_zero with "autoresume=5" to ensure the remote wakeup tests can run
with USBCV. There are no easy automated tests to run on Linux.
In fact, running USBCV is also the easiest way to test USB suspend and
resume too.

Thanks. I'll look into it and post results.

Post by Dmitry Baryshkov
I'll try making modules to work, fix things you noted and resubmit the patch.

Thanks. It basically looks OK, but that's code I've learned to be
cautious with. It's easy for subtle bugs to creep in ... as you may
have deduced by noticing that lockdep bug.

:)
Finding all possible "duplicate clk_disable" was also a bit of work.
--
With best wishes
Dmitry



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-***@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/l