Re: [PATCH v4 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/27/20 8:22 PM, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta:
> 
>> [...]
>> +static bool dytc_ignore_next_event;
> 
> As a sidenote: can the profile switching be triggered by means that's not the
> `/sys/firmware/acpi/platform_profile` attribute (e.g. a physical button)?
> Because if so, then I'm not sure if `dytc_ignore_next_event` achieves its purpose
> robustly, although I believe it won't cause issues in practice. I think it could
> be made more robust using a mutex to serialize and synchronize access to the DYTC
> interface, but I'm not sure if the effort is worth it.
> 
> 
>> +static bool dytc_profile_available;
>> +static enum platform_profile_option dytc_current_profile;
>> [...]
>> +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>> +{
>> +	int err, output;
>> +
>> +	dytc_profile_available = false;
>> +	dytc_ignore_next_event = false;
>> +
>> +	err = dytc_command(DYTC_CMD_QUERY, &output);
>> +	/*
>> +	 * If support isn't available (ENODEV) then don't return an error
>> +	 * and don't create the sysfs group
>> +	 */
>> +	if (err == -ENODEV)
>> +		return 0;
>> +	/* For all other errors we can flag the failure */
>> +	if (err)
>> +		return err;
>> +
>> +	/* Check DYTC is enabled and supports mode setting */
>> +	if (output & BIT(DYTC_QUERY_ENABLE_BIT)) {
>> +		/* Only DYTC v5.0 and later has this feature. */
>> +		int dytc_version;
>> +
>> +		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
>> +		if (dytc_version >= 5) {
>> +			dbg_printk(TPACPI_DBG_INIT,
>> +				   "DYTC version %d: thermal mode available\n", dytc_version);
>> +			/* Create platform_profile structure and register */
>> +			do {
>> +				err = platform_profile_register(&dytc_profile);
>> +			} while (err == -EINTR);
>> [...]
> 
> I'm wondering if this loop is really necessary?

It is the result of using mutex_interruptible inside platform_profile_register(),
once that is fixed (as I just requested in my review of patch 2/3) then this loop
can go away.

Regards,

Hans