Re: [PATCH] PCI: rcar: Always allocate MSI addresses in 32bit space

[Marek Vasut <marek.vasut@gmail.com>]

On 12/21/20 11:01 AM, Lorenzo Pieralisi wrote:

[...]

>>>>>> --- a/drivers/pci/controller/pcie-rcar-host.c
>>>>>> +++ b/drivers/pci/controller/pcie-rcar-host.c
>>>>>> @@ -753,7 +753,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host)
>>>>>>     	}
>>>>>>     	/* setup MSI data target */
>>>>>> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>>>>> +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA32, 0);
>>>>>
>>>>> This does not do what you want on !CONFIG_ZONE_DMA32 (ie arm LPAE).
>>>>
>>>> How come? I would expect GFP_DMA32 allocates a buffer below 4 GiB in any
>>>> case.
>>>
>>> For ARM LPAE allocation falls back to ZONE_NORMAL that happens to work
>>> because if there is memory > 4GB it ends up in ZONE_HIGHMEM, so this
>>> patch should still work on ARM LPAE too.
>>>
>>> Regardless, thoughts above the alternative approach (that saves you
>>> a page allocation) ?
>>
>> Since this is a bugfix, I would prefer it to be minimal.
> 
> Yes, I agree with you on that.

Then maybe it makes sense to apply this bugfix so others can benefit 
from it too ?

>> Also, in case there was some yet undiscovered hardware bug which would
>> let the MSI write through, having unused memory as the MSI destination
>> address would only lead to write into that memory -- instead of a
>> write into some other address.
>>
>> Changing this to some hard-coded address (any suggestions?) can be a
>> subsequent patch.
> 
> The idea was taking the address from the host controller inbound window
> (ie an address outside the dma-ranges ~(dma-ranges) and < 4GB), it
> should not matter which one.

Wouldn't that make the code quite unnecessarily complex for no gain ?
The above fix does just that in one line, unless there is some code in 
the PCI subsystem to select such an address already ?

> I agree though that this can be a
> subsequent patch even though usually we send for -rc* only fixes for
> patches that hit the previous merge window - this seems a quite
> longstanding (I traced it back to v3.16) one so it would wait till
> v5.12, there is time to refactor it.

I see, I was not aware of this policy toward bugfixes.