Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190210195217.18817-1-lkundrak@v3.sk> <20190210195217.18817-3-lkundrak@v3.sk>
In-Reply-To: <20190210195217.18817-3-lkundrak@v3.sk>
From:   Steve deRosier <steve.derosier@gmail.com>
Date:   Mon, 11 Feb 2019 06:10:49 -0800
Message-ID: <CALLGbRJFFeZjLmUvm58xu2KsQ+Vx+TysZaBRfT7MqoSD6T_y-Q@mail.gmail.com>
Subject: Re: [PATCH 2/3] libertas_tf: don't defer firmware loading until start()
To:     Lubomir Rintel <lkundrak@v3.sk>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Feb 10, 2019 at 11:52 AM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> In order to be able to get a MAC address before we register the device
> with ieee80211 we'll need to load the firmware way earlier.
>
> There seems to be one problem with this: the device seems to start
> with radio enabled and starts sending in frames right after the firmware
> load finishes. This might be a firmware bug. Disable the radio as soon
> as possible.
>

I've got a quibble with calling the behavior a bug. As far as I
remember, it's behaving as designed/specified and was quite
appropriate back when this driver originally went upstream. This is a
10 year old fw and driver and needs have changed - which doesn't mean
it was wrong to do originally.

Now, I'm not saying there's no bugs in the fw. Nor that it wouldn't be
nice to change the behavior to accommodate this change. If I still had
access to the firmware source I even might do so.

...
> @@ -648,6 +642,19 @@ struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
>
>         INIT_WORK(&priv->cmd_work, lbtf_cmd_work);
>         INIT_WORK(&priv->tx_work, lbtf_tx_work);
> +
> +       if (priv->ops->hw_prog_firmware(priv)) {
> +               lbtf_deb_usbd(&udev->dev, "Error programming the firmware\n");
> +               priv->ops->hw_reset_device(priv);
> +               goto err_init_adapter;
> +       }
> +
> +       /* The firmware seems to start with the radio enabled. Turn it
> +        * off before an actual mac80211 start callback is invoked.
> +        */
> +       priv->radioon = RADIO_OFF;

Maybe move this up a few lines to before you program the fw? Seems
appropriate to initialize it first. While I expect there's no chance
of a race as the
mac80211 start callback hasn't been invoked yet, superficially it
looks like there could be.


> +       lbtf_set_radio_control(priv);
> +
>         if (ieee80211_register_hw(hw))
>                 goto err_init_adapter;
>
> --
> 2.20.1
>

Reviewed-by: Steve deRosier <derosier@cal-sierra.com>