Message-ID: <1394382740.2861.135.camel@deadeye.wl.decadent.org.uk>
Date:	Sun, 09 Mar 2014 16:32:20 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Vince Bridgers <vbridgers2013@...il.com>
Cc:	devicetree@...r.kernel.org, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, robh+dt@...nel.org, pawel.moll@....com,
	mark.rutland@....com, ijc+devicetree@...lion.org.uk,
	galak@...eaurora.org, rob@...dley.net
Subject: Re: [PATCH RFC 3/3] Altera TSE: Add Altera Triple Speed Ethernet
 (TSE) Driver

On Sun, 2014-03-02 at 14:42 -0600, Vince Bridgers wrote:
[....]
> --- /dev/null
> +++ b/drivers/net/ethernet/altera/altera_msgdma.c
[...]
> +void msgdma_reset(struct altera_tse_private *priv)
> +{
> +	int counter;
> +	struct msgdma_csr *txcsr =
> +		(struct msgdma_csr *)priv->tx_dma_csr;
> +	struct msgdma_csr *rxcsr =
> +		(struct msgdma_csr *)priv->rx_dma_csr;

Don't remove the __iomem qualifier here (or anywhere).

[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/altera/altera_sgdma.c
> @@ -0,0 +1,558 @@
> +/* Altera TSE SGDMA and MSGDMA Linux driver
> + * Copyright (C) 2014 Altera Corporation. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/list.h>
> +#include "altera_utils.h"
> +#include "altera_tse.h"
> +#include "altera_sgdmahw.h"
> +#include "altera_sgdma.h"
> +
> +static void sgdma_descrip(struct sgdma_descrip *desc,
> +			  struct sgdma_descrip *ndesc,
> +			  unsigned int ndesc_phys,
> +			  dma_addr_t raddr,
> +			  dma_addr_t waddr,
> +			  u16 length,
> +			  int generate_eop,
> +			  int rfixed,
> +			  int wfixed);

[...]
> +int sgdma_initialize(struct altera_tse_private *priv)
> +{
> +	priv->descripsz = sizeof(struct sgdma_descrip);
> +	priv->txdescrips = priv->txdescmem/priv->descripsz;
> +	priv->rxdescrips = priv->rxdescmem/priv->descripsz;
> +
> +	priv->txctrlreg = SGDMA_CTRLREG_ILASTD;
> +
> +	priv->rxctrlreg = SGDMA_CTRLREG_IDESCRIP |
> +		      SGDMA_CTRLREG_ILASTD;
> +
> +	INIT_LIST_HEAD(&priv->txlisthd);
> +	INIT_LIST_HEAD(&priv->rxlisthd);
> +
> +	priv->rxdescphys = (dma_addr_t) 0;
> +	priv->txdescphys = (dma_addr_t) 0;

0 is *not* a reserved DMA address...

[...]
> +	if (dma_mapping_error(priv->device, priv->rxdescphys)) {

...which is why you have to use dma_mapping_error() to check it...

[...]
> +void sgdma_uninitialize(struct altera_tse_private *priv)
> +{
> +	if (priv->rxdescphys)
> +		dma_unmap_single(priv->device, priv->rxdescphys,
> +				 priv->rxdescmem, DMA_BIDIRECTIONAL);
> +
> +	if (priv->txdescphys)
> +		dma_unmap_single(priv->device, priv->txdescphys,
> +				 priv->txdescmem, DMA_BIDIRECTIONAL);
> +}

...so these conditions are wrong.

Looking back up the function:

> +	priv->rxdescphys = dma_map_single(priv->device, priv->rx_dma_desc,
> +					  priv->rxdescmem, DMA_BIDIRECTIONAL);
> +

rx_dma_desc is an MMIO pointer (similarly, tx_dma_desc).  What makes you
think that it's valid to DMA-map that?  It seems like you only use
rxdescmem with DMA sync functions, which suggests that the descriptor
access is very much reliant on quirks of the current NIOS port.

[...]
> +/* status is returned on upper 16 bits,
> + * length is returned in lower 16 bits
> + */
> +u32 sgdma_rx_status(struct altera_tse_private *priv)
> +{
> +	struct sgdma_csr *csr = (struct sgdma_csr *)priv->rx_dma_csr;
> +	struct sgdma_descrip *base = (struct sgdma_descrip *)priv->rx_dma_desc;
> +	struct sgdma_descrip *desc = NULL;
> +	int pktsrx;
> +	unsigned int rxstatus = 0;
> +	unsigned int pktlength = 0;
> +	unsigned int pktstatus = 0;
> +	struct tse_buffer *rxbuffer = NULL;
> +
> +	dma_sync_single_for_cpu(priv->device,
> +				priv->rxdescphys,
> +				priv->rxdescmem,
> +				DMA_BIDIRECTIONAL);

You want to sync *all* your RX buffers, every time you receive a packet?

> +	/* Issue read memory barrier before accessing rx descriptor */
> +	rmb();

Yes but why?  What is the prior read operation?

[...]
> +/* Private functions */
> +static void sgdma_descrip(struct sgdma_descrip *desc,
> +			  struct sgdma_descrip *ndesc,
> +			  unsigned int ndesc_phys,
> +			  dma_addr_t raddr,
> +			  dma_addr_t waddr,
> +			  u16 length,
> +			  int generate_eop,
> +			  int rfixed,
> +			  int wfixed)

This seems to be an unwieldy number of parameters, and the last three
are clearly redundant with each other as you end up doing:

[...]
> +	ctrl |= generate_eop;
> +	ctrl |= rfixed;
> +	ctrl |= wfixed;

[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/altera/altera_tse.c
[...]
> +#define RX_DESCRIPTORS 64
> +static int dma_rx_num = RX_DESCRIPTORS;
> +module_param(dma_rx_num, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(dma_rx_num, "Number of descriptors in the RX list");
> +
> +#define TX_DESCRIPTORS 64
> +static int dma_tx_num = TX_DESCRIPTORS;
> +module_param(dma_tx_num, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(dma_tx_num, "Number of descriptors in the TX list");

Module parameters for this are not acceptable.  Implement the ethtool
{get,set}_ringparam operations instead.

[...]
> +static void tse_free_tx_buffer(struct altera_tse_private *priv,
> +			       struct tse_buffer *buffer)
> +{
> +	if (buffer->dma_addr) {
> +		if (buffer->mapped_as_page)

The mapped_as_page field is never set, so remove the field and this
condition.

> +			dma_unmap_page(priv->device, buffer->dma_addr,
> +				       buffer->len, DMA_TO_DEVICE);
> +		else
> +			dma_unmap_single(priv->device, buffer->dma_addr,
> +					 buffer->len, DMA_TO_DEVICE);
> +		buffer->dma_addr = 0;
> +	}
> +	if (buffer->skb) {
> +		dev_kfree_skb_any(buffer->skb);
> +		buffer->skb = NULL;
> +	}
> +}
[...]
> +/* NAPI polling function
> + */
> +static int tse_poll(struct napi_struct *napi, int budget)
> +{
> +	struct altera_tse_private *priv =
> +			container_of(napi, struct altera_tse_private, napi);
> +	int rxcomplete = 0;
> +	int txcomplete = 0;
> +	unsigned long int flags;
> +
> +	txcomplete = tse_tx_complete(priv);
> +
> +	rxcomplete = tse_rx(priv, budget);
> +
> +	if (txcomplete+rxcomplete != budget) {
> +		napi_gro_flush(napi, false);
> +		__napi_complete(napi);
> +
> +		dev_dbg(priv->device,
> +			"NAPI Complete, did %d packets with budget %d\n",
> +			txcomplete+rxcomplete, budget);
> +	}
> +	spin_lock_irqsave(&priv->rxdma_irq_lock, flags);
> +	priv->enable_rxirq(priv);
> +	priv->enable_txirq(priv);
> +	spin_unlock_irqrestore(&priv->rxdma_irq_lock, flags);
> +	return rxcomplete + txcomplete;

So you count all the TX and RX completions, yet TX completion handling
is *not* limited by the budget.

I'm surprised this doesn't immediately produce warning messages from
NAPI.  You must *never* return a value greater than budget, and you
shou;ld not count TX completions.

> +}
[...]
> +static int tse_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +	struct altera_tse_private *priv = netdev_priv(dev);
> +	unsigned int max_mtu = priv->max_mtu;
> +	unsigned int min_mtu = ETH_ZLEN + ETH_FCS_LEN;
> +
> +	if (netif_running(dev)) {
> +		dev_err(priv->device, "must be stopped to change its MTU\n");
> +		return -EBUSY;
> +	}

This is true for many devices, but that is your problem as the driver
writer.  You should restart the hardware if necessary.

[...]
> +static void altera_tse_set_mcfilter(struct net_device *dev)
> +{
> +	struct altera_tse_private *priv = netdev_priv(dev);
> +	struct altera_tse_mac *mac = (struct altera_tse_mac *)priv->mac_dev;
> +	int i;
> +	struct netdev_hw_addr *ha;
> +
> +	/* clear the hash filter */
> +	for (i = 0; i < 64; i++)
> +		iowrite32(0, &(mac->hash_table[i]));

This means you drop all multicast traffic for a while, every time a new
multicast address is added.

Instead, you should construct a complete new bitmap before updating the
filter.

[...]
> +static int altera_tse_get_phy_iface_prop(struct platform_device *pdev,
> +					 phy_interface_t *iface)
> +{
> +	const void *prop;
> +	int len;
> +
> +	prop = of_get_property(pdev->dev.of_node, "phy-mode", &len);
> +	if (!prop)
> +		return -ENOENT;
> +	if (len < 4)
> +		return -EINVAL;
> +
> +	if (!strncmp((char *)prop, "mii", 3)) {
> +		*iface = PHY_INTERFACE_MODE_MII;
> +		return 0;
> +	} else if (!strncmp((char *)prop, "gmii", 4)) {
> +		*iface = PHY_INTERFACE_MODE_GMII;
> +		return 0;
> +	} else if (!strncmp((char *)prop, "rgmii-id", 8)) {
> +		*iface = PHY_INTERFACE_MODE_RGMII_ID;
> +		return 0;
> +	} else if (!strncmp((char *)prop, "rgmii", 5)) {
> +		*iface = PHY_INTERFACE_MODE_RGMII;
> +		return 0;
> +	} else if (!strncmp((char *)prop, "sgmii", 5)) {
> +		*iface = PHY_INTERFACE_MODE_SGMII;
> +		return 0;
> +	}

Is there really no common function for this?  Maybe you should add one.

> +	return -EINVAL;
> +}
> +
> +static int request_and_map(struct platform_device *pdev, const char *name,
> +			   struct resource **res, void __iomem **ptr)

The fact that you return the MMIO pointer by reference is problematic,
because the callers actually want to initialise pointers to specific
structure types.  They have to cast, and in fact they do the cast
wrongly as they don't include the __iomem qualifier.

Perhaps you could change this function to return the MMIO pointer
directly and use ERR_PTR for errors?

[...]
> +	ndev->mem_start = control_port->start;
> +	ndev->mem_end = control_port->end;

Not necessary, these fields are for ancient ISA crap where the user
might have to tell the driver which addresses to use.

[...]
> +	altera_tse_netdev_ops.ndo_set_rx_mode = tse_set_rx_mode;
> +
> +	if (priv->hash_filter)
> +		altera_tse_netdev_ops.ndo_set_rx_mode =
> +			tse_set_rx_mode_hashfilter;

Now imagine someone uses two instances of this IP block with different
capabilities.

[...]
> +/* Remove Altera TSE MAC device
> + */
> +static int altera_tse_remove(struct platform_device *pdev)
> +{
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +
> +	platform_set_drvdata(pdev, NULL);

Redundant, driver core does this.

> +	if (ndev) {
> +		altera_tse_mdio_destroy(ndev);

I think this needs to be done after unregister_netdev(), but I'm not
sure.

> +		netif_carrier_off(ndev);

Redundant.

> +		unregister_netdev(ndev);
> +		free_netdev(ndev);
> +	}
> +
> +	return 0;
> +}
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/altera/altera_tse_ethtool.c
[...]
> +#define TSE_STATS_LEN 31
> +
> +static char stat_gstrings[][ETH_GSTRING_LEN] = {

Please either use TSE_STATS_LEN as the outer dimension, or define
TSE_STATS_LEN using ARRAY_SIZE().

[...]
> +static void tse_get_drvinfo(struct net_device *dev,
> +			    struct ethtool_drvinfo *info)
> +{
> +	struct altera_tse_private *priv = netdev_priv(dev);
> +	u32 rev = ioread32(&priv->mac_dev->megacore_revision);
> +
> +	strcpy(info->driver, "Altera TSE MAC IP Driver");

This should match the driver name elsewhere, i.e.
ALTERA_TSE_RESOURCE_NAME.

> +	strcpy(info->version, "v8.0");
> +	snprintf(info->fw_version, ETHTOOL_FWVERS_LEN, "v%d.%d",
> +		 rev & 0xFFFF, (rev & 0xFFFF0000) >> 16);

That appears to be a hardware version, not a firmware version.

> +	sprintf(info->bus_info, "AVALON");

I don't think that's helpful.  Either put some sort of address here or
leave it blank.

> +}
[...]
> +static int tse_sset_count(struct net_device *dev, int sset)
> +{
> +	switch (sset) {
> +	case ETH_SS_STATS:
> +		return TSE_STATS_LEN;
> +	default:
> +		return -EOPNOTSUPP;

EINVAL

> +	}
> +}
> +
[...]
> +static int tse_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +	struct altera_tse_private *priv = netdev_priv(dev);
> +	struct phy_device *phydev = priv->phydev;
> +
> +	if (phydev == NULL)
> +		return -ENODEV;

EOPNOTSUPP

> +	return phy_ethtool_gset(phydev, cmd);
> +}
> +
> +static int tse_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +	struct altera_tse_private *priv = netdev_priv(dev);
> +	struct phy_device *phydev = priv->phydev;
> +
> +	if (phydev == NULL)
> +		return -ENODEV;

EOPNOTSUPP

> +	return phy_ethtool_sset(phydev, cmd);
> +}
[...]

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)

Powered by blists - more mailing lists