Re: [PATCH v17 10/22] cx/memdev: Indicate probe deferral

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 1 Jul 2025 09:25:44 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 7/1/25 9:07 AM, Alejandro Lucero Palau wrote:
> > 
> > On 6/30/25 17:20, Jonathan Cameron wrote:  
> >> Hi Dave,
> >>  
> >>>> +/*
> >>>> + * Try to get a locked reference on a memdev's CXL port topology
> >>>> + * connection. Be careful to observe when cxl_mem_probe() has deposited
> >>>> + * a probe deferral awaiting the arrival of the CXL root driver.
> >>>> + */
> >>>> +struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd)  
> >> Just focusing on this part.
> >>  
> >>> Annotation of __acquires() is needed here to annotate that this function is taking multiple locks and keeping the locks.  
> >> Messy because it's a conditional case and on error we never have
> >> a call marked __releases() so sparse may moan.
> >>
> >> In theory we have __cond_acquires() but I think the sparse tooling
> >> is still missing for that.
> >>
> >> One option is to hike the thing into a header as inline and use __acquire()
> >> in the appropriate places.  Then sparse can see the markings
> >> without problems.
> >>
> >> https://lore.kernel.org/all/20250305161652.GA18280@noisy.programming.kicks-ass.net/
> >>
> >> has some discussion on fixing the annotation issues around conditional locks
> >> for LLVM but for now I think we are still stuck.
> >>
> >> For the original __cond_acquires()
> >> https://lore.kernel.org/all/CAHk-=wjZfO9hGqJ2_hGQG3U_XzSh9_XaXze=HgPdvJbgrvASfA@mail.gmail.com/
> >>
> >> Linus posted sparse and kernel support but I think only the kernel bit merged
> >> as sparse is currently (I think) unmaintained.
> >>  
> > 
> > Not sure what is the conclusion to this: should I do it or not?  
> 
> Sounds like we can't with the way it's conditionally done.

All you can do today is hike the function implementation that conditionally
takes locks to the header as static inline and use explicit __acquire() markings
in paths where you exit with locks held.

Here's one I did earlier:

https://elixir.bootlin.com/linux/v6.16-rc4/source/include/linux/iio/iio.h#L674

static inline bool iio_device_claim_direct(struct iio_dev *indio_dev)
{
	if (!__iio_device_claim_direct(indio_dev))
		return false;

	__acquire(iio_dev);

	return true;
}


That exposes the marking so sparse can see it and correctly track the locking.

Or you could step up an maintain sparse (I've been trying to talk someone
into doing that but no luck yet ;)


> > 
> > 
> > I can not see the __acquires being used yet by cxl core so I wonder if this needs to be introduced only when new code is added or it should require a core revision for adding all required. I mean, those locks being used in other code parts but not "advertised" by __acquires, is not that a problem?  
> 
> It's only needed if you acquire a lock and leaving it held and then releases it in a different function. That allows sparse(?) to track if you are locking correctly. You don't need it if it's being done in the same function.

Exactly right.  There is work on going I believe to make this work
with LLVMs tracking, but right now that is too simplistic to generate
reliable results.  Note that sparse also sometimes gives false positives
if the code flow gets a bit complex but mostly that only happens when
the code probably needs a rethink anyway.

For now I'd go with do nothing here.

Jonathan

> 
> DJ
> 
> 
> > 
> >   
> >>>> +{
> >>>> +    struct cxl_port *endpoint;
> >>>> +    int rc = -ENXIO;
> >>>> +
> >>>> +    device_lock(&cxlmd->dev);  
> >>>> +> +    endpoint = cxlmd->endpoint;  
> >>>> +    if (!endpoint)
> >>>> +        goto err;
> >>>> +
> >>>> +    if (IS_ERR(endpoint)) {
> >>>> +        rc = PTR_ERR(endpoint);
> >>>> +        goto err;
> >>>> +    }
> >>>> +
> >>>> +    device_lock(&endpoint->dev);
> >>>> +    if (!endpoint->dev.driver)> +        goto err_endpoint;
> >>>> +
> >>>> +    return endpoint;
> >>>> +
> >>>> +err_endpoint:
> >>>> +    device_unlock(&endpoint->dev);
> >>>> +err:
> >>>> +    device_unlock(&cxlmd->dev);
> >>>> +    return ERR_PTR(rc);
> >>>> +}
> >>>> +EXPORT_SYMBOL_NS_GPL(cxl_acquire_endpoint, "CXL");
> >>>> +
> >>>> +void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)  
> >>> And __releases() here to release the lock annotations  
> >>>> +{
> >>>> +    device_unlock(&endpoint->dev);
> >>>> +    device_unlock(&cxlmd->dev);
> >>>> +}
> >>>> +EXPORT_SYMBOL_NS_GPL(cxl_release_endpoint, "CXL");  
> >>  
> 
>