Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-06 Thread Markus Elfring 
> I'm curious if I could still modify these commit message information for the 
> v1 patch,
> which has already been applied and queued up?

The maintainer found the provided information good enough.
Thus he committed the software correction with the subject
“nfp: abm: fix a memory leak bug” on 2020-05-04.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/net/ethernet/netronome/nfp/abm/main.c?id=bd4af432cc71b5fbfe4833510359a6ad3ada250d

So this change will probably be published “forever” since then.

I got the impression that the corresponding patch review contains helpful 
information.
I am curious then if it might affect the adjustment of related patches.


>> Will such considerations become relevant for any subsequent
>> software development approaches?
>
> Sorry, I actually don't familiar with these.

I am informed in the way that you can participate in university research groups.
Thus I assumed that you would like to add recent insights
from computer science areas.
I imagined that the bug report (combined with a patch) was triggered by
an evolving source code analysis approach which will be explained
in another research paper. Is such a view appropriate?
https://github.com/umnsec/cheq/

Regards,
Markus

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-05 Thread Markus Elfring 
> What do you think about changing:
> "But when nfp_nsp_has_hwinfo_lookup fail, the pointer is not released,.."
> to
…
> or
> "But when nfp_nsp_has_hwinfo_lookup fail,

I became curious about a related wording variant.

  But when a call of the function “…” failed,


> NSP resource is not cleaned up and unlocked."

I find such information also nicer. (The abbreviation “NSP” might need
another bit of clarification.)

I imagine there might be interests (eventually related to computer science)
to measure the corresponding object sizes because of a missed function call
and offer a more precise information in the commit message
(depending on the willingness to invest efforts in such a data determination).

Will such considerations become relevant for any subsequent
software development approaches?

Regards,
Markus

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-05 Thread Markus Elfring 
>> Do you distinguish between releasing items and the role of such a pointer?
> I didn't distinguish these. 

How do you think about to clarify this aspect a bit more (according to computer 
science)
besides your subsequent constructive feedback?

Regards,
Markus

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-04 Thread Markus Elfring 
> Thanks for your feedback, and yes, I'd like to further adjust the description 
> details
> to make the patch more clear and better.

Thanks for such a positive response.


> But because Jakub seems to prefer v1, so I'm somehow confused

Such a view can be reasonable.
The change acceptance varies between involved contributors.


> if I should submit a new version based on v1

Such a choice depends also on your willingness to clarify and improve
the software situation by better commit messages.


> or based on the version that has an addition of a jump target?

I propose that you can offer patch series with two update steps (for example)
according to the affected function implementation.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=47cf1b422e6093aee2a3e55d5e162112a2c69870#n102

1. Add a missing function call for the completion of the desired resource 
clean-up.

2. Adjust a bit of exception handling code so that it can be better reused
   at the end of this function.
   
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=47cf1b422e6093aee2a3e55d5e162112a2c69870#n450


I am curious if you are going to answer (my) previous questions and suggestions
(in constructive ways).

Regards,
Markus

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-04 Thread Markus Elfring 
> By the way, is there anything else that I need to improve for this patch?

I became curious if you would like to adjust further details from
the change description.
Other contributors might care less for presented concerns.

Regards,
Markus

Re: [PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-04 Thread Jakub Kicinski 
On Sun,  3 May 2020 15:49:32 -0500 wu000...@umn.edu wrote:
> From: Qiushi Wu 
> 
> In function nfp_abm_vnic_set_mac, pointer nsp is allocated by nfp_nsp_open.
> But when nfp_nsp_has_hwinfo_lookup fail, the pointer is not released,
> which can lead to a memory leak bug. Thus add a call of the function
> “nfp_nsp_close” for the completion of the exception handling.
> 
> Fixes: f6e71efdf9fb1 ("nfp: abm: look up MAC addresses via management FW")
> Signed-off-by: Qiushi Wu 

This just makes the code longer. v1 was perfectly fine, thanks for the
fix.

Re: [PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-04 Thread Markus Elfring 
> But when nfp_nsp_has_hwinfo_lookup fail, the pointer is not released,

Do you distinguish between releasing items and the role of such a pointer?


> which can lead to a memory leak bug.

Would you like to reconsider the usage of the word “can” for
such change descriptions?

Regards,
Markus

Re: [PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-04 Thread Markus Elfring 
> … Thus add a call of the function
> “nfp_nsp_close” for the completion of the exception handling.

I suggest to mention also the addition of a jump target because of
a Linux coding style concern.


…
> +++ b/drivers/net/ethernet/netronome/nfp/abm/main.c
…
> @@ -300,12 +297,16 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm 
> *abm, struct nfp_net *nn,
…
> +generate_random_address:
> + eth_hw_addr_random(nn->dp.netdev);
> + return;
>  }
…

I recommend to apply the check “RETURN_VOID” once more.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=0e698dfa282211e414076f9dc7e83c1c288314fd#n4866

Regards,
Markus

[PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

2020-05-03 Thread wu000273 
From: Qiushi Wu 

In function nfp_abm_vnic_set_mac, pointer nsp is allocated by nfp_nsp_open.
But when nfp_nsp_has_hwinfo_lookup fail, the pointer is not released,
which can lead to a memory leak bug. Thus add a call of the function
“nfp_nsp_close” for the completion of the exception handling.

Fixes: f6e71efdf9fb1 ("nfp: abm: look up MAC addresses via management FW")
Signed-off-by: Qiushi Wu 
---
 drivers/net/ethernet/netronome/nfp/abm/main.c | 21 ++-
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/abm/main.c 
b/drivers/net/ethernet/netronome/nfp/abm/main.c
index 9183b3e85d21..f196789f62fe 100644
--- a/drivers/net/ethernet/netronome/nfp/abm/main.c
+++ b/drivers/net/ethernet/netronome/nfp/abm/main.c
@@ -265,8 +265,7 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm 
*abm, struct nfp_net *nn,
 
if (id > pf->eth_tbl->count) {
nfp_warn(pf->cpp, "No entry for persistent MAC address\n");
-   eth_hw_addr_random(nn->dp.netdev);
-   return;
+   goto generate_random_address;
}
 
snprintf(hwinfo, sizeof(hwinfo), "eth%u.mac.pf%u",
@@ -276,14 +275,13 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm 
*abm, struct nfp_net *nn,
if (IS_ERR(nsp)) {
nfp_warn(pf->cpp, "Failed to access the NSP for persistent MAC 
address: %ld\n",
 PTR_ERR(nsp));
-   eth_hw_addr_random(nn->dp.netdev);
-   return;
+   goto generate_random_address;
}
 
if (!nfp_nsp_has_hwinfo_lookup(nsp)) {
nfp_warn(pf->cpp, "NSP doesn't support PF MAC generation\n");
-   eth_hw_addr_random(nn->dp.netdev);
-   return;
+   nfp_nsp_close(nsp);
+   goto generate_random_address;
}
 
err = nfp_nsp_hwinfo_lookup(nsp, hwinfo, sizeof(hwinfo));
@@ -291,8 +289,7 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm 
*abm, struct nfp_net *nn,
if (err) {
nfp_warn(pf->cpp, "Reading persistent MAC address failed: %d\n",
 err);
-   eth_hw_addr_random(nn->dp.netdev);
-   return;
+   goto generate_random_address;
}
 
if (sscanf(hwinfo, "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx",
@@ -300,12 +297,16 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm 
*abm, struct nfp_net *nn,
   _addr[3], _addr[4], _addr[5]) != 6) {
nfp_warn(pf->cpp, "Can't parse persistent MAC address (%s)\n",
 hwinfo);
-   eth_hw_addr_random(nn->dp.netdev);
-   return;
+   goto generate_random_address;
}
 
ether_addr_copy(nn->dp.netdev->dev_addr, mac_addr);
ether_addr_copy(nn->dp.netdev->perm_addr, mac_addr);
+   return;
+
+generate_random_address:
+   eth_hw_addr_random(nn->dp.netdev);
+   return;
 }
 
 static int
-- 
2.17.1