| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEensMxS-+RNj8j+QSYnBZzXLQ88M-d-4D=DvNr1y-pW81fAcg@mail.gmail.com>
Date: Sat, 18 May 2024 18:47:28 +0800
From: yanteng si <siyanteng01@...il.com>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Yanteng Si <siyanteng@...ngson.cn>, 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
Subject: Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add
Loongson GNET support
Serge Semin <fancer.lancer@...il.com> 于2024年5月18日周六 00:37写道：
>
> 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:
I see, res->irq will be droped. Thanks.
>
> 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.
Oh, I see. Thank you！
>
>
> 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...
>
> > 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.
Okay, maybe it would be better to send v13 after the window closes.
Thanks,
Yanteng
Powered by blists - more mailing lists