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@xxxxxxxxx> )=-
> -=(                          Web: http://www.mev.co.uk/  )=-