On 2019-02-18 16:03, Stanislaw Gruszka wrote: > On Mon, Feb 18, 2019 at 03:43:26PM +0100, Felix Fietkau wrote: >> On 2019-02-18 14:52, Stanislaw Gruszka wrote: >> > On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote: >> >> this is a misunderstanding. The warning is about memory alignment to 32 >> >> bit addresses, not about page alignment. This is a typical ARM >> >> restriction. Maybe we need to make sure in mt76 that the DMA buffer needs >> >> to be aligned. But it's also possible that the warning isn't the root >> >> cause of our problem. >> >> >> > >> > I see, it needs 4 bytes alignment . There is already dwc2 code checks >> > that and allocate new buffer if the alignment is not right: >> > dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg >> > is not NULL. I thought mt76usb already provide aligned buffers, but >> > looks it does not for one TX special case, which are PROBE REQUEST >> > frames. Other frames are aligned by inserting L2 header pad. One >> > solution for this would be just submit urb with NULL sg (same as >> > Lorenzo's patches do, but still allocating buffers via buf->sg), >> > but I think, you have right, we should provide 4 bytes aligned buffers >> > by default as other DMA hardware may require that. I'm attaching yet >> > another patch to test, which fix up alignment for PROBE REQUEST frames. >> This approach looks completely wrong to me. MMIO based hardware does not >> need 4-byte aligned buffers at all, other USB controllers do not need >> this either. >> As Lorenzo already pointed out, re-aligning the buffer is *very* >> expensive, so we should not do this in the driver just to work around >> quirks in a particular USB host driver. > > I decided to this patch because I thought some other USB & MMIO DMA > platforms might also require this alignment. But it was never triggered > in MMIO because on those mt76 is used in AP mode, hence no PROBE > REQUEST frames (and scan can be passive on STA mode). mt76 is regularly used and tested in STA and Mesh mode as well. No DMA alignment related issues there.
>> The way I see it, we have two choices. >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL > > I agree, if this is only needed for dwc2. Though I would investigate > if this is not a bug on other platforms as well. >From what I can see, using Lorenzo's patches seems to be the better solution, since they avoid these corner cases in dwc2 (and maybe other drivers as well). I will apply them and then we'll see if we need to do any further improvements later on. - Felix
