Re: [PATCH 3/3] staging: qlge: add comment explaining memory barrier

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Oct 31, 2022 at 10:25:16AM -0400, drake@draketalley.com wrote:
> From: Drake Talley <drake@draketalley.com>
> 
> codestyle change that fixes the following report from checkpatch:
> 
> > WARNING: memory barrier without comment
> > #2101: FILE: drivers/staging/qlge/qlge_main.c:2101:
> 
> The added comment identifies the next item from the circular
> buffer (rx_ring->curr_entry) and its handling/unmapping as the two
> operations that must not be reordered.  Based on the kernel
> documentation for memory barriers in circular buffers
> (https://www.kernel.org/doc/Documentation/circular-buffers.txt) and
> the presence of atomic operations in the current context I'm assuming
> this usage of the memory barrier is akin to what is explained in the
> linked doc.
> 
> There are a couple of other uncommented usages of memory barriers in
> the current file.  If this comment is adequate I can add similar
> comments to the others.
> 
> Signed-off-by: Drake Talley <drake@draketalley.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index c8403dbb5bad..f70390bce6d8 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -2098,6 +2098,12 @@ static int qlge_clean_outbound_rx_ring(struct rx_ring *rx_ring)
>  			     rx_ring->cq_id, prod, rx_ring->cnsmr_idx);
>  
>  		net_rsp = (struct qlge_ob_mac_iocb_rsp *)rx_ring->curr_entry;
> +		/*
> +		 * Ensure that the next item from the ring buffer is loaded
> +		 * before being processed.
> +		 * Adding rmb() prevents the compiler from reordering the read
> +		 * and subsequent handling of the outbound completion pointer.
> +		 */

Which "next item"?

>  		rmb();

>  		switch (net_rsp->opcode) {

So the opcode read is what you want to prevent from reordering?  Where
is the other users of this that could have changed it?

Changes like this are hard to determine if your comments are correct.
We know what a rmb() does, the question that needs to be answered here
is _why_ it is used here.  So try to step back and see if it really is
needed at all.

If it is needed, why?  And go from there on how to document this
properly.

thanks,

greg k-h