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

2017-03-08 Thread Cheah Kok Cheong 
Dear Ian,
  Thanks for taking the trouble to reply.

On Wed, Mar 08, 2017 at 11:13:49AM +, 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:  )=-
> -=(  Web: http://www.mev.co.uk/  )=-

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

2017-03-08 Thread Cheah Kok Cheong 
Dear Ian,
  Thanks for taking the trouble to reply.

On Wed, Mar 08, 2017 at 11:13:49AM +, 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:  )=-
> -=(  Web: http://www.mev.co.uk/  )=-

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

2017-03-08 Thread 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:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

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

2017-03-08 Thread 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:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

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

2017-03-08 Thread 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"

2017-03-08 Thread 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"

2017-03-07 Thread 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"

2017-03-07 Thread 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

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

2017-03-04 Thread 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 
---
 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

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

2017-03-04 Thread 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 
---
 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