Re: [PATCH v4 2/6] bus: mhi: host: pci_generic: Read SUBSYSTEM_VENDOR_ID for VF's to check status

[Vivek Pernamitta <quic_vpernami@quicinc.com>]

On 9/15/2025 2:22 PM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 9/12/2025 6:18 PM, Vivek.Pernamitta@quicinc.com wrote:
>> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>
>> In SR-IOV enabled devices, reading the VF DEVICE/VENDOR ID register
>> returns `FFFFh`, as specified in section 3.4.1.1 of the PCIe SR-IOV spec.
>> To accurately determine device activity, read the PCIe VENDOR_ID of
>> the Physical Function (PF) insteadcommit text and subject needs to be 
>> modified to reflect new changes
> 
>> Health check monitoring for Virtual Functions (VFs) has been disabled,
>> since VFs are not physical functions and lack direct hardware control.
>> This change prevents unnecessary CPU cycles from being consumed by VF
>> health checks, which are both unintended and non-functional.
>>
>> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
>> Reviewed-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   drivers/bus/mhi/host/pci_generic.c | 34 +++++++++++++++++++++++ 
>> +----------
>>   1 file changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/ 
>> host/pci_generic.c
>> index 
>> 8a605cb3b8e1e54ef4e699700f3f2660ad5cb093..6fa16975e320212a50e0b68ddb34db5ce711589c 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -1082,7 +1082,7 @@ static bool mhi_pci_is_alive(struct 
>> mhi_controller *mhi_cntrl)
>>       struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev);
>>       u16 vendor = 0;
>> -    if (pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor))
>> +    if (pci_read_config_word(pci_physfn(pdev), PCI_VENDOR_ID, &vendor))
> As you are invoking only for physical functions pci_physfn is not needed.
> 
> - Krishna Chaitanya
pci_physfn(pdev) was intentionally kept as this can be called for VF's
in error handle path. >>           return false;
>>       if (vendor == (u16) ~0 || vendor == 0)
>> @@ -1193,7 +1193,9 @@ static void mhi_pci_recovery_work(struct 
>> work_struct *work)
>>       dev_warn(&pdev->dev, "device recovery started\n");
>> -    timer_delete(&mhi_pdev->health_check_timer);
>> +    if (pdev->is_physfn)
>> +        timer_delete(&mhi_pdev->health_check_timer);
>> +
>>       pm_runtime_forbid(&pdev->dev);
>>       /* Clean up MHI state */
>> @@ -1220,7 +1222,10 @@ static void mhi_pci_recovery_work(struct 
>> work_struct *work)
>>       dev_dbg(&pdev->dev, "Recovery completed\n");
>>       set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>> -    mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>> +
>> +    if (pdev->is_physfn)
>> +        mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>> +
>>       return;
>>   err_unprepare:
>> @@ -1307,7 +1312,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>       else
>>           mhi_cntrl_config = info->config;
>> -    timer_setup(&mhi_pdev->health_check_timer, health_check, 0);
>> +    /* Initialize health check monitor only for Physical functions */
>> +    if (pdev->is_physfn)
>> +        timer_setup(&mhi_pdev->health_check_timer, health_check, 0);
>>       mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> @@ -1371,7 +1378,8 @@ static int mhi_pci_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>       set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>>       /* start health check */
>> -    mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>> +    if (pdev->is_physfn)
>> +        mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>>       /* Allow runtime suspend only if both PME from D3Hot and M3 are 
>> supported */
>>       if (pci_pme_capable(pdev, PCI_D3hot) && !(info->no_m3)) {
>> @@ -1396,7 +1404,8 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>>       struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>>       struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> -    timer_delete_sync(&mhi_pdev->health_check_timer);
>> +    if (pdev->is_physfn)
>> +        timer_delete_sync(&mhi_pdev->health_check_timer);
>>       cancel_work_sync(&mhi_pdev->recovery_work);
>>       if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>> @@ -1424,7 +1433,8 @@ static void mhi_pci_reset_prepare(struct pci_dev 
>> *pdev)
>>       dev_info(&pdev->dev, "reset\n");
>> -    timer_delete(&mhi_pdev->health_check_timer);
>> +    if (pdev->is_physfn)
>> +        timer_delete(&mhi_pdev->health_check_timer);
>>       /* Clean up MHI state */
>>       if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
>> @@ -1469,7 +1479,8 @@ static void mhi_pci_reset_done(struct pci_dev 
>> *pdev)
>>       }
>>       set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
>> -    mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>> +    if (pdev->is_physfn)
>> +        mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>>   }
>>   static pci_ers_result_t mhi_pci_error_detected(struct pci_dev *pdev,
>> @@ -1534,7 +1545,9 @@ static int  __maybe_unused 
>> mhi_pci_runtime_suspend(struct device *dev)
>>       if (test_and_set_bit(MHI_PCI_DEV_SUSPENDED, &mhi_pdev->status))
>>           return 0;
>> -    timer_delete(&mhi_pdev->health_check_timer);
>> +    if (pdev->is_physfn)
>> +        timer_delete(&mhi_pdev->health_check_timer);
>> +
>>       cancel_work_sync(&mhi_pdev->recovery_work);
>>       if (!test_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status) ||
>> @@ -1585,7 +1598,8 @@ static int __maybe_unused 
>> mhi_pci_runtime_resume(struct device *dev)
>>       }
>>       /* Resume health check */
>> -    mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>> +    if (pdev->is_physfn)
>> +        mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>>       /* It can be a remote wakeup (no mhi runtime_get), update access 
>> time */
>>       pm_runtime_mark_last_busy(dev);
>>