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
