On Thu, Feb 3, 2022 at 10:54 PM srinivas pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> 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.
I see.
Then I would convert struct acpi_fan_fif to u32 fields and extract it
directly (and sanitize step_size in the process), because using
acpi_extract_package() to retrieve it is not worth the resulting fuss
IMO.
> >
> > > + 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.
OK, so the code below can do
value *= fan->fif.step_size;
if (value + fan->fif.step_size > 100)
value = 100;
and max_size is still not needed IIUC.
>
> >
> > > 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.
> 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
>
