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
