On Tue, 2005-06-21 at 15:15, Jim Cromie wrote:
> Jim Cromie wrote:
>
> >
> > Im working with scx200_gpio driver, and extending it to operate
> > gpio pins on pc87366 chip, present on the soekris net-4801
> >
> > the chip has many functional-units, some of which are driven by
> > i2c/chips/pc87360.c
> >
> > both drivers have keep their own locks, which prevent other
> > uses of the same driver from corrupting operations,
> > but these offer no protection from interference from the other driver.
> >
> > Is that correct ?
> > And if so, how do I resolve it ?
> >
> > 1. 3rd module, which exports a single lock
You shouldn't need a 3rd module just to contain the exported
semaphores. We need to think about what we are protecting with the
locks. Locks are used to protect shared data. When you replace a
semaphore that is embedded in a structure with a global semaphore,
what are you protecting? What was the problem with how it was
implemented before? Maybe I'm missing something.
Bob
More comments below..
>
>
> OK - no one home. I guess its ok to talk to myself ;-)
>
> attached patch compiles, loads, loads its dependencies,
> IOW, it appears somewhat workable.
>
> But, Is it sane ?
> Is there a better way ?
>
> Depending upon responses here, Ill send to lm-sensors next.
>
> ______________________________________________________________________
> diff -ruN -X exclude-diffs linux-2.6.12-soekris/drivers/char/Makefile i2c/drivers/char/Makefile
> --- linux-2.6.12-soekris/drivers/char/Makefile 2005-06-18 07:04:14.000000000 -0600
> +++ i2c/drivers/char/Makefile 2005-06-21 12:04:41.000000000 -0600
> @@ -12,6 +12,8 @@
> obj-$(CONFIG_LEGACY_PTYS) += pty.o
> obj-$(CONFIG_UNIX98_PTYS) += pty.o
> obj-y += misc.o
> +obj-m += sio_lock.o
> +
> obj-$(CONFIG_VT) += vt_ioctl.o vc_screen.o consolemap.o \
> consolemap_deftbl.o selection.o keyboard.o
> obj-$(CONFIG_HW_CONSOLE) += vt.o defkeymap.o
> diff -ruN -X exclude-diffs linux-2.6.12-soekris/drivers/char/sio_lock.c i2c/drivers/char/sio_lock.c
> --- linux-2.6.12-soekris/drivers/char/sio_lock.c 1969-12-31 17:00:00.000000000 -0700
> +++ i2c/drivers/char/sio_lock.c 2005-06-21 12:46:23.000000000 -0600
> @@ -0,0 +1,47 @@
> +
> +
> +#include <linux/config.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +
> +#include <asm-i386/semaphore.h>
> +
> +MODULE_AUTHOR("Jim Cromie");
> +MODULE_LICENSE("Dual BSD/GPL");
> +
> +/**
> + a module/container for a lock to be shared between
> + drivers/i2c/chips/pc87360 and drivers/char/pc87366_gpio.
> +
> + The 1st module uses semaphores, the 2nd spinlocks. Im changing
> + the 2nd, cuz lm-sensors is maintained, and pc87366_gpio is a new
> + extension (ie mine) of an unmaintained module (scx200, so said
> + author, privately).
> +
> +*/
> +
> +//static DEFINE_SPINLOCK(sio_lock);
> +
> +struct semaphore sio_lock;
> +struct semaphore sio_update_lock;
> +
semaphore structs are not initialized. should use
DECLARE_MUTEX(sio_lock) to declare and initialize.
> +int sio_lock_init_module(void)
> +{
> + printk(KERN_NOTICE "sio_lock_init_module\n");
> + return 0;
> +}
> +
> +void sio_lock_cleanup_module(void)
> +{
> + printk(KERN_NOTICE "sio_lock_cleanup_module\n");
> +}
> +
> +EXPORT_SYMBOL(sio_lock);
> +EXPORT_SYMBOL(sio_update_lock);
> +
> +module_init(sio_lock_init_module);
> +module_exit(sio_lock_cleanup_module);
> +
> +
> +
> diff -ruN -X exclude-diffs linux-2.6.12-soekris/drivers/i2c/chips/pc87360.c i2c/drivers/i2c/chips/pc87360.c
> --- linux-2.6.12-soekris/drivers/i2c/chips/pc87360.c 2005-06-18 07:04:21.000000000 -0600
> +++ i2c/drivers/i2c/chips/pc87360.c 2005-06-21 12:48:12.000000000 -0600
> @@ -50,6 +50,24 @@
> static unsigned int extra_isa[3];
> static u8 confreg[4];
>
> +#define SHARED_LOCKS 1
> +
> +#if SHARED_LOCKS
> +extern struct semaphore sio_lock;
> +extern struct semaphore sio_update_lock;
> +
> +#define down_lock down(&sio_lock)
> +#define down_updatelock down(&sio_update_lock)
> +#define up_lock up(&sio_lock)
> +#define up_updatelock up(&sio_update_lock)
> +
> +#else
> +#define down_lock down(&(data->lock))
> +#define down_updatelock down(&(data->update_lock))
> +#define up_lock up(&(data->lock))
> +#define up_updatelock up(&(data->update_lock))
> +#endif
I do not believe it is recommended to define macros that do specific
function calls like this, as the resulting source code is unclear as
to what exactly is being locked without looking back at the macro
definition. I believe the correct method is to include the function
call.
> +
> enum chips { any_chip, pc87360, pc87363, pc87364, pc87365, pc87366 };
> static struct i2c_address_data addr_data = {
> .normal_i2c = normal_i2c,
> @@ -187,8 +205,10 @@
>
> struct pc87360_data {
> struct i2c_client client;
> +#if ! SHARED_LOCKS
> struct semaphore lock;
> struct semaphore update_lock;
> +#endif
> char valid; /* !=0 if following fields are valid */
> unsigned long last_updated; /* In jiffies */
>
> @@ -259,7 +279,7 @@
> struct pc87360_data *data = i2c_get_clientdata(client);
> long fan_min = simple_strtol(buf, NULL, 10);
>
> - down(&data->update_lock);
> + down_updatelock;
> fan_min = FAN_TO_REG(fan_min, FAN_DIV_FROM_REG(data->fan_status[nr]));
>
> /* If it wouldn't fit, change clock divisor */
> @@ -276,7 +296,7 @@
> /* Write new divider, preserve alarm bits */
> pc87360_write_value(data, LD_FAN, NO_BANK, PC87360_REG_FAN_STATUS(nr),
> data->fan_status[nr] & 0xF9);
> - up(&data->update_lock);
> + up_updatelock;
>
> return count;
> }
> @@ -339,12 +359,12 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->pwm[offset-1] = PWM_TO_REG(val, \
> FAN_CONFIG_INVERT(data->fan_conf, offset-1)); \
> pc87360_write_value(data, LD_FAN, NO_BANK, PC87360_REG_PWM(offset-1), \
> data->pwm[offset-1]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static DEVICE_ATTR(pwm##offset, S_IWUSR | S_IRUGO, \
> @@ -384,11 +404,11 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->in_min[offset] = IN_TO_REG(val, data->in_vref); \
> pc87360_write_value(data, LD_IN, offset, PC87365_REG_IN_MIN, \
> data->in_min[offset]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static ssize_t set_in##offset##_max(struct device *dev, const char *buf, \
> @@ -398,12 +418,12 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->in_max[offset] = IN_TO_REG(val, \
> data->in_vref); \
> pc87360_write_value(data, LD_IN, offset, PC87365_REG_IN_MAX, \
> data->in_max[offset]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static DEVICE_ATTR(in##offset##_input, S_IRUGO, \
> @@ -463,11 +483,11 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->in_min[offset+7] = IN_TO_REG(val, data->in_vref); \
> pc87360_write_value(data, LD_IN, offset+7, PC87365_REG_TEMP_MIN, \
> data->in_min[offset+7]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static ssize_t set_temp##offset##_max(struct device *dev, const char *buf, \
> @@ -477,11 +497,11 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->in_max[offset+7] = IN_TO_REG(val, data->in_vref); \
> pc87360_write_value(data, LD_IN, offset+7, PC87365_REG_TEMP_MAX, \
> data->in_max[offset+7]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static ssize_t set_temp##offset##_crit(struct device *dev, const char *buf, \
> @@ -491,11 +511,11 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->in_crit[offset-4] = IN_TO_REG(val, data->in_vref); \
> pc87360_write_value(data, LD_IN, offset+7, PC87365_REG_TEMP_CRIT, \
> data->in_crit[offset-4]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
> @@ -573,11 +593,11 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->temp_min[offset-1] = TEMP_TO_REG(val); \
> pc87360_write_value(data, LD_TEMP, offset-1, PC87365_REG_TEMP_MIN, \
> data->temp_min[offset-1]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static ssize_t set_temp##offset##_max(struct device *dev, const char *buf, \
> @@ -587,11 +607,11 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->temp_max[offset-1] = TEMP_TO_REG(val); \
> pc87360_write_value(data, LD_TEMP, offset-1, PC87365_REG_TEMP_MAX, \
> data->temp_max[offset-1]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static ssize_t set_temp##offset##_crit(struct device *dev, const char *buf, \
> @@ -601,11 +621,11 @@
> struct pc87360_data *data = i2c_get_clientdata(client); \
> long val = simple_strtol(buf, NULL, 10); \
> \
> - down(&data->update_lock); \
> + down_updatelock; \
> data->temp_crit[offset-1] = TEMP_TO_REG(val); \
> pc87360_write_value(data, LD_TEMP, offset-1, PC87365_REG_TEMP_CRIT, \
> data->temp_crit[offset-1]); \
> - up(&data->update_lock); \
> + up_updatelock; \
> return count; \
> } \
> static DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
> @@ -749,7 +769,7 @@
> new_client = &data->client;
> i2c_set_clientdata(new_client, data);
> new_client->addr = address;
> - init_MUTEX(&data->lock);
> + init_MUTEX(&sio_lock);
> new_client->adapter = adapter;
> new_client->driver = &pc87360_driver;
> new_client->flags = 0;
> @@ -782,7 +802,7 @@
>
> strcpy(new_client->name, name);
> data->valid = 0;
> - init_MUTEX(&data->update_lock);
> + init_MUTEX(&sio_update_lock);
>
> for (i = 0; i < 3; i++) {
> if (((data->address[i] = extra_isa[i]))
> @@ -1014,11 +1034,11 @@
> {
> int res;
>
> - down(&(data->lock));
> + down_lock;
> if (bank != NO_BANK)
> outb_p(bank, data->address[ldi] + PC87365_REG_BANK);
> res = inb_p(data->address[ldi] + reg);
> - up(&(data->lock));
> + up_lock;
>
> return res;
> }
> @@ -1026,11 +1046,11 @@
> static void pc87360_write_value(struct pc87360_data *data, u8 ldi, u8 bank,
> u8 reg, u8 value)
> {
> - down(&(data->lock));
> + down_lock;
> if (bank != NO_BANK)
> outb_p(bank, data->address[ldi] + PC87365_REG_BANK);
> outb_p(value, data->address[ldi] + reg);
> - up(&(data->lock));
> + up_lock;
> }
>
> static void pc87360_init_client(struct i2c_client *client, int use_thermistors)
> @@ -1202,7 +1222,7 @@
> struct pc87360_data *data = i2c_get_clientdata(client);
> u8 i;
>
> - down(&data->update_lock);
> + down_updatelock;
>
> if (time_after(jiffies, data->last_updated + HZ * 2) || !data->valid) {
> dev_dbg(&client->dev, "Data update\n");
> @@ -1302,7 +1322,7 @@
> data->valid = 1;
> }
>
> - up(&data->update_lock);
> + up_updatelock;
>
> return data;
> }
--
Kernelnewbies: Help each other learn about the Linux kernel.
Archive: http://mail.nl.linux.org/kernelnewbies/
FAQ: http://kernelnewbies.org/faq/
