| Message ID | 20230705143507.4120-1-machel@vivo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net:thunderx:Fix resource leaks in device_for_each_child_node() loops | expand |
On Wed, Jul 05, 2023 at 10:34:56PM +0800, Wang Ming wrote: > The device_for_each_child_node() loop in > bgx_init_of_phy() function should have > wnode_handle_put() before break > which could avoid resource leaks. > This patch could fix this bug. It is very strange typographic. You have ~80 chars per-line, while your longest line is 40 chars only. > > Signed-off-by: Wang Ming <machel@vivo.com> > --- > drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > index a317feb8decb..dad32d36a015 100644 > --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > @@ -1478,8 +1478,10 @@ static int bgx_init_of_phy(struct bgx *bgx) > * cannot handle it, so exit the loop. > */ > node = to_of_node(fwn); > - if (!node) > + if (!node) { > + fwnode_handle_put(fwn); > break; > + } > > of_get_mac_address(node, bgx->lmac[lmac].mac); > > @@ -1503,6 +1505,7 @@ static int bgx_init_of_phy(struct bgx *bgx) > lmac++; > if (lmac == bgx->max_lmac) { > of_node_put(node); > + fwnode_handle_put(fwn); > break; > } > } > -- > 2.25.1 > >
On 2023-07-05 22:34 +0800, Wang Ming wrote: > The device_for_each_child_node() loop in > bgx_init_of_phy() function should have > wnode_handle_put() before break ^ fwnode_handle_put() > which could avoid resource leaks. > This patch could fix this bug. > > Signed-off-by: Wang Ming <machel@vivo.com> > --- > drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > index a317feb8decb..dad32d36a015 100644 > --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > @@ -1478,8 +1478,10 @@ static int bgx_init_of_phy(struct bgx *bgx) > * cannot handle it, so exit the loop. > */ > node = to_of_node(fwn); > - if (!node) > + if (!node) { > + fwnode_handle_put(fwn); > break; > + } Fixes: eee326fd8334 ("net: thunderx: bgx: Use standard firmware node infrastructure.") ? > > of_get_mac_address(node, bgx->lmac[lmac].mac); > > @@ -1503,6 +1505,7 @@ static int bgx_init_of_phy(struct bgx *bgx) > lmac++; > if (lmac == bgx->max_lmac) { > of_node_put(node); > + fwnode_handle_put(fwn); \ fwnode_handle_put \ of_fwnode_put of_node_put(to_of_node(fwnode)); With your patch, there are now two references released on 'node' (two of_node_put(node) calls). One reference is from device_for_each_child_node(), where was the other reference taken?
> The device_for_each_child_node() loop in > bgx_init_of_phy() function should have > wnode_handle_put() before break > which could avoid resource leaks. > This patch could fix this bug. Please choose a better imperative change suggestion. See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94 How do you think about to add the tag “Fixes”? Regards, Markus
Hi Markus. Can you help me make a template to improve the wording? I have little experience in this area. Thank you
Hi They all come from device for each child node(). -----邮件原件----- 发件人: Benjamin Poirier <benjamin.poirier@gmail.com> 发送时间: 2023年7月6日 3:29 收件人: 王明-软件底层技术部 <machel@vivo.com> 抄送: Sunil Goutham <sgoutham@marvell.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com> 主题: Re: [PATCH net v2] net:thunderx:Fix resource leaks in device_for_each_child_node() loops [Some people who received this message don't often get email from benjamin.poirier@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] On 2023-07-05 22:34 +0800, Wang Ming wrote: > The device_for_each_child_node() loop in > bgx_init_of_phy() function should have > wnode_handle_put() before break ^ fwnode_handle_put() > which could avoid resource leaks. > This patch could fix this bug. > > Signed-off-by: Wang Ming <machel@vivo.com> > --- > drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > index a317feb8decb..dad32d36a015 100644 > --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > @@ -1478,8 +1478,10 @@ static int bgx_init_of_phy(struct bgx *bgx) > * cannot handle it, so exit the loop. > */ > node = to_of_node(fwn); > - if (!node) > + if (!node) { > + fwnode_handle_put(fwn); > break; > + } Fixes: eee326fd8334 ("net: thunderx: bgx: Use standard firmware node infrastructure.") ? > > of_get_mac_address(node, bgx->lmac[lmac].mac); > > @@ -1503,6 +1505,7 @@ static int bgx_init_of_phy(struct bgx *bgx) > lmac++; > if (lmac == bgx->max_lmac) { > of_node_put(node); > + fwnode_handle_put(fwn); \ fwnode_handle_put \ of_fwnode_put of_node_put(to_of_node(fwnode)); With your patch, there are now two references released on 'node' (two of_node_put(node) calls). One reference is from device_for_each_child_node(), where was the other reference taken?
> Can you help me make a template to improve the wording? I (and other contributors) are trying for a while. > I have little experience in this area. How will your experiences grow according to linked development documentation and further information from patch reviews? Regards, Markus
Hi Sorry, I resubmitted, but forgot about your point. Does this affect patch? -----邮件原件----- 发件人: Leon Romanovsky <leon@kernel.org> 发送时间: 2023年7月6日 3:10 收件人: 王明-软件底层技术部 <machel@vivo.com> 抄送: Sunil Goutham <sgoutham@marvell.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com> 主题: Re: [PATCH net v2] net:thunderx:Fix resource leaks in device_for_each_child_node() loops [Some people who received this message don't often get email from leon@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] On Wed, Jul 05, 2023 at 10:34:56PM +0800, Wang Ming wrote: > The device_for_each_child_node() loop in > bgx_init_of_phy() function should have > wnode_handle_put() before break > which could avoid resource leaks. > This patch could fix this bug. It is very strange typographic. You have ~80 chars per-line, while your longest line is 40 chars only. > > Signed-off-by: Wang Ming <machel@vivo.com> > --- > drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > index a317feb8decb..dad32d36a015 100644 > --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c > @@ -1478,8 +1478,10 @@ static int bgx_init_of_phy(struct bgx *bgx) > * cannot handle it, so exit the loop. > */ > node = to_of_node(fwn); > - if (!node) > + if (!node) { > + fwnode_handle_put(fwn); > break; > + } > > of_get_mac_address(node, bgx->lmac[lmac].mac); > > @@ -1503,6 +1505,7 @@ static int bgx_init_of_phy(struct bgx *bgx) > lmac++; > if (lmac == bgx->max_lmac) { > of_node_put(node); > + fwnode_handle_put(fwn); > break; > } > } > -- > 2.25.1 > >
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c index a317feb8decb..dad32d36a015 100644 --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c @@ -1478,8 +1478,10 @@ static int bgx_init_of_phy(struct bgx *bgx) * cannot handle it, so exit the loop. */ node = to_of_node(fwn); - if (!node) + if (!node) { + fwnode_handle_put(fwn); break; + } of_get_mac_address(node, bgx->lmac[lmac].mac); @@ -1503,6 +1505,7 @@ static int bgx_init_of_phy(struct bgx *bgx) lmac++; if (lmac == bgx->max_lmac) { of_node_put(node); + fwnode_handle_put(fwn); break; } }
The device_for_each_child_node() loop in bgx_init_of_phy() function should have wnode_handle_put() before break which could avoid resource leaks. This patch could fix this bug. Signed-off-by: Wang Ming <machel@vivo.com> --- drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)