Re: [Embedded-pv-devel] [PATCH v12] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.11.16 at 12:57, <andr2000@xxxxxxxxx> wrote:
> + * Request open - open a PCM stream for playback or capture:
> + *          0                 1                  2                3        
> octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+

I think individual requests would better spell out their values, instead
of all saying "operation".

> + * id - uint16_t, private guest value, echoed in response
> + * operation - uint8_t, XENSND_OP_OPEN
> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> + *   of the stream)

Please explain the common header fields just once, instead of for
each of the commands.

> + * gref_directory_start - grant_ref_t, a reference to the first shared page
> + *   describing shared buffer references. At least one page exists. If shared
> + *   buffer size exceeds what can be addressed by this single page, then
> + *   reference to the next page must be supplied (gref_dir_next_page below
> + *   is not NULL)

NULL is a pointer value. You mean zero (or one of its synonyms).

> +struct xensnd_page_directory {
> +    grant_ref_t gref_dir_next_page;
> +    uint32_t num_grefs;

You can't fit that many requests on one page anyway, so I think it
would be better to limit this to 16 bits, and leave 16 bits reserved
for future use (specified as must-be-zero). (Which made me look
back at other reserved fields: You them "padding" in the ASCII art,
but "reserved" in the structure definitions, and I don't think I've
found a place stating that they're required to be zero filled, again
for future extensibility.)

> +    /* zero-sized arrays are not allowed */
> +    grant_ref_t gref[1] /* Variable length */

The second comment is fine; I once again don't think the former
(describing C language aspects again) is needed.

> +struct xensnd_write_req {
> +    uint32_t offset;
> +    uint32_t len;
> +};
> +
> +struct xensnd_read_req {
> +    uint32_t offset;
> +    uint32_t len;
> +};

There should be just one of them, possibly named xensnd_rw_req.

> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
> + * values:
> + *
> + *          0                 1                  2                3        
> octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
> + * +-----------------+-----------------+-----------------+-----------------+

There's no i defined anywhere here. Did you mean the last one to
be XENSND_OP_OPEN.pcm_channels - 1 (just like for the two volume
operations)?

> +union xensnd_req {
> +    struct {
> +        uint16_t id;
> +        uint8_t operation;
> +        uint8_t stream_idx;
> +        uint32_t padding;

reserved (I don't see why you'd need padding here)

> +        union {
> +            struct xensnd_open_req open;
> +            struct xensnd_close_req close;
> +            struct xensnd_write_req write;
> +            struct xensnd_read_req read;
> +            struct xensnd_get_vol_req get_vol;
> +            struct xensnd_set_vol_req set_vol;
> +            struct xensnd_mute_req mute;
> +            struct xensnd_unmute_req unmute;
> +        } op;
> +    } data;
> +    uint8_t raw[64];

This is still misplaced imo, and belongs into the inner union, with the
array size suitably reduced. After all the initial part is mean to be
common for all verbs aiui, i.e. including future ones.

> +union xensnd_resp {
> +    struct {
> +        uint16_t id;
> +        uint8_t operation;
> +        uint8_t stream_idx;
> +        int8_t status;
> +        uint8_t padding[3];
> +    } data;
> +    uint8_t raw[64];

And here you don't need the field at all - just make the padding field
large enough.

Jan

_______________________________________________
Embedded-pv-devel mailing list
Embedded-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel