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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5e9c1d9f-4c9f-10fd-439e-26f7f2cc467e@gmail.com>
Date:   Sun, 24 Feb 2019 18:17:49 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...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

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?

[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?

> +
> +	*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?

> +
> +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.
-- 
Florian