| Message ID | 20201203135039.31474-2-patrick.havelange@essensium.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | freescale/fman: remove usage of __devm_request_region | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 203 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> -----Original Message----- > From: Patrick Havelange <patrick.havelange@essensium.com> > Sent: 03 December 2020 15:51 > 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 > Cc: Patrick Havelange <patrick.havelange@essensium.com> > Subject: [PATCH net 1/4] net: freescale/fman: Split the main resource > region reservation > > The main fman driver is only using some parts of the fman memory > region. > Split the reservation of the main region in 2, so that the other > regions that will be used by fman-port and fman-mac drivers can > be reserved properly and not be in conflict with the main fman > reservation. > > Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com> I think the problem you are trying to work on here is that the device tree entry that describes the FMan IP allots to the parent FMan device the whole memory-mapped registers area, as described in the device datasheet. The smaller FMan building blocks (ports, MDIO controllers, etc.) are each using a nested subset of this memory-mapped registers area. While this hierarchical depiction of the hardware has not posed a problem to date, it is possible to cause issues if both the FMan driver and any of the sub-blocks drivers are trying to exclusively reserve the registers area. I'm assuming this is the problem you are trying to address here, besides the stack corruption issue. While for the latter I think we can put together a quick fix, for the former I'd like to take a bit of time to select the best fix, if one is really needed. So, please, let's split the two problems and first address the incorrect stack memory use. Regards, Madalin
On 2020-12-03 16:47, Madalin Bucur wrote: >> -----Original Message----- >> From: Patrick Havelange <patrick.havelange@essensium.com> >> Sent: 03 December 2020 15:51 >> 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 >> Cc: Patrick Havelange <patrick.havelange@essensium.com> >> Subject: [PATCH net 1/4] net: freescale/fman: Split the main resource >> region reservation >> >> The main fman driver is only using some parts of the fman memory >> region. >> Split the reservation of the main region in 2, so that the other >> regions that will be used by fman-port and fman-mac drivers can >> be reserved properly and not be in conflict with the main fman >> reservation. >> >> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com> > > I think the problem you are trying to work on here is that the device > tree entry that describes the FMan IP allots to the parent FMan device the > whole memory-mapped registers area, as described in the device datasheet. > The smaller FMan building blocks (ports, MDIO controllers, etc.) are > each using a nested subset of this memory-mapped registers area. > While this hierarchical depiction of the hardware has not posed a problem > to date, it is possible to cause issues if both the FMan driver and any > of the sub-blocks drivers are trying to exclusively reserve the registers > area. I'm assuming this is the problem you are trying to address here, > besides the stack corruption issue. Yes exactly. I did not add this behaviour (having a main region and subdrivers using subregions), I'm just trying to correct what is already there. For example: this is some content of /proc/iomem for one board I'm working with, with the current existing code: ffe400000-ffe4fdfff : fman ffe4e0000-ffe4e0fff : mac ffe4e2000-ffe4e2fff : mac ffe4e4000-ffe4e4fff : mac ffe4e6000-ffe4e6fff : mac ffe4e8000-ffe4e8fff : mac and now with my patches: ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000 ffe400000-ffe480fff : fman ffe488000-ffe488fff : fman-port ffe489000-ffe489fff : fman-port ffe48a000-ffe48afff : fman-port ffe48b000-ffe48bfff : fman-port ffe48c000-ffe48cfff : fman-port ffe4a8000-ffe4a8fff : fman-port ffe4a9000-ffe4a9fff : fman-port ffe4aa000-ffe4aafff : fman-port ffe4ab000-ffe4abfff : fman-port ffe4ac000-ffe4acfff : fman-port ffe4c0000-ffe4dffff : fman ffe4e0000-ffe4e0fff : mac ffe4e2000-ffe4e2fff : mac ffe4e4000-ffe4e4fff : mac ffe4e6000-ffe4e6fff : mac ffe4e8000-ffe4e8fff : mac > While for the latter I think we can > put together a quick fix, for the former I'd like to take a bit of time > to select the best fix, if one is really needed. So, please, let's split > the two problems and first address the incorrect stack memory use. I have no idea how you can fix it without a (more correct this time) dummy region passed as parameter (and you don't want to use the first patch). But then it will be useless to do the call anyway, as it won't do any proper verification at all, so it could also be removed entirely, which begs the question, why do it at all in the first place (the devm_request_mem_region). I'm not an expert in that part of the code so feel free to correct me if I missed something. BR, Patrick H.
> -----Original Message----- > From: Patrick Havelange <patrick.havelange@essensium.com> > Sent: 08 December 2020 16:56 > 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-03 16:47, Madalin Bucur wrote: > >> -----Original Message----- > >> From: Patrick Havelange <patrick.havelange@essensium.com> > >> Sent: 03 December 2020 15:51 > >> 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 > >> Cc: Patrick Havelange <patrick.havelange@essensium.com> > >> Subject: [PATCH net 1/4] net: freescale/fman: Split the main resource > >> region reservation > >> > >> The main fman driver is only using some parts of the fman memory > >> region. > >> Split the reservation of the main region in 2, so that the other > >> regions that will be used by fman-port and fman-mac drivers can > >> be reserved properly and not be in conflict with the main fman > >> reservation. > >> > >> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com> > > > > I think the problem you are trying to work on here is that the device > > tree entry that describes the FMan IP allots to the parent FMan device > the > > whole memory-mapped registers area, as described in the device datasheet. > > The smaller FMan building blocks (ports, MDIO controllers, etc.) are > > each using a nested subset of this memory-mapped registers area. > > While this hierarchical depiction of the hardware has not posed a > problem > > to date, it is possible to cause issues if both the FMan driver and any > > of the sub-blocks drivers are trying to exclusively reserve the > registers > > area. I'm assuming this is the problem you are trying to address here, > > besides the stack corruption issue. > > Yes exactly. > I did not add this behaviour (having a main region and subdrivers using > subregions), I'm just trying to correct what is already there. > For example: this is some content of /proc/iomem for one board I'm > working with, with the current existing code: > ffe400000-ffe4fdfff : fman > ffe4e0000-ffe4e0fff : mac > ffe4e2000-ffe4e2fff : mac > ffe4e4000-ffe4e4fff : mac > ffe4e6000-ffe4e6fff : mac > ffe4e8000-ffe4e8fff : mac > > and now with my patches: > ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000 > ffe400000-ffe480fff : fman > ffe488000-ffe488fff : fman-port > ffe489000-ffe489fff : fman-port > ffe48a000-ffe48afff : fman-port > ffe48b000-ffe48bfff : fman-port > ffe48c000-ffe48cfff : fman-port > ffe4a8000-ffe4a8fff : fman-port > ffe4a9000-ffe4a9fff : fman-port > ffe4aa000-ffe4aafff : fman-port > ffe4ab000-ffe4abfff : fman-port > ffe4ac000-ffe4acfff : fman-port > ffe4c0000-ffe4dffff : fman > ffe4e0000-ffe4e0fff : mac > ffe4e2000-ffe4e2fff : mac > ffe4e4000-ffe4e4fff : mac > ffe4e6000-ffe4e6fff : mac > ffe4e8000-ffe4e8fff : mac > > > While for the latter I think we can > > put together a quick fix, for the former I'd like to take a bit of time > > to select the best fix, if one is really needed. So, please, let's split > > the two problems and first address the incorrect stack memory use. > > I have no idea how you can fix it without a (more correct this time) > dummy region passed as parameter (and you don't want to use the first > patch). But then it will be useless to do the call anyway, as it won't > do any proper verification at all, so it could also be removed entirely, > which begs the question, why do it at all in the first place (the > devm_request_mem_region). > > I'm not an expert in that part of the code so feel free to correct me if > I missed something. > > BR, > > Patrick H. Hi, Patrick, the DPAA entities are described in the device tree. Adding some hardcoding in the driver is not really the solution for this problem. And I'm not sure we have a clear problem statement to start with. Can you help me on that part? Madalin
>>> area. I'm assuming this is the problem you are trying to address here, >>> besides the stack corruption issue. >> >> Yes exactly. >> I did not add this behaviour (having a main region and subdrivers using >> subregions), I'm just trying to correct what is already there. >> For example: this is some content of /proc/iomem for one board I'm >> working with, with the current existing code: >> ffe400000-ffe4fdfff : fman >> ffe4e0000-ffe4e0fff : mac >> ffe4e2000-ffe4e2fff : mac >> ffe4e4000-ffe4e4fff : mac >> ffe4e6000-ffe4e6fff : mac >> ffe4e8000-ffe4e8fff : mac >> >> and now with my patches: >> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000 >> ffe400000-ffe480fff : fman >> ffe488000-ffe488fff : fman-port >> ffe489000-ffe489fff : fman-port >> ffe48a000-ffe48afff : fman-port >> ffe48b000-ffe48bfff : fman-port >> ffe48c000-ffe48cfff : fman-port >> ffe4a8000-ffe4a8fff : fman-port >> ffe4a9000-ffe4a9fff : fman-port >> ffe4aa000-ffe4aafff : fman-port >> ffe4ab000-ffe4abfff : fman-port >> ffe4ac000-ffe4acfff : fman-port >> ffe4c0000-ffe4dffff : fman >> ffe4e0000-ffe4e0fff : mac >> ffe4e2000-ffe4e2fff : mac >> ffe4e4000-ffe4e4fff : mac >> ffe4e6000-ffe4e6fff : mac >> ffe4e8000-ffe4e8fff : mac >> >>> While for the latter I think we can >>> put together a quick fix, for the former I'd like to take a bit of time >>> to select the best fix, if one is really needed. So, please, let's split >>> the two problems and first address the incorrect stack memory use. >> >> I have no idea how you can fix it without a (more correct this time) >> dummy region passed as parameter (and you don't want to use the first >> patch). But then it will be useless to do the call anyway, as it won't >> do any proper verification at all, so it could also be removed entirely, >> which begs the question, why do it at all in the first place (the >> devm_request_mem_region). >> >> I'm not an expert in that part of the code so feel free to correct me if >> I missed something. >> >> BR, >> >> Patrick H. > > Hi, Patrick, > > the DPAA entities are described in the device tree. Adding some hardcoding in > the driver is not really the solution for this problem. And I'm not sure we have I'm not seeing any problem here, the offsets used by the fman driver were already there, I just reorganized them in 2 blocks. > a clear problem statement to start with. Can you help me on that part? - The current call to __devm_request_region in fman_port.c is not correct. One way to fix this is to use devm_request_mem_region, however this requires that the main fman would not be reserving the whole region. This leads to the second problem: - Make sure the main fman driver is not reserving the whole region. Is that clearer like this ? Patrick H. > > Madalin >
> -----Original Message----- > From: Patrick Havelange <patrick.havelange@essensium.com> > Sent: 09 December 2020 16:17 > 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 > > >>> area. I'm assuming this is the problem you are trying to address here, > >>> besides the stack corruption issue. > >> > >> Yes exactly. > >> I did not add this behaviour (having a main region and subdrivers using > >> subregions), I'm just trying to correct what is already there. > >> For example: this is some content of /proc/iomem for one board I'm > >> working with, with the current existing code: > >> ffe400000-ffe4fdfff : fman > >> ffe4e0000-ffe4e0fff : mac > >> ffe4e2000-ffe4e2fff : mac > >> ffe4e4000-ffe4e4fff : mac > >> ffe4e6000-ffe4e6fff : mac > >> ffe4e8000-ffe4e8fff : mac > >> > >> and now with my patches: > >> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000 > >> ffe400000-ffe480fff : fman > >> ffe488000-ffe488fff : fman-port > >> ffe489000-ffe489fff : fman-port > >> ffe48a000-ffe48afff : fman-port > >> ffe48b000-ffe48bfff : fman-port > >> ffe48c000-ffe48cfff : fman-port > >> ffe4a8000-ffe4a8fff : fman-port > >> ffe4a9000-ffe4a9fff : fman-port > >> ffe4aa000-ffe4aafff : fman-port > >> ffe4ab000-ffe4abfff : fman-port > >> ffe4ac000-ffe4acfff : fman-port > >> ffe4c0000-ffe4dffff : fman > >> ffe4e0000-ffe4e0fff : mac > >> ffe4e2000-ffe4e2fff : mac > >> ffe4e4000-ffe4e4fff : mac > >> ffe4e6000-ffe4e6fff : mac > >> ffe4e8000-ffe4e8fff : mac > >> > >>> While for the latter I think we can > >>> put together a quick fix, for the former I'd like to take a bit of > time > >>> to select the best fix, if one is really needed. So, please, let's > split > >>> the two problems and first address the incorrect stack memory use. > >> > >> I have no idea how you can fix it without a (more correct this time) > >> dummy region passed as parameter (and you don't want to use the first > >> patch). But then it will be useless to do the call anyway, as it won't > >> do any proper verification at all, so it could also be removed entirely, > >> which begs the question, why do it at all in the first place (the > >> devm_request_mem_region). > >> > >> I'm not an expert in that part of the code so feel free to correct me > if > >> I missed something. > >> > >> BR, > >> > >> Patrick H. > > > > Hi, Patrick, > > > > the DPAA entities are described in the device tree. Adding some > hardcoding in > > the driver is not really the solution for this problem. And I'm not sure > we have > > I'm not seeing any problem here, the offsets used by the fman driver > were already there, I just reorganized them in 2 blocks. > > > a clear problem statement to start with. Can you help me on that part? > > - The current call to __devm_request_region in fman_port.c is not correct. > > One way to fix this is to use devm_request_mem_region, however this > requires that the main fman would not be reserving the whole region. > This leads to the second problem: > - Make sure the main fman driver is not reserving the whole region. > > Is that clearer like this ? > > Patrick H. The overlapping IO areas result from the device tree description, that in turn mimics the HW description in the manual. If we really want to remove the nesting, we should change the device trees, not the drivers. 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. 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). Madalin
On 2020-12-09 19:55, Madalin Bucur wrote: >> -----Original Message----- >> From: Patrick Havelange <patrick.havelange@essensium.com> >> Sent: 09 December 2020 16:17 >> 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 >> >>>>> area. I'm assuming this is the problem you are trying to address here, >>>>> besides the stack corruption issue. >>>> >>>> Yes exactly. >>>> I did not add this behaviour (having a main region and subdrivers using >>>> subregions), I'm just trying to correct what is already there. >>>> For example: this is some content of /proc/iomem for one board I'm >>>> working with, with the current existing code: >>>> ffe400000-ffe4fdfff : fman >>>> ffe4e0000-ffe4e0fff : mac >>>> ffe4e2000-ffe4e2fff : mac >>>> ffe4e4000-ffe4e4fff : mac >>>> ffe4e6000-ffe4e6fff : mac >>>> ffe4e8000-ffe4e8fff : mac >>>> >>>> and now with my patches: >>>> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000 >>>> ffe400000-ffe480fff : fman >>>> ffe488000-ffe488fff : fman-port >>>> ffe489000-ffe489fff : fman-port >>>> ffe48a000-ffe48afff : fman-port >>>> ffe48b000-ffe48bfff : fman-port >>>> ffe48c000-ffe48cfff : fman-port >>>> ffe4a8000-ffe4a8fff : fman-port >>>> ffe4a9000-ffe4a9fff : fman-port >>>> ffe4aa000-ffe4aafff : fman-port >>>> ffe4ab000-ffe4abfff : fman-port >>>> ffe4ac000-ffe4acfff : fman-port >>>> ffe4c0000-ffe4dffff : fman >>>> ffe4e0000-ffe4e0fff : mac >>>> ffe4e2000-ffe4e2fff : mac >>>> ffe4e4000-ffe4e4fff : mac >>>> ffe4e6000-ffe4e6fff : mac >>>> ffe4e8000-ffe4e8fff : mac >>>> >>>>> While for the latter I think we can >>>>> put together a quick fix, for the former I'd like to take a bit of >> time >>>>> to select the best fix, if one is really needed. So, please, let's >> split >>>>> the two problems and first address the incorrect stack memory use. >>>> >>>> I have no idea how you can fix it without a (more correct this time) >>>> dummy region passed as parameter (and you don't want to use the first >>>> patch). But then it will be useless to do the call anyway, as it won't >>>> do any proper verification at all, so it could also be removed entirely, >>>> which begs the question, why do it at all in the first place (the >>>> devm_request_mem_region). >>>> >>>> I'm not an expert in that part of the code so feel free to correct me >> if >>>> I missed something. >>>> >>>> BR, >>>> >>>> Patrick H. >>> >>> Hi, Patrick, >>> >>> the DPAA entities are described in the device tree. Adding some >> hardcoding in >>> the driver is not really the solution for this problem. And I'm not sure >> we have >> >> I'm not seeing any problem here, the offsets used by the fman driver >> were already there, I just reorganized them in 2 blocks. >> >>> a clear problem statement to start with. Can you help me on that part? >> >> - The current call to __devm_request_region in fman_port.c is not correct. >> >> One way to fix this is to use devm_request_mem_region, however this >> requires that the main fman would not be reserving the whole region. >> This leads to the second problem: >> - Make sure the main fman driver is not reserving the whole region. >> >> Is that clearer like this ? >> >> Patrick H. Hi, > > The overlapping IO areas result from the device tree description, that in turn > mimics the HW description in the manual. If we really want to remove the nesting, > we should change the device trees, not the drivers. 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. > 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. > 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. 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. Patrick H. > > Madalin >
> -----Original Message----- > From: Patrick Havelange <patrick.havelange@essensium.com> > Sent: 10 December 2020 10:49 > 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-09 19:55, Madalin Bucur wrote: > >> -----Original Message----- > >> From: Patrick Havelange <patrick.havelange@essensium.com> > >> Sent: 09 December 2020 16:17 > >> 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 > >> > >>>>> area. I'm assuming this is the problem you are trying to address > here, > >>>>> besides the stack corruption issue. > >>>> > >>>> Yes exactly. > >>>> I did not add this behaviour (having a main region and subdrivers > using > >>>> subregions), I'm just trying to correct what is already there. > >>>> For example: this is some content of /proc/iomem for one board I'm > >>>> working with, with the current existing code: > >>>> ffe400000-ffe4fdfff : fman > >>>> ffe4e0000-ffe4e0fff : mac > >>>> ffe4e2000-ffe4e2fff : mac > >>>> ffe4e4000-ffe4e4fff : mac > >>>> ffe4e6000-ffe4e6fff : mac > >>>> ffe4e8000-ffe4e8fff : mac > >>>> > >>>> and now with my patches: > >>>> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000 > >>>> ffe400000-ffe480fff : fman > >>>> ffe488000-ffe488fff : fman-port > >>>> ffe489000-ffe489fff : fman-port > >>>> ffe48a000-ffe48afff : fman-port > >>>> ffe48b000-ffe48bfff : fman-port > >>>> ffe48c000-ffe48cfff : fman-port > >>>> ffe4a8000-ffe4a8fff : fman-port > >>>> ffe4a9000-ffe4a9fff : fman-port > >>>> ffe4aa000-ffe4aafff : fman-port > >>>> ffe4ab000-ffe4abfff : fman-port > >>>> ffe4ac000-ffe4acfff : fman-port > >>>> ffe4c0000-ffe4dffff : fman > >>>> ffe4e0000-ffe4e0fff : mac > >>>> ffe4e2000-ffe4e2fff : mac > >>>> ffe4e4000-ffe4e4fff : mac > >>>> ffe4e6000-ffe4e6fff : mac > >>>> ffe4e8000-ffe4e8fff : mac > >>>> > >>>>> While for the latter I think we can > >>>>> put together a quick fix, for the former I'd like to take a bit of > >> time > >>>>> to select the best fix, if one is really needed. So, please, let's > >> split > >>>>> the two problems and first address the incorrect stack memory use. > >>>> > >>>> I have no idea how you can fix it without a (more correct this time) > >>>> dummy region passed as parameter (and you don't want to use the first > >>>> patch). But then it will be useless to do the call anyway, as it > won't > >>>> do any proper verification at all, so it could also be removed > entirely, > >>>> which begs the question, why do it at all in the first place (the > >>>> devm_request_mem_region). > >>>> > >>>> I'm not an expert in that part of the code so feel free to correct me > >> if > >>>> I missed something. > >>>> > >>>> BR, > >>>> > >>>> Patrick H. > >>> > >>> Hi, Patrick, > >>> > >>> the DPAA entities are described in the device tree. Adding some > >> hardcoding in > >>> the driver is not really the solution for this problem. And I'm not > sure > >> we have > >> > >> I'm not seeing any problem here, the offsets used by the fman driver > >> were already there, I just reorganized them in 2 blocks. > >> > >>> a clear problem statement to start with. Can you help me on that part? > >> > >> - The current call to __devm_request_region in fman_port.c is not > correct. > >> > >> One way to fix this is to use devm_request_mem_region, however this > >> requires that the main fman would not be reserving the whole region. > >> This leads to the second problem: > >> - Make sure the main fman driver is not reserving the whole region. > >> > >> Is that clearer like this ? > >> > >> Patrick H. > > Hi, Hi, Patrick, > > The overlapping IO areas result from the device tree description, that > in turn > > mimics the HW description in the manual. If we really want to remove the > nesting, > > we should change the device trees, not the drivers. > > 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) 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. And I also find the patch to obfuscate the already not so readable code so I'd opt for a simpler fix. Madalin
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 >
> -----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
diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c index ce0a121580f6..2e85209d560d 100644 --- a/drivers/net/ethernet/freescale/fman/fman.c +++ b/drivers/net/ethernet/freescale/fman/fman.c @@ -58,12 +58,15 @@ /* Modules registers offsets */ #define BMI_OFFSET 0x00080000 #define QMI_OFFSET 0x00080400 -#define KG_OFFSET 0x000C1000 -#define DMA_OFFSET 0x000C2000 -#define FPM_OFFSET 0x000C3000 -#define IMEM_OFFSET 0x000C4000 -#define HWP_OFFSET 0x000C7000 -#define CGP_OFFSET 0x000DB000 +#define SIZE_REGION_0 0x00081000 +#define POL_OFFSET 0x000C0000 +#define KG_OFFSET_FROM_POL 0x00001000 +#define DMA_OFFSET_FROM_POL 0x00002000 +#define FPM_OFFSET_FROM_POL 0x00003000 +#define IMEM_OFFSET_FROM_POL 0x00004000 +#define HWP_OFFSET_FROM_POL 0x00007000 +#define CGP_OFFSET_FROM_POL 0x0001B000 +#define SIZE_REGION_FROM_POL 0x00020000 /* Exceptions bit map */ #define EX_DMA_BUS_ERROR 0x80000000 @@ -1433,7 +1436,7 @@ static int clear_iram(struct fman *fman) struct fman_iram_regs __iomem *iram; int i, count; - iram = fman->base_addr + IMEM_OFFSET; + iram = fman->base_addr_pol + IMEM_OFFSET_FROM_POL; /* Enable the auto-increment */ iowrite32be(IRAM_IADD_AIE, &iram->iadd); @@ -1710,11 +1713,8 @@ static int set_num_of_open_dmas(struct fman *fman, u8 port_id, static int fman_config(struct fman *fman) { - void __iomem *base_addr; int err; - base_addr = fman->dts_params.base_addr; - fman->state = kzalloc(sizeof(*fman->state), GFP_KERNEL); if (!fman->state) goto err_fm_state; @@ -1740,13 +1740,14 @@ static int fman_config(struct fman *fman) fman->state->res = fman->dts_params.res; fman->exception_cb = fman_exceptions; fman->bus_error_cb = fman_bus_error; - fman->fpm_regs = base_addr + FPM_OFFSET; - fman->bmi_regs = base_addr + BMI_OFFSET; - fman->qmi_regs = base_addr + QMI_OFFSET; - fman->dma_regs = base_addr + DMA_OFFSET; - fman->hwp_regs = base_addr + HWP_OFFSET; - fman->kg_regs = base_addr + KG_OFFSET; - fman->base_addr = base_addr; + fman->fpm_regs = fman->dts_params.base_addr_pol + FPM_OFFSET_FROM_POL; + fman->bmi_regs = fman->dts_params.base_addr_0 + BMI_OFFSET; + fman->qmi_regs = fman->dts_params.base_addr_0 + QMI_OFFSET; + fman->dma_regs = fman->dts_params.base_addr_pol + DMA_OFFSET_FROM_POL; + fman->hwp_regs = fman->dts_params.base_addr_pol + HWP_OFFSET_FROM_POL; + fman->kg_regs = fman->dts_params.base_addr_pol + KG_OFFSET_FROM_POL; + fman->base_addr_0 = fman->dts_params.base_addr_0; + fman->base_addr_pol = fman->dts_params.base_addr_pol; spin_lock_init(&fman->spinlock); fman_defconfig(fman->cfg); @@ -1937,8 +1938,8 @@ static int fman_init(struct fman *fman) fman->state->exceptions &= ~FMAN_EX_QMI_SINGLE_ECC; /* clear CPG */ - memset_io((void __iomem *)(fman->base_addr + CGP_OFFSET), 0, - fman->state->fm_port_num_of_cg); + memset_io((void __iomem *)(fman->base_addr_pol + CGP_OFFSET_FROM_POL), + 0, fman->state->fm_port_num_of_cg); /* Save LIODN info before FMan reset * Skipping non-existent port 0 (i = 1) @@ -2717,13 +2718,11 @@ static struct fman *read_dts_node(struct platform_device *of_dev) { struct fman *fman; struct device_node *fm_node, *muram_node; - struct resource *res; + struct resource *tmp_res, *main_res; u32 val, range[2]; int err, irq; struct clk *clk; u32 clk_rate; - phys_addr_t phys_base_addr; - resource_size_t mem_size; fman = kzalloc(sizeof(*fman), GFP_KERNEL); if (!fman) @@ -2740,34 +2739,31 @@ static struct fman *read_dts_node(struct platform_device *of_dev) fman->dts_params.id = (u8)val; /* Get the FM interrupt */ - res = platform_get_resource(of_dev, IORESOURCE_IRQ, 0); - if (!res) { + tmp_res = platform_get_resource(of_dev, IORESOURCE_IRQ, 0); + if (!tmp_res) { dev_err(&of_dev->dev, "%s: Can't get FMan IRQ resource\n", __func__); goto fman_node_put; } - irq = res->start; + irq = tmp_res->start; /* Get the FM error interrupt */ - res = platform_get_resource(of_dev, IORESOURCE_IRQ, 1); - if (!res) { + tmp_res = platform_get_resource(of_dev, IORESOURCE_IRQ, 1); + if (!tmp_res) { dev_err(&of_dev->dev, "%s: Can't get FMan Error IRQ resource\n", __func__); goto fman_node_put; } - fman->dts_params.err_irq = res->start; + fman->dts_params.err_irq = tmp_res->start; /* Get the FM address */ - res = platform_get_resource(of_dev, IORESOURCE_MEM, 0); - if (!res) { + main_res = platform_get_resource(of_dev, IORESOURCE_MEM, 0); + if (!main_res) { dev_err(&of_dev->dev, "%s: Can't get FMan memory resource\n", __func__); goto fman_node_put; } - phys_base_addr = res->start; - mem_size = resource_size(res); - clk = of_clk_get(fm_node, 0); if (IS_ERR(clk)) { dev_err(&of_dev->dev, "%s: Failed to get FM%d clock structure\n", @@ -2832,22 +2828,47 @@ static struct fman *read_dts_node(struct platform_device *of_dev) } } - fman->dts_params.res = - devm_request_mem_region(&of_dev->dev, phys_base_addr, - mem_size, "fman"); - if (!fman->dts_params.res) { - dev_err(&of_dev->dev, "%s: request_mem_region() failed\n", + err = devm_request_resource(&of_dev->dev, &iomem_resource, main_res); + if (err) { + dev_err(&of_dev->dev, "%s: devm_request_resource() failed\n", + __func__); + goto fman_free; + } + + fman->dts_params.res = main_res; + + tmp_res = devm_request_mem_region(&of_dev->dev, main_res->start, + SIZE_REGION_0, "fman"); + if (!tmp_res) { + dev_err(&of_dev->dev, "%s: devm_request_mem_region() failed\n", __func__); goto fman_free; } - fman->dts_params.base_addr = - devm_ioremap(&of_dev->dev, phys_base_addr, mem_size); - if (!fman->dts_params.base_addr) { + fman->dts_params.base_addr_0 = + devm_ioremap(&of_dev->dev, tmp_res->start, + resource_size(tmp_res)); + if (!fman->dts_params.base_addr_0) { dev_err(&of_dev->dev, "%s: devm_ioremap() failed\n", __func__); goto fman_free; } + tmp_res = devm_request_mem_region(&of_dev->dev, + main_res->start + POL_OFFSET, + SIZE_REGION_FROM_POL, "fman"); + if (!tmp_res) { + dev_err(&of_dev->dev, "%s: devm_request_mem_region() failed\n", + __func__); + goto fman_free; + } + + fman->dts_params.base_addr_pol = + devm_ioremap(&of_dev->dev, tmp_res->start, + resource_size(tmp_res)); + if (!fman->dts_params.base_addr_pol) { + dev_err(&of_dev->dev, "%s: devm_ioremap() failed\n", __func__); + goto fman_free; + } fman->dev = &of_dev->dev; err = of_platform_populate(fm_node, NULL, NULL, &of_dev->dev); diff --git a/drivers/net/ethernet/freescale/fman/fman.h b/drivers/net/ethernet/freescale/fman/fman.h index f2ede1360f03..e6b339c57230 100644 --- a/drivers/net/ethernet/freescale/fman/fman.h +++ b/drivers/net/ethernet/freescale/fman/fman.h @@ -306,7 +306,11 @@ typedef irqreturn_t (fman_bus_error_cb)(struct fman *fman, u8 port_id, /* Structure that holds information received from device tree */ struct fman_dts_params { - void __iomem *base_addr; /* FMan virtual address */ + void __iomem *base_addr_0; /* FMan virtual address */ + void __iomem *base_addr_pol; /* FMan virtual address + * second region starting at + * policer offset + */ struct resource *res; /* FMan memory resource */ u8 id; /* FMan ID */ @@ -322,7 +326,8 @@ struct fman_dts_params { struct fman { struct device *dev; - void __iomem *base_addr; + void __iomem *base_addr_0; + void __iomem *base_addr_pol; struct fman_intr_src intr_mng[FMAN_EV_CNT]; struct fman_fpm_regs __iomem *fpm_regs;
The main fman driver is only using some parts of the fman memory region. Split the reservation of the main region in 2, so that the other regions that will be used by fman-port and fman-mac drivers can be reserved properly and not be in conflict with the main fman reservation. Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com> --- drivers/net/ethernet/freescale/fman/fman.c | 103 +++++++++++++-------- drivers/net/ethernet/freescale/fman/fman.h | 9 +- 2 files changed, 69 insertions(+), 43 deletions(-)