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 <el...@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
>> */
>
