Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[yshi <yang.shi@windriver.com>]

Hi Jeff,

Thanks for the comment. According your suggestion, I added 
local_irq_save()/local_irq_restore() in rtl8139_netpoll_controller().
Move the burden to netpoll layer. The following is the modified patch:

Replaced disable_irq()/enable_irq() with
local_irq_save()/local_irq_restore() to improve
netconsole/netpoll support on RTL8139 NIC driver.

Signed-off-by: Yang Shi <yang.shi@windriver.com>
---
 b/drivers/net/8139too.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
---

--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -2199,9 +2199,11 @@ static irqreturn_t rtl8139_interrupt (in
  */
 static void rtl8139_poll_controller(struct net_device *dev)
 {
-       disable_irq(dev->irq);
+       unsigned long flags;
+
+       local_irq_save(flags);
        rtl8139_interrupt(dev->irq, dev);
-       enable_irq(dev->irq);
+       local_irq_restore(flags);
 }
 #endif

Thanks.

Yang
Jeff Garzik 写道:
> yshi wrote:
>> In current RTL8139 NIC driver, spin_lock()/spin_unlock() is used
>> for irq handler. But for netconsole/netpoll, it prefers
>> spin_lock_irqsave()/spin_unlcok_irqrestore(). So this patch fixed
>> this problem to improve netconsole/netpoll support.
>>
>> Signed-off-by: Yang Shi <yang.shi@windriver.com>
>> ---
>> b/drivers/net/8139too.c |    9 +++++++++
>> 1 file changed, 9 insertions(+)
>> ---
>>
>> --- a/drivers/net/8139too.c
>> +++ b/drivers/net/8139too.c
>> @@ -2136,8 +2136,13 @@ static irqreturn_t rtl8139_interrupt (in
>>        u16 status, ackstat;
>>        int link_changed = 0; /* avoid bogus "uninit" warning */
>>        int handled = 0;
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> +       unsigned long flags;
>>
>> +       spin_lock_irqsave (&tp->lock, flags);
>> +#else
>>        spin_lock (&tp->lock);
>> +#endif
>>        status = RTL_R16 (IntrStatus);
>>
>>        /* shared irq? */
>> @@ -2185,7 +2190,11 @@ static irqreturn_t rtl8139_interrupt (in
>>                        RTL_W16 (IntrStatus, TxErr);
>>        }
>>  out:
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> +       spin_unlock_irqrestore (&tp->lock, flags);
>> +#else
>>        spin_unlock (&tp->lock);
>> +#endif
>
> This is bogus -- you should never need to slow down the hot path in 
> such a way.
>
> Add a local_irq_save()/restore() to rtl8139_poll_controller() or a 
> similar solution, putting the burden on the netpoll/netconsole layer.
>
>     Jeff
>
>
>
>