| Message ID | 8b1a6d05036d5d9527241b2345482b369331ce5c.1643531799.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | surface: surface3-wmi: Simplify resource management | expand |
On Sun, Jan 30, 2022 at 09:36:54AM +0100, Christophe JAILLET wrote: > 's3_wmi.input' is a managed resource, so there should be no need to free it > explicitly. > > Moreover, 's3_wmi' is a global variable. 's3_wmi.input' should be NULL > when this error handling path is executed, because it has not been > assigned yet. > > All this is puzzling. So simplify it and remove a few lines of code to have > it be more straightforward. Seems correct cleanup / fix to me Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Fixes: 3dda3b3798f9 ("platform/x86: Add custom surface3 platform device for controlling LID") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only > --- > drivers/platform/surface/surface3-wmi.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c > index 09ac9cfc40d8..b9a4b2d81f4b 100644 > --- a/drivers/platform/surface/surface3-wmi.c > +++ b/drivers/platform/surface/surface3-wmi.c > @@ -190,14 +190,11 @@ static int s3_wmi_create_and_register_input(struct platform_device *pdev) > > error = input_register_device(input); > if (error) > - goto out_err; > + return error; > > s3_wmi.input = input; > > return 0; > - out_err: > - input_free_device(s3_wmi.input); > - return error; > } > > static int __init s3_wmi_probe(struct platform_device *pdev) > -- > 2.32.0 >
On 1/30/22 09:36, Christophe JAILLET wrote: > 's3_wmi.input' is a managed resource, so there should be no need to free it > explicitly. > > Moreover, 's3_wmi' is a global variable. 's3_wmi.input' should be NULL > when this error handling path is executed, because it has not been > assigned yet. > > All this is puzzling. So simplify it and remove a few lines of code to have > it be more straightforward. > > Fixes: 3dda3b3798f9 ("platform/x86: Add custom surface3 platform device for controlling LID") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Right, that input_free_device(s3_wmi.input) seems wrong. Patch looks good to me. Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> > --- > Compile tested only > --- > drivers/platform/surface/surface3-wmi.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c > index 09ac9cfc40d8..b9a4b2d81f4b 100644 > --- a/drivers/platform/surface/surface3-wmi.c > +++ b/drivers/platform/surface/surface3-wmi.c > @@ -190,14 +190,11 @@ static int s3_wmi_create_and_register_input(struct platform_device *pdev) > > error = input_register_device(input); > if (error) > - goto out_err; > + return error; > > s3_wmi.input = input; > > return 0; > - out_err: > - input_free_device(s3_wmi.input); > - return error; > } > > static int __init s3_wmi_probe(struct platform_device *pdev)
Hi, On 1/30/22 09:36, Christophe JAILLET wrote: > 's3_wmi.input' is a managed resource, so there should be no need to free it > explicitly. > > Moreover, 's3_wmi' is a global variable. 's3_wmi.input' should be NULL > when this error handling path is executed, because it has not been > assigned yet. > > All this is puzzling. So simplify it and remove a few lines of code to have > it be more straightforward. > > Fixes: 3dda3b3798f9 ("platform/x86: Add custom surface3 platform device for controlling LID") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > Compile tested only > --- > drivers/platform/surface/surface3-wmi.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c > index 09ac9cfc40d8..b9a4b2d81f4b 100644 > --- a/drivers/platform/surface/surface3-wmi.c > +++ b/drivers/platform/surface/surface3-wmi.c > @@ -190,14 +190,11 @@ static int s3_wmi_create_and_register_input(struct platform_device *pdev) > > error = input_register_device(input); > if (error) > - goto out_err; > + return error; > > s3_wmi.input = input; > > return 0; > - out_err: > - input_free_device(s3_wmi.input); > - return error; > } > > static int __init s3_wmi_probe(struct platform_device *pdev) >
diff --git a/drivers/platform/surface/surface3-wmi.c b/drivers/platform/surface/surface3-wmi.c index 09ac9cfc40d8..b9a4b2d81f4b 100644 --- a/drivers/platform/surface/surface3-wmi.c +++ b/drivers/platform/surface/surface3-wmi.c @@ -190,14 +190,11 @@ static int s3_wmi_create_and_register_input(struct platform_device *pdev) error = input_register_device(input); if (error) - goto out_err; + return error; s3_wmi.input = input; return 0; - out_err: - input_free_device(s3_wmi.input); - return error; } static int __init s3_wmi_probe(struct platform_device *pdev)
's3_wmi.input' is a managed resource, so there should be no need to free it explicitly. Moreover, 's3_wmi' is a global variable. 's3_wmi.input' should be NULL when this error handling path is executed, because it has not been assigned yet. All this is puzzling. So simplify it and remove a few lines of code to have it be more straightforward. Fixes: 3dda3b3798f9 ("platform/x86: Add custom surface3 platform device for controlling LID") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only --- drivers/platform/surface/surface3-wmi.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)