| 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: <c09237c6-6661-4744-a9d3-7c3443f2820c@loongson.cn>
Date: Fri, 17 May 2024 18:37:50 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>, Huacai Chen <chenhuacai@...nel.org>
Cc: 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
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/>
>
>> 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.
Thanks,
Yanteng
Powered by blists - more mailing lists