netdev - Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add Loongson GNET support

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 17 May 2024 19:14:32 +0800
From: yanteng si <siyanteng01@...il.com>
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, chenhuacai@...nel.org, linux@...linux.org.uk, 
	guyinggang@...ngson.cn, netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add
 Loongson GNET support

Hi Serge,

I can't access siyanteng@...ngson.cn right now for some reason,
so let's switch to siyanteng01@...il.com.

Serge Semin <fancer.lancer@...il.com> 于2024年5月17日周五 17:48写道：

> > > > +
> > > > +static int loongson_gnet_dma_interrupt(struct stmmac_priv *priv,
> > > > +                                void __iomem *ioaddr,
> > > > +                                struct stmmac_extra_stats *x,
> > > > +                                u32 chan, u32 dir)
> > > > +{
> > > > + struct stmmac_pcpu_stats *stats =
> > > ...
> > > > + /* Clear the interrupt by writing a logic 1 to the CSR5[15-0] */
> > > 0x7ffff != CSR5[15-0]
> >
>
> > Hmmm, It should be CSR5[19-0]?
>
> 0x7ffff = [18-0]
> 0xfffff = [19-0]
>
> >
> > BTW, 0x1ffff != CSR5[15-0], too.
> >
> > It should be CSR5[16-0], right?
>
> Right. If you wish to fix that in the original code, that has to be
> done in a dedicated patch.
OK.
>
> > > > +
> > > > + 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);
>
> > > Hm, this must be justified and better being done in a separate patch.
> > OK.
>
> AFAICS the pci_enable_msi()/pci_disable_msi() calls can be dropped due
> to the Loongson GMAC not having the MSI IRQs delivered (at least
> that's what I got from the discussion and from the original driver,
> please correct my if I am wrong). Thus no need in the MSI capability
> being enabled. Meanwhile the multi-channels Loongson GNET will use the
> pci_alloc_irq_vectors()/pci_free_irq_vectors() functions for the IRQ
> vectors allocation and freeing which already perform the MSIs
> enable/disable by design.
Ok, I will try.
> * But once again, please drop the functions call in a separate patch
> submitted with the proper commit log justifying the removal.
Ok.
>
> > >
> > > Besides I don't see you freeing the IRQ vectors allocated in the
> > > loongson_dwmac_config_msi() method neither in probe(), nor in remove()
> > > functions. That's definitely wrong. What you need is to have a
> > > method antagonistic to loongson_dwmac_config_msi() (like
> > > loongson_dwmac_clear_msi()) which would execute the cleanup procedure.
> >
>
> > Hmmm, We can free it in struct pci_driver ->remove method.
> >
> > Just in loongson_dwmac_remove() call
> >
> > pci_free_irq_vectors(pdev);
>
> Sounds good. Although I would have implemented that in a more
> maintainable way:
>
> loongson_dwmac_config_msi()
> {
>         ...
> }
>
> loongson_dwmac_clear_msi()
> {
>         pci_free_irq_vectors(pdev)
> }
>
> ...
>
> loongson_dwmac_remove()
> {
>         ...
>         if (ld->loongson_id == DWMAC_CORE_LS2K2000)
>                 loongson_dwmac_clear_msi();
>         ...
> }
>
loongson_dwmac_clear_msi() looks better, and you'll see it in RFC/v13.

Thanks,
Yanteng