Re: [PATCH v9 1/5] firmware: cs_dsp: Add write sequencer interface

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

On Thu, Mar 14, 2024 at 09:40:34AM -0500, Jeff LaBundy wrote:
> On Mon, Mar 11, 2024 at 10:14:17AM +0000, Charles Keepax wrote:
> > On Sun, Mar 10, 2024 at 02:12:16PM -0500, Jeff LaBundy wrote:
> > > On Fri, Mar 08, 2024 at 10:24:17PM +0000, James Ogletree wrote:
> > I disagree here. This is the write function, there is no guarantee
> > this is even called at probe time. This is generic code going
> > into the DSP library and will presumably get re-used for different
> > purposes and on different parts in the future. Freeing on the error
> > path is much safer here.
> 
> Agreed that this won't be called during probe; all I mean to say is
> that I don't see any issue in hanging on to a bit of memory while the
> device is in an error state, before the module is unloaded and devres
> unrolls all of the device-managed resources.

I think that's the problem the assumption that all users will
completely bork the device if this operation fails. Likely but
far from certain.

> It's also perfectly fine to leave this as-is; the existing method is
> functionally correct, and I understand the perspective of having the
> generic library clean up immediately. If that's the intent however,
> the same error handling needs to be applied in cs_dsp_populate_wseq();
> currently only cs_dsp_wseq_write() calls devm_kfree() on error. Since
> L50 uses asynchronous FW loading, neither are called until after the
> device probes.
> 

Hmm... yes I guess in my head I wasn't thinking so much about
populate being called at runtime, but I am inclined to agree. We
should probably update populate to also free on the error path.

Thanks,
Charles