netdev - Re: [PATCH net-next] r8169: add new helper rtl8168g_enable_gphy

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]

Message-ID: <792711d6-6c35-02b3-2df6-1af02c581ef3@gmail.com>
Date:   Sun, 22 Mar 2020 00:29:51 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Joe Perches <joe@...ches.com>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] r8169: add new helper rtl8168g_enable_gphy_10m

On 21.03.2020 19:55, Joe Perches wrote:
> On Sat, 2020-03-21 at 19:08 +0100, Heiner Kallweit wrote:
>> Factor out setting GPHY 10M to new helper rtl8168g_enable_gphy_10m.
> []
>> diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c b/drivers/net/ethernet/realtek/r8169_phy_config.c
> []
>> @@ -796,6 +796,11 @@ static void rtl8168g_disable_aldps(struct phy_device *phydev)
>>  	phy_modify_paged(phydev, 0x0a43, 0x10, BIT(2), 0);
>>  }
>>  
>> +static void rtl8168g_enable_gphy_10m(struct phy_device *phydev)
>> +{
>> +	phy_modify_paged(phydev, 0x0a44, 0x11, 0, BIT(11));
>> +}
> 
> Perhaps this should be some generic to set characteristics like:
> 
> enum rtl8168g_char {
> 	...
> }
> 
> static void rtl8168g_enable_char(struct phy_device *phydev,
> 				 enum rtl8168g_char type)
> {
> 	switch (type) {
> 	case FOO:
> 		etc...
> 	}
> 
> 

Then we'd need one more such generic to disable characteristics like
in rtl8168g_disable_aldps(). Also we have existing functions like
rtl8168g_phy_adjust_10m_aldps() that change a characteristic, and
the function name is used to describe what the function does.
So yes, it would be an option, but I don't see that we would gain
anything. More the opposite, it would add overhead and we'd have a
mix of fct_what_it_does() and fct_generic(WHAT_IT_DOES).