[PATCH net-next] ethtool: Added a field fw dump_state

[PATCH net-next] ethtool: Added a field fw dump_state

[Anirban Chakraborty]

From: Manish chopra <manish.chopra@qlogic.com>

This field is added to enable/disable firmware dump.

Signed-off-by: Manish chopra <manish.chopra@qlogic.com>
Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
---
 include/linux/ethtool.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index e1d9e0e..6ebc7de 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -666,15 +666,22 @@ struct ethtool_flash {
  * 	 %ETHTOOL_GET_DUMP_DATA and this is returned as dump length by driver
  * 	 for %ETHTOOL_GET_DUMP_FLAG command
  * @data: data collected for get dump data operation
+ * @dump_state: state of the firmware dump. which can be enable/disable.
  */
+
+#define ETH_FW_DUMP_ENABLE	1
+#define ETH_FW_DUMP_DISABLE	0
+
 struct ethtool_dump {
 	__u32	cmd;
 	__u32	version;
 	__u32	flag;
 	__u32	len;
 	__u8	data[0];
+	__u8	dump_state;
 };
 
+
 /* for returning and changing feature sets */
 
 /**
-- 
1.7.1

[PATCH net-next] qlcnic: ethtool change to provide the dump_state information

[Anirban Chakraborty]

From: Manish chopra <manish.chopra@qlogic.com>

o dump_state will be either enabled/disabled that is 1/0

Signed-off-by: Manish chopra <manish.chopra@qlogic.com>
Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
---
 .../net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c    |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
index 89ddf7f..56f7ebc 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
@@ -1136,6 +1136,12 @@ qlcnic_get_dump_flag(struct net_device *netdev, struct ethtool_dump *dump)
 		dump->len = fw_dump->tmpl_hdr->size + fw_dump->size;
 	else
 		dump->len = 0;
+
+	if (fw_dump->enable)
+		dump->dump_state = ETH_FW_DUMP_ENABLE;
+	else
+		dump->dump_state = ETH_FW_DUMP_DISABLE;
+
 	dump->flag = fw_dump->tmpl_hdr->drv_cap_mask;
 	dump->version = adapter->fw_version;
 	return 0;
-- 
1.7.1

Re: [PATCH net-next] ethtool: Added a field fw dump_state

[Ben Hutchings]

On Fri, 2012-03-16 at 10:58 -0700, Anirban Chakraborty wrote:
> From: Manish chopra <manish.chopra@qlogic.com>
> 
> This field is added to enable/disable firmware dump.
> 
> Signed-off-by: Manish chopra <manish.chopra@qlogic.com>
> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
> ---
>  include/linux/ethtool.h |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index e1d9e0e..6ebc7de 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -666,15 +666,22 @@ struct ethtool_flash {
>   * 	 %ETHTOOL_GET_DUMP_DATA and this is returned as dump length by driver
>   * 	 for %ETHTOOL_GET_DUMP_FLAG command
>   * @data: data collected for get dump data operation
> + * @dump_state: state of the firmware dump. which can be enable/disable.
>   */
> +
> +#define ETH_FW_DUMP_ENABLE	1
> +#define ETH_FW_DUMP_DISABLE	0
> +
>  struct ethtool_dump {
>  	__u32	cmd;
>  	__u32	version;
>  	__u32	flag;
>  	__u32	len;
>  	__u8	data[0];
> +	__u8	dump_state;

Don't be ridiculous.

Ben.

>  };
>  
> +
>  /* for returning and changing feature sets */
>  
>  /**

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] ethtool: Added a field fw dump_state

[Anirban Chakraborty]

On 3/16/12 11:27 AM, "Ben Hutchings" <bhutchings@solarflare.com> wrote:

>On Fri, 2012-03-16 at 10:58 -0700, Anirban Chakraborty wrote:
>> From: Manish chopra <manish.chopra@qlogic.com>
>> 
>> This field is added to enable/disable firmware dump.
>> 
>> Signed-off-by: Manish chopra <manish.chopra@qlogic.com>
>> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>> ---
>>  include/linux/ethtool.h |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>> 
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index e1d9e0e..6ebc7de 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -666,15 +666,22 @@ struct ethtool_flash {
>>   * 	 %ETHTOOL_GET_DUMP_DATA and this is returned as dump length by
>>driver
>>   * 	 for %ETHTOOL_GET_DUMP_FLAG command
>>   * @data: data collected for get dump data operation
>> + * @dump_state: state of the firmware dump. which can be
>>enable/disable.
>>   */
>> +
>> +#define ETH_FW_DUMP_ENABLE	1
>> +#define ETH_FW_DUMP_DISABLE	0
>> +
>>  struct ethtool_dump {
>>  	__u32	cmd;
>>  	__u32	version;
>>  	__u32	flag;
>>  	__u32	len;
>>  	__u8	data[0];
>> +	__u8	dump_state;
>
>Don't be ridiculous.

Yeah I know, especially when there is a flag field already present there.
The only
reason, we considered for adding it is to keep the backward compatibility
of scripts.
Right now, the flag field sets/gets the dump level of fw. If we use it to
control the
dump state, then it would break the existing scripts, if there are any.

-Anirban

Re: [PATCH net-next] ethtool: Added a field fw dump_state

[Eric Dumazet]

On Fri, 2012-03-16 at 14:10 -0700, Anirban Chakraborty wrote:
> 
> On 3/16/12 11:27 AM, "Ben Hutchings" <bhutchings@solarflare.com> wrote:
> 
> >On Fri, 2012-03-16 at 10:58 -0700, Anirban Chakraborty wrote:
> >> +
> >>  struct ethtool_dump {
> >>  	__u32	cmd;
> >>  	__u32	version;
> >>  	__u32	flag;
> >>  	__u32	len;
> >>  	__u8	data[0];
> >> +	__u8	dump_state;
> >
> >Don't be ridiculous.
> 
> Yeah I know, especially when there is a flag field already present there.
> The only
> reason, we considered for adding it is to keep the backward compatibility
> of scripts.
> Right now, the flag field sets/gets the dump level of fw. If we use it to
> control the
> dump state, then it would break the existing scripts, if there are any.
> 

You missed the point... data[0] must be the last element in the
structure.

Re: [PATCH net-next] ethtool: Added a field fw dump_state

[Anirban Chakraborty]

On 3/16/12 2:22 PM, "Eric Dumazet" <eric.dumazet@gmail.com> wrote:

>On Fri, 2012-03-16 at 14:10 -0700, Anirban Chakraborty wrote:
>> 
>> On 3/16/12 11:27 AM, "Ben Hutchings" <bhutchings@solarflare.com> wrote:
>> 
>> >On Fri, 2012-03-16 at 10:58 -0700, Anirban Chakraborty wrote:
>> >> +
>> >>  struct ethtool_dump {
>> >>  	__u32	cmd;
>> >>  	__u32	version;
>> >>  	__u32	flag;
>> >>  	__u32	len;
>> >>  	__u8	data[0];
>> >> +	__u8	dump_state;
>> >
>> >Don't be ridiculous.
>> 
>> Yeah I know, especially when there is a flag field already present
>>there.
>> The only
>> reason, we considered for adding it is to keep the backward
>>compatibility
>> of scripts.
>> Right now, the flag field sets/gets the dump level of fw. If we use it
>>to
>> control the
>> dump state, then it would break the existing scripts, if there are any.
>> 
>
>You missed the point... data[0] must be the last element in the
>structure.
Yes, that was wrong struct. Sorry, I should have caught it earlier. We'll
resend it.

-Anirban

Re: [PATCH net-next] ethtool: Added a field fw dump_state

[Ben Hutchings]

On Fri, 2012-03-16 at 14:22 -0700, Eric Dumazet wrote:
> On Fri, 2012-03-16 at 14:10 -0700, Anirban Chakraborty wrote:
> > 
> > On 3/16/12 11:27 AM, "Ben Hutchings" <bhutchings@solarflare.com> wrote:
> > 
> > >On Fri, 2012-03-16 at 10:58 -0700, Anirban Chakraborty wrote:
> > >> +
> > >>  struct ethtool_dump {
> > >>  	__u32	cmd;
> > >>  	__u32	version;
> > >>  	__u32	flag;
> > >>  	__u32	len;
> > >>  	__u8	data[0];
> > >> +	__u8	dump_state;
> > >
> > >Don't be ridiculous.
> > 
> > Yeah I know, especially when there is a flag field already present there.
> > The only
> > reason, we considered for adding it is to keep the backward compatibility
> > of scripts.
> > Right now, the flag field sets/gets the dump level of fw. If we use it to
> > control the
> > dump state, then it would break the existing scripts, if there are any.
> > 
> 
> You missed the point... data[0] must be the last element in the
> structure.

Right.  And len is documented as not used by the ETHTOOL_SET_DUMP
command so we cannot say that when len == 1 then data[0] is this new
flag.

Just reserve some value of the flag to mean 'disable', say 0 or
0xffffffff.  If you want this 'disable' value to be understood by all
drivers and have ethtool support a keyword for it then also define a
macro for the value in ethtool.h.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] ethtool: Added a field fw dump_state

[Anirban Chakraborty]

On 3/16/12 2:37 PM, "Ben Hutchings" <bhutchings@solarflare.com> wrote:

>On Fri, 2012-03-16 at 14:22 -0700, Eric Dumazet wrote:
>> On Fri, 2012-03-16 at 14:10 -0700, Anirban Chakraborty wrote:
>> > 
>> > On 3/16/12 11:27 AM, "Ben Hutchings" <bhutchings@solarflare.com>
>>wrote:
>> > 
>> > >On Fri, 2012-03-16 at 10:58 -0700, Anirban Chakraborty wrote:
>> > >> +
>> > >>  struct ethtool_dump {
>> > >>  	__u32	cmd;
>> > >>  	__u32	version;
>> > >>  	__u32	flag;
>> > >>  	__u32	len;
>> > >>  	__u8	data[0];
>> > >> +	__u8	dump_state;
>> > >
>> > >Don't be ridiculous.
>> > 
>> > Yeah I know, especially when there is a flag field already present
>>there.
>> > The only
>> > reason, we considered for adding it is to keep the backward
>>compatibility
>> > of scripts.
>> > Right now, the flag field sets/gets the dump level of fw. If we use
>>it to
>> > control the
>> > dump state, then it would break the existing scripts, if there are
>>any.
>> > 
>> 
>> You missed the point... data[0] must be the last element in the
>> structure.
>
>Right.  And len is documented as not used by the ETHTOOL_SET_DUMP
>command so we cannot say that when len == 1 then data[0] is this new
>flag.
>
>Just reserve some value of the flag to mean 'disable', say 0 or
>0xffffffff.  If you want this 'disable' value to be understood by all
>drivers and have ethtool support a keyword for it then also define a
>macro for the value in ethtool.h.

Thanks for your comments. We'll take care of it next submittal.

-Anirban