| Message ID | 20241024140057.18548-1-wanghai38@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: qat - Fix missing destroy_workqueue in adf_init_aer() | expand |
On Thu, Oct 24, 2024 at 10:00:57PM +0800, Wang Hai wrote: > The adf_init_aer() won't destroy device_reset_wq when alloc_workqueue() > for device_sriov_wq failed. Add destroy_workqueue for device_reset_wq to > fix this issue. > > Fixes: 4469f9b23468 ("crypto: qat - re-enable sriov after pf reset") > Signed-off-by: Wang Hai <wanghai38@huawei.com> > --- > drivers/crypto/intel/qat/qat_common/adf_aer.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c > index ec7913ab00a2..907144ec7e65 100644 > --- a/drivers/crypto/intel/qat/qat_common/adf_aer.c > +++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c > @@ -281,8 +281,10 @@ int adf_init_aer(void) > return -EFAULT; > > device_sriov_wq = alloc_workqueue("qat_device_sriov_wq", 0, 0); > - if (!device_sriov_wq) > + if (!device_sriov_wq) { > + destroy_workqueue(device_reset_wq); The missing destroy_workqueue() here is intentional as the device_reset_wq is destroyed in adf_exit_aer() which is called if adf_init_aer() fails, see [1]. With this change, destroy_workqueue() is called twice. Regards,
On 2024/10/24 23:04, Cabiddu, Giovanni wrote: > On Thu, Oct 24, 2024 at 10:00:57PM +0800, Wang Hai wrote: >> The adf_init_aer() won't destroy device_reset_wq when alloc_workqueue() >> for device_sriov_wq failed. Add destroy_workqueue for device_reset_wq to >> fix this issue. >> >> Fixes: 4469f9b23468 ("crypto: qat - re-enable sriov after pf reset") >> Signed-off-by: Wang Hai <wanghai38@huawei.com> >> --- >> drivers/crypto/intel/qat/qat_common/adf_aer.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c >> index ec7913ab00a2..907144ec7e65 100644 >> --- a/drivers/crypto/intel/qat/qat_common/adf_aer.c >> +++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c >> @@ -281,8 +281,10 @@ int adf_init_aer(void) >> return -EFAULT; >> >> device_sriov_wq = alloc_workqueue("qat_device_sriov_wq", 0, 0); >> - if (!device_sriov_wq) >> + if (!device_sriov_wq) { >> + destroy_workqueue(device_reset_wq); > The missing destroy_workqueue() here is intentional as the device_reset_wq > is destroyed in adf_exit_aer() which is called if adf_init_aer() fails, > see [1]. > Hi, Giovanni. Thanks for the review. If adf_init_aer() fails, it will goto err_aer and then call adf_exit_misc_wq() instead of goto err_pf_wq and then call adf_exit_aer(). So this patch is needed. static long adf_ctl_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { ... if (adf_init_aer()) goto err_aer; ... err_pf_wq: adf_exit_aer(); // will not be called when adf_init_aer() failed err_aer: adf_exit_misc_wq(); err_misc_wq: ... } > With this change, destroy_workqueue() is called twice. > > Regards, >
On Fri, Oct 25, 2024 at 09:24:52AM +0800, Wang Hai wrote: > > > On 2024/10/24 23:04, Cabiddu, Giovanni wrote: > > On Thu, Oct 24, 2024 at 10:00:57PM +0800, Wang Hai wrote: > > > The adf_init_aer() won't destroy device_reset_wq when alloc_workqueue() > > > for device_sriov_wq failed. Add destroy_workqueue for device_reset_wq to > > > fix this issue. > > > > > > Fixes: 4469f9b23468 ("crypto: qat - re-enable sriov after pf reset") > > > Signed-off-by: Wang Hai <wanghai38@huawei.com> > > > --- > > > drivers/crypto/intel/qat/qat_common/adf_aer.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c > > > index ec7913ab00a2..907144ec7e65 100644 > > > --- a/drivers/crypto/intel/qat/qat_common/adf_aer.c > > > +++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c > > > @@ -281,8 +281,10 @@ int adf_init_aer(void) > > > return -EFAULT; > > > device_sriov_wq = alloc_workqueue("qat_device_sriov_wq", 0, 0); > > > - if (!device_sriov_wq) > > > + if (!device_sriov_wq) { > > > + destroy_workqueue(device_reset_wq); > > The missing destroy_workqueue() here is intentional as the device_reset_wq > > is destroyed in adf_exit_aer() which is called if adf_init_aer() fails, > > see [1]. > > > Hi, Giovanni. > > Thanks for the review. > > If adf_init_aer() fails, it will goto err_aer and then call > adf_exit_misc_wq() instead of goto err_pf_wq and then call > adf_exit_aer(). So this patch is needed. Sorry, I overlooked it. You are right. On the error path I would also set device_reset_wq to NULL. if (!device_sriov_wq) { destroy_workqueue(device_reset_wq); device_reset_wq = NULL; return -EFAULT; } Regards,
On 2024/10/25 16:32, Cabiddu, Giovanni wrote: > On Fri, Oct 25, 2024 at 09:24:52AM +0800, Wang Hai wrote: >> >> >> On 2024/10/24 23:04, Cabiddu, Giovanni wrote: >>> On Thu, Oct 24, 2024 at 10:00:57PM +0800, Wang Hai wrote: >>>> The adf_init_aer() won't destroy device_reset_wq when alloc_workqueue() >>>> for device_sriov_wq failed. Add destroy_workqueue for device_reset_wq to >>>> fix this issue. >>>> >>>> Fixes: 4469f9b23468 ("crypto: qat - re-enable sriov after pf reset") >>>> Signed-off-by: Wang Hai <wanghai38@huawei.com> >>>> --- >>>> drivers/crypto/intel/qat/qat_common/adf_aer.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c >>>> index ec7913ab00a2..907144ec7e65 100644 >>>> --- a/drivers/crypto/intel/qat/qat_common/adf_aer.c >>>> +++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c >>>> @@ -281,8 +281,10 @@ int adf_init_aer(void) >>>> return -EFAULT; >>>> device_sriov_wq = alloc_workqueue("qat_device_sriov_wq", 0, 0); >>>> - if (!device_sriov_wq) >>>> + if (!device_sriov_wq) { >>>> + destroy_workqueue(device_reset_wq); >>> The missing destroy_workqueue() here is intentional as the device_reset_wq >>> is destroyed in adf_exit_aer() which is called if adf_init_aer() fails, >>> see [1]. >>> >> Hi, Giovanni. >> >> Thanks for the review. >> >> If adf_init_aer() fails, it will goto err_aer and then call >> adf_exit_misc_wq() instead of goto err_pf_wq and then call >> adf_exit_aer(). So this patch is needed. > Sorry, I overlooked it. You are right. > On the error path I would also set device_reset_wq to NULL. > if (!device_sriov_wq) { > destroy_workqueue(device_reset_wq); > device_reset_wq = NULL; > return -EFAULT; > } > Thanks for the suggestion, I will send a v2 patch > Regards, >
diff --git a/drivers/crypto/intel/qat/qat_common/adf_aer.c b/drivers/crypto/intel/qat/qat_common/adf_aer.c index ec7913ab00a2..907144ec7e65 100644 --- a/drivers/crypto/intel/qat/qat_common/adf_aer.c +++ b/drivers/crypto/intel/qat/qat_common/adf_aer.c @@ -281,8 +281,10 @@ int adf_init_aer(void) return -EFAULT; device_sriov_wq = alloc_workqueue("qat_device_sriov_wq", 0, 0); - if (!device_sriov_wq) + if (!device_sriov_wq) { + destroy_workqueue(device_reset_wq); return -EFAULT; + } return 0; }
The adf_init_aer() won't destroy device_reset_wq when alloc_workqueue() for device_sriov_wq failed. Add destroy_workqueue for device_reset_wq to fix this issue. Fixes: 4469f9b23468 ("crypto: qat - re-enable sriov after pf reset") Signed-off-by: Wang Hai <wanghai38@huawei.com> --- drivers/crypto/intel/qat/qat_common/adf_aer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)