Re: Re: Re: [PATCH 1/3] dmaengine: ptdma: Remove obsolete 32-bit DMA mask fallback
From: Frank Li
Date: Mon Sep 14 2026 - 11:04:33 EST
On Mon, Sep 14, 2026 at 11:25:06AM +0800, Ruizhe Zhou wrote:
>
> >> Hi Frank,
> >>
> >>
> >> From: Frank Li <Frank.li@xxxxxxxxxxx>
> >> Date: 2026-09-04 03:26:36
> >> To: Ruizhe Zhou <zhouruizhe@xxxxxxxxxxx>
> >> Cc: Vinod Koul <vkoul@xxxxxxxxxx>,Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>,Logan Gunthorpe <logang@xxxxxxxxxxxx>,Orson Zhai <orsonzhai@xxxxxxxxx>,Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>,Frank Li <Frank.Li@xxxxxxxxxx>,Chunyan Zhang <zhang.lyra@xxxxxxxxx>,dmaengine@xxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH 1/3] dmaengine: ptdma: Remove obsolete 32-bit DMA mask fallback>On Thu, Sep 03, 2026 at 07:54:39PM +0800, Ruizhe Zhou wrote:
> >> >> [You don't often get email from zhouruizhe@xxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >> >>
> >> >> The DMA API guarantees support for masks of 32 bits or wider and
> >> >> explicitly identifies retrying a 32-bit mask after a wider request as
> >> >> incorrect:
> >> >> https://docs.kernel.org/core-api/dma-api-howto.html#dma-addressing-capabilities
> >> >>
> >> >> Remove the obsolete fallback while retaining the error check so that a
> >> >> genuine DMA setup failure is still reported and aborts initialization.
> >> >>
> >> >> Signed-off-by: Ruizhe Zhou <zhouruizhe@xxxxxxxxxxx>
> >> >> ---
> >> >> drivers/dma/amd/ptdma/ptdma-pci.c | 8 ++------
> >> >> 1 file changed, 2 insertions(+), 6 deletions(-)
> >> >>
> >> >> diff --git a/drivers/dma/amd/ptdma/ptdma-pci.c b/drivers/dma/amd/ptdma/ptdma-pci.c
> >> >> index 22739ff0c3c5..d36bb9c67325 100644
> >> >> --- a/drivers/dma/amd/ptdma/ptdma-pci.c
> >> >> +++ b/drivers/dma/amd/ptdma/ptdma-pci.c
> >> >> @@ -178,12 +178,8 @@ static int pt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> >>
> >> >> ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48));
> >> >> if (ret) {
> >> >> - ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> >> >> - if (ret) {
> >> >> - dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n",
> >> >> - ret);
> >> >> - goto e_err;
> >> >> - }
> >> >> + dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n", ret);
> >> >> + goto e_err;
> >> >
> >> >also needn't check return value, it always return success if mask >= 32.
> >
> >Add Christophe JAILLET and Christoph Hellwig
> >
> >https://lore.kernel.org/all/6a4df3e0a0849f179f9747f47b9c8cae53b29b59.1641752692.git.christophe.jaillet@xxxxxxxxxx/
> >
> >Frank
>
> Hi Christophe,
>
> I dug up your audit back in 2022, and it seems that you did a similar investigation:
> https://lore.kernel.org/all/97ef1b73-a9a4-6018-d52c-4108ff9de7ca@xxxxxxxxxx/
> Mind sharing your thoughts on this matter? As apparently my audit tells me that
> 64-bit mask may fail on certain arch, so the return check is needed even in that case.
>
> Also, Frank, I sent this to Hellwig, and he suggests me to fix those backends:
> https://lore.kernel.org/all/20260907054733.GA31481@xxxxxx/
> But once again, I am not well-informed on those particular backends and their HW requirements,
> so I'm not sure what exactly should be done here. Maybe cc the people who maintain these?
It is not big deal to check or not check return value about this patch.
The key point is that we need make thing clear and move to one directions.
If everyone agree, it should never return failure when >=32, we should
follow it to fix wrong implement. If not, we should add check and provide
clean document why it is failure ane what's means of these failure.
Avoid to discuss these again and again.
For my view, dma_set_mask_and_coherent() should indicate dev's DMA address
width only, which regardless system.
for example, Device A support 36bit address width, which should work at any
DMA supported system. system should allocate match devices's and related
parent bus requirement and provide DMA space address, which may difference
with CPU address space. This work should be done at dma mapping, not here.
Frank
>
> Best,
> Ruizhe
> >
> >>
> >> I did some more checking after your reply to make sure I understand
> >> whether dma_set_mask_and_coherent() can actually fail for a mask wider
> >> than 32 bits, and whether keeping the return-value check is necessary.
> >>
> >> I also read your August discussion with Michal Pecio on the same
> >> question:
> >>
> >> https://lore.kernel.org/all/aoMty9Dvt2bnWm74@SMW015318/
> >>
> >> In that discussion you pointed out that dev->dma_mask should already be
> >> initialized by the bus before driver probe, and asked whether there are
> >> any dma_supported() implementations which can actually reject such a
> >> mask.
> >>
> >> I agree that !dev->dma_mask does not look like an interesting failure
> >> case for a normally probed device. However, after going through the
> >> current dma_supported() paths and the in-tree .dma_supported callbacks,
> >> I found several other cases which appear to make the stronger "cannot
> >> fail for >32 bits" statement incorrect.
> >>
> >> The current path in kernel/dma/mapping.c is:
> >>
> >> static int dma_supported(struct device *dev, u64 mask)
> >> {
> >> const struct dma_map_ops *ops = get_dma_ops(dev);
> >>
> >> if (use_dma_iommu(dev)) {
> >> if (WARN_ON(ops))
> >> return false;
> >> return true;
> >> }
> >>
> >> if (ops) {
> >> if (!ops->dma_supported)
> >> return true;
> >> return ops->dma_supported(dev, mask);
> >> }
> >>
> >> return dma_direct_supported(dev, mask);
> >> }
> >>
> >> and dma_set_mask() does:
> >>
> >> if (!dev->dma_mask || !dma_supported(dev, mask))
> >> return -EIO;
> >>
> >> For the generic direct-DMA path, the situation is clear:
> >>
> >> int dma_direct_supported(struct device *dev, u64 mask)
> >> {
> >> ...
> >>
> >> if (mask >= DMA_BIT_MASK(32))
> >> return 1;
> >>
> >> ...
> >> }
> >>
> >> So dma_direct_supported() itself cannot reject a >=32-bit mask.
> >>
> >> However, this does not appear to be true for every path through
> >> dma_supported().
> >>
> >> My audit of the relevant current callbacks looks roughly like this:
> >>
> >> +----------------------------+---------------------+-----------------------+
> >> | Backend / callback | >32 can fail? | 64 can fail? |
> >> +----------------------------+---------------------+-----------------------+
> >> | dma_direct_supported() | No | No |
> >> | default dma-iommu | Yes, conflicting | Yes, conflicting |
> >> | | dma_ops state | dma_ops state |
> >> | ibmebus_dma_supported() | Yes, !=64 fails | No |
> >> | xen_grant_dma_supported() | Yes, !=64 fails | No |
> >> | xen_swiotlb_dma_supported | Yes, threshold | Not due to width |
> >> | ppc dma_iommu_supported() | Yes | Yes, if no table |
> >> | parisc sba_dma_supported() | Yes | Yes, if no IOC |
> >> | parisc ccio_supported() | No for >=32 | No for valid dev |
> >> | alpha_pci_supported() | Yes, threshold / | Can fail if no |
> >> | | mapping dependent | usable DMA path|
> >> | dma_dummy_supported() | Yes, always | Yes, always |
> >> +----------------------------+---------------------+-----------------------+
> >>
> >> There seem to be two separate issues here.
> >>
> >> First, "any mask wider than 32 bits cannot fail" has direct
> >> counterexamples.
> >>
> >> For example, arch/powerpc/platforms/pseries/ibmebus.c contains:
> >>
> >> static int ibmebus_dma_supported(struct device *dev, u64 mask)
> >> {
> >> return mask == DMA_BIT_MASK(64);
> >> }
> >>
> >> and installs it in ibmebus_dma_ops.
> >>
> >> Therefore a call such as:
> >>
> >> dma_set_mask(dev, DMA_BIT_MASK(40))
> >>
> >> will fail even though the mask is wider than 32 bits.
> >>
> >> drivers/xen/grant-dma-ops.c has the same rule:
> >>
> >> static int xen_grant_dma_supported(struct device *dev, u64 mask)
> >> {
> >> return mask == DMA_BIT_MASK(64);
> >> }
> >>
> >> and xen_grant_dma_ops installs this as .dma_supported.
> >>
> >> So, for that backend as well, DMA_BIT_MASK(40), for example, is
> >> rejected.
> >>
> >> These two cases seem to directly contradict the more general statement
> >> that dma_set_mask_and_coherent() cannot fail for a mask wider than
> >> 32 bits.
> >>
> >> There are also cases where even DMA_BIT_MASK(64) can fail.
> >>
> >> One example is the PowerPC legacy IOMMU backend in
> >> arch/powerpc/kernel/dma-iommu.c:
> >>
> >> int dma_iommu_dma_supported(struct device *dev, u64 mask)
> >> {
> >> struct iommu_table *tbl;
> >>
> >> ...
> >>
> >> tbl = get_iommu_table_base(dev);
> >>
> >> if (!tbl) {
> >> dev_err(dev,
> >> "Warning: IOMMU dma not supported: "
> >> "mask 0x%08llx, table unavailable\n",
> >> mask);
> >> return 0;
> >> }
> >>
> >> if (tbl->it_offset >
> >> (mask >> tbl->it_page_shift)) {
> >> ...
> >> return 0;
> >> }
> >>
> >> return 1;
> >> }
> >>
> >> If get_iommu_table_base() returns NULL, the callback rejects the mask
> >> regardless of whether it is 32, 40, or 64 bits.
> >>
> >> I understand that an unavailable IOMMU table may represent an
> >> unexpected or unusable DMA configuration rather than an ordinary
> >> address-width limitation, but that seems exactly like a reason for the
> >> driver to retain the error check and abort probe instead of proceeding
> >> as if DMA setup succeeded.
> >>
> >> There is a similar state-dependent failure in
> >> drivers/parisc/sba_iommu.c:
> >>
> >> ioc = GET_IOC(dev);
> >> if (!ioc)
> >> return 0;
> >>
> >> /*
> >> * The max IO Virt address will *always* < 30 bits.
> >> */
> >> return mask >= ...;
> >>
> >> For a valid IOC the required address range is below 32 bits, so a
> >> 64-bit mask works. But if GET_IOC(dev) fails, DMA_BIT_MASK(64) is still
> >> rejected.
> >>
> >> The default dma-iommu path also appears to contain an intentional
> >> failure condition:
> >>
> >> if (use_dma_iommu(dev)) {
> >> if (WARN_ON(ops))
> >> return false;
> >> return true;
> >> }
> >>
> >> The intended state is:
> >>
> >> use_dma_iommu(dev) == true
> >> get_dma_ops(dev) == NULL
> >>
> >> because the default IOMMU DMA implementation does not rely on a
> >> dma_map_ops instance.
> >>
> >> However, if use_dma_iommu(dev) is true and get_dma_ops(dev) is
> >> non-NULL, dma_supported() deliberately returns false, independent of
> >> mask width.
> >>
> >> This looks particularly significant because this was explicitly
> >> discussed when the default dma-iommu implementation was changed from
> >> dma_ops indirect calls to direct calls.
> >>
> >> Christoph suggested moving the consistency check out of the fast path
> >> and doing it in dma_set_mask(), "And fail the call while we're at it."
> >> Leon replied that he would add it to dma_supported():
> >>
> >> https://lore.kernel.org/all/20240718070406.GK5630@unreal/
> >>
> >> The same discussion also says that the default-IOMMU state implies
> >> !ops.
> >>
> >> So the resulting WARN_ON(ops) path appears to be an intentional reason
> >> for dma_set_mask() to return an error when the DMA backend state is
> >> inconsistent, including when mask == DMA_BIT_MASK(64).
> >>
> >> Another concrete case is dma_dummy_ops.
> >>
> >> kernel/dma/dummy.c has:
> >>
> >> static int dma_dummy_supported(struct device *hwdev, u64 mask)
> >> {
> >> return 0;
> >> }
> >>
> >> so dma_set_mask() fails for every mask, including DMA_BIT_MASK(64).
> >>
> >> I initially wondered whether this was only a theoretical sentinel that
> >> could never be seen by a driver probe, but there is a real setup path
> >> for it. acpi_dma_configure_id() does:
> >>
> >> if (attr == DEV_DMA_NOT_SUPPORTED) {
> >> set_dma_ops(dev, &dma_dummy_ops);
> >> return 0;
> >> }
> >>
> >> Since this returns 0 from DMA configuration after installing
> >> dma_dummy_ops, the device can continue through the driver-core setup
> >> with DMA deliberately marked unsupported. If such a DMA-using driver
> >> then calls dma_set_mask_and_coherent(), the return-value check is what
> >> prevents it from continuing with an unusable DMA backend.
> >>
> >> I agree this is not representative of the normal case for a healthy
> >> PCI device, but it does seem to demonstrate that a 64-bit mask is not an
> >> unconditional success guarantee of the DMA API itself.
> >>
> >> The formal DMA API documentation also seems to reflect this. In
> >> Documentation/core-api/dma-api.rst, under "DMA addressing
> >> limitations", it says:
> >>
> >> All the below functions which set a DMA mask may fail if the
> >> requested mask cannot be used with the device, or if the device is
> >> not capable of doing DMA.
> >>
> >> and dma_set_mask_and_coherent() is documented as returning zero on
> >> success and a negative error on failure.
> >>
> >> So I think there are two different statements here:
> >>
> >> 1. For a normally configured DMA-capable device, a 64-bit DMA mask
> >> should be supportable.
> >>
> >> 2. dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)) cannot return
> >> an error, so checking its return value is unnecessary.
> >>
> >> The first looks like the intended normal-case invariant, but I do not
> >> see how the second follows given the current dma_supported() paths
> >> above.
> >>
> >> In particular, the PowerPC IOMMU table failure, the
> >> use_dma_iommu(dev) && ops consistency check, and dma_dummy_ops are all
> >> paths where the mask width itself is not the problem but
> >> dma_set_mask_and_coherent(..., DMA_BIT_MASK(64)) can nevertheless
> >> report failure.
> >>
> >> That is why I currently think code like:
> >>
> >> ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> >> if (ret)
> >> return ret;
> >>
> >> still has value: the check is not probing whether 64-bit DMA
> >> addressing is supported versus some narrower address width. It is
> >> checking whether the DMA setup as a whole succeeded before the driver
> >> starts using DMA.
> >>
> >> Again, I do not work within the DMA subsystem, and therefore I won't exactly
> >> claim myself to be an expert on this. The above are some findings after some
> >> digging. So correct me if I am wrong.
> >>
> >> Am I missing an invariant which makes these failure paths unreachable
> >> for a driver calling dma_set_mask_and_coherent()? If not, then I believe the return
> >> value check is needed, and the DMA HOWTO document might need to mention that.
> >>
> >> Thanks,
> >> Ruizhe
> >>
> >>
> >>
> >>
> >> >
> >> >Frank
> >> >> }
> >> >>
> >> >> dev_set_drvdata(dev, pt);
> >> >> --
> >> >> 2.27.0
> >> >>
> >> >
> >