diff mbox

igb: report unsupported ethtool settings in set_coalesce

Message ID 20150522174950.16145.78447.stgit@htfujina-fc.jf.intel.com
State Changes Requested
Delegated to: Jeff Kirsher
Headers show

Commit Message

Fujinaka, Todd May 22, 2015, 5:49 p.m. UTC 
There are many settings possible using ethtool -C/--coalesce, but not
all of them are supported. Report failure when an unsupported option is
set.

Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Alexander Duyck May 22, 2015, 6:09 p.m. UTC | #1 
On 05/22/2015 10:49 AM, Todd Fujinaka wrote:
> There are many settings possible using ethtool -C/--coalesce, but not
> all of them are supported. Report failure when an unsupported option is
> set.
>
> Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_ethtool.c |   21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 109cad9..13560fe 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2159,6 +2159,27 @@ static int igb_set_coalesce(struct net_device *netdev,
>   	struct igb_adapter *adapter = netdev_priv(netdev);
>   	int i;
>
> +	if ((ec->rx_max_coalesced_frames != -1) ||
> +	    (ec->rx_coalesce_usecs_irq != -1) ||
> +	    (ec->rx_max_coalesced_frames_irq != -1) ||
> +	    (ec->tx_max_coalesced_frames != -1) ||
> +	    (ec->tx_coalesce_usecs_irq != -1) ||
> +	    (ec->stats_block_coalesce_usecs != -1) ||
> +	    (ec->use_adaptive_rx_coalesce != -1) ||
> +	    (ec->use_adaptive_tx_coalesce != -1) ||
> +	    (ec->pkt_rate_low != -1) ||
> +	    (ec->rx_coalesce_usecs_low != -1) ||
> +	    (ec->rx_max_coalesced_frames_low != -1) ||
> +	    (ec->tx_coalesce_usecs_low != -1) ||
> +	    (ec->tx_max_coalesced_frames_low != -1) ||
> +	    (ec->pkt_rate_high != -1) ||
> +	    (ec->rx_coalesce_usecs_high != -1) ||
> +	    (ec->rx_max_coalesced_frames_high != -1) ||
> +	    (ec->tx_coalesce_usecs_high != -1) ||
> +	    (ec->tx_max_coalesced_frames_high != -1) ||
> +	    (ec->rate_sample_interval != -1))
> +		return -ENOTSUPP;
> +
>   	if ((ec->rx_coalesce_usecs > IGB_MAX_ITR_USECS) ||
>   	    ((ec->rx_coalesce_usecs > 3) &&
>   	     (ec->rx_coalesce_usecs < IGB_MIN_ITR_USECS)) ||
>

Shouldn't these tests all give you a signed/unsigned mismatch since you 
are comparing an unsigned 32 bit value versus a signed value?

- Alex
Fujinaka, Todd May 22, 2015, 6:10 p.m. UTC | #2 
Those are unsigned? The code for ethtool set them all to -1.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka@intel.com
(503) 712-4565

-----Original Message-----
From: Alexander Duyck [mailto:alexander.h.duyck@redhat.com] 
Sent: Friday, May 22, 2015 11:09 AM
To: Fujinaka, Todd; intel-wired-lan@lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH] igb: report unsupported ethtool settings in set_coalesce



On 05/22/2015 10:49 AM, Todd Fujinaka wrote:
> There are many settings possible using ethtool -C/--coalesce, but not 
> all of them are supported. Report failure when an unsupported option 
> is set.
>
> Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_ethtool.c |   21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c 
> b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 109cad9..13560fe 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2159,6 +2159,27 @@ static int igb_set_coalesce(struct net_device *netdev,
>   	struct igb_adapter *adapter = netdev_priv(netdev);
>   	int i;
>
> +	if ((ec->rx_max_coalesced_frames != -1) ||
> +	    (ec->rx_coalesce_usecs_irq != -1) ||
> +	    (ec->rx_max_coalesced_frames_irq != -1) ||
> +	    (ec->tx_max_coalesced_frames != -1) ||
> +	    (ec->tx_coalesce_usecs_irq != -1) ||
> +	    (ec->stats_block_coalesce_usecs != -1) ||
> +	    (ec->use_adaptive_rx_coalesce != -1) ||
> +	    (ec->use_adaptive_tx_coalesce != -1) ||
> +	    (ec->pkt_rate_low != -1) ||
> +	    (ec->rx_coalesce_usecs_low != -1) ||
> +	    (ec->rx_max_coalesced_frames_low != -1) ||
> +	    (ec->tx_coalesce_usecs_low != -1) ||
> +	    (ec->tx_max_coalesced_frames_low != -1) ||
> +	    (ec->pkt_rate_high != -1) ||
> +	    (ec->rx_coalesce_usecs_high != -1) ||
> +	    (ec->rx_max_coalesced_frames_high != -1) ||
> +	    (ec->tx_coalesce_usecs_high != -1) ||
> +	    (ec->tx_max_coalesced_frames_high != -1) ||
> +	    (ec->rate_sample_interval != -1))
> +		return -ENOTSUPP;
> +
>   	if ((ec->rx_coalesce_usecs > IGB_MAX_ITR_USECS) ||
>   	    ((ec->rx_coalesce_usecs > 3) &&
>   	     (ec->rx_coalesce_usecs < IGB_MIN_ITR_USECS)) ||
>

Shouldn't these tests all give you a signed/unsigned mismatch since you are comparing an unsigned 32 bit value versus a signed value?

- Alex
Alexander H Duyck May 22, 2015, 6:29 p.m. UTC | #3 
I think I see what you are talking about.  They used s32 in the ethtool 
do_scoalesce call, and pointed the void pointer wanted_val at it which 
is how they did the conversion.

You might want to just == ~0 or ~(ec->...) instead to test the values.

- Alex

On 05/22/2015 11:10 AM, Fujinaka, Todd wrote:
> Those are unsigned? The code for ethtool set them all to -1.
>
> Todd Fujinaka
> Software Application Engineer
> Networking Division (ND)
> Intel Corporation
> todd.fujinaka@intel.com
> (503) 712-4565
>
> -----Original Message-----
> From: Alexander Duyck [mailto:alexander.h.duyck@redhat.com]
> Sent: Friday, May 22, 2015 11:09 AM
> To: Fujinaka, Todd; intel-wired-lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH] igb: report unsupported ethtool settings in set_coalesce
>
>
>
> On 05/22/2015 10:49 AM, Todd Fujinaka wrote:
>> There are many settings possible using ethtool -C/--coalesce, but not
>> all of them are supported. Report failure when an unsupported option
>> is set.
>>
>> Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
>> ---
>>    drivers/net/ethernet/intel/igb/igb_ethtool.c |   21 +++++++++++++++++++++
>>    1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> index 109cad9..13560fe 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -2159,6 +2159,27 @@ static int igb_set_coalesce(struct net_device *netdev,
>>    	struct igb_adapter *adapter = netdev_priv(netdev);
>>    	int i;
>>
>> +	if ((ec->rx_max_coalesced_frames != -1) ||
>> +	    (ec->rx_coalesce_usecs_irq != -1) ||
>> +	    (ec->rx_max_coalesced_frames_irq != -1) ||
>> +	    (ec->tx_max_coalesced_frames != -1) ||
>> +	    (ec->tx_coalesce_usecs_irq != -1) ||
>> +	    (ec->stats_block_coalesce_usecs != -1) ||
>> +	    (ec->use_adaptive_rx_coalesce != -1) ||
>> +	    (ec->use_adaptive_tx_coalesce != -1) ||
>> +	    (ec->pkt_rate_low != -1) ||
>> +	    (ec->rx_coalesce_usecs_low != -1) ||
>> +	    (ec->rx_max_coalesced_frames_low != -1) ||
>> +	    (ec->tx_coalesce_usecs_low != -1) ||
>> +	    (ec->tx_max_coalesced_frames_low != -1) ||
>> +	    (ec->pkt_rate_high != -1) ||
>> +	    (ec->rx_coalesce_usecs_high != -1) ||
>> +	    (ec->rx_max_coalesced_frames_high != -1) ||
>> +	    (ec->tx_coalesce_usecs_high != -1) ||
>> +	    (ec->tx_max_coalesced_frames_high != -1) ||
>> +	    (ec->rate_sample_interval != -1))
>> +		return -ENOTSUPP;
>> +
>>    	if ((ec->rx_coalesce_usecs > IGB_MAX_ITR_USECS) ||
>>    	    ((ec->rx_coalesce_usecs > 3) &&
>>    	     (ec->rx_coalesce_usecs < IGB_MIN_ITR_USECS)) ||
>>
> Shouldn't these tests all give you a signed/unsigned mismatch since you are comparing an unsigned 32 bit value versus a signed value?
>
> - Alex
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Fujinaka, Todd June 2, 2015, 11:02 p.m. UTC | #4 
This doesn't appear to work quite right. Jeff, can you pop this? I've redone it.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka@intel.com
(503) 712-4565

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck@gmail.com] 
Sent: Friday, May 22, 2015 11:29 AM
To: Fujinaka, Todd; Alexander Duyck; intel-wired-lan@lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH] igb: report unsupported ethtool settings in set_coalesce

I think I see what you are talking about.  They used s32 in the ethtool do_scoalesce call, and pointed the void pointer wanted_val at it which is how they did the conversion.

You might want to just == ~0 or ~(ec->...) instead to test the values.

- Alex

On 05/22/2015 11:10 AM, Fujinaka, Todd wrote:
> Those are unsigned? The code for ethtool set them all to -1.
>
> Todd Fujinaka
> Software Application Engineer
> Networking Division (ND)
> Intel Corporation
> todd.fujinaka@intel.com
> (503) 712-4565
>
> -----Original Message-----
> From: Alexander Duyck [mailto:alexander.h.duyck@redhat.com]
> Sent: Friday, May 22, 2015 11:09 AM
> To: Fujinaka, Todd; intel-wired-lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH] igb: report unsupported ethtool 
> settings in set_coalesce
>
>
>
> On 05/22/2015 10:49 AM, Todd Fujinaka wrote:
>> There are many settings possible using ethtool -C/--coalesce, but not 
>> all of them are supported. Report failure when an unsupported option 
>> is set.
>>
>> Signed-off-by: Todd Fujinaka <todd.fujinaka@intel.com>
>> ---
>>    drivers/net/ethernet/intel/igb/igb_ethtool.c |   21 +++++++++++++++++++++
>>    1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> index 109cad9..13560fe 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -2159,6 +2159,27 @@ static int igb_set_coalesce(struct net_device *netdev,
>>    	struct igb_adapter *adapter = netdev_priv(netdev);
>>    	int i;
>>
>> +	if ((ec->rx_max_coalesced_frames != -1) ||
>> +	    (ec->rx_coalesce_usecs_irq != -1) ||
>> +	    (ec->rx_max_coalesced_frames_irq != -1) ||
>> +	    (ec->tx_max_coalesced_frames != -1) ||
>> +	    (ec->tx_coalesce_usecs_irq != -1) ||
>> +	    (ec->stats_block_coalesce_usecs != -1) ||
>> +	    (ec->use_adaptive_rx_coalesce != -1) ||
>> +	    (ec->use_adaptive_tx_coalesce != -1) ||
>> +	    (ec->pkt_rate_low != -1) ||
>> +	    (ec->rx_coalesce_usecs_low != -1) ||
>> +	    (ec->rx_max_coalesced_frames_low != -1) ||
>> +	    (ec->tx_coalesce_usecs_low != -1) ||
>> +	    (ec->tx_max_coalesced_frames_low != -1) ||
>> +	    (ec->pkt_rate_high != -1) ||
>> +	    (ec->rx_coalesce_usecs_high != -1) ||
>> +	    (ec->rx_max_coalesced_frames_high != -1) ||
>> +	    (ec->tx_coalesce_usecs_high != -1) ||
>> +	    (ec->tx_max_coalesced_frames_high != -1) ||
>> +	    (ec->rate_sample_interval != -1))
>> +		return -ENOTSUPP;
>> +
>>    	if ((ec->rx_coalesce_usecs > IGB_MAX_ITR_USECS) ||
>>    	    ((ec->rx_coalesce_usecs > 3) &&
>>    	     (ec->rx_coalesce_usecs < IGB_MIN_ITR_USECS)) ||
>>
> Shouldn't these tests all give you a signed/unsigned mismatch since you are comparing an unsigned 32 bit value versus a signed value?
>
> - Alex
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Kirsher, Jeffrey T June 3, 2015, 9:32 a.m. UTC | #5 
On Tue, 2015-06-02 at 16:02 -0700, Fujinaka, Todd wrote:
> This doesn't appear to work quite right. Jeff, can you pop this? I've
> redone it.

I have removed it from my queue.
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 109cad9..13560fe 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2159,6 +2159,27 @@  static int igb_set_coalesce(struct net_device *netdev,
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	int i;
 
+	if ((ec->rx_max_coalesced_frames != -1) ||
+	    (ec->rx_coalesce_usecs_irq != -1) ||
+	    (ec->rx_max_coalesced_frames_irq != -1) ||
+	    (ec->tx_max_coalesced_frames != -1) ||
+	    (ec->tx_coalesce_usecs_irq != -1) ||
+	    (ec->stats_block_coalesce_usecs != -1) ||
+	    (ec->use_adaptive_rx_coalesce != -1) ||
+	    (ec->use_adaptive_tx_coalesce != -1) ||
+	    (ec->pkt_rate_low != -1) ||
+	    (ec->rx_coalesce_usecs_low != -1) ||
+	    (ec->rx_max_coalesced_frames_low != -1) ||
+	    (ec->tx_coalesce_usecs_low != -1) ||
+	    (ec->tx_max_coalesced_frames_low != -1) ||
+	    (ec->pkt_rate_high != -1) ||
+	    (ec->rx_coalesce_usecs_high != -1) ||
+	    (ec->rx_max_coalesced_frames_high != -1) ||
+	    (ec->tx_coalesce_usecs_high != -1) ||
+	    (ec->tx_max_coalesced_frames_high != -1) ||
+	    (ec->rate_sample_interval != -1))
+		return -ENOTSUPP;
+
 	if ((ec->rx_coalesce_usecs > IGB_MAX_ITR_USECS) ||
 	    ((ec->rx_coalesce_usecs > 3) &&
 	     (ec->rx_coalesce_usecs < IGB_MIN_ITR_USECS)) ||