[RFC/PATCH 17/22] W1: cleanup slave refcounting & more

From: Dmitry Torokhov
Date: Thu Apr 21 2005 - 03:21:29 EST

W1: clean-up slave device implementation:
- get rid of separate refcount, rely on driver model to
enforce lifetime rules;
- pin w1 module until slave device is registered with sysfs
to make sure W1 core stays loaded.
- drop 'name' attribute as we already have it in bus_id.

Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
---

w1.c | 165 ++++++++++++++++++++++++------------------------------------
w1.h | 3 -
w1_family.c | 2
w1_family.h | 2
w1_smem.c | 18 ------
w1_therm.c | 20 -------
6 files changed, 71 insertions(+), 139 deletions(-)

Index: dtor/drivers/w1/w1.c
===================================================================
--- dtor.orig/drivers/w1/w1.c
+++ dtor/drivers/w1/w1.c
@@ -108,9 +108,6 @@ static ssize_t w1_default_read_bin(struc
return sprintf(buf, "No family registered.\n");
}

-static struct device_attribute w1_slave_attribute =
- __ATTR(name, S_IRUGO, w1_default_read_name, NULL);
-
static struct device_attribute w1_slave_attribute_val =
__ATTR(value, S_IRUGO, w1_default_read_name, NULL);

@@ -126,166 +123,138 @@ static struct bin_attribute w1_slave_bin

static void w1_slave_release(struct device *dev)
{
- struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
+ struct w1_slave *slave = to_w1_slave(dev);

- complete(&sl->dev_released);
+ w1_family_put(slave->family);
+ kfree(slave);
+ module_put(THIS_MODULE);
}

-static int __w1_attach_slave_device(struct w1_slave *sl)
+static int __w1_attach_slave_device(struct w1_slave *slave)
{
int err;

- sl->dev.parent = &sl->master->dev;
- sl->dev.bus = &w1_bus_type;
- sl->dev.release = &w1_slave_release;
+ slave->dev.parent = &slave->master->dev;
+ slave->dev.bus = &w1_bus_type;
+ slave->dev.release = &w1_slave_release;

- snprintf(&sl->dev.bus_id[0], sizeof(sl->dev.bus_id),
- "%02x-%012llx",
- (unsigned int) sl->reg_num.family,
- (unsigned long long) sl->reg_num.id);
- snprintf (&sl->name[0], sizeof(sl->name),
+ snprintf(&slave->dev.bus_id[0], sizeof(slave->dev.bus_id),
"%02x-%012llx",
- (unsigned int) sl->reg_num.family,
- (unsigned long long) sl->reg_num.id);
+ (unsigned int) slave->reg_num.family,
+ (unsigned long long) slave->reg_num.id);

- dev_dbg(&sl->dev, "%s: registering %s.\n", __func__,
- &sl->dev.bus_id[0]);
+ dev_dbg(&slave->dev, "%s: registering %s.\n", __func__,
+ &slave->dev.bus_id[0]);

- err = device_register(&sl->dev);
+ err = device_register(&slave->dev);
if (err < 0) {
- dev_err(&sl->dev,
+ dev_err(&slave->dev,
"Device registration [%s] failed. err=%d\n",
- sl->dev.bus_id, err);
+ slave->dev.bus_id, err);
return err;
}

- memcpy(&sl->attr_bin, &w1_slave_bin_attribute, sizeof(sl->attr_bin));
- memcpy(&sl->attr_name, &w1_slave_attribute, sizeof(sl->attr_name));
- memcpy(&sl->attr_val, &w1_slave_attribute_val, sizeof(sl->attr_val));
-
- sl->attr_bin.read = sl->family->fops->rbin;
- sl->attr_name.show = sl->family->fops->rname;
- sl->attr_val.show = sl->family->fops->rval;
- sl->attr_val.attr.name = sl->family->fops->rvalname;
+ __module_get(THIS_MODULE);

- err = device_create_file(&sl->dev, &sl->attr_name);
- if (err < 0) {
- dev_err(&sl->dev,
- "sysfs file creation for [%s] failed. err=%d\n",
- sl->dev.bus_id, err);
- device_unregister(&sl->dev);
- return err;
- }
+ memcpy(&slave->attr_bin, &w1_slave_bin_attribute, sizeof(slave->attr_bin));
+ memcpy(&slave->attr_val, &w1_slave_attribute_val, sizeof(slave->attr_val));

- err = device_create_file(&sl->dev, &sl->attr_val);
+ slave->attr_bin.read = slave->family->fops->rbin;
+ slave->attr_val.show = slave->family->fops->rval;
+ slave->attr_val.attr.name = slave->family->fops->rvalname;
+
+ err = device_create_file(&slave->dev, &slave->attr_val);
if (err < 0) {
- dev_err(&sl->dev,
+ dev_err(&slave->dev,
"sysfs file creation for [%s] failed. err=%d\n",
- sl->dev.bus_id, err);
- device_remove_file(&sl->dev, &sl->attr_name);
- device_unregister(&sl->dev);
+ slave->dev.bus_id, err);
+ device_unregister(&slave->dev);
return err;
}

- err = sysfs_create_bin_file(&sl->dev.kobj, &sl->attr_bin);
+ err = sysfs_create_bin_file(&slave->dev.kobj, &slave->attr_bin);
if (err < 0) {
- dev_err(&sl->dev,
+ dev_err(&slave->dev,
"sysfs file creation for [%s] failed. err=%d\n",
- sl->dev.bus_id, err);
- device_remove_file(&sl->dev, &sl->attr_name);
- device_remove_file(&sl->dev, &sl->attr_val);
- device_unregister(&sl->dev);
+ slave->dev.bus_id, err);
+ device_remove_file(&slave->dev, &slave->attr_val);
+ device_unregister(&slave->dev);
return err;
}

- err = sysfs_create_group(&sl->dev.kobj, &w1_slave_defattr_group);
+ err = sysfs_create_group(&slave->dev.kobj, &w1_slave_defattr_group);
if (err < 0) {
- dev_err(&sl->dev,
+ dev_err(&slave->dev,
"sysfs group creation for [%s] failed. err=%d\n",
- sl->dev.bus_id, err);
- sysfs_remove_bin_file(&sl->dev.kobj, &sl->attr_bin);
- device_remove_file(&sl->dev, &sl->attr_name);
- device_remove_file(&sl->dev, &sl->attr_val);
- device_unregister(&sl->dev);
+ slave->dev.bus_id, err);
+ sysfs_remove_bin_file(&slave->dev.kobj, &slave->attr_bin);
+ device_remove_file(&slave->dev, &slave->attr_val);
+ device_unregister(&slave->dev);
return err;
}

- list_add_tail(&sl->node, &sl->master->slist);
+ list_add_tail(&slave->node, &slave->master->slist);

return 0;
}

-static int w1_attach_slave_device(struct w1_master *dev, struct w1_reg_num *rn)
+static int w1_attach_slave_device(struct w1_master *master, struct w1_reg_num *rn)
{
- struct w1_slave *sl;
+ struct w1_slave *slave;
struct w1_family *f;
int err;

- sl = kmalloc(sizeof(struct w1_slave), GFP_KERNEL);
- if (!sl) {
- dev_err(&dev->dev,
+ slave = kcalloc(1, sizeof(struct w1_slave), GFP_KERNEL);
+ if (!slave) {
+ dev_err(&master->dev,
"%s: failed to allocate new slave device.\n",
__func__);
return -ENOMEM;
}

- memset(sl, 0, sizeof(*sl));
-
- sl->master = dev;
- set_bit(W1_SLAVE_ACTIVE, &sl->flags);
-
- memcpy(&sl->reg_num, rn, sizeof(sl->reg_num));
- atomic_set(&sl->refcnt, 0);
- init_completion(&sl->dev_released);
+ slave->master = master;
+ slave->reg_num = *rn;
+ set_bit(W1_SLAVE_ACTIVE, &slave->flags);

spin_lock(&w1_flock);
f = w1_family_registered(rn->family);
if (!f) {
spin_unlock(&w1_flock);
- dev_info(&dev->dev, "Family %x for %02x.%012llx.%02x is not registered.\n",
- rn->family, rn->family,
- (unsigned long long)rn->id, rn->crc);
- kfree(sl);
+ dev_info(&master->dev,
+ "Family %x for %02x.%012llx.%02x is not registered.\n",
+ rn->family, rn->family,
+ (unsigned long long)rn->id, rn->crc);
+ kfree(slave);
return -ENODEV;
}
__w1_family_get(f);
spin_unlock(&w1_flock);

- sl->family = f;
+ slave->family = f;

-
- err = __w1_attach_slave_device(sl);
+ err = __w1_attach_slave_device(slave);
if (err < 0) {
- dev_err(&dev->dev, "%s: Attaching %s failed.\n", __func__,
- sl->name);
- w1_family_put(sl->family);
- kfree(sl);
+ dev_err(&master->dev, "%s: Attaching %s failed.\n", __func__,
+ slave->dev.bus_id);
+ w1_family_put(slave->family);
+ kfree(slave);
return err;
}

- sl->ttl = dev->slave_ttl;
+ slave->ttl = master->slave_ttl;

return 0;
}

-static void w1_slave_detach(struct w1_slave *sl)
+static void w1_slave_detach(struct w1_slave *slave)
{
- dev_info(&sl->dev, "%s: detaching %s.\n", __func__, sl->name);
+ dev_info(&slave->dev, "%s: detaching %s.\n",
+ __func__, slave->dev.bus_id);

- while (atomic_read(&sl->refcnt)) {
- printk(KERN_INFO "Waiting for %s to become free: refcnt=%d.\n",
- sl->name, atomic_read(&sl->refcnt));
-
- if (msleep_interruptible(1000))
- flush_signals(current);
- }
-
- sysfs_remove_group(&sl->dev.kobj, &w1_slave_defattr_group);
- sysfs_remove_bin_file(&sl->dev.kobj, &sl->attr_bin);
- device_remove_file(&sl->dev, &sl->attr_name);
- device_remove_file(&sl->dev, &sl->attr_val);
- device_unregister(&sl->dev);
- w1_family_put(sl->family);
+ sysfs_remove_group(&slave->dev.kobj, &w1_slave_defattr_group);
+ sysfs_remove_bin_file(&slave->dev.kobj, &slave->attr_bin);
+ device_remove_file(&slave->dev, &slave->attr_val);
+ device_unregister(&slave->dev);
}

static void w1_slave_found(struct w1_master *dev, u64 rn)
@@ -333,7 +302,6 @@ static void w1_master_scan_slaves(struct
!--slave->ttl) {
list_del(&slave->node);
w1_slave_detach(slave);
- kfree(slave);
}
}
up(&master->mutex);
@@ -554,7 +522,6 @@ void w1_remove_master_device(struct w1_m
list_for_each_entry_safe(slave, next, &master->slist, node) {
list_del(&slave->node);
w1_slave_detach(slave);
- kfree(slave);
}

sysfs_remove_group(&master->dev.kobj, &w1_master_defattr_group);
Index: dtor/drivers/w1/w1.h
===================================================================
--- dtor.orig/drivers/w1/w1.h
+++ dtor/drivers/w1/w1.h
@@ -62,10 +62,8 @@ struct w1_reg_num

struct w1_slave
{
- unsigned char name[W1_MAXNAMELEN];
struct list_head node;
struct w1_reg_num reg_num;
- atomic_t refcnt;
u8 rom[9];
unsigned long flags;
int ttl;
@@ -73,7 +71,6 @@ struct w1_slave
struct w1_master *master;
struct w1_family *family;
struct device dev;
- struct completion dev_released;

struct bin_attribute attr_bin;
struct device_attribute attr_name, attr_val;
Index: dtor/drivers/w1/w1_therm.c
===================================================================
--- dtor.orig/drivers/w1/w1_therm.c
+++ dtor/drivers/w1/w1_therm.c
@@ -41,24 +41,15 @@ static u8 bad_roms[][9] = {
{}
};

-static ssize_t w1_therm_read_name(struct device *, char *);
static ssize_t w1_therm_read_temp(struct device *, char *);
static ssize_t w1_therm_read_bin(struct kobject *, char *, loff_t, size_t);

static struct w1_family_ops w1_therm_fops = {
- .rname = &w1_therm_read_name,
.rbin = &w1_therm_read_bin,
.rval = &w1_therm_read_temp,
.rvalname = "temp1_input",
};

-static ssize_t w1_therm_read_name(struct device *dev, char *buf)
-{
- struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
-
- return sprintf(buf, "%s\n", sl->name);
-}
-
static inline int w1_convert_temp(u8 rom[9])
{
int t, h;
@@ -102,12 +93,8 @@ static ssize_t w1_therm_read_bin(struct
u8 rom[9], crc, verdict;
int i, max_trying = 10;

- atomic_inc(&sl->refcnt);
- smp_mb__after_atomic_inc();
- if (down_interruptible(&sl->master->mutex)) {
- count = 0;
- goto out_dec;
- }
+ if (down_interruptible(&sl->master->mutex))
+ return 0;

if (off > W1_SLAVE_DATA_SIZE) {
count = 0;
@@ -178,9 +165,6 @@ static ssize_t w1_therm_read_bin(struct
count += sprintf(buf + count, "t=%d\n", w1_convert_temp(rom));
out:
up(&dev->mutex);
-out_dec:
- smp_mb__before_atomic_inc();
- atomic_dec(&sl->refcnt);

return count;
}
Index: dtor/drivers/w1/w1_smem.c
===================================================================
--- dtor.orig/drivers/w1/w1_smem.c
+++ dtor/drivers/w1/w1_smem.c
@@ -35,24 +35,15 @@ MODULE_LICENSE("GPL");
MODULE_AUTHOR("Evgeniy Polyakov <johnpol@xxxxxxxxxxx>");
MODULE_DESCRIPTION("Driver for 1-wire Dallas network protocol, 64bit memory family.");

-static ssize_t w1_smem_read_name(struct device *, char *);
static ssize_t w1_smem_read_val(struct device *, char *);
static ssize_t w1_smem_read_bin(struct kobject *, char *, loff_t, size_t);

static struct w1_family_ops w1_smem_fops = {
- .rname = &w1_smem_read_name,
.rbin = &w1_smem_read_bin,
.rval = &w1_smem_read_val,
.rvalname = "id",
};

-static ssize_t w1_smem_read_name(struct device *dev, char *buf)
-{
- struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
-
- return sprintf(buf, "%s\n", sl->name);
-}
-
static ssize_t w1_smem_read_val(struct device *dev, char *buf)
{
struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
@@ -72,11 +63,8 @@ static ssize_t w1_smem_read_bin(struct k
struct w1_slave, dev);
int i;

- atomic_inc(&sl->refcnt);
- if (down_interruptible(&sl->master->mutex)) {
- count = 0;
- goto out_dec;
- }
+ if (down_interruptible(&sl->master->mutex))
+ return 0;

if (off > W1_SLAVE_DATA_SIZE) {
count = 0;
@@ -92,8 +80,6 @@ static ssize_t w1_smem_read_bin(struct k

out:
up(&sl->master->mutex);
-out_dec:
- atomic_dec(&sl->refcnt);

return count;
}
Index: dtor/drivers/w1/w1_family.c
===================================================================
--- dtor.orig/drivers/w1/w1_family.c
+++ dtor/drivers/w1/w1_family.c
@@ -30,7 +30,7 @@ static LIST_HEAD(w1_families);

static int w1_check_family(struct w1_family *f)
{
- if (!f->fops->rname || !f->fops->rbin || !f->fops->rval || !f->fops->rvalname)
+ if (!f->fops->rbin || !f->fops->rval || !f->fops->rvalname)
return -EINVAL;

return 0;
Index: dtor/drivers/w1/w1_family.h
===================================================================
--- dtor.orig/drivers/w1/w1_family.h
+++ dtor/drivers/w1/w1_family.h
@@ -34,9 +34,7 @@

struct w1_family_ops
{
- ssize_t (* rname)(struct device *, char *);
ssize_t (* rbin)(struct kobject *, char *, loff_t, size_t);
-
ssize_t (* rval)(struct device *, char *);
unsigned char rvalname[MAXNAMELEN];
};
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/