Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Sep 2022 17:14:07 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc:     Will McVicker <willmcvicker@google.com>,
        Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Rob Herring <robh@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>, kernel-team@android.com,
        Vidya Sagar <vidyas@nvidia.com>,
        Christoph Hellwig <hch@infradead.org>,
        Robin Murphy <robin.murphy@arm.com>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v5 2/2] PCI: dwc: Add support for 64-bit MSI target
 address
Message-ID: <20220930141407.mbpb33b4a6jitfyl@mobilestation>
References: <20220825185026.3816331-1-willmcvicker@google.com>
 <20220825185026.3816331-3-willmcvicker@google.com>
 <20220928120510.rkdlnczytxuioxcn@mobilestation>
 <YzbzUCikdpesdbLU@lpieralisi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YzbzUCikdpesdbLU@lpieralisi>
Precedence: bulk

On Fri, Sep 30, 2022 at 03:46:56PM +0200, Lorenzo Pieralisi wrote:
> On Wed, Sep 28, 2022 at 03:05:10PM +0300, Serge Semin wrote:
> > On Thu, Aug 25, 2022 at 06:50:25PM +0000, Will McVicker wrote:
> 
> [...]
> 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index 39f3b37d4033..8928a9a29d58 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -330,6 +330,9 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> > >  	u64 *msi_vaddr;
> > >  	int ret;
> > >  	u32 ctrl, num_ctrls;
> > > +	bool msi_64bit = false;
> > > +	bool retry_64bit = false;
> > > +	u16 msi_capabilities;
> > >  
> > >  	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++)
> > >  		pp->irq_mask[ctrl] = ~0;
> > > @@ -367,16 +370,33 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
> > >  						    dw_chained_msi_isr, pp);
> > >  	}
> > >  
> > > -	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > > -	if (ret)
> > > -		dev_warn(dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
> > 
> > > +	msi_capabilities = dw_pcie_msi_capabilities(pci);
> > > +	if (msi_capabilities & PCI_MSI_FLAGS_ENABLE)
> > > +		msi_64bit = msi_capabilities & PCI_MSI_FLAGS_64BIT;
> > 
> > Note this capability flag has nothing to do with the DW PCIe iMSI-RX
> > engine, which is used here to detect and report MSI TLPs. By design
> > iMSI-RX always support 64-bit addresses. If you imply having that flag
> > set by the DW PCIe platform drivers on the platform-specific probe
> > stage as an indication of MSI address range, then ok.
> 

> The MSI cap shows that - AFAICS - the RP can be programmed with
> a 64-bit MSI(DMA) address. It is fair to say that this is not
> enough to guarantee that the DMA mask for the host bridge can be
> inferred to be 64-bit though.

iMSI-RX always supports 64-bit bus addresses by design. The
MSI-control CSRs are unconditionally permit having 64-bit address
setup. So you can't even synthesize the DW PCIe RP IP-core with only
32-bits MSI support. AFAICS what @William is introducing here is
MSI_FLAGS_64BIT usage as a flag, which could be manually set by the
platform drivers and would indicate that the platform driver permits
having 64-bit MSI TLPs. I guess the platforms are supposed to know
better which PCIe device are going to live on the bus and set that
flag accordingly. It isn't true of course without the bus
pre-scanning.

> 
> I am inclined to drop this patch (only) from the PCI queue because
> it is unclear to me whether it really does the right thing.

Let's wait for the Robin response for my last comment regarding the
patch 1 fate too.

-Sergey

> 
> Lorenzo
> 
> > > -	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> > > -					GFP_KERNEL);
> > > -	if (!msi_vaddr) {
> > > -		dev_err(dev, "Failed to alloc and map MSI data\n");
> > > -		dw_pcie_free_msi(pp);
> > > -		return -ENOMEM;
> > > +	while (true) {
> > > +		dev_dbg(dev, "Setting MSI DMA mask to %s-bit.\n",
> > > +			retry_64bit ? "64" : "32");
> > 
> > > +		ret = dma_set_mask_and_coherent(dev, retry_64bit ?
> > > +						DMA_BIT_MASK(64) :
> > > +						DMA_BIT_MASK(32));
> > 
> > I'd suggest to just drop this. No DMA actually performed on getting the
> > MSI TLPs. So modifying the device DMA-mask due to something which
> > doesn't cause DMA and based on the flag which doesn't indicates the
> > device DMA-capability is at least inappropriate.
> > 
> > > +		if (ret)
> > > +			dev_warn(dev, "Failed to set DMA mask to %s-bit.\n",
> > > +				 retry_64bit ? "64" : "32");
> > > +
> > 
> > > +		msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
> > > +						GFP_KERNEL);
> > 
> > As I noted earlier the DMA-coherent memory can be too expensive. So
> > it's a waste of one allocating with no intent of usage. Instead of this
> > just get back the alloc_page() method here and pass the flag GFP_DMA32
> > to that function if MSI-capability reported the PCI_MSI_FLAGS_64BIT
> > unset.
> > 
> > -Sergey
> > 
> > > +		if (!msi_vaddr) {
> > > +			dev_err(dev, "Failed to alloc and map MSI data\n");
> > > +			if (msi_64bit && !retry_64bit) {
> > > +				retry_64bit = true;
> > > +				continue;
> > > +			}
> > > +
> > > +			dw_pcie_free_msi(pp);
> > > +			return -ENOMEM;
> > > +		}
> > > +		break;
> > >  	}
> > >  
> > >  	return 0;
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index c6725c519a47..650a7f22f9d0 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -82,6 +82,14 @@ u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
> > >  
> > > +u16 dw_pcie_msi_capabilities(struct dw_pcie *pci)
> > > +{
> > > +	u8 offset;
> > > +
> > > +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> > > +	return dw_pcie_readw_dbi(pci, offset + PCI_MSI_FLAGS);
> > > +}
> > > +
> > >  static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
> > >  					    u8 cap)
> > >  {
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index a871ae7eb59e..45fcdfc8c035 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -332,6 +332,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);
> > >  
> > >  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> > >  u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);
> > > +u16 dw_pcie_msi_capabilities(struct dw_pcie *pci);
> > >  
> > >  int dw_pcie_read(void __iomem *addr, int size, u32 *val);
> > >  int dw_pcie_write(void __iomem *addr, int size, u32 val);
> > > -- 
> > > 2.37.2.672.g94769d06f0-goog
> > > 
> > >