| 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
| ||
|
Date: Thu, 14 Mar 2024 21:08:41 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
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, guyinggang@...ngson.cn, netdev@...r.kernel.org,
chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 09/11] net: stmmac: dwmac-loongson: Fix half
duplex
在 2024/3/13 18:21, Russell King (Oracle) 写道:
> On Wed, Mar 13, 2024 at 05:24:52PM +0800, Yanteng Si wrote:
>> 在 2024/2/6 06:06, Serge Semin 写道:
>>> On Tue, Feb 06, 2024 at 12:58:17AM +0300, Serge Semin wrote:
>>>> On Tue, Jan 30, 2024 at 04:49:14PM +0800, Yanteng Si wrote:
>>>>> Current GNET does not support half duplex mode.
>>>>>
>>>>> Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
>>>>> Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
>>>>> Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
>>>>> ---
>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 11 ++++++++++-
>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
>>>>> include/linux/stmmac.h | 1 +
>>>>> 3 files changed, 13 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>>>>> index 264c4c198d5a..1753a3c46b77 100644
>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>>>>> @@ -432,8 +432,17 @@ static int loongson_gnet_config(struct pci_dev *pdev,
>>>>> struct stmmac_resources *res,
>>>>> struct device_node *np)
>>>>> {
>>>>> - if (pdev->revision == 0x00 || pdev->revision == 0x01)
>>>>> + switch (pdev->revision) {
>>>>> + case 0x00:
>>>>> + plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000 |
>>>>> + STMMAC_FLAG_DISABLE_HALF_DUPLEX;
>>>>> + break;
>>>>> + case 0x01:
>>>>> plat->flags |= STMMAC_FLAG_DISABLE_FORCE_1000;
>>>>> + break;
>>>>> + default:
>>>>> + break;
>>>>> + }
>>>> Move this change into the patch
>>>> [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support
>>>>
>>>>> return 0;
>>>>> }
>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>>> index 5617b40abbe4..3aa862269eb0 100644
>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>>> @@ -1201,7 +1201,8 @@ static int stmmac_init_phy(struct net_device *dev)
>>>>> static void stmmac_set_half_duplex(struct stmmac_priv *priv)
>>>>> {
>>>>> /* Half-Duplex can only work with single tx queue */
>>>>> - if (priv->plat->tx_queues_to_use > 1)
>>>>> + if (priv->plat->tx_queues_to_use > 1 ||
>>>>> + (STMMAC_FLAG_DISABLE_HALF_DUPLEX & priv->plat->flags))
>>>>> priv->phylink_config.mac_capabilities &=
>>>>> ~(MAC_10HD | MAC_100HD | MAC_1000HD);
>>>>> else
>>>>> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
>>>>> index 2810361e4048..197f6f914104 100644
>>>>> --- a/include/linux/stmmac.h
>>>>> +++ b/include/linux/stmmac.h
>>>>> @@ -222,6 +222,7 @@ struct dwmac4_addrs {
>>>>> #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11)
>>>>> #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12)
>>>>> #define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13)
>>>>> +#define STMMAC_FLAG_DISABLE_HALF_DUPLEX BIT(14)
>>>> Place the patch with this change before
>>>> [PATCH net-next v8 06/11] net: stmmac: dwmac-loongson: Add GNET support
>>>> as a pre-requisite/preparation patch. Don't forget a thorough
>>>> description of what is wrong with the GNET Half-Duplex mode.
>>> BTW what about re-defining the stmmac_ops.phylink_get_caps() callback
>>> instead of adding fixup flags in this patch and in the next one?
>> ok.
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> index ac1b48ff7199..b57e1325ce62 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> @@ -238,6 +234,13 @@ static int loongson_gnet_get_hw_feature(void __iomem
>> *ioaddr,
>> return 0;
>> }
Hi Russell,
>> +static void loongson_phylink_get_caps(struct stmmac_priv *priv)
>> +{
>> + priv->phylink_config.mac_capabilities = (MAC_10FD |
>> + MAC_100FD | MAC_1000FD) & ~(MAC_10HD | MAC_100HD | MAC_1000HD);
> Why is this so complicated? It would be silly if the _full duplex_
> definitions also defined the _half duplex_ bits. This should be just:
>
> priv->phylink_config.mac_capabilities = MAC_10FD | MAC_100FD |
> MAC_1000FD;
Yes, you are right. Our gnet device (7a2000) does not support
half-duplex, while the gnet device (2k2000) does.
I plan to use PCI IDand IP CORE as the condition to separate full-duplex
and half-duplex.
>
> if that is all you support. Do you not support any pause modes (they
> would need to be included as well here.)
I have tested it and our gnet device supports MAC_ASYM_PAUSE and
MAC_SYM_PAUSE, but gmac does not. I will fix this in the patch v9.
This is also easy to do because all GMAC devices have the same PCI ID.
>
> As to this approach, I don't think it's a good model to override the
> stmmac MAC operations. Instead, I would suggest that a better approach
> would be for the platform to provide its capabilities to the stmmac
> core code (maybe a new member in stmmac_priv) which, when set, is used
> to reduce the capabilities provided to phylink via
> priv->phylink_config.mac_capabilities.
>
> Why? The driver has several components that are involved in the
> overall capabilities, and the capabilities of the system is the
> logical subset of all these capabilities. One component should not
> be setting capabilities that a different component doesn't support.
Hi Serge,
It seems to be going back again, what do you think?
Thanks,
Yanteng
>
Powered by blists - more mailing lists