[dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[Shreyansh Jain]

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(-)

diff --git a/drivers/bus/dpaa/base/fman/fman.c b/drivers/bus/dpaa/base/fman/fman.c
index d0a8ee4..3816dba 100644
--- a/drivers/bus/dpaa/base/fman/fman.c
+++ b/drivers/bus/dpaa/base/fman/fman.c
@@ -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);
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index 8dad97e..9f33e44 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -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 */
-- 
2.9.3

Re: [dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[Shreyansh Jain]

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.

[dpdk-dev] [PATCH v2] bus/dpaa: fix memory allocation during bus scan

[Shreyansh Jain]

With the IOVA auto detection changes, bus scan is performed before
memory initialization. DPAA bus scan must not use rte_malloc in
its path.

Fixes: cf408c22476c ("eal: auto detect IOVA mode")
Cc: santosh.shukla@caviumnetworks.com

Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
v2:
 Changed the Fixes line to correct commit
 Correct position of fixes line

 drivers/bus/dpaa/base/fman/fman.c | 15 ++++++++-------
 drivers/net/dpaa/dpaa_ethdev.c    |  2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/dpaa/base/fman/fman.c b/drivers/bus/dpaa/base/fman/fman.c
index d0a8ee4..3816dba 100644
--- a/drivers/bus/dpaa/base/fman/fman.c
+++ b/drivers/bus/dpaa/base/fman/fman.c
@@ -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);
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c
index 8dad97e..9f33e44 100644
--- a/drivers/net/dpaa/dpaa_ethdev.c
+++ b/drivers/net/dpaa/dpaa_ethdev.c
@@ -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 */
-- 
2.9.3

Re: [dpdk-dev] [PATCH v2] bus/dpaa: fix memory allocation during bus scan

[Thomas Monjalon]

10/10/2017 11:34, Shreyansh Jain:
> With the IOVA auto detection changes, bus scan is performed before
> memory initialization. DPAA bus scan must not use rte_malloc in
> its path.
> 
> Fixes: cf408c22476c ("eal: auto detect IOVA mode")
> Cc: santosh.shukla@caviumnetworks.com
> 
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>

Applied, thanks

Re: [dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[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].

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/

Re: [dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[santosh]

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..

Re: [dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[Shreyansh Jain]

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..
> 

Re: [dpdk-dev] [PATCH] bus/dpaa: fix memory allocation during bus scan

[santosh]

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..
>>