Re: [PATCH v3] ACPI / fan: Properly handle fine grain control

[srinivas pandruvada]

On Thu, 2022-02-03 at 13:54 -0800, srinivas pandruvada wrote:
> On Thu, 2022-02-03 at 21:13 +0100, Rafael J. Wysocki wrote:
> > On Sat, Jan 29, 2022 at 12:51 AM Srinivas Pandruvada
> > <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> > > 
> > > 
> 
> [...]
> 
> > > To support fine grain control (when supported) via thermal sysfs:
> > > - cooling device max state is not _FPS state count but it will be
> > > 100 / _FIF.step_size
> > > - cooling device current state is 100 / _FIF.step_size
> > 
> > I don't quite understand this.
> > 
> > The max state and the current state should not always be the same,
> > should they?
> 
> This sentence needs correction.
> The  current_state is _FST.control/_FIF.step_size.
> 
> > 
> > > - cooling device set state will set the control value
> > > 
> 
> [...]
> 
> > > -       else
> > > +       if (fan->acpi4) {
> > > +               if (fan->fif.fine_grain_ctrl)
> > > +                       *state = 100 / (int)fan->fif.step_size;
> > 
> > Is it really necessary to explicitly cast fif.step_size to int?
> This was reported by LKP as fan->fif.step_size is 64 bit. This driver
> doesn't restrict to 64 bit.
> 
> "undefined reference to `__udivdi3" for i386-defconfig.
> > 
> > > +               else
> > > +                       *state = fan->fps_count - 1;
> > > 
> 
> [...]
> 
> > > -static int fan_get_state_acpi4(struct acpi_device *device,
> > > unsigned long *state)
> > > +static int fan_get_fps(struct acpi_device *device, struct
> > > acpi_fan_fst *fst)
> > 
> > Why is this called fan_get_fps()?  I'd rather call it
> > acpi_fan_get_fst().
> I can change that.
> 
> > 
> > >  {
> > >         struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL
> > > };
> > > -       struct acpi_fan *fan = acpi_driver_data(device);
> > >         union acpi_object *obj;
> > >         acpi_status status;
> > > -       int control, i;
> > > 
> > >         status = acpi_evaluate_object(device->handle, "_FST",
> > > NULL,
> > > &buffer);
> > >         if (ACPI_FAILURE(status)) {
> > > @@ -119,31 +130,51 @@ static int fan_get_state_acpi4(struct
> > > acpi_device *device, unsigned long *state)
> > >                 goto err;
> > >         }
> > > 
> > > -       control = obj->package.elements[1].integer.value;
> > > +       fst->revision = obj->package.elements[0].integer.value;
> > > +       fst->control = obj->package.elements[1].integer.value;
> > > +       fst->speed = obj->package.elements[2].integer.value;
> > > +
> > > +       status = 0;
> > > +err:
> > > +       kfree(obj);
> > > +       return status;
> > 
> > There is some confusion regarding the error return values in this
> > function, would be good to fix it while doing this.
> > 
> Let me check that.
> 
> > > +}
> > > +
> > > +static int fan_get_state_acpi4(struct acpi_device *device,
> > > unsigned long *state)
> > > +{
> > > +       struct acpi_fan *fan = acpi_driver_data(device);
> > > +       struct acpi_fan_fst fst;
> > > +       int status;
> > > +       int control, i;
> > > +
> > > +       status = fan_get_fps(device, &fst);
> > > +       if (status)
> > > +               return status;
> > > +
> > > +       control = fst.control;
> > > +
> > > +       if (fan->fif.fine_grain_ctrl) {
> > > +               /* This control should be same what we set using
> > > _FSL by spec */
> > > +               if (control > 100) {
> > > +                       dev_dbg(&device->dev, "Invalid control
> > > value returned\n");
> > > +                       return -EINVAL;
> > 
> > Why don't we fall back to the other method in this case?
> We can.
> 
> > 
> > > +               }
> > > +
> > > +               *state = control / (int)fan->fif.step_size;
> > 
> > Do we care about rounding errors?
> > 
> > Say control is 8 and step_size is 9.  Should this count as 0 or as
> > 1?
> > 
> We will not set control value to 8 in this case, so we shouldn't read
> 8. But if firmware setup at boot then it will be 0. The compensation
> to
> reach 100 is at last step which is allowed.
> 
> If step size is 9
> 
> thermal sysfs will display
>         max_state = 100/9 = 11
> 
> control         thermal sysfs cur_state
> 0-8             0
> 9-17            1
> 18-26           2
> 27-35           3
> 36-44           4
> 45-53           5
> 54-62           6
> 63-71           7
> 72-80           8
> 81-89           9
> 90-98           10
> 99-100          11
> 
> If step size is 10
> thermal sysfs
>         max_state = 100/10 = 10
> control         thermal sysfs cur_state
> 0-9             0
> 10-19           1
> 20-29           2
> 30-39           3
> 40-49           4
> 50-59           5
> 60-69           6
> 70-79           7
> 80-89           8
> 90-99           9
> 100             10
> 
> 
> 
> > > +               return 0;
> > > +       }
> 
> [...]
> 
> > > -       if (state >= fan->fps_count)
> > > +       if (state > max_state)
> > 
> > Say step_size is 9, so max_state is 11.  state == 12 would still be
> > valid, no?
> We are presenting thermal sysfs max_state as 11 in this case, so
> state 0-11 are valid. To reach 100, the spec allows to compensate the
> last step to reach 100%. So state 11 is 100% not 99% as in the above
> table.
> 
> If we present max_state as 12 then also user space can't choose max
> than 12, so (state > max_state) will be still an error.
> 
> If we present max state as 12 then in the above table:
> control         state
> ----------------------
> 90              10
> 99              11
> 100             12
> 
> Then last state will increase control value by 1.
> 
> 
> > 
> > >                 return -EINVAL;
> > > 
> > > -       status = acpi_execute_simple_method(device->handle,
> > > "_FSL",
> > > -                                           fan-
> > > > fps[state].control);
> > > +       if (fan->fif.fine_grain_ctrl) {
> > > +               int rem;
> > > +
> > > +               value *= fan->fif.step_size;
> > 
> > And you don't need the max_state computation for this, it's only
> > necessary to cap value at 100.  In which case you also wouldn't
> > need
> > rem etc.
Correct.
We don't need calculation. When the value == max_step, the control
value will be 100.


> For above example to set the state 11 for 100 for step size 9. If
> max_state is chosen as 12 then we can just cap.
> 
> > 
> > > +
> > > +               /*
> > > +                * In the event OSPM’s incremental selections of
> > > Level
> > > +                * using the StepSize field value do not sum to
> > > 100%,
> > > +                * OSPM may select an appropriate ending Level
> > > +                * increment to reach 100%.
> > > +                */
> > > +               rem = 100 - value;
> > > +               if (rem && rem < fan->fif.step_size)
> > > +                       value = 100;
> > > +       } else {
> > > +               value = fan->fps[state].control;
> > > 
> 
> [...]
> 
> > > +       if (!fan->fif.step_size)
> > > +               fan->fif.step_size = 1;
> > > +       /* If step size > 9, change to 9 (by spec valid values 1-
> > > 9)
> > > */
> > > +       if (fan->fif.step_size > 9)
> > 
> > I would do "else if" here, because both the above conditions cannot
> > hold at the same time.
> OK
> 
> > 
> > > +               fan->fif.step_size = 9;
> > >  err:
> > > 
> 
> [...]
> 
> > > +       sysfs_attr_init(&fan->fine_grain_control.attr);
> > > +       fan->fine_grain_control.show = show_fine_grain_control;
> > > +       fan->fine_grain_control.store = NULL;
> > > +       fan->fine_grain_control.attr.name = "fine_grain_control";
> > > +       fan->fine_grain_control.attr.mode = 0444;
> > > +       status = sysfs_create_file(&device->dev.kobj, &fan-
> > > > fine_grain_control.attr);
> > 
> > I would split the creation of the new attributes into a separate
> > file,
> > for clarity (and to help the review somewhat).
> > 
> We can move all the attributes including the current one to a new
> file.
> 
> Thanks,
> Srinivas
>