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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Andrew,

> From: Andrew Lunn, Sent: Friday, September 23, 2022 10:12 PM
> 
> > +/* 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?

I think the name is unclear...
Anyway, this hardware has 3 ethernet ports and 2 CPU ports.
So that the RSWITCH_NUM_HW is 5. Perhaps, RSWITCH_NUM_ALL_PORTS
is better name.

Perhaps, since the current driver supports 1 ethernet port and 1 CPU port only,
I should modify this driver for the current condition strictly.

> > +
> > +	for (i = 0; i < RSWITCH_NUM_ETHA; i++) {
> 
> RSWITCH_NUM_ETHA appears to be the number of ports?

Yes, this is number of ethernet 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?

This is needed to avoid about 10% packets loss when the hardware sends data
because the hardware will send much data if the limit is not set.

> > +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?

Yes.

> 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.

I got it. I'll rename them.
 
> > +}
> > +
> > +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?

The descriptor indexes (c->cur and c->dirty) are just counters so that
the index is always calculated by that. This implementation is similar
with other drivers/net/ethernet/renesas/ drivers. However, as you mentioned
above, this is not majority, I think...

Also, I realized that the function will cause an issue because the types of
c->cur and c->dirty are u32, but the type of start is int.

I'll fix the indexes handling.

> 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.

Yes, the num_ring means the size of the ring. So, I'll rename it as your suggestion.

> > +		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()?

Yes. I'll use it. Thanks!

> > +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.

Perhaps, c->skbs is better name than just c->skb.

> > +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?

Thank you for pointed it out. I should decrement/check boguscnt
before the barrier and read the length.

> > +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?

This cannot happen now. So, I'll drop it.
(I intended to create a netdev without an etha as a virtual device.
 But the current driver doesn't have such a feature.)

Best regards,
Yoshihiro Shimoda

>     Andrew