On Mon, Feb 05, 2024 at 12:04:21PM +0100, Alexander Lobakin wrote:
> Quite often, NIC devices do not need dma_sync operations on x86_64
> at least.
This is a fundamental property of the platform being DMA coherent,
and devices / platforms not having addressing limitations or other
need for bounce buffering (like all those whacky trusted platform
schemes). Nothing NIC-specific here.
> In case some device doesn't work with the shortcut:
> * include <linux/dma-map-ops.h> to the driver source;
> * call dma_set_skip_sync(dev, false) at the beginning of the probe
> callback. This will disable the shortcut and force DMA syncs.
No, drivers should never include dma-map-ops.h. If we have a legit
reason for drivers to ever call it it would have to move to
dma-mapping.h. But I see now reason why there would be such a need.
For now I'd suggest simply dropping this paragraph from the commit
message.
> if (dma_map_direct(dev, ops))
> + /*
> + * dma_skip_sync could've been set to false on first SWIOTLB
> + * buffer mapping, but @dma_addr is not necessary an SWIOTLB
> + * buffer. In this case, fall back to more granular check.
> + */
> return dma_direct_need_sync(dev, dma_addr);
> +
Nit: with such a long block comment adding curly braces would make the
code a bit more readable.
> +#ifdef CONFIG_DMA_NEED_SYNC
> +void dma_setup_skip_sync(struct device *dev)
> +{
> + const struct dma_map_ops *ops = get_dma_ops(dev);
> + bool skip;
> +
> + if (dma_map_direct(dev, ops))
> + /*
> + * dma_skip_sync will be set to false on first SWIOTLB buffer
> + * mapping, if any. During the device initialization, it's
> + * enough to check only for DMA coherence.
> + */
> + skip = dev_is_dma_coherent(dev);
> + else if (!ops->sync_single_for_device && !ops->sync_single_for_cpu)
> + /*
> + * Synchronization is not possible when none of DMA sync ops
> + * is set. This check precedes the below one as it disables
> + * the synchronization unconditionally.
> + */
> + skip = true;
> + else if (ops->flags & DMA_F_CAN_SKIP_SYNC)
> + /*
> + * Assume that when ``DMA_F_CAN_SKIP_SYNC`` is advertised,
> + * the conditions for synchronizing are the same as with
> + * the direct DMA.
> + */
> + skip = dev_is_dma_coherent(dev);
> + else
> + skip = false;
> +
> + dma_set_skip_sync(dev, skip);
I'd just assign directly to dev->dma_skip_sync instead of using a
local variable and the dma_set_skip_sync call - we are under
ifdef CONFIG_DMA_NEED_SYNC here and thus know is is available.
> +static inline void swiotlb_disable_dma_skip_sync(struct device *dev)
> +{
> + /*
> + * If dma_skip_sync was set, reset it to false on first SWIOTLB buffer
> + * mapping/allocation to always sync SWIOTLB buffers.
> + */
> + if (unlikely(dma_skip_sync(dev)))
> + dma_set_skip_sync(dev, false);
> +}
Nothing really swiotlb-specific here. Also the naming is a bit odd.
Maybe have a dma_set_skip_sync helper without the bool to enable
skipping, and a dma_clear_skip_sync that clear the flag. The optimization
to first check the flag here could just move into that latter
helper.
