Re: [PATCH v10 05/12] cxl: Cache endpoint decoder settings during PCI enumeration

[Srirangan Madhavan <smadhavan@nvidia.com>]

On 8/26/26 11:30 AM, Lucero Palau, Alejandro wrote:
>> +             up_read(&cxl_rwsem.dpa);
>> +             return 0;
>> +     }
> 
> 
> Why to release the lock here? or why is this needed at all?
> 
> 
> If I understand this correctly, this is happening before the cxl core
> does any sort of initialization, indeed, the potential pdev driver can
> not be probed either, that seems to be the reason here, to get the HDM
> data if committed for doing the reset before the driver binds. Who could
> be setting pdev->hdm?
> 
> 
> I can not see the necessity and if I'm right, you can avoid the
> pdev->hdm check as well at the end and avoid the call for releasing
> info. Another comment below.
> 

Hi Alejandro,

Yes, that is the intent: capture committed HDM state during PCI 
enumeration so it is available to reset paths before a driver binds.

pdev->hdm may already have been populated by the call from 
pci_bus_add_device() when the fallback call from CXL core enumeration 
runs. The initial check makes that later call a no-op. Since the lock is 
released during allocation and MMIO access, the final check protects the 
check-then-publish window if the two
call sites overlap.

> 
>> +     up_read(&cxl_rwsem.dpa);
>> +
>> +     rc = cxl_pci_hdm_capable(pdev);
>> +     if (rc)
>> +             return rc;
>> +
>> +     rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
>> +     if (rc)
>> +             return rc;
snip
>> +                             rc = rc2;
>> +             }
>> +     }
>> +
>> +     if (rc)
>> +             goto out_free_info;
>> +
>> +     down_write(&cxl_rwsem.dpa);
>> +     if (!pdev->hdm) {
>> +             pdev->hdm = info;
>> +             info = NULL;
>> +     }
>> +     up_write(&cxl_rwsem.dpa);
>> +
>> +     cxl_pci_hdm_info_free(info);
> 
> 
> This seems weird if info is being assigned above to the pdev. Maybe it
> would be clearer if done in an else branch instead:
> 
>         down_write(&cxl_rwsem.dpa);
>         if (!pdev->hdm) {
>                 pdev->hdm = info;
>                 info = NULL;
>         } else {
>                 cxl_pci_hdm_info_free(info);
>         }
>         up_write(&cxl_rwsem.dpa);
> 
> Thanks,
> 
> Alejandro.

In v11, I converted both lock regions to scoped guards and followed your 
suggestion to free the newly allocated duplicate explicitly in the else 
branch.

-- 
Regards,
Srirangan