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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Feb 2021 20:50:59 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alex Elder <elder@...aro.org>
Cc:     davem@...emloft.net, elder@...nel.org, evgreen@...omium.org,
        bjorn.andersson@...aro.org, cpratapa@...eaurora.org,
        subashab@...eaurora.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [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@...aro.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 */