On Mon, Aug 03, 2026 at 04:55:01PM -0700, Alison Schofield wrote:
> On Fri, Jul 31, 2026 at 01:48:10AM -0700, Anisa Su wrote:
> > From: Ira Weiny <[email protected]>
> >
> > Dynamic Capacity Devices (DCD) require event interrupts to process
> > memory addition or removal. BIOS may have control over non-DCD event
> > processing. DCD interrupt configuration needs to be separate from
> > memory event interrupt configuration.
> >
> > Factor out event interrupt setting validation.
>
> Hi Anisa,
>
> "Validation" seems like the wrong term here. The helper checks for
> firmware ownership of the memory event logs. It is not validating
> the event interrupt settings.
>
Fixed. Commit message now reads
Factor out the check for firmware ownership of the memory event logs.
> Which leads to my next comment -
>
> snip
> >
> >
> > +static bool cxl_event_validate_mem_policy(struct cxl_memdev_state *mds,
> > + struct cxl_event_interrupt_policy
> > *policy)
> > +{
> > + if (cxl_event_int_is_fw(policy->info_settings) ||
> > + cxl_event_int_is_fw(policy->warn_settings) ||
> > + cxl_event_int_is_fw(policy->failure_settings) ||
> > + cxl_event_int_is_fw(policy->fatal_settings)) {
> > + dev_err(mds->cxlds.dev,
> > + "FW still in control of Event Logs despite _OSC
> > settings\n");
> > + return false;
> > + }
> > +
> > + return true;
> > +}
>
> Why such a general name here? This helper seems to have one very specific
> purpose.
>
> cxl_event_validate_mem_policy() doesn't validate the policy. It checks whether
> firmware still owns the standard memory event logs despite _OSC granting OS
> control. I'd prefer naming it after that condition rather than "validate".
> Something like cxl_event_fw_owns_mem_logs() reads much closer to what the
> code actually does.
>
Renamed -- Naming it after the condition means the return value
has to invert, since true now means firmware does own them:
static bool cxl_event_fw_owns_mem_logs(struct cxl_event_interrupt_policy
*policy)
{
return cxl_event_int_is_fw(policy->info_settings) ||
cxl_event_int_is_fw(policy->warn_settings) ||
cxl_event_int_is_fw(policy->failure_settings) ||
cxl_event_int_is_fw(policy->fatal_settings);
}
so the caller loses its '!':
if (cxl_event_fw_owns_mem_logs(&policy)) {
dev_err(mds->cxlds.dev,
"FW still in control of Event Logs despite _OSC
settings\n");
return -EBUSY;
}
and I moved the dev_err out to the caller so
cxl_event_fw_owns_mem_logs() only answers the question "Does the FW own
event logs?"
Thanks,
Anisa
> --Alison
>
>
> > +
> > static int cxl_event_config(struct pci_host_bridge *host_bridge,
> > struct cxl_memdev_state *mds, bool irq_avail)
> > {
> > @@ -661,14 +676,8 @@ static int cxl_event_config(struct pci_host_bridge
> > *host_bridge,
> > if (rc)
> > return rc;
> >
> > - if (cxl_event_int_is_fw(policy.info_settings) ||
> > - cxl_event_int_is_fw(policy.warn_settings) ||
> > - cxl_event_int_is_fw(policy.failure_settings) ||
> > - cxl_event_int_is_fw(policy.fatal_settings)) {
> > - dev_err(mds->cxlds.dev,
> > - "FW still in control of Event Logs despite _OSC
> > settings\n");
> > + if (!cxl_event_validate_mem_policy(mds, &policy))
> > return -EBUSY;
> > - }
> >
> > rc = cxl_event_config_msgnums(mds, &policy);
> > if (rc)
> > --
> > 2.43.0
> >
