Re: [PATCH RFC] cxl/pci: Skip irq features if irq's are not supported

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

Alison Schofield wrote:
> On Mon, Jan 08, 2024 at 11:51:13PM -0800, Ira Weiny wrote:
> > CXL 3.1 Section 3.1.1 states:
> >  

[snip]

> >  /**
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 0155fb66b580..bb90ac011290 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -443,6 +443,12 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
> >  	if (!(cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ))
> >  		return 0;
> >  
> > +	if (!cxlds->irq_supported) {
> > +		dev_err(cxlds->dev, "Mailbox interrupts enabled but device indicates no interrupt vectors supported.\n");
> > +		dev_err(cxlds->dev, "Skip mailbox iterrupt configuration.\n");
> > +		return 0;
> > +	}
> > +
> 
> Commit msg says dev_warn() yet here it is dev_err()
> 
> Can you fit in one msg, something like:
> 	"Device does not support mailbox interrupts\n"
> 
> Perhaps skip the hard stops. No other dev_*() in this file adds them.

Dan had comments on cleaning up the error messages.  I'll do those.

> Documentation/process/coding-style.rst
> 
> Spellcheck

Thanks.

[snip]

> >  
> >  static irqreturn_t cxl_event_thread(int irq, void *id)
> > @@ -754,6 +762,13 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
> >  	if (!host_bridge->native_cxl_error)
> >  		return 0;
> >  
> > +	/* Polling not supported */
> 
> I understand this comment while reading it in the context of this patch.
> Lacking that context, maybe it deserves a bit more like you wrote in
> the commit log. Be clear that it's the driver that is not supporting
> polling, and when if or when the driver does add polling support they'll
> be an alternative method for processing events. IIUC  ;)

Yea I wanted to make it clear that polling is an option for the driver but
it was not supported because I don't anticipate the need.  But should a
device come along which requires polling (ie no irq support) it would be
nice to leave breadcrumbs...  but...

> 
> 
> > +	if (!mds->cxlds.irq_supported) {
> > +		dev_err(mds->cxlds.dev, "Host events enabled but device indicates no interrupt vectors supported.\n");
> > +		dev_err(mds->cxlds.dev, "Event polling is not supported, skip event processing.\n");

... here I mention polling not supported which is redundant to the
comment.  Comment should be deleted.

> > +		return 0;
> > +	}
> 
> Similar to above

Yep will clean up based on Dan's feedback.

Thanks for the review!
Ira

[snip]