lkml.org 
[lkml]   [2021]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next 1/7] net: ipa: restructure a few functions
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 */

\
 
 \ /
  Last update: 2021-02-05 05:53    [W:0.125 / U:0.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site