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

[Kiryl Shutsemau <kas@kernel.org>]

On Tue, Sep 08, 2026 at 02:56:20PM -0700, Nicolin Chen wrote:
> 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 :-/

Okay, fair enough. max_n_shift it is. Will post v6.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov