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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <zqzkanogzdaqvjjobnoidhl4tqeho5uqldsa2r46ttbxo3y4dt@tdouti2x652a>
Date: Wed, 20 Mar 2024 14:24:23 +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 11/11] net: stmmac: dwmac-loongson: Disable
 coe for some Loongson GNET

Hi Yanteng

On Wed, Mar 13, 2024 at 06:19:47PM +0800, Yanteng Si wrote:
> 
> 在 2024/3/13 17:52, Yanteng Si 写道:
> > 
> > 在 2024/2/6 06:18, Serge Semin 写道:
> > > On Tue, Jan 30, 2024 at 04:49:16PM +0800, Yanteng Si wrote:
> > > > Some chips of Loongson GNET does not support coe, so disable them.
> > > s/coe/Tx COE
> > OK.
> > > 
> > > > Set dma_cap->tx_coe to 0 and overwrite get_hw_feature.
> > > > 
> > > > 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  | 46
> > > > +++++++++++++++++++
> > > >   1 file changed, 46 insertions(+)
> > > > 
> > > > diff --git
> > > > a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > > b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > > index b78a73ea748b..8018d7d5f31b 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > > > @@ -196,6 +196,51 @@ static int dwlgmac_dma_interrupt(struct
> > > > stmmac_priv *priv, void __iomem *ioaddr,
> > > >       return ret;
> > > >   }
> > > >   +static int dwlgmac_get_hw_feature(void __iomem *ioaddr,
> > > Please use GNET-specific prefix.
> > OK. loongson_gnet_get_hw_feature()
> > > 
> > > > +                  struct dma_features *dma_cap)
> > > > +{
> > > > +    u32 hw_cap = readl(ioaddr + DMA_HW_FEATURE);
> > > > +
> > > > +    if (!hw_cap) {
> > > > +        /* 0x00000000 is the value read on old hardware that does not
> > > > +         * implement this register
> > > > +         */
> > > > +        return -EOPNOTSUPP;
> > > > +    }
> > > This doesn't seems like possible. All your devices have the
> > > HW-features register. If so please drop.
> > OK, drop it.
> > > 
> > > > +
> > > > +    dma_cap->mbps_10_100 = (hw_cap & DMA_HW_FEAT_MIISEL);
> > > > +    dma_cap->mbps_1000 = (hw_cap & DMA_HW_FEAT_GMIISEL) >> 1;
> > > > +    dma_cap->half_duplex = (hw_cap & DMA_HW_FEAT_HDSEL) >> 2;
> > > > +    dma_cap->hash_filter = (hw_cap & DMA_HW_FEAT_HASHSEL) >> 4;
> > > > +    dma_cap->multi_addr = (hw_cap & DMA_HW_FEAT_ADDMAC) >> 5;
> > > > +    dma_cap->pcs = (hw_cap & DMA_HW_FEAT_PCSSEL) >> 6;
> > > > +    dma_cap->sma_mdio = (hw_cap & DMA_HW_FEAT_SMASEL) >> 8;
> > > > +    dma_cap->pmt_remote_wake_up = (hw_cap & DMA_HW_FEAT_RWKSEL) >> 9;
> > > > +    dma_cap->pmt_magic_frame = (hw_cap & DMA_HW_FEAT_MGKSEL) >> 10;
> > > > +    /* MMC */
> > > > +    dma_cap->rmon = (hw_cap & DMA_HW_FEAT_MMCSEL) >> 11;
> > > > +    /* IEEE 1588-2002 */
> > > > +    dma_cap->time_stamp =
> > > > +        (hw_cap & DMA_HW_FEAT_TSVER1SEL) >> 12;
> > > > +    /* IEEE 1588-2008 */
> > > > +    dma_cap->atime_stamp = (hw_cap & DMA_HW_FEAT_TSVER2SEL) >> 13;
> > > > +    /* 802.3az - Energy-Efficient Ethernet (EEE) */
> > > > +    dma_cap->eee = (hw_cap & DMA_HW_FEAT_EEESEL) >> 14;
> > > > +    dma_cap->av = (hw_cap & DMA_HW_FEAT_AVSEL) >> 15;
> > > > +    /* TX and RX csum */
> > > > +    dma_cap->tx_coe = 0;
> > > > +    dma_cap->rx_coe_type1 = (hw_cap & DMA_HW_FEAT_RXTYP1COE) >> 17;
> > > > +    dma_cap->rx_coe_type2 = (hw_cap & DMA_HW_FEAT_RXTYP2COE) >> 18;
> > > > +    dma_cap->rxfifo_over_2048 = (hw_cap &
> > > > DMA_HW_FEAT_RXFIFOSIZE) >> 19;
> > > > +    /* TX and RX number of channels */
> > > > +    dma_cap->number_rx_channel = (hw_cap & DMA_HW_FEAT_RXCHCNT) >> 20;
> > > > +    dma_cap->number_tx_channel = (hw_cap & DMA_HW_FEAT_TXCHCNT) >> 22;
> > > > +    /* Alternate (enhanced) DESC mode */
> > > > +    dma_cap->enh_desc = (hw_cap & DMA_HW_FEAT_ENHDESSEL) >> 24;
> > > I am not sure whether you need to parse the capability register at all
> > > seeing this is a GNET-specific method. For that device all the
> > > capabilities are already known and can be just initialized in this
> > > method.
> > -dma_cap->tx_coe = (hw_cap & DMA_HW_FEAT_TXCOESEL) >> 16;
> > 
> > +dma_cap->tx_coe = 0;
> > 
> > I'm a little confused. Actually, I only modified this line, which is
> > used to fix the checksum.
> > 
> > 2k2000  of Loongson GNET does not support coe.
> 
> Specifically, it is to ensure the normal operation of multiple channels, as
> other channels except for channel 0 cannot perform checksum.

Originally I thought that Tx-COE was fully broken, but seeing it works
for channel 0 changes the situation. While we kept discussing your
series a useful patch was merged into the driver:
https://lore.kernel.org/netdev/20230916063312.7011-3-rohan.g.thomas@intel.com/
The stmmac_txq_cfg::coe_unsupported flag can be used to disable Tx COE
for the particular channels. You can just set the coe_unsupported flag
for the channels greater than zero and the skb_checksum_help() helper
will be utilized for them to calculate the packets control sum. This
will be the most optimal solution since channel zero will still be
serviced by the Tx Checksum Offload Engine and you won't need the
stmmac_dma_ops::get_hw_feature() callback redefinition.

-Serge(y)

> 
> Thanks,
> 
> Yanteng
> 
> 
> > 
> > 
> > Thanks,
> > Yanteng
> > 
> > > 
> > > -Serge(y)
> > > 
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > >   struct stmmac_pci_info {
> > > >       int (*setup)(struct pci_dev *pdev, struct
> > > > plat_stmmacenet_data *plat);
> > > >       int (*config)(struct pci_dev *pdev, struct
> > > > plat_stmmacenet_data *plat,
> > > > @@ -542,6 +587,7 @@ static int loongson_dwmac_probe(struct
> > > > pci_dev *pdev,
> > > >           ld->dwlgmac_dma_ops = dwmac1000_dma_ops;
> > > >           ld->dwlgmac_dma_ops.init_chan = dwlgmac_dma_init_channel;
> > > >           ld->dwlgmac_dma_ops.dma_interrupt = dwlgmac_dma_interrupt;
> > > > +        ld->dwlgmac_dma_ops.get_hw_feature = dwlgmac_get_hw_feature;
> > > >             plat->setup = loongson_setup;
> > > >           ret = loongson_dwmac_config_multi_msi(pdev, plat,
> > > > &res, np, 8);
> > > > -- 
> > > > 2.31.4
> > > > 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ