Sample implementation of a scheme to handle missing interrupts

Sample implementation of a scheme to handle missing interrupts

[Matthew Wilcox]

There has been some discussion recently (both on the mailing list and at
OLS) of MSI and it reminded me of a perennial problem that I have with
sym2.  When interrupts don't work, I often get the bug reports, because
the user seems sym2 spewing error messages right before the panic when
it fails to mount their root filesystem.

When a command times out, the midlayer calls eh_timed_out.  We can then
poke at the chip to see if we've missed an interrupt.  In this proof of
concept patch, I call the interrupt handler and see if it did anything.
I then check if the 'something' it did includes completing the command.

I believe this to be OK because the interrupt handler calls scsi_done()
which returns immediately because the timer has expired.  Then we return
EH_HANDLED from eh_timed_out which causes the midlayer to call
__scsi_done() on our behalf, completing the command properly.

I was travelling today, and my laptop doesn't have a sym2 card (anyone
got one in CardBus form factor?  ;-), so this is totally untested.
I wanted to get it out before I went to bed so people could comment.

I had to be careful not to reference a driver data structure which could
have been freed or reused.  I also decided to print (once) an error
message telling the user why they're seeing a 30-second lag between
commands completing.

For drivers testing for MSI, the driver probably wants to do something
more complex such as disabling MSIs and printing a backtrace (possibly
with a PCI hierarchy included for our benefit).

If we have any desire to do something resembling NAPI for storage, we'd
want to use a totally different scheme.  I suspect we don't, but am
willing to be proven wrong.

(this patch depends on this little patchlet, because I find
host_scribble such a terrible name:

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 66c9448..10e73a8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -115,13 +115,17 @@ struct scsi_cmnd {
         */
        struct scsi_pointer SCp;        /* Scratchpad used by some host adapters
 
-       unsigned char *host_scribble;   /* The host adapter is allowed to
-                                        * call scsi_malloc and get some memory
-                                        * and hang it here.  The host adapter
-                                        * is also expected to call scsi_free
-                                        * to release this memory.  (The memory
-                                        * obtained by scsi_malloc is guaranteed
-                                        * to be at an address < 16Mb). */
+       /*
+        * host_scribble is the old name and type for host_data.
+        * The host_data pointer belongs to the host; it should manage it,
+        * typically by storing a pointer to some command-specific data here.
+        * Setting it to NULL when the host has freed the data is recommended,
+        * but not enforced.
+        */
+       union {
+               unsigned char *host_scribble;
+               void *host_data;
+       };
 
        int result;             /* Status code from lower level driver */
 
)

diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.c b/drivers/scsi/sym53c8xx_2/sym_glue.c
index d39107b..c86260f 100644
--- a/drivers/scsi/sym53c8xx_2/sym_glue.c
+++ b/drivers/scsi/sym53c8xx_2/sym_glue.c
@@ -687,6 +614,35 @@ static int sym_eh_handler(int op, char *opname, struct scsi_cmnd *cmd)
 /*
  * Error handlers called from the eh thread (one thread per HBA).
  */
+
+/*
+ * eh_timed_out is called first when a command timed out.  We should check
+ * to see if the command has been completed and we've failed to spot the
+ * interrupt.  This can happen for a number of reasons, including buggy
+ * hardware and interrupt handler failures.
+ */
+static enum scsi_eh_timer_return sym53c8xx_eh_timed_out(struct scsi_cmnd *cmd)
+{
+	static int printed_warning;
+	int result = sym53c8xx_intr(0, cmd->device->host);
+
+	if (result == IRQ_NONE)
+		return EH_NOT_HANDLED;
+
+	/* When commands have been handled, host_data is set to NULL */
+	if (cmd->host_data)
+		return EH_NOT_HANDLED;
+
+	if (!printed_warning) {
+		scmd_printk(KERN_ERR, cmd, "Command timed out and was "
+				"completed from the error handler.  Check"
+				"your interrupt settings!\n");
+		printed_warning = 1;
+	}
+
+	return EH_HANDLED;
+}
+
 static int sym53c8xx_eh_abort_handler(struct scsi_cmnd *cmd)
 {
 	return sym_eh_handler(SYM_EH_ABORT, "ABORT", cmd);
@@ -1675,6 +1611,7 @@ static struct scsi_host_template sym2_template = {
 	.slave_alloc		= sym53c8xx_slave_alloc,
 	.slave_configure	= sym53c8xx_slave_configure,
 	.slave_destroy		= sym53c8xx_slave_destroy,
+	.eh_timed_out		= sym53c8xx_eh_timed_out,
 	.eh_abort_handler	= sym53c8xx_eh_abort_handler,
 	.eh_device_reset_handler = sym53c8xx_eh_device_reset_handler,
 	.eh_bus_reset_handler	= sym53c8xx_eh_bus_reset_handler,
diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index 22a6aae..dfa3ca1 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -4619,6 +4619,7 @@ struct sym_ccb *sym_get_ccb (struct sym_hcb *np, struct scsi_cmnd *cmd, u_char t
 	if (!qp)
 		goto out;
 	cp = sym_que_entry(qp, struct sym_ccb, link_ccbq);
+	cmd->host_data = cp;
 
 	{
 		/*
@@ -4731,6 +4732,7 @@ void sym_free_ccb (struct sym_hcb *np, struct sym_ccb *cp)
 {
 	struct sym_tcb *tp = &np->target[cp->target];
 	struct sym_lcb *lp = sym_lp(tp, cp->lun);
+	struct scsi_cmnd *cmd;
 
 	if (DEBUG_FLAGS & DEBUG_TAGS) {
 		sym_print_addr(cp->cmd, "ccb @%p freeing tag %d.\n",
@@ -4796,6 +4798,8 @@ void sym_free_ccb (struct sym_hcb *np, struct sym_ccb *cp)
 	/*
 	 *  Make this CCB available.
 	 */
+	cmd = cp->cmd;
+	cmd->host_data = NULL;
 	cp->cmd = NULL;
 	cp->host_status = HS_IDLE;
 	sym_remque(&cp->link_ccbq);

-- 
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."

Re: Sample implementation of a scheme to handle missing interrupts

[Zhao Forrest]

I'm new to sym2 driver, but have a generic question: is it safe to
call an interrupt handler(i.e. sym53c8xx_intr() in this case) in the
context of process(i.e. SCSI midlayer thread in this case)?

Thanks,
Forrest

On Mon, Jul 28, 2008 at 1:56 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>
> There has been some discussion recently (both on the mailing list and at
> OLS) of MSI and it reminded me of a perennial problem that I have with
> sym2.  When interrupts don't work, I often get the bug reports, because
> the user seems sym2 spewing error messages right before the panic when
> it fails to mount their root filesystem.
>
> When a command times out, the midlayer calls eh_timed_out.  We can then
> poke at the chip to see if we've missed an interrupt.  In this proof of
> concept patch, I call the interrupt handler and see if it did anything.
> I then check if the 'something' it did includes completing the command.
>
> I believe this to be OK because the interrupt handler calls scsi_done()
> which returns immediately because the timer has expired.  Then we return
> EH_HANDLED from eh_timed_out which causes the midlayer to call
> __scsi_done() on our behalf, completing the command properly.
>
> I was travelling today, and my laptop doesn't have a sym2 card (anyone
> got one in CardBus form factor?  ;-), so this is totally untested.
> I wanted to get it out before I went to bed so people could comment.
>
> I had to be careful not to reference a driver data structure which could
> have been freed or reused.  I also decided to print (once) an error
> message telling the user why they're seeing a 30-second lag between
> commands completing.
>
> For drivers testing for MSI, the driver probably wants to do something
> more complex such as disabling MSIs and printing a backtrace (possibly
> with a PCI hierarchy included for our benefit).
>
> If we have any desire to do something resembling NAPI for storage, we'd
> want to use a totally different scheme.  I suspect we don't, but am
> willing to be proven wrong.
>
> (this patch depends on this little patchlet, because I find
> host_scribble such a terrible name:
>
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 66c9448..10e73a8 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -115,13 +115,17 @@ struct scsi_cmnd {
>         */
>        struct scsi_pointer SCp;        /* Scratchpad used by some host adapters
>
> -       unsigned char *host_scribble;   /* The host adapter is allowed to
> -                                        * call scsi_malloc and get some memory
> -                                        * and hang it here.  The host adapter
> -                                        * is also expected to call scsi_free
> -                                        * to release this memory.  (The memory
> -                                        * obtained by scsi_malloc is guaranteed
> -                                        * to be at an address < 16Mb). */
> +       /*
> +        * host_scribble is the old name and type for host_data.
> +        * The host_data pointer belongs to the host; it should manage it,
> +        * typically by storing a pointer to some command-specific data here.
> +        * Setting it to NULL when the host has freed the data is recommended,
> +        * but not enforced.
> +        */
> +       union {
> +               unsigned char *host_scribble;
> +               void *host_data;
> +       };
>
>        int result;             /* Status code from lower level driver */
>
> )
>
> diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.c b/drivers/scsi/sym53c8xx_2/sym_glue.c
> index d39107b..c86260f 100644
> --- a/drivers/scsi/sym53c8xx_2/sym_glue.c
> +++ b/drivers/scsi/sym53c8xx_2/sym_glue.c
> @@ -687,6 +614,35 @@ static int sym_eh_handler(int op, char *opname, struct scsi_cmnd *cmd)
>  /*
>  * Error handlers called from the eh thread (one thread per HBA).
>  */
> +
> +/*
> + * eh_timed_out is called first when a command timed out.  We should check
> + * to see if the command has been completed and we've failed to spot the
> + * interrupt.  This can happen for a number of reasons, including buggy
> + * hardware and interrupt handler failures.
> + */
> +static enum scsi_eh_timer_return sym53c8xx_eh_timed_out(struct scsi_cmnd *cmd)
> +{
> +       static int printed_warning;
> +       int result = sym53c8xx_intr(0, cmd->device->host);
> +
> +       if (result == IRQ_NONE)
> +               return EH_NOT_HANDLED;
> +
> +       /* When commands have been handled, host_data is set to NULL */
> +       if (cmd->host_data)
> +               return EH_NOT_HANDLED;
> +
> +       if (!printed_warning) {
> +               scmd_printk(KERN_ERR, cmd, "Command timed out and was "
> +                               "completed from the error handler.  Check"
> +                               "your interrupt settings!\n");
> +               printed_warning = 1;
> +       }
> +
> +       return EH_HANDLED;
> +}
> +
>  static int sym53c8xx_eh_abort_handler(struct scsi_cmnd *cmd)
>  {
>        return sym_eh_handler(SYM_EH_ABORT, "ABORT", cmd);
> @@ -1675,6 +1611,7 @@ static struct scsi_host_template sym2_template = {
>        .slave_alloc            = sym53c8xx_slave_alloc,
>        .slave_configure        = sym53c8xx_slave_configure,
>        .slave_destroy          = sym53c8xx_slave_destroy,
> +       .eh_timed_out           = sym53c8xx_eh_timed_out,
>        .eh_abort_handler       = sym53c8xx_eh_abort_handler,
>        .eh_device_reset_handler = sym53c8xx_eh_device_reset_handler,
>        .eh_bus_reset_handler   = sym53c8xx_eh_bus_reset_handler,
> diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
> index 22a6aae..dfa3ca1 100644
> --- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
> +++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
> @@ -4619,6 +4619,7 @@ struct sym_ccb *sym_get_ccb (struct sym_hcb *np, struct scsi_cmnd *cmd, u_char t
>        if (!qp)
>                goto out;
>        cp = sym_que_entry(qp, struct sym_ccb, link_ccbq);
> +       cmd->host_data = cp;
>
>        {
>                /*
> @@ -4731,6 +4732,7 @@ void sym_free_ccb (struct sym_hcb *np, struct sym_ccb *cp)
>  {
>        struct sym_tcb *tp = &np->target[cp->target];
>        struct sym_lcb *lp = sym_lp(tp, cp->lun);
> +       struct scsi_cmnd *cmd;
>
>        if (DEBUG_FLAGS & DEBUG_TAGS) {
>                sym_print_addr(cp->cmd, "ccb @%p freeing tag %d.\n",
> @@ -4796,6 +4798,8 @@ void sym_free_ccb (struct sym_hcb *np, struct sym_ccb *cp)
>        /*
>         *  Make this CCB available.
>         */
> +       cmd = cp->cmd;
> +       cmd->host_data = NULL;
>        cp->cmd = NULL;
>        cp->host_status = HS_IDLE;
>        sym_remque(&cp->link_ccbq);
>
> --
> 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-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Sample implementation of a scheme to handle missing interrupts

[Matthew Wilcox]

On Mon, Jul 28, 2008 at 05:50:23PM +0800, Zhao Forrest wrote:
> I'm new to sym2 driver, but have a generic question: is it safe to
> call an interrupt handler(i.e. sym53c8xx_intr() in this case) in the
> context of process(i.e. SCSI midlayer thread in this case)?

Not in general.  I should have disabled interrupts before calling into
sym53c8xx_intr() (or in my specific case, just called sym_interrupt()).
It very much depends on how the specific driver is written.

-- 
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."

RE: Sample implementation of a scheme to handle missing interrupts

[Moore, Eric]

On Sunday, July 27, 2008 11:56 PM, Matthew Wilcox wrote:

> There has been some discussion recently (both on the mailing
> list and at
> OLS) of MSI and it reminded me of a perennial problem that I have with
> sym2.  When interrupts don't work, I often get the bug
> reports, because
> the user seems sym2 spewing error messages right before the panic when
> it fails to mount their root filesystem.
>

In my case, the MSI problem will manifest itself well before we bind with the scsi midlayer.    Meaning when there is a MSI problem, we can't even bring up the card.  Hence adding code in a eh_timed_out callback handler would be meaningless in solving our problem. What I need to do is find a problematic card, so I can verify some things.   My beliefs are that all the doorbell request at driver load time work becuase we are polling with interrupts masked.  The first command sent with interrupts enabled would be SendPortEnable.  If we have a timeout after calling mpt_handshake_req_reply_wait, we resend the port enable with MSI turned off, right?

Eric

RE: Sample implementation of a scheme to handle missing interrupts

[James Bottomley]

On Mon, 2008-07-28 at 13:01 -0600, Moore, Eric wrote:
> On Sunday, July 27, 2008 11:56 PM, Matthew Wilcox wrote:
> 
> > There has been some discussion recently (both on the mailing
> > list and at
> > OLS) of MSI and it reminded me of a perennial problem that I have with
> > sym2.  When interrupts don't work, I often get the bug
> > reports, because
> > the user seems sym2 spewing error messages right before the panic when
> > it fails to mount their root filesystem.
> >
> 
> In my case, the MSI problem will manifest itself well before we bind
> with the scsi midlayer.    Meaning when there is a MSI problem, we
> can't even bring up the card.  Hence adding code in a eh_timed_out
> callback handler would be meaningless in solving our problem. What I
> need to do is find a problematic card, so I can verify some things.

Actually, you don't need this.  I verified the behaviour of the MSI
problem simply by commenting out the request_irq.  It looks like there's
no simple way to simulate MSI misrouting, but perhaps I should look at
that, since it would be useful.

>    My beliefs are that all the doorbell request at driver load time
> work becuase we are polling with interrupts masked.

This is the actual bug report:

http://bugzilla.kernel.org/show_bug.cgi?id=11045

>   The first command sent with interrupts enabled would be
> SendPortEnable.  If we have a timeout after calling
> mpt_handshake_req_reply_wait, we resend the port enable with MSI
> turned off, right?

James

RE: Sample implementation of a scheme to handle missing interrupts

[Moore, Eric]

On Monday, July 28, 2008 2:45 PM, James Bottomley wrote:
> >
> > In my case, the MSI problem will manifest itself well before we bind
> > with the scsi midlayer.    Meaning when there is a MSI problem, we
> > can't even bring up the card.  Hence adding code in a eh_timed_out
> > callback handler would be meaningless in solving our problem. What I
> > need to do is find a problematic card, so I can verify some things.
>
> Actually, you don't need this.  I verified the behaviour of the MSI
> problem simply by commenting out the request_irq.  It looks
> like there's
> no simple way to simulate MSI misrouting, but perhaps I should look at
> that, since it would be useful.
>

Well today I found at FC929X where it fails when MSI enabled.   The problem is occuring at the end of mpt_do_ioc_recovery, after we enable interrupts, we are asking for random manufacturing config pages. I see it randomly timing out on the config pages, meaning some time GetLanConfigPages works, then other times it fails. Sometimes it fails later on for either GetIoUnitPage2 or mpt_get_manufacturing_pg_0.  So its randomly timing out on config pages, but most the time is the first one.   So did the following patch (hopefully its not butcherd by ccmail). What I'm doing is failling back to IOAPIC routing after we have a config page timeout.   This works scsi_misc tip with drives attached to both channels of the multifunction FC card.      I'm thinking we may still need do what Matt W. suggest
 ed just as I pointed, I see random timeouts with the config pages.  A side note, I found that pci_disable_msi() is not working in SLES10 SP2..    Did you have any other suggestion where this could be handled in a generic common method?


diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index d6a0074..e2804b2 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1754,7 +1754,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)

        if (mpt_msi_enable == -1) {
                /* Enable on SAS, disable on FC and SPI */
-               if (ioc->bus_type == SAS)
+               if (ioc->bus_type == SAS || ioc->bus_type == FC)
                        ioc->msi_enable = 1;
                else
                        ioc->msi_enable = 0;
@@ -5927,6 +5927,25 @@ mpt_config(MPT_ADAPTER *ioc, CONFIGPARMS *pCfg)
        /* mf has been freed - do not access */

        rc = pCfg->status;
+       if ((rc == MPT_CONFIG_ERROR) && ioc->msi_enable) {
+               ioc->msi_enable = 0;
+               printk(MYIOC_s_ERR_FMT "free_irq\n", ioc->name);
+               free_irq(ioc->pci_irq, ioc);
+               printk(MYIOC_s_ERR_FMT "pci_disable_msi\n", ioc->name);
+               pci_disable_msi(ioc->pcidev);
+               printk(MYIOC_s_ERR_FMT "request_irq\n", ioc->name);
+               if (request_irq(ioc->pcidev->irq, mpt_interrupt, IRQF_SHARED,
+                   ioc->name, ioc) < 0) {
+                       printk(MYIOC_s_ERR_FMT "Unable to allocate "
+                           "interrupt %d!\n", ioc->name, ioc->pcidev->irq);
+                       return -EBUSY;
+               }
+               pci_set_master(ioc->pcidev);
+               pci_set_drvdata(ioc->pcidev, ioc);
+               ioc->pci_irq = ioc->pcidev->irq;
+               printk(MYIOC_s_INFO_FMT "PCI-MSI is being disabled\n",
+                   ioc->name);
+       }

        return rc;
 }

RE: Sample implementation of a scheme to handle missing interrupts

[James Bottomley]

On Tue, 2008-07-29 at 17:38 -0600, Moore, Eric wrote:
> On Monday, July 28, 2008 2:45 PM, James Bottomley wrote:
> > >
> > > In my case, the MSI problem will manifest itself well before we bind
> > > with the scsi midlayer.    Meaning when there is a MSI problem, we
> > > can't even bring up the card.  Hence adding code in a eh_timed_out
> > > callback handler would be meaningless in solving our problem. What I
> > > need to do is find a problematic card, so I can verify some things.
> >
> > Actually, you don't need this.  I verified the behaviour of the MSI
> > problem simply by commenting out the request_irq.  It looks
> > like there's
> > no simple way to simulate MSI misrouting, but perhaps I should look at
> > that, since it would be useful.
> >
> 
> Well today I found at FC929X where it fails when MSI enabled.   The problem is occuring at the end of mpt_do_ioc_recovery, after we enable interrupts, we are asking for random manufacturing config pages. I see it randomly timing out on the config pages, meaning some time GetLanConfigPages works, then other times it fails. Sometimes it fails later on for either GetIoUnitPage2 or mpt_get_manufacturing_pg_0.  So its randomly timing out on config pages, but most the time is the first one.   So did the following patch (hopefully its not butcherd by ccmail). What I'm doing is failling back to IOAPIC routing after we have a config page timeout.   This works scsi_misc tip with drives attached to both channels of the multifunction FC card.      I'm thinking we may still need do what Matt W. sugge
 sted just as I pointed, I see random timeouts with the config pages.  A side note, I found that pci_disable_msi() is not working in SLES10 SP2..    Did you have any other suggestion where th
 is
>   could be handled in a generic common method?

Actually, I think mpt_config is too deep.  The first time we use it to
get a page is when the failure occurs ... I think that's the point we
can intercept and retry.

I'll send a common coded routine (plus a bit of generic infrastructure
to make this more standard) shortly.

James