Re: [PATCH v2 2/2] ath10k: add testmode

Michal Kazior [off-list ref] writes:

On 28 August 2014 09:30, Kalle Valo [off-list ref] wrote:

quoted

@@ -2483,8 +2484,12 @@ static int ath10k_start(struct ieee80211_hw *hw)
        case ATH10K_STATE_RESTARTED:
        case ATH10K_STATE_WEDGED:
                WARN_ON(1);
+               /* fall through */

Fall through? I see there's a goto ahead..

quoted

                ret = -EINVAL;
                goto err;

Fixed.

quoted

+/* "API" level of the ath10k testmode interface. Bump it after every
+ * incompatible interface change. */

I recall we reached a conclusion to use the following multiline
comment style in ath10k:

/* Foo
 * Bar
 */

Ah, here it is: http://www.spinics.net/lists/linux-wireless/msg123419.html

But this isn't really important.

I fixed all the multiline comments now.

And I did remember that we agreed about this but I just forgot how it's
supposed to look :) To be able to reasily refresh my memory I documented
this to the wiki:

http://wireless.kernel.org/en/users/Drivers/ath10k/CodingStyle#Comments

quoted

+       if (ar->testmode.utf != NULL)
+               /* utf image is already downloaded */
+               goto power_up;
+
+

2 empty newlines.

Fixed.

quoted

+       snprintf(filename, sizeof(filename), "%s/%s",
+                ar->hw_params.fw.dir, ATH10K_FW_UTF_FILE);
+
+       /* load utf image now and release only when the device is removed */
+       ret = request_firmware(&ar->testmode.utf, filename, ar->dev);
+       if (ret) {
+               ath10k_warn(ar, "failed to retrieve utf firmware '%s': %d\n",
+                           filename, ret);
+               goto out;
+       }
+
+       BUILD_BUG_ON(sizeof(ar->fw_features) !=
+                    sizeof(ar->testmode.orig_fw_features));
+

quoted

+       memcpy(ar->testmode.orig_fw_features, ar->fw_features,
+              sizeof(ar->fw_features));
+
+       /* force to use correct version of WMI interface */
+       memset(ar->fw_features, 0, sizeof(ar->fw_features));
+       __set_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features);

I'm a little worried about this.

Yeah, this is an ugly hack. But at the same time I don't see much point
of changing the rest of driver just because of testmode.

First of all the comment doesn't really explain why this is done, i.e.
utf.bin isn't FW IE encapsulated and as such fw_features are unknown.

I'll add that.

And then what about utf binaries which use 10.2 WMI? Or non-10.x WMI?

Right now I don't see any use of that. Think of this like FW API 1 where
we don't have any IE support.

quoted

+power_up:
+       spin_lock_bh(&ar->data_lock);
+
+       ar->testmode.utf_monitor = true;
+
+       spin_unlock_bh(&ar->data_lock);
+
+       ret = ath10k_hif_power_up(ar);
+       if (ret) {
+               ath10k_err(ar, "failed to power up hif (testmode): %d\n", ret);
+               ar->state = ATH10K_STATE_OFF;
+               goto out;
+       }
+
+       ret = ath10k_core_start(ar, ATH10K_FIRMWARE_MODE_UTF);
+       if (ret) {
+               ath10k_err(ar, "failed to start core (testmode): %d\n", ret);
+               ath10k_hif_power_down(ar);
+               ar->state = ATH10K_STATE_OFF;
+               goto out;
+       }

You don't seem to be freeing firmware on failure here. I'm aware
ath10k_testmode_destroy() takes care of it but if you happen to use a
defect fw (or hit some kind of a fw-hw combination bug) each
subsequent utf start will re-use the firmware binary - it won't
re-read the binary (giving you a chance to replace utf binary with a
different one) unless you reload the whole module or re-bind the
device<->driver via sysfs or physically.

That's a clear bug, will fix.

quoted

+static int ath10k_tm_cmd_wmi(struct ath10k *ar, struct nlattr *tb[])
+{
+       struct sk_buff *skb;
+       int ret, buf_len;
+       u32 cmd_id;
+       void *buf;
+
+       mutex_lock(&ar->conf_mutex);
+
+       if (ar->state != ATH10K_STATE_UTF) {
+               ret = EHOSTDOWN;

Shouldn't this be a negative number?

Fixed.

Also ath10k_tm_cmd_utf_stop() uses -ENETDOWN for this case.

I changed this to -ENETDOWN also.

-- 
Kalle Valo