RFC: snd_card_create() function

RFC: snd_card_create() function

[Takashi Iwai]

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

Re: RFC: snd_card_create() function

[James Courtier-Dutton]

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
>

Re: RFC: snd_card_create() function

[Takashi Iwai]

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
> >
> 

Re: RFC: snd_card_create() function

[James Courtier-Dutton]

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

Re: RFC: snd_card_create() function

[Takashi Iwai]

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

Re: RFC: snd_card_create() function

[James Courtier-Dutton]

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.

Re: RFC: snd_card_create() function

[Takashi Iwai]

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

Re: RFC: snd_card_create() function

[Krzysztof Helt]

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

Re: RFC: snd_card_create() function

[Takashi Iwai]

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

Re: RFC: snd_card_create() function

[Takashi Iwai]

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