| 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: <f9c5c697-6c3f-4cfb-aa60-2031b450a470@loongson.cn>
Date: Thu, 14 Mar 2024 21:13:58 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>
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
在 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。
> >> + }
> >> +
> >> + 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.
> >
> >> +
> >> + 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