Re: [PATCH v6 1/2] iommu/arm-smmu-v3: Add a cmdq_max_n_shift module parameter

[Jason Gunthorpe <jgg@nvidia.com>]

> [ ... 34 lines skipped ... ]
> @@ -40,6 +40,11 @@ module_param(disable_msipolling, bool, 0444);
>  MODULE_PARM_DESC(disable_msipolling,
>  	"Disable MSI-based polling for CMD_SYNC completion.");
>  
> +static unsigned int cmdq_max_n_shift;
> +module_param(cmdq_max_n_shift, uint, 0444);
> +MODULE_PARM_DESC(cmdq_max_n_shift,
> +	"Cap on the command queue depth, as log2 of the number of entries. Zero means the hardware maximum; the queue never shrinks below one page.");

The patch looks ok

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

But I'd rather use lg2 as name, especially user visible, instead of
n_shift. "shift" is a word for bitwise stuff, this is a 2**N log2
encoded value.

> [ ... 9 lines skipped ... ]
> +/**
> + * arm_smmu_queue_max_n_shift() - pick the log2 depth of a queue
> + * @ceiling: default log2 depth ceiling of the queue
> + * @ent_sz_shift: log2 of the queue entry size in bytes
> + * @shift: log2 depth asked for, or zero for the default
> + *
> + * @shift is floored at one page, because coherent DMA is page granular: a
> + * shallower queue occupies the same memory as one that fills the page, and
> + * arm_smmu_init_one_queue() stops shrinking at a page too.
> + */
> +static u32 arm_smmu_queue_max_n_shift(u32 ceiling, u32 ent_sz_shift, u32 shift)

These names took me a while to figure out. Something like this seems
much clearer:

 hw_max_lg2
 ent_size_lg2
 user_limit_lg2

And the flow can be simplified alot:

static u32 arm_smmu_queue_max_lg2(u32 hw_max_lg2, u32 ent_sz_lg2,
				  u32 user_limit_lg2)
{
	u32 floor_lg2 = PAGE_SHIFT - ent_sz_lg2;

	if (is_kdump_kernel())
		return min(hw_max_lg2, floor_lg2);
	user_limit_lg2 = max(user_limit_lg2, floor_lg2);
	return min(hw_max_lg2, user_limit_lg2);

Just pass the MAX's as the user limit:

static inline u32 arm_smmu_evtq_max_n_shift(u32 ceiling)
{
	return arm_smmu_queue_max_lg2(ceiling, EVTQ_ENT_SZ_SHIFT,
				      EVTQ_MAX_SZ_SHIFT);
With only one min:

	return arm_smmu_queue_max_lg2(ceiling, CMDQ_ENT_SZ_SHIFT,
				      min(CMDQ_MAX_SZ_SHIFT, cmdq_max_n_shift));

Jason

-- 
Jason