Re: [PATCH v3 2/3] net: ethernet: renesas: Add Ethernet Switch driver

[Andrew Lunn <andrew@lunn.ch>]

> +/* Forwarding engine block (MFWD) */
> +static void rswitch_fwd_init(struct rswitch_private *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < RSWITCH_NUM_HW; i++) {
> +		iowrite32(FWPC0_DEFAULT, priv->addr + FWPC0(i));
> +		iowrite32(0, priv->addr + FWPBFC(i));
> +	}

What is RSWITCH_NUM_HW?

> +
> +	for (i = 0; i < RSWITCH_NUM_ETHA; i++) {

RSWITCH_NUM_ETHA appears to be the number of ports?

> +static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate)
> +{
> +	u32 gwgrlulc, gwgrlc;
> +
> +	switch (rate) {
> +	case 1000:
> +		gwgrlulc = 0x0000005f;
> +		gwgrlc = 0x00010260;
> +		break;
> +	default:
> +		dev_err(&priv->pdev->dev, "%s: This rate is not supported (%d)\n", __func__, rate);
> +		return;
> +	}

Is this setting the Mbps between the switch matrix and the CPU? Why
limit the rate? Especially if you have 3 ports, would not 3000 make
sense?

> +static void rswitch_get_data_irq_status(struct rswitch_private *priv, u32 *dis)
> +{
> +	int i;
> +
> +	for (i = 0; i < RSWITCH_NUM_IRQ_REGS; i++) {
> +		dis[i] = ioread32(priv->addr + GWDIS(i));
> +		dis[i] &= ioread32(priv->addr + GWDIE(i));
> +	}
> +}
> +
> +static void rswitch_enadis_data_irq(struct rswitch_private *priv, int index, bool enable)
> +{
> +	u32 offs = enable ? GWDIE(index / 32) : GWDID(index / 32);
> +
> +	iowrite32(BIT(index % 32), priv->addr + offs);
> +}
> +
> +static void rswitch_ack_data_irq(struct rswitch_private *priv, int index)
> +{
> +	u32 offs = GWDIS(index / 32);
> +
> +	iowrite32(BIT(index % 32), priv->addr + offs);
> +}
> +
> +static bool rswitch_is_chain_rxed(struct rswitch_gwca_chain *c)
> +{
> +	struct rswitch_ext_ts_desc *desc;
> +	int entry;
> +
> +	entry = c->dirty % c->num_ring;
> +	desc = &c->ts_ring[entry];
> +
> +	if ((desc->die_dt & DT_MASK) != DT_FEMPTY)
> +		return true;
> +
> +	return false;

Is a chain a queue? Also known as a ring? The naming is different to
most drivers, which is making this harder to understand. Ideally, you
want to follow the basic naming the majority of other drivers use.

> +}
> +
> +static int rswitch_gwca_chain_alloc_skb(struct rswitch_gwca_chain *c,
> +					int start, int num)
> +{
> +	int i, index;
> +
> +	for (i = start; i < (start + num); i++) {
> +		index = i % c->num_ring;

Why this? Would it not make more sense to validate that start + num <
num_ring? It seems like bad parameters passed here could destroy some
other skb in the ring?

More naming... Here you use num_ring, not num_chain. Try to be
consistent. Also, num_ring makes my think of ring 7 of 9 rings. When
this actually appears to be the size of the ring. So c->ring_size
would be a better name.

> +		if (c->skb[index])
> +			continue;
> +		c->skb[index] = dev_alloc_skb(PKT_BUF_SZ + RSWITCH_ALIGN - 1);
> +		if (!c->skb[index])
> +			goto err;
> +		skb_reserve(c->skb[index], NET_IP_ALIGN);

netdev_alloc_skb_ip_align()?

> +static void rswitch_gwca_chain_free(struct net_device *ndev,
> +				    struct rswitch_gwca_chain *c)
> +{
> +	int i;
> +
> +	if (c->gptp) {
> +		dma_free_coherent(ndev->dev.parent,
> +				  sizeof(struct rswitch_ext_ts_desc) *
> +				  (c->num_ring + 1), c->ts_ring, c->ring_dma);
> +		c->ts_ring = NULL;
> +	} else {
> +		dma_free_coherent(ndev->dev.parent,
> +				  sizeof(struct rswitch_ext_desc) *
> +				  (c->num_ring + 1), c->ring, c->ring_dma);
> +		c->ring = NULL;
> +	}
> +
> +	if (!c->dir_tx) {
> +		for (i = 0; i < c->num_ring; i++)
> +			dev_kfree_skb(c->skb[i]);
> +	}
> +
> +	kfree(c->skb);
> +	c->skb = NULL;

When i see code like this, i wonder why an API call like
dev_kfree_skb() is not being used. I would suggest reaming this to
something other than skb, which has a very well understood meaning.

> +static bool rswitch_rx(struct net_device *ndev, int *quota)
> +{
> +	struct rswitch_device *rdev = netdev_priv(ndev);
> +	struct rswitch_gwca_chain *c = rdev->rx_chain;
> +	int entry = c->cur % c->num_ring;
> +	struct rswitch_ext_ts_desc *desc;
> +	int limit, boguscnt, num, ret;
> +	struct sk_buff *skb;
> +	dma_addr_t dma_addr;
> +	u16 pkt_len;
> +
> +	boguscnt = min_t(int, c->dirty + c->num_ring - c->cur, *quota);
> +	limit = boguscnt;
> +
> +	desc = &c->ts_ring[entry];
> +	while ((desc->die_dt & DT_MASK) != DT_FEMPTY) {
> +		dma_rmb();
> +		pkt_len = le16_to_cpu(desc->info_ds) & RX_DS;
> +		if (--boguscnt < 0)
> +			break;

Why the barrier, read the length and then decide to break out of the
loop?

> +static int rswitch_open(struct net_device *ndev)
> +{
> +	struct rswitch_device *rdev = netdev_priv(ndev);
> +	struct device_node *phy;
> +	int err = 0;
> +
> +	if (rdev->etha) {

Can this happen? What would a netdev without an etha mean?

    Andrew