Re: [PATCH 1/1] x86: Use resource_set_{range,size}() helpers

[Andy Shevchenko <andy@kernel.org>]

On Wed, Apr 16, 2025 at 03:18:56PM +0300, Ilpo Järvinen wrote:
> On Wed, 16 Apr 2025, Andy Shevchenko wrote:
> > On Wed, Apr 16, 2025 at 02:53:51PM +0300, Ilpo Järvinen wrote:
> > > On Wed, 16 Apr 2025, Andy Shevchenko wrote:
> > > > On Wed, Apr 16, 2025 at 01:13:18PM +0300, Ilpo Järvinen wrote:

...

> > > > > +			resource_set_range(res, 0xC0000, SZ_128K);
> > > > >  			res->flags = IORESOURCE_MEM | IORESOURCE_ROM_SHADOW |
> > > > >  				     IORESOURCE_PCI_FIXED;
> > > > 
> > > > I'm wondering why not DEFINE_RES_MEM() in such cases?
> > > 
> > > I guess you meant DEFINE_RES() as that seems to allow giving custom flags.
> > > However, DEFINE_RES*() will overwrite ->name which seems something that 
> > > ought to not be done here.
> > 
> > Okay, I haven't checked the initial state of name field here, so then
> > DEFINE_RES_MEM_NAMED()?  Or don't we have one?
> 
> There's pre-existing res->name on it and your suggestion would have 
> resulted in overwriting it with NULL. res->name seems to be filled earlier 
> by PCI probe code.

Okay, then the resource_*() may make more sense, indeed.

> > In any case I would rather see a one assignment for these cases than something
> > hidden behind proposed conversions.
> 
> TBH, these DEFINE_RES*() helpers are doing hidden things such as 
> blantantly overwriting ->name which I only realized after I had already 
> converted to it as per your suggestion.

They are specifically named in  capital letters, so main use is in static
assignments, but they are made compound literals, so it's possible to
initialise the on-stack or on-heap at run-time with this be kept in mind.
That's why there is nothing hidden, it implies that the struct is fully
assigned (with the provided fields and some defaults).

> And yes, it is possible to pass the pre-existing res->name to 
> DEFINE_RES_NAMED() if that what you insist, though it seems doing it for 
> the sake of DEFINE_RES*() interface rather than this code wanting to 
> really define the resource from scratch.
> 
> Given the hidden overwriting, please be careful on suggesting 
> DEFINE_RES*() conversions as it's not as trivial as it seems.

Yeah, seems not everybody aware of the difference with
foo_init_something() vs. FOO_INIT_SOMETHING() :-)

> > > I found one other case from the same file though which is truly defines
> > > a resource from scratch.

-- 
With Best Regards,
Andy Shevchenko