Re: [PATCH v3 1/5] ASoC: intel: sst-mfld-platform-pcm: Use __free(kfree) for stream pointer

[Bui Duc Phuc]

Hi Cezary,

On Wed, Jun 24, 2026 at 10:01 AM Bui Duc Phuc <phucduc.bui@xxxxxxxxx> wrote:
>
> Hi Cezary,
>
> Thank you for the feedback.
> I will definitely fix the commit titles and group the patches by driver
> (atom and avs) as you suggested in the next version.
>
> Regarding the ordering of this cleanup patch and the locking update,
> I wanted to share the rationale behind placing it before the locking patch.
>
> According to cleanup.h guidelines and community best practices (as
> also pointed out by Charles in a similar discussion [1]),
> mixing guard()/scoped_guard() with traditional goto labels is highly
> discouraged and error-prone.
>
> In sst_media_open(), there are currently two goto labels: one for
> mutex unlock and one for kfree(stream).
> If I introduce the scoped_guard for locking first, that patch would
> still be mixed with the remaining goto label for kfree.
>
> To ensure clean, safe, and bisectable patches without mixing guard and goto,
> I had to eliminate the kfree error-path first using __free(kfree).
> This allows the subsequent locking patch to completely and cleanly replace
> the remaining mutex goto pattern with scoped_guard.
>
> Please let me know if this makes sense, or if you still prefer a
> different approach to untangle this dependency.
>
> Best regards,
> Phuc
>
> [1] https://lore.kernel.org/all/ajUcTfG3vSGz3n3d@xxxxxxxxxxxxxxxxxxxxx/
>
> On Wed, Jun 24, 2026 at 2:35 AM Cezary Rojewski
> <cezary.rojewski@xxxxxxxxx> wrote:
> >
> > On 6/23/2026 12:57 PM, phucduc.bui@xxxxxxxxx wrote:
> > > From: bui duc phuc <phucduc.bui@xxxxxxxxx>
> > >
> > > Declare 'stream' with __free(kfree) so it is automatically freed when
> > > leaving scope. This allows direct returns from error paths and removes
> > > the explicit kfree(stream) call.
> > > Set 'stream' to NULL after ownership has been transferred to
> > > runtime->private_data to prevent it from being freed on the success path.
> >
> > Hi Phuc,
> >
> > Thank you for separating the change.  TheChỉ đổi guard() → code không đẹp, vẫn phải vật lộn với goto. design of the patchset is off
> > though.  __free() is by no means a requirement for the locking update -
> > it should be located at the back.  Also, grouping changes targeting the
> > same driver makes the entire patchset easier to follow.  Right now we
> > have 2 avs patches surrounded by atom ones from either side.
> >
> > Last but not least, please drop the filename in the commit title.  We do
> > not do that here - some filenames are long and the approach leaves
> > little space for answering the _what_ question when building a proper
> > commit title.  "ASoC: Intel: atom:" is what you want here.
> >
> > >   sound/soc/intel/atom/sst-mfld-platform-pcm.c | 19 +++++++++++++------
> > >   1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > ...
> >
> >
> > > @@ -347,12 +347,19 @@ static int sst_media_open(struct snd_pcm_substream *substream,
> > >       snd_pcm_hw_constraint_step(substream->runtime, 0,
> > >                          SNDRV_PCM_HW_PARAM_PERIODS, 2);
> > >
> > > -     return snd_pcm_hw_constraint_integer(runtime,
> > > -                      SNDRV_PCM_HW_PARAM_PERIODS);
> > > +     ret_val = snd_pcm_hw_constraint_integer(runtime,
> > > +                                             SNDRV_PCM_HW_PARAM_PERIODS);
> > > +
> > > +     if (ret_val < 0)
> > > +             return ret_val;
> > > +
> > > +     stream = NULL;
> > > +
> > > +     return ret_val;
> > > +
> >
> > The entire construct looks bad, unfortunately.  Guess the reason comes
> > from its follow up - the locking update.  The delta (+13, -6) isn't
> > exactly a good advertisement for the cleanup either.  Again, I see this
> > patch as last in the set.  Should lower the delta too.
> >
> > >   out_ops:
> > >       mutex_unlock(&sst_lock);
> > > -out_power_up:
> > > -     kfree(stream);
> > > +
> > >       return ret_val;
> > >   }
> > >
> >

After re-reading the cleanup.h guidelines,
I think I was mistaken about suggesting an ordering between the
__free() and guard() patches.
Sorry for the confusion.
Since both conversions are part of the same goto-to-scope-cleanup transition,
combining them into a single patch seems more appropriate.
That said, I'm happy to follow your preference.
Would you prefer a single patch or two separate patches?

Best regards,
Phuc