Re: [PATCH v2 03/23] dmaengine: sdxi: Add PCI initialization

[Nathan Lynch <nathan.lynch@amd.com>]

sashiko-bot@kernel.org writes:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The Kconfig symbol `CONFIG_SDXI` lacks a dependency on `PCI` or `PCI_MSI`, and the Makefile conditionally links `pci.o` using `sdxi-$(CONFIG_PCI_MSI) += pci.o`.
> - [Low] The return value of `dma_set_mask_and_coherent()` is ignored in `sdxi_pci_init()`.
> --
>
> commit bdb690a4c497e7a353f0143e03e32ba75daa6013
> Author: Nathan Lynch <nathan.lynch@amd.com>
>
> dmaengine: sdxi: Add PCI initialization
>
> This commit adds PCI initialization to bind an SDXI device via the class
> code. It maps the control registers and doorbell region, managing resources
> with devres and registering the device with the generic SDXI core.
>
>> diff --git a/drivers/dma/sdxi/Makefile b/drivers/dma/sdxi/Makefile
>> new file mode 100644
>> --- /dev/null
>> +++ b/drivers/dma/sdxi/Makefile
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +obj-$(CONFIG_SDXI) += sdxi.o
>> +
>> +sdxi-objs += device.o
>> +
>> +sdxi-$(CONFIG_PCI_MSI) += pci.o
>
> If CONFIG_SDXI is enabled but CONFIG_PCI_MSI is disabled, pci.o is omitted
> from the build. Because pci.c is the only file containing the driver
> registration macro (module_pci_driver) and the module license declaration,
> would this result in a module that lacks an initialization entry point and
> fails to load properly while also tainting the kernel?

I guess so. This is just to allow build coverage without PCI since SDXI
doesn't strictly require it. Maybe there should be two modules: one for
the PCI driver, which depends on another containing the bus-agnostic
code.

>
> [ ... ]
>
>> diff --git a/drivers/dma/sdxi/pci.c b/drivers/dma/sdxi/pci.c
>> new file mode 100644
>> --- /dev/null
>> +++ b/drivers/dma/sdxi/pci.c
> [ ... ]
>> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
>> +{
>> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = pcim_enable_device(pdev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to enable device\n");
>> +
>> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>
> The return value of dma_set_mask_and_coherent() is ignored here. If the
> platform or IOMMU does not support 64-bit DMA, this function returns an error
> code and leaves the device's DMA mask at its default 32-bit value.
>
> Is it expected to proceed without handling the fallback or logging a
> warning?

Review comments on v1 indicated the check isn't needed when the mask
argument is >=32:

https://lore.kernel.org/dmaengine/aeXJhhPgfGjGZa__@lizhi-Precision-Tower-5810/

And the DMA-API documentation appears to agree, although I find it a
little hard to follow:

https://docs.kernel.org/7.0/core-api/dma-api-howto.html#dma-addressing-capabilities

Generally I think it's bad to ignore the return value of an API that's
known to be fallible for some inputs/circumstances, even when using an
input known to render it infallible. Ideally there would be a separate
API (dma_upgrade_mask_* or something) with void return type that could
be used instead.

Anyway, I don't plan to add the error checking back.