Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Mon, Oct 01, 2007 at 07:27:30PM -0600, Matthew Wilcox wrote: The thing to remember is that sym2 is in transition from being a dual BSD/Linux driver to being a purely Linux driver. I was wondering about that; couldn't tell if the split in the code was historical, or being intentionally maintained. My gut instinct is to say ack, although prudence dictates that I should test first. Which might take a few days... Fine by me. I tested the patch, it worked great. It also seemed to recover much more quickly -- so quickly, in fact, that I thought something had gone wrong. I reviewed it one more time, it really does look good. A formal submission and acked by's at earliest convenience would be good. --linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Mon, Oct 01, 2007 at 07:27:30PM -0600, Matthew Wilcox wrote: Fine by me. Do you have the ability to produce failures on a whim on your platforms? Yes, although it is very platform specific -- there are actually transistors in the pci bridge chip, which actually short out lines, and so, from the point of view of the rest of the chip, it did actually see a real error. Its supposed to be a very realistic test. I've been vaguely musing a PCI device failure patch for x86, just so people can test driver failure paths. That would be good ... I've recently agreed to accept a fedex to test someone elses card for them, which is outside my usual activities. There's also supposed to be some PCI-X riser card out there, (never seen one) which has the ability to inject actual pci errors. Its the Agilent PCI BestX card; I got the impression they might not sell it anymore; dunno. One guy in the lab used to brush a grounding strap across the pins; this usually got a rise out of the audience. --linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Thu, Sep 27, 2007 at 06:34:37PM -0500, Linas Vepstas wrote:
Good catch. But no ... and I had to study this a bit. Bear with me:
I agree with the analysis which I've now snipped.
I think the race you describe above is harmless. The first time
that sym_eh_handler() will run, it will be with SYM_EH_ABORT,
in it doesn't matter if we lose that, since the device is hosed
anyway. At some later time, it will run with SYM_EH_DEVICE_RESET
and then SYM_EH_BUS_RESET and then SYM_EH_HOST_RESET, and we won't
miss those, since, by now, sym2_io_error_detected() will have run.
So, by my reading, I'd say that init_completion() in
sym2_io_error_detected() has to stay (although perhaps
it should be replaced by the INIT_COMPLETION() macro.)
Removing it will prevent correct operation on the second
and subsequent errors.
I think the fundamental problem is that completions aren't really
supposed to be used like this. Here's one attempt at using completions
perhaps a little more the way they're supposed to be used, although now
I've written it, I wonder if we shouldn't just use a waitqueue instead.
diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.c
b/drivers/scsi/sym53c8xx_2/sym_glue.c
index e8a4361..b425b89 100644
--- a/drivers/scsi/sym53c8xx_2/sym_glue.c
+++ b/drivers/scsi/sym53c8xx_2/sym_glue.c
@@ -602,6 +602,7 @@ static int sym_eh_handler(int op, char *opname, struct
scsi_cmnd *cmd)
struct sym_hcb *np = SYM_SOFTC_PTR(cmd);
struct sym_ucmd *ucmd = SYM_UCMD_PTR(cmd);
struct Scsi_Host *host = cmd-device-host;
+ struct pci_dev *pdev = np-s.device;
SYM_QUEHEAD *qp;
int cmd_queued = 0;
int sts = -1;
@@ -616,9 +617,19 @@ static int sym_eh_handler(int op, char *opname, struct
scsi_cmnd *cmd)
* point in hurrying; take a leisurely wait.
*/
#define WAIT_FOR_PCI_RECOVERY 35
- if (pci_channel_offline(np-s.device)) {
- int finished_reset = wait_for_completion_timeout(
- np-s.io_reset_wait, WAIT_FOR_PCI_RECOVERY*HZ);
+ if (pci_channel_offline(pdev)) {
+ struct host_data *hostdata = shost_priv(host);
+ int finished_reset = 0;
+ init_completion(eh_done);
+ spin_lock_irq(host-host_lock);
+ if (!hostdata-io_reset)
+ hostdata-io_reset = eh_done;
+ if (!pci_channel_offline(pdev))
+ finished_reset = 1;
+ spin_unlock_irq(host-host_lock);
+ if (!finished_reset)
+ finished_reset = wait_for_completion_timeout(
+ hostdata-io_reset, WAIT_FOR_PCI_RECOVERY*HZ);
if (!finished_reset)
return SCSI_FAILED;
}
@@ -1396,7 +1407,6 @@ static struct Scsi_Host * __devinit sym_attach(struct
scsi_host_template *tpnt,
np-maxoffs = dev-chip.offset_max;
np-maxburst= dev-chip.burst_max;
np-myaddr = dev-host_id;
- init_completion(np-s.io_reset_wait);
/*
* Edit its name.
@@ -1842,15 +1852,12 @@ static void __devexit sym2_remove(struct pci_dev *pdev)
static pci_ers_result_t sym2_io_error_detected(struct pci_dev *pdev,
enum pci_channel_state state)
{
- struct sym_hcb *np = pci_get_drvdata(pdev);
-
/* If slot is permanently frozen, turn everything off */
if (state == pci_channel_io_perm_failure) {
sym2_remove(pdev);
return PCI_ERS_RESULT_DISCONNECT;
}
- init_completion(np-s.io_reset_wait);
disable_irq(pdev-irq);
pci_disable_device(pdev);
@@ -1912,7 +1919,7 @@ static pci_ers_result_t sym2_io_slot_reset(struct pci_dev
*pdev)
sym_name(np));
if (pci_enable_device(pdev)) {
- printk(KERN_ERR %s: Unable to enable afer PCI reset\n,
+ printk(KERN_ERR %s: Unable to enable after PCI reset\n,
sym_name(np));
return PCI_ERS_RESULT_DISCONNECT;
}
@@ -1953,7 +1960,14 @@ static pci_ers_result_t sym2_io_slot_reset(struct
pci_dev *pdev)
static void sym2_io_resume(struct pci_dev *pdev)
{
struct sym_hcb *np = pci_get_drvdata(pdev);
- complete_all(np-s.io_reset_wait);
+ struct Scsi_Host *shost = np-s.host;
+ struct host_data *hostdata = shost_priv(shost);
+
+ spin_lock_irq(shost-host_lock);
+ if (hostdata-io_reset)
+ complete_all(hostdata-io_reset);
+ hostdata-io_reset = NULL;
+ spin_unlock_irq(shost-host_lock);
}
static void sym2_get_signalling(struct Scsi_Host *shost)
diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.h
b/drivers/scsi/sym53c8xx_2/sym_glue.h
index a172cc5..b961f70 100644
--- a/drivers/scsi/sym53c8xx_2/sym_glue.h
+++ b/drivers/scsi/sym53c8xx_2/sym_glue.h
@@ -180,9 +180,6 @@ struct sym_shcb {
char
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Mon, Oct 01, 2007 at 02:12:47PM -0600, Matthew Wilcox wrote: I think the fundamental problem is that completions aren't really supposed to be used like this. Here's one attempt at using completions perhaps a little more the way they're supposed to be used, Yes, that looks very good to me. I see it solves a bug that I hadn't been quite aware of. I don't understand why struct host_data is preferable to struct sym_shcb (is it because this is the structure that is naturally protectected by the spinlock?) My gut instinct is to say ack, although prudence dictates that I should test first. Which might take a few days... although now I've written it, I wonder if we shouldn't just use a waitqueue instead. I thought that earlier versions of the driver used waitqueues (I vaguely remember eh_wait in the code), which were later converted to completions (I also vaguely recall thinking that the new code was more elegant/simpler). I converted my patch to use the completions likewise, and, as you've clearly shown, did a rather sloppy job in the conversion. I'm tempted to go with this patch; but if you prod, I could attempt a wait-queue based patch. --linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Wed, Sep 26, 2007 at 09:02:16AM -0600, Matthew Wilcox wrote: On Fri, Apr 20, 2007 at 03:47:20PM -0500, Linas Vepstas wrote: Implement the so-called first failure data capture (FFDC) for the symbios PCI error recovery. After a PCI error event is reported, the driver requests that MMIO be enabled. Once enabled, it then reads and dumps assorted status registers, and concludes by requesting the usual reset sequence. + /* Request that MMIO be enabled, so register dump can be taken. */ + return PCI_ERS_RESULT_CAN_RECOVER; +} I'm a little concerned by the mention of MMIO. It's entirely possible for the sym2 driver to be using ioports to access the card rather than MMIO. Is it simply that it can't on the platform you test on? The comment is misleading. I've been in the bad habit of calling it mmio whenever its not DMA. The habit is because there are two distinct enable bits in the pci-host bridge during error recovery: one to enable mmio/ioports, and the other to enable DMA. If the adapter has gone crazy, I don't want to enable DMA, so that it doesn't scribble to bad places. But, by enabling mmio/ioports, perhaps it can be finessed back into a semi-sane state, e.g. sane enough to perform a dump of its internal state. --linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Thu, Sep 27, 2007 at 05:00:22PM -0500, Linas Vepstas wrote: On Wed, Sep 26, 2007 at 09:02:16AM -0600, Matthew Wilcox wrote: I'm a little concerned by the mention of MMIO. It's entirely possible for the sym2 driver to be using ioports to access the card rather than MMIO. Is it simply that it can't on the platform you test on? The comment is misleading. I've been in the bad habit of calling it mmio whenever its not DMA. OK, cool, thanks. I'll update the comment for you. One last thing (sorry, I only just noticed): In the error handler, we wait_for_completion(io_reset_wait). In sym2_io_error_detected, we init_completion(io_reset_wait). Isn't it possible that we hit the error handler before we hit the io_error_detected path, and thus the completion wait is lost? Since the completion is already initialised in sym_attach(), I don't think we need to initialise it in sym2_io_error_detected(). Makes sense to just delete it? -- Intel are signing my paycheques ... these opinions are still mine Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Thu, Sep 27, 2007 at 04:10:31PM -0600, Matthew Wilcox wrote: In the error handler, we wait_for_completion(io_reset_wait). In sym2_io_error_detected, we init_completion(io_reset_wait). Isn't it possible that we hit the error handler before we hit the io_error_detected path, and thus the completion wait is lost? Since the completion is already initialised in sym_attach(), I don't think we need to initialise it in sym2_io_error_detected(). Makes sense to just delete it? Good catch. But no ... and I had to study this a bit. Bear with me: It is enough to call init_completion() once, and not once per use: it initializes spinlocks, which shouldn't be intialized twice. But, that completion might be used multiple times when there are multiple errors, and so, before using it a second time, one must set completion-done = 0. The INIT_COMPLETION() macro does this. One must have completion-done = 0 before every use, as otherwise, wait_for_completion() won't actually wait. And since complete_all() sets x-done += UINT_MAX/2, I'm pretty sure x-done won't be zero the next time we use it, unless we make it so. So I need to find a place to safely call INIT_COMPLETION() again, after the completion has been used. At the moment, I'm stumped as to where to do this. [think ... think ... think] I think the race you describe above is harmless. The first time that sym_eh_handler() will run, it will be with SYM_EH_ABORT, in it doesn't matter if we lose that, since the device is hosed anyway. At some later time, it will run with SYM_EH_DEVICE_RESET and then SYM_EH_BUS_RESET and then SYM_EH_HOST_RESET, and we won't miss those, since, by now, sym2_io_error_detected() will have run. So, by my reading, I'd say that init_completion() in sym2_io_error_detected() has to stay (although perhaps it should be replaced by the INIT_COMPLETION() macro.) Removing it will prevent correct operation on the second and subsequent errors. --Linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Fri, Apr 20, 2007 at 03:47:20PM -0500, Linas Vepstas wrote: Implement the so-called first failure data capture (FFDC) for the symbios PCI error recovery. After a PCI error event is reported, the driver requests that MMIO be enabled. Once enabled, it then reads and dumps assorted status registers, and concludes by requesting the usual reset sequence. + /* Request that MMIO be enabled, so register dump can be taken. */ + return PCI_ERS_RESULT_CAN_RECOVER; +} I'm a little concerned by the mention of MMIO. It's entirely possible for the sym2 driver to be using ioports to access the card rather than MMIO. Is it simply that it can't on the platform you test on? -- Intel are signing my paycheques ... these opinions are still mine Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
