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."
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
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."
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
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 chip_name[8];
struct pci_dev *device;
- /* Waiter for clearing of frozen PCI bus */
- struct completion io_reset_wait;
-
struct Scsi_Host *host;
void __iomem * ioaddr; /* MMIO kernel io address */
@@ -223,6 +220,7 @@ struct sym_device {
*/
struct host_data {
struct sym_hcb *ncb;
+ struct completion *io_reset; /* PCI error handling */
};
static inline struct sym_hcb * sym_get_hcb(struct Scsi_Host *host)
--
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."
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
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."
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
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