* [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend
@ 2012-12-11 15:26 Steve Glendinning
[not found] ` <1355239561-2740-1-git-send-email-steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org>
2012-12-11 16:27 ` Joe Perches
0 siblings, 2 replies; 7+ messages in thread
From: Steve Glendinning @ 2012-12-11 15:26 UTC (permalink / raw)
To: netdev; +Cc: linux-usb, ming.lei, oneukum, gregkh, Steve Glendinning
This patch enables USB dynamic autosuspend for LAN9500A. This
saves very little power in itself, but it allows power saving
in upstream hubs/hosts.
The earlier devices in this family (LAN9500/9512/9514) do not
support this feature.
Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
drivers/net/usb/smsc95xx.c | 159 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 158 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 9b73670..bdd51fd 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -55,6 +55,13 @@
#define FEATURE_PHY_NLP_CROSSOVER (0x02)
#define FEATURE_AUTOSUSPEND (0x04)
+#define SUSPEND_SUSPEND0 (0x01)
+#define SUSPEND_SUSPEND1 (0x02)
+#define SUSPEND_SUSPEND2 (0x04)
+#define SUSPEND_SUSPEND3 (0x08)
+#define SUSPEND_ALLMODES (SUSPEND_SUSPEND0 | SUSPEND_SUSPEND1 | \
+ SUSPEND_SUSPEND2 | SUSPEND_SUSPEND3)
+
struct smsc95xx_priv {
u32 mac_cr;
u32 hash_hi;
@@ -62,6 +69,7 @@ struct smsc95xx_priv {
u32 wolopts;
spinlock_t mac_cr_lock;
u8 features;
+ u8 suspend_flags;
};
static bool turbo_mode = true;
@@ -1341,6 +1349,8 @@ static int smsc95xx_enter_suspend0(struct usbnet *dev)
if (ret < 0)
netdev_warn(dev->net, "Error reading PM_CTRL\n");
+ pdata->suspend_flags |= SUSPEND_SUSPEND0;
+
return ret;
}
@@ -1393,11 +1403,14 @@ static int smsc95xx_enter_suspend1(struct usbnet *dev)
if (ret < 0)
netdev_warn(dev->net, "Error writing PM_CTRL\n");
+ pdata->suspend_flags |= SUSPEND_SUSPEND1;
+
return ret;
}
static int smsc95xx_enter_suspend2(struct usbnet *dev)
{
+ struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
u32 val;
int ret;
@@ -1414,9 +1427,105 @@ static int smsc95xx_enter_suspend2(struct usbnet *dev)
if (ret < 0)
netdev_warn(dev->net, "Error writing PM_CTRL\n");
+ pdata->suspend_flags |= SUSPEND_SUSPEND2;
+
return ret;
}
+static int smsc95xx_enter_suspend3(struct usbnet *dev)
+{
+ struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
+ u32 val;
+ int ret;
+
+ ret = smsc95xx_read_reg_nopm(dev, RX_FIFO_INF, &val);
+ if (ret < 0) {
+ netdev_warn(dev->net, "Error reading RX_FIFO_INF");
+ return ret;
+ }
+
+ if (val & 0xFFFF) {
+ netdev_info(dev->net, "rx fifo not empty in autosuspend");
+ return -EBUSY;
+ }
+
+ ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
+ if (ret < 0) {
+ netdev_warn(dev->net, "Error reading PM_CTRL");
+ return ret;
+ }
+
+ val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
+ val |= PM_CTL_SUS_MODE_3 | PM_CTL_RES_CLR_WKP_STS;
+
+ ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+ if (ret < 0) {
+ netdev_warn(dev->net, "Error writing PM_CTRL");
+ return ret;
+ }
+
+ /* clear wol status */
+ val &= ~PM_CTL_WUPS_;
+ val |= PM_CTL_WUPS_WOL_;
+
+ ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
+ if (ret < 0) {
+ netdev_warn(dev->net, "Error writing PM_CTRL");
+ return ret;
+ }
+
+ pdata->suspend_flags |= SUSPEND_SUSPEND3;
+
+ return 0;
+}
+
+static int smsc95xx_autosuspend(struct usbnet *dev, u32 link_up)
+{
+ struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
+ int ret;
+
+ if (!netif_running(dev->net)) {
+ /* interface is ifconfig down so fully power down hw */
+ netdev_dbg(dev->net, "autosuspend entering SUSPEND2");
+ return smsc95xx_enter_suspend2(dev);
+ }
+
+ if (!link_up) {
+ /* link is down so enter EDPD mode, but only if device can
+ * reliably resume from it. This check should be redundant
+ * as current FEATURE_AUTOSUSPEND parts also support
+ * FEATURE_PHY_NLP_CROSSOVER but it's included for clarity */
+ if (!(pdata->features & FEATURE_PHY_NLP_CROSSOVER)) {
+ netdev_warn(dev->net, "EDPD not supported");
+ return -EBUSY;
+ }
+
+ netdev_dbg(dev->net, "autosuspend entering SUSPEND1");
+
+ /* enable PHY wakeup events for if cable is attached */
+ ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
+ PHY_INT_MASK_ANEG_COMP_);
+ if (ret < 0) {
+ netdev_warn(dev->net, "error enabling PHY wakeup ints");
+ return ret;
+ }
+
+ netdev_info(dev->net, "entering SUSPEND1 mode");
+ return smsc95xx_enter_suspend1(dev);
+ }
+
+ /* enable PHY wakeup events so we remote wakeup if cable is pulled */
+ ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
+ PHY_INT_MASK_LINK_DOWN_);
+ if (ret < 0) {
+ netdev_warn(dev->net, "error enabling PHY wakeup ints");
+ return ret;
+ }
+
+ netdev_dbg(dev->net, "autosuspend entering SUSPEND3");
+ return smsc95xx_enter_suspend3(dev);
+}
+
static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
{
struct usbnet *dev = usb_get_intfdata(intf);
@@ -1424,15 +1533,35 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
u32 val, link_up;
int ret;
+ /* TODO: don't indicate this feature to usb framework if
+ * our current hardware doesn't have the capability
+ */
+ if ((message.event == PM_EVENT_AUTO_SUSPEND) &&
+ (!(pdata->features & FEATURE_AUTOSUSPEND))) {
+ netdev_warn(dev->net, "autosuspend not supported");
+ return -EBUSY;
+ }
+
ret = usbnet_suspend(intf, message);
if (ret < 0) {
netdev_warn(dev->net, "usbnet_suspend error\n");
return ret;
}
+ if (pdata->suspend_flags) {
+ netdev_warn(dev->net, "error during last resume");
+ pdata->suspend_flags = 0;
+ }
+
/* determine if link is up using only _nopm functions */
link_up = smsc95xx_link_ok_nopm(dev);
+ if (message.event == PM_EVENT_AUTO_SUSPEND) {
+ ret = smsc95xx_autosuspend(dev, link_up);
+ goto done;
+ }
+
+ /* if we get this far we're not autosuspending */
/* if no wol options set, or if link is down and we're not waking on
* PHY activity, enter lowest power SUSPEND2 mode
*/
@@ -1694,12 +1823,18 @@ static int smsc95xx_resume(struct usb_interface *intf)
{
struct usbnet *dev = usb_get_intfdata(intf);
struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
+ u8 suspend_flags = pdata->suspend_flags;
int ret;
u32 val;
BUG_ON(!dev);
- if (pdata->wolopts) {
+ netdev_dbg(dev->net, "resume suspend_flags=0x%02x", suspend_flags);
+
+ /* do this first to ensure it's cleared even in error case */
+ pdata->suspend_flags = 0;
+
+ if (suspend_flags & SUSPEND_ALLMODES) {
/* clear wake-up sources */
ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
if (ret < 0) {
@@ -1891,6 +2026,26 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
return skb;
}
+static int smsc95xx_manage_power(struct usbnet *dev, int on)
+{
+ struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
+
+ dev->intf->needs_remote_wakeup = on;
+
+ if (pdata->features & FEATURE_AUTOSUSPEND)
+ return 0;
+
+ /* this chip revision doesn't support autosuspend */
+ netdev_info(dev->net, "hardware doesn't support USB autosuspend\n");
+
+ if (on)
+ usb_autopm_get_interface_no_resume(dev->intf);
+ else
+ usb_autopm_put_interface_no_suspend(dev->intf);
+
+ return 0;
+}
+
static const struct driver_info smsc95xx_info = {
.description = "smsc95xx USB 2.0 Ethernet",
.bind = smsc95xx_bind,
@@ -1900,6 +2055,7 @@ static const struct driver_info smsc95xx_info = {
.rx_fixup = smsc95xx_rx_fixup,
.tx_fixup = smsc95xx_tx_fixup,
.status = smsc95xx_status,
+ .manage_power = smsc95xx_manage_power,
.flags = FLAG_ETHER | FLAG_SEND_ZLP | FLAG_LINK_INTR,
};
@@ -2007,6 +2163,7 @@ static struct usb_driver smsc95xx_driver = {
.reset_resume = smsc95xx_resume,
.disconnect = usbnet_disconnect,
.disable_hub_initiated_lpm = 1,
+ .supports_autosuspend = 1,
};
module_usb_driver(smsc95xx_driver);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend
[not found] ` <1355239561-2740-1-git-send-email-steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org>
@ 2012-12-11 16:13 ` Ming Lei
2012-12-14 13:35 ` Steve Glendinning
0 siblings, 1 reply; 7+ messages in thread
From: Ming Lei @ 2012-12-11 16:13 UTC (permalink / raw)
To: Steve Glendinning
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
oneukum-l3A5Bk7waGM, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
On Tue, Dec 11, 2012 at 11:26 PM, Steve Glendinning
<steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org> wrote:
> This patch enables USB dynamic autosuspend for LAN9500A. This
> saves very little power in itself, but it allows power saving
> in upstream hubs/hosts.
>
> The earlier devices in this family (LAN9500/9512/9514) do not
> support this feature.
>
> Signed-off-by: Steve Glendinning <steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org>
> ---
> drivers/net/usb/smsc95xx.c | 159 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 158 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 9b73670..bdd51fd 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -55,6 +55,13 @@
> #define FEATURE_PHY_NLP_CROSSOVER (0x02)
> #define FEATURE_AUTOSUSPEND (0x04)
>
> +#define SUSPEND_SUSPEND0 (0x01)
> +#define SUSPEND_SUSPEND1 (0x02)
> +#define SUSPEND_SUSPEND2 (0x04)
> +#define SUSPEND_SUSPEND3 (0x08)
> +#define SUSPEND_ALLMODES (SUSPEND_SUSPEND0 | SUSPEND_SUSPEND1 | \
> + SUSPEND_SUSPEND2 | SUSPEND_SUSPEND3)
> +
> struct smsc95xx_priv {
> u32 mac_cr;
> u32 hash_hi;
> @@ -62,6 +69,7 @@ struct smsc95xx_priv {
> u32 wolopts;
> spinlock_t mac_cr_lock;
> u8 features;
> + u8 suspend_flags;
> };
>
> static bool turbo_mode = true;
> @@ -1341,6 +1349,8 @@ static int smsc95xx_enter_suspend0(struct usbnet *dev)
> if (ret < 0)
> netdev_warn(dev->net, "Error reading PM_CTRL\n");
>
> + pdata->suspend_flags |= SUSPEND_SUSPEND0;
> +
> return ret;
> }
>
> @@ -1393,11 +1403,14 @@ static int smsc95xx_enter_suspend1(struct usbnet *dev)
> if (ret < 0)
> netdev_warn(dev->net, "Error writing PM_CTRL\n");
>
> + pdata->suspend_flags |= SUSPEND_SUSPEND1;
> +
> return ret;
> }
>
> static int smsc95xx_enter_suspend2(struct usbnet *dev)
> {
> + struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
> u32 val;
> int ret;
>
> @@ -1414,9 +1427,105 @@ static int smsc95xx_enter_suspend2(struct usbnet *dev)
> if (ret < 0)
> netdev_warn(dev->net, "Error writing PM_CTRL\n");
>
> + pdata->suspend_flags |= SUSPEND_SUSPEND2;
> +
> return ret;
> }
>
> +static int smsc95xx_enter_suspend3(struct usbnet *dev)
> +{
> + struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
> + u32 val;
> + int ret;
> +
> + ret = smsc95xx_read_reg_nopm(dev, RX_FIFO_INF, &val);
> + if (ret < 0) {
> + netdev_warn(dev->net, "Error reading RX_FIFO_INF");
> + return ret;
> + }
> +
> + if (val & 0xFFFF) {
> + netdev_info(dev->net, "rx fifo not empty in autosuspend");
> + return -EBUSY;
> + }
> +
> + ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
> + if (ret < 0) {
> + netdev_warn(dev->net, "Error reading PM_CTRL");
> + return ret;
> + }
> +
> + val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
> + val |= PM_CTL_SUS_MODE_3 | PM_CTL_RES_CLR_WKP_STS;
> +
> + ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
> + if (ret < 0) {
> + netdev_warn(dev->net, "Error writing PM_CTRL");
> + return ret;
> + }
> +
> + /* clear wol status */
> + val &= ~PM_CTL_WUPS_;
> + val |= PM_CTL_WUPS_WOL_;
> +
> + ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
> + if (ret < 0) {
> + netdev_warn(dev->net, "Error writing PM_CTRL");
> + return ret;
> + }
> +
> + pdata->suspend_flags |= SUSPEND_SUSPEND3;
> +
> + return 0;
> +}
> +
> +static int smsc95xx_autosuspend(struct usbnet *dev, u32 link_up)
> +{
> + struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
> + int ret;
> +
> + if (!netif_running(dev->net)) {
> + /* interface is ifconfig down so fully power down hw */
> + netdev_dbg(dev->net, "autosuspend entering SUSPEND2");
> + return smsc95xx_enter_suspend2(dev);
> + }
> +
> + if (!link_up) {
> + /* link is down so enter EDPD mode, but only if device can
> + * reliably resume from it. This check should be redundant
> + * as current FEATURE_AUTOSUSPEND parts also support
> + * FEATURE_PHY_NLP_CROSSOVER but it's included for clarity */
> + if (!(pdata->features & FEATURE_PHY_NLP_CROSSOVER)) {
> + netdev_warn(dev->net, "EDPD not supported");
> + return -EBUSY;
> + }
> +
> + netdev_dbg(dev->net, "autosuspend entering SUSPEND1");
> +
> + /* enable PHY wakeup events for if cable is attached */
> + ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
> + PHY_INT_MASK_ANEG_COMP_);
> + if (ret < 0) {
> + netdev_warn(dev->net, "error enabling PHY wakeup ints");
> + return ret;
> + }
> +
> + netdev_info(dev->net, "entering SUSPEND1 mode");
> + return smsc95xx_enter_suspend1(dev);
> + }
> +
> + /* enable PHY wakeup events so we remote wakeup if cable is pulled */
> + ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
> + PHY_INT_MASK_LINK_DOWN_);
> + if (ret < 0) {
> + netdev_warn(dev->net, "error enabling PHY wakeup ints");
> + return ret;
> + }
> +
> + netdev_dbg(dev->net, "autosuspend entering SUSPEND3");
> + return smsc95xx_enter_suspend3(dev);
> +}
> +
> static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
> {
> struct usbnet *dev = usb_get_intfdata(intf);
> @@ -1424,15 +1533,35 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
> u32 val, link_up;
> int ret;
>
> + /* TODO: don't indicate this feature to usb framework if
> + * our current hardware doesn't have the capability
> + */
> + if ((message.event == PM_EVENT_AUTO_SUSPEND) &&
> + (!(pdata->features & FEATURE_AUTOSUSPEND))) {
> + netdev_warn(dev->net, "autosuspend not supported");
> + return -EBUSY;
> + }
> +
> ret = usbnet_suspend(intf, message);
> if (ret < 0) {
> netdev_warn(dev->net, "usbnet_suspend error\n");
> return ret;
> }
>
> + if (pdata->suspend_flags) {
> + netdev_warn(dev->net, "error during last resume");
> + pdata->suspend_flags = 0;
> + }
> +
> /* determine if link is up using only _nopm functions */
> link_up = smsc95xx_link_ok_nopm(dev);
>
> + if (message.event == PM_EVENT_AUTO_SUSPEND) {
> + ret = smsc95xx_autosuspend(dev, link_up);
> + goto done;
> + }
> +
> + /* if we get this far we're not autosuspending */
> /* if no wol options set, or if link is down and we're not waking on
> * PHY activity, enter lowest power SUSPEND2 mode
> */
> @@ -1694,12 +1823,18 @@ static int smsc95xx_resume(struct usb_interface *intf)
> {
> struct usbnet *dev = usb_get_intfdata(intf);
> struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
> + u8 suspend_flags = pdata->suspend_flags;
> int ret;
> u32 val;
>
> BUG_ON(!dev);
>
> - if (pdata->wolopts) {
> + netdev_dbg(dev->net, "resume suspend_flags=0x%02x", suspend_flags);
> +
> + /* do this first to ensure it's cleared even in error case */
> + pdata->suspend_flags = 0;
> +
> + if (suspend_flags & SUSPEND_ALLMODES) {
> /* clear wake-up sources */
> ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
> if (ret < 0) {
> @@ -1891,6 +2026,26 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> return skb;
> }
>
> +static int smsc95xx_manage_power(struct usbnet *dev, int on)
> +{
> + struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
> +
> + dev->intf->needs_remote_wakeup = on;
> +
> + if (pdata->features & FEATURE_AUTOSUSPEND)
> + return 0;
> +
> + /* this chip revision doesn't support autosuspend */
> + netdev_info(dev->net, "hardware doesn't support USB autosuspend\n");
> +
> + if (on)
> + usb_autopm_get_interface_no_resume(dev->intf);
> + else
> + usb_autopm_put_interface_no_suspend(dev->intf);
The above line should be
usb_autopm_put_interface(dev->intf);
> +
> + return 0;
> +}
IMO, it is better to keep smsc95xx_info.manage_power as NULL
for devices without FEATURE_AUTOSUSPEND, so that fewer code
and follow the current .mange_power usage(see its comment).
Currently, if the function pointer of manage_power is set, it means that
the device supports USB autosuspend, but your trick will make the
assumption not true for some smsc devices.
> +
> static const struct driver_info smsc95xx_info = {
> .description = "smsc95xx USB 2.0 Ethernet",
> .bind = smsc95xx_bind,
> @@ -1900,6 +2055,7 @@ static const struct driver_info smsc95xx_info = {
> .rx_fixup = smsc95xx_rx_fixup,
> .tx_fixup = smsc95xx_tx_fixup,
> .status = smsc95xx_status,
> + .manage_power = smsc95xx_manage_power,
> .flags = FLAG_ETHER | FLAG_SEND_ZLP | FLAG_LINK_INTR,
> };
>
> @@ -2007,6 +2163,7 @@ static struct usb_driver smsc95xx_driver = {
> .reset_resume = smsc95xx_resume,
> .disconnect = usbnet_disconnect,
> .disable_hub_initiated_lpm = 1,
> + .supports_autosuspend = 1,
> };
>
> module_usb_driver(smsc95xx_driver);
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend
2012-12-11 15:26 [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend Steve Glendinning
[not found] ` <1355239561-2740-1-git-send-email-steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org>
@ 2012-12-11 16:27 ` Joe Perches
2012-12-14 13:38 ` Steve Glendinning
1 sibling, 1 reply; 7+ messages in thread
From: Joe Perches @ 2012-12-11 16:27 UTC (permalink / raw)
To: Steve Glendinning; +Cc: netdev, linux-usb, ming.lei, oneukum, gregkh
On Tue, 2012-12-11 at 15:26 +0000, Steve Glendinning wrote:
> This patch enables USB dynamic autosuspend for LAN9500A. This
> saves very little power in itself, but it allows power saving
> in upstream hubs/hosts.
[]> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
[]
> + ret = smsc95xx_read_reg_nopm(dev, RX_FIFO_INF, &val);
> + if (ret < 0) {
> + netdev_warn(dev->net, "Error reading RX_FIFO_INF");
Please remember terminating newlines.
If you are always going to warn bad reads or writes,
it'd be good to change smsc95xx_read_reg_nopm
and write to emit the message.
It saves ~3% of code, 1K
$ size drivers/net/usb/smsc95xx.o*
text data bss dec hex filename
27064 2724 6072 35860 8c14 drivers/net/usb/smsc95xx.o.new
27921 2724 6256 36901 9025 drivers/net/usb/smsc95xx.o.old
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/usb/smsc95xx.c | 126 +++++++++++++++-----------------------------
1 files changed, 42 insertions(+), 84 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 9b73670..e86eca7 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -125,13 +125,28 @@ static int __must_check __smsc95xx_write_reg(struct usbnet *dev, u32 index,
static int __must_check smsc95xx_read_reg_nopm(struct usbnet *dev, u32 index,
u32 *data)
{
- return __smsc95xx_read_reg(dev, index, data, 1);
+ int rtn = __smsc95xx_read_reg(dev, index, data, 1);
+ if (rtn < 0)
+ netdev_warn(dev->net, "Error reading %#x:%s\n",
+ index,
+ index == PM_CTRL ? "PM_CTRL" :
+ index == WUCSR ? "WUCSR" :
+ "unknown");
+ return rtn;
}
static int __must_check smsc95xx_write_reg_nopm(struct usbnet *dev, u32 index,
u32 data)
{
- return __smsc95xx_write_reg(dev, index, data, 1);
+ int rtn = __smsc95xx_write_reg(dev, index, data, 1);
+ if (rtn < 0)
+ netdev_warn(dev->net, "Error writing %#x:%s\n",
+ index,
+ index == PM_CTRL ? "PM_CTRL" :
+ index == WUCSR ? "WUCSR" :
+ index == WUFF ? "WUFF" :
+ "unknown");
+ return rtn;
}
static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
@@ -1308,19 +1323,15 @@ static int smsc95xx_enter_suspend0(struct usbnet *dev)
int ret;
ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading PM_CTRL\n");
+ if (ret < 0)
return ret;
- }
val &= (~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_));
val |= PM_CTL_SUS_MODE_0;
ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing PM_CTRL\n");
+ if (ret < 0)
return ret;
- }
/* clear wol status */
val &= ~PM_CTL_WUPS_;
@@ -1331,15 +1342,11 @@ static int smsc95xx_enter_suspend0(struct usbnet *dev)
val |= PM_CTL_WUPS_ED_;
ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing PM_CTRL\n");
+ if (ret < 0)
return ret;
- }
/* read back PM_CTRL */
ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
- if (ret < 0)
- netdev_warn(dev->net, "Error reading PM_CTRL\n");
return ret;
}
@@ -1371,27 +1378,21 @@ static int smsc95xx_enter_suspend1(struct usbnet *dev)
/* enter SUSPEND1 mode */
ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading PM_CTRL\n");
+ if (ret < 0)
return ret;
- }
val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
val |= PM_CTL_SUS_MODE_1;
ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing PM_CTRL\n");
+ if (ret < 0)
return ret;
- }
/* clear wol status, enable energy detection */
val &= ~PM_CTL_WUPS_;
val |= (PM_CTL_WUPS_ED_ | PM_CTL_ED_EN_);
ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
- if (ret < 0)
- netdev_warn(dev->net, "Error writing PM_CTRL\n");
return ret;
}
@@ -1402,17 +1403,13 @@ static int smsc95xx_enter_suspend2(struct usbnet *dev)
int ret;
ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading PM_CTRL\n");
+ if (ret < 0)
return ret;
- }
val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
val |= PM_CTL_SUS_MODE_2;
ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
- if (ret < 0)
- netdev_warn(dev->net, "Error writing PM_CTRL\n");
return ret;
}
@@ -1442,32 +1439,24 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
/* disable energy detect (link up) & wake up events */
ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading WUCSR\n");
+ if (ret < 0)
goto done;
- }
val &= ~(WUCSR_MPEN_ | WUCSR_WAKE_EN_);
ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUCSR\n");
+ if (ret < 0)
goto done;
- }
ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading PM_CTRL\n");
+ if (ret < 0)
goto done;
- }
val &= ~(PM_CTL_ED_EN_ | PM_CTL_WOL_EN_);
ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing PM_CTRL\n");
+ if (ret < 0)
goto done;
- }
ret = smsc95xx_enter_suspend2(dev);
goto done;
@@ -1565,7 +1554,6 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
for (i = 0; i < (wuff_filter_count * 4); i++) {
ret = smsc95xx_write_reg_nopm(dev, WUFF, filter_mask[i]);
if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUFF\n");
kfree(filter_mask);
goto done;
}
@@ -1574,67 +1562,51 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
for (i = 0; i < (wuff_filter_count / 4); i++) {
ret = smsc95xx_write_reg_nopm(dev, WUFF, command[i]);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUFF\n");
+ if (ret < 0)
goto done;
- }
}
for (i = 0; i < (wuff_filter_count / 4); i++) {
ret = smsc95xx_write_reg_nopm(dev, WUFF, offset[i]);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUFF\n");
+ if (ret < 0)
goto done;
- }
}
for (i = 0; i < (wuff_filter_count / 2); i++) {
ret = smsc95xx_write_reg_nopm(dev, WUFF, crc[i]);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUFF\n");
+ if (ret < 0)
goto done;
- }
}
/* clear any pending pattern match packet status */
ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading WUCSR\n");
+ if (ret < 0)
goto done;
- }
val |= WUCSR_WUFR_;
ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUCSR\n");
+ if (ret < 0)
goto done;
- }
}
if (pdata->wolopts & WAKE_MAGIC) {
/* clear any pending magic packet status */
ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading WUCSR\n");
+ if (ret < 0)
goto done;
- }
val |= WUCSR_MPR_;
ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUCSR\n");
+ if (ret < 0)
goto done;
- }
}
/* enable/disable wakeup sources */
ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading WUCSR\n");
+ if (ret < 0)
goto done;
- }
if (pdata->wolopts & (WAKE_BCAST | WAKE_MCAST | WAKE_ARP | WAKE_UCAST)) {
netdev_info(dev->net, "enabling pattern match wakeup\n");
@@ -1653,17 +1625,13 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
}
ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUCSR\n");
+ if (ret < 0)
goto done;
- }
/* enable wol wakeup source */
ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading PM_CTRL\n");
+ if (ret < 0)
goto done;
- }
val |= PM_CTL_WOL_EN_;
@@ -1672,10 +1640,8 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
val |= PM_CTL_ED_EN_;
ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing PM_CTRL\n");
+ if (ret < 0)
goto done;
- }
/* enable receiver to enable frame reception */
smsc95xx_start_rx_path(dev, 1);
@@ -1702,34 +1668,26 @@ static int smsc95xx_resume(struct usb_interface *intf)
if (pdata->wolopts) {
/* clear wake-up sources */
ret = smsc95xx_read_reg_nopm(dev, WUCSR, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading WUCSR\n");
+ if (ret < 0)
return ret;
- }
val &= ~(WUCSR_WAKE_EN_ | WUCSR_MPEN_);
ret = smsc95xx_write_reg_nopm(dev, WUCSR, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing WUCSR\n");
+ if (ret < 0)
return ret;
- }
/* clear wake-up status */
ret = smsc95xx_read_reg_nopm(dev, PM_CTRL, &val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error reading PM_CTRL\n");
+ if (ret < 0)
return ret;
- }
val &= ~PM_CTL_WOL_EN_;
val |= PM_CTL_WUPS_;
ret = smsc95xx_write_reg_nopm(dev, PM_CTRL, val);
- if (ret < 0) {
- netdev_warn(dev->net, "Error writing PM_CTRL\n");
+ if (ret < 0)
return ret;
- }
}
ret = usbnet_resume(intf);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend
2012-12-11 16:13 ` Ming Lei
@ 2012-12-14 13:35 ` Steve Glendinning
[not found] ` <CAKh2mn7C3r_bvKAv4p3AHco=9JpmewOAJ8JwhtFPw50yDy5cUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Steve Glendinning @ 2012-12-14 13:35 UTC (permalink / raw)
To: Ming Lei
Cc: Steve Glendinning, netdev, linux-usb, Oliver Neukum, Greg Kroah-Hartman
On 11 December 2012 16:13, Ming Lei <ming.lei@canonical.com> wrote:
> On Tue, Dec 11, 2012 at 11:26 PM, Steve Glendinning
> <steve.glendinning@shawell.net> wrote:
>> +
>> + if (on)
>> + usb_autopm_get_interface_no_resume(dev->intf);
>> + else
>> + usb_autopm_put_interface_no_suspend(dev->intf);
>
> The above line should be
>
> usb_autopm_put_interface(dev->intf);
Thanks Ming, I've updated this.
> IMO, it is better to keep smsc95xx_info.manage_power as NULL
> for devices without FEATURE_AUTOSUSPEND, so that fewer code
> and follow the current .mange_power usage(see its comment).
>
> Currently, if the function pointer of manage_power is set, it means that
> the device supports USB autosuspend, but your trick will make the
> assumption not true for some smsc devices.
Oliver?
--
Steve Glendinning
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend
2012-12-11 16:27 ` Joe Perches
@ 2012-12-14 13:38 ` Steve Glendinning
0 siblings, 0 replies; 7+ messages in thread
From: Steve Glendinning @ 2012-12-14 13:38 UTC (permalink / raw)
To: Joe Perches
Cc: Steve Glendinning, netdev, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Ming Lei, Oliver Neukum, Greg Kroah-Hartman
On 11 December 2012 16:27, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Tue, 2012-12-11 at 15:26 +0000, Steve Glendinning wrote:
> []> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> []
>> + ret = smsc95xx_read_reg_nopm(dev, RX_FIFO_INF, &val);
>> + if (ret < 0) {
>> + netdev_warn(dev->net, "Error reading RX_FIFO_INF");
>
> Please remember terminating newlines.
Oops, I've fixed this now in my tree.
> If you are always going to warn bad reads or writes,
> it'd be good to change smsc95xx_read_reg_nopm
> and write to emit the message.
>
> It saves ~3% of code, 1K
> $ size drivers/net/usb/smsc95xx.o*
> text data bss dec hex filename
> 27064 2724 6072 35860 8c14 drivers/net/usb/smsc95xx.o.new
> 27921 2724 6256 36901 9025 drivers/net/usb/smsc95xx.o.old
>
> Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
Thanks for this Joe, actually as the register access functions already
print a warning with the register number I don't think we need to log
this at the calling site at all. I'll rip out all the warnings that
don't add any significant new information (i.e. *why* the call may
have failed, or what its failure means).
--
Steve Glendinning
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend
[not found] ` <CAKh2mn7C3r_bvKAv4p3AHco=9JpmewOAJ8JwhtFPw50yDy5cUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-12-14 14:22 ` Oliver Neukum
[not found] ` <4228102.c0z2lbILKD-ugxBuEnWX9yG/4A2pS7c2Q@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Oliver Neukum @ 2012-12-14 14:22 UTC (permalink / raw)
To: Steve Glendinning
Cc: Ming Lei, Steve Glendinning, netdev,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
On Friday 14 December 2012 13:35:04 Steve Glendinning wrote:
> Thanks Ming, I've updated this.
Very good. Good catch Ming.
> > IMO, it is better to keep smsc95xx_info.manage_power as NULL
> > for devices without FEATURE_AUTOSUSPEND, so that fewer code
> > and follow the current .mange_power usage(see its comment).
> >
> > Currently, if the function pointer of manage_power is set, it means that
> > the device supports USB autosuspend, but your trick will make the
manage_power() can return errors. So the assumption was always
strictly speaking invalid.
> > assumption not true for some smsc devices.
>
> Oliver?
On second thought, I think that if a driver can do manage_power(), even
only for a subset of devices, it should implement it. Doctoring the table of methods
is very, very ugly, especially as this not protected by a lock.
But this is not nice. We need a better way.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend
[not found] ` <4228102.c0z2lbILKD-ugxBuEnWX9yG/4A2pS7c2Q@public.gmane.org>
@ 2012-12-14 16:24 ` Ming Lei
0 siblings, 0 replies; 7+ messages in thread
From: Ming Lei @ 2012-12-14 16:24 UTC (permalink / raw)
To: Oliver Neukum
Cc: Steve Glendinning, Steve Glendinning, netdev,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Greg Kroah-Hartman
On Fri, Dec 14, 2012 at 10:22 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>
> On second thought, I think that if a driver can do manage_power(), even
> only for a subset of devices, it should implement it. Doctoring the table of methods
> is very, very ugly,
Sorry, why is it very ugly? netdev_ops/ethtool_ops is still set dynamically
inside bind().
> especially as this not protected by a lock.
Looks lock isn't needed since probe/remove and open/close path
can't be concurrent.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-12-14 16:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-11 15:26 [PATCHv2][RFC] smsc95xx: enable dynamic autosuspend Steve Glendinning
[not found] ` <1355239561-2740-1-git-send-email-steve.glendinning-nksJyM/082jR7s880joybQ@public.gmane.org>
2012-12-11 16:13 ` Ming Lei
2012-12-14 13:35 ` Steve Glendinning
[not found] ` <CAKh2mn7C3r_bvKAv4p3AHco=9JpmewOAJ8JwhtFPw50yDy5cUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-12-14 14:22 ` Oliver Neukum
[not found] ` <4228102.c0z2lbILKD-ugxBuEnWX9yG/4A2pS7c2Q@public.gmane.org>
2012-12-14 16:24 ` Ming Lei
2012-12-11 16:27 ` Joe Perches
2012-12-14 13:38 ` Steve Glendinning
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.