Re: [PATCH v3 29/40] cxl/pci: Implement wait for media active

[Dan Williams <dan.j.williams@intel.com>]

On Mon, Jan 31, 2022 at 10:30 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sun, 23 Jan 2022 16:31:13 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > From: Ben Widawsky <ben.widawsky@intel.com>
> >
> > CXL 2.0 8.1.3.8.2 states:
> >
> >   Memory_Active: When set, indicates that the CXL Range 1 memory is
> >   fully initialized and available for software use. Must be set within
> >   Range 1. Memory_Active_Timeout of deassertion of reset to CXL device
> >   if CXL.mem HwInit Mode=1
> >
> > Unfortunately, Memory_Active can take quite a long time depending on
> > media size (up to 256s per 2.0 spec). Provide a callback for the
> > eventual establishment of CXL.mem operations via the 'cxl_mem' driver
> > the 'struct cxl_memdev'. The implementation waits for 60s by default for
> > now and can be overridden by the mbox_ready_time module parameter.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > [djbw: switch to sleeping wait]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Not being a memory device person, I'm not sure whether my query below
> is realistic but I worry a little that minimum sleep if not immediately
> ready of 1 second is a bit long.

Perhaps, but I think the chance of getting to this point is slim in
the common case where platform firmware has already done CXL memory
init.

> Perhaps that's something to optimize once there are a large number
> of implementations to assess if it is worth bothering or not.

Sounds good.

>
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
>
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 5c43886dc2af..513cb0e2a70a 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -49,7 +49,7 @@
> >  static unsigned short mbox_ready_timeout = 60;
> >  module_param(mbox_ready_timeout, ushort, 0600);
> >  MODULE_PARM_DESC(mbox_ready_timeout,
> > -              "seconds to wait for mailbox ready status");
> > +              "seconds to wait for mailbox ready / memory active status");
> >
> >  static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
> >  {
> > @@ -417,6 +417,51 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
> >       return -ETIMEDOUT;
> >  }
> >
> > +/*
> > + * Wait up to @mbox_ready_timeout for the device to report memory
> > + * active.
> > + */
> > +static int wait_for_media_ready(struct cxl_dev_state *cxlds)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> > +     int d = cxlds->cxl_dvsec;
> > +     bool active = false;
> > +     u64 md_status;
> > +     int rc, i;
> > +
> > +     rc = wait_for_valid(cxlds);
> > +     if (rc)
> > +             return rc;
> > +
> > +     for (i = mbox_ready_timeout; i; i--) {
> > +             u32 temp;
> > +             int rc;
> > +
> > +             rc = pci_read_config_dword(
> > +                     pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp);
> > +             if (rc)
> > +                     return rc;
> > +
> > +             active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp);
> > +             if (active)
> > +                     break;
> > +             msleep(1000);
> Whilst it can be a while, this seems a bit of an excessive step to me.
> If the thing is ready in 10msecs we stil end up waiting a second.
> Might be worth checking more often, or doing some sort of fall off
> in frequency of checking.

I dunno, when the minimum hardware precision in the spec is 1 second
it's not clear that the driver can do better than this in practice.
Let's see what real platforms do. Part of me also thinks that this is
an incentive for devices to get ready before the OS might penalize
them with a coarse wait.