| Message ID | 681e03c1-d19e-44de-bc45-e71ce14c5ed2@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: rtw88: usb: Simplify rtw_usb_write_data | expand |
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
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.)
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.
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 --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); }
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(-)