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

From: Patrick Havelange
Date: Thu Dec 10 2020 - 05:07:51 EST


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

[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