| Message ID | 1458035836-1843-2-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4a6c5e6a8d29e4d33858227db9179e91aa8a7407 |
| Headers | show |
On Tue, 15 Mar 2016 10:57:11 +0100, Vinod Koul wrote: > > It is always better to use list_for_each_entry_safe() while doing > cleanup. So use this instead of open coding this in list in > snd_hdac_stream_free_all() > > Signed-off-by: Jeeja KP <jeeja.kp@intel.com> > Signed-off-by: Vinod Koul <vinod.koul@intel.com> While this change is fine, we shouldn't trust blindly list_for_each_safe() as always safe. It assumes that the list removal is done only for the current item. But it's not always true. The loop in the current code is one of standard idiom in such a case. In anyway, take my ack when Mark applies it: Acked-by: Takashi Iwai <tiwai@suse.de> Takashi > --- > sound/hda/ext/hdac_ext_stream.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c > index 023cc4cad5c1..626f3bb24c55 100644 > --- a/sound/hda/ext/hdac_ext_stream.c > +++ b/sound/hda/ext/hdac_ext_stream.c > @@ -104,12 +104,11 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_init_all); > */ > void snd_hdac_stream_free_all(struct hdac_ext_bus *ebus) > { > - struct hdac_stream *s; > + struct hdac_stream *s, *_s; > struct hdac_ext_stream *stream; > struct hdac_bus *bus = ebus_to_hbus(ebus); > > - while (!list_empty(&bus->stream_list)) { > - s = list_first_entry(&bus->stream_list, struct hdac_stream, list); > + list_for_each_entry_safe(s, _s, &bus->stream_list, list) { > stream = stream_to_hdac_ext_stream(s); > snd_hdac_ext_stream_decouple(ebus, stream, false); > list_del(&s->list); > -- > 1.9.1 >
On Tue, Mar 15, 2016 at 11:00:53AM +0100, Takashi Iwai wrote: > On Tue, 15 Mar 2016 10:57:11 +0100, > Vinod Koul wrote: > > > > It is always better to use list_for_each_entry_safe() while doing > > cleanup. So use this instead of open coding this in list in > > snd_hdac_stream_free_all() > > > > Signed-off-by: Jeeja KP <jeeja.kp@intel.com> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > While this change is fine, we shouldn't trust blindly > list_for_each_safe() as always safe. It assumes that the list removal > is done only for the current item. But it's not always true. The > loop in the current code is one of standard idiom in such a case. Yes thanks for the warning :) > > In anyway, take my ack when Mark applies it: > Acked-by: Takashi Iwai <tiwai@suse.de> Will add this for v2 >-- ~Vinod
diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c index 023cc4cad5c1..626f3bb24c55 100644 --- a/sound/hda/ext/hdac_ext_stream.c +++ b/sound/hda/ext/hdac_ext_stream.c @@ -104,12 +104,11 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_init_all); */ void snd_hdac_stream_free_all(struct hdac_ext_bus *ebus) { - struct hdac_stream *s; + struct hdac_stream *s, *_s; struct hdac_ext_stream *stream; struct hdac_bus *bus = ebus_to_hbus(ebus); - while (!list_empty(&bus->stream_list)) { - s = list_first_entry(&bus->stream_list, struct hdac_stream, list); + list_for_each_entry_safe(s, _s, &bus->stream_list, list) { stream = stream_to_hdac_ext_stream(s); snd_hdac_ext_stream_decouple(ebus, stream, false); list_del(&s->list);