Re: [RFC-patch 1/3] SuperIO locks coordinator

["Randy.Dunlap" <rdunlap@xenotime.net>]

On Thu, 14 Sep 2006 01:13:03 -0600 Jim Cromie wrote:

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/Kconfig 6locks-2/drivers/isa/Kconfig
> --- 6locks-1/drivers/isa/Kconfig	1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/drivers/isa/Kconfig	2006-09-13 09:54:18.000000000 -0600
> @@ -0,0 +1,7 @@
> +
> +config SUPERIO_LOCKS
> +	tristate "Super-IO port sharing"
> +	help
> +	  this module provides locks for use by drivers which need to

          This

> +	  share access to a multi-function device via its superio port, 
> +	  and which register that port.

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/superio_locks.c 6locks-2/drivers/isa/superio_locks.c
> --- 6locks-1/drivers/isa/superio_locks.c	1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/drivers/isa/superio_locks.c	2006-09-13 14:56:32.000000000 -0600
> @@ -0,0 +1,169 @@
> +
> +/**
> +   module provides a means for modules to register their use of a
> +   Super-IO port, and provides an access-lock for the registering
> +   modules to use to coordinate with each other.  Consider it a
> +   parking-attendant's key-board.  Design is perhaps ISA centric,
> +   maybe formalize this, with (platform|isa)_driver.
> +*/

Two comments about that comment:

a.  Kernel long-comment style is:

/*
 * This module provides a means for modules to register
 * their use of a Super-IO port, ...
 */

b.  Don't use "/**" to begin a comment block unless the comment block
contains kernel-doc formatted comments.  (This one does not.)


> +
> +/* superio_get() checks whether the expected SuperIO device is
> +   present at a specific cmd-addr.  Use in loop to scan.
> +*/

For functions that are not static (and when you want this merged,
not just RFC), please include kernel-doc comments describing the
function and its parameters.  See Documentation/kernel-doc-nano-HOWTO.txt
for more info.

> +struct superio* superio_get(u16 cmd_addr, u8 dev_id_addr,
> +			    u8 want_devid)
> +{
> +	int slot, rc, mydevid;
> +
> +	mutex_lock(&reservation_lock);
> +
> +	/* share any already allocated lock for this cmd_addr, device-id */
> +	for (slot = 0; slot < max_locks; slot++) {
> +		if (sio_locks[slot].users 
> +		    && cmd_addr == sio_locks[slot].sioaddr
> +		    && want_devid == sio_locks[slot].devid) {
> +
> +			if (sio_locks[slot].users == 255) {
> +				dprintk("too many drivers sharing port %x\n", cmd_addr);
> +				mutex_unlock(&reservation_lock);
> +				return 0;
> +			}
> +			sio_locks[slot].users++;
> +			dprintk("sharing port:%x dev:%x users:%d\n",
> +				cmd_addr, want_devid, sio_locks[slot].users);
> +			mutex_unlock(&reservation_lock);
> +			return &sio_locks[slot];
> +		}
> +	}
> +	/* read the device-id-address */
> +	outb(dev_id_addr, cmd_addr);
> +	mydevid = inb(cmd_addr+1);
> +
> +	/* but 1st, check that the cmd register remembers the val just written */
> +	rc = inb(cmd_addr);
> +	if (rc != dev_id_addr) {
> +		dprintk("superio_cmdaddr %x absent %d\n", cmd_addr, rc);
> +		mutex_unlock(&reservation_lock);
> +		return NULL;
> +	}
> +	/* test for the desired device id value */
> +	if (mydevid != want_devid) {
> +		mutex_unlock(&reservation_lock);
> +		return NULL;
> +	}
> +	/* find 1st unused slot */
> +	for (slot = 0; slot < max_locks; slot++)
> +		if (!sio_locks[slot].users)
> +			break;
> +
> +	if (slot >= max_locks) {
> +		printk(KERN_ERR "No superio-locks left. increase max_locks\n");
> +		mutex_unlock(&reservation_lock);
> +		return NULL;
> +	}
> +	dprintk("allocating slot %d, addr %x for device %x\n",
> +		slot, cmd_addr, want_devid);
> +
> +	sio_locks[slot].sioaddr = cmd_addr;
> +	sio_locks[slot].devid = want_devid;
> +	sio_locks[slot].users = 1;
> +	num_locks++;
> +
> +	mutex_unlock(&reservation_lock);
> +	return &sio_locks[slot];
> +}
> +EXPORT_SYMBOL_GPL(superio_get);
> +
> +/* array args must be null terminated */

Also needs kernel-doc function comment header.

> +struct superio* superio_find(u16 cmd_addrs[], u8 devid_addr,
> +			     u8 want_devids[])
> +{
> +	int i, j;
> +	struct superio* gate;
> +
> +	for (i = 0; cmd_addrs[i]; i++) {
> +		for (j = 0; want_devids[j]; j++) {
> +			gate = superio_get(cmd_addrs[i], devid_addr,
> +					   want_devids[j]);
> +			if (gate) {
> +				dprintk("found devid:%x port:%x\n",
> +					want_devids[j], cmd_addrs[i]);
> +				return gate;
> +			} else
> +				dprintk("no devid:%x at port:%x\n",
> +					want_devids[j], cmd_addrs[i]);
> +		}
> +	}
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(superio_find);
> +
> +void superio_release(struct superio* const gate)

Ditto.

> +{
> +	if (gate < &sio_locks[0] || gate >= &sio_locks[max_locks]) {
> +		printk(KERN_ERR
> +		       " superio: attempt to release corrupted superio-lock"
> +		       " %p vs %p\n", gate, &sio_locks);
> +		return;
> +	}
> +	if (!(--gate->users))
> +		dprintk("releasing last user of superio-port %x\n", gate->sioaddr);
> +	return;
> +}
> +EXPORT_SYMBOL_GPL(superio_release);
> +

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/include/linux/superio-locks.h 6locks-2/include/linux/superio-locks.h
> --- 6locks-1/include/linux/superio-locks.h	1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/include/linux/superio-locks.h	2006-09-13 14:21:08.000000000 -0600
> @@ -0,0 +1,55 @@
> +#include <linux/mutex.h>
> +#include <asm/io.h>
> +
> +/* Super-IO ports are found in low-pin-count hardware (typically ISA,
> +   any others ?).  They usually provide access to many functional
> +   units, so many drivers must share the superio port.  This struct
> +   provides a lock that allows the drivers to coordinate access to that
> +   port.
> +*/

Use long-comment style, please.

> +struct superio {
> +	struct mutex lock;	/* lock shared amongst user drivers */
> +	u16 sioaddr;		/* port's tested cmd-address */
> +	u8 devid;		/* devid found by the registering driver */
> +	u8 users;		/* I cant imagine >256 user drivers */
> +};
> +
> +/* array args must be null terminated */
> +struct superio* superio_find(u16 sioaddrs[], u8 devid_addr, u8 devid_vals[]);
> +struct superio* superio_get(u16 sioaddr, u8 devid_addr, u8 devid_val);
> +void superio_release(struct superio* const gate);
> +
> +/* these locking ops do not address the idling & activation of some
> +   superio devices, which will, once 'locked', ignore accesses until
> +   the 'unlock' sequence is done 1st.  Unfortunately these sequences
> +   vary by device, and in any case don't protect 2 drivers from
> +   stepping on each other's operations.
> +
> +   Callbacks are a possible approach, but every driver using a device
> +   would have to provide them, and only the 1st loaded module would
> +   actually succeed in registering them.  Furthermore, if any driver
> +   accessing a port uses the idle/activate sequences, they all must.
> +   On the whole, this is complexity w/o benefit.
> +*/

long-comment style, please.

---
~Randy