Re: [PATCH v5 3/7] PCI/ATS: Decouple pci_ats_supported() from pci_prepare_ats()

[Baolu Lu <baolu.lu@linux.intel.com>]

On 5/29/26 04:23, Pranjal Shrivastava wrote:
> Currently, pci_prepare_ats() internally calls pci_ats_supported() and
> returns -EINVAL if the device does not support ATS. While this provides
> a safety check, it conflates support detection with configuration.
> 
> Update pci_prepare_ats() to remove the internal support check. This
> decouples support verification from the configuration phase, ensuring
> that drivers can distinguish between a device that does not support ATS
> and one that has a true configuration error (e.g. STU mismatch).
> 
> Update the function documentation to mandate that callers must verify
> ATS support (via pci_ats_supported()) before calling pci_prepare_ats().
> 
> Signed-off-by: Pranjal Shrivastava <praan@google.com>
> ---
>   drivers/pci/ats.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 8057c24b0469..92c2a6bc2dcc 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -56,7 +56,9 @@ EXPORT_SYMBOL_GPL(pci_ats_supported);
>    * @ps: the IOMMU page shift
>    *
>    * This must be done by the IOMMU driver on the PF before any VFs are created to
> - * ensure that the VF can have ATS enabled.
> + * ensure that the VF can have ATS enabled. Callers must verify that ATS is
> + * supported by the device (e.g. via pci_ats_supported()) before calling this
> + * function.
>    *
>    * Returns 0 on success, or negative on failure.
>    */
> @@ -64,9 +66,6 @@ int pci_prepare_ats(struct pci_dev *dev, int ps)
>   {
>   	u16 ctrl;
>   
> -	if (!pci_ats_supported(dev))
> -		return -EINVAL;
> -
>   	if (WARN_ON(dev->ats_enabled))
>   		return -EBUSY;
>   

I am not sure that the removal above ensures that 'drivers can
distinguish between a device that does not support ATS and one that has
a true configuration error (e.g., STU mismatch)', especially considering
that this helper already has a return value that explicitly conveys the
failure reason.

Furthermore, if a caller misuses this API by calling it against a non-
ATS device, the following code executes:

         ctrl = PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
         pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);

This causes the driver to attempt a write to an invalid or non-existent
PCI configuration space address. Instead of removing the check from the
function entirely, how about adding a WARN_ON() around it?

	if (WARN_ON(!pci_ats_supported(dev)))
		return -EINVAL;

Thanks,
baolu