Messages in this thread Patch in this message | | | From | "Verma, Vishal L" <> | Subject | Re: [PATCH v2 1/4] cxl: add a firmware update mechanism using the sysfs firmware loader | Date | Thu, 8 Jun 2023 20:15:48 +0000 |
| |
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!
| |