[21/23] mpi3mr: switch to using ->device_configure

Commit Message

Christoph Hellwig April 9, 2024, 2:37 p.m. UTC

Switch to the ->device_configure method instead of ->slave_configure
and update the block limits on the passed in queue_limits instead
of using the per-limit accessors.

Note that mpi3mr also updates the limits from an event handler that
iterates all SCSI devices.  This is also updated to use the
queue_limits, but the complete locking of this path probably means
it already is completely broken and needs a proper audit.

Signed-off-by: Christoph Hellwig <[e-mail]>
Reviewed-by: Damien Le Moal <[e-mail]>
Reviewed-by: Hannes Reinecke <[e-mail]>
---
 drivers/scsi/mpi3mr/mpi3mr.h    |  1 -
 drivers/scsi/mpi3mr/mpi3mr_os.c | 76 ++++++++++++++-------------------
 2 files changed, 31 insertions(+), 46 deletions(-)

Comments

Johannes Thumshirn April 9, 2024, 3:34 p.m. UTC | #1

On 09.04.24 16:42, Christoph Hellwig wrote:
> +static void mpi3mr_configure_nvme_dev(struct mpi3mr_tgt_dev *tgt_dev,
> +		struct queue_limits *lim)
> +{
> +	u8 pgsz = tgt_dev->dev_spec.pcie_inf.pgsz ? : MPI3MR_DEFAULT_PGSZEXP;
> +
> +	lim->max_hw_sectors = tgt_dev->dev_spec.pcie_inf.mdts / 512;
> +	lim->virt_boundary_mask = (1 << pgsz) - 1;
> +}
> +
> +static void mpi3mr_configure_tgt_dev(struct mpi3mr_tgt_dev *tgt_dev,
> +		struct queue_limits *lim)
> +{
> +	if (tgt_dev->dev_type == MPI3_DEVICE_DEVFORM_PCIE &&
> +	    (tgt_dev->dev_spec.pcie_inf.dev_info &
> +	     MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_MASK) ==
> +			MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_NVME_DEVICE)
> +		mpi3mr_configure_nvme_dev(tgt_dev, lim);
> +}
> +

[...]

> -	switch (tgt_dev->dev_type) {
> -	case MPI3_DEVICE_DEVFORM_PCIE:
> -		/*The block layer hw sector size = 512*/
> -		if ((tgt_dev->dev_spec.pcie_inf.dev_info &
> -		    MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_MASK) ==
> -		    MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_NVME_DEVICE) {
> -			blk_queue_max_hw_sectors(sdev->request_queue,
> -			    tgt_dev->dev_spec.pcie_inf.mdts / 512);
> -			if (tgt_dev->dev_spec.pcie_inf.pgsz == 0)
> -				blk_queue_virt_boundary(sdev->request_queue,
> -				    ((1 << MPI3MR_DEFAULT_PGSZEXP) - 1));
> -			else
> -				blk_queue_virt_boundary(sdev->request_queue,
> -				    ((1 << tgt_dev->dev_spec.pcie_inf.pgsz) - 1));
> -		}
> -		break;
> -	default:
> -		break;
> -	}
> -
> +	mpi3mr_configure_tgt_dev(tgt_dev, lim);

Why did you split this into two functions, with the innermost function 
being only called once?

While it's slightly less of a mess to read this would be fully 
sufficient and IMHO more readable (please excuse the whitespace damage):

static void mpi3mr_configure_tgt_dev(struct mpi3mr_tgt_dev *tgt_dev,
		struct queue_limits *lim)
{
	u8 pgsz;

	if (tgt_dev->dev_type != MPI3_DEVICE_DEVFORM_PCIE)
		return;
	if (tgt_dev->dev_spec.pcie_inf.dev_info &
	     MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_MASK) !=
			MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_NVME_DEVICE)
		return;

	if (tgt_dev->dev_spec.pcie_inf.pgsz)
		pgsz = tgt_dev->dev_spec.pcie_inf.pgsz;
	else
		pgsz = MPI3MR_DEFAULT_PGSZEXP;

	lim->max_hw_sectors = tgt_dev->dev_spec.pcie_inf.mdts / 512;
	lim->virt_boundary_mask = (1 << pgsz) - 1;
}

Christoph Hellwig April 10, 2024, 4:25 a.m. UTC | #2

On Tue, Apr 09, 2024 at 03:34:13PM +0000, Johannes Thumshirn wrote:
> Why did you split this into two functions, with the innermost function 
> being only called once?
> 
> While it's slightly less of a mess to read this would be fully 
> sufficient and IMHO more readable (please excuse the whitespace damage):

Because having a helper for a specific type of device just simply
is good code struture.  It might not matter too much now, but as
soon as something else gets added your version turns into a mess
quickly.

But it turns out the rebase caused a real mess in this patch as it
marks a function static that now gets used outside the fіle in the
scsi tree, and has a weird rename in not actually visible characters,
so I'm resending it.

Patch

diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
index d1c93978f28a58..f5a1529fa53715 100644
--- a/drivers/scsi/mpi3mr/mpi3mr.h
+++ b/drivers/scsi/mpi3mr/mpi3mr.h
@@ -1352,7 +1352,6 @@  void mpi3mr_wait_for_host_io(struct mpi3mr_ioc *mrioc, u32 timeout);
 void mpi3mr_cleanup_fwevt_list(struct mpi3mr_ioc *mrioc);
 void mpi3mr_flush_host_io(struct mpi3mr_ioc *mrioc);
 void mpi3mr_invalidate_devhandles(struct mpi3mr_ioc *mrioc);
-void mpi3mr_refresh_tgtdevs(struct mpi3mr_ioc *mrioc);
 void mpi3mr_flush_delayed_cmd_lists(struct mpi3mr_ioc *mrioc);
 void mpi3mr_check_rh_fault_ioc(struct mpi3mr_ioc *mrioc, u32 reason_code);
 void mpi3mr_print_fault_info(struct mpi3mr_ioc *mrioc);
diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 5f975e0db38831..17b4e70b9dbaf6 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -986,6 +986,25 @@  static int mpi3mr_change_queue_depth(struct scsi_device *sdev,
 	return retval;
 }
 
+static void mpi3mr_configure_nvme_dev(struct mpi3mr_tgt_dev *tgt_dev,
+		struct queue_limits *lim)
+{
+	u8 pgsz = tgt_dev->dev_spec.pcie_inf.pgsz ? : MPI3MR_DEFAULT_PGSZEXP;
+
+	lim->max_hw_sectors = tgt_dev->dev_spec.pcie_inf.mdts / 512;
+	lim->virt_boundary_mask = (1 << pgsz) - 1;
+}
+
+static void mpi3mr_configure_tgt_dev(struct mpi3mr_tgt_dev *tgt_dev,
+		struct queue_limits *lim)
+{
+	if (tgt_dev->dev_type == MPI3_DEVICE_DEVFORM_PCIE &&
+	    (tgt_dev->dev_spec.pcie_inf.dev_info &
+	     MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_MASK) ==
+			MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_NVME_DEVICE)
+		mpi3mr_configure_nvme_dev(tgt_dev, lim);
+}
+
 /**
  * mpi3mr_update_sdev - Update SCSI device information
  * @sdev: SCSI device reference
@@ -1001,31 +1020,17 @@  static void
 mpi3mr_update_sdev(struct scsi_device *sdev, void *data)
 {
 	struct mpi3mr_tgt_dev *tgtdev;
+	struct queue_limits lim;
 
 	tgtdev = (struct mpi3mr_tgt_dev *)data;
 	if (!tgtdev)
 		return;
 
 	mpi3mr_change_queue_depth(sdev, tgtdev->q_depth);
-	switch (tgtdev->dev_type) {
-	case MPI3_DEVICE_DEVFORM_PCIE:
-		/*The block layer hw sector size = 512*/
-		if ((tgtdev->dev_spec.pcie_inf.dev_info &
-		    MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_MASK) ==
-		    MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_NVME_DEVICE) {
-			blk_queue_max_hw_sectors(sdev->request_queue,
-			    tgtdev->dev_spec.pcie_inf.mdts / 512);
-			if (tgtdev->dev_spec.pcie_inf.pgsz == 0)
-				blk_queue_virt_boundary(sdev->request_queue,
-				    ((1 << MPI3MR_DEFAULT_PGSZEXP) - 1));
-			else
-				blk_queue_virt_boundary(sdev->request_queue,
-				    ((1 << tgtdev->dev_spec.pcie_inf.pgsz) - 1));
-		}
-		break;
-	default:
-		break;
-	}
+
+	lim = queue_limits_start_update(sdev->request_queue);
+	mpi3mr_configure_tgt_dev(tgtdev, &lim);
+	WARN_ON_ONCE(queue_limits_commit_update(sdev->request_queue, &lim));
 }
 
 /**
@@ -1038,8 +1043,7 @@  mpi3mr_update_sdev(struct scsi_device *sdev, void *data)
  *
  * Return: Nothing.
  */
-
-void mpi3mr_refresh_tgtdevs(struct mpi3mr_ioc *mrioc)
+static void mpi3mr_rfresh_tgtdevs(struct mpi3mr_ioc *mrioc)
 {
 	struct mpi3mr_tgt_dev *tgtdev, *tgtdev_next;
 	struct mpi3mr_stgt_priv_data *tgt_priv;
@@ -4393,15 +4397,17 @@  static void mpi3mr_target_destroy(struct scsi_target *starget)
 }
 
 /**
- * mpi3mr_slave_configure - Slave configure callback handler
+ * mpi3mr_device_configure - Slave configure callback handler
  * @sdev: SCSI device reference
+ * @lim: queue limits
  *
  * Configure queue depth, max hardware sectors and virt boundary
  * as required
  *
  * Return: 0 always.
  */
-static int mpi3mr_slave_configure(struct scsi_device *sdev)
+static int mpi3mr_device_configure(struct scsi_device *sdev,
+		struct queue_limits *lim)
 {
 	struct scsi_target *starget;
 	struct Scsi_Host *shost;
@@ -4432,28 +4438,8 @@  static int mpi3mr_slave_configure(struct scsi_device *sdev)
 	sdev->eh_timeout = MPI3MR_EH_SCMD_TIMEOUT;
 	blk_queue_rq_timeout(sdev->request_queue, MPI3MR_SCMD_TIMEOUT);
 
-	switch (tgt_dev->dev_type) {
-	case MPI3_DEVICE_DEVFORM_PCIE:
-		/*The block layer hw sector size = 512*/
-		if ((tgt_dev->dev_spec.pcie_inf.dev_info &
-		    MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_MASK) ==
-		    MPI3_DEVICE0_PCIE_DEVICE_INFO_TYPE_NVME_DEVICE) {
-			blk_queue_max_hw_sectors(sdev->request_queue,
-			    tgt_dev->dev_spec.pcie_inf.mdts / 512);
-			if (tgt_dev->dev_spec.pcie_inf.pgsz == 0)
-				blk_queue_virt_boundary(sdev->request_queue,
-				    ((1 << MPI3MR_DEFAULT_PGSZEXP) - 1));
-			else
-				blk_queue_virt_boundary(sdev->request_queue,
-				    ((1 << tgt_dev->dev_spec.pcie_inf.pgsz) - 1));
-		}
-		break;
-	default:
-		break;
-	}
-
+	mpi3mr_configure_tgt_dev(tgt_dev, lim);
 	mpi3mr_tgtdev_put(tgt_dev);
-
 	return retval;
 }
 
@@ -4921,7 +4907,7 @@  static const struct scsi_host_template mpi3mr_driver_template = {
 	.queuecommand			= mpi3mr_qcmd,
 	.target_alloc			= mpi3mr_target_alloc,
 	.slave_alloc			= mpi3mr_slave_alloc,
-	.slave_configure		= mpi3mr_slave_configure,
+	.device_configure		= mpi3mr_device_configure,
 	.target_destroy			= mpi3mr_target_destroy,
 	.slave_destroy			= mpi3mr_slave_destroy,
 	.scan_finished			= mpi3mr_scan_finished,