| Message ID | 1308071606-6333-1-git-send-email-carolyn.wyborny@intel.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Carolyn Wyborny wrote: > This RFC patch adds functions to get and set DMA Coalescing feature > configuration. > > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> > --- > include/linux/ethtool.h | 15 ++++++++++++++- > net/core/ethtool.c | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+), 1 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index c6a850a..efed754 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -703,6 +703,14 @@ enum ethtool_sfeatures_retval_bits { > ETHTOOL_F_COMPAT__BIT, > }; > > +/* for configuring DMA coalescing parameters of chip */ > +struct ethtool_dmac { > + __u32 cmd; /* ETHTOOL_{G,S}DMAC */ > + > + /* tune setting, options and validation determined by device. */ > + __u32 tune; > +}; [...] I don't think we should be adding operations that have no generic semantics at all. Further, we will not add any operations without at least one implementation as an example. Secondly, ethtool operations that only get/set a 32-bit value should use struct ethtool_value for the parameter. Ben.
>-----Original Message----- >From: Ben Hutchings [mailto:bhutchings@solarflare.com] >Sent: Tuesday, June 14, 2011 11:52 AM >To: Wyborny, Carolyn >Cc: netdev@vger.kernel.org >Subject: Re: [RFC 2/2] ethtool: Add support for DMA Coalescing feature >config to ethtool. > >Carolyn Wyborny wrote: >> This RFC patch adds functions to get and set DMA Coalescing feature >> configuration. >> >> Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> >> --- >> include/linux/ethtool.h | 15 ++++++++++++++- >> net/core/ethtool.c | 32 ++++++++++++++++++++++++++++++++ >> 2 files changed, 46 insertions(+), 1 deletions(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index c6a850a..efed754 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -703,6 +703,14 @@ enum ethtool_sfeatures_retval_bits { >> ETHTOOL_F_COMPAT__BIT, >> }; >> >> +/* for configuring DMA coalescing parameters of chip */ >> +struct ethtool_dmac { >> + __u32 cmd; /* ETHTOOL_{G,S}DMAC */ >> + >> + /* tune setting, options and validation determined by device. */ >> + __u32 tune; >> +}; >[...] > >I don't think we should be adding operations that have no generic >semantics at all. Further, we will not add any operations without at >least one implementation as an example. > >Secondly, ethtool operations that only get/set a 32-bit value should >use struct ethtool_value for the parameter. > >Ben. > >-- >Ben Hutchings, Senior Software Engineer, Solarflare Communications >Not speaking for my employer; that's the marketing department's job. >They asked us to note that Solarflare product names are trademarked. Ok, will send up update with the suggested changes and an implementation as an example. I have one, but will wait to synch it with the updated patch. Do you want another RFC or a regular patch submission? Thanks, Carolyn Carolyn Wyborny Linux Development LAN Access Division Intel Corporation -- 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 Tue, 2011-06-14 at 13:19 -0700, Wyborny, Carolyn wrote: [...] > Ok, will send up update with the suggested changes and an > implementation as an example. I have one, but will wait to synch it > with the updated patch. Do you want another RFC or a regular patch > submission? That's really for you to decide. Ben.
From: Carolyn Wyborny <carolyn.wyborny@intel.com> Date: Tue, 14 Jun 2011 10:13:26 -0700 > This RFC patch adds functions to get and set DMA Coalescing feature > configuration. > > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> What in the world is "DMA Coalescing feature", what does it do? Might it want parameters and not just want to be turned on or off? This change, at a minimum, needs a more descriptive commit log entry. So that other driver authors can understand what exactly the feature is and therefore whether they might like to add support for it to their drivers. -- 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
>-----Original Message----- >From: David Miller [mailto:davem@davemloft.net] >Sent: Thursday, June 16, 2011 8:34 PM >To: Wyborny, Carolyn >Cc: netdev@vger.kernel.org; bhutchings@solarflare.com >Subject: Re: [RFC 2/2] ethtool: Add support for DMA Coalescing feature >config to ethtool. > >From: Carolyn Wyborny <carolyn.wyborny@intel.com> >Date: Tue, 14 Jun 2011 10:13:26 -0700 > >> This RFC patch adds functions to get and set DMA Coalescing feature >> configuration. >> >> Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> > >What in the world is "DMA Coalescing feature", what does it do? > >Might it want parameters and not just want to be turned on or off? > >This change, at a minimum, needs a more descriptive commit log entry. >So that other driver authors can understand what exactly the feature >is and therefore whether they might like to add support for it to >their drivers. I will add a fuller description of the feature in my updated patch. I thought the feature was more well known. Quick description is that it's a power saving feature that causes the adapter to coalesce its DMA writes at low traffic times to save power on the platform by reducing wakeups. The parameter is intended as a simple u32 value, not just an on or off, but also to allow a variety of configuration by adapter vendors, with validation of the input on the driver side. Since I left out the implementation in my patch, this wasn't clear. I will also fix this in my next submission. Thanks, Carolyn Carolyn Wyborny Linux Development LAN Access Division Intel Corporation -- 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
From: "Wyborny, Carolyn" <carolyn.wyborny@intel.com> Date: Fri, 17 Jun 2011 08:50:11 -0700 > I will add a fuller description of the feature in my updated patch. > I thought the feature was more well known. Quick description is that > it's a power saving feature that causes the adapter to coalesce its > DMA writes at low traffic times to save power on the platform by > reducing wakeups. The parameter is intended as a simple u32 value, > not just an on or off, but also to allow a variety of configuration > by adapter vendors, with validation of the input on the driver side. > Since I left out the implementation in my patch, this wasn't clear. > I will also fix this in my next submission. The value cannot have adapter specific meaning, you must define it precisely and in a generic manner, such that the user can specify the same setting across different card types. -- 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
>-----Original Message----- >From: David Miller [mailto:davem@davemloft.net] >Sent: Friday, June 17, 2011 11:54 AM >To: Wyborny, Carolyn >Cc: netdev@vger.kernel.org; bhutchings@solarflare.com >Subject: Re: [RFC 2/2] ethtool: Add support for DMA Coalescing feature >config to ethtool. > >From: "Wyborny, Carolyn" <carolyn.wyborny@intel.com> >Date: Fri, 17 Jun 2011 08:50:11 -0700 > >> I will add a fuller description of the feature in my updated patch. >> I thought the feature was more well known. Quick description is that >> it's a power saving feature that causes the adapter to coalesce its >> DMA writes at low traffic times to save power on the platform by >> reducing wakeups. The parameter is intended as a simple u32 value, >> not just an on or off, but also to allow a variety of configuration >> by adapter vendors, with validation of the input on the driver side. >> Since I left out the implementation in my patch, this wasn't clear. >> I will also fix this in my next submission. > >The value cannot have adapter specific meaning, you must define it >precisely and in a generic manner, such that the user can specify the >same setting across different card types. Ok, good point. I will refine the definition of the parameter in the next submission, once the dust clears on the major revisions in progress. Thanks, Carolyn Carolyn Wyborny Linux Development LAN Access Division Intel Corporation -- 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 Tue, 2011-06-21 at 10:23 -0700, Wyborny, Carolyn wrote: > > >-----Original Message----- > >From: David Miller [mailto:davem@davemloft.net] > >Sent: Friday, June 17, 2011 11:54 AM > >To: Wyborny, Carolyn > >Cc: netdev@vger.kernel.org; bhutchings@solarflare.com > >Subject: Re: [RFC 2/2] ethtool: Add support for DMA Coalescing feature > >config to ethtool. > > > >From: "Wyborny, Carolyn" <carolyn.wyborny@intel.com> > >Date: Fri, 17 Jun 2011 08:50:11 -0700 > > > >> I will add a fuller description of the feature in my updated patch. > >> I thought the feature was more well known. Quick description is that > >> it's a power saving feature that causes the adapter to coalesce its > >> DMA writes at low traffic times to save power on the platform by > >> reducing wakeups. The parameter is intended as a simple u32 value, > >> not just an on or off, but also to allow a variety of configuration > >> by adapter vendors, with validation of the input on the driver side. > >> Since I left out the implementation in my patch, this wasn't clear. > >> I will also fix this in my next submission. > > > >The value cannot have adapter specific meaning, you must define it > >precisely and in a generic manner, such that the user can specify the > >same setting across different card types. > > Ok, good point. I will refine the definition of the parameter in the > next submission, once the dust clears on the major revisions in > progress. You may wish to propose a new command structure that covers both IRQ and DMA moderation. They seem to be related, since DMA cannot be delayed longer than the corresponding IRQ. We are currently lacking a way to specify different IRQ moderation for multiqueue devices where the queues are not all used in the same way. Ben.
>-----Original Message----- >From: Ben Hutchings [mailto:bhutchings@solarflare.com] >Sent: Tuesday, June 21, 2011 10:38 AM >To: Wyborny, Carolyn >Cc: David Miller; netdev@vger.kernel.org >Subject: RE: [RFC 2/2] ethtool: Add support for DMA Coalescing feature >config to ethtool. > >On Tue, 2011-06-21 at 10:23 -0700, Wyborny, Carolyn wrote: >> >> >-----Original Message----- >> >From: David Miller [mailto:davem@davemloft.net] >> >Sent: Friday, June 17, 2011 11:54 AM >> >To: Wyborny, Carolyn >> >Cc: netdev@vger.kernel.org; bhutchings@solarflare.com >> >Subject: Re: [RFC 2/2] ethtool: Add support for DMA Coalescing >feature >> >config to ethtool. >> > >> >From: "Wyborny, Carolyn" <carolyn.wyborny@intel.com> >> >Date: Fri, 17 Jun 2011 08:50:11 -0700 >> > >> >> I will add a fuller description of the feature in my updated patch. >> >> I thought the feature was more well known. Quick description is >that >> >> it's a power saving feature that causes the adapter to coalesce its >> >> DMA writes at low traffic times to save power on the platform by >> >> reducing wakeups. The parameter is intended as a simple u32 value, >> >> not just an on or off, but also to allow a variety of configuration >> >> by adapter vendors, with validation of the input on the driver >side. >> >> Since I left out the implementation in my patch, this wasn't clear. >> >> I will also fix this in my next submission. >> > >> >The value cannot have adapter specific meaning, you must define it >> >precisely and in a generic manner, such that the user can specify the >> >same setting across different card types. >> >> Ok, good point. I will refine the definition of the parameter in the >> next submission, once the dust clears on the major revisions in >> progress. > >You may wish to propose a new command structure that covers both IRQ and >DMA moderation. They seem to be related, since DMA cannot be delayed >longer than the corresponding IRQ. We are currently lacking a way to >specify different IRQ moderation for multiqueue devices where the queues >are not all used in the same way. > >Ben. > >-- >Ben Hutchings, Senior Software 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. I will try to do this. Confirming you are suggesting a replacement or an alternate for the current -c/-C coalesce settings with perhaps some room reserved for some future coalescing type features and enable settings per queue? I agree they are related and initially considered trying to add DMAC to the current coalescing settings, but they are full. Carolyn Carolyn Wyborny Linux Development LAN Access Division Intel Corporation
On Thu, 2011-06-30 at 10:52 -0700, Wyborny, Carolyn wrote: [...] > >You may wish to propose a new command structure that covers both IRQ and > >DMA moderation. They seem to be related, since DMA cannot be delayed > >longer than the corresponding IRQ. We are currently lacking a way to > >specify different IRQ moderation for multiqueue devices where the queues > >are not all used in the same way. > > > >Ben. > > > >-- > >Ben Hutchings, Senior Software 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. > > I will try to do this. Confirming you are suggesting a replacement or > an alternate for the current -c/-C coalesce settings with perhaps some > room reserved for some future coalescing type features and enable > settings per queue? [...] Yes. I'm not insisting that you must do this instead of just defining the operations for DMA coalescing, but it would be helpful. Ben.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index c6a850a..efed754 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -703,6 +703,14 @@ enum ethtool_sfeatures_retval_bits { ETHTOOL_F_COMPAT__BIT, }; +/* for configuring DMA coalescing parameters of chip */ +struct ethtool_dmac { + __u32 cmd; /* ETHTOOL_{G,S}DMAC */ + + /* tune setting, options and validation determined by device. */ + __u32 tune; +}; + #define ETHTOOL_F_UNSUPPORTED (1 << ETHTOOL_F_UNSUPPORTED__BIT) #define ETHTOOL_F_WISH (1 << ETHTOOL_F_WISH__BIT) #define ETHTOOL_F_COMPAT (1 << ETHTOOL_F_COMPAT__BIT) @@ -877,6 +885,8 @@ bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported); * and flag of the device. * @get_dump_data: Get dump data. * @set_dump: Set dump specific flags to the device. + * @get_dmac: Get DMA Coalescing setting from device. + * @set_dmac: Set DMA Coalescing setting. * * All operations are optional (i.e. the function pointer may be set * to %NULL) and callers must take this into account. Callers must @@ -955,7 +965,8 @@ struct ethtool_ops { int (*get_dump_data)(struct net_device *, struct ethtool_dump *, void *); int (*set_dump)(struct net_device *, struct ethtool_dump *); - + void (*get_dmac)(struct net_device *, struct ethtool_dmac *); + int (*set_dmac)(struct net_device *, struct ethtool_dmac *); }; #endif /* __KERNEL__ */ @@ -1029,6 +1040,8 @@ struct ethtool_ops { #define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */ #define ETHTOOL_GET_DUMP_FLAG 0x0000003f /* Get dump settings */ #define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ +#define ETHTOOL_GDMAC 0X00000041 /* Get DMA Coalsce settings */ +#define ETHTOOL_SDMAC 0X00000042 /* Set DMA Coalsce settings */ /* compatibility with older code */ #define SPARC_ETH_GSET ETHTOOL_GSET diff --git a/net/core/ethtool.c b/net/core/ethtool.c index fd14116..0f122d3 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1926,6 +1926,32 @@ out: vfree(data); return ret; } +static int ethtool_set_dmac(struct net_device *dev, void __user *useraddr) +{ + struct ethtool_dmac dmac; + + if (!dev->ethtool_ops->set_dmac) + return -EOPNOTSUPP; + + if (copy_from_user(&dmac, useraddr, sizeof(dmac))) + return -EFAULT; + + return dev->ethtool_ops->set_dmac(dev, &dmac); +} + +static int ethtool_get_dmac(struct net_device *dev, void __user *useraddr) +{ + struct ethtool_dmac dmac = { .cmd = ETHTOOL_GDMAC }; + + if (!dev->ethtool_ops->get_dmac) + return -EOPNOTSUPP; + + dev->ethtool_ops->get_dmac(dev, &dmac); + + if (copy_to_user(useraddr, &dmac, sizeof(dmac))) + return -EFAULT; + return 0; +} /* The main entry point in this file. Called from net/core/dev.c */ @@ -2152,6 +2178,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) case ETHTOOL_GET_DUMP_DATA: rc = ethtool_get_dump_data(dev, useraddr); break; + case ETHTOOL_GDMAC: + rc = ethtool_get_dmac(dev, useraddr); + break; + case ETHTOOL_SDMAC: + rc = ethtool_set_dmac(dev, useraddr); + break; default: rc = -EOPNOTSUPP; }
This RFC patch adds functions to get and set DMA Coalescing feature configuration. Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> --- include/linux/ethtool.h | 15 ++++++++++++++- net/core/ethtool.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletions(-)