linux-kernel - Re: [PATCH v4 2/6] bus: mhi: host: pci_generic: Read SUBSYSTEM_VENDOR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0e18613c-f78f-42df-8c10-8fb1c7b5ff8d@oss.qualcomm.com>
Date: Mon, 15 Sep 2025 16:47:41 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: Vivek Pernamitta <quic_vpernami@...cinc.com>, Vivek.Pernamitta@...cinc.com,
        Manivannan Sadhasivam <mani@...nel.org>
Cc: mhi@...ts.linux.dev, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/6] bus: mhi: host: pci_generic: Read
 SUBSYSTEM_VENDOR_ID for VF's to check status



On 9/15/2025 4:19 PM, Vivek Pernamitta wrote:
> 
> 
> On 9/15/2025 2:22 PM, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 9/12/2025 6:18 PM, Vivek.Pernamitta@...cinc.com wrote:
>>> From: Vivek Pernamitta <quic_vpernami@...cinc.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@...cinc.com>
>>> Reviewed-by: Krishna Chaitanya Chundru 
>>> <krishna.chundru@....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;
can you include that in the commit text.

- Krishna Chaitanya.
>>>       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);
>>>
>