Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: Ml2BFr7B6cELGF0ItfXK0jp8O3bZWofF7xmYsKXe4SXEmaocAOn60RsqQNfbsNwjoS/VOAJcil
 +EBqBTeDEEuA==
IronPort-SDR: V9yfDqOKgEkDoVuXKkzHcvKp3hdqolILRcCM9667pnbs8jPDD4VbFLstrqg3bEWPe9hx1SRe1T
 a1VzYd155XcQ==
Date:   Tue, 26 Jan 2021 23:18:14 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Paul Gortmaker <paul.gortmaker@windriver.com>
Cc:     linux-kernel@vger.kernel.org, lizefan@huawei.com, mingo@kernel.org,
        tglx@linutronix.de, josh@joshtriplett.org, yury.norov@gmail.com,
        peterz@infradead.org, paulmck@kernel.org, fweisbec@gmail.com,
        linux@rasmusvillemoes.dk
Subject: Re: [PATCH 3/8] lib: bitmap: fold nbits into region struct
Message-ID: <YBCHFrO3LS+Hztx/@smile.fi.intel.com>
References: <20210126171141.122639-1-paul.gortmaker@windriver.com>
 <20210126171141.122639-4-paul.gortmaker@windriver.com>
 <YBCGqfW0hKSgo9Rl@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YBCGqfW0hKSgo9Rl@smile.fi.intel.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Tue, Jan 26, 2021 at 11:16:25PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 26, 2021 at 12:11:36PM -0500, Paul Gortmaker wrote:
> > This will reduce parameter passing and enable using nbits as part
> > of future dynamic region parameter parsing.

...

> >  	struct region r;
> >  	long ret;
> >  
> > -	bitmap_zero(maskp, nmaskbits);
> > +	r.nbits = nmaskbits;

Alternatively (though I personally don't prefer it) you can clarify in the
definition block the initial values.

	struct region r = { .nbist = nmaskbits };

> > +	bitmap_zero(maskp, r.nbits);
> 
> This sounds not right from style perspective.
> You have completely uninitialized r on stack, then you assign only one value
> for immediate use here and...
> 
> >  	while (buf) {
> >  		buf = bitmap_find_region(buf);
> > @@ -655,7 +656,7 @@ int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> >  		if (ret)
> >  			return ret;
> >  
> > -		ret = bitmap_set_region(&r, maskp, nmaskbits);
> > +		ret = bitmap_set_region(&r, maskp);
> 
> ...hiding this fact here. Which I would expect that &r may be rewritten here.
> 
> I would leave these unchanged and simple assign the value in
> bitmap_set_region().

-- 
With Best Regards,
Andy Shevchenko