[gPXE-devel] [PATCH] [sky2] Disable interrupts by default

[Stefan Hajnoczi]

On Thu, Jul 29, 2010 at 8:00 PM, Joshua Oreman <oremanj at rwcr.net> wrote:
> On Thu, Jul 29, 2010 at 11:56 AM, Stefan Hajnoczi <stefanha at gmail.com> wrote:
>> Device interrupts should be disabled by default in the sky2 driver.
>> They are explicitly enabled when a PXE NBP client program uses the UNDI
>> interface, which is designed around interrupts.  Having interrupts
>> enabled when no handler is installed to service them results in hangs
>> because the spurious interrupt is never reset and remains high.
>
> Looks good to me. Thanks, Stefan!
>
> Feel free to push after 24 hours.
>
> -- Josh

Thanks the the review Josh.

Gilles: I'd be thankful if you're able to run two tests with the patch applied:

1. DHCP and TFTP or HTTP boot using a gPXE sky2 image.

If you want a quick over-the-internet boot test, try this:
dhcp net0
chain http://vmsplice.net/tinycore.gpxe

It will boot Tiny Core Linux over HTTP.

2. Same test but from a chainloaded undionly.kpxe.  This means first
loading undionly.kpxe from a gPXE sky2 image:

dhcp net0
chain http://myserver/undionly.kpxe

And then at undionly.kpxe's gPXE prompt:

dhcp net0
chain http://vmsplice.net/tinycore.gpxe

The undionly.kpxe gPXE image will be loading Tiny Core Linux using its
UNDI driver.  The gPXE sky2 image underneath provides the UNDI
interface and performs the actual network I/O using the native sky2
driver.

I don't have a sky2 card here and have no datasheet either.  Testing
would be very helpful.

Thanks,
Stefan

>
>> Signed-off-by: Stefan Hajnoczi <stefanha at gmail.com>
>> ---
>> I wrote this on the train.  Now after comparing against Daniel's original patch
>> it is very similar and so may require further debugging to get the UNDI case
>> working.
>>
>>  src/drivers/net/sky2.c |   24 ++++++++++--------------
>>  1 files changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/drivers/net/sky2.c b/src/drivers/net/sky2.c
>> index 00940af..07487ac 100644
>> --- a/src/drivers/net/sky2.c
>> +++ b/src/drivers/net/sky2.c
>> @@ -1130,7 +1130,7 @@ static int sky2_up(struct net_device *dev)
>>        struct sky2_port *sky2 = netdev_priv(dev);
>>        struct sky2_hw *hw = sky2->hw;
>>        unsigned port = sky2->port;
>> -       u32 imask, ramsize;
>> +       u32 ramsize;
>>        int err = -ENOMEM;
>>
>>        netdev_link_down(dev);
>> @@ -1198,11 +1198,6 @@ static int sky2_up(struct net_device *dev)
>>        if (err)
>>                goto err_out;
>>
>> -       /* Enable interrupts from phy/mac for port */
>> -       imask = sky2_read32(hw, B0_IMSK);
>> -       imask |= portirq_msk[port];
>> -       sky2_write32(hw, B0_IMSK, imask);
>> -
>>        DBGIO(PFX "%s: le bases: st %p [%x], rx %p [%x], tx %p [%x]\n",
>>              dev->name, hw->st_le, hw->st_dma, sky2->rx_le, sky2->rx_le_map,
>>              sky2->tx_le, sky2->tx_le_map);
>> @@ -1314,7 +1309,6 @@ static void sky2_down(struct net_device *dev)
>>        struct sky2_hw *hw = sky2->hw;
>>        unsigned port = sky2->port;
>>        u16 ctrl;
>> -       u32 imask;
>>
>>        /* Never really got started! */
>>        if (!sky2->tx_le)
>> @@ -1323,9 +1317,7 @@ static void sky2_down(struct net_device *dev)
>>        DBG2(PFX "%s: disabling interface\n", dev->name);
>>
>>        /* Disable port IRQ */
>> -       imask = sky2_read32(hw, B0_IMSK);
>> -       imask &= ~portirq_msk[port];
>> -       sky2_write32(hw, B0_IMSK, imask);
>> +       netdev_irq(dev, 0);
>>
>>        sky2_gmac_reset(hw, port);
>>
>> @@ -2250,9 +2242,14 @@ static void sky2_net_irq(struct net_device *dev, int enable)
>>
>>        u32 imask = sky2_read32(hw, B0_IMSK);
>>        if (enable)
>> -               imask |= portirq_msk[sky2->port];
>> +               imask |= Y2_IS_BASE | portirq_msk[sky2->port];
>>        else
>>                imask &= ~portirq_msk[sky2->port];
>> +
>> +       /* Fully turn off interrupts if disabled for both ports */
>> +       if ((imask & ~Y2_IS_BASE) == 0)
>> +               imask = 0;
>> +
>>        sky2_write32(hw, B0_IMSK, imask);
>>  }
>>
>> @@ -2321,7 +2318,8 @@ static int sky2_probe(struct pci_device *pdev,
>>                goto err_out_free_netdev;
>>        }
>>
>> -       sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
>> +       /* Disable interrupts by default */
>> +       sky2_write32(hw, B0_IMSK, 0);
>>
>>        sky2_show_addr(dev);
>>
>> @@ -2370,8 +2368,6 @@ static void sky2_remove(struct pci_device *pdev)
>>        for (i = hw->ports-1; i >= 0; --i)
>>                unregister_netdev(hw->dev[i]);
>>
>> -       sky2_write32(hw, B0_IMSK, 0);
>> -
>>        sky2_power_aux(hw);
>>
>>        sky2_write16(hw, B0_Y2LED, LED_STAT_OFF);
>> --
>> 1.7.1
>>
>>
>