| 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: Tue, 19 Mar 2024 18:53:25 +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@...ngson.cn, linux@...linux.org.uk, guyinggang@...ngson.cn,
netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 07/11] net: stmmac: dwmac-loongson: Add
multi-channel supports for loongson
On Thu, Mar 14, 2024 at 09:13:58PM +0800, Yanteng Si wrote:
>
> 在 2024/3/14 17:33, Yanteng Si 写道:
> > 在 2024/2/6 05:28, Serge Semin 写道:
> > > On Tue, Jan 30, 2024 at 04:48:19PM +0800, Yanteng Si wrote:
> > >> Request allocation for MSI for specific versions.
> > >>
> > > Please elaborate what is actually done in the patch. What device
> > > version it is dedicated for (Loongson GNET?), what IRQs the patch
> > > adds, etc.
> > gnet_device core IP IRQ
> > 7a2000 0x37 legacy
> > 2k2000 0x10 multi_msi
> > >
> > > BTW will GNET device work without this patch? If no you need to either
> > > merge it into the patch introducing the GNET-device support or place
> > > it before that patch (6/11).
> > OK, GNET device work need this patch.
> > > >> Some features of Loongson platforms are bound to the GMAC_VERSION
> > >> register. We have to read its value in order to get the correct channel
> > >> number.
> > > This message seems misleading. I don't see you doing that in the patch below...
> > Because part of our gnet hardware (7a2000) core IP is 0x37
> > >
> > >> Signed-off-by: Yanteng Si<siyanteng@...ngson.cn>
> > >> Signed-off-by: Feiyang Chen<chenfeiyang@...ngson.cn>
> > >> Signed-off-by: Yinggang Gu<guyinggang@...ngson.cn>
> > >> ---
> > >> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 57 +++++++++++++++----
> > >> 1 file changed, 46 insertions(+), 11 deletions(-)
> > >>
> > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > >> index 584f7322bd3e..60d0a122d7c9 100644
> > >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > >> @@ -98,10 +98,10 @@ static void dwlgmac_dma_init_channel(struct stmmac_priv *priv,
> > >> if (dma_cfg->aal)
> > >> value |= DMA_BUS_MODE_AAL;
> > >>
> > > >> - writel(value, ioaddr + DMA_BUS_MODE);
> > >> + writel(value, ioaddr + DMA_CHAN_BUS_MODE(chan));
> > >> >> /* Mask interrupts by writing to CSR7 */
> > >> - writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_INTR_ENA);
> > >> + writel(DMA_INTR_DEFAULT_MASK_LOONGSON, ioaddr + DMA_CHAN_INTR_ENA(chan));
> > > Em, why is this here? There is a patch
> > > [PATCH net-next v8 05/11] net: stmmac: dwmac-loongson: Add Loongson-specific register definitions
> > > in this series which was supposed to introduce the fully functional
> > > GNET-specific callbacks. Move this change in there.
> > OK.
> > >
> > >> }
> > >> >> static int dwlgmac_dma_interrupt(struct stmmac_priv *priv,
> > void __iomem *ioaddr,
> > >> @@ -238,6 +238,45 @@ static int loongson_dwmac_config_legacy(struct pci_dev *pdev,
> > >> return 0;
> > >> }
> > >> >> +static int loongson_dwmac_config_multi_msi(struct pci_dev
> > *pdev,
> > > This method seems like the GNET-specific one. What about using the
> > > appropriate prefix then?
> > OK. loongson_gnet_config_multi_msi()
> >
> > >
> > >> + struct plat_stmmacenet_data *plat,
> > >> + struct stmmac_resources *res,
> > >> + struct device_node *np,
> > >> + int channel_num)
> > > Why do you need this parametrization? Since this method is
> > > GNET-specific what about defining a macro with the channels amount and
> > > using it here?
> >
> > OK.
> >
> > #define CHANNEL_NUM 8
> >
> > >
> > >> +{
> > >> + 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);
> > > In what conditions is this possible? Will the
> > > loongson_dwmac_config_legacy() method work in that case? Did you test
> > > it out?
> > Failed to apply for msi interrupt when the interrupt number is not enough，For
> > example, there are a large number of devices。
Then those platforms will _require_ to have the DT-node specified. This
will define the DT-bindings which I doubt you imply here. Am I wrong?
Once again have you tested the loongson_dwmac_config_legacy() method
working in the case of the pci_alloc_irq_vectors() failure?
> > >> + }
> > >> +
> > >> + plat->rx_queues_to_use = channel_num;
> > >> + plat->tx_queues_to_use = channel_num;
> > > This is supposed to be initialized in the setup() methods. Please move
> > > it to the dedicated patch.
> > No, referring to my previous reply, only the 0x10 gnet device has 8 channels,
> > and the 0x37 device has a single channel.
Yes. You have a perfectly suitable method for it. It's
loongson_gnet_data(). Init the number of channels there based on the
value read from the GMAC_VERSION.SNPSVER field. Thus the
loongson_gnet_config_multi_msi() will get to be more coherent setting
up the MSI IRQs only.
Am I missing something in the last two notes chunks regard? If yes,
let's submit v9 as you see it. We'll get back to this part there then.
-Serge(y)
> > >
> > >> +
> > >> + res->irq = pci_irq_vector(pdev, 0);
> > >> + res->wol_irq = res->irq;
> > > Once again. wol_irq is optional. If there is no dedicated WoL IRQ
> > > leave the field as zero.
> > OK.
> >
> > 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;
> > >> + dev_info(&pdev->dev, "%s: multi MSI enablement successful\n", __func__);
> > > What's the point in printing this message especially with the __func__
> > > prefix? You'll always be able to figure out the allocated IRQs by
> > > means of procfs. I suggest to drop it.
> >
> > OK.
> >
> > >
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >> static void loongson_default_data(struct pci_dev *pdev,
> > >> struct plat_stmmacenet_data *plat)
> > >> {
> > >> @@ -296,11 +335,8 @@ static int loongson_gmac_config(struct pci_dev *pdev,
> > >> struct stmmac_resources *res,
> > >> struct device_node *np)
> > >> {
> > >> - int ret;
> > >> -
> > >> - ret = loongson_dwmac_config_legacy(pdev, plat, res, np);
> > >> >> - return ret;
> > >> + return 0;
> > >> }
> > >> >> static struct stmmac_pci_info loongson_gmac_pci_info = {
> > >> @@ -380,11 +416,7 @@ static int loongson_gnet_config(struct pci_dev *pdev,
> > >> struct stmmac_resources *res,
> > >> struct device_node *np)
> > >> {
> > >> - int ret;
> > >> -
> > >> - ret = loongson_dwmac_config_legacy(pdev, plat, res, np);
> > >> -
> > >> - return ret;
> > >> + return 0;
> > >> }
> > > Here you are dropping the changes you just introduced leaving the
> > > config() methods empty... Why not to place the
> > > loongson_dwmac_config_legacy() invocation in the probe() method right
> > > at the patches introducing the config() functions and not to add the
> > > config() callback in the first place?
> > OK, I will try.
> >
> > Thanks,
> >
> > Yanteng
> >
> > >
> > > -Serge(y)
> > >
> > >> >> static struct stmmac_pci_info loongson_gnet_pci_info = {
> > >> @@ -483,6 +515,9 @@ static int loongson_dwmac_probe(struct pci_dev *pdev,
> > >> ld->dwlgmac_dma_ops.dma_interrupt = dwlgmac_dma_interrupt;
> > >> >> plat->setup = loongson_setup;
> > >> + ret = loongson_dwmac_config_multi_msi(pdev, plat, &res, np, 8);
> > >> + } else {
> > >> + ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
> > >> }
> > >> >> plat->bsp_priv = ld;
> > >> -- >> 2.31.4
> > >>
>
Powered by blists - more mailing lists