Thank you for looking into this in detail, Ilpo! I tried to address
everything you mentioned in my v2 of this patch, but will comment on
each of your comments below so it will hopefully be more clear with
exactly what I changed, where, and the reasoning behind it.
Den mån 9 dec. 2024 kl 19:49 skrev Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx>:
>
> > +#define pr_debug_prefixed(...) pr_debug("[DEBUG] " __VA_ARGS__)
> > +
> > +#define print_acpi_object_buffer_debug(header_str, buf_ptr, buf_len) \
> > + do { \
> > + pr_debug_prefixed("%s\n", header_str); \
> > + print_hex_dump_debug("samsung_galaxybook: [DEBUG] ", \
>
> You can use pr_fmt() wrapping here so you don't need to hardcode the
> prefix.
>
After comments from Armin (which I agree with!) I have actually just
completely removed all of the pr_* functions including these macros in
the v2 of this patch, and for detailed buffer traces I have
implemented a new tracepoint system + event for samsung_galaxybook. I
assume now as the format of dev_dbg / warn / error looks correct
without defining pr_fmt(), it is also not needed ? (I have removed it
for now in v2 but can add it back if it is needed).
> > +static char *get_acpi_device_description(struct acpi_device *acpi_dev)
> > +{
> > + struct acpi_buffer str_buf = { ACPI_ALLOCATE_BUFFER, NULL };
> > + union acpi_object *str_obj;
> > + struct acpi_buffer name_buf = { ACPI_ALLOCATE_BUFFER, NULL };
>
> Reverse xmas tree order is preferred (unless there's a good reason to
> break it due to dependency).
>
Thanks for catching this; hopefully I have now fixed everything to use
reverse xmas tree ordering in the entire module. Please feel free to
let me know if I missed anything!
> > + if (ACPI_SUCCESS(status) && str_buf.length > 0) {
> > + str_obj = str_buf.pointer;
> > + char *buf = kzalloc(sizeof(*buf) * str_obj->buffer.length, GFP_KERNEL);
>
> Don't mix variable declarations with, always put them first in the
> block/function and leave blank line in between. There's only one exception
> and that is using cleanup.h when ordering of the declarations matter but
> that's the only exception (it's the reason why it's allowed at all).
>
This was originally done as a "workaround" to get a device description
from the "_STR" method in the same way that ACPI's device_sysfs.c did
it, ... but yes I agree it was not at all optimal and because I have
actually removed everything to do with ACPI fan devices in v2 of this
patch, all of this is gone now, too :)
> > + if (ACPI_FAILURE(status)) {
> > + pr_err("failed %s with ACPI method %s; got %s\n", purpose_str, method,
> > + acpi_format_exception(status));
>
> Use dev_err()
>
(+ all other references to using dev_* in your comments...)
Thank you for pointing this out; I have hopefully switched everything
to use dev_* functions for printing (and removed most of them,
actually) but please feel free to mention if I seem to have missed or
should adjust anything.
> > +
> > + memcpy(ret, out_obj->buffer.pointer, len);
>
> In kernel, ret is typically int so I suggest you pick some other name for
> the variable (Yes, I went back to find out what the type is in this case).
>
Good catch here, as well! I have renamed these variables a bit; now
this is called "out_buf" (and the incoming buffer is called "in_buf")
so hopefully it is super clear but please feel free to make any other
suggestion.
> > +static int start_on_lid_open_acpi_set(struct samsung_galaxybook *galaxybook, const bool value)
> > +{
> > + struct sawb buf = { 0 };
> > + int err;
> > +
> > + buf.safn = SAFN;
> > + buf.sasb = SASB_POWER_MANAGEMENT;
> > + buf.gunm = GUNM_POWER_MANAGEMENT;
> > + buf.guds[0] = GUDS_START_ON_LID_OPEN;
> > + buf.guds[1] = GUDS_START_ON_LID_OPEN_SET;
> > + buf.guds[2] = value;
>
> This relies on bool -> u8 implicit conversion. While it probably works as
> is, I'd prefer to make the conversion explicit with e.g. ?: operator (and
> let the compiler optimize it if it wants to).
>
Now in v2 I changed this to `buf.guds[2] = value ? 1 : 0` -- please
let me know if that is what you had in mind otherwise I can adjust
this to anything!
> > + pr_debug_prefixed("turned start_on_lid_open %s\n", value ? "on (1)" : "off (0)");
>
> Use a helper from linux/string_choices.h.
>
> I'd change it to:
> dev_dbg(..., "start_on_lid_open %s\n", str_enabled_disabled(value));
>
Really great recommendation, but even here as I removed all of this
stuff as well then there is no longer a need for string_choices.h I
think!
> > + pr_debug_prefixed("start_on_lid_open is currently %s\n",
> > + (buf.guds[1] ? "on (1)" : "off (0)"));
>
> I suspect a debug print like this is not going to be very useful. You get
> the result right out of the sysfs anyway so why bother printing at all and
> it's anyway not the raw value but synthetized on/off string.
>
Yes, agreed, and I removed all of these (but please feel free to say
if I seemed to have missed something!).
> > +static DEVICE_ATTR_RW(start_on_lid_open);
> > +
> > +/* USB Charge (USB ports can charge other devices even when device is powered off) */
> > +
> > +static int usb_charge_acpi_set(struct samsung_galaxybook *galaxybook, const bool value)
>
> Don't leave empty line in between.
>
Actually the comments like these were intended to be a sort of
separator for all of the related functions etc related to each
individual attribute (e.g. "usb_charge" as an attribute has 4
functions and a device_attribute defined, then the next set of
functions+attribute are for "allow_recording" etc). I left this empty
line so that the comment would not be associated only with the single
usb_charge_acpi_set function but instead could be considered a sort of
label for the entire grouping.
How should comments like this be formatted or does it just not even
make sense to have them at all and they should be removed ?
> > +static int charge_control_end_threshold_acpi_set(struct samsung_galaxybook *galaxybook,
> > + const u8 value)
>
> While certainly not forbidden, using const on plain integer types is not
> extremely useful. In fact, if it wouldn't be const, you could do the 100
> -> 0 mapping for it separately and not do it twice below.
>
> [...]
>
> Put comment on line before it so it's easier to read.
>
> "off" -> "no threshold" ?
>
> [...]
Good idea, now I have handled this in the v2 of the patch as follows:
if (value > 100)
return -EINVAL;
/* if setting to 100, should be set to 0 (no threshold) */
if (value == 100)
value = 0;
Does this make sense now or do you see anything that should be adjusted here?
>
> Do you want to differentiate 0 from 0? Should this function actually
> return -EINVAL if somebody attempts to set 0 threshold?
>
And regarding this, the device requires that you send 0 to represent
that the feature is "turned off", so to speak (no threshold is enabled
and the battery will charge all the way to 100%). So yes, in my mind,
we want to send 0 to the device if you are attempting to set either 0
or 100. Also I seem to recall that I tried to dig into how this is
handled in upower and the coming features in GNOME, and have a vague
memory that I saw somewhere in there that they were also converting
100 to a 0, but now I am having a bit of trouble finding this again.
Do you know if it would be better to have this driver provide an
interface where "100" means "no threshold" and that it should be
translated within the driver (that samsung_galaxybook sends a 0 to the
ACPI in case the user has requested "100" ?) or is it better if "0"
means "no threshold/charge to 100%" (or both?)?
I can also do some testing with the device to see if it accepts the
value 100 anyway, and how it behaves, though I would be a little
concerned with this longer term as it is not how the driver and
settings applications work in Windows (they are hard-coded with a
toggle and it always sets either 0 (off) or 80 (on)), and I could see
where even if it works today, sending the value of 100 to mean "off"
could be altered by potential BIOS updates?
> > +out_free:
> > + ACPI_FREE(response.pointer);
> > + return ret;
>
> You're mixing err and ret within a file as the error/return code variable.
> It would be nice to be consistent within a file.
>
Here I have tried to look over and make sure to use "err" for
everything but please say if it seems I missed something!
> > + response_obj = response.pointer;
> > + if (!response_obj || response_obj->type != ACPI_TYPE_INTEGER ||
> > + response_obj->integer.value > INT_MAX ||
>
> I don't know what's the logic behind doing bound check here but not in the
> previous function.
>
Removed all fan stuff from the driver in v2 so all of this is gone now anyway :)
> But why you need this whole dev_ext_attribute thing in the first place?
>
Though the fan stuff is removed, I have done this now with the battery
charge control attribute. The reason in this new case is to be able to
give a pointer to the "samsung_galaxybook" struct so that the handle
can be grabbed from this private data and avoid using a global pointer
variable.
> > +static enum platform_profile_option
> > +profile_performance_mode(struct samsung_galaxybook *galaxybook, const u8 performance_mode)
> > +{
> > + for (int i = 0; i < PLATFORM_PROFILE_LAST; i++)
> > + if (galaxybook->profile_performance_modes[i] == performance_mode)
> > + return i;
> > + return -1;
>
> Returning value that is not part of enum looks a bit hacky.
>
Good catch, this was a total oversight. Now in v2 of this patch I have
changed this to a more typical int return code function (returning
-ENODATA) and the value is assigned to a pointer that the caller
passes in. Please let me know if anything looks fishy with this or it
seems ok now!
> > +/* copied from platform_profile.c; better if this could be fetched from a public function, maybe? */
>
> You are allowed to propose patches in the patch series for things you
> need. :-)
>
> > +static const char *const profile_names[] = {
> > + [PLATFORM_PROFILE_LOW_POWER] = "low-power",
> > + [PLATFORM_PROFILE_COOL] = "cool",
> > + [PLATFORM_PROFILE_QUIET] = "quiet",
> > + [PLATFORM_PROFILE_BALANCED] = "balanced",
> > + [PLATFORM_PROFILE_BALANCED_PERFORMANCE] = "balanced-performance",
> > + [PLATFORM_PROFILE_PERFORMANCE] = "performance",
> > +};
> > +static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>
> Is this assert compatible with the custom platform profile series that is
> practically ready to be merged?
>
All of this was just for printing the strings out in the debug
information, and really not actually necessary. So I have removed it
all, now.
> > + galaxybook->profile_performance_modes =
> > + kzalloc(sizeof(u8) * PLATFORM_PROFILE_LAST, GFP_KERNEL);
>
> kcalloc() ?
>
Great catch, this has been changed.
> > + /* if no performance modes were mapped (err is still -ENODEV) then stop and fail here */
> > + if (err)
> > + return err;
>
> It would be much more obvious to count number of mapped modes with a
> variable and not play with err variable like this. You needed lots of
> comment to explain which all could be dropped and this could just return
> -ENODEV directly.
>
Agreed and now I have added a new int "mapped_profiles" and tweaked
this logic a bit so hopefully it is slightly more self-explanatory.
Please say if it seems like I am still missing the mark on this one!
> I'll stop here as I'm out of time.
>
> --
> i.
>
Thank you again for all of your help and feedback! Please feel free to
look at the rest of the changes I added with the v2 of the patch and
send any feedback that you would like.
Best regards,
Joshua
> [...]
