| 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: <pdyqoki5qw4zabz3uv5ff2e2o43htcr6xame652zmbqh23tjji@lt5gmp6m3lkm>
Date: Wed, 8 May 2024 17:38:52 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com,
alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com,
chenhuacai@...nel.org, 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 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?
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