linux-kernel - Re: [PATCH RFC 1/8] Introduce dynamic clock devices

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: <201011080738.41871.arnd@arndb.de>
Date:	Mon, 8 Nov 2010 07:38:41 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Richard Cochran <richardcochran@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Christoph Lameter <cl@...ux.com>,
	John Stultz <johnstul@...ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH RFC 1/8] Introduce dynamic clock devices

On Thursday 04 November 2010, Richard Cochran wrote:
> This patch adds support for adding and removing clock devices. The clock
> lifetime cycle is patterned after usb devices. Each clock is represented
> by a standard character device. In addition, the driver may optionally
> implemented custom character device operations.
> 
> The clock devices do not yet do anything useful. This patch merely
> provides some needed infrastructure.
> +/**
> + * create_clock_device() - register a new clock

I'd call this clock_device_create() etc, putting all clock_device functions
in a common namespace with the clock_device_ prefix

> +#define MAX_CLKDEV BITS_PER_LONG
> +static DECLARE_BITMAP(clocks_map, MAX_CLKDEV);
> +static DEFINE_MUTEX(clocks_mux); /* protects 'clocks_map' */

Using "_mux" as a postfix for the name is highly unusual, this normally
means multiplex, not mutual exlusion.

You could probably avoid the need for this mutex altogether if you use
the atomic bit operations (e.g. test_and_set_bit).

> +struct clock_device {
> +	struct file_operations fops;
> +	struct file_operations *driver_fops;
> +	struct clock_device_operations *ops;
> +	struct cdev cdev;
> +	struct kref kref;
> +	struct mutex mux;
> +	void *priv;
> +	int index;
> +	bool zombie;
> +};

You should really not need the file_operations here, neither the
struct nor the pointer. Just define a static file_operations
structure containing clock_device_open and clock_device_release,
and whatever else you might need, then add the driver's operations
to clock_device_operations, and pass the clock_device pointer
directly to them, instead of passing the file/inode pointers.

	Arnd

--
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/