[dpdk-dev] bus/dpaa: fix memory allocation during bus scan
Checks
Commit Message
Fixes: 5b22cf744689 ("bus/dpaa: introducing FMan configurations")
Fixes: 37f9b54bd3cf ("net/dpaa: support Tx and Rx queue setup")
Cc: shreyansh.jain@nxp.com
With the IOVA auto detection changes, bus scan is performed before
memory initialization. DPAA bus scan must not use rte_malloc in
its path.
Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
drivers/bus/dpaa/base/fman/fman.c | 15 ++++++++-------
drivers/net/dpaa/dpaa_ethdev.c | 2 +-
2 files changed, 9 insertions(+), 8 deletions(-)
Comments
10/10/2017 09:01, Shreyansh Jain:
> Fixes: 5b22cf744689 ("bus/dpaa: introducing FMan configurations")
> Fixes: 37f9b54bd3cf ("net/dpaa: support Tx and Rx queue setup")
These lines should appear after the explanation.
> Cc: shreyansh.jain@nxp.com
>
> With the IOVA auto detection changes, bus scan is performed before
> memory initialization. DPAA bus scan must not use rte_malloc in
> its path.
If the scan has been broken by IOVA detection, you should reference
IOVA in Fixes line, not DPAA.
On Tuesday 10 October 2017 01:09 PM, Thomas Monjalon wrote:
> 10/10/2017 09:01, Shreyansh Jain:
>> Fixes: 5b22cf744689 ("bus/dpaa: introducing FMan configurations")
>> Fixes: 37f9b54bd3cf ("net/dpaa: support Tx and Rx queue setup")
>
> These lines should appear after the explanation.
Ok. That I missed - I will fix and send updated patch.
>
>> Cc: shreyansh.jain@nxp.com
>>
>> With the IOVA auto detection changes, bus scan is performed before
>> memory initialization. DPAA bus scan must not use rte_malloc in
>> its path.
>
> If the scan has been broken by IOVA detection, you should reference
> IOVA in Fixes line, not DPAA.
I was of two minds before sending this patch with above Fixes lines:
This change is because of IOVA but at the time IOVA patch was
introduced, this bus was not part of master. So, ideally, in the
net-next itself the integration should have been with fixed code - but,
I ended up verifying base patches without IOVA patches (some earlier
snapshot of net-next).
Anyways, I will send another patch with IOVA as fix line.
Hi Thomas,
On Tuesday 10 October 2017 01:09 PM, Thomas Monjalon wrote:
> 10/10/2017 09:01, Shreyansh Jain:
>> Fixes: 5b22cf744689 ("bus/dpaa: introducing FMan configurations")
>> Fixes: 37f9b54bd3cf ("net/dpaa: support Tx and Rx queue setup")
> These lines should appear after the explanation.
>
>> Cc: shreyansh.jain@nxp.com
>>
>> With the IOVA auto detection changes, bus scan is performed before
>> memory initialization. DPAA bus scan must not use rte_malloc in
>> its path.
> If the scan has been broken by IOVA detection, you should reference
> IOVA in Fixes line, not DPAA.
>
hmm.. IOVA not breaking scanning!, Refer this [1].
We(me/hemant) has discussed about same on thread[1] and agreed to
do respective changes and remove rte_ memory dependency from code base
at scan time..
Thanks.
[1] http://dpdk.org/dev/patchwork/patch/26764/
10/10/2017 16:05, santosh:
> Hi Thomas,
>
> On Tuesday 10 October 2017 01:09 PM, Thomas Monjalon wrote:
> > 10/10/2017 09:01, Shreyansh Jain:
> >> Fixes: 5b22cf744689 ("bus/dpaa: introducing FMan configurations")
> >> Fixes: 37f9b54bd3cf ("net/dpaa: support Tx and Rx queue setup")
> > These lines should appear after the explanation.
> >
> >> Cc: shreyansh.jain@nxp.com
> >>
> >> With the IOVA auto detection changes, bus scan is performed before
> >> memory initialization. DPAA bus scan must not use rte_malloc in
> >> its path.
> > If the scan has been broken by IOVA detection, you should reference
> > IOVA in Fixes line, not DPAA.
> >
> hmm.. IOVA not breaking scanning!, Refer this [1].
It is breaking. A break is a behaviour or interface change.
When moving init order, you break behaviour.
I don't say it is bad.
I say only it is the primary cause of this change.
The Fixes: line is also a help when backporting patches.
This patch needs to be backported only if IOVA patch is also backported.
> We(me/hemant) has discussed about same on thread[1] and agreed to
> do respective changes and remove rte_ memory dependency from code base
> at scan time..
>
> Thanks.
>
> [1] http://dpdk.org/dev/patchwork/patch/26764/
You already discussed about this issue, fine.
Santosh, as you insist to talk again about it, one more comment:
It is very good to have discussions on the mailing list.
It would be perfect if all these informations were explicitly given
in the commit messages.
For instance, saying that the scan cannot use rte_malloc anymore is
a valuable tip for other developpers.
On Tuesday 10 October 2017 09:47 PM, Thomas Monjalon wrote:
> 10/10/2017 16:05, santosh:
>> Hi Thomas,
>>
>> On Tuesday 10 October 2017 01:09 PM, Thomas Monjalon wrote:
>>> 10/10/2017 09:01, Shreyansh Jain:
>>>> Fixes: 5b22cf744689 ("bus/dpaa: introducing FMan configurations")
>>>> Fixes: 37f9b54bd3cf ("net/dpaa: support Tx and Rx queue setup")
>>> These lines should appear after the explanation.
>>>
>>>> Cc: shreyansh.jain@nxp.com
>>>>
>>>> With the IOVA auto detection changes, bus scan is performed before
>>>> memory initialization. DPAA bus scan must not use rte_malloc in
>>>> its path.
>>> If the scan has been broken by IOVA detection, you should reference
>>> IOVA in Fixes line, not DPAA.
>>>
>> hmm.. IOVA not breaking scanning!, Refer this [1].
> It is breaking. A break is a behaviour or interface change.
> When moving init order, you break behaviour.
> I don't say it is bad.
> I say only it is the primary cause of this change.
disagree!. Why so: Legacy PCI/bus scan implementation don't
use rte_ lib as they don;t need to.. Refer [1] for detailing.
However, dpaa is and that we agree to align with legacy.
So its a open question : Who fixes who?
> The Fixes: line is also a help when backporting patches.
> This patch needs to be backported only if IOVA patch is also backported.
IMO, would prefer backport: rather fixes: tag in above case, more verbose I guess.
>> We(me/hemant) has discussed about same on thread[1] and agreed to
>> do respective changes and remove rte_ memory dependency from code base
>> at scan time..
>>
>> Thanks.
>>
>> [1] http://dpdk.org/dev/patchwork/patch/26764/
> You already discussed about this issue, fine.
>
> Santosh, as you insist to talk again about it, one more comment:
>
> It is very good to have discussions on the mailing list.
Thanks, That makes me think that I didn't break, indeed did what was needed in agnostic way.
> It would be perfect if all these informations were explicitly given
> in the commit messages.
> For instance, saying that the scan cannot use rte_malloc anymore is
> a valuable tip for other developpers.
Agree, but scan wasn;t using for PCI/bus case.. so one can;t be sure whether to
mention or not..
On Tuesday 10 October 2017 10:25 PM, santosh wrote:
>
> On Tuesday 10 October 2017 09:47 PM, Thomas Monjalon wrote:
>> 10/10/2017 16:05, santosh:
>>> Hi Thomas,
>>>
>>> On Tuesday 10 October 2017 01:09 PM, Thomas Monjalon wrote:
>>>> 10/10/2017 09:01, Shreyansh Jain:
>>>>> Fixes: 5b22cf744689 ("bus/dpaa: introducing FMan configurations")
>>>>> Fixes: 37f9b54bd3cf ("net/dpaa: support Tx and Rx queue setup")
>>>> These lines should appear after the explanation.
>>>>
>>>>> Cc: shreyansh.jain@nxp.com
>>>>>
>>>>> With the IOVA auto detection changes, bus scan is performed before
>>>>> memory initialization. DPAA bus scan must not use rte_malloc in
>>>>> its path.
>>>> If the scan has been broken by IOVA detection, you should reference
>>>> IOVA in Fixes line, not DPAA.
>>>>
>>> hmm.. IOVA not breaking scanning!, Refer this [1].
>> It is breaking. A break is a behaviour or interface change.
>> When moving init order, you break behaviour.
>> I don't say it is bad.
>> I say only it is the primary cause of this change.
>
> disagree!. Why so: Legacy PCI/bus scan implementation don't
> use rte_ lib as they don;t need to.. Refer [1] for detailing.
> However, dpaa is and that we agree to align with legacy.
> So its a open question : Who fixes who?
Just because PCI/bus-scan didn't use rte_malloc didn't mean that no one
else can use it. For DPAA, the case was different and ideally I
shouldn't have used rte_malloc. [1] was a discussion based on FSLMC and
that was incorrect implementation.
But, that is not a general case.
If we go by pre-IOVA patch era:
-> Subsystem A initialized
-> Subsystem B initialized
-> bus scan
`-> implementation free to use all initialized subsystem
post-IOVA era
-> Subsystem A initialized
-> bus scan
`-> implementation free to use all initialized subsystem
-> Subsystem B initialized
Essentially, it means IOVA made a change in the expected behavior of the
implementation.
For case of DPAA, as that was still in development, it certainly can be
said that it should have been fixed within dev cycle itself. That was
the prime reason I used 'Fixes' to my own patches in v1 of this patch.
But that said, I agree with what Thomas is saying. Fixes is only
indicative of which patched changed the status-quo. And it helps
maintainers know dependency tree. In this case, DPAA patches came
*before* IOVA was added and hence the mistake in committed version came
out *after* IOVA patches.
>
>> The Fixes: line is also a help when backporting patches.
>> This patch needs to be backported only if IOVA patch is also backported.
>
> IMO, would prefer backport: rather fixes: tag in above case, more verbose I guess.
>
>>> We(me/hemant) has discussed about same on thread[1] and agreed to
>>> do respective changes and remove rte_ memory dependency from code base
>>> at scan time..
>>>
>>> Thanks.
>>>
>>> [1] http://dpdk.org/dev/patchwork/patch/26764/
>> You already discussed about this issue, fine.
>>
>> Santosh, as you insist to talk again about it, one more comment:
>>
>> It is very good to have discussions on the mailing list.
>
> Thanks, That makes me think that I didn't break, indeed did what was needed in agnostic way.
Again, IOVA patch changes status quo, but that was a known fact and it
was missed by DPAA patches. Now, for maintenance reasons, "Fixes" should
actually be IOVA patch. I am not sure why you disagree with that.
>
>> It would be perfect if all these informations were explicitly given
>> in the commit messages.
>> For instance, saying that the scan cannot use rte_malloc anymore is
>> a valuable tip for other developpers.
>
> Agree, but scan wasn;t using for PCI/bus case.. so one can;t be sure whether to
> mention or not..
>
On Wednesday 11 October 2017 10:47 AM, Shreyansh Jain wrote:
> On Tuesday 10 October 2017 10:25 PM, santosh wrote:
>>
>> On Tuesday 10 October 2017 09:47 PM, Thomas Monjalon wrote:
>>> 10/10/2017 16:05, santosh:
>>>> Hi Thomas,
>>>>
>>>> On Tuesday 10 October 2017 01:09 PM, Thomas Monjalon wrote:
>>>>> 10/10/2017 09:01, Shreyansh Jain:
>>>>>> Fixes: 5b22cf744689 ("bus/dpaa: introducing FMan configurations")
>>>>>> Fixes: 37f9b54bd3cf ("net/dpaa: support Tx and Rx queue setup")
>>>>> These lines should appear after the explanation.
>>>>>
>>>>>> Cc: shreyansh.jain@nxp.com
>>>>>>
>>>>>> With the IOVA auto detection changes, bus scan is performed before
>>>>>> memory initialization. DPAA bus scan must not use rte_malloc in
>>>>>> its path.
>>>>> If the scan has been broken by IOVA detection, you should reference
>>>>> IOVA in Fixes line, not DPAA.
>>>>>
>>>> hmm.. IOVA not breaking scanning!, Refer this [1].
>>> It is breaking. A break is a behaviour or interface change.
>>> When moving init order, you break behaviour.
>>> I don't say it is bad.
>>> I say only it is the primary cause of this change.
>>
>> disagree!. Why so: Legacy PCI/bus scan implementation don't
>> use rte_ lib as they don;t need to.. Refer [1] for detailing.
>> However, dpaa is and that we agree to align with legacy.
>> So its a open question : Who fixes who?
>
> Just because PCI/bus-scan didn't use rte_malloc didn't mean that no one else can use it. For DPAA, the case was different and ideally I shouldn't have used
That means that Scan should do scan only. if not then programmer creating a dependency like it was in past.
> rte_malloc. [1] was a discussion based on FSLMC and that was incorrect implementation.
>
yes But thread did address on _not_ keeping any rte_ memory dependency at scan time.
> But, that is not a general case.
>
> If we go by pre-IOVA patch era:
>
> -> Subsystem A initialized
> -> Subsystem B initialized
> -> bus scan
> `-> implementation free to use all initialized subsystem
>
> post-IOVA era
>
>
> -> Subsystem A initialized
> -> bus scan
> `-> implementation free to use all initialized subsystem
> -> Subsystem B initialized
>
And that the problem with Pre-era that some platform implementation created a dependency on
scan which wasn't necessary and that was blocking new infra.
> Essentially, it means IOVA made a change in the expected behavior of the implementation.
I disagree. IOVA asked to remove those dependency for those platform., If PCI/BUS works w/o dependency
then why not fslmc that same we discussed in thread. You have created
a programming dependency in your platform code at scan time, which after discussion agreed and
now this patch is removing that dependency.
Ideally, Like for other Patches hemant did: You should have blocked IOVA series and let this patch and
other platform dependency patch merged before IOVA.. but that didn't happen perhaps
lack of co-ordination.
> For case of DPAA, as that was still in development, it certainly can be said that it should have been fixed within dev cycle itself. That was the prime reason I used 'Fixes' to my own patches in v1 of this patch.
>
> But that said, I agree with what Thomas is saying. Fixes is only indicative of which patched changed the status-quo. And it helps maintainers know dependency tree. In this case, DPAA patches came *before* IOVA was added and hence the mistake in committed version came out *after* IOVA patches.
>
You should have blocked IOVA in that case, as said above like Hemant did for one of his patch.
Anyways, I'm not objecting on Fixes: tags valid or not. Yes, from git point of view it is correct
But from design point of view - I disagree.
>>
>>> The Fixes: line is also a help when backporting patches.
>>> This patch needs to be backported only if IOVA patch is also backported.
>>
>> IMO, would prefer backport: rather fixes: tag in above case, more verbose I guess.
>>
>>>> We(me/hemant) has discussed about same on thread[1] and agreed to
>>>> do respective changes and remove rte_ memory dependency from code base
>>>> at scan time..
>>>>
>>>> Thanks.
>>>>
>>>> [1] http://dpdk.org/dev/patchwork/patch/26764/
>>> You already discussed about this issue, fine.
>>>
>>> Santosh, as you insist to talk again about it, one more comment:
>>>
>>> It is very good to have discussions on the mailing list.
>>
>> Thanks, That makes me think that I didn't break, indeed did what was needed in agnostic way.
>
> Again, IOVA patch changes status quo, but that was a known fact and it was missed by DPAA patches. Now, for maintenance reasons, "Fixes" should actually be IOVA patch. I am not sure why you disagree with that.
>
Read above reason for disagreement.
>>
>>> It would be perfect if all these informations were explicitly given
>>> in the commit messages.
>>> For instance, saying that the scan cannot use rte_malloc anymore is
>>> a valuable tip for other developpers.
>>
>> Agree, but scan wasn;t using for PCI/bus case.. so one can;t be sure whether to
>> mention or not..
>>
@@ -42,8 +42,6 @@
#include <sys/ioctl.h>
#include <ifaddrs.h>
-#include <rte_malloc.h>
-
/* This header declares the driver interface we implement */
#include <fman.h>
#include <of.h>
@@ -72,15 +70,18 @@ if_destructor(struct __fman_if *__if)
{
struct fman_if_bpool *bp, *tmpbp;
+ if (!__if)
+ return;
+
if (__if->__if.mac_type == fman_offline)
goto cleanup;
list_for_each_entry_safe(bp, tmpbp, &__if->__if.bpool_list, node) {
list_del(&bp->node);
- rte_free(bp);
+ free(bp);
}
cleanup:
- rte_free(__if);
+ free(__if);
}
static int
@@ -208,7 +209,7 @@ fman_if_init(const struct device_node *dpa_node)
mprop = "fsl,fman-mac";
/* Allocate an object for this network interface */
- __if = rte_malloc(NULL, sizeof(*__if), RTE_CACHE_LINE_SIZE);
+ __if = malloc(sizeof(*__if));
if (!__if) {
FMAN_ERR(-ENOMEM, "malloc(%zu)\n", sizeof(*__if));
goto err;
@@ -464,7 +465,7 @@ fman_if_init(const struct device_node *dpa_node)
uint64_t bpool_host[6] = {0};
const char *pname;
/* Allocate an object for the pool */
- bpool = rte_malloc(NULL, sizeof(*bpool), RTE_CACHE_LINE_SIZE);
+ bpool = malloc(sizeof(*bpool));
if (!bpool) {
FMAN_ERR(-ENOMEM, "malloc(%zu)\n", sizeof(*bpool));
goto err;
@@ -603,7 +604,7 @@ fman_finish(void)
-errno, strerror(errno));
printf("Tearing down %s\n", __if->node_path);
list_del(&__if->__if.node);
- rte_free(__if);
+ free(__if);
}
close(fman_ccsr_map_fd);
@@ -921,7 +921,7 @@ dpaa_dev_init(struct rte_eth_dev *eth_dev)
/* reset bpool list, initialize bpool dynamically */
list_for_each_entry_safe(bp, tmp_bp, &cfg->fman_if->bpool_list, node) {
list_del(&bp->node);
- rte_free(bp);
+ free(bp);
}
/* Populate ethdev structure */