Re: [PATCH] wl1251: dynamically allocate memory used for DMA

[Arnd Bergmann <arnd@arndb.de>]

On Mon, May 2, 2022 at 2:38 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> With introduction of vmap'ed stacks, stack parameters can no
> longer be used for DMA and now leads to kernel panic.
>
> It happens at several places for the wl1251 (e.g. when
> accessed through SDIO) making it unuseable on e.g. the
> OpenPandora.
>
> We solve this by allocating temporary buffers or use wl1251_read32().

This looks all correct to me. I had another look at the related wlcore
driver now,
and see that the same problem existed there but was fixed back in 2012
in a different way, see 690142e98826 ("wl12xx: fix DMA-API-related warnings").

The approach in the wlcore driver appears to be simpler because it
avoids dynamic memory allocation and the associated error handling.
However, it probably makes another problem worse that also exists
here:

 static inline u32 wl1251_read32(struct wl1251 *wl, int addr)
 {
       u32 response;
       wl->if_ops->read(wl, addr, &wl->buffer_32, sizeof(wl->buffer_32));
       return le32_to_cpu(wl->buffer_32);
 }

I think the 'buffer_32' member of 'struct wl1251' needs an explicit
'__cacheline_aligned' attribute to avoid potentially clobbering
some of the structure during a DMA write.

I don't know if anyone cares enough about the two drivers to
have an opinion. I've added Luca to Cc, but he hasn't maintained
the driver since 2013 and probably doesn't.

It's probably ok to just apply your patch for the moment to fix
the regression we saw on the machines that we know use this.

One more detail:

> diff --git a/drivers/net/wireless/ti/wl1251/event.c b/drivers/net/wireless/ti/wl1251/event.c
> index e6d426edab56b..e945aafd88ee5 100644
> --- a/drivers/net/wireless/ti/wl1251/event.c
> +++ b/drivers/net/wireless/ti/wl1251/event.c
> @@ -169,11 +169,9 @@ int wl1251_event_wait(struct wl1251 *wl, u32 mask, int timeout_ms)
>                 msleep(1);
>
>                 /* read from both event fields */
> -               wl1251_mem_read(wl, wl->mbox_ptr[0], &events_vector,
> -                               sizeof(events_vector));
> +               events_vector = wl1251_mem_read32(wl, wl->mbox_ptr[0]);
>                 event = events_vector & mask;
> -               wl1251_mem_read(wl, wl->mbox_ptr[1], &events_vector,
> -                               sizeof(events_vector));
> +               events_vector = wl1251_mem_read32(wl, wl->mbox_ptr[1]);
>                 event |= events_vector & mask;

This appears to change endianness of the data, on big-endian kernels.
Is that intentional?

My first guess would be that the driver never worked correctly on big-endian
machines, and that the change is indeed correct, but on the other hand
the conversion was added in commit ac9e2d9afa90 ("wl1251: convert
32-bit values to le32 before writing to the chip") in a way that suggests it
was meant to work on both.

       Arnd