| Message ID | 1451100926-20637-4-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 26 Dec 2015 04:35:24 +0100, Takashi Sakamoto wrote: > > When work of sound card registration fails, bus reset on IEEE 1394 > can schedule the work again. In this case, currently instances of PCM and > RawMIDI devices are not released, but allocated and errors occurs. > > This commit solves this issue. The allocated data is kept and released > at any failures in the work. Aren't they be released in snd_card_free() in the later error path? Takashi > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > --- > sound/firewire/dice/dice-midi.c | 2 ++ > sound/firewire/dice/dice-pcm.c | 2 ++ > sound/firewire/dice/dice.c | 30 +++++++++++++++++++++--------- > sound/firewire/dice/dice.h | 2 ++ > 4 files changed, 27 insertions(+), 9 deletions(-) > > diff --git a/sound/firewire/dice/dice-midi.c b/sound/firewire/dice/dice-midi.c > index 151b09f..908b43f 100644 > --- a/sound/firewire/dice/dice-midi.c > +++ b/sound/firewire/dice/dice-midi.c > @@ -153,5 +153,7 @@ int snd_dice_create_midi(struct snd_dice *dice) > if ((midi_out_ports > 0) && (midi_in_ports > 0)) > rmidi->info_flags |= SNDRV_RAWMIDI_INFO_DUPLEX; > > + dice->rmidi_dev = rmidi; > + > return 0; > } > diff --git a/sound/firewire/dice/dice-pcm.c b/sound/firewire/dice/dice-pcm.c > index 9b34319..6246ce3 100644 > --- a/sound/firewire/dice/dice-pcm.c > +++ b/sound/firewire/dice/dice-pcm.c > @@ -426,5 +426,7 @@ int snd_dice_create_pcm(struct snd_dice *dice) > if (playback > 0) > snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops); > > + dice->pcm_dev = pcm; > + > return 0; > } > diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c > index 0bc7d08..21ad894 100644 > --- a/sound/firewire/dice/dice.c > +++ b/sound/firewire/dice/dice.c > @@ -202,33 +202,45 @@ static void do_registration(struct work_struct *work) > if (dice->card->shutdown || dice->registered) > return; > > + dice->pcm_dev = NULL; > + dice->rmidi_dev = NULL; > + > err = snd_dice_transaction_init(dice); > if (err < 0) > - goto end; > + goto error; > > err = dice_read_params(dice); > if (err < 0) > - goto end; > + goto error; > > dice_card_strings(dice); > > err = snd_dice_create_pcm(dice); > if (err < 0) > - goto end; > + goto error; > > err = snd_dice_create_midi(dice); > if (err < 0) > - goto end; > + goto error; > > err = snd_card_register(dice->card); > if (err < 0) > - goto end; > + goto error; > > dice->registered = true; > -end: > - if (err < 0) > - dev_info(&dice->unit->device, > - "Sound card registration failed: %d\n", err); > + > + return; > +error: > + snd_dice_transaction_destroy(dice); > + > + if (dice->pcm_dev) > + snd_device_free(dice->card, dice->pcm_dev); > + > + if (dice->rmidi_dev) > + snd_device_free(dice->card, dice->rmidi_dev); > + > + dev_info(&dice->unit->device, > + "Sound card registration failed: %d\n", err); > } > > static void schedule_registration(struct snd_dice *dice) > diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h > index 3d5ebeb..d632ac1 100644 > --- a/sound/firewire/dice/dice.h > +++ b/sound/firewire/dice/dice.h > @@ -47,6 +47,8 @@ struct snd_dice { > > bool registered; > struct delayed_work dwork; > + struct snd_pcm *pcm_dev; > + struct snd_rawmidi *rmidi_dev; > > /* Offsets for sub-addresses */ > unsigned int global_offset; > -- > 2.5.0 >
On 2015?12?29? 18:00, Takashi Iwai wrote: > On Sat, 26 Dec 2015 04:35:24 +0100, > Takashi Sakamoto wrote: >> >> When work of sound card registration fails, bus reset on IEEE 1394 >> can schedule the work again. In this case, currently instances of PCM and >> RawMIDI devices are not released, but allocated and errors occurs. >> >> This commit solves this issue. The allocated data is kept and released >> at any failures in the work. > > Aren't they be released in snd_card_free() in the later error path? Not in the error path, but indeed in card free processing. They're not released anymore. I should have used snd_pcm_free() and snd_rawmidi_free() for this purpose. (I misunderstand they should be used after calling snd_card_register().) Thanks Takashi Sakamoto > Takashi > > >> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> >> --- >> sound/firewire/dice/dice-midi.c | 2 ++ >> sound/firewire/dice/dice-pcm.c | 2 ++ >> sound/firewire/dice/dice.c | 30 +++++++++++++++++++++--------- >> sound/firewire/dice/dice.h | 2 ++ >> 4 files changed, 27 insertions(+), 9 deletions(-) >> >> diff --git a/sound/firewire/dice/dice-midi.c b/sound/firewire/dice/dice-midi.c >> index 151b09f..908b43f 100644 >> --- a/sound/firewire/dice/dice-midi.c >> +++ b/sound/firewire/dice/dice-midi.c >> @@ -153,5 +153,7 @@ int snd_dice_create_midi(struct snd_dice *dice) >> if ((midi_out_ports > 0) && (midi_in_ports > 0)) >> rmidi->info_flags |= SNDRV_RAWMIDI_INFO_DUPLEX; >> >> + dice->rmidi_dev = rmidi; >> + >> return 0; >> } >> diff --git a/sound/firewire/dice/dice-pcm.c b/sound/firewire/dice/dice-pcm.c >> index 9b34319..6246ce3 100644 >> --- a/sound/firewire/dice/dice-pcm.c >> +++ b/sound/firewire/dice/dice-pcm.c >> @@ -426,5 +426,7 @@ int snd_dice_create_pcm(struct snd_dice *dice) >> if (playback > 0) >> snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops); >> >> + dice->pcm_dev = pcm; >> + >> return 0; >> } >> diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c >> index 0bc7d08..21ad894 100644 >> --- a/sound/firewire/dice/dice.c >> +++ b/sound/firewire/dice/dice.c >> @@ -202,33 +202,45 @@ static void do_registration(struct work_struct *work) >> if (dice->card->shutdown || dice->registered) >> return; >> >> + dice->pcm_dev = NULL; >> + dice->rmidi_dev = NULL; >> + >> err = snd_dice_transaction_init(dice); >> if (err < 0) >> - goto end; >> + goto error; >> >> err = dice_read_params(dice); >> if (err < 0) >> - goto end; >> + goto error; >> >> dice_card_strings(dice); >> >> err = snd_dice_create_pcm(dice); >> if (err < 0) >> - goto end; >> + goto error; >> >> err = snd_dice_create_midi(dice); >> if (err < 0) >> - goto end; >> + goto error; >> >> err = snd_card_register(dice->card); >> if (err < 0) >> - goto end; >> + goto error; >> >> dice->registered = true; >> -end: >> - if (err < 0) >> - dev_info(&dice->unit->device, >> - "Sound card registration failed: %d\n", err); >> + >> + return; >> +error: >> + snd_dice_transaction_destroy(dice); >> + >> + if (dice->pcm_dev) >> + snd_device_free(dice->card, dice->pcm_dev); >> + >> + if (dice->rmidi_dev) >> + snd_device_free(dice->card, dice->rmidi_dev); >> + >> + dev_info(&dice->unit->device, >> + "Sound card registration failed: %d\n", err); >> } >> >> static void schedule_registration(struct snd_dice *dice) >> diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h >> index 3d5ebeb..d632ac1 100644 >> --- a/sound/firewire/dice/dice.h >> +++ b/sound/firewire/dice/dice.h >> @@ -47,6 +47,8 @@ struct snd_dice { >> >> bool registered; >> struct delayed_work dwork; >> + struct snd_pcm *pcm_dev; >> + struct snd_rawmidi *rmidi_dev; >> >> /* Offsets for sub-addresses */ >> unsigned int global_offset; >> -- >> 2.5.0 >> >
On 2015?12?29? 19:56, Takashi Sakamoto wrote: > On 2015?12?29? 18:00, Takashi Iwai wrote: >> On Sat, 26 Dec 2015 04:35:24 +0100, >> Takashi Sakamoto wrote: >>> >>> When work of sound card registration fails, bus reset on IEEE 1394 >>> can schedule the work again. In this case, currently instances of PCM and >>> RawMIDI devices are not released, but allocated and errors occurs. >>> >>> This commit solves this issue. The allocated data is kept and released >>> at any failures in the work. >> >> Aren't they be released in snd_card_free() in the later error path? > > Not in the error path, but indeed in card free processing. They're not > released anymore. I should have used snd_pcm_free() and > snd_rawmidi_free() for this purpose. (I misunderstand they should be > used after calling snd_card_register().) I realized that both of snd_rawmidi_free() and snd_pcm_free() are called by snd_device_free(), then all of allocated memory blocks are freed. Which error pathes do you think to cause the problems? Regards Takashi Sakamoto
On Tue, 29 Dec 2015 12:37:06 +0100, Takashi Sakamoto wrote: > > On 2015?12?29? 19:56, Takashi Sakamoto wrote: > > On 2015?12?29? 18:00, Takashi Iwai wrote: > >> On Sat, 26 Dec 2015 04:35:24 +0100, > >> Takashi Sakamoto wrote: > >>> > >>> When work of sound card registration fails, bus reset on IEEE 1394 > >>> can schedule the work again. In this case, currently instances of PCM and > >>> RawMIDI devices are not released, but allocated and errors occurs. > >>> > >>> This commit solves this issue. The allocated data is kept and released > >>> at any failures in the work. > >> > >> Aren't they be released in snd_card_free() in the later error path? > > > > Not in the error path, but indeed in card free processing. They're not > > released anymore. I should have used snd_pcm_free() and > > snd_rawmidi_free() for this purpose. (I misunderstand they should be > > used after calling snd_card_register().) > > I realized that both of snd_rawmidi_free() and snd_pcm_free() are called > by snd_device_free(), then all of allocated memory blocks are freed. > > Which error pathes do you think to cause the problems? I don't think it causing problems but just superfluous to call explicitly there. You (should) call snd_card_free*() in the error path, and these components are released there automatically. In which code path did you see the leak? This already sounds strange. Takashi
Hi, On Dec 30 2015 17:09, Takashi Iwai wrote: > On Tue, 29 Dec 2015 12:37:06 +0100, > Takashi Sakamoto wrote: >> >> On 2015?12?29? 19:56, Takashi Sakamoto wrote: >>> On 2015?12?29? 18:00, Takashi Iwai wrote: >>>> On Sat, 26 Dec 2015 04:35:24 +0100, >>>> Takashi Sakamoto wrote: >>>>> >>>>> When work of sound card registration fails, bus reset on IEEE 1394 >>>>> can schedule the work again. In this case, currently instances of PCM and >>>>> RawMIDI devices are not released, but allocated and errors occurs. >>>>> >>>>> This commit solves this issue. The allocated data is kept and released >>>>> at any failures in the work. >>>> >>>> Aren't they be released in snd_card_free() in the later error path? >>> >>> Not in the error path, but indeed in card free processing. They're not >>> released anymore. I should have used snd_pcm_free() and >>> snd_rawmidi_free() for this purpose. (I misunderstand they should be >>> used after calling snd_card_register().) >> >> I realized that both of snd_rawmidi_free() and snd_pcm_free() are called >> by snd_device_free(), then all of allocated memory blocks are freed. >> >> Which error pathes do you think to cause the problems? > > I don't think it causing problems but just superfluous to call > explicitly there. You (should) call snd_card_free*() in the error > path, and these components are released there automatically. I had two reason not to call snd_card_free() family in error path of the 'do_registration(). Originally, the 'do_registration()' and .remove() are on a race condition against sound card instance. But now. remove() waits to finish a work of the sound card registration, therefore snd_card_free() can be called in error path of the work. Another reason is the memory object for driver_data. Currently I use struct snd_dice allocated by snd_card_new(). If calling snd_card_new() in the work, I must allocate it in .probe() callback independently. I thought it a bit hard to maintain the memory object against the race condition between the work, .update() and .remove() and sound card free. But the race condition becomes simpler when .remove() waits to finish the work. OK. I'll modify and post the new patchset. > In which code path did you see the leak? This already sounds > strange. It was my misunderstanding a few days ago. Thanks Takashi Sakamoto
diff --git a/sound/firewire/dice/dice-midi.c b/sound/firewire/dice/dice-midi.c index 151b09f..908b43f 100644 --- a/sound/firewire/dice/dice-midi.c +++ b/sound/firewire/dice/dice-midi.c @@ -153,5 +153,7 @@ int snd_dice_create_midi(struct snd_dice *dice) if ((midi_out_ports > 0) && (midi_in_ports > 0)) rmidi->info_flags |= SNDRV_RAWMIDI_INFO_DUPLEX; + dice->rmidi_dev = rmidi; + return 0; } diff --git a/sound/firewire/dice/dice-pcm.c b/sound/firewire/dice/dice-pcm.c index 9b34319..6246ce3 100644 --- a/sound/firewire/dice/dice-pcm.c +++ b/sound/firewire/dice/dice-pcm.c @@ -426,5 +426,7 @@ int snd_dice_create_pcm(struct snd_dice *dice) if (playback > 0) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &playback_ops); + dice->pcm_dev = pcm; + return 0; } diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c index 0bc7d08..21ad894 100644 --- a/sound/firewire/dice/dice.c +++ b/sound/firewire/dice/dice.c @@ -202,33 +202,45 @@ static void do_registration(struct work_struct *work) if (dice->card->shutdown || dice->registered) return; + dice->pcm_dev = NULL; + dice->rmidi_dev = NULL; + err = snd_dice_transaction_init(dice); if (err < 0) - goto end; + goto error; err = dice_read_params(dice); if (err < 0) - goto end; + goto error; dice_card_strings(dice); err = snd_dice_create_pcm(dice); if (err < 0) - goto end; + goto error; err = snd_dice_create_midi(dice); if (err < 0) - goto end; + goto error; err = snd_card_register(dice->card); if (err < 0) - goto end; + goto error; dice->registered = true; -end: - if (err < 0) - dev_info(&dice->unit->device, - "Sound card registration failed: %d\n", err); + + return; +error: + snd_dice_transaction_destroy(dice); + + if (dice->pcm_dev) + snd_device_free(dice->card, dice->pcm_dev); + + if (dice->rmidi_dev) + snd_device_free(dice->card, dice->rmidi_dev); + + dev_info(&dice->unit->device, + "Sound card registration failed: %d\n", err); } static void schedule_registration(struct snd_dice *dice) diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h index 3d5ebeb..d632ac1 100644 --- a/sound/firewire/dice/dice.h +++ b/sound/firewire/dice/dice.h @@ -47,6 +47,8 @@ struct snd_dice { bool registered; struct delayed_work dwork; + struct snd_pcm *pcm_dev; + struct snd_rawmidi *rmidi_dev; /* Offsets for sub-addresses */ unsigned int global_offset;
When work of sound card registration fails, bus reset on IEEE 1394 can schedule the work again. In this case, currently instances of PCM and RawMIDI devices are not released, but allocated and errors occurs. This commit solves this issue. The allocated data is kept and released at any failures in the work. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/firewire/dice/dice-midi.c | 2 ++ sound/firewire/dice/dice-pcm.c | 2 ++ sound/firewire/dice/dice.c | 30 +++++++++++++++++++++--------- sound/firewire/dice/dice.h | 2 ++ 4 files changed, 27 insertions(+), 9 deletions(-)