Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Patrick Havelange <patrick.havelange@essensium.com>]

On 2020-12-10 10:05, Madalin Bucur wrote:
>> -----Original Message-----
>> From: Patrick Havelange <patrick.havelange@essensium.com>

[snipped]

>>
>> But then that change would not be compatible with the existing device
>> trees in already existing hardware. I'm not sure how to handle that case
>> properly.
> 
> One needs to be backwards compatible with the old device trees, so we do not
> really have a simple answer, I know.
> 
>>> If we want to hack it,
>>> instead of splitting ioremaps, we can reserve 4 kB in the FMan driver,
>>> and keep the ioremap as it is now, with the benefit of less code churn.
>>
>> but then the ioremap and the memory reservation do not match. Why bother
>> at all then with the mem reservation, just ioremap only and be done with
>> it. What I'm saying is, I don't see the point of having a "fake"
>> reservation call if it does not correspond that what is being used.
> 
> The reservation is not fake, it just covering the first portion of the ioremap.
> Another hypothetical FMan driver would presumably reserve and ioremap starting
> from the same point, thus the desired error would be met.
> 
> Regarding removing reservation altogether, yes, we can do that, in the child
> device drivers. That will fix that use after free issue you've found and align
> with the custom, hierarchical structure of the FMan devices/drivers. But would
> leave them without the double use guard we have when using the reservation.
> 
>>> In the end, what the reservation is trying to achieve is to make sure
>> there
>>> is a single driver controlling a certain peripeheral, and this basic
>>> requirement would be addressed by that change plus devm_of_iomap() for
>> child
>>> devices (ports, MACs).
>>
>> Again, correct me if I'm wrong, but with the fake mem reservation, it
>> would *not* make sure that a single driver is controlling a certain
>> peripheral.
> 
> Actually, it would. If the current FMan driver reserves the first part of the FMan
> memory, then another FMan driver (I do not expect a random driver trying to map the
> FMan register area)

Ha!, now I understand your point. I still think it is not a clean 
solution, as the reservation do not match the ioremap usage.

> would likely try to use that same part (with the same or
> a different size, it does not matter, there will be an error anyway) and the
> reservation attempt will fail. If we fix the child device drivers, then they
> will have normal mappings and reservations.
> 
>> My point is, either have a *correct* mem reservation, or don't have one
>> at all. There is no point in trying to cheat the system.
> 
> Now we do not have correct reservations for the child devices because the
> parent takes it all for himself. Reduce the parent reservation and make room
> for correct reservations for the child. The two-sections change you've made may
> try to be correct but it's overkill for the purpose of detecting double use.

But it is not overkill if we want to detect potential subdrivers mapping 
sections that would not start at the main fman region (but still part of 
the main fman region).

> And I also find the patch to obfuscate the already not so readable code so I'd
> opt for a simpler fix.

As said already, I'm not in favor of having a reservation that do not 
match the real usage.

And in my opinion, having a mismatch with the mem reservation and the 
mem usage is also the kind of obfuscation that we want to avoid.

Yes now the code is slightly more complex, but it is also slightly more 
correct.

I'm not seeing currently another way on how to make it simpler *and* 
correct at the same time.

Patrick H.

> 
> Madalin
>