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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Mon, Oct 01, 2007 at 05:41:32PM -0500, Linas Vepstas wrote: > 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?) The thing to remember is that sym2 is in transition from being a dual BSD/Linux driver to being a purely Linux driver. I know, it's been more than two years, and I'm still not done. My latest thing I'm looking at fixing is to make Scsi_Host very much the primary entity in the sym2 driver, and leave just the device/scripts-accessible stuff in the hcb. > My gut instinct is to say "ack", although prudence dictates that > I should test first. Which might take a few days... Fine by me. Do you have the ability to produce failures on a whim on your platforms? I've been vaguely musing a PCI device failure patch for x86, just so people can test driver failure paths. > 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. eh_wait (when I removed it) contained a completion ... I think it used to be a semaphore, some time before 2.6.12 > I'm tempted to go with this patch; but if you prod, I could attempt > a wait-queue based patch. Let's leave it with a completion for now. I think it works out more efficiently in the end. -- 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." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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(
- >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(_done);
+ spin_lock_irq(host->host_lock);
+ if (!hostdata->io_reset)
+ hostdata->io_reset = _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(>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(>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(>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 {
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Mon, Oct 01, 2007 at 05:41:32PM -0500, Linas Vepstas wrote: 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?) The thing to remember is that sym2 is in transition from being a dual BSD/Linux driver to being a purely Linux driver. I know, it's been more than two years, and I'm still not done. My latest thing I'm looking at fixing is to make Scsi_Host very much the primary entity in the sym2 driver, and leave just the device/scripts-accessible stuff in the hcb. My gut instinct is to say ack, although prudence dictates that I should test first. Which might take a few days... Fine by me. Do you have the ability to produce failures on a whim on your platforms? I've been vaguely musing a PCI device failure patch for x86, just so people can test driver failure paths. 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. eh_wait (when I removed it) contained a completion ... I think it used to be a semaphore, some time before 2.6.12 I'm tempted to go with this patch; but if you prod, I could attempt a wait-queue based patch. Let's leave it with a completion for now. I think it works out more efficiently in the end. -- 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Wed, May 09, 2007 at 03:26:21PM -0500, Linas Vepstas wrote: > Hi Matthew, > > I had been hoping these patches might make it into 2.6.22, > ... this is a nag note; please forward upstream. ... should I repost the patches? --linas - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
On Wed, May 09, 2007 at 03:26:21PM -0500, Linas Vepstas wrote: Hi Matthew, I had been hoping these patches might make it into 2.6.22, ... this is a nag note; please forward upstream. ... should I repost the patches? --linas - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
Hi Matthew, I had been hoping these patches might make it into 2.6.22, ... this is a nag note; please forward upstream. --linas 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. > > (includes a whitespace fix for bad indentation). > > Signed-off-by: Linas Vepstas <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
Hi Matthew, I had been hoping these patches might make it into 2.6.22, ... this is a nag note; please forward upstream. --linas 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. (includes a whitespace fix for bad indentation). Signed-off-by: Linas Vepstas [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
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.
(includes a whitespace fix for bad indentation).
Signed-off-by: Linas Vepstas <[EMAIL PROTECTED]>
drivers/scsi/sym53c8xx_2/sym_glue.c | 15 +++
drivers/scsi/sym53c8xx_2/sym_glue.h |1 +
drivers/scsi/sym53c8xx_2/sym_hipd.c | 18 ++
3 files changed, 30 insertions(+), 4 deletions(-)
Index: linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_glue.c
===
--- linux-2.6.21-rc4-git4.orig/drivers/scsi/sym53c8xx_2/sym_glue.c
2007-04-20 12:52:01.0 -0500
+++ linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_glue.c 2007-04-20
15:25:35.0 -0500
@@ -1987,6 +1987,20 @@ static pci_ers_result_t sym2_io_error_de
disable_irq(pdev->irq);
pci_disable_device(pdev);
+ /* Request that MMIO be enabled, so register dump can be taken. */
+ return PCI_ERS_RESULT_CAN_RECOVER;
+}
+
+/**
+ * sym2_io_slot_dump -- Enable MMIO and dump debug registers
+ * @pdev: pointer to PCI device
+ */
+static pci_ers_result_t sym2_io_slot_dump (struct pci_dev *pdev)
+{
+ struct sym_hcb *np = pci_get_drvdata(pdev);
+
+ sym_dump_registers(np);
+
/* Request a slot reset. */
return PCI_ERS_RESULT_NEED_RESET;
}
@@ -2241,6 +2255,7 @@ MODULE_DEVICE_TABLE(pci, sym2_id_table);
static struct pci_error_handlers sym2_err_handler = {
.error_detected = sym2_io_error_detected,
+ .mmio_enabled = sym2_io_slot_dump,
.slot_reset = sym2_io_slot_reset,
.resume = sym2_io_resume,
};
Index: linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_glue.h
===
--- linux-2.6.21-rc4-git4.orig/drivers/scsi/sym53c8xx_2/sym_glue.h
2007-04-20 12:15:07.0 -0500
+++ linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_glue.h 2007-04-20
15:21:31.0 -0500
@@ -270,5 +270,6 @@ void sym_xpt_async_bus_reset(struct sym_
void sym_xpt_async_sent_bdr(struct sym_hcb *np, int target);
int sym_setup_data_and_start (struct sym_hcb *np, struct scsi_cmnd *csio,
struct sym_ccb *cp);
void sym_log_bus_error(struct sym_hcb *np);
+void sym_dump_registers(struct sym_hcb *np);
#endif /* SYM_GLUE_H */
Index: linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_hipd.c
===
--- linux-2.6.21-rc4-git4.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c
2007-04-20 12:18:59.0 -0500
+++ linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_hipd.c 2007-04-20
15:18:01.0 -0500
@@ -1180,10 +1180,10 @@ static void sym_log_hard_error(struct sy
scr_to_cpu((int) *(u32 *)(script_base + script_ofs)));
}
-printf ("%s: regdump:", sym_name(np));
-for (i=0; i<24;i++)
-printf (" %02x", (unsigned)INB_OFF(np, i));
-printf (".\n");
+ printf ("%s: regdump:", sym_name(np));
+ for (i=0; i<24;i++)
+ printf (" %02x", (unsigned)INB_OFF(np, i));
+ printf (".\n");
/*
* PCI BUS error.
@@ -1192,6 +1192,16 @@ static void sym_log_hard_error(struct sy
sym_log_bus_error(np);
}
+void sym_dump_registers(struct sym_hcb *np)
+{
+ u_short sist;
+ u_char dstat;
+
+ sist = INW(np, nc_sist);
+ dstat = INB(np, nc_dstat);
+ sym_log_hard_error(np, sist, dstat);
+}
+
static struct sym_chip sym_dev_table[] = {
{PCI_DEVICE_ID_NCR_53C810, 0x0f, "810", 4, 8, 4, 64,
FE_ERL}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2]: PCI Error Recovery: Symbios SCSI First Failure
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.
(includes a whitespace fix for bad indentation).
Signed-off-by: Linas Vepstas [EMAIL PROTECTED]
drivers/scsi/sym53c8xx_2/sym_glue.c | 15 +++
drivers/scsi/sym53c8xx_2/sym_glue.h |1 +
drivers/scsi/sym53c8xx_2/sym_hipd.c | 18 ++
3 files changed, 30 insertions(+), 4 deletions(-)
Index: linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_glue.c
===
--- linux-2.6.21-rc4-git4.orig/drivers/scsi/sym53c8xx_2/sym_glue.c
2007-04-20 12:52:01.0 -0500
+++ linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_glue.c 2007-04-20
15:25:35.0 -0500
@@ -1987,6 +1987,20 @@ static pci_ers_result_t sym2_io_error_de
disable_irq(pdev-irq);
pci_disable_device(pdev);
+ /* Request that MMIO be enabled, so register dump can be taken. */
+ return PCI_ERS_RESULT_CAN_RECOVER;
+}
+
+/**
+ * sym2_io_slot_dump -- Enable MMIO and dump debug registers
+ * @pdev: pointer to PCI device
+ */
+static pci_ers_result_t sym2_io_slot_dump (struct pci_dev *pdev)
+{
+ struct sym_hcb *np = pci_get_drvdata(pdev);
+
+ sym_dump_registers(np);
+
/* Request a slot reset. */
return PCI_ERS_RESULT_NEED_RESET;
}
@@ -2241,6 +2255,7 @@ MODULE_DEVICE_TABLE(pci, sym2_id_table);
static struct pci_error_handlers sym2_err_handler = {
.error_detected = sym2_io_error_detected,
+ .mmio_enabled = sym2_io_slot_dump,
.slot_reset = sym2_io_slot_reset,
.resume = sym2_io_resume,
};
Index: linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_glue.h
===
--- linux-2.6.21-rc4-git4.orig/drivers/scsi/sym53c8xx_2/sym_glue.h
2007-04-20 12:15:07.0 -0500
+++ linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_glue.h 2007-04-20
15:21:31.0 -0500
@@ -270,5 +270,6 @@ void sym_xpt_async_bus_reset(struct sym_
void sym_xpt_async_sent_bdr(struct sym_hcb *np, int target);
int sym_setup_data_and_start (struct sym_hcb *np, struct scsi_cmnd *csio,
struct sym_ccb *cp);
void sym_log_bus_error(struct sym_hcb *np);
+void sym_dump_registers(struct sym_hcb *np);
#endif /* SYM_GLUE_H */
Index: linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_hipd.c
===
--- linux-2.6.21-rc4-git4.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c
2007-04-20 12:18:59.0 -0500
+++ linux-2.6.21-rc4-git4/drivers/scsi/sym53c8xx_2/sym_hipd.c 2007-04-20
15:18:01.0 -0500
@@ -1180,10 +1180,10 @@ static void sym_log_hard_error(struct sy
scr_to_cpu((int) *(u32 *)(script_base + script_ofs)));
}
-printf (%s: regdump:, sym_name(np));
-for (i=0; i24;i++)
-printf ( %02x, (unsigned)INB_OFF(np, i));
-printf (.\n);
+ printf (%s: regdump:, sym_name(np));
+ for (i=0; i24;i++)
+ printf ( %02x, (unsigned)INB_OFF(np, i));
+ printf (.\n);
/*
* PCI BUS error.
@@ -1192,6 +1192,16 @@ static void sym_log_hard_error(struct sy
sym_log_bus_error(np);
}
+void sym_dump_registers(struct sym_hcb *np)
+{
+ u_short sist;
+ u_char dstat;
+
+ sist = INW(np, nc_sist);
+ dstat = INB(np, nc_dstat);
+ sym_log_hard_error(np, sist, dstat);
+}
+
static struct sym_chip sym_dev_table[] = {
{PCI_DEVICE_ID_NCR_53C810, 0x0f, 810, 4, 8, 4, 64,
FE_ERL}
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
