Re: [PATCH v16 04/22] cxl: Move register/capability check to driver

[Dan Williams <dan.j.williams@intel.com>]

Alejandro Lucero Palau wrote:
> 
> On 5/22/25 20:51, Dan Williams wrote:
> > Alejandro Lucero Palau wrote:
> > [..]
> >> You did not like to add a new capability field to current structs
> >> because the information needed was already there in the map. I think it
> >> was a fair comment, so the caps, a variable the caller gives, is set
> >> during the discovery without any internal struct added.
> > The objection was not limited to data structure changes, it also
> > includes sprinkling an @caps argument throughout the stack for an
> > as yet to be determined benefit.
> >
> >> Regarding what you suggest below, I have to disagree. This change was
> >> introduced for dealing with a driver using CXL, that is a Type2 or
> >> future Type1 driver. IMO, most of the innerworkings should be hidden to
> >> those clients and therefore working with the map struct is far from
> >> ideal, and it is not currently accessible from those drivers.
> > Checking a couple validity flags in a now public (in include/cxl/pci.h)
> > data-structure is far from ideal?
> >
> >> With these new drivers the core does not know what should be there, so
> >> the check is delayed and left to the driver.
> > Correct, left to the driver to read from an existing mechanism.
> >
> >> IMO, from a Type2/Type1 driver perspective, it is better to deal with
> >> caps expected/found than being aware of those internal CXL register
> >> discovery and maps.
> > Not if a maintainer of the CXL register discovery and maps remains
> > unconvinced to merge a parallel redundant mechanism to achieve the exact
> > same goal.
> 
> 
> OK. You are the maintainer and you'll get what you want. I'm not going 
> to fight this if none else back me up.
> 
> 
> Because you refer to your maintainer position, let me just say, in a 
> critical but constructive way, I'm a bit pissed off with this.
> 
> 
> It is not because we disagree nor because you as the maintainer have a 
> weight on this I don't. I accept that and I am also happy to discuss all 
> this with you even if I end up doing the things your way. I'm pissed off 
> because you have been silent during months, with other people in the CXL 
> kernel community reviewing the patches, commenting and raising concerns. 
> I think it is discouraging that you, as the maintainer, allow me and 
> these people involved in those reviews, going through a path you 
> disagree with and say nothing. Again, it is not because you have another 
> view, surely more relevant than those less used to the code involved, 
> but because you disappear for so long.

That is fair, and you are justified in being upset.

I cringed when realizing that here we are yet again at a late hour and I
have fundamental comments that postpone the merge.

It is also the case that my time is contended and I need to make
priority calls. My criteria for keeping this at the bottom of the queue,
wrongly or rightly, was that I had the sense that this is still a
performance optimization not a fundamental blocker for end users. Where
making progress on other priorities unblocked a larger number of
stakeholders or had a larger impact on end users.

There is also something missing in the CXL patch review process. It
should not be the case that we have this many review tags and versions
yet still have a memory leak in patch1, and mismatched object validity
expectations throughout. For my part I am going to take ownership of the
fact that the lifetime and locking rules of the CXL object model are not
well understood and will offer a presentation of that at the next CXL
collab meeting. If the review load for lifetime and locking can scale to
more people that hopefully helps me be less of a bottleneck ("pain in
the neck?") going forward.

> I need some days off now, what is well-aligned with a four-day long 
> weekend for me, but I'll be back with new energy next week for 
> addressing all those concerns.

Your time and effort here are appreciated. Our discussion on the
register enumeration did make me realize the shaky foundations you were
looking to enhance. That back and forth revealed a path to get to what
we both want which is less complexity exported to leaf drivers *and*
minimal incremental burden on top of the core. I.e. a minimal
devm_cxl_add_memdev() is likely all SFC needs to do for basic CXL init.
Lets work towards that.

Again, apologies for the late rug pull.