[PATCH 0/3] cleanup style for staging qlge driver

[PATCH 0/3] cleanup style for staging qlge driver

[drake]

From: Drake Talley <drake@drake.talley.com>

This patch series addresses a handful of reports from checkpatch from qlge_main.c


Thanks,
Drake

Drake Talley (3):
  staging: qlge: Separate multiple assignments
  staging: qlge: replace msleep with usleep_range
  staging: qlge: add comment explaining memory barrier

 drivers/staging/qlge/qlge_main.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/3] staging: qlge: Separate multiple assignments

[drake]

From: Drake Talley <drake@draketalley.com>

Adhere to coding style.

Reported by checkpatch:

> CHECK: multiple assignments should be avoided
> #4088: FILE: drivers/staging/qlge/qlge_main.c:4088

> CHECK: multiple assignments should be avoided
> #4108: FILE: drivers/staging/qlge/qlge_main.c:4108:

Signed-off-by: Drake Talley <drake@draketalley.com>
---
 drivers/staging/qlge/qlge_main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 1ead7793062a..8c1fdd8ebba0 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -4085,7 +4085,12 @@ static struct net_device_stats *qlge_get_stats(struct net_device
 	int i;
 
 	/* Get RX stats. */
-	pkts = mcast = dropped = errors = bytes = 0;
+	pkts = 0;
+	mcast = 0;
+	dropped = 0;
+	errors = 0;
+	bytes = 0;
+
 	for (i = 0; i < qdev->rss_ring_count; i++, rx_ring++) {
 		pkts += rx_ring->rx_packets;
 		bytes += rx_ring->rx_bytes;
@@ -4100,7 +4105,10 @@ static struct net_device_stats *qlge_get_stats(struct net_device
 	ndev->stats.multicast = mcast;
 
 	/* Get TX stats. */
-	pkts = errors = bytes = 0;
+	pkts = 0;
+	errors = 0;
+	bytes = 0;
+
 	for (i = 0; i < qdev->tx_ring_count; i++, tx_ring++) {
 		pkts += tx_ring->tx_packets;
 		bytes += tx_ring->tx_bytes;
-- 
2.34.1

Re: [PATCH 1/3] staging: qlge: Separate multiple assignments

[Greg Kroah-Hartman]

On Mon, Oct 31, 2022 at 10:25:14AM -0400, drake@draketalley.com wrote:
> From: Drake Talley <drake@draketalley.com>
> 
> Adhere to coding style.
> 
> Reported by checkpatch:
> 
> > CHECK: multiple assignments should be avoided
> > #4088: FILE: drivers/staging/qlge/qlge_main.c:4088
> 
> > CHECK: multiple assignments should be avoided
> > #4108: FILE: drivers/staging/qlge/qlge_main.c:4108:
> 
> Signed-off-by: Drake Talley <drake@draketalley.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index 1ead7793062a..8c1fdd8ebba0 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -4085,7 +4085,12 @@ static struct net_device_stats *qlge_get_stats(struct net_device
>  	int i;
>  
>  	/* Get RX stats. */
> -	pkts = mcast = dropped = errors = bytes = 0;
> +	pkts = 0;
> +	mcast = 0;
> +	dropped = 0;
> +	errors = 0;
> +	bytes = 0;
> +
>  	for (i = 0; i < qdev->rss_ring_count; i++, rx_ring++) {
>  		pkts += rx_ring->rx_packets;
>  		bytes += rx_ring->rx_bytes;
> @@ -4100,7 +4105,10 @@ static struct net_device_stats *qlge_get_stats(struct net_device
>  	ndev->stats.multicast = mcast;
>  
>  	/* Get TX stats. */
> -	pkts = errors = bytes = 0;
> +	pkts = 0;
> +	errors = 0;
> +	bytes = 0;
> +
>  	for (i = 0; i < qdev->tx_ring_count; i++, tx_ring++) {
>  		pkts += tx_ring->tx_packets;
>  		bytes += tx_ring->tx_bytes;
> -- 
> 2.34.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what is needed in order to
  properly describe the change.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH 2/3] staging: qlge: replace msleep with usleep_range

[drake]

From: Drake Talley <drake@draketalley.com>

Since msleep may delay for up to 20ms, usleep_range is recommended for
short durations in the docs linked in the below warning.  I set the
range to 1000-2000 based on looking at other usages of usleep_range.

Reported by checkpatch:

WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst

Signed-off-by: Drake Talley <drake@draketalley.com>
---
 drivers/staging/qlge/qlge_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 8c1fdd8ebba0..c8403dbb5bad 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -3890,7 +3890,7 @@ static int qlge_close(struct net_device *ndev)
 	 * (Rarely happens, but possible.)
 	 */
 	while (!test_bit(QL_ADAPTER_UP, &qdev->flags))
-		msleep(1);
+		usleep_range(1000, 2000);
 
 	/* Make sure refill_work doesn't re-enable napi */
 	for (i = 0; i < qdev->rss_ring_count; i++)
-- 
2.34.1

Re: [PATCH 2/3] staging: qlge: replace msleep with usleep_range

[Greg Kroah-Hartman]

On Mon, Oct 31, 2022 at 10:25:15AM -0400, drake@draketalley.com wrote:
> From: Drake Talley <drake@draketalley.com>
> 
> Since msleep may delay for up to 20ms, usleep_range is recommended for
> short durations in the docs linked in the below warning.  I set the
> range to 1000-2000 based on looking at other usages of usleep_range.
> 
> Reported by checkpatch:
> 
> WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst
> 
> Signed-off-by: Drake Talley <drake@draketalley.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index 8c1fdd8ebba0..c8403dbb5bad 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -3890,7 +3890,7 @@ static int qlge_close(struct net_device *ndev)
>  	 * (Rarely happens, but possible.)
>  	 */
>  	while (!test_bit(QL_ADAPTER_UP, &qdev->flags))
> -		msleep(1);
> +		usleep_range(1000, 2000);

Please see the mailing list archives for why making this type of change
without the hardware to test it is not a good idea.

sorry,

greg k-h

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

[drake]

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.
+		 */
 		rmb();
 		switch (net_rsp->opcode) {
 		case OPCODE_OB_MAC_TSO_IOCB:
-- 
2.34.1

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

[Greg Kroah-Hartman]

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