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
