On Mon, Mar 3, 2025 at 11:08 PM Himanshu Madhani
<himanshu.madhani@xxxxxxxxxx> wrote:
>
>
> Hi Chandrakanth,
>
> Code looks fine … just couple of small nits into messages for better
> readability.
>
> On 3/3/25 01:40, Chandrakanth Patil wrote:
> > Task Abort support is added to handle SCSI command timeouts, ensuring
> > recovery and cleanup of timed-out commands. This completes the error
> > handling framework for mpi3mr driver, which already includes device
> > reset, target reset, bus reset, and host reset.
> >
> > Signed-off-by: Chandrakanth Patil <chandrakanth.patil@xxxxxxxxxxxx>
> > Signed-off-by: Sathya Prakash <sathya.prakash@xxxxxxxxxxxx>
> > ---
> > drivers/scsi/mpi3mr/mpi3mr_os.c | 99 +++++++++++++++++++++++++++++++++
> > 1 file changed, 99 insertions(+)
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > index e3547ea42613..6a8f3d3a5668 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > @@ -3839,6 +3839,18 @@ int mpi3mr_issue_tm(struct mpi3mr_ioc *mrioc, u8 tm_type,
> > tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, handle);
> >
> > if (scmd) {
> > + if (tm_type == MPI3_SCSITASKMGMT_TASKTYPE_ABORT_TASK) {
> > + cmd_priv = scsi_cmd_priv(scmd);
> > + if (!cmd_priv)
> > + goto out_unlock;
> > +
> > + struct op_req_qinfo *op_req_q;
> > +
> > + op_req_q = &mrioc->req_qinfo[cmd_priv->req_q_idx];
> > + tm_req.task_host_tag = cpu_to_le16(cmd_priv->host_tag);
> > + tm_req.task_request_queue_id =
> > + cpu_to_le16(op_req_q->qid);
> > + }
> > sdev = scmd->device;
> > sdev_priv_data = sdev->hostdata;
> > scsi_tgt_priv_data = ((sdev_priv_data) ?
> > @@ -4387,6 +4399,92 @@ static int mpi3mr_eh_dev_reset(struct scsi_cmnd *scmd)
> > return retval;
> > }
> >
> > +/**
> > + * mpi3mr_eh_abort- Abort error handling callback
>
>
> ^^^ function comment should be reworded as "Callback function for abort
> error handling"
>
> > + * @scmd: SCSI command reference
> > + *
> > + * Issue Abort Task Management if the command is in LLD scope
> > + * and verify if it is aborted successfully and return status
> > + * accordingly.
> > + *
> > + * Return: SUCCESS of successful abort the scmd else FAILED
> > + */
> > +static int mpi3mr_eh_abort(struct scsi_cmnd *scmd)
> > +{
> > + struct mpi3mr_ioc *mrioc = shost_priv(scmd->device->host);
> > + struct mpi3mr_stgt_priv_data *stgt_priv_data;
> > + struct mpi3mr_sdev_priv_data *sdev_priv_data;
> > + struct scmd_priv *cmd_priv;
> > + u16 dev_handle, timeout = MPI3MR_ABORTTM_TIMEOUT;
> > + u8 resp_code = 0;
> > + int retval = FAILED, ret = 0;
> > + struct request *rq = scsi_cmd_to_rq(scmd);
> > + unsigned long scmd_age_ms = jiffies_to_msecs(jiffies - scmd->jiffies_at_alloc);
> > + unsigned long scmd_age_sec = scmd_age_ms / HZ;
> > +
> > + sdev_printk(KERN_INFO, scmd->device,
> > + "%s: attempting abort task for scmd(%p)\n", mrioc->name, scmd);
> > +
> > + sdev_printk(KERN_INFO, scmd->device,
> > + "%s: scmd(0x%p) is outstanding for %lus %lums, timeout %us, retries %d, allowed %d\n",
> > + mrioc->name, scmd, scmd_age_sec, scmd_age_ms % HZ, rq->timeout / HZ,
> > + scmd->retries, scmd->allowed);
> > +
> > + scsi_print_command(scmd);
> > +
> > + sdev_priv_data = scmd->device->hostdata;
> > + if (!sdev_priv_data || !sdev_priv_data->tgt_priv_data) {
> > + sdev_printk(KERN_INFO, scmd->device,
> > + "%s: device is not available, abort task is not issued\n",
> ^^^
> This message can be reworded as
> "%s: Device not available, Skip issuing abort task\n"
>
> > + mrioc->name);
> > + retval = SUCCESS;
> > + goto out;
> > + }
> > +
> > + stgt_priv_data = sdev_priv_data->tgt_priv_data;
> > + dev_handle = stgt_priv_data->dev_handle;
> > +
> > + cmd_priv = scsi_cmd_priv(scmd);
> > + if (!cmd_priv->in_lld_scope ||
> > + cmd_priv->host_tag == MPI3MR_HOSTTAG_INVALID) {
> > + sdev_printk(KERN_INFO, scmd->device,
> > + "%s: scmd is not in LLD scope, abort task is not issued\n",
>
> Same here, message should be reworded
> "%s: scmd (0x%p) not in LLD scope, Skip issuing Abort Task.\n"
>
> Also add scmd pointer for easy debugging in the future.
>
> > + mrioc->name);
> > + retval = SUCCESS;
> > + goto out;
> > + }
> > +
> > + if (stgt_priv_data->dev_removed) {
> > + sdev_printk(KERN_INFO, scmd->device,
> > + "%s: device(handle = 0x%04x) is removed, abort task is not issued\n",
>
> This message should be reworded as
> "%s: Device (handle = 0x%04x) removed, Skip issuing Abort Task.\n"
>
> > + mrioc->name, dev_handle);
> > + retval = FAILED;
> > + goto out;
> > + }
> > +
> > + ret = mpi3mr_issue_tm(mrioc, MPI3_SCSITASKMGMT_TASKTYPE_ABORT_TASK,
> > + dev_handle, sdev_priv_data->lun_id, MPI3MR_HOSTTAG_BLK_TMS,
> > + timeout, &mrioc->host_tm_cmds, &resp_code, scmd);
> > +
> > + if (ret)
> > + goto out;
> > +
> > + if (cmd_priv->in_lld_scope) {
> > + sdev_printk(KERN_INFO, scmd->device,
> > + "%s: scmd was not terminated, abort task is failed\n",
>
> This message could be reworded as
> "%s: Abort task failed. scmd (0x%p) was not terminated.\n"
>
>
> > + mrioc->name);
> > + goto out;
> > + }
> > +
> > + retval = SUCCESS;
> > +out:
> > + sdev_printk(KERN_INFO, scmd->device,
> > + "%s: abort task is %s for scmd(%p)\n", mrioc->name,
> > + ((retval == SUCCESS) ? "SUCCESS" : "FAILED"), scmd);
> > +
>
> Message printed for Successful case should be "SUCCEEDED" for better
> readability. Something like
>
> "%s: Abort Task %s for scmd (0x%p)\n"
>
> > + return retval;
> > +}
> > +
> > /**
> > * mpi3mr_scan_start - Scan start callback handler
> > * @shost: SCSI host reference
> > @@ -5069,6 +5167,7 @@ static const struct scsi_host_template mpi3mr_driver_template = {
> > .scan_finished = mpi3mr_scan_finished,
> > .scan_start = mpi3mr_scan_start,
> > .change_queue_depth = mpi3mr_change_queue_depth,
> > + .eh_abort_handler = mpi3mr_eh_abort,
> > .eh_device_reset_handler = mpi3mr_eh_dev_reset,
> > .eh_target_reset_handler = mpi3mr_eh_target_reset,
> > .eh_bus_reset_handler = mpi3mr_eh_bus_reset,
>
> once you fix these messages feel free to add
>
> Reviewed-by: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
>
> --
> Himanshu Madhani Oracle Linux Engineering
Thanks for the review, Himanshu.
I have made the suggested changes and resubmitted the patch as V2.
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
