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

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

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.

> > +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.

Thanks,
Charles