Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b17db058-c59e-8014-81ed-0bbd9a1564cc@amd.com>
Date:   Tue, 13 Jun 2023 10:28:19 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v5 23/26] cxl/pci: Disable root port interrupts in RCH
 mode
Content-Language: en-US
To:     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
References: <20230607221651.2454764-1-terry.bowman@amd.com>
 <20230607221651.2454764-24-terry.bowman@amd.com>
 <64878016974b0_1433ac29473@dwillia2-xfh.jf.intel.com.notmuch>
From:   Terry Bowman <Terry.Bowman@amd.com>
In-Reply-To: <64878016974b0_1433ac29473@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?U1NZMm5wNW00cm1wbmRYNFF4ZkpIYmZ3Tm9RZHkreEozMExJK2FLWUo3bCtC?=
 =?utf-8?B?cG8rMS9XZ1N3Rkw5cDhNWS9BMTRVdFY2RGNRNUZsWFo2RzRQTmRJOGpEM2py?=
 =?utf-8?B?ckcreW05MTlrTzJYa1UyVmhlTC9kOXdkcUpudEtFYVh4bDNYc0s2c3dlU25i?=
 =?utf-8?B?eFp3MVF1RkVnYi8rY3ltMzl4SEZ2b2tCTlp1TGZzNEFCdjZocllLNWh6MXMw?=
 =?utf-8?B?anVleXRTa2duMHdteXFCYi9VSXRHQXBZVC8vaUlUMzZha1M1UmdUNlFKbENa?=
 =?utf-8?B?OUZhY1dPdnNFUm51RzR4TTg3MTdmVUpvbFIvUDdmZnZldXRNd1J2cGt3R25j?=
 =?utf-8?B?d2dRMDA5b3Fabkg1NThjbVFUMFUxYzFRbXlCUXJFUHRUOENkUkxJc3lNbzRy?=
 =?utf-8?B?NitTZzhjQ203cUtOSnFZbk1MVFozV29DVS9BYTJCa1NiWXVUTGRkR202Vm1n?=
 =?utf-8?B?Z0hiTVY0UW5zclFkQ0tNSHhQc3RURXpPclZWK0s3dm9JckRtUGtMY3BNS1Iv?=
 =?utf-8?B?MCtuNWFRbzlRbmhCVTNuQ3BYUEZVL003cTIzR1ZRRVVLdmlpL0tkbkYwc1NR?=
 =?utf-8?B?SVU4cTBYcUh5OXZuVjQ1VEFuZnVUOUJJcFFKSnYzb3hPbzZsUXpDc0FJcjZX?=
 =?utf-8?B?c1hSNVYwT252dFNBbnJZTklFbjhtblp2dE1CbTFXV0dIYTlINXVQNDQzcXJV?=
 =?utf-8?B?Z1hhVTNxMkVlSUExb3djNGJTT3dpQVJ4d1pHV2V2N3p0WHlzQVE2ejRjdkh1?=
 =?utf-8?B?MFhUa2haRmwvcUIvdW1PMWV4MkpZN0JIU0t0YXdtMm02Z3Q1Z1p0OU5DWDVZ?=
 =?utf-8?B?SXlzWFlYU3k2aHRmTkZBbkhtbTR4UTBCdXBlWU5jbXE3a3B6NjFBRjkvUG5Y?=
 =?utf-8?B?eUh6ZjJsc1dmOG1GOUtLU0s4eC9yOFhYdndkdGFFVm9waVc3V0M3Yy9weTF6?=
 =?utf-8?B?bkpwNTZxNU5LNDZsaVJjZ3E0WkN5b09Sd3ovdmRUV05kSWJPWVhpaTIxUHo5?=
 =?utf-8?B?cFRET2I0TDJ2cEJtK3pRTzZ0bFRsSDZZL2F3N0lWSmFidmsrQXY2Q2U0cTdQ?=
 =?utf-8?B?OFUyRTdHYzFjQTkza0xXNjBZTkdqL0tEa01QNFFNdHo5dDhPZDArNm9PSENv?=
 =?utf-8?B?cEU0c2tQcE4wdzlrTVhzdlVFT0x2TE52dFUvd1Z6czlWb0hSNkNCUysxdlBS?=
 =?utf-8?B?Z3E3QWRQQlRWWjBrWWZEYmoyZUp0YVF1ZWZVYWJFVjh6TVlIZGhrOE02UVk5?=
 =?utf-8?B?UDJJTkg2a05zN2hUbDBTZFhIa0M3VXl0cEw0SVVLdDFtOWJiaElnclVXVHdz?=
 =?utf-8?B?TmdjN3FvTHo3a0hEVytRVUE4N0l3eTY3YkRmQldmT3JWMWh4aEtDYWhOckh0?=
 =?utf-8?B?dGtobGJ5Z3Q5WFZUeVhtTHZNSENmYmpaV0h5cDhDb29IYzVuekJoeDdzeFVN?=
 =?utf-8?B?eVQ2WXZUK1pnNnZCUmFITWkwdFIxNUpUeXpBUjdBeTRBdjd2VEhLa1RjWVpQ?=
 =?utf-8?B?Uk16YldnQkwyY3BnNHdQOXNOVmgrSUgrTEhwbzVBQzcyd3JYTStlMkd3QUor?=
 =?utf-8?B?ajVxU0VUeFFqeDk0WkdjSHE1dXQ2b1VtSDFWZ0dxejRwZXRZUkViT01aRmMr?=
 =?utf-8?B?bWtIYjQ5QXd3MUlVSFlxb1ZuaE5rWDVHNUI0ZkkwZkt1Z2Fkd1Q2blVZOU5Y?=
 =?utf-8?B?SHZSS2RYU3pDYmpTTlFoU3ZGM2JmMXpJTnVyRGpZN052aXdKSWN0MEVDdFBu?=
 =?utf-8?B?WmZSV2xUVnFyQmZteDNQYjkwM2N5QkJUcDM2N1NobklvVXg2Mjd2N3FiNHR2?=
 =?utf-8?B?WURUQmR5RE9VQVJGakRlUFpzenhoSk8yZGc0N00wUVp3a0t1WlFuWjBGR3ZN?=
 =?utf-8?B?dXNlWnhUN3pzc2tFc0pWOHd1TXFYOHo5QjdCVGl6ZmVJNzJZbTZVZ1BGb1h2?=
 =?utf-8?B?eHBORFVLdFI5QWxSdDNvYlM2YzFwTlREZTg3TkJWcE5VU0U1MVZEakpzWkc2?=
 =?utf-8?B?SjlVWEE1UE9Sd3VWelBDckhJN2dtRUUvbVZremdYak1saXJOeXNvKzZ4ak5N?=
 =?utf-8?B?U2dEck85emhRUCsxNHRiTVRJSTg5N2ZxTGxKa1JldHhGWXFjN2dGWHhReVh2?=
 =?utf-8?Q?qS+NwB9C2nzUpHwQqMIjRm/FQ?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bad8bbc3-78e2-4796-cd59-08db6c22d25b
X-MS-Exchange-CrossTenant-AuthSource: DS0PR12MB6390.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2023 15:28:21.8064
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LIrzJf3lpvO5URfq9uv2xnke5l9hPls73IcHAB5WF8J9luodfEYeTHmoBzzScpfAX7ucRIXwJL+lQ8+vn66QgA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB7887
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Dan,

Thanks for the review.

On 6/12/23 15:29, 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>
>> ---
>>  drivers/cxl/core/port.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index bc5d0ee9da54..828ae69086c4 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -981,6 +981,30 @@ static int cxl_dport_map_regs(struct cxl_dport *dport)
>>  	return cxl_dport_map_rch_aer(dport);
>>  }
>>  
>> +static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>> +{
>> +	void __iomem *aer_base = dport->regs.dport_aer;
>> +	u32 aer_cmd_mask, aer_cmd;
>> +
>> +	if (!dport->rch || !aer_base)
>> +		return;
> 
> Does this need to check ->rch? There is no path that sets
> ->regs.dport_aer in the VH case, right?
> 

Correct. This can be simplified to only check !aer_base because 
aer_base is only set if dport->rch.

>> +
>> +	/*
>> +	 * Disable RCH root port command interrupts.
>> +	 * CXL 3.0 12.2.1.1 - RCH Downstream Port-detected Errors
>> +	 *
>> +	 * This sequnce may not be necessary. CXL spec states disabling
>> +	 * the root cmd register's interrupts is required. But, PCI spec
>> +	 * shows these are disabled by default on reset.
>> +	 */
>> +	aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
>> +			PCI_ERR_ROOT_CMD_NONFATAL_EN |
>> +			PCI_ERR_ROOT_CMD_FATAL_EN);
>> +	aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
>> +	aer_cmd &= ~aer_cmd_mask;
>> +	writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
> 
> What about the scenario where @host_bridge->native_cxl_error is false?
> Should the driver unconditionally touch AER registers?
> 

Right. The driver should only touch the AER registers if the AER control is OSC negotiated to OS.
I'll add check for @host_bridge->native_cxl_error. 

>> +}
>> +
>>  static struct cxl_dport *
>>  __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  		     int port_id, resource_size_t component_reg_phys,
>> @@ -1038,6 +1062,8 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>>  	if (rc && rc != -ENODEV)
>>  		return ERR_PTR(rc);
>>  
>> +	cxl_disable_rch_root_ints(dport);
>> +
> 
> It feels odd to modify hardware state within an object setup helper. Is
> there a reason this needs to be settled before __devm_cxl_add_dport()
> returns? If not this feels like a helper that cxl_acpi should call to
> change the state of the @dport instance it allocated.
Sure. This can be moved into the caller, add_host_bridge_dport(), after calling devm_cxl_add_rch_dport().

Regards,
Terry