> -----Original Message----- > From: Patrick Havelange <patrick.havela...@essensium.com> > Sent: 10 December 2020 12:06 > To: Madalin Bucur <madalin.bu...@nxp.com>; David S. Miller > <da...@davemloft.net>; Jakub Kicinski <k...@kernel.org>; > netdev@vger.kernel.org; linux-ker...@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.havela...@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
