| 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: <36655415-551d-11e7-a834-10bb6f07b2d0@gmail.com>
Date: Thu, 26 Mar 2020 00:21:45 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Vivien Didelot <vivien.didelot@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <jakub.kicinski@...ronome.com>,
murali.policharla@...adcom.com,
Stephen Hemminger <stephen@...workplumber.org>,
Jiri Pirko <jiri@...nulli.us>,
Ido Schimmel <idosch@...sch.org>,
Jakub Kicinski <kuba@...nel.org>,
Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 02/10] net: phy: bcm7xx: Add jumbo frame
configuration to PHY
On 25.03.2020 23:45, Vladimir Oltean wrote:
> On Wed, 25 Mar 2020 at 17:44, Heiner Kallweit <hkallweit1@...il.com> wrote:
>>
>> On 25.03.2020 16:22, Vladimir Oltean wrote:
>>> From: Murali Krishna Policharla <murali.policharla@...adcom.com>
>>>
>>> Add API to configure jumbo frame settings in PHY during initial PHY
>>> configuration.
>>>
>>> Signed-off-by: Murali Krishna Policharla <murali.policharla@...adcom.com>
>>> Reviewed-by: Scott Branden <scott.branden@...adcom.com>
>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>>> ---
>>> drivers/net/phy/bcm-phy-lib.c | 28 ++++++++++++++++++++++++++++
>>> drivers/net/phy/bcm-phy-lib.h | 1 +
>>> drivers/net/phy/bcm7xxx.c | 4 ++++
>>> include/linux/brcmphy.h | 1 +
>>> 4 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
>>> index e0d3310957ff..a26c80e13b43 100644
>>> --- a/drivers/net/phy/bcm-phy-lib.c
>>> +++ b/drivers/net/phy/bcm-phy-lib.c
>>> @@ -423,6 +423,34 @@ int bcm_phy_28nm_a0b0_afe_config_init(struct phy_device *phydev)
>>> }
>>> EXPORT_SYMBOL_GPL(bcm_phy_28nm_a0b0_afe_config_init);
>>>
>>> +int bcm_phy_enable_jumbo(struct phy_device *phydev)
>>> +{
>>> + int val = 0, ret = 0;
>>> +
>>> + ret = phy_write(phydev, MII_BCM54XX_AUX_CTL,
>>> + MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + val = phy_read(phydev, MII_BCM54XX_AUX_CTL);
>>> +
>>> + /* Enable extended length packet reception */
>>> + val |= MII_BCM54XX_AUXCTL_ACTL_EXT_PKT_LEN;
>>> + ret = phy_write(phydev, MII_BCM54XX_AUX_CTL, val);
>>> +
>>
>> There are different helpers already in bcm-phy-lib,
>> e.g. bcm54xx_auxctl_read. Also bcm_phy_write_misc()
>> has has quite something in common with your new function.
>> It would be good if a helper could be used here.
>>
>
> Thanks Heiner.
> I'm not quite sure the operation is performed correctly though? My
> books are telling me that the "Receive Extended Packet Length" field
> is accessible via the Auxiliary Control Register 0x18 when the shadow
> value is 000, not 111 as this patch is doing. At least for BCM54xxx in
> terms of which the macros are defined. Am I wrong?
>
I didn't check the datasheet, so I can't really comment on whether the
patch does what it is supposed to do. My point was that the following
pattern occurs several times in the driver:
phy_write(phydev, MII_BCM54XX_AUX_CTL, VAL_1);
phy_modify(phydev, MII_BCM54XX_AUX_CTL, VAL_2, VAL_3)
(or phy_clear_bits/phy_set_bits instead of phy_modify)
Therefore a generic helper could make sense, or it could at least be
written as such a two-liner always.
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + val = phy_read(phydev, MII_BCM54XX_ECR);
>>> +
>>> + /* Enable 10K byte packet length reception */
>>> + val |= BIT(0);
>>> + ret = phy_write(phydev, MII_BCM54XX_ECR, val);
>>> +
>>
>> Why not use phy_set_bits() ?
>>
>
> Well, the reason is that I didn't write the patch. I'll simplify it.
>
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(bcm_phy_enable_jumbo);
>>> +
>>> MODULE_DESCRIPTION("Broadcom PHY Library");
>>> MODULE_LICENSE("GPL v2");
>>> MODULE_AUTHOR("Broadcom Corporation");
>>> diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
>>> index c86fb9d1240c..129df819be8c 100644
>>> --- a/drivers/net/phy/bcm-phy-lib.h
>>> +++ b/drivers/net/phy/bcm-phy-lib.h
>>> @@ -65,5 +65,6 @@ void bcm_phy_get_stats(struct phy_device *phydev, u64 *shadow,
>>> struct ethtool_stats *stats, u64 *data);
>>> void bcm_phy_r_rc_cal_reset(struct phy_device *phydev);
>>> int bcm_phy_28nm_a0b0_afe_config_init(struct phy_device *phydev);
>>> +int bcm_phy_enable_jumbo(struct phy_device *phydev);
>>>
>>> #endif /* _LINUX_BCM_PHY_LIB_H */
>>> diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
>>> index af8eabe7a6d4..692048d86ab1 100644
>>> --- a/drivers/net/phy/bcm7xxx.c
>>> +++ b/drivers/net/phy/bcm7xxx.c
>>> @@ -178,6 +178,10 @@ static int bcm7xxx_28nm_config_init(struct phy_device *phydev)
>>> break;
>>> }
>>>
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = bcm_phy_enable_jumbo(phydev);
>>> if (ret)
>>> return ret;
>>>
>>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
>>> index b475e7f20d28..19bd86019e93 100644
>>> --- a/include/linux/brcmphy.h
>>> +++ b/include/linux/brcmphy.h
>>> @@ -119,6 +119,7 @@
>>> #define MII_BCM54XX_AUXCTL_SHDWSEL_AUXCTL 0x00
>>> #define MII_BCM54XX_AUXCTL_ACTL_TX_6DB 0x0400
>>> #define MII_BCM54XX_AUXCTL_ACTL_SMDSP_ENA 0x0800
>>> +#define MII_BCM54XX_AUXCTL_ACTL_EXT_PKT_LEN 0x4000
>>>
>>> #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC 0x07
>>> #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN 0x0010
>>>
>>
>
> Regards,
> -Vladimir
> .
>
Powered by blists - more mailing lists