Received-SPF: pass (google.com: domain of linux-kernel+bounces-22507-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 10 Jan 2024 08:51:23 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Alison Schofield <alison.schofield@intel.com>, Ira Weiny
	<ira.weiny@intel.com>
CC: Davidlohr Bueso <dave@stgolabs.net>, Jonathan Cameron
	<jonathan.cameron@huawei.com>, Dave Jiang <dave.jiang@intel.com>, "Vishal
 Verma" <vishal.l.verma@intel.com>, Dan Williams <dan.j.williams@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC] cxl/pci: Skip irq features if irq's are not supported
Message-ID: <659ecb0b70b8f_a3379294e9@iweiny-mobl.notmuch>
References: <20240108-dont-fail-irq-v1-1-4407228debd2@intel.com>
 <ZZ3W5q4UzvLJ9kG+@aschofie-mobl2>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZZ3W5q4UzvLJ9kG+@aschofie-mobl2>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?BNKr944r/v31aV3/Ahp1bFKRGETldpCY2iZsadsJ5zo3ULWcfPLU6WWPafCA?=
 =?us-ascii?Q?AwSoZqAIKM58AMBifNU34iB6KUKTATP9QSQsTvuFIFdLOFrNukQMkj8XvhPL?=
 =?us-ascii?Q?yiBLw0jF3iqyx72402OMFB7Cz6TrF9XlbfFNkZhCM12dXPFE/xVe+jFA+Mze?=
 =?us-ascii?Q?2pHxNZ0d7NTJQ1bhqiMQs+F5CUVMmoZytzoGYdDDpJC/IozJat2Md6+5Wbrz?=
 =?us-ascii?Q?F0//qE4r+lT5i7lLjXF8LfmazINyoO7MG8mPOPwIhu8oApRpij750W5KVGLW?=
 =?us-ascii?Q?BiJVMT6Oza1RLLE8zTje6YeWDEfFHmFshjQ4nBhwhNNiBuMHC/HfyGsD7mJr?=
 =?us-ascii?Q?P2mA612Vu1Thl0lSsT36dhKhDXdAbKD1frnVvQWIYISmGhF8rixFR6rogPtJ?=
 =?us-ascii?Q?RBFUE34UrjSXO3onlSE7GfnkX22pqZu5nUtXpibEE6XyDtJAe5poowuBoCqn?=
 =?us-ascii?Q?ROECLVpAY5f6edfxILqsIeDtIU75N4LEA3dpwUaR0dSovCeLnJJZClClVyg6?=
 =?us-ascii?Q?+xkXLRtQ+0DStCqhlbFqlljuZX6JXV28RPLDhpBdc2htXQpD1m70mIoJsPYR?=
 =?us-ascii?Q?+B1YswFoAT9UAaWk2EFvJh+cXvnYvXIZobjs7MZJeTw5hLDLoz11MxjJSBpx?=
 =?us-ascii?Q?GUIBcfYw+qVx6vjljn2R0HXEGkqj38CW7oGiwUAKAo77vos07bQrGVdXO489?=
 =?us-ascii?Q?0tu0lW+OMvnC+2e8cF9v3xRgDP6MeWIGS/h2XR0td14USyhvVf8aPPvLwRRF?=
 =?us-ascii?Q?9AqU6SjpuT3i9jFPgDdvwfOuB7+nOhocfXBGyexAA3N6amXFESJEmTcIFOgu?=
 =?us-ascii?Q?QqjGPuOg8BR5jxrw6N9m2WOzMtRv+GYurgPpnZtLdn2ekA3HzhgbSq/eXY8C?=
 =?us-ascii?Q?SrFLdJpRfCGD2MyeNi0bX/wiWFCaK2yM23q1fbaztNo5IpWrxGRggkNMPN2y?=
 =?us-ascii?Q?eyuIhYCfvVfWqoow5OGv4P/vNxLsPjquO5j0LuL2+Hlg/yALNXfn+GTsMYel?=
 =?us-ascii?Q?+vya4qFoHDfb/VqncdECNiYjpYoHDXMwhmJ0T9r9tEY+s9YWQbZk1gTUo8on?=
 =?us-ascii?Q?jHtKlikMXCfqAIVWbGr/NYDRqpW4HJe61FbudvXe0fA2G8vw5e1tOTazPPsx?=
 =?us-ascii?Q?f1BUNF7VkMOXRj1zL8l/gQS16CK21rSnwKERG+L7QbT3At1a8ZG/Wf0w/p7Z?=
 =?us-ascii?Q?uRgfSaI3BstK1SDmy/akajN2gdLnY23vqYjx0naN3eqm8fz2k4yKNRjsXemS?=
 =?us-ascii?Q?mgnFPotI0sEFkZXn8NsKt6dpEGAwPh92rD1YcY01BMKygO2RrqJO4Tqe5grk?=
 =?us-ascii?Q?Lp65Qz0eol+loYObwfzeqLceEhly3GhqjIR+HWWR52g671NbJ1w5mQz1AI/o?=
 =?us-ascii?Q?WBrmiWybWZKhj2BshXtfZtaa8LB/D6N3lQvHh46KtmhRsl3pxqLena9E0VRj?=
 =?us-ascii?Q?jjUlwtcQvTz/Rg9wXLSglctLknFP28qOdDaNh9KLfouSNGL1fS5fI1zNskej?=
 =?us-ascii?Q?BUsQnaXfSyiE+kqgRXQdSiXcxNMnTr1retS0uu7r9qF4o67dblM5aAPKCgFd?=
 =?us-ascii?Q?Vuucdu2VDGBOR0jMbdsPmtvXZ415TewEzI28pr1a?=
X-MS-Exchange-CrossTenant-Network-Message-Id: de4f034e-7f37-4cc6-7956-08dc11fc63c8
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2024 16:51:28.5426
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dU259ULK+gZTIk6FrPbeZ84L3c4qsD6mUpWBODoP+FU9YSTSpX7A67VY0ojbZry88s3tObp/SO7Xrk4XOvYQPA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7695
X-OriginatorOrg: 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]