[PATCH v4] sound: rawmidi: Add framing mode

[Takashi Iwai]

On Mon, 12 Apr 2021 12:26:49 +0200,
Jaroslav Kysela wrote:
> 
> Dne 12. 04. 21 v 12:04 Takashi Iwai napsal(a):
> > On Mon, 12 Apr 2021 11:43:02 +0200,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 12. 04. 21 v 11:31 Takashi Iwai napsal(a):
> >>> On Sun, 11 Apr 2021 19:52:21 +0200,
> >>> Jaroslav Kysela wrote:
> >>>>>>>   struct snd_rawmidi_params {
> >>>>>>>   	int stream;
> >>>>>>>   	size_t buffer_size;		/* queue size in bytes */
> >>>>>>>   	size_t avail_min;		/* minimum avail bytes for wakeup */
> >>>>>>>   	unsigned int no_active_sensing: 1; /* do not send active sensing byte in close() */
> >>>>>>> -	unsigned char reserved[16];	/* reserved for future use */
> >>>>>>> +	unsigned char framing;		/* For input data only, frame incoming data */
> >>>>>> We can move this flag to above 32-bit word (no_active_sensing). I'm not sure,
> >>>>>> if we need 8 bits for this. It's first change after 20 years. Another flag may
> >>>>>> obsolete this one.
> >>>>>
> >>>>> Not sure what you mean by this, could you show the code? Framing is an 
> >>>>> enum rather than a flag, in case we find other framing formats with 
> >>>>> other sizes that would obsolete this one.
> >>>>
> >>>> unsigned int no_active_sensing: 1;
> >>>> unsigned int framing32: 1;
> >>>> unsigned int framing64: 1; /* future extension, framing32 == 0 in this case */
> >>>>
> >>>> or:
> >>>>
> >>>> unsigned int framing_mode: 3;	/* three bits for future types */
> >>>>
> >>>> perhaps also:
> >>>>
> >>>> unsigned int clock_type: 3;	/* three bits for the clock type selection */
> >>>
> >>> The usage of bit fields in an ioctl struct is a bad idea from the
> >>> compat layer POV.  Let's avoid it.
> >>
> >> What exactly do you mean?
> > 
> > The compat layer has the hard time to deal with the conversion of bit
> > fields to a different struct.  And, it's a nightmare for the emulator
> > like QEMU.  If it has to convert the ioctl struct, it has to consider
> > about the byte order as well (and there is no strict definition how to
> > put the bit fields in C language).
> 
> The endianness of the 32-bit word can be changed quite easily - and the
> situation is similar to bit flags stored in the 32-bit word. Nobody prevents
> QEMU to work with the whole 32-bit word instead bit fields in this case.

And how you assure that the bit field is set to which bit position?
There is no guarantee defined in C language about that.

> The linux compat layer may copy the whole 32-bit word, too.
> 
> I think that your argument is not so strong here.

Oh please, this is my experience years ago while discussing with the
QEMU developers in the past.  Using a bit field for an ioctl struct is
a horrible idea, after all.

> > That said, a bit field can be useful for the internal object
> > definition (if there is no racy access), but not for an object for the
> > communication that may be interpreted among different architectures.
> 
> In this case, we have 31 free bits and this information can be stored there. I
> would prefer to keep the reserved bytes for some large fields.

Again, C language doesn't define the position of the bit fields.
That's the primary problem.

If we really have to save the space, it's a nice workaround.  But for
other purposes, there is really little merit and more flip side by
that.


Takashi