2021-05-25 00:00:08

by Nizam Haider

Subject: [PATCH v3 1/2] 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]>
---
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 | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/char/pcmcia/scr24x_cs.c b/drivers/char/pcmcia/scr24x_cs.c
index 47feb39af34c..b48e79356611 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)
@@ -272,12 +273,20 @@ static int scr24x_probe(struct pcmcia_device *link)

ret = pcmcia_enable_device(link);
if (ret < 0) {
+ cdev_del(&dev->c_dev);
pcmcia_disable_device(link);
goto err;
}

- 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);
+ cdev_del(&dev->c_dev);
+ pcmcia_disable_device(link);
+ goto err;
+ }

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

2021-05-27 20:49:26

by Greg Kroah-Hartman

Subject: Re: [PATCH v3 1/2] char: pcmcia: scr24x_cs: Fix failure handling of device_create()

On Tue, May 25, 2021 at 03:22:01AM +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]>
> ---
> 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 | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/pcmcia/scr24x_cs.c b/drivers/char/pcmcia/scr24x_cs.c
> index 47feb39af34c..b48e79356611 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)
> @@ -272,12 +273,20 @@ static int scr24x_probe(struct pcmcia_device *link)
>
> ret = pcmcia_enable_device(link);
> if (ret < 0) {
> + cdev_del(&dev->c_dev);
> pcmcia_disable_device(link);
> goto err;
> }
>
> - 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);
> + cdev_del(&dev->c_dev);
> + pcmcia_disable_device(link);
> + goto err;
> + }

The "better" way to do this is to have more err_: labels that do the
unwinding for you, so that you do not have to duplicate the same logic
in multiple places, like you are doing here.

Can you change this patch to do that instead? Should be shorter overall
than this one and easier to maintain over time.

thanks,

greg k-h