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

[Madalin Bucur <madalin.bucur@nxp.com>]

> -----Original Message-----
> From: Patrick Havelange <patrick.havelange@essensium.com>
> Sent: 10 December 2020 12:06
> To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource
> region reservation
> 
> 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.


Ok, we've taken note on your report and will put together a fix.

Regards,
Madalin