From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.17]) (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 865C91FF5E3 for ; Tue, 9 Dec 2025 10:25:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.17 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765275946; cv=none; b=sBBqtqlOxDYVZmpgPeQSaSU4D2tJonI3YVSXwCyxiSWKeJWLprOcDBtsRTpD4LubgKwf2CkHygSy3jT1Uyw2bTPMdoFjPmW/Lk3ovKiDb9jEOamN4GXxzHDS9+wLGhEXIINbbgVFR03wikl+D+uAcxFz/lf/UNSvnv2YNKhesSU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765275946; c=relaxed/simple; bh=6fSg+Ugu7qsha6WMuwNbzx4myfN1ferTVhz84bJyzvs=; h=From:Date:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=mSsCDDDOycOU8MYuHdApgIIaSA1JDLwMcTQubeYi177e9HR8feAeXHJ3KdiKYIJinVs8tfTBKfYrtBk3iVwY21IQWbbLmMF3ID9bEgFPOVrpIz35B354/4bKg3ZZDX7P1Uw4KeGjY1JbtnJMUOXvsR9JP44QK/G2BGhafkOSNs0= 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=RKheD0JB; arc=none smtp.client-ip=198.175.65.17 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="RKheD0JB" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1765275945; x=1796811945; h=from:date:to:cc:subject:in-reply-to:message-id: references:mime-version:content-id; bh=6fSg+Ugu7qsha6WMuwNbzx4myfN1ferTVhz84bJyzvs=; b=RKheD0JBWa7jAgI+Qw5qd/HQpelZ73cIHlvYZHS2XTB5iHHvFJwK4TTZ WU2kQaZ8Ba6TY14Yrfx2K1N8tVLzSGn7r1e1NxSrBZzhFQ/Pv7SQ1db/f QTvG+7pnd2zUyoHZeZO3blx/PPRxwHXksb0v8+6notdkxJuG5qyKqrN2s RDM2L6egOTkowtVp4+p5+N8SojPw1hBcromYnOaw45bkLaUoOwaVfXZsa cQkhVraOgEZmW0A/Vhcfg6EpCJ+CznZgbItpIAeNOLPVZ5UWhjGbnN2vy SOdR837oL9lEv/FyHyFBAp00d0LMdyUmr3l5FQwwAV+Ph388TdEz0RoHQ g==; X-CSE-ConnectionGUID: R2m3qXiSTIO8uzp77TUz4w== X-CSE-MsgGUID: cf+PeufxTKitMjs8csYDRA== X-IronPort-AV: E=McAfee;i="6800,10657,11636"; a="67190629" X-IronPort-AV: E=Sophos;i="6.20,261,1758610800"; d="scan'208";a="67190629" Received: from orviesa001.jf.intel.com ([10.64.159.141]) by orvoesa109.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Dec 2025 02:25:45 -0800 X-CSE-ConnectionGUID: /hq7gyYrRqSuJgDqpZV7qA== X-CSE-MsgGUID: w0kltI9XROyTZbeXVURUqQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.20,261,1758610800"; d="scan'208";a="233571863" Received: from ijarvine-mobl1.ger.corp.intel.com (HELO localhost) ([10.245.245.139]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Dec 2025 02:25:42 -0800 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Tue, 9 Dec 2025 12:25:38 +0200 (EET) To: Christian Marangi cc: Andy Shevchenko , Andrew Morton , LKML Subject: Re: [PATCH] resource: add WARN_ON_ONCE for resource_size() and document misusage In-Reply-To: <20251208202921.16819-1-ansuelsmth@gmail.com> Message-ID: <2c80f567-d632-b37e-c419-db059b1f2771@linux.intel.com> References: <20251208202921.16819-1-ansuelsmth@gmail.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-1498444205-1765275679=:1137" Content-ID: <6e9c9369-a31b-9523-10fc-fa42b07806c9@linux.intel.com> 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-1498444205-1765275679=:1137 Content-Type: text/plain; CHARSET=ISO-8859-15 Content-Transfer-Encoding: QUOTED-PRINTABLE Content-ID: <540b6473-32d5-2c28-8248-5652971e5d05@linux.intel.com> On Mon, 8 Dec 2025, Christian Marangi wrote: > 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 > probably result in resource_size() returning unexpected values. (for > example returning 1 if the resource descriptor is all set to zero) Move . after the () part. Or make the parenthesis part a real sentence. In general, you should first explain the problem in the changelog and then= =20 the solution, not to go directly to the solution. Especially in the=20 changes such as this that are non-trivial to understand and are meant to=20 enforce good code hygiene/standard. So I think you should reorder some of= =20 the sentences to fix the ordering. > Historically, it was assumed that resource_size was ALWAYS used AFTER For function names, always use () in the changelog. > correct API fill the data of the resource descriptor (or errors out) Add . Somehow I fail to connect that "(or errors out)" to rest of what is being= =20 said in the sentence, maybe rephrase it? >=20 > But lack of comments might have introduced some logic error and some logic error -> logic errors > confusion in any user of resource_size() with it used on resource > description initialized to all zero. description -> descriptor It's hard to understand what this tries to say, I suggest you try to=20 rephrase it. Maybe something along these lines: =2E.. and using resource_size() for a resource descriptor that is initialized to all zeros. Using resource_size() for a resource descriptor that has zero start and end yields to resource size of 1 (this is correct= =20 and necessary behavior!) which may beconfusing to some callers. > The normal way to inizialize an "uninizialized" resource descriptor 2x iniz typos > would be to use DEFINE_RES macro or resource_set_range() ideally with a > proper flag set to it. proper flag -> proper type ? I'd remove the resource_set_range() part actually since it's meant to be=20 used for a resource that is already initialized and the code only wants to= =20 change the address range. > Hence initializing a resource descriptor to all zero and passing it to > resource_size() would actually produce a size of 1. This would naturally belong earlier when you talk about using=20 resource_size() for a descriptor that is initialized to all zero. (I ended up writing this into the earlier suggestion how to rephrase=20 things so if you use it, you can drop this as it then becomes duplicate). > Correct comments on the usage of this helper in conjunction of WARN Add kernel doc to resource_size() ... > should prevent from now on any possible misusage of this and permit > to catch and fix any possible BUG caused by this logic confusion. >=20 > Link: https://lore.kernel.org/all/20251207215359.28895-1-ansuelsmth@gmail= =2Ecom/T/#m990492684913c5a158ff0e5fc90697d8ad95351b > Suggested-by: Ilpo J=E4rvinen > Signed-off-by: Christian Marangi > --- > include/linux/ioport.h | 12 ++++++++++++ > 1 file changed, 12 insertions(+) >=20 > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index e8b2d6aa4013..8b60f820993c 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -286,8 +286,20 @@ static inline void resource_set_range(struct resourc= e *res, > =09resource_set_size(res, size); > } > =20 > +/** > + * resource_size - Get the size of the resource > + * @res: Resource descriptor > + * > + * This MUST be used ONLY with correctly inizialized resource descriptor= =2E initialized I'd add: Do not use resource_size() as a proxy for checking validity of @res or for= =20 checking if @res is in a resource tree (use flags checks or call=20 resource_assigned() instead). > + * Passing a resource descriptor with zeroed flags will produce a WARN The caller must ensure @res is properly initialized. > + * signaling a misusage of this helper and probably a BUG in the user > + * of this helper. > + * > + * Return: Size of the resource calculated from resource end - start + 1= =2E I'd put this into the main description before the notes/warning on what to= =20 not do. And only say here: * Return: size of the resource. > + */ > static inline resource_size_t resource_size(const struct resource *res) > { > +=09WARN_ON_ONCE(!res->flags); As lkp also found out, you need to add #include > =09return res->end - res->start + 1; > } > static inline unsigned long resource_type(const struct resource *res) >=20 --=20 i. --8323328-1498444205-1765275679=:1137--