Re: [PATCH] Fix clk->enabled counter

Hi,

David Brownell wrote:

> On Saturday 19 January 2008, Dmitry Baryshkov wrote:
>> > 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.

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

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

>> 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/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel