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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <659ecb0b70b8f_a3379294e9@iweiny-mobl.notmuch>
Date: Wed, 10 Jan 2024 08:51:23 -0800
From: Ira Weiny <ira.weiny@...el.com>
To: Alison Schofield <alison.schofield@...el.com>, Ira Weiny
	<ira.weiny@...el.com>
CC: Davidlohr Bueso <dave@...olabs.net>, Jonathan Cameron
	<jonathan.cameron@...wei.com>, Dave Jiang <dave.jiang@...el.com>, "Vishal
 Verma" <vishal.l.verma@...el.com>, Dan Williams <dan.j.williams@...el.com>,
	<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] cxl/pci: Skip irq features if irq's are not supported

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]