Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <57cd7a00-7d1a-4cdb-b287-df0c8a557be9@amd.com>
Date:   Mon, 9 Oct 2023 09:55:31 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v11 11/20] cxl/pci: Add RCH downstream port AER register
 discovery
Content-Language: en-US
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>,
        Robert Richter <rrichter@amd.com>
Cc:     Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Dave Jiang <dave.jiang@intel.com>, linux-cxl@vger.kernel.org,
        linux-kernel@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>
References: <20230927154339.1600738-1-rrichter@amd.com>
 <20230927154339.1600738-12-rrichter@amd.com>
 <20231002155334.00004e58@Huawei.com>
From:   Terry Bowman <Terry.Bowman@amd.com>
In-Reply-To: <20231002155334.00004e58@Huawei.com>
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?NnNOcVhEY0ZIUFRLWDdHL0JRWXVGbzdzM0VQTnlidnRjWHBuTlFBNFh2YzdV?=
 =?utf-8?B?RVNzb0FoS3FlSjRTVE1DWTVDN2ZCZjFMNnJEQmdEbFRNclYxdVRmV2szc21s?=
 =?utf-8?B?TFJRMXBGcStRR3ZDeHFHaTRhaDBRZ0pHOXhQREJISUdkSmZNZGZXMmJxWHEr?=
 =?utf-8?B?aG9UbzBoc05ISDYwaXBQYUV6TkFOVkhXSVVRN0tvMTNvdktYMHhkZkJzaDlz?=
 =?utf-8?B?NzlheHFEcXRtZmZVQ3c1d2hVWjBmdnplOEhDd1cwZ0lHeCtmRGRQUzBUUWFI?=
 =?utf-8?B?RzFwZmplUlFybzZkUjd4dFdYUGtqeHNpK3dSdmUxcVo4TG1qWkJmQ0kzYmhB?=
 =?utf-8?B?ekhkR2MwaGI3MFB2K1F1ZGlHT3Rwa2dUcjI4ZzIrcjcxY2tTOFE2MW01TjFQ?=
 =?utf-8?B?ZXFSaUtzSVBua2NQMzM4RmNlVWJGTmtQUVIzOWNXQlNKRi9WaWJ0dmo2Vkgv?=
 =?utf-8?B?VnJrd0hYSW4yb2c5a2RvZjN3USt0VEgrRjNxNHMrM3FsNmtMbmF3VXVwVTUx?=
 =?utf-8?B?ZFJNbW55M2FLZUxDaFpyR0Z4VjUxMTRoa1ZITjVyR0J1bVV3cGRoVmljZ3BG?=
 =?utf-8?B?VHcyeWQ2bTliZ1c1SUVxbC9WTmtFRjNuMklRQmo1VHVMcDZ2SE1MME1hcVhw?=
 =?utf-8?B?YXJVR0NobFJpR1FESlc1RktOdXZFTnNEbW92Y1krOXkwWjdFaHZIMTFpc2Va?=
 =?utf-8?B?MmR4UlZPSjI5RnVkQkpkL1RCVEVVS25WWGE3QVYzbmowWDM3cFpTdHlGOVFW?=
 =?utf-8?B?a3dBK0t2Z2IwWmtvVytMZ0lJYXpJK3FqY2JFZ0R4enk3c2xENmJIWUw1NXR4?=
 =?utf-8?B?dEFzMWZaNS8xNGpnc1ZXckptcXZyT1BWNmRiNG15QXBkQ3VIcFkzOUIyMHUy?=
 =?utf-8?B?ZVNoYWFkbXJoUWt2WjhCMC9ZY3RZL2lCR0RaZzVxTGliYzg0eW9DRTY3b2tw?=
 =?utf-8?B?OFE1dkpid2NaY2VGOS92cmZJTHBObzFpZ2QrdysyZnUwVEhmR3pQaDdMVWNU?=
 =?utf-8?B?Y2xJV3hGUGtTd2VyL01CRnlCaWJWdElvNXhHblN3WU1uUU9CeUNLWE1DL05m?=
 =?utf-8?B?VjdJQnlMSi9mR2tsV2Jmc0I2aXJYajdYaXdGVGVWQXBxTVpTaisvZ250NzRR?=
 =?utf-8?B?RnNuM3c5QUxNWGlBVDE4UTZlSmlKSkxmVnZkNWNYYkh0bkFzNkljVFdZMzFD?=
 =?utf-8?B?NVNZMlcySlBEenZjTGZiM3BGbnJ1ZHdjMUsxSTBiZ0ttNWpIa2hoODhGQ09a?=
 =?utf-8?B?M1VnRmZGOHdONlU0R2o0cnJBQm9MUDFGSkl5TlVFUHRHckZ6VjF1dS9mZVZL?=
 =?utf-8?B?TitROWlONGVXVVRoV09VaWVINGJ4N3pmcWFyZHpweGRYOFFWdk1ydXJLUFF3?=
 =?utf-8?B?MmphTkFMNHd1YTdtWm1HTk1SSktyZmQxbjhsYkYzSDZaN1F3cU8xSWR6UGpi?=
 =?utf-8?B?SWhWT3N1NThhMTZOMExCUGhESmFyNkIzd1VpMVh3a3BENmZMUUlEZkpjU1lN?=
 =?utf-8?B?M3ViQVgvaEM3OEgzZVM1cmJZaUM5ejRCdFJkOXZjbHBQQ1hJVGUrdUxoVjhO?=
 =?utf-8?B?dU9PQkNRam1CV0NZengrUHhYVE1mSjYzK1ZKSVBPZXQ3NTJ5dDhhQnk1SCtB?=
 =?utf-8?B?SnkzZHJwTGFCSTZOVmFqWldUQXQ0TWhucFhTbCtBNXRTd296MTNqZmJrV1pK?=
 =?utf-8?B?QmxMa3hiaDVabnd6ZjVxL3RzRklidVdncm9WbkgyVmdWSWR6OFkycXhrUVZy?=
 =?utf-8?B?dHhWcXVqVEpwSWFGU0hJT1NWeDkyaVNMeWdhMi9WN3N0b0NzK1hlbmNkaXdG?=
 =?utf-8?B?Q1h1NVV3ak9PVVNDc2UzWThCL0p3V1BHQlNpWnltUkN6Mkp6U1c1eDZlNFdC?=
 =?utf-8?B?aUtSVWp3R1owV0h0NjhIQ0xQek9VUmUyQTkvMkZJYXBYTnMrZ1ZraE9FSEU4?=
 =?utf-8?B?QloxRmtOZ2N6dzJVckVNY0ZESnFjalZWVkxtQm05UTM1eHVvN3g1WC9iV010?=
 =?utf-8?B?d0NEYzZDRklIMnJwc3pYNkxmRElPK0xTRDFINGNzRURrSXIyUGdLVlhmR2gx?=
 =?utf-8?B?Vm9QRStJMGJoeThkeFRvUVZWZUVMeWVuenY5SUZhaUhVRE96Y1ROa1RqY0t0?=
 =?utf-8?Q?KVpkx3M1R/IBb9CV4EFDrIVPi?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ea9801d5-0b78-4804-afcb-08dbc8d7ca50
X-MS-Exchange-CrossTenant-AuthSource: DS0PR12MB6390.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Oct 2023 14:55:34.2059
 (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: 0ENIn16SKwV4RfdM8SSbRL4Kp+jCx+P+2Lgnn5AxK9luTp347lKZiIg567H1KOY+olNvU8cvVpr6lE/h8U2OOw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR12MB8445
X-Spam-Status: No, score=2.7 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS autolearn=no
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.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 (lipwig.vger.email [0.0.0.0]); Mon, 09 Oct 2023 07:55:51 -0700 (PDT)
X-Spam-Level: **

Hi Jonathan,

I added responses inline below.

On 10/2/23 09:53, Jonathan Cameron wrote:
> On Wed, 27 Sep 2023 17:43:30 +0200
> Robert Richter <rrichter@amd.com> wrote:
> 
>> From: Terry Bowman <terry.bowman@amd.com>
>>
>> Restricted CXL host (RCH) downstream port AER information is not currently
>> logged while in the error state. One problem preventing the error logging
>> is the AER and RAS registers are not accessible. The CXL driver requires
>> changes to find RCH downstream port AER and RAS registers for purpose of
>> error logging.
>>
>> RCH downstream ports are not enumerated during a PCI bus scan and are
>> instead discovered using system firmware, ACPI in this case.[1] The
>> downstream port is implemented as a Root Complex Register Block (RCRB).
>> The RCRB is a 4k memory block containing PCIe registers based on the PCIe
>> root port.[2] The RCRB includes AER extended capability registers used for
>> reporting errors. Note, the RCH's AER Capability is located in the RCRB
>> memory space instead of PCI configuration space, thus its register access
>> is different. Existing kernel PCIe AER functions can not be used to manage
>> the downstream port AER capabilities and RAS registers because the port was
>> not enumerated during PCI scan and the registers are not PCI config
>> accessible.
>>
>> Discover RCH downstream port AER extended capability registers. Use MMIO
>> accesses to search for extended AER capability in RCRB register space.
>>
>> [1] CXL 3.0 Spec, 9.11.2 - System Firmware View of CXL 1.1 Hierarchy
>> [2] CXL 3.0 Spec, 8.2.1.1 - RCH Downstream Port RCRB
>>
>> Co-developed-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> Signed-off-by: Robert Richter <rrichter@amd.com>
> 
> This doesn't look right. IIRC Co-dev tag should be just
> before the SoB.
> 

Yes, that needs to be swapped.

>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> 
> New day, fresh questions....
> 
> 
>> ---
>>  drivers/cxl/core/core.h |  1 +
>>  drivers/cxl/core/pci.c  |  6 ++++++
>>  drivers/cxl/core/regs.c | 35 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
>> index 45e7e044cf4a..f470ef5c0a6a 100644
>> --- a/drivers/cxl/core/core.h
>> +++ b/drivers/cxl/core/core.h
>> @@ -73,6 +73,7 @@ struct cxl_rcrb_info;
>>  resource_size_t __rcrb_to_component(struct device *dev,
>>  				    struct cxl_rcrb_info *ri,
>>  				    enum cxl_rcrb which);
>> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
>>  
>>  extern struct rw_semaphore cxl_dpa_rwsem;
>>  
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 6ba3b7370816..4c6c5c7ba5a3 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -722,6 +722,12 @@ static bool cxl_report_and_clear(struct cxl_dev_state *cxlds)
>>  
>>  void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
>>  {
>> +	struct device *dport_dev = dport->dport_dev;
>> +	struct pci_host_bridge *host_bridge;
>> +
>> +	host_bridge = to_pci_host_bridge(dport_dev);
>> +	if (host_bridge->native_cxl_error)
>> +		dport->rcrb.aer_cap = cxl_rcrb_to_aer(dport_dev, dport->rcrb.base);
>>  }
>>  EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_parent_dport, CXL);
>>  
>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
>> index e0fbe964f6f0..6e502f02899b 100644
>> --- a/drivers/cxl/core/regs.c
>> +++ b/drivers/cxl/core/regs.c
>> @@ -470,6 +470,41 @@ int cxl_setup_regs(struct cxl_register_map *map)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_setup_regs, CXL);
>>  
>> +u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb)
>> +{
>> +	void __iomem *addr;
>> +	u16 offset = 0;
>> +	u32 cap_hdr;
>> +
>> +	if (WARN_ON_ONCE(rcrb == CXL_RESOURCE_NONE))
>> +		return 0;
>> +
>> +	if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
>> +		return 0;
>> +
>> +	addr = ioremap(rcrb, SZ_4K);
>> +	if (!addr) {
> 
> Given this handling exists, below, perhaps a goto?
Yes, will make that change.

> Also, why isn't this an error?  A comment would be good for that.
> 
The intent is to avoid failing the driver probe in the case of missing RCH 
error handling.

>> +		release_mem_region(rcrb, SZ_4K);
>> +		return 0;
>> +	}
>> +
>> +	cap_hdr = readl(addr + offset);
>> +	while (PCI_EXT_CAP_ID(cap_hdr) != PCI_EXT_CAP_ID_ERR) {
>> +		offset = PCI_EXT_CAP_NEXT(cap_hdr);
>> +		if (!offset)
>> +			break;
>> +		cap_hdr = readl(addr + offset);
>> +	}
>> +
>> +	if (offset)
> 
> Add a comment / specification reference for why an offset of 0 is not valid.
> Of the top of my head I'm not sure though there may be a requirement for
> something else coming first...
> 

I can add spec documentation:

'For Extended Capabilities implemented in Configuration Space, this offset is 
relative to the beginning of PCI-compatible Configuration Space and thus must 
always be either 000h (for terminating list of Capabilities) or greater than 0FFh.' [1]

[1] - PCI 6.0 - 7.9.7.1 RCRB Header Extended Capability Header (Offset 00h)

Regards,
Terry

>> +		dev_dbg(dev, "found AER extended capability (0x%x)\n", offset);
>> +
>> +	iounmap(addr);
>> +	release_mem_region(rcrb, SZ_4K);
>> +
>> +	return offset;
>> +}
>> +
>>  resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri,
>>  				    enum cxl_rcrb which)
>>  {
>