| Message ID | 1433179605-31821-1-git-send-email-thenzl@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>-----Original Message----- >From: Tomas Henzl [mailto:thenzl@redhat.com] >Sent: Monday, June 01, 2015 10:57 PM >To: linux-scsi@vger.kernel.org >Cc: kashyap.desai@avagotech.com; Sumit.Saxena@avagotech.com >Subject: [PATCH] regression, megaraid - fix irq setup process > >This fixes a regression caused by commit >d3557fc8be11d25f316884581f487684f8e7dad3 >megaraid_sas : Add separate function for setting up IRQs This makes boot end >with 'root does not exist' message on certain adapters. > >The bug is that the driver does not setup ints for cards without msi-x support. >This patch fixes it, in addition to that it moves irq-enable call after tasklet >initialisation - otherwise a kernel panic may occur, when an int arrives before >the tasklet is ready. Thanks for pointing it out. Tasklet is only scheduled for MFI adapters inside ISR routine and all commands fired prior to "tasklet_init" are in polled mode and MFI adapters will not send interrupt for the same. Fusion adapters may send the interrupts but their ISR function don't use tasklet at all so there should not be any problem. We will move tasklet_init prior to enable_intr. Calling enable_intr later after firing few more DCMDs will cause more DCMDs to be fired in polled mode(though fusion adapter will send interrupt but will not free up MFI frame used). I agree that cleaning up of this is required. I will send a patch to clean it up and address few additional stuff. > >Signed-off-by: Tomas Henzl <thenzl@redhat.com> >--- > drivers/scsi/megaraid/megaraid_sas_base.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > >diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c >b/drivers/scsi/megaraid/megaraid_sas_base.c >index a022c39153..3771d6fff5 100644 >--- a/drivers/scsi/megaraid/megaraid_sas_base.c >+++ b/drivers/scsi/megaraid/megaraid_sas_base.c >@@ -4619,18 +4619,16 @@ static int megasas_init_fw(struct >megasas_instance *instance) > instance->msix_vectors = i; > else > instance->msix_vectors = 0; >+ } > >- dev_info(&instance->pdev->dev, >- "firmware supports msix\t: (%d)", fw_msix_count); >- dev_info(&instance->pdev->dev, >- "current msix/online cpus\t: (%d/%d)\n", >- instance->msix_vectors, (unsigned >int)num_online_cpus()); >+ dev_info(&instance->pdev->dev, "firmware supports msix\t: (%d)", >+ fw_msix_count); >+ dev_info(&instance->pdev->dev, "current msix/online cpus\t: >(%d/%d)\n", >+ instance->msix_vectors, (unsigned int)num_online_cpus()); > >- if (instance->msix_vectors ? >- megasas_setup_irqs_msix(instance, 1) : >- megasas_setup_irqs_ioapic(instance)) >- goto fail_setup_irqs; >- } >+ if (instance->msix_vectors ? megasas_setup_irqs_msix(instance, 1) : >+ megasas_setup_irqs_ioapic(instance)) >+ goto fail_setup_irqs; > > instance->ctrl_info = kzalloc(sizeof(struct megasas_ctrl_info), > GFP_KERNEL); >@@ -4646,8 +4644,6 @@ static int megasas_init_fw(struct megasas_instance >*instance) > /* Get operational params, sge flags, send init cmd to controller */ > if (instance->instancet->init_adapter(instance)) > goto fail_init_adapter; >- instance->instancet->enable_intr(instance); >- > printk(KERN_ERR "megasas: INIT adapter done\n"); > > /** for passthrough >@@ -4769,6 +4765,8 @@ static int megasas_init_fw(struct megasas_instance >*instance) > tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet, > (unsigned long)instance); > >+ instance->instancet->enable_intr(instance); >+ > /* Launch SR-IOV heartbeat timer */ > if (instance->requestorId) { > if (!megasas_sriov_start_heartbeat(instance, 1)) >-- >1.9.3 -- 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
On 06/02/2015 09:06 AM, Sumit Saxena wrote: >> -----Original Message----- >> From: Tomas Henzl [mailto:thenzl@redhat.com] >> Sent: Monday, June 01, 2015 10:57 PM >> To: linux-scsi@vger.kernel.org >> Cc: kashyap.desai@avagotech.com; Sumit.Saxena@avagotech.com >> Subject: [PATCH] regression, megaraid - fix irq setup process >> >> This fixes a regression caused by commit >> d3557fc8be11d25f316884581f487684f8e7dad3 >> megaraid_sas : Add separate function for setting up IRQs This makes boot > end >> with 'root does not exist' message on certain adapters. >> >> The bug is that the driver does not setup ints for cards without msi-x > support. >> This patch fixes it, in addition to that it moves irq-enable call after > tasklet >> initialisation - otherwise a kernel panic may occur, when an int arrives > before >> the tasklet is ready. > > Thanks for pointing it out. > Tasklet is only scheduled for MFI adapters inside ISR routine and all > commands fired prior to "tasklet_init" are in polled mode and MFI > adapters will not send interrupt for the same. > Fusion adapters may send the interrupts but their ISR function don't use > tasklet at all so there should not be any problem. On my card it happens - and kernel panics. > We will move tasklet_init prior to enable_intr. Calling enable_intr later > after firing few more DCMDs will cause more DCMDs to be fired in polled > mode(though fusion adapter will send interrupt but will not free up MFI > frame used). > I agree that cleaning up of this is required. I will send a patch to clean > it up and address few additional stuff. This patch fixes a regression caused by your previous patch, a solution is needed soon. Please don't forget that this patch fixes also the issue of the driver not setting the irq at all for cards without msi-x support and add it to you fix. --tm > >> >> Signed-off-by: Tomas Henzl <thenzl@redhat.com> >> --- >> drivers/scsi/megaraid/megaraid_sas_base.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c >> b/drivers/scsi/megaraid/megaraid_sas_base.c >> index a022c39153..3771d6fff5 100644 >> --- a/drivers/scsi/megaraid/megaraid_sas_base.c >> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c >> @@ -4619,18 +4619,16 @@ static int megasas_init_fw(struct >> megasas_instance *instance) >> instance->msix_vectors = i; >> else >> instance->msix_vectors = 0; >> + } >> >> - dev_info(&instance->pdev->dev, >> - "firmware supports msix\t: (%d)", fw_msix_count); >> - dev_info(&instance->pdev->dev, >> - "current msix/online cpus\t: (%d/%d)\n", >> - instance->msix_vectors, (unsigned >> int)num_online_cpus()); >> + dev_info(&instance->pdev->dev, "firmware supports msix\t: (%d)", >> + fw_msix_count); >> + dev_info(&instance->pdev->dev, "current msix/online cpus\t: >> (%d/%d)\n", >> + instance->msix_vectors, (unsigned int)num_online_cpus()); >> >> - if (instance->msix_vectors ? >> - megasas_setup_irqs_msix(instance, 1) : >> - megasas_setup_irqs_ioapic(instance)) >> - goto fail_setup_irqs; >> - } >> + if (instance->msix_vectors ? megasas_setup_irqs_msix(instance, 1) > : >> + megasas_setup_irqs_ioapic(instance)) >> + goto fail_setup_irqs; >> >> instance->ctrl_info = kzalloc(sizeof(struct megasas_ctrl_info), >> GFP_KERNEL); >> @@ -4646,8 +4644,6 @@ static int megasas_init_fw(struct megasas_instance >> *instance) >> /* Get operational params, sge flags, send init cmd to controller > */ >> if (instance->instancet->init_adapter(instance)) >> goto fail_init_adapter; >> - instance->instancet->enable_intr(instance); >> - >> printk(KERN_ERR "megasas: INIT adapter done\n"); >> >> /** for passthrough >> @@ -4769,6 +4765,8 @@ static int megasas_init_fw(struct megasas_instance >> *instance) >> tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet, >> (unsigned long)instance); >> >> + instance->instancet->enable_intr(instance); >> + >> /* Launch SR-IOV heartbeat timer */ >> if (instance->requestorId) { >> if (!megasas_sriov_start_heartbeat(instance, 1)) > >> -- >> 1.9.3 > -- > 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 > -- 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
>-----Original Message----- >From: Tomas Henzl [mailto:thenzl@redhat.com] >Sent: Tuesday, June 02, 2015 3:20 PM >To: Sumit Saxena; linux-scsi@vger.kernel.org >Cc: Kashyap Desai >Subject: Re: [PATCH] regression, megaraid - fix irq setup process > >On 06/02/2015 09:06 AM, Sumit Saxena wrote: >>> -----Original Message----- >>> From: Tomas Henzl [mailto:thenzl@redhat.com] >>> Sent: Monday, June 01, 2015 10:57 PM >>> To: linux-scsi@vger.kernel.org >>> Cc: kashyap.desai@avagotech.com; Sumit.Saxena@avagotech.com >>> Subject: [PATCH] regression, megaraid - fix irq setup process >>> >>> This fixes a regression caused by commit >>> d3557fc8be11d25f316884581f487684f8e7dad3 >>> megaraid_sas : Add separate function for setting up IRQs This makes >>> boot >> end >>> with 'root does not exist' message on certain adapters. >>> >>> The bug is that the driver does not setup ints for cards without >>> msi-x >> support. >>> This patch fixes it, in addition to that it moves irq-enable call >>> after >> tasklet >>> initialisation - otherwise a kernel panic may occur, when an int >>> arrives >> before >>> the tasklet is ready. >> >> Thanks for pointing it out. >> Tasklet is only scheduled for MFI adapters inside ISR routine and all >> commands fired prior to "tasklet_init" are in polled mode and MFI >> adapters will not send interrupt for the same. >> Fusion adapters may send the interrupts but their ISR function don't >> use tasklet at all so there should not be any problem. >On my card it happens - and kernel panics. > >> We will move tasklet_init prior to enable_intr. Calling enable_intr >> later after firing few more DCMDs will cause more DCMDs to be fired in >> polled mode(though fusion adapter will send interrupt but will not >> free up MFI frame used). >> I agree that cleaning up of this is required. I will send a patch to >> clean it up and address few additional stuff. >This patch fixes a regression caused by your previous patch, a solution is >needed soon. >Please don't forget that this patch fixes also the issue of the driver not >setting >the irq at all for cards without msi-x support and add it to you fix. Tomas, I have sent modified patch in which tasklet_init will be done before enable_intr (instead of calling enable_intr at later point of time). that patch should also fix the issue. Thanks, Sumit > >--tm >> >>> >>> Signed-off-by: Tomas Henzl <thenzl@redhat.com> >>> --- >>> drivers/scsi/megaraid/megaraid_sas_base.c | 22 ++++++++++------------ >>> 1 file changed, 10 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c >>> b/drivers/scsi/megaraid/megaraid_sas_base.c >>> index a022c39153..3771d6fff5 100644 >>> --- a/drivers/scsi/megaraid/megaraid_sas_base.c >>> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c >>> @@ -4619,18 +4619,16 @@ static int megasas_init_fw(struct >>> megasas_instance *instance) >>> instance->msix_vectors = i; >>> else >>> instance->msix_vectors = 0; >>> + } >>> >>> - dev_info(&instance->pdev->dev, >>> - "firmware supports msix\t: (%d)", fw_msix_count); >>> - dev_info(&instance->pdev->dev, >>> - "current msix/online cpus\t: (%d/%d)\n", >>> - instance->msix_vectors, (unsigned >>> int)num_online_cpus()); >>> + dev_info(&instance->pdev->dev, "firmware supports msix\t: (%d)", >>> + fw_msix_count); >>> + dev_info(&instance->pdev->dev, "current msix/online cpus\t: >>> (%d/%d)\n", >>> + instance->msix_vectors, (unsigned int)num_online_cpus()); >>> >>> - if (instance->msix_vectors ? >>> - megasas_setup_irqs_msix(instance, 1) : >>> - megasas_setup_irqs_ioapic(instance)) >>> - goto fail_setup_irqs; >>> - } >>> + if (instance->msix_vectors ? megasas_setup_irqs_msix(instance, 1) >> : >>> + megasas_setup_irqs_ioapic(instance)) >>> + goto fail_setup_irqs; >>> >>> instance->ctrl_info = kzalloc(sizeof(struct megasas_ctrl_info), >>> GFP_KERNEL); >>> @@ -4646,8 +4644,6 @@ static int megasas_init_fw(struct >>> megasas_instance >>> *instance) >>> /* Get operational params, sge flags, send init cmd to controller >> */ >>> if (instance->instancet->init_adapter(instance)) >>> goto fail_init_adapter; >>> - instance->instancet->enable_intr(instance); >>> - >>> printk(KERN_ERR "megasas: INIT adapter done\n"); >>> >>> /** for passthrough >>> @@ -4769,6 +4765,8 @@ static int megasas_init_fw(struct >>> megasas_instance >>> *instance) >>> tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet, >>> (unsigned long)instance); >>> >>> + instance->instancet->enable_intr(instance); >>> + >>> /* Launch SR-IOV heartbeat timer */ >>> if (instance->requestorId) { >>> if (!megasas_sriov_start_heartbeat(instance, 1)) >> >>> -- >>> 1.9.3 >> -- >> 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 >> -- 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
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c index a022c39153..3771d6fff5 100644 --- a/drivers/scsi/megaraid/megaraid_sas_base.c +++ b/drivers/scsi/megaraid/megaraid_sas_base.c @@ -4619,18 +4619,16 @@ static int megasas_init_fw(struct megasas_instance *instance) instance->msix_vectors = i; else instance->msix_vectors = 0; + } - dev_info(&instance->pdev->dev, - "firmware supports msix\t: (%d)", fw_msix_count); - dev_info(&instance->pdev->dev, - "current msix/online cpus\t: (%d/%d)\n", - instance->msix_vectors, (unsigned int)num_online_cpus()); + dev_info(&instance->pdev->dev, "firmware supports msix\t: (%d)", + fw_msix_count); + dev_info(&instance->pdev->dev, "current msix/online cpus\t: (%d/%d)\n", + instance->msix_vectors, (unsigned int)num_online_cpus()); - if (instance->msix_vectors ? - megasas_setup_irqs_msix(instance, 1) : - megasas_setup_irqs_ioapic(instance)) - goto fail_setup_irqs; - } + if (instance->msix_vectors ? megasas_setup_irqs_msix(instance, 1) : + megasas_setup_irqs_ioapic(instance)) + goto fail_setup_irqs; instance->ctrl_info = kzalloc(sizeof(struct megasas_ctrl_info), GFP_KERNEL); @@ -4646,8 +4644,6 @@ static int megasas_init_fw(struct megasas_instance *instance) /* Get operational params, sge flags, send init cmd to controller */ if (instance->instancet->init_adapter(instance)) goto fail_init_adapter; - instance->instancet->enable_intr(instance); - printk(KERN_ERR "megasas: INIT adapter done\n"); /** for passthrough @@ -4769,6 +4765,8 @@ static int megasas_init_fw(struct megasas_instance *instance) tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet, (unsigned long)instance); + instance->instancet->enable_intr(instance); + /* Launch SR-IOV heartbeat timer */ if (instance->requestorId) { if (!megasas_sriov_start_heartbeat(instance, 1))
This fixes a regression caused by commit d3557fc8be11d25f316884581f487684f8e7dad3 megaraid_sas : Add separate function for setting up IRQs This makes boot end with 'root does not exist' message on certain adapters. The bug is that the driver does not setup ints for cards without msi-x support. This patch fixes it, in addition to that it moves irq-enable call after tasklet initialisation - otherwise a kernel panic may occur, when an int arrives before the tasklet is ready. Signed-off-by: Tomas Henzl <thenzl@redhat.com> --- drivers/scsi/megaraid/megaraid_sas_base.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)