[PATCH net-next 0/6] net: ipa: GSI interrupt updates

[PATCH net-next 0/6] net: ipa: GSI interrupt updates

[Alex Elder]

This series implements some updates for the GSI interrupt code,
buliding on some bug fixes implemented last month.

The first two are simple changes made to improve readability and
consistency.  The third replaces all msleep() calls with comparable
usleep_range() calls.

The remainder make some more substantive changes to make the code
align with recommendations from Qualcomm.  The fourth implements a
much shorter timeout for completion GSI commands, and the fifth
implements a longer delay between retries of the STOP channel
command.  Finally, the last implements retries for stopping TX
channels (in addition to RX channels).

					-Alex

Alex Elder (6):
  net: ipa: a few simple renames
  net: ipa: introduce some interrupt helpers
  net: ipa: use usleep_range()
  net: ipa: change GSI command timeout
  net: ipa: change stop channel retry delay
  net: ipa: retry TX channel stop commands

 drivers/net/ipa/gsi.c          | 140 +++++++++++++++++++--------------
 drivers/net/ipa/ipa_endpoint.c |   4 +-
 2 files changed, 83 insertions(+), 61 deletions(-)

-- 
2.20.1

[PATCH net-next 1/6] net: ipa: a few simple renames

[Alex Elder]

The return value of gsi_command() is true if successful or false if
we time out waiting for a completion interrupt.

Rename the variables in the three callers of gsi_command() to be
"timeout", to make it more obvious that's the only reason for
failure.

In addition, add a "gsi_" prefix to evt_ring_command() so its name
is consistent with the convention used for GSI channel and generic
commands.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 14d9a791924bf..b5913ce0bc943 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -326,13 +326,13 @@ gsi_evt_ring_state(struct gsi *gsi, u32 evt_ring_id)
 }
 
 /* Issue an event ring command and wait for it to complete */
-static void evt_ring_command(struct gsi *gsi, u32 evt_ring_id,
-			     enum gsi_evt_cmd_opcode opcode)
+static void gsi_evt_ring_command(struct gsi *gsi, u32 evt_ring_id,
+				 enum gsi_evt_cmd_opcode opcode)
 {
 	struct gsi_evt_ring *evt_ring = &gsi->evt_ring[evt_ring_id];
 	struct completion *completion = &evt_ring->completion;
 	struct device *dev = gsi->dev;
-	bool success;
+	bool timeout;
 	u32 val;
 
 	/* We only perform one event ring command at a time, and event
@@ -354,13 +354,13 @@ static void evt_ring_command(struct gsi *gsi, u32 evt_ring_id,
 	val = u32_encode_bits(evt_ring_id, EV_CHID_FMASK);
 	val |= u32_encode_bits(opcode, EV_OPCODE_FMASK);
 
-	success = gsi_command(gsi, GSI_EV_CH_CMD_OFFSET, val, completion);
+	timeout = !gsi_command(gsi, GSI_EV_CH_CMD_OFFSET, val, completion);
 
 	/* Disable the interrupt again */
 	gsi_irq_type_disable(gsi, GSI_EV_CTRL);
 	iowrite32(0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET);
 
-	if (success)
+	if (!timeout)
 		return;
 
 	dev_err(dev, "GSI command %u for event ring %u timed out, state %u\n",
@@ -380,7 +380,7 @@ static int gsi_evt_ring_alloc_command(struct gsi *gsi, u32 evt_ring_id)
 		return -EINVAL;
 	}
 
-	evt_ring_command(gsi, evt_ring_id, GSI_EVT_ALLOCATE);
+	gsi_evt_ring_command(gsi, evt_ring_id, GSI_EVT_ALLOCATE);
 
 	/* If successful the event ring state will have changed */
 	if (evt_ring->state == GSI_EVT_RING_STATE_ALLOCATED)
@@ -405,7 +405,7 @@ static void gsi_evt_ring_reset_command(struct gsi *gsi, u32 evt_ring_id)
 		return;
 	}
 
-	evt_ring_command(gsi, evt_ring_id, GSI_EVT_RESET);
+	gsi_evt_ring_command(gsi, evt_ring_id, GSI_EVT_RESET);
 
 	/* If successful the event ring state will have changed */
 	if (evt_ring->state == GSI_EVT_RING_STATE_ALLOCATED)
@@ -426,7 +426,7 @@ static void gsi_evt_ring_de_alloc_command(struct gsi *gsi, u32 evt_ring_id)
 		return;
 	}
 
-	evt_ring_command(gsi, evt_ring_id, GSI_EVT_DE_ALLOC);
+	gsi_evt_ring_command(gsi, evt_ring_id, GSI_EVT_DE_ALLOC);
 
 	/* If successful the event ring state will have changed */
 	if (evt_ring->state == GSI_EVT_RING_STATE_NOT_ALLOCATED)
@@ -456,7 +456,7 @@ gsi_channel_command(struct gsi_channel *channel, enum gsi_ch_cmd_opcode opcode)
 	u32 channel_id = gsi_channel_id(channel);
 	struct gsi *gsi = channel->gsi;
 	struct device *dev = gsi->dev;
-	bool success;
+	bool timeout;
 	u32 val;
 
 	/* We only perform one channel command at a time, and channel
@@ -477,13 +477,13 @@ gsi_channel_command(struct gsi_channel *channel, enum gsi_ch_cmd_opcode opcode)
 
 	val = u32_encode_bits(channel_id, CH_CHID_FMASK);
 	val |= u32_encode_bits(opcode, CH_OPCODE_FMASK);
-	success = gsi_command(gsi, GSI_CH_CMD_OFFSET, val, completion);
+	timeout = !gsi_command(gsi, GSI_CH_CMD_OFFSET, val, completion);
 
 	/* Disable the interrupt again */
 	gsi_irq_type_disable(gsi, GSI_CH_CTRL);
 	iowrite32(0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET);
 
-	if (success)
+	if (!timeout)
 		return;
 
 	dev_err(dev, "GSI command %u for channel %u timed out, state %u\n",
@@ -1627,7 +1627,7 @@ static int gsi_generic_command(struct gsi *gsi, u32 channel_id,
 			       enum gsi_generic_cmd_opcode opcode)
 {
 	struct completion *completion = &gsi->completion;
-	bool success;
+	bool timeout;
 	u32 val;
 
 	/* The error global interrupt type is always enabled (until we
@@ -1650,12 +1650,12 @@ static int gsi_generic_command(struct gsi *gsi, u32 channel_id,
 	val |= u32_encode_bits(channel_id, GENERIC_CHID_FMASK);
 	val |= u32_encode_bits(GSI_EE_MODEM, GENERIC_EE_FMASK);
 
-	success = gsi_command(gsi, GSI_GENERIC_CMD_OFFSET, val, completion);
+	timeout = !gsi_command(gsi, GSI_GENERIC_CMD_OFFSET, val, completion);
 
 	/* Disable the GP_INT1 IRQ type again */
 	iowrite32(BIT(ERROR_INT), gsi->virt + GSI_CNTXT_GLOB_IRQ_EN_OFFSET);
 
-	if (success)
+	if (!timeout)
 		return gsi->result;
 
 	dev_err(gsi->dev, "GSI generic command %u to channel %u timed out\n",
-- 
2.20.1

[PATCH net-next 2/6] net: ipa: introduce some interrupt helpers

[Alex Elder]

Create a new function gsi_irq_ev_ctrl_enable() that encapsulates
enabling the event ring control GSI interrupt type, and enables a
single event ring to signal that interrupt.  When an event ring
changes state as a result of an event ring command, it triggers this
interrupt.

Create an inverse function gsi_irq_ev_ctrl_disable() as well.
Because only one event ring at a time is enabled for this interrupt,
we can simply disable the interrupt for *all* channels.

Create a pair of helpers that serve the same purpose for channel
commands.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c | 94 ++++++++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index b5913ce0bc943..e5681a39b5fc6 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -220,6 +220,58 @@ static void gsi_irq_teardown(struct gsi *gsi)
 	/* Nothing to do */
 }
 
+/* Event ring commands are performed one at a time.  Their completion
+ * is signaled by the event ring control GSI interrupt type, which is
+ * only enabled when we issue an event ring command.  Only the event
+ * ring being operated on has this interrupt enabled.
+ */
+static void gsi_irq_ev_ctrl_enable(struct gsi *gsi, u32 evt_ring_id)
+{
+	u32 val = BIT(evt_ring_id);
+
+	/* There's a small chance that a previous command completed
+	 * after the interrupt was disabled, so make sure we have no
+	 * pending interrupts before we enable them.
+	 */
+	iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_CLR_OFFSET);
+
+	iowrite32(val, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET);
+	gsi_irq_type_enable(gsi, GSI_EV_CTRL);
+}
+
+/* Disable event ring control interrupts */
+static void gsi_irq_ev_ctrl_disable(struct gsi *gsi)
+{
+	gsi_irq_type_disable(gsi, GSI_EV_CTRL);
+	iowrite32(0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET);
+}
+
+/* Channel commands are performed one at a time.  Their completion is
+ * signaled by the channel control GSI interrupt type, which is only
+ * enabled when we issue a channel command.  Only the channel being
+ * operated on has this interrupt enabled.
+ */
+static void gsi_irq_ch_ctrl_enable(struct gsi *gsi, u32 channel_id)
+{
+	u32 val = BIT(channel_id);
+
+	/* There's a small chance that a previous command completed
+	 * after the interrupt was disabled, so make sure we have no
+	 * pending interrupts before we enable them.
+	 */
+	iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_CLR_OFFSET);
+
+	iowrite32(val, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET);
+	gsi_irq_type_enable(gsi, GSI_CH_CTRL);
+}
+
+/* Disable channel control interrupts */
+static void gsi_irq_ch_ctrl_disable(struct gsi *gsi)
+{
+	gsi_irq_type_disable(gsi, GSI_CH_CTRL);
+	iowrite32(0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET);
+}
+
 static void gsi_irq_ieob_enable(struct gsi *gsi, u32 evt_ring_id)
 {
 	bool enable_ieob = !gsi->ieob_enabled_bitmap;
@@ -335,30 +387,15 @@ static void gsi_evt_ring_command(struct gsi *gsi, u32 evt_ring_id,
 	bool timeout;
 	u32 val;
 
-	/* We only perform one event ring command at a time, and event
-	 * control interrupts should only occur when such a command
-	 * is issued here.  Only permit *this* event ring to trigger
-	 * an interrupt, and only enable the event control IRQ type
-	 * when we expect it to occur.
-	 *
-	 * There's a small chance that a previous command completed
-	 * after the interrupt was disabled, so make sure we have no
-	 * pending interrupts before we enable them.
-	 */
-	iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_CLR_OFFSET);
-
-	val = BIT(evt_ring_id);
-	iowrite32(val, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET);
-	gsi_irq_type_enable(gsi, GSI_EV_CTRL);
+	/* Enable the completion interrupt for the command */
+	gsi_irq_ev_ctrl_enable(gsi, evt_ring_id);
 
 	val = u32_encode_bits(evt_ring_id, EV_CHID_FMASK);
 	val |= u32_encode_bits(opcode, EV_OPCODE_FMASK);
 
 	timeout = !gsi_command(gsi, GSI_EV_CH_CMD_OFFSET, val, completion);
 
-	/* Disable the interrupt again */
-	gsi_irq_type_disable(gsi, GSI_EV_CTRL);
-	iowrite32(0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET);
+	gsi_irq_ev_ctrl_disable(gsi);
 
 	if (!timeout)
 		return;
@@ -459,29 +496,14 @@ gsi_channel_command(struct gsi_channel *channel, enum gsi_ch_cmd_opcode opcode)
 	bool timeout;
 	u32 val;
 
-	/* We only perform one channel command at a time, and channel
-	 * control interrupts should only occur when such a command is
-	 * issued here.  So we only permit *this* channel to trigger
-	 * an interrupt and only enable the channel control IRQ type
-	 * when we expect it to occur.
-	 *
-	 * There's a small chance that a previous command completed
-	 * after the interrupt was disabled, so make sure we have no
-	 * pending interrupts before we enable them.
-	 */
-	iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_CLR_OFFSET);
-
-	val = BIT(channel_id);
-	iowrite32(val, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET);
-	gsi_irq_type_enable(gsi, GSI_CH_CTRL);
+	/* Enable the completion interrupt for the command */
+	gsi_irq_ch_ctrl_enable(gsi, channel_id);
 
 	val = u32_encode_bits(channel_id, CH_CHID_FMASK);
 	val |= u32_encode_bits(opcode, CH_OPCODE_FMASK);
 	timeout = !gsi_command(gsi, GSI_CH_CMD_OFFSET, val, completion);
 
-	/* Disable the interrupt again */
-	gsi_irq_type_disable(gsi, GSI_CH_CTRL);
-	iowrite32(0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET);
+	gsi_irq_ch_ctrl_disable(gsi);
 
 	if (!timeout)
 		return;
-- 
2.20.1

[PATCH net-next 3/6] net: ipa: use usleep_range()

[Alex Elder]

The use of msleep() for small periods (less than 20 milliseconds) is
not recommended because the actual delay can be much different than
expected.

We use msleep(1) in several places in the IPA driver to insert short
delays.  Replace them with usleep_range calls, which should reliably
delay a period in the range requested.

Fixes: 650d1603825d8 ("soc: qcom: ipa: the generic software interface")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c          | 5 +++--
 drivers/net/ipa/ipa_endpoint.c | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index e5681a39b5fc6..93b143ba92be5 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -611,7 +611,8 @@ static void gsi_channel_reset_command(struct gsi_channel *channel)
 	struct device *dev = channel->gsi->dev;
 	enum gsi_channel_state state;
 
-	msleep(1);	/* A short delay is required before a RESET command */
+	/* A short delay is required before a RESET command */
+	usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
 
 	state = gsi_channel_state(channel);
 	if (state != GSI_CHANNEL_STATE_STOPPED &&
@@ -900,7 +901,7 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id)
 		ret = gsi_channel_stop_command(channel);
 		if (ret != -EAGAIN)
 			break;
-		msleep(1);
+		usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
 	} while (retries--);
 
 	mutex_unlock(&gsi->mutex);
diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 9f4be9812a1f3..688a3dd40510a 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -1378,7 +1378,7 @@ static int ipa_endpoint_reset_rx_aggr(struct ipa_endpoint *endpoint)
 	do {
 		if (!ipa_endpoint_aggr_active(endpoint))
 			break;
-		msleep(1);
+		usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
 	} while (retries--);
 
 	/* Check one last time */
@@ -1399,7 +1399,7 @@ static int ipa_endpoint_reset_rx_aggr(struct ipa_endpoint *endpoint)
 	 */
 	gsi_channel_reset(gsi, endpoint->channel_id, true);
 
-	msleep(1);
+	usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
 
 	goto out_suspend_again;
 
-- 
2.20.1

[PATCH net-next 4/6] net: ipa: change GSI command timeout

[Alex Elder]

The GSI command timeout is currently 5 seconds, which is much higher
than it should be.

Express the timeout in milliseconds rather than seconds, and reduce
it to 50 milliseconds.

Fixes: 650d1603825d8 ("soc: qcom: ipa: the generic software interface")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 93b143ba92be5..4de769166978b 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -89,7 +89,7 @@
 /* Delay period for interrupt moderation (in 32KHz IPA internal timer ticks) */
 #define GSI_EVT_RING_INT_MODT		(32 * 1) /* 1ms under 32KHz clock */
 
-#define GSI_CMD_TIMEOUT			5	/* seconds */
+#define GSI_CMD_TIMEOUT			50	/* milliseconds */
 
 #define GSI_CHANNEL_STOP_RX_RETRIES	10
 #define GSI_CHANNEL_MODEM_HALT_RETRIES	10
@@ -359,11 +359,13 @@ static u32 gsi_ring_index(struct gsi_ring *ring, u32 offset)
 static bool
 gsi_command(struct gsi *gsi, u32 reg, u32 val, struct completion *completion)
 {
+	unsigned long timeout = msecs_to_jiffies(GSI_CMD_TIMEOUT);
+
 	reinit_completion(completion);
 
 	iowrite32(val, gsi->virt + reg);
 
-	return !!wait_for_completion_timeout(completion, GSI_CMD_TIMEOUT * HZ);
+	return !!wait_for_completion_timeout(completion, timeout);
 }
 
 /* Return the hardware's notion of the current state of an event ring */
-- 
2.20.1

[PATCH net-next 5/6] net: ipa: change stop channel retry delay

[Alex Elder]

If a GSI stop channel command leaves the channel in STOP_IN_PROC
state, we retry the stop command after a 1-2 millisecond delay.

I have been told that a 3-5 millisecond delay is a better choice.

Fixes: 650d1603825d8 ("soc: qcom: ipa: the generic software interface")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 4de769166978b..5c163f9c0ea7b 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -903,7 +903,7 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id)
 		ret = gsi_channel_stop_command(channel);
 		if (ret != -EAGAIN)
 			break;
-		usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC);
+		usleep_range(3 * USEC_PER_MSEC, 5 * USEC_PER_MSEC);
 	} while (retries--);
 
 	mutex_unlock(&gsi->mutex);
-- 
2.20.1

[PATCH net-next 6/6] net: ipa: retry TX channel stop commands

[Alex Elder]

For RX channels we issue a stop command more than once if necessary
to allow it to stop.  It turns out that TX channels could also
require retries.

Retry channel stop commands if necessary regardless of the channel
direction.  Rename the symbol defining the retry count so it's not
RX-specific.

Fixes: 650d1603825d8 ("soc: qcom: ipa: the generic software interface")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 5c163f9c0ea7b..5b29f7d9d6ac1 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -91,7 +91,7 @@
 
 #define GSI_CMD_TIMEOUT			50	/* milliseconds */
 
-#define GSI_CHANNEL_STOP_RX_RETRIES	10
+#define GSI_CHANNEL_STOP_RETRIES	10
 #define GSI_CHANNEL_MODEM_HALT_RETRIES	10
 
 #define GSI_MHI_EVENT_ID_START		10	/* 1st reserved event id */
@@ -889,14 +889,11 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id)
 int gsi_channel_stop(struct gsi *gsi, u32 channel_id)
 {
 	struct gsi_channel *channel = &gsi->channel[channel_id];
-	u32 retries;
+	u32 retries = GSI_CHANNEL_STOP_RETRIES;
 	int ret;
 
 	gsi_channel_freeze(channel);
 
-	/* RX channels might require a little time to enter STOPPED state */
-	retries = channel->toward_ipa ? 0 : GSI_CHANNEL_STOP_RX_RETRIES;
-
 	mutex_lock(&gsi->mutex);
 
 	do {
-- 
2.20.1

Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates

[Saeed Mahameed]

On Wed, 2021-01-13 at 11:15 -0600, Alex Elder wrote:
> This series implements some updates for the GSI interrupt code,
> buliding on some bug fixes implemented last month.
> 
> The first two are simple changes made to improve readability and
> consistency.  The third replaces all msleep() calls with comparable
> usleep_range() calls.
> 
> The remainder make some more substantive changes to make the code
> align with recommendations from Qualcomm.  The fourth implements a
> much shorter timeout for completion GSI commands, and the fifth
> implements a longer delay between retries of the STOP channel
> command.  Finally, the last implements retries for stopping TX
> channels (in addition to RX channels).
> 
> 					-Alex
> 

A minor thing that bothers me about this series is that it looks like
it is based on magic numbers and some redefined constant values
according to some mysterious sources ;-) .. It would be nice to have
some wording in the commit messages explaining reasoning and maybe
"semi-official" sources behind the changes.

LGMT code style wise :) 

Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>


> Alex Elder (6):
>   net: ipa: a few simple renames
>   net: ipa: introduce some interrupt helpers
>   net: ipa: use usleep_range()
>   net: ipa: change GSI command timeout
>   net: ipa: change stop channel retry delay
>   net: ipa: retry TX channel stop commands
> 
>  drivers/net/ipa/gsi.c          | 140 +++++++++++++++++++----------
> ----
>  drivers/net/ipa/ipa_endpoint.c |   4 +-
>  2 files changed, 83 insertions(+), 61 deletions(-)
> 

Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates

[Jakub Kicinski]

On Thu, 14 Jan 2021 15:22:54 -0800 Saeed Mahameed wrote:
> On Wed, 2021-01-13 at 11:15 -0600, Alex Elder wrote:
> > This series implements some updates for the GSI interrupt code,
> > buliding on some bug fixes implemented last month.
> > 
> > The first two are simple changes made to improve readability and
> > consistency.  The third replaces all msleep() calls with comparable
> > usleep_range() calls.
> > 
> > The remainder make some more substantive changes to make the code
> > align with recommendations from Qualcomm.  The fourth implements a
> > much shorter timeout for completion GSI commands, and the fifth
> > implements a longer delay between retries of the STOP channel
> > command.  Finally, the last implements retries for stopping TX
> > channels (in addition to RX channels).
>
> A minor thing that bothers me about this series is that it looks like
> it is based on magic numbers and some redefined constant values
> according to some mysterious sources ;-) .. It would be nice to have
> some wording in the commit messages explaining reasoning and maybe
> "semi-official" sources behind the changes.
> 
> LGMT code style wise :) 
> 
> Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>

Dropped the fixes tags (since its not a series of fixes) and applied.

Thanks!

Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates

[Alex Elder]

On 1/14/21 8:08 PM, Jakub Kicinski wrote:
> Dropped the fixes tags (since its not a series of fixes) and applied.

Thanks for applying these.

Do you only want "Fixes" tags on patches posted for the net
branch (and not net-next)?

I think I might have debated sending these as bug fixes but
decided they weren't serious enough to warrant that.  Anyway
if I know it's important to *not* use that tag I can avoid
including it.

Thanks.

					-Alex

Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates

[Jakub Kicinski]

On Fri, 15 Jan 2021 04:42:14 -0600 Alex Elder wrote:
> On 1/14/21 8:08 PM, Jakub Kicinski wrote:
> > Dropped the fixes tags (since its not a series of fixes) and applied.  
> 
> Thanks for applying these.
> 
> Do you only want "Fixes" tags on patches posted for the net
> branch (and not net-next)?
> 
> I think I might have debated sending these as bug fixes but
> decided they weren't serious enough to warrant that.  Anyway
> if I know it's important to *not* use that tag I can avoid
> including it.

The point of the tag is to facilitate backporting. If something is
important enough to back port is should also be important enough 
to go to net, no?

Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates

[Alex Elder]

On 1/15/21 12:59 PM, Jakub Kicinski wrote:
> On Fri, 15 Jan 2021 04:42:14 -0600 Alex Elder wrote:
>> On 1/14/21 8:08 PM, Jakub Kicinski wrote:
>>> Dropped the fixes tags (since its not a series of fixes) and applied.
>>
>> Thanks for applying these.
>>
>> Do you only want "Fixes" tags on patches posted for the net
>> branch (and not net-next)?
>>
>> I think I might have debated sending these as bug fixes but
>> decided they weren't serious enough to warrant that.  Anyway
>> if I know it's important to *not* use that tag I can avoid
>> including it.
> 
> The point of the tag is to facilitate backporting. If something is
> important enough to back port is should also be important enough
> to go to net, no?

Yes, agreed and understood.	-Alex

Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates

[Alex Elder]

On 1/14/21 5:22 PM, Saeed Mahameed wrote:
> On Wed, 2021-01-13 at 11:15 -0600, Alex Elder wrote:
>> This series implements some updates for the GSI interrupt code,
>> buliding on some bug fixes implemented last month.
>>
>> The first two are simple changes made to improve readability and
>> consistency.  The third replaces all msleep() calls with comparable
>> usleep_range() calls.
>>
>> The remainder make some more substantive changes to make the code
>> align with recommendations from Qualcomm.  The fourth implements a
>> much shorter timeout for completion GSI commands, and the fifth
>> implements a longer delay between retries of the STOP channel
>> command.  Finally, the last implements retries for stopping TX
>> channels (in addition to RX channels).
>>
>> 					-Alex
>>
> 
> A minor thing that bothers me about this series is that it looks like
> it is based on magic numbers and some redefined constant values
> according to some mysterious sources ;-) .. It would be nice to have
> some wording in the commit messages explaining reasoning and maybe
> "semi-official" sources behind the changes.

I understand this, and agree with the sentiment.

This code is ultimately derived from code published on
codeaurora.org (CAF), but it is now quite different from
what you'll find there.

While investigating some issues recently I discovered that
the details on the retry logic and timeouts, etc. no longer
matched what I saw on CAF.  I inquired and got some updated
information, and implemented in this series what I learned.

To be honest I don't know precisely where these details
are defined.  Even if I did, it wouldn't help much,
because it would be found in an internal hardware
specification of some kind, and I have no ability to
make that public.  Still, I agree, it would be nice
to have a reference.

I would absolutely have mentioned where these magic
values came from if I could (or if I knew). As you
you noticed, the commit messages were intentionally
vague on it.

Thank you very much for the review.

					-Alex

> LGMT code style wise :)
> 
> Reviewed-by: Saeed Mahameed <saeedm@nvidia.com>
> 
> 
>> Alex Elder (6):
>>    net: ipa: a few simple renames
>>    net: ipa: introduce some interrupt helpers
>>    net: ipa: use usleep_range()
>>    net: ipa: change GSI command timeout
>>    net: ipa: change stop channel retry delay
>>    net: ipa: retry TX channel stop commands
>>
>>   drivers/net/ipa/gsi.c          | 140 +++++++++++++++++++----------
>> ----
>>   drivers/net/ipa/ipa_endpoint.c |   4 +-
>>   2 files changed, 83 insertions(+), 61 deletions(-)
>>
>