>-----Original Message----- >From: Hannes Reinecke [mailto:hare@xxxxxxx] >Sent: Friday, May 15, 2015 3:04 PM >To: Sumit.Saxena@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx >Cc: stable@xxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx; >hch@xxxxxxxxxxxxx; jbottomley@xxxxxxxxxxxxx; thenzl@xxxxxxxxxx; >kashyap.desai@xxxxxxxxxxxxx >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@xxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx> >> Signed-off-by: Kashyap Desai <kashyap.desai@xxxxxxxxxxxxx> >> Signed-off-by: Sumit Saxena <sumit.saxena@xxxxxxxxxxxxx> >> --- >> 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@xxxxxxx +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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
