Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Andy Fleming]

On Nov 30, 2006, at 12:07, Maciej W. Rozycki wrote:


On Mon, 23 Oct 2006, Maciej W. Rozycki wrote:

I'm not too enthusiastic about requiring the ethernet drivers to  
call
phy_disconnect in a separate thread after close is called.   
Assuming there's
not some sort of squash work queue function that can be invoked  
with
rtnl_lock held, I think phy_disconnect should schedule itself to  
flush the
queue.  This would also require that mdiobus_unregister hold off  
on freeing
phydevs if any of the phys were still waiting for pending  
flush_pending calls
to finish.  Which would, in turn, require mdiobus_unregister to  
schedule

cleaning up memory for some later time.


 This could work, indeed.

I'm not enthusiastic about that implementation, either, but it  
maintains the
abstractions I consider important for this code.  The ethernet  
driver should
not need to know what structures the PHY lib uses to implement  
its interrupt

handling, and how to work around their failings, IMHO.


 Agreed.


 So what's the plan?

 Here's a new version of the patch that addresses your other concerns.



So I think the problem is we still don't understand the problem, and  
the solution to the problem, except that it's causing your driver to  
lock up.  Most of the changes below are fine with me.  The confusing  
one is still the check for current_is_keventd().  This is related in  
some way to why the driver code invokes phy_disconnect from a  
work_queue.  I admit, though, I'm not familiar enough with the work  
queue infrastructure to understand the problem.  But I'm very certain  
that creating a work queue for the sole purpose of disconnecting from  
the PHY is crufty.


Can you try again to convey how this solves your problem, so we can  
try to figure out if there's a better way?


Andy


-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Maciej W. Rozycki]

On Mon, 23 Oct 2006, Maciej W. Rozycki wrote:

  I'm not too enthusiastic about requiring the ethernet drivers to call
  phy_disconnect in a separate thread after close is called.  Assuming 
  there's
  not some sort of squash work queue function that can be invoked with
  rtnl_lock held, I think phy_disconnect should schedule itself to flush the
  queue.  This would also require that mdiobus_unregister hold off on freeing
  phydevs if any of the phys were still waiting for pending flush_pending 
  calls
  to finish.  Which would, in turn, require mdiobus_unregister to schedule
  cleaning up memory for some later time.
 
  This could work, indeed.
 
  I'm not enthusiastic about that implementation, either, but it maintains the
  abstractions I consider important for this code.  The ethernet driver should
  not need to know what structures the PHY lib uses to implement its interrupt
  handling, and how to work around their failings, IMHO.
 
  Agreed.

 So what's the plan?

 Here's a new version of the patch that addresses your other concerns.

  Maciej

patch-mips-2.6.18-20060920-phy-irq-18
diff -up --recursive --new-file 
linux-mips-2.6.18-20060920.macro/drivers/net/phy/phy.c 
linux-mips-2.6.18-20060920/drivers/net/phy/phy.c
--- linux-mips-2.6.18-20060920.macro/drivers/net/phy/phy.c  2006-08-05 
04:58:46.0 +
+++ linux-mips-2.6.18-20060920/drivers/net/phy/phy.c2006-11-30 
17:58:37.0 +
@@ -7,6 +7,7 @@
  * Author: Andy Fleming
  *
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
+ * Copyright (c) 2006  Maciej W. Rozycki
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -32,6 +33,8 @@
 #include linux/mii.h
 #include linux/ethtool.h
 #include linux/phy.h
+#include linux/timer.h
+#include linux/workqueue.h
 
 #include asm/io.h
 #include asm/irq.h
@@ -484,6 +487,9 @@ static irqreturn_t phy_interrupt(int irq
 {
struct phy_device *phydev = phy_dat;
 
+   if (PHY_HALTED == phydev-state)
+   return IRQ_NONE;/* It can't be ours.  */
+
/* The MDIO bus is not allowed to be written in interrupt
 * context, so we need to disable the irq here.  A work
 * queue will write the PHY to disable and clear the
@@ -577,6 +583,13 @@ int phy_stop_interrupts(struct phy_devic
if (err)
phy_error(phydev);
 
+   /*
+* Finish any pending work; we might have been scheduled
+* to be called from keventd ourselves, though.
+*/
+   if (!current_is_keventd())
+   flush_scheduled_work();
+
free_irq(phydev-irq, phydev);
 
return err;
@@ -596,14 +609,17 @@ static void phy_change(void *data)
goto phy_err;
 
spin_lock(phydev-lock);
+
if ((PHY_RUNNING == phydev-state) || (PHY_NOLINK == phydev-state))
phydev-state = PHY_CHANGELINK;
-   spin_unlock(phydev-lock);
 
enable_irq(phydev-irq);
 
/* Reenable interrupts */
-   err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
+   if (PHY_HALTED != phydev-state)
+   err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
+
+   spin_unlock(phydev-lock);
 
if (err)
goto irq_enable_err;
@@ -624,15 +640,15 @@ void phy_stop(struct phy_device *phydev)
if (PHY_HALTED == phydev-state)
goto out_unlock;
 
-   if (phydev-irq != PHY_POLL) {
-   /* Clear any pending interrupts */
-   phy_clear_interrupt(phydev);
+   phydev-state = PHY_HALTED;
 
+   if (phydev-irq != PHY_POLL) {
/* Disable PHY Interrupts */
phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
-   }
 
-   phydev-state = PHY_HALTED;
+   /* Clear any pending interrupts */
+   phy_clear_interrupt(phydev);
+   }
 
 out_unlock:
spin_unlock(phydev-lock);
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Maciej W. Rozycki]

On Fri, 20 Oct 2006, Andy Fleming wrote:

 I've been trying to figure out this problem since you posted this, and I'm not
 sure I understand it fully (And I apologize profusely for the horror that is
 the PHY interrupt handling code.  I'd love to rewrite it if there's some

 First of all I don't see much of the need to use soft timers with an 
interrupt-driven PHY.  Most of the state changes could be invoked straight 
from phy_change(), perhaps with an exception for the autonegotiation 
timeout.

 cleaner way.).  But let me see if I can follow the chain of reasoning that led
 to this patch, and see if we can figure out a solution that doesn't involve
 creating a work queue just for bringing down the PHY.

 Avoiding a separate work queue was my intent as well.

 1) Invoking phy_stop is meant to stop the system from looking for PHY status
 updates.  Currently, another PHY sharing the interrupt can cause the HALTED
 PHY to reenable interrupts.  Checking for HALTED in the interrupt handler
 fixes this, but it's incorrect.  The phy_interrupt handler does not grab the
 lock, and so you could get this:
 
 phy_stop
   lock
   clear any pending interrupts
   disable interrupts on this PHY
 --- interrupt from another PHY causes this PHY's interrupt handler to be
 invoked
   HALTED isn't set, so phy_change is scheduled
 --- set HALTED, unlock
 
 scheduled work is done:
   interrupt is reenabled
 
 Sadly, I think the only way to fix this problem is to have phy_change check
 for HALTED, and react appropriately.

 Please have a look at how I have rewritten phy_stop() to avoid this 
problem with no need for a lock -- HALTED is set first and only then 
interrupts are masked and cleared for this PHY.  It can be done without 
locking because the interrupt handler is strictly a consumer and 
phy_stop() is strictly a producer and we do not care about any other 
transitions [1].

 2) The PHY lib doesn't clear out remaining work in the work queue when it's
 bringing down a PHY.  This is clearly wrong, but I'm confused how *any* driver
 does this?  It seems to me that any network driver which has a work queue is
 going to be unable to flush the pending work when it is brought down.  So
 what's the solution to this?

 The only driver that seems to care is tg3.c and it gets away by other 
means.  Note that network drivers can quite easily handle and ignore 
deferred interrupt work as long as it arrives before they are removed from 
memory.  All that is required is calling flush_scheduled_work() from their 
module_exit() call at the very latest.

 I'm not too enthusiastic about requiring the ethernet drivers to call
 phy_disconnect in a separate thread after close is called.  Assuming there's
 not some sort of squash work queue function that can be invoked with
 rtnl_lock held, I think phy_disconnect should schedule itself to flush the
 queue.  This would also require that mdiobus_unregister hold off on freeing
 phydevs if any of the phys were still waiting for pending flush_pending calls
 to finish.  Which would, in turn, require mdiobus_unregister to schedule
 cleaning up memory for some later time.

 This could work, indeed.

 I'm not enthusiastic about that implementation, either, but it maintains the
 abstractions I consider important for this code.  The ethernet driver should
 not need to know what structures the PHY lib uses to implement its interrupt
 handling, and how to work around their failings, IMHO.

 Agreed.

 @@ -484,6 +487,9 @@ static irqreturn_t phy_interrupt(int irq
  {
   struct phy_device *phydev = phy_dat;
 
 +if (PHY_HALTED == phydev-state)
 +return IRQ_NONE;/* It can't be ours.  */
 +
 
 
 As I mentioned, this doesn't protect it, since it doesn't grab the lock.  And
 it can't grab the lock, or we'd have to disable interrupts while doing phy
 transactions.  And we can't do that, because one design goal is to allow some
 bus drivers to use interrupts to signal that the transaction has completed.
 Admittedly, this is still quite broken right now.  I'm looking into using
 semaphores, on the theory that I can sleep when I grab them.  But that would
 still prevent taking the semaphore in the interrupt controller.  This needs to
 be moved to phy_change (which you have done, anyway), and we just have to let
 the actual handler figure out whether it's safe to do anything.

 There is no problem with accessing the state here -- see above[1] and
below[2].

 @@ -577,6 +583,13 @@ int phy_stop_interrupts(struct phy_devic
   if (err)
phy_error(phydev);
 
 +/*
 + * Finish any pending work; we might have been scheduled
 + * to be called from keventd ourselves, though.
 + */
 +if (!current_is_keventd())
 +flush_scheduled_work();
 +
 
 
 And this is what is making you move your call to phy_disconnect to a work
 queue function, right?  Does this make it so phy_stop_interrupts (and anything
 that calls it) can't be called with rtnl_lock held? 

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Andy Fleming]

On Oct 3, 2006, at 10:18, Maciej W. Rozycki wrote:


Hello,

 This patch fixes a couple of problems discovered with interrupt  
handling

in the phylib core, namely:

1. The driver uses timer and workqueue calls, but does not include
   linux/timer.h nor linux/workqueue.h.



Good catch.




2. The driver uses schedule_work() for handling interrupts, but  
does not

   make sure any pending work scheduled thus has been completed before
   driver's structures get freed from memory.  This is especially
   important as interrupts may keep arriving if the line is shared  
with

   another PHY.

   The solution is to ignore phy_interrupt() calls if the reported  
device

   has already been halted and calling flush_scheduled_work() from
   phy_stop_interrupts() (but guarded with current_is_keventd() in  
case

   the function has been called through keventd from the MAC device's
   close call to avoid a deadlock on the netlink lock).



I've been trying to figure out this problem since you posted this,  
and I'm not sure I understand it fully (And I apologize profusely for  
the horror that is the PHY interrupt handling code.  I'd love to  
rewrite it if there's some cleaner way.).  But let me see if I can  
follow the chain of reasoning that led to this patch, and see if we  
can figure out a solution that doesn't involve creating a work queue  
just for bringing down the PHY.


1) Invoking phy_stop is meant to stop the system from looking for PHY  
status updates.  Currently, another PHY sharing the interrupt can  
cause the HALTED PHY to reenable interrupts.  Checking for HALTED in  
the interrupt handler fixes this, but it's incorrect.  The  
phy_interrupt handler does not grab the lock, and so you could get this:


phy_stop
lock
clear any pending interrupts
disable interrupts on this PHY
--- interrupt from another PHY causes this PHY's interrupt handler  
to be invoked

HALTED isn't set, so phy_change is scheduled
--- set HALTED, unlock

scheduled work is done:
interrupt is reenabled

Sadly, I think the only way to fix this problem is to have phy_change  
check for HALTED, and react appropriately.


2) The PHY lib doesn't clear out remaining work in the work queue  
when it's bringing down a PHY.  This is clearly wrong, but I'm  
confused how *any* driver does this?  It seems to me that any network  
driver which has a work queue is going to be unable to flush the  
pending work when it is brought down.  So what's the solution to this?


I'm not too enthusiastic about requiring the ethernet drivers to call  
phy_disconnect in a separate thread after close is called.   
Assuming there's not some sort of squash work queue function that  
can be invoked with rtnl_lock held, I think phy_disconnect should  
schedule itself to flush the queue.  This would also require that  
mdiobus_unregister hold off on freeing phydevs if any of the phys  
were still waiting for pending flush_pending calls to finish.  Which  
would, in turn, require mdiobus_unregister to schedule cleaning up  
memory for some later time.


I'm not enthusiastic about that implementation, either, but it  
maintains the abstractions I consider important for this code.  The  
ethernet driver should not need to know what structures the PHY lib  
uses to implement its interrupt handling, and how to work around  
their failings, IMHO.


And now, to be more specific, a few comments on the patch, directly:



 Please consider.

  Maciej

Signed-off-by: Maciej W. Rozycki [EMAIL PROTECTED]

patch-mips-2.6.18-20060920-phy-irq-16
diff -up --recursive --new-file linux-mips-2.6.18-20060920.macro/ 
drivers/net/phy/phy.c linux-mips-2.6.18-20060920/drivers/net/phy/phy.c
--- linux-mips-2.6.18-20060920.macro/drivers/net/phy/phy.c	 
2006-08-05 04:58:46.0 +
+++ linux-mips-2.6.18-20060920/drivers/net/phy/phy.c	2006-10-03  
14:19:21.0 +

@@ -7,6 +7,7 @@
  * Author: Andy Fleming
  *
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
+ * Copyright (c) 2006  Maciej W. Rozycki
  *
  * This program is free software; you can redistribute  it and/or  
modify it
  * under  the terms of  the GNU General  Public License as  
published by the

@@ -32,6 +33,8 @@
 #include linux/mii.h
 #include linux/ethtool.h
 #include linux/phy.h
+#include linux/timer.h
+#include linux/workqueue.h

 #include asm/io.h
 #include asm/irq.h
@@ -484,6 +487,9 @@ static irqreturn_t phy_interrupt(int irq
 {
struct phy_device *phydev = phy_dat;

+   if (PHY_HALTED == phydev-state)
+   return IRQ_NONE;/* It can't be ours.  */
+



As I mentioned, this doesn't protect it, since it doesn't grab the  
lock.  And it can't grab the lock, or we'd have to disable interrupts  
while doing phy transactions.  And we can't do that, because one  
design goal is to allow some bus drivers to use interrupts to signal  
that the transaction has completed.  Admittedly, this is still quite  
broken right now.  I'm 

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Andrew Morton]

On Fri, 20 Oct 2006 16:40:20 -0500
Andy Fleming [EMAIL PROTECTED] wrote:

 The solution is to ignore phy_interrupt() calls if the reported  
  device
 has already been halted and calling flush_scheduled_work() from
 phy_stop_interrupts() (but guarded with current_is_keventd() in  
  case
 the function has been called through keventd from the MAC device's
 close call to avoid a deadlock on the netlink lock).
 
 
 I've been trying to figure out this problem since you posted this,  
 and I'm not sure I understand it fully

Me either, but I basically gave up.

If it is deadlocky for keventd to call flush_scheduled_work() I fail to see
why it is not deadlocky for other processes.

If the caller of flush_scheduled_work() holds rtnl_lock, and if a queued
work callback also takes rtnl_lock then the flush_scheduled_work() caller
will deadlock regardless of whether or not it is keventd.

Because the flush_scheduled_work() caller holds a lock which will prevent
the flush from completing.

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Maciej W. Rozycki]

On Mon, 16 Oct 2006, Andrew Morton wrote:

 Vaguely.  Why doesn't it deadlock if !current_is_keventd()?  I mean,
 whether or not the caller is keventd, the flush_scheduled_work() caller
 will still be dependent upon rtnl_lock() being acquirable.

 This !current_is_keventd() condition is just what it is, i.e. a check 
whether phy_stop_interrupts() has been scheduled through keventd or not.  
If it has, then flush_scheduled_work() cannot be called; it is not needed 
anyway.  Otherwise phy_stop_interrupts() has to make sure no deferred 
calls to phy_change() will be made once it has finished.

 In all cases the assumption is the caller has made sure rtnl_lock() is 
not held at the time phy_stop_interrupts() is called.  That's the very 
reason for scheduling phy_disconnect() (and hence phy_stop_interrupts()) 
through keventd.

  Maciej
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Maciej W. Rozycki]

Andrew,

 I don't get it.  If some code does
 
   rtnl_lock();
   flush_scheduled_work();
 
 and there's some work scheduled which does rtnl_lock() then it'll deadlock.
 
 But it'll deadlock whether or not the caller of flush_scheduled_work() is
 keventd.
 
 Calling flush_scheduled_work() under locks is generally a bad idea.

 Indeed -- this is why I avoid it.

 I'd have thought that was still deadlockable.  Could you describe the
 deadlock more completely please?

 The simplest sequence of calls that prevents races here is as follows:

unregister_netdev();
  rtnl_lock();
  unregister_netdevice();
dev_close();
  sbmac_close();
phy_stop();
phy_disconnect();
  phy_stop_interrupts();
phy_disable_interrupts();
flush_scheduled_work();
free_irq();
  phy_detach();
mdiobus_unregister();
  rtnl_unlock();

We want to call flush_scheduled_work() from phy_stop_interrupts(), because 
there may still be calls to phy_change() waiting for the keventd to 
process and mdiobus_unregister() frees structures needed by phy_change().  
This may deadlock because of the call to rtnl_lock() though.

 So the modified sequence I have implemented is as follows:

unregister_netdev();
  rtnl_lock();
  unregister_netdevice();
dev_close();
  sbmac_close();
phy_stop();
schedule_work(); [sbmac_phy_disconnect()]
  rtnl_unlock();

and then:

sbmac_phy_disconnect();
  phy_disconnect();
phy_stop_interrupts();
  phy_disable_interrupts();
  free_irq();
phy_detach();
  mdiobus_unregister();

This guarantees calls to phy_change() for this PHY unit will not be run 
after mdiobus_unregister(), because any such calls have been placed in the 
queue before sbmac_phy_disconnect() (phy_stop() prevents the interrupt 
handler from issuing new calls to phy_change()).

 We still need flush_scheduled_work() to be called from 
phy_stop_interrupts() though, in case some other MAC driver calls 
phy_disconnect() (or phy_stop_interrupts(), depending on the abstraction 
layer of phylib used) directly rather than using keventd.  This is 
possible if phy_disconnect() is called from the driver's module_exit() 
call, which may make sense for devices that are known not to have their 
MII interface available as an external connector.  Hence the:

if (!current_is_keventd())
  flush_scheduled_work();

sequence in phy_stop_interrupts().  Of course, we can force all drivers 
using phylib (in the interrupt mode) to call phy_disconnect() through 
keventd instead.

 Does it sound clearer?

  Maciej
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Andrew Morton]

On Mon, 16 Oct 2006 15:50:55 +0100 (BST)
Maciej W. Rozycki [EMAIL PROTECTED] wrote:

 Andrew,
 
  I don't get it.  If some code does
  
  rtnl_lock();
  flush_scheduled_work();
  
  and there's some work scheduled which does rtnl_lock() then it'll deadlock.
  
  But it'll deadlock whether or not the caller of flush_scheduled_work() is
  keventd.
  
  Calling flush_scheduled_work() under locks is generally a bad idea.
 
  Indeed -- this is why I avoid it.
 
  I'd have thought that was still deadlockable.  Could you describe the
  deadlock more completely please?
 
  The simplest sequence of calls that prevents races here is as follows:
 
 unregister_netdev();
   rtnl_lock();
   unregister_netdevice();
 dev_close();
   sbmac_close();
 phy_stop();
 phy_disconnect();
   phy_stop_interrupts();
 phy_disable_interrupts();
 flush_scheduled_work();
 free_irq();
   phy_detach();
 mdiobus_unregister();
   rtnl_unlock();
 
 We want to call flush_scheduled_work() from phy_stop_interrupts(), because 
 there may still be calls to phy_change() waiting for the keventd to 
 process and mdiobus_unregister() frees structures needed by phy_change().  
 This may deadlock because of the call to rtnl_lock() though.
 
  So the modified sequence I have implemented is as follows:
 
 unregister_netdev();
   rtnl_lock();
   unregister_netdevice();
 dev_close();
   sbmac_close();
 phy_stop();
 schedule_work(); [sbmac_phy_disconnect()]
   rtnl_unlock();
 
 and then:
 
 sbmac_phy_disconnect();
   phy_disconnect();
 phy_stop_interrupts();
   phy_disable_interrupts();
   free_irq();
 phy_detach();
   mdiobus_unregister();
 
 This guarantees calls to phy_change() for this PHY unit will not be run 
 after mdiobus_unregister(), because any such calls have been placed in the 
 queue before sbmac_phy_disconnect() (phy_stop() prevents the interrupt 
 handler from issuing new calls to phy_change()).
 
  We still need flush_scheduled_work() to be called from 
 phy_stop_interrupts() though, in case some other MAC driver calls 
 phy_disconnect() (or phy_stop_interrupts(), depending on the abstraction 
 layer of phylib used) directly rather than using keventd.  This is 
 possible if phy_disconnect() is called from the driver's module_exit() 
 call, which may make sense for devices that are known not to have their 
 MII interface available as an external connector.  Hence the:
 
 if (!current_is_keventd())
   flush_scheduled_work();
 
 sequence in phy_stop_interrupts().  Of course, we can force all drivers 
 using phylib (in the interrupt mode) to call phy_disconnect() through 
 keventd instead.
 
  Does it sound clearer?
 

Vaguely.  Why doesn't it deadlock if !current_is_keventd()?  I mean,
whether or not the caller is keventd, the flush_scheduled_work() caller
will still be dependent upon rtnl_lock() being acquirable.


-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Andrew Morton]

On Tue, 3 Oct 2006 16:18:35 +0100 (BST)
Maciej W. Rozycki [EMAIL PROTECTED] wrote:

 
 2. The driver uses schedule_work() for handling interrupts, but does not 
make sure any pending work scheduled thus has been completed before 
driver's structures get freed from memory.  This is especially 
important as interrupts may keep arriving if the line is shared with 
another PHY.
 
The solution is to ignore phy_interrupt() calls if the reported device 
has already been halted and calling flush_scheduled_work() from 
phy_stop_interrupts() (but guarded with current_is_keventd() in case 
the function has been called through keventd from the MAC device's 
close call to avoid a deadlock on the netlink lock).
 

eww, hack.

Also not module-friendly:

WARNING: current_is_keventd [drivers/net/phy/libphy.ko] undefined!

Does this

static void flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)
{
if (cwq-thread == current) {
/*
 * Probably keventd trying to flush its own queue. So simply run
 * it by hand rather than deadlocking.
 */
run_workqueue(cwq);

not work???
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Maciej W. Rozycki]

On Thu, 5 Oct 2006, Andrew Morton wrote:

  2. The driver uses schedule_work() for handling interrupts, but does not 
 make sure any pending work scheduled thus has been completed before 
 driver's structures get freed from memory.  This is especially 
 important as interrupts may keep arriving if the line is shared with 
 another PHY.
  
 The solution is to ignore phy_interrupt() calls if the reported device 
 has already been halted and calling flush_scheduled_work() from 
 phy_stop_interrupts() (but guarded with current_is_keventd() in case 
 the function has been called through keventd from the MAC device's 
 close call to avoid a deadlock on the netlink lock).
  
 
 eww, hack.
 
 Also not module-friendly:
 
 WARNING: current_is_keventd [drivers/net/phy/libphy.ko] undefined!
 
 Does this
 
 static void flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)
 {
   if (cwq-thread == current) {
   /*
* Probably keventd trying to flush its own queue. So simply run
* it by hand rather than deadlocking.
*/
   run_workqueue(cwq);
 
 not work???

 It does, too well even! -- in the case I am trying to take care of we are 
run with rtnl_mutex held as a result of rtnl_lock() called from 
unregister_netdev() and there is some work already in the workqueue 
(linkwatch_event(), apparently) wanting to acquire the same lock.  Hence a 
deadlock.  It seems problematic elsewhere as well -- see tg3.c -- but a 
different way to deal with it has been found there.

 I am not that fond of this solution, but at least it works, unlike 
calling flush_scheduled_work() here, which deadlocks the current CPU in my 
system instantly.  Any suggestions as to how to do this differently?  
Perhaps linkwatch_event() should be scheduled differently (using a 
separate workqueue?)...  Or does dev_close() have to be called under this 
lock from unregister_netdevice()?  Perhaps code like this would do:

while (dev-flags  IFF_UP) {
rtnl_unlock();
dev_close(dev);
rtnl_lock();
}

  Maciej
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Andrew Morton]

On Fri, 6 Oct 2006 12:26:22 +0100 (BST)
Maciej W. Rozycki [EMAIL PROTECTED] wrote:

 On Thu, 5 Oct 2006, Andrew Morton wrote:
 
   2. The driver uses schedule_work() for handling interrupts, but does not 
  make sure any pending work scheduled thus has been completed before 
  driver's structures get freed from memory.  This is especially 
  important as interrupts may keep arriving if the line is shared with 
  another PHY.
   
  The solution is to ignore phy_interrupt() calls if the reported device 
  has already been halted and calling flush_scheduled_work() from 
  phy_stop_interrupts() (but guarded with current_is_keventd() in case 
  the function has been called through keventd from the MAC device's 
  close call to avoid a deadlock on the netlink lock).
   
  
  eww, hack.
  
  Also not module-friendly:
  
  WARNING: current_is_keventd [drivers/net/phy/libphy.ko] undefined!
  
  Does this
  
  static void flush_cpu_workqueue(struct cpu_workqueue_struct *cwq)
  {
  if (cwq-thread == current) {
  /*
   * Probably keventd trying to flush its own queue. So simply run
   * it by hand rather than deadlocking.
   */
  run_workqueue(cwq);
  
  not work???
 
  It does, too well even! -- in the case I am trying to take care of we are 
 run with rtnl_mutex held as a result of rtnl_lock() called from 
 unregister_netdev() and there is some work already in the workqueue 
 (linkwatch_event(), apparently) wanting to acquire the same lock.  Hence a 
 deadlock.

I don't get it.  If some code does

rtnl_lock();
flush_scheduled_work();

and there's some work scheduled which does rtnl_lock() then it'll deadlock.

But it'll deadlock whether or not the caller of flush_scheduled_work() is
keventd.

Calling flush_scheduled_work() under locks is generally a bad idea.

  It seems problematic elsewhere as well -- see tg3.c -- but a 
 different way to deal with it has been found there.
 
  I am not that fond of this solution, but at least it works, unlike 
 calling flush_scheduled_work() here, which deadlocks the current CPU in my 
 system instantly.  Any suggestions as to how to do this differently?  
 Perhaps linkwatch_event() should be scheduled differently (using a 
 separate workqueue?)...

I'd have thought that was still deadlockable.  Could you describe the
deadlock more completely please?

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch 3/6] 2.6.18: sb1250-mac: Phylib IRQ handling fixes

[Maciej W. Rozycki]

Hello,

 This patch fixes a couple of problems discovered with interrupt handling 
in the phylib core, namely:

1. The driver uses timer and workqueue calls, but does not include 
   linux/timer.h nor linux/workqueue.h.

2. The driver uses schedule_work() for handling interrupts, but does not 
   make sure any pending work scheduled thus has been completed before 
   driver's structures get freed from memory.  This is especially 
   important as interrupts may keep arriving if the line is shared with 
   another PHY.

   The solution is to ignore phy_interrupt() calls if the reported device 
   has already been halted and calling flush_scheduled_work() from 
   phy_stop_interrupts() (but guarded with current_is_keventd() in case 
   the function has been called through keventd from the MAC device's 
   close call to avoid a deadlock on the netlink lock).

 Please consider.

  Maciej

Signed-off-by: Maciej W. Rozycki [EMAIL PROTECTED]

patch-mips-2.6.18-20060920-phy-irq-16
diff -up --recursive --new-file 
linux-mips-2.6.18-20060920.macro/drivers/net/phy/phy.c 
linux-mips-2.6.18-20060920/drivers/net/phy/phy.c
--- linux-mips-2.6.18-20060920.macro/drivers/net/phy/phy.c  2006-08-05 
04:58:46.0 +
+++ linux-mips-2.6.18-20060920/drivers/net/phy/phy.c2006-10-03 
14:19:21.0 +
@@ -7,6 +7,7 @@
  * Author: Andy Fleming
  *
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
+ * Copyright (c) 2006  Maciej W. Rozycki
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -32,6 +33,8 @@
 #include linux/mii.h
 #include linux/ethtool.h
 #include linux/phy.h
+#include linux/timer.h
+#include linux/workqueue.h
 
 #include asm/io.h
 #include asm/irq.h
@@ -484,6 +487,9 @@ static irqreturn_t phy_interrupt(int irq
 {
struct phy_device *phydev = phy_dat;
 
+   if (PHY_HALTED == phydev-state)
+   return IRQ_NONE;/* It can't be ours.  */
+
/* The MDIO bus is not allowed to be written in interrupt
 * context, so we need to disable the irq here.  A work
 * queue will write the PHY to disable and clear the
@@ -577,6 +583,13 @@ int phy_stop_interrupts(struct phy_devic
if (err)
phy_error(phydev);
 
+   /*
+* Finish any pending work; we might have been scheduled
+* to be called from keventd ourselves, though.
+*/
+   if (!current_is_keventd())
+   flush_scheduled_work();
+
free_irq(phydev-irq, phydev);
 
return err;
@@ -603,7 +616,8 @@ static void phy_change(void *data)
enable_irq(phydev-irq);
 
/* Reenable interrupts */
-   err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
+   if (PHY_HALTED != phydev-state)
+   err = phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED);
 
if (err)
goto irq_enable_err;
@@ -624,18 +638,24 @@ void phy_stop(struct phy_device *phydev)
if (PHY_HALTED == phydev-state)
goto out_unlock;
 
-   if (phydev-irq != PHY_POLL) {
-   /* Clear any pending interrupts */
-   phy_clear_interrupt(phydev);
+   phydev-state = PHY_HALTED;
 
+   if (phydev-irq != PHY_POLL) {
/* Disable PHY Interrupts */
phy_config_interrupt(phydev, PHY_INTERRUPT_DISABLED);
-   }
 
-   phydev-state = PHY_HALTED;
+   /* Clear any pending interrupts */
+   phy_clear_interrupt(phydev);
+   }
 
 out_unlock:
spin_unlock(phydev-lock);
+
+   /*
+* Cannot call flush_scheduled_work() here as desired because
+* of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
+* will not reenable interrupts.
+*/
 }
 
 
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html