The attached patch adds support for suspend/resume to the sis900 driver.
With this patch on resume the NIC is fully configured and operational,
before a module reload was needed because of the complete lack of
suspend/resume callbacks.
I added two functions, sis900_suspend and sis900_resume, with their
pointers in struct pci_driver. A vector of 16 u32 was then needed to the
to keep PCI data during suspend. I added it in struct sis900_private.
I updated the revision number to reflect my changes.
Looking at the code I also killed three typos.
The patch doesn't touch any other code.
Since I don't know anything on ethernet drivers the rule 'works for me'
is fully valid.
Please consider this patch for inclusion when the freeze is over, the
sis900 driver is not updated since 2002 (looking at the changelog) and
the email address listed in the MAINTAINERS file bounces.
I'll try to keep the patch updated on new kernel releases at:
http://teg.homeunix.org/kernel_patches.html
Bye.
--
------------------------------
Daniele Venzano
Web: http://teg.homeunix.org
Daniele Venzano <[email protected]> wrote:
>
> The attached patch adds support for suspend/resume to the sis900 driver.
...
> +static int sis900_suspend(struct pci_dev *pci_dev, u32 state)
> +{
> + struct net_device *net_dev = pci_get_drvdata(pci_dev);
> + struct sis900_private *sis_priv = net_dev->priv;
> + long ioaddr = net_dev->base_addr;
> + unsigned long flags;
> +
> + if(!netif_running(net_dev))
> + return 0;
> + netif_stop_queue(net_dev);
> +
> + netif_device_detach(net_dev);
> + spin_lock_irqsave(&sis_priv->lock, flags);
> +
> + /* Stop the chip's Tx and Rx Status Machine */
> + outl(RxDIS | TxDIS | inl(ioaddr + cr), ioaddr + cr);
> +
> + pci_set_power_state(pci_dev, 3);
pci_set_power_state() can sleep, so we shouldn't be calling it
under spin_lock_irqsave(). Is it necessary to hold the lock
here?
On Sun, Nov 02, 2003 at 11:12:54AM -0800, Andrew Morton wrote:
> > + spin_lock_irqsave(&sis_priv->lock, flags);
> > +
> > + /* Stop the chip's Tx and Rx Status Machine */
> > + outl(RxDIS | TxDIS | inl(ioaddr + cr), ioaddr + cr);
> > +
> > + pci_set_power_state(pci_dev, 3);
>
> pci_set_power_state() can sleep, so we shouldn't be calling it
> under spin_lock_irqsave(). Is it necessary to hold the lock
> here?
I don't know enough of the driver to express an opinion, but probably
the lock is needed only for the outl call, if needed at all. If nothing
new comes out, tomorrow I'll rediff and send a new patch.
--
------------------------------
Daniele Venzano
Web: http://teg.homeunix.org
On Sun, Nov 02, 2003 at 11:12:54AM -0800, Andrew Morton wrote:
> pci_set_power_state() can sleep, so we shouldn't be calling it
> under spin_lock_irqsave(). Is it necessary to hold the lock
> here?
New patch with locking completely removed, since in a similar
function none was used.
I think also the 8139too driver has the same locking problem in
rtl8139_suspend, do you want a patch ?
--
------------------------------
Daniele Venzano
Web: http://teg.homeunix.org
Daniele Venzano <[email protected]> wrote:
>
> On Sun, Nov 02, 2003 at 11:12:54AM -0800, Andrew Morton wrote:
> > pci_set_power_state() can sleep, so we shouldn't be calling it
> > under spin_lock_irqsave(). Is it necessary to hold the lock
> > here?
>
> New patch with locking completely removed, since in a similar
> function none was used.
OK. I think. Net driver suspend handlers in general seem a bit racy wrt
interrupt activity as well as SMP. Maybe I'm missing something.
> I think also the 8139too driver has the same locking problem in
> rtl8139_suspend, do you want a patch ?
Wouldn't hurt, thanks. It's one way to wake Jeff up ;)
8139too just does netif_device_detach(), whereas your sis900 patch does
netif_stop_queue() and then netif_device_detach().
I don't know which is right, really. 8139too will end up with a
non-stopped queue if __LINK_STATE_PRESENT is clear. The sis900 approach is
certainly safe enough, but it'd be nice to know what netif_device_detach()
is trying to do there.
On Mon, Nov 03, 2003 at 12:06:47PM -0800, Andrew Morton wrote:
> OK. I think. Net driver suspend handlers in general seem a bit racy wrt
> interrupt activity as well as SMP. Maybe I'm missing something.
Looking at the logs I just discovered the following line buried in the
middle of all that Bad: scheduling while atomic during resume:
eth0: Abnormal interrupt, status 0x03008200
This seems to happen with all the patches I sent you, but then the
network card is working correctly, it seems a leftover handled correctly
on the first interrupt..
> > I think also the 8139too driver has the same locking problem in
> > rtl8139_suspend, do you want a patch ?
>
> Wouldn't hurt, thanks. It's one way to wake Jeff up ;)
Attached you find a patch that just moves up the lock release before the
call to pci_set_power_state. (and Jeff is CCed)
> 8139too just does netif_device_detach(), whereas your sis900 patch does
> netif_stop_queue() and then netif_device_detach().
>
> I don't know which is right, really. 8139too will end up with a
> non-stopped queue if __LINK_STATE_PRESENT is clear. The sis900 approach is
> certainly safe enough, but it'd be nice to know what netif_device_detach()
> is trying to do there.
I tried to comment out the stop_queue call, and suspending with the
network cable unplugged. The card keeps working on resume, as I was
expecting.
Perhaps we could keep only the stop_queue call and still remain on the
safe side, certainly doing two times the same call is dumb.
With this setup (only stop_queue) I did not get any Abnormal interrupt
message, but I don't know if they are related.
So in the end you get two patches, the 8139too and the sis900 with no
locking and no netif_device_detach.
Both are available at http://teg.homeunix.org/kernel_patches.html
--
------------------------------
Daniele Venzano
Web: http://teg.homeunix.org