On Tue, 9 Dec 2025, Christian Marangi wrote: > On Tue, Dec 09, 2025 at 05:48:53PM +0200, Ilpo Järvinen wrote: > > On Tue, 9 Dec 2025, Christian Marangi wrote: > > > > > Commit 900730dc4705 ("wifi: ath: Use > > > of_reserved_mem_region_to_resource() for "memory-region"") uncovered a > > > fragility in the usage of the resource_size() helper that might result > > > in its misusage as a way to check for initialization of a passed resource > > > descriptor. > > > > > > In the referenced commit, resource_size() is wrongly assumed to return > > > 0 when a resource descriptor is init to all zero while in reality it > > > would return 1. > > > > > > This is caused by the fact that resource_size() calculates the size > > > with the following logic: > > > > > > end - start + 1 > > > > > > that with an all zero resource descriptor: > > > > > > 0 - 0 + 1 > > > > > > returns 1. > > > > > > One reason the BUG in the reference commit might have been introduced > > > is a logic error in the actual usage of resource_size(). > > > > > > Historically, it was assumed that resource_size() was ALWAYS > > > used AFTER APIs filled the data of the resource descriptor (or in case of > > > any error from such APIs, resource descriptor set to an invalid state) > > > > Missing final . > > > > > But lack of comments on what should be the proper usage of > > > resource_size() might have introduced some confusion in the specific > > > case of passing a resource descriptor initialized to all zeros. > > > > > > As described in the example, using resource_size() for a resource > > > descriptor that has zero start and end yields to resource size of 1 > > > (this is correct and necessary behavior!) which may beconfusing to > > > > be confusing > > > > > some callers. > > > > > > Hence it's ALWAYS wrong to initialize (and use) a resource descriptor > > > to all zero following the usual pattern: > > > > > > struct resource res = {}; > > > > > > The correct way to initialize an "uninitialized" resource descriptor would > > > be to use DEFINE_RES macro ideally with a proper type set to it > > > (for example by initializing it to zero start/size and IORESOURCE_UNSET). > > > > I don't exactly like the wording here as technically IORESOURCE_UNSET is > > not a resource type (IMO, it would be better to leave flags to zero > > when type is not valid, and test for that and not IORESOURCE_UNSET). > > > > Yes I guess IORESOURCE_UNSET is strictly a flag than a type. > > Maybe a bit OT but I think it's sensible to define for any future fix > related to this. > > My idea here is to give good practice for the case of defining a zero > resource descriptor. > > I feel leaving the flags as zero might pose the same current problem > with user still declaring resource descriptor with > > struct resource res = {}; > > and then checking with > > if (!res.flags) { ... > > Setting the flags with IORESOURCE_UNSET seems more robust to me. > > And with this pattern we can also introduce 2 helper. > > (example DEFINE_RES_UNSET and resource_is_unsed()) Requiring to use DEFINE_RES_UNSET() (or like) risks many devs not knowing about it and using DEFINE_RES() instead. One could consider making a plain DEFINE_RES() without any arguments default to this behavior using preprocessor argument COUNT_ARGS(__VA_ARGS__) trickery (used e.g. in pci_dev_for_each_resource() if you want an example how it invokes other macros). So this code would result in start = 0, end = -1, flags = IORESOURCE_UNSET: struct resource res = DEFINE_RES(); -- i. > > In any case, preferrably resource would be directly initialized with a > > valid type, but that is not possible in the case of ath11k because the > > called function is filling res. > > > > From the point of view of resource_size(), the more important aspect, > > however, is that DEFINE_RES() handles the start and end address setup > > correctly. > > > > > To catch any possible misusage of resource_size() helper, emit a WARN if > > > we detect the passed resource descriptor have zeroed flags. This would > > > signal the resource descriptor is not correctly inizialized and will > > > > initialized > > > > > probably result in resource_size() returning unexpected sizes (for > > > example returning 1 if the resource descriptor is all set to zero). > > > > I'd remove the parenthesis part as it is already covered by what was > > said above. > > > > > Also add kernel doc to resource_size() that in conjunction of WARN > > > should prevent from now on any possible misusage of this helper and > > > permit to catch and fix any possible BUG caused by this logic confusion. > > > > > > Link: https://lore.kernel.org/all/20251207215359.28895-1-ansuelsmth@gmail.com/T/#m990492684913c5a158ff0e5fc90697d8ad95351b > > > Suggested-by: Ilpo Järvinen > > > Signed-off-by: Christian Marangi > > > --- > > > Changes v2: > > > - Improve commit description > > > - Improve kdoc > > > - Add bug.h include > > > > > > include/linux/ioport.h | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > > > index e8b2d6aa4013..c087e49e1927 100644 > > > --- a/include/linux/ioport.h > > > +++ b/include/linux/ioport.h > > > @@ -11,6 +11,7 @@ > > > > > > #ifndef __ASSEMBLY__ > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -286,8 +287,30 @@ static inline void resource_set_range(struct resource *res, > > > resource_set_size(res, size); > > > } > > > > > > +/** > > > + * resource_size - Get the size of the resource > > > + * @res: Resource descriptor > > > + * > > > + * Calculated size is derived from @res end and start values following > > > + * the logic: > > > + * > > > + * end - start + 1 > > > + * > > > + * This MUST be used ONLY with correctly initialized @res descriptor. > > > + * > > > + * Do NOT use resource_size() as a proxy for checking validity of @res or > > > + * for checking if @res is in a resource tree (use flags checks or call > > > + * resource_assigned() instead). > > > + * > > > + * The caller MUST ensure @res is properly initialized, passing a @res > > > > This is repeating what is above but I'd not remove this but use this > > wording above as it clearly states caller is responsible (instead of > > a passive voice). > > > > > + * descriptor with zeroed flags will produce a WARN signaling a misusage > > > + * of this helper and probably a BUG in the user of this helper. > > > + * > > > + * Return: size of the resource. > > > + */ > > > static inline resource_size_t resource_size(const struct resource *res) > > > { > > > + WARN_ON_ONCE(!res->flags); > > > return res->end - res->start + 1; > > > } > > > static inline unsigned long resource_type(const struct resource *res) > > > > > > > -- > > i. > > >