* [PATCH RFC 0/3] libertas: powersave fixes
@ 2011-04-07 20:25 Vasily Khoruzhick
2011-04-07 20:25 ` [PATCH RFC 1/3] libertas_spi: cancel packet work on module removal Vasily Khoruzhick
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Vasily Khoruzhick @ 2011-04-07 20:25 UTC (permalink / raw)
To: Dan Williams, John W. Linville, libertas-dev, linux-wireless,
anarsoul
This series attempts to make libertas driver a bit powersave friendlier by
adding .set_power callback, so common part can disable hw when it's necessary
(when iface is down or before going into suspend). Also it adds implementation of
.set_power callback for if_spi subdriver.
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH RFC 1/3] libertas_spi: cancel packet work on module removal 2011-04-07 20:25 [PATCH RFC 0/3] libertas: powersave fixes Vasily Khoruzhick @ 2011-04-07 20:25 ` Vasily Khoruzhick 2011-04-07 21:43 ` Dan Williams 2011-04-07 20:25 ` [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers Vasily Khoruzhick 2011-04-07 20:26 ` [PATCH RFC 3/3] libertas_spi: introduce set_power callback Vasily Khoruzhick 2 siblings, 1 reply; 12+ messages in thread From: Vasily Khoruzhick @ 2011-04-07 20:25 UTC (permalink / raw) To: Dan Williams, John W. Linville, libertas-dev, linux-wireless, anarsoul Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> --- drivers/net/wireless/libertas/if_spi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c index 078ef43..d893560 100644 --- a/drivers/net/wireless/libertas/if_spi.c +++ b/drivers/net/wireless/libertas/if_spi.c @@ -1195,6 +1195,7 @@ static int __devexit libertas_spi_remove(struct spi_device *spi) lbs_remove_card(priv); /* will call free_netdev */ free_irq(spi->irq, card); + cancel_work_sync(&card->packet_work); flush_workqueue(card->workqueue); destroy_workqueue(card->workqueue); if (card->pdata->teardown) -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/3] libertas_spi: cancel packet work on module removal 2011-04-07 20:25 ` [PATCH RFC 1/3] libertas_spi: cancel packet work on module removal Vasily Khoruzhick @ 2011-04-07 21:43 ` Dan Williams 0 siblings, 0 replies; 12+ messages in thread From: Dan Williams @ 2011-04-07 21:43 UTC (permalink / raw) To: Vasily Khoruzhick; +Cc: John W. Linville, libertas-dev, linux-wireless On Thu, 2011-04-07 at 23:25 +0300, Vasily Khoruzhick wrote: > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> Acked-by: Dan Williams <dcbw@redhat.com> > --- > drivers/net/wireless/libertas/if_spi.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c > index 078ef43..d893560 100644 > --- a/drivers/net/wireless/libertas/if_spi.c > +++ b/drivers/net/wireless/libertas/if_spi.c > @@ -1195,6 +1195,7 @@ static int __devexit libertas_spi_remove(struct spi_device *spi) > lbs_remove_card(priv); /* will call free_netdev */ > > free_irq(spi->irq, card); > + cancel_work_sync(&card->packet_work); > flush_workqueue(card->workqueue); > destroy_workqueue(card->workqueue); > if (card->pdata->teardown) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers 2011-04-07 20:25 [PATCH RFC 0/3] libertas: powersave fixes Vasily Khoruzhick 2011-04-07 20:25 ` [PATCH RFC 1/3] libertas_spi: cancel packet work on module removal Vasily Khoruzhick @ 2011-04-07 20:25 ` Vasily Khoruzhick 2011-04-07 22:35 ` Dan Williams 2011-04-08 9:10 ` Alberto Panizzo 2011-04-07 20:26 ` [PATCH RFC 3/3] libertas_spi: introduce set_power callback Vasily Khoruzhick 2 siblings, 2 replies; 12+ messages in thread From: Vasily Khoruzhick @ 2011-04-07 20:25 UTC (permalink / raw) To: Dan Williams, John W. Linville, libertas-dev, linux-wireless, anarsoul Add set_power callback, so driver can disable device when interface is down or before going into suspend Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> --- drivers/net/wireless/libertas/cmd.c | 18 ++++- drivers/net/wireless/libertas/dev.h | 3 +- drivers/net/wireless/libertas/main.c | 177 ++++++++++++++++++++++------------ 3 files changed, 135 insertions(+), 63 deletions(-) diff --git a/drivers/net/wireless/libertas/cmd.c b/drivers/net/wireless/libertas/cmd.c index 7e8a658..d0e7df6 100644 --- a/drivers/net/wireless/libertas/cmd.c +++ b/drivers/net/wireless/libertas/cmd.c @@ -84,6 +84,7 @@ static u8 is_command_allowed_in_ps(u16 cmd) int lbs_update_hw_spec(struct lbs_private *priv) { struct cmd_ds_get_hw_spec cmd; + struct cmd_ds_802_11_mac_address hw_addr_cmd; int ret = -1; u32 i; @@ -151,8 +152,20 @@ int lbs_update_hw_spec(struct lbs_private *priv) memcpy(priv->mesh_dev->dev_addr, priv->current_addr, ETH_ALEN); priv->copied_hwaddr = 1; + } else { + /* Copy addr back to hw */ + memcpy(priv->current_addr, priv->dev->dev_addr, ETH_ALEN); + hw_addr_cmd.hdr.size = cpu_to_le16(sizeof(hw_addr_cmd)); + hw_addr_cmd.action = cpu_to_le16(CMD_ACT_SET); + memcpy(hw_addr_cmd.macadd, priv->current_addr, ETH_ALEN); + + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, + &hw_addr_cmd); + if (ret) + lbs_deb_net("set MAC address failed\n"); } + out: lbs_deb_leave(LBS_DEB_CMD); return ret; @@ -1111,6 +1124,7 @@ out: void lbs_set_mac_control(struct lbs_private *priv) { + int ret; struct cmd_ds_mac_control cmd; lbs_deb_enter(LBS_DEB_CMD); @@ -1119,7 +1133,9 @@ void lbs_set_mac_control(struct lbs_private *priv) cmd.action = cpu_to_le16(priv->mac_control); cmd.reserved = 0; - lbs_cmd_async(priv, CMD_MAC_CONTROL, &cmd.hdr, sizeof(cmd)); + ret = lbs_cmd_with_response(priv, CMD_MAC_CONTROL, &cmd); + if (ret) + lbs_deb_net("set MAC control failed\n"); lbs_deb_leave(LBS_DEB_CMD); } diff --git a/drivers/net/wireless/libertas/dev.h b/drivers/net/wireless/libertas/dev.h index bc461eb..9a873bc 100644 --- a/drivers/net/wireless/libertas/dev.h +++ b/drivers/net/wireless/libertas/dev.h @@ -90,12 +90,13 @@ struct lbs_private { void *card; u8 fw_ready; u8 surpriseremoved; - u8 setup_fw_on_resume; + int power_on_cnt; int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb); void (*reset_card) (struct lbs_private *priv); int (*enter_deep_sleep) (struct lbs_private *priv); int (*exit_deep_sleep) (struct lbs_private *priv); int (*reset_deep_sleep_wakeup) (struct lbs_private *priv); + void (*set_power) (struct lbs_private *priv, int enable); /* Adapter info (from EEPROM) */ u32 fwrelease; diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c index ca8149c..7922ead 100644 --- a/drivers/net/wireless/libertas/main.c +++ b/drivers/net/wireless/libertas/main.c @@ -89,6 +89,73 @@ u8 lbs_data_rate_to_fw_index(u32 rate) return 0; } +/** + * @brief This function gets the HW spec from the firmware and sets + * some basic parameters. + * + * @param priv A pointer to struct lbs_private structure + * @return 0 or -1 + */ +static int lbs_setup_firmware(struct lbs_private *priv) +{ + int ret = -1; + s16 curlevel = 0, minlevel = 0, maxlevel = 0; + + lbs_deb_enter(LBS_DEB_FW); + + /* Read MAC address from firmware */ + memset(priv->current_addr, 0xff, ETH_ALEN); + ret = lbs_update_hw_spec(priv); + if (ret) + goto done; + + /* Read power levels if available */ + ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); + if (ret == 0) { + priv->txpower_cur = curlevel; + priv->txpower_min = minlevel; + priv->txpower_max = maxlevel; + } + + /* Send cmd to FW to enable 11D function */ + ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); + + lbs_set_mac_control(priv); +done: + lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); + return ret; +} + +/** + * @brief This function enables device + * + * @param priv A pointer to struct lbs_private structure + */ +static void lbs_power_on(struct lbs_private *priv) +{ + if (priv->set_power) { + priv->power_on_cnt++; + if (priv->power_on_cnt == 1) { + priv->set_power(priv, 1); + lbs_setup_firmware(priv); + lbs_init_mesh(priv); + } + } +} + +/** + * @brief This function disables device + * + * @param priv A pointer to struct lbs_private structure + */ +static void lbs_power_off(struct lbs_private *priv) +{ + if (priv->set_power) { + priv->power_on_cnt--; + if (priv->power_on_cnt == 0) + priv->set_power(priv, 0); + } +} /** * @brief This function opens the ethX interface @@ -103,6 +170,8 @@ static int lbs_dev_open(struct net_device *dev) lbs_deb_enter(LBS_DEB_NET); + lbs_power_on(priv); + spin_lock_irq(&priv->driver_lock); priv->stopping = false; @@ -136,13 +205,14 @@ static int lbs_eth_stop(struct net_device *dev) netif_stop_queue(dev); spin_unlock_irq(&priv->driver_lock); - schedule_work(&priv->mcast_work); cancel_delayed_work_sync(&priv->scan_work); if (priv->scan_req) { cfg80211_scan_done(priv->scan_req, false); priv->scan_req = NULL; } + lbs_power_off(priv); + lbs_deb_leave(LBS_DEB_NET); return 0; } @@ -201,15 +271,16 @@ int lbs_set_mac_address(struct net_device *dev, void *addr) /* In case it was called from the mesh device */ dev = priv->dev; + if (priv->power_on_cnt) { + cmd.hdr.size = cpu_to_le16(sizeof(cmd)); + cmd.action = cpu_to_le16(CMD_ACT_SET); + memcpy(cmd.macadd, phwaddr->sa_data, ETH_ALEN); - cmd.hdr.size = cpu_to_le16(sizeof(cmd)); - cmd.action = cpu_to_le16(CMD_ACT_SET); - memcpy(cmd.macadd, phwaddr->sa_data, ETH_ALEN); - - ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, &cmd); - if (ret) { - lbs_deb_net("set MAC address failed\n"); - goto done; + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, &cmd); + if (ret) { + lbs_deb_net("set MAC address failed\n"); + goto done; + } } memcpy(priv->current_addr, phwaddr->sa_data, ETH_ALEN); @@ -539,59 +610,27 @@ static int lbs_thread(void *data) return 0; } -/** - * @brief This function gets the HW spec from the firmware and sets - * some basic parameters. - * - * @param priv A pointer to struct lbs_private structure - * @return 0 or -1 - */ -static int lbs_setup_firmware(struct lbs_private *priv) -{ - int ret = -1; - s16 curlevel = 0, minlevel = 0, maxlevel = 0; - - lbs_deb_enter(LBS_DEB_FW); - - /* Read MAC address from firmware */ - memset(priv->current_addr, 0xff, ETH_ALEN); - ret = lbs_update_hw_spec(priv); - if (ret) - goto done; - - /* Read power levels if available */ - ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); - if (ret == 0) { - priv->txpower_cur = curlevel; - priv->txpower_min = minlevel; - priv->txpower_max = maxlevel; - } - - /* Send cmd to FW to enable 11D function */ - ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); - - lbs_set_mac_control(priv); -done: - lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); - return ret; -} - int lbs_suspend(struct lbs_private *priv) { - int ret; + int ret = 0; lbs_deb_enter(LBS_DEB_FW); - if (priv->is_deep_sleep) { - ret = lbs_set_deep_sleep(priv, 0); - if (ret) { - lbs_pr_err("deep sleep cancellation failed: %d\n", ret); - return ret; + if (priv->set_power) + lbs_power_off(priv); + else { + if (priv->is_deep_sleep) { + ret = lbs_set_deep_sleep(priv, 0); + if (ret) { + lbs_pr_err( + "deep sleep cancellation failed: %d\n", ret); + return ret; + } + priv->deep_sleep_required = 1; } - priv->deep_sleep_required = 1; - } - ret = lbs_set_host_sleep(priv, 1); + ret = lbs_set_host_sleep(priv, 1); + } netif_device_detach(priv->dev); if (priv->mesh_dev) @@ -604,26 +643,26 @@ EXPORT_SYMBOL_GPL(lbs_suspend); int lbs_resume(struct lbs_private *priv) { - int ret; + int ret = 0; lbs_deb_enter(LBS_DEB_FW); - ret = lbs_set_host_sleep(priv, 0); + if (priv->set_power) + lbs_power_on(priv); + else + ret = lbs_set_host_sleep(priv, 0); netif_device_attach(priv->dev); if (priv->mesh_dev) netif_device_attach(priv->mesh_dev); - if (priv->deep_sleep_required) { + if (!priv->set_power && priv->deep_sleep_required) { priv->deep_sleep_required = 0; ret = lbs_set_deep_sleep(priv, 1); if (ret) lbs_pr_err("deep sleep activation failed: %d\n", ret); } - if (priv->setup_fw_on_resume) - ret = lbs_setup_firmware(priv); - lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); return ret; } @@ -917,6 +956,9 @@ void lbs_remove_card(struct lbs_private *priv) priv->surpriseremoved = 1; kthread_stop(priv->main_thread); + /* Disable card power if it's still on */ + lbs_power_off(priv); + lbs_free_adapter(priv); lbs_cfg_free(priv); free_netdev(dev); @@ -944,6 +986,16 @@ int lbs_start_card(struct lbs_private *priv) lbs_deb_enter(LBS_DEB_MAIN); + /* We're not using lbs_power_on here because we don't want + * to setup firmware twice. + * TODO: replace following code with lbs_power_on() when set_power + * callback become mandatory. + */ + if (priv->set_power) { + priv->power_on_cnt++; + priv->set_power(priv, 1); + } + /* poke the firmware */ ret = lbs_setup_firmware(priv); if (ret) @@ -964,6 +1016,9 @@ int lbs_start_card(struct lbs_private *priv) ret = 0; + /* Init is done, so we can disable card power */ + lbs_power_off(priv); + done: lbs_deb_leave_args(LBS_DEB_MAIN, "ret %d", ret); return ret; -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers 2011-04-07 20:25 ` [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers Vasily Khoruzhick @ 2011-04-07 22:35 ` Dan Williams 2011-04-08 9:54 ` Alberto Panizzo 2011-04-08 11:42 ` Vasily Khoruzhick 2011-04-08 9:10 ` Alberto Panizzo 1 sibling, 2 replies; 12+ messages in thread From: Dan Williams @ 2011-04-07 22:35 UTC (permalink / raw) To: Vasily Khoruzhick; +Cc: John W. Linville, libertas-dev, linux-wireless On Thu, 2011-04-07 at 23:25 +0300, Vasily Khoruzhick wrote: > Add set_power callback, so driver can disable device when > interface is down or before going into suspend > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > --- > drivers/net/wireless/libertas/cmd.c | 18 ++++- > drivers/net/wireless/libertas/dev.h | 3 +- > drivers/net/wireless/libertas/main.c | 177 ++++++++++++++++++++++------------ > 3 files changed, 135 insertions(+), 63 deletions(-) > > diff --git a/drivers/net/wireless/libertas/cmd.c b/drivers/net/wireless/libertas/cmd.c > index 7e8a658..d0e7df6 100644 > --- a/drivers/net/wireless/libertas/cmd.c > +++ b/drivers/net/wireless/libertas/cmd.c > @@ -84,6 +84,7 @@ static u8 is_command_allowed_in_ps(u16 cmd) > int lbs_update_hw_spec(struct lbs_private *priv) > { > struct cmd_ds_get_hw_spec cmd; > + struct cmd_ds_802_11_mac_address hw_addr_cmd; > int ret = -1; > u32 i; > > @@ -151,8 +152,20 @@ int lbs_update_hw_spec(struct lbs_private *priv) > memcpy(priv->mesh_dev->dev_addr, > priv->current_addr, ETH_ALEN); > priv->copied_hwaddr = 1; > + } else { > + /* Copy addr back to hw */ > + memcpy(priv->current_addr, priv->dev->dev_addr, ETH_ALEN); > + hw_addr_cmd.hdr.size = cpu_to_le16(sizeof(hw_addr_cmd)); > + hw_addr_cmd.action = cpu_to_le16(CMD_ACT_SET); > + memcpy(hw_addr_cmd.macadd, priv->current_addr, ETH_ALEN); > + > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, > + &hw_addr_cmd); > + if (ret) > + lbs_deb_net("set MAC address failed\n"); Might want to make a note here that this part is only used if the MAC was set while the card was powered down. > } > > + > out: > lbs_deb_leave(LBS_DEB_CMD); > return ret; > @@ -1111,6 +1124,7 @@ out: > > void lbs_set_mac_control(struct lbs_private *priv) > { > + int ret; > struct cmd_ds_mac_control cmd; > > lbs_deb_enter(LBS_DEB_CMD); > @@ -1119,7 +1133,9 @@ void lbs_set_mac_control(struct lbs_private *priv) > cmd.action = cpu_to_le16(priv->mac_control); > cmd.reserved = 0; > > - lbs_cmd_async(priv, CMD_MAC_CONTROL, &cmd.hdr, sizeof(cmd)); > + ret = lbs_cmd_with_response(priv, CMD_MAC_CONTROL, &cmd); > + if (ret) > + lbs_deb_net("set MAC control failed\n"); > > lbs_deb_leave(LBS_DEB_CMD); > } > diff --git a/drivers/net/wireless/libertas/dev.h b/drivers/net/wireless/libertas/dev.h > index bc461eb..9a873bc 100644 > --- a/drivers/net/wireless/libertas/dev.h > +++ b/drivers/net/wireless/libertas/dev.h > @@ -90,12 +90,13 @@ struct lbs_private { > void *card; > u8 fw_ready; > u8 surpriseremoved; > - u8 setup_fw_on_resume; > + int power_on_cnt; > int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb); > void (*reset_card) (struct lbs_private *priv); > int (*enter_deep_sleep) (struct lbs_private *priv); > int (*exit_deep_sleep) (struct lbs_private *priv); > int (*reset_deep_sleep_wakeup) (struct lbs_private *priv); > + void (*set_power) (struct lbs_private *priv, int enable); > > /* Adapter info (from EEPROM) */ > u32 fwrelease; > diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c > index ca8149c..7922ead 100644 > --- a/drivers/net/wireless/libertas/main.c > +++ b/drivers/net/wireless/libertas/main.c > @@ -89,6 +89,73 @@ u8 lbs_data_rate_to_fw_index(u32 rate) > return 0; > } > > +/** > + * @brief This function gets the HW spec from the firmware and sets > + * some basic parameters. > + * > + * @param priv A pointer to struct lbs_private structure > + * @return 0 or -1 > + */ > +static int lbs_setup_firmware(struct lbs_private *priv) > +{ > + int ret = -1; > + s16 curlevel = 0, minlevel = 0, maxlevel = 0; > + > + lbs_deb_enter(LBS_DEB_FW); > + > + /* Read MAC address from firmware */ > + memset(priv->current_addr, 0xff, ETH_ALEN); > + ret = lbs_update_hw_spec(priv); > + if (ret) > + goto done; > + > + /* Read power levels if available */ > + ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); > + if (ret == 0) { > + priv->txpower_cur = curlevel; > + priv->txpower_min = minlevel; > + priv->txpower_max = maxlevel; > + } > + > + /* Send cmd to FW to enable 11D function */ > + ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); > + > + lbs_set_mac_control(priv); > +done: > + lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > + return ret; > +} > + > +/** > + * @brief This function enables device > + * > + * @param priv A pointer to struct lbs_private structure > + */ > +static void lbs_power_on(struct lbs_private *priv) > +{ > + if (priv->set_power) { > + priv->power_on_cnt++; > + if (priv->power_on_cnt == 1) { > + priv->set_power(priv, 1); > + lbs_setup_firmware(priv); > + lbs_init_mesh(priv); > + } > + } > +} > + > +/** > + * @brief This function disables device > + * > + * @param priv A pointer to struct lbs_private structure > + */ > +static void lbs_power_off(struct lbs_private *priv) > +{ > + if (priv->set_power) { > + priv->power_on_cnt--; > + if (priv->power_on_cnt == 0) > + priv->set_power(priv, 0); > + } > +} > > /** > * @brief This function opens the ethX interface > @@ -103,6 +170,8 @@ static int lbs_dev_open(struct net_device *dev) > > lbs_deb_enter(LBS_DEB_NET); > > + lbs_power_on(priv); > + > spin_lock_irq(&priv->driver_lock); > priv->stopping = false; > > @@ -136,13 +205,14 @@ static int lbs_eth_stop(struct net_device *dev) > netif_stop_queue(dev); > spin_unlock_irq(&priv->driver_lock); > > - schedule_work(&priv->mcast_work); Any reason this is getting removed? > cancel_delayed_work_sync(&priv->scan_work); > if (priv->scan_req) { > cfg80211_scan_done(priv->scan_req, false); > priv->scan_req = NULL; > } > > + lbs_power_off(priv); > + So the idea here is that when the device is opened, the card powers up, but when it's closed, the card powers down? If that's the case, we can make mesh work with this too if you also call lbs_power_on()/lbs_power_off() from the mesh.c functions that open/close the mesh device, since power on/off is refcounted, essentially. > lbs_deb_leave(LBS_DEB_NET); > return 0; > } > @@ -201,15 +271,16 @@ int lbs_set_mac_address(struct net_device *dev, void *addr) > > /* In case it was called from the mesh device */ > dev = priv->dev; > + if (priv->power_on_cnt) { > + cmd.hdr.size = cpu_to_le16(sizeof(cmd)); > + cmd.action = cpu_to_le16(CMD_ACT_SET); > + memcpy(cmd.macadd, phwaddr->sa_data, ETH_ALEN); > > - cmd.hdr.size = cpu_to_le16(sizeof(cmd)); > - cmd.action = cpu_to_le16(CMD_ACT_SET); > - memcpy(cmd.macadd, phwaddr->sa_data, ETH_ALEN); > - > - ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, &cmd); > - if (ret) { > - lbs_deb_net("set MAC address failed\n"); > - goto done; > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, &cmd); > + if (ret) { > + lbs_deb_net("set MAC address failed\n"); > + goto done; > + } > } > > memcpy(priv->current_addr, phwaddr->sa_data, ETH_ALEN); > @@ -539,59 +610,27 @@ static int lbs_thread(void *data) > return 0; > } > > -/** > - * @brief This function gets the HW spec from the firmware and sets > - * some basic parameters. > - * > - * @param priv A pointer to struct lbs_private structure > - * @return 0 or -1 > - */ > -static int lbs_setup_firmware(struct lbs_private *priv) > -{ > - int ret = -1; > - s16 curlevel = 0, minlevel = 0, maxlevel = 0; > - > - lbs_deb_enter(LBS_DEB_FW); > - > - /* Read MAC address from firmware */ > - memset(priv->current_addr, 0xff, ETH_ALEN); > - ret = lbs_update_hw_spec(priv); > - if (ret) > - goto done; > - > - /* Read power levels if available */ > - ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); > - if (ret == 0) { > - priv->txpower_cur = curlevel; > - priv->txpower_min = minlevel; > - priv->txpower_max = maxlevel; > - } > - > - /* Send cmd to FW to enable 11D function */ > - ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); > - > - lbs_set_mac_control(priv); > -done: > - lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > - return ret; > -} > - > int lbs_suspend(struct lbs_private *priv) > { > - int ret; > + int ret = 0; > > lbs_deb_enter(LBS_DEB_FW); > > - if (priv->is_deep_sleep) { > - ret = lbs_set_deep_sleep(priv, 0); > - if (ret) { > - lbs_pr_err("deep sleep cancellation failed: %d\n", ret); > - return ret; > + if (priv->set_power) > + lbs_power_off(priv); > + else { > + if (priv->is_deep_sleep) { > + ret = lbs_set_deep_sleep(priv, 0); > + if (ret) { > + lbs_pr_err( > + "deep sleep cancellation failed: %d\n", ret); > + return ret; > + } > + priv->deep_sleep_required = 1; > } > - priv->deep_sleep_required = 1; > - } > > - ret = lbs_set_host_sleep(priv, 1); > + ret = lbs_set_host_sleep(priv, 1); > + } > > netif_device_detach(priv->dev); > if (priv->mesh_dev) > @@ -604,26 +643,26 @@ EXPORT_SYMBOL_GPL(lbs_suspend); > > int lbs_resume(struct lbs_private *priv) > { > - int ret; > + int ret = 0; > > lbs_deb_enter(LBS_DEB_FW); > > - ret = lbs_set_host_sleep(priv, 0); > + if (priv->set_power) > + lbs_power_on(priv); > + else > + ret = lbs_set_host_sleep(priv, 0); > > netif_device_attach(priv->dev); > if (priv->mesh_dev) > netif_device_attach(priv->mesh_dev); > > - if (priv->deep_sleep_required) { > + if (!priv->set_power && priv->deep_sleep_required) { > priv->deep_sleep_required = 0; > ret = lbs_set_deep_sleep(priv, 1); > if (ret) > lbs_pr_err("deep sleep activation failed: %d\n", ret); > } > > - if (priv->setup_fw_on_resume) > - ret = lbs_setup_firmware(priv); > - > lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > return ret; > } > @@ -917,6 +956,9 @@ void lbs_remove_card(struct lbs_private *priv) > priv->surpriseremoved = 1; > kthread_stop(priv->main_thread); > > + /* Disable card power if it's still on */ > + lbs_power_off(priv); > + > lbs_free_adapter(priv); > lbs_cfg_free(priv); > free_netdev(dev); > @@ -944,6 +986,16 @@ int lbs_start_card(struct lbs_private *priv) > > lbs_deb_enter(LBS_DEB_MAIN); > > + /* We're not using lbs_power_on here because we don't want > + * to setup firmware twice. > + * TODO: replace following code with lbs_power_on() when set_power > + * callback become mandatory. > + */ > + if (priv->set_power) { > + priv->power_on_cnt++; > + priv->set_power(priv, 1); > + } > + > /* poke the firmware */ > ret = lbs_setup_firmware(priv); > if (ret) > @@ -964,6 +1016,9 @@ int lbs_start_card(struct lbs_private *priv) > > ret = 0; > > + /* Init is done, so we can disable card power */ > + lbs_power_off(priv); > + > done: > lbs_deb_leave_args(LBS_DEB_MAIN, "ret %d", ret); > return ret; The thing that worries me here (and it's not your fault) is that there are 4 different and overlapping ways to do power saving with libertas: 1) normal 802.11 power save (CMD_802_11_PS_MODE): independent of everything else 2) deep sleep + host sleep: SDIO-only for now, used by OLPC, but could be used by all bus types. Bus remains powered unless the host sleeps too, and it requires bus-specific interaction (GPIO) to wake up the card. Doesn't require a firmware reload because the card is still powered. 3) bus sleep/power on bus power events: requires a firmware reload when the bus comes back because the bus and card aren't powered, triggered by some external suspend/resume mechanism 4) bus sleep/power on dev open/close: same as #3 except the trigger is internal to the driver and it's getting pretty complicated in the code... You'll want to do any of [2, 3, 4] depending on your platform, so that's really the only difference. I'd think that we'd want to enable the dev open/close power save stuff by default. We want deep sleep when we know that (a) the card will be powered despite the bus or host being unpowered, and that's a platform decision. I wonder if there's a better way to integrate all this stuff so it's less confusing in the code? Dan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers 2011-04-07 22:35 ` Dan Williams @ 2011-04-08 9:54 ` Alberto Panizzo 2011-04-08 11:42 ` Vasily Khoruzhick 1 sibling, 0 replies; 12+ messages in thread From: Alberto Panizzo @ 2011-04-08 9:54 UTC (permalink / raw) To: Dan Williams Cc: Vasily Khoruzhick, linux-wireless, John W. Linville, libertas-dev On Thu, 2011-04-07 at 17:35 -0500, Dan Williams wrote: > On Thu, 2011-04-07 at 23:25 +0300, Vasily Khoruzhick wrote: > > Add set_power callback, so driver can disable device when > > interface is down or before going into suspend > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > --- > > drivers/net/wireless/libertas/cmd.c | 18 ++++- > > drivers/net/wireless/libertas/dev.h | 3 +- > > drivers/net/wireless/libertas/main.c | 177 ++++++++++++++++++++++------------ > > 3 files changed, 135 insertions(+), 63 deletions(-) > > > > diff --git a/drivers/net/wireless/libertas/cmd.c b/drivers/net/wireless/libertas/cmd.c > > index 7e8a658..d0e7df6 100644 > > --- a/drivers/net/wireless/libertas/cmd.c > > +++ b/drivers/net/wireless/libertas/cmd.c > > @@ -84,6 +84,7 @@ static u8 is_command_allowed_in_ps(u16 cmd) > > int lbs_update_hw_spec(struct lbs_private *priv) > > { > > struct cmd_ds_get_hw_spec cmd; > > + struct cmd_ds_802_11_mac_address hw_addr_cmd; > > int ret = -1; > > u32 i; > > > > @@ -151,8 +152,20 @@ int lbs_update_hw_spec(struct lbs_private *priv) > > memcpy(priv->mesh_dev->dev_addr, > > priv->current_addr, ETH_ALEN); > > priv->copied_hwaddr = 1; > > + } else { > > + /* Copy addr back to hw */ > > + memcpy(priv->current_addr, priv->dev->dev_addr, ETH_ALEN); > > + hw_addr_cmd.hdr.size = cpu_to_le16(sizeof(hw_addr_cmd)); > > + hw_addr_cmd.action = cpu_to_le16(CMD_ACT_SET); > > + memcpy(hw_addr_cmd.macadd, priv->current_addr, ETH_ALEN); > > + > > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, > > + &hw_addr_cmd); > > + if (ret) > > + lbs_deb_net("set MAC address failed\n"); > > Might want to make a note here that this part is only used if the MAC > was set while the card was powered down. > > > } > > > > + > > out: > > lbs_deb_leave(LBS_DEB_CMD); > > return ret; > > @@ -1111,6 +1124,7 @@ out: > > > > void lbs_set_mac_control(struct lbs_private *priv) > > { > > + int ret; > > struct cmd_ds_mac_control cmd; > > > > lbs_deb_enter(LBS_DEB_CMD); > > @@ -1119,7 +1133,9 @@ void lbs_set_mac_control(struct lbs_private *priv) > > cmd.action = cpu_to_le16(priv->mac_control); > > cmd.reserved = 0; > > > > - lbs_cmd_async(priv, CMD_MAC_CONTROL, &cmd.hdr, sizeof(cmd)); > > + ret = lbs_cmd_with_response(priv, CMD_MAC_CONTROL, &cmd); > > + if (ret) > > + lbs_deb_net("set MAC control failed\n"); > > > > lbs_deb_leave(LBS_DEB_CMD); > > } > > diff --git a/drivers/net/wireless/libertas/dev.h b/drivers/net/wireless/libertas/dev.h > > index bc461eb..9a873bc 100644 > > --- a/drivers/net/wireless/libertas/dev.h > > +++ b/drivers/net/wireless/libertas/dev.h > > @@ -90,12 +90,13 @@ struct lbs_private { > > void *card; > > u8 fw_ready; > > u8 surpriseremoved; > > - u8 setup_fw_on_resume; > > + int power_on_cnt; > > int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb); > > void (*reset_card) (struct lbs_private *priv); > > int (*enter_deep_sleep) (struct lbs_private *priv); > > int (*exit_deep_sleep) (struct lbs_private *priv); > > int (*reset_deep_sleep_wakeup) (struct lbs_private *priv); > > + void (*set_power) (struct lbs_private *priv, int enable); > > > > /* Adapter info (from EEPROM) */ > > u32 fwrelease; > > diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c > > index ca8149c..7922ead 100644 > > --- a/drivers/net/wireless/libertas/main.c > > +++ b/drivers/net/wireless/libertas/main.c > > @@ -89,6 +89,73 @@ u8 lbs_data_rate_to_fw_index(u32 rate) > > return 0; > > } > > > > +/** > > + * @brief This function gets the HW spec from the firmware and sets > > + * some basic parameters. > > + * > > + * @param priv A pointer to struct lbs_private structure > > + * @return 0 or -1 > > + */ > > +static int lbs_setup_firmware(struct lbs_private *priv) > > +{ > > + int ret = -1; > > + s16 curlevel = 0, minlevel = 0, maxlevel = 0; > > + > > + lbs_deb_enter(LBS_DEB_FW); > > + > > + /* Read MAC address from firmware */ > > + memset(priv->current_addr, 0xff, ETH_ALEN); > > + ret = lbs_update_hw_spec(priv); > > + if (ret) > > + goto done; > > + > > + /* Read power levels if available */ > > + ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); > > + if (ret == 0) { > > + priv->txpower_cur = curlevel; > > + priv->txpower_min = minlevel; > > + priv->txpower_max = maxlevel; > > + } > > + > > + /* Send cmd to FW to enable 11D function */ > > + ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); > > + > > + lbs_set_mac_control(priv); > > +done: > > + lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > > + return ret; > > +} > > + > > +/** > > + * @brief This function enables device > > + * > > + * @param priv A pointer to struct lbs_private structure > > + */ > > +static void lbs_power_on(struct lbs_private *priv) > > +{ > > + if (priv->set_power) { > > + priv->power_on_cnt++; > > + if (priv->power_on_cnt == 1) { > > + priv->set_power(priv, 1); > > + lbs_setup_firmware(priv); > > + lbs_init_mesh(priv); > > + } > > + } > > +} > > + > > +/** > > + * @brief This function disables device > > + * > > + * @param priv A pointer to struct lbs_private structure > > + */ > > +static void lbs_power_off(struct lbs_private *priv) > > +{ > > + if (priv->set_power) { > > + priv->power_on_cnt--; > > + if (priv->power_on_cnt == 0) > > + priv->set_power(priv, 0); > > + } > > +} > > > > /** > > * @brief This function opens the ethX interface > > @@ -103,6 +170,8 @@ static int lbs_dev_open(struct net_device *dev) > > > > lbs_deb_enter(LBS_DEB_NET); > > > > + lbs_power_on(priv); > > + > > spin_lock_irq(&priv->driver_lock); > > priv->stopping = false; > > > > @@ -136,13 +205,14 @@ static int lbs_eth_stop(struct net_device *dev) > > netif_stop_queue(dev); > > spin_unlock_irq(&priv->driver_lock); > > > > - schedule_work(&priv->mcast_work); > > Any reason this is getting removed? > > > cancel_delayed_work_sync(&priv->scan_work); > > if (priv->scan_req) { > > cfg80211_scan_done(priv->scan_req, false); > > priv->scan_req = NULL; > > } > > > > + lbs_power_off(priv); > > + > > So the idea here is that when the device is opened, the card powers up, > but when it's closed, the card powers down? If that's the case, we can > make mesh work with this too if you also call > lbs_power_on()/lbs_power_off() from the mesh.c functions that open/close > the mesh device, since power on/off is refcounted, essentially. > > > lbs_deb_leave(LBS_DEB_NET); > > return 0; > > } > > @@ -201,15 +271,16 @@ int lbs_set_mac_address(struct net_device *dev, void *addr) > > > > /* In case it was called from the mesh device */ > > dev = priv->dev; > > + if (priv->power_on_cnt) { > > + cmd.hdr.size = cpu_to_le16(sizeof(cmd)); > > + cmd.action = cpu_to_le16(CMD_ACT_SET); > > + memcpy(cmd.macadd, phwaddr->sa_data, ETH_ALEN); > > > > - cmd.hdr.size = cpu_to_le16(sizeof(cmd)); > > - cmd.action = cpu_to_le16(CMD_ACT_SET); > > - memcpy(cmd.macadd, phwaddr->sa_data, ETH_ALEN); > > - > > - ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, &cmd); > > - if (ret) { > > - lbs_deb_net("set MAC address failed\n"); > > - goto done; > > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, &cmd); > > + if (ret) { > > + lbs_deb_net("set MAC address failed\n"); > > + goto done; > > + } > > } > > > > memcpy(priv->current_addr, phwaddr->sa_data, ETH_ALEN); > > @@ -539,59 +610,27 @@ static int lbs_thread(void *data) > > return 0; > > } > > > > -/** > > - * @brief This function gets the HW spec from the firmware and sets > > - * some basic parameters. > > - * > > - * @param priv A pointer to struct lbs_private structure > > - * @return 0 or -1 > > - */ > > -static int lbs_setup_firmware(struct lbs_private *priv) > > -{ > > - int ret = -1; > > - s16 curlevel = 0, minlevel = 0, maxlevel = 0; > > - > > - lbs_deb_enter(LBS_DEB_FW); > > - > > - /* Read MAC address from firmware */ > > - memset(priv->current_addr, 0xff, ETH_ALEN); > > - ret = lbs_update_hw_spec(priv); > > - if (ret) > > - goto done; > > - > > - /* Read power levels if available */ > > - ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); > > - if (ret == 0) { > > - priv->txpower_cur = curlevel; > > - priv->txpower_min = minlevel; > > - priv->txpower_max = maxlevel; > > - } > > - > > - /* Send cmd to FW to enable 11D function */ > > - ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); > > - > > - lbs_set_mac_control(priv); > > -done: > > - lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > > - return ret; > > -} > > - > > int lbs_suspend(struct lbs_private *priv) > > { > > - int ret; > > + int ret = 0; > > > > lbs_deb_enter(LBS_DEB_FW); > > > > - if (priv->is_deep_sleep) { > > - ret = lbs_set_deep_sleep(priv, 0); > > - if (ret) { > > - lbs_pr_err("deep sleep cancellation failed: %d\n", ret); > > - return ret; > > + if (priv->set_power) > > + lbs_power_off(priv); > > + else { > > + if (priv->is_deep_sleep) { > > + ret = lbs_set_deep_sleep(priv, 0); > > + if (ret) { > > + lbs_pr_err( > > + "deep sleep cancellation failed: %d\n", ret); > > + return ret; > > + } > > + priv->deep_sleep_required = 1; > > } > > - priv->deep_sleep_required = 1; > > - } > > > > - ret = lbs_set_host_sleep(priv, 1); > > + ret = lbs_set_host_sleep(priv, 1); > > + } > > > > netif_device_detach(priv->dev); > > if (priv->mesh_dev) > > @@ -604,26 +643,26 @@ EXPORT_SYMBOL_GPL(lbs_suspend); > > > > int lbs_resume(struct lbs_private *priv) > > { > > - int ret; > > + int ret = 0; > > > > lbs_deb_enter(LBS_DEB_FW); > > > > - ret = lbs_set_host_sleep(priv, 0); > > + if (priv->set_power) > > + lbs_power_on(priv); > > + else > > + ret = lbs_set_host_sleep(priv, 0); > > > > netif_device_attach(priv->dev); > > if (priv->mesh_dev) > > netif_device_attach(priv->mesh_dev); > > > > - if (priv->deep_sleep_required) { > > + if (!priv->set_power && priv->deep_sleep_required) { > > priv->deep_sleep_required = 0; > > ret = lbs_set_deep_sleep(priv, 1); > > if (ret) > > lbs_pr_err("deep sleep activation failed: %d\n", ret); > > } > > > > - if (priv->setup_fw_on_resume) > > - ret = lbs_setup_firmware(priv); > > - > > lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > > return ret; > > } > > @@ -917,6 +956,9 @@ void lbs_remove_card(struct lbs_private *priv) > > priv->surpriseremoved = 1; > > kthread_stop(priv->main_thread); > > > > + /* Disable card power if it's still on */ > > + lbs_power_off(priv); > > + > > lbs_free_adapter(priv); > > lbs_cfg_free(priv); > > free_netdev(dev); > > @@ -944,6 +986,16 @@ int lbs_start_card(struct lbs_private *priv) > > > > lbs_deb_enter(LBS_DEB_MAIN); > > > > + /* We're not using lbs_power_on here because we don't want > > + * to setup firmware twice. > > + * TODO: replace following code with lbs_power_on() when set_power > > + * callback become mandatory. > > + */ > > + if (priv->set_power) { > > + priv->power_on_cnt++; > > + priv->set_power(priv, 1); > > + } > > + > > /* poke the firmware */ > > ret = lbs_setup_firmware(priv); > > if (ret) > > @@ -964,6 +1016,9 @@ int lbs_start_card(struct lbs_private *priv) > > > > ret = 0; > > > > + /* Init is done, so we can disable card power */ > > + lbs_power_off(priv); > > + > > done: > > lbs_deb_leave_args(LBS_DEB_MAIN, "ret %d", ret); > > return ret; > > The thing that worries me here (and it's not your fault) is that there > are 4 different and overlapping ways to do power saving with libertas: > > 1) normal 802.11 power save (CMD_802_11_PS_MODE): independent of > everything else > > 2) deep sleep + host sleep: SDIO-only for now, used by OLPC, but could > be used by all bus types. Bus remains powered unless the host sleeps > too, and it requires bus-specific interaction (GPIO) to wake up the > card. Doesn't require a firmware reload because the card is still > powered. > > 3) bus sleep/power on bus power events: requires a firmware reload when > the bus comes back because the bus and card aren't powered, triggered by > some external suspend/resume mechanism > > 4) bus sleep/power on dev open/close: same as #3 except the trigger is > internal to the driver > > and it's getting pretty complicated in the code... You'll want to do > any of [2, 3, 4] depending on your platform, so that's really the only > difference. I'd think that we'd want to enable the dev open/close power > save stuff by default. > > We want deep sleep when we know that (a) the card will be powered > despite the bus or host being unpowered, and that's a platform decision. > I wonder if there's a better way to integrate all this stuff so it's > less confusing in the code? > > Dan Maybe I can say something on this. We got the libertas driver from 2.6.34 working very well with Power Save mode and Host sleep with an 8686 chip and SDIO interface. This means that: While the wifi chip is connected to an AP: - We can suspend the CPU maintaining the wifi chip in full power. And this is lot reliable. - We can suspend the CPU when the wifi chip in Power Save mode. This is less reliable since the libertas driver do not manage the firmware specifications strictly in this mode. This have to be fixed redesigning the libertas driver in the way of Power save complete awareness: I think that all power save command should be moved to a new command layer between the cmd.c and the bus interface where to implement the documentation logic. There should not be any direct calls to command executions, since there are strict rules in times where commands are allowed and what command are allowed too. We got this working also modifying MMC subsystem code. Because in PS mode the chip do not accept any bus commands, consequently, when resuming, the MMC subsystem does not to do any probes of SDIO bus state if the wifi chip is in PS mode. Well, unfortunately the code is really bound in our use case (fits the Android opportunistic power management) and needs cleanups to get merged. I know that this hurt your ears, but the cleanup is scheduled and I hope this will happen asap. We got tested also the wake filters as the 8686 documentation describe. These filters are working properly but the driver do not manage these in the right way so from the first wake and next suspend the wake conditions got lost. I didn't gone too deep in this because I think this is something that will be managed proficiently in the new power aware layer I told. >From our tests, we found that if the station remains connected to one AP, all the power management can be really useful to save power in a reliable way. But if the station is moving through different rooms with different APs (which are broadcasting the same SSID) If the host is suspended and wake conditions are bound to the wifi chip, the wifi chip does not be able to wake the system on network events because (I think and this have to be proven) the wifi chip is listening at the original AP and does not manage roaming correctly. This even when the wifi chip is maintained full active. Lot of things I know. But even if we will not have enough time to cleanup the code for submission I am really open to be a functionality tester. Best regards, Alberto! > _______________________________________________ > libertas-dev mailing list > libertas-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/libertas-dev ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers 2011-04-07 22:35 ` Dan Williams 2011-04-08 9:54 ` Alberto Panizzo @ 2011-04-08 11:42 ` Vasily Khoruzhick 1 sibling, 0 replies; 12+ messages in thread From: Vasily Khoruzhick @ 2011-04-08 11:42 UTC (permalink / raw) To: Dan Williams; +Cc: John W. Linville, libertas-dev, linux-wireless On Friday 08 April 2011 01:35:31 Dan Williams wrote: > > + } else { > > + /* Copy addr back to hw */ > > + memcpy(priv->current_addr, priv->dev->dev_addr, ETH_ALEN); > > + hw_addr_cmd.hdr.size = cpu_to_le16(sizeof(hw_addr_cmd)); > > + hw_addr_cmd.action = cpu_to_le16(CMD_ACT_SET); > > + memcpy(hw_addr_cmd.macadd, priv->current_addr, ETH_ALEN); > > + > > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, > > + &hw_addr_cmd); > > + if (ret) > > + lbs_deb_net("set MAC address failed\n"); > > Might want to make a note here that this part is only used if the MAC > was set while the card was powered down. Ok > > @@ -136,13 +205,14 @@ static int lbs_eth_stop(struct net_device *dev) > > > > netif_stop_queue(dev); > > spin_unlock_irq(&priv->driver_lock); > > > > - schedule_work(&priv->mcast_work); > > Any reason this is getting removed? It breaks powerdown, and I do not see a reason to do something with multicast here. Would be nice if someone can clarify why it's here. > > + lbs_power_off(priv); > > + > > So the idea here is that when the device is opened, the card powers up, > but when it's closed, the card powers down? Yep > If that's the case, we can > make mesh work with this too if you also call > lbs_power_on()/lbs_power_off() from the mesh.c functions that open/close > the mesh device, since power on/off is refcounted, essentially. Can anyone explain what mesh is? How can I test it? (I have 2 devices with libertas wifi). Of course, I can add lbs_power_{on,off} to mesh.c functions, (and then we need to protect power_on_cnt with mutex) > The thing that worries me here (and it's not your fault) is that there > are 4 different and overlapping ways to do power saving with libertas: > > 1) normal 802.11 power save (CMD_802_11_PS_MODE): independent of > everything else This one is broken at the time, right? > 2) deep sleep + host sleep: SDIO-only for now, used by OLPC, but could > be used by all bus types. Bus remains powered unless the host sleeps > too, and it requires bus-specific interaction (GPIO) to wake up the > card. Doesn't require a firmware reload because the card is still > powered. I can't use this one on my device, as I don't know gpio num that wakes card (and I'm not sure if there's such gpio at all). > 3) bus sleep/power on bus power events: requires a firmware reload when > the bus comes back because the bus and card aren't powered, triggered by > some external suspend/resume mechanism > > 4) bus sleep/power on dev open/close: same as #3 except the trigger is > internal to the driver > > and it's getting pretty complicated in the code... You'll want to do > any of [2, 3, 4] depending on your platform, so that's really the only > difference. I'd think that we'd want to enable the dev open/close power > save stuff by default. > > We want deep sleep when we know that (a) the card will be powered > despite the bus or host being unpowered, and that's a platform decision. > I wonder if there's a better way to integrate all this stuff so it's > less confusing in the code? Deep sleep looks pretty much like power off, the only difference we do not need to upload fw to device/reconfigure it, so maybe we can unify them? I.e. use deep sleep if it's available in dev open/close, if it's not available - use set_power(), if nothing is available - do nothing. In suspend it makes sense to disable device at all if there's no WoL capability (or it's not implemented yet) > Dan Regards Vasily ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers 2011-04-07 20:25 ` [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers Vasily Khoruzhick 2011-04-07 22:35 ` Dan Williams @ 2011-04-08 9:10 ` Alberto Panizzo 2011-04-08 9:36 ` Vasily Khoruzhick 1 sibling, 1 reply; 12+ messages in thread From: Alberto Panizzo @ 2011-04-08 9:10 UTC (permalink / raw) To: Vasily Khoruzhick Cc: Dan Williams, John W. Linville, libertas-dev, linux-wireless Hi Vasily, On Thu, 2011-04-07 at 23:25 +0300, Vasily Khoruzhick wrote: > Add set_power callback, so driver can disable device when > interface is down or before going into suspend > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > --- > drivers/net/wireless/libertas/cmd.c | 18 ++++- > drivers/net/wireless/libertas/dev.h | 3 +- > drivers/net/wireless/libertas/main.c | 177 ++++++++++++++++++++++------------ > 3 files changed, 135 insertions(+), 63 deletions(-) > > diff --git a/drivers/net/wireless/libertas/cmd.c b/drivers/net/wireless/libertas/cmd.c > index 7e8a658..d0e7df6 100644 > --- a/drivers/net/wireless/libertas/cmd.c > +++ b/drivers/net/wireless/libertas/cmd.c > @@ -84,6 +84,7 @@ static u8 is_command_allowed_in_ps(u16 cmd) > int lbs_update_hw_spec(struct lbs_private *priv) > { > struct cmd_ds_get_hw_spec cmd; > + struct cmd_ds_802_11_mac_address hw_addr_cmd; > int ret = -1; > u32 i; > > @@ -151,8 +152,20 @@ int lbs_update_hw_spec(struct lbs_private *priv) > memcpy(priv->mesh_dev->dev_addr, > priv->current_addr, ETH_ALEN); > priv->copied_hwaddr = 1; > + } else { > + /* Copy addr back to hw */ > + memcpy(priv->current_addr, priv->dev->dev_addr, ETH_ALEN); > + hw_addr_cmd.hdr.size = cpu_to_le16(sizeof(hw_addr_cmd)); > + hw_addr_cmd.action = cpu_to_le16(CMD_ACT_SET); > + memcpy(hw_addr_cmd.macadd, priv->current_addr, ETH_ALEN); > + > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, > + &hw_addr_cmd); > + if (ret) > + lbs_deb_net("set MAC address failed\n"); Does this be really related to the subject of the patch? > } > > + And this new line? > out: > lbs_deb_leave(LBS_DEB_CMD); > return ret; > @@ -1111,6 +1124,7 @@ out: > > void lbs_set_mac_control(struct lbs_private *priv) > { > + int ret; > struct cmd_ds_mac_control cmd; > > lbs_deb_enter(LBS_DEB_CMD); > @@ -1119,7 +1133,9 @@ void lbs_set_mac_control(struct lbs_private *priv) > cmd.action = cpu_to_le16(priv->mac_control); > cmd.reserved = 0; > > - lbs_cmd_async(priv, CMD_MAC_CONTROL, &cmd.hdr, sizeof(cmd)); > + ret = lbs_cmd_with_response(priv, CMD_MAC_CONTROL, &cmd); > + if (ret) > + lbs_deb_net("set MAC control failed\n"); Same here. > > lbs_deb_leave(LBS_DEB_CMD); > } > diff --git a/drivers/net/wireless/libertas/dev.h b/drivers/net/wireless/libertas/dev.h > index bc461eb..9a873bc 100644 > --- a/drivers/net/wireless/libertas/dev.h > +++ b/drivers/net/wireless/libertas/dev.h > @@ -90,12 +90,13 @@ struct lbs_private { > void *card; > u8 fw_ready; > u8 surpriseremoved; > - u8 setup_fw_on_resume; > + int power_on_cnt; > int (*hw_host_to_card) (struct lbs_private *priv, u8 type, u8 *payload, u16 nb); > void (*reset_card) (struct lbs_private *priv); > int (*enter_deep_sleep) (struct lbs_private *priv); > int (*exit_deep_sleep) (struct lbs_private *priv); > int (*reset_deep_sleep_wakeup) (struct lbs_private *priv); > + void (*set_power) (struct lbs_private *priv, int enable); > > /* Adapter info (from EEPROM) */ > u32 fwrelease; > diff --git a/drivers/net/wireless/libertas/main.c b/drivers/net/wireless/libertas/main.c > index ca8149c..7922ead 100644 > --- a/drivers/net/wireless/libertas/main.c > +++ b/drivers/net/wireless/libertas/main.c > @@ -89,6 +89,73 @@ u8 lbs_data_rate_to_fw_index(u32 rate) > return 0; > } > > +/** > + * @brief This function gets the HW spec from the firmware and sets > + * some basic parameters. > + * > + * @param priv A pointer to struct lbs_private structure > + * @return 0 or -1 > + */ > +static int lbs_setup_firmware(struct lbs_private *priv) > +{ > + int ret = -1; > + s16 curlevel = 0, minlevel = 0, maxlevel = 0; > + > + lbs_deb_enter(LBS_DEB_FW); > + > + /* Read MAC address from firmware */ > + memset(priv->current_addr, 0xff, ETH_ALEN); > + ret = lbs_update_hw_spec(priv); > + if (ret) > + goto done; > + > + /* Read power levels if available */ > + ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); > + if (ret == 0) { > + priv->txpower_cur = curlevel; > + priv->txpower_min = minlevel; > + priv->txpower_max = maxlevel; > + } > + > + /* Send cmd to FW to enable 11D function */ > + ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); > + > + lbs_set_mac_control(priv); > +done: > + lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > + return ret; > + Same for this function > } > + > +/** > + * @brief This function enables device > + * > + * @param priv A pointer to struct lbs_private structure > + */ > +static void lbs_power_on(struct lbs_private *priv) > +{ > + if (priv->set_power) { > + priv->power_on_cnt++; > + if (priv->power_on_cnt == 1) { Here you have room for race conditions. power_on_cnt should be protected by a mutex. > + priv->set_power(priv, 1); > + lbs_setup_firmware(priv); Do you think that all users needs 8002.11D? > + lbs_init_mesh(priv); > + } > + } > +} Ah, now I understand why the previous.. > + > +/** > + * @brief This function disables device > + * > + * @param priv A pointer to struct lbs_private structure > + */ > +static void lbs_power_off(struct lbs_private *priv) > +{ > + if (priv->set_power) { > + priv->power_on_cnt--; > + if (priv->power_on_cnt == 0) Same here for race conditions. > + priv->set_power(priv, 0); > + } > +} > > /** > * @brief This function opens the ethX interface > @@ -103,6 +170,8 @@ static int lbs_dev_open(struct net_device *dev) > > lbs_deb_enter(LBS_DEB_NET); > > + lbs_power_on(priv); > + > spin_lock_irq(&priv->driver_lock); > priv->stopping = false; > > @@ -136,13 +205,14 @@ static int lbs_eth_stop(struct net_device *dev) > netif_stop_queue(dev); > spin_unlock_irq(&priv->driver_lock); > > - schedule_work(&priv->mcast_work); > cancel_delayed_work_sync(&priv->scan_work); > if (priv->scan_req) { > cfg80211_scan_done(priv->scan_req, false); > priv->scan_req = NULL; > } > > + lbs_power_off(priv); > + > lbs_deb_leave(LBS_DEB_NET); > return 0; > } > @@ -201,15 +271,16 @@ int lbs_set_mac_address(struct net_device *dev, void *addr) > > /* In case it was called from the mesh device */ > dev = priv->dev; > + if (priv->power_on_cnt) { Same here, you need at least a read lock. > + cmd.hdr.size = cpu_to_le16(sizeof(cmd)); > + cmd.action = cpu_to_le16(CMD_ACT_SET); > + memcpy(cmd.macadd, phwaddr->sa_data, ETH_ALEN); > > - cmd.hdr.size = cpu_to_le16(sizeof(cmd)); > - cmd.action = cpu_to_le16(CMD_ACT_SET); > - memcpy(cmd.macadd, phwaddr->sa_data, ETH_ALEN); > - > - ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, &cmd); > - if (ret) { > - lbs_deb_net("set MAC address failed\n"); > - goto done; > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, &cmd); > + if (ret) { > + lbs_deb_net("set MAC address failed\n"); > + goto done; > + } > } > > memcpy(priv->current_addr, phwaddr->sa_data, ETH_ALEN); > @@ -539,59 +610,27 @@ static int lbs_thread(void *data) > return 0; > } > > -/** > - * @brief This function gets the HW spec from the firmware and sets > - * some basic parameters. > - * > - * @param priv A pointer to struct lbs_private structure > - * @return 0 or -1 > - */ > -static int lbs_setup_firmware(struct lbs_private *priv) > -{ > - int ret = -1; > - s16 curlevel = 0, minlevel = 0, maxlevel = 0; > - > - lbs_deb_enter(LBS_DEB_FW); > - > - /* Read MAC address from firmware */ > - memset(priv->current_addr, 0xff, ETH_ALEN); > - ret = lbs_update_hw_spec(priv); > - if (ret) > - goto done; > - > - /* Read power levels if available */ > - ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); > - if (ret == 0) { > - priv->txpower_cur = curlevel; > - priv->txpower_min = minlevel; > - priv->txpower_max = maxlevel; > - } > - > - /* Send cmd to FW to enable 11D function */ > - ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); > - > - lbs_set_mac_control(priv); > -done: > - lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > - return ret; > -} > - > int lbs_suspend(struct lbs_private *priv) > { > - int ret; > + int ret = 0; > > lbs_deb_enter(LBS_DEB_FW); > > - if (priv->is_deep_sleep) { > - ret = lbs_set_deep_sleep(priv, 0); > - if (ret) { > - lbs_pr_err("deep sleep cancellation failed: %d\n", ret); > - return ret; > + if (priv->set_power) > + lbs_power_off(priv); Do you really want to power off the chip on suspend only because set_power exist? Is this something that should be driven by userspace? And don't you want to setup any wake conditions? > + else { > + if (priv->is_deep_sleep) { > + ret = lbs_set_deep_sleep(priv, 0); > + if (ret) { > + lbs_pr_err( > + "deep sleep cancellation failed: %d\n", ret); > + return ret; > + } > + priv->deep_sleep_required = 1; > } > - priv->deep_sleep_required = 1; > - } > > - ret = lbs_set_host_sleep(priv, 1); > + ret = lbs_set_host_sleep(priv, 1); > + } > > netif_device_detach(priv->dev); > if (priv->mesh_dev) > @@ -604,26 +643,26 @@ EXPORT_SYMBOL_GPL(lbs_suspend); > > int lbs_resume(struct lbs_private *priv) > { > - int ret; > + int ret = 0; > > lbs_deb_enter(LBS_DEB_FW); > > - ret = lbs_set_host_sleep(priv, 0); > + if (priv->set_power) > + lbs_power_on(priv); > + else > + ret = lbs_set_host_sleep(priv, 0); > > netif_device_attach(priv->dev); > if (priv->mesh_dev) > netif_device_attach(priv->mesh_dev); > > - if (priv->deep_sleep_required) { > + if (!priv->set_power && priv->deep_sleep_required) { > priv->deep_sleep_required = 0; > ret = lbs_set_deep_sleep(priv, 1); > if (ret) > lbs_pr_err("deep sleep activation failed: %d\n", ret); > } > > - if (priv->setup_fw_on_resume) > - ret = lbs_setup_firmware(priv); > - > lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > return ret; > } > @@ -917,6 +956,9 @@ void lbs_remove_card(struct lbs_private *priv) > priv->surpriseremoved = 1; > kthread_stop(priv->main_thread); > > + /* Disable card power if it's still on */ > + lbs_power_off(priv); > + > lbs_free_adapter(priv); > lbs_cfg_free(priv); > free_netdev(dev); > @@ -944,6 +986,16 @@ int lbs_start_card(struct lbs_private *priv) > > lbs_deb_enter(LBS_DEB_MAIN); > > + /* We're not using lbs_power_on here because we don't want > + * to setup firmware twice. > + * TODO: replace following code with lbs_power_on() when set_power > + * callback become mandatory. > + */ > + if (priv->set_power) { > + priv->power_on_cnt++; > + priv->set_power(priv, 1); > + } > + > /* poke the firmware */ > ret = lbs_setup_firmware(priv); > if (ret) > @@ -964,6 +1016,9 @@ int lbs_start_card(struct lbs_private *priv) > > ret = 0; > > + /* Init is done, so we can disable card power */ > + lbs_power_off(priv); > + > done: > lbs_deb_leave_args(LBS_DEB_MAIN, "ret %d", ret); > return ret; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers 2011-04-08 9:10 ` Alberto Panizzo @ 2011-04-08 9:36 ` Vasily Khoruzhick 2011-04-08 10:23 ` Alberto Panizzo 0 siblings, 1 reply; 12+ messages in thread From: Vasily Khoruzhick @ 2011-04-08 9:36 UTC (permalink / raw) To: Alberto Panizzo Cc: Dan Williams, John W. Linville, libertas-dev, linux-wireless On Friday 08 April 2011 12:10:53 Alberto Panizzo wrote: > > + } else { > > + /* Copy addr back to hw */ > > + memcpy(priv->current_addr, priv->dev->dev_addr, ETH_ALEN); > > + hw_addr_cmd.hdr.size = cpu_to_le16(sizeof(hw_addr_cmd)); > > + hw_addr_cmd.action = cpu_to_le16(CMD_ACT_SET); > > + memcpy(hw_addr_cmd.macadd, priv->current_addr, ETH_ALEN); > > + > > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, > > + &hw_addr_cmd); > > + if (ret) > > + lbs_deb_net("set MAC address failed\n"); > > Does this be really related to the subject of the patch? Yep, this function is called right after power on, and we have to copy mac address back to hardware (in case it was changed). > > - lbs_cmd_async(priv, CMD_MAC_CONTROL, &cmd.hdr, sizeof(cmd)); > > + ret = lbs_cmd_with_response(priv, CMD_MAC_CONTROL, &cmd); > > + if (ret) > > + lbs_deb_net("set MAC control failed\n"); > > Same here. I changed cmd to be sync to be sure that there's no host<->card activity during power down. Otherwise this command timedout. > > +/** > > + * @brief This function gets the HW spec from the firmware and sets > > + * some basic parameters. > > + * > > + * @param priv A pointer to struct lbs_private structure > > + * @return 0 or -1 > > + */ > > +static int lbs_setup_firmware(struct lbs_private *priv) > > +{ > > + int ret = -1; > > + s16 curlevel = 0, minlevel = 0, maxlevel = 0; > > + > > + lbs_deb_enter(LBS_DEB_FW); > > + > > + /* Read MAC address from firmware */ > > + memset(priv->current_addr, 0xff, ETH_ALEN); > > + ret = lbs_update_hw_spec(priv); > > + if (ret) > > + goto done; > > + > > + /* Read power levels if available */ > > + ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); > > + if (ret == 0) { > > + priv->txpower_cur = curlevel; > > + priv->txpower_min = minlevel; > > + priv->txpower_max = maxlevel; > > + } > > + > > + /* Send cmd to FW to enable 11D function */ > > + ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); > > + > > + lbs_set_mac_control(priv); > > +done: > > + lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > > + return ret; > > + > > Same for this function Moved it to avoid forward declatation. > > +static void lbs_power_on(struct lbs_private *priv) > > +{ > > + if (priv->set_power) { > > + priv->power_on_cnt++; > > + if (priv->power_on_cnt == 1) { > > Here you have room for race conditions. power_on_cnt should be > protected by a mutex. I'm pretty sure that iface open/close (up/down?) is protected with mutex. So why bother here? > > + priv->set_power(priv, 1); > > + lbs_setup_firmware(priv); > > Do you think that all users needs 8002.11D? It does nothing if mesh is not enabled in config. > > @@ -201,15 +271,16 @@ int lbs_set_mac_address(struct net_device *dev, > > void *addr) > > > > /* In case it was called from the mesh device */ > > dev = priv->dev; > > > > + if (priv->power_on_cnt) { > > Same here, you need at least a read lock. In this place there's no iface yet (we're still in probe callback of driver). lbs_power_on/lbs_power_off are called only on iface up/down and in suspend/resume handlers (at least, for now), so we can't get race condition here. > > int lbs_suspend(struct lbs_private *priv) > > { > > > > - int ret; > > + int ret = 0; > > > > lbs_deb_enter(LBS_DEB_FW); > > > > - if (priv->is_deep_sleep) { > > - ret = lbs_set_deep_sleep(priv, 0); > > - if (ret) { > > - lbs_pr_err("deep sleep cancellation failed: %d\n", ret); > > - return ret; > > + if (priv->set_power) > > + lbs_power_off(priv); > > Do you really want to power off the chip on suspend only because > set_power exist? > Is this something that should be driven by userspace? > > And don't you want to setup any wake conditions? Unfortunately there's no datasheet on libertas chip. And looks like there was no WOL support in driver, so why keep it enabled? Thanks for review! Regards Vasily ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers 2011-04-08 9:36 ` Vasily Khoruzhick @ 2011-04-08 10:23 ` Alberto Panizzo 2011-04-08 11:30 ` Vasily Khoruzhick 0 siblings, 1 reply; 12+ messages in thread From: Alberto Panizzo @ 2011-04-08 10:23 UTC (permalink / raw) To: Vasily Khoruzhick Cc: Dan Williams, John W. Linville, libertas-dev, linux-wireless On Fri, 2011-04-08 at 12:36 +0300, Vasily Khoruzhick wrote: > On Friday 08 April 2011 12:10:53 Alberto Panizzo wrote: > > > + } else { > > > + /* Copy addr back to hw */ > > > + memcpy(priv->current_addr, priv->dev->dev_addr, ETH_ALEN); > > > + hw_addr_cmd.hdr.size = cpu_to_le16(sizeof(hw_addr_cmd)); > > > + hw_addr_cmd.action = cpu_to_le16(CMD_ACT_SET); > > > + memcpy(hw_addr_cmd.macadd, priv->current_addr, ETH_ALEN); > > > + > > > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, > > > + &hw_addr_cmd); > > > + if (ret) > > > + lbs_deb_net("set MAC address failed\n"); > > > > Does this be really related to the subject of the patch? > > Yep, this function is called right after power on, and we have to copy mac > address back to hardware (in case it was changed). So, are you assuming that someone can change the wifi card while this is powered off? > > > > - lbs_cmd_async(priv, CMD_MAC_CONTROL, &cmd.hdr, sizeof(cmd)); > > > + ret = lbs_cmd_with_response(priv, CMD_MAC_CONTROL, &cmd); > > > + if (ret) > > > + lbs_deb_net("set MAC control failed\n"); > > > > Same here. > > I changed cmd to be sync to be sure that there's no host<->card activity > during power down. Otherwise this command timedout. > > > > +/** > > > + * @brief This function gets the HW spec from the firmware and sets > > > + * some basic parameters. > > > + * > > > + * @param priv A pointer to struct lbs_private structure > > > + * @return 0 or -1 > > > + */ > > > +static int lbs_setup_firmware(struct lbs_private *priv) > > > +{ > > > + int ret = -1; > > > + s16 curlevel = 0, minlevel = 0, maxlevel = 0; > > > + > > > + lbs_deb_enter(LBS_DEB_FW); > > > + > > > + /* Read MAC address from firmware */ > > > + memset(priv->current_addr, 0xff, ETH_ALEN); > > > + ret = lbs_update_hw_spec(priv); > > > + if (ret) > > > + goto done; > > > + > > > + /* Read power levels if available */ > > > + ret = lbs_get_tx_power(priv, &curlevel, &minlevel, &maxlevel); > > > + if (ret == 0) { > > > + priv->txpower_cur = curlevel; > > > + priv->txpower_min = minlevel; > > > + priv->txpower_max = maxlevel; > > > + } > > > + > > > + /* Send cmd to FW to enable 11D function */ > > > + ret = lbs_set_snmp_mib(priv, SNMP_MIB_OID_11D_ENABLE, 1); > > > + > > > + lbs_set_mac_control(priv); > > > +done: > > > + lbs_deb_leave_args(LBS_DEB_FW, "ret %d", ret); > > > + return ret; > > > + > > > > Same for this function > > Moved it to avoid forward declatation. Mmm, personally I like more the forward declaration.. the moving do not allow readers if you have modified something in the function. > > > > +static void lbs_power_on(struct lbs_private *priv) > > > +{ > > > + if (priv->set_power) { > > > + priv->power_on_cnt++; > > > + if (priv->power_on_cnt == 1) { > > > > Here you have room for race conditions. power_on_cnt should be > > protected by a mutex. > > I'm pretty sure that iface open/close (up/down?) is protected with mutex. So > why bother here? > > > > + priv->set_power(priv, 1); > > > + lbs_setup_firmware(priv); > > > > Do you think that all users needs 8002.11D? > > It does nothing if mesh is not enabled in config. > > > > @@ -201,15 +271,16 @@ int lbs_set_mac_address(struct net_device *dev, > > > void *addr) > > > > > > /* In case it was called from the mesh device */ > > > dev = priv->dev; > > > > > > + if (priv->power_on_cnt) { > > > > Same here, you need at least a read lock. > > In this place there's no iface yet (we're still in probe callback of driver). > > lbs_power_on/lbs_power_off are called only on iface up/down and in > suspend/resume handlers (at least, for now), so we can't get race condition > here. Ok, I understand what you are doing. It's the same as removing the module when the interface is not used.. I think this driver needs a good documentation of software flows: You are adding a mechanism that is really useful for saving power while the wlan interface is not used. But this should not be the same as the policy this driver use through suspensions. I know that at this moment, this driver is not managing Power Save very well and ok, this allow you to give up and turn off the card during suspension. > > > > int lbs_suspend(struct lbs_private *priv) > > > { > > > > > > - int ret; > > > + int ret = 0; > > > > > > lbs_deb_enter(LBS_DEB_FW); > > > > > > - if (priv->is_deep_sleep) { > > > - ret = lbs_set_deep_sleep(priv, 0); > > > - if (ret) { > > > - lbs_pr_err("deep sleep cancellation failed: %d\n", ret); > > > - return ret; > > > + if (priv->set_power) > > > + lbs_power_off(priv); > > > > Do you really want to power off the chip on suspend only because > > set_power exist? > > Is this something that should be driven by userspace? > > > > And don't you want to setup any wake conditions? > > Unfortunately there's no datasheet on libertas chip. And looks like there was > no WOL support in driver, so why keep it enabled? Look for "CMD_802_11_HOST_SLEEP_CFG pdf" in google, you will be excited on the results..! > > Thanks for review! > > Regards > Vasily ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers 2011-04-08 10:23 ` Alberto Panizzo @ 2011-04-08 11:30 ` Vasily Khoruzhick 0 siblings, 0 replies; 12+ messages in thread From: Vasily Khoruzhick @ 2011-04-08 11:30 UTC (permalink / raw) To: Alberto Panizzo Cc: Dan Williams, John W. Linville, libertas-dev, linux-wireless On Friday 08 April 2011 13:23:14 Alberto Panizzo wrote: > On Fri, 2011-04-08 at 12:36 +0300, Vasily Khoruzhick wrote: > > On Friday 08 April 2011 12:10:53 Alberto Panizzo wrote: > > > > + } else { > > > > + /* Copy addr back to hw */ > > > > + memcpy(priv->current_addr, priv->dev->dev_addr, ETH_ALEN); > > > > + hw_addr_cmd.hdr.size = cpu_to_le16(sizeof(hw_addr_cmd)); > > > > + hw_addr_cmd.action = cpu_to_le16(CMD_ACT_SET); > > > > + memcpy(hw_addr_cmd.macadd, priv->current_addr, ETH_ALEN); > > > > + > > > > + ret = lbs_cmd_with_response(priv, CMD_802_11_MAC_ADDRESS, > > > > + &hw_addr_cmd); > > > > + if (ret) > > > > + lbs_deb_net("set MAC address failed\n"); > > > > > > Does this be really related to the subject of the patch? > > > > Yep, this function is called right after power on, and we have to copy > > mac address back to hardware (in case it was changed). > > So, are you assuming that someone can change the wifi card while this > is powered off? Nope, I assume someone can do 'ifconfig wlan0 hw ether xx:xx:xx:xx:xx:xx' while card is powered down. Or when it's powered up - we need then to load choosen mac into hw back after power cycle sequence. > > Moved it to avoid forward declatation. > > Mmm, personally I like more the forward declaration.. the moving do not > allow readers if you have modified something in the function. Depends on person, someone hates forward declarations, someone likes them. I have nothing against them, but most part of kernel maintainers do not like them for some reason. > > In this place there's no iface yet (we're still in probe callback of > > driver). > > > > lbs_power_on/lbs_power_off are called only on iface up/down and in > > suspend/resume handlers (at least, for now), so we can't get race > > condition here. > > Ok, I understand what you are doing. It's the same as removing the > module when the interface is not used.. Not same, wlan0 is still here :) > I think this driver needs a good documentation of software flows: You > are adding a mechanism that is really useful for saving power while the > wlan interface is not used. But this should not be the same as the > policy this driver use through suspensions. Hmm, driver is expected to disable hw or put it into powersaving mode in .ndo_stop(). It's already documented, isn't it? > I know that at this moment, this driver is not managing Power Save very > well and ok, this allow you to give up and turn off the card during > suspension. Host goes suspend (i.e. bus is not active anymore), and there's no way to put device into deep sleep mode in some cases (I'm not sure if gpio to wake card wired somewhere on my device :)), so looks like there's no choice. > Look for "CMD_802_11_HOST_SLEEP_CFG pdf" in google, you will be excited > on the results..! Thanks, will take a look on fw datasheet. Regards Vasily ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH RFC 3/3] libertas_spi: introduce set_power callback 2011-04-07 20:25 [PATCH RFC 0/3] libertas: powersave fixes Vasily Khoruzhick 2011-04-07 20:25 ` [PATCH RFC 1/3] libertas_spi: cancel packet work on module removal Vasily Khoruzhick 2011-04-07 20:25 ` [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers Vasily Khoruzhick @ 2011-04-07 20:26 ` Vasily Khoruzhick 2 siblings, 0 replies; 12+ messages in thread From: Vasily Khoruzhick @ 2011-04-07 20:26 UTC (permalink / raw) To: Dan Williams, John W. Linville, libertas-dev, linux-wireless, anarsoul Introduce set_power callback for spi cards, it makes driver a bit powersave friendlier. Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> --- drivers/net/wireless/libertas/if_spi.c | 54 ++++++++++++------------------- 1 files changed, 21 insertions(+), 33 deletions(-) diff --git a/drivers/net/wireless/libertas/if_spi.c b/drivers/net/wireless/libertas/if_spi.c index d893560..1e629b3 100644 --- a/drivers/net/wireless/libertas/if_spi.c +++ b/drivers/net/wireless/libertas/if_spi.c @@ -1068,18 +1068,25 @@ static void if_spi_resume_worker(struct work_struct *work) card = container_of(work, struct if_spi_card, resume_work); if (card->suspended) { - if (card->pdata->setup) - card->pdata->setup(card->spi); + lbs_resume(card->priv); - /* Init card ... */ - if_spi_init_card(card); + card->suspended = 0; + } +} - enable_irq(card->spi->irq); +static void if_spi_set_power(struct lbs_private *priv, int enable) +{ + struct if_spi_card *card = priv->card; - /* And resume it ... */ - lbs_resume(card->priv); + if (enable) { + card->pdata->setup(card->spi); + if_spi_init_card(card); + enable_irq(card->spi->irq); - card->suspended = 0; + } else { + flush_workqueue(card->workqueue); + disable_irq(card->spi->irq); + card->pdata->teardown(card->spi); } } @@ -1097,17 +1104,11 @@ static int __devinit if_spi_probe(struct spi_device *spi) goto out; } - if (pdata->setup) { - err = pdata->setup(spi); - if (err) - goto out; - } - /* Allocate card structure to represent this specific device */ card = kzalloc(sizeof(struct if_spi_card), GFP_KERNEL); if (!card) { err = -ENOMEM; - goto teardown; + goto out; } spi_set_drvdata(spi, card); card->pdata = pdata; @@ -1118,13 +1119,6 @@ static int __devinit if_spi_probe(struct spi_device *spi) INIT_LIST_HEAD(&card->data_packet_list); spin_lock_init(&card->buffer_lock); - /* Initialize the SPI Interface Unit */ - - /* Firmware load */ - err = if_spi_init_card(card); - if (err) - goto free_card; - /* Register our card with libertas. * This will call alloc_etherdev */ priv = lbs_add_card(card, &spi->dev); @@ -1133,9 +1127,10 @@ static int __devinit if_spi_probe(struct spi_device *spi) goto free_card; } card->priv = priv; - priv->setup_fw_on_resume = 1; priv->card = card; priv->hw_host_to_card = if_spi_host_to_card; + if (pdata->setup && pdata->teardown) + priv->set_power = if_spi_set_power; priv->enter_deep_sleep = NULL; priv->exit_deep_sleep = NULL; priv->reset_deep_sleep_wakeup = NULL; @@ -1153,6 +1148,9 @@ static int __devinit if_spi_probe(struct spi_device *spi) goto terminate_workqueue; } + /* Disable IRQ, hw is not ready yet */ + disable_irq(spi->irq); + /* Start the card. * This will call register_netdev, and we'll start * getting interrupts... */ @@ -1173,9 +1171,6 @@ terminate_workqueue: lbs_remove_card(priv); /* will call free_netdev */ free_card: free_if_spi_card(card); -teardown: - if (pdata->teardown) - pdata->teardown(spi); out: lbs_deb_leave_args(LBS_DEB_SPI, "err %d\n", err); return err; @@ -1198,8 +1193,6 @@ static int __devexit libertas_spi_remove(struct spi_device *spi) cancel_work_sync(&card->packet_work); flush_workqueue(card->workqueue); destroy_workqueue(card->workqueue); - if (card->pdata->teardown) - card->pdata->teardown(spi); free_if_spi_card(card); lbs_deb_leave(LBS_DEB_SPI); return 0; @@ -1212,11 +1205,6 @@ static int if_spi_suspend(struct device *dev) if (!card->suspended) { lbs_suspend(card->priv); - flush_workqueue(card->workqueue); - disable_irq(spi->irq); - - if (card->pdata->teardown) - card->pdata->teardown(spi); card->suspended = 1; } -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-04-08 11:44 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-07 20:25 [PATCH RFC 0/3] libertas: powersave fixes Vasily Khoruzhick 2011-04-07 20:25 ` [PATCH RFC 1/3] libertas_spi: cancel packet work on module removal Vasily Khoruzhick 2011-04-07 21:43 ` Dan Williams 2011-04-07 20:25 ` [PATCH RFC 2/3] libertas: add set_power callback for iface subdrivers Vasily Khoruzhick 2011-04-07 22:35 ` Dan Williams 2011-04-08 9:54 ` Alberto Panizzo 2011-04-08 11:42 ` Vasily Khoruzhick 2011-04-08 9:10 ` Alberto Panizzo 2011-04-08 9:36 ` Vasily Khoruzhick 2011-04-08 10:23 ` Alberto Panizzo 2011-04-08 11:30 ` Vasily Khoruzhick 2011-04-07 20:26 ` [PATCH RFC 3/3] libertas_spi: introduce set_power callback Vasily Khoruzhick
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).