[alsa-devel] RFC: snd_card_create() function
Hi,
so far, we use snd_card_new() function to create a card instance. A known problem regarding this API is that it doesn't return a proper error code, thus the probe callback always returns -ENOMEM (or whatever the driver defines) no matter which error occurred actually. A typical case is the card slot conflict. Even in such a case, it returns -ENOMEM, which may mislead the user.
For fixing this situation, I'd like to convert from snd_card_new() to a new function, snd_card_create():
int snd_card_create(int idx, const char *id, struct module *module, int extra_size, struct snd_card **card_ret);
and provides snd_card_new() as a wrapper with deprecated flag:
static inline __deprecated struct snd_card *snd_card_new(int idx, const char *id, struct module *module, int extra_size) { struct snd_card *card; if (snd_card_create(idx, id, module, extra_size, &card) < 0) return NULL; return card; }
The merit of creating a new function is that the older and out-of-tree codes (like xfi) work without modification. There is alternative option, namely to use PTR_ERR() and keep the function as is. But this has no advantage in practice -- it can't detect the old API at build time, and it may Oops.
The preliminary patches are found in test/snd_card_new-err branch of sound git tree: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git test/snd_card_new-err
If anyone finds problems with this, please let me know.
thanks,
Takashi
Sounds good to me. One can keep the wrapper outside of the kernel, and only available in alsa-driver for those out of tree people.
Kind Regards
James
2009/1/12 Takashi Iwai tiwai@suse.de:
Hi,
so far, we use snd_card_new() function to create a card instance. A known problem regarding this API is that it doesn't return a proper error code, thus the probe callback always returns -ENOMEM (or whatever the driver defines) no matter which error occurred actually. A typical case is the card slot conflict. Even in such a case, it returns -ENOMEM, which may mislead the user.
For fixing this situation, I'd like to convert from snd_card_new() to a new function, snd_card_create():
int snd_card_create(int idx, const char *id, struct module *module, int extra_size, struct snd_card **card_ret);
and provides snd_card_new() as a wrapper with deprecated flag:
static inline __deprecated struct snd_card *snd_card_new(int idx, const char *id, struct module *module, int extra_size) { struct snd_card *card; if (snd_card_create(idx, id, module, extra_size, &card) < 0) return NULL; return card; }
The merit of creating a new function is that the older and out-of-tree codes (like xfi) work without modification. There is alternative option, namely to use PTR_ERR() and keep the function as is. But this has no advantage in practice -- it can't detect the old API at build time, and it may Oops.
The preliminary patches are found in test/snd_card_new-err branch of sound git tree: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git test/snd_card_new-err
If anyone finds problems with this, please let me know.
thanks,
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Mon, 12 Jan 2009 14:42:06 +0000, James Courtier-Dutton wrote:
Sounds good to me. One can keep the wrapper outside of the kernel, and only available in alsa-driver for those out of tree people.
Yes, that's good. I think it'd be better to do that after one kernel-cycle later for a softer landing, though.
thanks,
Takashi
Kind Regards
James
2009/1/12 Takashi Iwai tiwai@suse.de:
Hi,
so far, we use snd_card_new() function to create a card instance. A known problem regarding this API is that it doesn't return a proper error code, thus the probe callback always returns -ENOMEM (or whatever the driver defines) no matter which error occurred actually. A typical case is the card slot conflict. Even in such a case, it returns -ENOMEM, which may mislead the user.
For fixing this situation, I'd like to convert from snd_card_new() to a new function, snd_card_create():
int snd_card_create(int idx, const char *id, struct module *module, int extra_size, struct snd_card **card_ret);
and provides snd_card_new() as a wrapper with deprecated flag:
static inline __deprecated struct snd_card *snd_card_new(int idx, const char *id, struct module *module, int extra_size) { struct snd_card *card; if (snd_card_create(idx, id, module, extra_size, &card) < 0) return NULL; return card; }
The merit of creating a new function is that the older and out-of-tree codes (like xfi) work without modification. There is alternative option, namely to use PTR_ERR() and keep the function as is. But this has no advantage in practice -- it can't detect the old API at build time, and it may Oops.
The preliminary patches are found in test/snd_card_new-err branch of sound git tree: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git test/snd_card_new-err
If anyone finds problems with this, please let me know.
thanks,
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
2009/1/12 Takashi Iwai tiwai@suse.de:
At Mon, 12 Jan 2009 14:42:06 +0000, James Courtier-Dutton wrote:
Sounds good to me. One can keep the wrapper outside of the kernel, and only available in alsa-driver for those out of tree people.
Yes, that's good. I think it'd be better to do that after one kernel-cycle later for a softer landing, though.
How soft do we need this? The change is probably max of 3 lines of code per sound card that has an out of tree driver. The only one I am aware of is the xfi one. With the wrapper being more than 3 lines of code being put into the kernel only to be removed again seems a little un-necessary to me. For internal kernel changes, I don't think we should care about out of kernel drivers. None of the rest of the kernel developers go out of their way for anything out-of-mainline.
Kind Regards
James
At Mon, 12 Jan 2009 15:06:51 +0000, James Courtier-Dutton wrote:
2009/1/12 Takashi Iwai tiwai@suse.de:
At Mon, 12 Jan 2009 14:42:06 +0000, James Courtier-Dutton wrote:
Sounds good to me. One can keep the wrapper outside of the kernel, and only available in alsa-driver for those out of tree people.
Yes, that's good. I think it'd be better to do that after one kernel-cycle later for a softer landing, though.
How soft do we need this?
One kernel cycle is enough.
The change is probably max of 3 lines of code per sound card that has an out of tree driver. The only one I am aware of is the xfi one. With the wrapper being more than 3 lines of code being put into the kernel only to be removed again seems a little un-necessary to me. For internal kernel changes, I don't think we should care about out of kernel drivers. None of the rest of the kernel developers go out of their way for anything out-of-mainline.
Well, my main concern isn't about out-of-kernel drivers like xfi, but the drivers that will come from other trees. The codes outside ALSA tree, for example, V4L, can't be always controlled by us (suppose a new V4L driver comes in for the next kernel). And these are more or less "mainline" development.
If you see the linux-next development, you'll find how the internal API can be a pain among several tree merges. A typical API change (that has been often seen in driver-core area) introduces first a wrapper while converting all in the present kernel, then kill it at the next cycle.
Takashi
Takashi Iwai wrote:
At Mon, 12 Jan 2009 15:06:51 +0000, James Courtier-Dutton wrote:
2009/1/12 Takashi Iwai tiwai@suse.de:
At Mon, 12 Jan 2009 14:42:06 +0000, James Courtier-Dutton wrote:
Sounds good to me. One can keep the wrapper outside of the kernel, and only available in alsa-driver for those out of tree people.
Yes, that's good. I think it'd be better to do that after one kernel-cycle later for a softer landing, though.
How soft do we need this?
One kernel cycle is enough.
The change is probably max of 3 lines of code per sound card that has an out of tree driver. The only one I am aware of is the xfi one. With the wrapper being more than 3 lines of code being put into the kernel only to be removed again seems a little un-necessary to me. For internal kernel changes, I don't think we should care about out of kernel drivers. None of the rest of the kernel developers go out of their way for anything out-of-mainline.
Well, my main concern isn't about out-of-kernel drivers like xfi, but the drivers that will come from other trees. The codes outside ALSA tree, for example, V4L, can't be always controlled by us (suppose a new V4L driver comes in for the next kernel). And these are more or less "mainline" development.
If you see the linux-next development, you'll find how the internal API can be a pain among several tree merges. A typical API change (that has been often seen in driver-core area) introduces first a wrapper while converting all in the present kernel, then kill it at the next cycle.
Ah! I did not think of V4L. Once cycle seems OK to me. I hope it creates a warning message to the V4L developers, although, I don't see why you cannot include a patch to V4L together with all the other alsa cards drivers that would get into linux-next.
At Wed, 14 Jan 2009 15:14:54 +0000, James Courtier-Dutton wrote:
Takashi Iwai wrote:
At Mon, 12 Jan 2009 15:06:51 +0000, James Courtier-Dutton wrote:
2009/1/12 Takashi Iwai tiwai@suse.de:
At Mon, 12 Jan 2009 14:42:06 +0000, James Courtier-Dutton wrote:
Sounds good to me. One can keep the wrapper outside of the kernel, and only available in alsa-driver for those out of tree people.
Yes, that's good. I think it'd be better to do that after one kernel-cycle later for a softer landing, though.
How soft do we need this?
One kernel cycle is enough.
The change is probably max of 3 lines of code per sound card that has an out of tree driver. The only one I am aware of is the xfi one. With the wrapper being more than 3 lines of code being put into the kernel only to be removed again seems a little un-necessary to me. For internal kernel changes, I don't think we should care about out of kernel drivers. None of the rest of the kernel developers go out of their way for anything out-of-mainline.
Well, my main concern isn't about out-of-kernel drivers like xfi, but the drivers that will come from other trees. The codes outside ALSA tree, for example, V4L, can't be always controlled by us (suppose a new V4L driver comes in for the next kernel). And these are more or less "mainline" development.
If you see the linux-next development, you'll find how the internal API can be a pain among several tree merges. A typical API change (that has been often seen in driver-core area) introduces first a wrapper while converting all in the present kernel, then kill it at the next cycle.
Ah! I did not think of V4L. Once cycle seems OK to me. I hope it creates a warning message to the V4L developers, although, I don't see why you cannot include a patch to V4L together with all the other alsa cards drivers that would get into linux-next.
It's possible, but often not too easy if a new V4L driver is introduced in their tree. In that case, either we or they import their/out patch. This can be a mess once if the tree gets rebased or a similar thing happens.
Of course, I already patched the existing drivers (also found in usb gadget). But any upcoming stuff can be hard to expect...
thanks,
Takashi
On Mon, 12 Jan 2009 15:35:19 +0100 Takashi Iwai tiwai@suse.de wrote:
Hi,
so far, we use snd_card_new() function to create a card instance. A known problem regarding this API is that it doesn't return a proper error code, thus the probe callback always returns -ENOMEM (or whatever the driver defines) no matter which error occurred actually. A typical case is the card slot conflict. Even in such a case, it returns -ENOMEM, which may mislead the user.
Another approach is PTR_ERR macro used already in the kernel (see include/linux/err.h). It encodes the error code in the returned pointer value. There are already hndy macros/inlines to get this code, test error, etc.
Regards, Krzysztof
---------------------------------------------------------------------- Speak Up. Angielski szybko i skutecznie. 3 miesiace nauki gratis. Sprawdz. >> http://link.interia.pl/f2019
At Mon, 12 Jan 2009 17:08:28 +0100, Krzysztof Helt wrote:
On Mon, 12 Jan 2009 15:35:19 +0100 Takashi Iwai tiwai@suse.de wrote:
Hi,
so far, we use snd_card_new() function to create a card instance. A known problem regarding this API is that it doesn't return a proper error code, thus the probe callback always returns -ENOMEM (or whatever the driver defines) no matter which error occurred actually. A typical case is the card slot conflict. Even in such a case, it returns -ENOMEM, which may mislead the user.
Another approach is PTR_ERR macro used already in the kernel (see include/linux/err.h). It encodes the error code in the returned pointer value. There are already hndy macros/inlines to get this code, test error, etc.
Yep. However, as I mentioned in the paragraph after the text above, it gives you little advantage in this case.
If an error occurs, the new function returns a non-NULL value. Since the old caller side does only a NULL-check, it would accept the error pointer as is, ended up an Oops.
More badly, this old-style return-value check can't be detected at the compile time because the API (at least the function calling) is unchanged.
So, PTR_ERR() is nice to be introduced at the first place but pretty bad as a transition from a NULL/non-NULL API.
Takashi
At Mon, 12 Jan 2009 15:35:19 +0100, I wrote:
Hi,
so far, we use snd_card_new() function to create a card instance. A known problem regarding this API is that it doesn't return a proper error code, thus the probe callback always returns -ENOMEM (or whatever the driver defines) no matter which error occurred actually. A typical case is the card slot conflict. Even in such a case, it returns -ENOMEM, which may mislead the user.
For fixing this situation, I'd like to convert from snd_card_new() to a new function, snd_card_create():
int snd_card_create(int idx, const char *id, struct module *module, int extra_size, struct snd_card **card_ret);
and provides snd_card_new() as a wrapper with deprecated flag:
static inline __deprecated struct snd_card *snd_card_new(int idx, const char *id, struct module *module, int extra_size) { struct snd_card *card; if (snd_card_create(idx, id, module, extra_size, &card) < 0) return NULL; return card; }
The merit of creating a new function is that the older and out-of-tree codes (like xfi) work without modification. There is alternative option, namely to use PTR_ERR() and keep the function as is. But this has no advantage in practice -- it can't detect the old API at build time, and it may Oops.
The preliminary patches are found in test/snd_card_new-err branch of sound git tree: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git test/snd_card_new-err
If anyone finds problems with this, please let me know.
Since no big issues came up, I merged that branch to master branch. Thanks for all your comments.
Takashi
participants (4)
-
James Courtier-Dutton -
James Courtier-Dutton -
Krzysztof Helt -
Takashi Iwai