Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 于2021年10月13日周三 下午2:58写道：
>
> [Meta comment, you didn't have anyone on the To: line, so that messes
> with replies...]
>
I'll add To: line in my next email, thanks for pointing out.
> On Wed, Oct 13, 2021 at 12:53:14AM +0800, Yunhao Tian wrote:
> > From: Yunhao Tian <t123yh.xyz@xxxxxxxxx>
> >
> > Currently, an array is used to contain all snd_kcontrol_new objects
> > of the audio gadget. This is unnecessary and possibly prone to an
> > (unlikely happen) race condition between the assignment of name
> > and call of snd_ctl_new1 if two audio gadget is being set up simutaneously.
> > This patch removes the global snd_kcontrol_new array and initialize
> > individual snd_kcontrol_new object when it's being used.
> >
> > Signed-off-by: Yunhao Tian <t123yh.xyz@xxxxxxxxx>
> > ---
> > drivers/usb/gadget/function/u_audio.c | 65 +++++++++++----------------
> > 1 file changed, 25 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> > index c5f39998c653..1f4226d75dd8 100644
> > --- a/drivers/usb/gadget/function/u_audio.c
> > +++ b/drivers/usb/gadget/function/u_audio.c
> > @@ -27,12 +27,6 @@
> > #define PRD_SIZE_MAX PAGE_SIZE
> > #define MIN_PERIODS 4
> >
> > -enum {
> > - UAC_FBACK_CTRL,
> > - UAC_MUTE_CTRL,
> > - UAC_VOLUME_CTRL,
> > -};
> > -
> > /* Runtime data params for one stream */
> > struct uac_rtd_params {
> > struct snd_uac_chip *uac; /* parent chip */
> > @@ -914,31 +908,6 @@ static int u_audio_volume_put(struct snd_kcontrol *kcontrol,
> > return change;
> > }
> >
> > -
> > -static struct snd_kcontrol_new u_audio_controls[] = {
> > - [UAC_FBACK_CTRL] {
> > - .iface = SNDRV_CTL_ELEM_IFACE_PCM,
> > - .name = "Capture Pitch 1000000",
> > - .info = u_audio_pitch_info,
> > - .get = u_audio_pitch_get,
> > - .put = u_audio_pitch_put,
> > - },
> > - [UAC_MUTE_CTRL] {
> > - .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > - .name = "", /* will be filled later */
> > - .info = u_audio_mute_info,
> > - .get = u_audio_mute_get,
> > - .put = u_audio_mute_put,
> > - },
> > - [UAC_VOLUME_CTRL] {
> > - .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > - .name = "", /* will be filled later */
> > - .info = u_audio_volume_info,
> > - .get = u_audio_volume_get,
> > - .put = u_audio_volume_put,
> > - },
> > -};
> > -
> > int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
> > const char *card_name)
> > {
> > @@ -946,6 +915,7 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
> > struct snd_card *card;
> > struct snd_pcm *pcm;
> > struct snd_kcontrol *kctl;
> > + struct snd_kcontrol_new kctl_new;
> > struct uac_params *params;
> > int p_chmask, c_chmask;
> > int i, err;
> > @@ -1043,8 +1013,14 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
> > strscpy(card->mixername, card_name, sizeof(card->driver));
> >
> > if (c_chmask && g_audio->in_ep_fback) {
> > - kctl = snd_ctl_new1(&u_audio_controls[UAC_FBACK_CTRL],
> > - &uac->c_prm);
> > + kctl_new = (struct snd_kcontrol_new) {
> > + .iface = SNDRV_CTL_ELEM_IFACE_PCM,
> > + .name = "Capture Pitch 1000000",
> > + .info = u_audio_pitch_info,
> > + .get = u_audio_pitch_get,
> > + .put = u_audio_pitch_put,
> > + };
> > + kctl = snd_ctl_new1(&kctl_new, &uac->c_prm);
>
> Did you test this code? Now this data is on the stack and will be
> removed later on, while the original code's data will persist after this
> function returns.
>
> Are you sure this is ok?
>
> thanks,
>
> greg k-h
Hi greg,
snd_ctl_new1 copies all members of the kctl_new struct to its own
data structure, shown in the following code (in which ncontrol is kctl_new):
kctl->id.iface = ncontrol->iface;
kctl->id.device = ncontrol->device;
kctl->id.subdevice = ncontrol->subdevice;
if (ncontrol->name) {
strscpy(kctl->id.name, ncontrol->name, sizeof(kctl->id.name));
if (strcmp(ncontrol->name, kctl->id.name) != 0)
pr_warn("ALSA: Control name '%s' truncated to '%s'\n",
ncontrol->name, kctl->id.name);
}
kctl->id.index = ncontrol->index;
so the data in kctl_new doesn't matter after calling snd_ctl_new1.
IMO It's fine to put kctl_new on the stack.
The patch is tested to be working.
