[PATCH] drivers: rewrite and remove a superfluous parameter.

[PATCH] drivers: rewrite and remove a superfluous parameter.

[JunASAKA]

I noticed there is a superfluous "*hdr" parameter in rtl8xxxu module
when I am trying to fix some bugs for the rtl8192eu wifi dongle. This
parameter can be removed and then gained from the skb object to make the
function more beautiful.

Signed-off-by: JunASAKA <JunASAKA@zzy040330.moe>
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index ac641a56efb0..4c3d97e8e51f 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -4767,9 +4767,10 @@ static u32 rtl8xxxu_80211_to_rtl_queue(u32 queue)
 	return rtlqueue;
 }
 
-static u32 rtl8xxxu_queue_select(struct ieee80211_hdr *hdr, struct sk_buff *skb)
+static u32 rtl8xxxu_queue_select(struct sk_buff *skb)
 {
 	u32 queue;
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
 
 	if (ieee80211_is_mgmt(hdr->frame_control))
 		queue = TXDESC_QUEUE_MGNT;
@@ -5118,7 +5119,7 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
 	if (control && control->sta)
 		sta = control->sta;
 
-	queue = rtl8xxxu_queue_select(hdr, skb);
+	queue = rtl8xxxu_queue_select(skb);
 
 	tx_desc = skb_push(skb, tx_desc_size);
 
-- 
2.38.1

Re: [PATCH] drivers: rewrite and remove a superfluous parameter.

[Bitterblue Smith]

On 29/11/2022 06:34, JunASAKA wrote:
> I noticed there is a superfluous "*hdr" parameter in rtl8xxxu module
> when I am trying to fix some bugs for the rtl8192eu wifi dongle. This
> parameter can be removed and then gained from the skb object to make the
> function more beautiful.
> 
> Signed-off-by: JunASAKA <JunASAKA@zzy040330.moe>
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index ac641a56efb0..4c3d97e8e51f 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4767,9 +4767,10 @@ static u32 rtl8xxxu_80211_to_rtl_queue(u32 queue)
>  	return rtlqueue;
>  }
>  
> -static u32 rtl8xxxu_queue_select(struct ieee80211_hdr *hdr, struct sk_buff *skb)
> +static u32 rtl8xxxu_queue_select(struct sk_buff *skb)
>  {
>  	u32 queue;
> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>  
>  	if (ieee80211_is_mgmt(hdr->frame_control))
>  		queue = TXDESC_QUEUE_MGNT;
> @@ -5118,7 +5119,7 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>  	if (control && control->sta)
>  		sta = control->sta;
>  
> -	queue = rtl8xxxu_queue_select(hdr, skb);
> +	queue = rtl8xxxu_queue_select(skb);
>  
>  	tx_desc = skb_push(skb, tx_desc_size);
>  

See the recent discussion about this here:
https://lore.kernel.org/linux-wireless/acd30174-4541-7343-e49a-badd199f4151@gmail.com/
https://lore.kernel.org/linux-wireless/2af44c28-1c12-46b9-85b9-011560bf7f7e@gmail.com/

Any luck with the bugs?

reply to Bitterblue Smith

[JunASAKA]

Hi Bitterblue Smith,

Thanks for your reply. I've seen the discussion.
As for the bugs of the module, my Tenda U1 wifi module which is using the rtl8192eu chip running into problems with the rtl8xxxu module, more information can be found here: https://bugzilla.kernel.org/show_bug.cgi?id=216746. I want to solve this problem but I haven't got enough experience upon it. I'll appreciate it if you could do me a favour on it. Thanks again.

Jun ASAKA.

reply to Bitterblue Smith

[JunASAKA]

Hi Bitterblue Smith,

 	I have seen the patch you've mentioned. Actually, when I was trying to address the rtl8192eu problem, I saw that patch and
considered it would tackle my problem, but it turns out that it doesn't work for me. And I found this rtl8xxxu_queue_select() 
function which has a *hdr parameter that can be gained from skb since skb is indeed neccessary for this function to work.
	What do you think of these? And please take a look of my problem above on your convenience, thanks a lot.

Thanks and Regards,
Jun ASAKA.

Re: reply to Bitterblue Smith

[Bitterblue Smith]

On 29/11/2022 16:32, JunASAKA wrote:
> Hi Bitterblue Smith,
> 
> Thanks for your reply. I've seen the discussion.
> As for the bugs of the module, my Tenda U1 wifi module which is using the rtl8192eu chip running into problems with the rtl8xxxu module, more information can be found here: https://bugzilla.kernel.org/show_bug.cgi?id=216746. I want to solve this problem but I haven't got enough experience upon it. I'll appreciate it if you could do me a favour on it. Thanks again.
> 
> Jun ASAKA.

My only idea is to compare all the code with the working driver.
I'm still busy with other things, though.

reply to Bitterblue Smith

[JunASAKA]

	I think you're right and I am comparing those sorces. But the realtek official driver 
is far different from the one in rtl8xxxu module. I think it's difficult for me to do it, but
I am trying my best.

Jun ASAKA.

Re: reply to Bitterblue Smith (was [PATCH] drivers: rewrite and remove a superfluous parameter.)

[Willy Tarreau]

Hello,

On Tue, Nov 29, 2022 at 11:22:12PM +0800, JunASAKA wrote:
> 	I think you're right and I am comparing those sorces. But the realtek official driver 
> is far different from the one in rtl8xxxu module. I think it's difficult for me to do it, but
> I am trying my best.

Would you please preserve the original subject from the thread
of messages you are responding to instead of replacing it with a
useless "reply to Bitterblue Smith" which nobody knows what it's
about and which doesn't bring anything to the discussion since it's
written in your message who you're responding to ? It's the third
such message you send to the list, at first I tagged them as spam
until I saw some responses. Most readers will likely do the same,
and even by doing this the best you're doing is to train anti-spam
systems to learn these as valid messages (which they do not look
like).

Thanks in advance,
Willy

Re: [PATCH] drivers: rewrite and remove a superfluous parameter.

[JunASAKA]

I see. Sorry for that.

Jun ASAKA.

Re: [PATCH] drivers: rewrite and remove a superfluous parameter.

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

On November 29, 2022 3:06:37 PM Bitterblue Smith <rtl8821cerfe2@gmail.com> 
wrote:

> On 29/11/2022 06:34, JunASAKA wrote:
>> I noticed there is a superfluous "*hdr" parameter in rtl8xxxu module
>> when I am trying to fix some bugs for the rtl8192eu wifi dongle. This
>> parameter can be removed and then gained from the skb object to make the
>> function more beautiful.
>>
>> Signed-off-by: JunASAKA <JunASAKA@zzy040330.moe>
>> ---
>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c 
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index ac641a56efb0..4c3d97e8e51f 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -4767,9 +4767,10 @@ static u32 rtl8xxxu_80211_to_rtl_queue(u32 queue)
>> return rtlqueue;
>> }
>>
>> -static u32 rtl8xxxu_queue_select(struct ieee80211_hdr *hdr, struct sk_buff 
>> *skb)
>> +static u32 rtl8xxxu_queue_select(struct sk_buff *skb)
>> {
>> u32 queue;
>> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>
>> if (ieee80211_is_mgmt(hdr->frame_control))
>> queue = TXDESC_QUEUE_MGNT;
>> @@ -5118,7 +5119,7 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>> if (control && control->sta)
>> sta = control->sta;
>>
>> - queue = rtl8xxxu_queue_select(hdr, skb);
>> + queue = rtl8xxxu_queue_select(skb);
>>
>> tx_desc = skb_push(skb, tx_desc_size);
>
> See the recent discussion about this here:
> https://lore.kernel.org/linux-wireless/acd30174-4541-7343-e49a-badd199f4151@gmail.com/
> https://lore.kernel.org/linux-wireless/2af44c28-1c12-46b9-85b9-011560bf7f7e@gmail.com/

Not sure why I looked but I did. You may want to look at rtl8xxxu_tx() 
which is the .tx callback that mac80211 uses and the first statement in 
there is also assuming skb->data points to the 802.11 header.

Regards,
Arend
>





[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH] drivers: rewrite and remove a superfluous parameter.

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 2654 bytes --]

On December 6, 2022 6:59:36 PM Arend Van Spriel 
<arend.vanspriel@broadcom.com> wrote:

> On November 29, 2022 3:06:37 PM Bitterblue Smith <rtl8821cerfe2@gmail.com>
> wrote:
>
>> On 29/11/2022 06:34, JunASAKA wrote:
>>> I noticed there is a superfluous "*hdr" parameter in rtl8xxxu module
>>> when I am trying to fix some bugs for the rtl8192eu wifi dongle. This
>>> parameter can be removed and then gained from the skb object to make the
>>> function more beautiful.
>>>
>>> Signed-off-by: JunASAKA <JunASAKA@zzy040330.moe>
>>> ---
>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> index ac641a56efb0..4c3d97e8e51f 100644
>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> @@ -4767,9 +4767,10 @@ static u32 rtl8xxxu_80211_to_rtl_queue(u32 queue)
>>> return rtlqueue;
>>> }
>>>
>>> -static u32 rtl8xxxu_queue_select(struct ieee80211_hdr *hdr, struct sk_buff
>>> *skb)
>>> +static u32 rtl8xxxu_queue_select(struct sk_buff *skb)
>>> {
>>> u32 queue;
>>> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>>
>>> if (ieee80211_is_mgmt(hdr->frame_control))
>>> queue = TXDESC_QUEUE_MGNT;
>>> @@ -5118,7 +5119,7 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>>> if (control && control->sta)
>>> sta = control->sta;
>>>
>>> - queue = rtl8xxxu_queue_select(hdr, skb);
>>> + queue = rtl8xxxu_queue_select(skb);
>>>
>>> tx_desc = skb_push(skb, tx_desc_size);
>>
>> See the recent discussion about this here:
>> https://lore.kernel.org/linux-wireless/acd30174-4541-7343-e49a-badd199f4151@gmail.com/
>> https://lore.kernel.org/linux-wireless/2af44c28-1c12-46b9-85b9-011560bf7f7e@gmail.com/
>
> Not sure why I looked but I did. You may want to look at rtl8xxxu_tx()
> which is the .tx callback that mac80211 uses and the first statement in
> there is also assuming skb->data points to the 802.11 header.

Here the documentation of the .tx callback:

@tx: Handler that 802.11 module calls for each transmitted frame.
 * skb contains the buffer *starting from the IEEE 802.11 header*.
 * The low-level driver should send the frame out based on
 * configuration in the TX control data. This handler should,
 * preferably, never fail and stop queues appropriately.
 * Must be atomic.

I don't see any pushes or pulls before the queue select so that would mean 
mac80211 is not complying to the described behavior.

Regards,
Arend

>
> Regards,
> Arend
>>




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH] drivers: rewrite and remove a superfluous parameter.

[Bitterblue Smith]

On 06/12/2022 20:19, Arend Van Spriel wrote:
> On December 6, 2022 6:59:36 PM Arend Van Spriel <arend.vanspriel@broadcom.com> wrote:
> 
>> On November 29, 2022 3:06:37 PM Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> wrote:
>>
>>> On 29/11/2022 06:34, JunASAKA wrote:
>>>> I noticed there is a superfluous "*hdr" parameter in rtl8xxxu module
>>>> when I am trying to fix some bugs for the rtl8192eu wifi dongle. This
>>>> parameter can be removed and then gained from the skb object to make the
>>>> function more beautiful.
>>>>
>>>> Signed-off-by: JunASAKA <JunASAKA@zzy040330.moe>
>>>> ---
>>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>>> index ac641a56efb0..4c3d97e8e51f 100644
>>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>>> @@ -4767,9 +4767,10 @@ static u32 rtl8xxxu_80211_to_rtl_queue(u32 queue)
>>>> return rtlqueue;
>>>> }
>>>>
>>>> -static u32 rtl8xxxu_queue_select(struct ieee80211_hdr *hdr, struct sk_buff
>>>> *skb)
>>>> +static u32 rtl8xxxu_queue_select(struct sk_buff *skb)
>>>> {
>>>> u32 queue;
>>>> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>>>
>>>> if (ieee80211_is_mgmt(hdr->frame_control))
>>>> queue = TXDESC_QUEUE_MGNT;
>>>> @@ -5118,7 +5119,7 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>>>> if (control && control->sta)
>>>> sta = control->sta;
>>>>
>>>> - queue = rtl8xxxu_queue_select(hdr, skb);
>>>> + queue = rtl8xxxu_queue_select(skb);
>>>>
>>>> tx_desc = skb_push(skb, tx_desc_size);
>>>
>>> See the recent discussion about this here:
>>> https://lore.kernel.org/linux-wireless/acd30174-4541-7343-e49a-badd199f4151@gmail.com/
>>> https://lore.kernel.org/linux-wireless/2af44c28-1c12-46b9-85b9-011560bf7f7e@gmail.com/
>>
>> Not sure why I looked but I did. You may want to look at rtl8xxxu_tx()
>> which is the .tx callback that mac80211 uses and the first statement in
>> there is also assuming skb->data points to the 802.11 header.
> 
> Here the documentation of the .tx callback:
> 
> @tx: Handler that 802.11 module calls for each transmitted frame.
> * skb contains the buffer *starting from the IEEE 802.11 header*.
> * The low-level driver should send the frame out based on
> * configuration in the TX control data. This handler should,
> * preferably, never fail and stop queues appropriately.
> * Must be atomic.
> 
> I don't see any pushes or pulls before the queue select so that would mean mac80211 is not complying to the described behavior.
> 
> Regards,
> Arend
> 
>>
>> Regards,
>> Arend
>>>
> 
> 
> 
mac80211 is behaving as described in the documentation, as far as I know.
Technically, rtl8xxxu_queue_select's hdr parameter is not needed.

Re: [PATCH] drivers: rewrite and remove a superfluous parameter.

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]

On December 6, 2022 7:40:43 PM Bitterblue Smith <rtl8821cerfe2@gmail.com> 
wrote:

> On 06/12/2022 20:19, Arend Van Spriel wrote:
>> On December 6, 2022 6:59:36 PM Arend Van Spriel 
>> <arend.vanspriel@broadcom.com> wrote:
>>
>>> On November 29, 2022 3:06:37 PM Bitterblue Smith <rtl8821cerfe2@gmail.com>
>>> wrote:
>>>
>>>> On 29/11/2022 06:34, JunASAKA wrote:
>>>>> I noticed there is a superfluous "*hdr" parameter in rtl8xxxu module
>>>>> when I am trying to fix some bugs for the rtl8192eu wifi dongle. This
>>>>> parameter can be removed and then gained from the skb object to make the
>>>>> function more beautiful.
>>>>>
>>>>> Signed-off-by: JunASAKA <JunASAKA@zzy040330.moe>
>>>>> ---
>>>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++--
>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>>>> index ac641a56efb0..4c3d97e8e51f 100644
>>>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>>>> @@ -4767,9 +4767,10 @@ static u32 rtl8xxxu_80211_to_rtl_queue(u32 queue)
>>>>> return rtlqueue;
>>>>> }
>>>>>
>>>>> -static u32 rtl8xxxu_queue_select(struct ieee80211_hdr *hdr, struct sk_buff
>>>>> *skb)
>>>>> +static u32 rtl8xxxu_queue_select(struct sk_buff *skb)
>>>>> {
>>>>> u32 queue;
>>>>> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>>>>>
>>>>> if (ieee80211_is_mgmt(hdr->frame_control))
>>>>> queue = TXDESC_QUEUE_MGNT;
>>>>> @@ -5118,7 +5119,7 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>>>>> if (control && control->sta)
>>>>> sta = control->sta;
>>>>>
>>>>> - queue = rtl8xxxu_queue_select(hdr, skb);
>>>>> + queue = rtl8xxxu_queue_select(skb);
>>>>>
>>>>> tx_desc = skb_push(skb, tx_desc_size);
>>>>
>>>> See the recent discussion about this here:
>>>> https://lore.kernel.org/linux-wireless/acd30174-4541-7343-e49a-badd199f4151@gmail.com/
>>>> https://lore.kernel.org/linux-wireless/2af44c28-1c12-46b9-85b9-011560bf7f7e@gmail.com/
>>>
>>> Not sure why I looked but I did. You may want to look at rtl8xxxu_tx()
>>> which is the .tx callback that mac80211 uses and the first statement in
>>> there is also assuming skb->data points to the 802.11 header.
>>
>> Here the documentation of the .tx callback:
>>
>> @tx: Handler that 802.11 module calls for each transmitted frame.
>> * skb contains the buffer *starting from the IEEE 802.11 header*.
>> * The low-level driver should send the frame out based on
>> * configuration in the TX control data. This handler should,
>> * preferably, never fail and stop queues appropriately.
>> * Must be atomic.
>>
>> I don't see any pushes or pulls before the queue select so that would mean 
>> mac80211 is not complying to the described behavior.
>>
>> Regards,
>> Arend
>>
>>>
>>> Regards,
>>> Arend
>>>>
> mac80211 is behaving as described in the documentation, as far as I know.
> Technically, rtl8xxxu_queue_select's hdr parameter is not needed.

Okay. Then I probably misunderstood the gist of the discussion you referred 
to. Now I see the issue got fixed by your patch (1st URL). So sorry for the 
noise.

Regards,
Arend



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]