Re: [PATCH v2 1/4] cxl: add a firmware update mechanism using the sysfs firmware loader

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Thu, 2023-06-08 at 15:49 +0100, Jonathan Cameron wrote:
> 
<..>
> > +
> > +static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
> > +                                      u32 offset, u32 size, u32 *written)
> > +{
> > +       struct cxl_dev_state *cxlds = fwl->dd_handle;
> > +       struct cxl_memdev *cxlmd = cxlds->cxlmd;
> > +       struct cxl_mbox_transfer_fw *transfer;
> > +       struct cxl_mbox_cmd mbox_cmd;
> > +       u32 cur_size, remaining;
> > +       size_t size_in;
> > +       int rc;
> > +
> > +       *written = 0;
> > +
> > +       /* Offset has to be aligned to 128B (CXL-3.0 8.2.9.3.2 Table 8-57) */
> > +       if (!IS_ALIGNED(offset, CXL_FW_TRANSFER_ALIGNMENT)) {
> > +               dev_err(&cxlmd->dev,
> > +                       "misaligned offset for FW transfer slice (%u)\n",
> > +                       offset);
> > +               return FW_UPLOAD_ERR_RW_ERROR;
> > +       }
> > +
> > +       /* Pick transfer size based on cxlds->payload_size */
> > +       cur_size = min_t(size_t, size, cxlds->payload_size - sizeof(*transfer));
> 
> If size > cxlds->payload_size - sizeof(*transfer) what ensures that the step
> we take forwards results in the next read having an offset that is 128B aligned?
> 
> I think cur_size needs to be forced to be a multiple of 128Bytes as well.

The fact that sizeof(*transfer) is 128 bytes, and payload_size is a
power of 2 starting with 256 should ensure alignment. Dan noted this
here, before which I did force alignment explicitly:

https://lore.kernel.org/linux-cxl/646c313f20907_33fb329412@dwillia2-xfh.jf.intel.com.notmuch/

This probably deserves a comment though - I'll add that.

> 
<..>

> > +
> > +int cxl_memdev_setup_fw_upload(struct cxl_dev_state *cxlds)
> > +{
> > +       struct cxl_memdev *cxlmd = cxlds->cxlmd;
> 
> cxlmd.dev is only thing used, so I'd have a local variable
> for that instead of cxlmd.
> 
> 
> > +       struct fw_upload *fwl;
> > +       int rc;
> > +
> > +       if (!test_bit(CXL_MEM_COMMAND_ID_GET_FW_INFO, cxlds->enabled_cmds))
> > +               return 0;
> > +
> > +       fwl = firmware_upload_register(THIS_MODULE, &cxlmd->dev,
> > +                                      dev_name(&cxlmd->dev),
> > +                                      &cxl_memdev_fw_ops, cxlds);
> > +       if (IS_ERR(fwl)) {
> > +               dev_err(&cxlmd->dev, "Failed to register firmware loader\n");
> > +               return PTR_ERR(fwl);
> 
> It's called from probe only so could use dev_err_probe() for slight
> simplification.

From what I can tell, this ends up looking like:

	fwl = firmware_upload_register(THIS_MODULE, dev,
dev_name(dev),
				       &cxl_memdev_fw_ops, cxlds);
	rc = dev_err_probe(dev, PTR_ERR(fwl),
			   "Failed to register firmware loader\n");
	if (rc)
		return rc;

Is that what you meant? Happy to make the change if so.

> 
> > @@ -581,7 +888,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
> >  
> >         rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
> >         if (rc)
> > -               return ERR_PTR(rc);
> > +               goto err;
> 
> Why is this change here?   Fairly sure it results in a duplicate release.

Ah yep I think an artifact from the previous rev where I had the fw
setup happening in this function.

Also agree with all other comments that I didn't address, making those
changes for v3.

Thanks for the review!