Re: [gianfar PATCH] RFC v2: Add rx_ntuple feature

[Ben Hutchings <bhutchings@solarflare.com>]

On Tue, 2011-05-17 at 15:18 +0200, Sebastian Pöhn wrote:
> This patch implements rx ntuple filtering for the gianfar driver.
> Changes since last version:
> #Added code is now re-entrant
> #Consolidation of convert routines
> 
> I am planing to do some final testing. After that I hope I can send the
> final patch.
> 
> Signed-off-by: Sebastian Poehn <sebastian.poehn@belden.com>
> ---
> 
>  drivers/net/gianfar.c         |   16 +-
>  drivers/net/gianfar.h         |   44 ++
>  drivers/net/gianfar_ethtool.c | 1006
> +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1062 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index ff60b23..ddd4007 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
[...]
> @@ -1042,6 +1048,9 @@ static int gfar_probe(struct platform_device *ofdev)
>  	if (priv->device_flags & FSL_GIANFAR_DEV_HAS_VLAN)
>  		dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
> 
> +	if (priv->device_flags & FSL_GIANFAR_DEV_HAS_RX_FILER)
> +		dev->features |= NETIF_F_NTUPLE;
> +

It should be possible to turn this feature on and off, so add it to
hw_features and handle it in gfar_set_features() by clearing filters
when it's being turned off.

[...]
> --- a/drivers/net/gianfar.h
> +++ b/drivers/net/gianfar.h
[...]
> @@ -1066,6 +1069,9 @@ struct gfar_private {
> 
>  	struct vlan_group *vlgrp;
> 
> +	/* RX queue filer rule set*/
> +	struct ethtool_rx_ntuple_list ntuple_list;
> +	struct mutex rx_queue_access;
> 
>  	/* Hash registers and their width */
>  	u32 __iomem *hash_regs[16];

Don't use struct ethtool_rx_ntuple_list; it is going to be removed once
ixgbe is converted to implement RX NFC.

Really, at this point I would say: please implement RX NFC, not RX
n-tuple.

> @@ -1140,6 +1146,16 @@ static inline void gfar_write_filer(struct
> gfar_private *priv,
>  	gfar_write(&regs->rqfpr, fpr);
>  }
> 
> +static inline void gfar_read_filer(struct gfar_private *priv,
> +		unsigned int far, unsigned int *fcr, unsigned int *fpr)
> +{
> +	struct gfar __iomem *regs = priv->gfargrp[0].regs;
> +
> +	gfar_write(&regs->rqfar, far);
> +	*fcr = gfar_read(&regs->rqfcr);
> +	*fpr = gfar_read(&regs->rqfpr);
> +}
> +
>  extern void lock_rx_qs(struct gfar_private *priv);
>  extern void lock_tx_qs(struct gfar_private *priv);
>  extern void unlock_rx_qs(struct gfar_private *priv);
> @@ -1157,4 +1173,32 @@ int gfar_set_features(struct net_device *dev, u32
> features);
> 
>  extern const struct ethtool_ops gfar_ethtool_ops;
> 
> +#define ESWFULL 160
> +#define EHWFULL 161
> +#define EOUTOFRANGE 162

You can't just make up new error codes like this.

[...]
> --- a/drivers/net/gianfar_ethtool.c
> +++ b/drivers/net/gianfar_ethtool.c
[...]
> +static int gfar_add_table_entry(struct ethtool_rx_ntuple_flow_spec *flow,
> +		struct ethtool_rx_ntuple_list *list)
> +{
> +	struct ethtool_rx_ntuple_flow_spec_container *temp;
> +	temp = kmalloc(sizeof(struct ethtool_rx_ntuple_flow_spec_container),
> +			GFP_KERNEL);
> +	if (temp == NULL)
> +		return -ENOMEM;
> +	memcpy(&temp->fs, flow, sizeof(struct ethtool_rx_ntuple_flow_spec));
> +	list_add_tail(&temp->list, &list->list);
> +	list->count++;
> +
> +	if (~flow->data_mask)
> +		printk(KERN_WARNING
> +			"User-specific data is not supported by hardware!\n");
> +	if (flow->flow_type == IP_USER_FLOW)
> +		if (flow->m_u.usr_ip4_spec.ip_ver != 255)
> +			printk(KERN_WARNING
> +				"IP-Version is not supported by hardware!\n");

That's not right; the value of ip_ver must be 4 and the mask must be 0.

[...]
> +static int gfar_process_filer_changes(struct ethtool_rx_ntuple_flow_spec
> *flow,
> +		struct gfar_private *priv)
> +{
> +	struct ethtool_rx_ntuple_flow_spec_container *j;
> +	struct filer_table *tab;
> +	s32 i = 0;
> +	s32 ret = 0;
> +
> +	/*So index is set to zero, too!*/
> +	tab = kzalloc(sizeof(struct filer_table), GFP_KERNEL);
> +	if (tab == NULL) {
> +		printk(KERN_WARNING "Can not get memory!\n");
> +		return -ENOMEM;
> +	}
> +
> +	j = gfar_search_table_entry(flow, &priv->ntuple_list);
> +
> +	if (flow->action == ETHTOOL_RXNTUPLE_ACTION_CLEAR) {
> +		if (j != NULL)
> +			gfar_del_table_entry(j, &priv->ntuple_list);
> +		else {
> +			printk(KERN_WARNING
> +			"Deleting this rule is not possible,"
> +			" because it does not exist!\n");
> +			return -1;

-1 is -EPERM.  You should return -ENOENT and not log anything.

> +		}
> +	} else if (j != NULL) {
> +		printk(KERN_WARNING "Adding this rule is not possible,"
> +			" because it already exists!\n");
> +		return -1;

You should return -EEXIST and not log anything.

> +	}
> +
> +	/*Now convert the existing filer data from flow_spec into
> +	* filer tables binary format*/
> +	list_for_each_entry(j, &priv->ntuple_list.list, list) {
> +		ret = gfar_convert_to_filer(&j->fs, tab);
> +		if (ret == -ESWFULL) {
> +			printk(KERN_WARNING
> +			"Adding this rule is not possible,"
> +			" because there is not space left!\n");
> +			goto end;
> +		}
> +	}
[...]

I think the error code should be -EBUSY if the hardware or software
table is full.

Also, please run checkpatch.pl over your changes and fix the style
errors it finds.

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.