Re: [PATCH v12 5/8] cxl/pci: Factor out interrupt policy check

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
> >