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

On 2/4/21 10:50 PM, Jakub Kicinski wrote:

On Wed,  3 Feb 2021 09:28:49 -0600 Alex Elder wrote:

quoted

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 <redacted>
---
 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

quoted

 	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?

quoted

+		/* 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 */