| 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: Tue, 31 Aug 2021 22:10:43 +0800
From: Jie Luo <luoj@...eaurora.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
kuba@...nel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, sricharan@...eaurora.org
Subject: Re: [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver
On 8/30/2021 9:48 PM, Andrew Lunn wrote:
> On Mon, Aug 30, 2021 at 07:07:32PM +0800, Luo Jie wrote:
>> qca8081 is a single port ethernet phy chip that supports
>> 10/100/1000/2500 Mbps mode.
>>
>> Signed-off-by: Luo Jie <luoj@...eaurora.org>
>> ---
>> drivers/net/phy/at803x.c | 389 ++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 338 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>> index ecae26f11aa4..2b3563ae152f 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>> @@ -33,10 +33,10 @@
>> #define AT803X_SFC_DISABLE_JABBER BIT(0)
>>
>> #define AT803X_SPECIFIC_STATUS 0x11
>> -#define AT803X_SS_SPEED_MASK (3 << 14)
>> -#define AT803X_SS_SPEED_1000 (2 << 14)
>> -#define AT803X_SS_SPEED_100 (1 << 14)
>> -#define AT803X_SS_SPEED_10 (0 << 14)
>> +#define AT803X_SS_SPEED_MASK GENMASK(15, 14)
>> +#define AT803X_SS_SPEED_1000 2
>> +#define AT803X_SS_SPEED_100 1
>> +#define AT803X_SS_SPEED_10 0
> This looks like an improvement, and nothing to do with qca8081. Please
> make it an separate patch.
will make it an separate patch in the next patch series.
>> #define AT803X_SS_DUPLEX BIT(13)
>> #define AT803X_SS_SPEED_DUPLEX_RESOLVED BIT(11)
>> #define AT803X_SS_MDIX BIT(6)
>> @@ -158,6 +158,8 @@
>> #define QCA8337_PHY_ID 0x004dd036
>> #define QCA8K_PHY_ID_MASK 0xffffffff
>>
>> +#define QCA8081_PHY_ID 0x004dd101
>> +
> Maybe keep all the PHY_ID together?
will move it to make PHY_ID together in the next patch series.
>
>> #define QCA8K_DEVFLAGS_REVISION_MASK GENMASK(2, 0)
>>
>> #define AT803X_PAGE_FIBER 0
>> @@ -167,7 +169,73 @@
>> #define AT803X_KEEP_PLL_ENABLED BIT(0)
>> #define AT803X_DISABLE_SMARTEEE BIT(1)
>>
>> @@ -711,11 +779,18 @@ static void at803x_remove(struct phy_device *phydev)
>>
>> static int at803x_get_features(struct phy_device *phydev)
>> {
>> - int err;
>> + int val;
> Why? The driver pretty consistently uses err for return values which
> are errors.
will keep err unchanged in the next patch set.
>
>>
>> - err = genphy_read_abilities(phydev);
>> - if (err)
>> - return err;
>> + val = genphy_read_abilities(phydev);
>> + if (val)
>> + return val;
>> +
>> + if (at803x_match_phy_id(phydev, QCA8081_PHY_ID)) {
>> + val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
> You don't check if val indicates if there was an error.
thanks Andrew for the comment, will add the check here.
>
>> +
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported,
>> + val & MDIO_PMA_NG_EXTABLE_2_5GBT);
>> + }
>>
>> if (!at803x_match_phy_id(phydev, ATH8031_PHY_ID))
>> return 0;
>> @@ -935,44 +1010,44 @@ static void at803x_link_change_notify(struct phy_device *phydev)
>> }
>> }
>>
>> -static int at803x_read_status(struct phy_device *phydev)
>> +static int at803x_read_specific_status(struct phy_device *phydev)
>> {
>> - int ss, err, old_link = phydev->link;
>> -
>> - /* Update the link, but return if there was an error */
>> - err = genphy_update_link(phydev);
>> - if (err)
>> - return err;
>> -
>> - /* why bother the PHY if nothing can have changed */
>> - if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
>> - return 0;
>> + int val;
>>
>> - phydev->speed = SPEED_UNKNOWN;
>> - phydev->duplex = DUPLEX_UNKNOWN;
>> - phydev->pause = 0;
>> - phydev->asym_pause = 0;
>> + val = phy_read(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL);
>> + if (val < 0)
>> + return val;
>>
>> - err = genphy_read_lpa(phydev);
>> - if (err < 0)
>> - return err;
>> + switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, val)) {
>> + case AT803X_SFC_MANUAL_MDI:
>> + phydev->mdix_ctrl = ETH_TP_MDI;
>> + break;
>> + case AT803X_SFC_MANUAL_MDIX:
>> + phydev->mdix_ctrl = ETH_TP_MDI_X;
>> + break;
>> + case AT803X_SFC_AUTOMATIC_CROSSOVER:
>> + phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>> + break;
>> + }
>>
>> /* Read the AT8035 PHY-Specific Status register, which indicates the
>> * speed and duplex that the PHY is actually using, irrespective of
>> * whether we are in autoneg mode or not.
>> */
>> - ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
>> - if (ss < 0)
>> - return ss;
>> + val = phy_read(phydev, AT803X_SPECIFIC_STATUS);
>> + if (val < 0)
>> + return val;
> What was actually wrong with ss?
>
> Is this another case of just copying code from your other driver,
> rather than cleanly extending the existing driver?
>
> There are two many changes here all at once. Please break this patch
> up. You are aiming for lots of small patches which are obviously
> correct. Part of being obviously correct is having a good commit
> message, and that gets much easier when a patch is small.
>
> Andrew
Hi Andrew,
i separate the phy specific status into a new function
at803x_read_specific_status, since this function
need to be used for both at803x phy driver and qca8081 phy driver.
i will break the patch into the minimal changes and provide the commit
message in detail in the next
patch series.
thanks for your helpful comments.
Powered by blists - more mailing lists