Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andy Fleming
On Dec 7, 2006, at 11:05, Jeff Garzik wrote: Yes, I merged the code, but looking deeper at phy its clear I missed some things. Looking into libphy's workqueue stuff, it has the following sequence: disable interrupts schedule_work() ... time passes ... ...

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Linus Torvalds
On Thu, 7 Dec 2006, Andrew Morton wrote: > > We _can_ trust it in the context of > > void flush_work(struct work_struct *work) Yes, but the way the bits are defined, the "pending" bit is not meaningful as a synchronization event, for example - because _other_ users can't trust it once

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 7 Dec 2006 10:01:56 -0800 (PST) Linus Torvalds <[EMAIL PROTECTED]> wrote: > > > On Thu, 7 Dec 2006, Andrew Morton wrote: > > > > umm.. Putting a work_struct* into struct cpu_workqueue_struct and then > > doing appropriate things with cpu_workqueue_struct.lock might work. > > Yeah,

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 7 Dec 2006 09:57:15 -0800 Andrew Morton <[EMAIL PROTECTED]> wrote: > On Thu, 07 Dec 2006 12:05:38 -0500 > Jeff Garzik <[EMAIL PROTECTED]> wrote: > > > Yes, I merged the code, but looking deeper at phy its clear I missed > > some things. > > > > Looking into libphy's workqueue stuff, it

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Maciej W. Rozycki
On Thu, 7 Dec 2006, Jeff Garzik wrote: > Looking into libphy's workqueue stuff, it has the following sequence: > > disable interrupts > schedule_work() > > ... time passes ... > ... workqueue routine is called ... > > enable interrupts > handle interrupt >

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Linus Torvalds
On Thu, 7 Dec 2006, Andrew Morton wrote: > > umm.. Putting a work_struct* into struct cpu_workqueue_struct and then > doing appropriate things with cpu_workqueue_struct.lock might work. Yeah, that looks sane. We can't hide anything in "struct work", because we can't trust it any more once

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 07 Dec 2006 12:05:38 -0500 Jeff Garzik <[EMAIL PROTECTED]> wrote: > Yes, I merged the code, but looking deeper at phy its clear I missed > some things. > > Looking into libphy's workqueue stuff, it has the following sequence: > > disable interrupts > schedule_work() > >

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 7 Dec 2006 08:49:02 -0800 (PST) Linus Torvalds <[EMAIL PROTECTED]> wrote: > > > On Wed, 6 Dec 2006, Andrew Morton wrote: > > > > But this will return to the caller if the callback is presently running on > > a different CPU. The whole point here is to be able to reliably kill off > >

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Jeff Garzik
Yes, I merged the code, but looking deeper at phy its clear I missed some things. Looking into libphy's workqueue stuff, it has the following sequence: disable interrupts schedule_work() ... time passes ... ... workqueue routine is called ... enable

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Linus Torvalds
On Wed, 6 Dec 2006, Andrew Morton wrote: > > But this will return to the caller if the callback is presently running on > a different CPU. The whole point here is to be able to reliably kill off > the pending work so that the caller can free resources. I mentioned that in one of the emails.

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Maciej W. Rozycki
On Wed, 6 Dec 2006, Linus Torvalds wrote: > I didn't get any answers on this. I'd like to get this issue resolved, but > since I don't even use libphy on my main machine, I need somebody else to > test it for me. I tested it in the evening with the system I implemented it for originally. It

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
> On Thu, 07 Dec 2006 10:29:39 + David Howells <[EMAIL PROTECTED]> wrote: > Andrew Morton <[EMAIL PROTECTED]> wrote: > > > I guess I don't understand exactly what problem the noautorel stuff is > > trying to solve. It _seems_ to me that in all cases we can simply stuff > > the old `data'

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread David Howells
Andrew Morton <[EMAIL PROTECTED]> wrote: > I guess I don't understand exactly what problem the noautorel stuff is > trying to solve. It _seems_ to me that in all cases we can simply stuff > the old `data' field in alongside the controlling work_struct or > delayed_work which wants to operate on

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread David Howells
Andrew Morton [EMAIL PROTECTED] wrote: I guess I don't understand exactly what problem the noautorel stuff is trying to solve. It _seems_ to me that in all cases we can simply stuff the old `data' field in alongside the controlling work_struct or delayed_work which wants to operate on it.

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 07 Dec 2006 10:29:39 + David Howells [EMAIL PROTECTED] wrote: Andrew Morton [EMAIL PROTECTED] wrote: I guess I don't understand exactly what problem the noautorel stuff is trying to solve. It _seems_ to me that in all cases we can simply stuff the old `data' field in

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Maciej W. Rozycki
On Wed, 6 Dec 2006, Linus Torvalds wrote: I didn't get any answers on this. I'd like to get this issue resolved, but since I don't even use libphy on my main machine, I need somebody else to test it for me. I tested it in the evening with the system I implemented it for originally. It

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Linus Torvalds
On Wed, 6 Dec 2006, Andrew Morton wrote: But this will return to the caller if the callback is presently running on a different CPU. The whole point here is to be able to reliably kill off the pending work so that the caller can free resources. I mentioned that in one of the emails. We do

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Jeff Garzik
Yes, I merged the code, but looking deeper at phy its clear I missed some things. Looking into libphy's workqueue stuff, it has the following sequence: disable interrupts schedule_work() ... time passes ... ... workqueue routine is called ... enable

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 7 Dec 2006 08:49:02 -0800 (PST) Linus Torvalds [EMAIL PROTECTED] wrote: On Wed, 6 Dec 2006, Andrew Morton wrote: But this will return to the caller if the callback is presently running on a different CPU. The whole point here is to be able to reliably kill off the pending

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 07 Dec 2006 12:05:38 -0500 Jeff Garzik [EMAIL PROTECTED] wrote: Yes, I merged the code, but looking deeper at phy its clear I missed some things. Looking into libphy's workqueue stuff, it has the following sequence: disable interrupts schedule_work() ...

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Linus Torvalds
On Thu, 7 Dec 2006, Andrew Morton wrote: umm.. Putting a work_struct* into struct cpu_workqueue_struct and then doing appropriate things with cpu_workqueue_struct.lock might work. Yeah, that looks sane. We can't hide anything in struct work, because we can't trust it any more once it's

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Maciej W. Rozycki
On Thu, 7 Dec 2006, Jeff Garzik wrote: Looking into libphy's workqueue stuff, it has the following sequence: disable interrupts schedule_work() ... time passes ... ... workqueue routine is called ... enable interrupts handle interrupt I really

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 7 Dec 2006 09:57:15 -0800 Andrew Morton [EMAIL PROTECTED] wrote: On Thu, 07 Dec 2006 12:05:38 -0500 Jeff Garzik [EMAIL PROTECTED] wrote: Yes, I merged the code, but looking deeper at phy its clear I missed some things. Looking into libphy's workqueue stuff, it has the

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andrew Morton
On Thu, 7 Dec 2006 10:01:56 -0800 (PST) Linus Torvalds [EMAIL PROTECTED] wrote: On Thu, 7 Dec 2006, Andrew Morton wrote: umm.. Putting a work_struct* into struct cpu_workqueue_struct and then doing appropriate things with cpu_workqueue_struct.lock might work. Yeah, that looks

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Linus Torvalds
On Thu, 7 Dec 2006, Andrew Morton wrote: We _can_ trust it in the context of void flush_work(struct work_struct *work) Yes, but the way the bits are defined, the pending bit is not meaningful as a synchronization event, for example - because _other_ users can't trust it once

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-07 Thread Andy Fleming
On Dec 7, 2006, at 11:05, Jeff Garzik wrote: Yes, I merged the code, but looking deeper at phy its clear I missed some things. Looking into libphy's workqueue stuff, it has the following sequence: disable interrupts schedule_work() ... time passes ... ...

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Andrew Morton
On Wed, 6 Dec 2006 22:42:07 -0800 Andrew Morton <[EMAIL PROTECTED]> wrote: > But I wouldn't want to think about an implementation as long as we have > that WORK_STRUCT_NOAUTOREL horror in there. Can we just nuke that? Only > three drivers need it and I bet they can be modified to use the usual

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Andrew Morton
On Wed, 6 Dec 2006 17:21:50 -0800 (PST) Linus Torvalds <[EMAIL PROTECTED]> wrote: > > > On Wed, 6 Dec 2006, Linus Torvalds wrote: > > > > How about something like this? > > I didn't get any answers on this. I'd like to get this issue resolved, but > since I don't even use libphy on my main

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Linus Torvalds wrote: > > How about something like this? I didn't get any answers on this. I'd like to get this issue resolved, but since I don't even use libphy on my main machine, I need somebody else to test it for me. Just to remind you all, here's the patch again.

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, David Howells wrote: > > Linus Torvalds <[EMAIL PROTECTED]> wrote: > > > (a) "volatile" on kernel data is basically always a bug, and you should > > use locking. > > But what about when you're building a lock? Actually, I suspect correct usage > of asm constraints

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread David Howells
Linus Torvalds <[EMAIL PROTECTED]> wrote: > (a) "volatile" on kernel data is basically always a bug, and you should > use locking. But what about when you're building a lock? Actually, I suspect correct usage of asm constraints and memory barriers trumps volatile anyway even there.

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Linus Torvalds wrote: > > and remove the "volatile" from all the bitop accessor functions. It might also be interesting to see if this would change code-size at all. There's a number of things that check different bits in the same word right now, and they just reload the

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Linus Torvalds wrote: > > Sadly, gcc doesn't do it in this case. Still, I'd rather keep the source > clean, and hope that the compiler improves eventually, than to make the > code uglier. Actually, it's our own damn fault. We've long had our arguments "const volatile" to

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Jeff Garzik wrote: > > Honestly I wonder if some of these situations really want > "kill_scheduled_work_unless_it_is_already_running_right_now_if_so_wait_for_it" We could do that, and the code to do it would be fairly close to what the "run it" code is. Just replace the

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Linus Torvalds wrote: > > Gcc should do it for us, afaik. I didn't check, but gcc is generally > pretty good at combining logical operations like this, because it's very > common. Sadly, gcc doesn't do it in this case. Still, I'd rather keep the source clean, and hope

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, David Howells wrote: > > + if (get_wq_data(work) == cwq > + && test_bit(WORK_STRUCT_PENDING, >management) > > I wonder if those can be combined, perhaps: Gcc should do it for us, afaik. I didn't check, but gcc is generally pretty good at combining logical

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread David Howells
Linus Torvalds <[EMAIL PROTECTED]> wrote: > test_bit() with a constant number is done very much in C, and very much on > purpose. _Exactly_ to allow the compiler to combine these kinds of things. Ah... I've read that several times, and each time I've assumed it's referring to *addr not nr as

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Jeff Garzik
Honestly I wonder if some of these situations really want "kill_scheduled_work_unless_it_is_already_running_right_now_if_so_wait_for_it" Since its during shutdown, usually the task just wants to know that the code in the workqueue won't be touching the hardware or data structures after

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread David Howells
Linus Torvalds <[EMAIL PROTECTED]> wrote: > How about something like this? At first glance, this looks reasonable. It also looks like it should be used to replace a lot of the cancel_delayed_work() calls that attempt to cancel _undelayed_ work items. That would allow a number of work items to

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Andrew Morton wrote: > > I think so too. But it would be imprudent to hang around waiting for me > to write it :( How about something like this? This (a) depends on the just-merged "struct work" cleanup (b) is totally untested (c) probably kills you slowly and

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Andrew Morton
On Wed, 6 Dec 2006 15:25:22 + (GMT) "Maciej W. Rozycki" <[EMAIL PROTECTED]> wrote: > > Maybe the lesson here is that flush_scheduled_work() is a bad function. > > It should really be flush_this_work(struct work_struct *w). That is in > > fact what approximately 100% of the

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Maciej W. Rozycki
On Tue, 5 Dec 2006, Andrew Morton wrote: > But running flush_scheduled_work() from within dev_close() is a very > sensible thing to do, and dev_close is called under rtnl_lock(). > davem is -> thattaway ;) And when within dev_close() there is quite a chance there is linkwatch_event() somewhere

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Maciej W. Rozycki
On Tue, 5 Dec 2006, Andy Fleming wrote: > We need to make sure there are no more pending phy_change() invocations in the > work queue, this is true, however I'm not convinced that this avoids the > problem. And now that I come back to this email after Linus's response, let > me add that I agree

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Maciej W. Rozycki
On Tue, 5 Dec 2006, Andy Fleming wrote: We need to make sure there are no more pending phy_change() invocations in the work queue, this is true, however I'm not convinced that this avoids the problem. And now that I come back to this email after Linus's response, let me add that I agree with

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Maciej W. Rozycki
On Tue, 5 Dec 2006, Andrew Morton wrote: But running flush_scheduled_work() from within dev_close() is a very sensible thing to do, and dev_close is called under rtnl_lock(). davem is - thattaway ;) And when within dev_close() there is quite a chance there is linkwatch_event() somewhere in

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Andrew Morton
On Wed, 6 Dec 2006 15:25:22 + (GMT) Maciej W. Rozycki [EMAIL PROTECTED] wrote: Maybe the lesson here is that flush_scheduled_work() is a bad function. It should really be flush_this_work(struct work_struct *w). That is in fact what approximately 100% of the flush_scheduled_work()

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Andrew Morton wrote: I think so too. But it would be imprudent to hang around waiting for me to write it :( How about something like this? This (a) depends on the just-merged struct work cleanup (b) is totally untested (c) probably kills you slowly and painfully

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread David Howells
Linus Torvalds [EMAIL PROTECTED] wrote: How about something like this? At first glance, this looks reasonable. It also looks like it should be used to replace a lot of the cancel_delayed_work() calls that attempt to cancel _undelayed_ work items. That would allow a number of work items to be

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Jeff Garzik
Honestly I wonder if some of these situations really want kill_scheduled_work_unless_it_is_already_running_right_now_if_so_wait_for_it Since its during shutdown, usually the task just wants to know that the code in the workqueue won't be touching the hardware or data structures after this

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread David Howells
Linus Torvalds [EMAIL PROTECTED] wrote: test_bit() with a constant number is done very much in C, and very much on purpose. _Exactly_ to allow the compiler to combine these kinds of things. Ah... I've read that several times, and each time I've assumed it's referring to *addr not nr as being

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, David Howells wrote: + if (get_wq_data(work) == cwq + test_bit(WORK_STRUCT_PENDING, work-management) I wonder if those can be combined, perhaps: Gcc should do it for us, afaik. I didn't check, but gcc is generally pretty good at combining logical

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Linus Torvalds wrote: Gcc should do it for us, afaik. I didn't check, but gcc is generally pretty good at combining logical operations like this, because it's very common. Sadly, gcc doesn't do it in this case. Still, I'd rather keep the source clean, and hope that

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Jeff Garzik wrote: Honestly I wonder if some of these situations really want kill_scheduled_work_unless_it_is_already_running_right_now_if_so_wait_for_it We could do that, and the code to do it would be fairly close to what the run it code is. Just replace the

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Linus Torvalds wrote: Sadly, gcc doesn't do it in this case. Still, I'd rather keep the source clean, and hope that the compiler improves eventually, than to make the code uglier. Actually, it's our own damn fault. We've long had our arguments const volatile to

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Linus Torvalds wrote: and remove the volatile from all the bitop accessor functions. It might also be interesting to see if this would change code-size at all. There's a number of things that check different bits in the same word right now, and they just reload the word

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread David Howells
Linus Torvalds [EMAIL PROTECTED] wrote: (a) volatile on kernel data is basically always a bug, and you should use locking. But what about when you're building a lock? Actually, I suspect correct usage of asm constraints and memory barriers trumps volatile anyway even there. David - To

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, David Howells wrote: Linus Torvalds [EMAIL PROTECTED] wrote: (a) volatile on kernel data is basically always a bug, and you should use locking. But what about when you're building a lock? Actually, I suspect correct usage of asm constraints and memory

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Linus Torvalds
On Wed, 6 Dec 2006, Linus Torvalds wrote: How about something like this? I didn't get any answers on this. I'd like to get this issue resolved, but since I don't even use libphy on my main machine, I need somebody else to test it for me. Just to remind you all, here's the patch again.

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Andrew Morton
On Wed, 6 Dec 2006 17:21:50 -0800 (PST) Linus Torvalds [EMAIL PROTECTED] wrote: On Wed, 6 Dec 2006, Linus Torvalds wrote: How about something like this? I didn't get any answers on this. I'd like to get this issue resolved, but since I don't even use libphy on my main machine, I

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-06 Thread Andrew Morton
On Wed, 6 Dec 2006 22:42:07 -0800 Andrew Morton [EMAIL PROTECTED] wrote: But I wouldn't want to think about an implementation as long as we have that WORK_STRUCT_NOAUTOREL horror in there. Can we just nuke that? Only three drivers need it and I bet they can be modified to use the usual

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Roland Dreier
> Ah. The point is that the phy code doesn't want to flush _all_ pending > callbacks. It only wants to flush its own one. And its own one doesn't > take rtnl_lock(). OK, got it. You're absolutely correct. > Maybe the lesson here is that flush_scheduled_work() is a bad function. > It

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Roland Dreier
> But running flush_scheduled_work() from within dev_close() is a very > sensible thing to do, and dev_close is called under rtnl_lock(). I can't argue with that -- this has actually bitten me in the past. Hmm, I'll try to understand why we need rtnl_lock() to cover dev_close... - R. - To

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andrew Morton
On Tue, 05 Dec 2006 13:37:37 -0800 Roland Dreier <[EMAIL PROTECTED]> wrote: > > a) Ban the calling of flush_scheduled_work() from under rtnl_lock(). > >Sounds hard. > > Unfortunate if this is happening a lot. It seems like the most > sensible fix -- flush_scheduled_work() is in effect

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Roland Dreier
> a) Ban the calling of flush_scheduled_work() from under rtnl_lock(). >Sounds hard. Unfortunate if this is happening a lot. It seems like the most sensible fix -- flush_scheduled_work() is in effect calling into an unknown and changeable in the future set of functions (since it waits for

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andrew Morton
On Tue, 5 Dec 2006 14:59:31 -0600 Andy Fleming <[EMAIL PROTECTED]> wrote: > Ok, I think this is the summary: > > - phy_change() is the work queue callback function (scheduled when a > PHY interrupt occurs) > > - dev_close() invokes the controller's stop/close/whatever function, > and it

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andy Fleming
On Dec 5, 2006, at 14:39, Andrew Morton wrote: On Tue, 5 Dec 2006 17:48:05 + (GMT) "Maciej W. Rozycki" <[EMAIL PROTECTED]> wrote: Essentially there is a race when disconnecting from a PHY, because interrupt delivery uses the event queue for processing. The function to handle

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andrew Morton
On Tue, 5 Dec 2006 17:48:05 + (GMT) "Maciej W. Rozycki" <[EMAIL PROTECTED]> wrote: > Essentially there is a race when disconnecting from a PHY, because > interrupt delivery uses the event queue for processing. The function to > handle interrupts that is called from the event queue is

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andrew Morton
On Tue, 5 Dec 2006 10:07:21 -0800 (PST) Linus Torvalds <[EMAIL PROTECTED]> wrote: > > > On Tue, 5 Dec 2006, Maciej W. Rozycki wrote: > > > > One way of avoiding it is calling flush_scheduled_work() from > > phy_stop_interrupts(). This is fine as long as a caller of > > phy_stop_interrupts()

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andy Fleming
On Dec 5, 2006, at 11:48, Maciej W. Rozycki wrote: Essentially there is a race when disconnecting from a PHY, because interrupt delivery uses the event queue for processing. The function to handle interrupts that is called from the event queue is phy_change(). It takes a pointer to a

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Maciej W. Rozycki
On Mon, 4 Dec 2006, Steve Fox wrote: > Unfortunately Maciej didn't get cc'ed correctly on your mail. Hopefully > I've added the right person to this post as well as Andy who has also > recently changed this code. Thanks -- my parser of the LKML does not always trigger. Maciej - To

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Linus Torvalds
On Tue, 5 Dec 2006, Maciej W. Rozycki wrote: > > One way of avoiding it is calling flush_scheduled_work() from > phy_stop_interrupts(). This is fine as long as a caller of > phy_stop_interrupts() (not necessarily the immediate one calling into > libphy) does not hold the netlink lock. > >

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Maciej W. Rozycki
On Sun, 3 Dec 2006, Andrew Morton wrote: > wtf? That code was merged? This bug has been known for months and after > several unsuccessful attempts at trying to understand what on earth that > hackery is supposed to be doing I gave up on it and asked Jeff to look after > it. I am surprised it

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Roland Dreier
But running flush_scheduled_work() from within dev_close() is a very sensible thing to do, and dev_close is called under rtnl_lock(). I can't argue with that -- this has actually bitten me in the past. Hmm, I'll try to understand why we need rtnl_lock() to cover dev_close... - R. - To

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Roland Dreier
Ah. The point is that the phy code doesn't want to flush _all_ pending callbacks. It only wants to flush its own one. And its own one doesn't take rtnl_lock(). OK, got it. You're absolutely correct. Maybe the lesson here is that flush_scheduled_work() is a bad function. It should

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Maciej W. Rozycki
On Sun, 3 Dec 2006, Andrew Morton wrote: wtf? That code was merged? This bug has been known for months and after several unsuccessful attempts at trying to understand what on earth that hackery is supposed to be doing I gave up on it and asked Jeff to look after it. I am surprised it got

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Linus Torvalds
On Tue, 5 Dec 2006, Maciej W. Rozycki wrote: One way of avoiding it is calling flush_scheduled_work() from phy_stop_interrupts(). This is fine as long as a caller of phy_stop_interrupts() (not necessarily the immediate one calling into libphy) does not hold the netlink lock. If a

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Maciej W. Rozycki
On Mon, 4 Dec 2006, Steve Fox wrote: Unfortunately Maciej didn't get cc'ed correctly on your mail. Hopefully I've added the right person to this post as well as Andy who has also recently changed this code. Thanks -- my parser of the LKML does not always trigger. Maciej - To unsubscribe

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andy Fleming
On Dec 5, 2006, at 11:48, Maciej W. Rozycki wrote: Essentially there is a race when disconnecting from a PHY, because interrupt delivery uses the event queue for processing. The function to handle interrupts that is called from the event queue is phy_change(). It takes a pointer to a

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andrew Morton
On Tue, 5 Dec 2006 10:07:21 -0800 (PST) Linus Torvalds [EMAIL PROTECTED] wrote: On Tue, 5 Dec 2006, Maciej W. Rozycki wrote: One way of avoiding it is calling flush_scheduled_work() from phy_stop_interrupts(). This is fine as long as a caller of phy_stop_interrupts() (not

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andrew Morton
On Tue, 5 Dec 2006 17:48:05 + (GMT) Maciej W. Rozycki [EMAIL PROTECTED] wrote: Essentially there is a race when disconnecting from a PHY, because interrupt delivery uses the event queue for processing. The function to handle interrupts that is called from the event queue is

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andy Fleming
On Dec 5, 2006, at 14:39, Andrew Morton wrote: On Tue, 5 Dec 2006 17:48:05 + (GMT) Maciej W. Rozycki [EMAIL PROTECTED] wrote: Essentially there is a race when disconnecting from a PHY, because interrupt delivery uses the event queue for processing. The function to handle interrupts

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andrew Morton
On Tue, 5 Dec 2006 14:59:31 -0600 Andy Fleming [EMAIL PROTECTED] wrote: Ok, I think this is the summary: - phy_change() is the work queue callback function (scheduled when a PHY interrupt occurs) - dev_close() invokes the controller's stop/close/whatever function, and it calls

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Roland Dreier
a) Ban the calling of flush_scheduled_work() from under rtnl_lock(). Sounds hard. Unfortunate if this is happening a lot. It seems like the most sensible fix -- flush_scheduled_work() is in effect calling into an unknown and changeable in the future set of functions (since it waits for

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-05 Thread Andrew Morton
On Tue, 05 Dec 2006 13:37:37 -0800 Roland Dreier [EMAIL PROTECTED] wrote: a) Ban the calling of flush_scheduled_work() from under rtnl_lock(). Sounds hard. Unfortunate if this is happening a lot. It seems like the most sensible fix -- flush_scheduled_work() is in effect calling

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-04 Thread Steve Fox
On Sun, 2006-12-03 at 01:16 -0800, Andrew Morton wrote: > On Sun, 03 Dec 2006 00:50:55 -0500 > Ben Collins <[EMAIL PROTECTED]> wrote: > > > Fixes this problem when libphy is compiled as module: > > > > WARNING: "current_is_keventd" [drivers/net/phy/libphy.ko] undefined! > > > > diff --git

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-04 Thread Steve Fox
On Sun, 2006-12-03 at 01:16 -0800, Andrew Morton wrote: On Sun, 03 Dec 2006 00:50:55 -0500 Ben Collins [EMAIL PROTECTED] wrote: Fixes this problem when libphy is compiled as module: WARNING: current_is_keventd [drivers/net/phy/libphy.ko] undefined! diff --git a/kernel/workqueue.c

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-03 Thread Andrew Morton
On Sun, 03 Dec 2006 00:50:55 -0500 Ben Collins <[EMAIL PROTECTED]> wrote: > Fixes this problem when libphy is compiled as module: > > WARNING: "current_is_keventd" [drivers/net/phy/libphy.ko] undefined! > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c > index 17c2f03..1cf226b 100644 >

Re: [PATCH] Export current_is_keventd() for libphy

2006-12-03 Thread Andrew Morton
On Sun, 03 Dec 2006 00:50:55 -0500 Ben Collins [EMAIL PROTECTED] wrote: Fixes this problem when libphy is compiled as module: WARNING: current_is_keventd [drivers/net/phy/libphy.ko] undefined! diff --git a/kernel/workqueue.c b/kernel/workqueue.c index 17c2f03..1cf226b 100644 ---

[PATCH] Export current_is_keventd() for libphy

2006-12-02 Thread Ben Collins
Fixes this problem when libphy is compiled as module: WARNING: "current_is_keventd" [drivers/net/phy/libphy.ko] undefined! diff --git a/kernel/workqueue.c b/kernel/workqueue.c index 17c2f03..1cf226b 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -608,6 +608,7 @@ int

[PATCH] Export current_is_keventd() for libphy

2006-12-02 Thread Ben Collins
Fixes this problem when libphy is compiled as module: WARNING: current_is_keventd [drivers/net/phy/libphy.ko] undefined! diff --git a/kernel/workqueue.c b/kernel/workqueue.c index 17c2f03..1cf226b 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -608,6 +608,7 @@ int