netdev - Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add Loongson GNET support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 17 May 2024 12:07:34 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>, 
	Huacai Chen <chenhuacai@...nel.org>
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 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.

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

-Serge(y)

> 
> 
> Thanks,
> 
> Yanteng
>