netdev - Re: [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0350413e-be0e-39ac-411f-f4fa9470855b@gmail.com>
Date:   Mon, 25 Feb 2019 08:34:35 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        David Miller <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support

On 25.02.2019 03:17, Florian Fainelli wrote:
> Le 2/24/19 à 1:36 PM, Heiner Kallweit a écrit :
>> This adds HWMON support for the temperature sensor and the related
>> alarms on the 107/108/109 chips. This patch is based on work from
>> Nikita and Andrew. I added:
>> - support for changing alarm thresholds via sysfs
>> - move HWMON code to a separate source file to improve maintainability
>> - smaller changes like using IS_REACHABLE instead of ifdef
>>   (avoids problems if PHY driver is built in and HWMON is a module)
>>
>> v2:
>> - remove struct aqr_priv
>> - rename header file to aquantia.h
>>
>> Signed-off-by: Nikita Yushchenko <nikita.yoush@...entembedded.com>
> 
> Should not Nikita be the author for that patch? Some minor nits below:
> 
>> Signed-off-by: Andrew Lunn <andrew@...n.ch>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/phy/Makefile         |   2 +-
>>  drivers/net/phy/aquantia.h       |  16 ++
>>  drivers/net/phy/aquantia_hwmon.c | 245 +++++++++++++++++++++++++++++++
>>  drivers/net/phy/aquantia_main.c  |   4 +
>>  4 files changed, 266 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/net/phy/aquantia.h
>>  create mode 100644 drivers/net/phy/aquantia_hwmon.c
>>
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index b0845adaf..c48596626 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -45,7 +45,7 @@ sfp-obj-$(CONFIG_SFP)		+= sfp-bus.o
>>  obj-y				+= $(sfp-obj-y) $(sfp-obj-m)
>>  
>>  obj-$(CONFIG_AMD_PHY)		+= amd.o
>> -aquantia-objs			+= aquantia_main.o
>> +aquantia-objs			+= aquantia_main.o aquantia_hwmon.o
> 
> Since you are adding a stub if CONFIG_HWMON is not reachable, does not
> that mean you would want to do something like:
> 
> ifdef CONFIG_HWMON
> aquanti-objs	+= aquantia_hwmon.o
> endif
> 
> in this makefile?
> 
Yepp, this should be changed as suggested by you.

> [snip]
> 
>> +static int aqr_hwmon_get(struct phy_device *phydev, int reg, long *value)
>> +{
>> +	int temp = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
>> +
>> +	if (temp < 0)
>> +		return temp;
>> +
>> +	/* register value is 2's complement with LSB = 1/256th degree Celsius */
>> +	if (temp > 0x8000)
>> +		temp -= 0x10000;
> 
> Would decimal or BIT() notation be easier to use/read here?
> 
For the comparison I agree, we can write it as: if (temp & BIT(15))
And yes, the offset we could write decimal.

An alternative could be to write: temp = (s16)temp;
I tested it and it works, but I'm not sure whether this is covered
by C standard and works with all supported compilers.

>> +
>> +	*value = temp * 1000 / 256;
>> +
>> +	return 0;
>> +}
>> +
>> +static int aqr_hwmon_set(struct phy_device *phydev, int reg, long value)
>> +{
>> +	int temp;
>> +
>> +	if (value >= 128000 || value < -128000)
>> +		return -ERANGE;
>> +
>> +	temp = value * 256 / 1000;
>> +
>> +	if (temp < 0)
>> +		temp += 0x10000;
> 
> Likewise
> 
>> +
>> +	return phy_write_mmd(phydev, MDIO_MMD_VEND1, reg, temp);
>> +}
>> +
>> +static int aqr_hwmon_status1(struct phy_device *phydev, int bit, long *value)
>> +{
>> +	int reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GENERAL_STAT1);
>> +
>> +	if (reg < 0)
>> +		return reg;
>> +
>> +	*value = !!(reg & bit);
>> +
>> +	return 0;
>> +}
> 
> Can you also make this function take a register offset as parameter such
> that you can eliminate open coding that in the case hwmon_temp_input below?
> 
OK

>> +
>> +static int aqr_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> +			  u32 attr, int channel, long *value)
>> +{
>> +	struct phy_device *phydev = dev_get_drvdata(dev);
>> +	int reg;
>> +
>> +	if (type != hwmon_temp)
>> +		return -EOPNOTSUPP;
>> +
>> +	switch (attr) {
>> +	case hwmon_temp_input:
>> +		reg = phy_read_mmd(phydev, MDIO_MMD_VEND1,
>> +				   VEND1_THERMAL_STAT2);
>> +		if (reg < 0)
>> +			return reg;
>> +		if (!(reg & VEND1_THERMAL_STAT2_VALID))
>> +			return -EIO;
> 
> And use that helper here.
> 
>> +
>> +		return aqr_hwmon_get(phydev, VEND1_THERMAL_STAT1, value);
>> +
> 
> [snip]
> 
>> +	hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>> +	if (!hwmon_name)
>> +		return -ENOMEM;
>> +
>> +	for (i = j = 0; hwmon_name[i]; i++) {
>> +		if (isalnum(hwmon_name[i])) {
>> +			if (i != j)
>> +				hwmon_name[j] = hwmon_name[i];
>> +			j++;
>> +		}
>> +	}
>> +	hwmon_name[j] = '\0';
> 
> I am always baffled that drivers need to take care of these details...
> but that seems to be the way to go.
> 
IIRC we still have a similar construction site in phylib.
If a PHY driver name includes e.g. a slash then sysfs is broken.
There should be a general helper e.g. in lib/string.c, because we have
this across subsystems wherever a device name is used in a sysfs path.