RE: [PATCH] megaraid_sas : Modify return value of megasas_issue_blocked_cmd() and wait_and_poll() to consider command status returned by firmware

[Sumit Saxena <sumit.saxena@avagotech.com>]

>-----Original Message-----
>From: Hannes Reinecke [mailto:hare@suse.de]
>Sent: Friday, May 15, 2015 3:04 PM
>To: Sumit.Saxena@avagotech.com; linux-scsi@vger.kernel.org
>Cc: stable@vger.kernel.org; martin.petersen@oracle.com;
>hch@infradead.org; jbottomley@parallels.com; thenzl@redhat.com;
>kashyap.desai@avagotech.com
>Subject: Re: [PATCH] megaraid_sas : Modify return value of
>megasas_issue_blocked_cmd() and wait_and_poll() to consider command
>status returned by firmware
>
>On 05/06/2015 03:31 PM, Sumit.Saxena@avagotech.com wrote:
>> This patch is rebased on top of recently sent 18 patches(submitted by
me)
>for megaraid_sas driver.
>>
>> Change the return value of wait_and_poll() and
>> megsas_issue_blocked_cmd() based on MFI_STAT returned by firmware for
>> that command. Earlier driver always send return type based on command
>> completion (but never check MFI_STAT_OK for that command), so even if
>> command is failed by firmware still driver will return SUCCESS status
from
>these functions wait_and_poll() and megsas_issue_blocked_cmd() and if
>caller of these functions does not check command status (MFI_STAT), then
it
>may endup using invalid data returned in DMA buffers(one of the example
is
>megasas_ld_list_query DCMD). Best thing to avoid this type of issue is do
>error handling and set proper return type from caller function
wait_and_poll()
>and megsas_issue_blocked_cmd().
>>
>> The change proposed in this patch will fix the regression introduced
>> in patch- "90dc9d9 megaraid_sas : MFI MPT linked list corruption fix"
inside
>function megasas_ld_list_query().
>> Prior to this MFI MPT linked list corruption fix patch,
>> megasas_ld_list_query() function used to check DCMD status(returned by
>> firmware) but with this linked list corruption fix patch, DCMD status
will not
>be checked inside function megasas_ld_list_query() and introduced this
issue
>of wrong data being used by function megasas_ld_list_query().
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Kashyap Desai <kashyap.desai@avagotech.com>
>> Signed-off-by: Sumit Saxena <sumit.saxena@avagotech.com>
>> ---
>>  drivers/scsi/megaraid/megaraid_sas.h        |    2 +-
>>  drivers/scsi/megaraid/megaraid_sas_base.c   |   67
+++++++++++------------
>----
>>  drivers/scsi/megaraid/megaraid_sas_fusion.c |    3 +-
>>  3 files changed, 30 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas.h
>> b/drivers/scsi/megaraid/megaraid_sas.h
>> index 53a3c3f..20c3754 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas.h
>> +++ b/drivers/scsi/megaraid/megaraid_sas.h
>> @@ -1894,7 +1894,7 @@ struct megasas_cmd {
>>
>>  	u32 index;
>>  	u8 sync_cmd;
>> -	u8 cmd_status;
>> +	u8 cmd_status_drv;
>>  	u8 abort_aen;
>>  	u8 retry_for_fw_reset;
>>
>Can you please avoid the renaming here?
>It doesn't serve any purpose, and keeping it will make the diff smaller.

For readability we make this "cmd_status_drv", since this is driver's
internal structure(so drv suffix). Same name "cmd_status "is used in
multiple structs(megasas_hdr, megasas_init_frame, megasas_io_frame) which
are shared across driver and other components(firmware and applications).
Also, keeping it "cmd_status" will not make patch smaller since "ENODATA"
is also replaced by "MFI_STAT_INVALID_STATUS" in most of lines, where
"cmd_status_drv" is referenced.

Thanks,
Sumit
>
>Cheers,
>
>Hannes
>--
>Dr. Hannes Reinecke		               zSeries & Storage
>hare@suse.de			               +49 911 74053 688
>SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
>GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB
21284
>(AG Nürnberg)
--
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