| 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: <eqecwmi3guwda3wloxcttkx2xlteupvrsetb5ro5abupwhxqyu@ypliwpyswy23>
Date: Tue, 6 Feb 2024 00:28:16 +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 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.
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).
> 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...
>
> 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.
> }
>
> 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?
> + 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?
> +{
> + 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?
> + }
> +
> + 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.
> +
> + 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.
> +
> + /* 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.
> +
> + 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?
-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