Re: [PATCH 7/7] driver-core : convert semaphore to mutex in struct class

["Dave Young" <hidave.darkstar@gmail.com>]

On Jan 18, 2008 3:38 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> On Fri, Jan 18, 2008 at 01:31:17PM +0800, Dave Young wrote:
> > On Jan 18, 2008 11:18 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> ...
> > > Yeah, might be better to wait until class_device is gone, otherwise you
> > > may need to fix stuff that is just going to be removed. Your change to
> > > have iterators for the class devices look like a nice preparation for
> > > future changes though.
> > >
> > > Our rough plan is:
> > >  2.6.25:
> > >   - get the ~100 patches in Greg's tree (in -mm) merged :)
> > >  2.6.26:
> > > ???  - remove the 20 char limit in "struct device"
> > >   - get rid of "struct class_device"
> >
> > Fine, thanks.
> >
> > Let's wait for other people's comment.
>
> Dave, I doubt you'll ever manage to do this if you're going to wait:
> probably there will be always some new changes like this around...

Maybe you are right.

>
> IMHO, it would be nice to get the real state of current lockdep
> problems here to figure out if there is any chance to do this right &
> without any warnings with current lockdep. If I got it right from
> earlier threads it might be impossible with USB, at least.

I don't think so, usb doesn't be affected by struct class mutex, they
only use the lock of struct device. As I replied before, the lockdep
issue exist only between class_interface and class_device.

>
> So, since I think these nesting levels seem to be wrong in 7/7 patch,
> maybe it's better to exclude it from this patchset, and to try this as
> testing for some time.

I may file the updated patch with more nesting changes and test it of
course. Actually I should have done it, thanks.

>
> My proposal is to do the annotations with mutex_lock_nested(),
> everywhere in this patch, according to 'real' relations between these
> classes from thread POV, so e.g.:
>
> mutex_lock_nested(&some_class->mutex, CLASS_PARENT);
> mutex_lock_nested(&some_class->mutex, CLASS_CLASS);
> ...or more if needed:
> mutex_lock_nested(&some_class->mutex, CLASS_CHILD);
> mutex_lock_nested(&some_class->mutex, CLASS_ROOT);
> ...etc.
>
> using adequate names for these enums, and only after this check what
> lockdep thinks about it. Then, if there are no obvious mistakes, but
> lockdep doesn't like it, send the patch and report without trying to
> 'silence' lockdep, so we could see what's going on, and if there are
> any chances to make it right.

1) Using CLASS_NORMAL/CLASS_PARENT/CLASS_CHILD will be enough.
or
2) Simply add SINGLE_LEVEL_NESTING in class_device_add and other
class_device functions because it is the only possible nest-lock place
as I know.

Do you agree?

>
> Thanks,
> Jarek P.
>