Re: [PATCH V3 1/8] cxl/mem: Read, trace, and clear events on driver load

[Ira Weiny <ira.weiny@intel.com>]

On Fri, Dec 09, 2022 at 02:33:20PM -0800, Dan Williams wrote:
> Ira Weiny wrote:
> [..]
> > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > > index 3a66aadb4df0..86c84611a168 100644
> > > > --- a/drivers/cxl/pci.c
> > > > +++ b/drivers/cxl/pci.c
> > > > @@ -417,8 +417,44 @@ static void disable_aer(void *pdev)
> > > >  	pci_disable_pcie_error_reporting(pdev);
> > > >  }
> > > >  
> > > > +static void cxl_mem_free_event_buffer(void *buf)
> > > > +{
> > > > +	kvfree(buf);
> > > > +}
> > > > +
> > > > +/*
> > > > + * There is a single buffer for reading event logs from the mailbox.  All logs
> > > > + * share this buffer protected by the cxlds->event_log_lock.
> > > > + */
> > > > +static void cxl_mem_alloc_event_buf(struct cxl_dev_state *cxlds)
> > > > +{
> > > > +	struct cxl_get_event_payload *buf;
> > > > +
> > > > +	dev_dbg(cxlds->dev, "Allocating event buffer size %zu\n",
> > > > +		cxlds->payload_size);
> > > > +
> > > > +	buf = kvmalloc(cxlds->payload_size, GFP_KERNEL);
> > > > +	if (WARN_ON_ONCE(!buf))
> > > 
> > > No, why is event init so special that it behaves differently than all
> > > the other init-time allocations this driver does?
> > 
> > Previous review agreed that a warn on once would be printed if this universal
> > buffer was not allocated.
> > 
> > > 
> > > > +		return;
> > > 
> > > return -ENOMEM;
> > > 
> > > > +
> > > > +	if (WARN_ON_ONCE(devm_add_action_or_reset(cxlds->dev,
> > > > +			 cxl_mem_free_event_buffer, buf)))
> > > > +		return;
> > > 
> > > ditto.
> > 
> > I'll change both of these with a dev_err() and bail during init.
> 
> No real need to dev_err() for a simple memory allocation faliure, but
> at least it is better than a WARN

Ok no error then.

> 
> > 
> > > 
> > > > +
> > > > +	cxlds->event.buf = buf;
> > > > +}
> > > > +
> > > > +static void cxl_clear_event_logs(struct cxl_dev_state *cxlds)
> > > > +{
> > > > +	/* Force read and clear of all logs */
> > > > +	cxl_mem_get_event_records(cxlds, CXLDEV_EVENT_STATUS_ALL);
> > > > +	/* Ensure prior partial reads are handled, by starting over again */
> > > 
> > > What partial reads? cxl_mem_get_event_records() reads every log until
> > > each returns an empty result. Any remaining events after this returns
> > > are events that fired during the retrieval.
> > 
> > Jonathan was concerned that something could read part of the log and because of
> > the statefullness of the log processing this reading of the log could start in
> > the beginning.  Perhaps from a previous driver unload while reading?
> 
> The driver will not unload without completing any current executions of
> the event retrieval thread otherwise that's an irq shutdown bug.
> 
> > I guess I was also thinking the BIOS could leave things this way?  But I think
> > we should not be here if the BIOS was ever involved right?
> 
> If the OS has CXL Error control and all Event irqs are steered to the OS
> then the driver must be allowed to assume that it has exclusive control
> over event retrieval and clearing.
> 
> > > So I do not think cxl_clear_event_logs() needs to exist, just call
> > > cxl_mem_get_event_records(CXLDEV_EVENT_STATUS_ALL) once and that's it.
> > 
> > That was my inclination but Jonathan's comments got me thinking I was wrong.
> 
> Perhaps that was before we realized the recent CXL _OSC entanglement.

Yea that could have been.  I'm not clear on the order of the comments.

Ok this should be good to go.  Reworking the rest of the series.

Thanks for the review!
Ira