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

[Jeff LaBundy <jeff@labundy.com>]

Hi Charles,

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:
> > > +	switch (op_code) {
> > > +	case CS_DSP_WSEQ_FULL:
> > > +		cs_dsp_chunk_write(&ch, 32, op_new->address);
> > > +		cs_dsp_chunk_write(&ch, 32, op_new->data);
> > > +		break;
> > > +	case CS_DSP_WSEQ_L16:
> > > +	case CS_DSP_WSEQ_H16:
> > > +		cs_dsp_chunk_write(&ch, 24, op_new->address);
> > > +		cs_dsp_chunk_write(&ch, 16, op_new->data);
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		cs_dsp_err(dsp, "Op code not supported: %X\n", op_code);
> > > +		goto op_new_free;
> > 
> > There is no need to drop down and call devm_kfree() here; it's sufficient
> > to simply return -EINVAL. The devres core will free any instances of
> > cs_dsp_wseq_op when the driver is unbound.
> > 
> > I imagine you're calling devm_kfree() to protect against the case where
> > the driver successfully probes, but the contents of the firmware are found
> > to be invalid later. In that case, yes, this newly allocated memory will
> > persist throughout the length of the driver's life.
> > 
> > That's an error condition though; if it happens, the customer will surely
> > remove the module, correct the .wmfw or .bin file, then insert the module
> > again. All we need to do here is make sure that memory does not leak after
> > the module is removed, and device-managed allocation already handles this.
> > 
> 
> 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.

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.

> 
> > > +int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
> > > +			    const struct reg_sequence *reg_seq, int num_regs,
> > > +			    u8 op_code, bool update)
> > > +{
> > > +	int ret, i;
> > > +
> > > +	for (i = 0; i < num_regs; i++) {
> > > +		ret = cs_dsp_wseq_write(dsp, wseq, reg_seq[i].reg,
> > > +					reg_seq[i].def, update, op_code);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > 
> > This is absolutely a nit-pick, but in cs_dsp_wseq_init(), you use the pattern
> > of breaking on error and always returning ret; here you return on error. Both
> > are functionally equivalent but it would be nice to be consistent in terms of
> > style.
> > 
> > If you do opt to match cs_dsp_wseq_multi_write() to cs_dsp_wseq_init(), I do
> > think you'll need to initialize ret to zero here as well to avoid a compiler
> > warning for the num_regs = 0 case.
> 
> I would be inclined to make cs_dsp_wseq_init function like this
> one personally, rather than the other way around. Generally best
> to return if there is no clean up to do.

Makes sense to me.

> 
> Thanks,
> Charles

Kind regards,
Jeff LaBundy