[RFC][PATCH] Add suspend and resume support to uli526x

[RFC][PATCH] Add suspend and resume support to uli526x

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Add suspend/resume support to the uli526x network driver (tested on x86_64,
with 'Ethernet controller: ALi Corporation M5263 Ethernet Controller, rev 40').

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 drivers/net/tulip/uli526x.c |  120 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 105 insertions(+), 15 deletions(-)

Index: linux-2.6.22-rc3/drivers/net/tulip/uli526x.c
===================================================================
--- linux-2.6.22-rc3.orig/drivers/net/tulip/uli526x.c	2007-06-03 12:10:41.000000000 +0200
+++ linux-2.6.22-rc3/drivers/net/tulip/uli526x.c	2007-06-03 12:14:33.000000000 +0200
@@ -220,6 +220,10 @@ static int mode = 8;
 static int uli526x_open(struct net_device *);
 static int uli526x_start_xmit(struct sk_buff *, struct net_device *);
 static int uli526x_stop(struct net_device *);
+#ifdef CONFIG_PM
+static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state);
+static int uli526x_resume(struct pci_dev *pdev);
+#endif
 static struct net_device_stats * uli526x_get_stats(struct net_device *);
 static void uli526x_set_filter_mode(struct net_device *);
 static const struct ethtool_ops netdev_ethtool_ops;
@@ -425,21 +429,14 @@ static void __devexit uli526x_remove_one
 
 
 /*
- *	Open the interface.
- *	The interface is opened whenever "ifconfig" activates it.
+ *	Bring the interface up.
+ *	Used by uli526x_open and uli526x_resume.
  */
 
-static int uli526x_open(struct net_device *dev)
+static void uli526x_up(struct net_device *dev)
 {
-	int ret;
 	struct uli526x_board_info *db = netdev_priv(dev);
 
-	ULI526X_DBUG(0, "uli526x_open", 0);
-
-	ret = request_irq(dev->irq, &uli526x_interrupt, IRQF_SHARED, dev->name, dev);
-	if (ret)
-		return ret;
-
 	/* system variable init */
 	db->cr6_data = CR6_DEFAULT | uli526x_cr6_user_set;
 	db->tx_packet_cnt = 0;
@@ -467,7 +464,25 @@ static int uli526x_open(struct net_devic
 	db->timer.data = (unsigned long)dev;
 	db->timer.function = &uli526x_timer;
 	add_timer(&db->timer);
+}
+
+
+/*
+ *	Open the interface.
+ *	The interface is opened whenever "ifconfig" activates it.
+ */
 
+static int uli526x_open(struct net_device *dev)
+{
+	int ret;
+
+	ULI526X_DBUG(0, "uli526x_open", 0);
+
+	ret = request_irq(dev->irq, &uli526x_interrupt, IRQF_SHARED, dev->name, dev);
+	if (ret)
+		return ret;
+
+	uli526x_up(dev);
 	return 0;
 }
 
@@ -613,17 +628,15 @@ static int uli526x_start_xmit(struct sk_
 
 
 /*
- *	Stop the interface.
- *	The interface is stopped when it is brought.
+ *	Take the interface down.
+ *	Used by uli526x_stop and uli526x_suspend.
  */
 
-static int uli526x_stop(struct net_device *dev)
+static void uli526x_down(struct net_device *dev)
 {
 	struct uli526x_board_info *db = netdev_priv(dev);
 	unsigned long ioaddr = dev->base_addr;
 
-	ULI526X_DBUG(0, "uli526x_stop", 0);
-
 	/* disable system */
 	netif_stop_queue(dev);
 
@@ -634,6 +647,21 @@ static int uli526x_stop(struct net_devic
 	outl(ULI526X_RESET, ioaddr + DCR0);
 	udelay(5);
 	phy_write(db->ioaddr, db->phy_addr, 0, 0x8000, db->chip_id);
+}
+
+
+/*
+ *	Stop the interface.
+ *	The interface is stopped when it is brought.
+ */
+
+static int uli526x_stop(struct net_device *dev)
+{
+	struct uli526x_board_info *db = netdev_priv(dev);
+
+	ULI526X_DBUG(0, "uli526x_stop", 0);
+
+	uli526x_down(dev);
 
 	/* free interrupt */
 	free_irq(dev->irq, dev);
@@ -654,6 +682,64 @@ static int uli526x_stop(struct net_devic
 }
 
 
+#ifdef CONFIG_PM
+
+/*
+ *	Suspend the interface.
+ */
+
+static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+
+	ULI526X_DBUG(0, "uli526x_suspend", 0);
+
+	if (dev && netdev_priv(dev)) {
+		if (netif_running(dev)) {
+			netif_device_detach(dev);
+			uli526x_down(dev);
+		}
+		pci_save_state(pdev);
+		pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
+		pci_disable_device(pdev);
+		pci_set_power_state(pdev, pci_choose_state(pdev, state));
+	}
+	return 0;
+}
+
+/*
+ *	Resume the interface.
+ */
+
+static int uli526x_resume(struct pci_dev *pdev)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+	struct uli526x_board_info *db = netdev_priv(dev);
+	int err;
+
+	ULI526X_DBUG(0, "uli526x_resume", 0);
+
+	if (dev && db) {
+		pci_set_power_state(pdev, PCI_D0);
+		err = pci_enable_device(pdev);
+		if (err) {
+			printk(KERN_WARNING "%s: Could not enable device \n",
+				dev->name);
+			return err;
+		}
+		pci_restore_state(pdev);
+		pci_set_master(pdev);
+		if (netif_running(dev)) {
+			uli526x_up(dev);
+			netif_device_attach(dev);
+		}
+	}
+	return 0;
+}
+
+#endif /* CONFIG_PM */
+
+
 /*
  *	M5261/M5263 insterrupt handler
  *	receive the packet to upper layer, free the transmitted packet
@@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
 	.id_table	= uli526x_pci_tbl,
 	.probe		= uli526x_init_one,
 	.remove		= __devexit_p(uli526x_remove_one),
+#ifdef CONFIG_PM
+	.suspend	= uli526x_suspend,
+	.resume		= uli526x_resume,
+#endif
 };
 
 MODULE_AUTHOR("Peer Chen, peer.chen@uli.com.tw");

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Add suspend/resume support to the uli526x network driver (tested on x86_64,
> with 'Ethernet controller: ALi Corporation M5263 Ethernet Controller, rev 40').
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Looks ok to me.

> +#ifdef CONFIG_PM
> +
> +/*
> + *	Suspend the interface.
> + */
> +
> +static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	struct net_device *dev = pci_get_drvdata(pdev);
> +
> +	ULI526X_DBUG(0, "uli526x_suspend", 0);
> +
> +	if (dev && netdev_priv(dev)) {
> +		if (netif_running(dev)) {
> +			netif_device_detach(dev);
> +			uli526x_down(dev);
> +		}
> +		pci_save_state(pdev);
> +		pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
> +		pci_disable_device(pdev);
> +		pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +	}
> +	return 0;
> +}
> +
> +/*
> + *	Resume the interface.
> + */
> +
> +static int uli526x_resume(struct pci_dev *pdev)
> +{
> +	struct net_device *dev = pci_get_drvdata(pdev);
> +	struct uli526x_board_info *db = netdev_priv(dev);
> +	int err;
> +
> +	ULI526X_DBUG(0, "uli526x_resume", 0);
> +
> +	if (dev && db) {
> +		pci_set_power_state(pdev, PCI_D0);
> +		err = pci_enable_device(pdev);
> +		if (err) {
> +			printk(KERN_WARNING "%s: Could not enable device \n",
> +				dev->name);
> +			return err;
> +		}
> +		pci_restore_state(pdev);
> +		pci_set_master(pdev);
> +		if (netif_running(dev)) {
> +			uli526x_up(dev);
> +			netif_device_attach(dev);
> +		}
> +	}
> +	return 0;
> +}
> +
#else 
#define *_resume NULL
#define *_suspend NULL

> +#endif /* CONFIG_PM */

> @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
>  	.id_table	= uli526x_pci_tbl,
>  	.probe		= uli526x_init_one,
>  	.remove		= __devexit_p(uli526x_remove_one),
> +#ifdef CONFIG_PM
> +	.suspend	= uli526x_suspend,
> +	.resume		= uli526x_resume,
> +#endif

...so that this ifdef is not needed?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Rafael J. Wysocki]

Hi,

On Monday, 4 June 2007 13:11, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Add suspend/resume support to the uli526x network driver (tested on x86_64,
> > with 'Ethernet controller: ALi Corporation M5263 Ethernet Controller, rev 40').
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Looks ok to me.
> 
> > +#ifdef CONFIG_PM
> > +
> > +/*
> > + *	Suspend the interface.
> > + */
> > +
> > +static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state)
> > +{
> > +	struct net_device *dev = pci_get_drvdata(pdev);
> > +
> > +	ULI526X_DBUG(0, "uli526x_suspend", 0);
> > +
> > +	if (dev && netdev_priv(dev)) {
> > +		if (netif_running(dev)) {
> > +			netif_device_detach(dev);
> > +			uli526x_down(dev);
> > +		}
> > +		pci_save_state(pdev);
> > +		pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
> > +		pci_disable_device(pdev);
> > +		pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > +	}
> > +	return 0;
> > +}
> > +
> > +/*
> > + *	Resume the interface.
> > + */
> > +
> > +static int uli526x_resume(struct pci_dev *pdev)
> > +{
> > +	struct net_device *dev = pci_get_drvdata(pdev);
> > +	struct uli526x_board_info *db = netdev_priv(dev);
> > +	int err;
> > +
> > +	ULI526X_DBUG(0, "uli526x_resume", 0);
> > +
> > +	if (dev && db) {
> > +		pci_set_power_state(pdev, PCI_D0);
> > +		err = pci_enable_device(pdev);
> > +		if (err) {
> > +			printk(KERN_WARNING "%s: Could not enable device \n",
> > +				dev->name);
> > +			return err;
> > +		}
> > +		pci_restore_state(pdev);
> > +		pci_set_master(pdev);
> > +		if (netif_running(dev)) {
> > +			uli526x_up(dev);
> > +			netif_device_attach(dev);
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> #else 
> #define *_resume NULL
> #define *_suspend NULL

> > +#endif /* CONFIG_PM */
> 
> > @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
> >  	.id_table	= uli526x_pci_tbl,
> >  	.probe		= uli526x_init_one,
> >  	.remove		= __devexit_p(uli526x_remove_one),
> > +#ifdef CONFIG_PM
> > +	.suspend	= uli526x_suspend,
> > +	.resume		= uli526x_resume,
> > +#endif
> 
> ...so that this ifdef is not needed?

OK, why not.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 2555 bytes --]

Hi.

On Mon, 2007-06-04 at 15:49 +0200, Rafael J. Wysocki wrote:
> On Monday, 4 June 2007 13:11, Pavel Machek wrote:
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Add suspend/resume support to the uli526x network driver (tested on x86_64,
> > > with 'Ethernet controller: ALi Corporation M5263 Ethernet Controller, rev 40').
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Looks ok to me.
> > 
> > > +#ifdef CONFIG_PM
> > > +
> > > +/*
> > > + *	Suspend the interface.
> > > + */
> > > +
> > > +static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state)
> > > +{
> > > +	struct net_device *dev = pci_get_drvdata(pdev);
> > > +
> > > +	ULI526X_DBUG(0, "uli526x_suspend", 0);
> > > +
> > > +	if (dev && netdev_priv(dev)) {
> > > +		if (netif_running(dev)) {
> > > +			netif_device_detach(dev);
> > > +			uli526x_down(dev);
> > > +		}
> > > +		pci_save_state(pdev);
> > > +		pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
> > > +		pci_disable_device(pdev);
> > > +		pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + *	Resume the interface.
> > > + */
> > > +
> > > +static int uli526x_resume(struct pci_dev *pdev)
> > > +{
> > > +	struct net_device *dev = pci_get_drvdata(pdev);
> > > +	struct uli526x_board_info *db = netdev_priv(dev);
> > > +	int err;
> > > +
> > > +	ULI526X_DBUG(0, "uli526x_resume", 0);
> > > +
> > > +	if (dev && db) {
> > > +		pci_set_power_state(pdev, PCI_D0);
> > > +		err = pci_enable_device(pdev);
> > > +		if (err) {
> > > +			printk(KERN_WARNING "%s: Could not enable device \n",
> > > +				dev->name);
> > > +			return err;
> > > +		}
> > > +		pci_restore_state(pdev);
> > > +		pci_set_master(pdev);
> > > +		if (netif_running(dev)) {
> > > +			uli526x_up(dev);
> > > +			netif_device_attach(dev);
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > #else 
> > #define *_resume NULL
> > #define *_suspend NULL
> 
> > > +#endif /* CONFIG_PM */
> > 
> > > @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
> > >  	.id_table	= uli526x_pci_tbl,
> > >  	.probe		= uli526x_init_one,
> > >  	.remove		= __devexit_p(uli526x_remove_one),
> > > +#ifdef CONFIG_PM
> > > +	.suspend	= uli526x_suspend,
> > > +	.resume		= uli526x_resume,
> > > +#endif
> > 
> > ...so that this ifdef is not needed?
> 
> OK, why not.

Because it's uglier and #ifdef is the established way of doing things?

Regards,

Nigel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Rafael J. Wysocki]

Hi,

On Monday, 4 June 2007 23:16, Nigel Cunningham wrote:
> Hi.
> 
> On Mon, 2007-06-04 at 15:49 +0200, Rafael J. Wysocki wrote:
> > On Monday, 4 June 2007 13:11, Pavel Machek wrote:
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > Add suspend/resume support to the uli526x network driver (tested on x86_64,
> > > > with 'Ethernet controller: ALi Corporation M5263 Ethernet Controller, rev 40').
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Looks ok to me.
> > > 
> > > > +#ifdef CONFIG_PM
> > > > +
> > > > +/*
> > > > + *	Suspend the interface.
> > > > + */
> > > > +
> > > > +static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state)
> > > > +{
> > > > +	struct net_device *dev = pci_get_drvdata(pdev);
> > > > +
> > > > +	ULI526X_DBUG(0, "uli526x_suspend", 0);
> > > > +
> > > > +	if (dev && netdev_priv(dev)) {
> > > > +		if (netif_running(dev)) {
> > > > +			netif_device_detach(dev);
> > > > +			uli526x_down(dev);
> > > > +		}
> > > > +		pci_save_state(pdev);
> > > > +		pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
> > > > +		pci_disable_device(pdev);
> > > > +		pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + *	Resume the interface.
> > > > + */
> > > > +
> > > > +static int uli526x_resume(struct pci_dev *pdev)
> > > > +{
> > > > +	struct net_device *dev = pci_get_drvdata(pdev);
> > > > +	struct uli526x_board_info *db = netdev_priv(dev);
> > > > +	int err;
> > > > +
> > > > +	ULI526X_DBUG(0, "uli526x_resume", 0);
> > > > +
> > > > +	if (dev && db) {
> > > > +		pci_set_power_state(pdev, PCI_D0);
> > > > +		err = pci_enable_device(pdev);
> > > > +		if (err) {
> > > > +			printk(KERN_WARNING "%s: Could not enable device \n",
> > > > +				dev->name);
> > > > +			return err;
> > > > +		}
> > > > +		pci_restore_state(pdev);
> > > > +		pci_set_master(pdev);
> > > > +		if (netif_running(dev)) {
> > > > +			uli526x_up(dev);
> > > > +			netif_device_attach(dev);
> > > > +		}
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > #else 
> > > #define *_resume NULL
> > > #define *_suspend NULL
> > 
> > > > +#endif /* CONFIG_PM */
> > > 
> > > > @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
> > > >  	.id_table	= uli526x_pci_tbl,
> > > >  	.probe		= uli526x_init_one,
> > > >  	.remove		= __devexit_p(uli526x_remove_one),
> > > > +#ifdef CONFIG_PM
> > > > +	.suspend	= uli526x_suspend,
> > > > +	.resume		= uli526x_resume,
> > > > +#endif
> > > 
> > > ...so that this ifdef is not needed?
> > 
> > OK, why not.
> 
> Because it's uglier and #ifdef is the established way of doing things?

Hmm, I have no strong opinion.  I slightly prefer the #ifdef, but well.

Perhaps I'll send it to Andrew 'as is' if no one else has any more comments. ;-)

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Pavel Machek]

Hi!

> > > > +#endif /* CONFIG_PM */
> > > 
> > > > @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
> > > >  	.id_table	= uli526x_pci_tbl,
> > > >  	.probe		= uli526x_init_one,
> > > >  	.remove		= __devexit_p(uli526x_remove_one),
> > > > +#ifdef CONFIG_PM
> > > > +	.suspend	= uli526x_suspend,
> > > > +	.resume		= uli526x_resume,
> > > > +#endif
> > > 
> > > ...so that this ifdef is not needed?
> > 
> > OK, why not.
> 
> Because it's uglier and #ifdef is the established way of doing things?

Actually the way I suggested is nicer, IIRC akpm invented it. It keeps
ifdefs localized around the block that _needs_ to be ifdefed.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

Hi.

On Tue, 2007-06-05 at 00:41 +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > +#endif /* CONFIG_PM */
> > > > 
> > > > > @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
> > > > >  	.id_table	= uli526x_pci_tbl,
> > > > >  	.probe		= uli526x_init_one,
> > > > >  	.remove		= __devexit_p(uli526x_remove_one),
> > > > > +#ifdef CONFIG_PM
> > > > > +	.suspend	= uli526x_suspend,
> > > > > +	.resume		= uli526x_resume,
> > > > > +#endif
> > > > 
> > > > ...so that this ifdef is not needed?
> > > 
> > > OK, why not.
> > 
> > Because it's uglier and #ifdef is the established way of doing things?
> 
> Actually the way I suggested is nicer, IIRC akpm invented it. It keeps
> ifdefs localized around the block that _needs_ to be ifdefed.

The localised point is true. I'll also admit that 'nicer'/'uglier' is a
matter of aesthetics and therefore personal opinion.

I guess that leaves the question, "What's the precedent to follow?" or
"Is there a driver that's already got this new format?".

Regards,

Nigel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Pavel Machek]

On Tue 2007-06-05 08:49:04, Nigel Cunningham wrote:
> Hi.
> 
> On Tue, 2007-06-05 at 00:41 +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > > > > +#endif /* CONFIG_PM */
> > > > > 
> > > > > > @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
> > > > > >  	.id_table	= uli526x_pci_tbl,
> > > > > >  	.probe		= uli526x_init_one,
> > > > > >  	.remove		= __devexit_p(uli526x_remove_one),
> > > > > > +#ifdef CONFIG_PM
> > > > > > +	.suspend	= uli526x_suspend,
> > > > > > +	.resume		= uli526x_resume,
> > > > > > +#endif
> > > > > 
> > > > > ...so that this ifdef is not needed?
> > > > 
> > > > OK, why not.
> > > 
> > > Because it's uglier and #ifdef is the established way of doing things?
> > 
> > Actually the way I suggested is nicer, IIRC akpm invented it. It keeps
> > ifdefs localized around the block that _needs_ to be ifdefed.
> 
> The localised point is true. I'll also admit that 'nicer'/'uglier' is a
> matter of aesthetics and therefore personal opinion.
> 
> I guess that leaves the question, "What's the precedent to follow?" or
> "Is there a driver that's already got this new format?".

for example net/ne.c ...

pavel@amd:/data/l/linux/drivers$ grep "_suspend NULL" */*.c
leds/leds-ams-delta.c:#define ams_delta_led_suspend NULL
leds/leds-net48xx.c:#define net48xx_led_suspend NULL
leds/leds-s3c24xx.c:#define s3c24xx_led_suspend NULL
leds/leds-tosa.c:#define tosaled_suspend NULL
leds/leds-wrap.c:#define wrap_led_suspend NULL
misc/tifm_7xx1.c:#define tifm_7xx1_suspend NULL
misc/tifm_core.c:#define tifm_device_suspend NULL
net/3c509.c:#define el3_suspend NULL
net/forcedeth.c:#define nv_suspend NULL
net/ne.c:#define ne_drv_suspend NULL
parport/parport_ax88796.c:#define parport_ax88796_suspend NULL
rtc/rtc-at91rm9200.c:#define at91_rtc_suspend NULL
rtc/rtc-omap.c:#define omap_rtc_suspend NULL
rtc/rtc-s3c.c:#define s3c_rtc_suspend NULL
serial/8250_pnp.c:#define serial_pnp_suspend NULL
serial/atmel_serial.c:#define atmel_serial_suspend NULL
serial/s3c2410.c:#define s3c24xx_serial_suspend NULL
spi/pxa2xx_spi.c:#define pxa2xx_spi_suspend NULL
spi/spi_bfin5xx.c:#define bfin5xx_spi_suspend NULL
spi/spi_imx.c:#define spi_imx_suspend NULL
spi/spi_s3c24xx.c:#define s3c24xx_spi_suspend NULL
spi/spi_s3c24xx_gpio.c:#define s3c2410_spigpio_suspend NULL
video/arkfb.c:#define ark_pci_suspend NULL
video/au1100fb.c:#define au1100fb_drv_suspend NULL
video/s3c2410fb.c:#define s3c2410fb_suspend NULL
video/skeletonfb.c:#define xxxfb_suspend NULL
video/skeletonfb.c:#define xxxfb_suspend NULL
video/sm501fb.c:#define sm501fb_suspend NULL

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]

Hi.

On Tue, 2007-06-05 at 00:53 +0200, Pavel Machek wrote:
> On Tue 2007-06-05 08:49:04, Nigel Cunningham wrote:
> > Hi.
> > 
> > On Tue, 2007-06-05 at 00:41 +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > > +#endif /* CONFIG_PM */
> > > > > > 
> > > > > > > @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
> > > > > > >  	.id_table	= uli526x_pci_tbl,
> > > > > > >  	.probe		= uli526x_init_one,
> > > > > > >  	.remove		= __devexit_p(uli526x_remove_one),
> > > > > > > +#ifdef CONFIG_PM
> > > > > > > +	.suspend	= uli526x_suspend,
> > > > > > > +	.resume		= uli526x_resume,
> > > > > > > +#endif
> > > > > > 
> > > > > > ...so that this ifdef is not needed?
> > > > > 
> > > > > OK, why not.
> > > > 
> > > > Because it's uglier and #ifdef is the established way of doing things?
> > > 
> > > Actually the way I suggested is nicer, IIRC akpm invented it. It keeps
> > > ifdefs localized around the block that _needs_ to be ifdefed.
> > 
> > The localised point is true. I'll also admit that 'nicer'/'uglier' is a
> > matter of aesthetics and therefore personal opinion.
> > 
> > I guess that leaves the question, "What's the precedent to follow?" or
> > "Is there a driver that's already got this new format?".
> 
> for example net/ne.c ...
> 
> pavel@amd:/data/l/linux/drivers$ grep "_suspend NULL" */*.c
> leds/leds-ams-delta.c:#define ams_delta_led_suspend NULL
> leds/leds-net48xx.c:#define net48xx_led_suspend NULL
> leds/leds-s3c24xx.c:#define s3c24xx_led_suspend NULL
> leds/leds-tosa.c:#define tosaled_suspend NULL
> leds/leds-wrap.c:#define wrap_led_suspend NULL
> misc/tifm_7xx1.c:#define tifm_7xx1_suspend NULL
> misc/tifm_core.c:#define tifm_device_suspend NULL
> net/3c509.c:#define el3_suspend NULL
> net/forcedeth.c:#define nv_suspend NULL
> net/ne.c:#define ne_drv_suspend NULL
> parport/parport_ax88796.c:#define parport_ax88796_suspend NULL
> rtc/rtc-at91rm9200.c:#define at91_rtc_suspend NULL
> rtc/rtc-omap.c:#define omap_rtc_suspend NULL
> rtc/rtc-s3c.c:#define s3c_rtc_suspend NULL
> serial/8250_pnp.c:#define serial_pnp_suspend NULL
> serial/atmel_serial.c:#define atmel_serial_suspend NULL
> serial/s3c2410.c:#define s3c24xx_serial_suspend NULL
> spi/pxa2xx_spi.c:#define pxa2xx_spi_suspend NULL
> spi/spi_bfin5xx.c:#define bfin5xx_spi_suspend NULL
> spi/spi_imx.c:#define spi_imx_suspend NULL
> spi/spi_s3c24xx.c:#define s3c24xx_spi_suspend NULL
> spi/spi_s3c24xx_gpio.c:#define s3c2410_spigpio_suspend NULL
> video/arkfb.c:#define ark_pci_suspend NULL
> video/au1100fb.c:#define au1100fb_drv_suspend NULL
> video/s3c2410fb.c:#define s3c2410fb_suspend NULL
> video/skeletonfb.c:#define xxxfb_suspend NULL
> video/skeletonfb.c:#define xxxfb_suspend NULL
> video/sm501fb.c:#define sm501fb_suspend NULL

Cool. That's a lot more than I would have expected. Ok. I'll drop my
objection then. (Still think it's ugly though :>).

Regards,

Nigel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Valerie Henson]

On Sun, Jun 03, 2007 at 12:37:35PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Add suspend/resume support to the uli526x network driver (tested on x86_64,
> with 'Ethernet controller: ALi Corporation M5263 Ethernet Controller, rev 40').
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Nice work!  Looks good to me.

Signed-off-by: Val Henson <val@nmt.edu>

-VAL

>  drivers/net/tulip/uli526x.c |  120 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 105 insertions(+), 15 deletions(-)
> 
> Index: linux-2.6.22-rc3/drivers/net/tulip/uli526x.c
> ===================================================================
> --- linux-2.6.22-rc3.orig/drivers/net/tulip/uli526x.c	2007-06-03 12:10:41.000000000 +0200
> +++ linux-2.6.22-rc3/drivers/net/tulip/uli526x.c	2007-06-03 12:14:33.000000000 +0200
> @@ -220,6 +220,10 @@ static int mode = 8;
>  static int uli526x_open(struct net_device *);
>  static int uli526x_start_xmit(struct sk_buff *, struct net_device *);
>  static int uli526x_stop(struct net_device *);
> +#ifdef CONFIG_PM
> +static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state);
> +static int uli526x_resume(struct pci_dev *pdev);
> +#endif
>  static struct net_device_stats * uli526x_get_stats(struct net_device *);
>  static void uli526x_set_filter_mode(struct net_device *);
>  static const struct ethtool_ops netdev_ethtool_ops;
> @@ -425,21 +429,14 @@ static void __devexit uli526x_remove_one
>  
>  
>  /*
> - *	Open the interface.
> - *	The interface is opened whenever "ifconfig" activates it.
> + *	Bring the interface up.
> + *	Used by uli526x_open and uli526x_resume.
>   */
>  
> -static int uli526x_open(struct net_device *dev)
> +static void uli526x_up(struct net_device *dev)
>  {
> -	int ret;
>  	struct uli526x_board_info *db = netdev_priv(dev);
>  
> -	ULI526X_DBUG(0, "uli526x_open", 0);
> -
> -	ret = request_irq(dev->irq, &uli526x_interrupt, IRQF_SHARED, dev->name, dev);
> -	if (ret)
> -		return ret;
> -
>  	/* system variable init */
>  	db->cr6_data = CR6_DEFAULT | uli526x_cr6_user_set;
>  	db->tx_packet_cnt = 0;
> @@ -467,7 +464,25 @@ static int uli526x_open(struct net_devic
>  	db->timer.data = (unsigned long)dev;
>  	db->timer.function = &uli526x_timer;
>  	add_timer(&db->timer);
> +}
> +
> +
> +/*
> + *	Open the interface.
> + *	The interface is opened whenever "ifconfig" activates it.
> + */
>  
> +static int uli526x_open(struct net_device *dev)
> +{
> +	int ret;
> +
> +	ULI526X_DBUG(0, "uli526x_open", 0);
> +
> +	ret = request_irq(dev->irq, &uli526x_interrupt, IRQF_SHARED, dev->name, dev);
> +	if (ret)
> +		return ret;
> +
> +	uli526x_up(dev);
>  	return 0;
>  }
>  
> @@ -613,17 +628,15 @@ static int uli526x_start_xmit(struct sk_
>  
>  
>  /*
> - *	Stop the interface.
> - *	The interface is stopped when it is brought.
> + *	Take the interface down.
> + *	Used by uli526x_stop and uli526x_suspend.
>   */
>  
> -static int uli526x_stop(struct net_device *dev)
> +static void uli526x_down(struct net_device *dev)
>  {
>  	struct uli526x_board_info *db = netdev_priv(dev);
>  	unsigned long ioaddr = dev->base_addr;
>  
> -	ULI526X_DBUG(0, "uli526x_stop", 0);
> -
>  	/* disable system */
>  	netif_stop_queue(dev);
>  
> @@ -634,6 +647,21 @@ static int uli526x_stop(struct net_devic
>  	outl(ULI526X_RESET, ioaddr + DCR0);
>  	udelay(5);
>  	phy_write(db->ioaddr, db->phy_addr, 0, 0x8000, db->chip_id);
> +}
> +
> +
> +/*
> + *	Stop the interface.
> + *	The interface is stopped when it is brought.
> + */
> +
> +static int uli526x_stop(struct net_device *dev)
> +{
> +	struct uli526x_board_info *db = netdev_priv(dev);
> +
> +	ULI526X_DBUG(0, "uli526x_stop", 0);
> +
> +	uli526x_down(dev);
>  
>  	/* free interrupt */
>  	free_irq(dev->irq, dev);
> @@ -654,6 +682,64 @@ static int uli526x_stop(struct net_devic
>  }
>  
>  
> +#ifdef CONFIG_PM
> +
> +/*
> + *	Suspend the interface.
> + */
> +
> +static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	struct net_device *dev = pci_get_drvdata(pdev);
> +
> +	ULI526X_DBUG(0, "uli526x_suspend", 0);
> +
> +	if (dev && netdev_priv(dev)) {
> +		if (netif_running(dev)) {
> +			netif_device_detach(dev);
> +			uli526x_down(dev);
> +		}
> +		pci_save_state(pdev);
> +		pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
> +		pci_disable_device(pdev);
> +		pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +	}
> +	return 0;
> +}
> +
> +/*
> + *	Resume the interface.
> + */
> +
> +static int uli526x_resume(struct pci_dev *pdev)
> +{
> +	struct net_device *dev = pci_get_drvdata(pdev);
> +	struct uli526x_board_info *db = netdev_priv(dev);
> +	int err;
> +
> +	ULI526X_DBUG(0, "uli526x_resume", 0);
> +
> +	if (dev && db) {
> +		pci_set_power_state(pdev, PCI_D0);
> +		err = pci_enable_device(pdev);
> +		if (err) {
> +			printk(KERN_WARNING "%s: Could not enable device \n",
> +				dev->name);
> +			return err;
> +		}
> +		pci_restore_state(pdev);
> +		pci_set_master(pdev);
> +		if (netif_running(dev)) {
> +			uli526x_up(dev);
> +			netif_device_attach(dev);
> +		}
> +	}
> +	return 0;
> +}
> +
> +#endif /* CONFIG_PM */
> +
> +
>  /*
>   *	M5261/M5263 insterrupt handler
>   *	receive the packet to upper layer, free the transmitted packet
> @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
>  	.id_table	= uli526x_pci_tbl,
>  	.probe		= uli526x_init_one,
>  	.remove		= __devexit_p(uli526x_remove_one),
> +#ifdef CONFIG_PM
> +	.suspend	= uli526x_suspend,
> +	.resume		= uli526x_resume,
> +#endif
>  };
>  
>  MODULE_AUTHOR("Peer Chen, peer.chen@uli.com.tw");

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Stephen Hemminger]

I hope soon to add suspend/resume to the network device class
and remove driver specific suspend/resume from lots of devices.

The class suspend routine would just be:
	pci_save_state
	dev->stop

resume is
	pci_restore_state
	dev->open

for many devices that is all they need.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [RFC][PATCH] Add suspend and resume support to uli526x

[Rafael J. Wysocki]

On Tuesday, 5 June 2007 07:56, Stephen Hemminger wrote:
> I hope soon to add suspend/resume to the network device class
> and remove driver specific suspend/resume from lots of devices.
> 
> The class suspend routine would just be:
> 	pci_save_state
> 	dev->stop
> 
> resume is
> 	pci_restore_state
> 	dev->open
> 
> for many devices that is all they need.

Well, that would be nice, but does it mean there's no need for the $subject
patch?

Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth