[PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Cheah Kok Cheong]

Change to unsigned to allow removal of negative value check in
init section. Use smaller data type since the max possible
value currently is 48.

Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com>
---
 drivers/staging/comedi/comedi_fops.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 57e8599..354d264 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -76,8 +76,8 @@ struct comedi_file {
 #define COMEDI_NUM_SUBDEVICE_MINORS	\
 	(COMEDI_NUM_MINORS - COMEDI_NUM_BOARD_MINORS)
 
-static int comedi_num_legacy_minors;
-module_param(comedi_num_legacy_minors, int, 0444);
+static unsigned short comedi_num_legacy_minors;
+module_param(comedi_num_legacy_minors, ushort, 0444);
 MODULE_PARM_DESC(comedi_num_legacy_minors,
 		 "number of comedi minor devices to reserve for non-auto-configured devices (default 0)"
 		);
@@ -2857,8 +2857,7 @@ static int __init comedi_init(void)
 
 	pr_info("version " COMEDI_RELEASE " - http://www.comedi.org\n");
 
-	if (comedi_num_legacy_minors < 0 ||
-	    comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
+	if (comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
 		pr_err("invalid value for module parameter \"comedi_num_legacy_minors\".  Valid values are 0 through %i.\n",
 		       COMEDI_NUM_BOARD_MINORS);
 		return -EINVAL;
-- 
2.7.4

[PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Cheah Kok Cheong]

If comedi module is loaded with the following max allowed parameter
[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
device will fail.

In this case a default to auto-configuration comedi_test module failed
to load with the following messages.

comedi_test comedi_testd: ran out of minor numbers for board device files
comedi_test comedi_testd: driver 'comedi_test' could not create device.
comedi_test: unable to auto-configure device

This is due to changes in commit 38b9722a4414
("staging: comedi: avoid releasing legacy minors automatically") which
will not allocate a minor number when comedi_num_legacy_minors equals
COMEDI_NUM_BOARD_MINORS. COMEDI_NUM_BOARD_MINORS is defined to be
0x30 which is 48.

This goes for a simple fix which limit comedi_num_legacy_minors to 47
instead of tinkering with comedi_alloc_board_minor() and
comedi_release_hardware_device().

Fix: commit 38b9722a4414 ("staging: comedi: avoid releasing legacy minors
automatically")

Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com>
---
 drivers/staging/comedi/comedi_fops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 354d264..339854f 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2857,9 +2857,9 @@ static int __init comedi_init(void)
 
 	pr_info("version " COMEDI_RELEASE " - http://www.comedi.org\n");
 
-	if (comedi_num_legacy_minors > COMEDI_NUM_BOARD_MINORS) {
+	if (comedi_num_legacy_minors >= COMEDI_NUM_BOARD_MINORS) {
 		pr_err("invalid value for module parameter \"comedi_num_legacy_minors\".  Valid values are 0 through %i.\n",
-		       COMEDI_NUM_BOARD_MINORS);
+		       COMEDI_NUM_BOARD_MINORS - 1);
 		return -EINVAL;
 	}
 
-- 
2.7.4

Re: [PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Dan Carpenter]

On Sun, Mar 05, 2017 at 03:22:33AM +0800, Cheah Kok Cheong wrote:
> If comedi module is loaded with the following max allowed parameter
> [comedi_num_legacy_minors=48], subsequent loading of an auto-configured
> device will fail.

Don't set comedi_num_legacy_minors=48, then?

This doesn't seem like the right fix at all.  Why only allow one auto
configured board?  Why not 5 or 10?

regards,
dan carpenter

Re: [PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Cheah Kok Cheong]

Dear Dan,
  Thanks for reviewing this patch.

On Wed, Mar 08, 2017 at 08:54:47AM +0300, Dan Carpenter wrote:
> On Sun, Mar 05, 2017 at 03:22:33AM +0800, Cheah Kok Cheong wrote:
> > If comedi module is loaded with the following max allowed parameter
> > [comedi_num_legacy_minors=48], subsequent loading of an auto-configured
> > device will fail.
> 
> Don't set comedi_num_legacy_minors=48, then?
> 
> This doesn't seem like the right fix at all.  Why only allow one auto
> configured board?  Why not 5 or 10?
> 

Let me explain, the original intended behaviour is to allow user to
reserve up to 48 minor numbers for legacy devices.

Therefore [sudo modprobe comedi comedi_num_legacy_minors=3] 
will allocate minor number 0, 1, 2 for legacy devices.
Subsequent loading of an auto-configured device will use minor number 3.
And the next one number 4 so on and so forth.

Now for the corner case of [comedi_num_legacy_minors=48] which
is supposed to reserve minor number 0 till 47 for legacy devices,
and is supposed to allocate number 48 and so on for auto-configured
devices, does not allocate number 48 anymore after commit
38b9722a4414.

This is due to the changes in comedi_alloc_board_minor().

As to why I chose to limit [comedi_num_legacy_minors=47], is given
in the commit log.

This will allow user to allocate 0 till 46 for legacy devices and
subsequent auto-configured devices will start from 47 and so forth.

I don't think anybody will miss one less number for legacy devices
otherwise there'll be complains earlier on.

Thanks.

Brgds,
CheahKC 

> regards,
> dan carpenter
> 

Re: [PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Ian Abbott]

On 08/03/17 10:08, Cheah Kok Cheong wrote:
> Dear Dan,
>   Thanks for reviewing this patch.
>
> On Wed, Mar 08, 2017 at 08:54:47AM +0300, Dan Carpenter wrote:
>> On Sun, Mar 05, 2017 at 03:22:33AM +0800, Cheah Kok Cheong wrote:
>>> If comedi module is loaded with the following max allowed parameter
>>> [comedi_num_legacy_minors=48], subsequent loading of an auto-configured
>>> device will fail.
>>
>> Don't set comedi_num_legacy_minors=48, then?
>>
>> This doesn't seem like the right fix at all.  Why only allow one auto
>> configured board?  Why not 5 or 10?
>>
>
> Let me explain, the original intended behaviour is to allow user to
> reserve up to 48 minor numbers for legacy devices.
>
> Therefore [sudo modprobe comedi comedi_num_legacy_minors=3]
> will allocate minor number 0, 1, 2 for legacy devices.
> Subsequent loading of an auto-configured device will use minor number 3.
> And the next one number 4 so on and so forth.
>
> Now for the corner case of [comedi_num_legacy_minors=48] which
> is supposed to reserve minor number 0 till 47 for legacy devices,
> and is supposed to allocate number 48 and so on for auto-configured
> devices, does not allocate number 48 anymore after commit
> 38b9722a4414.

Both legacy and auto-configured devices will have minor numbers less 
than 48, which is the total number of devices currently supported by 
Comedi.  Minor numbers 48 and above have been reserved for dynamic 
allocation to those Comedi subdevices that support asynchronous commands.

> This is due to the changes in comedi_alloc_board_minor().
>
> As to why I chose to limit [comedi_num_legacy_minors=47], is given
> in the commit log.
>
> This will allow user to allocate 0 till 46 for legacy devices and
> subsequent auto-configured devices will start from 47 and so forth.
>
> I don't think anybody will miss one less number for legacy devices
> otherwise there'll be complains earlier on.

As Dan implies above, if you want auto-configured devices to work, set 
comedi_num_legacy_minors to a value less than 48.  Further limiting 
comedi_num_legacy_minors to 47 seems a bit arbitrary.

The comedi_test module mentioned in your commit can still be used when 
comedi_num_legacy_minors=48 by configuring a "legacy" comedi_test 
device.  Most of the other Comedi drivers support "legacy" devices 
exclusive-or auto-configured devices.

> Thanks.
>
> Brgds,
> CheahKC
>
>> regards,
>> dan carpenter
>>


-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

Re: [PATCH 2/2] Staging: comedi: comedi_fops: Fix "out of minor numbers for board device files"

[Cheah Kok Cheong]

Dear Ian,
  Thanks for taking the trouble to reply.

On Wed, Mar 08, 2017 at 11:13:49AM +0000, Ian Abbott wrote:
> On 08/03/17 10:08, Cheah Kok Cheong wrote:
> >Dear Dan,
> >  Thanks for reviewing this patch.
> >
> >On Wed, Mar 08, 2017 at 08:54:47AM +0300, Dan Carpenter wrote:
> >>On Sun, Mar 05, 2017 at 03:22:33AM +0800, Cheah Kok Cheong wrote:
> >>>If comedi module is loaded with the following max allowed parameter
> >>>[comedi_num_legacy_minors=48], subsequent loading of an auto-configured
> >>>device will fail.
> >>
> >>Don't set comedi_num_legacy_minors=48, then?
> >>
> >>This doesn't seem like the right fix at all.  Why only allow one auto
> >>configured board?  Why not 5 or 10?
> >>
> >
> >Let me explain, the original intended behaviour is to allow user to
> >reserve up to 48 minor numbers for legacy devices.
> >
> >Therefore [sudo modprobe comedi comedi_num_legacy_minors=3]
> >will allocate minor number 0, 1, 2 for legacy devices.
> >Subsequent loading of an auto-configured device will use minor number 3.
> >And the next one number 4 so on and so forth.
> >
> >Now for the corner case of [comedi_num_legacy_minors=48] which
> >is supposed to reserve minor number 0 till 47 for legacy devices,
> >and is supposed to allocate number 48 and so on for auto-configured
> >devices, does not allocate number 48 anymore after commit
> >38b9722a4414.
> 
> Both legacy and auto-configured devices will have minor numbers less than
> 48, which is the total number of devices currently supported by Comedi.
> Minor numbers 48 and above have been reserved for dynamic allocation to
> those Comedi subdevices that support asynchronous commands.
> 

Thanks for the explanation.

> >This is due to the changes in comedi_alloc_board_minor().
> >
> >As to why I chose to limit [comedi_num_legacy_minors=47], is given
> >in the commit log.
> >
> >This will allow user to allocate 0 till 46 for legacy devices and
> >subsequent auto-configured devices will start from 47 and so forth.
> >
> >I don't think anybody will miss one less number for legacy devices
> >otherwise there'll be complains earlier on.
> 
> As Dan implies above, if you want auto-configured devices to work, set
> comedi_num_legacy_minors to a value less than 48.  Further limiting
> comedi_num_legacy_minors to 47 seems a bit arbitrary.
> 
> The comedi_test module mentioned in your commit can still be used when
> comedi_num_legacy_minors=48 by configuring a "legacy" comedi_test device.
> Most of the other Comedi drivers support "legacy" devices exclusive-or
> auto-configured devices.
> 

Ah ok now I understand what Dan is trying to say actually. Thanks for the
detailed clarification. Sorry for the inconvenience caused.

Thanks.

Brgds,
CheahKC

> >Thanks.
> >
> >Brgds,
> >CheahKC
> >
> >>regards,
> >>dan carpenter
> >>
> 
> 
> -- 
> -=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
> -=(                          Web: http://www.mev.co.uk/  )=-

Re: [PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Greg KH]

On Sun, Mar 05, 2017 at 03:22:32AM +0800, Cheah Kok Cheong wrote:
> Change to unsigned to allow removal of negative value check in
> init section.

Why?

> Use smaller data type since the max possible value currently is 48.

Does it matter?

thanks,

greg k-h

Re: [PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Cheah Kok Cheong]

Dear Greg,
  Thanks for taking the time to review.

On Tue, Mar 07, 2017 at 08:01:38PM +0100, Greg KH wrote:
> On Sun, Mar 05, 2017 at 03:22:32AM +0800, Cheah Kok Cheong wrote:
> > Change to unsigned to allow removal of negative value check in
> > init section.
> 
> Why?
> 

User can input a -ve number as parameter for module loading.
This will be caught by the mentioned check and cause loading to fail.
I think the original intention here is to inform user via kernel log
the acceptable input range.

Now if a user doesn't know how to access the log, it's of no help.

If a user does know how to access the log, probably also know how
to use modinfo or know that reserving a -ve number of minors is
not acceptable.

IMHO, this check is pointless and best enforced in module_param.
 
> > Use smaller data type since the max possible value currently is 48.
> 
> Does it matter?
> 

You are right it doesn't matter and not worth the effort doing unless
concurrent work is being done in this area.
Which is what patch 2/2 is trying to do here.
Please note that I've sent V2. Patch 1/2 unchanged and 2/2 with amended
commit log.

Thanks,
Brgds,
CheahKC

> thanks,
> 
> greg k-h

Re: [PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Greg KH]

On Wed, Mar 08, 2017 at 05:38:12PM +0800, Cheah Kok Cheong wrote:
> Dear Greg,
>   Thanks for taking the time to review.
> 
> On Tue, Mar 07, 2017 at 08:01:38PM +0100, Greg KH wrote:
> > On Sun, Mar 05, 2017 at 03:22:32AM +0800, Cheah Kok Cheong wrote:
> > > Change to unsigned to allow removal of negative value check in
> > > init section.
> > 
> > Why?
> > 
> 
> User can input a -ve number as parameter for module loading.

Then they are foolish to do so :)

> This will be caught by the mentioned check and cause loading to fail.
> I think the original intention here is to inform user via kernel log
> the acceptable input range.

Either is fine.

> Now if a user doesn't know how to access the log, it's of no help.

They know how to set a module parameter as root but do not know of the
kernel log?  That's trying a bit too hard :)

> If a user does know how to access the log, probably also know how
> to use modinfo or know that reserving a -ve number of minors is
> not acceptable.
> 
> IMHO, this check is pointless and best enforced in module_param.

Ok, but it's really a minor, or no, real issue at all here.

thanks,

greg k-h

Re: [PATCH 1/2] Staging: comedi: comedi_fops: Change comedi_num_legacy_minors type

[Cheah Kok Cheong]

On Wed, Mar 08, 2017 at 10:45:26AM +0100, Greg KH wrote:
> On Wed, Mar 08, 2017 at 05:38:12PM +0800, Cheah Kok Cheong wrote:
> > Dear Greg,
> >   Thanks for taking the time to review.
> > 
> > On Tue, Mar 07, 2017 at 08:01:38PM +0100, Greg KH wrote:
> > > On Sun, Mar 05, 2017 at 03:22:32AM +0800, Cheah Kok Cheong wrote:
> > > > Change to unsigned to allow removal of negative value check in
> > > > init section.
> > > 
> > > Why?
> > > 
> > 
> > User can input a -ve number as parameter for module loading.
> 
> Then they are foolish to do so :)
> 
> > This will be caught by the mentioned check and cause loading to fail.
> > I think the original intention here is to inform user via kernel log
> > the acceptable input range.
> 
> Either is fine.
> 
> > Now if a user doesn't know how to access the log, it's of no help.
> 
> They know how to set a module parameter as root but do not know of the
> kernel log?  That's trying a bit too hard :)
> 
> > If a user does know how to access the log, probably also know how
> > to use modinfo or know that reserving a -ve number of minors is
> > not acceptable.
> > 
> > IMHO, this check is pointless and best enforced in module_param.
> 
> Ok, but it's really a minor, or no, real issue at all here.
> 

I agree with you and that's why I mentioned it's not worth doing
unless there's concurrent work in this area.

Thanks.
Brgds,
CheahKC

> thanks,
> 
> greg k-h