On 2022-06-29 12:02, Robin Murphy wrote: > On 2022-06-29 16:39, Logan Gunthorpe wrote: >> On 2022-06-29 03:05, Robin Murphy wrote: >>> On 2022-06-15 17:12, Logan Gunthorpe wrote: >>> Does this serve any useful purpose? If a page is determined to be device >>> memory, it's not going to suddenly stop being device memory, and if the >>> underlying sg is recycled to point elsewhere then sg_assign_page() will >>> still (correctly) clear this flag anyway. Trying to reason about this >>> beyond superficial API symmetry - i.e. why exactly would a caller need >>> to call it, and what would the implications be of failing to do so - >>> seems to lead straight to confusion. >>> >>> In fact I'd be inclined to have sg_assign_page() be responsible for >>> setting the flag automatically as well, and thus not need >>> sg_dma_mark_bus_address() either, however I can see the argument for >>> doing it this way round to not entangle the APIs too much, so I don't >>> have any great objection to that. >> >> Yes, I think you misunderstand what this is for. The SG_DMA_BUS_ADDDRESS >> flag doesn't mark the segment for the page, but for the dma address. It >> cannot be set in sg_assign_page() seeing it's not a property of the page >> but a property of the dma_address in the sgl. >> >> It's not meant for use by regular SG users, it's only meant for use >> inside DMA mapping implementations. The purpose is to know whether a >> given dma_address in the SGL is a bus address or regular memory because >> the two different types must be unmapped differently. We can't rely on >> the page because, as you know, many dma_map_sg() the dma_address entry >> in the sgl does not map to the same memory as the page. Or to put it >> another way: is_pci_p2pdma_page(sg->page) does not imply that >> sg->dma_address points to a bus address. >> >> Does that make sense? > > Ah, you're quite right, in trying to take in the whole series at once > first thing in the morning I did fail to properly grasp that detail, so > indeed the sg_assign_page() thing couldn't possibly work, but as I said > that's fine anyway. I still think the lifecycle management is a bit off > though - equivalently, a bus address doesn't stop being a bus address, > so it would seem appropriate to update this flag appropriately whenever > sg_dma_address() is assigned to, and not when it isn't. Yes, that's pretty much the way the code is now. The only two places sg_dma_mark_bus_address() is called are in the two pci_p2pdma helpers that set the dma address to the bus address. The lines before both calls set the dma_address and dma_len. Logan
