Re: [PATCH v17 02/22] sfc: add cxl support

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

On Mon, 30 Jun 2025 15:55:39 +0100
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> On 6/30/25 15:52, Alejandro Lucero Palau wrote:
> >
> > On 6/25/25 17:37, Jonathan Cameron wrote:  
> >> On Tue, 24 Jun 2025 15:13:35 +0100
> >> <alejandro.lucero-palau@amd.com> wrote:
> >>  
> >>> From: Alejandro Lucero <alucerop@amd.com>
> >>>
> >>> Add CXL initialization based on new CXL API for accel drivers and make
> >>> it dependent on kernel CXL configuration.
> >>>
> >>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> >>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> >>> Reviewed-by: Alison Schofield <alison.schofield@intel.com>  
> >> Hi Alejandro,
> >>
> >> I think I'm missing something with respect to the relative life times.
> >> Throwing one devm_ call into the middle of a probe is normally a recipe
> >> for at least hard to read code, if not actual bugs.  It should be done
> >> with care and accompanied by at least a comment.  
> >
> >
> > Hi Jonathan,
> >
> >
> > I agree devm_* being harder in general and prone to some subtle 
> > problems, but I can not see an issue here apart from the objects kept 
> > until device unbinding. But I think adding some comment can help.
> >
> >
> > <snip>
> >  
> >> +
> >> +    dvsec = pci_find_dvsec_capability(pci_dev, PCI_VENDOR_ID_CXL,
> >> +                      CXL_DVSEC_PCIE_DEVICE);
> >> +    if (!dvsec)
> >> +        return 0;
> >> +
> >> +    pci_dbg(pci_dev, "CXL_DVSEC_PCIE_DEVICE capability found\n");
> >> +
> >> +    /* Create a cxl_dev_state embedded in the cxl struct using cxl 
> >> core api
> >> +     * specifying no mbox available.
> >> +     */
> >> +    cxl = devm_cxl_dev_state_create(&pci_dev->dev, CXL_DEVTYPE_DEVMEM,
> >> +                    pci_dev->dev.id, dvsec, struct efx_cxl,
> >> +                    cxlds, false);
> >> The life time of this will outlast everything else in the efx driver.
> >> Is that definitely safe to do?  Mostly from a reviewability and 
> >> difficulty
> >> of reasoning we avoid such late releasing of resources.
> >>
> >> Perhaps add to the comment before this call what you are doing to 
> >> ensure that
> >> it is fine to release this after everything in efx_pci_remove()
> >>
> >> Or wrap it up in a devres group and release that group in 
> >> efx_cxl_exit().
> >>
> >> See devres_open_group(), devres_release_group()
> >>
> >>  
> >
> > As I said above, I can not see a problem here, but maybe to explicitly 
> > managed those resources with a devres group makes it simpler, so I 
> > think it is a good advice to follow.
> >
> >
> > Thanks!
> >
> >  
> 
> FWIW, I just want to add that although I agree with this, it is somehow 
> counterintuitive to me as the goal of devm is to avoid to care about 
> when to release those allocations.

In my view not quite.  It's to enforce that those allocations are released
in the reverse order of the devm setup calls - which is almost always
the right thing to do as long as whole driver is using devm.

There are uses like you describe though so it's not a universal case
of one or the other.  One advantage of the devres group thing is that
folk who are not keen on devm can effectively have normal manual release
flows.

Jonathan