linux-kernel - Re: [PATCH v2] hwmon: (it87) Reapply probe path chip registers settings after resume

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]

Date:   Wed, 26 Jul 2017 21:26:15 +0200
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Guenter Roeck <linux@...ck-us.net>,
        Jean Delvare <jdelvare@...e.com>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: (it87) Reapply probe path chip registers
 settings after resume

On 26.07.2017 04:17, Guenter Roeck wrote:
> On 07/24/2017 12:37 PM, 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>
>> ---
>> Changes from v1: Move code of common probe / resume steps to new functions
>> so we don't need to make large parts of probe function conditional on a
>> newly added 'resume' parameter.
>>
> 
> Much better. Please split cleanup and pm functionality addition into two patches
> to simplify review.

Will do.

>> @@ -2986,8 +3027,6 @@ 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");
> 
> I don't think this is a good idea. Besides, it only removes one message.

That one message was moved to probe path only since otherwise it could be
misleading.

Specifically, if we hit this registry values on resume where we should
disable PWM interface (but where this interface was previously enabled)
we would print that we are "disabling PWM interface" but we won't actually
do it (since doing this would at least involve a removal of some already
registered hwmon groups which seems not possible with the current API).

> I would suggest to check for enable_pwm_interface in the resume function.
> If it is not set, don't bother calling it87_check_pwm() again.

Will do.

>> @@ -3140,9 +3183,77 @@ 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");
>> +
> I don't think this message provides any real value.

It helps to know that the workaround was done on resume (just like the
original message did on probe path), but I can change it to dev_dbg() to
not make the driver more noisy unnecessarily.

> Besides, it isn't always VCCH5V.

In datasheets of all three chips in which this routing override is set
(it8720, it8782, it8783) it is explicitly stated that this bit controls
an "internal voltage divider for VCCH5V".
Also all these three chips have VCCH of 5 volts.

I can change the message on probe path to print the voltage name as
VCCH5V in all 3 chips in the cleanup patch so there is no longer
an inconsistency here.
  
>> +        reg2c |= BIT(1);
>> +        superio_outb(data->sioaddr, IT87_SIO_PINX2_REG,
>> +                 reg2c);
>> +    }
>> +
> 
> Problem with this code is that it applies to _all_ chips.
> The original code only applied to it8783 (this message), and
> only if bit 2 of register 0x27 was set, or to it8720 and it8782
> under certain conditions (though there the message is about VCCH).
> 
> I understand you try to cover that condition with the checks above,
> but there is no guarantee that this works for all chips (and that
> it will continue to work going forward as new chips are added).
> 
> Please consider adding a flag such as "need_in7_reroute" instead.
> That would simplify the checks here and make it more explicit.

Will do.

Maciej