[PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions

[PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions

[Manivannan Sadhasivam]

From: Bhaumik Bhatt <bbhatt@codeaurora.org>

MHI reads the channel ID from the event ring element sent by the
device which can be any value between 0 and 255. In order to
prevent any out of bound accesses, add a check against the maximum
number of channels supported by the controller and those channels
not configured yet so as to skip processing of that event ring
element.

Cc: stable@vger.kernel.org
Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora.org
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/core/main.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 22acde118bc3..ed07421c4870 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl,
 	cmd_pkt = mhi_to_virtual(mhi_ring, ptr);
 
 	chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
-	mhi_chan = &mhi_cntrl->mhi_chan[chan];
-	write_lock_bh(&mhi_chan->lock);
-	mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
-	complete(&mhi_chan->completion);
-	write_unlock_bh(&mhi_chan->lock);
+	WARN_ON(chan >= mhi_cntrl->max_chan);
+
+	if (chan < mhi_cntrl->max_chan &&
+	    mhi_cntrl->mhi_chan[chan].configured) {
+		mhi_chan = &mhi_cntrl->mhi_chan[chan];
+		write_lock_bh(&mhi_chan->lock);
+		mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
+		complete(&mhi_chan->completion);
+		write_unlock_bh(&mhi_chan->lock);
+	}
 
 	mhi_del_ring_element(mhi_cntrl, mhi_ring);
 }
-- 
2.25.1

[PATCH 2/8] bus: mhi: core: Fix power down latency

[Manivannan Sadhasivam]

From: Loic Poulain <loic.poulain@linaro.org>

On graceful power-down/disable transition, when an MHI reset is
performed, the MHI device loses its context, including interrupt
configuration. However, the current implementation is waiting for
event(irq) driven state change to confirm reset has been completed,
which never happens, and causes reset timeout, leading to unexpected
high latency of the mhi_power_down procedure (up to 45 seconds).

Fix that by moving to the recently introduced poll_reg_field method,
waiting for the reset bit to be cleared, in the same way as the
power_on procedure.

Cc: stable@vger.kernel.org
Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state transitions")
Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
Link: https://lore.kernel.org/r/1620029090-8975-1-git-send-email-loic.poulain@linaro.org
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/core/pm.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index e2e59a341fef..704a5e225097 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -465,23 +465,15 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl)
 
 	/* Trigger MHI RESET so that the device will not access host memory */
 	if (!MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) {
-		u32 in_reset = -1;
-		unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms);
-
 		dev_dbg(dev, "Triggering MHI Reset in device\n");
 		mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
 
 		/* Wait for the reset bit to be cleared by the device */
-		ret = wait_event_timeout(mhi_cntrl->state_event,
-					 mhi_read_reg_field(mhi_cntrl,
-							    mhi_cntrl->regs,
-							    MHICTRL,
-							    MHICTRL_RESET_MASK,
-							    MHICTRL_RESET_SHIFT,
-							    &in_reset) ||
-					!in_reset, timeout);
-		if (!ret || in_reset)
-			dev_err(dev, "Device failed to exit MHI Reset state\n");
+		ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
+				 MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0,
+				 25000);
+		if (ret)
+			dev_err(dev, "Device failed to clear MHI Reset\n");
 
 		/*
 		 * Device will clear BHI_INTVEC as a part of RESET processing,
-- 
2.25.1

[PATCH 3/8] bus: mhi: Wait for M2 state during system resume

[Manivannan Sadhasivam]

From: Baochen Qiang <bqiang@codeaurora.org>

During system resume, MHI host triggers M3->M0 transition and then waits
for target device to enter M0 state. Once done, the device queues a state
change event into ctrl event ring and notifies MHI host by raising an
interrupt, where a tasklet is scheduled to process this event. In most
cases, the tasklet is served timely and wait operation succeeds.

However, there are cases where CPU is busy and cannot serve this tasklet
for some time. Once delay goes long enough, the device moves itself to M1
state and also interrupts MHI host after inserting a new state change
event to ctrl ring. Later when CPU finally has time to process the ring,
there will be two events:

1. For M3->M0 event, which is the first event to be processed queued first.
   The tasklet handler serves the event, updates device state to M0 and
   wakes up the task.

2. For M0->M1 event, which is processed later, the tasklet handler
   triggers M1->M2 transition and updates device state to M2 directly,
   then wakes up the MHI host (if it is still sleeping on this wait queue).

Note that although MHI host has been woken up while processing the first
event, it may still has no chance to run before the second event is
processed. In other words, MHI host has to keep waiting till timeout
causing the M0 state to be missed.

kernel log here:
...
Apr 15 01:45:14 test-NUC8i7HVK kernel: [ 4247.911251] mhi 0000:06:00.0: Entered with PM state: M3, MHI state: M3
Apr 15 01:45:14 test-NUC8i7HVK kernel: [ 4247.917762] mhi 0000:06:00.0: State change event to state: M0
Apr 15 01:45:14 test-NUC8i7HVK kernel: [ 4247.917767] mhi 0000:06:00.0: State change event to state: M1
Apr 15 01:45:14 test-NUC8i7HVK kernel: [ 4338.788231] mhi 0000:06:00.0: Did not enter M0 state, MHI state: M2, PM state: M2
...

Fix this issue by simply adding M2 as a valid state for resume.

Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1

Cc: stable@vger.kernel.org
Fixes: 0c6b20a1d720 ("bus: mhi: core: Add support for MHI suspend and resume")
Signed-off-by: Baochen Qiang <bqiang@codeaurora.org>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Link: https://lore.kernel.org/r/20210524040312.14409-1-bqiang@codeaurora.org
[mani: slightly massaged the commit message]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/core/pm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 704a5e225097..bbf6cd04861e 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -926,6 +926,7 @@ int mhi_pm_resume(struct mhi_controller *mhi_cntrl)
 
 	ret = wait_event_timeout(mhi_cntrl->state_event,
 				 mhi_cntrl->dev_state == MHI_STATE_M0 ||
+				 mhi_cntrl->dev_state == MHI_STATE_M2 ||
 				 MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
 				 msecs_to_jiffies(mhi_cntrl->timeout_ms));
 
-- 
2.25.1

[PATCH 5/8] bus: mhi: pci-generic: Add missing 'pci_disable_pcie_error_reporting()' calls

[Manivannan Sadhasivam]

From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

If an error occurs after a 'pci_enable_pcie_error_reporting()' call, it
must be undone by a corresponding 'pci_disable_pcie_error_reporting()'
call

Add the missing call in the error handling path of the probe and in the
remove function.

Cc: <stable@vger.kernel.org>
Fixes: b012ee6bfe2a ("mhi: pci_generic: Add PCI error handlers")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Link: https://lore.kernel.org/r/f70c14701f4922d67e717633c91b6c481b59f298.1623445348.git.christophe.jaillet@wanadoo.fr
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index 7c810f02a2ef..d84b74396c6a 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -665,7 +665,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	err = mhi_register_controller(mhi_cntrl, mhi_cntrl_config);
 	if (err)
-		return err;
+		goto err_disable_reporting;
 
 	/* MHI bus does not power up the controller by default */
 	err = mhi_prepare_for_power_up(mhi_cntrl);
@@ -699,6 +699,8 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhi_unprepare_after_power_down(mhi_cntrl);
 err_unregister:
 	mhi_unregister_controller(mhi_cntrl);
+err_disable_reporting:
+	pci_disable_pcie_error_reporting(pdev);
 
 	return err;
 }
@@ -721,6 +723,7 @@ static void mhi_pci_remove(struct pci_dev *pdev)
 		pm_runtime_get_noresume(&pdev->dev);
 
 	mhi_unregister_controller(mhi_cntrl);
+	pci_disable_pcie_error_reporting(pdev);
 }
 
 static void mhi_pci_shutdown(struct pci_dev *pdev)
-- 
2.25.1

[PATCH 7/8] bus: mhi: pci_generic: Apply no-op for wake using sideband wake boolean

[Manivannan Sadhasivam]

From: Bhaumik Bhatt <bbhatt@codeaurora.org>

Devices such as SDX24 do not have the provision for inband wake
doorbell in the form of channel 127 and instead have a sideband
GPIO for it. Newer devices such as SDX55 or SDX65 support inband
wake method by default. Ensure the functionality is used based on
this such that device wake stays held when a client driver uses
mhi_device_get() API or the equivalent debugfs entry.

Cc: stable@vger.kernel.org
Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake operations")
Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Link: https://lore.kernel.org/r/1624053302-22470-1-git-send-email-bbhatt@codeaurora.org
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index d84b74396c6a..eb9263bd1bd8 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -32,6 +32,8 @@
  * @edl: emergency download mode firmware path (if any)
  * @bar_num: PCI base address register to use for MHI MMIO register space
  * @dma_data_width: DMA transfer word size (32 or 64 bits)
+ * @sideband_wake: Devices using dedicated sideband GPIO for wakeup instead
+ *                 of inband wake support (such as sdx24)
  */
 struct mhi_pci_dev_info {
 	const struct mhi_controller_config *config;
@@ -40,6 +42,7 @@ struct mhi_pci_dev_info {
 	const char *edl;
 	unsigned int bar_num;
 	unsigned int dma_data_width;
+	bool sideband_wake;
 };
 
 #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
@@ -242,7 +245,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
 	.edl = "qcom/sdx65m/edl.mbn",
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.sideband_wake = false
 };
 
 static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
@@ -251,7 +255,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
 	.edl = "qcom/sdx55m/edl.mbn",
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.sideband_wake = false
 };
 
 static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
@@ -259,7 +264,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx24_info = {
 	.edl = "qcom/prog_firehose_sdx24.mbn",
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.sideband_wake = true
 };
 
 static const struct mhi_channel_config mhi_quectel_em1xx_channels[] = {
@@ -301,7 +307,8 @@ static const struct mhi_pci_dev_info mhi_quectel_em1xx_info = {
 	.edl = "qcom/prog_firehose_sdx24.mbn",
 	.config = &modem_quectel_em1xx_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.sideband_wake = true
 };
 
 static const struct mhi_channel_config mhi_foxconn_sdx55_channels[] = {
@@ -339,7 +346,8 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx55_info = {
 	.edl = "qcom/sdx55m/edl.mbn",
 	.config = &modem_foxconn_sdx55_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
-	.dma_data_width = 32
+	.dma_data_width = 32,
+	.sideband_wake = false
 };
 
 static const struct pci_device_id mhi_pci_id_table[] = {
@@ -640,9 +648,12 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhi_cntrl->status_cb = mhi_pci_status_cb;
 	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
 	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
-	mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
-	mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
-	mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
+
+	if (info->sideband_wake) {
+		mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
+		mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
+		mhi_cntrl->wake_toggle = mhi_pci_wake_toggle_nop;
+	}
 
 	err = mhi_pci_claim(mhi_cntrl, info->bar_num, DMA_BIT_MASK(info->dma_data_width));
 	if (err)
-- 
2.25.1

Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions

[Greg KH]

On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
> From: Bhaumik Bhatt <bbhatt@codeaurora.org>
> 
> MHI reads the channel ID from the event ring element sent by the
> device which can be any value between 0 and 255. In order to
> prevent any out of bound accesses, add a check against the maximum
> number of channels supported by the controller and those channels
> not configured yet so as to skip processing of that event ring
> element.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora.org
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/bus/mhi/core/main.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 22acde118bc3..ed07421c4870 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl,
>  	cmd_pkt = mhi_to_virtual(mhi_ring, ptr);
>  
>  	chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
> -	mhi_chan = &mhi_cntrl->mhi_chan[chan];
> -	write_lock_bh(&mhi_chan->lock);
> -	mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
> -	complete(&mhi_chan->completion);
> -	write_unlock_bh(&mhi_chan->lock);
> +	WARN_ON(chan >= mhi_cntrl->max_chan);

What can ever trigger this WARN_ON()?  Do you mean to reboot a machine
if panic-on-warn is set?

If this can actually happen, then check for it and recover properly,
don't just blindly warn and then keep on going.

thanks,

greg k-h

Re: [PATCH 7/8] bus: mhi: pci_generic: Apply no-op for wake using sideband wake boolean

[Greg KH]

On Mon, Jun 21, 2021 at 09:46:15PM +0530, Manivannan Sadhasivam wrote:
> From: Bhaumik Bhatt <bbhatt@codeaurora.org>
> 
> Devices such as SDX24 do not have the provision for inband wake
> doorbell in the form of channel 127 and instead have a sideband
> GPIO for it. Newer devices such as SDX55 or SDX65 support inband
> wake method by default. Ensure the functionality is used based on
> this such that device wake stays held when a client driver uses
> mhi_device_get() API or the equivalent debugfs entry.
> 
> Cc: stable@vger.kernel.org
> Fixes: e3e5e6508fc1 ("bus: mhi: pci_generic: No-Op for device_wake operations")
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Link: https://lore.kernel.org/r/1624053302-22470-1-git-send-email-bbhatt@codeaurora.org
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/bus/mhi/pci_generic.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index d84b74396c6a..eb9263bd1bd8 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -32,6 +32,8 @@
>   * @edl: emergency download mode firmware path (if any)
>   * @bar_num: PCI base address register to use for MHI MMIO register space
>   * @dma_data_width: DMA transfer word size (32 or 64 bits)
> + * @sideband_wake: Devices using dedicated sideband GPIO for wakeup instead
> + *                 of inband wake support (such as sdx24)
>   */
>  struct mhi_pci_dev_info {
>  	const struct mhi_controller_config *config;
> @@ -40,6 +42,7 @@ struct mhi_pci_dev_info {
>  	const char *edl;
>  	unsigned int bar_num;
>  	unsigned int dma_data_width;
> +	bool sideband_wake;
>  };
>  
>  #define MHI_CHANNEL_CONFIG_UL(ch_num, ch_name, el_count, ev_ring) \
> @@ -242,7 +245,8 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
>  	.edl = "qcom/sdx65m/edl.mbn",
>  	.config = &modem_qcom_v1_mhiv_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> -	.dma_data_width = 32
> +	.dma_data_width = 32,
> +	.sideband_wake = false

Please put a trailing , here and for the other definitions, so in the
future you will not have to modify the previous line, like you had to do
here.

thanks,

greg k-h

Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions

[Manivannan Sadhasivam]

On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
> On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
> > From: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > 
> > MHI reads the channel ID from the event ring element sent by the
> > device which can be any value between 0 and 255. In order to
> > prevent any out of bound accesses, add a check against the maximum
> > number of channels supported by the controller and those channels
> > not configured yet so as to skip processing of that event ring
> > element.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
> > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora.org
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/bus/mhi/core/main.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > index 22acde118bc3..ed07421c4870 100644
> > --- a/drivers/bus/mhi/core/main.c
> > +++ b/drivers/bus/mhi/core/main.c
> > @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl,
> >  	cmd_pkt = mhi_to_virtual(mhi_ring, ptr);
> >  
> >  	chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
> > -	mhi_chan = &mhi_cntrl->mhi_chan[chan];
> > -	write_lock_bh(&mhi_chan->lock);
> > -	mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
> > -	complete(&mhi_chan->completion);
> > -	write_unlock_bh(&mhi_chan->lock);
> > +	WARN_ON(chan >= mhi_cntrl->max_chan);
> 
> What can ever trigger this WARN_ON()?  Do you mean to reboot a machine
> if panic-on-warn is set?
> 
> If this can actually happen, then check for it and recover properly,
> don't just blindly warn and then keep on going.
> 

We can't do much here other than warning the user and dropping the
command.

There is no recovery possible because, the device has sent the command
completion event on a wrong channel. It can't happen usually unless a
malcious device sits on the other end.

Thanks,
Mani

> thanks,
> 
> greg k-h

Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions

[Greg KH]

On Thu, Jun 24, 2021 at 08:02:48PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
> > On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
> > > From: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > 
> > > MHI reads the channel ID from the event ring element sent by the
> > > device which can be any value between 0 and 255. In order to
> > > prevent any out of bound accesses, add a check against the maximum
> > > number of channels supported by the controller and those channels
> > > not configured yet so as to skip processing of that event ring
> > > element.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
> > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > > Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora.org
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/bus/mhi/core/main.c | 15 ++++++++++-----
> > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > index 22acde118bc3..ed07421c4870 100644
> > > --- a/drivers/bus/mhi/core/main.c
> > > +++ b/drivers/bus/mhi/core/main.c
> > > @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl,
> > >  	cmd_pkt = mhi_to_virtual(mhi_ring, ptr);
> > >  
> > >  	chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
> > > -	mhi_chan = &mhi_cntrl->mhi_chan[chan];
> > > -	write_lock_bh(&mhi_chan->lock);
> > > -	mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
> > > -	complete(&mhi_chan->completion);
> > > -	write_unlock_bh(&mhi_chan->lock);
> > > +	WARN_ON(chan >= mhi_cntrl->max_chan);
> > 
> > What can ever trigger this WARN_ON()?  Do you mean to reboot a machine
> > if panic-on-warn is set?
> > 
> > If this can actually happen, then check for it and recover properly,
> > don't just blindly warn and then keep on going.
> > 
> 
> We can't do much here other than warning the user and dropping the
> command.

But you didn't warn anyone.  Well, you rebooted the machine, is that ok?
If this can be triggered by a user, this should never happen.

Do not use WARN_ON() ever please.

> There is no recovery possible because, the device has sent the command
> completion event on a wrong channel. It can't happen usually unless a
> malcious device sits on the other end.

Then just eat the message and move on, please do not crash the box.

thanks,

gre k-h

Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions

[Manivannan Sadhasivam]

On Thu, Jun 24, 2021 at 04:39:51PM +0200, Greg KH wrote:
> On Thu, Jun 24, 2021 at 08:02:48PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
> > > On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
> > > > From: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > 
> > > > MHI reads the channel ID from the event ring element sent by the
> > > > device which can be any value between 0 and 255. In order to
> > > > prevent any out of bound accesses, add a check against the maximum
> > > > number of channels supported by the controller and those channels
> > > > not configured yet so as to skip processing of that event ring
> > > > element.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
> > > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > > > Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora.org
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  drivers/bus/mhi/core/main.c | 15 ++++++++++-----
> > > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > > index 22acde118bc3..ed07421c4870 100644
> > > > --- a/drivers/bus/mhi/core/main.c
> > > > +++ b/drivers/bus/mhi/core/main.c
> > > > @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl,
> > > >  	cmd_pkt = mhi_to_virtual(mhi_ring, ptr);
> > > >  
> > > >  	chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
> > > > -	mhi_chan = &mhi_cntrl->mhi_chan[chan];
> > > > -	write_lock_bh(&mhi_chan->lock);
> > > > -	mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
> > > > -	complete(&mhi_chan->completion);
> > > > -	write_unlock_bh(&mhi_chan->lock);
> > > > +	WARN_ON(chan >= mhi_cntrl->max_chan);
> > > 
> > > What can ever trigger this WARN_ON()?  Do you mean to reboot a machine
> > > if panic-on-warn is set?
> > > 
> > > If this can actually happen, then check for it and recover properly,
> > > don't just blindly warn and then keep on going.
> > > 
> > 
> > We can't do much here other than warning the user and dropping the
> > command.
> 
> But you didn't warn anyone.  Well, you rebooted the machine, is that ok?
> If this can be triggered by a user, this should never happen.
> 
> Do not use WARN_ON() ever please.
> 
> > There is no recovery possible because, the device has sent the command
> > completion event on a wrong channel. It can't happen usually unless a
> > malcious device sits on the other end.
> 
> Then just eat the message and move on, please do not crash the box.
> 

Okay. We'll spit an error message and drop the event.

Thanks,
Mani

> thanks,
> 
> gre k-h

Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions

[Greg KH]

On Thu, Jun 24, 2021 at 08:17:52PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 24, 2021 at 04:39:51PM +0200, Greg KH wrote:
> > On Thu, Jun 24, 2021 at 08:02:48PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
> > > > On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
> > > > > From: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > > 
> > > > > MHI reads the channel ID from the event ring element sent by the
> > > > > device which can be any value between 0 and 255. In order to
> > > > > prevent any out of bound accesses, add a check against the maximum
> > > > > number of channels supported by the controller and those channels
> > > > > not configured yet so as to skip processing of that event ring
> > > > > element.
> > > > > 
> > > > > Cc: stable@vger.kernel.org
> > > > > Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
> > > > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > > > > Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora.org
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > ---
> > > > >  drivers/bus/mhi/core/main.c | 15 ++++++++++-----
> > > > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > > > index 22acde118bc3..ed07421c4870 100644
> > > > > --- a/drivers/bus/mhi/core/main.c
> > > > > +++ b/drivers/bus/mhi/core/main.c
> > > > > @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl,
> > > > >  	cmd_pkt = mhi_to_virtual(mhi_ring, ptr);
> > > > >  
> > > > >  	chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
> > > > > -	mhi_chan = &mhi_cntrl->mhi_chan[chan];
> > > > > -	write_lock_bh(&mhi_chan->lock);
> > > > > -	mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
> > > > > -	complete(&mhi_chan->completion);
> > > > > -	write_unlock_bh(&mhi_chan->lock);
> > > > > +	WARN_ON(chan >= mhi_cntrl->max_chan);
> > > > 
> > > > What can ever trigger this WARN_ON()?  Do you mean to reboot a machine
> > > > if panic-on-warn is set?
> > > > 
> > > > If this can actually happen, then check for it and recover properly,
> > > > don't just blindly warn and then keep on going.
> > > > 
> > > 
> > > We can't do much here other than warning the user and dropping the
> > > command.
> > 
> > But you didn't warn anyone.  Well, you rebooted the machine, is that ok?
> > If this can be triggered by a user, this should never happen.
> > 
> > Do not use WARN_ON() ever please.
> > 
> > > There is no recovery possible because, the device has sent the command
> > > completion event on a wrong channel. It can't happen usually unless a
> > > malcious device sits on the other end.
> > 
> > Then just eat the message and move on, please do not crash the box.
> > 
> 
> Okay. We'll spit an error message and drop the event.

If this can be triggered by a user, don't provide a way to DoS the
kernel error log.

thanks,

greg k-h

Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions

[Manivannan Sadhasivam]

On Thu, Jun 24, 2021 at 05:27:17PM +0200, Greg KH wrote:
> On Thu, Jun 24, 2021 at 08:17:52PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jun 24, 2021 at 04:39:51PM +0200, Greg KH wrote:
> > > On Thu, Jun 24, 2021 at 08:02:48PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Jun 24, 2021 at 03:50:45PM +0200, Greg KH wrote:
> > > > > On Mon, Jun 21, 2021 at 09:46:09PM +0530, Manivannan Sadhasivam wrote:
> > > > > > From: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > > > 
> > > > > > MHI reads the channel ID from the event ring element sent by the
> > > > > > device which can be any value between 0 and 255. In order to
> > > > > > prevent any out of bound accesses, add a check against the maximum
> > > > > > number of channels supported by the controller and those channels
> > > > > > not configured yet so as to skip processing of that event ring
> > > > > > element.
> > > > > > 
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Fixes: 1d3173a3bae7 ("bus: mhi: core: Add support for processing events from client device")
> > > > > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > > > > > Link: https://lore.kernel.org/r/1619481538-4435-1-git-send-email-bbhatt@codeaurora.org
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > ---
> > > > > >  drivers/bus/mhi/core/main.c | 15 ++++++++++-----
> > > > > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > > > > index 22acde118bc3..ed07421c4870 100644
> > > > > > --- a/drivers/bus/mhi/core/main.c
> > > > > > +++ b/drivers/bus/mhi/core/main.c
> > > > > > @@ -773,11 +773,16 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl,
> > > > > >  	cmd_pkt = mhi_to_virtual(mhi_ring, ptr);
> > > > > >  
> > > > > >  	chan = MHI_TRE_GET_CMD_CHID(cmd_pkt);
> > > > > > -	mhi_chan = &mhi_cntrl->mhi_chan[chan];
> > > > > > -	write_lock_bh(&mhi_chan->lock);
> > > > > > -	mhi_chan->ccs = MHI_TRE_GET_EV_CODE(tre);
> > > > > > -	complete(&mhi_chan->completion);
> > > > > > -	write_unlock_bh(&mhi_chan->lock);
> > > > > > +	WARN_ON(chan >= mhi_cntrl->max_chan);
> > > > > 
> > > > > What can ever trigger this WARN_ON()?  Do you mean to reboot a machine
> > > > > if panic-on-warn is set?
> > > > > 
> > > > > If this can actually happen, then check for it and recover properly,
> > > > > don't just blindly warn and then keep on going.
> > > > > 
> > > > 
> > > > We can't do much here other than warning the user and dropping the
> > > > command.
> > > 
> > > But you didn't warn anyone.  Well, you rebooted the machine, is that ok?
> > > If this can be triggered by a user, this should never happen.
> > > 
> > > Do not use WARN_ON() ever please.
> > > 
> > > > There is no recovery possible because, the device has sent the command
> > > > completion event on a wrong channel. It can't happen usually unless a
> > > > malcious device sits on the other end.
> > > 
> > > Then just eat the message and move on, please do not crash the box.
> > > 
> > 
> > Okay. We'll spit an error message and drop the event.
> 
> If this can be triggered by a user, don't provide a way to DoS the
> kernel error log.
> 

The term "user" is a bit vague here. Only a malcious device sits on the PCIe
bus that claims a defined VID/PID can trigger this error. And we do need to tell
the user of the host machine that the device tried to do something wrong.

So I guess the error log is fine here?

Thanks,
Mani

> thanks,
> 
> greg k-h