2021-05-27 19:39:08

by Nizam Haider

Subject: [PATCH V4] char: pcmcia: scr24x_cs: Fix failure handling of device_create()

From: Nijam Haider <[email protected]>

Ignored error in device_create() and pcmcia_enable_device()
this patch implements proper error handling.

Signed-off-by: Nijam Haider <[email protected]>
---
V3 -> V4: Added label and moved the cleanup code
V2 -> V3: Added description, Changelog and removed whitespace error
V1 -> V2: Split the patch into two parts and addressed review comments
---
drivers/char/pcmcia/scr24x_cs.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/char/pcmcia/scr24x_cs.c b/drivers/char/pcmcia/scr24x_cs.c
index 47feb39af34c..ba84b4dd13d3 100644
--- a/drivers/char/pcmcia/scr24x_cs.c
+++ b/drivers/char/pcmcia/scr24x_cs.c
@@ -233,6 +233,7 @@ static int scr24x_probe(struct pcmcia_device *link)
{
struct scr24x_dev *dev;
int ret;
+ struct device *dev_ret;

dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev)
@@ -271,22 +272,28 @@ static int scr24x_probe(struct pcmcia_device *link)
goto err;

ret = pcmcia_enable_device(link);
- if (ret < 0) {
- pcmcia_disable_device(link);
- goto err;
- }
+ if (ret < 0)
+ goto err_device;

- device_create(scr24x_class, NULL, MKDEV(MAJOR(scr24x_devt), dev->devno),
+ dev_ret = device_create(scr24x_class, NULL, MKDEV(MAJOR(scr24x_devt), dev->devno),
NULL, "scr24x%d", dev->devno);
+ if (IS_ERR(dev_ret)) {
+ dev_err(&link->dev, "device_create failed for %d\n",
+ dev->devno);
+ goto err_device;
+ }

dev_info(&link->dev, "SCR24x Chip Card Interface\n");
return 0;

+err_device:
+ pcmcia_disable_device(link);
+ cdev_del(&dev->c_dev);
err:
if (dev->devno < SCR24X_DEVS)
clear_bit(dev->devno, scr24x_minors);
kfree (dev);
- return ret;
+ return -ENODEV;
}

static void scr24x_remove(struct pcmcia_device *link)
--
2.17.1

2021-05-28 07:46:48

by Lubomir Rintel

Subject: Re: [PATCH V4] char: pcmcia: scr24x_cs: Fix failure handling of device_create()

Hi,

much better now, thank you.

Acked-by: Lubomir Rintel <[email protected]>

There's one small remark below.

On Fri, May 28, 2021 at 01:04:29AM +0530, [email protected] wrote:
> From: Nijam Haider <[email protected]>
>
> Ignored error in device_create() and pcmcia_enable_device()
> this patch implements proper error handling.
>
> Signed-off-by: Nijam Haider <[email protected]>
> ---
> V3 -> V4: Added label and moved the cleanup code
> V2 -> V3: Added description, Changelog and removed whitespace error
> V1 -> V2: Split the patch into two parts and addressed review comments
> ---
> drivers/char/pcmcia/scr24x_cs.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/pcmcia/scr24x_cs.c b/drivers/char/pcmcia/scr24x_cs.c
> index 47feb39af34c..ba84b4dd13d3 100644
> --- a/drivers/char/pcmcia/scr24x_cs.c
> +++ b/drivers/char/pcmcia/scr24x_cs.c
> @@ -233,6 +233,7 @@ static int scr24x_probe(struct pcmcia_device *link)
> {
> struct scr24x_dev *dev;
> int ret;
> + struct device *dev_ret;
>
> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> if (!dev)
> @@ -271,22 +272,28 @@ static int scr24x_probe(struct pcmcia_device *link)
> goto err;
>
> ret = pcmcia_enable_device(link);
> - if (ret < 0) {
> - pcmcia_disable_device(link);
> - goto err;
> - }
> + if (ret < 0)
> + goto err_device;
>
> - device_create(scr24x_class, NULL, MKDEV(MAJOR(scr24x_devt), dev->devno),
> + dev_ret = device_create(scr24x_class, NULL, MKDEV(MAJOR(scr24x_devt), dev->devno),
> NULL, "scr24x%d", dev->devno);
> + if (IS_ERR(dev_ret)) {
> + dev_err(&link->dev, "device_create failed for %d\n",
> + dev->devno);
> + goto err_device;
> + }
>
> dev_info(&link->dev, "SCR24x Chip Card Interface\n");
> return 0;
>
> +err_device:
> + pcmcia_disable_device(link);
> + cdev_del(&dev->c_dev);
> err:
> if (dev->devno < SCR24X_DEVS)
> clear_bit(dev->devno, scr24x_minors);
> kfree (dev);
> - return ret;
> + return -ENODEV;

Why -ENODEV? Couldn't we propagate the original error with something
like "ret = PTR_ERR(dev_ret);" just before the "goto err_device"?

Thank you,
Lubo

> }
>
> static void scr24x_remove(struct pcmcia_device *link)
> --
> 2.17.1
>