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

[Bui Duc Phuc]

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.  The 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;
> >   }
> >
>