Re: [PATCH 1/4] cxl/mem: Fully decode device capability header

[Ira Weiny]

On Wed, May 19, 2021 at 05:50:44PM -0700, Dan Williams wrote:
> On Thu, May 6, 2021 at 3:37 PM <ira.weiny@xxxxxxxxx> wrote:
> >
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > Previously only the capability ID and offset were decoded.
> >
> > Create a version MASK and decode the additional version and length
> > fields of the header.
> >
> 
> I'm not seeing a justification for why Linux would want this patch?
> 
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > ---
> >  drivers/cxl/core.c | 15 ++++++++++++---
> >  drivers/cxl/cxl.h  |  1 +
> >  2 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index b3c3532b53f7..21553386e218 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -501,12 +501,21 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base,
> >
> >         for (cap = 1; cap <= cap_count; cap++) {
> >                 void __iomem *register_block;
> > -               u32 offset;
> > +               u32 hdr, offset, __maybe_unused length;
> 
> What's the point of reading the length and not using it?

Then length is needed in a future patch and it seemed easier to split by saying
we are just going to decode the entire header as a patch.

Ben caught this too because sparse found the unused variable...

Let me see about squashing this into the future patch.

> If this is
> used in a future patch then wait until then to add it.

ok.

> 
> >                 u16 cap_id;
> > +               u8 version;
> > +
> > +               hdr = readl(base + cap * 0x10);
> > +
> > +               cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, hdr);
> > +               version = FIELD_GET(CXLDEV_CAP_HDR_VERSION_MASK, hdr);
> > +               if (version != 1)
> > +                       dev_err(dev, "Vendor cap ID: %x incorrect version (0x%x)\n",
> > +                               cap_id, version);
> 
> It's not an error. Any future version needs to be backwards
> compatible. All this is doing is ensuring that when hardware is
> updated old kernels will start spamming the log.

Yep...

After deleting this I think I should just 'forward squash' the length into the
patch where I need it.

Ira

> 
> >
> > -               cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
> > -                                  readl(base + cap * 0x10));
> >                 offset = readl(base + cap * 0x10 + 0x4);
> > +               length = readl(base + cap * 0x10 + 0x8);
> > +
> >                 register_block = base + offset;
> >
> >                 switch (cap_id) {
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 0211f44c95a2..9b315c069557 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -15,6 +15,7 @@
> >  #define   CXLDEV_CAP_ARRAY_COUNT_MASK GENMASK_ULL(47, 32)
> >  /* CXL 2.0 8.2.8.2 CXL Device Capability Header Register */
> >  #define CXLDEV_CAP_HDR_CAP_ID_MASK GENMASK(15, 0)
> > +#define CXLDEV_CAP_HDR_VERSION_MASK GENMASK(23, 16)
> >  /* CXL 2.0 8.2.8.2.1 CXL Device Capabilities */
> >  #define CXLDEV_CAP_CAP_ID_DEVICE_STATUS 0x1
> >  #define CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX 0x2
> > --
> > 2.28.0.rc0.12.gb6a658bd00c9
> >