[PATCH] regression, megaraid - fix irq setup process

[PATCH] regression, megaraid - fix irq setup process

[Tomas Henzl]

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(-)

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

RE: [PATCH] regression, megaraid - fix irq setup process

[Sumit Saxena]

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

Re: [PATCH] regression, megaraid - fix irq setup process

[Tomas Henzl]

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
> 

RE: [PATCH] regression, megaraid - fix irq setup process

[Sumit Saxena]

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