[PATCH net-next 0/7] net: ipa: a mix of small improvements

[PATCH net-next 0/7] net: ipa: a mix of small improvements

[Alex Elder]

This series contains a sort of unrelated set of code cleanups.

The first two are things I wanted to do in a series that updated
some NAPI code recently.  I didn't want to change things in a way
that affected existing testing so I set these aside for later
(i.e., now).

The third makes a change to event ring handling that's similar to
what was done a while back for channels.  There's little benefit to
cacheing the current state of an event ring, so with this we'll just
fetch the state from hardware whenever we need it.

The fourth patch removes the definitions of two unused symbols.

The fifth replaces a count that is always 0 or 1 with a Boolean.

The sixth removes a build-time validation check that doesn't really
provide benefit.

And the last one fixes a problem (in two spots) that could cause a
build-time check to fail "bogusly".

					-Alex

Alex Elder (7):
  net: ipa: restructure a few functions
  net: ipa: synchronize NAPI only for suspend
  net: ipa: do not cache event ring state
  net: ipa: remove two unused register definitions
  net: ipa: use a Boolean rather than count when replenishing
  net: ipa: get rid of status size constraint
  net: ipa: avoid field overflow

 drivers/net/ipa/gsi.c          | 94 ++++++++++++++++++----------------
 drivers/net/ipa/gsi.h          |  1 -
 drivers/net/ipa/gsi_reg.h      | 10 ----
 drivers/net/ipa/ipa_endpoint.c | 38 +++++++-------
 drivers/net/ipa/ipa_reg.h      | 22 +++++---
 5 files changed, 84 insertions(+), 81 deletions(-)

-- 
2.20.1

[PATCH net-next 1/7] net: ipa: restructure a few functions

[Alex Elder]

Make __gsi_channel_start() and __gsi_channel_stop() more structurally
and semantically similar to each other:
  - Restructure __gsi_channel_start() to always return at the end of
    the function, similar to the way __gsi_channel_stop() does.
  - Move the mutex calls out of gsi_channel_stop_retry() and into
    __gsi_channel_stop().

Restructure gsi_channel_stop() to always return at the end of the
function, like gsi_channel_start() does.

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

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 53640447bf123..2671b76ebcfe3 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -873,17 +873,17 @@ static void gsi_channel_deprogram(struct gsi_channel *channel)
 
 static int __gsi_channel_start(struct gsi_channel *channel, bool start)
 {
-	struct gsi *gsi = channel->gsi;
-	int ret;
+	int ret = 0;
 
-	if (!start)
-		return 0;
+	if (start) {
+		struct gsi *gsi = channel->gsi;
 
-	mutex_lock(&gsi->mutex);
+		mutex_lock(&gsi->mutex);
 
-	ret = gsi_channel_start_command(channel);
+		ret = gsi_channel_start_command(channel);
 
-	mutex_unlock(&gsi->mutex);
+		mutex_unlock(&gsi->mutex);
+	}
 
 	return ret;
 }
@@ -910,11 +910,8 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id)
 static int gsi_channel_stop_retry(struct gsi_channel *channel)
 {
 	u32 retries = GSI_CHANNEL_STOP_RETRIES;
-	struct gsi *gsi = channel->gsi;
 	int ret;
 
-	mutex_lock(&gsi->mutex);
-
 	do {
 		ret = gsi_channel_stop_command(channel);
 		if (ret != -EAGAIN)
@@ -922,19 +919,26 @@ static int gsi_channel_stop_retry(struct gsi_channel *channel)
 		usleep_range(3 * USEC_PER_MSEC, 5 * USEC_PER_MSEC);
 	} while (retries--);
 
-	mutex_unlock(&gsi->mutex);
-
 	return ret;
 }
 
 static int __gsi_channel_stop(struct gsi_channel *channel, bool stop)
 {
-	int ret;
+	int ret = 0;
 
 	/* Wait for any underway transactions to complete before stopping. */
 	gsi_channel_trans_quiesce(channel);
 
-	ret = stop ? gsi_channel_stop_retry(channel) : 0;
+	if (stop) {
+		struct gsi *gsi = channel->gsi;
+
+		mutex_lock(&gsi->mutex);
+
+		ret = gsi_channel_stop_retry(channel);
+
+		mutex_unlock(&gsi->mutex);
+	}
+
 	/* Finally, ensure NAPI polling has finished. */
 	if (!ret)
 		napi_synchronize(&channel->napi);
@@ -948,15 +952,14 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id)
 	struct gsi_channel *channel = &gsi->channel[channel_id];
 	int ret;
 
-	/* Only disable the completion interrupt if stop is successful */
 	ret = __gsi_channel_stop(channel, true);
-	if (ret)
-		return ret;
+	if (ret) {
+		/* Disable the completion interrupt and NAPI if successful */
+		gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id);
+		napi_disable(&channel->napi);
+	}
 
-	gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id);
-	napi_disable(&channel->napi);
-
-	return 0;
+	return ret;
 }
 
 /* Reset and reconfigure a channel, (possibly) enabling the doorbell engine */
-- 
2.20.1

[PATCH net-next 2/7] net: ipa: synchronize NAPI only for suspend

[Alex Elder]

When stopping a channel, gsi_channel_stop() will ensure NAPI
polling is complete when it calls napi_disable().  So there is no
need to call napi_synchronize() in that case.

Move the call to napi_synchronize() out of __gsi_channel_stop()
and into gsi_channel_suspend(), so it's only used where needed.

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

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 2671b76ebcfe3..420d0f3bfae9a 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -939,10 +939,6 @@ static int __gsi_channel_stop(struct gsi_channel *channel, bool stop)
 		mutex_unlock(&gsi->mutex);
 	}
 
-	/* Finally, ensure NAPI polling has finished. */
-	if (!ret)
-		napi_synchronize(&channel->napi);
-
 	return ret;
 }
 
@@ -984,8 +980,14 @@ void gsi_channel_reset(struct gsi *gsi, u32 channel_id, bool doorbell)
 int gsi_channel_suspend(struct gsi *gsi, u32 channel_id, bool stop)
 {
 	struct gsi_channel *channel = &gsi->channel[channel_id];
+	int ret;
 
-	return __gsi_channel_stop(channel, stop);
+	/* Synchronize NAPI if successful, to ensure polling has finished. */
+	ret = __gsi_channel_stop(channel, stop);
+	if (!ret)
+		napi_synchronize(&channel->napi);
+
+	return ret;
 }
 
 /* Resume a suspended channel (starting will be requested if STOPPED) */
-- 
2.20.1

[PATCH net-next 3/7] net: ipa: do not cache event ring state

[Alex Elder]

An event ring's state only needs to be known when it is allocated,
reset, or deallocated.  We check an event ring's state both before
and after performing an event ring control command that changes
its state.  These are only issued at startup and shutdown, so there
is very little value in caching the state.

Stop recording a copy of the channel's last known state, and instead
fetch the true state from hardware whenever it's needed.  In such
cases, *do* record the state in a local variable, in case an error
message reports it (so the value reported is the value seen).

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c | 39 +++++++++++++++++++++------------------
 drivers/net/ipa/gsi.h |  1 -
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 420d0f3bfae9a..0f44e374c0a7e 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -408,30 +408,31 @@ static void gsi_evt_ring_command(struct gsi *gsi, u32 evt_ring_id,
 		return;
 
 	dev_err(dev, "GSI command %u for event ring %u timed out, state %u\n",
-		opcode, evt_ring_id, evt_ring->state);
+		opcode, evt_ring_id, gsi_evt_ring_state(gsi, evt_ring_id));
 }
 
 /* Allocate an event ring in NOT_ALLOCATED state */
 static int gsi_evt_ring_alloc_command(struct gsi *gsi, u32 evt_ring_id)
 {
-	struct gsi_evt_ring *evt_ring = &gsi->evt_ring[evt_ring_id];
+	enum gsi_evt_ring_state state;
 
 	/* Get initial event ring state */
-	evt_ring->state = gsi_evt_ring_state(gsi, evt_ring_id);
-	if (evt_ring->state != GSI_EVT_RING_STATE_NOT_ALLOCATED) {
+	state = gsi_evt_ring_state(gsi, evt_ring_id);
+	if (state != GSI_EVT_RING_STATE_NOT_ALLOCATED) {
 		dev_err(gsi->dev, "event ring %u bad state %u before alloc\n",
-			evt_ring_id, evt_ring->state);
+			evt_ring_id, state);
 		return -EINVAL;
 	}
 
 	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)
+	state = gsi_evt_ring_state(gsi, evt_ring_id);
+	if (state == GSI_EVT_RING_STATE_ALLOCATED)
 		return 0;
 
 	dev_err(gsi->dev, "event ring %u bad state %u after alloc\n",
-		evt_ring_id, evt_ring->state);
+		evt_ring_id, state);
 
 	return -EIO;
 }
@@ -439,45 +440,48 @@ static int gsi_evt_ring_alloc_command(struct gsi *gsi, u32 evt_ring_id)
 /* Reset a GSI event ring in ALLOCATED or ERROR state. */
 static void gsi_evt_ring_reset_command(struct gsi *gsi, u32 evt_ring_id)
 {
-	struct gsi_evt_ring *evt_ring = &gsi->evt_ring[evt_ring_id];
-	enum gsi_evt_ring_state state = evt_ring->state;
+	enum gsi_evt_ring_state state;
 
+	state = gsi_evt_ring_state(gsi, evt_ring_id);
 	if (state != GSI_EVT_RING_STATE_ALLOCATED &&
 	    state != GSI_EVT_RING_STATE_ERROR) {
 		dev_err(gsi->dev, "event ring %u bad state %u before reset\n",
-			evt_ring_id, evt_ring->state);
+			evt_ring_id, state);
 		return;
 	}
 
 	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)
+	state = gsi_evt_ring_state(gsi, evt_ring_id);
+	if (state == GSI_EVT_RING_STATE_ALLOCATED)
 		return;
 
 	dev_err(gsi->dev, "event ring %u bad state %u after reset\n",
-		evt_ring_id, evt_ring->state);
+		evt_ring_id, state);
 }
 
 /* Issue a hardware de-allocation request for an allocated event ring */
 static void gsi_evt_ring_de_alloc_command(struct gsi *gsi, u32 evt_ring_id)
 {
-	struct gsi_evt_ring *evt_ring = &gsi->evt_ring[evt_ring_id];
+	enum gsi_evt_ring_state state;
 
-	if (evt_ring->state != GSI_EVT_RING_STATE_ALLOCATED) {
+	state = gsi_evt_ring_state(gsi, evt_ring_id);
+	if (state != GSI_EVT_RING_STATE_ALLOCATED) {
 		dev_err(gsi->dev, "event ring %u state %u before dealloc\n",
-			evt_ring_id, evt_ring->state);
+			evt_ring_id, state);
 		return;
 	}
 
 	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)
+	state = gsi_evt_ring_state(gsi, evt_ring_id);
+	if (state == GSI_EVT_RING_STATE_NOT_ALLOCATED)
 		return;
 
 	dev_err(gsi->dev, "event ring %u bad state %u after dealloc\n",
-		evt_ring_id, evt_ring->state);
+		evt_ring_id, state);
 }
 
 /* Fetch the current state of a channel from hardware */
@@ -1104,7 +1108,6 @@ static void gsi_isr_evt_ctrl(struct gsi *gsi)
 		event_mask ^= BIT(evt_ring_id);
 
 		evt_ring = &gsi->evt_ring[evt_ring_id];
-		evt_ring->state = gsi_evt_ring_state(gsi, evt_ring_id);
 
 		complete(&evt_ring->completion);
 	}
diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h
index 96c9aed397aad..d674db0ba4eb0 100644
--- a/drivers/net/ipa/gsi.h
+++ b/drivers/net/ipa/gsi.h
@@ -142,7 +142,6 @@ enum gsi_evt_ring_state {
 struct gsi_evt_ring {
 	struct gsi_channel *channel;
 	struct completion completion;	/* signals event ring state changes */
-	enum gsi_evt_ring_state state;
 	struct gsi_ring ring;
 };
 
-- 
2.20.1

[PATCH net-next 4/7] net: ipa: remove two unused register definitions

[Alex Elder]

We do not support inter-EE channel or event ring commands.  Inter-EE
interrupts are disabled (and never re-enabled) for all channels and
event rings, so we have no need for the GSI registers that clear
those interrupt conditions.  So remove their definitions.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi_reg.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/ipa/gsi_reg.h b/drivers/net/ipa/gsi_reg.h
index 0e138bbd82053..299456e70f286 100644
--- a/drivers/net/ipa/gsi_reg.h
+++ b/drivers/net/ipa/gsi_reg.h
@@ -59,16 +59,6 @@
 #define GSI_INTER_EE_N_SRC_EV_CH_IRQ_OFFSET(ee) \
 			(0x0000c01c + 0x1000 * (ee))
 
-#define GSI_INTER_EE_SRC_CH_IRQ_CLR_OFFSET \
-			GSI_INTER_EE_N_SRC_CH_IRQ_CLR_OFFSET(GSI_EE_AP)
-#define GSI_INTER_EE_N_SRC_CH_IRQ_CLR_OFFSET(ee) \
-			(0x0000c028 + 0x1000 * (ee))
-
-#define GSI_INTER_EE_SRC_EV_CH_IRQ_CLR_OFFSET \
-			GSI_INTER_EE_N_SRC_EV_CH_IRQ_CLR_OFFSET(GSI_EE_AP)
-#define GSI_INTER_EE_N_SRC_EV_CH_IRQ_CLR_OFFSET(ee) \
-			(0x0000c02c + 0x1000 * (ee))
-
 #define GSI_CH_C_CNTXT_0_OFFSET(ch) \
 		GSI_EE_N_CH_C_CNTXT_0_OFFSET((ch), GSI_EE_AP)
 #define GSI_EE_N_CH_C_CNTXT_0_OFFSET(ch, ee) \
-- 
2.20.1

[PATCH net-next 5/7] net: ipa: use a Boolean rather than count when replenishing

[Alex Elder]

The count argument to ipa_endpoint_replenish() is only ever 0 or 1,
and always will be (because we always handle each receive buffer in
a single transaction).  Rename the argument to be add_one and change
it to be Boolean.

Update the function description to reflect the current code.

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

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 7a46c790afbef..bff5d6ffd1186 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -1020,31 +1020,34 @@ static int ipa_endpoint_replenish_one(struct ipa_endpoint *endpoint)
 }
 
 /**
- * ipa_endpoint_replenish() - Replenish the Rx packets cache.
+ * ipa_endpoint_replenish() - Replenish endpoint receive buffers
  * @endpoint:	Endpoint to be replenished
- * @count:	Number of buffers to send to hardware
+ * @add_one:	Whether this is replacing a just-consumed buffer
  *
- * Allocate RX packet wrapper structures with maximal socket buffers
- * for an endpoint.  These are supplied to the hardware, which fills
- * them with incoming data.
+ * The IPA hardware can hold a fixed number of receive buffers for an RX
+ * endpoint, based on the number of entries in the underlying channel ring
+ * buffer.  If an endpoint's "backlog" is non-zero, it indicates how many
+ * more receive buffers can be supplied to the hardware.  Replenishing for
+ * an endpoint can be disabled, in which case requests to replenish a
+ * buffer are "saved", and transferred to the backlog once it is re-enabled
+ * again.
  */
-static void ipa_endpoint_replenish(struct ipa_endpoint *endpoint, u32 count)
+static void ipa_endpoint_replenish(struct ipa_endpoint *endpoint, bool add_one)
 {
 	struct gsi *gsi;
 	u32 backlog;
 
 	if (!endpoint->replenish_enabled) {
-		if (count)
-			atomic_add(count, &endpoint->replenish_saved);
+		if (add_one)
+			atomic_inc(&endpoint->replenish_saved);
 		return;
 	}
 
-
 	while (atomic_dec_not_zero(&endpoint->replenish_backlog))
 		if (ipa_endpoint_replenish_one(endpoint))
 			goto try_again_later;
-	if (count)
-		atomic_add(count, &endpoint->replenish_backlog);
+	if (add_one)
+		atomic_inc(&endpoint->replenish_backlog);
 
 	return;
 
@@ -1052,8 +1055,8 @@ static void ipa_endpoint_replenish(struct ipa_endpoint *endpoint, u32 count)
 	/* The last one didn't succeed, so fix the backlog */
 	backlog = atomic_inc_return(&endpoint->replenish_backlog);
 
-	if (count)
-		atomic_add(count, &endpoint->replenish_backlog);
+	if (add_one)
+		atomic_inc(&endpoint->replenish_backlog);
 
 	/* Whenever a receive buffer transaction completes we'll try to
 	 * replenish again.  It's unlikely, but if we fail to supply even
@@ -1080,7 +1083,7 @@ static void ipa_endpoint_replenish_enable(struct ipa_endpoint *endpoint)
 	/* Start replenishing if hardware currently has no buffers */
 	max_backlog = gsi_channel_tre_max(gsi, endpoint->channel_id);
 	if (atomic_read(&endpoint->replenish_backlog) == max_backlog)
-		ipa_endpoint_replenish(endpoint, 0);
+		ipa_endpoint_replenish(endpoint, false);
 }
 
 static void ipa_endpoint_replenish_disable(struct ipa_endpoint *endpoint)
@@ -1099,7 +1102,7 @@ static void ipa_endpoint_replenish_work(struct work_struct *work)
 
 	endpoint = container_of(dwork, struct ipa_endpoint, replenish_work);
 
-	ipa_endpoint_replenish(endpoint, 0);
+	ipa_endpoint_replenish(endpoint, false);
 }
 
 static void ipa_endpoint_skb_copy(struct ipa_endpoint *endpoint,
@@ -1300,7 +1303,7 @@ static void ipa_endpoint_rx_complete(struct ipa_endpoint *endpoint,
 {
 	struct page *page;
 
-	ipa_endpoint_replenish(endpoint, 1);
+	ipa_endpoint_replenish(endpoint, true);
 
 	if (trans->cancelled)
 		return;
-- 
2.20.1

[PATCH net-next 6/7] net: ipa: get rid of status size constraint

[Alex Elder]

There is a build-time check that the packet status structure is a
multiple of 4 bytes in size.  It's not clear where that constraint
comes from, but the structure defines what hardware provides so its
definition won't change.  Get rid of the check; it adds no value.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_endpoint.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index bff5d6ffd1186..7209ee3c31244 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -174,9 +174,6 @@ static bool ipa_endpoint_data_valid(struct ipa *ipa, u32 count,
 	enum ipa_endpoint_name name;
 	u32 limit;
 
-	/* Not sure where this constraint come from... */
-	BUILD_BUG_ON(sizeof(struct ipa_status) % 4);
-
 	if (count > IPA_ENDPOINT_COUNT) {
 		dev_err(dev, "too many endpoints specified (%u > %u)\n",
 			count, IPA_ENDPOINT_COUNT);
-- 
2.20.1

[PATCH net-next 7/7] net: ipa: avoid field overflow

[Alex Elder]

It's possible that the length passed to ipa_header_size_encoded()
is larger than what can be represented by the HDR_LEN field alone
(starting with IPA v4.5).  If we attempted that, u32_encode_bits()
would trigger a build-time error.

Avoid this problem by masking off high-order bits of the value
encoded as the lower portion of the header length.

The same sort of problem exists in ipa_metadata_offset_encoded(),
so implement the same fix there.

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

diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
index e6b0827a244ec..732e691e9aa62 100644
--- a/drivers/net/ipa/ipa_reg.h
+++ b/drivers/net/ipa/ipa_reg.h
@@ -408,15 +408,18 @@ enum ipa_cs_offload_en {
 static inline u32 ipa_header_size_encoded(enum ipa_version version,
 					  u32 header_size)
 {
+	u32 size = header_size & field_mask(HDR_LEN_FMASK);
 	u32 val;
 
-	val = u32_encode_bits(header_size, HDR_LEN_FMASK);
-	if (version < IPA_VERSION_4_5)
+	val = u32_encode_bits(size, HDR_LEN_FMASK);
+	if (version < IPA_VERSION_4_5) {
+		/* ipa_assert(header_size == size); */
 		return val;
+	}
 
 	/* IPA v4.5 adds a few more most-significant bits */
-	header_size >>= hweight32(HDR_LEN_FMASK);
-	val |= u32_encode_bits(header_size, HDR_LEN_MSB_FMASK);
+	size = header_size >> hweight32(HDR_LEN_FMASK);
+	val |= u32_encode_bits(size, HDR_LEN_MSB_FMASK);
 
 	return val;
 }
@@ -425,15 +428,18 @@ static inline u32 ipa_header_size_encoded(enum ipa_version version,
 static inline u32 ipa_metadata_offset_encoded(enum ipa_version version,
 					      u32 offset)
 {
+	u32 off = offset & field_mask(HDR_OFST_METADATA_FMASK);
 	u32 val;
 
-	val = u32_encode_bits(offset, HDR_OFST_METADATA_FMASK);
-	if (version < IPA_VERSION_4_5)
+	val = u32_encode_bits(off, HDR_OFST_METADATA_FMASK);
+	if (version < IPA_VERSION_4_5) {
+		/* ipa_assert(offset == off); */
 		return val;
+	}
 
 	/* IPA v4.5 adds a few more most-significant bits */
-	offset >>= hweight32(HDR_OFST_METADATA_FMASK);
-	val |= u32_encode_bits(offset, HDR_OFST_METADATA_MSB_FMASK);
+	off = offset >> hweight32(HDR_OFST_METADATA_FMASK);
+	val |= u32_encode_bits(off, HDR_OFST_METADATA_MSB_FMASK);
 
 	return val;
 }
-- 
2.20.1

Re: [PATCH net-next 1/7] net: ipa: restructure a few functions

[Jakub Kicinski]

On Wed,  3 Feb 2021 09:28:49 -0600 Alex Elder wrote:
> Make __gsi_channel_start() and __gsi_channel_stop() more structurally
> and semantically similar to each other:
>   - Restructure __gsi_channel_start() to always return at the end of
>     the function, similar to the way __gsi_channel_stop() does.
>   - Move the mutex calls out of gsi_channel_stop_retry() and into
>     __gsi_channel_stop().
> 
> Restructure gsi_channel_stop() to always return at the end of the
> function, like gsi_channel_start() does.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/gsi.c | 45 +++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
> index 53640447bf123..2671b76ebcfe3 100644
> --- a/drivers/net/ipa/gsi.c
> +++ b/drivers/net/ipa/gsi.c
> @@ -873,17 +873,17 @@ static void gsi_channel_deprogram(struct gsi_channel *channel)
>  
>  static int __gsi_channel_start(struct gsi_channel *channel, bool start)
>  {
> -	struct gsi *gsi = channel->gsi;
> -	int ret;
> +	int ret = 0;
>  
> -	if (!start)
> -		return 0;
> +	if (start) {
> +		struct gsi *gsi = channel->gsi;
>  
> -	mutex_lock(&gsi->mutex);
> +		mutex_lock(&gsi->mutex);
>  
> -	ret = gsi_channel_start_command(channel);
> +		ret = gsi_channel_start_command(channel);
>  
> -	mutex_unlock(&gsi->mutex);
> +		mutex_unlock(&gsi->mutex);
> +	}

nit: I thought just recently Willem pointed out that keeping main flow
     unindented is considered good style, maybe it doesn't apply here
     perfectly, but I'd think it still applies. Why have the entire
     body of the function indented?

>  	return ret;
>  }
> @@ -910,11 +910,8 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id)
>  static int gsi_channel_stop_retry(struct gsi_channel *channel)
>  {
>  	u32 retries = GSI_CHANNEL_STOP_RETRIES;
> -	struct gsi *gsi = channel->gsi;
>  	int ret;
>  
> -	mutex_lock(&gsi->mutex);
> -
>  	do {
>  		ret = gsi_channel_stop_command(channel);
>  		if (ret != -EAGAIN)
> @@ -922,19 +919,26 @@ static int gsi_channel_stop_retry(struct gsi_channel *channel)
>  		usleep_range(3 * USEC_PER_MSEC, 5 * USEC_PER_MSEC);
>  	} while (retries--);
>  
> -	mutex_unlock(&gsi->mutex);
> -
>  	return ret;
>  }
>  
>  static int __gsi_channel_stop(struct gsi_channel *channel, bool stop)
>  {
> -	int ret;
> +	int ret = 0;
>  
>  	/* Wait for any underway transactions to complete before stopping. */
>  	gsi_channel_trans_quiesce(channel);
>  
> -	ret = stop ? gsi_channel_stop_retry(channel) : 0;
> +	if (stop) {
> +		struct gsi *gsi = channel->gsi;
> +
> +		mutex_lock(&gsi->mutex);
> +
> +		ret = gsi_channel_stop_retry(channel);
> +
> +		mutex_unlock(&gsi->mutex);
> +	}
> +
>  	/* Finally, ensure NAPI polling has finished. */
>  	if (!ret)
>  		napi_synchronize(&channel->napi);
> @@ -948,15 +952,14 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id)
>  	struct gsi_channel *channel = &gsi->channel[channel_id];
>  	int ret;
>  
> -	/* Only disable the completion interrupt if stop is successful */
>  	ret = __gsi_channel_stop(channel, true);
> -	if (ret)
> -		return ret;
> +	if (ret) {

This inverts the logic, right? Is it intentional?

> +		/* Disable the completion interrupt and NAPI if successful */
> +		gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id);
> +		napi_disable(&channel->napi);
> +	}
>  
> -	gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id);
> -	napi_disable(&channel->napi);
> -
> -	return 0;
> +	return ret;
>  }
>  
>  /* Reset and reconfigure a channel, (possibly) enabling the doorbell engine */

Re: [PATCH net-next 2/7] net: ipa: synchronize NAPI only for suspend

[Jakub Kicinski]

On Wed,  3 Feb 2021 09:28:50 -0600 Alex Elder wrote:
>  int gsi_channel_suspend(struct gsi *gsi, u32 channel_id, bool stop)
>  {
>  	struct gsi_channel *channel = &gsi->channel[channel_id];
> +	int ret;
>  
> -	return __gsi_channel_stop(channel, stop);
> +	/* Synchronize NAPI if successful, to ensure polling has finished. */
> +	ret = __gsi_channel_stop(channel, stop);
> +	if (!ret)
> +		napi_synchronize(&channel->napi);
> +
> +	return ret;

nit:

	ret = function();
	if (ret)
		return ret;

	/* success path: do something else */

	return 0;

Re: [PATCH net-next 1/7] net: ipa: restructure a few functions

[Alex Elder]

On 2/4/21 10:50 PM, Jakub Kicinski wrote:
> On Wed,  3 Feb 2021 09:28:49 -0600 Alex Elder wrote:
>> Make __gsi_channel_start() and __gsi_channel_stop() more structurally
>> and semantically similar to each other:
>>   - Restructure __gsi_channel_start() to always return at the end of
>>     the function, similar to the way __gsi_channel_stop() does.
>>   - Move the mutex calls out of gsi_channel_stop_retry() and into
>>     __gsi_channel_stop().
>>
>> Restructure gsi_channel_stop() to always return at the end of the
>> function, like gsi_channel_start() does.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  drivers/net/ipa/gsi.c | 45 +++++++++++++++++++++++--------------------
>>  1 file changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
>> index 53640447bf123..2671b76ebcfe3 100644
>> --- a/drivers/net/ipa/gsi.c
>> +++ b/drivers/net/ipa/gsi.c
>> @@ -873,17 +873,17 @@ static void gsi_channel_deprogram(struct gsi_channel *channel)
>>  
>>  static int __gsi_channel_start(struct gsi_channel *channel, bool start)
>>  {
>> -	struct gsi *gsi = channel->gsi;
>> -	int ret;
>> +	int ret = 0;
>>  
>> -	if (!start)
>> -		return 0;
>> +	if (start) {
>> +		struct gsi *gsi = channel->gsi;
>>  
>> -	mutex_lock(&gsi->mutex);
>> +		mutex_lock(&gsi->mutex);
>>  
>> -	ret = gsi_channel_start_command(channel);
>> +		ret = gsi_channel_start_command(channel);
>>  
>> -	mutex_unlock(&gsi->mutex);
>> +		mutex_unlock(&gsi->mutex);
>> +	}
> 
> nit: I thought just recently Willem pointed out that keeping main flow
>      unindented is considered good style, maybe it doesn't apply here
>      perfectly, but I'd think it still applies. Why have the entire
>      body of the function indented?

I *like* keeping the main flow un-indented (the way
it was).

It's a little funny, because one of my motivations for
doing it this way was how I interpreted the comment
from Willem (and echoed by you).  He said, "...easier
to parse when the normal control flow is linear and
the error path takes a branch (or goto, if reused)."
And now that I read it again, I see that's what he
was saying.

But the way I interpreted it was "don't return early
for the success case," because that's what the code
in question that elicited that comment was doing.

In any case I concur with your comment and prefer the
code the other way.  I will post v2 that will fix this,
both here and in __gsi_channel_start().

Thanks.

					-Alex

> 
>>  	return ret;
>>  }
>> @@ -910,11 +910,8 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id)
>>  static int gsi_channel_stop_retry(struct gsi_channel *channel)
>>  {
>>  	u32 retries = GSI_CHANNEL_STOP_RETRIES;
>> -	struct gsi *gsi = channel->gsi;
>>  	int ret;
>>  
>> -	mutex_lock(&gsi->mutex);
>> -
>>  	do {
>>  		ret = gsi_channel_stop_command(channel);
>>  		if (ret != -EAGAIN)
>> @@ -922,19 +919,26 @@ static int gsi_channel_stop_retry(struct gsi_channel *channel)
>>  		usleep_range(3 * USEC_PER_MSEC, 5 * USEC_PER_MSEC);
>>  	} while (retries--);
>>  
>> -	mutex_unlock(&gsi->mutex);
>> -
>>  	return ret;
>>  }
>>  
>>  static int __gsi_channel_stop(struct gsi_channel *channel, bool stop)
>>  {
>> -	int ret;
>> +	int ret = 0;
>>  
>>  	/* Wait for any underway transactions to complete before stopping. */
>>  	gsi_channel_trans_quiesce(channel);
>>  
>> -	ret = stop ? gsi_channel_stop_retry(channel) : 0;
>> +	if (stop) {
>> +		struct gsi *gsi = channel->gsi;
>> +
>> +		mutex_lock(&gsi->mutex);
>> +
>> +		ret = gsi_channel_stop_retry(channel);
>> +
>> +		mutex_unlock(&gsi->mutex);
>> +	}
>> +
>>  	/* Finally, ensure NAPI polling has finished. */
>>  	if (!ret)
>>  		napi_synchronize(&channel->napi);
>> @@ -948,15 +952,14 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id)
>>  	struct gsi_channel *channel = &gsi->channel[channel_id];
>>  	int ret;
>>  
>> -	/* Only disable the completion interrupt if stop is successful */
>>  	ret = __gsi_channel_stop(channel, true);
>> -	if (ret)
>> -		return ret;
>> +	if (ret) {
> 
> This inverts the logic, right? Is it intentional?
> 
>> +		/* Disable the completion interrupt and NAPI if successful */
>> +		gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id);
>> +		napi_disable(&channel->napi);
>> +	}
>>  
>> -	gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id);
>> -	napi_disable(&channel->napi);
>> -
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  /* Reset and reconfigure a channel, (possibly) enabling the doorbell engine */
> 

Re: [PATCH net-next 2/7] net: ipa: synchronize NAPI only for suspend

[Alex Elder]

On 2/4/21 10:53 PM, Jakub Kicinski wrote:
> On Wed,  3 Feb 2021 09:28:50 -0600 Alex Elder wrote:
>>  int gsi_channel_suspend(struct gsi *gsi, u32 channel_id, bool stop)
>>  {
>>  	struct gsi_channel *channel = &gsi->channel[channel_id];
>> +	int ret;
>>  
>> -	return __gsi_channel_stop(channel, stop);
>> +	/* Synchronize NAPI if successful, to ensure polling has finished. */
>> +	ret = __gsi_channel_stop(channel, stop);
>> +	if (!ret)
>> +		napi_synchronize(&channel->napi);
>> +
>> +	return ret;
> 
> nit:
> 
> 	ret = function();
> 	if (ret)
> 		return ret;
> 
> 	/* success path: do something else */
> 
> 	return 0;

No problem, I'm happy with it the way you suggest.  I will
update in v2.   Thank you.

					-Alex