[1/5] wifi: wilc1000: set preamble size to auto as default in wilc_init_fw_config()

Commit Message

Alexis Lothoré Jan. 15, 2024, 2:56 p.m. UTC

From: Ajay Singh <[e-mail]>

Changed the default value preamble to WILC_FW_PREAMBLE_AUTO in
wilc_init_fw_config().

Signed-off-by: Ajay Singh <[e-mail]>
Signed-off-by: Alexis Lothoré <[e-mail]>
---
 drivers/net/wireless/microchip/wilc1000/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kalle Valo Jan. 18, 2024, 9:31 a.m. UTC | #1

Alexis Lothoré <[e-mail]> wrote:

> From: Ajay Singh <[e-mail]>
> 
> Changed the default value preamble to WILC_FW_PREAMBLE_AUTO in
> wilc_init_fw_config().
> 
> Signed-off-by: Ajay Singh <[e-mail]>
> Signed-off-by: Alexis Lothoré <[e-mail]>

The commit message should always answer to the question "Why?". I can add that
if you tell me what to add.

Alexis Lothoré Jan. 18, 2024, 3:08 p.m. UTC | #2

Hi Kalle,

On 1/18/24 10:31, Kalle Valo wrote:
> Alexis Lothoré <[e-mail]> wrote:
> 
>> From: Ajay Singh <[e-mail]>
>>
>> Changed the default value preamble to WILC_FW_PREAMBLE_AUTO in
>> wilc_init_fw_config().
>>
>> Signed-off-by: Ajay Singh <[e-mail]>
>> Signed-off-by: Alexis Lothoré <[e-mail]>
> 
> The commit message should always answer to the question "Why?". I can add that
> if you tell me what to add.

Yeah, sorry for the lack of description, I may have forgotten to update this
one. I suggest to update it with the following message:

"WILC driver currently applies some default configuration whenever the firmware
is initialized, and sets the default preamble size to short. However, despite
this passed option, firmware is also able to successfully connect to access
points only using long preamble, so this setting does not really enforce short
preambles and is misleading regarding applied configuration.

Update default configuration and make it match the firmware behavior by passing
the existing WILC_FW_PREAMBLE_AUTO value (2 instead of 0). The updated setting
does not really alter firmware behavior since it is still capable to connect to
both short preamble and long preamble access points, but at list the setting now
expresses for real the corresponding firmware behavior"

To give a bit of context around this one: I do not have access to the firmware
internals, I just took the patch from Ajay and I merely did some tests around it
with multiple APs (basically, making a WILC STA connect and ping the AP), and
ensured with wireshark to get at least one AP be really "locked" with long
preamble, with WILC managing to connect to it.

Thanks,

Alexis

Ajay Singh Jan. 18, 2024, 4:52 p.m. UTC | #3

On 1/18/24 08:08, Alexis Lothoré wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Kalle,
> 
> On 1/18/24 10:31, Kalle Valo wrote:
>> Alexis Lothoré <[e-mail]> wrote:
>>
>>> From: Ajay Singh <[e-mail]>
>>>
>>> Changed the default value preamble to WILC_FW_PREAMBLE_AUTO in
>>> wilc_init_fw_config().
>>>
>>> Signed-off-by: Ajay Singh <[e-mail]>
>>> Signed-off-by: Alexis Lothoré <[e-mail]>
>>
>> The commit message should always answer to the question "Why?". I can add that
>> if you tell me what to add.
> 
> Yeah, sorry for the lack of description, I may have forgotten to update this
> one. I suggest to update it with the following message:
> 
> "WILC driver currently applies some default configuration whenever the firmware
> is initialized, and sets the default preamble size to short. However, despite
> this passed option, firmware is also able to successfully connect to access
> points only using long preamble, so this setting does not really enforce short
> preambles and is misleading regarding applied configuration.
> 
> Update default configuration and make it match the firmware behavior by passing
> the existing WILC_FW_PREAMBLE_AUTO value (2 instead of 0). The updated setting
> does not really alter firmware behavior since it is still capable to connect to
> both short preamble and long preamble access points, but at list the setting now
> expresses for real the corresponding firmware behavior"
> 
> To give a bit of context around this one: I do not have access to the firmware
> internals, I just took the patch from Ajay and I merely did some tests around it
> with multiple APs (basically, making a WILC STA connect and ping the AP), and
> ensured with wireshark to get at least one AP be really "locked" with long
> preamble, with WILC managing to connect to it.
> 

Here are some more details about this change. It have been implemented
to address the transmission(Tx) blackout issue observed in the 802.11b
mode. The modification has no impact on the other modes, which will
continue to work as they did in the previous implementation. This change
will allow the 802.11b transmission(2, 5.5, 11Mbps) to use long preamble.


Regards,
Ajay

Alexis Lothoré Jan. 19, 2024, 7:43 a.m. UTC | #4

On 1/18/24 17:52, [e-mail] wrote:
> On 1/18/24 08:08, Alexis Lothoré wrote:

[...]

>> "WILC driver currently applies some default configuration whenever the firmware
>> is initialized, and sets the default preamble size to short. However, despite
>> this passed option, firmware is also able to successfully connect to access
>> points only using long preamble, so this setting does not really enforce short
>> preambles and is misleading regarding applied configuration.
>>
>> Update default configuration and make it match the firmware behavior by passing
>> the existing WILC_FW_PREAMBLE_AUTO value (2 instead of 0). The updated setting
>> does not really alter firmware behavior since it is still capable to connect to
>> both short preamble and long preamble access points, but at list the setting now
>> expresses for real the corresponding firmware behavior"
>>
>> To give a bit of context around this one: I do not have access to the firmware
>> internals, I just took the patch from Ajay and I merely did some tests around it
>> with multiple APs (basically, making a WILC STA connect and ping the AP), and
>> ensured with wireshark to get at least one AP be really "locked" with long
>> preamble, with WILC managing to connect to it.
>>
> 
> Here are some more details about this change. It have been implemented
> to address the transmission(Tx) blackout issue observed in the 802.11b
> mode. The modification has no impact on the other modes, which will
> continue to work as they did in the previous implementation. This change
> will allow the 802.11b transmission(2, 5.5, 11Mbps) to use long preamble.

Ah, so it fixes a specific bug (and makes parts of my suggested description
wrong). Has there been any report about this "TX blackout issue" on the mailing
lists ? If so, we could enrich the message with some details from the report and
add some missing Reported-By/Fixes/Closes tags.

Alexis

Ajay Singh Jan. 19, 2024, 6:19 p.m. UTC | #5

Hi Alexis,

On 1/19/24 00:43, Alexis Lothoré wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 1/18/24 17:52, [e-mail] wrote:
>> On 1/18/24 08:08, Alexis Lothoré wrote:
> 
> [...]
> 
>>> "WILC driver currently applies some default configuration whenever the firmware
>>> is initialized, and sets the default preamble size to short. However, despite
>>> this passed option, firmware is also able to successfully connect to access
>>> points only using long preamble, so this setting does not really enforce short
>>> preambles and is misleading regarding applied configuration.
>>>
>>> Update default configuration and make it match the firmware behavior by passing
>>> the existing WILC_FW_PREAMBLE_AUTO value (2 instead of 0). The updated setting
>>> does not really alter firmware behavior since it is still capable to connect to
>>> both short preamble and long preamble access points, but at list the setting now
>>> expresses for real the corresponding firmware behavior"
>>>
>>> To give a bit of context around this one: I do not have access to the firmware
>>> internals, I just took the patch from Ajay and I merely did some tests around it
>>> with multiple APs (basically, making a WILC STA connect and ping the AP), and
>>> ensured with wireshark to get at least one AP be really "locked" with long
>>> preamble, with WILC managing to connect to it.
>>>
>>
>> Here are some more details about this change. It have been implemented
>> to address the transmission(Tx) blackout issue observed in the 802.11b
>> mode. The modification has no impact on the other modes, which will
>> continue to work as they did in the previous implementation. This change
>> will allow the 802.11b transmission(2, 5.5, 11Mbps) to use long preamble.
> 
> Ah, so it fixes a specific bug (and makes parts of my suggested description
> wrong). Has there been any report about this "TX blackout issue" on the mailing
> lists ? If so, we could enrich the message with some details from the report and
> add some missing Reported-By/Fixes/Closes tags.
> 

For this issue, there are no details on the mailing lists. It was
discovered by internal QA team.

Regards,
Ajay

Kalle Valo Feb. 12, 2024, 3:36 p.m. UTC | #6

Alexis Lothoré <[e-mail]> wrote:

> From: Ajay Singh <[e-mail]>
> 
> WILC driver currently applies some default configuration whenever the firmware
> is initialized, and sets the default preamble size to short. However, despite
> this passed option, firmware is also able to successfully connect to access
> points only using long preamble, so this setting does not really enforce short
> preambles and is misleading regarding applied configuration.
> 
> Update default configuration and make it match the firmware behavior by passing
> the existing WILC_FW_PREAMBLE_AUTO value (2 instead of 0). The updated setting
> does not really alter firmware behavior since it is still capable to connect to
> both short preamble and long preamble access points, but at list the setting now
> expresses for real the corresponding firmware behavior.
> 
> More info: it has been implemented to address the transmission (Tx) blackout
> issue observed in the 802.11b mode. The modification has no impact on the other
> modes, which will continue to work as they did in the previous implementation.
> This change will allow the 802.11b transmission (2, 5.5, 11Mbps) to use long
> preamble.
> 
> 
> Signed-off-by: Ajay Singh <[e-mail]>
> Signed-off-by: Alexis Lothoré <[e-mail]>

Patch applied to wireless-next.git, thanks.

a8e5fefa9123 wifi: wilc1000: set preamble size to auto as default in wilc_init_fw_config()

Patch

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 91d71e0f7ef2..8923eb64c964 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -284,7 +284,7 @@  static int wilc_init_fw_config(struct net_device *dev, struct wilc_vif *vif)
 	if (!wilc_wlan_cfg_set(vif, 0, WID_11G_OPERATING_MODE, &b, 1, 0, 0))
 		goto fail;
 
-	b = WILC_FW_PREAMBLE_SHORT;
+	b = WILC_FW_PREAMBLE_AUTO;
 	if (!wilc_wlan_cfg_set(vif, 0, WID_PREAMBLE, &b, 1, 0, 0))
 		goto fail;