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

[Shreyansh Jain <shreyansh.jain@nxp.com>]

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