Re: Sysfs and suicidal attributes

[Tejun Heo]

Hello,

Alan Stern wrote:
>> When a bus device is being woken up and rescanning the bus, all
>> prerequisites for the bus should have been resumed already.  As long as
>> the newly added device is placed after the bus device itself, things
>> should be safe.  Well, that's the theory at least.
> 
> What actually happened in practice was that a device send a wakeup
> request and hence was resumed without the PM core's knowledge.  As a
> result it was still on the "suspended" list when its new child was
> added to the end of the "resumed" list.  Then when the PM core got
> around to the parent, the parent was placed after the child.  Since the 
> "resumed" list is iterated in reverse order for suspending, you see the 
> problem.
> 
> If the PM core had been holding the parent's semaphore this wouldn't 
> have occurred.  But it does illustrate the pitfalls of adding a device 
> during resume.

Oh I see.  Considering such cases, I guess what's needed is allowing
"addition of devices hanging off of resumed or resuming bus during
system resume".

>>> I had envisioned using SRCU to synchronize existing device additions
>>> with the beginning of a suspend.  down_read_trylock should work just as
>>> well (aside from cacheline bouncing).
>> How high performance device addition/removal should be?  Wouldn't rwlock
>> be enough?
> 
> rwlock is a type of spinlock, right?  Hence it can't sleep and can't be
> used for device addition/removal.  Also rwlocks aren't fair and are
> subject to livelock.

rwsem of course.  Sorry about the confusion.  Fairness doesn't matter
here.  The synchronization is suspend vs. addition with priority on
suspend (suspend does down_write, removal does down_read_trylock).

>> Hmmm... locking all device mutexes sounds scary to me but I tend to be a
>> chicken and lockdep is great, so it might just work.  :-)
> 
> These aren't mutexes; they are semaphores (although they are used as 
> mutexes).  I speculate that part of the reason they have never been 
> converted to mutexes is because of potential lockdep issues.
> 
> The order of locking isn't a problem.  The list itself defines the 
> correct order.

I'm still not a big fan of locking (or downing) all device semaphores
when the same effect can be achieved with a rwsem.  One thing I like
about rwsem approach is that it can never cause a deadlock because one
side uses trylock exclusively.

If you lock all the device sems, failing addition becomes more difficult
(can be done but then you need something like the rwsem anyway).
Without special handling of dev->sem, when a driver screws up (try to
add a child device from ->suspend), it will deadlock while suspend is in
progress, which isn't pretty.

Please also note that currently dev->sem does not protect against adding
children.  It says it does in the comment on the definition of the field
but it's never acquired during device_add().

>> Why is that necessary?  What happens if there's some long-running
>> read/write operation using bin attr?
> 
> That is certainly a possible problem.  I don't know if there are any 
> such long-running operations.
> 
> Is it necessary?  No.  But it would make things easier if suspend 
> handling could be localized in one place (the sysfs core) instead of 
> spread out among a bunch of attribute methods.

Yeah, I'm all for centralizing things.  I just didn't see what could be
simplified in LLDs by freezing sysfs access.  Can you point me to any
example node which can benefit from such thing?

> Using a freezable workqueue for the callbacks would also localize the 
> suspend handling.

But the freezable workqueue thing isn't really necessary if any of the
discussed solutions is implemented, right?

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/