From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5B27E329C4D for ; Tue, 9 Dec 2025 16:45:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.15 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765298712; cv=none; b=IyrYpDdogaNKv58cLEdCnrAtkcasSwXKl73b2mgZ4/ffu2JBi43B+zI7cl6uPanSaNiAhC8FpVcPc7SDVn2abVg3bIBudIP+EqfqNzRIpI7Zlg/BAQ3kU9/7d0lMCE1WkKnH8iE+4hBXPVeMHpjTMUMXRcCaOxluHItWJ/JrJ8s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765298712; c=relaxed/simple; bh=wvzQu81QIUl7jTquszCwBRAicK7ShYv2/fN18YIQssE=; h=From:Date:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=mDeXb6Q959GXw5UjK01FU54cBMUC/U6ADQzkhKWvA2VQg9WpjeWRs/LqJjipNKYo4FjC9YZMm4SRDXTBMbFfDPw1sXlwzi/DNd0PJmd7c51Sg8c+6msJx9ECeDwfJmJJuKBxoCrW3EyrZIjnTCIM1QVdrTgaQRUgEp+1lKclfwc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=EYcan8/U; arc=none smtp.client-ip=198.175.65.15 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="EYcan8/U" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1765298711; x=1796834711; h=from:date:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=wvzQu81QIUl7jTquszCwBRAicK7ShYv2/fN18YIQssE=; b=EYcan8/ULORpLhpykzQswsr4+7sIpygeeV2EIWMeIJYa81foMZAm7CKn w12x+JeZE8ayefg+9piFcuBMvx+Ws21NN2tLjdurW0RHC5Zz9dcFyCe+N hFyb7B0EW1plXWKeoLP0zvwB8F/b6SmPvLdeV5TpBcGyPp17ioLqSrg9X kDx224MPxG4pCb3leCMcl5RMa6gB3iVHX5VQ+1ANGTb3BAt7kUDRk3cEw EzbGK98tjKUx9FotiSPekbzoEg4yXtMH8brNY3znGvHPdUqpqrclyYfQu X7FcIA/aGPY/rp1bjDRNq1OIJBvFXVf4eBffQ803bzLigt45zN2zHYh2t g==; X-CSE-ConnectionGUID: en6wEAPdQo2RTLQPNoJ39Q== X-CSE-MsgGUID: +VmfHWxlRa+hLeDPUBb7tw== X-IronPort-AV: E=McAfee;i="6800,10657,11637"; a="70878773" X-IronPort-AV: E=Sophos;i="6.20,261,1758610800"; d="scan'208";a="70878773" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by orvoesa107.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Dec 2025 08:45:10 -0800 X-CSE-ConnectionGUID: tDP+XTT+RzWWqMk9EJUFbw== X-CSE-MsgGUID: WSKeoqxBRuGBa/t7CQn6qA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.20,261,1758610800"; d="scan'208";a="219607652" Received: from ijarvine-mobl1.ger.corp.intel.com (HELO localhost) ([10.245.245.139]) by fmviesa002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Dec 2025 08:45:07 -0800 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Tue, 9 Dec 2025 18:45:04 +0200 (EET) To: Christian Marangi cc: Andrew Morton , Andy Shevchenko , LKML Subject: Re: [PATCH v2] resource: add WARN_ON_ONCE for resource_size() and document misusage In-Reply-To: <693848ea.050a0220.1466f.6473@mx.google.com> Message-ID: References: <20251209150150.9525-1-ansuelsmth@gmail.com> <678c3644-47da-f72d-a72d-5cc9b4230a99@linux.intel.com> <693848ea.050a0220.1466f.6473@mx.google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323328-1875142702-1765298406=:1137" Content-ID: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-1875142702-1765298406=:1137 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: QUOTED-PRINTABLE Content-ID: On Tue, 9 Dec 2025, Christian Marangi wrote: > On Tue, Dec 09, 2025 at 05:48:53PM +0200, Ilpo J=E4rvinen wrote: > > On Tue, 9 Dec 2025, Christian Marangi wrote: > >=20 > > > 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 resul= t > > > in its misusage as a way to check for initialization of a passed reso= urce > > > descriptor. > > >=20 > > > In the referenced commit, resource_size() is wrongly assumed to retur= n > > > 0 when a resource descriptor is init to all zero while in reality it > > > would return 1. > > >=20 > > > This is caused by the fact that resource_size() calculates the size > > > with the following logic: > > >=20 > > > =09end - start + 1 > > >=20 > > > that with an all zero resource descriptor: > > >=20 > > > =090 - 0 + 1 > > >=20 > > > returns 1. > > >=20 > > > One reason the BUG in the reference commit might have been introduced > > > is a logic error in the actual usage of resource_size(). > > >=20 > > > Historically, it was assumed that resource_size() was ALWAYS > > > used AFTER APIs filled the data of the resource descriptor (or in cas= e of > > > any error from such APIs, resource descriptor set to an invalid state= ) > >=20 > > Missing final . > >=20 > > > 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. > > >=20 > > > 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 > >=20 > > be confusing > >=20 > > > some callers. > > >=20 > > > Hence it's ALWAYS wrong to initialize (and use) a resource descriptor > > > to all zero following the usual pattern: > > >=20 > > > =09struct resource res =3D {}; > > >=20 > > > 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_UNS= ET). > >=20 > > I don't exactly like the wording here as technically IORESOURCE_UNSET i= s=20 > > not a resource type (IMO, it would be better to leave flags to zero=20 > > when type is not valid, and test for that and not IORESOURCE_UNSET). > > >=20 > Yes I guess IORESOURCE_UNSET is strictly a flag than a type. >=20 > Maybe a bit OT but I think it's sensible to define for any future fix > related to this. >=20 > My idea here is to give good practice for the case of defining a zero > resource descriptor. >=20 > I feel leaving the flags as zero might pose the same current problem > with user still declaring resource descriptor with=20 >=20 > =09struct resource res =3D {}; >=20 > and then checking with=20 >=20 > =09if (!res.flags) { ... >=20 > Setting the flags with IORESOURCE_UNSET seems more robust to me. >=20 > And with this pattern we can also introduce 2 helper. >=20 > (example DEFINE_RES_UNSET and resource_is_unsed()) Requiring to use DEFINE_RES_UNSET() (or like) risks many devs not knowing= =20 about it and using DEFINE_RES() instead. One could consider making a plain DEFINE_RES() without any arguments=20 default to this behavior using preprocessor argument=20 COUNT_ARGS(__VA_ARGS__) trickery (used e.g. in=20 pci_dev_for_each_resource() if you want an example how it invokes other=20 macros). So this code would result in start =3D 0, end =3D -1, flags =3D=20 IORESOURCE_UNSET: =09struct resource res =3D DEFINE_RES(); --=20 i. > > In any case, preferrably resource would be directly initialized with a= =20 > > valid type, but that is not possible in the case of ath11k because the= =20 > > called function is filling res. > >=20 > > From the point of view of resource_size(), the more important aspect,= =20 > > however, is that DEFINE_RES() handles the start and end address setup= =20 > > correctly. > >=20 > > > To catch any possible misusage of resource_size() helper, emit a WARN= if > > > we detect the passed resource descriptor have zeroed flags. This woul= d > > > signal the resource descriptor is not correctly inizialized and will > >=20 > > initialized > >=20 > > > probably result in resource_size() returning unexpected sizes (for > > > example returning 1 if the resource descriptor is all set to zero). > >=20 > > I'd remove the parenthesis part as it is already covered by what was=20 > > said above. > >=20 > > > 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 confusi= on. > > >=20 > > > Link: https://lore.kernel.org/all/20251207215359.28895-1-ansuelsmth@g= mail.com/T/#m990492684913c5a158ff0e5fc90697d8ad95351b > > > Suggested-by: Ilpo J=E4rvinen > > > Signed-off-by: Christian Marangi > > > --- > > > Changes v2: > > > - Improve commit description > > > - Improve kdoc > > > - Add bug.h include > > >=20 > > > include/linux/ioport.h | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > >=20 > > > 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 @@ > > > =20 > > > #ifndef __ASSEMBLY__ > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -286,8 +287,30 @@ static inline void resource_set_range(struct res= ource *res, > > > =09resource_set_size(res, size); > > > } > > > =20 > > > +/** > > > + * resource_size - Get the size of the resource > > > + * @res: Resource descriptor > > > + * > > > + * Calculated size is derived from @res end and start values followi= ng > > > + * the logic: > > > + * > > > + *=09end - start + 1 > > > + * > > > + * This MUST be used ONLY with correctly initialized @res descriptor= =2E > > > + * > > > + * Do NOT use resource_size() as a proxy for checking validity of @r= es or > > > + * for checking if @res is in a resource tree (use flags checks or c= all > > > + * resource_assigned() instead). > > > + * > > > + * The caller MUST ensure @res is properly initialized, passing a @r= es > >=20 > > This is repeating what is above but I'd not remove this but use this=20 > > wording above as it clearly states caller is responsible (instead of=20 > > a passive voice). > >=20 > > > + * descriptor with zeroed flags will produce a WARN signaling a misu= sage > > > + * 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 *r= es) > > > { > > > +=09WARN_ON_ONCE(!res->flags); > > > =09return res->end - res->start + 1; > > > } > > > static inline unsigned long resource_type(const struct resource *res= ) > > >=20 > >=20 > > --=20 > > i. >=20 >=20 >=20 --8323328-1875142702-1765298406=:1137--