Re: [PATCH v2 7/7] habanalabs: create two char devices per ASIC

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jul 30, 2019 at 12:47:24PM +0300, Oded Gabbay wrote:
> This patch changes the driver to create two char devices for each ASIC
> it discovers. This is done to allow system/monitoring applications to
> query the device for stats, information, idle state and more, while also
> allowing the deep-learning application to send work to the ASIC.
> 
> One char device is the original device, hlX. IOCTL calls through this
> device file can perform any task on the device (compute, memory, queries).
> The open function for this device will fail if it was called before but
> the file-descriptor it created was not completely released yet (the
> release callback function is not called from the kernel until all
> instances of that FD are closed). The driver needs to keep this behavior
> to support backward compatibility with existing userspace, which count
> that the open will fail if the device is "occupied".
> 
> The second char device is called "hl_controlDx", where x is the minor
> number of the original char device + 64 (HL_CONTROL_MINOR). Applications
> that open this device can only call the INFO IOCTL. There is no limitation
> on the number of applications opening this device.
> 
> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>

This reminds me of the old tty "control" devices we finally got rid of
many years ago.  The more things change... :)

Anyway, looks sane to me.  If you are ok with this userspace api, no
objection from me.

Only one comment below:

> --- a/drivers/misc/habanalabs/habanalabs.h
> +++ b/drivers/misc/habanalabs/habanalabs.h
> @@ -51,6 +51,8 @@
>  /* MMU */
>  #define MMU_HASH_TABLE_BITS		7 /* 1 << 7 buckets */
>  
> +#define HL_CONTROL_MINOR		64

Don't try to segment your "dev" and "control" device nodes by minor
number range, as you will run into problems once you have a system with
more than 64 of these in the box at once.

Just use the whole range, and do:
	dev = minor N
	control = minor N+1
and so on.  That makes your control device node an "odd" minor and your
"real" device an "even".

Given that all existing systems should be using devtmpfs, you should not
have any problem with changing the minor number of them at the next
reboot, right?

thanks,

greg k-h