Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains_addr()

[Andy Shevchenko]

Wed, Jul 02, 2025 at 12:16:07PM -0700, Alison Schofield kirjoitti:
> On Wed, Jul 02, 2025 at 11:16:36AM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
> > > In CXL subsystem, many functions need to check an address availability
> > > by checking if the resource range contains the address. Providing a new
> > > helper function resource_contains_addr() to check if the resource range
> > > contains the input address.
> > 
> > resources are about ranges and not addresses. At bare minimum naming #####
> > here. Also there is no symmetry with the intersection API. But I would argue
> > to use resource_contains() and just provide necessary parameter with both
> > start and end to be set at the same value.
> > 
> > 	struct resource r = DEFINE_RES...(addr);
> > 
> > 	if (resource_contains, res, &r)
> > 		...do stuff...

(1) ^^^

> Thanks for the review. Two alternative approaches were considered:
> 
> 1) A CXL-only helper with direct comparison:
> This duplicates range checking logic that's already common in the
> resource API. We'd end up with CXL specific when the resource API
> already provides the semantic framework for containment checks.

> 2) A CXL helper using temporary resource + resource_contains():
> While this maintains API consistency, it's unnecessarily heavyweight.

How is it? It's mostly matter of stack usage. Do I miss anything?

> Creating a temporary resource struct just to check if a single address
> falls within a range feels like overengineering.
> (This is your suggestion above.)

I don' think so. This won't create anything, it will declare the resource
data on a stack.

...

> This patch proposes, a new helper in ioport.h, provides a clean, reusable
> API that follows the existing resource_contains() pattern while being
> optimized for the single address case. I guess I can agree with the lack
> of symmetry with the existing API, but that's only if one thinks an API
> cannot be extended. The new helper actually complements resource_contains()
> nicely: one checks resource vs resource containment, the other checks
> address vs resource containment.

The point is _there is no address_ in the resources! There is a _range_.

> Revisited the name, I can see how extending an existing name, like 
> resource_contains() is a poor choice because then one might expect the
> pair to be resource_contains_resource() and resource_contains_addr()
> which it is not.
> 
> Do either of these fit better, or something else?
> - resource_includes_addr() 
> - resource_has_addr()

That's why (see above) naming sucks. Doing it local to CXL makes it clearer as
CXL knows the semantic of the range, struct resource doesn't and shouldn't.
(Yeah, I know that we have something there that relies on the range being an
address range, but those APIs provide more and TBH I would rather split them
to a separate header)

> The new helper would immediately be used 7 times in the CXL subsystem.
> What are you thinking the bar is for adding to ioport.h?

It's only a single subsystem, find 3+ more outside and we can reconsider.
Also for the symmetrical API,

-- 
With Best Regards,
Andy Shevchenko