diff mbox series

wifi: rtw88: usb: Simplify rtw_usb_write_data

Message ID 681e03c1-d19e-44de-bc45-e71ce14c5ed2@gmail.com (mailing list archive)
State Superseded
Delegated to: Ping-Ke Shih
Headers show
Series wifi: rtw88: usb: Simplify rtw_usb_write_data | expand

Commit Message

Bitterblue Smith May 2, 2024, 9:23 p.m. UTC
The skb created in this function always has the same headroom,
the chip's TX descriptor size. Use chip->tx_pkt_desc_sz directly.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
This is the patch I promised earlier:
https://lore.kernel.org/linux-wireless/cae2d330-a4fb-4570-9dde-09684af23ffd@gmail.com/
---
 drivers/net/wireless/realtek/rtw88/usb.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Larry Finger May 2, 2024, 10:18 p.m. UTC | #1
On 5/2/24 4:23 PM, Bitterblue Smith wrote:
> The skb created in this function always has the same headroom,
> the chip's TX descriptor size. Use chip->tx_pkt_desc_sz directly.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
> This is the patch I promised earlier:
> https://lore.kernel.org/linux-wireless/cae2d330-a4fb-4570-9dde-09684af23ffd@gmail.com/
> ---
>   drivers/net/wireless/realtek/rtw88/usb.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/usb.c b/usb.c
> index 1dfe7c6ae4ba..ff57976b9d3b 100644
> --- a/usb.c
> +++ b/usb.c
> @@ -440,23 +440,21 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>   {
>   	const struct rtw_chip_info *chip = rtwdev->chip;
>   	struct sk_buff *skb;
> -	unsigned int desclen, headsize, size;
> +	unsigned int size;
>   	u8 qsel;
>   	int ret = 0;
>    
>   	size =pkt_info->tx_pkt_size;
>   	qsel = pkt_info->qsel;
> -	desclen = chip->tx_pkt_desc_sz;
> -	headsize = pkt_info->offset ? pkt_info->offset : desclen;
>   
> -	skb = dev_alloc_skb(headsize + size);
> +	skb = dev_alloc_skb(chip->tx_pkt_desc_sz + size);
>   	if (unlikely(!skb))
>   		return -ENOMEM;
>   
> -	skb_reserve(skb, headsize);
> +	skb_reserve(skb, chip->tx_pkt_desc_sz);
>   	skb_put_data(skb, buf, size);
> -	skb_push(skb, headsize);
> -	memset(skb->data, 0, headsize);
> +	skb_push(skb, chip->tx_pkt_desc_sz);
> +	memset(skb->data, 0, chip->tx_pkt_desc_sz);
>   	rtw_tx_fill_tx_desc(pkt_info, skb);
>   	rtw_tx_fill_txdesc_checksum(rtwdev, pkt_info, skb->data);
>   
> @@ -471,12 +469,10 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>   static int rtw_usb_write_data_rsvd_page(struct rtw_dev *rtwdev, u8 *buf,
>   					u32 size)
>   {
> -	const struct rtw_chip_info *chip = rtwdev->chip;
>   	struct rtw_tx_pkt_info pkt_info = {0};
>   
>   	pkt_info.tx_pkt_size = size;
>   	pkt_info.qsel = TX_DESC_QSEL_BEACON;
> -	pkt_info.offset = chip->tx_pkt_desc_sz;
>   
>   	return rtw_usb_write_data(rtwdev, &pkt_info, buf);
>   }

This patch doesn't work. When I add it and start an 8822bu, I get:

[   46.695755] usb 3-6: new high-speed USB device number 4 using xhci_hcd
[   46.844397] usb 3-6: New USB device found, idVendor=0bda, idProduct=b82c, 
bcdDevice= 2.10
[   46.844404] usb 3-6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[   46.844406] usb 3-6: Product: 802.11ac NIC
[   46.844408] usb 3-6: Manufacturer: Realtek
[   46.844410] usb 3-6: SerialNumber: 123456
[   47.524214] rtw_8822bu 3-6:1.2: Firmware version 27.2.0, H2C version 13
[   47.573043] rtw_8822bu 3-6:1.2: error beacon valid
[   47.573165] rtw_8822bu 3-6:1.2: failed to download rsvd page
[   47.573488] rtw_8822bu 3-6:1.2: failed to download firmware
[   47.576745] rtw_8822bu 3-6:1.2: failed to setup chip efuse info
[   47.576750] rtw_8822bu 3-6:1.2: failed to setup chip information
[   47.577302] rtw_8822bu 3-6:1.2: probe with driver rtw_8822bu failed with 
error -16

When I added code to test if chip->tx_pkt_desc_sz was equal to 
pkt_info->tx_pkt_size at entry, it reported that there was a difference.

This patch may work for some of the devices, but clearly not for all.

NACK.

Larry
Ping-Ke Shih May 3, 2024, 12:35 a.m. UTC | #2
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> The skb created in this function always has the same headroom,
> the chip's TX descriptor size. Use chip->tx_pkt_desc_sz directly.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
> This is the patch I promised earlier:
> https://lore.kernel.org/linux-wireless/cae2d330-a4fb-4570-9dde-09684af23ffd@gmail.com/
> ---
>  drivers/net/wireless/realtek/rtw88/usb.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/usb.c b/usb.c
> index 1dfe7c6ae4ba..ff57976b9d3b 100644
> --- a/usb.c
> +++ b/usb.c
> @@ -440,23 +440,21 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>  {
>         const struct rtw_chip_info *chip = rtwdev->chip;
>         struct sk_buff *skb;
> -       unsigned int desclen, headsize, size;
> +       unsigned int size;
>         u8 qsel;
>         int ret = 0;
> 
>         size = pkt_info->tx_pkt_size;
>         qsel = pkt_info->qsel;
> -       desclen = chip->tx_pkt_desc_sz;
> -       headsize = pkt_info->offset ? pkt_info->offset : desclen;
> 
> -       skb = dev_alloc_skb(headsize + size);
> +       skb = dev_alloc_skb(chip->tx_pkt_desc_sz + size);
>         if (unlikely(!skb))
>                 return -ENOMEM;
> 
> -       skb_reserve(skb, headsize);
> +       skb_reserve(skb, chip->tx_pkt_desc_sz);
>         skb_put_data(skb, buf, size);
> -       skb_push(skb, headsize);
> -       memset(skb->data, 0, headsize);
> +       skb_push(skb, chip->tx_pkt_desc_sz);
> +       memset(skb->data, 0, chip->tx_pkt_desc_sz);
>         rtw_tx_fill_tx_desc(pkt_info, skb);
>         rtw_tx_fill_txdesc_checksum(rtwdev, pkt_info, skb->data);
> 
> @@ -471,12 +469,10 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>  static int rtw_usb_write_data_rsvd_page(struct rtw_dev *rtwdev, u8 *buf,
>                                         u32 size)
>  {
> -       const struct rtw_chip_info *chip = rtwdev->chip;
>         struct rtw_tx_pkt_info pkt_info = {0};
> 
>         pkt_info.tx_pkt_size = size;
>         pkt_info.qsel = TX_DESC_QSEL_BEACON;
> -       pkt_info.offset = chip->tx_pkt_desc_sz;

pkt_info.offset is still used by rtw_tx_fill_tx_desc(), so you can't remove
this line. I think this is because Larry NAKed this patch.
Ping-Ke Shih May 3, 2024, 12:38 a.m. UTC | #3
Larry Finger <larry.finger@gmail.com> wrote:
> 
> On 5/2/24 4:23 PM, Bitterblue Smith wrote:
> > The skb created in this function always has the same headroom,
> > the chip's TX descriptor size. Use chip->tx_pkt_desc_sz directly.
> >
> > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> > ---
> > This is the patch I promised earlier:
> > https://lore.kernel.org/linux-wireless/cae2d330-a4fb-4570-9dde-09684af23ffd@gmail.com/
> > ---
> >   drivers/net/wireless/realtek/rtw88/usb.c | 14 +++++---------
> >   1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/usb.c b/usb.c
> > index 1dfe7c6ae4ba..ff57976b9d3b 100644
> > --- a/usb.c
> > +++ b/usb.c
> > @@ -440,23 +440,21 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
> >   {
> >       const struct rtw_chip_info *chip = rtwdev->chip;
> >       struct sk_buff *skb;
> > -     unsigned int desclen, headsize, size;
> > +     unsigned int size;
> >       u8 qsel;
> >       int ret = 0;
> >
> >       size =pkt_info->tx_pkt_size;
> >       qsel = pkt_info->qsel;
> > -     desclen = chip->tx_pkt_desc_sz;
> > -     headsize = pkt_info->offset ? pkt_info->offset : desclen;
> >
> > -     skb = dev_alloc_skb(headsize + size);
> > +     skb = dev_alloc_skb(chip->tx_pkt_desc_sz + size);
> 
> When I added code to test if chip->tx_pkt_desc_sz was equal to
> pkt_info->tx_pkt_size at entry, it reported that there was a difference.

This patch didn't touch pkt_info->tx_pkt_size. Instead, I expected
pkt_info->offset was equal to chip->tx_pkt_desc_sz or 0.
(I replied the reason why it doesn't work by another mail in the same thread.)
Bitterblue Smith May 3, 2024, 9:53 a.m. UTC | #4
On 03/05/2024 01:18, Larry Finger wrote:
> On 5/2/24 4:23 PM, Bitterblue Smith wrote:
>> The skb created in this function always has the same headroom,
>> the chip's TX descriptor size. Use chip->tx_pkt_desc_sz directly.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>> This is the patch I promised earlier:
>> https://lore.kernel.org/linux-wireless/cae2d330-a4fb-4570-9dde-09684af23ffd@gmail.com/
>> ---
>>   drivers/net/wireless/realtek/rtw88/usb.c | 14 +++++---------
>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/usb.c b/usb.c
>> index 1dfe7c6ae4ba..ff57976b9d3b 100644
>> --- a/usb.c
>> +++ b/usb.c
>> @@ -440,23 +440,21 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>>   {
>>       const struct rtw_chip_info *chip = rtwdev->chip;
>>       struct sk_buff *skb;
>> -    unsigned int desclen, headsize, size;
>> +    unsigned int size;
>>       u8 qsel;
>>       int ret = 0;
>>          size =pkt_info->tx_pkt_size;
>>       qsel = pkt_info->qsel;
>> -    desclen = chip->tx_pkt_desc_sz;
>> -    headsize = pkt_info->offset ? pkt_info->offset : desclen;
>>   -    skb = dev_alloc_skb(headsize + size);
>> +    skb = dev_alloc_skb(chip->tx_pkt_desc_sz + size);
>>       if (unlikely(!skb))
>>           return -ENOMEM;
>>   -    skb_reserve(skb, headsize);
>> +    skb_reserve(skb, chip->tx_pkt_desc_sz);
>>       skb_put_data(skb, buf, size);
>> -    skb_push(skb, headsize);
>> -    memset(skb->data, 0, headsize);
>> +    skb_push(skb, chip->tx_pkt_desc_sz);
>> +    memset(skb->data, 0, chip->tx_pkt_desc_sz);
>>       rtw_tx_fill_tx_desc(pkt_info, skb);
>>       rtw_tx_fill_txdesc_checksum(rtwdev, pkt_info, skb->data);
>>   @@ -471,12 +469,10 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>>   static int rtw_usb_write_data_rsvd_page(struct rtw_dev *rtwdev, u8 *buf,
>>                       u32 size)
>>   {
>> -    const struct rtw_chip_info *chip = rtwdev->chip;
>>       struct rtw_tx_pkt_info pkt_info = {0};
>>         pkt_info.tx_pkt_size = size;
>>       pkt_info.qsel = TX_DESC_QSEL_BEACON;
>> -    pkt_info.offset = chip->tx_pkt_desc_sz;
>>         return rtw_usb_write_data(rtwdev, &pkt_info, buf);
>>   }
> 
> This patch doesn't work. When I add it and start an 8822bu, I get:
> 
> [   46.695755] usb 3-6: new high-speed USB device number 4 using xhci_hcd
> [   46.844397] usb 3-6: New USB device found, idVendor=0bda, idProduct=b82c, bcdDevice= 2.10
> [   46.844404] usb 3-6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> [   46.844406] usb 3-6: Product: 802.11ac NIC
> [   46.844408] usb 3-6: Manufacturer: Realtek
> [   46.844410] usb 3-6: SerialNumber: 123456
> [   47.524214] rtw_8822bu 3-6:1.2: Firmware version 27.2.0, H2C version 13
> [   47.573043] rtw_8822bu 3-6:1.2: error beacon valid
> [   47.573165] rtw_8822bu 3-6:1.2: failed to download rsvd page
> [   47.573488] rtw_8822bu 3-6:1.2: failed to download firmware
> [   47.576745] rtw_8822bu 3-6:1.2: failed to setup chip efuse info
> [   47.576750] rtw_8822bu 3-6:1.2: failed to setup chip information
> [   47.577302] rtw_8822bu 3-6:1.2: probe with driver rtw_8822bu failed with error -16
> 
> When I added code to test if chip->tx_pkt_desc_sz was equal to pkt_info->tx_pkt_size at entry, it reported that there was a difference.
> 
> This patch may work for some of the devices, but clearly not for all.
> 
> NACK.
> 
> Larry

Thank you for testing. Indeed, the second hunk is breaking
the firmware upload.
Bitterblue Smith May 3, 2024, 10:13 a.m. UTC | #5
On 03/05/2024 03:35, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> The skb created in this function always has the same headroom,
>> the chip's TX descriptor size. Use chip->tx_pkt_desc_sz directly.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>> This is the patch I promised earlier:
>> https://lore.kernel.org/linux-wireless/cae2d330-a4fb-4570-9dde-09684af23ffd@gmail.com/
>> ---
>>  drivers/net/wireless/realtek/rtw88/usb.c | 14 +++++---------
>>  1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/usb.c b/usb.c
>> index 1dfe7c6ae4ba..ff57976b9d3b 100644
>> --- a/usb.c
>> +++ b/usb.c
>> @@ -440,23 +440,21 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>>  {
>>         const struct rtw_chip_info *chip = rtwdev->chip;
>>         struct sk_buff *skb;
>> -       unsigned int desclen, headsize, size;
>> +       unsigned int size;
>>         u8 qsel;
>>         int ret = 0;
>>
>>         size = pkt_info->tx_pkt_size;
>>         qsel = pkt_info->qsel;
>> -       desclen = chip->tx_pkt_desc_sz;
>> -       headsize = pkt_info->offset ? pkt_info->offset : desclen;
>>
>> -       skb = dev_alloc_skb(headsize + size);
>> +       skb = dev_alloc_skb(chip->tx_pkt_desc_sz + size);
>>         if (unlikely(!skb))
>>                 return -ENOMEM;
>>
>> -       skb_reserve(skb, headsize);
>> +       skb_reserve(skb, chip->tx_pkt_desc_sz);
>>         skb_put_data(skb, buf, size);
>> -       skb_push(skb, headsize);
>> -       memset(skb->data, 0, headsize);
>> +       skb_push(skb, chip->tx_pkt_desc_sz);
>> +       memset(skb->data, 0, chip->tx_pkt_desc_sz);
>>         rtw_tx_fill_tx_desc(pkt_info, skb);
>>         rtw_tx_fill_txdesc_checksum(rtwdev, pkt_info, skb->data);
>>
>> @@ -471,12 +469,10 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>>  static int rtw_usb_write_data_rsvd_page(struct rtw_dev *rtwdev, u8 *buf,
>>                                         u32 size)
>>  {
>> -       const struct rtw_chip_info *chip = rtwdev->chip;
>>         struct rtw_tx_pkt_info pkt_info = {0};
>>
>>         pkt_info.tx_pkt_size = size;
>>         pkt_info.qsel = TX_DESC_QSEL_BEACON;
>> -       pkt_info.offset = chip->tx_pkt_desc_sz;
> 
> pkt_info.offset is still used by rtw_tx_fill_tx_desc(), so you can't remove
> this line. I think this is because Larry NAKed this patch. 
> 

Yes, that's the problem. I will send v2 without this hunk.
diff mbox series

Patch

diff --git a/usb.c b/usb.c
index 1dfe7c6ae4ba..ff57976b9d3b 100644
--- a/usb.c
+++ b/usb.c
@@ -440,23 +440,21 @@  static int rtw_usb_write_data(struct rtw_dev *rtwdev,
 {
 	const struct rtw_chip_info *chip = rtwdev->chip;
 	struct sk_buff *skb;
-	unsigned int desclen, headsize, size;
+	unsigned int size;
 	u8 qsel;
 	int ret = 0;
 
 	size = pkt_info->tx_pkt_size;
 	qsel = pkt_info->qsel;
-	desclen = chip->tx_pkt_desc_sz;
-	headsize = pkt_info->offset ? pkt_info->offset : desclen;
 
-	skb = dev_alloc_skb(headsize + size);
+	skb = dev_alloc_skb(chip->tx_pkt_desc_sz + size);
 	if (unlikely(!skb))
 		return -ENOMEM;
 
-	skb_reserve(skb, headsize);
+	skb_reserve(skb, chip->tx_pkt_desc_sz);
 	skb_put_data(skb, buf, size);
-	skb_push(skb, headsize);
-	memset(skb->data, 0, headsize);
+	skb_push(skb, chip->tx_pkt_desc_sz);
+	memset(skb->data, 0, chip->tx_pkt_desc_sz);
 	rtw_tx_fill_tx_desc(pkt_info, skb);
 	rtw_tx_fill_txdesc_checksum(rtwdev, pkt_info, skb->data);
 
@@ -471,12 +469,10 @@  static int rtw_usb_write_data(struct rtw_dev *rtwdev,
 static int rtw_usb_write_data_rsvd_page(struct rtw_dev *rtwdev, u8 *buf,
 					u32 size)
 {
-	const struct rtw_chip_info *chip = rtwdev->chip;
 	struct rtw_tx_pkt_info pkt_info = {0};
 
 	pkt_info.tx_pkt_size = size;
 	pkt_info.qsel = TX_DESC_QSEL_BEACON;
-	pkt_info.offset = chip->tx_pkt_desc_sz;
 
 	return rtw_usb_write_data(rtwdev, &pkt_info, buf);
 }