Christian Lamparter <chunkeey@xxxxxxxxx> writes: > On 2020-08-26 18:02, Kalle Valo wrote: >> Christian Lamparter <chunkeey@xxxxxxxxx> writes: >> >>> On 2020-08-02 15:29, Jia-Ju Bai wrote: >>>> In p54p_tx(), skb->data is mapped to streaming DMA on line 337: >>>> mapping = pci_map_single(..., skb->data, ...); >>>> >>>> Then skb->data is accessed on line 349: >>>> desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; >>>> >>>> This access may cause data inconsistency between CPU cache and hardware. >>>> >>>> To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in >>>> a local variable before DMA mapping, and then the driver accesses this >>>> local variable instead of skb->data. >>> >>> Interesting. Please bear with me here. From my understanding, the >>> streaming direction is set to PCI_DMA_TODEVICE. So is it really >>> possible for the hardware to interfere with the data without the IOMMU >>> catching this? >> >> Also is there any documentation about this scenario? I would like to >> understand this better. > > I usually rely on the information present in Documentation: > <https://www.kernel.org/doc/Documentation/DMA-API-HOWTO.txt> > > The relevant extract for p54's DMA_TO_DEVICE decision likely comes from: > > "For Networking drivers, it's a rather simple affair. For transmit > packets, map/unmap them with the DMA_TO_DEVICE direction > specifier. For receive packets, just the opposite, map/unmap them > with the DMA_FROM_DEVICE direction specifier." > > "Only streaming mappings specify a direction, consistent mappings > implicitly have a direction attribute setting of DMA_BIDIRECTIONAL." This is not very clearly written, I guess it's assumed everyone know this stuff :) > But looking around on the Internet, I came across this in "Chapter 15. > Memory Mapping and DMA" of the Linux Device Drivers, 3rd Edition: > > <https://www.oreilly.com/library/view/linux-device-drivers/0596005903/ch15.html> > > |Setting up streaming DMA mappings > |[...] > | > |Some important rules apply to streaming DMA mappings: > | * [...] > | > | * Once a buffer has been mapped, it belongs to the device, not the > | processor. Until the buffer has been unmapped, the driver should > not | touch its contents in any way. Only after dma_unmap_single has > been | called is it safe for the driver to access the contents of > the > | buffer (with one exception that we see shortly). Among other things, > | this rule implies that a buffer being written to a device cannot be > | mapped until it contains all the data to write." > | > | [...] (More informative text, but only) > > From the sentence "Once a buffer has been mapped, it belongs to the > device, not the processor". I think that Jia-Ju Bai's patch is doing > exactly this "by the book". Yeah, this is much better and understandable. Thanks for checking. > Therefore, it should be applied and backported: > > Cc: <stable@xxxxxxxxxxxxxxx> Ok, I'll add that. -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
