linux-kernel - Re: [PATCH v3 5/5] platform/chrome: cros_kbd_led

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]

Message-ID: <YocewB/lLJhIAuQP@google.com>
Date:   Fri, 20 May 2022 12:53:20 +0800
From:   Tzung-Bi Shih <tzungbi@...nel.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     bleung@...omium.org, groeck@...omium.org, robh+dt@...nel.org,
        chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/5] platform/chrome: cros_kbd_led_backlight: support
 EC PWM backend

On Thu, May 19, 2022 at 03:40:21PM -0700, Matthias Kaehlcke wrote:
> On Mon, Mar 21, 2022 at 04:55:47PM +0800, Tzung-Bi Shih wrote:
> > +struct keyboard_led_private {
> 
> Why 'private', isn't this more a 'cros_ec_kdb_bl' or similar?

It is just drvdata.  I would prefer to keep the original prefix
"keyboard_led_" if you wouldn't have strong opinion.

> > +static int
> > +keyboard_led_set_brightness_blocking_ec_pwm(struct led_classdev *cdev,
> > +					    enum led_brightness brightness)
> 
> nit: since there is only a blocking version of .set_brightness you could omit
> 'blocking' in the function name.

Ack, will fix it in next version.

> > +	struct {
> > +		struct cros_ec_command msg;
> > +		struct ec_params_pwm_set_keyboard_backlight params;
> > +	} __packed buf;
> > +	struct ec_params_pwm_set_keyboard_backlight *params = &buf.params;
> > +	struct cros_ec_command *msg = &buf.msg;
> > +	struct keyboard_led_private *private =
> > +		container_of(cdev, struct keyboard_led_private, cdev);
> > +
> > +	memset(&buf, 0, sizeof(buf));
> > +
> > +	msg->version = 0;
> 
> not strictly needed since you do the memset above, I guess it's
> fine to keep the assignment if you want to be explicit about the
> version.

Ack, let's remove them in next version.

> > +static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
> > +{
> > +	struct keyboard_led_private *private = platform_get_drvdata(pdev);
> > +
> > +	private->ec = dev_get_drvdata(pdev->dev.parent);
> > +	if (!private->ec) {
> > +		dev_err(&pdev->dev, "no parent EC device\n");
> > +		return -EINVAL;
> > +	}
> 
> The only thing this 'init' function does is assigning private->ec. Wouldn't
> it be clearer to do this directly in probe() from where callback is called?
> It could be with the condition that the device as a DT node.

No.  The probe() isn't aware of the device is from ACPI or OF.

> Is it actually possible that the keyboard backlight device gets instantiated
> if there is no EC parent?

It shouldn't be but just in case.

> > +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
> > +	.init = keyboard_led_init_ec_pwm_null,
> 
> Is this really needed?
> 
> keyboard_led_probe() checks if .init is assigned before invoking the callback:
> 
> 	if (drvdata->init) {
> 		error = drvdata->init(pdev);
> 
> The whole 'else' branch could be eliminated if .of_match_table of the driver
> only is assigned when CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is set. IMO that
> would preferable over creating 'stubs'.

CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent.  The stubs
were created to avoid compile errors if CONFIG_OF=y but
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n.

However, I just realized it could also have compile errors if CONFIG_OF=n and
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=y.  The `keyboard_led_drvdata_ec_pwm` is
unused.

In any case, I agree with you.  Let's remove the stubs in next version.  I
would use __maybe_unused for some of them.