Re:Re: Re: [PATCH 1/3] dmaengine: ptdma: Remove obsolete 32-bit DMA mask fallback

[Ruizhe Zhou]

>> 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?

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
>> >>
>> >
>