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

[Pranjal Shrivastava]

On Fri, May 29, 2026 at 02:29:16PM +0800, Baolu Lu wrote:
> 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@xxxxxxxxxx>
> > ---
> >   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,

Ack. The idea was that all callers will check pci_ats_supported() before
calling pci_prepare_ats() but I guess I didn't consider callers abusing 
this for non-ATS cases.

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

Ack. That makes sense. I'll post another version!

Thanks,
Praan