Matthew,
Please apply and forward upstream. This patch is nearly identical
to the one from a month earlier, except that it fixes some
indentation problems, and shortens a few over-length lines.
This patch has been in an -mm tree since 29 Sept, and a variant
of it has been shipping for over a year in Novell SLES9, where
it has not received any complaints/defects/fixes/etc.
You had previously mentioned some objections to this patch,
I assume these were about the bad indentation? Are there other
concerns?
--linas
Various PCI bus errors can be signaled by newer PCI controllers.
This patch adds the PCI error recovery callbacks to the Symbios
SCSI device driver. The patch has been tested, and appears to
work well.
Signed-off-by: Linas Vepstas <[email protected]>
--
drivers/scsi/sym53c8xx_2/sym_glue.c | 99 ++++++++++++++++++++++++++++++++++++
drivers/scsi/sym53c8xx_2/sym_glue.h | 4 +
drivers/scsi/sym53c8xx_2/sym_hipd.c | 10 +++
3 files changed, 113 insertions(+)
Index: linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_glue.c
===================================================================
--- linux-2.6.19-rc1-git11.orig/drivers/scsi/sym53c8xx_2/sym_glue.c 2006-10-20 12:25:11.000000000 -0500
+++ linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_glue.c 2006-10-20 12:41:15.000000000 -0500
@@ -659,6 +659,11 @@ static irqreturn_t sym53c8xx_intr(int ir
if (DEBUG_FLAGS & DEBUG_TINY) printf_debug ("[");
+ /* Avoid spinloop trying to handle interrupts on frozen device */
+ if ((np->s.device->error_state != pci_channel_io_normal) &&
+ (np->s.device->error_state != 0))
+ return IRQ_HANDLED;
+
spin_lock_irqsave(np->s.host->host_lock, flags);
sym_interrupt(np);
spin_unlock_irqrestore(np->s.host->host_lock, flags);
@@ -726,6 +731,19 @@ static int sym_eh_handler(int op, char *
dev_warn(&cmd->device->sdev_gendev, "%s operation started.\n", opname);
+ /* We may be in an error condition because the PCI bus
+ * went down. In this case, we need to wait until the
+ * PCI bus is reset, the card is reset, and only then
+ * proceed with the scsi error recovery. There's no
+ * point in hurrying; take a leisurely wait.
+ */
+#define WAIT_FOR_PCI_RECOVERY 35
+ if ((np->s.device->error_state != pci_channel_io_normal) &&
+ (np->s.device->error_state != 0) &&
+ (wait_for_completion_timeout(&np->s.io_reset_wait,
+ WAIT_FOR_PCI_RECOVERY*HZ) == 0))
+ return SCSI_FAILED;
+
spin_lock_irq(host->host_lock);
/* This one is queued in some place -> to wait for completion */
FOR_EACH_QUEUED_ELEMENT(&np->busy_ccbq, qp) {
@@ -1510,6 +1528,7 @@ static struct Scsi_Host * __devinit sym_
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.
@@ -1948,6 +1967,79 @@ static void __devexit sym2_remove(struct
attach_count--;
}
+/**
+ * sym2_io_error_detected() -- called when PCI error is detected
+ * @pdev: pointer to PCI device
+ * @state: current state of the PCI slot
+ */
+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);
+
+ /* Request a slot reset. */
+ return PCI_ERS_RESULT_NEED_RESET;
+}
+
+/**
+ * sym2_io_slot_reset() -- called when the pci bus has been reset.
+ * @pdev: pointer to PCI device
+ *
+ * Restart the card from scratch.
+ */
+static pci_ers_result_t sym2_io_slot_reset (struct pci_dev *pdev)
+{
+ struct sym_hcb *np = pci_get_drvdata(pdev);
+
+ printk(KERN_INFO "%s: recovering from a PCI slot reset\n",
+ sym_name(np));
+
+ if (pci_enable_device(pdev)) {
+ printk(KERN_ERR "%s: Unable to enable afer PCI reset\n",
+ sym_name(np));
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+
+ pci_set_master(pdev);
+ enable_irq(pdev->irq);
+
+ /* Perform host reset only on one instance of the card */
+ if (PCI_FUNC (pdev->devfn) == 0) {
+ if (sym_reset_scsi_bus(np, 0)) {
+ printk(KERN_ERR "%s: Unable to reset scsi host\n",
+ sym_name(np));
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+ sym_start_up(np, 1);
+ }
+
+ return PCI_ERS_RESULT_RECOVERED;
+}
+
+/**
+ * sym2_io_resume() -- resume normal ops after PCI reset
+ * @pdev: pointer to PCI device
+ *
+ * Called when the error recovery driver tells us that its
+ * OK to resume normal operation. Use completion to allow
+ * halted scsi ops to resume.
+ */
+static void sym2_io_resume (struct pci_dev *pdev)
+{
+ struct sym_hcb *np = pci_get_drvdata(pdev);
+ complete_all(&np->s.io_reset_wait);
+}
+
static void sym2_get_signalling(struct Scsi_Host *shost)
{
struct sym_hcb *np = sym_get_hcb(shost);
@@ -2110,11 +2202,18 @@ static struct pci_device_id sym2_id_tabl
MODULE_DEVICE_TABLE(pci, sym2_id_table);
+static struct pci_error_handlers sym2_err_handler = {
+ .error_detected = sym2_io_error_detected,
+ .slot_reset = sym2_io_slot_reset,
+ .resume = sym2_io_resume,
+};
+
static struct pci_driver sym2_driver = {
.name = NAME53C8XX,
.id_table = sym2_id_table,
.probe = sym2_probe,
.remove = __devexit_p(sym2_remove),
+ .err_handler = &sym2_err_handler,
};
static int __init sym2_init(void)
Index: linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_glue.h
===================================================================
--- linux-2.6.19-rc1-git11.orig/drivers/scsi/sym53c8xx_2/sym_glue.h 2006-10-20 12:25:11.000000000 -0500
+++ linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_glue.h 2006-10-20 12:41:16.000000000 -0500
@@ -40,6 +40,7 @@
#ifndef SYM_GLUE_H
#define SYM_GLUE_H
+#include <linux/completion.h>
#include <linux/delay.h>
#include <linux/ioport.h>
#include <linux/pci.h>
@@ -179,6 +180,9 @@ 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 */
Index: linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_hipd.c
===================================================================
--- linux-2.6.19-rc1-git11.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c 2006-10-20 12:25:11.000000000 -0500
+++ linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_hipd.c 2006-10-20 12:41:16.000000000 -0500
@@ -2761,6 +2761,7 @@ void sym_interrupt (struct sym_hcb *np)
u_char istat, istatc;
u_char dstat;
u_short sist;
+ u_int icnt;
/*
* interrupt on the fly ?
@@ -2802,6 +2803,7 @@ void sym_interrupt (struct sym_hcb *np)
sist = 0;
dstat = 0;
istatc = istat;
+ icnt = 0;
do {
if (istatc & SIP)
sist |= INW(np, nc_sist);
@@ -2809,6 +2811,14 @@ void sym_interrupt (struct sym_hcb *np)
dstat |= INB(np, nc_dstat);
istatc = INB(np, nc_istat);
istat |= istatc;
+
+ /* Prevent deadlock waiting on a condition that may never clear. */
+ icnt ++;
+ if (icnt > 100) {
+ if ((np->s.device->error_state != pci_channel_io_normal)
+ && (np->s.device->error_state != 0))
+ return;
+ }
} while (istatc & (SIP|DIP));
if (DEBUG_FLAGS & DEBUG_TINY)
On Fri, Oct 20, 2006 at 01:05:10PM -0500, Linas Vepstas wrote:
> Index: linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_glue.c
> ===================================================================
> --- linux-2.6.19-rc1-git11.orig/drivers/scsi/sym53c8xx_2/sym_glue.c 2006-10-20 12:25:11.000000000 -0500
> +++ linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_glue.c 2006-10-20 12:41:15.000000000 -0500
> @@ -659,6 +659,11 @@ static irqreturn_t sym53c8xx_intr(int ir
>
> if (DEBUG_FLAGS & DEBUG_TINY) printf_debug ("[");
>
> + /* Avoid spinloop trying to handle interrupts on frozen device */
> + if ((np->s.device->error_state != pci_channel_io_normal) &&
> + (np->s.device->error_state != 0))
> + return IRQ_HANDLED;
> +
This needs to be before the printf_debug call.
> @@ -726,6 +731,19 @@ static int sym_eh_handler(int op, char *
>
> dev_warn(&cmd->device->sdev_gendev, "%s operation started.\n", opname);
>
> + /* We may be in an error condition because the PCI bus
> + * went down. In this case, we need to wait until the
> + * PCI bus is reset, the card is reset, and only then
> + * proceed with the scsi error recovery. There's no
> + * point in hurrying; take a leisurely wait.
> + */
> +#define WAIT_FOR_PCI_RECOVERY 35
> + if ((np->s.device->error_state != pci_channel_io_normal) &&
> + (np->s.device->error_state != 0) &&
> + (wait_for_completion_timeout(&np->s.io_reset_wait,
> + WAIT_FOR_PCI_RECOVERY*HZ) == 0))
> + return SCSI_FAILED;
> +
Is it safe / reasonable / a good idea to sleep for 35 seconds in the EH
handler? I'm not that familiar with how the EH code works. It has its
own thread, so I suppose that's OK.
Are the driver's data structures still intact after a reset?
I generally prefer not to be so perlish in conditionals, ie:
if ((np->s.device->error_state != pci_channel_io_normal) &&
(np->s.device->error_state != 0) {
int timed_out = wait_for_completion_timeout(
&np->s.io_reset_wait, WAIT_FOR_PCI_RECOVERY*HZ);
if (!timed_out)
return SCSI_FAILED;
}
Why is the condition so complicated though? What does 0 mean if it's
not io_normal? At least let's hide that behind a convenience macro:
if (abnormal_error_state(np->s.device->error_state)) {
...
}
> Index: linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_hipd.c
> ===================================================================
> --- linux-2.6.19-rc1-git11.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c 2006-10-20 12:25:11.000000000 -0500
> +++ linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_hipd.c 2006-10-20 12:41:16.000000000 -0500
> @@ -2761,6 +2761,7 @@ void sym_interrupt (struct sym_hcb *np)
> u_char istat, istatc;
> u_char dstat;
> u_short sist;
> + u_int icnt;
The cryptic names in this routine are actually register names. Calling
a counter 'icnt' is unhelpful (rather than fitting in with the style).
Just 'i' will do.
> /*
> * interrupt on the fly ?
> @@ -2802,6 +2803,7 @@ void sym_interrupt (struct sym_hcb *np)
> sist = 0;
> dstat = 0;
> istatc = istat;
> + icnt = 0;
> do {
> if (istatc & SIP)
> sist |= INW(np, nc_sist);
> @@ -2809,6 +2811,14 @@ void sym_interrupt (struct sym_hcb *np)
> dstat |= INB(np, nc_dstat);
> istatc = INB(np, nc_istat);
> istat |= istatc;
> +
> + /* Prevent deadlock waiting on a condition that may never clear. */
> + icnt ++;
> + if (icnt > 100) {
> + if ((np->s.device->error_state != pci_channel_io_normal)
> + && (np->s.device->error_state != 0))
> + return;
> + }
> } while (istatc & (SIP|DIP));
Though, since INB and INW will return 0xff and 0xffff, why not use that
as our test rather than using a counter?
if (sist == 0xffff && dstat == 0xff) {
if (abnormal_error_state(np->s.device->error_state)
return;
}
On Tue, 2006-10-31 at 11:55 -0700, Matthew Wilcox wrote:
> Is it safe / reasonable / a good idea to sleep for 35 seconds in the EH
> handler? I'm not that familiar with how the EH code works. It has its
> own thread, so I suppose that's OK.
Yes, each host has its own thread. Ordinarily it would be impolite to
delay recovery this long, but I assume that since the card is wedged
there's nothing else it could be doing anyway.
Just for my own edification, what happens on the dual function (dual
channel) boards? We have two threads there and two separate I/O
processors. I assume a PCI error will kill both, do we need to do
something about this?
James
> I generally prefer not to be so perlish in conditionals, ie:
>
> if ((np->s.device->error_state != pci_channel_io_normal) &&
> (np->s.device->error_state != 0) {
> int timed_out = wait_for_completion_timeout(
> &np->s.io_reset_wait, WAIT_FOR_PCI_RECOVERY*HZ);
> if (!timed_out)
> return SCSI_FAILED;
> }
>
> Why is the condition so complicated though? What does 0 mean if it's
> not io_normal? At least let's hide that behind a convenience macro:
>
On Tue, Oct 31, 2006 at 02:24:01PM -0500, James Bottomley wrote:
>
> Just for my own edification, what happens on the dual function (dual
> channel) boards? We have two threads there and two separate I/O
> processors. I assume a PCI error will kill both,
Yes.
> do we need to do
> something about this?
I'm not sure, and actually, I have not thought about
or tested this case for the symbios.
The answer depends on the h/w design. On PCI
multi-function cards, the PCI reset callbacks will
get called for each PCI function. (Each function
gets to vote/veto how it wants te reset to proceed).
If the hardware supports completely independent scsi
host initialization for each scsi i/o processor,
then things should just work.
If some part of the card init sequence needs to run
only once, even when there are two i/o processors, then
this needs to be protected against. I presume that
using if(0 == PCI_FUNC(pdev->devfn)) is enough to
make sure the hardware initilization is called only once
for the card -- i.e. by calling it only for PCI
function zero. If there needs to be some additional
locking to make sure that the card initialization
completes before the i/o processor initialization
starts ... well, I don't know about that.
--linas
On Tue, Oct 31, 2006 at 11:55:07AM -0700, Matthew Wilcox wrote:
> On Fri, Oct 20, 2006 at 01:05:10PM -0500, Linas Vepstas wrote:
> > Index: linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_glue.c
>
> This needs to be before the printf_debug call.
Right. This'll be in the next patch submision.
> > @@ -726,6 +731,19 @@ static int sym_eh_handler(int op, char *
> > + /* We may be in an error condition because the PCI bus
> > + * went down. In this case, we need to wait until the
> > + * PCI bus is reset, the card is reset, and only then
> > + * proceed with the scsi error recovery. There's no
> > + * point in hurrying; take a leisurely wait.
> > + */
> > +#define WAIT_FOR_PCI_RECOVERY 35
> > + if ((np->s.device->error_state != pci_channel_io_normal) &&
> > + (np->s.device->error_state != 0) &&
> > + (wait_for_completion_timeout(&np->s.io_reset_wait,
> > + WAIT_FOR_PCI_RECOVERY*HZ) == 0))
> > + return SCSI_FAILED;
> > +
>
> Is it safe / reasonable / a good idea to sleep for 35 seconds in the EH
> handler? I'm not that familiar with how the EH code works. It has its
> own thread, so I suppose that's OK.
As James pointed out, the pci channel is is not available until the
reset sequence is done. The 35 seconds seemed like a reasonable time
to wait for the pci reset to complete; hopefuly it will complete much
sooner. If the pci reset fails for some reason, then things are hosed,
and the sysadmin will need to intervene.
> Are the driver's data structures still intact after a reset?
They should be. No one is attempting to free or shut down the driver.
> I generally prefer not to be so perlish in conditionals, ie:
I wasn't sure what style is popular. Actually, I agree with you on
that, and picked the other style cause i thought it was prefered.
Nex patch submission will be more nested.
> if ((np->s.device->error_state != pci_channel_io_normal) &&
> (np->s.device->error_state != 0) {
>
> Why is the condition so complicated though? What does 0 mean if it's
> not io_normal?
Its an unresolved stupidity. For some imagined, hypothetical
reason, it momentarily seemed wise to make pci_channel_io_normal
be non-zero; but this imagined reason, although vague, did manage
to bite back, as the above code demonstrates.
> At least let's hide that behind a convenience macro:
>
> if (abnormal_error_state(np->s.device->error_state)) {
Should I submit a patch to make pci_channel_io_normal be zero,
or submit a patch to define abnormal_error_state, or both?
Both, probably; I don't have much of an opinion.
> Though, since INB and INW will return 0xff and 0xffff, why not use that
> as our test rather than using a counter?
Right. I wanted to avoid checking for specific values,
as that vaguely seemed more robust; the direct check is easier.
I'll change this.
On Tue, Oct 31, 2006 at 05:13:34PM -0600, Linas Vepstas wrote:
> On Tue, Oct 31, 2006 at 11:55:07AM -0700, Matthew Wilcox wrote:
> > On Fri, Oct 20, 2006 at 01:05:10PM -0500, Linas Vepstas wrote:
> > > Index: linux-2.6.19-rc1-git11/drivers/scsi/sym53c8xx_2/sym_glue.c
> >
> > This needs to be
[...]
Matthew,
Here is a revised patch, cleaning up the assorted messes.
It does depend on
[PATCH 1/2]: Renumber PCI error enums to start at zero
which I just mailed out to GregKH, to add a small
pci_channel_offline() utility.
--linas
Various PCI bus errors can be signaled by newer PCI controllers.
This patch adds the PCI error recovery callbacks to the Symbios
SCSI device driver. The patch has been tested, and appears to
work well.
Signed-off-by: Linas Vepstas <[email protected]>
--
drivers/scsi/sym53c8xx_2/sym_glue.c | 100 ++++++++++++++++++++++++++++++++++++
drivers/scsi/sym53c8xx_2/sym_glue.h | 4 +
drivers/scsi/sym53c8xx_2/sym_hipd.c | 6 ++
3 files changed, 110 insertions(+)
Index: linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_glue.c
===================================================================
--- linux-2.6.19-rc4-git3.orig/drivers/scsi/sym53c8xx_2/sym_glue.c 2006-11-01 16:15:33.000000000 -0600
+++ linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_glue.c 2006-11-01 16:21:00.000000000 -0600
@@ -657,6 +657,10 @@ static irqreturn_t sym53c8xx_intr(int ir
unsigned long flags;
struct sym_hcb *np = (struct sym_hcb *)dev_id;
+ /* Avoid spinloop trying to handle interrupts on frozen device */
+ if (pci_channel_offline(np->s.device->error_state))
+ return IRQ_HANDLED;
+
if (DEBUG_FLAGS & DEBUG_TINY) printf_debug ("[");
spin_lock_irqsave(np->s.host->host_lock, flags);
@@ -726,6 +730,21 @@ static int sym_eh_handler(int op, char *
dev_warn(&cmd->device->sdev_gendev, "%s operation started.\n", opname);
+ /* We may be in an error condition because the PCI bus
+ * went down. In this case, we need to wait until the
+ * PCI bus is reset, the card is reset, and only then
+ * proceed with the scsi error recovery. There's no
+ * point in hurrying; take a leisurely wait.
+ */
+#define WAIT_FOR_PCI_RECOVERY 35
+ if (pci_channel_offline(np->s.device->error_state))
+ {
+ int finished_reset = wait_for_completion_timeout(
+ &np->s.io_reset_wait, WAIT_FOR_PCI_RECOVERY*HZ);
+ if (!finished_reset)
+ return SCSI_FAILED;
+ }
+
spin_lock_irq(host->host_lock);
/* This one is queued in some place -> to wait for completion */
FOR_EACH_QUEUED_ELEMENT(&np->busy_ccbq, qp) {
@@ -1510,6 +1529,7 @@ static struct Scsi_Host * __devinit sym_
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.
@@ -1948,6 +1968,79 @@ static void __devexit sym2_remove(struct
attach_count--;
}
+/**
+ * sym2_io_error_detected() -- called when PCI error is detected
+ * @pdev: pointer to PCI device
+ * @state: current state of the PCI slot
+ */
+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);
+
+ /* Request a slot reset. */
+ return PCI_ERS_RESULT_NEED_RESET;
+}
+
+/**
+ * sym2_io_slot_reset() -- called when the pci bus has been reset.
+ * @pdev: pointer to PCI device
+ *
+ * Restart the card from scratch.
+ */
+static pci_ers_result_t sym2_io_slot_reset (struct pci_dev *pdev)
+{
+ struct sym_hcb *np = pci_get_drvdata(pdev);
+
+ printk(KERN_INFO "%s: recovering from a PCI slot reset\n",
+ sym_name(np));
+
+ if (pci_enable_device(pdev)) {
+ printk(KERN_ERR "%s: Unable to enable afer PCI reset\n",
+ sym_name(np));
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+
+ pci_set_master(pdev);
+ enable_irq(pdev->irq);
+
+ /* Perform host reset only on one instance of the card */
+ if (PCI_FUNC (pdev->devfn) == 0) {
+ if (sym_reset_scsi_bus(np, 0)) {
+ printk(KERN_ERR "%s: Unable to reset scsi host\n",
+ sym_name(np));
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+ sym_start_up(np, 1);
+ }
+
+ return PCI_ERS_RESULT_RECOVERED;
+}
+
+/**
+ * sym2_io_resume() -- resume normal ops after PCI reset
+ * @pdev: pointer to PCI device
+ *
+ * Called when the error recovery driver tells us that its
+ * OK to resume normal operation. Use completion to allow
+ * halted scsi ops to resume.
+ */
+static void sym2_io_resume (struct pci_dev *pdev)
+{
+ struct sym_hcb *np = pci_get_drvdata(pdev);
+ complete_all(&np->s.io_reset_wait);
+}
+
static void sym2_get_signalling(struct Scsi_Host *shost)
{
struct sym_hcb *np = sym_get_hcb(shost);
@@ -2110,11 +2203,18 @@ static struct pci_device_id sym2_id_tabl
MODULE_DEVICE_TABLE(pci, sym2_id_table);
+static struct pci_error_handlers sym2_err_handler = {
+ .error_detected = sym2_io_error_detected,
+ .slot_reset = sym2_io_slot_reset,
+ .resume = sym2_io_resume,
+};
+
static struct pci_driver sym2_driver = {
.name = NAME53C8XX,
.id_table = sym2_id_table,
.probe = sym2_probe,
.remove = __devexit_p(sym2_remove),
+ .err_handler = &sym2_err_handler,
};
static int __init sym2_init(void)
Index: linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_glue.h
===================================================================
--- linux-2.6.19-rc4-git3.orig/drivers/scsi/sym53c8xx_2/sym_glue.h 2006-09-19 22:42:06.000000000 -0500
+++ linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_glue.h 2006-11-01 16:21:00.000000000 -0600
@@ -40,6 +40,7 @@
#ifndef SYM_GLUE_H
#define SYM_GLUE_H
+#include <linux/completion.h>
#include <linux/delay.h>
#include <linux/ioport.h>
#include <linux/pci.h>
@@ -179,6 +180,9 @@ 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 */
Index: linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_hipd.c
===================================================================
--- linux-2.6.19-rc4-git3.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c 2006-09-19 22:42:06.000000000 -0500
+++ linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_hipd.c 2006-11-01 16:21:00.000000000 -0600
@@ -2809,6 +2809,12 @@ void sym_interrupt (struct sym_hcb *np)
dstat |= INB(np, nc_dstat);
istatc = INB(np, nc_istat);
istat |= istatc;
+
+ /* Prevent deadlock waiting on a condition that may never clear. */
+ if (unlikely(sist == 0xffff && dstat == 0xff)) {
+ if (unlikely(pci_channel_offline(np->s.device->error_state)))
+ return;
+ }
} while (istatc & (SIP|DIP));
if (DEBUG_FLAGS & DEBUG_TINY)
On Wed, Nov 01, 2006 at 06:07:46PM -0600, Linas Vepstas wrote:
[...]
Matthew,
You seem to always see me at my dullest. Here's another attempt.
This patch depends on
[PATCH 1/2 v2]: Renumber PCI error enums to start at zero
which I just mailed out to GregKH, to add a small
pci_channel_offline() utility.
--linas
Various PCI bus errors can be signaled by newer PCI controllers.
This patch adds the PCI error recovery callbacks to the Symbios
SCSI device driver. The patch has been tested, and appears to
work well.
Signed-off-by: Linas Vepstas <[email protected]>
--
drivers/scsi/sym53c8xx_2/sym_glue.c | 100 ++++++++++++++++++++++++++++++++++++
drivers/scsi/sym53c8xx_2/sym_glue.h | 4 +
drivers/scsi/sym53c8xx_2/sym_hipd.c | 6 ++
3 files changed, 110 insertions(+)
Index: linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_glue.c
===================================================================
--- linux-2.6.19-rc4-git3.orig/drivers/scsi/sym53c8xx_2/sym_glue.c 2006-11-01 19:13:15.000000000 -0600
+++ linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_glue.c 2006-11-01 19:14:11.000000000 -0600
@@ -657,6 +657,10 @@ static irqreturn_t sym53c8xx_intr(int ir
unsigned long flags;
struct sym_hcb *np = (struct sym_hcb *)dev_id;
+ /* Avoid spinloop trying to handle interrupts on frozen device */
+ if (pci_channel_offline(np->s.device))
+ return IRQ_HANDLED;
+
if (DEBUG_FLAGS & DEBUG_TINY) printf_debug ("[");
spin_lock_irqsave(np->s.host->host_lock, flags);
@@ -726,6 +730,21 @@ static int sym_eh_handler(int op, char *
dev_warn(&cmd->device->sdev_gendev, "%s operation started.\n", opname);
+ /* We may be in an error condition because the PCI bus
+ * went down. In this case, we need to wait until the
+ * PCI bus is reset, the card is reset, and only then
+ * proceed with the scsi error recovery. There's no
+ * 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 (!finished_reset)
+ return SCSI_FAILED;
+ }
+
spin_lock_irq(host->host_lock);
/* This one is queued in some place -> to wait for completion */
FOR_EACH_QUEUED_ELEMENT(&np->busy_ccbq, qp) {
@@ -1510,6 +1529,7 @@ static struct Scsi_Host * __devinit sym_
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.
@@ -1948,6 +1968,79 @@ static void __devexit sym2_remove(struct
attach_count--;
}
+/**
+ * sym2_io_error_detected() -- called when PCI error is detected
+ * @pdev: pointer to PCI device
+ * @state: current state of the PCI slot
+ */
+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);
+
+ /* Request a slot reset. */
+ return PCI_ERS_RESULT_NEED_RESET;
+}
+
+/**
+ * sym2_io_slot_reset() -- called when the pci bus has been reset.
+ * @pdev: pointer to PCI device
+ *
+ * Restart the card from scratch.
+ */
+static pci_ers_result_t sym2_io_slot_reset (struct pci_dev *pdev)
+{
+ struct sym_hcb *np = pci_get_drvdata(pdev);
+
+ printk(KERN_INFO "%s: recovering from a PCI slot reset\n",
+ sym_name(np));
+
+ if (pci_enable_device(pdev)) {
+ printk(KERN_ERR "%s: Unable to enable afer PCI reset\n",
+ sym_name(np));
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+
+ pci_set_master(pdev);
+ enable_irq(pdev->irq);
+
+ /* Perform host reset only on one instance of the card */
+ if (PCI_FUNC (pdev->devfn) == 0) {
+ if (sym_reset_scsi_bus(np, 0)) {
+ printk(KERN_ERR "%s: Unable to reset scsi host\n",
+ sym_name(np));
+ return PCI_ERS_RESULT_DISCONNECT;
+ }
+ sym_start_up(np, 1);
+ }
+
+ return PCI_ERS_RESULT_RECOVERED;
+}
+
+/**
+ * sym2_io_resume() -- resume normal ops after PCI reset
+ * @pdev: pointer to PCI device
+ *
+ * Called when the error recovery driver tells us that its
+ * OK to resume normal operation. Use completion to allow
+ * halted scsi ops to resume.
+ */
+static void sym2_io_resume (struct pci_dev *pdev)
+{
+ struct sym_hcb *np = pci_get_drvdata(pdev);
+ complete_all(&np->s.io_reset_wait);
+}
+
static void sym2_get_signalling(struct Scsi_Host *shost)
{
struct sym_hcb *np = sym_get_hcb(shost);
@@ -2110,11 +2203,18 @@ static struct pci_device_id sym2_id_tabl
MODULE_DEVICE_TABLE(pci, sym2_id_table);
+static struct pci_error_handlers sym2_err_handler = {
+ .error_detected = sym2_io_error_detected,
+ .slot_reset = sym2_io_slot_reset,
+ .resume = sym2_io_resume,
+};
+
static struct pci_driver sym2_driver = {
.name = NAME53C8XX,
.id_table = sym2_id_table,
.probe = sym2_probe,
.remove = __devexit_p(sym2_remove),
+ .err_handler = &sym2_err_handler,
};
static int __init sym2_init(void)
Index: linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_glue.h
===================================================================
--- linux-2.6.19-rc4-git3.orig/drivers/scsi/sym53c8xx_2/sym_glue.h 2006-11-01 19:13:15.000000000 -0600
+++ linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_glue.h 2006-11-01 19:13:19.000000000 -0600
@@ -40,6 +40,7 @@
#ifndef SYM_GLUE_H
#define SYM_GLUE_H
+#include <linux/completion.h>
#include <linux/delay.h>
#include <linux/ioport.h>
#include <linux/pci.h>
@@ -179,6 +180,9 @@ 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 */
Index: linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_hipd.c
===================================================================
--- linux-2.6.19-rc4-git3.orig/drivers/scsi/sym53c8xx_2/sym_hipd.c 2006-11-01 19:13:15.000000000 -0600
+++ linux-2.6.19-rc4-git3/drivers/scsi/sym53c8xx_2/sym_hipd.c 2006-11-01 19:13:19.000000000 -0600
@@ -2809,6 +2809,12 @@ void sym_interrupt (struct sym_hcb *np)
dstat |= INB(np, nc_dstat);
istatc = INB(np, nc_istat);
istat |= istatc;
+
+ /* Prevent deadlock waiting on a condition that may never clear. */
+ if (unlikely(sist == 0xffff && dstat == 0xff)) {
+ if (unlikely(pci_channel_offline(np->s.device)))
+ return;
+ }
} while (istatc & (SIP|DIP));
if (DEBUG_FLAGS & DEBUG_TINY)
On Wed, Nov 01, 2006 at 07:19:37PM -0600, Linas Vepstas wrote:
> @@ -657,6 +657,10 @@ static irqreturn_t sym53c8xx_intr(int ir
> unsigned long flags;
> struct sym_hcb *np = (struct sym_hcb *)dev_id;
>
> + /* Avoid spinloop trying to handle interrupts on frozen device */
> + if (pci_channel_offline(np->s.device))
> + return IRQ_HANDLED;
> +
> if (DEBUG_FLAGS & DEBUG_TINY) printf_debug ("[");
>
> spin_lock_irqsave(np->s.host->host_lock, flags);
Just wondering ... should we really be returning HANDLED? What if the
IRQ is shared? Will the hardware de-assert the level interrupt when it
puts the device in reset (ie is this a transitory glitch?), or do we
have to cope with a screaming interrupt?
> +#define WAIT_FOR_PCI_RECOVERY 35
> + if (pci_channel_offline(np->s.device))
> + {
I prefer if () {
> +static pci_ers_result_t sym2_io_slot_reset (struct pci_dev *pdev)
> +{
> + struct sym_hcb *np = pci_get_drvdata(pdev);
> +
> + printk(KERN_INFO "%s: recovering from a PCI slot reset\n",
> + sym_name(np));
> +
> + if (pci_enable_device(pdev)) {
> + printk(KERN_ERR "%s: Unable to enable afer PCI reset\n",
> + sym_name(np));
> + return PCI_ERS_RESULT_DISCONNECT;
> + }
> +
> + pci_set_master(pdev);
> + enable_irq(pdev->irq);
Hm. If we need to call pci_set_master, then we're also going to need to
call pci_set_mwi (if appropriate) which is currently done in
sym_set_workarounds(). Except you don't have a sym_device, or a
sym_chip around at this point. Bother. Need to do some refactoring to
take care of that.
> + /* Prevent deadlock waiting on a condition that may never clear. */
> + if (unlikely(sist == 0xffff && dstat == 0xff)) {
> + if (unlikely(pci_channel_offline(np->s.device)))
> + return;
> + }
I like the first unlikely ... but I'd drop the second one. If they are
both ffff ff, I'd say it's quite likely ;-) Anyway, the first unlikely
is good enough a hint to GCC, IMO.
Thanks!
On Tue, Oct 31, 2006 at 05:13:34PM -0600, Linas Vepstas wrote:
...
> > Though, since INB and INW will return 0xff and 0xffff, why not use that
> > as our test rather than using a counter?
>
> Right. I wanted to avoid checking for specific values,
> as that vaguely seemed more robust; the direct check is easier.
ISTR some chipsets return 0 or the most recent data on the bus
when INB/INW master-abort. Maybe this an ISA bus behavior?
Or is config space access the only space which behaves this way
for master abort on PCI?
I'm looking at drivers/pci/probe.c:pci_scan_device().
thanks,
grant
On Wed, Nov 01, 2006 at 09:46:33PM -0700, Grant Grundler wrote:
> ISTR some chipsets return 0 or the most recent data on the bus
> when INB/INW master-abort. Maybe this an ISA bus behavior?
It's not a master-abort; it won't get as far as the device.
Documentation/pci-error-recovery.txt says reads return 0xffffffff.
> Or is config space access the only space which behaves this way
> for master abort on PCI?
> I'm looking at drivers/pci/probe.c:pci_scan_device().
As the comment says, the boards which do this are broken. It's highly
unlikely those boards will support error isolation and recovery ;-)
On Thu, Jul 05, 2007 at 12:54:06PM -0600, Matthew Wilcox wrote:
> On Thu, Jul 05, 2007 at 11:28:38AM -0700, Andrew Morton wrote:
> > Well you've sent it a couple of times, and I've sent it in five more times
> > over the past year. Once we were told "awaiting maintainer ack".
> >
> > This situation is fairly stupid. How about we make you the maintainer?
>
> Last time I looked at it, I still wasn't comfortable with it. I'm going
> to look at it again.
Please do. Its burning the proverbial hole in my pocket; I'd really
like to get this off my list of things I worry about.
> I'm fairly sure Linas doesn't want to be the sym2 maintainer. It's
> still an ugly pile of junk that needs cleaning up.
Heh. I have no difficulty living with ugly code: its actually a
great excuse to fix things instead of doing "real work" :-)
Rather, the menagerie of hardware I have access to is constantly
changing; I don't have a symbios card just right now, and it might
take a few days to even find someone who did. Which is an incredibly
unpleasent, unrewarding activity.
--linas