linux-kernel - Re: [RFD] voltage/current regulator consumer interface

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: <20090421125528.GD25828@sirena.org.uk>
Date:	Tue, 21 Apr 2009 13:55:29 +0100
From:	Mark Brown <broonie@...ena.org.uk>
To:	Mike Rapoport <mike@...pulab.co.il>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFD] voltage/current regulator consumer interface

On Tue, Apr 21, 2009 at 03:00:41PM +0300, Mike Rapoport wrote:

> I've managed to create some preliminary "line-consumer" driver. I don't really
> like the name, but I couldn't think of something better and "virtual" is
> already taken :)

"userspace" or "user"?  The goal here is to let userspace applications
be consumers.

> +static ssize_t set_state(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	struct line_consumer_data *data = dev_get_drvdata(dev);
> +	bool enabled;
> +	int ret;
> +
> +	/*
> +	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
> +	 * will be shared with show_state(), above.
> +	 */
> +	if (sysfs_streq(buf, "enabled\n") == 0)
> +		enabled = true;
> +	else if (sysfs_streq(buf, "disabled\n") == 0)
> +		enabled = false;

I'd be inclined to also accept 1 and 0 here.

> +static DEVICE_ATTR(name, 0444, show_name, NULL);
> +static DEVICE_ATTR(state, 0666, show_state, set_state);

Permissions for set_state() should probably be tighter?

> +	for (i = 0; i < ARRAY_SIZE(attributes); i++) {
> +		ret = device_create_file(&pdev->dev, attributes[i]);
> +		if (ret != 0)
> +			goto err_create_attrs;
> +	}

device_add_attributes()?

> +	if (pdata->init_on)
> +		ret = regulator_bulk_enable(drvdata->num_supplies,
> +					    drvdata->supplies);
> +	else
> +		ret = regulator_bulk_disable(drvdata->num_supplies,
> +					     drvdata->supplies);

The disable case will lead to unbalanced enables and disables, loosing
a reference to the supply.  Just don't do anything if the supplies
should not be enabled.

> +static int regulator_line_consumer_remove(struct platform_device *pdev)
> +{
> +	struct line_consumer_data *drvdata = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(attributes); i++)
> +		device_remove_file(&pdev->dev, attributes[i]);
> +
> +	regulator_bulk_free(drvdata->num_supplies, drvdata->supplies);
> +	kfree(drvdata->supplies);
> +	kfree(drvdata);
> +
> +	return 0;
> +}

Hrm.  You probably want to disable the supplies if you weren't using
them here.
--
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/