diff mbox series

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

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

Checks

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

Commit Message

Patrick Havelange Dec. 3, 2020, 1:50 p.m. UTC
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(-)

Comments

Madalin Bucur Dec. 3, 2020, 3:47 p.m. UTC | #1
> -----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
Patrick Havelange Dec. 8, 2020, 2:55 p.m. UTC | #2
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.
Madalin Bucur Dec. 9, 2020, 9:10 a.m. UTC | #3
> -----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
Patrick Havelange Dec. 9, 2020, 2:16 p.m. UTC | #4
>>> 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
>
Madalin Bucur Dec. 9, 2020, 6:55 p.m. UTC | #5
> -----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
Patrick Havelange Dec. 10, 2020, 8:49 a.m. UTC | #6
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
>
Madalin Bucur Dec. 10, 2020, 9:05 a.m. UTC | #7
> -----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
Patrick Havelange Dec. 10, 2020, 10:05 a.m. UTC | #8
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
>
Madalin Bucur Dec. 10, 2020, 10:46 a.m. UTC | #9
> -----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 mbox series

Patch

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;