Re: [bug report] platform/x86: thinkpad_acpi: Add Thinkpad Edge E531 fan support

[Matthias Fetzer <kontakt@xxxxxxxxxxxxxxxxxx>]

Hi Dan and Hans,

Am 27.08.24 um 11:09 schrieb Hans de Goede:
> Hi Dan,
>
> On 8/27/24 10:45 AM, Dan Carpenter wrote:
> > Hello Matthias Fetzer,
> >
> > Commit 57d0557dfa49 ("platform/x86: thinkpad_acpi: Add Thinkpad Edge
> > E531 fan support") from Aug 16, 2024 (linux-next), leads to the
> > following Smatch static checker warning:
> >
> > 	drivers/platform/x86/thinkpad_acpi.c:8387 fan_set_enable()
> > 	error: uninitialized symbol 's'.
> >
> > drivers/platform/x86/thinkpad_acpi.c
> >      8319 static int fan_set_enable(void)
> >      8320 {
> >      8321         u8 s;
> >      8322         int rc;
> >      8323
> >      8324         if (!fan_control_allowed)
> >      8325                 return -EPERM;
> >      8326
> >      8327         if (mutex_lock_killable(&fan_mutex))
> >      8328                 return -ERESTARTSYS;
> >      8329
> >      8330         switch (fan_control_access_mode) {
> >      8331         case TPACPI_FAN_WR_ACPI_FANS:
> >      8332         case TPACPI_FAN_WR_TPEC:
> >      8333                 rc = fan_get_status(&s);
> >      8334                 if (rc)
> >      8335                         break;
> >      8336
> >      8337                 /* Don't go out of emergency fan mode */
> >      8338                 if (s != 7) {
> >      8339                         s &= 0x07;
> >      8340                         s |= TP_EC_FAN_AUTO | 4; /* min fan speed 4 */
> >      8341                 }
> >      8342
> >      8343                 if (!acpi_ec_write(fan_status_offset, s))
> >      8344                         rc = -EIO;
> >      8345                 else {
> >      8346                         tp_features.fan_ctrl_status_undef = 0;
> >      8347                         rc = 0;
> >      8348                 }
> >      8349                 break;
> >      8350
> >      8351         case TPACPI_FAN_WR_ACPI_SFAN:
> >      8352                 rc = fan_get_status(&s);
> >      8353                 if (rc)
> >      8354                         break;
> >      8355
> >      8356                 s &= 0x07;
> >      8357
> >      8358                 /* Set fan to at least level 4 */
> >      8359                 s |= 4;
> >      8360
> >      8361                 if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", s))
> >      8362                         rc = -EIO;
> >      8363                 else
> >      8364                         rc = 0;
> >      8365                 break;
> >      8366
> >      8367         case TPACPI_FAN_WR_ACPI_FANW:
> >      8368                 if (!acpi_evalf(fanw_handle, NULL, NULL, "vdd", 0x8106, 0x05)) {
> >      8369                         rc = -EIO;
> >      8370                         break;
> >      8371                 }
> >      8372                 if (!acpi_evalf(fanw_handle, NULL, NULL, "vdd", 0x8100, 0x00)) {
> >      8373                         rc = -EIO;
> >      8374                         break;
> >      8375                 }
> >      8376
> >      8377                 rc = 0;
> >
> > s isn't set on this path
> >
> >      8378                 break;
> >      8379
> >      8380         default:
> >      8381                 rc = -ENXIO;
> >      8382         }
> >      8383
> >      8384         mutex_unlock(&fan_mutex);
> >      8385
> >      8386         if (!rc)
> > --> 8387                 vdbg_printk(TPACPI_DBG_FAN,
> >      8388                         "fan control: set fan control register to 0x%02x\n",
> >      8389                         s);
> >                                   ^
> > Here
> >
> >      8390         return rc;
> >      8391 }
>
> Thank you for reporting this.
>
> Since this is just a debug print I think it is best to fix this by just
> initializing s to 0 when it is declared and then just log 0
> in the TPACPI_FAN_WR_ACPI_FANW case.
>
> Anyone got any better suggestions ?
>
> Regards,
>
> Hans

Since there is no single value and the registers are always set to
the same static values this would be the right solution in my opinion.

Best regards,
Matthias