diff mbox

[v2,15/21] ath10k: support CT firmware flag.

Message ID 1462986153-16318-16-git-send-email-greearb@candelatech.com (mailing list archive)
State Rejected
Delegated to: Kalle Valo
Headers show

Commit Message

Ben Greear May 11, 2016, 5:02 p.m. UTC
From: Ben Greear <greearb@candelatech.com>

Add placeholder so CT firmware can more easily co-exist with upstream
kernel.  CT firmware should be backwards compatible with existing kernels,
but it also has many new features.  Subsequent patches, if acceptable for
upstream, can enable and further describe those features.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 1 +
 drivers/net/wireless/ath/ath10k/core.h | 3 +++
 2 files changed, 4 insertions(+)

Comments

Kalle Valo Sept. 14, 2016, 2:30 p.m. UTC | #1
greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> Add placeholder so CT firmware can more easily co-exist with upstream
> kernel.  CT firmware should be backwards compatible with existing kernels,
> but it also has many new features.  Subsequent patches, if acceptable for
> upstream, can enable and further describe those features.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 1 +
>  drivers/net/wireless/ath/ath10k/core.h | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index fa71d57..49c85c3 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -235,6 +235,7 @@ static const char *const ath10k_core_fw_feature_str[] = {
>  	[ATH10K_FW_FEATURE_SUPPORTS_ADAPTIVE_CCA] = "adaptive-cca",
>  	[ATH10K_FW_FEATURE_MFP_SUPPORT] = "mfp",
>  	[ATH10K_FW_FEATURE_PEER_FLOW_CONTROL] = "peer-flow-ctrl",
> +	[ATH10K_FW_FEATURE_WMI_10X_CT] = "wmi-10.x-CT",
>  };
>  
>  static unsigned int ath10k_core_get_fw_feature_str(char *buf,
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index cb6aa8c..f9e3b20 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -566,6 +566,9 @@ enum ath10k_fw_features {
>  	 */
>  	ATH10K_FW_FEATURE_PEER_FLOW_CONTROL = 13,
>  
> +	/* Firmware from Candela Technologies, enables more VIFs, etc */
> +	ATH10K_FW_FEATURE_WMI_10X_CT = 31,

The idea of firmware feature flags to enable (or disable) one particular
feature, not a group of features. This way it's easy to enable certain
features on different firmware branches. It also makes the maintenance
easier as you don't need to remember all the different features one flag
enables.
Ben Greear Sept. 14, 2016, 3:24 p.m. UTC | #2
On 09/14/2016 07:30 AM, Valo, Kalle wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Add placeholder so CT firmware can more easily co-exist with upstream
>> kernel.  CT firmware should be backwards compatible with existing kernels,
>> but it also has many new features.  Subsequent patches, if acceptable for
>> upstream, can enable and further describe those features.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>   drivers/net/wireless/ath/ath10k/core.c | 1 +
>>   drivers/net/wireless/ath/ath10k/core.h | 3 +++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
>> index fa71d57..49c85c3 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.c
>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>> @@ -235,6 +235,7 @@ static const char *const ath10k_core_fw_feature_str[] = {
>>   	[ATH10K_FW_FEATURE_SUPPORTS_ADAPTIVE_CCA] = "adaptive-cca",
>>   	[ATH10K_FW_FEATURE_MFP_SUPPORT] = "mfp",
>>   	[ATH10K_FW_FEATURE_PEER_FLOW_CONTROL] = "peer-flow-ctrl",
>> +	[ATH10K_FW_FEATURE_WMI_10X_CT] = "wmi-10.x-CT",
>>   };
>>
>>   static unsigned int ath10k_core_get_fw_feature_str(char *buf,
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>> index cb6aa8c..f9e3b20 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -566,6 +566,9 @@ enum ath10k_fw_features {
>>   	 */
>>   	ATH10K_FW_FEATURE_PEER_FLOW_CONTROL = 13,
>>
>> +	/* Firmware from Candela Technologies, enables more VIFs, etc */
>> +	ATH10K_FW_FEATURE_WMI_10X_CT = 31,
>
> The idea of firmware feature flags to enable (or disable) one particular
> feature, not a group of features. This way it's easy to enable certain
> features on different firmware branches. It also makes the maintenance
> easier as you don't need to remember all the different features one flag
> enables.

One potential use for this flag:  Bug reports could be automatically directed to me instead
of QCA.

I have actually re-written much of my earlier logic so that it uses specific
feature flags now.  I still like the idea of having the driver (and user)
know it is using CT firmware, but I can live without this flag if needed.

Even if I change generation of my new firmware, all of my older firmware
will have this flag set though, so I would still prefer the flag exist
in the driver, even if it is not used for any serious decisions.

Thanks,
Ben
Kalle Valo Sept. 15, 2016, 2:15 p.m. UTC | #3
Ben Greear <greearb@candelatech.com> writes:

> On 09/14/2016 07:30 AM, Valo, Kalle wrote:
>> greearb@candelatech.com writes:
>>
>>> From: Ben Greear <greearb@candelatech.com>
>>>
>>> Add placeholder so CT firmware can more easily co-exist with upstream
>>> kernel.  CT firmware should be backwards compatible with existing kernels,
>>> but it also has many new features.  Subsequent patches, if acceptable for
>>> upstream, can enable and further describe those features.
>>>
>>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>> ---
>>>   drivers/net/wireless/ath/ath10k/core.c | 1 +
>>>   drivers/net/wireless/ath/ath10k/core.h | 3 +++
>>>   2 files changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
>>> index fa71d57..49c85c3 100644
>>> --- a/drivers/net/wireless/ath/ath10k/core.c
>>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>>> @@ -235,6 +235,7 @@ static const char *const ath10k_core_fw_feature_str[] = {
>>>   	[ATH10K_FW_FEATURE_SUPPORTS_ADAPTIVE_CCA] = "adaptive-cca",
>>>   	[ATH10K_FW_FEATURE_MFP_SUPPORT] = "mfp",
>>>   	[ATH10K_FW_FEATURE_PEER_FLOW_CONTROL] = "peer-flow-ctrl",
>>> +	[ATH10K_FW_FEATURE_WMI_10X_CT] = "wmi-10.x-CT",
>>>   };
>>>
>>>   static unsigned int ath10k_core_get_fw_feature_str(char *buf,
>>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>>> index cb6aa8c..f9e3b20 100644
>>> --- a/drivers/net/wireless/ath/ath10k/core.h
>>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>>> @@ -566,6 +566,9 @@ enum ath10k_fw_features {
>>>   	 */
>>>   	ATH10K_FW_FEATURE_PEER_FLOW_CONTROL = 13,
>>>
>>> +	/* Firmware from Candela Technologies, enables more VIFs, etc */
>>> +	ATH10K_FW_FEATURE_WMI_10X_CT = 31,
>>
>> The idea of firmware feature flags to enable (or disable) one particular
>> feature, not a group of features. This way it's easy to enable certain
>> features on different firmware branches. It also makes the maintenance
>> easier as you don't need to remember all the different features one flag
>> enables.
>
> One potential use for this flag:  Bug reports could be automatically directed to me instead
> of QCA.
>
> I have actually re-written much of my earlier logic so that it uses specific
> feature flags now.  I still like the idea of having the driver (and user)
> know it is using CT firmware, but I can live without this flag if needed.

You can do with firmware version string, add something like "-CT" (maybe
you already have?) and it's clearly visible in boot logs and also when
firmware crashes.
Ben Greear Sept. 15, 2016, 2:43 p.m. UTC | #4
On 09/15/2016 07:15 AM, Valo, Kalle wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> On 09/14/2016 07:30 AM, Valo, Kalle wrote:
>>> greearb@candelatech.com writes:
>>>
>>>> From: Ben Greear <greearb@candelatech.com>
>>>>
>>>> Add placeholder so CT firmware can more easily co-exist with upstream
>>>> kernel.  CT firmware should be backwards compatible with existing kernels,
>>>> but it also has many new features.  Subsequent patches, if acceptable for
>>>> upstream, can enable and further describe those features.
>>>>
>>>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>>> ---
>>>>    drivers/net/wireless/ath/ath10k/core.c | 1 +
>>>>    drivers/net/wireless/ath/ath10k/core.h | 3 +++
>>>>    2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
>>>> index fa71d57..49c85c3 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/core.c
>>>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>>>> @@ -235,6 +235,7 @@ static const char *const ath10k_core_fw_feature_str[] = {
>>>>    	[ATH10K_FW_FEATURE_SUPPORTS_ADAPTIVE_CCA] = "adaptive-cca",
>>>>    	[ATH10K_FW_FEATURE_MFP_SUPPORT] = "mfp",
>>>>    	[ATH10K_FW_FEATURE_PEER_FLOW_CONTROL] = "peer-flow-ctrl",
>>>> +	[ATH10K_FW_FEATURE_WMI_10X_CT] = "wmi-10.x-CT",
>>>>    };
>>>>
>>>>    static unsigned int ath10k_core_get_fw_feature_str(char *buf,
>>>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>>>> index cb6aa8c..f9e3b20 100644
>>>> --- a/drivers/net/wireless/ath/ath10k/core.h
>>>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>>>> @@ -566,6 +566,9 @@ enum ath10k_fw_features {
>>>>    	 */
>>>>    	ATH10K_FW_FEATURE_PEER_FLOW_CONTROL = 13,
>>>>
>>>> +	/* Firmware from Candela Technologies, enables more VIFs, etc */
>>>> +	ATH10K_FW_FEATURE_WMI_10X_CT = 31,
>>>
>>> The idea of firmware feature flags to enable (or disable) one particular
>>> feature, not a group of features. This way it's easy to enable certain
>>> features on different firmware branches. It also makes the maintenance
>>> easier as you don't need to remember all the different features one flag
>>> enables.
>>
>> One potential use for this flag:  Bug reports could be automatically directed to me instead
>> of QCA.
>>
>> I have actually re-written much of my earlier logic so that it uses specific
>> feature flags now.  I still like the idea of having the driver (and user)
>> know it is using CT firmware, but I can live without this flag if needed.
>
> You can do with firmware version string, add something like "-CT" (maybe
> you already have?) and it's clearly visible in boot logs and also when
> firmware crashes.

Yes, that is a worse hack, but it works.

Thing is, I have years of older, tested firmware around, with this feature flag
set.  So, it is going to warn when booting in a kernel that does not understand
this flag.

So, if you want CT firmware support at all in the upstream kernel, then I wish
you would let me add the enum and string to make it work as expected.  That will also
let these old firmware do IBSS with one of the other patches I posted.  Yes, it would
be nice to have an IBSS feature flag, but we have not for years, and again, this would
let my older firmware work.

It feels a bit like pulling teeth to get patches upstream.  You are basically
waving through this huge hack to set registers for ack-timing and stuff, but
then you are arguing about a simple feature flag enum?

Thanks,
Ben
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index fa71d57..49c85c3 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -235,6 +235,7 @@  static const char *const ath10k_core_fw_feature_str[] = {
 	[ATH10K_FW_FEATURE_SUPPORTS_ADAPTIVE_CCA] = "adaptive-cca",
 	[ATH10K_FW_FEATURE_MFP_SUPPORT] = "mfp",
 	[ATH10K_FW_FEATURE_PEER_FLOW_CONTROL] = "peer-flow-ctrl",
+	[ATH10K_FW_FEATURE_WMI_10X_CT] = "wmi-10.x-CT",
 };
 
 static unsigned int ath10k_core_get_fw_feature_str(char *buf,
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index cb6aa8c..f9e3b20 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -566,6 +566,9 @@  enum ath10k_fw_features {
 	 */
 	ATH10K_FW_FEATURE_PEER_FLOW_CONTROL = 13,
 
+	/* Firmware from Candela Technologies, enables more VIFs, etc */
+	ATH10K_FW_FEATURE_WMI_10X_CT = 31,
+
 	/* keep last */
 	ATH10K_FW_FEATURE_COUNT,
 };