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

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

[nizamhaider786]

From: Nijam Haider <nizamhaider786@gmail.com>

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

Signed-off-by: Nijam Haider <nizamhaider786@gmail.com>
---
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

[PATCH v3 2/2] char: pcmcia: scr24x_cs: Fix redundant fops

[nizamhaider786]

From: Nijam Haider <nizamhaider786@gmail.com>

Removed redundant fops assignment, which was already done in cdev_init()

Signed-off-by: Nijam Haider <nizamhaider786@gmail.com>
---
V2 -> V3: Added changelog and squishing the subject and description
V1 -> V2: Spliting the patch into two
---
 drivers/char/pcmcia/scr24x_cs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/char/pcmcia/scr24x_cs.c b/drivers/char/pcmcia/scr24x_cs.c
index b48e79356611..c41e9bf3a3f1 100644
--- a/drivers/char/pcmcia/scr24x_cs.c
+++ b/drivers/char/pcmcia/scr24x_cs.c
@@ -266,7 +266,6 @@ static int scr24x_probe(struct pcmcia_device *link)
 
 	cdev_init(&dev->c_dev, &scr24x_fops);
 	dev->c_dev.owner = THIS_MODULE;
-	dev->c_dev.ops = &scr24x_fops;
 	ret = cdev_add(&dev->c_dev, MKDEV(MAJOR(scr24x_devt), dev->devno), 1);
 	if (ret < 0)
 		goto err;
-- 
2.7.4

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

[Greg KH]

On Tue, May 25, 2021 at 03:22:01AM +0530, nizamhaider786@gmail.com wrote:
> From: Nijam Haider <nizamhaider786@gmail.com>
> 
> Ignored error in device_create() and pcmcia_enable_device()
> this patch implements proper error handling.
> 
> Signed-off-by: Nijam Haider <nizamhaider786@gmail.com>
> ---
> 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