2023-05-14 20:11:49

by Martin Blumenstingl

[permalink] [raw]
Subject: [PATCH wireless v1] wifi: rtw88: sdio: Always use two consecutive bytes for word operations

The Allwinner sunxi-mmc controller cannot handle word (16 bit)
transfers. So and sdio_{read,write}w fails with messages like the
following example using an RTL8822BS (but the same problems were also
observed with RTL8822CS and RTL8723DS chips):
rtw_8822bs mmc1:0001:1: Firmware version 27.2.0, H2C version 13
sunxi-mmc 4021000.mmc: unaligned scatterlist: os f80 length 2
sunxi-mmc 4021000.mmc: map DMA failed
rtw_8822bs mmc1:0001:1: sdio read16 failed (0x10230): -22

Use two consecutive single byte accesses for word operations instead. It
turns out that upon closer inspection this is also what the vendor
driver does, even though it does have support for sdio_{read,write}w. So
we can conclude that the rtw88 chips do support word access but only on
SDIO controllers that also support it. Since there's no way to detect if
the controller supports word access or not the rtw88 sdio driver
switches to the easiest approach: avoiding word access.

Reported-by: Larry Finger <[email protected]>
Closes: https://lore.kernel.org/linux-wireless/[email protected]/
Reported-by: Rudi Heitbaum <[email protected]>
Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/net/wireless/realtek/rtw88/sdio.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index af0459a79899..06fce7c3adda 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -87,11 +87,6 @@ static void rtw_sdio_writew(struct rtw_dev *rtwdev, u16 val, u32 addr,
u8 buf[2];
int i;

- if (rtw_sdio_use_memcpy_io(rtwdev, addr, 2)) {
- sdio_writew(rtwsdio->sdio_func, val, addr, err_ret);
- return;
- }
-
*(__le16 *)buf = cpu_to_le16(val);

for (i = 0; i < 2; i++) {
@@ -125,9 +120,6 @@ static u16 rtw_sdio_readw(struct rtw_dev *rtwdev, u32 addr, int *err_ret)
u8 buf[2];
int i;

- if (rtw_sdio_use_memcpy_io(rtwdev, addr, 2))
- return sdio_readw(rtwsdio->sdio_func, addr, err_ret);
-
for (i = 0; i < 2; i++) {
buf[i] = sdio_readb(rtwsdio->sdio_func, addr + i, err_ret);
if (*err_ret)
--
2.40.1



2023-05-15 00:42:24

by Ping-Ke Shih

[permalink] [raw]
Subject: RE: [PATCH wireless v1] wifi: rtw88: sdio: Always use two consecutive bytes for word operations



> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Monday, May 15, 2023 4:04 AM
> To: [email protected]
> Cc: [email protected]; Ping-Ke Shih <[email protected]>; [email protected];
> [email protected]; Martin Blumenstingl <[email protected]>; Larry Finger
> <[email protected]>; Rudi Heitbaum <[email protected]>
> Subject: [PATCH wireless v1] wifi: rtw88: sdio: Always use two consecutive bytes for word operations
>
> The Allwinner sunxi-mmc controller cannot handle word (16 bit)
> transfers. So and sdio_{read,write}w fails with messages like the
> following example using an RTL8822BS (but the same problems were also
> observed with RTL8822CS and RTL8723DS chips):
> rtw_8822bs mmc1:0001:1: Firmware version 27.2.0, H2C version 13
> sunxi-mmc 4021000.mmc: unaligned scatterlist: os f80 length 2
> sunxi-mmc 4021000.mmc: map DMA failed
> rtw_8822bs mmc1:0001:1: sdio read16 failed (0x10230): -22
>
> Use two consecutive single byte accesses for word operations instead. It
> turns out that upon closer inspection this is also what the vendor
> driver does, even though it does have support for sdio_{read,write}w. So
> we can conclude that the rtw88 chips do support word access but only on
> SDIO controllers that also support it. Since there's no way to detect if
> the controller supports word access or not the rtw88 sdio driver
> switches to the easiest approach: avoiding word access.
>
> Reported-by: Larry Finger <[email protected]>
> Closes: https://lore.kernel.org/linux-wireless/[email protected]/

"Closes:" seems not a regular tag. Use "Link: " instead.

> Reported-by: Rudi Heitbaum <[email protected]>

Followed by a "Link: " if you have another one.

> Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/net/wireless/realtek/rtw88/sdio.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index af0459a79899..06fce7c3adda 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -87,11 +87,6 @@ static void rtw_sdio_writew(struct rtw_dev *rtwdev, u16 val, u32 addr,
> u8 buf[2];
> int i;
>
> - if (rtw_sdio_use_memcpy_io(rtwdev, addr, 2)) {
> - sdio_writew(rtwsdio->sdio_func, val, addr, err_ret);
> - return;
> - }
> -
> *(__le16 *)buf = cpu_to_le16(val);
>
> for (i = 0; i < 2; i++) {
> @@ -125,9 +120,6 @@ static u16 rtw_sdio_readw(struct rtw_dev *rtwdev, u32 addr, int *err_ret)
> u8 buf[2];
> int i;
>
> - if (rtw_sdio_use_memcpy_io(rtwdev, addr, 2))
> - return sdio_readw(rtwsdio->sdio_func, addr, err_ret);
> -
> for (i = 0; i < 2; i++) {
> buf[i] = sdio_readb(rtwsdio->sdio_func, addr + i, err_ret);
> if (*err_ret)
> --
> 2.40.1


2023-05-15 11:55:41

by Ping-Ke Shih

[permalink] [raw]
Subject: Re: [PATCH wireless v1] wifi: rtw88: sdio: Always use two consecutive bytes for word operations

On Mon, 2023-05-15 at 13:59 +0300, Kalle Valo wrote:
>
> Ping-Ke Shih <[email protected]> writes:
>
> > > -----Original Message-----
> > > From: Martin Blumenstingl <[email protected]>
> > > Sent: Monday, May 15, 2023 4:04 AM
> > > To: [email protected]
> > > Cc: [email protected]; Ping-Ke Shih <[email protected]>; [email protected];
> > > [email protected]; Martin Blumenstingl <[email protected]>; Larry Finger
> > > <[email protected]>; Rudi Heitbaum <[email protected]>
> > > Subject: [PATCH wireless v1] wifi: rtw88: sdio: Always use two consecutive bytes for word
> > > operations
> > >
> > > The Allwinner sunxi-mmc controller cannot handle word (16 bit)
> > > transfers. So and sdio_{read,write}w fails with messages like the
> > > following example using an RTL8822BS (but the same problems were also
> > > observed with RTL8822CS and RTL8723DS chips):
> > > rtw_8822bs mmc1:0001:1: Firmware version 27.2.0, H2C version 13
> > > sunxi-mmc 4021000.mmc: unaligned scatterlist: os f80 length 2
> > > sunxi-mmc 4021000.mmc: map DMA failed
> > > rtw_8822bs mmc1:0001:1: sdio read16 failed (0x10230): -22
> > >
> > > Use two consecutive single byte accesses for word operations instead. It
> > > turns out that upon closer inspection this is also what the vendor
> > > driver does, even though it does have support for sdio_{read,write}w. So
> > > we can conclude that the rtw88 chips do support word access but only on
> > > SDIO controllers that also support it. Since there's no way to detect if
> > > the controller supports word access or not the rtw88 sdio driver
> > > switches to the easiest approach: avoiding word access.
> > >
> > > Reported-by: Larry Finger <[email protected]>
> > > Closes:
> > > https://lore.kernel.org/linux-wireless/[email protected]/
> >
> > "Closes:" seems not a regular tag. Use "Link: " instead.
>
> Actually the documentation now talks about Closes tag:
>
> https://docs.kernel.org/process/5.Posting.html#patch-formatting-and-changelogs
>
> I guess this tag is a recent addition?
>

Thanks for information.

Then, this patch looks good to me.

Reviewed-by: Ping-Ke Shih <[email protected]>

2023-05-15 12:27:02

by Julian Calaby

[permalink] [raw]
Subject: Re: [PATCH wireless v1] wifi: rtw88: sdio: Always use two consecutive bytes for word operations

Hi Martin,

On Mon, May 15, 2023 at 6:12 AM Martin Blumenstingl
<[email protected]> wrote:
>
> The Allwinner sunxi-mmc controller cannot handle word (16 bit)
> transfers. So and sdio_{read,write}w fails with messages like the
> following example using an RTL8822BS (but the same problems were also
> observed with RTL8822CS and RTL8723DS chips):
> rtw_8822bs mmc1:0001:1: Firmware version 27.2.0, H2C version 13
> sunxi-mmc 4021000.mmc: unaligned scatterlist: os f80 length 2
> sunxi-mmc 4021000.mmc: map DMA failed
> rtw_8822bs mmc1:0001:1: sdio read16 failed (0x10230): -22
>
> Use two consecutive single byte accesses for word operations instead. It
> turns out that upon closer inspection this is also what the vendor
> driver does, even though it does have support for sdio_{read,write}w. So
> we can conclude that the rtw88 chips do support word access but only on
> SDIO controllers that also support it. Since there's no way to detect if
> the controller supports word access or not the rtw88 sdio driver
> switches to the easiest approach: avoiding word access.
>
> Reported-by: Larry Finger <[email protected]>
> Closes: https://lore.kernel.org/linux-wireless/[email protected]/
> Reported-by: Rudi Heitbaum <[email protected]>
> Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
> Signed-off-by: Martin Blumenstingl <[email protected]>

The linux-sunxi folks might have comments, so I've added them to CC.

> ---
> drivers/net/wireless/realtek/rtw88/sdio.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index af0459a79899..06fce7c3adda 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -87,11 +87,6 @@ static void rtw_sdio_writew(struct rtw_dev *rtwdev, u16 val, u32 addr,
> u8 buf[2];
> int i;
>
> - if (rtw_sdio_use_memcpy_io(rtwdev, addr, 2)) {
> - sdio_writew(rtwsdio->sdio_func, val, addr, err_ret);
> - return;
> - }
> -
> *(__le16 *)buf = cpu_to_le16(val);
>
> for (i = 0; i < 2; i++) {
> @@ -125,9 +120,6 @@ static u16 rtw_sdio_readw(struct rtw_dev *rtwdev, u32 addr, int *err_ret)
> u8 buf[2];
> int i;
>
> - if (rtw_sdio_use_memcpy_io(rtwdev, addr, 2))
> - return sdio_readw(rtwsdio->sdio_func, addr, err_ret);
> -
> for (i = 0; i < 2; i++) {
> buf[i] = sdio_readb(rtwsdio->sdio_func, addr + i, err_ret);
> if (*err_ret)
> --
> 2.40.1
>

Thanks,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/