| Message ID | 20211129214721.1668325-4-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL port prep work | expand |
On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > The Memory Device Status register is inspected in the same way for at > least two flows in the CXL Type 3 Memory Device Software Guide > (Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence, > and 2.13.10 Media ready sequence. Extract this common functionality for > use by both. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/pci.c | 33 +++++++++++++++++++++++++-------- > 1 file changed, 25 insertions(+), 8 deletions(-) > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index a6ea9811a05b..6c8d09fb3a17 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds, > return 0; > } > > +/* > + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory > + * Device Software Guide > + */ This version did not address the feedback about referencing the CXL specification instead of the software guide. > +static int check_device_status(struct cxl_dev_state *cxlds) > +{ > + const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET); > + > + if (md_status & CXLMDEV_DEV_FATAL) { > + dev_err(cxlds->dev, "Fatal: replace device\n"); > + return -EIO; > + } > + > + if (md_status & CXLMDEV_FW_HALT) { > + dev_err(cxlds->dev, "FWHalt: reset or replace device\n"); > + return -EBUSY; > + } > + > + return 0; > +} > + > /** > * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox. > * @cxlds: The device state to gain access to. > @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds) > * Hardware shouldn't allow a ready status but also have failure bits > * set. Spit out an error, this should be a bug report > */ > - rc = -EFAULT; > - if (md_status & CXLMDEV_DEV_FATAL) { > - dev_err(dev, "mbox: reported ready, but fatal\n"); > + rc = check_device_status(cxlds); > + if (rc) > goto out; > - } > - if (md_status & CXLMDEV_FW_HALT) { > - dev_err(dev, "mbox: reported ready, but halted\n"); > - goto out; > - } > + > if (CXLMDEV_RESET_NEEDED(md_status)) { > dev_err(dev, "mbox: reported ready, but reset needed\n"); > + rc = -EFAULT; > goto out; It also did not address the feedback that the reset needed check can be dropped because per the spec* it is redundant with the other checks above. The driver need not give up on mailbox commands just because the media is not ready. So this wants a lead-in patch to clean that up first. "This field returns non-zero value if FW Halt is set or Media Status is not in the ready state." Going further I do not think the driver should preemptively abort commands due to device-fatal, or even firmware halt for that matter. The true sign of failure and dead firmware is timed-out responses to incoming commands. Just in case there are some commands still functional in a device-fatal condition, or the firmware-halt indicator was a false positive, let the command through, and then if it fails report these forensic statuses. "Mailbox Interfaces Ready" is the only status that needs to be checked preemptively, and only once at the beginning of time until the next command timeout where the driver collects it to find out what happened. I'll take a shot at a lead-in patch to that effect.
On 21-12-02 09:09:44, Dan Williams wrote: > On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > The Memory Device Status register is inspected in the same way for at > > least two flows in the CXL Type 3 Memory Device Software Guide > > (Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence, > > and 2.13.10 Media ready sequence. Extract this common functionality for > > use by both. > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > drivers/cxl/pci.c | 33 +++++++++++++++++++++++++-------- > > 1 file changed, 25 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index a6ea9811a05b..6c8d09fb3a17 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds, > > return 0; > > } > > > > +/* > > + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory > > + * Device Software Guide > > + */ > > This version did not address the feedback about referencing the CXL > specification instead of the software guide. > Apologies, I missed that request. I apparently missed your email entirely as I was going through the revision. > > +static int check_device_status(struct cxl_dev_state *cxlds) > > +{ > > + const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET); > > + > > + if (md_status & CXLMDEV_DEV_FATAL) { > > + dev_err(cxlds->dev, "Fatal: replace device\n"); > > + return -EIO; > > + } > > + > > + if (md_status & CXLMDEV_FW_HALT) { > > + dev_err(cxlds->dev, "FWHalt: reset or replace device\n"); > > + return -EBUSY; > > + } > > + > > + return 0; > > +} > > + > > /** > > * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox. > > * @cxlds: The device state to gain access to. > > @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds) > > * Hardware shouldn't allow a ready status but also have failure bits > > * set. Spit out an error, this should be a bug report > > */ > > - rc = -EFAULT; > > - if (md_status & CXLMDEV_DEV_FATAL) { > > - dev_err(dev, "mbox: reported ready, but fatal\n"); > > + rc = check_device_status(cxlds); > > + if (rc) > > goto out; > > - } > > - if (md_status & CXLMDEV_FW_HALT) { > > - dev_err(dev, "mbox: reported ready, but halted\n"); > > - goto out; > > - } > > + > > if (CXLMDEV_RESET_NEEDED(md_status)) { > > dev_err(dev, "mbox: reported ready, but reset needed\n"); > > + rc = -EFAULT; > > goto out; > > It also did not address the feedback that the reset needed check can > be dropped because per the spec* it is redundant with the other checks > above. The driver need not give up on mailbox commands just because > the media is not ready. So this wants a lead-in patch to clean that up > first. Media ready droppage comes in the next patch. I can reorder them if you'd like, or I suppose you could do it. > > "This field returns non-zero value if FW Halt is set or Media Status > is not in the ready state." > > Going further I do not think the driver should preemptively abort > commands due to device-fatal, or even firmware halt for that matter. > The true sign of failure and dead firmware is timed-out responses to > incoming commands. Just in case there are some commands still > functional in a device-fatal condition, or the firmware-halt indicator > was a false positive, let the command through, and then if it fails > report these forensic statuses. I admit I'm not looking for spec reference at this exact moment. I think trying to throw commands at the device that has already told us is fatal, is a bad idea. > > "Mailbox Interfaces Ready" is the only status that needs to be checked > preemptively, and only once at the beginning of time until the next > command timeout where the driver collects it to find out what > happened. I think this happens in a later patch too, if I catch your meaning. > > I'll take a shot at a lead-in patch to that effect.
On Thu, Dec 2, 2021 at 9:24 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > On 21-12-02 09:09:44, Dan Williams wrote: > > On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > The Memory Device Status register is inspected in the same way for at > > > least two flows in the CXL Type 3 Memory Device Software Guide > > > (Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence, > > > and 2.13.10 Media ready sequence. Extract this common functionality for > > > use by both. > > > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > --- > > > drivers/cxl/pci.c | 33 +++++++++++++++++++++++++-------- > > > 1 file changed, 25 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > > index a6ea9811a05b..6c8d09fb3a17 100644 > > > --- a/drivers/cxl/pci.c > > > +++ b/drivers/cxl/pci.c > > > @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds, > > > return 0; > > > } > > > > > > +/* > > > + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory > > > + * Device Software Guide > > > + */ > > > > This version did not address the feedback about referencing the CXL > > specification instead of the software guide. > > > > Apologies, I missed that request. I apparently missed your email entirely as I > was going through the revision. > > > > +static int check_device_status(struct cxl_dev_state *cxlds) > > > +{ > > > + const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET); > > > + > > > + if (md_status & CXLMDEV_DEV_FATAL) { > > > + dev_err(cxlds->dev, "Fatal: replace device\n"); > > > + return -EIO; > > > + } > > > + > > > + if (md_status & CXLMDEV_FW_HALT) { > > > + dev_err(cxlds->dev, "FWHalt: reset or replace device\n"); > > > + return -EBUSY; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox. > > > * @cxlds: The device state to gain access to. > > > @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds) > > > * Hardware shouldn't allow a ready status but also have failure bits > > > * set. Spit out an error, this should be a bug report > > > */ > > > - rc = -EFAULT; > > > - if (md_status & CXLMDEV_DEV_FATAL) { > > > - dev_err(dev, "mbox: reported ready, but fatal\n"); > > > + rc = check_device_status(cxlds); > > > + if (rc) > > > goto out; > > > - } > > > - if (md_status & CXLMDEV_FW_HALT) { > > > - dev_err(dev, "mbox: reported ready, but halted\n"); > > > - goto out; > > > - } > > > + > > > if (CXLMDEV_RESET_NEEDED(md_status)) { > > > dev_err(dev, "mbox: reported ready, but reset needed\n"); > > > + rc = -EFAULT; > > > goto out; > > > > It also did not address the feedback that the reset needed check can > > be dropped because per the spec* it is redundant with the other checks > > above. The driver need not give up on mailbox commands just because > > the media is not ready. So this wants a lead-in patch to clean that up > > first. > > Media ready droppage comes in the next patch. I can reorder them if you'd like, > or I suppose you could do it. Sure, I'll reorder. > > > > > "This field returns non-zero value if FW Halt is set or Media Status > > is not in the ready state." > > > > Going further I do not think the driver should preemptively abort > > commands due to device-fatal, or even firmware halt for that matter. > > The true sign of failure and dead firmware is timed-out responses to > > incoming commands. Just in case there are some commands still > > functional in a device-fatal condition, or the firmware-halt indicator > > was a false positive, let the command through, and then if it fails > > report these forensic statuses. > > I admit I'm not looking for spec reference at this exact moment. I think trying > to throw commands at the device that has already told us is fatal, is a bad > idea. It is always going to be a race condition. The driver can check preemptively all it wants, but inevitably there will be cases where the device goes fatal immediately after checking. So, executing commands against a device with an error status set cannot be prevented. I am asking to just delete the soft hopeful code and use the hard timeout fallback exclusively. > > "Mailbox Interfaces Ready" is the only status that needs to be checked > > preemptively, and only once at the beginning of time until the next > > command timeout where the driver collects it to find out what > > happened. > > I think this happens in a later patch too, if I catch your meaning. Yeah, looks like that patch needs to come first and then a patch to switch to timeout-only error handling.
On 21-12-02 09:32:59, Dan Williams wrote: > On Thu, Dec 2, 2021 at 9:24 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > On 21-12-02 09:09:44, Dan Williams wrote: > > > On Mon, Nov 29, 2021 at 1:47 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > > The Memory Device Status register is inspected in the same way for at > > > > least two flows in the CXL Type 3 Memory Device Software Guide > > > > (Revision: 1.0): 2.13.9 Device discovery and mailbox ready sequence, > > > > and 2.13.10 Media ready sequence. Extract this common functionality for > > > > use by both. > > > > > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > > --- > > > > drivers/cxl/pci.c | 33 +++++++++++++++++++++++++-------- > > > > 1 file changed, 25 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > > > index a6ea9811a05b..6c8d09fb3a17 100644 > > > > --- a/drivers/cxl/pci.c > > > > +++ b/drivers/cxl/pci.c > > > > @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds, > > > > return 0; > > > > } > > > > > > > > +/* > > > > + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory > > > > + * Device Software Guide > > > > + */ > > > > > > This version did not address the feedback about referencing the CXL > > > specification instead of the software guide. > > > > > > > Apologies, I missed that request. I apparently missed your email entirely as I > > was going through the revision. > > > > > > +static int check_device_status(struct cxl_dev_state *cxlds) > > > > +{ > > > > + const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET); > > > > + > > > > + if (md_status & CXLMDEV_DEV_FATAL) { > > > > + dev_err(cxlds->dev, "Fatal: replace device\n"); > > > > + return -EIO; > > > > + } > > > > + > > > > + if (md_status & CXLMDEV_FW_HALT) { > > > > + dev_err(cxlds->dev, "FWHalt: reset or replace device\n"); > > > > + return -EBUSY; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /** > > > > * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox. > > > > * @cxlds: The device state to gain access to. > > > > @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds) > > > > * Hardware shouldn't allow a ready status but also have failure bits > > > > * set. Spit out an error, this should be a bug report > > > > */ > > > > - rc = -EFAULT; > > > > - if (md_status & CXLMDEV_DEV_FATAL) { > > > > - dev_err(dev, "mbox: reported ready, but fatal\n"); > > > > + rc = check_device_status(cxlds); > > > > + if (rc) > > > > goto out; > > > > - } > > > > - if (md_status & CXLMDEV_FW_HALT) { > > > > - dev_err(dev, "mbox: reported ready, but halted\n"); > > > > - goto out; > > > > - } > > > > + > > > > if (CXLMDEV_RESET_NEEDED(md_status)) { > > > > dev_err(dev, "mbox: reported ready, but reset needed\n"); > > > > + rc = -EFAULT; > > > > goto out; > > > > > > It also did not address the feedback that the reset needed check can > > > be dropped because per the spec* it is redundant with the other checks > > > above. The driver need not give up on mailbox commands just because > > > the media is not ready. So this wants a lead-in patch to clean that up > > > first. > > > > Media ready droppage comes in the next patch. I can reorder them if you'd like, > > or I suppose you could do it. > > Sure, I'll reorder. > > > > > > > > > "This field returns non-zero value if FW Halt is set or Media Status > > > is not in the ready state." > > > > > > Going further I do not think the driver should preemptively abort > > > commands due to device-fatal, or even firmware halt for that matter. > > > The true sign of failure and dead firmware is timed-out responses to > > > incoming commands. Just in case there are some commands still > > > functional in a device-fatal condition, or the firmware-halt indicator > > > was a false positive, let the command through, and then if it fails > > > report these forensic statuses. > > > > I admit I'm not looking for spec reference at this exact moment. I think trying > > to throw commands at the device that has already told us is fatal, is a bad > > idea. > > It is always going to be a race condition. The driver can check > preemptively all it wants, but inevitably there will be cases where > the device goes fatal immediately after checking. So, executing > commands against a device with an error status set cannot be > prevented. I am asking to just delete the soft hopeful code and use > the hard timeout fallback exclusively. > It doesn't bother me much to do so, but it's not my preference. If you'd like to change it, go ahead and I will review it. When I wrote the original implementation, my idea was that command timeout and error conditions driven by interrupts (media issues, firmware issues, whatever) would be disparate paths. My belief is the spec allows so much latitude, extra paranoia makes sense. > > > "Mailbox Interfaces Ready" is the only status that needs to be checked > > > preemptively, and only once at the beginning of time until the next > > > command timeout where the driver collects it to find out what > > > happened. > > > > I think this happens in a later patch too, if I catch your meaning. > > Yeah, looks like that patch needs to come first and then a patch to > switch to timeout-only error handling.
On Fri, Dec 3, 2021 at 5:18 PM Ben Widawsky <ben.widawsky@intel.com> wrote: [..] > > > > It doesn't bother me much to do so, but it's not my preference. If you'd like to > change it, go ahead and I will review it. When I wrote the original > implementation, my idea was that command timeout and error conditions driven by > interrupts (media issues, firmware issues, whatever) would be disparate paths. > My belief is the spec allows so much latitude, extra paranoia makes sense. I think device drivers are where the "only the paranoid survive" mantra falls apart. Paranoid drivers fall over earlier than necessary. I just picture Dave trying to disable HAL 9000, but can't even attempt to send the mailbox command because the CXL device's media disabled status is set.
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index a6ea9811a05b..6c8d09fb3a17 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -182,6 +182,27 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds, return 0; } +/* + * Implements roughly the bottom half of Figure 42 of the CXL Type 3 Memory + * Device Software Guide + */ +static int check_device_status(struct cxl_dev_state *cxlds) +{ + const u64 md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET); + + if (md_status & CXLMDEV_DEV_FATAL) { + dev_err(cxlds->dev, "Fatal: replace device\n"); + return -EIO; + } + + if (md_status & CXLMDEV_FW_HALT) { + dev_err(cxlds->dev, "FWHalt: reset or replace device\n"); + return -EBUSY; + } + + return 0; +} + /** * cxl_pci_mbox_get() - Acquire exclusive access to the mailbox. * @cxlds: The device state to gain access to. @@ -231,17 +252,13 @@ static int cxl_pci_mbox_get(struct cxl_dev_state *cxlds) * Hardware shouldn't allow a ready status but also have failure bits * set. Spit out an error, this should be a bug report */ - rc = -EFAULT; - if (md_status & CXLMDEV_DEV_FATAL) { - dev_err(dev, "mbox: reported ready, but fatal\n"); + rc = check_device_status(cxlds); + if (rc) goto out; - } - if (md_status & CXLMDEV_FW_HALT) { - dev_err(dev, "mbox: reported ready, but halted\n"); - goto out; - } + if (CXLMDEV_RESET_NEEDED(md_status)) { dev_err(dev, "mbox: reported ready, but reset needed\n"); + rc = -EFAULT; goto out; }