Re: [PATCH v5 3/3] PCI: Set dma-can-stall for HiSilicon chips

[Robin Murphy]

On 2021-08-26 19:26, Bjorn Helgaas wrote:
> [+cc Will, Robin, Joerg, hoping for an ack]
>
> On Tue, Jul 13, 2021 at 10:54:36AM +0800, Zhangfei Gao wrote:
> > HiSilicon KunPeng920 and KunPeng930 have devices appear as PCI but are
> > actually on the AMBA bus. These fake PCI devices can support SVA via
> > SMMU stall feature, by setting dma-can-stall for ACPI platforms.
> >
> > Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> > Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
> > ---
> >   drivers/pci/quirks.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 5d46ac6..03b0f98 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -1823,10 +1823,23 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI
> >   
> >   static void quirk_huawei_pcie_sva(struct pci_dev *pdev)
> >   {
> > +	struct property_entry properties[] = {
> > +		PROPERTY_ENTRY_BOOL("dma-can-stall"),
>
> "dma-can-stall" is used in arm_smmu_probe_device() to help set
> master->stall_enabled.
>
> I don't know the implications, so it'd be nice to get an ack from a
> maintainer of that code.

If it helps,

Acked-by: Robin Murphy <robin.murphy@xxxxxxx>

Normally stalling must not be enabled for PCI devices, since it would 
break the PCI requirement for free-flowing writes and may lead to 
deadlock. We expect PCI devices to support ATS and PRI if they want to 
be fault-tolerant, so there's no ACPI binding to describe anything else, 
even when a "PCI" device turns out to be a regular old SoC device 
dressed up as a RCiEP and normal rules don't apply.

I'm taking it on trust that stalling really is safe for all possible 
matching devices here (in general, deadlock may still be possible in the 
SoC interconnect depending on topology, hence why it's an explicit 
opt-in even for platform devices), but TBH either way I think I'd rather 
have this as a quirk in the kernel under our control, than have vendors 
attempt to play tricks with _DSD properties out in the field :)

Cheers,
Robin.

> > +		{},
> > +	};
> > +
> >   	if (pdev->revision != 0x21 && pdev->revision != 0x30)
> >   		return;
> >   
> >   	pdev->pasid_no_tlp = 1;
> > +
> > +	/*
> > +	 * Set the dma-can-stall property on ACPI platforms. Device tree
> > +	 * can set it directly.
> > +	 */
> > +	if (!pdev->dev.of_node &&
> > +	    device_add_properties(&pdev->dev, properties))
> > +		pci_warn(pdev, "could not add stall property");
> >   }
> >   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);
> >   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa251, quirk_huawei_pcie_sva);
> > --
> > 2.7.4