linux-kernel - Re: [PATCH v3 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <elqkwbuht6jimwdqtc26casnvkpoofrni5jsdg3vy2fjsz7h5f@zcnl42wj3g5q>
Date: Fri, 5 Sep 2025 20:20:22 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Vivek.Pernamitta@...cinc.com
Cc: mhi@...ts.linux.dev, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Vivek Pernamitta <quic_vpernami@...cinc.com>, 
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Subject: Re: [PATCH v3 2/6] bus: mhi: host: pci_generic: Read
 SUBSYSTEM_VENDOR_ID for VF's to check status

On Thu, Aug 21, 2025 at 06:25:34PM GMT, Vivek.Pernamitta@...cinc.com wrote:
> From: Vivek Pernamitta <quic_vpernami@...cinc.com>
> 
> - In SR-IOV enabled devices, the VF DEVICE/VENDOR ID register returns
>   FFFFh when read (as per PCIe SR-IOV spec section 3.4.1.1).
> - To determine device activity, read the PCIe VENDOR_ID of the Physical
>   Function (PF) instead.
> - Disabled health check monitoring for Virtual Functions (VFs), as they
>   are not physical functions and lack direct hardware control.
> - This avoids unnecessary CPU cycles consumed by VF health checks, which
>   are unintended and non-functional.
> 

Reword the description in imperative mood and try to come up with a paragraph
with continuity instead of points.

> 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 | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 2967ed4cec29d1c1eeb581ed44cf9afcea90f533..df18627a9feb4f2d53ba1ca6a4e8087dc23c7873 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -1094,8 +1094,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))
> -		return false;
> +	pci_read_config_word(pci_physfn(pdev), PCI_VENDOR_ID, &vendor);

You removed the return value check. Was that intentional?

>  
>  	if (vendor == (u16) ~0 || vendor == 0)
>  		return false;
> @@ -1205,7 +1204,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);

I really hate these if () checks everywhere. So I came up with the below macro:

	#define mhi_timer(pdev, timer_func, ...) \
		do { if (pdev->is_physfn) timer_func(__VA_ARGS__); } while(0)

	mhi_timer(pdev, timer_delete, &mhi_pdev->health_check_timer);

But mhi_timer() provides no information on what the macro is supposed to do and
I cannot think of a better name.

So maybe we have to live with the if () checks.

- Mani

-- 
மணிவண்ணன் சதாசிவம்