| Message ID | 1331920711-16049-1-git-send-email-anirban.chakraborty@qlogic.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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 */ > > /**
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */ /**