[PATCH 0/3] staging: most: fix aim-cdev issues

[PATCH 0/3] staging: most: fix aim-cdev issues

[Christian Gromm]

This patch set is needed to fix issues of the aim-cdev module.

Christian Gromm (3):
  staging: most: aim-cdev: fix reported error codes
  staging: most: aim-cdev: report error returned by alloc_chrdev_region
  staging: most: aim-cdev: relocate call to ida_init()

 drivers/staging/most/aim-cdev/cdev.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

-- 
1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] staging: most: aim-cdev: fix reported error codes

[Christian Gromm]

Currently, the aim-cdev is returning different error codes for the same
root cause. This patch is needed to get rid of the module's inconsistency
when reporting errors.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/aim-cdev/cdev.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/most/aim-cdev/cdev.c b/drivers/staging/most/aim-cdev/cdev.c
index de4f76a..6ca2440 100644
--- a/drivers/staging/most/aim-cdev/cdev.c
+++ b/drivers/staging/most/aim-cdev/cdev.c
@@ -130,7 +130,7 @@ static int aim_open(struct inode *inode, struct file *filp)
 	if (!c->dev) {
 		pr_info("WARN: Device is destroyed\n");
 		mutex_unlock(&c->io_mutex);
-		return -EBUSY;
+		return -ENODEV;
 	}
 
 	if (c->access_ref) {
@@ -201,7 +201,7 @@ static ssize_t aim_write(struct file *filp, const char __user *buf,
 	}
 
 	if (unlikely(!c->dev)) {
-		ret = -EPIPE;
+		ret = -ENODEV;
 		goto unlock;
 	}
 
@@ -256,7 +256,7 @@ aim_read(struct file *filp, char __user *buf, size_t count, loff_t *offset)
 	/* make sure we don't submit to gone devices */
 	if (unlikely(!c->dev)) {
 		mutex_unlock(&c->io_mutex);
-		return -EIO;
+		return -ENODEV;
 	}
 
 	to_copy = min_t(size_t,
@@ -366,7 +366,7 @@ static int aim_rx_completion(struct mbo *mbo)
 	spin_lock(&c->unlink);
 	if (!c->access_ref || !c->dev) {
 		spin_unlock(&c->unlink);
-		return -EFAULT;
+		return -ENODEV;
 	}
 	kfifo_in(&c->fifo, &mbo, 1);
 	spin_unlock(&c->unlink);
@@ -499,6 +499,8 @@ static struct most_aim cdev_aim = {
 
 static int __init mod_init(void)
 {
+	int err;
+
 	pr_info("init()\n");
 
 	INIT_LIST_HEAD(&channel_list);
@@ -506,16 +508,17 @@ static int __init mod_init(void)
 	ida_init(&minor_id);
 
 	if (alloc_chrdev_region(&aim_devno, 0, 50, "cdev") < 0)
-		return -EIO;
+		return -ENOMEM;
 	major = MAJOR(aim_devno);
 
 	aim_class = class_create(THIS_MODULE, "most_cdev_aim");
 	if (IS_ERR(aim_class)) {
 		pr_err("no udev support\n");
+		err = PTR_ERR(aim_class);
 		goto free_cdev;
 	}
-
-	if (most_register_aim(&cdev_aim))
+	err = most_register_aim(&cdev_aim);
+	if (err)
 		goto dest_class;
 	return 0;
 
@@ -523,7 +526,7 @@ dest_class:
 	class_destroy(aim_class);
 free_cdev:
 	unregister_chrdev_region(aim_devno, 1);
-	return -EIO;
+	return err;
 }
 
 static void __exit mod_exit(void)
-- 
1.9.1

[PATCH 2/3] staging: most: aim-cdev: report error returned by alloc_chrdev_region

[Christian Gromm]

This patch forwards the error code returned by function
alloc_chrdev_region(). It is needed to stop the module
from hiding the actual cause of failure.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/aim-cdev/cdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/most/aim-cdev/cdev.c b/drivers/staging/most/aim-cdev/cdev.c
index 6ca2440..db6f458 100644
--- a/drivers/staging/most/aim-cdev/cdev.c
+++ b/drivers/staging/most/aim-cdev/cdev.c
@@ -507,8 +507,9 @@ static int __init mod_init(void)
 	spin_lock_init(&ch_list_lock);
 	ida_init(&minor_id);
 
-	if (alloc_chrdev_region(&aim_devno, 0, 50, "cdev") < 0)
-		return -ENOMEM;
+	err = alloc_chrdev_region(&aim_devno, 0, 50, "cdev");
+	if (err < 0)
+		return err;
 	major = MAJOR(aim_devno);
 
 	aim_class = class_create(THIS_MODULE, "most_cdev_aim");
-- 
1.9.1

[PATCH 3/3] staging: most: aim-cdev: relocate call to ida_init()

[Christian Gromm]

This patch moves the initialization of the idr structure towards the end
of the module's init routine. This keeps the code compact and eliminates
the need of having to call ida_destroy() in case the function exits with
an exception.

Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/aim-cdev/cdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/most/aim-cdev/cdev.c b/drivers/staging/most/aim-cdev/cdev.c
index db6f458..3864009 100644
--- a/drivers/staging/most/aim-cdev/cdev.c
+++ b/drivers/staging/most/aim-cdev/cdev.c
@@ -505,7 +505,6 @@ static int __init mod_init(void)
 
 	INIT_LIST_HEAD(&channel_list);
 	spin_lock_init(&ch_list_lock);
-	ida_init(&minor_id);
 
 	err = alloc_chrdev_region(&aim_devno, 0, 50, "cdev");
 	if (err < 0)
@@ -521,6 +520,7 @@ static int __init mod_init(void)
 	err = most_register_aim(&cdev_aim);
 	if (err)
 		goto dest_class;
+	ida_init(&minor_id);
 	return 0;
 
 dest_class:
-- 
1.9.1

Re: [PATCH 3/3] staging: most: aim-cdev: relocate call to ida_init()

[Greg KH]

On Fri, Aug 19, 2016 at 01:09:35PM +0200, Christian Gromm wrote:
> This patch moves the initialization of the idr structure towards the end
> of the module's init routine. This keeps the code compact and eliminates
> the need of having to call ida_destroy() in case the function exits with
> an exception.
> 
> Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> ---
>  drivers/staging/most/aim-cdev/cdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/most/aim-cdev/cdev.c b/drivers/staging/most/aim-cdev/cdev.c
> index db6f458..3864009 100644
> --- a/drivers/staging/most/aim-cdev/cdev.c
> +++ b/drivers/staging/most/aim-cdev/cdev.c
> @@ -505,7 +505,6 @@ static int __init mod_init(void)
>  
>  	INIT_LIST_HEAD(&channel_list);
>  	spin_lock_init(&ch_list_lock);
> -	ida_init(&minor_id);
>  
>  	err = alloc_chrdev_region(&aim_devno, 0, 50, "cdev");
>  	if (err < 0)
> @@ -521,6 +520,7 @@ static int __init mod_init(void)
>  	err = most_register_aim(&cdev_aim);
>  	if (err)
>  		goto dest_class;
> +	ida_init(&minor_id);

But can't the minor_id be used before this call?  most_register_aim()
could call into a driver and have a cdev be wanted, right?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: most: aim-cdev: relocate call to ida_init()

[Christian Gromm]

On Sun, 21 Aug 2016 16:50:20 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Aug 19, 2016 at 01:09:35PM +0200, Christian Gromm wrote:
> > This patch moves the initialization of the idr structure towards the end
> > of the module's init routine. This keeps the code compact and eliminates
> > the need of having to call ida_destroy() in case the function exits with
> > an exception.
> > 
> > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > ---
> >  drivers/staging/most/aim-cdev/cdev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/most/aim-cdev/cdev.c b/drivers/staging/most/aim-cdev/cdev.c
> > index db6f458..3864009 100644
> > --- a/drivers/staging/most/aim-cdev/cdev.c
> > +++ b/drivers/staging/most/aim-cdev/cdev.c
> > @@ -505,7 +505,6 @@ static int __init mod_init(void)
> >  
> >  	INIT_LIST_HEAD(&channel_list);
> >  	spin_lock_init(&ch_list_lock);
> > -	ida_init(&minor_id);
> >  
> >  	err = alloc_chrdev_region(&aim_devno, 0, 50, "cdev");
> >  	if (err < 0)
> > @@ -521,6 +520,7 @@ static int __init mod_init(void)
> >  	err = most_register_aim(&cdev_aim);
> >  	if (err)
> >  		goto dest_class;
> > +	ida_init(&minor_id);
> 
> But can't the minor_id be used before this call?  most_register_aim()
> could call into a driver and have a cdev be wanted, right?

No. After calling most_register_aim() the device appears in sysfs. Then
the user needs to establish a link to a certain channel of the hardware
by writing to the sysfs attribute add_link. And this is when minor_id is
being used to create a new device that will show up in /dev.

See store_add_link() function of the core module.


regards,
Chris

> 
> thanks,
> 
> greg k-h

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: most: aim-cdev: relocate call to ida_init()

[Greg KH]

On Mon, Aug 22, 2016 at 10:58:07AM +0200, Christian Gromm wrote:
> On Sun, 21 Aug 2016 16:50:20 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Aug 19, 2016 at 01:09:35PM +0200, Christian Gromm wrote:
> > > This patch moves the initialization of the idr structure towards the end
> > > of the module's init routine. This keeps the code compact and eliminates
> > > the need of having to call ida_destroy() in case the function exits with
> > > an exception.
> > > 
> > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > ---
> > >  drivers/staging/most/aim-cdev/cdev.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/most/aim-cdev/cdev.c b/drivers/staging/most/aim-cdev/cdev.c
> > > index db6f458..3864009 100644
> > > --- a/drivers/staging/most/aim-cdev/cdev.c
> > > +++ b/drivers/staging/most/aim-cdev/cdev.c
> > > @@ -505,7 +505,6 @@ static int __init mod_init(void)
> > >  
> > >  	INIT_LIST_HEAD(&channel_list);
> > >  	spin_lock_init(&ch_list_lock);
> > > -	ida_init(&minor_id);
> > >  
> > >  	err = alloc_chrdev_region(&aim_devno, 0, 50, "cdev");
> > >  	if (err < 0)
> > > @@ -521,6 +520,7 @@ static int __init mod_init(void)
> > >  	err = most_register_aim(&cdev_aim);
> > >  	if (err)
> > >  		goto dest_class;
> > > +	ida_init(&minor_id);
> > 
> > But can't the minor_id be used before this call?  most_register_aim()
> > could call into a driver and have a cdev be wanted, right?
> 
> No. After calling most_register_aim() the device appears in sysfs. Then
> the user needs to establish a link to a certain channel of the hardware
> by writing to the sysfs attribute add_link. And this is when minor_id is
> being used to create a new device that will show up in /dev.
> 
> See store_add_link() function of the core module.

Ok, I'll trust you :)

Can you resend this, it's gone from my queue now...

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel