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

[Verma, Vishal L]

On Thu, 2023-05-18 at 19:58 -0700, Alison Schofield wrote:
> > 
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 8c3302fc7738..0ecee5b558f4 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> 
> snip
> 
> > + * Get Firmware Info
> > + * CXL rev 3.0 section 8.2.9.3.1; Table 8-56
> > + */
> > +struct cxl_mbox_get_fw_info {
> > +       u8 num_slots;
> > +       u8 slot_info;
> > +       u8 activation_cap;
> > +       u8 reserved[13];
> > +       char slot_1_revision[0x10];
> > +       char slot_2_revision[0x10];
> > +       char slot_3_revision[0x10];
> > +       char slot_4_revision[0x10];
> 
> The practice here is to use decimals [16]

I looked around, and see cxl_identify, cxl_event_mem_module, and
cxl_event_dram all have a few hex ones thrown in each. So I looked to
the spec to see if there was any consistency there, and unfortunately
it isn't either. The spec does say 'Length in bytes' 16 for these fw
revision fields though, so I think it makes sense changing these to
decimal..

> 
> > +} __packed;
> > +
> 
> snip
> 
> > + * Transfer Firmware Input Payload
> > + * CXL rev 3.0 section 8.2.9.3.2; Table 8-57
> > + */
> > +struct cxl_mbox_transfer_fw {
> > +       u8 action;
> > +       u8 slot;
> > +       u8 reserved[2];
> > +       __le32 offset;
> > +       u8 reserved2[0x78];
> 
> Here too.

..however for this one the spec has '78h'. I think we should just match
the spec in these cases so anyone cross referencing the header and spec
is saved from doing a bunch of math for these.

> 
> That's all for now.
> 
Thanks for taking a look!