> -----Original Message-----
> From: Hannes Reinecke [mailto:h...@suse.com]
> Sent: Monday, February 06, 2017 4:14 PM
> To: Shivasharan S; linux-scsi@vger.kernel.org
> Cc: martin.peter...@oracle.com; the...@redhat.com;
> j...@linux.vnet.ibm.com; kashyap.de...@broadcom.com;
> sumit.sax...@broadcom.com
> Subject: Re: [PATCH 08/39] megaraid_sas: megasas_get_request_descriptor
> always return valid desc
>
> On 02/06/2017 10:59 AM, Shivasharan S wrote:
> > No functional change. Code clean up. Removing error code which is not
> > valid scenario.
> >
> > Signed-off-by: Shivasharan S <shivasharan.srikanteshw...@broadcom.com>
> > Signed-off-by: Kashyap Desai <kashyap.de...@broadcom.com>
> > ---
> > drivers/scsi/megaraid/megaraid_sas_base.c | 4 +---
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 33
> > +++++------------------------
> > 2 files changed, 6 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> > b/drivers/scsi/megaraid/megaraid_sas_base.c
> > index 80fcdf5..138d028 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> > @@ -5602,9 +5602,7 @@ megasas_register_aen(struct megasas_instance
> *instance, u32 seq_num,
> > /*
> > * Issue the aen registration frame
> > */
> > - instance->instancet->issue_dcmd(instance, cmd);
> > -
> > - return 0;
> > + return instance->instancet->issue_dcmd(instance, cmd);
> > }
> >
> > /**
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index d25268a..1ec482e 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -974,8 +974,7 @@ megasas_sync_pd_seq_num(struct megasas_instance
> *instance, bool pend) {
> > dcmd->mbox.b[0] = MEGASAS_DCMD_MBOX_PEND_FLAG;
> > dcmd->flags = cpu_to_le16(MFI_FRAME_DIR_WRITE);
> > instance->jbod_seq_cmd = cmd;
> > - instance->instancet->issue_dcmd(instance, cmd);
> > - return 0;
> > + return instance->instancet->issue_dcmd(instance, cmd);
> > }
> >
> > dcmd->flags = cpu_to_le16(MFI_FRAME_DIR_READ); @@ -1115,7
> +1114,7 @@
> > megasas_get_map_info(struct megasas_instance *instance) int
> > megasas_sync_map_info(struct megasas_instance *instance) {
> > - int ret = 0, i;
> > + int i;
> > struct megasas_cmd *cmd;
> > struct megasas_dcmd_frame *dcmd;
> > u32 size_sync_info, num_lds;
> > @@ -1182,9 +1181,7 @@ megasas_sync_map_info(struct megasas_instance
> > *instance)
> >
> > instance->map_update_cmd = cmd;
> >
> > - instance->instancet->issue_dcmd(instance, cmd);
> > -
> > - return ret;
> > + return instance->instancet->issue_dcmd(instance, cmd);
> > }
> >
> > /*
> > @@ -2438,18 +2435,12 @@ megasas_build_io_fusion(struct
> megasas_instance *instance,
> > return 0;
> > }
> >
> > -union MEGASAS_REQUEST_DESCRIPTOR_UNION *
> > +static union MEGASAS_REQUEST_DESCRIPTOR_UNION *
> > megasas_get_request_descriptor(struct megasas_instance *instance, u16
> > index) {
> > u8 *p;
> > struct fusion_context *fusion;
> >
> > - if (index >= instance->max_mpt_cmds) {
> > - dev_err(&instance->pdev->dev, "Invalid SMID (0x%x)request
for
> "
> > - "descriptor for scsi%d\n", index,
> > - instance->host->host_no);
> > - return NULL;
> > - }
> > fusion = instance->ctrl_context;
> > p = fusion->req_frames_desc +
> > sizeof(union MEGASAS_REQUEST_DESCRIPTOR_UNION) *
> index; @@ -2959,7
> > +2950,7 @@ build_mpt_mfi_pass_thru(struct megasas_instance *instance,
> > union MEGASAS_REQUEST_DESCRIPTOR_UNION * build_mpt_cmd(struct
> > megasas_instance *instance, struct megasas_cmd *cmd) {
> > - union MEGASAS_REQUEST_DESCRIPTOR_UNION *req_desc;
> > + union MEGASAS_REQUEST_DESCRIPTOR_UNION *req_desc = NULL;
> > u16 index;
> >
> > if (build_mpt_mfi_pass_thru(instance, cmd)) { @@ -2971,9 +2962,6
@@
> > build_mpt_cmd(struct megasas_instance *instance, struct megasas_cmd
> > *cmd)
> >
> > req_desc = megasas_get_request_descriptor(instance, index - 1);
> >
> > - if (!req_desc)
> > - return NULL;
> > -
> > req_desc->Words = 0;
> > req_desc->SCSIIO.RequestFlags =
> (MPI2_REQ_DESCRIPT_FLAGS_SCSI_IO <<
> >
> MEGASAS_REQ_DESCRIPT_FLAGS_TYPE_SHIFT);
> > @@ -2996,11 +2984,6 @@ megasas_issue_dcmd_fusion(struct
> megasas_instance *instance,
> > union MEGASAS_REQUEST_DESCRIPTOR_UNION *req_desc;
> >
> > req_desc = build_mpt_cmd(instance, cmd);
> > - if (!req_desc) {
> > - dev_info(&instance->pdev->dev, "Failed from %s %d\n",
> > - __func__, __LINE__);
> > - return DCMD_NOT_FIRED;
> > - }
> >
> > megasas_fire_cmd_fusion(instance, req_desc);
> > return DCMD_SUCCESS;
> > @@ -3437,12 +3420,6 @@ megasas_issue_tm(struct megasas_instance
> > *instance, u16 device_handle,
> >
> > req_desc = megasas_get_request_descriptor(instance,
> > (cmd_fusion->index - 1));
> > - if (!req_desc) {
> > - dev_err(&instance->pdev->dev, "Failed from %s %d\n",
> > - __func__, __LINE__);
> > - megasas_return_cmd(instance, cmd_mfi);
> > - return -ENOMEM;
> > - }
> >
> > cmd_fusion->request_desc = req_desc;
> > req_desc->Words = 0;
> >
> Here are two things merged into one; the one is the removal of the
'return 0'
> line when calling issue_dcmd(), and the other is the check for an empty
> req_desc.
>
> Personally I would leave the latter in; it's always good to have a
sanity check,
> just in case.
Hi Hannes,
In past, megaraid_sas driver was link list based for MPT and MFI frame and
very unrealistic error handling was added to prevent or delay some bug.
Ideally req_desc is tied with the message frame, so if message frame is
available there must be a corresponding req_desc available as well.
Now megaraid_sas driver gets tags from block layer and also it has already
reserved certain tags for internal use.
We are not allowing any failure to receive fusion command for internal
processing (termed as DCMD in megaraid).
Driver reserve few commands as below and expect command for internal
operation should be always available.
instance->max_scsi_cmds = instance->max_fw_cmds -
(MEGASAS_FUSION_INTERNAL_CMDS +
MEGASAS_FUSION_IOCTL_CMDS);
In case of error handling, since there is a check for return of
megasas_get_cmd() we don't really need req_desc level of check.
cmd = megasas_get_cmd(instance);
if (!cmd) {
printk(KERN_DEBUG "megasas (%s): Failed to get cmd\n",
__FUNCTION__);
return -ENOMEM;
}
>
> But in either case, please make this two patches.
Sure, I will resend two separate patches for this.
But I am not sure if sending only affected patch is good enough or
complete series.
I will figure out if sending an update on this single patch works.
Martin -
Are you OK if I re-send only above impacted patch or like to see complete
series with v2 tag and updated patch numbers?
Thanks,
Shivasharan
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke zSeries & Storage
> h...@suse.com +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton HRB 21284 (AG
> Nürnberg)
