| 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
| ||
|
Date: Wed, 8 May 2024 22:58:16 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Yanteng Si <siyanteng@...ngson.cn>, 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,
On Wed, May 8, 2024 at 10:38 PM Serge Semin <fancer.lancer@...il.com> wrote:
>
> On Tue, May 07, 2024 at 09:35:24PM +0800, Yanteng Si wrote:
> > Hi Serge,
> >
> > 在 2024/5/6 18:39, Serge Semin 写道:
> > > On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote:
> > > > ...
> > > > +static int loongson_dwmac_config_msi(struct pci_dev *pdev,
> > > > + struct plat_stmmacenet_data *plat,
> > > > + struct stmmac_resources *res,
> > > > + struct device_node *np)
> > > > +{
> > > > + int i, ret, vecs;
> > > > +
> > > > + vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1);
> > > > + ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI);
> > > > + if (ret < 0) {
> > > > + dev_info(&pdev->dev,
> > > > + "MSI enable failed, Fallback to legacy interrupt\n");
> > > > + return loongson_dwmac_config_legacy(pdev, plat, res, np);
> > > > + }
> > > > +
> > > > + res->irq = pci_irq_vector(pdev, 0);
> > > > + res->wol_irq = 0;
> > > > +
> > > > + /* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx |
> > > > + * --------- ----- -------- -------- ... -------- --------
> > > > + * IRQ NUM | 0 | 1 | 2 | ... | 15 | 16 |
> > > > + */
> > > > + for (i = 0; i < CHANNEL_NUM; i++) {
> > > > + res->rx_irq[CHANNEL_NUM - 1 - i] =
> > > > + pci_irq_vector(pdev, 1 + i * 2);
> > > > + res->tx_irq[CHANNEL_NUM - 1 - i] =
> > > > + pci_irq_vector(pdev, 2 + i * 2);
> > > > + }
> > > > +
> > > > + plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > ...
> > > > static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > {
> > > > struct plat_stmmacenet_data *plat;
> > > > int ret, i, bus_id, phy_mode;
> > > > struct stmmac_pci_info *info;
> > > > struct stmmac_resources res;
> > > > + struct loongson_data *ld;
> > > > struct device_node *np;
> > > > np = dev_of_node(&pdev->dev);
> > > > @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> > > > return -ENOMEM;
> > > > plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
> > > > - if (!plat->dma_cfg) {
> > > > - ret = -ENOMEM;
> > > > - goto err_put_node;
> > > > - }
> > > > + if (!plat->dma_cfg)
> > > > + return -ENOMEM;
> > > > +
> > > > + ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL);
> > > > + if (!ld)
> > > > + return -ENOMEM;
> > > > /* Enable pci device */
> > > > ret = pci_enable_device(pdev);
> > > > @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> > > > plat->phy_interface = phy_mode;
> > > > }
> > > > - pci_enable_msi(pdev);
> > > > + plat->bsp_priv = ld;
> > > > + plat->setup = loongson_dwmac_setup;
> > > > + ld->dev = &pdev->dev;
> > > > +
> > > > memset(&res, 0, sizeof(res));
> > > > res.addr = pcim_iomap_table(pdev)[0];
> > > > + ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff;
> > > > +
> > > > + switch (ld->gmac_verion) {
> > > > + case LOONGSON_DWMAC_CORE_1_00:
> > > > + plat->rx_queues_to_use = CHANNEL_NUM;
> > > > + plat->tx_queues_to_use = CHANNEL_NUM;
> > > > +
> > > > + /* Only channel 0 supports checksum,
> > > > + * so turn off checksum to enable multiple channels.
> > > > + */
> > > > + for (i = 1; i < CHANNEL_NUM; i++)
> > > > + plat->tx_queues_cfg[i].coe_unsupported = 1;
> > > > - plat->tx_queues_to_use = 1;
> > > > - plat->rx_queues_to_use = 1;
> > > > + ret = loongson_dwmac_config_msi(pdev, plat, &res, np);
> > > > + break;
> > > > + default: /* 0x35 device and 0x37 device. */
> > > > + plat->tx_queues_to_use = 1;
> > > > + plat->rx_queues_to_use = 1;
> > > > - ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
> > > > + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
> > > > + break;
> > > > + }
> > > Let's now talk about this change.
> > >
> > > First of all, one more time. You can't miss the return value check
> > > because if any of the IRQ config method fails then the driver won't
> > > work! The first change that introduces the problem is in the patch
> > > [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy
> > OK!
> > >
> > > Second, as I already mentioned in another message sent to this patch
> > > you are missing the PCI MSI IRQs freeing in the cleanup-on-error path
> > > and in the device/driver remove() function. It's definitely wrong.
> > You are right! I will do it.
> > > Thirdly, you said that the node-pointer is now optional and introduced
> > > the patch
> > > [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support
> > > If so and the DT-based setting up isn't mandatory then I would
> > > suggest to proceed with the entire so called legacy setups only if the
> > > node-pointer has been found, otherwise the pure PCI-based setup would
> > > be performed. So the patches 10-13 (in your v12 order) would look
> >
> > In this case, MSI will not be enabled when the node-pointer is found.
> >
> > .
> >
> >
> > In fact, a large fraction of 2k devices are DT-based, of course, many are
> > PCI-based.
>
> Then please summarise which devices need the DT-node pointer which
> don't? And most importantly if they do why do they need the DT-node?
Whether we need DT-nodes doesn't depend on device type, but depends on
the BIOS type. When we boot with UEFI+ACPI, we don't need DT-node,
when we boot with PMON+FDT, we need DT-node. Loongson machines may
have either BIOS types.
Huacai
>
> AFAICS currently both LS2K1000 and LS7A1000 GMACs require the DT-node
> to get the MAC and LPI IRQ signals. AFAICS from your series LS7A2000
> GNET is also DT-based for the same reason. But the LS2K2000 GNET case
> is different. You say that some of the platforms have the respective
> DT-node some don't, but at the same time you submitting this patch
> which permits the MSI IRQs only for the LS7A2000 GNET. It looks
> contradicting. Does it mean that the GNET devices may generate the
> IRQs via both legacy (an IRQ signal directly connected to the system
> GIC) and the PCI MSI ways?
>
> Let's get the question to the more generic level. Are the Loongson
> GNET and GMAC controllers able to generate the IRQs via both ways:
> physical IRQ signal and PCI MSI?
>
> Please don't consider this as a vastly meticulous review. I am just
> trying to figure out how to make things less complicated and fix the
> driver to permitting only the cases which are actually possible.
>
> -Serge(y)
>
> >
> >
> > Thanks,
> >
> > Yanteng
> >
> >
Powered by blists - more mailing lists