Re: [PATCH v5 06/14] PCI: rockchip-ep: Fix MSI IRQ data mapping

From: Niklas Cassel <cassel@kernel.org>
To: Damien Le Moal <dlemoal@kernel.org>
Cc: "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Kishon Vijay Abraham I" <kishon@kernel.org>,
	"Shawn Lin" <shawn.lin@rock-chips.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, "Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Heiko Stuebner" <heiko@sntech.de>,
	devicetree@vger.kernel.org, linux-rockchip@lists.infradead.org,
	"Rick Wertenbroek" <rick.wertenbroek@gmail.com>
Subject: Re: [PATCH v5 06/14] PCI: rockchip-ep: Fix MSI IRQ data mapping
Date: Thu, 17 Oct 2024 11:52:34 +0200	[thread overview]
Message-ID: <ZxDeYqfti0iiK8D2@ryzen.lan> (raw)
In-Reply-To: <20241017015849.190271-7-dlemoal@kernel.org>

On Thu, Oct 17, 2024 at 10:58:41AM +0900, Damien Le Moal wrote:
> The call to rockchip_pcie_prog_ep_ob_atu() used to map the PCI address
> of MSI data to the memory window allocated on probe for IRQs is done
> in rockchip_pcie_ep_send_msi_irq() assuming a fixed alignment to a
> 256B boundary of the PCI address.  This is not correct as the alignment
> constraint for the RK3399 PCI mapping depends on the number of bits of
> address changing in the mapped region. This leads to an unstable system
> which sometimes work and sometimes does not (crashing on paging faults
> when memcpy_toio() or memcpy_fromio() are used).
> 
> Similar to regular data mapping, the MSI data mapping must thus be
> handled according to the information provided by
> rockchip_pcie_ep_align_addr(). Modify rockchip_pcie_ep_send_msi_irq()
> to use rockchip_pcie_ep_align_addr() to correctly program entry 0 of
> the ATU for sending MSI IRQs.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index f6959f9b94b7..dcd1b5415602 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -379,9 +379,10 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
>  {
>  	struct rockchip_pcie *rockchip = &ep->rockchip;
>  	u32 flags, mme, data, data_mask;
> +	size_t irq_pci_size, offset;
> +	u64 irq_pci_addr;
>  	u8 msi_count;
>  	u64 pci_addr;
> -	u32 r;
>  
>  	/* Check MSI enable bit */
>  	flags = rockchip_pcie_read(&ep->rockchip,
> @@ -417,18 +418,21 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
>  				       PCI_MSI_ADDRESS_LO);
>  
>  	/* Set the outbound region if needed. */
> -	if (unlikely(ep->irq_pci_addr != (pci_addr & PCIE_ADDR_MASK) ||
> +	irq_pci_size = ~PCIE_ADDR_MASK + 1;
> +	irq_pci_addr = rockchip_pcie_ep_align_addr(ep->epc,
> +						   pci_addr & PCIE_ADDR_MASK,
> +						   &irq_pci_size, &offset);
> +	if (unlikely(ep->irq_pci_addr != irq_pci_addr ||
>  		     ep->irq_pci_fn != fn)) {
> -		r = rockchip_ob_region(ep->irq_phys_addr);
> -		rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> -					     ep->irq_phys_addr,
> -					     pci_addr & PCIE_ADDR_MASK,
> -					     ~PCIE_ADDR_MASK + 1);
> -		ep->irq_pci_addr = (pci_addr & PCIE_ADDR_MASK);
> +		rockchip_pcie_prog_ep_ob_atu(rockchip, fn,
> +					rockchip_ob_region(ep->irq_phys_addr),
> +					ep->irq_phys_addr,
> +					irq_pci_addr, irq_pci_size);
> +		ep->irq_pci_addr = irq_pci_addr;
>  		ep->irq_pci_fn = fn;
>  	}
>  
> -	writew(data, ep->irq_cpu_addr + (pci_addr & ~PCIE_ADDR_MASK));
> +	writew(data, ep->irq_cpu_addr + offset + (pci_addr & ~PCIE_ADDR_MASK));
>  	return 0;
>  }
>  
> -- 
> 2.47.0
> 

Nice catch.

For DWC, in dw_pcie_ep_raise_msi_irq()
https://github.com/torvalds/linux/blob/v6.12-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L519-L522

and in dw_pcie_ep_raise_msix_irq()
https://github.com/torvalds/linux/blob/v6.12-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L603-L606

We also make sure that the address that we map is aligned,
and then write to the correct offset within that mapping:
ep->msi_mem + aligned_offset;
in order to write to the actual MSI address.

To me, it looks like doing a similar change as this patch does,
to dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq(),
would make the PCI endpoint code more consistent overall.

Thoughts?

Kind regards,
Niklas

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip