On Tue, Oct 29, 2019 at 11:57:21AM +0100, Mark Kettenis wrote: > > Date: Tue, 29 Oct 2019 11:31:31 +0100 > > From: Stefan Sperling <[email protected]> > > > > On Mon, Oct 28, 2019 at 08:48:41PM +0100, Mark Kettenis wrote: > > > > Date: Mon, 28 Oct 2019 19:28:29 +0100 > > > > From: Stefan Sperling <[email protected]> > > > > > > > > Newer iwm(4) firmware versions will require paging to host DRAM. > > > > This diff implements support for this. It was written by Imre Vadasz in > > > > 2017. > > > > > > > > I would like to get review by people with experience in the kernel > > > > memory > > > > subsystem and DMA, to check that this diff isn't doing something stupid. > > > > > > I have trouble understanding what the bus_dmamap_sync() calls in > > > iwm_alloc_fw_paging_mem() are supposed to achieve. > > > > The driver has to initialize these pages with data provided in the firmware > > image. This is done with memcpy(). I guess these bus_dma_sync calls are > > supposed to ensure that the correct data appears on the device side when > > the pages are read from there. Note that the syncs occur both before > > memcpy() > > in iwm_fill_paging_mem() and after memcpy() in iwm_send_paging_cmd(). > > x86-specific behaviour aside, is there any reason why those syncs should > > not be required? > > Unless the deviceactually writes to those pages, the syncs before the > memcpy() should not be necessary. You do need to sync after the > memcpy() although on x86 that is mostly for the compiler's benefit, to > make sure it doesn't reorder things in a way that pokes the hardware > before the memcpy() is done.
But syncs won't do any harm even in case the device does not perform any writes, correct? I haven't yet checked whether the firmware actually modifies those pages. And I don't know if such behaviour would always occur or be triggered in specific circumstances. > > I am a bit worried that iwm_fw_paging() is only called in error paths. > > Allocated pages should probably be torn down either when the device goes > > down, or when it is detached. We don't currently implement detach in this > > driver, so the pages are never freed if no errors occur (the same applies > > to Rx/Tx rings, so perhaps keeping the allocation persistent is OK). > > Do you think we should alloc/free these pages whenever we do up/down? > > That might not be the best approach as you run the risk that > allocations will fail if you up the interface again later on. This is > more likely to be an issue if you need phys contig memory, as such > allocations may fail even when there is plenty of free memory due to > fragmentations. Right. So a persistent allocation, as implemented by the patch, makes sense.
