diff mbox series

[net] octeontx2-pf: Fix resource leakage in VF driver unbind

Message ID 20230109061325.21395-1-hkelam@marvell.com
State New
Headers
Series [net] octeontx2-pf: Fix resource leakage in VF driver unbind |

Commit Message

Hariprasad Kelam Jan. 9, 2023, 6:13 a.m. UTC 
  resources allocated like mcam entries to support the Ntuple feature
and hash tables for the tc feature are not getting freed in driver
unbind. This patch fixes the issue.

Fixes: 2da489432747 ("octeontx2-pf: devlink params support to set mcam entry count")
Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
---
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Leon Romanovsky Jan. 10, 2023, 9:57 a.m. UTC | #1 
On Mon, Jan 09, 2023 at 11:43:25AM +0530, Hariprasad Kelam wrote:
> resources allocated like mcam entries to support the Ntuple feature
> and hash tables for the tc feature are not getting freed in driver
> unbind. This patch fixes the issue.

It is not clear where in otx2vf_probe() these resource are allocated.
Please add the stack trace to the commit message.

Thanks

> 
> Fixes: 2da489432747 ("octeontx2-pf: devlink params support to set mcam entry count")
> Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> index 86653bb8e403..7f8ffbf79cf7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> @@ -758,6 +758,8 @@ static void otx2vf_remove(struct pci_dev *pdev)
>  	if (vf->otx2_wq)
>  		destroy_workqueue(vf->otx2_wq);
>  	otx2_ptp_destroy(vf);
> +	otx2_mcam_flow_del(vf);
> +	otx2_shutdown_tc(vf);
>  	otx2vf_disable_mbox_intr(vf);
>  	otx2_detach_resources(&vf->mbox);
>  	if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
> -- 
> 2.17.1
>
patchwork-bot+netdevbpf@kernel.org Jan. 10, 2023, 10:20 a.m. UTC | #2 
Hello:

This patch was applied to netdev/net.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 9 Jan 2023 11:43:25 +0530 you wrote:
> resources allocated like mcam entries to support the Ntuple feature
> and hash tables for the tc feature are not getting freed in driver
> unbind. This patch fixes the issue.
> 
> Fixes: 2da489432747 ("octeontx2-pf: devlink params support to set mcam entry count")
> Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> 
> [...]

Here is the summary with links:
  - [net] octeontx2-pf: Fix resource leakage in VF driver unbind
    https://git.kernel.org/netdev/net/c/53da7aec3298

You are awesome, thank you!
Leon Romanovsky Jan. 10, 2023, 10:29 a.m. UTC | #3 
On Tue, Jan 10, 2023 at 10:20:15AM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This patch was applied to netdev/net.git (master)
> by Paolo Abeni <pabeni@redhat.com>:
> 
> On Mon, 9 Jan 2023 11:43:25 +0530 you wrote:
> > resources allocated like mcam entries to support the Ntuple feature
> > and hash tables for the tc feature are not getting freed in driver
> > unbind. This patch fixes the issue.
> > 
> > Fixes: 2da489432747 ("octeontx2-pf: devlink params support to set mcam entry count")
> > Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> > 
> > [...]
> 
> Here is the summary with links:
>   - [net] octeontx2-pf: Fix resource leakage in VF driver unbind
>     https://git.kernel.org/netdev/net/c/53da7aec3298

Paolo,

I don't think that this patch should be applied.

It looks like wrong Fixes to me and I don't see clearly how structures
were allocated on VF which were cleared in this patch.

Thanks

> 
> You are awesome, thank you!
> -- 
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
> 
>
Paolo Abeni Jan. 10, 2023, 10:43 a.m. UTC | #4 
Hello,

On Tue, 2023-01-10 at 12:29 +0200, Leon Romanovsky wrote:
> On Tue, Jan 10, 2023 at 10:20:15AM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> > Hello:
> > 
> > This patch was applied to netdev/net.git (master)
> > by Paolo Abeni <pabeni@redhat.com>:
> > 
> > On Mon, 9 Jan 2023 11:43:25 +0530 you wrote:
> > > resources allocated like mcam entries to support the Ntuple feature
> > > and hash tables for the tc feature are not getting freed in driver
> > > unbind. This patch fixes the issue.
> > > 
> > > Fixes: 2da489432747 ("octeontx2-pf: devlink params support to set mcam entry count")
> > > Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> > > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> > > 
> > > [...]
> > 
> > Here is the summary with links:
> >   - [net] octeontx2-pf: Fix resource leakage in VF driver unbind
> >     https://git.kernel.org/netdev/net/c/53da7aec3298
> 
> I don't think that this patch should be applied.
> 
> It looks like wrong Fixes to me and I don't see clearly how structures
> were allocated on VF which were cleared in this patch.

My understanding is that the resource allocation happens via:

otx2_dl_mcam_count_set()

which is registered as the devlink parameter write operation on the vf
by the fixes commit - the patch looks legit to me.

Did I miss something?

Thanks!

Paolo
Leon Romanovsky Jan. 10, 2023, 11:11 a.m. UTC | #5 
On Tue, Jan 10, 2023 at 11:43:28AM +0100, Paolo Abeni wrote:
> Hello,
> 
> On Tue, 2023-01-10 at 12:29 +0200, Leon Romanovsky wrote:
> > On Tue, Jan 10, 2023 at 10:20:15AM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> > > Hello:
> > > 
> > > This patch was applied to netdev/net.git (master)
> > > by Paolo Abeni <pabeni@redhat.com>:
> > > 
> > > On Mon, 9 Jan 2023 11:43:25 +0530 you wrote:
> > > > resources allocated like mcam entries to support the Ntuple feature
> > > > and hash tables for the tc feature are not getting freed in driver
> > > > unbind. This patch fixes the issue.
> > > > 
> > > > Fixes: 2da489432747 ("octeontx2-pf: devlink params support to set mcam entry count")
> > > > Signed-off-by: Hariprasad Kelam <hkelam@marvell.com>
> > > > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> > > > 
> > > > [...]
> > > 
> > > Here is the summary with links:
> > >   - [net] octeontx2-pf: Fix resource leakage in VF driver unbind
> > >     https://git.kernel.org/netdev/net/c/53da7aec3298
> > 
> > I don't think that this patch should be applied.
> > 
> > It looks like wrong Fixes to me and I don't see clearly how structures
> > were allocated on VF which were cleared in this patch.
> 
> My understanding is that the resource allocation happens via:
> 
> otx2_dl_mcam_count_set()
> 
> which is registered as the devlink parameter write operation on the vf
> by the fixes commit - the patch looks legit to me.
> 
> Did I miss something?

No, you are right. I'm not sure if I would be able to see that OTX2_FLAG_MCAM_ENTRIES_ALLOC
flag without your hint.

Thanks
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
index 86653bb8e403..7f8ffbf79cf7 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
@@ -758,6 +758,8 @@  static void otx2vf_remove(struct pci_dev *pdev)
 	if (vf->otx2_wq)
 		destroy_workqueue(vf->otx2_wq);
 	otx2_ptp_destroy(vf);
+	otx2_mcam_flow_del(vf);
+	otx2_shutdown_tc(vf);
 	otx2vf_disable_mbox_intr(vf);
 	otx2_detach_resources(&vf->mbox);
 	if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))