[alsa-devel] Removal of indirect access in control API
Hi,
ALSA control elements have "indirect" flag, which is supposed to allow the indirect access via pointer. But, as far as I know, this feature has never been used nor implemented, and unsupported via 32/64bit compat layer.
So, I'd like to get rid of this now. The clean-up patch is below.
If you have objections, please let me know.
thanks,
Takashi
---
diff -r 34e3c66d7a5c core/control.c --- a/core/control.c Mon Nov 12 18:04:54 2007 +0100 +++ b/core/control.c Mon Nov 12 18:05:16 2007 +0100 @@ -232,8 +232,6 @@ struct snd_kcontrol *snd_ctl_new1(const access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE : (ncontrol->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE| SNDRV_CTL_ELEM_ACCESS_INACTIVE| - SNDRV_CTL_ELEM_ACCESS_DINDIRECT| - SNDRV_CTL_ELEM_ACCESS_INDIRECT| SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE| SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK)); kctl.info = ncontrol->info; @@ -692,7 +690,7 @@ int snd_ctl_elem_read(struct snd_card *c struct snd_kcontrol *kctl; struct snd_kcontrol_volatile *vd; unsigned int index_offset; - int result, indirect; + int result;
down_read(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id); @@ -701,17 +699,12 @@ int snd_ctl_elem_read(struct snd_card *c } else { index_offset = snd_ctl_get_ioff(kctl, &control->id); vd = &kctl->vd[index_offset]; - indirect = vd->access & SNDRV_CTL_ELEM_ACCESS_INDIRECT ? 1 : 0; - if (control->indirect != indirect) { - result = -EACCES; - } else { - if ((vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && kctl->get != NULL) { - snd_ctl_build_ioff(&control->id, kctl, index_offset); - result = kctl->get(kctl, control); - } else { - result = -EPERM; - } - } + if ((vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && + kctl->get != NULL) { + snd_ctl_build_ioff(&control->id, kctl, index_offset); + result = kctl->get(kctl, control); + } else + result = -EPERM; } up_read(&card->controls_rwsem); return result; @@ -748,7 +741,7 @@ int snd_ctl_elem_write(struct snd_card * struct snd_kcontrol *kctl; struct snd_kcontrol_volatile *vd; unsigned int index_offset; - int result, indirect; + int result;
down_read(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id); @@ -757,23 +750,19 @@ int snd_ctl_elem_write(struct snd_card * } else { index_offset = snd_ctl_get_ioff(kctl, &control->id); vd = &kctl->vd[index_offset]; - indirect = vd->access & SNDRV_CTL_ELEM_ACCESS_INDIRECT ? 1 : 0; - if (control->indirect != indirect) { - result = -EACCES; + if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || + kctl->put == NULL || + (file && vd->owner && vd->owner != file)) { + result = -EPERM; } else { - if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || - kctl->put == NULL || - (file && vd->owner != NULL && vd->owner != file)) { - result = -EPERM; - } else { - snd_ctl_build_ioff(&control->id, kctl, index_offset); - result = kctl->put(kctl, control); - } - if (result > 0) { - up_read(&card->controls_rwsem); - snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &control->id); - return 0; - } + snd_ctl_build_ioff(&control->id, kctl, index_offset); + result = kctl->put(kctl, control); + } + if (result > 0) { + up_read(&card->controls_rwsem); + snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, + &control->id); + return 0; } } up_read(&card->controls_rwsem); diff -r 34e3c66d7a5c include/asound.h --- a/include/asound.h Mon Nov 12 18:04:54 2007 +0100 +++ b/include/asound.h Mon Nov 12 18:05:16 2007 +0100 @@ -738,8 +738,7 @@ typedef int __bitwise snd_ctl_elem_iface #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */ #define SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK (1<<28) /* kernel use a TLV callback */ #define SNDRV_CTL_ELEM_ACCESS_USER (1<<29) /* user space element */ -#define SNDRV_CTL_ELEM_ACCESS_DINDIRECT (1<<30) /* indirect access for matrix dimensions in the info structure */ -#define SNDRV_CTL_ELEM_ACCESS_INDIRECT (1<<31) /* indirect access for element value in the value structure */ +/* bits 30 and 31 are obsoleted (for indirect access) */
/* for further details see the ACPI and PCI power management specification */ #define SNDRV_CTL_POWER_D0 0x0000 /* full On */ @@ -793,30 +792,25 @@ struct snd_ctl_elem_info { } value; union { unsigned short d[4]; /* dimensions */ - unsigned short *d_ptr; /* indirect */ } dimen; unsigned char reserved[64-4*sizeof(unsigned short)]; };
struct snd_ctl_elem_value { struct snd_ctl_elem_id id; /* W: element ID */ - unsigned int indirect: 1; /* W: use indirect pointer (xxx_ptr member) */ + unsigned int indirect: 1; /* W: indirect access - obsoleted */ union { union { long value[128]; - long *value_ptr; } integer; union { long long value[64]; - long long *value_ptr; } integer64; union { unsigned int item[128]; - unsigned int *item_ptr; } enumerated; union { unsigned char data[512]; - unsigned char *data_ptr; } bytes; struct snd_aes_iec958 iec958; } value; /* RO */
On Thu, 15 Nov 2007, Takashi Iwai wrote:
Hi,
ALSA control elements have "indirect" flag, which is supposed to allow the indirect access via pointer. But, as far as I know, this feature has never been used nor implemented, and unsupported via 32/64bit compat layer.
So, I'd like to get rid of this now. The clean-up patch is below.
If you have objections, please let me know.
I have objection. This layer was designed to support large mixer matrixes like in the HDSPM driver (and if I remember correctly - implementation was started after discussion with authors of RME drviers). HDSPM driver uses hwdep interface now (probably because it was ported from the OSS code). I think that it would be worth to use standard ALSA control interface to track changes thus to finish indirect interface.
Which problems are with the 32/64 bit conversion layer? I suppose that it is possible to write an ioctl converters.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project
At Thu, 15 Nov 2007 18:29:06 +0100 (CET), Jaroslav Kysela wrote:
On Thu, 15 Nov 2007, Takashi Iwai wrote:
Hi,
ALSA control elements have "indirect" flag, which is supposed to allow the indirect access via pointer. But, as far as I know, this feature has never been used nor implemented, and unsupported via 32/64bit compat layer.
So, I'd like to get rid of this now. The clean-up patch is below.
If you have objections, please let me know.
I have objection. This layer was designed to support large mixer matrixes like in the HDSPM driver (and if I remember correctly - implementation was started after discussion with authors of RME drviers). HDSPM driver uses hwdep interface now (probably because it was ported from the OSS code). I think that it would be worth to use standard ALSA control interface to track changes thus to finish indirect interface.
Well, I don't think the current design would work ever. If the mixer matrix is too large for linear control elements, then it's simply too much for the control API.
The control API doesn't have to be perfect to handle everything. Let's keep as simple as possible peacefully.
And, above all, passing through the pointer for ioctls is a bad idea in general. You'll understand if you see how ALSA qemu support got messy due to that.
Which problems are with the 32/64 bit conversion layer? I suppose that it is possible to write an ioctl converters.
Oh, it's possible but not preferable (I'll never do). The ioctl32 wrapper for control API is already a huge mess, next to PCM API. Addition of indirect access would add incredible complexity just for this minor (and even not-yet-implemented) reason.
Takashi
participants (2)
-
Jaroslav Kysela
-
Takashi Iwai