Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Tue, 26 Sep 2023 14:47:01 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Terry Bowman <Terry.Bowman@amd.com>,
        Dan Williams <dan.j.williams@intel.com>,
        <alison.schofield@intel.com>, <vishal.l.verma@intel.com>,
        <ira.weiny@intel.com>, <bwidawsk@kernel.org>,
        <dave.jiang@intel.com>, <Jonathan.Cameron@huawei.com>,
        <linux-cxl@vger.kernel.org>
CC:     <rrichter@amd.com>, <linux-kernel@vger.kernel.org>,
        <bhelgaas@google.com>
Subject: Re: [PATCH v10 12/15] cxl/pci: Disable root port interrupts in RCH
 mode
Message-ID: <6513515526995_bf9129471@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230831170248.185078-1-terry.bowman@amd.com>
 <20230831170248.185078-5-terry.bowman@amd.com>
 <6504a5e23cee9_d7cc8294b7@dwillia2-xfh.jf.intel.com.notmuch>
 <f27f685c-6b53-8955-735f-e9c0c04354a5@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <f27f685c-6b53-8955-735f-e9c0c04354a5@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?EARsAhRax+yIU/ePGZlR0/kBSCkJLjrqm9UHVf/3tc866HbjxwzReJiTxvOt?=
 =?us-ascii?Q?mlIJ2C6zyejSToioai1A9twaurdPvblZVAQpATgiP7GRv6Ys5pUHMQJiO7XK?=
 =?us-ascii?Q?UDfOideh1Kd1bNqX8qjna5k3kTlGEQDmr4/EhCLVCmwV/gmtILiXFweOo2oQ?=
 =?us-ascii?Q?bam4MICCXGLvYHUzHvvFtfSWnMUMPTMdBSdPOt9vuu5wX0Oi4DIcG2WL53Tv?=
 =?us-ascii?Q?54X/K0C/E0TXnzSQe0tBawhQFDS7Hv5Co2bIWpY89nvotMkGlCWRyzJcBW7L?=
 =?us-ascii?Q?QCnS/3M2Y55J0I75OLiQI7rO2RSQBIDRMS3RIbM4yOtKwNlu5xTgqX2aX8c2?=
 =?us-ascii?Q?989KfX9pNBcKLkZl6c/8drgNnKoJBU1pedL7i6PlFQo77eI+Iot81VuzBLNE?=
 =?us-ascii?Q?JSpfIWG7mkyUWOtgPVEAuZ8f56nWIZHx5bipsznbYW9O3OAc6lAbjl6VUGRA?=
 =?us-ascii?Q?VTypJWys5o+0vlLza2M2Du8xvVJhDCEdc1B2uDfzHZrdORMP5C7qFyPPTLDd?=
 =?us-ascii?Q?fLAFjbK9TqkTdWs+PfpxzcIAUHeTrR5uyS4JW4A0tya5jiBb50NN1ZxvQMe/?=
 =?us-ascii?Q?RyqgENGJ8exrenYLqQL9B6bPbZd3WgKUL0rQvURn7fycXYBkAG5+1WbzI3KN?=
 =?us-ascii?Q?l3eZNeKZoFhRNy4dps7HorS7/MLM1jXUErcOHbttz/wT1M6WL9VtCjwRtttj?=
 =?us-ascii?Q?rdWXDlzU3cQExUcrOTq4bmxKdwPOhpWgO4aCV4G6r1yabflE26DGafdyCTpU?=
 =?us-ascii?Q?k4fzL6BSg0cdwdvzBun/ckxDA2H2GLHQ0RIMhYDhAXF4dWRE2utw01uzDRrM?=
 =?us-ascii?Q?nO9g6xWF4uJD+uTUNOFe+nqgmNAzVe3fQo6IULr6r1GAYc5jIaZya5NcjwOP?=
 =?us-ascii?Q?gZ3dU3WwA9uQ9PfAEUOO0hiJI/N9gyF2Zy4Oc8tueOsHUoaPkG9Ww9j8W3+U?=
 =?us-ascii?Q?wIgsMg4hIRBpHGfgdklGGQf5c4NAignxSdshbXSUhbQW9bY6nqo+YxWo9Vx0?=
 =?us-ascii?Q?FF3mNkWrznzXm4pV0163lzbLNYXhh6Y49qr6a5sWKoADWjilxiy7bZYJvqIB?=
 =?us-ascii?Q?pkRjBm/jGBvj8R2jvom0mFLo3qrGlRinzU/hINm0Ol3DrHliKy9SMVueSMHd?=
 =?us-ascii?Q?eg6/KbGKXFf1mFx9LDg7fA+rzOZIrufQDsbN2MYoUFRMQ4x0qM3IIc33N0Nw?=
 =?us-ascii?Q?4StATULXNWtfSOS4C94WnGvfQnLSd1ZM6XKn28nbHzdo23lnnxbsYUYYWCKn?=
 =?us-ascii?Q?HG50Rcu+y6Dl9JQdlsjaN7zjCNC/78zUnvws8LdS5iliIvxU6x+NyZIvGIkZ?=
 =?us-ascii?Q?5uUz7Zg8P0hsJxkTU6bzcnEcOq9xlzNtK2BnyyHxOn9NaG6tb6uLUDF4WqiK?=
 =?us-ascii?Q?kgaLH05Zh9L3i6ES727FYLdD2EHJfAt8kgZqKKhf6jPwguFTD6VDXgplyxe8?=
 =?us-ascii?Q?y3n0nIg4EjVZ4vPQCbIRbj0dIl5KfssNF3yi6q36kT36kLh84hFjuMpRPBb0?=
 =?us-ascii?Q?/c0N3lgxvhv0UgqzShAUe2u/deCDra9cL2q6X+VlOtfpEHNaCowFADKy3/kX?=
 =?us-ascii?Q?UgvgX+zYbKFYuz+7KyLon9D4CYvTVCzshlaTjtotRBk+8IbN5kly+wZxkabT?=
 =?us-ascii?Q?/g=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1bac2512-146e-4415-cb49-08dbbeda1f44
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2023 21:47:04.1922
 (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: S+STk7BoOwwOQOYiSO81HDcBkPOcuWmJUtgjJVSN+Z92TbmBWvBg9VpoDIPKW77V5YchwnUUNwuo8mFT8VpvF14xYn+FoX2V/zPgl4OYAaw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5307
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 26 Sep 2023 16:42:57 -0700 (PDT)

Terry Bowman wrote:
> Hi Dan,
> 
> I added comments below.
> 
> On 9/15/23 13:43, Dan Williams wrote:
> > Terry Bowman wrote:
> >> The RCH root port contains root command AER registers that should not be
> >> enabled.[1] Disable these to prevent root port interrupts.
> >>
> >> [1] CXL 3.0 - 12.2.1.1 RCH Downstream Port-detected Errors
> >>
> >> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> >> Signed-off-by: Robert Richter <rrichter@amd.com>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > [..]
> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> >> index 2a22a7ed4704..d195af72ed65 100644
> >> --- a/drivers/cxl/core/port.c
> >> +++ b/drivers/cxl/core/port.c
> >> @@ -1042,6 +1042,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> >>  
> >>  	cxl_dport_map_regs(dport);
> >>  
> >> +	if (dport->rch)
> >> +		cxl_disable_rch_root_ints(dport);
> >> +
> > 
> > Similar to the comment about cxl_dport_map_regs() not being appropriate
> > in an enumeration routine, this also needs to move out of _add_dport. It
> > occurs to me that it should also be undone on driver detach just like
> > other device "enables".
> 
> Ok. I will move out of enumeration. 
> 
> Per the 'undo' request: This is a RCH downstream port (dport) with PCIe root port 
> capability. PCI spec states root port error reporting is disabled by default at 
> powerup. And SW does *not* enable the root port errors because the RCH dport is *not* 
> bound to a root port driver (missing BDF, etc). This mask is added to follow the 
> CXL spec precisely and if the rest of the system behaves as expected should not 
> be necessary. 

Ah, got it perhaps add a comment to sanity check that the hardware is in
the per-spec state. Are you certain that even in firmware-first error
handling it is safe for the driver to unconditionally disable these
interrupts?

> I don't believe masking should be 'undone' in driver detach or elsewhere. Adding 
> the 'undo' masking would potentially introduce RCH dport root port interrupt 
> reporting which is incorrect for the RCH/RCD mode. Only CXL components (device, 
> uport, switch) may reside under the RCH dport and never want RCH dport reporting 
> root port errors. RCEC reports the root complex errors in RCH/RCD mode.

Ok, that also seems to suggest that even in the firmware-first case the
driver should make sure they are off per-spec.