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

Johannes Thumshirn
2024-04-09T15:34:22Z

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;
}