2023-03-16 02:39:32

by Jiasheng Jiang

[permalink] [raw]
Subject: [PATCH] octeontx2-vf: Add missing free for alloc_percpu

Add the free_percpu for the allocated "vf->hw.lmt_info" in order to avoid
memory leak, same as the "pf->hw.lmt_info" in
`drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c`.

Fixes: 5c0512072f65 ("octeontx2-pf: cn10k: Use runtime allocated LMTLINE region")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
index 7f8ffbf79cf7..9db2e2d218bb 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
@@ -709,6 +709,8 @@ static int otx2vf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
err_ptp_destroy:
otx2_ptp_destroy(vf);
err_detach_rsrc:
+ if (vf->hw.lmt_info)
+ free_percpu(vf->hw.lmt_info);
if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
qmem_free(vf->dev, vf->dync_lmt);
otx2_detach_resources(&vf->mbox);
@@ -762,6 +764,8 @@ static void otx2vf_remove(struct pci_dev *pdev)
otx2_shutdown_tc(vf);
otx2vf_disable_mbox_intr(vf);
otx2_detach_resources(&vf->mbox);
+ if (vf->hw.lmt_info)
+ free_percpu(vf->hw.lmt_info);
if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
qmem_free(vf->dev, vf->dync_lmt);
otx2vf_vfaf_mbox_destroy(vf);
--
2.25.1



2023-03-16 08:10:25

by Michal Swiatkowski

[permalink] [raw]
Subject: Re: [PATCH] octeontx2-vf: Add missing free for alloc_percpu

On Thu, Mar 16, 2023 at 10:39:11AM +0800, Jiasheng Jiang wrote:
> Add the free_percpu for the allocated "vf->hw.lmt_info" in order to avoid
> memory leak, same as the "pf->hw.lmt_info" in
> `drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c`.
>
> Fixes: 5c0512072f65 ("octeontx2-pf: cn10k: Use runtime allocated LMTLINE region")
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> index 7f8ffbf79cf7..9db2e2d218bb 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> @@ -709,6 +709,8 @@ static int otx2vf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> err_ptp_destroy:
> otx2_ptp_destroy(vf);
> err_detach_rsrc:
> + if (vf->hw.lmt_info)
> + free_percpu(vf->hw.lmt_info);
> if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
> qmem_free(vf->dev, vf->dync_lmt);
I wonder if it wouldn't be more error prune when You create a function
to unroll cn10k_lmtst_init() like cn10k_lmtst_deinit(). These two if can
be there, maybe also sth else is missing.

Otherwise it is fine
Reviewed-by: Michal Swiatkowski <[email protected]>

> otx2_detach_resources(&vf->mbox);
> @@ -762,6 +764,8 @@ static void otx2vf_remove(struct pci_dev *pdev)
> otx2_shutdown_tc(vf);
> otx2vf_disable_mbox_intr(vf);
> otx2_detach_resources(&vf->mbox);
> + if (vf->hw.lmt_info)
> + free_percpu(vf->hw.lmt_info);
> if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
> qmem_free(vf->dev, vf->dync_lmt);
> otx2vf_vfaf_mbox_destroy(vf);
> --
> 2.25.1
>

2023-03-16 09:02:44

by Geetha sowjanya

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH] octeontx2-vf: Add missing free for alloc_percpu



>-----Original Message-----
>From: Michal Swiatkowski <[email protected]>
>Sent: Thursday, March 16, 2023 1:40 PM
>To: Jiasheng Jiang <[email protected]>
>Cc: Sunil Kovvuri Goutham <[email protected]>; Geethasowjanya Akula <[email protected]>; Subbaraya Sundeep Bhatta <[email protected]>; Hariprasad Kelam <[email protected]>; >[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
>Subject: [EXT] Re: [PATCH] octeontx2-vf: Add missing free for alloc_percpu

>External Email

>----------------------------------------------------------------------
>On Thu, Mar 16, 2023 at 10:39:11AM +0800, Jiasheng Jiang wrote:
>> Add the free_percpu for the allocated "vf->hw.lmt_info" in order to
>> avoid memory leak, same as the "pf->hw.lmt_info" in
>> `drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c`.
>>
>> Fixes: 5c0512072f65 ("octeontx2-pf: cn10k: Use runtime allocated
>> LMTLINE region")
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>> ---
>> drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> index 7f8ffbf79cf7..9db2e2d218bb 100644
>> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> @@ -709,6 +709,8 @@ static int otx2vf_probe(struct pci_dev *pdev,
>> const struct pci_device_id *id)
>> err_ptp_destroy:
>> otx2_ptp_destroy(vf);
>> err_detach_rsrc:
>> + if (vf->hw.lmt_info)
>> + free_percpu(vf->hw.lmt_info);
>> if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
>> qmem_free(vf->dev, vf->dync_lmt);
>I wonder if it wouldn't be more error prune when You create a function to unroll cn10k_lmtst_init() like cn10k_lmtst_deinit(). These two if can be there, maybe also sth else is missing.

>Otherwise it is fine
>Reviewed-by: Michal Swiatkowski <[email protected]>

>> otx2_detach_resources(&vf->mbox);
>> @@ -762,6 +764,8 @@ static void otx2vf_remove(struct pci_dev *pdev)
>> otx2_shutdown_tc(vf);
>> otx2vf_disable_mbox_intr(vf);
>> otx2_detach_resources(&vf->mbox);
>> + if (vf->hw.lmt_info)
>> + free_percpu(vf->hw.lmt_info);

Ack. Thanks for the patch.

>> if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
>> qmem_free(vf->dev, vf->dync_lmt);
>> otx2vf_vfaf_mbox_destroy(vf);
>> --
>> 2.25.1
>>

Geetha.

2023-03-16 09:59:19

by Jiasheng Jiang

[permalink] [raw]
Subject: Re: Re: [PATCH] octeontx2-vf: Add missing free for alloc_percpu

On Thu, Mar 16, 2023 at 10:39:11AM +0800, Jiasheng Jiang wrote:
> On Thu, Mar 16, 2023 at 10:39:11AM +0800, Jiasheng Jiang wrote:
>> Add the free_percpu for the allocated "vf->hw.lmt_info" in order to avoid
>> memory leak, same as the "pf->hw.lmt_info" in
>> `drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c`.
>>
>> Fixes: 5c0512072f65 ("octeontx2-pf: cn10k: Use runtime allocated LMTLINE region")
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>> ---
>> drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> index 7f8ffbf79cf7..9db2e2d218bb 100644
>> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> @@ -709,6 +709,8 @@ static int otx2vf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> err_ptp_destroy:
>> otx2_ptp_destroy(vf);
>> err_detach_rsrc:
>> + if (vf->hw.lmt_info)
>> + free_percpu(vf->hw.lmt_info);
>> if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
>> qmem_free(vf->dev, vf->dync_lmt);
> I wonder if it wouldn't be more error prune when You create a function
> to unroll cn10k_lmtst_init() like cn10k_lmtst_deinit(). These two if can
> be there, maybe also sth else is missing.
>
> Otherwise it is fine
> Reviewed-by: Michal Swiatkowski <[email protected]>

I try my best and find nothing else missing.

Thanks,
Jiang


2023-03-17 04:25:14

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH] octeontx2-vf: Add missing free for alloc_percpu

On Thu, 16 Mar 2023 10:39:11 +0800 Jiasheng Jiang wrote:
> + if (vf->hw.lmt_info)
> + free_percpu(vf->hw.lmt_info);
> if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
> qmem_free(vf->dev, vf->dync_lmt);
> otx2_detach_resources(&vf->mbox);
> @@ -762,6 +764,8 @@ static void otx2vf_remove(struct pci_dev *pdev)
> otx2_shutdown_tc(vf);
> otx2vf_disable_mbox_intr(vf);
> otx2_detach_resources(&vf->mbox);
> + if (vf->hw.lmt_info)
> + free_percpu(vf->hw.lmt_info);

No need for the if () checks, free_percpu() seems to handle NULL just
fine.

2023-03-17 06:34:40

by Jiasheng Jiang

[permalink] [raw]
Subject: Re: Re: [PATCH] octeontx2-vf: Add missing free for alloc_percpu

On Fri, Mar 17, 2023 at 12:24:46AM +0800, Jakub Kicinski wrote:
> On Thu, 16 Mar 2023 10:39:11 +0800 Jiasheng Jiang wrote:
>> + if (vf->hw.lmt_info)
>> + free_percpu(vf->hw.lmt_info);
>> if (test_bit(CN10K_LMTST, &vf->hw.cap_flag))
>> qmem_free(vf->dev, vf->dync_lmt);
>> otx2_detach_resources(&vf->mbox);
>> @@ -762,6 +764,8 @@ static void otx2vf_remove(struct pci_dev *pdev)
>> otx2_shutdown_tc(vf);
>> otx2vf_disable_mbox_intr(vf);
>> otx2_detach_resources(&vf->mbox);
>> + if (vf->hw.lmt_info)
>> + free_percpu(vf->hw.lmt_info);
>
> No need for the if () checks, free_percpu() seems to handle NULL just
> fine.

I will submit a v2 removing the checks.

Thanks,
Jiang