linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] MHI patches for v5.14
@ 2021-06-21 16:16 Manivannan Sadhasivam
  2021-06-21 16:16 ` [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions Manivannan Sadhasivam
                   ` (8 more replies)
  0 siblings, 9 replies; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	Manivannan Sadhasivam

Hi Greg,

Here is the MHI patch series for v5.14.

Summary of the patches:

1. Fixed an issue observed during the system resume where the host timesout
waiting for the M0 state. This has been fixed by adding M2 as the valid
resume state.

2. Added validation for the channel ID read from event ring.

3. Fixed the MHI wake routines used for the newer modems such as SDX55 and
SDX65 by using no-op routines only for the older modems and then relying on
the default routines provided by MHI stack for newer ones.

4. Added the missing "pci_disable_pcie_error_reporting()" call in
pci_generic controller error path.

5. Added support for processing the events based on the priorities. Earlier
a fixed priority was used for all events.

6. Fixed the power down latency by polling the device reset register
instead of waiting for the state change event.

7. Added a dedicated flag to the MHI client transfer APIs for inbound
buffer allocation by the MHI stack. Since this patch modifies the MHI
client drivers under "net/", Ack has been collected from the netdev
maintainer.

8. Added support for Cinterion MV31-W modem in pci_generic controller:
https://www.thalesgroup.com/en/markets/digital-identity-and-security/iot/iot-connectivity/products/iot-products/mv31-w-ultra-high

Thanks,
Mani

Baochen Qiang (1):
  bus: mhi: Wait for M2 state during system resume

Bhaumik Bhatt (2):
  bus: mhi: core: Validate channel ID when processing command
    completions
  bus: mhi: pci_generic: Apply no-op for wake using sideband wake
    boolean

Christophe JAILLET (1):
  bus: mhi: pci-generic: Add missing
    'pci_disable_pcie_error_reporting()' calls

Hemant Kumar (1):
  bus: mhi: core: Add support for processing priority of event ring

Loic Poulain (2):
  bus: mhi: core: Fix power down latency
  bus: mhi: Add inbound buffers allocation flag

ULRICH Thomas (1):
  bus: mhi: pci_generic: Add Cinterion MV31-W PCIe to MHI

 drivers/bus/mhi/core/init.c      |  3 +-
 drivers/bus/mhi/core/internal.h  |  2 +-
 drivers/bus/mhi/core/main.c      | 35 ++++++++++++-----
 drivers/bus/mhi/core/pm.c        | 19 +++------
 drivers/bus/mhi/pci_generic.c    | 67 ++++++++++++++++++++++++++++----
 drivers/net/mhi/net.c            |  2 +-
 drivers/net/wwan/mhi_wwan_ctrl.c |  2 +-
 include/linux/mhi.h              | 14 ++++++-
 net/qrtr/mhi.c                   |  2 +-
 9 files changed, 107 insertions(+), 39 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
@ 2021-06-21 16:16 ` Manivannan Sadhasivam
  2021-06-24 13:50   ` Greg KH
  2021-06-21 16:16 ` [PATCH 2/8] bus: mhi: core: Fix power down latency Manivannan Sadhasivam
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, Manivannan Sadhasivam, Jeffrey Hugo

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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 2/8] bus: mhi: core: Fix power down latency
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
  2021-06-21 16:16 ` [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions Manivannan Sadhasivam
@ 2021-06-21 16:16 ` Manivannan Sadhasivam
  2021-06-21 16:16 ` [PATCH 3/8] bus: mhi: Wait for M2 state during system resume Manivannan Sadhasivam
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, 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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 3/8] bus: mhi: Wait for M2 state during system resume
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
  2021-06-21 16:16 ` [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions Manivannan Sadhasivam
  2021-06-21 16:16 ` [PATCH 2/8] bus: mhi: core: Fix power down latency Manivannan Sadhasivam
@ 2021-06-21 16:16 ` Manivannan Sadhasivam
  2021-06-21 16:16 ` [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag Manivannan Sadhasivam
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	Baochen Qiang, stable, 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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
                   ` (2 preceding siblings ...)
  2021-06-21 16:16 ` [PATCH 3/8] bus: mhi: Wait for M2 state during system resume Manivannan Sadhasivam
@ 2021-06-21 16:16 ` Manivannan Sadhasivam
  2021-06-24 13:51   ` Greg KH
  2021-06-21 16:16 ` [PATCH 5/8] bus: mhi: pci-generic: Add missing 'pci_disable_pcie_error_reporting()' calls Manivannan Sadhasivam
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	Manivannan Sadhasivam, Jakub Kicinski

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

Currently, the MHI controller driver defines which channels should
have their inbound buffers allocated and queued. But ideally, this is
something that should be decided by the MHI device driver instead,
which actually deals with that buffers.

Add a flag parameter to mhi_prepare_for_transfer allowing to specify
if buffers have to be allocated and queued by the MHI stack.

Keep auto_queue flag for now, but should be removed at some point.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Jakub Kicinski <kuba@kernel.org>
Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/core/internal.h  |  2 +-
 drivers/bus/mhi/core/main.c      | 11 ++++++++---
 drivers/net/mhi/net.c            |  2 +-
 drivers/net/wwan/mhi_wwan_ctrl.c |  2 +-
 include/linux/mhi.h              | 12 +++++++++++-
 net/qrtr/mhi.c                   |  2 +-
 6 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 5b9ea66b92dc..672052fe3b44 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -682,7 +682,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
 		      struct image_info *img_info);
 void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
-			struct mhi_chan *mhi_chan);
+			struct mhi_chan *mhi_chan, enum mhi_chan_flags flags);
 int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
 		       struct mhi_chan *mhi_chan);
 void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index ed07421c4870..8ac73f9e92a6 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -1428,7 +1428,8 @@ static void mhi_unprepare_channel(struct mhi_controller *mhi_cntrl,
 }
 
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
-			struct mhi_chan *mhi_chan)
+			struct mhi_chan *mhi_chan,
+			enum mhi_chan_flags flags)
 {
 	int ret = 0;
 	struct device *dev = &mhi_chan->mhi_dev->dev;
@@ -1453,6 +1454,9 @@ int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
 	if (ret)
 		goto error_pm_state;
 
+	if (mhi_chan->dir == DMA_FROM_DEVICE)
+		mhi_chan->pre_alloc = !!(flags & MHI_CH_INBOUND_ALLOC_BUFS);
+
 	/* Pre-allocate buffer for xfer ring */
 	if (mhi_chan->pre_alloc) {
 		int nr_el = get_nr_avail_ring_elements(mhi_cntrl,
@@ -1608,7 +1612,8 @@ void mhi_reset_chan(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan)
 }
 
 /* Move channel to start state */
-int mhi_prepare_for_transfer(struct mhi_device *mhi_dev)
+int mhi_prepare_for_transfer(struct mhi_device *mhi_dev,
+			     enum mhi_chan_flags flags)
 {
 	int ret, dir;
 	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
@@ -1619,7 +1624,7 @@ int mhi_prepare_for_transfer(struct mhi_device *mhi_dev)
 		if (!mhi_chan)
 			continue;
 
-		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan);
+		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan, flags);
 		if (ret)
 			goto error_open_chan;
 	}
diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
index 0d8293a47a56..774e32960e09 100644
--- a/drivers/net/mhi/net.c
+++ b/drivers/net/mhi/net.c
@@ -327,7 +327,7 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
 	u64_stats_init(&mhi_netdev->stats.tx_syncp);
 
 	/* Start MHI channels */
-	err = mhi_prepare_for_transfer(mhi_dev);
+	err = mhi_prepare_for_transfer(mhi_dev, 0);
 	if (err)
 		goto out_err;
 
diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
index 1bc6b69aa530..1e18420ce404 100644
--- a/drivers/net/wwan/mhi_wwan_ctrl.c
+++ b/drivers/net/wwan/mhi_wwan_ctrl.c
@@ -110,7 +110,7 @@ static int mhi_wwan_ctrl_start(struct wwan_port *port)
 	int ret;
 
 	/* Start mhi device's channel(s) */
-	ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev);
+	ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev, 0);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 944aa3aa3035..86cea5256e3c 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -59,6 +59,14 @@ enum mhi_flags {
 	MHI_CHAIN = BIT(2),
 };
 
+/**
+ * enum mhi_chan_flags - MHI channel flags
+ * @MHI_CH_INBOUND_ALLOC_BUFS: Automatically allocate and queue inbound buffers
+ */
+enum mhi_chan_flags {
+	MHI_CH_INBOUND_ALLOC_BUFS = BIT(0),
+};
+
 /**
  * enum mhi_device_type - Device types
  * @MHI_DEVICE_XFER: Handles data transfer
@@ -719,8 +727,10 @@ void mhi_device_put(struct mhi_device *mhi_dev);
  *                            host and device execution environments match and
  *                            channels are in a DISABLED state.
  * @mhi_dev: Device associated with the channels
+ * @flags: MHI channel flags
  */
-int mhi_prepare_for_transfer(struct mhi_device *mhi_dev);
+int mhi_prepare_for_transfer(struct mhi_device *mhi_dev,
+			     enum mhi_chan_flags flags);
 
 /**
  * mhi_unprepare_from_transfer - Reset UL and DL channels for data transfer.
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
index fa611678af05..29b4fa3b72ab 100644
--- a/net/qrtr/mhi.c
+++ b/net/qrtr/mhi.c
@@ -79,7 +79,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
 	int rc;
 
 	/* start channels */
-	rc = mhi_prepare_for_transfer(mhi_dev);
+	rc = mhi_prepare_for_transfer(mhi_dev, MHI_CH_INBOUND_ALLOC_BUFS);
 	if (rc)
 		return rc;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 5/8] bus: mhi: pci-generic: Add missing 'pci_disable_pcie_error_reporting()' calls
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
                   ` (3 preceding siblings ...)
  2021-06-21 16:16 ` [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag Manivannan Sadhasivam
@ 2021-06-21 16:16 ` Manivannan Sadhasivam
  2021-06-21 16:16 ` [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring Manivannan Sadhasivam
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	Christophe JAILLET, stable, 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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
                   ` (4 preceding siblings ...)
  2021-06-21 16:16 ` [PATCH 5/8] bus: mhi: pci-generic: Add missing 'pci_disable_pcie_error_reporting()' calls Manivannan Sadhasivam
@ 2021-06-21 16:16 ` Manivannan Sadhasivam
  2021-06-24 13:53   ` Greg KH
  2021-06-21 16:16 ` [PATCH 7/8] bus: mhi: pci_generic: Apply no-op for wake using sideband wake boolean Manivannan Sadhasivam
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	Manivannan Sadhasivam

From: Hemant Kumar <hemantk@codeaurora.org>

Event ring priorities are currently set to 1 and are unused.
Default processing priority for event rings is set to regular
tasklet. Controllers can choose to use high priority tasklet
scheduling for certain event rings critical for processing such
as ones transporting control information if they wish to avoid
system scheduling delays for those packets. In order to support
these use cases, allow controllers to set event ring priority to
high.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Link: https://lore.kernel.org/r/1624053903-24653-2-git-send-email-bbhatt@codeaurora.org
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/core/init.c | 3 +--
 drivers/bus/mhi/core/main.c | 9 +++++++--
 include/linux/mhi.h         | 2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index c81b377fca8f..444676034bf0 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -673,8 +673,7 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
 				&mhi_cntrl->mhi_chan[mhi_event->chan];
 		}
 
-		/* Priority is fixed to 1 for now */
-		mhi_event->priority = 1;
+		mhi_event->priority = event_cfg->priority;
 
 		mhi_event->db_cfg.brstmode = event_cfg->mode;
 		if (MHI_INVALID_BRSTMODE(mhi_event->db_cfg.brstmode))
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 8ac73f9e92a6..3775c77dec63 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -454,10 +454,15 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
 
 		if (mhi_dev)
 			mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
-	} else {
-		tasklet_schedule(&mhi_event->task);
+
+		return IRQ_HANDLED;
 	}
 
+	if (!mhi_event->priority)
+		tasklet_hi_schedule(&mhi_event->task);
+	else
+		tasklet_schedule(&mhi_event->task);
+
 	return IRQ_HANDLED;
 }
 
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 86cea5256e3c..bf23c213429c 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -250,7 +250,7 @@ struct mhi_channel_config {
  * @irq_moderation_ms: Delay irq for additional events to be aggregated
  * @irq: IRQ associated with this ring
  * @channel: Dedicated channel number. U32_MAX indicates a non-dedicated ring
- * @priority: Priority of this ring. Use 1 for now
+ * @priority: Processing priority of this ring. 0 is high and 1 is regular
  * @mode: Doorbell mode
  * @data_type: Type of data this ring will process
  * @hardware_event: This ring is associated with hardware channels
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 7/8] bus: mhi: pci_generic: Apply no-op for wake using sideband wake boolean
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
                   ` (5 preceding siblings ...)
  2021-06-21 16:16 ` [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring Manivannan Sadhasivam
@ 2021-06-21 16:16 ` Manivannan Sadhasivam
  2021-06-24 13:54   ` Greg KH
  2021-06-21 16:16 ` [PATCH 8/8] bus: mhi: pci_generic: Add Cinterion MV31-W PCIe to MHI Manivannan Sadhasivam
  2021-06-24 13:54 ` [PATCH 0/8] MHI patches for v5.14 Greg KH
  8 siblings, 1 reply; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, 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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 8/8] bus: mhi: pci_generic: Add Cinterion MV31-W PCIe to MHI
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
                   ` (6 preceding siblings ...)
  2021-06-21 16:16 ` [PATCH 7/8] bus: mhi: pci_generic: Apply no-op for wake using sideband wake boolean Manivannan Sadhasivam
@ 2021-06-21 16:16 ` Manivannan Sadhasivam
  2021-06-24 13:54   ` Greg KH
  2021-06-24 13:54 ` [PATCH 0/8] MHI patches for v5.14 Greg KH
  8 siblings, 1 reply; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-21 16:16 UTC (permalink / raw)
  To: gregkh
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	ULRICH Thomas, Manivannan Sadhasivam

From: ULRICH Thomas <thomas.ulrich@thalesgroup.com>

This patch adds VendorID/ProductID and MBIM Channel Definitions for
M.2 Modem Card (PCIe Variant) to MHI PCI generic controller driver.

Cinterion MV31-W (by Thales)
Additional information on such Modem Card (USB or PCIe variant) is
available at:
https://www.thalesgroup.com/en/markets/digital-identity-and-security/iot/iot-connectivity/products/iot-products/mv31-w-ultra-high

Signed-off-by: ULRICH Thomas <thomas.ulrich@thalesgroup.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Link: https://lore.kernel.org/r/PAZP264MB284690134DA010698E6B3BDDE60A9@PAZP264MB2846.FRAP264.PROD.OUTLOOK.COM
[mani: fixed the subject, whitespace, and added sideband_wake field]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/pci_generic.c | 37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index eb9263bd1bd8..1773cb3173bc 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -350,6 +350,40 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx55_info = {
 	.sideband_wake = false
 };
 
+static const struct mhi_channel_config mhi_mv31_channels[] = {
+	MHI_CHANNEL_CONFIG_UL(0, "LOOPBACK", 64, 0),
+	MHI_CHANNEL_CONFIG_DL(1, "LOOPBACK", 64, 0),
+	/* MBIM Control Channel */
+	MHI_CHANNEL_CONFIG_UL(12, "MBIM", 64, 0),
+	MHI_CHANNEL_CONFIG_DL(13, "MBIM", 64, 0),
+	/* MBIM Data Channel */
+	MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0_MBIM", 512, 2),
+	MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0_MBIM", 512, 3),
+};
+
+static struct mhi_event_config mhi_mv31_events[] = {
+	MHI_EVENT_CONFIG_CTRL(0, 256),
+	MHI_EVENT_CONFIG_DATA(1, 256),
+	MHI_EVENT_CONFIG_HW_DATA(2, 1024, 100),
+	MHI_EVENT_CONFIG_HW_DATA(3, 1024, 101)
+};
+
+static const struct mhi_controller_config modem_mv31_config = {
+	.max_channels = 128,
+	.timeout_ms = 20000,
+	.num_channels = ARRAY_SIZE(mhi_mv31_channels),
+	.ch_cfg = mhi_mv31_channels,
+	.num_events = ARRAY_SIZE(mhi_mv31_events),
+	.event_cfg = mhi_mv31_events,
+};
+
+static const struct mhi_pci_dev_info mhi_mv31_info = {
+	.name = "cinterion-mv31",
+	.config = &modem_mv31_config,
+	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
+	.dma_data_width = 32
+};
+
 static const struct pci_device_id mhi_pci_id_table[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, 0x0306),
 		.driver_data = (kernel_ulong_t) &mhi_qcom_sdx55_info },
@@ -370,6 +404,9 @@ static const struct pci_device_id mhi_pci_id_table[] = {
 	/* DW5930e (sdx55), Non-eSIM, It's also T99W175 */
 	{ PCI_DEVICE(PCI_VENDOR_ID_FOXCONN, 0xe0b1),
 		.driver_data = (kernel_ulong_t) &mhi_foxconn_sdx55_info },
+	/* MV31-W (Cinterion) */
+	{ PCI_DEVICE(0x1269, 0x00b3),
+		.driver_data = (kernel_ulong_t) &mhi_mv31_info },
 	{  }
 };
 MODULE_DEVICE_TABLE(pci, mhi_pci_id_table);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions
  2021-06-21 16:16 ` [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions Manivannan Sadhasivam
@ 2021-06-24 13:50   ` Greg KH
  2021-06-24 14:32     ` Manivannan Sadhasivam
  0 siblings, 1 reply; 27+ messages in thread
From: Greg KH @ 2021-06-24 13:50 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, Jeffrey Hugo

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag
  2021-06-21 16:16 ` [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag Manivannan Sadhasivam
@ 2021-06-24 13:51   ` Greg KH
  2021-06-24 15:39     ` Loic Poulain
  0 siblings, 1 reply; 27+ messages in thread
From: Greg KH @ 2021-06-24 13:51 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	Jakub Kicinski

On Mon, Jun 21, 2021 at 09:46:12PM +0530, Manivannan Sadhasivam wrote:
> From: Loic Poulain <loic.poulain@linaro.org>
> 
> Currently, the MHI controller driver defines which channels should
> have their inbound buffers allocated and queued. But ideally, this is
> something that should be decided by the MHI device driver instead,
> which actually deals with that buffers.
> 
> Add a flag parameter to mhi_prepare_for_transfer allowing to specify
> if buffers have to be allocated and queued by the MHI stack.
> 
> Keep auto_queue flag for now, but should be removed at some point.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Acked-by: Jakub Kicinski <kuba@kernel.org>
> Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/bus/mhi/core/internal.h  |  2 +-
>  drivers/bus/mhi/core/main.c      | 11 ++++++++---
>  drivers/net/mhi/net.c            |  2 +-
>  drivers/net/wwan/mhi_wwan_ctrl.c |  2 +-
>  include/linux/mhi.h              | 12 +++++++++++-
>  net/qrtr/mhi.c                   |  2 +-
>  6 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 5b9ea66b92dc..672052fe3b44 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -682,7 +682,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
>  		      struct image_info *img_info);
>  void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
>  int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
> -			struct mhi_chan *mhi_chan);
> +			struct mhi_chan *mhi_chan, enum mhi_chan_flags flags);
>  int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
>  		       struct mhi_chan *mhi_chan);
>  void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index ed07421c4870..8ac73f9e92a6 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -1428,7 +1428,8 @@ static void mhi_unprepare_channel(struct mhi_controller *mhi_cntrl,
>  }
>  
>  int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
> -			struct mhi_chan *mhi_chan)
> +			struct mhi_chan *mhi_chan,
> +			enum mhi_chan_flags flags)
>  {
>  	int ret = 0;
>  	struct device *dev = &mhi_chan->mhi_dev->dev;
> @@ -1453,6 +1454,9 @@ int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
>  	if (ret)
>  		goto error_pm_state;
>  
> +	if (mhi_chan->dir == DMA_FROM_DEVICE)
> +		mhi_chan->pre_alloc = !!(flags & MHI_CH_INBOUND_ALLOC_BUFS);
> +
>  	/* Pre-allocate buffer for xfer ring */
>  	if (mhi_chan->pre_alloc) {
>  		int nr_el = get_nr_avail_ring_elements(mhi_cntrl,
> @@ -1608,7 +1612,8 @@ void mhi_reset_chan(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan)
>  }
>  
>  /* Move channel to start state */
> -int mhi_prepare_for_transfer(struct mhi_device *mhi_dev)
> +int mhi_prepare_for_transfer(struct mhi_device *mhi_dev,
> +			     enum mhi_chan_flags flags)
>  {
>  	int ret, dir;
>  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> @@ -1619,7 +1624,7 @@ int mhi_prepare_for_transfer(struct mhi_device *mhi_dev)
>  		if (!mhi_chan)
>  			continue;
>  
> -		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan);
> +		ret = mhi_prepare_channel(mhi_cntrl, mhi_chan, flags);
>  		if (ret)
>  			goto error_open_chan;
>  	}
> diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
> index 0d8293a47a56..774e32960e09 100644
> --- a/drivers/net/mhi/net.c
> +++ b/drivers/net/mhi/net.c
> @@ -327,7 +327,7 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
>  	u64_stats_init(&mhi_netdev->stats.tx_syncp);
>  
>  	/* Start MHI channels */
> -	err = mhi_prepare_for_transfer(mhi_dev);
> +	err = mhi_prepare_for_transfer(mhi_dev, 0);
>  	if (err)
>  		goto out_err;
>  
> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index 1bc6b69aa530..1e18420ce404 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> @@ -110,7 +110,7 @@ static int mhi_wwan_ctrl_start(struct wwan_port *port)
>  	int ret;
>  
>  	/* Start mhi device's channel(s) */
> -	ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev);
> +	ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev, 0);
>  	if (ret)
>  		return ret;
>  
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 944aa3aa3035..86cea5256e3c 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -59,6 +59,14 @@ enum mhi_flags {
>  	MHI_CHAIN = BIT(2),
>  };
>  
> +/**
> + * enum mhi_chan_flags - MHI channel flags
> + * @MHI_CH_INBOUND_ALLOC_BUFS: Automatically allocate and queue inbound buffers
> + */
> +enum mhi_chan_flags {
> +	MHI_CH_INBOUND_ALLOC_BUFS = BIT(0),

Why is an enumerated type a bitfield?

Please just use integers for enumerated types.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring
  2021-06-21 16:16 ` [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring Manivannan Sadhasivam
@ 2021-06-24 13:53   ` Greg KH
  2021-06-24 14:24     ` Manivannan Sadhasivam
  0 siblings, 1 reply; 27+ messages in thread
From: Greg KH @ 2021-06-24 13:53 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain

On Mon, Jun 21, 2021 at 09:46:14PM +0530, Manivannan Sadhasivam wrote:
> From: Hemant Kumar <hemantk@codeaurora.org>
> 
> Event ring priorities are currently set to 1 and are unused.
> Default processing priority for event rings is set to regular
> tasklet. Controllers can choose to use high priority tasklet
> scheduling for certain event rings critical for processing such
> as ones transporting control information if they wish to avoid
> system scheduling delays for those packets. In order to support
> these use cases, allow controllers to set event ring priority to
> high.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Link: https://lore.kernel.org/r/1624053903-24653-2-git-send-email-bbhatt@codeaurora.org
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/bus/mhi/core/init.c | 3 +--
>  drivers/bus/mhi/core/main.c | 9 +++++++--
>  include/linux/mhi.h         | 2 +-
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index c81b377fca8f..444676034bf0 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -673,8 +673,7 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
>  				&mhi_cntrl->mhi_chan[mhi_event->chan];
>  		}
>  
> -		/* Priority is fixed to 1 for now */
> -		mhi_event->priority = 1;
> +		mhi_event->priority = event_cfg->priority;
>  
>  		mhi_event->db_cfg.brstmode = event_cfg->mode;
>  		if (MHI_INVALID_BRSTMODE(mhi_event->db_cfg.brstmode))
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 8ac73f9e92a6..3775c77dec63 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -454,10 +454,15 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>  
>  		if (mhi_dev)
>  			mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
> -	} else {
> -		tasklet_schedule(&mhi_event->task);
> +
> +		return IRQ_HANDLED;
>  	}
>  
> +	if (!mhi_event->priority)
> +		tasklet_hi_schedule(&mhi_event->task);
> +	else
> +		tasklet_schedule(&mhi_event->task);
> +
>  	return IRQ_HANDLED;
>  }
>  
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 86cea5256e3c..bf23c213429c 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -250,7 +250,7 @@ struct mhi_channel_config {
>   * @irq_moderation_ms: Delay irq for additional events to be aggregated
>   * @irq: IRQ associated with this ring
>   * @channel: Dedicated channel number. U32_MAX indicates a non-dedicated ring
> - * @priority: Priority of this ring. Use 1 for now
> + * @priority: Processing priority of this ring. 0 is high and 1 is regular

Why is 0 high and 1 low?  Does that feel backwards?

Shouldn't this be a boolean, or if not, an enumerated type so that
people can read the code over time?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 7/8] bus: mhi: pci_generic: Apply no-op for wake using sideband wake boolean
  2021-06-21 16:16 ` [PATCH 7/8] bus: mhi: pci_generic: Apply no-op for wake using sideband wake boolean Manivannan Sadhasivam
@ 2021-06-24 13:54   ` Greg KH
  0 siblings, 0 replies; 27+ messages in thread
From: Greg KH @ 2021-06-24 13:54 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain, stable

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 8/8] bus: mhi: pci_generic: Add Cinterion MV31-W PCIe to MHI
  2021-06-21 16:16 ` [PATCH 8/8] bus: mhi: pci_generic: Add Cinterion MV31-W PCIe to MHI Manivannan Sadhasivam
@ 2021-06-24 13:54   ` Greg KH
  0 siblings, 0 replies; 27+ messages in thread
From: Greg KH @ 2021-06-24 13:54 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	ULRICH Thomas

On Mon, Jun 21, 2021 at 09:46:16PM +0530, Manivannan Sadhasivam wrote:
> From: ULRICH Thomas <thomas.ulrich@thalesgroup.com>
> 
> This patch adds VendorID/ProductID and MBIM Channel Definitions for
> M.2 Modem Card (PCIe Variant) to MHI PCI generic controller driver.
> 
> Cinterion MV31-W (by Thales)
> Additional information on such Modem Card (USB or PCIe variant) is
> available at:
> https://www.thalesgroup.com/en/markets/digital-identity-and-security/iot/iot-connectivity/products/iot-products/mv31-w-ultra-high
> 
> Signed-off-by: ULRICH Thomas <thomas.ulrich@thalesgroup.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Link: https://lore.kernel.org/r/PAZP264MB284690134DA010698E6B3BDDE60A9@PAZP264MB2846.FRAP264.PROD.OUTLOOK.COM
> [mani: fixed the subject, whitespace, and added sideband_wake field]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/bus/mhi/pci_generic.c | 37 +++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
> index eb9263bd1bd8..1773cb3173bc 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -350,6 +350,40 @@ static const struct mhi_pci_dev_info mhi_foxconn_sdx55_info = {
>  	.sideband_wake = false
>  };
>  
> +static const struct mhi_channel_config mhi_mv31_channels[] = {
> +	MHI_CHANNEL_CONFIG_UL(0, "LOOPBACK", 64, 0),
> +	MHI_CHANNEL_CONFIG_DL(1, "LOOPBACK", 64, 0),
> +	/* MBIM Control Channel */
> +	MHI_CHANNEL_CONFIG_UL(12, "MBIM", 64, 0),
> +	MHI_CHANNEL_CONFIG_DL(13, "MBIM", 64, 0),
> +	/* MBIM Data Channel */
> +	MHI_CHANNEL_CONFIG_HW_UL(100, "IP_HW0_MBIM", 512, 2),
> +	MHI_CHANNEL_CONFIG_HW_DL(101, "IP_HW0_MBIM", 512, 3),
> +};
> +
> +static struct mhi_event_config mhi_mv31_events[] = {
> +	MHI_EVENT_CONFIG_CTRL(0, 256),
> +	MHI_EVENT_CONFIG_DATA(1, 256),
> +	MHI_EVENT_CONFIG_HW_DATA(2, 1024, 100),
> +	MHI_EVENT_CONFIG_HW_DATA(3, 1024, 101)

trailing ',' please.

> +};
> +
> +static const struct mhi_controller_config modem_mv31_config = {
> +	.max_channels = 128,
> +	.timeout_ms = 20000,
> +	.num_channels = ARRAY_SIZE(mhi_mv31_channels),
> +	.ch_cfg = mhi_mv31_channels,
> +	.num_events = ARRAY_SIZE(mhi_mv31_events),
> +	.event_cfg = mhi_mv31_events,
> +};
> +
> +static const struct mhi_pci_dev_info mhi_mv31_info = {
> +	.name = "cinterion-mv31",
> +	.config = &modem_mv31_config,
> +	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> +	.dma_data_width = 32

Trailing ',' please.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/8] MHI patches for v5.14
  2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
                   ` (7 preceding siblings ...)
  2021-06-21 16:16 ` [PATCH 8/8] bus: mhi: pci_generic: Add Cinterion MV31-W PCIe to MHI Manivannan Sadhasivam
@ 2021-06-24 13:54 ` Greg KH
  2021-06-24 14:20   ` Manivannan Sadhasivam
  8 siblings, 1 reply; 27+ messages in thread
From: Greg KH @ 2021-06-24 13:54 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain

On Mon, Jun 21, 2021 at 09:46:08PM +0530, Manivannan Sadhasivam wrote:
> Hi Greg,
> 
> Here is the MHI patch series for v5.14.
> 
> Summary of the patches:

I took some of them :)


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 0/8] MHI patches for v5.14
  2021-06-24 13:54 ` [PATCH 0/8] MHI patches for v5.14 Greg KH
@ 2021-06-24 14:20   ` Manivannan Sadhasivam
  0 siblings, 0 replies; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-24 14:20 UTC (permalink / raw)
  To: Greg KH; +Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain

On Thu, Jun 24, 2021 at 03:54:51PM +0200, Greg KH wrote:
> On Mon, Jun 21, 2021 at 09:46:08PM +0530, Manivannan Sadhasivam wrote:
> > Hi Greg,
> > 
> > Here is the MHI patch series for v5.14.
> > 
> > Summary of the patches:
> 
> I took some of them :)
> 

Thanks! I'll send the left over ones after fixing the comments.

Thanks,
Mani

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring
  2021-06-24 13:53   ` Greg KH
@ 2021-06-24 14:24     ` Manivannan Sadhasivam
  2021-06-24 14:40       ` Greg KH
  0 siblings, 1 reply; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-24 14:24 UTC (permalink / raw)
  To: Greg KH; +Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain

On Thu, Jun 24, 2021 at 03:53:33PM +0200, Greg KH wrote:
> On Mon, Jun 21, 2021 at 09:46:14PM +0530, Manivannan Sadhasivam wrote:
> > From: Hemant Kumar <hemantk@codeaurora.org>
> > 
> > Event ring priorities are currently set to 1 and are unused.
> > Default processing priority for event rings is set to regular
> > tasklet. Controllers can choose to use high priority tasklet
> > scheduling for certain event rings critical for processing such
> > as ones transporting control information if they wish to avoid
> > system scheduling delays for those packets. In order to support
> > these use cases, allow controllers to set event ring priority to
> > high.
> > 
> > Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Link: https://lore.kernel.org/r/1624053903-24653-2-git-send-email-bbhatt@codeaurora.org
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/bus/mhi/core/init.c | 3 +--
> >  drivers/bus/mhi/core/main.c | 9 +++++++--
> >  include/linux/mhi.h         | 2 +-
> >  3 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > index c81b377fca8f..444676034bf0 100644
> > --- a/drivers/bus/mhi/core/init.c
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -673,8 +673,7 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
> >  				&mhi_cntrl->mhi_chan[mhi_event->chan];
> >  		}
> >  
> > -		/* Priority is fixed to 1 for now */
> > -		mhi_event->priority = 1;
> > +		mhi_event->priority = event_cfg->priority;
> >  
> >  		mhi_event->db_cfg.brstmode = event_cfg->mode;
> >  		if (MHI_INVALID_BRSTMODE(mhi_event->db_cfg.brstmode))
> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > index 8ac73f9e92a6..3775c77dec63 100644
> > --- a/drivers/bus/mhi/core/main.c
> > +++ b/drivers/bus/mhi/core/main.c
> > @@ -454,10 +454,15 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
> >  
> >  		if (mhi_dev)
> >  			mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
> > -	} else {
> > -		tasklet_schedule(&mhi_event->task);
> > +
> > +		return IRQ_HANDLED;
> >  	}
> >  
> > +	if (!mhi_event->priority)
> > +		tasklet_hi_schedule(&mhi_event->task);
> > +	else
> > +		tasklet_schedule(&mhi_event->task);
> > +
> >  	return IRQ_HANDLED;
> >  }
> >  
> > diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> > index 86cea5256e3c..bf23c213429c 100644
> > --- a/include/linux/mhi.h
> > +++ b/include/linux/mhi.h
> > @@ -250,7 +250,7 @@ struct mhi_channel_config {
> >   * @irq_moderation_ms: Delay irq for additional events to be aggregated
> >   * @irq: IRQ associated with this ring
> >   * @channel: Dedicated channel number. U32_MAX indicates a non-dedicated ring
> > - * @priority: Priority of this ring. Use 1 for now
> > + * @priority: Processing priority of this ring. 0 is high and 1 is regular
> 
> Why is 0 high and 1 low?  Does that feel backwards?
> 

That's because, "1" was used from the beginning by the controller drivers
as the regular priority. And I thought of using "0" as high priority so
that we can leave the controller drivers unmodified.

> Shouldn't this be a boolean, or if not, an enumerated type so that
> people can read the code over time?
> 

Bhaumik proposed an enum but I wanted 0/1 so that the controller drivers
can be untouched. Also, I don't see any immediate requirement for other
priorities.

Will make it a bool then.

Thanks,
Mani

> thanks,
> 
> greg k-h

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions
  2021-06-24 13:50   ` Greg KH
@ 2021-06-24 14:32     ` Manivannan Sadhasivam
  2021-06-24 14:39       ` Greg KH
  0 siblings, 1 reply; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-24 14:32 UTC (permalink / raw)
  To: Greg KH
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, Jeffrey Hugo

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions
  2021-06-24 14:32     ` Manivannan Sadhasivam
@ 2021-06-24 14:39       ` Greg KH
  2021-06-24 14:47         ` Manivannan Sadhasivam
  0 siblings, 1 reply; 27+ messages in thread
From: Greg KH @ 2021-06-24 14:39 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, Jeffrey Hugo

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring
  2021-06-24 14:24     ` Manivannan Sadhasivam
@ 2021-06-24 14:40       ` Greg KH
  2021-06-24 14:50         ` Manivannan Sadhasivam
  0 siblings, 1 reply; 27+ messages in thread
From: Greg KH @ 2021-06-24 14:40 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain

On Thu, Jun 24, 2021 at 07:54:53PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 24, 2021 at 03:53:33PM +0200, Greg KH wrote:
> > On Mon, Jun 21, 2021 at 09:46:14PM +0530, Manivannan Sadhasivam wrote:
> > > From: Hemant Kumar <hemantk@codeaurora.org>
> > > 
> > > Event ring priorities are currently set to 1 and are unused.
> > > Default processing priority for event rings is set to regular
> > > tasklet. Controllers can choose to use high priority tasklet
> > > scheduling for certain event rings critical for processing such
> > > as ones transporting control information if they wish to avoid
> > > system scheduling delays for those packets. In order to support
> > > these use cases, allow controllers to set event ring priority to
> > > high.
> > > 
> > > Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Link: https://lore.kernel.org/r/1624053903-24653-2-git-send-email-bbhatt@codeaurora.org
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/bus/mhi/core/init.c | 3 +--
> > >  drivers/bus/mhi/core/main.c | 9 +++++++--
> > >  include/linux/mhi.h         | 2 +-
> > >  3 files changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > > index c81b377fca8f..444676034bf0 100644
> > > --- a/drivers/bus/mhi/core/init.c
> > > +++ b/drivers/bus/mhi/core/init.c
> > > @@ -673,8 +673,7 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
> > >  				&mhi_cntrl->mhi_chan[mhi_event->chan];
> > >  		}
> > >  
> > > -		/* Priority is fixed to 1 for now */
> > > -		mhi_event->priority = 1;
> > > +		mhi_event->priority = event_cfg->priority;
> > >  
> > >  		mhi_event->db_cfg.brstmode = event_cfg->mode;
> > >  		if (MHI_INVALID_BRSTMODE(mhi_event->db_cfg.brstmode))
> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > index 8ac73f9e92a6..3775c77dec63 100644
> > > --- a/drivers/bus/mhi/core/main.c
> > > +++ b/drivers/bus/mhi/core/main.c
> > > @@ -454,10 +454,15 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
> > >  
> > >  		if (mhi_dev)
> > >  			mhi_notify(mhi_dev, MHI_CB_PENDING_DATA);
> > > -	} else {
> > > -		tasklet_schedule(&mhi_event->task);
> > > +
> > > +		return IRQ_HANDLED;
> > >  	}
> > >  
> > > +	if (!mhi_event->priority)
> > > +		tasklet_hi_schedule(&mhi_event->task);
> > > +	else
> > > +		tasklet_schedule(&mhi_event->task);
> > > +
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> > > index 86cea5256e3c..bf23c213429c 100644
> > > --- a/include/linux/mhi.h
> > > +++ b/include/linux/mhi.h
> > > @@ -250,7 +250,7 @@ struct mhi_channel_config {
> > >   * @irq_moderation_ms: Delay irq for additional events to be aggregated
> > >   * @irq: IRQ associated with this ring
> > >   * @channel: Dedicated channel number. U32_MAX indicates a non-dedicated ring
> > > - * @priority: Priority of this ring. Use 1 for now
> > > + * @priority: Processing priority of this ring. 0 is high and 1 is regular
> > 
> > Why is 0 high and 1 low?  Does that feel backwards?
> > 
> 
> That's because, "1" was used from the beginning by the controller drivers
> as the regular priority. And I thought of using "0" as high priority so
> that we can leave the controller drivers unmodified.

There's no problem modifying everyone, how much work is that?

> > Shouldn't this be a boolean, or if not, an enumerated type so that
> > people can read the code over time?
> > 
> 
> Bhaumik proposed an enum but I wanted 0/1 so that the controller drivers
> can be untouched. Also, I don't see any immediate requirement for other
> priorities.
> 
> Will make it a bool then.

Rename it when you change it so that you know you catch all existing
users.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions
  2021-06-24 14:39       ` Greg KH
@ 2021-06-24 14:47         ` Manivannan Sadhasivam
  2021-06-24 15:27           ` Greg KH
  0 siblings, 1 reply; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-24 14:47 UTC (permalink / raw)
  To: Greg KH
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, Jeffrey Hugo

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring
  2021-06-24 14:40       ` Greg KH
@ 2021-06-24 14:50         ` Manivannan Sadhasivam
  0 siblings, 0 replies; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-24 14:50 UTC (permalink / raw)
  To: Greg KH; +Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain

On Thu, Jun 24, 2021 at 04:40:38PM +0200, Greg KH wrote:
> On Thu, Jun 24, 2021 at 07:54:53PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jun 24, 2021 at 03:53:33PM +0200, Greg KH wrote:
> > > On Mon, Jun 21, 2021 at 09:46:14PM +0530, Manivannan Sadhasivam wrote:
> > > > From: Hemant Kumar <hemantk@codeaurora.org>
> > > > 
> > > > Event ring priorities are currently set to 1 and are unused.
> > > > Default processing priority for event rings is set to regular
> > > > tasklet. Controllers can choose to use high priority tasklet
> > > > scheduling for certain event rings critical for processing such
> > > > as ones transporting control information if they wish to avoid
> > > > system scheduling delays for those packets. In order to support
> > > > these use cases, allow controllers to set event ring priority to
> > > > high.
> > > > 
> > > > Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> > > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Link: https://lore.kernel.org/r/1624053903-24653-2-git-send-email-bbhatt@codeaurora.org
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  drivers/bus/mhi/core/init.c | 3 +--
> > > >  drivers/bus/mhi/core/main.c | 9 +++++++--
> > > >  include/linux/mhi.h         | 2 +-
> > > >  3 files changed, 9 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > > > index c81b377fca8f..444676034bf0 100644
> > > > --- a/drivers/bus/mhi/core/init.c
> > > > +++ b/drivers/bus/mhi/core/init.c

[...]

> > That's because, "1" was used from the beginning by the controller drivers
> > as the regular priority. And I thought of using "0" as high priority so
> > that we can leave the controller drivers unmodified.
> 
> There's no problem modifying everyone, how much work is that?
> 

I thought of minimizing the diff if we can avoid...err

> > > Shouldn't this be a boolean, or if not, an enumerated type so that
> > > people can read the code over time?
> > > 
> > 
> > Bhaumik proposed an enum but I wanted 0/1 so that the controller drivers
> > can be untouched. Also, I don't see any immediate requirement for other
> > priorities.
> > 
> > Will make it a bool then.
> 
> Rename it when you change it so that you know you catch all existing
> users.
> 

Okay. Bhaumik, can you please use the enum (as you did)?

Thanks,
Mani

> thanks,
> 
> greg k-h

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions
  2021-06-24 14:47         ` Manivannan Sadhasivam
@ 2021-06-24 15:27           ` Greg KH
  2021-06-24 15:56             ` Manivannan Sadhasivam
  0 siblings, 1 reply; 27+ messages in thread
From: Greg KH @ 2021-06-24 15:27 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, Jeffrey Hugo

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag
  2021-06-24 13:51   ` Greg KH
@ 2021-06-24 15:39     ` Loic Poulain
  2021-06-24 16:48       ` Greg KH
  0 siblings, 1 reply; 27+ messages in thread
From: Loic Poulain @ 2021-06-24 15:39 UTC (permalink / raw)
  To: Greg KH
  Cc: Manivannan Sadhasivam, Hemant Kumar, Bhaumik Bhatt,
	linux-arm-msm, open list, Jakub Kicinski

Hi Greg,

On Thu, 24 Jun 2021 at 15:51, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 21, 2021 at 09:46:12PM +0530, Manivannan Sadhasivam wrote:
> > From: Loic Poulain <loic.poulain@linaro.org>
> >
> > Currently, the MHI controller driver defines which channels should
> > have their inbound buffers allocated and queued. But ideally, this is
> > something that should be decided by the MHI device driver instead,
> > which actually deals with that buffers.
> >
> > Add a flag parameter to mhi_prepare_for_transfer allowing to specify
> > if buffers have to be allocated and queued by the MHI stack.
> >
> > Keep auto_queue flag for now, but should be removed at some point.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Acked-by: Jakub Kicinski <kuba@kernel.org>
> > Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
[...]
> > +/**
> > + * enum mhi_chan_flags - MHI channel flags
> > + * @MHI_CH_INBOUND_ALLOC_BUFS: Automatically allocate and queue inbound buffers
> > + */
> > +enum mhi_chan_flags {
> > +     MHI_CH_INBOUND_ALLOC_BUFS = BIT(0),
>
> Why is an enumerated type a bitfield?
>
> Please just use integers for enumerated types.

This 'trick' for listing flags is used in other places like drm,
mac80211, etc...: grep -r "BIT(0)," ./include/

I don't understand why it would not be right? should we simply use
a list of defines for this?

Regards,
Loic

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions
  2021-06-24 15:27           ` Greg KH
@ 2021-06-24 15:56             ` Manivannan Sadhasivam
  0 siblings, 0 replies; 27+ messages in thread
From: Manivannan Sadhasivam @ 2021-06-24 15:56 UTC (permalink / raw)
  To: Greg KH
  Cc: hemantk, bbhatt, linux-arm-msm, linux-kernel, loic.poulain,
	stable, Jeffrey Hugo

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag
  2021-06-24 15:39     ` Loic Poulain
@ 2021-06-24 16:48       ` Greg KH
  2021-06-24 19:01         ` Loic Poulain
  0 siblings, 1 reply; 27+ messages in thread
From: Greg KH @ 2021-06-24 16:48 UTC (permalink / raw)
  To: Loic Poulain
  Cc: Manivannan Sadhasivam, Hemant Kumar, Bhaumik Bhatt,
	linux-arm-msm, open list, Jakub Kicinski

On Thu, Jun 24, 2021 at 05:39:58PM +0200, Loic Poulain wrote:
> Hi Greg,
> 
> On Thu, 24 Jun 2021 at 15:51, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jun 21, 2021 at 09:46:12PM +0530, Manivannan Sadhasivam wrote:
> > > From: Loic Poulain <loic.poulain@linaro.org>
> > >
> > > Currently, the MHI controller driver defines which channels should
> > > have their inbound buffers allocated and queued. But ideally, this is
> > > something that should be decided by the MHI device driver instead,
> > > which actually deals with that buffers.
> > >
> > > Add a flag parameter to mhi_prepare_for_transfer allowing to specify
> > > if buffers have to be allocated and queued by the MHI stack.
> > >
> > > Keep auto_queue flag for now, but should be removed at some point.
> > >
> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Acked-by: Jakub Kicinski <kuba@kernel.org>
> > > Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> [...]
> > > +/**
> > > + * enum mhi_chan_flags - MHI channel flags
> > > + * @MHI_CH_INBOUND_ALLOC_BUFS: Automatically allocate and queue inbound buffers
> > > + */
> > > +enum mhi_chan_flags {
> > > +     MHI_CH_INBOUND_ALLOC_BUFS = BIT(0),
> >
> > Why is an enumerated type a bitfield?
> >
> > Please just use integers for enumerated types.
> 
> This 'trick' for listing flags is used in other places like drm,
> mac80211, etc...: grep -r "BIT(0)," ./include/

An enum is a list of values that are unique.  Not values you can mush
together into a single variable and look at the bit masks of.

> I don't understand why it would not be right? should we simply use
> a list of defines for this?

What are you using this for?  If you are going to combine them, then
yes, use #defines.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag
  2021-06-24 16:48       ` Greg KH
@ 2021-06-24 19:01         ` Loic Poulain
  0 siblings, 0 replies; 27+ messages in thread
From: Loic Poulain @ 2021-06-24 19:01 UTC (permalink / raw)
  To: Greg KH
  Cc: Manivannan Sadhasivam, Hemant Kumar, Bhaumik Bhatt,
	linux-arm-msm, open list, Jakub Kicinski

On Thu, 24 Jun 2021 at 18:48, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jun 24, 2021 at 05:39:58PM +0200, Loic Poulain wrote:
> > Hi Greg,
> >
> > On Thu, 24 Jun 2021 at 15:51, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jun 21, 2021 at 09:46:12PM +0530, Manivannan Sadhasivam wrote:
> > > > From: Loic Poulain <loic.poulain@linaro.org>
> > > >
> > > > Currently, the MHI controller driver defines which channels should
> > > > have their inbound buffers allocated and queued. But ideally, this is
> > > > something that should be decided by the MHI device driver instead,
> > > > which actually deals with that buffers.
> > > >
> > > > Add a flag parameter to mhi_prepare_for_transfer allowing to specify
> > > > if buffers have to be allocated and queued by the MHI stack.
> > > >
> > > > Keep auto_queue flag for now, but should be removed at some point.
> > > >
> > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > > Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Acked-by: Jakub Kicinski <kuba@kernel.org>
> > > > Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > [...]
> > > > +/**
> > > > + * enum mhi_chan_flags - MHI channel flags
> > > > + * @MHI_CH_INBOUND_ALLOC_BUFS: Automatically allocate and queue inbound buffers
> > > > + */
> > > > +enum mhi_chan_flags {
> > > > +     MHI_CH_INBOUND_ALLOC_BUFS = BIT(0),
> > >
> > > Why is an enumerated type a bitfield?
> > >
> > > Please just use integers for enumerated types.
> >
> > This 'trick' for listing flags is used in other places like drm,
> > mac80211, etc...: grep -r "BIT(0)," ./include/
>
> An enum is a list of values that are unique.  Not values you can mush
> together into a single variable and look at the bit masks of.
>
> > I don't understand why it would not be right? should we simply use
> > a list of defines for this?
>
> What are you using this for?  If you are going to combine them, then
> yes, use #defines.

Yes, it's for combining, thanks for the clarification.
Loic

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2021-06-24 18:52 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-21 16:16 [PATCH 0/8] MHI patches for v5.14 Manivannan Sadhasivam
2021-06-21 16:16 ` [PATCH 1/8] bus: mhi: core: Validate channel ID when processing command completions Manivannan Sadhasivam
2021-06-24 13:50   ` Greg KH
2021-06-24 14:32     ` Manivannan Sadhasivam
2021-06-24 14:39       ` Greg KH
2021-06-24 14:47         ` Manivannan Sadhasivam
2021-06-24 15:27           ` Greg KH
2021-06-24 15:56             ` Manivannan Sadhasivam
2021-06-21 16:16 ` [PATCH 2/8] bus: mhi: core: Fix power down latency Manivannan Sadhasivam
2021-06-21 16:16 ` [PATCH 3/8] bus: mhi: Wait for M2 state during system resume Manivannan Sadhasivam
2021-06-21 16:16 ` [PATCH 4/8] bus: mhi: Add inbound buffers allocation flag Manivannan Sadhasivam
2021-06-24 13:51   ` Greg KH
2021-06-24 15:39     ` Loic Poulain
2021-06-24 16:48       ` Greg KH
2021-06-24 19:01         ` Loic Poulain
2021-06-21 16:16 ` [PATCH 5/8] bus: mhi: pci-generic: Add missing 'pci_disable_pcie_error_reporting()' calls Manivannan Sadhasivam
2021-06-21 16:16 ` [PATCH 6/8] bus: mhi: core: Add support for processing priority of event ring Manivannan Sadhasivam
2021-06-24 13:53   ` Greg KH
2021-06-24 14:24     ` Manivannan Sadhasivam
2021-06-24 14:40       ` Greg KH
2021-06-24 14:50         ` Manivannan Sadhasivam
2021-06-21 16:16 ` [PATCH 7/8] bus: mhi: pci_generic: Apply no-op for wake using sideband wake boolean Manivannan Sadhasivam
2021-06-24 13:54   ` Greg KH
2021-06-21 16:16 ` [PATCH 8/8] bus: mhi: pci_generic: Add Cinterion MV31-W PCIe to MHI Manivannan Sadhasivam
2021-06-24 13:54   ` Greg KH
2021-06-24 13:54 ` [PATCH 0/8] MHI patches for v5.14 Greg KH
2021-06-24 14:20   ` Manivannan Sadhasivam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).