netdev - Re: [net PATCH 5/6] fbnic: Cleanup handling of completions

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]

Message-ID: <20250502104523.GH3339421@horms.kernel.org>
Date: Fri, 2 May 2025 11:45:23 +0100
From: Simon Horman <horms@...nel.org>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [net PATCH 5/6] fbnic: Cleanup handling of completions

On Thu, May 01, 2025 at 04:30:22PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@...com>
> 
> There was an issue in that if we were to shutdown we could be left with
> a completion in flight as the mailbox went away. To address that I have
> added an fbnic_mbx_evict_all_cmpl function that is meant to essentially
> create a "broken pipe" type response so that all callers will receive an
> error indicating that the connection has been broken as a result of us
> shutting down the mailbox.
> 
> In addition the naming was inconsistent between the creation and clearing
> of completions. Since the cmpl seems to be the common suffix to use for the
> handling of cmpl_data I went through and renamed fbnic_fw_clear_compl to
> fbnic_fw_clear_cmpl.

I do see this is somehow related to the fix described in the first
paragraph. But I don't think renaming functions like this is appropriate
for net.

> Fixes: 378e5cc1c6c6 ("eth: fbnic: hwmon: Add completion infrastructure for firmware requests")
> 
> Signed-off-by: Alexander Duyck <alexanderduyck@...com>
> ---
>  drivers/net/ethernet/meta/fbnic/fbnic_fw.c  |   22 +++++++++++++++++++++-
>  drivers/net/ethernet/meta/fbnic/fbnic_fw.h  |    2 +-
>  drivers/net/ethernet/meta/fbnic/fbnic_mac.c |    2 +-
>  3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index d019191d6ae9..efc0176f1a9a 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -928,6 +928,23 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
>  	return attempts ? 0 : -ETIMEDOUT;
>  }
>  
> +static void __fbnic_fw_evict_cmpl(struct fbnic_fw_completion *cmpl_data)
> +{
> +	cmpl_data->result = -EPIPE;
> +	complete(&cmpl_data->done);
> +}
> +
> +static void fbnic_mbx_evict_all_cmpl(struct fbnic_dev *fbd)
> +{
> +	struct fbnic_fw_completion *cmpl_data;
> +
> +	cmpl_data = fbd->cmpl_data;
> +	if (cmpl_data)
> +		__fbnic_fw_evict_cmpl(cmpl_data);
> +
> +	memset(&fbd->cmpl_data, 0, sizeof(fbd->cmpl_data));

Maybe I've been staring at my screen for too long, but could this
be expressed more succinctly as:

	fbd->cmpl_data = NULL;

And if so, it seems that step can be skipped if cmpl_data is already
NULL, which is already checked.

Leading to the following, which is somehow easier on my brain.

static void fbnic_mbx_evict_all_cmpl(struct fbnic_dev *fbd)
{
	if (fbd->cmpl_data) {
		__fbnic_fw_evict_cmpl(fbd->cmpl_data);
		fbd->cmpl_data = NULL;
	}
}

> +}
> +
>  void fbnic_mbx_flush_tx(struct fbnic_dev *fbd)
>  {
>  	unsigned long timeout = jiffies + 10 * HZ + 1;
> @@ -945,6 +962,9 @@ void fbnic_mbx_flush_tx(struct fbnic_dev *fbd)
>  	/* Read tail to determine the last tail state for the ring */
>  	tail = tx_mbx->tail;
>  
> +	/* Flush any completions as we are no longer processing Rx */
> +	fbnic_mbx_evict_all_cmpl(fbd);
> +
>  	spin_unlock_irq(&fbd->fw_tx_lock);
>  
>  	/* Give firmware time to process packet,
> @@ -983,7 +1003,7 @@ void fbnic_fw_init_cmpl(struct fbnic_fw_completion *fw_cmpl,
>  	kref_init(&fw_cmpl->ref_count);
>  }
>  
> -void fbnic_fw_clear_compl(struct fbnic_dev *fbd)
> +void fbnic_fw_clear_cmpl(struct fbnic_dev *fbd)
>  {
>  	unsigned long flags;
>  
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> index a3618e7826c2..2d5e0ff1982c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> @@ -69,7 +69,7 @@ int fbnic_fw_xmit_tsene_read_msg(struct fbnic_dev *fbd,
>  				 struct fbnic_fw_completion *cmpl_data);
>  void fbnic_fw_init_cmpl(struct fbnic_fw_completion *cmpl_data,
>  			u32 msg_type);
> -void fbnic_fw_clear_compl(struct fbnic_dev *fbd);
> +void fbnic_fw_clear_cmpl(struct fbnic_dev *fbd);
>  void fbnic_fw_put_cmpl(struct fbnic_fw_completion *cmpl_data);
>  
>  #define fbnic_mk_full_fw_ver_str(_rev_id, _delim, _commit, _str, _str_sz) \
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
> index dde4a37116e2..7e54f82535f6 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
> @@ -744,7 +744,7 @@ static int fbnic_mac_get_sensor_asic(struct fbnic_dev *fbd, int id,
>  
>  	*val = *sensor;
>  exit_cleanup:
> -	fbnic_fw_clear_compl(fbd);
> +	fbnic_fw_clear_cmpl(fbd);
>  exit_free:
>  	fbnic_fw_put_cmpl(fw_cmpl);
>  
> 
> 
>