[PATCH v2 0/5] bus: mhi: host: Enable SRIOV support in MHI driver

[PATCH v2 0/5] bus: mhi: host: Enable SRIOV support in MHI driver

[Vivek.Pernamitta]

This patch introduces several enhancements for the SRIOV support in MHI driver
focusing on enabling SRIOV and improving the MHI driver removal process.

- Add support to enable SRIOV.

- Add support to read SUBSYSTEM_VENDOR_ID for VF's to check status.

- Implement support for separate controller configurations for both
  Virtual Functions (VF) and Physical Functions (PF).
  The PF takes on a supervisory role and will have bootup information
  such as SAHARA, DIAG, and NDB (for file system sync data, etc.).
  VFs can handle function-specific data transfers, such as data plane
  or hardware data.

- Perform a graceful removal of the MHI driver. Upon driver removal,
  the host driver will perform a SOC_RESET on the driver remove callback.
  This ensures device reset gracefully.
  
- honor sys_err at power_up state
  In mhi_sync_power_up() host waits for device to enter in to mission mode
  but SYS_ERR is an valid state, If device sends an SYS_ERR host will bail
  out for wait_event_timeout() as MHI is in error state and calls
  mhi_power_down which will teardown MHI driver probe.
  If there is any SYS_ERR, sys_err handler needs to process SYS_ERR state
  and queues the next state transition for device to bring in to Mission
  mode, so mhi_sync_power_up() will wait for device to enter in to
  mission mode.

Signed-off-by: 

---
Changes in v2:
- Changed order of patchsets from V1 as per Konrad comments.
- Added spec version for separate controller configurations for both VF andi
  PF as per Krishna comments.
- Updated git commit message as per Krishna comments..
- Added mhi_pci_remove in shutdown callback in pci_generic instead of
  duplicating the same sequence in both as per Krishna comments..
- Link to v1: https://lore.kernel.org/r/20250703-sriov_vdev_next-20250630-v1-0-87071d1047e3@quicinc.com
---

---
Vivek Pernamitta (5):
      bus: mhi: host: Add support for separate controller configurations for VF and PF
      bus: mhi: host: pci_generic: Read SUBSYSTEM_VENDOR_ID for VF's to check status
      bus: mhi: host: pci_generic: Remove MHI driver and ensure graceful device recovery
      bus: mhi: host: pci_generic: Add SRIOV support for PCIe device
      bus: host: mhi: Need to honor sys_err at power_up state

 drivers/bus/mhi/host/internal.h    |  2 ++
 drivers/bus/mhi/host/pci_generic.c | 44 +++++++++++++++++++++++++++++++++-----
 drivers/bus/mhi/host/pm.c          |  2 +-
 3 files changed, 42 insertions(+), 6 deletions(-)
---
base-commit: 1343433ed38923a21425c602e92120a1f1db5f7a
change-id: 20250701-sriov_vdev_next-20250630-0ee75f15d03b

Best regards,
-- 
Vivek Pernamitta <<quic_vpernami@quicinc.com>>

[PATCH v2 1/5] bus: mhi: host: Add support for separate controller configurations for VF and PF

[Vivek.Pernamitta]

From: Vivek Pernamitta <quic_vpernami@quicinc.com>

Implement support for separate controller configurations for both
Virtual Functions (VF) and Physical Functions (PF).

This enhancement allows for more flexible and efficient management of
resources. The PF takes on a supervisory role and will have bootup
information such as SAHARA, DIAG, and NDB (for file system sync data,
etc.). VFs can handle resources associated with the main data movement
of the Function are available to the SI (system image) as per PCIe SRIOV
spec (rev 0.9 1.Architectural overview)

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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 5c01c23d0bcfedd23f975e99845d5fa88940ccde..7d0ac1c34ddf95ace2a85e5f08884f51604d9b0f 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -34,6 +34,7 @@
 /**
  * struct mhi_pci_dev_info - MHI PCI device specific information
  * @config: MHI controller configuration
+ * @vf_config: MHI controller configuration for Virtual function (optional)
  * @name: name of the PCI module
  * @fw: firmware path (if any)
  * @edl: emergency download mode firmware path (if any)
@@ -47,6 +48,7 @@
  */
 struct mhi_pci_dev_info {
 	const struct mhi_controller_config *config;
+	const struct mhi_controller_config *vf_config;
 	const char *name;
 	const char *fw;
 	const char *edl;
@@ -1242,9 +1244,14 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return -ENOMEM;
 
 	INIT_WORK(&mhi_pdev->recovery_work, mhi_pci_recovery_work);
+
+	if (pdev->is_virtfn && info->vf_config)
+		mhi_cntrl_config = info->vf_config;
+	else
+		mhi_cntrl_config = info->config;
+
 	timer_setup(&mhi_pdev->health_check_timer, health_check, 0);
 
-	mhi_cntrl_config = info->config;
 	mhi_cntrl = &mhi_pdev->mhi_cntrl;
 
 	mhi_cntrl->cntrl_dev = &pdev->dev;

-- 
2.34.1

[PATCH v2 2/5] bus: mhi: host: pci_generic: Read SUBSYSTEM_VENDOR_ID for VF's to check status

[Vivek.Pernamitta]

From: Vivek Pernamitta <quic_vpernami@quicinc.com>

In SRIOV enabled devices, the VF DEVICE/VENDOR ID register returns FFFFh
when read (PCIe SRIOV spec-3.4.1.1). Therefore, read the PCIe
SUBSYSTEM_VENDOR_ID to check if the device is active.

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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 7d0ac1c34ddf95ace2a85e5f08884f51604d9b0f..4bafe93b56c54e2b091786e7fcd68a36c8247b8e 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -1025,8 +1025,10 @@ 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;
+	if (pdev->is_virtfn)
+		pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &vendor);
+	else
+		pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor);
 
 	if (vendor == (u16) ~0 || vendor == 0)
 		return false;

-- 
2.34.1

[PATCH v2 3/5] bus: mhi: host: pci_generic: Remove MHI driver and ensure graceful device recovery

[Vivek.Pernamitta]

From: Vivek Pernamitta <quic_vpernami@quicinc.com>

When the MHI driver is removed from the host side, it is crucial to ensure
graceful recovery of the device. To achieve this, the host driver will
perform the following steps:

1. Disable SRIOV for any SRIOV-enabled devices on the Physical Function.
2. Perform a SOC_RESET on Physical Function (PF).

Disabling SRIOV ensures that all virtual functions are properly shut down,
preventing any potential issues during the reset process. Performing
SOC_RESET on each physical function guarantees that the device is fully
reset and ready for subsequent operations.

Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
---
 drivers/bus/mhi/host/pci_generic.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 4bafe93b56c54e2b091786e7fcd68a36c8247b8e..2d1381006293412fbc593316e5c7f0f59ac74da8 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -45,6 +45,8 @@
  * @sideband_wake: Devices using dedicated sideband GPIO for wakeup instead
  *		   of inband wake support (such as sdx24)
  * @no_m3: M3 not supported
+ * @reset_on_driver_unbind: Set true for devices support SOC reset and
+ *				 perform it when unbinding driver
  */
 struct mhi_pci_dev_info {
 	const struct mhi_controller_config *config;
@@ -58,6 +60,7 @@ struct mhi_pci_dev_info {
 	unsigned int mru_default;
 	bool sideband_wake;
 	bool no_m3;
+	bool reset_on_driver_unbind;
 };
 
 #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
@@ -300,6 +303,7 @@ static const struct mhi_pci_dev_info mhi_qcom_qdu100_info = {
 	.dma_data_width = 32,
 	.sideband_wake = false,
 	.no_m3 = true,
+	.reset_on_driver_unbind = true,
 };
 
 static const struct mhi_channel_config mhi_qcom_sa8775p_channels[] = {
@@ -970,6 +974,7 @@ struct mhi_pci_device {
 	struct work_struct recovery_work;
 	struct timer_list health_check_timer;
 	unsigned long status;
+	bool reset_on_driver_unbind;
 };
 
 static int mhi_pci_read_reg(struct mhi_controller *mhi_cntrl,
@@ -1270,6 +1275,11 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhi_cntrl->mru = info->mru_default;
 	mhi_cntrl->name = info->name;
 
+	/* Assign reset functionalities only for PF */
+	if (pdev->is_physfn)
+		mhi_pdev->reset_on_driver_unbind = info->reset_on_driver_unbind;
+
+
 	if (info->edl_trigger)
 		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
 
@@ -1336,7 +1346,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return err;
 }
 
-static void mhi_pci_remove(struct pci_dev *pdev)
+static void mhi_pci_resource_deinit(struct pci_dev *pdev)
 {
 	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
 	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
@@ -1352,6 +1362,20 @@ static void mhi_pci_remove(struct pci_dev *pdev)
 	/* balancing probe put_noidle */
 	if (pci_pme_capable(pdev, PCI_D3hot))
 		pm_runtime_get_noresume(&pdev->dev);
+}
+
+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;
+
+	/* Disable SRIOV */
+	pci_disable_sriov(pdev);
+	mhi_pci_resource_deinit(pdev);
+	if (mhi_pdev->reset_on_driver_unbind) {
+		dev_info(&pdev->dev, "perform SOC reset\n");
+		mhi_soc_reset(mhi_cntrl);
+	}
 
 	mhi_unregister_controller(mhi_cntrl);
 }

-- 
2.34.1

[PATCH v2 4/5] bus: mhi: host: pci_generic: Add SRIOV support for PCIe device

[Vivek.Pernamitta]

From: Vivek Pernamitta <quic_vpernami@quicinc.com>

Add SR-IOV support for PCIe devices.
pci_sriov_configure_simple() will enable or disable SR-IOV for devices
that don't require any specific PF setup before enabling SR-IOV.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 2d1381006293412fbc593316e5c7f0f59ac74da8..a64b5c365c920ef2edfebc994e82d6385ad7ddbd 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -1640,7 +1640,8 @@ static struct pci_driver mhi_pci_driver = {
 	.remove		= mhi_pci_remove,
 	.shutdown	= mhi_pci_shutdown,
 	.err_handler	= &mhi_pci_err_handler,
-	.driver.pm	= &mhi_pci_pm_ops
+	.driver.pm	= &mhi_pci_pm_ops,
+	.sriov_configure = pci_sriov_configure_simple
 };
 module_pci_driver(mhi_pci_driver);
 

-- 
2.34.1

[PATCH v2 5/5] bus: host: mhi: Need to honor sys_err at power_up state

[Vivek.Pernamitta]

From: Vivek Pernamitta <quic_vpernami@quicinc.com>

In mhi_sync_power_up() host waits for device to enter in to mission mode
but SYS_ERR is an valid state, If device sends an SYS_ERR host will bail
out for wait_event_timeout() as MHI is in error state and if MHI is tear
downed sys err cant't be serviced and mhi can't be recovered.

If there is any SYS_ERR, sys_err handler needs to process SYS_ERR state
and queues the next state transition for device to bring in to Mission
mode, so mhi_sync_power_up() needs to wait for device to enter in to
mission mode.

Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
---
 drivers/bus/mhi/host/internal.h | 2 ++
 drivers/bus/mhi/host/pm.c       | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index 1054e67bb450d2634771d092ed42bbdd63380472..1aec3bb68f9712f5476b0fc3efd8b2efc4d745dc 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -170,6 +170,8 @@ enum mhi_pm_state {
 							MHI_PM_IN_ERROR_STATE(pm_state))
 #define MHI_PM_IN_SUSPEND_STATE(pm_state)		(pm_state & \
 							(MHI_PM_M3_ENTER | MHI_PM_M3))
+#define MHI_PM_IN_UNRECOVERABLE_ERROR(pm_state)		((pm_state == MHI_PM_FW_DL_ERR) || \
+							(pm_state >= MHI_PM_SYS_ERR_FAIL))
 
 #define NR_OF_CMD_RINGS					1
 #define CMD_EL_PER_RING					128
diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
index 2af34980e14250cada75c981b690bc9581715212..fc9713d4021571aebd995a4524eafbcf0128fbd1 100644
--- a/drivers/bus/mhi/host/pm.c
+++ b/drivers/bus/mhi/host/pm.c
@@ -1280,7 +1280,7 @@ int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
 		mhi_cntrl->ready_timeout_ms : mhi_cntrl->timeout_ms;
 	wait_event_timeout(mhi_cntrl->state_event,
 			   MHI_IN_MISSION_MODE(mhi_cntrl->ee) ||
-			   MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
+			   MHI_PM_IN_UNRECOVERABLE_ERROR(mhi_cntrl->pm_state),
 			   msecs_to_jiffies(timeout_ms));
 
 	ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;

-- 
2.34.1

Re: [PATCH v2 3/5] bus: mhi: host: pci_generic: Remove MHI driver and ensure graceful device recovery

[Konrad Dybcio]

On 7/10/25 10:58 AM, Vivek.Pernamitta@quicinc.com wrote:
> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> 
> When the MHI driver is removed from the host side, it is crucial to ensure
> graceful recovery of the device. To achieve this, the host driver will
> perform the following steps:
> 
> 1. Disable SRIOV for any SRIOV-enabled devices on the Physical Function.
> 2. Perform a SOC_RESET on Physical Function (PF).
> 
> Disabling SRIOV ensures that all virtual functions are properly shut down,
> preventing any potential issues during the reset process. Performing
> SOC_RESET on each physical function guarantees that the device is fully
> reset and ready for subsequent operations.
> 
> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
> ---
>  drivers/bus/mhi/host/pci_generic.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 4bafe93b56c54e2b091786e7fcd68a36c8247b8e..2d1381006293412fbc593316e5c7f0f59ac74da8 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -45,6 +45,8 @@
>   * @sideband_wake: Devices using dedicated sideband GPIO for wakeup instead
>   *		   of inband wake support (such as sdx24)
>   * @no_m3: M3 not supported
> + * @reset_on_driver_unbind: Set true for devices support SOC reset and
> + *				 perform it when unbinding driver
>   */
>  struct mhi_pci_dev_info {
>  	const struct mhi_controller_config *config;
> @@ -58,6 +60,7 @@ struct mhi_pci_dev_info {
>  	unsigned int mru_default;
>  	bool sideband_wake;
>  	bool no_m3;
> +	bool reset_on_driver_unbind;
>  };
>  
>  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> @@ -300,6 +303,7 @@ static const struct mhi_pci_dev_info mhi_qcom_qdu100_info = {
>  	.dma_data_width = 32,
>  	.sideband_wake = false,
>  	.no_m3 = true,
> +	.reset_on_driver_unbind = true,

It seems rather unlikely that out off all MHI devices, only QDU100
needs this quirk when working under SR-IOV

>  };
>  
>  static const struct mhi_channel_config mhi_qcom_sa8775p_channels[] = {
> @@ -970,6 +974,7 @@ struct mhi_pci_device {
>  	struct work_struct recovery_work;
>  	struct timer_list health_check_timer;
>  	unsigned long status;
> +	bool reset_on_driver_unbind;
>  };
>  
>  static int mhi_pci_read_reg(struct mhi_controller *mhi_cntrl,
> @@ -1270,6 +1275,11 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	mhi_cntrl->mru = info->mru_default;
>  	mhi_cntrl->name = info->name;
>  
> +	/* Assign reset functionalities only for PF */
> +	if (pdev->is_physfn)
> +		mhi_pdev->reset_on_driver_unbind = info->reset_on_driver_unbind;
> +
> +

Double \n

>  	if (info->edl_trigger)
>  		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>  
> @@ -1336,7 +1346,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return err;
>  }
>  
> -static void mhi_pci_remove(struct pci_dev *pdev)
> +static void mhi_pci_resource_deinit(struct pci_dev *pdev)
>  {
>  	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>  	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> @@ -1352,6 +1362,20 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>  	/* balancing probe put_noidle */
>  	if (pci_pme_capable(pdev, PCI_D3hot))
>  		pm_runtime_get_noresume(&pdev->dev);
> +}
> +
> +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;
> +
> +	/* Disable SRIOV */
> +	pci_disable_sriov(pdev);
> +	mhi_pci_resource_deinit(pdev);
> +	if (mhi_pdev->reset_on_driver_unbind) {

Can we not simply check for pdev->is_physfn?

> +		dev_info(&pdev->dev, "perform SOC reset\n");

Is the logspam really necessary?

> +		mhi_soc_reset(mhi_cntrl);
> +	}

Konrad

Re: [PATCH v2 4/5] bus: mhi: host: pci_generic: Add SRIOV support for PCIe device

[Konrad Dybcio]

On 7/10/25 10:58 AM, Vivek.Pernamitta@quicinc.com wrote:
> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> 
> Add SR-IOV support for PCIe devices.
> pci_sriov_configure_simple() will enable or disable SR-IOV for devices
> that don't require any specific PF setup before enabling SR-IOV.
> 
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 2d1381006293412fbc593316e5c7f0f59ac74da8..a64b5c365c920ef2edfebc994e82d6385ad7ddbd 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -1640,7 +1640,8 @@ static struct pci_driver mhi_pci_driver = {
>  	.remove		= mhi_pci_remove,
>  	.shutdown	= mhi_pci_shutdown,
>  	.err_handler	= &mhi_pci_err_handler,
> -	.driver.pm	= &mhi_pci_pm_ops
> +	.driver.pm	= &mhi_pci_pm_ops,
> +	.sriov_configure = pci_sriov_configure_simple

If you add a comma at the end of the newly-added line,
future diffs will be shorter

Konrad

Re: [PATCH v2 5/5] bus: host: mhi: Need to honor sys_err at power_up state

[Konrad Dybcio]

On 7/10/25 10:58 AM, Vivek.Pernamitta@quicinc.com wrote:
> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> 
> In mhi_sync_power_up() host waits for device to enter in to mission mode
> but SYS_ERR is an valid state, If device sends an SYS_ERR host will bail
> out for wait_event_timeout() as MHI is in error state and if MHI is tear
> downed sys err cant't be serviced and mhi can't be recovered.
> 
> If there is any SYS_ERR, sys_err handler needs to process SYS_ERR state
> and queues the next state transition for device to bring in to Mission
> mode, so mhi_sync_power_up() needs to wait for device to enter in to
> mission mode.

This is very difficult to read, please rephrase the commit message

Konrad

Re: [PATCH v2 3/5] bus: mhi: host: pci_generic: Remove MHI driver and ensure graceful device recovery

[Vivek Pernamitta]

On 7/10/2025 6:12 PM, Konrad Dybcio wrote:
> On 7/10/25 10:58 AM, Vivek.Pernamitta@quicinc.com wrote:
>> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>
>> When the MHI driver is removed from the host side, it is crucial to ensure
>> graceful recovery of the device. To achieve this, the host driver will
>> perform the following steps:
>>
>> 1. Disable SRIOV for any SRIOV-enabled devices on the Physical Function.
>> 2. Perform a SOC_RESET on Physical Function (PF).
>>
>> Disabling SRIOV ensures that all virtual functions are properly shut down,
>> preventing any potential issues during the reset process. Performing
>> SOC_RESET on each physical function guarantees that the device is fully
>> reset and ready for subsequent operations.
>>
>> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/pci_generic.c | 26 +++++++++++++++++++++++++-
>>   1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>> index 4bafe93b56c54e2b091786e7fcd68a36c8247b8e..2d1381006293412fbc593316e5c7f0f59ac74da8 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -45,6 +45,8 @@
>>    * @sideband_wake: Devices using dedicated sideband GPIO for wakeup instead
>>    *		   of inband wake support (such as sdx24)
>>    * @no_m3: M3 not supported
>> + * @reset_on_driver_unbind: Set true for devices support SOC reset and
>> + *				 perform it when unbinding driver
>>    */
>>   struct mhi_pci_dev_info {
>>   	const struct mhi_controller_config *config;
>> @@ -58,6 +60,7 @@ struct mhi_pci_dev_info {
>>   	unsigned int mru_default;
>>   	bool sideband_wake;
>>   	bool no_m3;
>> +	bool reset_on_driver_unbind;
>>   };
>>   
>>   #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
>> @@ -300,6 +303,7 @@ static const struct mhi_pci_dev_info mhi_qcom_qdu100_info = {
>>   	.dma_data_width = 32,
>>   	.sideband_wake = false,
>>   	.no_m3 = true,
>> +	.reset_on_driver_unbind = true,
> 
> It seems rather unlikely that out off all MHI devices, only QDU100
> needs this quirk when working under SR-IOV
The reset_on_driver_unbind flag has been explicitly added for the QDU100
device. While other devices might enter RAMDUMP mode and wait when a SOC
reset is issued, the QDU100 device will pass through without entering
RAMDUMP mode
> 
>>   };
>>   
>>   static const struct mhi_channel_config mhi_qcom_sa8775p_channels[] = {
>> @@ -970,6 +974,7 @@ struct mhi_pci_device {
>>   	struct work_struct recovery_work;
>>   	struct timer_list health_check_timer;
>>   	unsigned long status;
>> +	bool reset_on_driver_unbind;
>>   };
>>   
>>   static int mhi_pci_read_reg(struct mhi_controller *mhi_cntrl,
>> @@ -1270,6 +1275,11 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   	mhi_cntrl->mru = info->mru_default;
>>   	mhi_cntrl->name = info->name;
>>   
>> +	/* Assign reset functionalities only for PF */
>> +	if (pdev->is_physfn)
>> +		mhi_pdev->reset_on_driver_unbind = info->reset_on_driver_unbind;
>> +
>> +
> 
> Double \n
sure will correct in next patchset
> 
>>   	if (info->edl_trigger)
>>   		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>>   
>> @@ -1336,7 +1346,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   	return err;
>>   }
>>   
>> -static void mhi_pci_remove(struct pci_dev *pdev)
>> +static void mhi_pci_resource_deinit(struct pci_dev *pdev)
>>   {
>>   	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>>   	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
>> @@ -1352,6 +1362,20 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>>   	/* balancing probe put_noidle */
>>   	if (pci_pme_capable(pdev, PCI_D3hot))
>>   		pm_runtime_get_noresume(&pdev->dev);
>> +}
>> +
>> +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;
>> +
>> +	/* Disable SRIOV */
>> +	pci_disable_sriov(pdev);
>> +	mhi_pci_resource_deinit(pdev);
>> +	if (mhi_pdev->reset_on_driver_unbind) {
> 
> Can we not simply check for pdev->is_physfn?
The reset_on_driver_unbind flag has been explicitly added for the QDU100 
device. While other devices might enter RAMDUMP mode and wait when a SOC 
reset is issued, the QDU100 device will pass through without entering 
RAMDUMP mode.
> 
>> +		dev_info(&pdev->dev, "perform SOC reset\n");
> 
> Is the logspam really necessary?
> 
>> +		mhi_soc_reset(mhi_cntrl);
>> +	}
> 
> Konrad

Re: [PATCH v2 3/5] bus: mhi: host: pci_generic: Remove MHI driver and ensure graceful device recovery

[Konrad Dybcio]

On 7/23/25 2:11 PM, Vivek Pernamitta wrote:
> 
> 
> On 7/10/2025 6:12 PM, Konrad Dybcio wrote:
>> On 7/10/25 10:58 AM, Vivek.Pernamitta@quicinc.com wrote:
>>> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>>
>>> When the MHI driver is removed from the host side, it is crucial to ensure
>>> graceful recovery of the device. To achieve this, the host driver will
>>> perform the following steps:
>>>
>>> 1. Disable SRIOV for any SRIOV-enabled devices on the Physical Function.
>>> 2. Perform a SOC_RESET on Physical Function (PF).
>>>
>>> Disabling SRIOV ensures that all virtual functions are properly shut down,
>>> preventing any potential issues during the reset process. Performing
>>> SOC_RESET on each physical function guarantees that the device is fully
>>> reset and ready for subsequent operations.
>>>
>>> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>> ---

[...]

>> It seems rather unlikely that out off all MHI devices, only QDU100
>> needs this quirk when working under SR-IOV
> The reset_on_driver_unbind flag has been explicitly added for the QDU100
> device. While other devices might enter RAMDUMP mode and wait when a SOC
> reset is issued, the QDU100 device will pass through without entering
> RAMDUMP mode

Rather inconveniently, this is not something that you mentioned in
the commit message.. Especially in the middle of a series that focuses
on enabling SR-IOV which suggests it's strictly related to virtualization,
making it difficult or impossible to understand the problem that this
patch is actually solving.

Please rewrite the commit message.

Konrad

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

[Manivannan Sadhasivam]

On Thu, Jul 10, 2025 at 02:28:33PM GMT, Vivek.Pernamitta@quicinc.com wrote:
> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> 
> In SRIOV enabled devices, the VF DEVICE/VENDOR ID register returns FFFFh
> when read (PCIe SRIOV spec-3.4.1.1). Therefore, read the PCIe
> SUBSYSTEM_VENDOR_ID to check if the device is active.
> 
> 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 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 7d0ac1c34ddf95ace2a85e5f08884f51604d9b0f..4bafe93b56c54e2b091786e7fcd68a36c8247b8e 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -1025,8 +1025,10 @@ 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;
> +	if (pdev->is_virtfn)
> +		pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &vendor);
> +	else
> +		pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor);

You should not read the sub Vendor ID for VF. PCIe spec suggests reading the PF
Vendor ID for VF. So you should just use pci_physfn() API as below:

	pci_read_config_word(pci_physfn(pdev), PCI_VENDOR_ID, &vendor);

This will work for both PF and VF.

- Mani

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

Re: [PATCH v2 3/5] bus: mhi: host: pci_generic: Remove MHI driver and ensure graceful device recovery

[Manivannan Sadhasivam]

On Thu, Jul 10, 2025 at 02:28:34PM GMT, Vivek.Pernamitta@quicinc.com wrote:
> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> 
> When the MHI driver is removed from the host side, it is crucial to ensure
> graceful recovery of the device. To achieve this, the host driver will
> perform the following steps:

Please rewrite the description in an imperative tone.

> 
> 1. Disable SRIOV for any SRIOV-enabled devices on the Physical Function.

You haven't enabled SR-IOV at this point, but only in the next patch. So you
need to swap patches 3 and 4.

> 2. Perform a SOC_RESET on Physical Function (PF).
> 
> Disabling SRIOV ensures that all virtual functions are properly shut down,
> preventing any potential issues during the reset process. Performing
> SOC_RESET on each physical function guarantees that the device is fully

Each physical function? How many PF does this device support? It should be
described somewhere on the total PF and VF it supports.

> reset and ready for subsequent operations.
> 
> Signed-off-by: Vivek Pernamitta <quic_vpernami@quicinc.com>
> ---
>  drivers/bus/mhi/host/pci_generic.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 4bafe93b56c54e2b091786e7fcd68a36c8247b8e..2d1381006293412fbc593316e5c7f0f59ac74da8 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -45,6 +45,8 @@
>   * @sideband_wake: Devices using dedicated sideband GPIO for wakeup instead
>   *		   of inband wake support (such as sdx24)
>   * @no_m3: M3 not supported
> + * @reset_on_driver_unbind: Set true for devices support SOC reset and
> + *				 perform it when unbinding driver

	reset_on_remove: Reset the device while removing the driver

>   */
>  struct mhi_pci_dev_info {
>  	const struct mhi_controller_config *config;
> @@ -58,6 +60,7 @@ struct mhi_pci_dev_info {
>  	unsigned int mru_default;
>  	bool sideband_wake;
>  	bool no_m3;
> +	bool reset_on_driver_unbind;
>  };
>  
>  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> @@ -300,6 +303,7 @@ static const struct mhi_pci_dev_info mhi_qcom_qdu100_info = {
>  	.dma_data_width = 32,
>  	.sideband_wake = false,
>  	.no_m3 = true,
> +	.reset_on_driver_unbind = true,
>  };
>  
>  static const struct mhi_channel_config mhi_qcom_sa8775p_channels[] = {
> @@ -970,6 +974,7 @@ struct mhi_pci_device {
>  	struct work_struct recovery_work;
>  	struct timer_list health_check_timer;
>  	unsigned long status;
> +	bool reset_on_driver_unbind;
>  };
>  
>  static int mhi_pci_read_reg(struct mhi_controller *mhi_cntrl,
> @@ -1270,6 +1275,11 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	mhi_cntrl->mru = info->mru_default;
>  	mhi_cntrl->name = info->name;
>  
> +	/* Assign reset functionalities only for PF */

Remove the comment.

> +	if (pdev->is_physfn)
> +		mhi_pdev->reset_on_driver_unbind = info->reset_on_driver_unbind;
> +
> +

Extra newline.

>  	if (info->edl_trigger)
>  		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>  
> @@ -1336,7 +1346,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return err;
>  }
>  
> -static void mhi_pci_remove(struct pci_dev *pdev)
> +static void mhi_pci_resource_deinit(struct pci_dev *pdev)

Please do not create unsymmetric functions. There is no mhi_pci_resource_init().

>  {
>  	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
>  	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
> @@ -1352,6 +1362,20 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>  	/* balancing probe put_noidle */
>  	if (pci_pme_capable(pdev, PCI_D3hot))
>  		pm_runtime_get_noresume(&pdev->dev);
> +}
> +
> +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;
> +
> +	/* Disable SRIOV */

This one too.

> +	pci_disable_sriov(pdev);
> +	mhi_pci_resource_deinit(pdev);
> +	if (mhi_pdev->reset_on_driver_unbind) {
> +		dev_info(&pdev->dev, "perform SOC reset\n");

This is just a spam, please drop.

- Mani

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

Re: [PATCH v2 5/5] bus: host: mhi: Need to honor sys_err at power_up state

[Manivannan Sadhasivam]

On Thu, Jul 10, 2025 at 02:46:51PM GMT, Konrad Dybcio wrote:
> On 7/10/25 10:58 AM, Vivek.Pernamitta@quicinc.com wrote:
> > From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> > 
> > In mhi_sync_power_up() host waits for device to enter in to mission mode
> > but SYS_ERR is an valid state, If device sends an SYS_ERR host will bail
> > out for wait_event_timeout() as MHI is in error state and if MHI is tear
> > downed sys err cant't be serviced and mhi can't be recovered.
> > 
> > If there is any SYS_ERR, sys_err handler needs to process SYS_ERR state
> > and queues the next state transition for device to bring in to Mission
> > mode, so mhi_sync_power_up() needs to wait for device to enter in to
> > mission mode.
> 
> This is very difficult to read, please rephrase the commit message
> 

+1

- Mani

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

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

[Krishna Chaitanya Chundru]

On 8/6/2025 10:29 PM, Manivannan Sadhasivam wrote:
> On Thu, Jul 10, 2025 at 02:28:33PM GMT, Vivek.Pernamitta@quicinc.com wrote:
>> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>
>> In SRIOV enabled devices, the VF DEVICE/VENDOR ID register returns FFFFh
>> when read (PCIe SRIOV spec-3.4.1.1). Therefore, read the PCIe
>> SUBSYSTEM_VENDOR_ID to check if the device is active.
>>
>> 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 | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>> index 7d0ac1c34ddf95ace2a85e5f08884f51604d9b0f..4bafe93b56c54e2b091786e7fcd68a36c8247b8e 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -1025,8 +1025,10 @@ 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;
>> +	if (pdev->is_virtfn)
>> +		pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &vendor);
>> +	else
>> +		pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor);
> 
> You should not read the sub Vendor ID for VF. PCIe spec suggests reading the PF
> Vendor ID for VF. So you should just use pci_physfn() API as below:
> 
> 	pci_read_config_word(pci_physfn(pdev), PCI_VENDOR_ID, &vendor);
> 
> This will work for both PF and VF.
> 
This will defeat the purpose of having health check monitor for VF,
as we are always reading PF vendor ID and will not know VF status at all.

- Krishna Chaitanya.
> - Mani
> 

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

[Manivannan Sadhasivam]

On Thu, Aug 07, 2025 at 12:39:26PM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/6/2025 10:29 PM, Manivannan Sadhasivam wrote:
> > On Thu, Jul 10, 2025 at 02:28:33PM GMT, Vivek.Pernamitta@quicinc.com wrote:
> > > From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> > > 
> > > In SRIOV enabled devices, the VF DEVICE/VENDOR ID register returns FFFFh
> > > when read (PCIe SRIOV spec-3.4.1.1). Therefore, read the PCIe
> > > SUBSYSTEM_VENDOR_ID to check if the device is active.
> > > 
> > > 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 | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> > > index 7d0ac1c34ddf95ace2a85e5f08884f51604d9b0f..4bafe93b56c54e2b091786e7fcd68a36c8247b8e 100644
> > > --- a/drivers/bus/mhi/host/pci_generic.c
> > > +++ b/drivers/bus/mhi/host/pci_generic.c
> > > @@ -1025,8 +1025,10 @@ 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;
> > > +	if (pdev->is_virtfn)
> > > +		pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &vendor);
> > > +	else
> > > +		pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor);
> > 
> > You should not read the sub Vendor ID for VF. PCIe spec suggests reading the PF
> > Vendor ID for VF. So you should just use pci_physfn() API as below:
> > 
> > 	pci_read_config_word(pci_physfn(pdev), PCI_VENDOR_ID, &vendor);
> > 
> > This will work for both PF and VF.
> > 
> This will defeat the purpose of having health check monitor for VF,
> as we are always reading PF vendor ID and will not know VF status at all.

Do we really have a usecase to perform health check for VFs? Health check is
supposed to happen for devices that can fail abruptly.

- Mani

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

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

[Krishna Chaitanya Chundru]

On 8/7/2025 1:43 PM, Manivannan Sadhasivam wrote:
> On Thu, Aug 07, 2025 at 12:39:26PM GMT, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 8/6/2025 10:29 PM, Manivannan Sadhasivam wrote:
>>> On Thu, Jul 10, 2025 at 02:28:33PM GMT, Vivek.Pernamitta@quicinc.com wrote:
>>>> From: Vivek Pernamitta <quic_vpernami@quicinc.com>
>>>>
>>>> In SRIOV enabled devices, the VF DEVICE/VENDOR ID register returns FFFFh
>>>> when read (PCIe SRIOV spec-3.4.1.1). Therefore, read the PCIe
>>>> SUBSYSTEM_VENDOR_ID to check if the device is active.
>>>>
>>>> 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 | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
>>>> index 7d0ac1c34ddf95ace2a85e5f08884f51604d9b0f..4bafe93b56c54e2b091786e7fcd68a36c8247b8e 100644
>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>> @@ -1025,8 +1025,10 @@ 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;
>>>> +	if (pdev->is_virtfn)
>>>> +		pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &vendor);
>>>> +	else
>>>> +		pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor);
>>>
>>> You should not read the sub Vendor ID for VF. PCIe spec suggests reading the PF
>>> Vendor ID for VF. So you should just use pci_physfn() API as below:
>>>
>>> 	pci_read_config_word(pci_physfn(pdev), PCI_VENDOR_ID, &vendor);
>>>
>>> This will work for both PF and VF.
>>>
>> This will defeat the purpose of having health check monitor for VF,
>> as we are always reading PF vendor ID and will not know VF status at all.
> 
> Do we really have a usecase to perform health check for VFs? Health check is
> supposed to happen for devices that can fail abruptly.
> 
yeah as VF is not a physical link we can disable health check monitor
for VF's in the probe itself.

- Krishna Chaitanya.
> - Mani
> 

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

[Manivannan Sadhasivam]

On Thu, Aug 07, 2025 at 01:45:42PM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/7/2025 1:43 PM, Manivannan Sadhasivam wrote:
> > On Thu, Aug 07, 2025 at 12:39:26PM GMT, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 8/6/2025 10:29 PM, Manivannan Sadhasivam wrote:
> > > > On Thu, Jul 10, 2025 at 02:28:33PM GMT, Vivek.Pernamitta@quicinc.com wrote:
> > > > > From: Vivek Pernamitta <quic_vpernami@quicinc.com>
> > > > > 
> > > > > In SRIOV enabled devices, the VF DEVICE/VENDOR ID register returns FFFFh
> > > > > when read (PCIe SRIOV spec-3.4.1.1). Therefore, read the PCIe
> > > > > SUBSYSTEM_VENDOR_ID to check if the device is active.
> > > > > 
> > > > > 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 | 6 ++++--
> > > > >    1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> > > > > index 7d0ac1c34ddf95ace2a85e5f08884f51604d9b0f..4bafe93b56c54e2b091786e7fcd68a36c8247b8e 100644
> > > > > --- a/drivers/bus/mhi/host/pci_generic.c
> > > > > +++ b/drivers/bus/mhi/host/pci_generic.c
> > > > > @@ -1025,8 +1025,10 @@ 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;
> > > > > +	if (pdev->is_virtfn)
> > > > > +		pci_read_config_word(pdev, PCI_SUBSYSTEM_VENDOR_ID, &vendor);
> > > > > +	else
> > > > > +		pci_read_config_word(pdev, PCI_VENDOR_ID, &vendor);
> > > > 
> > > > You should not read the sub Vendor ID for VF. PCIe spec suggests reading the PF
> > > > Vendor ID for VF. So you should just use pci_physfn() API as below:
> > > > 
> > > > 	pci_read_config_word(pci_physfn(pdev), PCI_VENDOR_ID, &vendor);
> > > > 
> > > > This will work for both PF and VF.
> > > > 
> > > This will defeat the purpose of having health check monitor for VF,
> > > as we are always reading PF vendor ID and will not know VF status at all.
> > 
> > Do we really have a usecase to perform health check for VFs? Health check is
> > supposed to happen for devices that can fail abruptly.
> > 
> yeah as VF is not a physical link we can disable health check monitor
> for VF's in the probe itself.
> 

Oh well yes. Otherwise, we will end up with 'num_vfs' number of health monitor
threads eating up the CPU time for no good reason.

- Mani

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