Date: Fri, 17 May 2024 19:37:47 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: Huacai Chen <chenhuacai@...nel.org>, andrew@...n.ch, 
	hkallweit1@...il.com, peppe.cavallaro@...com, alexandre.torgue@...s.st.com, 
	joabreu@...opsys.com, Jose.Abreu@...opsys.com, linux@...linux.org.uk, 
	guyinggang@...ngson.cn, netdev@...r.kernel.org, chris.chenfeiyang@...il.com, 
	siyanteng01@...il.com
Subject: Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add
 Loongson GNET support

On Fri, May 17, 2024 at 06:37:50PM +0800, Yanteng Si wrote:
> Hi Serge,
> 
> 在 2024/5/17 17:07, Serge Semin 写道:
> > On Fri, May 17, 2024 at 04:42:51PM +0800, Yanteng Si wrote:
> > > Hi Huacai, Serge,
> > > 
> > > 在 2024/5/15 21:55, Huacai Chen 写道:
> > > > > > > Once again about the naming. From the retrospective point of view the
> > > > > > > so called legacy PCI IRQs (in fact PCI INTx) and the platform IRQs
> > > > > > > look similar because these are just the level-type signals connected
> > > > > > > to the system IRQ controller. But when it comes to the PCI_Express_,
> > > > > > > the implementation is completely different. The PCIe INTx is just the
> > > > > > > PCIe TLPs of special type, like MSI. Upon receiving these special
> > > > > > > messages the PCIe host controller delivers the IRQ up to the
> > > > > > > respective system IRQ controller. So in order to avoid the confusion
> > > > > > > between the actual legacy PCI INTx, PCI Express INTx and the just
> > > > > > > platform IRQs, it's better to emphasize the actual way of the IRQs
> > > > > > > delivery. In this case it's the later method.
> > > > > > You are absolutely right, and I think I found a method to use your
> > > > > > framework to solve our problems:
> > > > > > 
> > > > > >      static int loongson_dwmac_config_irqs(struct pci_dev *pdev,
> > > > > >                                             struct plat_stmmacenet_data *plat,
> > > > > >                                             struct stmmac_resources *res)
> > > > > >      {
> > > > > >          int i, ret, vecs;
> > > > > > 
> > > > > >          /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx |
> > > > > >           * --------- ----- -------- --------  ...  -------- --------
> > > > > >           * IRQ NUM  |  0  |   1    |   2    | ... |   15   |   16   |
> > > > > >           */
> > > > > >          vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1;
> > > > > >          ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_INTX);
> > > > > >          if (ret < 0) {
> > > > > >                  dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n");
> > > > > >                  return ret;
> > > > > >          }
> > > > > >         if (ret >= vecs) {
> > > > > >                  for (i = 0; i < plat->rx_queues_to_use; i++) {
> > > > > >                          res->rx_irq[CHANNELS_NUM - 1 - i] =
> > > > > >                                  pci_irq_vector(pdev, 1 + i * 2);
> > > > > >                  }
> > > > > >                  for (i = 0; i < plat->tx_queues_to_use; i++) {
> > > > > >                          res->tx_irq[CHANNELS_NUM - 1 - i] =
> > > > > >                                  pci_irq_vector(pdev, 2 + i * 2);
> > > > > >                  }
> > > > > > 
> > > > > >                  plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > > > >          }
> > > > > > 
> > > > > >          res->irq = pci_irq_vector(pdev, 0);
> > > > > > 
> > > > > >        if (np) {
> > > > > >            res->irq = of_irq_get_byname(np, "macirq");
> > > > > >            if (res->irq < 0) {
> > > > > >               dev_err(&pdev->dev, "IRQ macirq not found\n");
> > > > > >               return -ENODEV;
> > > > > >            }
> > > > > > 
> > > > > >            res->wol_irq = of_irq_get_byname(np, "eth_wake_irq");
> > > > > >            if (res->wol_irq < 0) {
> > > > > >               dev_info(&pdev->dev,
> > > > > >                    "IRQ eth_wake_irq not found, using macirq\n");
> > > > > >               res->wol_irq = res->irq;
> > > > > >            }
> > > > > > 
> > > > > >            res->lpi_irq = of_irq_get_byname(np, "eth_lpi");
> > > > > >            if (res->lpi_irq < 0) {
> > > > > >               dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
> > > > > >               return -ENODEV;
> > > > > >            }
> > > > > >        }
> > > > > >          return 0;
> > > > > >      }
> > > > > > 
> > > > > > If your agree, Yanteng can use this method in V13, then avoid furthur changes.
> > > > > Since yesterday I have been too relaxed sitting back to explain in
> > > > > detail the problems with the code above. Shortly speaking, no to the
> > > > > method designed as above.
> > > > This function is copy-paste from your version which you suggest to
> > > > Yanteng, and plus the fallback parts for DT. If you don't want to
> > > > discuss it any more, we can discuss after V13.
> > My conclusion is the same. no to _your_ (Huacai) version of the code.
> > I suggest to Huacai dig dipper in the function semantic and find out
> > the problems it has. Meanwhile I'll keep relaxing...
> > 
> > > > BTW, we cannot remove "res->wol_irq = res->irq", because Loongson
> > > > GMAC/GNET indeed supports WoL.
> > > Okay, I will not drop it in v13.
> > Apparently Huacai isn't well familiar with what he is reviewing. Once
> > again the initialization is useless. Drop it.
> 

> Hmm, to be honest, I'm still a little confused about this.
> 
> When we first designed the driver, we looked at intel,See:
> 
> $: vim drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c +953
> 
> static int stmmac_config_single_msi(struct pci_dev *pdev,
>                     struct plat_stmmacenet_data *plat,
>                     struct stmmac_resources *res)
> {
>     int ret;
> 
>     ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>     if (ret < 0) {
>         dev_info(&pdev->dev, "%s: Single IRQ enablement failed\n",
>              __func__);
>         return ret;
>     }
> 
>     res->irq = pci_irq_vector(pdev, 0);
>     res->wol_irq = res->irq;
> 
> Why can't we do this?
> 
> Intel Patch thread link <https://lore.kernel.org/netdev/20210316121823.18659-5-weifeng.voon@intel.com/>

First of all the Intel' STMMAC patches isn't something what can be
referred to as a good-practice example. A significant part of the
mess in the plat_stmmacenet_data structure is their doing.

Secondly as I already said several times initializing res->wol_irq
with res->irq is _useless_. It's because of the way the WoL IRQ line
is requested:

stmmac_request_irq_single(struct net_device *dev)
{
	...
	if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
                ret = request_irq(priv->wol_irq, stmmac_interrupt,
                                  IRQF_SHARED, dev->name, dev);
		...
        }
	...
}

stmmac_request_irq_multi_msi(struct net_device *dev)
{
	...
	if (priv->wol_irq > 0 && priv->wol_irq != dev->irq) {
                int_name = priv->int_name_wol;
                sprintf(int_name, "%s:%s", dev->name, "wol");
                ret = request_irq(priv->wol_irq,
                                  stmmac_mac_interrupt,
                                  0, int_name, dev);
		...
        }
	...
}

See, even if you initialize priv->wol_irq with dev->irq (res->irq) it
will have the same effect as if you had it left uninitialized
(pre-initialized with zero). So from both maintainability and
readability points of view it's better to avoid a redundant code
especially if it causes an ill coding practice reproduction.


Interestingly to note that having res->wol_irq initialized with
res->irq had been required before another Intel' commit:
8532f613bc78 ("net: stmmac: introduce MSI Interrupt routines for mac, safety, RX & TX")
(submitted sometime around the commit you are referring to).
In that commit Intel' developers themself fixed the semantics in the
STMMAC core driver, but didn't bother with fixing the platform drivers
and even the Intel' DWMAC PCI driver has been left with that redundant
line of the code. Sigh...

> 
> 
> > 
> > > All right. I have been preparing v13 and will send it as soon as possible.
> > > 
> > > Let's continue the discussion in v13. Of course, I will copy the part that
> > > has
> > > 
> > > not received a clear reply to v13.
> > > 
> > Note the merge window has been opened and the 'net-next' tree is now
> > closed. So either you submit your series as RFC or wait for the window
> > being closed.
> > 

> Ok, if I'm fast enough, I'll send an RFC to talk about msi and legacy.

It's up to you. But please be aware, I'll be busy next week with my
own patches cooking up. So I won't be able to actively participate in
your patches review.

-Serge(y)

> 
> 
> Thanks,
> 
> Yanteng
> 

Powered by blists - more mailing lists