Re: [PATCH v13 08/15] cxl: Refresh cached PCI HDM decoder settings

[Srirangan Madhavan]

On 9/23/26 2:40 PM, Cheatham, Benjamin wrote:
> External email: Use caution opening links or attachments
>
>
> On 9/22/2026 3:39 AM, Srirangan Madhavan wrote:
> > Early PCI discovery creates the HDM cache, while later CXL enumeration and
> > decoder operations provide updated programming state.
> >
> > Refresh the PCI snapshot when decoders are enumerated, committed, or reset
> > so reset recovery need not walk the CXL topology. Ignore updates when no
> > cache exists and reject decoder-count mismatches.
> >
> > Signed-off-by: Srirangan Madhavan <smadhavan@xxxxxxxxxx>
> > ---
> >   drivers/cxl/core/hdm.c | 66 ++++++++++++++++++++++++++++++++++++++++++
> >   include/cxl/cxl.h      | 22 ++++++++++++++
> >   include/linux/pci.h    |  6 ++++
> >   3 files changed, 94 insertions(+)
> >
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 98268e8e15b2..0ae250ea4ce3 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -753,6 +753,51 @@ static void cxl_decoder_snapshot(struct cxl_decoder *cxld,
> >        }
> >   }
> >
> > +#ifdef CONFIG_CXL_RESET
>
> The CXL maintainers are normally pretty against #ifdefs in c code. I was going to suggest
> moving it to resource.c, but it looks like cxl_decoder_snapshot() needs to also be in this
> file. So, I think the fix is to put:
>
> if (!IS_ENABLED(CONFIG_CXL_RESET))
>          return;
>
> immediately after the variable declarations.

Fixed in v14 patch 9. Now cxl_hdm_refresh_decoder() is compiled without 
it. CXL_BUS selects CXL_RESET in patch 6, so whenever hdm.c is built, 
reset support is enabled; the IS_ENABLED() check is unnecessary.

> > +static void cxl_hdm_refresh_decoder(struct cxl_hdm *cxlhdm,
> > +                                 struct cxl_decoder *cxld)
> > +{
> > +     struct cxl_port *port = cxlhdm->port;
> > +     void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> > +     struct pci_dev *pdev __free(pci_dev_put) =
> > +             cxl_port_get_uport_pci_dev(port);
>
> This line needs to be at the first usage, i.e. right before "if (!pdev || !hdm)" below, to prevent
> lifetime bugs.

In v14, the pdev declaration is immediately before if (!pdev || !hdm), 
so its cleanup lifetime begins at first use.

> > @@ -1089,9 +1136,27 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> >   {
> >        void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> >        struct cxl_port *port = cxlhdm->port;
> > +#ifdef CONFIG_CXL_RESET
> > +     struct pci_dev *pdev __free(pci_dev_put) =
> > +             cxl_port_get_uport_pci_dev(port);
> > +#endif
>
> This shouldn't be here, and if you move it to be inside the below if statement
> then you won't need the #ifdef as well.
<snip>
> > +#endif
> > +
>
> Add IS_ENABLED(CONFIG_CXL_RESET) to the above if condition, like above.

pdev declaration is now inside the endpoint/HDM check, immediately 
before it is used. Removed the #ifdef. Since CXL_BUS selects CXL_RESET, 
an IS_ENABLED(CONFIG_CXL_RESET) condition is unnecessary whenever this 
function is built.

--
Regards,
Srirangan