RE: [PATCH rtw-next 2/2] wifi: rtw88: usb: Upload the firmware in bigger chunks

[Ping-Ke Shih <pkshih@realtek.com>]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> On 08/05/2025 06:29, Ping-Ke Shih wrote:
> > : Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >> RTL8811AU stops responding during the firmware download on some systems:
> >>
> >> [  809.256440] rtw_8821au 5-2.1:1.0: Firmware version 42.4.0, H2C version 0
> >> [  812.759142] rtw_8821au 5-2.1:1.0 wlp48s0f4u2u1: renamed from wlan0
> >> [  837.315388] rtw_8821au 1-4:1.0: write register 0x1ef4 failed with -110
> >> [  867.524259] rtw_8821au 1-4:1.0: write register 0x1ef8 failed with -110
> >> [  868.930976] rtw_8821au 5-2.1:1.0 wlp48s0f4u2u1: entered promiscuous mode
> >> [  897.730952] rtw_8821au 1-4:1.0: write register 0x1efc failed with -110
> >>
> >> Maybe it takes too long when writing the firmware 4 bytes at a time.
> >>
> >> Write 196 bytes at a time for RTL8821AU, RTL8811AU, and RTL8812AU,
> >> and 254 bytes at a time for RTL8723DU. These are the sizes used in
> >> their official drivers. Tested with all these chips.
> >>
> >> Cc: stable@vger.kernel.org
> >> Link: https://github.com/lwfinger/rtw88/issues/344
> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> >
> > Acked-by: Ping-Ke Shih <pkshih@realtek.com>
> >
> > [..]
> >
> >> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> >> index b16db579fdce..ad15ce12ab7f 100644
> >> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> >> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> >> @@ -165,6 +165,60 @@ static void rtw_usb_write32(struct rtw_dev *rtwdev, u32 addr, u32 val)
> >>         rtw_usb_write(rtwdev, addr, val, 4);
> >>  }
> >>
> >> +static void rtw_usb_write_firmware_page(struct rtw_dev *rtwdev, u32 page,
> >> +                                       const u8 *data, u32 size)
> >> +{
> >> +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> >> +       struct usb_device *udev = rtwusb->udev;
> >> +       u32 addr = FW_8192C_START_ADDRESS;
> >
> > FW_8192C_START_ADDRESS is existing already. But something like
> > RTW_USB_FW_START_ADDRESS would be better.
> >
> 
> I agree, because rtw88 doesn't handle RTL8192C. There is
> FW_START_ADDR_LEGACY in fw.h. I must not have noticed it before.
> Should I send v2 for this?

Yes, please. I don't change patch content during committing to prevent mess up
something. Since you only change the naming, please carry my Ack-by to next
version.

> 
> >> +       u8 *data_dup, *buf;
> >> +       u32 n, block_size;
> >> +       int ret;
> >> +
> >> +       switch (rtwdev->chip->id) {
> >> +       case RTW_CHIP_TYPE_8723D:
> >> +               block_size = 254;
> >> +               break;
> >> +       default:
> >> +               block_size = 196;
> >> +               break;
> >> +       }
> >> +
> >> +       data_dup = kmemdup(data, size, GFP_KERNEL);
> >
> > This is because type of argument `data` of usb_control_msg() is not const, right?
> > Do you know if usb_control_msg() will actually modify the data?
> >
> 
> No, it's because usb_control_msg() rejects memory allocated by
> vmalloc(). I don't remember what error it printed. Maybe because the
> memory is not suitable for DMA.

Do you mean firmware is placed in memory allocated by vmalloc()?
If so, it makes sense to the reason you said.