Hi, On Thu, Jun 15, 2023 at 3:13 AM Robin Murphy <robin.murphy@xxxxxxx> wrote: > > On 2023-06-15 00:59, Douglas Anderson wrote: > > This reverts commit 7bd6680b47fa4cd53ee1047693c09825e212a6f5. > > > > When booting a sc7180-trogdor based device on mainline, I see errors > > that look like this: > > > > qcom_scm firmware:scm: Assign memory protection call failed -22 > > qcom_rmtfs_mem 94600000.memory: assign memory failed > > qcom_rmtfs_mem: probe of 94600000.memory failed with error -22 > > > > The device still boots OK, but WiFi doesn't work. > > > > The failure only seems to happen when > > CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y. When I don't have that set then > > everything is peachy. Presumably something about the extra > > initialization disagrees with the change to drop cache invalidation. > > AFAICS init_on_alloc essentially just adds __GFP_ZERO to the page > allocation. Right, but it does so without `__GFP_ZERO` getting into the page flags. That means that this removal of "__GFP_ZERO" in dma_direct_alloc() doesn't actually remove the zeroing when CONFIG_INIT_ON_ALLOC_DEFAULT_ON IS USED: /* we always manually zero the memory once we are done */ page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO, true); > This should make no difference to a DMA allocation given > that dma_alloc_attrs explicitly zeros its allocation anyway. However... > for the non-coherent case, the DMA API's memset will be done through the > non-cacheable remap, while __GFP_ZERO can leave behind cached zeros for > the linear map alias. Thus what I assume must be happening here is that > "DMA" from the firmware is still making cacheable accesses to the buffer > and getting those zeros instead of whatever actual data which was > subsequently written non-cacheably direct to RAM. So either the firmware > still needs fixing to make non-cacheable accesses, or the SCM driver > needs to correctly describe it as coherent. I'm a little confused, but that's pretty normal for me. :-P Looking at the SCM driver, I see it doing the allocation in qcom_scm_assign_mem() as: dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL); Isn't that the SCM driver describing it as coherent? I guess the reason that the SCM driver is doing this is that it's passing a chunk of memory to the firmware but it passes it to firmware via a _physical_ address, not a virtual one. I guess this makes sense to do when passing a chunk of memory to firmware since you wouldn't want to pass the kernel's virtual address there... Presumably the fact that the firmware gets a physical address means that the firmware needs to map this address somehow itself. I can try to dig up what the firmware is doing if needed (what attributes it uses to map), but I guess the hope is that it shouldn't matter. As long as the kernel can guarantee that the contents that it needs have been flushed out to memory then I think we're supposed to be good, right? In any case, I dumped a stack crawl to try to show the path where the init happens, since there are lots of conditionals. I see this: kernel_init_pages+0x68/0x6c post_alloc_hook+0x40/0x90 prep_new_page+0x34/0x68 get_page_from_freelist+0x894/0xe64 __alloc_pages+0x12c/0xd24 __dma_direct_alloc_pages+0x9c/0x170 dma_direct_alloc+0x254/0x4bc dma_alloc_attrs+0xe4/0x1e4 qcom_scm_assign_mem+0xb0/0x258 So looking at dma_direct_alloc(), I guess: 1. We call __dma_direct_alloc_pages() to allocate the page. We try to turn off __GFP_ZERO but CONFIG_INIT_ON_ALLOC_DEFAULT_ON overrides us. 2. As part of __dma_direct_alloc_pages(), we use the existing (cachable) mapping of the page and write zeros. 3. The "remap" variable is set for arm64 so we call arch_dma_prep_coherent(). That used to do a "clean and invalidate" but now does just a "clean" and that's what broke me. Talking out of my rear end, I guess the issue here might be that it _does_ matter how the firmware maps this memory because it has to match how the kernel has it mapped because if you map the same physical memory twice then the attributes need to match. Maybe the old "invalidate" just worked around this issue? If this wild guessing is correct, maybe a more correct solution would be to simply unmap the memory from the kernel before passing the physical address to the firmware, if that's possible... -Doug
