Patchwork [5/5] smsc95xx: enable power saving mode during system suspend

login
register
mail settings
Submitter Steve Glendinning
Date Sept. 26, 2012, 12:40 p.m.
Message ID <1348663224-30403-6-git-send-email-steve.glendinning@shawell.net>
Download mbox | patch
Permalink /patch/187070/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Steve Glendinning - Sept. 26, 2012, 12:40 p.m.
This patch enables the device to enter its lowest power SUSPEND2
state during system suspend, instead of staying up using full power.

Patch updated to not add two pointers to .suspend & .resume.

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc95xx.c |   27 ++++++++++++++++++++++++++-
 drivers/net/usb/smsc95xx.h |    6 +++++-
 2 files changed, 31 insertions(+), 2 deletions(-)
Bjørn Mork - Sept. 26, 2012, 2:23 p.m.
Steve Glendinning <steve.glendinning@shawell.net> writes:

> +static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct usbnet *dev = usb_get_intfdata(intf);
> +	int ret;
> +	u32 val;
> +
> +	BUG_ON(!dev);

That's not very user friendly.  Why not just return here?



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Glendinning - Sept. 26, 2012, 3:16 p.m.
On 26 September 2012 15:23, Bjørn Mork <bjorn@mork.no> wrote:
> Steve Glendinning <steve.glendinning@shawell.net> writes:
>
>> +static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
>> +{
>> +     struct usbnet *dev = usb_get_intfdata(intf);
>> +     int ret;
>> +     u32 val;
>> +
>> +     BUG_ON(!dev);
>
> That's not very user friendly.  Why not just return here?

I hadn't thought that was a situation that could arise, is it?  Would
this happen if the USB device was removed during suspend?

-Steve
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork - Sept. 26, 2012, 3:48 p.m.
Steve Glendinning <steve@shawell.net> writes:
> On 26 September 2012 15:23, Bjørn Mork <bjorn@mork.no> wrote:
>> Steve Glendinning <steve.glendinning@shawell.net> writes:
>>
>>> +static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
>>> +{
>>> +     struct usbnet *dev = usb_get_intfdata(intf);
>>> +     int ret;
>>> +     u32 val;
>>> +
>>> +     BUG_ON(!dev);
>>
>> That's not very user friendly.  Why not just return here?
>
> I hadn't thought that was a situation that could arise, is it?  Would
> this happen if the USB device was removed during suspend?

No, it should not happen.  But then, why test at all?


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Glendinning - Sept. 26, 2012, 3:58 p.m.
>> I hadn't thought that was a situation that could arise, is it?  Would
>> this happen if the USB device was removed during suspend?
>
> No, it should not happen.  But then, why test at all?

I thought it was common practice to add these tests to document an
assumption the developer made that later code relies on?  I had
assumed that the !dev condition should not be possible, hence the
simple BUG test.  If it is possible then I agree - I definitely need
to handle this more gracefully.

In this case, asserting that dev is not NULL will make the code fail
loudly there instead of a few lines down when the netdev_info call
dereferences dev->net.  Either way something bad will happen!

-Steve
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork - Sept. 26, 2012, 4:17 p.m.
Steve Glendinning <steve@shawell.net> writes:

>>> I hadn't thought that was a situation that could arise, is it?  Would
>>> this happen if the USB device was removed during suspend?
>>
>> No, it should not happen.  But then, why test at all?
>
> I thought it was common practice to add these tests to document an
> assumption the developer made that later code relies on?  I had
> assumed that the !dev condition should not be possible, hence the
> simple BUG test.  If it is possible then I agree - I definitely need
> to handle this more gracefully.
>
> In this case, asserting that dev is not NULL will make the code fail
> loudly there instead of a few lines down when the netdev_info call
> dereferences dev->net.  Either way something bad will happen!

Yes, but you are a lot less likely to know about it if you BUG out.  The
user will be left with no other choice than hitting reset or poweroff.
What's the point of that?

If your driver crashes but the machine is left running, then the user
may forward the Oops to you.  That's much more useful.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Glendinning - Sept. 27, 2012, 8:04 a.m.
On 26 September 2012 17:17, Bjørn Mork <bjorn@mork.no> wrote:
> Yes, but you are a lot less likely to know about it if you BUG out.  The
> user will be left with no other choice than hitting reset or poweroff.
> What's the point of that?
>
> If your driver crashes but the machine is left running, then the user
> may forward the Oops to you.  That's much more useful.

Good point, I hadn't considered that.

So for user reportability am I better off to use WARN_ON in this case,
or simply remove the check and let the null pointer dereference
happen?

-Steve
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork - Sept. 27, 2012, 5:14 p.m.
Steve Glendinning <steve@shawell.net> writes:
> On 26 September 2012 17:17, Bjørn Mork <bjorn@mork.no> wrote:
>> Yes, but you are a lot less likely to know about it if you BUG out.  The
>> user will be left with no other choice than hitting reset or poweroff.
>> What's the point of that?
>>
>> If your driver crashes but the machine is left running, then the user
>> may forward the Oops to you.  That's much more useful.
>
> Good point, I hadn't considered that.
>
> So for user reportability am I better off to use WARN_ON in this case,
> or simply remove the check and let the null pointer dereference
> happen?

Exactly.  See also http://permalink.gmane.org/gmane.linux.kernel/1365689


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index f29860d..6c89a08 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1018,6 +1018,31 @@  static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
 	}
 }
 
+static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct usbnet *dev = usb_get_intfdata(intf);
+	int ret;
+	u32 val;
+
+	BUG_ON(!dev);
+
+	ret = usbnet_suspend(intf, message);
+	check_warn_return(ret, "usbnet_suspend error");
+
+	netdev_info(dev->net, "entering SUSPEND2 mode");
+
+	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
+	check_warn_return(ret, "Error reading PM_CTRL");
+
+	val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
+	val |= PM_CTL_SUS_MODE_2;
+
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
+	check_warn_return(ret, "Error writing PM_CTRL");
+
+	return 0;
+}
+
 static void smsc95xx_rx_csum_offload(struct sk_buff *skb)
 {
 	skb->csum = *(u16 *)(skb_tail_pointer(skb) - 2);
@@ -1280,7 +1305,7 @@  static struct usb_driver smsc95xx_driver = {
 	.name		= "smsc95xx",
 	.id_table	= products,
 	.probe		= usbnet_probe,
-	.suspend	= usbnet_suspend,
+	.suspend	= smsc95xx_suspend,
 	.resume		= usbnet_resume,
 	.reset_resume	= usbnet_resume,
 	.disconnect	= usbnet_disconnect,
diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h
index a275b62..89ad925 100644
--- a/drivers/net/usb/smsc95xx.h
+++ b/drivers/net/usb/smsc95xx.h
@@ -84,12 +84,16 @@ 
 #define HW_CFG_BCE_			(0x00000002)
 #define HW_CFG_SRST_			(0x00000001)
 
+#define RX_FIFO_INF			(0x18)
+
 #define PM_CTRL				(0x20)
+#define PM_CTL_RES_CLR_WKP_STS		(0x00000200)
 #define PM_CTL_DEV_RDY_			(0x00000080)
 #define PM_CTL_SUS_MODE_		(0x00000060)
 #define PM_CTL_SUS_MODE_0		(0x00000000)
 #define PM_CTL_SUS_MODE_1		(0x00000020)
-#define PM_CTL_SUS_MODE_2		(0x00000060)
+#define PM_CTL_SUS_MODE_2		(0x00000040)
+#define PM_CTL_SUS_MODE_3		(0x00000060)
 #define PM_CTL_PHY_RST_			(0x00000010)
 #define PM_CTL_WOL_EN_			(0x00000008)
 #define PM_CTL_ED_EN_			(0x00000004)