Re: [PATCH] cxl: Wrap iterations over afu slices inside 'afu_list_lock'

[Vaibhav Jain]

Thanks for reviewing this patch Fred,

Frederic Barrat  writes:

>>   
>>  afu_result = cxl_vphb_error_detected(afu, state);
>>   
>> -cxl_context_detach_all(afu);
>> -cxl_ops->afu_deactivate_mode(afu, afu->current_mode);
>> -pci_deconfigure_afu(afu);
>> +if (afu != NULL) {
>> +cxl_context_detach_all(afu);
>> +cxl_ops->afu_deactivate_mode(afu, afu->current_mode);
>> +pci_deconfigure_afu(afu);
>> +}
>
> I can see you're also checking if cxl_vphb_error_detected() is called 
> with a NULL afu from within the function, but why not move the call to 
> cxl_vphb_error_detected() within that "if (afu != NULL)...  " statement? 
> Otherwise, it looks suspicious when reading the code.
Yes, agree. However this was triggerring gcc compile warning
'maybe-uninitialized' for 'afu_result', hence removed the call to
cxl_vphb_error_detected() outside the branch. Have fixed this in v2 with
an explicit initialization of 'afu_result'.

>
>
>> @@ -2051,10 +2067,11 @@ static void cxl_pci_resume(struct pci_dev *pdev)
>>   * This is not the place to be checking if everything came back up
>>   * properly, because there's no return value: do that in slot_reset.
>>   */
>> +spin_lock(>afu_list_lock);
>>  for (i = 0; i < adapter->slices; i++) {
>>  afu = adapter->afu[i];
>>   
>> -if (afu->phb == NULL)
>> +if (afu || afu->phb == NULL)
>>  continue;
>
>
> if (afu == NULL ...
Thanks for catching this. Have fixed this in v2.
-- 
Vaibhav Jain 
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH] cxl: Wrap iterations over afu slices inside 'afu_list_lock'

[Frederic Barrat]

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index c79ba1c699ad..28c28bceb063 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c



@@ -1932,14 +1935,20 @@ static pci_ers_result_t cxl_pci_error_detected(struct 
pci_dev *pdev,
 * * In slot_reset, free the old resources and allocate new ones.
 * * In resume, clear the flag to allow things to start.
 */
+
+   /* Make sure no one else changes the afu list */
+   spin_lock(>afu_list_lock);
+
for (i = 0; i < adapter->slices; i++) {
afu = adapter->afu[i];
  
  		afu_result = cxl_vphb_error_detected(afu, state);
  
-		cxl_context_detach_all(afu);

-   cxl_ops->afu_deactivate_mode(afu, afu->current_mode);
-   pci_deconfigure_afu(afu);
+   if (afu != NULL) {
+   cxl_context_detach_all(afu);
+   cxl_ops->afu_deactivate_mode(afu, afu->current_mode);
+   pci_deconfigure_afu(afu);
+   }


I can see you're also checking if cxl_vphb_error_detected() is called 
with a NULL afu from within the function, but why not move the call to 
cxl_vphb_error_detected() within that "if (afu != NULL)...  " statement? 
Otherwise, it looks suspicious when reading the code.




@@ -2051,10 +2067,11 @@ static void cxl_pci_resume(struct pci_dev *pdev)
 * This is not the place to be checking if everything came back up
 * properly, because there's no return value: do that in slot_reset.
 */
+   spin_lock(>afu_list_lock);
for (i = 0; i < adapter->slices; i++) {
afu = adapter->afu[i];
  
-		if (afu->phb == NULL)

+   if (afu || afu->phb == NULL)
continue;



if (afu == NULL ...

  Fred