[alsa-devel] [PATCH 28/44] fireworks: Add command/response functionality into hwdep interface
Takashi Sakamoto
o-takashi at sakamocchi.jp
Fri Apr 4 13:11:12 CEST 2014
Hi Clemens,
(Apr 04 2014 18:31), Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> +++ b/include/uapi/sound/firewire.h
>> #define SNDRV_FIREWIRE_EVENT_LOCK_STATUS 0x000010cc
>> #define SNDRV_FIREWIRE_EVENT_DICE_NOTIFICATION 0xd1ce004e
>> +#define SNDRV_FIREWIRE_EVENT_EFW_RESPONSE 0x4e617475
>
> Why "Natu"? ;-)
Hehe, it's my association from "Fireworks" in Japanese.
I believe cultures in Continental Europe have the similar association ;)
For practical reason, I have no other ideas to fit in the 4 bytes.
>> +/* each field should be in big endian */
>> +struct snd_efw_transaction {
>> + uint32_t length;
>> + uint32_t version;
>> + uint32_t seqnum;
>> + uint32_t category;
>> + uint32_t command;
>> + uint32_t status;
>> + uint32_t params[0];
>> +};
>
> uint32_t is not guaranteed to be available in a userspace header.
> Add <linux/types.h>, and use __u32.
Can I request the reason?
Here I force users to include <stdint.h> and use types defined in ISO
C99. I think this way is standard. (but not standard between
kernel/user-land?)
>> +++ b/sound/firewire/fireworks/fireworks.c
>> static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
>> static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
>> static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP;
>> +unsigned int resp_buf_size = 1024;
>> +bool resp_buf_debug = false;
>
> When the driver is compiled into the kernel, these variable names could
> introduce conflicts. Use a prefix like "efw_" or "fireworks_"; the
> parameter names can then be made pretty by using module_param_named().
I never pay attension to compiling as built-in.
OK. I try to use the macro, maybe with "snd_efw" so as easily realizing
it's in Sound subsystem.
>> +static long
>> +hwdep_write(struct snd_hwdep *hwdep, const char __user *data, long count,
>> + loff_t *offset)
>> +{
>> + ...
>> + if (count < sizeof(struct snd_efw_transaction))
>> + return -EINVAL;
>
> The size must not be larger than allowed for a single write transaction.
> This would eventually be caught by the firewire core, but checking it
> here would avoid allocating lots of memory.
I investigate the length of all commands and realize the maximum bytes
is 304 (0x130) for isochronous channel mapping (see
FFADO:src/fireworks/efc/efc_cmds_ioconfig.h). With leaving some space
for this value, I think 0x140 is better for this purpose.
Thank you
Takashi Sakamoto
o-takashi at sakamocchi.jp
More information about the Alsa-devel
mailing list