Re: [PATCH v13 4/7] PCI: dwc: Handle MSIs routed to multiple GIC interrupts

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 06/06/2022 19:27, Johan Hovold wrote:
> On Fri, Jun 03, 2022 at 10:41:34AM +0300, Dmitry Baryshkov wrote:
>> On some of Qualcomm platforms each group of 32 MSI vectors is routed to the
>> separate GIC interrupt. Implement support for such configurations by
>> parsing "msi0" ... "msiN" interrupts and attaching them to the chained
>> handler.
>>
>> Note, that if DT doesn't list an array of MSI interrupts and uses single
>> "msi" IRQ, the driver will limit the amount of supported MSI vectors
>> accordingly (to 32).
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   .../pci/controller/dwc/pcie-designware-host.c | 63 +++++++++++++++++--
>>   1 file changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 85c1160792e1..d1f9e20df903 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -289,6 +289,46 @@ static void dw_pcie_msi_init(struct pcie_port *pp)
>>   	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
>>   }
>>   
>> +static int dw_pcie_parse_split_msi_irq(struct pcie_port *pp)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct device *dev = pci->dev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	int irq;
>> +	u32 ctrl, max_vectors;
>> +
>> +	/* Parse as many IRQs as described in the devicetree. */
>> +	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++) {
>> +		char *msi_name = "msiX";
>> +
>> +		msi_name[3] = '0' + ctrl;
> 
> This oopses here as the string constant is read only:
> 
> 	[   19.787973] Unable to handle kernel write to read-only memory at virtual address ffffaa14f831afd3
> 
> Did you not test the series before posting?

Interesting enough the posted series works for me. Maybe I have a 
different set of debugging options. But thanks for spotting this. I'll 
post v14.

> 
> You need to define msi_name as:
> 
> 	char msi_name[] = "msiX";
> 
>> +		irq = platform_get_irq_byname_optional(pdev, msi_name);
>> +		if (irq == -ENXIO)
>> +			break;
>> +		if (irq < 0)
>> +			return dev_err_probe(dev, irq,
>> +					     "Failed to parse MSI IRQ '%s'\n",
>> +					     msi_name);
>> +
>> +		pp->msi_irq[ctrl] = irq;
>> +	}
> 
> Johan


-- 
With best wishes
Dmitry