Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <da5bc564-76c4-4078-97ab-1d543933071e@amd.com>
Date:   Mon, 16 Oct 2023 08:40:23 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v11 10/20] cxl/pci: Introduce config option PCIEAER_CXL
From:   Terry Bowman <Terry.Bowman@amd.com>
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-11-rrichter@amd.com>
 <20231002154628.00004f9b@Huawei.com>
 <21fa3d41-3585-40b4-b919-d3b66557e9d8@amd.com>
Content-Language: en-US
In-Reply-To: <21fa3d41-3585-40b4-b919-d3b66557e9d8@amd.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?ZDBCMHhJbW9WN2xWWlNCYUFVZ2dZKzFFUEFTM1NPUndOcFc3K0VaNWw2ZE05?=
 =?utf-8?B?RC9TODNLTUJJMHFBd1Q5RFQ2SEtubDhPQnkwQW5ZSE13WGtIUXN2UG5MbnNx?=
 =?utf-8?B?U1pscHV0YWg1dkdCYWRvRmZNUFZ4RWFCeEpUc2ZaYmZoUkJmT1FoYmRKWkdC?=
 =?utf-8?B?eWpkVEpTQU4zUlRZNGI4eTUyeGFzY2ZEZUdUQnNFanZMbzBoem1ydTFQa2Fq?=
 =?utf-8?B?UDFsN1hMUlo4WkFPaFA3NWp2Q3pGZys0ZDl1TklLclpUSmhoTy9wekROcUEx?=
 =?utf-8?B?alp3dGFTWjVicWxIYk5oV2Vlb2hFTUJCNDZOTGhjSjJXZ1VnYWNXeXRRN0tL?=
 =?utf-8?B?VU1UaTFLTit2b2pMZ0pqa3ZUS1JWdXJ5NnYxc0Y0V052RW5kcVQ4QnpGZEdl?=
 =?utf-8?B?bzdmakVNQWw3em5GSU03SUpjbHUvUnZVQi9wWUd5QjVnOVU2MnVsS2ZGaGxQ?=
 =?utf-8?B?MlJobE9CK00wYlROVkFNak5zVlNmSXFOSnAxOEdkNlJrNTRsU1AwN0ZYRnJw?=
 =?utf-8?B?U2h1aWRMMTlPTWEyWjU0b216dGtUVU9MVjVyVTc3c1QrVHdjRzlKY3dWUk4x?=
 =?utf-8?B?ZFgwVTd4Y0IzNGhPTXFkVDg1ZjV1bFZIVGU4NW90MGdOWGk5L0xqVmwxUmky?=
 =?utf-8?B?c2hGbDVJS3YxREYvSkJ6Sm4yRTNYU08xWkg5c0lkQUVLUTZGMm5lSC9TSkRk?=
 =?utf-8?B?b09lY2hhVVJDZVMwWHBPYmxEaXZLZUVodXlmd0FnQU5XeHBiVCtFaDU1M2Za?=
 =?utf-8?B?QnZoVllyT21ZcEdiRE9UdXhNQmw4bXJIaWMvd3ExYXYvbnl0aURxbG1lK21q?=
 =?utf-8?B?N2NYemJBL3hpbzIyOVJsU0ZpYjU0SndRcjBlS3AwYlZFL2FqQWl2emd1anFs?=
 =?utf-8?B?bmgzM3g1WWh6UTVCWk1odDlPRFh1QVlwOWVJSE5KRFgzSGxhTjR4MG52VUZH?=
 =?utf-8?B?UER2RXlFbzdlYVJmSUZUUHNCL0NlSVQ2OU13Ty9qSTBNaFB0Uyt2bzNZMDNq?=
 =?utf-8?B?Mkt6Yk5VbE1ISHpxUHV0eWNUNncvRG9JYzJvOVNEajUyYWVGcjl4dlpGSUN1?=
 =?utf-8?B?RkgzTkI1Qm5VTTNydVMvUTBjS1hWQnYvMU81VGMrM0pia2VWSWV1QUVNZHNM?=
 =?utf-8?B?WWN5NUVTYVNpTythTVVoQmNYbWppaE5KV2lxcm5EK2lHY3VoU0pNZXRmTWVs?=
 =?utf-8?B?d2R4WVpyK2F4b1V5U0Q2ZDhVeVR5WE11VWNDQ0NCRnNpQkZDdkpwRHM5LzlN?=
 =?utf-8?B?VVRVOFlOYkdKOXp3WVhNZEF3bG9FeTV1SHo2SmtxZUN4K0Y4YkVhQzI0WWZS?=
 =?utf-8?B?eFc1Zm11RGxtMUtnVGFMRDhiZ3oxUXdHSDhQanJkQ3E0RlEvV2d4eFcyQUNN?=
 =?utf-8?B?TERrNzBpY2ZVWnFpUk9OR2lSRHVaUmNOWS9RNjQ4bm16TlZTVlQvMERCb3or?=
 =?utf-8?B?VlBJRDFQNFdyVnJSMlU5SVA4MTRvTVFTM1czNWlCNGQxR3pyT3I2cWdMTit3?=
 =?utf-8?B?bm85cTViZENnbkNTMmZSdmMzd1MvQTdCWkJnQjQ0Y2tCT0RrbnJhNFRGVHBK?=
 =?utf-8?B?T05qYTNjOVNVT24zMHh1WHJ1Yy9zcTNOc24zYWdsVXI4bGU5dlozMkk5YkJ0?=
 =?utf-8?B?N1BoNEJxSHRjbWlSWDlxY1FTWm1ubHQ4dUVRcUYyeEs1RU4yNEV4QjBxUFd1?=
 =?utf-8?B?VUU4SFU4RzEvbEJCMWhySDhEcHd1SjFQTGVNQzdNVXp6cmxURDhkQTZmdjhs?=
 =?utf-8?B?NVpjbEN5VE9uLzRNYmliSk5FUWhWRWxDejFCb25kR3BUZ0VMZHdsN21yLzlW?=
 =?utf-8?B?eDVLVG42cEdXbVIrTGgwZHFLdEN3dUNFVEFrQWFWek1id0k1d2hjZVdpRHpT?=
 =?utf-8?B?dFFIK1ppb0lwT2FiRXBXSERhWWJWUmUzRFdycmJsYjIwTHFjTVFxeWpscC9w?=
 =?utf-8?B?c0JCL1BhSVI2YXhZeVFkQTR5WGwyTHJkZVhxUUxHNk9QTzZhYWpvYkt2NThS?=
 =?utf-8?B?cW5uVjVSOXhnRzlaOEFUeTI4NVhXZzU4dTlyTWdYclpZU0JqRkZ4VWNxdlB6?=
 =?utf-8?B?UGwxZkhQSGdFYmdqVUVlc01TeG5TUEo4czlYcllNb0wzM2xvd0NOVlpGbWNB?=
 =?utf-8?Q?oXw1+DkdT1gPW+dKp97WxW1G7?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8590af3a-fe0c-43ca-82fa-08dbce4d73f5
X-MS-Exchange-CrossTenant-AuthSource: DS0PR12MB6390.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Oct 2023 13:40:25.8707
 (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: 17vKH4NM8NjGvchO7RJRxz97mU+MonCJWXoH0tyiA38rwL41pHVARk761QRV91J4vGOrDIYyDQqUESqpHG3IXA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB6639
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 pete.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 (pete.vger.email [0.0.0.0]); Mon, 16 Oct 2023 06:41:09 -0700 (PDT)

Hi Jonathan,

I added a response below.

On 10/9/23 09:44, Terry Bowman wrote:
> Hi Jonathan,
> 
> I added responses inline below.
> 
> On 10/2/23 09:46, Jonathan Cameron wrote:
>> On Wed, 27 Sep 2023 17:43:29 +0200
>> Robert Richter <rrichter@amd.com> wrote:
>>
>>> CXL error handling depends on AER.
>>>
>>> Introduce config option PCIEAER_CXL in preparation of the AER dport
>>> error handling. Also, introduce the stub function
>>> devm_cxl_setup_parent_dport() to setup dports.
>>>
>>> This is in preparation of follow on patches.
>>>
>>> Note the Kconfg part of the option is added in a later patch to enable
>>> it once coding of the feature is complete.
>>>
>>> Signed-off-by: Robert Richter <rrichter@amd.com>
>>
>> Feels like it should just be combined with a later patch that fills
>> some of this in as on it's own it's just a weird snippet of code :)
>>
> 
> We will look to merge with the following patch.
> 
>> Still, one comment inline anyway.
>>
>>
>>> ---
>>>  drivers/cxl/core/pci.c | 9 +++++++++
>>>  drivers/cxl/cxl.h      | 7 +++++++
>>>  drivers/cxl/mem.c      | 2 ++
>>>  3 files changed, 18 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>>> index c7a7887ebdcf..6ba3b7370816 100644
>>> --- a/drivers/cxl/core/pci.c
>>> +++ b/drivers/cxl/core/pci.c
>>> @@ -718,6 +718,15 @@ static bool cxl_report_and_clear(struct cxl_dev_state *cxlds)
>>>  	return true;
>>>  }
>>>  
>>> +#ifdef CONFIG_PCIEAER_CXL
>>> +
>>> +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
>>> +{
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_parent_dport, CXL);
>>> +
>>> +#endif
>>> +
>>>  pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>>>  				    pci_channel_state_t state)
>>>  {
>>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>>> index c07064e0c136..cfa2f6bede41 100644
>>> --- a/drivers/cxl/cxl.h
>>> +++ b/drivers/cxl/cxl.h
>>> @@ -704,6 +704,13 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>>>  					 struct device *dport_dev, int port_id,
>>>  					 resource_size_t rcrb);
>>>  
>>> +#ifdef CONFIG_PCIEAER_CXL
>>> +void devm_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
>>> +#else
>>> +static inline void devm_cxl_setup_parent_dport(struct device *host,
>>> +					       struct cxl_dport *dport) { }
>>> +#endif
>>> +
>>>  struct cxl_decoder *to_cxl_decoder(struct device *dev);
>>>  struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
>>>  struct cxl_switch_decoder *to_cxl_switch_decoder(struct device *dev);
>>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>>> index 04107058739b..61ca21c020fa 100644
>>> --- a/drivers/cxl/mem.c
>>> +++ b/drivers/cxl/mem.c
>>> @@ -157,6 +157,8 @@ static int cxl_mem_probe(struct device *dev)
>>>  	else
>>>  		endpoint_parent = &parent_port->dev;
>>>  
>>> +	devm_cxl_setup_parent_dport(dev, dport);
>>
>> devm calls can always fail (because if nothing else you have to register
>> some cleanup and that involves an allocation.  If you want to ignore
>> that I'd expect a comment here.
>>
> 
> We will add error handling here.
> 
> Regards,
> Terry
>

Found devm_cxl_setup_parent_dport() is a NULL function without return value.
 
Regards,
Terry