netdev - Re: [PATCH v5 3/9] net: stmmac: Add Loongson DWGMAC definitions

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]

Date: Tue, 21 Nov 2023 17:55:24 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Andrew Lunn <andrew@...n.ch>
Cc: hkallweit1@...il.com, peppe.cavallaro@...com,
 alexandre.torgue@...s.st.com, joabreu@...opsys.com, fancer.lancer@...il.com,
 Jose.Abreu@...opsys.com, chenhuacai@...ngson.cn, linux@...linux.org.uk,
 dongbiao@...ngson.cn, guyinggang@...ngson.cn, netdev@...r.kernel.org,
 loongarch@...ts.linux.dev, chris.chenfeiyang@...il.com
Subject: Re: [PATCH v5 3/9] net: stmmac: Add Loongson DWGMAC definitions

Hi Andrew,

在 2023/11/12 04:07, Andrew Lunn 写道:
>> +#ifdef	CONFIG_DWMAC_LOONGSON
>> +#define DMA_INTR_ABNORMAL	(DMA_INTR_ENA_AIE_LOONGSON | DMA_INTR_ENA_AIE | \
>> +				DMA_INTR_ENA_FBE | DMA_INTR_ENA_UNE)
>> +#else
>>   #define DMA_INTR_ABNORMAL	(DMA_INTR_ENA_AIE | DMA_INTR_ENA_FBE | \
>>   				DMA_INTR_ENA_UNE)
>> +#endif
> The aim is to produce one kernel which runs on all possible
> variants. So we don't like to see this sort of #ifdef. Please try to
> remove them.

We now run into a tricky problem: we only have a few register 
definitions(DMA_XXX_LOONGSON)

that are not the same as the dwmac1000 register definition.


In this case, if we use these "#ifdef", it will reuse most of the 
dwmac1000 code to

reduce maintenance stress, at the cost of sacrificing a little 
readability; If we created

a new xxx_dma.h, it would add a new set of code similar to dwmac1000, 
which is exactly

what the v4 patch did, which was great for readability, but it also made 
the code more

maintenance stress, and we got reviews complaining in v4. That is, we 
need to find a

balance between readability and maintainability.


however， we haven't yet come up with a way to do both, so it would be 
great if you

could give us some advice on this.


v4:<https://lore.kernel.org/loongarch/cover.1692696115.git.chenfeiyang@loongson.cn/>

>
>> @@ -167,7 +167,7 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
>>   	struct stmmac_txq_stats *txq_stats = &priv->xstats.txq_stats[chan];
>>   	int ret = 0;
>>   	/* read the status register (CSR5) */
>> -	u32 intr_status = readl(ioaddr + DMA_STATUS);
>> +	u32 intr_status = readl(ioaddr + DMA_CHAN_STATUS(chan));
>>   
> Please break this patch up. Changes like the above are simple to
> understand. So have one patch which just adds these macros and makes
> use of them.
OK!
>
>>   #ifdef DWMAC_DMA_DEBUG
>>   	/* Enable it to monitor DMA rx/tx status in case of critical problems */
>> @@ -182,7 +182,7 @@ int dwmac_dma_interrupt(struct stmmac_priv *priv, void __iomem *ioaddr,
>>   		intr_status &= DMA_STATUS_MSK_TX;
>>   
>>   	/* ABNORMAL interrupts */
>> -	if (unlikely(intr_status & DMA_STATUS_AIS)) {
>> +	if (unlikely(intr_status & DMA_ABNOR_INTR_STATUS)) {
> However, this is not obviously correct. You need to explain in the
> commit message why this change is needed.
>
> Lots of small patches make the understanding easier.

Because Loongson has two AIS, so:


#ifdef    CONFIG_DWMAC_LOONGSON
...
#define DMA_ABNOR_INTR_STATUS        (DMA_STATUS_TX_AIS_LOONGSON | 
DMA_STATUS_RX_AIS_LOONGSON)
...
#else
...
#define DMA_ABNOR_INTR_STATUS        DMA_STATUS_AIS
...
#endif

>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 7371713c116d..aafc75fa14a0 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -7062,6 +7062,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>>   	/* dwmac-sun8i only work in chain mode */
>>   	if (priv->plat->flags & STMMAC_FLAG_HAS_SUN8I)
>>   		chain_mode = 1;
>> +
>>   	priv->chain_mode = chain_mode;
> Please avoid white space changes.

OK!


Thanks for your review!


Thanks,

Yanteng

>
>         Andrew