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@xxxxxxxxxx>
>> ---
>>  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 */
>