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

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Sep 04, 2026 at 07:15:36PM +0800, 周睿哲 wrote:
> Hi Frank,
>
>
> From: Frank Li <Frank.li@oss.nxp.com>
> Date: 2026-09-04 03:26:36
> To:  Ruizhe Zhou <zhouruizhe@resnics.com>
> Cc:  Vinod Koul <vkoul@kernel.org>,Basavaraj Natikar <Basavaraj.Natikar@amd.com>,Logan Gunthorpe <logang@deltatee.com>,Orson Zhai <orsonzhai@gmail.com>,Baolin Wang <baolin.wang@linux.alibaba.com>,Frank Li <Frank.Li@kernel.org>,Chunyan Zhang <zhang.lyra@gmail.com>,dmaengine@vger.kernel.org,linux-kernel@vger.kernel.org
> 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@resnics.com. 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@resnics.com>
> >> ---
> >>  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@wanadoo.fr/

Frank

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