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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
