Re: [PATCH v7] platform/x86: samsung-galaxybook: Add samsung-galaxybook driver

["Kurt Borja" <kuurtb@gmail.com>]

Hi Joshua,

I have some comments on the platform profile section. The most important
one is the platform_profile probe one, the rest are suggetions.

On Fri Jan 17, 2025 at 7:43 PM -05, Joshua Grisham wrote:
> Add a new driver for Samsung Galaxy Book series notebook devices with the
> following features:
>
> - Keyboard backlight control
> - Battery extension with charge control end threshold
> - Controller for Samsung's performance modes using the platform profile
>   interface
> - Adds firmware-attributes to control various system features
> - Handles various hotkeys and notifications
>
> Signed-off-by: Joshua Grisham <josh@joshuagrisham.com>

...

> +/*
> + * Platform Profile / Performance mode
> + */
> +
> +static int performance_mode_acpi_set(struct samsung_galaxybook *galaxybook,
> +				     const u8 performance_mode)
> +{
> +	struct sawb buf = {};
> +
> +	buf.safn = GB_SAFN;
> +	buf.sasb = GB_SASB_PERFORMANCE_MODE;
> +	export_guid(buf.caid, &GB_PERFORMANCE_MODE_GUID);
> +	buf.fncn = GB_FNCN_PERFORMANCE_MODE;
> +	buf.subn = GB_SUBN_PERFORMANCE_MODE_SET;
> +	buf.iob0 = performance_mode;
> +
> +	return galaxybook_acpi_method(galaxybook, GB_ACPI_METHOD_PERFORMANCE_MODE,
> +				      &buf, GB_SAWB_LEN_PERFORMANCE_MODE);
> +}
> +
> +static int performance_mode_acpi_get(struct samsung_galaxybook *galaxybook, u8 *performance_mode)
> +{
> +	struct sawb buf = {};
> +	int err;
> +
> +	buf.safn = GB_SAFN;
> +	buf.sasb = GB_SASB_PERFORMANCE_MODE;
> +	export_guid(buf.caid, &GB_PERFORMANCE_MODE_GUID);
> +	buf.fncn = GB_FNCN_PERFORMANCE_MODE;
> +	buf.subn = GB_SUBN_PERFORMANCE_MODE_GET;
> +
> +	err = galaxybook_acpi_method(galaxybook, GB_ACPI_METHOD_PERFORMANCE_MODE,
> +				     &buf, GB_SAWB_LEN_PERFORMANCE_MODE);
> +	if (err)
> +		return err;
> +
> +	*performance_mode = buf.iob0;
> +
> +	return 0;
> +}
> +
> +static int get_performance_mode_profile(struct samsung_galaxybook *galaxybook,
> +					const u8 performance_mode,
> +					enum platform_profile_option *profile)
> +{
> +	switch (performance_mode) {
> +	case GB_PERFORMANCE_MODE_SILENT:
> +		*profile = PLATFORM_PROFILE_LOW_POWER;
> +		break;
> +	case GB_PERFORMANCE_MODE_QUIET:
> +		*profile = PLATFORM_PROFILE_QUIET;
> +		break;
> +	case GB_PERFORMANCE_MODE_OPTIMIZED:
> +	case GB_PERFORMANCE_MODE_OPTIMIZED_LEGACY:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case GB_PERFORMANCE_MODE_PERFORMANCE:
> +	case GB_PERFORMANCE_MODE_PERFORMANCE_LEGACY:
> +		/* balanced-performance maps to Performance when Ultra exists */
> +		if (test_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
> +			     galaxybook->platform_profile_choices))
> +			*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
> +		else
> +			*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	case GB_PERFORMANCE_MODE_ULTRA:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	case GB_PERFORMANCE_MODE_IGNORE1:
> +	case GB_PERFORMANCE_MODE_IGNORE2:
> +		return -EOPNOTSUPP;
> +	default:
> +		dev_warn(&galaxybook->platform->dev,
> +			 "unrecognized performance mode 0x%x\n", performance_mode);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int galaxybook_platform_profile_set(struct device *dev,
> +					   enum platform_profile_option profile)
> +{
> +	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
> +
> +	return performance_mode_acpi_set(galaxybook,
> +					 galaxybook->profile_performance_modes[profile]);
> +}
> +
> +static int galaxybook_platform_profile_get(struct device *dev,
> +					   enum platform_profile_option *profile)
> +{
> +	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
> +	u8 performance_mode;
> +	int err;
> +
> +	err = performance_mode_acpi_get(galaxybook, &performance_mode);
> +	if (err)
> +		return err;
> +
> +	return get_performance_mode_profile(galaxybook, performance_mode, profile);
> +}
> +
> +static int galaxybook_platform_profile_probe(void *drvdata, unsigned long *choices)
> +{
> +	struct samsung_galaxybook *galaxybook = drvdata;
> +	int i;
> +
> +	for_each_set_bit(i, galaxybook->platform_profile_choices, PLATFORM_PROFILE_LAST)
> +		set_bit(i, choices);

The intended use of this callback is to "probe" for available choices
here. You should move all galaxybook_performance_mode_profile_init()
logic to this method. This would eliminate the need to have a copy of
the choices bitmap.

> +
> +	return 0;
> +}
> +
> +static const struct platform_profile_ops galaxybook_platform_profile_ops = {
> +	.probe = galaxybook_platform_profile_probe,
> +	.profile_get = galaxybook_platform_profile_get,
> +	.profile_set = galaxybook_platform_profile_set,
> +};
> +
> +static int galaxybook_performance_mode_init(struct samsung_galaxybook *galaxybook)
> +{
> +	enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
> +	u8 performance_mode;
> +	int err;
> +	int i;
> +
> +	err = performance_mode_acpi_get(galaxybook, &performance_mode);
> +	if (err)
> +		return err;
> +
> +	err = get_performance_mode_profile(galaxybook, performance_mode, &profile);
> +	if (err)

If this method failed we can't safely continue. I think you should
return here, else you may get an out of bounds access bellow.

> +		dev_warn(&galaxybook->platform->dev,
> +			 "initial startup performance mode 0x%x is not mapped\n",
> +			 performance_mode);
> +
> +	for_each_set_bit(i, galaxybook->platform_profile_choices, PLATFORM_PROFILE_LAST)
> +		dev_dbg(&galaxybook->platform->dev,
> +			"enabled platform profile %d using performance mode 0x%x\n",
> +			i, galaxybook->profile_performance_modes[i]);

Maybe we can log this directly in the switch-case block inside
galaxybook_performance_mode_profile_init() instead of having to iterate.

> +
> +	/* ensure startup performance_mode matches that mapped to its profile */
> +	if (galaxybook->profile_performance_modes[profile] == performance_mode)
> +		return 0;
> +
> +	/* otherwise, if balanced is enabled, use it as the default */
> +	if (test_bit(PLATFORM_PROFILE_BALANCED, galaxybook->platform_profile_choices))
> +		return performance_mode_acpi_set(galaxybook,
> +			galaxybook->profile_performance_modes[PLATFORM_PROFILE_BALANCED]);
> +
> +	/* otherwise, find the first enabled profile and use that instead */
> +	profile = find_next_bit_wrap(galaxybook->platform_profile_choices,
> +				     PLATFORM_PROFILE_LAST,
> +				     0);
> +
> +	if (profile == PLATFORM_PROFILE_LAST) {
> +		dev_err(&galaxybook->platform->dev,
> +			"no platform profiles have been enabled\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return performance_mode_acpi_set(galaxybook,
> +					 galaxybook->profile_performance_modes[profile]);
> +}
> +
> +#define gb_pfmode(profile) galaxybook->profile_performance_modes[profile]
> +
> +static int galaxybook_performance_mode_profile_init(struct samsung_galaxybook *galaxybook)
> +{
> +	enum platform_profile_option profile;
> +	struct sawb buf = {};
> +	unsigned int i;
> +	int err;
> +
> +	/* fetch supported performance mode values from ACPI method */
> +	buf.safn = GB_SAFN;
> +	buf.sasb = GB_SASB_PERFORMANCE_MODE;
> +	export_guid(buf.caid, &GB_PERFORMANCE_MODE_GUID);
> +	buf.fncn = GB_FNCN_PERFORMANCE_MODE;
> +	buf.subn = GB_SUBN_PERFORMANCE_MODE_LIST;
> +
> +	err = galaxybook_acpi_method(galaxybook, GB_ACPI_METHOD_PERFORMANCE_MODE,
> +				     &buf, GB_SAWB_LEN_PERFORMANCE_MODE);
> +	if (err) {
> +		dev_dbg(&galaxybook->platform->dev,
> +			"failed to get supported performance modes, error %d\n", err);
> +		return err;
> +	}
> +
> +	/* set initial default profile performance mode mappings */
> +	gb_pfmode(PLATFORM_PROFILE_LOW_POWER) = GB_PERFORMANCE_MODE_SILENT;
> +	gb_pfmode(PLATFORM_PROFILE_QUIET) = GB_PERFORMANCE_MODE_QUIET;
> +	gb_pfmode(PLATFORM_PROFILE_BALANCED) = GB_PERFORMANCE_MODE_OPTIMIZED_LEGACY;
> +	gb_pfmode(PLATFORM_PROFILE_PERFORMANCE) = GB_PERFORMANCE_MODE_PERFORMANCE_LEGACY;
> +	gb_pfmode(PLATFORM_PROFILE_LAST) = GB_PERFORMANCE_MODE_INVALID;
> +
> +	/*
> +	 * Value returned in iob0 will have the number of supported performance
> +	 * modes per device. The performance mode values will then be given as a
> +	 * list after this (iob1-iobX). Loop through the supported values and
> +	 * enable their mapped platform_profile choice, overriding "legacy"
> +	 * values along the way if a non-legacy value exists.
> +	 */
> +	for (i = 1; i <= buf.iob0; i++) {
> +		dev_dbg(&galaxybook->platform->dev,
> +			"device supports performance mode 0x%x\n", buf.iob_values[i]);
> +		err = get_performance_mode_profile(galaxybook, buf.iob_values[i], &profile);

Here we pass iob_values[i] through a switch-case block inside
get_performance_mode_profile() and then we do it again bellow. Maybe
this all could be done here, without having to call
get_performance_mode_profile().

> +		if (err)
> +			continue;
> +		switch (buf.iob_values[i]) {
> +		case GB_PERFORMANCE_MODE_OPTIMIZED:
> +			/* override legacy Optimized value */
> +			gb_pfmode(profile) = GB_PERFORMANCE_MODE_OPTIMIZED;
> +			break;
> +		case GB_PERFORMANCE_MODE_PERFORMANCE:
> +			/* override legacy Performance value */
> +			gb_pfmode(profile) = GB_PERFORMANCE_MODE_PERFORMANCE;
> +			break;
> +		case GB_PERFORMANCE_MODE_ULTRA:
> +			/*
> +			 * if Ultra is supported, downgrade performance to
> +			 * balanced-performance
> +			 */

I haven't been following the entire discussion, so I don't know if Armin
changed his mind but I agree with him. 

I think GB_PERFORMANCE_MODE_PERFORMANCE_LEGACY should be statically
mapped to BALANCED_PERFORMANCE and TURBO should be PERFORMANCE. This
would simplify a lot of the logic here.

> +			if (test_bit(PLATFORM_PROFILE_PERFORMANCE,
> +				     galaxybook->platform_profile_choices)) {
> +				gb_pfmode(PLATFORM_PROFILE_BALANCED_PERFORMANCE) =
> +					gb_pfmode(PLATFORM_PROFILE_PERFORMANCE);
> +				set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
> +					galaxybook->platform_profile_choices);
> +			}
> +			/* override performance profile to use Ultra's value */
> +			gb_pfmode(profile) = GB_PERFORMANCE_MODE_ULTRA;
> +			break;
> +		default:
> +			break;
> +		}
> +		set_bit(profile, galaxybook->platform_profile_choices);
> +	}
> +
> +	err = galaxybook_performance_mode_init(galaxybook);

If the main goal of this method is to set an initial profile maybe we
can just directly set it after finding GB_PERFORMANCE_MODE_OPTIMIZED?

This would eliminate a bit of indirection.

Do you know if all devices support OPTIMIZED? either legacy or
non-legacy.

> +	if (err) {
> +		dev_dbg(&galaxybook->platform->dev,
> +			"failed to initialize performance mode, error %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int galaxybook_platform_profile_init(struct samsung_galaxybook *galaxybook)
> +{
> +	struct device *ppdev;
> +	int err;
> +
> +	err = galaxybook_performance_mode_profile_init(galaxybook);
> +	if (err)
> +		return 0;
> +
> +	galaxybook->has_performance_mode = true;
> +
> +	ppdev = devm_platform_profile_register(&galaxybook->platform->dev, DRIVER_NAME,
> +					       galaxybook, &galaxybook_platform_profile_ops);
> +	if (IS_ERR(ppdev))
> +		return PTR_ERR(ppdev);
> +
> +	platform_profile_notify(ppdev);

No need to notify after registering. You can directly return
PTR_ERR_OR_ZERO().

~ Kurt

> <snip>