Re: [PATCH v2 2/2] ath10k: add testmode
On 08/29/14 08:03, Kalle Valo wrote: Arend van Spriel writes: On 08/28/14 10:02, Kalle Valo wrote: Kalle Valo writes: Johannes suggested to put this to a separate file as that way it's easier for the user space. In v3 I'm planning to create testmode_uapi.h for this. I suppose that file will/should end up in include/uapi/... I was thinking not to put this to the include directory. This is just a testmode interface used only by few people, not a proper driver interface. I see. In that case I would avoid the term 'uapi'. I think it will impose certain expectations. so wouldn't it be better to call it ath10k_testmode.h? We already have testmode.h so having ath10k_testmode.h in the same directory would be confusing. Would testmode_i.h be any better? What does it contain? Looks like command and attribute definitions for your testmode support. Maybe testmode_defs.h? As long as it is not uapi. Regards, Arend ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: add testmode
Michal Kazior writes:
> On 28 August 2014 09:30, Kalle Valo wrote:
>> @@ -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..
>> ret = -EINVAL;
>> goto err;
Fixed.
>> +/* "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
>> + if (ar->testmode.utf != NULL)
>> + /* utf image is already downloaded */
>> + goto power_up;
>> +
>> +
>
> 2 empty newlines.
Fixed.
>> + 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));
>> +
>
>> + 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.
>> +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.
>> +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
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: add testmode
Arend van Spriel writes: > On 08/28/14 10:02, Kalle Valo wrote: >> Kalle Valo writes: > >> Johannes suggested to put this to a separate file as that way it's >> easier for the user space. In v3 I'm planning to create testmode_uapi.h >> for this. > > I suppose that file will/should end up in include/uapi/... I was thinking not to put this to the include directory. This is just a testmode interface used only by few people, not a proper driver interface. > so wouldn't it be better to call it ath10k_testmode.h? We already have testmode.h so having ath10k_testmode.h in the same directory would be confusing. Would testmode_i.h be any better? -- Kalle Valo ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: add testmode
On 28 August 2014 09:30, Kalle Valo wrote:
> Add testmode interface for starting and using UTF firmware which is used to
> run
> factory tests. This is implemented by adding new state ATH10K_STATE_UTF and
> user
> space can enable this state with ATH10K_TM_CMD_UTF_START command. To go back
> to
> normal mode user space can send ATH10K_TM_CMD_UTF_STOP.
>
> Signed-off-by: Kalle Valo
> ---
> drivers/net/wireless/ath/ath10k/Makefile |1
> drivers/net/wireless/ath/ath10k/core.c | 88 --
> drivers/net/wireless/ath/ath10k/core.h | 22 +-
> drivers/net/wireless/ath/ath10k/debug.c|5
> drivers/net/wireless/ath/ath10k/debug.h|1
> drivers/net/wireless/ath/ath10k/hw.h |2
> drivers/net/wireless/ath/ath10k/mac.c | 10 +
> drivers/net/wireless/ath/ath10k/testmode.c | 404
>
> drivers/net/wireless/ath/ath10k/testmode.h | 46 +++
> drivers/net/wireless/ath/ath10k/wmi.c | 17 +
> 10 files changed, 566 insertions(+), 30 deletions(-)
> create mode 100644 drivers/net/wireless/ath/ath10k/testmode.c
> create mode 100644 drivers/net/wireless/ath/ath10k/testmode.h
>
> diff --git a/drivers/net/wireless/ath/ath10k/Makefile
> b/drivers/net/wireless/ath/ath10k/Makefile
> index 2cfb63ca9327..8b1b1adb477a 100644
> --- a/drivers/net/wireless/ath/ath10k/Makefile
> +++ b/drivers/net/wireless/ath/ath10k/Makefile
> @@ -11,6 +11,7 @@ ath10k_core-y += mac.o \
> bmi.o
>
> ath10k_core-$(CONFIG_ATH10K_DEBUGFS) += spectral.o
> +ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o
> ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o
>
> obj-$(CONFIG_ATH10K_PCI) += ath10k_pci.o
> diff --git a/drivers/net/wireless/ath/ath10k/core.c
> b/drivers/net/wireless/ath/ath10k/core.c
> index 651a6da8adf5..c01a37526ad5 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -26,6 +26,7 @@
> #include "bmi.h"
> #include "debug.h"
> #include "htt.h"
> +#include "testmode.h"
>
> unsigned int ath10k_debug_mask;
> static bool uart_print;
> @@ -257,21 +258,42 @@ static int ath10k_download_and_run_otp(struct ath10k
> *ar)
> return 0;
> }
>
> -static int ath10k_download_fw(struct ath10k *ar)
> +static int ath10k_download_fw(struct ath10k *ar, enum ath10k_firmware_mode
> mode)
> {
> - u32 address;
> + u32 address, data_len;
> + const char *mode_name;
> + const void *data;
> int ret;
>
> address = ar->hw_params.patch_load_addr;
>
> - ret = ath10k_bmi_fast_download(ar, address, ar->firmware_data,
> - ar->firmware_len);
> + switch (mode) {
> + case ATH10K_FIRMWARE_MODE_NORMAL:
> + data = ar->firmware_data;
> + data_len = ar->firmware_len;
> + mode_name = "normal";
> + break;
> + case ATH10K_FIRMWARE_MODE_UTF:
> + data = ar->testmode.utf->data;
> + data_len = ar->testmode.utf->size;
> + mode_name = "utf";
> + break;
> + default:
> + ath10k_err(ar, "unknown firmware mode: %d\n", mode);
> + return -EINVAL;
> + }
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
> + "boot uploading firmware image %p len %d mode %s\n",
> + data, data_len, mode_name);
> +
> + ret = ath10k_bmi_fast_download(ar, address, data, data_len);
> if (ret) {
> - ath10k_err(ar, "could not write fw (%d)\n", ret);
> - goto exit;
> + ath10k_err(ar, "failed to download %s firmware: %d\n",
> + mode_name, ret);
> + return ret;
> }
>
> -exit:
> return ret;
> }
>
> @@ -567,7 +589,8 @@ success:
> return 0;
> }
>
> -static int ath10k_init_download_firmware(struct ath10k *ar)
> +static int ath10k_init_download_firmware(struct ath10k *ar,
> +enum ath10k_firmware_mode mode)
> {
> int ret;
>
> @@ -583,7 +606,7 @@ static int ath10k_init_download_firmware(struct ath10k
> *ar)
> return ret;
> }
>
> - ret = ath10k_download_fw(ar);
> + ret = ath10k_download_fw(ar, mode);
> if (ret) {
> ath10k_err(ar, "failed to download firmware: %d\n", ret);
> return ret;
> @@ -685,12 +708,15 @@ static void ath10k_core_restart(struct work_struct
> *work)
> case ATH10K_STATE_WEDGED:
> ath10k_warn(ar, "device is wedged, will not restart\n");
> break;
> + case ATH10K_STATE_UTF:
> + ath10k_warn(ar, "firmware restart in UTF mode not
> supported\n");
> + break;
> }
>
> mutex_unlock(&ar->conf_mutex);
> }
>
> -int ath10k_core_start(struct ath10k *ar)
> +int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode)
> {
> int status;
Re: [PATCH v2 2/2] ath10k: add testmode
On 08/28/14 10:02, Kalle Valo wrote:
Kalle Valo writes:
Add testmode interface for starting and using UTF firmware which is used to run
factory tests. This is implemented by adding new state ATH10K_STATE_UTF and user
space can enable this state with ATH10K_TM_CMD_UTF_START command. To go back to
normal mode user space can send ATH10K_TM_CMD_UTF_STOP.
Signed-off-by: Kalle Valo
[...]
+/* "API" level of the ath10k testmode interface. Bump it after every
+ * incompatible interface change. */
+#define ATH10K_TESTMODE_VERSION_MAJOR 1
+
+/* Bump this after every _compatible_ interface change, for example
+ * addition of a new command or an attribute. */
+#define ATH10K_TESTMODE_VERSION_MINOR 0
+
+#define ATH10K_TM_DATA_MAX_LEN 5000
+
+enum ath10k_tm_attr {
+ __ATH10K_TM_ATTR_INVALID= 0,
+ ATH10K_TM_ATTR_CMD = 1,
+ ATH10K_TM_ATTR_DATA = 2,
+ ATH10K_TM_ATTR_WMI_CMDID= 3,
+ ATH10K_TM_ATTR_VERSION_MAJOR= 4,
+ ATH10K_TM_ATTR_VERSION_MINOR= 5,
+
+ /* keep last */
+ __ATH10K_TM_ATTR_AFTER_LAST,
+ ATH10K_TM_ATTR_MAX = __ATH10K_TM_ATTR_AFTER_LAST - 1,
+};
+
+/* All ath10k testmode interface commands specified in
+ * ATH10K_TM_ATTR_CMD */
+enum ath10k_tm_cmd {
+ /* Returns the supported ath10k testmode interface version in
+* ATH10K_TM_ATTR_VERSION. Always guaranteed to work. User space
+* uses this to verify it's using the correct version of the
+* testmode interface */
+ ATH10K_TM_CMD_GET_VERSION = 0,
+
+ /* Boots the UTF firmware, the netdev interface must be down at the
+* time. */
+ ATH10K_TM_CMD_UTF_START = 1,
+
+ /* Shuts down the UTF firmware and puts the driver back into OFF
+* state. */
+ ATH10K_TM_CMD_UTF_STOP = 2,
+
+ /* The command used to transmit a WMI command to the firmware and
+* the event to receive WMI events from the firmware. Without
+* struct wmi_cmd_hdr header, only the WMI payload. Command id is
+* provided with ATH10K_TM_ATTR_WMI_CMDID and payload in
+* ATH10K_TM_ATTR_DATA.*/
+ ATH10K_TM_CMD_WMI = 3,
+};
Johannes suggested to put this to a separate file as that way it's
easier for the user space. In v3 I'm planning to create testmode_uapi.h
for this.
I suppose that file will/should end up in include/uapi/... so wouldn't
it be better to call it ath10k_testmode.h?
Regards,
Arend
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH v2 2/2] ath10k: add testmode
Kalle Valo writes:
> Add testmode interface for starting and using UTF firmware which is used to
> run
> factory tests. This is implemented by adding new state ATH10K_STATE_UTF and
> user
> space can enable this state with ATH10K_TM_CMD_UTF_START command. To go back
> to
> normal mode user space can send ATH10K_TM_CMD_UTF_STOP.
>
> Signed-off-by: Kalle Valo
[...]
> +/* "API" level of the ath10k testmode interface. Bump it after every
> + * incompatible interface change. */
> +#define ATH10K_TESTMODE_VERSION_MAJOR 1
> +
> +/* Bump this after every _compatible_ interface change, for example
> + * addition of a new command or an attribute. */
> +#define ATH10K_TESTMODE_VERSION_MINOR 0
> +
> +#define ATH10K_TM_DATA_MAX_LEN 5000
> +
> +enum ath10k_tm_attr {
> + __ATH10K_TM_ATTR_INVALID= 0,
> + ATH10K_TM_ATTR_CMD = 1,
> + ATH10K_TM_ATTR_DATA = 2,
> + ATH10K_TM_ATTR_WMI_CMDID= 3,
> + ATH10K_TM_ATTR_VERSION_MAJOR= 4,
> + ATH10K_TM_ATTR_VERSION_MINOR= 5,
> +
> + /* keep last */
> + __ATH10K_TM_ATTR_AFTER_LAST,
> + ATH10K_TM_ATTR_MAX = __ATH10K_TM_ATTR_AFTER_LAST - 1,
> +};
> +
> +/* All ath10k testmode interface commands specified in
> + * ATH10K_TM_ATTR_CMD */
> +enum ath10k_tm_cmd {
> + /* Returns the supported ath10k testmode interface version in
> + * ATH10K_TM_ATTR_VERSION. Always guaranteed to work. User space
> + * uses this to verify it's using the correct version of the
> + * testmode interface */
> + ATH10K_TM_CMD_GET_VERSION = 0,
> +
> + /* Boots the UTF firmware, the netdev interface must be down at the
> + * time. */
> + ATH10K_TM_CMD_UTF_START = 1,
> +
> + /* Shuts down the UTF firmware and puts the driver back into OFF
> + * state. */
> + ATH10K_TM_CMD_UTF_STOP = 2,
> +
> + /* The command used to transmit a WMI command to the firmware and
> + * the event to receive WMI events from the firmware. Without
> + * struct wmi_cmd_hdr header, only the WMI payload. Command id is
> + * provided with ATH10K_TM_ATTR_WMI_CMDID and payload in
> + * ATH10K_TM_ATTR_DATA.*/
> + ATH10K_TM_CMD_WMI = 3,
> +};
Johannes suggested to put this to a separate file as that way it's
easier for the user space. In v3 I'm planning to create testmode_uapi.h
for this.
--
Kalle Valo
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH v2 2/2] ath10k: add testmode
Add testmode interface for starting and using UTF firmware which is used to run
factory tests. This is implemented by adding new state ATH10K_STATE_UTF and user
space can enable this state with ATH10K_TM_CMD_UTF_START command. To go back to
normal mode user space can send ATH10K_TM_CMD_UTF_STOP.
Signed-off-by: Kalle Valo
---
drivers/net/wireless/ath/ath10k/Makefile |1
drivers/net/wireless/ath/ath10k/core.c | 88 --
drivers/net/wireless/ath/ath10k/core.h | 22 +-
drivers/net/wireless/ath/ath10k/debug.c|5
drivers/net/wireless/ath/ath10k/debug.h|1
drivers/net/wireless/ath/ath10k/hw.h |2
drivers/net/wireless/ath/ath10k/mac.c | 10 +
drivers/net/wireless/ath/ath10k/testmode.c | 404
drivers/net/wireless/ath/ath10k/testmode.h | 46 +++
drivers/net/wireless/ath/ath10k/wmi.c | 17 +
10 files changed, 566 insertions(+), 30 deletions(-)
create mode 100644 drivers/net/wireless/ath/ath10k/testmode.c
create mode 100644 drivers/net/wireless/ath/ath10k/testmode.h
diff --git a/drivers/net/wireless/ath/ath10k/Makefile
b/drivers/net/wireless/ath/ath10k/Makefile
index 2cfb63ca9327..8b1b1adb477a 100644
--- a/drivers/net/wireless/ath/ath10k/Makefile
+++ b/drivers/net/wireless/ath/ath10k/Makefile
@@ -11,6 +11,7 @@ ath10k_core-y += mac.o \
bmi.o
ath10k_core-$(CONFIG_ATH10K_DEBUGFS) += spectral.o
+ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o
ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o
obj-$(CONFIG_ATH10K_PCI) += ath10k_pci.o
diff --git a/drivers/net/wireless/ath/ath10k/core.c
b/drivers/net/wireless/ath/ath10k/core.c
index 651a6da8adf5..c01a37526ad5 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -26,6 +26,7 @@
#include "bmi.h"
#include "debug.h"
#include "htt.h"
+#include "testmode.h"
unsigned int ath10k_debug_mask;
static bool uart_print;
@@ -257,21 +258,42 @@ static int ath10k_download_and_run_otp(struct ath10k *ar)
return 0;
}
-static int ath10k_download_fw(struct ath10k *ar)
+static int ath10k_download_fw(struct ath10k *ar, enum ath10k_firmware_mode
mode)
{
- u32 address;
+ u32 address, data_len;
+ const char *mode_name;
+ const void *data;
int ret;
address = ar->hw_params.patch_load_addr;
- ret = ath10k_bmi_fast_download(ar, address, ar->firmware_data,
- ar->firmware_len);
+ switch (mode) {
+ case ATH10K_FIRMWARE_MODE_NORMAL:
+ data = ar->firmware_data;
+ data_len = ar->firmware_len;
+ mode_name = "normal";
+ break;
+ case ATH10K_FIRMWARE_MODE_UTF:
+ data = ar->testmode.utf->data;
+ data_len = ar->testmode.utf->size;
+ mode_name = "utf";
+ break;
+ default:
+ ath10k_err(ar, "unknown firmware mode: %d\n", mode);
+ return -EINVAL;
+ }
+
+ ath10k_dbg(ar, ATH10K_DBG_BOOT,
+ "boot uploading firmware image %p len %d mode %s\n",
+ data, data_len, mode_name);
+
+ ret = ath10k_bmi_fast_download(ar, address, data, data_len);
if (ret) {
- ath10k_err(ar, "could not write fw (%d)\n", ret);
- goto exit;
+ ath10k_err(ar, "failed to download %s firmware: %d\n",
+ mode_name, ret);
+ return ret;
}
-exit:
return ret;
}
@@ -567,7 +589,8 @@ success:
return 0;
}
-static int ath10k_init_download_firmware(struct ath10k *ar)
+static int ath10k_init_download_firmware(struct ath10k *ar,
+enum ath10k_firmware_mode mode)
{
int ret;
@@ -583,7 +606,7 @@ static int ath10k_init_download_firmware(struct ath10k *ar)
return ret;
}
- ret = ath10k_download_fw(ar);
+ ret = ath10k_download_fw(ar, mode);
if (ret) {
ath10k_err(ar, "failed to download firmware: %d\n", ret);
return ret;
@@ -685,12 +708,15 @@ static void ath10k_core_restart(struct work_struct *work)
case ATH10K_STATE_WEDGED:
ath10k_warn(ar, "device is wedged, will not restart\n");
break;
+ case ATH10K_STATE_UTF:
+ ath10k_warn(ar, "firmware restart in UTF mode not supported\n");
+ break;
}
mutex_unlock(&ar->conf_mutex);
}
-int ath10k_core_start(struct ath10k *ar)
+int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode)
{
int status;
@@ -703,7 +729,7 @@ int ath10k_core_start(struct ath10k *ar)
goto err;
}
- status = ath10k_init_download_firmware(ar);
+ status = ath10k_init_download_firmware(ar, mode);
if (status)
goto err;
@@ -760,10 +786,12 @@ int ath10k_core_star
