Date:	Mon, 9 Jun 2014 00:19:41 +0200
From:	Francois Romieu <romieu@...zoreil.com>
To:	Andy Green <andy.green@...aro.org>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] net-next: ethernet driver: Fujist OGMA

Andy Green <andy.green@...aro.org> :
[...]

Partial review below. The driver needs more.

> diff --git a/drivers/net/ethernet/fujitsu/ogma/ogma.h b/drivers/net/ethernet/fujitsu/ogma/ogma.h
> new file mode 100644
> index 0000000..126e458
> --- /dev/null
> +++ b/drivers/net/ethernet/fujitsu/ogma/ogma.h
[...]
> +struct ogma_priv {
> +	u32 core_enabled_flag:1;
> +	u32 gmac_rx_running_flag:1;
> +	u32 gmac_tx_running_flag:1;
> +	u32 gmac_mode_valid_flag:1;
> +	u32 normal_desc_ring_valid:1;
> +	void *base_addr;

Add __iomem annotation.

Please rename it (ioaddr, whatever). It looks like net_device.base_addr

[...]
> +static inline void ogma_write_reg(struct ogma_priv *priv, u32 reg_addr,
> +				    u32 value)
> +{
> +	writel(value, (priv->base_addr) + (reg_addr << 2));

Excess parenthesis. The driver contains quite some of those.

> +}
> +
> +static inline u32 ogma_read_reg(struct ogma_priv *priv, u32 reg_addr)
> +{
> +	return readl(priv->base_addr + (reg_addr << 2));
> +}
> +
> +static inline void pfdep_set_pkt_buf_type(void *skb, bool recv_buf_flag)
> +{
> +	struct sk_buff *skb_p = (struct sk_buff *)skb;
> +	(*((bool *) skb_p->cb)) = recv_buf_flag;

Missing empty line.

[...]
> +extern const struct net_device_ops ogma_netdev_ops;
> +extern const struct ethtool_ops ogma_ethtool_ops;
> +
> +int ogma_start_gmac(struct ogma_priv *priv, bool rx_flag, bool tx_flag);
> +int ogma_stop_gmac(struct ogma_priv *priv, bool rx_flag, bool tx_flag);
> +int ogma_set_gmac_mode(struct ogma_priv *priv,
> +		       const struct ogma_gmac_mode *mode);
> +void ogma_set_phy_reg(struct ogma_priv *priv, u8 phy_addr, u8 reg_addr,
> +		      u16 value);
> +u16 ogma_get_phy_reg(struct ogma_priv *priv, u8 phy_addr, u8 reg_addr);
> +int ogma_start_desc_ring(struct ogma_priv *priv, unsigned int id);
> +int ogma_stop_desc_ring(struct ogma_priv *priv, unsigned int id);
> +u16 ogma_get_rx_num(struct ogma_priv *priv, unsigned int id);
> +u16 ogma_get_tx_avail_num(struct ogma_priv *priv, unsigned int id);
> +int ogma_clean_tx_desc_ring(struct ogma_priv *priv, unsigned int id);
> +int ogma_clean_rx_desc_ring(struct ogma_priv *priv, unsigned int id);
> +int ogma_set_tx_pkt_data(struct ogma_priv *priv, unsigned int id,
> +	const struct ogma_tx_pkt_ctrl *tx_ctrl, u8 scat_num,
> +		const struct ogma_frag_info *scat, struct sk_buff *skb);
> +int ogma_get_rx_pkt_data(struct ogma_priv *priv, unsigned int id,
> +	struct ogma_rx_pkt_info *rx_pkt_info_p, struct ogma_frag_info *frag,
> +						u16 *len, struct sk_buff **skb);
> +int ogma_ring_irq_enable(struct ogma_priv *priv,
> +			      unsigned int id, u32 irq_factor);
> +int ogma_ring_irq_disable(struct ogma_priv *priv,
> +			       unsigned int id, u32 irq_factor);
> +int ogma_set_irq_coalesce_param(struct ogma_priv *priv, unsigned int id,
> +		u16 int_pktcnt, bool int_tmr_unit_ms_flag, u16 int_tmr_cnt);
> +
> +int ogma_alloc_desc_ring(struct ogma_priv *priv, unsigned int id);
> +void ogma_free_desc_ring(struct ogma_priv *priv, struct ogma_desc_ring *desc);
> +int ogma_setup_rx_desc_ring(struct ogma_priv *priv,
> +						struct ogma_desc_ring *desc);
> +int ogma_netdev_napi_poll(struct napi_struct *napi_p, int budget);

Excess public scope ?

> diff --git a/drivers/net/ethernet/fujitsu/ogma/ogma_desc_ring_access.c b/drivers/net/ethernet/fujitsu/ogma/ogma_desc_ring_access.c
> new file mode 100644
> index 0000000..f11a834
> --- /dev/null
> +++ b/drivers/net/ethernet/fujitsu/ogma/ogma_desc_ring_access.c
[...]
> +static void desc_lock(struct ogma_desc_ring *desc, bool *ctx)
> +{
> +	if (in_softirq()) {
> +		*ctx = 1;	/* Mark that bh is already disabled. */
> +		spin_lock(&desc->spinlock_desc);
> +	} else {
> +		*ctx = 0;
> +		spin_lock_bh(&desc->spinlock_desc);
> +	}
> +}

Device drivers should not need this kind of locking.

Let's see:
- ogma_ring_irq_enable
  Only called from the xmit handler, whence in locally disabled bh context.

- ogma_start_desc_ring
  Only called through ogma_netdev_ops.ndo_open, thus in bh enabled context.
  One may ask if open() _really_ needs this lock anyway as it can delay
  both napi processing and tx queueing.

- ogma_stop_desc_ring
  It's called in more places than ogma_start_desc_ring but it should
  work the same (disable NAPI in close, wait for quiescence, etc.).

- ogma_get_rx_num
  Called from NAPI context. No need to disable bh.

- ogma_get_tx_avail_num
  Called from NAPI context or xmit handler. No need to disable bh.
  
- ogma_clean_tx_desc_ring
  Called from NAPI, xmit or ogma_netdev_ops.ndo_open. The latter can
  control the formers. Should be a plain spinlock.

- ogma_clean_rx_desc_ring
  Only called through ogma_netdev_ops.ndo_open. See above.

- ogma_set_tx_pkt_data
  Only called from the xmit handler, whence in locally disabled bh context.

- ogma_get_rx_pkt_data
  Only called from NAPI context. No need to disable bh.

Executive summary: desc_lock should go away.

> +
> +static void desc_unlock(struct ogma_desc_ring *desc, bool *ctx)
> +{
> +	if (*ctx)
> +		spin_unlock(&desc->spinlock_desc);
> +	else
> +		spin_unlock_bh(&desc->spinlock_desc);
> +}
> +
> +static void ogma_check_desc_own_sanity(const struct ogma_desc_ring *desc,
> +					u16 idx, unsigned int expected_own)
> +{
> +	u32 tmp = *(u32 *)((void *)desc->ring_vaddr + desc->len * idx);
> +
> +	BUG_ON((tmp >> 31) != expected_own);
> +}
> +
> +int ogma_ring_irq_enable(struct ogma_priv *priv, unsigned int id,
> +								u32 irq_factor)
> +{
> +	int ret = 0;
> +	struct ogma_desc_ring *desc = &priv->desc_ring[id];
> +	bool ctx;

Please use inverted xmas tree style for variable declaration.

> +
> +	desc_lock(desc, &ctx);
> +
> +	if (!desc->running_flag) {
> +		dev_err(priv->dev, "desc ring not running\n");
> +		ret = -ENODEV;
> +		goto bail;
> +	}
> +
> +	ogma_write_reg(priv, ads_irq_set[id], irq_factor);
> +
> +bail:
> +	desc_unlock(desc, &ctx);
> +
> +	return ret;
> +}
> +
> +int ogma_ring_irq_disable(struct ogma_priv *priv,
> +			       unsigned int id, u32 irq_factor)

int ogma_ring_irq_disable(struct ogma_priv *priv, unsigned int id,
			  u32 irq_factor)

> +{
> +	ogma_write_reg(priv, desc_ring_irq_inten_clr_reg_addr[id], irq_factor);
> +
> +	return 0;

Why does it return anything ? It should be void.

> +}
> +
> +static int alloc_pkt_buf(struct device *dev, u16 len, void **addr_p,
> +				phys_addr_t *phys, struct sk_buff **pskb)
> +{
> +	struct sk_buff *skb = dev_alloc_skb(len + 2);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	/* Reserve leading 2 bytes to make IP header word-aligned. */
> +	skb_reserve(skb, 2);

netdev_alloc_skb_ip_align

> +
> +	*phys = dma_map_single(dev, skb->data, (size_t) len, DMA_FROM_DEVICE);
> +	if (!*phys) {
> +		dev_kfree_skb(skb);
> +
> +		return -ENOMEM;
> +	}
> +
> +	*addr_p = (void *)skb->data;
> +	*pskb = skb;
> +
> +	/* Mark that this is a receiving buffer. */
> +	pfdep_set_pkt_buf_type(skb, 1);

The comment underlines a poor naming.

> +
> +	return 0;
> +}
> +
> +void kfree_pkt_buf(struct device *dev, u16 len, void *addr,
> +	phys_addr_t phys_addr, bool last_flag, struct sk_buff *skb)
> +{
> +	dma_unmap_single(dev, phys_addr, (size_t) len,
> +			 (pfdep_is_recv_pkt_buf(skb) ?
> +			  DMA_FROM_DEVICE : DMA_TO_DEVICE));

Excess parenthesis.

> +
> +	if (last_flag)
> +		dev_kfree_skb(skb);
> +}
> +
> +int ogma_alloc_desc_ring(struct ogma_priv *priv, unsigned int id)
> +{
> +	int ret = 0;
> +	struct ogma_desc_ring *desc = &priv->desc_ring[id];
> +	struct ogma_desc_param *param = &priv->desc_ring[id].param;
> +	u8 rx_desc_entry_len = 0;
> +
> +	if ((priv->param.desc_param[id].valid_flag) &&
> +	    ((priv->param.desc_param[id].entry_num < OGMA_DESC_MIN) ||
> +	     (priv->param.desc_param[id].entry_num > OGMA_DESC_MAX))) {

A local variable for &priv->param.desc_param[id] would not hurt.

[...]
> +	desc->ring_vaddr = dma_alloc_coherent(NULL,
> +		desc->len * param->entry_num, &desc->deschys_addr, GFP_KERNEL);

Indent should line things up with the first character after the parenthesis.

> +	if (!desc->ring_vaddr) {
> +		ret = -ENOMEM;
> +		dev_err(priv->dev, "%s: failed to alloc\n", __func__);
> +		goto err;
> +	}
> +
> +	memset(desc->ring_vaddr, 0, (u32) desc->len * param->entry_num);
> +	desc->frag = kmalloc(sizeof(struct ogma_frag_info) * param->entry_num,
> +								GFP_NOWAIT);

Indent.

> +	if (!desc->frag) {
> +		ret = -ENOMEM;
> +		dev_err(priv->dev, "%s: failed to alloc\n", __func__);
> +		goto err;
> +	}
> +
> +	memset(desc->frag, 0, sizeof(struct ogma_frag_info) * param->entry_num);
> +	desc->priv = kmalloc(sizeof(struct sk_buff *) * param->entry_num,
> +								GFP_NOWAIT);

Indent.

> +	if (!desc->priv) {
> +		ret = -ENOMEM;
> +		dev_err(priv->dev, "%s: failed to alloc priv\n", __func__);
> +		goto err;
> +	}
> +
> +	memset(desc->priv, 0, sizeof(struct sk_buff *) * param->entry_num);
> +
> +	return 0;
> +
> +err:
> +	ogma_free_desc_ring(priv, desc);
> +
> +	return ret;
> +}
> +
> +void ogma_uninit_pkt_desc_ring(struct ogma_priv *priv,
> +			       struct ogma_desc_ring *desc)
> +{
> +	u16 idx;
> +	u32 tmp;
> +	bool last_flag;
> +	int count = desc->param.entry_num;
> +
> +	for (idx = 0; idx < count; idx++) {
> +		if (!desc->frag[idx].addr)
> +			continue;
> +
> +		tmp = *((u32 *)((void *)desc->ring_vaddr + (desc->len * idx)));
> +		last_flag = (tmp >> 8) & 1;
> +		kfree_pkt_buf(priv->dev, desc->frag[idx].len,
> +			desc->frag[idx].addr, desc->frag[idx].phys_addr,
> +			      last_flag, desc->priv[idx]);

Broken indent. The driver contains a lot of those. :o(

Please add a local variable for desc->frag[idx].

[...]
> +static void ogma_set_rx_desc_entry(struct ogma_priv *priv,
> +	struct ogma_desc_ring *desc, u16 idx, const struct ogma_frag_info *frag,
> +						     struct sk_buff *skb)
> +{
> +	struct ogma_rx_desc_entry rx_desc_entry;
> +
> +	ogma_check_desc_own_sanity(desc, idx, 0);
> +
> +	memset(&rx_desc_entry, 0, sizeof(rx_desc_entry));
> +
> +	rx_desc_entry.attr = (1 << OGMA_RX_PKT_DESC_RING_OWN_FIELD) |
> +			     (1 << OGMA_RX_PKT_DESC_RING_FS_FIELD) |
> +			     (1 << OGMA_RX_PKT_DESC_RING_LS_FIELD);
> +
> +	rx_desc_entry.data_buf_addr = frag->phys_addr;
> +	rx_desc_entry.buf_len_info = frag->len;
> +
> +	if (idx == (desc->param.entry_num - 1))
> +		rx_desc_entry.attr |= (0x1U << OGMA_RX_PKT_DESC_RING_LD_FIELD);
> +
> +	memcpy(((void *)((void *)desc->ring_vaddr + desc->len * idx + 4)),
> +		(void *)((void *)&rx_desc_entry + 4), desc->len - 4);

Scary void * fetish.

[...]
> diff --git a/drivers/net/ethernet/fujitsu/ogma/ogma_netdev.c b/drivers/net/ethernet/fujitsu/ogma/ogma_netdev.c
> new file mode 100644
> index 0000000..2f69a29
> --- /dev/null
> +++ b/drivers/net/ethernet/fujitsu/ogma/ogma_netdev.c
[...]
> +const struct net_device_ops ogma_netdev_ops = {
> +	.ndo_open = ogma_netdev_open,
> +	.ndo_stop = ogma_netdev_stop,
> +	.ndo_start_xmit = ogma_netdev_start_xmit,
> +	.ndo_set_features = ogma_netdev_set_features,
> +	.ndo_get_stats = ogma_netdev_get_stats,
> +	.ndo_change_mtu = ogma_netdev_change_mtu,
> +	.ndo_set_mac_address = ogma_netdev_set_macaddr,
> +	.ndo_validate_addr = eth_validate_addr,
> +};

Please use tabs before "=" to line things up.

> diff --git a/drivers/net/ethernet/fujitsu/ogma/ogma_platform.c b/drivers/net/ethernet/fujitsu/ogma/ogma_platform.c
> new file mode 100644
> index 0000000..176d746
> --- /dev/null
> +++ b/drivers/net/ethernet/fujitsu/ogma/ogma_platform.c
[...]
> +int ogma_configure_normal_mode(struct ogma_priv *priv,
> +			       const struct ogma_normal_mode_hwconf
> +			       *normal_mode_hwconf_p)
> +{
[...]
> +	timeout = WAIT_FW_RDY_TIMEOUT;
> +	while (timeout-- && (ogma_read_reg(priv,
> +					   desc_ads
> +					   [OGMA_RING_NRM_RX])
> +			     & (0x1UL << OGMA_REG_DESC_RING_CONFIG_CFG_UP)))

:o(

[...]
> +static int ogma_probe(struct platform_device *pdev)
> +{
[...]
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Missing base resource\n");
> +		goto bail1;
> +	}
> +
> +	priv->base_addr = ioremap_nocache(res->start,
> +						res->end - res->start + 1);
> +	if (!priv->base_addr) {
> +		dev_err(&pdev->dev, "ioremap_nocache() failed\n");
> +		err = -EINVAL;
> +		goto bail1;

bail1, bail2, bail3 and friends are not sane label names.

I haven't found any __le{8, 16, 32} nor cpu_to_xyz in the driver. I would
had expected some in the descriptors.

-- 
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists