Re: [PATCH 6/6] scsi: mvsas: Use sas_task_find_rq() for tagging

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 29/09/2022 03:22, Damien Le Moal wrote:
On 9/28/22 21:27, John Garry wrote:
The request associated with a scsi command coming from the block layer
has a unique tag, so use that when possible for getting a slot.

Unfortunately we don't support reserved commands in the SCSI midlayer yet.
As such, SMP tasks - as an example - will not have a request associated, so
in the interim continue to manage those tags for that type of sas_task
internally.

We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
but what those 2 slots are used for is not obvious.

Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
---
  drivers/scsi/mvsas/mv_defs.h |  1 +
  drivers/scsi/mvsas/mv_init.c |  4 ++--
  drivers/scsi/mvsas/mv_sas.c  | 22 +++++++++++++++++-----
  drivers/scsi/mvsas/mv_sas.h  |  1 -
  4 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
index 7123a2efbf58..8ef174cd4d37 100644
--- a/drivers/scsi/mvsas/mv_defs.h
+++ b/drivers/scsi/mvsas/mv_defs.h
@@ -40,6 +40,7 @@ enum driver_configuration {
  	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
  	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
  	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
+	MVS_RSVD_SLOTS		= 4,
  	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
  };
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index c85fb812ad43..d834ed9e8e4a 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
  			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
  			goto err_out;
  	}
-	mvi->tags_num = slot_nr;
+	mvi->tags_num = MVS_RSVD_SLOTS;

Same comment as for pm8001: do you really need this field if the value
is always MVS_RSVD_SLOTS ?

Right, I don't need this struct member. Again I can just use this macro directly.


return 0;
  err_out:
@@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
  	mvi->sas = sha;
  	mvi->shost = shost;
- mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
+	mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL);

Field name ? reserved_tags ?
Also, the alloc seems wrong. This will allocate 4 bytes, but you only
need 4 bits. You could make this an unsigned long and not allocate
anything.

Well spotted. I should have questioned more why they had >>3 previously.

But I would rather keep as a bitmap, i.e. *unsigned long for simplicity.

Same remark for pm8001 by the way.

I think it's ok as it uses bitmap_zalloc()


That would cap MVS_RSVD_SLOTS to BITS_PER_LONG maximum, but that is easy
to check at compile time with a #if/#error.


As above, I'd rather keep as a bitmap. It's a little inefficient, but is a one off in the driver.

Thanks,
John





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux