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

[Kiryl Shutsemau <kas@kernel.org>]

On Mon, Sep 07, 2026 at 02:56:26PM -0700, Nicolin Chen wrote:
> On Mon, Sep 07, 2026 at 10:58:34AM +0100, Kiryl Shutsemau (Meta) wrote:
> 
> I still think that cmdq_max_n_shift can slightly tidy things here.
> 
> > +static u32 arm_smmu_queue_max_n_shift(u32 ceiling, u32 ent_sz_shift,
> > +				      u32 entries)
> 
> Here, all three inputs would have been "shifts", instead of two
> "shifts" and one "number of entries".
> 
> > +{
> > +	u32 floor = PAGE_SHIFT - ent_sz_shift;
> > +
> > +	if (!entries)
> > +		return ceiling;
> > +
> > +	return min(ceiling, max(ilog2(entries), floor));
> 
> And I see Sashiko keeps complaining against the ilog2 here:

It does build: GCC 15 and clang 21, at -O2 and -Os, without a warning.

But the reason is not obvious.

ilog2() on a runtime u32 returns int, and minmax.h only accepts an int
against a u32 when __is_nonneg() can prove it non-negative at compile
time.

__ilog2_u32() is fls(n) - 1, so that proof only exists because the
if (entries) guard lets the compiler see entries != 0 through the inlined
fls().

But this is fragile. If a compiler does not get there, or a later change
that moves the guard, it turns it into a BUILD_BUG_ON.

Rather than a max_t() cast, we can give the shift its type first:

	if (entries) {
		new_ceiling = ilog2(entries);
		new_ceiling = max(new_ceiling, floor);
	} else if (is_kdump_kernel()) {

Two u32s, nothing left for the compiler to prove, same result.

If it looks good, I can re-spin v6 with the change.

Thanks for the review and the test!

-- 
  Kiryl Shutsemau / Kirill A. Shutemov