| 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: <b6a9d2049c744492bac90d62ccc6f2c7@realtek.com>
Date: Thu, 14 Nov 2024 04:01:34 +0000
From: Justin Lai <justinlai0215@...ltek.com>
To: Simon Horman <horms@...nel.org>
CC: "kuba@...nel.org" <kuba@...nel.org>,
"davem@...emloft.net"
<davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"andrew+netdev@...n.ch"
<andrew+netdev@...n.ch>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org"
<netdev@...r.kernel.org>,
Ping-Ke Shih <pkshih@...ltek.com>,
Larry Chiu
<larry.chiu@...ltek.com>
Subject: RE: [PATCH net-next 1/2] rtase: Add support for RTL907XD-VA PCIe port
>
> On Mon, Nov 11, 2024 at 10:55:31AM +0800, Justin Lai wrote:
> > Add RTL907XD-VA hardware version and modify the speed reported by
> > .get_link_ksettings in ethtool_ops.
> >
> > Signed-off-by: Justin Lai <justinlai0215@...ltek.com>
>
> Hi Justin,
>
> this seems to be doing several things:
>
> 1) Adding defines for existing values
> 2) Correcting the speed for RTL907XD-V1
> 3) Adding support for RTL907XD-VA
>
> I think these would be best handled as 3 patches.
> And I wonder if 2) is a bug fix for net rather than an enhancement for net-next.
Ok, I'll try to break down the patch into more detailed parts to
make it clearer.
>
> > ---
> > drivers/net/ethernet/realtek/rtase/rtase.h | 10 +++++--
> > .../net/ethernet/realtek/rtase/rtase_main.c | 26 ++++++++++++++-----
> > 2 files changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h
> > b/drivers/net/ethernet/realtek/rtase/rtase.h
> > index 583c33930f88..2bbfcad613ab 100644
> > --- a/drivers/net/ethernet/realtek/rtase/rtase.h
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase.h
> > @@ -9,7 +9,11 @@
> > #ifndef RTASE_H
> > #define RTASE_H
> >
> > -#define RTASE_HW_VER_MASK 0x7C800000
> > +#define RTASE_HW_VER_MASK 0x7C800000
> > +#define RTASE_HW_VER_906X_7XA 0x00800000 #define
> > +RTASE_HW_VER_906X_7XC 0x04000000 #define
> RTASE_HW_VER_907XD_V1
> > +0x04800000 #define RTASE_HW_VER_907XD_VA 0x08000000
> >
> > #define RTASE_RX_DMA_BURST_256 4
> > #define RTASE_TX_DMA_BURST_UNLIMITED 7 @@ -170,7 +174,7 @@
> enum
> > rtase_registers {
> > RTASE_INT_MITI_TX = 0x0A00,
> > RTASE_INT_MITI_RX = 0x0A80,
> >
> > - RTASE_VLAN_ENTRY_0 = 0xAC80,
> > + RTASE_VLAN_ENTRY_0 = 0xAC80,
>
> This change doesn't seem related to the rest of the patch.
I'll separate this into an additional patch and upload it.
>
> > };
> >
> > enum rtase_desc_status_bit {
> > @@ -327,6 +331,8 @@ struct rtase_private {
> > u16 int_nums;
> > u16 tx_int_mit;
> > u16 rx_int_mit;
> > +
> > + u32 hw_ver;
> > };
> >
> > #define RTASE_LSO_64K 64000
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > index f8777b7663d3..73ebdf0bc376 100644
> > --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > @@ -1714,10 +1714,22 @@ static int rtase_get_settings(struct net_device
> *dev,
> > struct ethtool_link_ksettings *cmd) {
> > u32 supported = SUPPORTED_MII | SUPPORTED_Pause |
> > SUPPORTED_Asym_Pause;
> > + const struct rtase_private *tp = netdev_priv(dev);
> >
> >
> ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
> > supported);
> > - cmd->base.speed = SPEED_5000;
> > +
> > + switch (tp->hw_ver) {
> > + case RTASE_HW_VER_906X_7XA:
> > + case RTASE_HW_VER_906X_7XC:
> > + cmd->base.speed = SPEED_5000;
> > + break;
> > + case RTASE_HW_VER_907XD_V1:
> > + case RTASE_HW_VER_907XD_VA:
> > + cmd->base.speed = SPEED_10000;
> > + break;
> > + }
> > +
> > cmd->base.duplex = DUPLEX_FULL;
> > cmd->base.port = PORT_MII;
> > cmd->base.autoneg = AUTONEG_DISABLE;
>
> > @@ -1974,13 +1986,15 @@ static void
> > rtase_init_software_variable(struct pci_dev *pdev,
> >
> > static bool rtase_check_mac_version_valid(struct rtase_private *tp)
> > {
> > - u32 hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) &
> RTASE_HW_VER_MASK;
> > bool known_ver = false;
> >
> > - switch (hw_ver) {
> > - case 0x00800000:
> > - case 0x04000000:
> > - case 0x04800000:
> > + tp->hw_ver = rtase_r32(tp, RTASE_TX_CONFIG_0) &
> > + RTASE_HW_VER_MASK;
>
> Now that this is setting tp->hw_ver perhaps the name of the function should be
> changed? Perhaps rtase_set_mac_version() ? Perhaps a single patch can be
> created that reworks this function, preparing for other work, by:
>
> * Changes the name of the function
> * Sets tp->hw_ver
> * Changes the return type from bool to int
> (as is currently done as part of patch 2/2)
This function is not simply used to set tp->hw_ver. Its primary purpose
is to validate the MAC version. Since hw_ver is also used elsewhere, it
is stored in tp->hw_ver. Therefore, I don't believe the function name
needs to be changed. However, I will group the remaining two items into
a separate patch and include it in this patch set.
>
> Although a refactor, perhaps that could be part of a series for net that also
> includes two more patches that depend on it and:
>
> * Correct the speed for RTL907XD-V1
> * Corrects error handling in the case where the version is invalid
> (as is currently done as part of patch 2/2)
Thank you for your valuable suggestions. I will upload the three patches
as discussed to the net.
>
> And then any remaning enhancements can be addressed as follow-up patches
> for net-next.
Ok, I will do that.
>
>
> > +
> > + switch (tp->hw_ver) {
> > + case RTASE_HW_VER_906X_7XA:
> > + case RTASE_HW_VER_906X_7XC:
> > + case RTASE_HW_VER_907XD_V1:
> > + case RTASE_HW_VER_907XD_VA:
> > known_ver = true;
> > break;
> > }
> > --
> > 2.34.1
> >
Powered by blists - more mailing lists