[PATCH net 0/2] tg3: Disallow 0 rx coalesce time and correctly report RSS queues in tg3_get_rxnfc

[PATCH net 0/2] tg3: Disallow 0 rx coalesce time and correctly report RSS queues in tg3_get_rxnfc

[skallam]

From: Siva Reddy Kallam <siva.kallam@broadcom.com>

First patch:
        Diasllow rx coalescing time to be 0

Second patch:
        Report the correct number of RSS queues through tg3_get_rxnfc

Satish Baddipadige (1):
  tg3: Fix for diasllow rx coalescing time to be 0

Siva Reddy Kallam (1):
  tg3: Report the correct number of RSS queues through tg3_get_rxnfc

 drivers/net/ethernet/broadcom/tg3.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

[PATCH net 1/2] tg3: Fix for diasllow rx coalescing time to be 0

[skallam]

From: Satish Baddipadige <satish.baddipadige@broadcom.com>

When the rx coalescing time is 0, interrupts
are not generated from the controller and rx path hangs.
To avoid this rx hang, updating the driver to not allow
rx coalescing time to be 0.

Signed-off-by: Satish Baddipadige <satish.baddipadige@broadcom.com>
Signed-off-by: Siva Reddy Kallam <siva.kallam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index ff300f7..f3c6c91 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -14014,6 +14014,7 @@ static int tg3_set_coalesce(struct net_device *dev, struct ethtool_coalesce *ec)
 	}
 
 	if ((ec->rx_coalesce_usecs > MAX_RXCOL_TICKS) ||
+	    (!ec->rx_coalesce_usecs) ||
 	    (ec->tx_coalesce_usecs > MAX_TXCOL_TICKS) ||
 	    (ec->rx_max_coalesced_frames > MAX_RXMAX_FRAMES) ||
 	    (ec->tx_max_coalesced_frames > MAX_TXMAX_FRAMES) ||
-- 
1.7.1

Re: [PATCH net 1/2] tg3: Fix for diasllow rx coalescing time to be 0

[Rick Jones]

On 08/02/2016 09:13 PM, skallam wrote:
> From: Satish Baddipadige <satish.baddipadige@broadcom.com>
>
> When the rx coalescing time is 0, interrupts
> are not generated from the controller and rx path hangs.
> To avoid this rx hang, updating the driver to not allow
> rx coalescing time to be 0.
>
> Signed-off-by: Satish Baddipadige <satish.baddipadige@broadcom.com>
> Signed-off-by: Siva Reddy Kallam <siva.kallam@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/tg3.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index ff300f7..f3c6c91 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -14014,6 +14014,7 @@ static int tg3_set_coalesce(struct net_device *dev, struct ethtool_coalesce *ec)
>  	}
>
>  	if ((ec->rx_coalesce_usecs > MAX_RXCOL_TICKS) ||
> +	    (!ec->rx_coalesce_usecs) ||
>  	    (ec->tx_coalesce_usecs > MAX_TXCOL_TICKS) ||
>  	    (ec->rx_max_coalesced_frames > MAX_RXMAX_FRAMES) ||
>  	    (ec->tx_max_coalesced_frames > MAX_TXMAX_FRAMES) ||
>

Should anything then happen with:

         /* No rx interrupts will be generated if both are zero */
         if ((ec->rx_coalesce_usecs == 0) &&
             (ec->rx_max_coalesced_frames == 0))
                 return -EINVAL;


which is the next block of code?  The logic there seems to suggest that 
it was intended to be able to have an rx_coalesce_usecs of 0 and rely on 
packet arrival to trigger an interrupt.  Presumably setting 
rx_max_coalesced_frames to 1 to disable interrupt coalescing.

happy benchmarking,

rick jones

Re: [PATCH net 1/2] tg3: Fix for diasllow rx coalescing time to be 0

[Michael Chan]

On Wed, Aug 3, 2016 at 9:04 AM, Rick Jones <rick.jones2@hpe.com> wrote:
>
> Should anything then happen with:
>
>         /* No rx interrupts will be generated if both are zero */
>         if ((ec->rx_coalesce_usecs == 0) &&
>             (ec->rx_max_coalesced_frames == 0))
>                 return -EINVAL;
>
>
> which is the next block of code?  The logic there seems to suggest that it
> was intended to be able to have an rx_coalesce_usecs of 0 and rely on packet
> arrival to trigger an interrupt.  Presumably setting rx_max_coalesced_frames
> to 1 to disable interrupt coalescing.
>

I remember writing this block of code over 10 years ago for early
generations of the chip.  Newer chips seem to behave differently and
rx_coalesce_usecs can never be zero.  So this block can be removed now
that the condition can never be true.  We should probably leave a
comment there for future reference.

[PATCH net 2/2] tg3: Report the correct number of RSS queues through tg3_get_rxnfc

[skallam]

From: Siva Reddy Kallam <siva.kallam@broadcom.com>

This patch remove the wrong substraction from info->data in
tg3_get_rxnfc function. Without this patch, the number of RSS
queues reported is less by one.

Reported-by: Michal Soltys <soltys@ziu.info>
Signed-off-by: Siva Reddy Kallam <siva.kallam@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index f3c6c91..6592612 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -12552,10 +12552,6 @@ static int tg3_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
 				info->data = TG3_RSS_MAX_NUM_QS;
 		}
 
-		/* The first interrupt vector only
-		 * handles link interrupts.
-		 */
-		info->data -= 1;
 		return 0;
 
 	default:
-- 
1.7.1

Re: [PATCH net 0/2] tg3: Disallow 0 rx coalesce time and correctly report RSS queues in tg3_get_rxnfc

[David Miller]

From: skallam <siva.kallam@broadcom.com>
Date: Wed,  3 Aug 2016 09:43:58 +0530

> First patch:
>         Diasllow rx coalescing time to be 0
> 
> Second patch:
>         Report the correct number of RSS queues through tg3_get_rxnfc

Series applied, thanks.

Re: [PATCH net 1/2] tg3: Fix for diasllow rx coalescing time to be 0

[Siva Reddy Kallam]

On Thu, Aug 4, 2016 at 3:45 AM, Michael Chan <michael.chan@broadcom.com> wrote:
> On Wed, Aug 3, 2016 at 9:04 AM, Rick Jones <rick.jones2@hpe.com> wrote:
>>
>> Should anything then happen with:
>>
>>         /* No rx interrupts will be generated if both are zero */
>>         if ((ec->rx_coalesce_usecs == 0) &&
>>             (ec->rx_max_coalesced_frames == 0))
>>                 return -EINVAL;
>>
>>
>> which is the next block of code?  The logic there seems to suggest that it
>> was intended to be able to have an rx_coalesce_usecs of 0 and rely on packet
>> arrival to trigger an interrupt.  Presumably setting rx_max_coalesced_frames
>> to 1 to disable interrupt coalescing.
>>
>
> I remember writing this block of code over 10 years ago for early
> generations of the chip.  Newer chips seem to behave differently and
> rx_coalesce_usecs can never be zero.  So this block can be removed now
> that the condition can never be true.  We should probably leave a
> comment there for future reference.
Thanks Rick for identifying this.
Thanks Michael for your inputs. I will submit a patch with removal of
this block of code and add a comment for future reference.