Re: [PATCH 03/25] media: dvbdev: convert DVB device types into an enum

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Thu, 21 Sep 2017 09:06:54 -0400
Michael Ira Krufky <mkrufky@linuxtv.org> escreveu:

> On Wed, Sep 20, 2017 at 3:11 PM, Mauro Carvalho Chehab
> <mchehab@s-opensource.com> wrote:

> > +enum dvb_device_type {
> > +       DVB_DEVICE_SEC,
> > +       DVB_DEVICE_FRONTEND,
> > +       DVB_DEVICE_DEMUX,
> > +       DVB_DEVICE_DVR,
> > +       DVB_DEVICE_CA,
> > +       DVB_DEVICE_NET,
> > +
> > +       DVB_DEVICE_VIDEO,
> > +       DVB_DEVICE_AUDIO,
> > +       DVB_DEVICE_OSD,
> > +};  
> 
> maybe instead:
> ```
> enum dvb_device_type {
>  DVB_DEVICE_SEC      = 0,
>  DVB_DEVICE_FRONTEND = 1,
>  DVB_DEVICE_DEMUX    = 2,
>  DVB_DEVICE_DVR      = 3,
>  DVB_DEVICE_CA       = 4,
>  DVB_DEVICE_NET      = 5,
> 
>  DVB_DEVICE_VIDEO    = 6,
>  DVB_DEVICE_AUDIO    = 7,
>  DVB_DEVICE_OSD      = 8,
> };
> ```
> 
> ...and then maybe `nums2minor()` can be optimized to take advantage of
> that assignment.

That will break userspace :-) The DVB minor ranges are
(from Documentation/drivers.txt):

 212 char	LinuxTV.org DVB driver subsystem
		  0 = /dev/dvb/adapter0/video0    first video decoder of first c
ard
		  1 = /dev/dvb/adapter0/audio0    first audio decoder of first c
ard
		  2 = /dev/dvb/adapter0/sec0      (obsolete/unused)
		  3 = /dev/dvb/adapter0/frontend0 first frontend device of first
 card
		  4 = /dev/dvb/adapter0/demux0    first demux device of first ca
rd
		  5 = /dev/dvb/adapter0/dvr0      first digital video recoder de
vice of first card
		  6 = /dev/dvb/adapter0/ca0       first common access port of fi
rst card
		  7 = /dev/dvb/adapter0/net0      first network device of first 
card
		  8 = /dev/dvb/adapter0/osd0      first on-screen-display device
 of first card

Btw, the main reason to add a switch at nums2minor() is due to that:
it requires an specific number for each device type, and it is very easy
to forget about that when looking at the implementation.

As some day we may get rid of video/audio/osd types, it sounded worth to 
add an extra code. Please notice, however, that nums2minor only exists
if !CONFIG_DVB_DYNAMIC_MINORS.

I suspect that, except for some embedded devices, the default is
to have it enabled everywhere.

Yet, after revisiting it and checking the generated assembly code,
I guess we could fold the enclosed patch, in order to simplify the
static minor support of the DVB core:


--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -68,22 +68,20 @@ static const char * const dnames[] = {
 #else
 #define DVB_MAX_IDS            4
 
-static int nums2minor(int num, enum dvb_device_type type, int id)
-{
-       int n = (num << 6) | (id << 4);
+static const u8 minor_type[] = {
+       [DVB_DEVICE_VIDEO]      = 0,
+       [DVB_DEVICE_AUDIO]      = 1,
+       [DVB_DEVICE_SEC]        = 2,
+       [DVB_DEVICE_FRONTEND]   = 3,
+       [DVB_DEVICE_DEMUX]      = 4,
+       [DVB_DEVICE_DVR]        = 5,
+       [DVB_DEVICE_CA]         = 6,
+       [DVB_DEVICE_NET]        = 7,
+       [DVB_DEVICE_OSD]        = 8,
+};
 
-       switch (type) {
-       case DVB_DEVICE_VIDEO:          return n;
-       case DVB_DEVICE_AUDIO:          return n | 1;
-       case DVB_DEVICE_SEC:            return n | 2;
-       case DVB_DEVICE_FRONTEND:       return n | 3;
-       case DVB_DEVICE_DEMUX:          return n | 4;
-       case DVB_DEVICE_DVR:            return n | 5;
-       case DVB_DEVICE_CA:             return n | 6;
-       case DVB_DEVICE_NET:            return n | 7;
-       case DVB_DEVICE_OSD:            return n | 8;
-       }
-}
+#define nums2minor(num, type, id) \
+       (((num) << 6) | ((id) << 4) | minor_type[type])
 
 #define MAX_DVB_MINORS         (DVB_MAX_ADAPTERS*64)
 #endif

With the above change, it seems that the code is likely only a few bytes
longer than the original code, and make it clearer about the
static minor numbering requirements.

The generated i386 asm code at the read only data segment is:

minor_type:
	.byte	2
	.byte	3
	.byte	4
	.byte	5
	.byte	6
	.byte	7
	.byte	0
	.byte	1
	.byte	8

And at the code segment:

# drivers/media/dvb-core/dvbdev.c:511: 	minor = nums2minor(adap->num, type, id);
	.loc 1 511 0
	movl	-20(%ebp), %eax	# %sfp, adap
.LVL295:
	movl	(%eax), %eax	# adap_29(D)->num, adap_29(D)->num
.LVL296:
	movl	%eax, -24(%ebp)	# adap_29(D)->num, %sfp
	movl	%eax, %edx	# adap_29(D)->num, adap_29(D)->num
	movl	12(%ebp), %eax	# type, tmp265
	sall	$6, %edx	#, adap_29(D)->num
	movzbl	minor_type(%eax), %eax	# minor_type, tmp193
	orl	%eax, %edx	# tmp193, tmp194
	movl	-16(%ebp), %eax	# %sfp, tmp195
	movl	%edx, %edi	# tmp194, tmp194
	sall	$4, %eax	#, tmp195
	orl	%eax, %edi	# tmp195, tmp194

That seems good enough on my eyes.

Thanks,
Mauro