linux-kernel - Re: [PATCHv2 1/2] leds: add driver for LM3530 ALS

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: <20110120160407.00ffc28e.akpm@linux-foundation.org>
Date:	Thu, 20 Jan 2011 16:04:07 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Shreshtha Kumar SAHU <shreshthakumar.sahu@...ricsson.com>
Cc:	<linus.walleij@...ricsson.com>,
	<richard.purdie@...uxfoundation.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv2 1/2] leds: add driver for LM3530 ALS

On Thu, 20 Jan 2011 15:40:49 +0530
Shreshtha Kumar SAHU <shreshthakumar.sahu@...ricsson.com> wrote:

> From: Shreshtha Kumar Sahu <shreshthakumar.sahu@...ricsson.com>
> 
> simple backlight driver for National Semiconductor LM3530.
> Presently only manual mode is supported, PWM and ALS support
> to be added.
> 
>
> ...
>
> +static int lm3530_get_mode_from_str(const char *str, int count)
> +{
> +	int i;
> +
> +	if (str[0] == '\n')
> +		return -1;

Why is this here?  I think the function would work OK if it was removed?

> +	for (i = 0; i < LM3530_BL_MODE_MAX; i++)

Could use ARRAY_SIZE(mode_map) here and do away with
LM3530_BL_MODE_MAX.

> +		if (!strncmp(mode_map[i].mode, str, count))

Why strncmp?  The code will treat input of the form "alsfoo" as "als",
which is sloppy.

> +			return mode_map[i].mode_val;
> +
> +	return -1;
> +}
> +
>
> ...
>
> +static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
> +				   *attr, const char *buf, size_t size)
> +{
> +	int err;
> +	struct i2c_client *client = container_of(
> +					dev->parent, struct i2c_client, dev);
> +	struct lm3530_data *drvdata = i2c_get_clientdata(client);
> +	int mode;
> +
> +	mode = lm3530_get_mode_from_str(buf, size-1);

And why the "size-1"?  Assuming there's a \n?  But userspace should be
able to do write(fd, 3, "als")?

Perhaps all this can be tidied up with

	mode = lm3530_get_mode_from_str(strim(buf));

Alternatively, see sysfs_streq() - it was added to address these sorts
of things.

> +	if (mode < 0) {
> +		dev_err(dev, "Invalid mode\n");
> +		return -EINVAL;
> +	}
> +
> +	if (mode == LM3530_BL_MODE_MANUAL)
> +		drvdata->mode = LM3530_BL_MODE_MANUAL;
> +	else if (mode == LM3530_BL_MODE_ALS)
> +		drvdata->mode = LM3530_BL_MODE_ALS;
> +	else if (mode == LM3530_BL_MODE_PWM) {
> +		dev_err(dev, "PWM mode not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	err = lm3530_init_registers(drvdata);
> +	if (err) {
> +		dev_err(dev, "Setting %s Mode failed :%d\n", buf, err);
> +		return err;
> +	}
> +
> +	return sizeof(drvdata->mode);
> +}
> +
>
> ...
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/