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]
Date:   Sun, 23 Jul 2017 13:01:19 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>,
        Jean Delvare <jdelvare@...e.com>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (it87) Reapply probe path chip registers settings
 after resume

On 07/23/2017 09:12 AM, Maciej S. Szmigiero wrote:
> After a suspend / resume cycle we possibly need to reapply chip registers
> settings that we had set or fixed in a probe path, since they might have
> been reset to default values or set incorrectly by a BIOS again.
> 
> Tested on a Gigabyte M720-US3 board, which requires routing internal VCCH5V
> to in7 (and had it wrong again on resume from S3).
> 
> Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>

I understand the problem, but this looks quite invasive. Can this be solved
without changing the driver all over the place ?

Guenter

> ---
>   drivers/hwmon/it87.c | 148 ++++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 112 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 4dfc7238313e..e33af2e5f4ce 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -497,6 +497,7 @@ static const struct it87_devices it87_devices[] = {
>   #define has_vin3_5v(data)	((data)->features & FEAT_VIN3_5V)
>   
>   struct it87_sio_data {
> +	int sioaddr;
>   	enum chips type;
>   	/* Values read from Super-I/O config space */
>   	u8 revision;
> @@ -517,6 +518,7 @@ struct it87_sio_data {
>    */
>   struct it87_data {
>   	const struct attribute_group *groups[7];
> +	int sioaddr;
>   	enum chips type;
>   	u32 features;
>   	u8 peci_mask;
> @@ -2389,10 +2391,11 @@ static const struct attribute_group it87_group_auto_pwm = {
>   };
>   
>   /* SuperIO detection - will change isa_address if a chip is found */
> -static int __init it87_find(int sioaddr, unsigned short *address,
> +static int __init it87_find(unsigned short *address,
>   			    struct it87_sio_data *sio_data)
>   {
>   	int err;
> +	int sioaddr = sio_data->sioaddr;
>   	u16 chip_type;
>   	const char *board_vendor, *board_name;
>   	const struct it87_devices *config;
> @@ -2828,13 +2831,13 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>   	return err;
>   }
>   
> -/* Called when we have found a new IT87. */
> -static void it87_init_device(struct platform_device *pdev)
> +/* Called when we have found a new IT87 or on resume. */
> +static void it87_init_device(struct platform_device *pdev, bool resume)
>   {
>   	struct it87_sio_data *sio_data = dev_get_platdata(&pdev->dev);
>   	struct it87_data *data = platform_get_drvdata(pdev);
>   	int tmp, i;
> -	u8 mask;
> +	u8 mask, fan_main_ctrl;
>   
>   	/*
>   	 * For each PWM channel:
> @@ -2849,7 +2852,7 @@ static void it87_init_device(struct platform_device *pdev)
>   	 * these have separate registers for the temperature mapping and the
>   	 * manual duty cycle.
>   	 */
> -	for (i = 0; i < NUM_AUTO_PWM; i++) {
> +	for (i = 0; !resume && i < NUM_AUTO_PWM; i++) {
>   		data->pwm_temp_map[i] = i;
>   		data->pwm_duty[i] = 0x7f;	/* Full speed */
>   		data->auto_pwm[i][3] = 0x7f;	/* Full speed, hard-coded */
> @@ -2889,14 +2892,18 @@ static void it87_init_device(struct platform_device *pdev)
>   
>   	/* Check if tachometers are reset manually or by some reason */
>   	mask = 0x70 & ~(sio_data->skip_fan << 4);
> -	data->fan_main_ctrl = it87_read_value(data, IT87_REG_FAN_MAIN_CTRL);
> -	if ((data->fan_main_ctrl & mask) == 0) {
> +	fan_main_ctrl = it87_read_value(data, IT87_REG_FAN_MAIN_CTRL);
> +	if ((fan_main_ctrl & mask) == 0) {
>   		/* Enable all fan tachometers */
> -		data->fan_main_ctrl |= mask;
> +		fan_main_ctrl |= mask;
>   		it87_write_value(data, IT87_REG_FAN_MAIN_CTRL,
> -				 data->fan_main_ctrl);
> +				 fan_main_ctrl);
> +	}
> +
> +	if (!resume) {
> +		data->fan_main_ctrl = fan_main_ctrl;
> +		data->has_fan = (fan_main_ctrl >> 4) & 0x07;
>   	}
> -	data->has_fan = (data->fan_main_ctrl >> 4) & 0x07;
>   
>   	tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
>   
> @@ -2910,27 +2917,29 @@ static void it87_init_device(struct platform_device *pdev)
>   		}
>   	}
>   
> -	/* Check for additional fans */
> -	if (has_five_fans(data)) {
> -		if (tmp & BIT(4))
> -			data->has_fan |= BIT(3); /* fan4 enabled */
> -		if (tmp & BIT(5))
> -			data->has_fan |= BIT(4); /* fan5 enabled */
> -		if (has_six_fans(data) && (tmp & BIT(2)))
> -			data->has_fan |= BIT(5); /* fan6 enabled */
> -	}
> -
> -	/* Fan input pins may be used for alternative functions */
> -	data->has_fan &= ~sio_data->skip_fan;
> +	if (!resume) {
> +		/* Check for additional fans */
> +		if (has_five_fans(data)) {
> +			if (tmp & BIT(4))
> +				data->has_fan |= BIT(3); /* fan4 enabled */
> +			if (tmp & BIT(5))
> +				data->has_fan |= BIT(4); /* fan5 enabled */
> +			if (has_six_fans(data) && (tmp & BIT(2)))
> +				data->has_fan |= BIT(5); /* fan6 enabled */
> +		}
>   
> -	/* Check if pwm5, pwm6 are enabled */
> -	if (has_six_pwm(data)) {
> -		/* The following code may be IT8620E specific */
> -		tmp = it87_read_value(data, IT87_REG_FAN_DIV);
> -		if ((tmp & 0xc0) == 0xc0)
> -			sio_data->skip_pwm |= BIT(4);
> -		if (!(tmp & BIT(3)))
> -			sio_data->skip_pwm |= BIT(5);
> +		/* Fan input pins may be used for alternative functions */
> +		data->has_fan &= ~sio_data->skip_fan;
> +
> +		/* Check if pwm5, pwm6 are enabled */
> +		if (has_six_pwm(data)) {
> +			/* The following code may be IT8620E specific */
> +			tmp = it87_read_value(data, IT87_REG_FAN_DIV);
> +			if ((tmp & 0xc0) == 0xc0)
> +				sio_data->skip_pwm |= BIT(4);
> +			if (!(tmp & BIT(3)))
> +				sio_data->skip_pwm |= BIT(5);
> +		}
>   	}
>   
>   	/* Start monitoring */
> @@ -2940,7 +2949,7 @@ static void it87_init_device(struct platform_device *pdev)
>   }
>   
>   /* Return 1 if and only if the PWM interface is safe to use */
> -static int it87_check_pwm(struct device *dev)
> +static int it87_check_pwm(struct device *dev, bool resume)
>   {
>   	struct it87_data *data = dev_get_drvdata(dev);
>   	/*
> @@ -2986,8 +2995,10 @@ static int it87_check_pwm(struct device *dev)
>   				 "PWM configuration is too broken to be fixed\n");
>   		}
>   
> -		dev_info(dev,
> -			 "Detected broken BIOS defaults, disabling PWM interface\n");
> +		if (!resume)
> +			dev_info(dev,
> +				 "Detected broken BIOS defaults, disabling PWM interface\n");
> +
>   		return 0;
>   	} else if (fix_pwm_polarity) {
>   		dev_info(dev,
> @@ -3020,6 +3031,7 @@ static int it87_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	data->addr = res->start;
> +	data->sioaddr = sio_data->sioaddr;
>   	data->type = sio_data->type;
>   	data->features = it87_devices[sio_data->type].features;
>   	data->peci_mask = it87_devices[sio_data->type].peci_mask;
> @@ -3057,7 +3069,7 @@ static int it87_probe(struct platform_device *pdev)
>   	mutex_init(&data->update_lock);
>   
>   	/* Check PWM configuration */
> -	enable_pwm_interface = it87_check_pwm(dev);
> +	enable_pwm_interface = it87_check_pwm(dev, false);
>   
>   	/* Starting with IT8721F, we handle scaling of internal voltages */
>   	if (has_12mv_adc(data)) {
> @@ -3110,7 +3122,7 @@ static int it87_probe(struct platform_device *pdev)
>   	data->has_beep = !!sio_data->beep_pin;
>   
>   	/* Initialize the IT87 chip */
> -	it87_init_device(pdev);
> +	it87_init_device(pdev, false);
>   
>   	if (!sio_data->skip_vid) {
>   		data->has_vid = true;
> @@ -3140,9 +3152,72 @@ static int it87_probe(struct platform_device *pdev)
>   	return PTR_ERR_OR_ZERO(hwmon_dev);
>   }
>   
> +static int it87_resume_sio(struct platform_device *pdev)
> +{
> +	struct it87_data *data = dev_get_drvdata(&pdev->dev);
> +	int err;
> +	int reg2c;
> +
> +	if (!(data->in_internal & BIT(1)))
> +		return 0;
> +
> +	if (has_in7_internal(data))
> +		return 0;
> +
> +	err = superio_enter(data->sioaddr);
> +	if (err)
> +		return err;
> +
> +	superio_select(data->sioaddr, GPIO);
> +
> +	reg2c = superio_inb(data->sioaddr, IT87_SIO_PINX2_REG);
> +	if (!(reg2c & BIT(1))) {
> +		dev_notice(&pdev->dev,
> +			   "Routing internal VCCH5V to in7 again");
> +
> +		reg2c |= BIT(1);
> +		superio_outb(data->sioaddr, IT87_SIO_PINX2_REG,
> +			     reg2c);
> +	}
> +
> +	superio_exit(data->sioaddr);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused it87_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct it87_data *data = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = it87_resume_sio(pdev);
> +	if (err)
> +		dev_err(dev,
> +			"Unable to resume Super I/O (%d)",
> +			err);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	it87_check_pwm(dev, true);
> +	it87_init_device(pdev, true);
> +
> +	/* force update */
> +	data->valid = 0;
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	it87_update_device(dev);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(it87_dev_pm_ops, NULL, it87_resume);
> +
>   static struct platform_driver it87_driver = {
>   	.driver = {
>   		.name	= DRVNAME,
> +		.pm     = &it87_dev_pm_ops,
>   	},
>   	.probe	= it87_probe,
>   };
> @@ -3208,8 +3283,9 @@ static int __init sm_it87_init(void)
>   
>   	for (i = 0; i < ARRAY_SIZE(sioaddr); i++) {
>   		memset(&sio_data, 0, sizeof(struct it87_sio_data));
> +		sio_data.sioaddr = sioaddr[i];
>   		isa_address[i] = 0;
> -		err = it87_find(sioaddr[i], &isa_address[i], &sio_data);
> +		err = it87_find(&isa_address[i], &sio_data);
>   		if (err || isa_address[i] == 0)
>   			continue;
>   		/*
> 

Powered by blists - more mailing lists