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

[Arnd Bergmann <arnd@arndb.de>]

On Mon, May 2, 2022 at 4:47 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> > Am 02.05.2022 um 16:06 schrieb Arnd Bergmann <arnd@arndb.de>:
> > On Mon, May 2, 2022 at 2:38 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >
> > The approach in the wlcore driver appears to be simpler because it
> > avoids dynamic memory allocation and the associated error handling.
>
> It looks as if it just avoids kmalloc/free sequences in event handling
> by allocating a big enough buffer once.
>
> wl1271_cmd_wait_for_event_or_timeout() allocates it like we do now.

Ah right, I missed that one.

> > 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.
>
> Well, there seems to be quite some common code but indeed devices
> using these older chips are getting rare so it is probably not worth
> combining code. And testing needs someone who owns boards
> with both chips...

No, I wasn't even thinking of combining code, just whether there
is value in keeping the similar bits in sync to ensure we have the
same set of bugs on both ;-)

I think the best fix for both drivers would be to keep the DMA
members (partition and buffer_32) in the respective device
structures, but mark each one as aligned.

> > 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.
>
> wl1251_event_wait() seems to work with the masks provided by code.
> So I guess the conversion to le32 is harmless on the OpenPandora.
> Most likely it should be done on big endian devices. I.e. we might have
> done the right thing.
>
> Let's see if someone compains or knows more. Otherwise we should
> fix it just for the Pandora and N900 (both omap3 based) as the only
> upstream users.

Ok. In general I like ensure we keep things working for big-endian
kernels, which are meant to work on any ARMv6+ or newer machine.
Most of the time it's just a couple of drivers (like this one) that get
in the way of actually doing it, but then again very few people ever
care about big-endian ARMv6/v7.

If we don't have a reason to believe this one is actually wrong, I think
fixing the endian issue silently is fine, as is ignoring the potential
other endian issues in the same driver that nobody complained about
in the past decade ;-)

       Arnd