diff mbox series

[17/24] snic: reserve tag for TMF

Message ID 20220502213820.3187-18-hare@suse.de
State Superseded
Headers show
Series [01/24] csiostor: use fc_block_rport() | expand

Commit Message

Hannes Reinecke May 2, 2022, 9:38 p.m. UTC 
Rather than re-using the failed command the snic driver should
reserve one command for TMFs.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/snic/snic.h      |  3 ++-
 drivers/scsi/snic/snic_main.c |  3 +++
 drivers/scsi/snic/snic_scsi.c | 43 +++++++++++++++++------------------
 3 files changed, 26 insertions(+), 23 deletions(-)

Comments

Bart Van Assche May 3, 2022, 4:18 p.m. UTC | #1 
On 5/2/22 14:38, Hannes Reinecke wrote:
> diff --git a/drivers/scsi/snic/snic_main.c b/drivers/scsi/snic/snic_main.c
> index 29d56396058c..f344cbc27923 100644
> --- a/drivers/scsi/snic/snic_main.c
> +++ b/drivers/scsi/snic/snic_main.c
> @@ -512,6 +512,9 @@ snic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   					 max_t(u32, SNIC_MIN_IO_REQ, max_ios));
>   
>   	snic->max_tag_id = shost->can_queue;
> +	/* Reserve one reset command */
> +	shost->can_queue--;
> +	snic->tmf_tag_id = shost->can_queue;
>   
>   	shost->max_lun = snic->config.luns_per_tgt;
>   	shost->max_id = SNIC_MAX_TARGET;

Hmm ... shouldn't cmd_per_lun also be decreased?

Thanks,

Bart.
Bart Van Assche May 3, 2022, 8:09 p.m. UTC | #2 
On 5/3/22 11:31, Hannes Reinecke wrote:
> On 5/3/22 09:18, Bart Van Assche wrote:
>> On 5/2/22 14:38, Hannes Reinecke wrote:
>>> diff --git a/drivers/scsi/snic/snic_main.c 
>>> b/drivers/scsi/snic/snic_main.c
>>> index 29d56396058c..f344cbc27923 100644
>>> --- a/drivers/scsi/snic/snic_main.c
>>> +++ b/drivers/scsi/snic/snic_main.c
>>> @@ -512,6 +512,9 @@ snic_probe(struct pci_dev *pdev, const struct 
>>> pci_device_id *ent)
>>>                        max_t(u32, SNIC_MIN_IO_REQ, max_ios));
>>>       snic->max_tag_id = shost->can_queue;
>>> +    /* Reserve one reset command */
>>> +    shost->can_queue--;
>>> +    snic->tmf_tag_id = shost->can_queue;
>>>       shost->max_lun = snic->config.luns_per_tgt;
>>>       shost->max_id = SNIC_MAX_TARGET;
>>
>> Hmm ... shouldn't cmd_per_lun also be decreased?
>>
> Shudder. No. Why?

I found the answer. The following code from scsi_add_host_with_dma() 
reduces cmd_per_lun so it is not necessary to do that from inside 
snic_probe():

	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
				   shost->can_queue);

Bart.
John Garry May 9, 2022, 6:44 a.m. UTC | #3 
On 07/05/2022 08:32, Hannes Reinecke wrote:
> On 5/6/22 06:28, John Garry wrote:
>>> diff --git a/drivers/scsi/snic/snic_main.c 
>>> b/drivers/scsi/snic/snic_main.c
>>> index 29d56396058c..f344cbc27923 100644
>>> --- a/drivers/scsi/snic/snic_main.c
>>> +++ b/drivers/scsi/snic/snic_main.c
>>> @@ -512,6 +512,9 @@ snic_probe(struct pci_dev *pdev, const struct 
>>> pci_device_id *ent)
>>>                        max_t(u32, SNIC_MIN_IO_REQ, max_ios));
>>>       snic->max_tag_id = shost->can_queue;
>>> +    /* Reserve one reset command */
>>> +    shost->can_queue--;
>>> +    snic->tmf_tag_id = shost->can_queue;
>>>       shost->max_lun = snic->config.luns_per_tgt;
>>>       shost->max_id = SNIC_MAX_TARGET;
>>> diff --git a/drivers/scsi/snic/snic_scsi.c 
>>> b/drivers/scsi/snic/snic_scsi.c
>>> index 5f17666f3e1d..e18c8c5e4b46 100644
>>> --- a/drivers/scsi/snic/snic_scsi.c
>>> +++ b/drivers/scsi/snic/snic_scsi.c
>>> @@ -1018,17 +1018,6 @@ snic_hba_reset_cmpl_handler(struct snic *snic, 
>>> struct snic_fw_req *fwreq)
>>>                 "reset_cmpl: type = %x, hdr_stat = %x, cmnd_id = %x, 
>>> hid = %x, ctx = %lx\n",
>>>                 typ, hdr_stat, cmnd_id, hid, ctx);
>>> -    /* spl case, host reset issued through ioctl */
>>> -    if (cmnd_id == SCSI_NO_TAG) {
>>> -        rqi = (struct snic_req_info *) ctx;
>>> -        SNIC_HOST_INFO(snic->shost,
>>> -                   "reset_cmpl:Tag %d ctx %lx cmpl stat %s\n",
>>> -                   cmnd_id, ctx, snic_io_status_to_str(hdr_stat));
>>> -        sc = rqi->sc;
>>> -
>>> -        goto ioctl_hba_rst;
>>> -    }
>>> -
>>>       if (cmnd_id >= snic->max_tag_id) {
>>>           SNIC_HOST_ERR(snic->shost,
>>>                     "reset_cmpl: Tag 0x%x out of Range,HdrStat %s\n",
>>> @@ -1039,7 +1028,6 @@ snic_hba_reset_cmpl_handler(struct snic *snic, 
>>> struct snic_fw_req *fwreq)
>>>       }
>>>       sc = scsi_host_find_tag(snic->shost, cmnd_id);
>>> -ioctl_hba_rst:
>>>       if (!sc) {
>>>           atomic64_inc(&snic->s_stats.io.sc_null);
>>>           SNIC_HOST_ERR(snic->shost,
>>> @@ -1725,7 +1713,7 @@ snic_dr_clean_single_req(struct snic *snic,
>>>   {
>>>       struct snic_req_info *rqi = NULL;
>>>       struct snic_tgt *tgt = NULL;
>>> -    struct scsi_cmnd *sc = NULL;
>>> +    struct scsi_cmnd *sc;
>>>       spinlock_t *io_lock = NULL;
>>>       u32 sv_state = 0, tmf = 0;
>>>       DECLARE_COMPLETION_ONSTACK(tm_done);
>>> @@ -2238,13 +2226,6 @@ snic_issue_hba_reset(struct snic *snic, struct 
>>> scsi_cmnd *sc)
>>>           goto hba_rst_end;
>>>       }
>>> -    if (snic_cmd_tag(sc) == SCSI_NO_TAG) {
>>> -        memset(scsi_cmd_priv(sc), 0,
>>> -            sizeof(struct snic_internal_io_state));
>>> -        SNIC_HOST_INFO(snic->shost, "issu_hr:Host reset thru 
>>> ioctl.\n");
>>> -        rqi->sc = sc;
>>> -    }
>>> -
>>>       req = rqi_to_req(rqi);
>>>       io_lock = snic_io_lock_hash(snic, sc);
>>> @@ -2319,11 +2300,13 @@ snic_issue_hba_reset(struct snic *snic, 
>>> struct scsi_cmnd *sc)
>>>   } /* end of snic_issue_hba_reset */
>>>   int
>>> -snic_reset(struct Scsi_Host *shost, struct scsi_cmnd *sc)
>>> +snic_reset(struct Scsi_Host *shost)
>>>   {
>>>       struct snic *snic = shost_priv(shost);
>>> +    struct scsi_cmnd *sc = NULL;
>>>       enum snic_state sv_state;
>>>       unsigned long flags;
>>> +    u32 start_time  = jiffies;
>>>       int ret = FAILED;
>>>       /* Set snic state as SNIC_FWRESET*/
>>> @@ -2348,6 +2331,18 @@ snic_reset(struct Scsi_Host *shost, struct 
>>> scsi_cmnd *sc)
>>>       while (atomic_read(&snic->ios_inflight))
>>>           schedule_timeout(msecs_to_jiffies(1));
>>> +    sc = scsi_host_find_tag(shost, snic->tmf_tag_id);
>>
>> Maybe I am missing something but this does not seem right. As I see, 
>> blk-mq has driver tags in range [0, snic->tmf_tag_id - 1], so we 
>> cannot call scsi_host_find_tag() -> 
>> blk_mq_unique_tag_to_tag(snic->tmf_tag_id)
>>

Hi Hannes,

 > We do decrease 'can_queue' by '1' just at the start of this patch, hence
 > the last tag is always available for TMF.

So has this code changed this here:
https://lore.kernel.org/linux-scsi/de4a7479-3dbf-0842-f8f7-5d82b8bd6ea6@suse.de/

At a glance it looks the same.

Thanks,
John
diff mbox series

Patch

diff --git a/drivers/scsi/snic/snic.h b/drivers/scsi/snic/snic.h
index 4ec7e30678e1..e40492d36031 100644
--- a/drivers/scsi/snic/snic.h
+++ b/drivers/scsi/snic/snic.h
@@ -310,6 +310,7 @@  struct snic {
 	struct list_head spl_cmd_list;
 
 	unsigned int max_tag_id;
+	unsigned int tmf_tag_id;
 	atomic_t ios_inflight;		/* io in flight counter */
 
 	struct vnic_snic_config config;
@@ -380,7 +381,7 @@  int snic_queuecommand(struct Scsi_Host *, struct scsi_cmnd *);
 int snic_abort_cmd(struct scsi_cmnd *);
 int snic_device_reset(struct scsi_cmnd *);
 int snic_host_reset(struct scsi_cmnd *);
-int snic_reset(struct Scsi_Host *, struct scsi_cmnd *);
+int snic_reset(struct Scsi_Host *);
 void snic_shutdown_scsi_cleanup(struct snic *);
 
 
diff --git a/drivers/scsi/snic/snic_main.c b/drivers/scsi/snic/snic_main.c
index 29d56396058c..f344cbc27923 100644
--- a/drivers/scsi/snic/snic_main.c
+++ b/drivers/scsi/snic/snic_main.c
@@ -512,6 +512,9 @@  snic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 					 max_t(u32, SNIC_MIN_IO_REQ, max_ios));
 
 	snic->max_tag_id = shost->can_queue;
+	/* Reserve one reset command */
+	shost->can_queue--;
+	snic->tmf_tag_id = shost->can_queue;
 
 	shost->max_lun = snic->config.luns_per_tgt;
 	shost->max_id = SNIC_MAX_TARGET;
diff --git a/drivers/scsi/snic/snic_scsi.c b/drivers/scsi/snic/snic_scsi.c
index 5f17666f3e1d..e18c8c5e4b46 100644
--- a/drivers/scsi/snic/snic_scsi.c
+++ b/drivers/scsi/snic/snic_scsi.c
@@ -1018,17 +1018,6 @@  snic_hba_reset_cmpl_handler(struct snic *snic, struct snic_fw_req *fwreq)
 		      "reset_cmpl: type = %x, hdr_stat = %x, cmnd_id = %x, hid = %x, ctx = %lx\n",
 		      typ, hdr_stat, cmnd_id, hid, ctx);
 
-	/* spl case, host reset issued through ioctl */
-	if (cmnd_id == SCSI_NO_TAG) {
-		rqi = (struct snic_req_info *) ctx;
-		SNIC_HOST_INFO(snic->shost,
-			       "reset_cmpl:Tag %d ctx %lx cmpl stat %s\n",
-			       cmnd_id, ctx, snic_io_status_to_str(hdr_stat));
-		sc = rqi->sc;
-
-		goto ioctl_hba_rst;
-	}
-
 	if (cmnd_id >= snic->max_tag_id) {
 		SNIC_HOST_ERR(snic->shost,
 			      "reset_cmpl: Tag 0x%x out of Range,HdrStat %s\n",
@@ -1039,7 +1028,6 @@  snic_hba_reset_cmpl_handler(struct snic *snic, struct snic_fw_req *fwreq)
 	}
 
 	sc = scsi_host_find_tag(snic->shost, cmnd_id);
-ioctl_hba_rst:
 	if (!sc) {
 		atomic64_inc(&snic->s_stats.io.sc_null);
 		SNIC_HOST_ERR(snic->shost,
@@ -1725,7 +1713,7 @@  snic_dr_clean_single_req(struct snic *snic,
 {
 	struct snic_req_info *rqi = NULL;
 	struct snic_tgt *tgt = NULL;
-	struct scsi_cmnd *sc = NULL;
+	struct scsi_cmnd *sc;
 	spinlock_t *io_lock = NULL;
 	u32 sv_state = 0, tmf = 0;
 	DECLARE_COMPLETION_ONSTACK(tm_done);
@@ -2238,13 +2226,6 @@  snic_issue_hba_reset(struct snic *snic, struct scsi_cmnd *sc)
 		goto hba_rst_end;
 	}
 
-	if (snic_cmd_tag(sc) == SCSI_NO_TAG) {
-		memset(scsi_cmd_priv(sc), 0,
-			sizeof(struct snic_internal_io_state));
-		SNIC_HOST_INFO(snic->shost, "issu_hr:Host reset thru ioctl.\n");
-		rqi->sc = sc;
-	}
-
 	req = rqi_to_req(rqi);
 
 	io_lock = snic_io_lock_hash(snic, sc);
@@ -2319,11 +2300,13 @@  snic_issue_hba_reset(struct snic *snic, struct scsi_cmnd *sc)
 } /* end of snic_issue_hba_reset */
 
 int
-snic_reset(struct Scsi_Host *shost, struct scsi_cmnd *sc)
+snic_reset(struct Scsi_Host *shost)
 {
 	struct snic *snic = shost_priv(shost);
+	struct scsi_cmnd *sc = NULL;
 	enum snic_state sv_state;
 	unsigned long flags;
+	u32 start_time  = jiffies;
 	int ret = FAILED;
 
 	/* Set snic state as SNIC_FWRESET*/
@@ -2348,6 +2331,18 @@  snic_reset(struct Scsi_Host *shost, struct scsi_cmnd *sc)
 	while (atomic_read(&snic->ios_inflight))
 		schedule_timeout(msecs_to_jiffies(1));
 
+	sc = scsi_host_find_tag(shost, snic->tmf_tag_id);
+	if (!sc) {
+		SNIC_HOST_ERR(shost,
+			      "reset:Host Reset Failed to allocate sc.\n");
+		spin_lock_irqsave(&snic->snic_lock, flags);
+		snic_set_state(snic, sv_state);
+		spin_unlock_irqrestore(&snic->snic_lock, flags);
+		atomic64_inc(&snic->s_stats.reset.hba_reset_fail);
+		ret = FAILED;
+
+		goto reset_end;
+	}
 	ret = snic_issue_hba_reset(snic, sc);
 	if (ret) {
 		SNIC_HOST_ERR(shost,
@@ -2365,6 +2360,10 @@  snic_reset(struct Scsi_Host *shost, struct scsi_cmnd *sc)
 	ret = SUCCESS;
 
 reset_end:
+	SNIC_TRC(shost->host_no, sc ? snic_cmd_tag(sc) : SCSI_NO_TAG,
+		 (ulong) sc, jiffies_to_msecs(jiffies - start_time),
+		 0, 0, 0);
+
 	return ret;
 } /* end of snic_reset */
 
@@ -2387,7 +2386,7 @@  snic_host_reset(struct scsi_cmnd *sc)
 		      sc, sc->cmnd[0], scsi_cmd_to_rq(sc),
 		      snic_cmd_tag(sc), CMD_FLAGS(sc));
 
-	ret = snic_reset(shost, sc);
+	ret = snic_reset(shost);
 
 	SNIC_TRC(shost->host_no, snic_cmd_tag(sc), (ulong) sc,
 		 jiffies_to_msecs(jiffies - start_time),