linux-kernel - Re: [PATCH 4/5] leds: add ChromeOS EC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 28 May 2024 07:15:07 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
	Benson Leung <bleung@...omium.org>,
	Guenter Roeck <groeck@...omium.org>, linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev,
	Dustin Howett <dustin@...ett.net>,
	Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH 4/5] leds: add ChromeOS EC driver

On Tue, May 28, 2024 at 07:25:07AM +0200, Thomas Weißschuh wrote:
> On 2024-05-28 05:09:29+0000, Tzung-Bi Shih wrote:
> > On Mon, May 20, 2024 at 12:00:32PM +0200, Thomas Weißschuh wrote:
> > > +static int cros_ec_led_count_subleds(struct device *dev,
> > > +				     struct ec_response_led_control *resp,
> > > +				     unsigned int *max_brightness)
> > > +{
> > > +	unsigned int range, common_range = 0;
> > > +	int num_subleds = 0;
> > > +	size_t i;
> > > +
> > > +	for (i = 0; i < EC_LED_COLOR_COUNT; i++) {
> > > +		range = resp->brightness_range[i];
> > > +
> > > +		if (!range)
> > > +			continue;
> > > +
> > > +		num_subleds++;
> > > +
> > > +		if (!common_range)
> > > +			common_range = range;
> > > +
> > > +		if (common_range != range) {
> > > +			/* The multicolor LED API expects a uniform max_brightness */
> > > +			dev_warn(dev, "Inconsistent LED brightness values\n");
> > > +			return -EINVAL;
> > > +		}
> > 
> > What if the array is [0, 1, 1]?
> 
> The "0" will be skipped by 
> 
> if (!range)
> 	continue;
> 
> And the two "1" will pass the check.

Ack.

> > > +static int __init cros_ec_led_init(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = led_trigger_register(&cros_ec_led_trigger);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = platform_driver_register(&cros_ec_led_driver);
> > > +	if (ret)
> > > +		led_trigger_unregister(&cros_ec_led_trigger);
> > > +
> > > +	return ret;
> > > +};
> > > +module_init(cros_ec_led_init);
> > > +
> > > +static void __exit cros_ec_led_exit(void)
> > > +{
> > > +	platform_driver_unregister(&cros_ec_led_driver);
> > > +	led_trigger_unregister(&cros_ec_led_trigger);
> > > +};
> > > +module_exit(cros_ec_led_exit);
> > 
> > I wonder it could use module_led_trigger() and module_platform_driver().
> 
> This won't compile as the macros generate various duplicate symbols.
> 
> Also the order is important, so I think the explicit logic is clearer.

I'm not sure if it is feasible by separating the trigger part to
drivers/leds/trigger/ and specify it in `default_trigger`.