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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2bf4a3a8f34e71aa2b2be2b88fe22f58004eb699.camel@intel.com>
Date:   Fri, 19 May 2023 20:24:13 +0000
From:   "Verma, Vishal L" <vishal.l.verma@...el.com>
To:     "Schofield, Alison" <alison.schofield@...el.com>
CC:     "Jiang, Dave" <dave.jiang@...el.com>,
        "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "Weight, Russell H" <russell.h.weight@...el.com>,
        "bwidawsk@...nel.org" <bwidawsk@...nel.org>,
        "dave@...olabs.net" <dave@...olabs.net>,
        "Weiny, Ira" <ira.weiny@...el.com>
Subject: Re: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs
 firmware loader

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!