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

[Nicolin Chen <nicolinc@nvidia.com>]

On Tue, Sep 08, 2026 at 10:19:44AM +0100, Kiryl Shutsemau wrote:
> 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.

It looks probably okay.. though I still don't get why you aren't
picking the straightforward "max_n_shift" over "max_entries".

max_n_shift is used by both HW and SW, and it does not have such
converting problem or need min/max, making the code cleaner :-/

Nicolin