Hi Andrew,

From: Andrew Lunn, Sent: Friday, September 23, 2022 10:12 PM

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+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