[PATCH 1/35] W1: fix deadlocks and remove w1_control_thread

[PATCH 1/35] W1: fix deadlocks and remove w1_control_thread

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 19455 bytes --]

Here is an improvement to help in a dynamic ticks kenrel.  I'm
removing w1_control_thread as it would continuously sleep for one
second intervals when it only had two tasks, reconnect slave devices
if a new family is detected (usually happens one on bootup), and when
the one wire system is being unloaded.  The majority of the time there
isn't anything to do, the ax doesn't chop out a thread this obvious
very often.

This fixes a deadlock of removing a slave device driver when a slave
is currently using it.  The slaves using that device driver needs to
be unregistered first.

Now that the slaves would be unregistered to fix that deadlock, there
was a locking order deadlock introduced.  This was resolved by adding
another argument to the search callback so it didn't need to acquire
the other lock (and it is faster as it doesn't have to search).

This fixes an infinite loop for when a slave device driver is loaded
and there will be at least two slave devices unclaimed.

w1_family.h 1.3
w1.h 1.5
The function prototype for w1_reconnect_slaves was moved to the w1.h
header as it is defined in the w1.c source file.

masters/ds1wm.c 1.2
Changes to the 1-wire search routine to pass w1_master back to the
callback.  This is untested, I don't have the hardware.

w1.c 1.2
w1_family.c 1.3
Fixed the infinite loop that happens if a slave device module is
removed from the kernel and there is a slave using that driver family.
That involves unattaching all slaves that are using that family after
the family has been removed from the list of available family drivers.
This is not a race condition, it will happen everytime.

w1.c 1.3
w1.h 1.6
w1_int.c 1.2
- w1_control_thread removed
  - 1 second wake up polling when not required
  - infinite loop with two (or more) unclaimed slave devices
  - did not handle removal of slave device drivers
- w1_reconnect_slaves enhanced
  - now does the reconnect work of w1_control
  - fixes infinite loop
  - handles removal of slave devices
- w1_detach_family removed
- w1_search_master function removed (fix deadlock from above)
- w1_slave_found_callback
  - takes the w1_master pointer so w1_search_master is not required
- w1_bus_master
  - search now takes a pointer to w1_master for the callback

w1_control just sounds to me like a bad idea.  Here is a kernel thread
that only has two things to do.  Die and respond to slave reconnect
events.  The die task is only executed when the one wire system is
unloaded (happens once in the module lifetime).  Slave reconnect
events only happen when a slave device driver is loaded and there are
four such drivers which might happen once on boot, or more frequently
for development.  Why have a kernel thread wake up 3600 times an hour
for five events that will happen during the lifetime of a normal
system operation?

The slave reconnect was incorrectly coded.  When a new slave device
driver (and family code) was available it was intended to give slaves
using the default device a chance to use the specific slave device
driver.  It was using the list_for_each_entry_safe and took the brute
force method of any device using the default device was detached and
reconnected.  If there was a slave device driver that matched the
family code it would be added back to the list with that driver,
otherwise it would be added back with the default driver.  It only
takes two devices that don't match the current set of family codes to
be added to the end of the list to get into an infinite loop of
detach, add to the end of the list, get next entry.

w1_control die cleanup has been moved to the w1_int.c
__w1_remove_master_device routine.  Now the cleanup can be executed
immediately without the maximum one second wait.                                w1_destroy_master_attributes and w1_slave_detach are no longer static
so they can be called from w1_int.c.

w1_reconnect_slaves now does the work of adding a family code or
removing it instead of setting a flag for w1_control thread to check
and do later.  For an added family it will only detach slaves that
will be using that family code fixing the infinte loop.  It will also
detach devices using the family code when a family code is being
removed, reattaching them with the default driver.

w1_family.c on slave device driver unload would remove the driver from
the list of availble family codes, but it did not disconnect slaves
that might be using the driver being removed.  If a slave was using
the driver then the reference count would never become zero and
w1_unregiter_family would never return.  With the improved
w1_reconnect_slaves it will now detach slaves.

Remove previously introduced w1_detach_family and add an argument to
w1_reconnect_slaves because the two functions only differ on selecting
which slaves are detached and attached.

Deadlock (introduced when w1_control_thread was removed as the code             is being executed now rather than later with a different locking
order).  Operations on the bus hold the mutex for that bus.  The bus
search holds the mutex while in progress.  When a new slave is found
the callback w1_slave_found is called which then calls
w1_search_master to search for the master device that matches the
void* argument.  It locks the w1_mlock to search through w1_masters.
On my small two device network the bus search takes 7.7 seconds which
can hold the bus mutex for quite some time.

Execute rmmod on one of the slave device drivers and part of the
family cleanup will detach slave using that family code.  The
w1_reconnect_slaves locks w1_mlock to access w1_masters, then locks
each bus mutex to scan the set of slaves.
bus search, lock mutex
rmmod slave device driver
        w1_reconnect_slaves lock w1_mlock
        block on mutex (which bus search has)
bus search finds slave
        w1_search_master block w1_mlock (which slave rmmod has)
        DEADLOCK
I should also note that on my system with the default bus search,
43.5% of the time is spent in the bus search.  That's a huge time
window for the deadlock.

Solution, remove w1_search_master and instead pass the w1_master
pointer to the find slave callback so it doesn't have to search the
w1_masters list and does not require the w1_mlock.
The callback now has the prototype,
typedef void (* w1_slave_found_callback)(struct w1_master *, u64);

w1_io.c 1.5
Added the extra w1_master pointer when calling search.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds1wm.c |    6 +-
 drivers/w1/w1.c            |  145 ++++++++++----------------------------------
 drivers/w1/w1.h            |   19 +++++-
 drivers/w1/w1_family.c     |    6 ++-
 drivers/w1/w1_family.h     |    1 -
 drivers/w1/w1_int.c        |   13 ++++
 drivers/w1/w1_io.c         |    3 +-
 7 files changed, 71 insertions(+), 122 deletions(-)

diff --git a/drivers/w1/masters/ds1wm.c b/drivers/w1/masters/ds1wm.c
index 10211e4..ea894bf 100644
--- a/drivers/w1/masters/ds1wm.c
+++ b/drivers/w1/masters/ds1wm.c
@@ -274,8 +274,8 @@ static u8 ds1wm_reset_bus(void *data)
 	return 0;
 }
 
-static void ds1wm_search(void *data, u8 search_type,
-			 w1_slave_found_callback slave_found)
+static void ds1wm_search(void *data, struct w1_master *master_dev,
+			u8 search_type, w1_slave_found_callback slave_found)
 {
 	struct ds1wm_data *ds1wm_data = data;
 	int i;
@@ -313,7 +313,7 @@ static void ds1wm_search(void *data, u8 search_type,
 	ds1wm_write_register(ds1wm_data, DS1WM_CMD, ~DS1WM_CMD_SRA);
 	ds1wm_reset(ds1wm_data);
 
-	slave_found(ds1wm_data, rom_id);
+	slave_found(master_dev, rom_id);
 }
 
 /* --------------------------------------------------------------------- */
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 7293c9b..2d11fe7 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -46,20 +46,16 @@ MODULE_AUTHOR("Evgeniy Polyakov <johnpol@2ka.mipt.ru>");
 MODULE_DESCRIPTION("Driver for 1-wire Dallas network protocol.");
 
 static int w1_timeout = 10;
-static int w1_control_timeout = 1;
 int w1_max_slave_count = 10;
 int w1_max_slave_ttl = 10;
 
 module_param_named(timeout, w1_timeout, int, 0);
-module_param_named(control_timeout, w1_control_timeout, int, 0);
 module_param_named(max_slave_count, w1_max_slave_count, int, 0);
 module_param_named(slave_ttl, w1_max_slave_ttl, int, 0);
 
 DEFINE_MUTEX(w1_mlock);
 LIST_HEAD(w1_masters);
 
-static struct task_struct *w1_control_thread;
-
 static int w1_master_match(struct device *dev, struct device_driver *drv)
 {
 	return 1;
@@ -390,7 +386,7 @@ int w1_create_master_attributes(struct w1_master *master)
 	return sysfs_create_group(&master->dev.kobj, &w1_master_defattr_group);
 }
 
-static void w1_destroy_master_attributes(struct w1_master *master)
+void w1_destroy_master_attributes(struct w1_master *master)
 {
 	sysfs_remove_group(&master->dev.kobj, &w1_master_defattr_group);
 }
@@ -567,7 +563,7 @@ static int w1_attach_slave_device(struct w1_master *dev, struct w1_reg_num *rn)
 	return 0;
 }
 
-static void w1_slave_detach(struct w1_slave *sl)
+void w1_slave_detach(struct w1_slave *sl)
 {
 	struct w1_netlink_msg msg;
 
@@ -591,24 +587,6 @@ static void w1_slave_detach(struct w1_slave *sl)
 	kfree(sl);
 }
 
-static struct w1_master *w1_search_master(void *data)
-{
-	struct w1_master *dev;
-	int found = 0;
-
-	mutex_lock(&w1_mlock);
-	list_for_each_entry(dev, &w1_masters, w1_master_entry) {
-		if (dev->bus_master->data == data) {
-			found = 1;
-			atomic_inc(&dev->refcnt);
-			break;
-		}
-	}
-	mutex_unlock(&w1_mlock);
-
-	return (found)?dev:NULL;
-}
-
 struct w1_master *w1_search_master_id(u32 id)
 {
 	struct w1_master *dev;
@@ -656,34 +634,48 @@ struct w1_slave *w1_search_slave(struct w1_reg_num *id)
 	return (found)?sl:NULL;
 }
 
-void w1_reconnect_slaves(struct w1_family *f)
+void w1_reconnect_slaves(struct w1_family *f, int attach)
 {
+	struct w1_slave *sl, *sln;
 	struct w1_master *dev;
 
 	mutex_lock(&w1_mlock);
 	list_for_each_entry(dev, &w1_masters, w1_master_entry) {
-		dev_dbg(&dev->dev, "Reconnecting slaves in %s into new family %02x.\n",
-				dev->name, f->fid);
-		set_bit(W1_MASTER_NEED_RECONNECT, &dev->flags);
+		dev_dbg(&dev->dev, "Reconnecting slaves in device %s for family %02x.\n",
+			dev->name, f->fid);
+		mutex_lock(&dev->mutex);
+		list_for_each_entry_safe(sl, sln, &dev->slist, w1_slave_entry) {
+			/* If it is a new family, slaves with the default
+			 * family driver and are that family will be
+			 * connected.  If the family is going away, devices
+			 * matching that family are reconneced.
+			 */
+			if((attach && sl->family->fid == W1_FAMILY_DEFAULT
+				&& sl->reg_num.family == f->fid) ||
+				(!attach && sl->family->fid == f->fid)) {
+				struct w1_reg_num rn;
+
+				memcpy(&rn, &sl->reg_num, sizeof(rn));
+				w1_slave_detach(sl);
+
+				w1_attach_slave_device(dev, &rn);
+			}
+		}
+		dev_dbg(&dev->dev, "Reconnecting slaves in device %s has been finished.\n", dev->name);
+		mutex_unlock(&dev->mutex);
 	}
 	mutex_unlock(&w1_mlock);
 }
 
-static void w1_slave_found(void *data, u64 rn)
+static void w1_slave_found(struct w1_master *dev, u64 rn)
 {
 	int slave_count;
 	struct w1_slave *sl;
 	struct list_head *ent;
 	struct w1_reg_num *tmp;
-	struct w1_master *dev;
 	u64 rn_le = cpu_to_le64(rn);
 
-	dev = w1_search_master(data);
-	if (!dev) {
-		printk(KERN_ERR "Failed to find w1 master device for data %p, "
-		       "it is impossible.\n", data);
-		return;
-	}
+	atomic_inc(&dev->refcnt);
 
 	tmp = (struct w1_reg_num *) &rn;
 
@@ -785,76 +777,11 @@ void w1_search(struct w1_master *dev, u8 search_type, w1_slave_found_callback cb
 			if ( (desc_bit == last_zero) || (last_zero < 0))
 				last_device = 1;
 			desc_bit = last_zero;
-			cb(dev->bus_master->data, rn);
+			cb(dev, rn);
 		}
 	}
 }
 
-static int w1_control(void *data)
-{
-	struct w1_slave *sl, *sln;
-	struct w1_master *dev, *n;
-	int have_to_wait = 0;
-
-	set_freezable();
-	while (!kthread_should_stop() || have_to_wait) {
-		have_to_wait = 0;
-
-		try_to_freeze();
-		msleep_interruptible(w1_control_timeout * 1000);
-
-		list_for_each_entry_safe(dev, n, &w1_masters, w1_master_entry) {
-			if (!kthread_should_stop() && !dev->flags)
-				continue;
-			/*
-			 * Little race: we can create thread but not set the flag.
-			 * Get a chance for external process to set flag up.
-			 */
-			if (!dev->initialized) {
-				have_to_wait = 1;
-				continue;
-			}
-
-			if (kthread_should_stop() || test_bit(W1_MASTER_NEED_EXIT, &dev->flags)) {
-				set_bit(W1_MASTER_NEED_EXIT, &dev->flags);
-
-				mutex_lock(&w1_mlock);
-				list_del(&dev->w1_master_entry);
-				mutex_unlock(&w1_mlock);
-
-				mutex_lock(&dev->mutex);
-				list_for_each_entry_safe(sl, sln, &dev->slist, w1_slave_entry) {
-					w1_slave_detach(sl);
-				}
-				w1_destroy_master_attributes(dev);
-				mutex_unlock(&dev->mutex);
-				atomic_dec(&dev->refcnt);
-				continue;
-			}
-
-			if (test_bit(W1_MASTER_NEED_RECONNECT, &dev->flags)) {
-				dev_dbg(&dev->dev, "Reconnecting slaves in device %s.\n", dev->name);
-				mutex_lock(&dev->mutex);
-				list_for_each_entry_safe(sl, sln, &dev->slist, w1_slave_entry) {
-					if (sl->family->fid == W1_FAMILY_DEFAULT) {
-						struct w1_reg_num rn;
-
-						memcpy(&rn, &sl->reg_num, sizeof(rn));
-						w1_slave_detach(sl);
-
-						w1_attach_slave_device(dev, &rn);
-					}
-				}
-				dev_dbg(&dev->dev, "Reconnecting slaves in device %s has been finished.\n", dev->name);
-				clear_bit(W1_MASTER_NEED_RECONNECT, &dev->flags);
-				mutex_unlock(&dev->mutex);
-			}
-		}
-	}
-
-	return 0;
-}
-
 void w1_search_process(struct w1_master *dev, u8 search_type)
 {
 	struct w1_slave *sl, *sln;
@@ -932,18 +859,13 @@ static int w1_init(void)
 		goto err_out_master_unregister;
 	}
 
-	w1_control_thread = kthread_run(w1_control, NULL, "w1_control");
-	if (IS_ERR(w1_control_thread)) {
-		retval = PTR_ERR(w1_control_thread);
-		printk(KERN_ERR "Failed to create control thread. err=%d\n",
-			retval);
-		goto err_out_slave_unregister;
-	}
-
 	return 0;
 
+#if 0
+/* For undoing the slave register if there was a step after it. */
 err_out_slave_unregister:
 	driver_unregister(&w1_slave_driver);
+#endif
 
 err_out_master_unregister:
 	driver_unregister(&w1_master_driver);
@@ -959,13 +881,12 @@ static void w1_fini(void)
 {
 	struct w1_master *dev;
 
+	/* Set netlink removal messages and some cleanup */
 	list_for_each_entry(dev, &w1_masters, w1_master_entry)
 		__w1_remove_master_device(dev);
 
 	w1_fini_netlink();
 
-	kthread_stop(w1_control_thread);
-
 	driver_unregister(&w1_slave_driver);
 	driver_unregister(&w1_master_driver);
 	bus_unregister(&w1_bus_type);
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index f1df534..1533cb3 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -77,7 +77,7 @@ struct w1_slave
 	struct completion	released;
 };
 
-typedef void (* w1_slave_found_callback)(void *, u64);
+typedef void (* w1_slave_found_callback)(struct w1_master *, u64);
 
 
 /**
@@ -142,12 +142,14 @@ struct w1_bus_master
 	 */
 	u8		(*reset_bus)(void *);
 
-	/** Really nice hardware can handles the different types of ROM search */
-	void		(*search)(void *, u8, w1_slave_found_callback);
+	/** Really nice hardware can handles the different types of ROM search
+	 *  w1_master* is passed to the slave found callback.
+	 */
+	void		(*search)(void *, struct w1_master *,
+		u8, w1_slave_found_callback);
 };
 
 #define W1_MASTER_NEED_EXIT		0
-#define W1_MASTER_NEED_RECONNECT	1
 
 struct w1_master
 {
@@ -181,12 +183,21 @@ struct w1_master
 };
 
 int w1_create_master_attributes(struct w1_master *);
+void w1_destroy_master_attributes(struct w1_master *master);
 void w1_search(struct w1_master *dev, u8 search_type, w1_slave_found_callback cb);
 void w1_search_devices(struct w1_master *dev, u8 search_type, w1_slave_found_callback cb);
 struct w1_slave *w1_search_slave(struct w1_reg_num *id);
 void w1_search_process(struct w1_master *dev, u8 search_type);
 struct w1_master *w1_search_master_id(u32 id);
 
+/* Disconnect and reconnect devices in the given family.  Used for finding
+ * unclaimed devices after a family has been registered or releasing devices
+ * after a family has been unregistered.  Set attach to 1 when a new family
+ * has just been registered, to 0 when it has been unregistered.
+ */
+void w1_reconnect_slaves(struct w1_family *f, int attach);
+void w1_slave_detach(struct w1_slave *sl);
+
 u8 w1_triplet(struct w1_master *dev, int bdir);
 void w1_write_8(struct w1_master *, u8);
 int w1_reset_bus(struct w1_master *);
diff --git a/drivers/w1/w1_family.c b/drivers/w1/w1_family.c
index a3c95bd..8c35f9c 100644
--- a/drivers/w1/w1_family.c
+++ b/drivers/w1/w1_family.c
@@ -53,7 +53,8 @@ int w1_register_family(struct w1_family *newf)
 	}
 	spin_unlock(&w1_flock);
 
-	w1_reconnect_slaves(newf);
+	/* check default devices against the new set of drivers */
+	w1_reconnect_slaves(newf, 1);
 
 	return ret;
 }
@@ -77,6 +78,9 @@ void w1_unregister_family(struct w1_family *fent)
 
 	spin_unlock(&w1_flock);
 
+	/* deatch devices using this family code */
+	w1_reconnect_slaves(fent, 0);
+
 	while (atomic_read(&fent->refcnt)) {
 		printk(KERN_INFO "Waiting for family %u to become free: refcnt=%d.\n",
 				fent->fid, atomic_read(&fent->refcnt));
diff --git a/drivers/w1/w1_family.h b/drivers/w1/w1_family.h
index ef1e1da..296a87e 100644
--- a/drivers/w1/w1_family.h
+++ b/drivers/w1/w1_family.h
@@ -63,6 +63,5 @@ void __w1_family_get(struct w1_family *);
 struct w1_family * w1_family_registered(u8);
 void w1_unregister_family(struct w1_family *);
 int w1_register_family(struct w1_family *);
-void w1_reconnect_slaves(struct w1_family *f);
 
 #endif /* __W1_FAMILY_H */
diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 6840dfe..15a798f 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -148,10 +148,23 @@ err_out_free_dev:
 void __w1_remove_master_device(struct w1_master *dev)
 {
 	struct w1_netlink_msg msg;
+	struct w1_slave *sl, *sln;
 
 	set_bit(W1_MASTER_NEED_EXIT, &dev->flags);
 	kthread_stop(dev->thread);
 
+	mutex_lock(&w1_mlock);
+	list_del(&dev->w1_master_entry);
+	mutex_unlock(&w1_mlock);
+
+	mutex_lock(&dev->mutex);
+	list_for_each_entry_safe(sl, sln, &dev->slist, w1_slave_entry) {
+		w1_slave_detach(sl);
+	}
+	w1_destroy_master_attributes(dev);
+	mutex_unlock(&dev->mutex);
+	atomic_dec(&dev->refcnt);
+
 	while (atomic_read(&dev->refcnt)) {
 		dev_info(&dev->dev, "Waiting for %s to become free: refcnt=%d.\n",
 				dev->name, atomic_read(&dev->refcnt));
diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c
index 30b6fbf..0056ef6 100644
--- a/drivers/w1/w1_io.c
+++ b/drivers/w1/w1_io.c
@@ -277,7 +277,8 @@ void w1_search_devices(struct w1_master *dev, u8 search_type, w1_slave_found_cal
 {
 	dev->attempts++;
 	if (dev->bus_master->search)
-		dev->bus_master->search(dev->bus_master->data, search_type, cb);
+		dev->bus_master->search(dev->bus_master->data, dev,
+			search_type, cb);
 	else
 		w1_search(dev, search_type, cb);
 }
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 2/35] W1: abort search early on on exit

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

w1.c 1.5
Early abort if the master driver or the hardware goes away in the
middle of a bus search operation.  The alternative is to spam the
print buffer up to 64*64 times with read errors in the case of USB.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 2d11fe7..6fb4110 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -771,6 +771,11 @@ void w1_search(struct w1_master *dev, u8 search_type, w1_slave_found_callback cb
 			/* extract the direction taken & update the device number */
 			tmp64 = (triplet_ret >> 2);
 			rn |= (tmp64 << i);
+
+			if(test_bit(W1_MASTER_NEED_EXIT, &dev->flags)) {
+				printk(KERN_INFO "Abort w1_search (exiting)\n");
+				return;
+			}
 		}
 
 		if ( (triplet_ret & 0x03) != 0x03 ) {
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 3/35] W1: don't delay search start

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

w1.c 1.6
w1_int.c 1.4
Move the creation of the w1_process thread to after the device has
been initialized.  This way w1_process doesn't have to check to see if
it has been initialized and the bus search can proceed without
sleeping.  That also eliminates two checks in the w1_process loop.
The sleep now happens at the end of the loop not the beginning.

Also added a comment for why the atomic_set was 2.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c     |   19 ++++++-------------
 drivers/w1/w1_int.c |   26 +++++++++++++++++---------
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 6fb4110..9a09f3a 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -812,21 +812,14 @@ int w1_process(void *data)
 	struct w1_master *dev = (struct w1_master *) data;
 
 	while (!kthread_should_stop() && !test_bit(W1_MASTER_NEED_EXIT, &dev->flags)) {
+		if(dev->search_count) {
+			mutex_lock(&dev->mutex);
+			w1_search_process(dev, W1_SEARCH);
+			mutex_unlock(&dev->mutex);
+		}
+
 		try_to_freeze();
 		msleep_interruptible(w1_timeout * 1000);
-
-		if (kthread_should_stop() || test_bit(W1_MASTER_NEED_EXIT, &dev->flags))
-			break;
-
-		if (!dev->initialized)
-			continue;
-
-		if (dev->search_count == 0)
-			continue;
-
-		mutex_lock(&dev->mutex);
-		w1_search_process(dev, W1_SEARCH);
-		mutex_unlock(&dev->mutex);
 	}
 
 	atomic_dec(&dev->refcnt);
diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 15a798f..bb6cbbe 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -61,6 +61,9 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 	dev->slave_ttl		= slave_ttl;
         dev->search_count	= -1; /* continual scan */
 
+	/* 1 for w1_process to decrement
+	 * 1 for __w1_remove_master_device to decrement
+	 */
 	atomic_set(&dev->refcnt, 2);
 
 	INIT_LIST_HEAD(&dev->slist);
@@ -109,23 +112,23 @@ int w1_add_master_device(struct w1_bus_master *master)
 	if (!dev)
 		return -ENOMEM;
 
+	retval =  w1_create_master_attributes(dev);
+	if (retval)
+		goto err_out_free_dev;
+
+	memcpy(dev->bus_master, master, sizeof(struct w1_bus_master));
+
+	dev->initialized = 1;
+
 	dev->thread = kthread_run(&w1_process, dev, "%s", dev->name);
 	if (IS_ERR(dev->thread)) {
 		retval = PTR_ERR(dev->thread);
 		dev_err(&dev->dev,
 			 "Failed to create new kernel thread. err=%d\n",
 			 retval);
-		goto err_out_free_dev;
+		goto err_out_rm_attr;
 	}
 
-	retval =  w1_create_master_attributes(dev);
-	if (retval)
-		goto err_out_kill_thread;
-
-	memcpy(dev->bus_master, master, sizeof(struct w1_bus_master));
-
-	dev->initialized = 1;
-
 	mutex_lock(&w1_mlock);
 	list_add(&dev->w1_master_entry, &w1_masters);
 	mutex_unlock(&w1_mlock);
@@ -137,8 +140,13 @@ int w1_add_master_device(struct w1_bus_master *master)
 
 	return 0;
 
+#if 0 /* Thread cleanup code, not required currently. */
 err_out_kill_thread:
+	set_bit(W1_MASTER_NEED_EXIT, &dev->flags);
 	kthread_stop(dev->thread);
+#endif
+err_out_rm_attr:
+	w1_destroy_master_attributes(dev);
 err_out_free_dev:
 	w1_free_dev(dev);
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 4/35] W1: w1_process, allow wakeup in sleep

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

w1.c 1.7 1.8
msleep_interruptible only wakes for signals, but we don't use them.
We want kthread_stop to wake it up, so call schedule_timeout
ourselves.  This is prep-work for blocking when there isn't anything            to do, instead of sleeping in a loop, as something has to be able to
wake it up.
Added a check for termination after the bus search, otherwise it would
sleep for the full time value if kthread_stop was called while
w1_process was in the bus search.  This would cause a deadlock when it
is blocking instead of sleeping with a timeout.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 9a09f3a..ad89d85 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -810,6 +810,10 @@ void w1_search_process(struct w1_master *dev, u8 search_type)
 int w1_process(void *data)
 {
 	struct w1_master *dev = (struct w1_master *) data;
+	/* As long as w1_timeout is only set by a module parameter the sleep
+	 * time can be calculated in jiffies once.
+	 */
+	const unsigned long jtime=msecs_to_jiffies(w1_timeout * 1000);
 
 	while (!kthread_should_stop() && !test_bit(W1_MASTER_NEED_EXIT, &dev->flags)) {
 		if(dev->search_count) {
@@ -819,7 +823,12 @@ int w1_process(void *data)
 		}
 
 		try_to_freeze();
-		msleep_interruptible(w1_timeout * 1000);
+		__set_current_state(TASK_INTERRUPTIBLE);
+
+		if(kthread_should_stop() || test_bit(W1_MASTER_NEED_EXIT, &dev->flags))
+			break;
+
+		schedule_timeout(jtime);
 	}
 
 	atomic_dec(&dev->refcnt);
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 5/35] W1: w1_process, remove W1_MASTER_NEED_EXIT, it is redundant

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

w1.c 1.9
w1.h 1.7
w1_int.c 1.5
Removed W1_MASTER_NEED_EXIT along with the flags variable it used as
they were redundant.  The kthread_stop() and kthread_should_stop() do
the same thing and were already being used and checked.  I also think
there was a race condition in the current implemention.  If
W1_MASTER_NEED_EXIT was set, the w1_process woke up, then exited
because that flag was set before the remove master thread called
kthread_stop, kthread_stop would never return.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c     |    6 +++---
 drivers/w1/w1.h     |    4 ----
 drivers/w1/w1_int.c |    2 --
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index ad89d85..7dd34f2 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -772,7 +772,7 @@ void w1_search(struct w1_master *dev, u8 search_type, w1_slave_found_callback cb
 			tmp64 = (triplet_ret >> 2);
 			rn |= (tmp64 << i);
 
-			if(test_bit(W1_MASTER_NEED_EXIT, &dev->flags)) {
+			if(kthread_should_stop()) {
 				printk(KERN_INFO "Abort w1_search (exiting)\n");
 				return;
 			}
@@ -815,7 +815,7 @@ int w1_process(void *data)
 	 */
 	const unsigned long jtime=msecs_to_jiffies(w1_timeout * 1000);
 
-	while (!kthread_should_stop() && !test_bit(W1_MASTER_NEED_EXIT, &dev->flags)) {
+	while (!kthread_should_stop()) {
 		if(dev->search_count) {
 			mutex_lock(&dev->mutex);
 			w1_search_process(dev, W1_SEARCH);
@@ -825,7 +825,7 @@ int w1_process(void *data)
 		try_to_freeze();
 		__set_current_state(TASK_INTERRUPTIBLE);
 
-		if(kthread_should_stop() || test_bit(W1_MASTER_NEED_EXIT, &dev->flags))
+		if(kthread_should_stop())
 			break;
 
 		schedule_timeout(jtime);
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index 1533cb3..c47ca1e 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -149,8 +149,6 @@ struct w1_bus_master
 		u8, w1_slave_found_callback);
 };
 
-#define W1_MASTER_NEED_EXIT		0
-
 struct w1_master
 {
 	struct list_head	w1_master_entry;
@@ -169,8 +167,6 @@ struct w1_master
 	void			*priv;
 	int			priv_size;
 
-	long			flags;
-
 	struct task_struct	*thread;
 	struct mutex		mutex;
 
diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index bb6cbbe..3388d8f 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -142,7 +142,6 @@ int w1_add_master_device(struct w1_bus_master *master)
 
 #if 0 /* Thread cleanup code, not required currently. */
 err_out_kill_thread:
-	set_bit(W1_MASTER_NEED_EXIT, &dev->flags);
 	kthread_stop(dev->thread);
 #endif
 err_out_rm_attr:
@@ -158,7 +157,6 @@ void __w1_remove_master_device(struct w1_master *dev)
 	struct w1_netlink_msg msg;
 	struct w1_slave *sl, *sln;
 
-	set_bit(W1_MASTER_NEED_EXIT, &dev->flags);
 	kthread_stop(dev->thread);
 
 	mutex_lock(&w1_mlock);
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 6/35] W1: w1_process, block when there's nothing to do

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

w1.c 1.10
Instead of sleeping with a timeout, it will now sleep with a timeout
only when there is an active bus search.  When the search is not
active it will block until something wakes it up.  kthread_stop and
changing the search_count will wake up the thread.  Now if the search
value is changed (to request a new search) it will wake up and do the
search immediately, rather than waiting for the end of the current
timeout.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 7dd34f2..5d8c2c7 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -251,6 +251,7 @@ static ssize_t w1_master_attribute_store_search(struct device * dev,
 	mutex_lock(&md->mutex);
 	md->search_count = simple_strtol(buf, NULL, 0);
 	mutex_unlock(&md->mutex);
+	wake_up_process(md->thread);
 
 	return count;
 }
@@ -828,7 +829,11 @@ int w1_process(void *data)
 		if(kthread_should_stop())
 			break;
 
-		schedule_timeout(jtime);
+		/* Only sleep when the search is active. */
+		if(dev->search_count)
+			schedule_timeout(jtime);
+		else
+			schedule();
 	}
 
 	atomic_dec(&dev->refcnt);
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 7/35] W1: feature, enable hardware strong pullup

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 7432 bytes --]

Add a strong pullup option to the w1 system.  This supplies extra
power for parasite powered devices.

The one wire bus requires at a minimum one wire and ground.  The
common wire is used for sending and receiving data as well as
supplying power to devices that are parasite powered.  Temperature
sensors are one device that can be parasite powered and require no bus
transactions while the temperature conversion is progress.  A bus
transaction would pull the common wire down.  In addition to the
normal pullup resistor, which allows a device to pull the bus line
down to send data, some devices support a strong pullup mechanism to
deliver more power.  The documentation lists this as useful for larger
networks and when temperature sensors are exposed to higher
temperatures they can exceed the current supplied by a simple pullup
resistor.  This patch adds a mechanism to support a strong pullup, and
fall back to sleeping with the line high otherwise.  This patch
doesn't enable the strong pullup for any master or slave device, those
will follow as they require this patch.

The hardware USB 2490 one wire bus master has a bit on a few commands
which will enable the strong pullup as soon as the command finishes
executing.  That requires the ds2490 driver to be notified if it
should be set before the command is executed.  That's why I added the
w1_next_pullup.  That will call the master set_pullup just before the
next write, and reset the value to zero after it has completed.  If
the master driver doesn't support the set_pullup, it will delay the
appropriate amount of time after the write returns.

w1.h 1.2 1.3 1.4
Add set_pullup to w1_bus_master, and add pullup_duration to w1_master
to hold the strong pullup duration for the next write operation.

w1_int.c 1.3
Add a check to disable set_pullup and print a warning message when the
operation isn't supported.

w1_io.c 1.2 1.3 1.4 1.6
Add w1_next_pullup, call w1_pre_write and w1_post_write to call
set_pullup before and after, or just sleep after if set_pullup isn't
supported.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.h     |   10 +++++++
 drivers/w1/w1_int.c |   12 +++++++++
 drivers/w1/w1_io.c  |   66 +++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index c47ca1e..bad7c7c 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -142,6 +142,12 @@ struct w1_bus_master
 	 */
 	u8		(*reset_bus)(void *);
 
+	/**
+	 * Put out a strong pull-up pulse of the specified duration.
+	 * @return -1=Error, 0=completed
+	 */
+	u8		(*set_pullup)(void *, int);
+
 	/** Really nice hardware can handles the different types of ROM search
 	 *  w1_master* is passed to the slave found callback.
 	 */
@@ -167,6 +173,9 @@ struct w1_master
 	void			*priv;
 	int			priv_size;
 
+	/** Strong pullup duration in milliseconds, zero disabled. */
+	int			pullup_duration;
+
 	struct task_struct	*thread;
 	struct mutex		mutex;
 
@@ -201,6 +210,7 @@ u8 w1_calc_crc8(u8 *, int);
 void w1_write_block(struct w1_master *, const u8 *, int);
 u8 w1_read_block(struct w1_master *, u8 *, int);
 int w1_reset_select_slave(struct w1_slave *sl);
+void w1_next_pullup(struct w1_master *, int);
 
 static inline struct w1_slave* dev_to_w1_slave(struct device *dev)
 {
diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 3388d8f..932a242 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -107,6 +107,18 @@ int w1_add_master_device(struct w1_bus_master *master)
 		printk(KERN_ERR "w1_add_master_device: invalid function set\n");
 		return(-EINVAL);
         }
+	/* While it would be electrically possible to make a device that
+	 * generated a strong pullup in bit bang mode, only hardare that
+	 * controls 1-wire time frames are even expected to support a strong
+	 * pullup.  w1_io.c would need to support calling set_pullup before
+	 * the last write_bit operation of a w1_write_8 which it currently
+	 * doesn't.
+	 */
+	if(!master->write_byte && !master->touch_bit && master->set_pullup) {
+		printk(KERN_ERR "w1_add_master_device: set_pullup requires "
+			"write_byte or touch_bit, disabling\n");
+		master->set_pullup=NULL;
+	}
 
 	dev = w1_alloc_dev(w1_ids++, w1_max_slave_count, w1_max_slave_ttl, &w1_master_driver, &w1_master_device);
 	if (!dev)
diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c
index 0056ef6..46c6994 100644
--- a/drivers/w1/w1_io.c
+++ b/drivers/w1/w1_io.c
@@ -93,6 +93,38 @@ static void w1_write_bit(struct w1_master *dev, int bit)
 }
 
 /**
+ * Pre-write operation, currently only supporting strong pullups.
+ * Program the hardware for a strong pullup, if one has been requested and
+ * the hardware supports it.
+ *
+ * @param dev     the master device
+ */
+static void w1_pre_write(struct w1_master *dev)
+{
+	if(dev->pullup_duration && dev->bus_master->set_pullup)
+		dev->bus_master->set_pullup(dev->bus_master->data,
+			dev->pullup_duration);
+}
+
+/**
+ * Post-write operation, currently only supporting strong pullups.
+ * If a strong pullup was requested, clear it if the hardware supports
+ * them, or execute the delay otherwise, in either case clear the request.
+ *
+ * @param dev     the master device
+ */
+static void w1_post_write(struct w1_master *dev)
+{
+	if(dev->pullup_duration) {
+		if(dev->bus_master->set_pullup)
+			dev->bus_master->set_pullup(dev->bus_master->data, 0);
+		else
+			msleep(dev->pullup_duration);
+		dev->pullup_duration=0;
+	}
+}
+
+/**
  * Writes 8 bits.
  *
  * @param dev     the master device
@@ -102,11 +134,17 @@ void w1_write_8(struct w1_master *dev, u8 byte)
 {
 	int i;
 
-	if (dev->bus_master->write_byte)
+	if (dev->bus_master->write_byte) {
+		w1_pre_write(dev);
 		dev->bus_master->write_byte(dev->bus_master->data, byte);
+	}
 	else
-		for (i = 0; i < 8; ++i)
+		for (i = 0; i < 8; ++i) {
+			if(i==7)
+				w1_pre_write(dev);
 			w1_touch_bit(dev, (byte >> i) & 0x1);
+		}
+	w1_post_write(dev);
 }
 EXPORT_SYMBOL_GPL(w1_write_8);
 
@@ -203,11 +241,14 @@ void w1_write_block(struct w1_master *dev, const u8 *buf, int len)
 {
 	int i;
 
-	if (dev->bus_master->write_block)
+	if (dev->bus_master->write_block) {
+		w1_pre_write(dev);
 		dev->bus_master->write_block(dev->bus_master->data, buf, len);
+	}
 	else
 		for (i = 0; i < len; ++i)
-			w1_write_8(dev, buf[i]);
+			w1_write_8(dev, buf[i]); // calls w1_pre_write
+	w1_post_write(dev);
 }
 EXPORT_SYMBOL_GPL(w1_write_block);
 
@@ -306,3 +347,20 @@ int w1_reset_select_slave(struct w1_slave *sl)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(w1_reset_select_slave);
+
+/**
+ * Put out a strong pull-up of the specified duration after the next write
+ * operation.  Not all hardware supports strong pullups.  Hardware that
+ * doesn't support strong pullups will sleep for the given time after the
+ * write operation without a strong pullup.  This is a one shot request for
+ * the next write, specifying zero will clear a previous request.
+ * The w1 master lock must be held.
+ *
+ * @param delay	time in milliseconds
+ * @return	0=success, anything else=error
+ */
+void w1_next_pullup(struct w1_master *dev, int delay)
+{
+	dev->pullup_duration=delay;
+}
+EXPORT_SYMBOL_GPL(w1_next_pullup);
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 8/35] W1: feature, w1_therm.c use strong pullup

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

w1_therm.c 1.3 1.4 1.10
Use the strong pullup not the sleep.  Note that the pullup goes before
W1_CONVERT_TEMP not after.
+w1_next_pullup(dev, tm);
 w1_write_8(dev, W1_CONVERT_TEMP);
-msleep(tm);

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/slaves/w1_therm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index fb28aca..dd26db2 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -192,10 +192,10 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj,
 			int count = 0;
 			unsigned int tm = 750;
 
+			/* 750ms strong pullup (or delay) after the convert */
+			w1_next_pullup(dev, tm);
 			w1_write_8(dev, W1_CONVERT_TEMP);
 
-			msleep(tm);
-
 			if (!w1_reset_select_slave(sl)) {
 
 				w1_write_8(dev, W1_READ_SCRATCHPAD);
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 9/35] W1: create documentation for the w1_therm slave device

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 2823 bytes --]

New directory for temperature sensor documentation.

Documentation/w1/00-INDEX 1.2
Added new subdirectory slaves

Documentation/w1/slaves/00-INDEX 1.1 1.2
Added index and w1_therm entry.

Documentation/w1/slaves/w1_therm 1.1 1.2
Initial entry for the temperature sensor.

Signed-off-by: David Fries <david@fries.net>
---
 Documentation/w1/00-INDEX        |    2 ++
 Documentation/w1/slaves/00-INDEX |    4 ++++
 Documentation/w1/slaves/w1_therm |   35 +++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/Documentation/w1/00-INDEX b/Documentation/w1/00-INDEX
index 5270cf4..cb49802 100644
--- a/Documentation/w1/00-INDEX
+++ b/Documentation/w1/00-INDEX
@@ -1,5 +1,7 @@
 00-INDEX
 	- This file
+slaves/
+	- Drivers that provide support for specific family codes.
 masters/
 	- Individual chips providing 1-wire busses.
 w1.generic
diff --git a/Documentation/w1/slaves/00-INDEX b/Documentation/w1/slaves/00-INDEX
new file mode 100644
index 0000000..f8101d6
--- /dev/null
+++ b/Documentation/w1/slaves/00-INDEX
@@ -0,0 +1,4 @@
+00-INDEX
+	- This file
+w1_therm
+	- The Maxim/Dallas Semiconductor ds18*20 temperature sensor.
diff --git a/Documentation/w1/slaves/w1_therm b/Documentation/w1/slaves/w1_therm
new file mode 100644
index 0000000..7b045ae
--- /dev/null
+++ b/Documentation/w1/slaves/w1_therm
@@ -0,0 +1,35 @@
+Kernel driver w1_therm
+====================
+
+Supported chips:
+  * Maxim ds18*20 based temperature sensors.
+
+Author: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
+
+
+Description
+-----------
+
+w1_therm provides basic temperature conversion for ds18*20 devices.
+supported family codes:
+W1_THERM_DS18S20	0x10
+W1_THERM_DS1822		0x22
+W1_THERM_DS18B20	0x28
+Support is provided through the sysfs w1_slave file.  Each open and
+read sequence will initiate a temperature conversion then provide two
+lines of ASCII output.  The first line contains the nine hex bytes
+read along with a calculated crc value and YES or NO if it matched.
+If the crc matched the returned values are retained.  The second line
+displays the retained values along with a temperature in millidegrees
+Centigrade after t=.
+
+w1_therm selects the current slave device for temperature conversion
+and provides a strong pullup if the master devices supports one and
+delays 750ms for the conversion.
+
+Parasite powered devices are limited to one slave performing a
+temperature conversion at a time.  If none of the devices are parasite
+powered it would be possible to convert all the devices at the same
+time and then go back to read individual sensors.  That isn't
+currently supported.  The driver also doesn't support reduced
+precision (which would also reduce the conversion time).
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 10/35] W1: be able to manually add and remove slaves

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 6147 bytes --]

w1.c 1.11 1.12 1.14
I'm intending to be working with a static network and run without the
bus searching, but bus searching is currently the only way to add
slave devices.  This allows any slave to be added or removed.  There
is one file for adding and another for removing.  If the files are
read a short comment including the expected serial number format is
returned.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 137 insertions(+), 3 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 5d8c2c7..3ca1f4d 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -56,6 +56,9 @@ module_param_named(slave_ttl, w1_max_slave_ttl, int, 0);
 DEFINE_MUTEX(w1_mlock);
 LIST_HEAD(w1_masters);
 
+static int w1_attach_slave_device(struct w1_master *dev, struct w1_reg_num *rn);
+void w1_slave_detach(struct w1_slave *sl);
+
 static int w1_master_match(struct device *dev, struct device_driver *drv)
 {
 	return 1;
@@ -321,7 +324,8 @@ static ssize_t w1_master_attribute_show_slave_count(struct device *dev, struct d
 	return count;
 }
 
-static ssize_t w1_master_attribute_show_slaves(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t w1_master_attribute_show_slaves(struct device *dev,
+	struct device_attribute *attr, char *buf)
 {
 	struct w1_master *md = dev_to_w1_master(dev);
 	int c = PAGE_SIZE;
@@ -346,6 +350,134 @@ static ssize_t w1_master_attribute_show_slaves(struct device *dev, struct device
 	return PAGE_SIZE - c;
 }
 
+static ssize_t w1_master_attribute_show_add(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	int c = PAGE_SIZE;
+	c -= snprintf(buf+PAGE_SIZE - c, c,
+		"write device id xx-xxxxxxxxxxxx to add slave\n");
+	return PAGE_SIZE - c;
+}
+
+static int w1_atoreg_num(struct device * dev, const char *buf, size_t count,
+	struct w1_reg_num *rn)
+{
+	unsigned int family;
+	u64 id;
+	int i;
+	u64 rn64_le;
+	/* The CRC value isn't read from the user because the sysfs directory
+	 * doesn't include it and most messages from the bus search don't
+	 * print it either.  It would be unreasonable for the user to then
+	 * provide it.
+	 */
+	const char *error_msg="bad slave string format, expecting "
+		"ff-dddddddddddd\n";
+
+	if(buf[2]!='-') {
+		dev_err(dev, "%s", error_msg);
+		return -EINVAL;
+	}
+	i=sscanf(buf, "%02x-%012llx", &family, &id);
+	if(i!=2) {
+		dev_err(dev, "%s", error_msg);
+		return -EINVAL;
+	}
+	rn->family=family;
+	rn->id=id;
+
+	rn64_le=cpu_to_le64(*(u64*)rn);
+	rn->crc=w1_calc_crc8((u8 *)&rn64_le, 7);
+
+	#if 0
+	dev_info(dev, "With CRC device is %02x.%012llx.%02x.\n",
+		  rn->family, (unsigned long long)rn->id, rn->crc);
+	#endif
+
+	return 0;
+}
+
+/* Searches the slaves in the w1_master and returns a pointer or NULL.
+ * Note: must hold the mutex
+ */
+static struct w1_slave *w1_slave_search_device(struct w1_master *dev,
+	struct w1_reg_num *rn)
+{
+	struct w1_slave *sl;
+	list_for_each_entry(sl, &dev->slist, w1_slave_entry) {
+		if (sl->reg_num.family == rn->family &&
+				sl->reg_num.id == rn->id &&
+				sl->reg_num.crc == rn->crc) {
+			return sl;
+		}
+	}
+	return NULL;
+}
+
+static ssize_t w1_master_attribute_store_add(struct device * dev,
+						struct device_attribute *attr,
+						const char * buf, size_t count)
+{
+	struct w1_master *md = dev_to_w1_master(dev);
+	struct w1_reg_num rn;
+	struct w1_slave *sl;
+	ssize_t result=count;
+
+	if(w1_atoreg_num(dev, buf, count, &rn))
+		return -EINVAL;
+
+	mutex_lock(&md->mutex);
+	sl=w1_slave_search_device(md, &rn);
+	/* It would be nice to do a targeted search one the one-wire bus
+	 * for the new device to see if it is out there or not.  But the
+	 * current search doesn't support that.
+	 */
+	if(sl) {
+		dev_info(dev, "Device %s already exists\n", sl->name);
+		result=-EINVAL;
+	} else {
+		w1_attach_slave_device(md, &rn);
+	}
+	mutex_unlock(&md->mutex);
+
+	return result;
+}
+
+static ssize_t w1_master_attribute_show_remove(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	int c = PAGE_SIZE;
+	c -= snprintf(buf+PAGE_SIZE - c, c,
+		"write device id xx-xxxxxxxxxxxx to remove slave\n");
+	return PAGE_SIZE - c;
+}
+
+static ssize_t w1_master_attribute_store_remove(struct device * dev,
+						struct device_attribute *attr,
+						const char * buf, size_t count)
+{
+	struct w1_master *md = dev_to_w1_master(dev);
+	struct w1_reg_num rn;
+	struct w1_slave *sl;
+	ssize_t result=count;
+
+	if(w1_atoreg_num(dev, buf, count, &rn))
+		return -EINVAL;
+
+	mutex_lock(&md->mutex);
+	sl=w1_slave_search_device(md, &rn);
+	if(sl) {
+		w1_slave_detach(sl);
+	} else {
+		dev_info(dev, "Device %02x-%012llx doesn't exists\n", rn.family,
+			(unsigned long long)rn.id);
+		result=-EINVAL;
+	}
+	mutex_unlock(&md->mutex);
+
+	return result;
+}
+
 #define W1_MASTER_ATTR_RO(_name, _mode)				\
 	struct device_attribute w1_master_attribute_##_name =	\
 		__ATTR(w1_master_##_name, _mode,		\
@@ -365,6 +497,8 @@ static W1_MASTER_ATTR_RO(attempts, S_IRUGO);
 static W1_MASTER_ATTR_RO(timeout, S_IRUGO);
 static W1_MASTER_ATTR_RO(pointer, S_IRUGO);
 static W1_MASTER_ATTR_RW(search, S_IRUGO | S_IWUGO);
+static W1_MASTER_ATTR_RW(add, S_IRUGO | S_IWUGO);
+static W1_MASTER_ATTR_RW(remove, S_IRUGO | S_IWUGO);
 
 static struct attribute *w1_master_default_attrs[] = {
 	&w1_master_attribute_name.attr,
@@ -375,6 +509,8 @@ static struct attribute *w1_master_default_attrs[] = {
 	&w1_master_attribute_timeout.attr,
 	&w1_master_attribute_pointer.attr,
 	&w1_master_attribute_search.attr,
+	&w1_master_attribute_add.attr,
+	&w1_master_attribute_remove.attr,
 	NULL
 };
 
@@ -670,9 +806,7 @@ void w1_reconnect_slaves(struct w1_family *f, int attach)
 
 static void w1_slave_found(struct w1_master *dev, u64 rn)
 {
-	int slave_count;
 	struct w1_slave *sl;
-	struct list_head *ent;
 	struct w1_reg_num *tmp;
 	u64 rn_le = cpu_to_le64(rn);
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 11/35] W1: recode w1_slave_found logic

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

w1.c 1.14
Rewrote the confusion logic in w1_slave_found.  The function is called
for each device detected by a bus search.  If the slave was found a
flag is set to show that it is still active.  If it wasn't found it
will be attached.

It would previously search through the slave list marking the slave
active if it was found.  It used a counter to determine if it exited
the loop early.  If it searched through all slaves in the list the crc
value was verified and then attached the device if it did.

Now it uses the new w1_attach_slave_device function to find the slave
device and mark the device active if it was found.  If the slave
wasn't found it will add the device if the crc is correct.  Much more
understandable.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c |   24 ++++++------------------
 1 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 3ca1f4d..b18eedb 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -814,24 +814,12 @@ static void w1_slave_found(struct w1_master *dev, u64 rn)
 
 	tmp = (struct w1_reg_num *) &rn;
 
-	slave_count = 0;
-	list_for_each(ent, &dev->slist) {
-
-		sl = list_entry(ent, struct w1_slave, w1_slave_entry);
-
-		if (sl->reg_num.family == tmp->family &&
-		    sl->reg_num.id == tmp->id &&
-		    sl->reg_num.crc == tmp->crc) {
-			set_bit(W1_SLAVE_ACTIVE, (long *)&sl->flags);
-			break;
-		}
-
-		slave_count++;
-	}
-
-	if (slave_count == dev->slave_count &&
-		rn && ((rn >> 56) & 0xff) == w1_calc_crc8((u8 *)&rn_le, 7)) {
-		w1_attach_slave_device(dev, tmp);
+	sl = w1_slave_search_device(dev, tmp);
+	if(sl) {
+		set_bit(W1_SLAVE_ACTIVE, (long *)&sl->flags);
+	} else {
+		if(rn && tmp->crc == w1_calc_crc8((u8 *)&rn_le, 7))
+			w1_attach_slave_device(dev, tmp);
 	}
 
 	atomic_dec(&dev->refcnt);
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 12/35] W1: new module parameter search_count

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]

w1_int.c 1.6
Added a new module parameter search_count which allows overriding the
default search count.  -1 continual, 0 disabled, N that many times.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1_int.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 932a242..f1806e2 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -30,6 +30,8 @@
 #include "w1_int.h"
 
 static u32 w1_ids = 1;
+static int w1_search_count = -1; /* Default is continual scan */
+module_param_named(search_count, w1_search_count, int, 0);
 
 static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 				       struct device_driver *driver,
@@ -59,7 +61,7 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 	dev->initialized	= 0;
 	dev->id			= id;
 	dev->slave_ttl		= slave_ttl;
-        dev->search_count	= -1; /* continual scan */
+        dev->search_count	= w1_search_count;
 
 	/* 1 for w1_process to decrement
 	 * 1 for __w1_remove_master_device to decrement
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 13/35] W1: Document add, remove, and search_count.

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]

Documentation/w1/w1.generic 1.2
Document w1_master_add, w1_master_remove, and search_count.

Signed-off-by: David Fries <david@fries.net>
---
 Documentation/w1/w1.generic |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/Documentation/w1/w1.generic b/Documentation/w1/w1.generic
index 4c6509d..98111a3 100644
--- a/Documentation/w1/w1.generic
+++ b/Documentation/w1/w1.generic
@@ -79,10 +79,12 @@ w1 master sysfs interface
 <xx-xxxxxxxxxxxxx> - a directory for a found device. The format is family-serial
 bus                - (standard) symlink to the w1 bus
 driver             - (standard) symlink to the w1 driver
+w1_master_add      - Manually register a slave device
 w1_master_attempts - the number of times a search was attempted
 w1_master_max_slave_count
                    - the maximum slaves that may be attached to a master
 w1_master_name     - the name of the device (w1_bus_masterX)
+w1_master_remove   - Manually remove a slave device
 w1_master_search   - the number of searches left to do, -1=continual (default)
 w1_master_slave_count
                    - the number of slaves found
@@ -90,7 +92,13 @@ w1_master_slaves   - the names of the slaves, one per line
 w1_master_timeout  - the delay in seconds between searches
 
 If you have a w1 bus that never changes (you don't add or remove devices),
-you can set w1_master_search to a positive value to disable searches.
+you can set the module parameter search_count to a small positive number
+for an initially small number of bus searches.  Alternatively it could be
+set to zero, then manually add the slave device serial numbers by
+w1_master_add device file.  The w1_master_add and w1_master_remove files
+generally only make sense when searching is disabled, as a search will
+redetect manually removed devices that are present and timeout manually
+added devices that aren't on the bus.
 
 
 w1 slave sysfs interface
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 14/35] W1: w1_slave_read_id multiple short read bug

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

This is more to point out the bug in w1_slave_read_id, the next patch
rewrites the routine.

w1.c 1.15
Reading at an offset other than zero, ie reading less than 8 bytes at
a time would result in reading the first bytes over and over until 8
bytes were returned.  Added the offset to the buffer.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index b18eedb..dbd9930 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -110,13 +110,13 @@ static ssize_t w1_slave_read_id(struct kobject *kobj,
 {
 	struct w1_slave *sl = kobj_to_w1_slave(kobj);
 
-	if (off > 8) {
+	if (off >= 8) {
 		count = 0;
 	} else {
 		if (off + count > 8)
 			count = 8 - off;
 
-		memcpy(buf, (u8 *)&sl->reg_num, count);
+		memcpy(buf, (u8 *)&sl->reg_num+off, count);
 	}
 
 	return count;
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 15/35] W1: w1_slave_read_id from bin_attribute to device_attribute

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

w1.c 1.16
Switching w1_slave_read_id from being a bin_attribute to a
device_attribute.  It only has to return 8 bytes per read and lets              kobject handle the buffering.  That simplifies the logic in
w1_slave_read_id.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c |   35 ++++++++++-------------------------
 1 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index dbd9930..32cfb6a 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -104,35 +104,20 @@ static ssize_t w1_slave_read_name(struct device *dev, struct device_attribute *a
 	return sprintf(buf, "%s\n", sl->name);
 }
 
-static ssize_t w1_slave_read_id(struct kobject *kobj,
-				struct bin_attribute *bin_attr,
-				char *buf, loff_t off, size_t count)
+static ssize_t w1_slave_read_id(struct device *dev,
+	struct device_attribute *attr, char *buf)
 {
-	struct w1_slave *sl = kobj_to_w1_slave(kobj);
-
-	if (off >= 8) {
-		count = 0;
-	} else {
-		if (off + count > 8)
-			count = 8 - off;
-
-		memcpy(buf, (u8 *)&sl->reg_num+off, count);
-	}
+	struct w1_slave *sl = dev_to_w1_slave(dev);
+	ssize_t count=sizeof(sl->reg_num);
 
+	memcpy(buf, (u8 *)&sl->reg_num, count);
 	return count;
 }
 
 static struct device_attribute w1_slave_attr_name =
 	__ATTR(name, S_IRUGO, w1_slave_read_name, NULL);
-
-static struct bin_attribute w1_slave_attr_bin_id = {
-      .attr = {
-              .name = "id",
-              .mode = S_IRUGO,
-      },
-      .size = 8,
-      .read = w1_slave_read_id,
-};
+static struct device_attribute w1_slave_attr_id =
+	__ATTR(id, S_IRUGO, w1_slave_read_id, NULL);
 
 /* Default family */
 
@@ -612,7 +597,7 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
 	}
 
 	/* Create "id" entry */
-	err = sysfs_create_bin_file(&sl->dev.kobj, &w1_slave_attr_bin_id);
+	err = device_create_file(&sl->dev, &w1_slave_attr_id);
 	if (err < 0) {
 		dev_err(&sl->dev,
 			"sysfs file creation for [%s] failed. err=%d\n",
@@ -634,7 +619,7 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
 	return 0;
 
 out_rem2:
-	sysfs_remove_bin_file(&sl->dev.kobj, &w1_slave_attr_bin_id);
+	device_remove_file(&sl->dev, &w1_slave_attr_id);
 out_rem1:
 	device_remove_file(&sl->dev, &w1_slave_attr_name);
 out_unreg:
@@ -716,7 +701,7 @@ void w1_slave_detach(struct w1_slave *sl)
 	msg.type = W1_SLAVE_REMOVE;
 	w1_netlink_send(sl->master, &msg);
 
-	sysfs_remove_bin_file(&sl->dev.kobj, &w1_slave_attr_bin_id);
+	device_remove_file(&sl->dev, &w1_slave_attr_id);
 	device_remove_file(&sl->dev, &w1_slave_attr_name);
 	device_unregister(&sl->dev);
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 16/35] W1: w1_therm fix user buffer overflow and cat

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 5501 bytes --]

This switches w1_therm from bin_attribute to device_attribute which
fixes a buffer overflow and some bad behavior.

slaves/w1_therm.c 1.8
Switching the sysfs read from bin_attribute to device_attribute.  The
data is far under PAGE_SIZE so the binary interface isn't required.
As the device_attribute interface will make one call to w1_therm_read per
file open and buffer, the result is, the following problems go away.

buffer overflow:
	Execute a short read on w1_slave and w1_therm_read_bin would still
	return the full string size worth of data clobbering the user space
	buffer when it returned.  Switching to device_attribute avoids the
	buffer overflow problems.  With the snprintf formatted output dealing
	with short reads without doing a conversion per read would have
	been difficult.
bad behavior:
	`cat w1_slave` would cause two temperature conversions to take place.
	Previously the code assumed W1_SLAVE_DATA_SIZE would be returned with
	each read.  It would not return 0 unless the offset was less
	than W1_SLAVE_DATA_SIZE.  The result was the first read did a
	temperature conversion, filled the buffer and returned, the
	offset in the second read would be less than
	W1_SLAVE_DATA_SIZE and also fill the buffer and return, the
	third read would finnally have a big enough offset to return 0
	and cause cat to stop.  Now w1_therm_read will be called at
	most once per open.

w1.h 1.8
w1_therm is no longer using the bin_attribute so the
W1_SLAVE_DATA_SIZE is no longer being used.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/slaves/w1_therm.c |   53 ++++++++++++++---------------------------
 drivers/w1/w1.h              |    1 -
 2 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index dd26db2..ca47421 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -42,26 +42,20 @@ static u8 bad_roms[][9] = {
 				{}
 			};
 
-static ssize_t w1_therm_read_bin(struct kobject *, struct bin_attribute *,
-				 char *, loff_t, size_t);
+static ssize_t w1_therm_read(struct device *device,
+	struct device_attribute *attr, char *buf);
 
-static struct bin_attribute w1_therm_bin_attr = {
-	.attr = {
-		.name = "w1_slave",
-		.mode = S_IRUGO,
-	},
-	.size = W1_SLAVE_DATA_SIZE,
-	.read = w1_therm_read_bin,
-};
+static struct device_attribute w1_therm_attr =
+	__ATTR(w1_slave, S_IRUGO, w1_therm_read, NULL);
 
 static int w1_therm_add_slave(struct w1_slave *sl)
 {
-	return sysfs_create_bin_file(&sl->dev.kobj, &w1_therm_bin_attr);
+	return device_create_file(&sl->dev, &w1_therm_attr);
 }
 
 static void w1_therm_remove_slave(struct w1_slave *sl)
 {
-	sysfs_remove_bin_file(&sl->dev.kobj, &w1_therm_bin_attr);
+	device_remove_file(&sl->dev, &w1_therm_attr);
 }
 
 static struct w1_family_ops w1_therm_fops = {
@@ -160,30 +154,19 @@ static int w1_therm_check_rom(u8 rom[9])
 	return 0;
 }
 
-static ssize_t w1_therm_read_bin(struct kobject *kobj,
-				 struct bin_attribute *bin_attr,
-				 char *buf, loff_t off, size_t count)
+static ssize_t w1_therm_read(struct device *device,
+	struct device_attribute *attr, char *buf)
 {
-	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	struct w1_slave *sl = dev_to_w1_slave(device);
 	struct w1_master *dev = sl->master;
 	u8 rom[9], crc, verdict;
 	int i, max_trying = 10;
+	ssize_t c=PAGE_SIZE;
 
 	mutex_lock(&sl->master->mutex);
 
-	if (off > W1_SLAVE_DATA_SIZE) {
-		count = 0;
-		goto out;
-	}
-	if (off + count > W1_SLAVE_DATA_SIZE) {
-		count = 0;
-		goto out;
-	}
-
-	memset(buf, 0, count);
 	memset(rom, 0, sizeof(rom));
 
-	count = 0;
 	verdict = 0;
 	crc = 0;
 
@@ -200,7 +183,7 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj,
 
 				w1_write_8(dev, W1_READ_SCRATCHPAD);
 				if ((count = w1_read_block(dev, rom, 9)) != 9) {
-					dev_warn(&dev->dev, "w1_read_block() returned %d instead of 9.\n", count);
+					dev_warn(device, "w1_read_block() returned %u instead of 9.\n", count);
 				}
 
 				crc = w1_calc_crc8(rom, 8);
@@ -215,22 +198,22 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj,
 	}
 
 	for (i = 0; i < 9; ++i)
-		count += sprintf(buf + count, "%02x ", rom[i]);
-	count += sprintf(buf + count, ": crc=%02x %s\n",
+		c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", rom[i]);
+	c -= snprintf(buf + PAGE_SIZE - c, c, ": crc=%02x %s\n",
 			   crc, (verdict) ? "YES" : "NO");
 	if (verdict)
 		memcpy(sl->rom, rom, sizeof(sl->rom));
 	else
-		dev_warn(&dev->dev, "18S20 doesn't respond to CONVERT_TEMP.\n");
+		dev_warn(device, "18S20 doesn't respond to CONVERT_TEMP.\n");
 
 	for (i = 0; i < 9; ++i)
-		count += sprintf(buf + count, "%02x ", sl->rom[i]);
+		c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", sl->rom[i]);
 
-	count += sprintf(buf + count, "t=%d\n", w1_convert_temp(rom, sl->family->fid));
-out:
+	c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n",
+		w1_convert_temp(rom, sl->family->fid));
 	mutex_unlock(&dev->mutex);
 
-	return count;
+	return PAGE_SIZE - c;
 }
 
 static int __init w1_therm_init(void)
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index bad7c7c..ab2944a 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -46,7 +46,6 @@ struct w1_reg_num
 #include "w1_family.h"
 
 #define W1_MAXNAMELEN		32
-#define W1_SLAVE_DATA_SIZE	128
 
 #define W1_SEARCH		0xF0
 #define W1_ALARM_SEARCH		0xEC
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 17/35] W1: w1_family, remove unused variable need_exit

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

w1_family.c 1.2
w1_family.h 1.2
Removed the w1_family structure member variable need_exit.  It was
only being set and never used.  Even if it were to be used it is a
polling type operation.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1_family.c |    7 +------
 drivers/w1/w1_family.h |    1 -
 2 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/drivers/w1/w1_family.c b/drivers/w1/w1_family.c
index 8c35f9c..4a09904 100644
--- a/drivers/w1/w1_family.c
+++ b/drivers/w1/w1_family.c
@@ -48,7 +48,6 @@ int w1_register_family(struct w1_family *newf)
 
 	if (!ret) {
 		atomic_set(&newf->refcnt, 0);
-		newf->need_exit = 0;
 		list_add_tail(&newf->family_entry, &w1_families);
 	}
 	spin_unlock(&w1_flock);
@@ -73,9 +72,6 @@ void w1_unregister_family(struct w1_family *fent)
 			break;
 		}
 	}
-
-	fent->need_exit = 1;
-
 	spin_unlock(&w1_flock);
 
 	/* deatch devices using this family code */
@@ -113,8 +109,7 @@ struct w1_family * w1_family_registered(u8 fid)
 
 static void __w1_family_put(struct w1_family *f)
 {
-	if (atomic_dec_and_test(&f->refcnt))
-		f->need_exit = 1;
+	atomic_dec(&f->refcnt);
 }
 
 void w1_family_put(struct w1_family *f)
diff --git a/drivers/w1/w1_family.h b/drivers/w1/w1_family.h
index 296a87e..3053133 100644
--- a/drivers/w1/w1_family.h
+++ b/drivers/w1/w1_family.h
@@ -53,7 +53,6 @@ struct w1_family
 	struct w1_family_ops	*fops;
 
 	atomic_t		refcnt;
-	u8			need_exit;
 };
 
 extern spinlock_t w1_flock;
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 18/35] W1: w1_therm consistent mutex access code cleanup

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

w1_therm.c 1.7
sl->master->mutex and dev->mutex refer to the same mutex variable, but
be consistent and use the same set of pointers for the lock and unlock
calls.  It is less confusing (and one less pointer dereference this
way).

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/slaves/w1_therm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index ca47421..284acac 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -163,7 +163,7 @@ static ssize_t w1_therm_read(struct device *device,
 	int i, max_trying = 10;
 	ssize_t c=PAGE_SIZE;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&dev->mutex);
 
 	memset(rom, 0, sizeof(rom));
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 19/35] W1: w1_int.c use first available master number

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 2634 bytes --]

w1_int.c 1.8
Follow the example of other devices (like the joystick device).  Pick           the first available id for each detected device.  For a one device
system suspending and resuming would at least keep the 1 id.  It would
at least make it easier on the user program.  That and it would reduce
holes, plugin master, plugin master, remove 2nd master, plug in
master, then you have 1, 2 intead of 1, 3.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1_int.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index f1806e2..9ec468a 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -29,7 +29,6 @@
 #include "w1_netlink.h"
 #include "w1_int.h"
 
-static u32 w1_ids = 1;
 static int w1_search_count = -1; /* Default is continual scan */
 module_param_named(search_count, w1_search_count, int, 0);
 
@@ -98,9 +97,10 @@ static void w1_free_dev(struct w1_master *dev)
 
 int w1_add_master_device(struct w1_bus_master *master)
 {
-	struct w1_master *dev;
+	struct w1_master *dev, *entry;
 	int retval = 0;
 	struct w1_netlink_msg msg;
+	int id, found;
 
         /* validate minimum functionality */
         if (!(master->touch_bit && master->reset_bus) &&
@@ -122,13 +122,32 @@ int w1_add_master_device(struct w1_bus_master *master)
 		master->set_pullup=NULL;
 	}
 
-	dev = w1_alloc_dev(w1_ids++, w1_max_slave_count, w1_max_slave_ttl, &w1_master_driver, &w1_master_device);
-	if (!dev)
+	/* Lock until the device is added (or not) to w1_masters. */
+	mutex_lock(&w1_mlock);
+	/* Search for the first available id (starting at 1). */
+	id=0;
+	do {
+		++id;
+		found=0;
+		list_for_each_entry(entry, &w1_masters, w1_master_entry) {
+			if(entry->id==id) {
+				found=1;
+				break;
+			}
+		}
+	}while(found);
+
+	dev = w1_alloc_dev(id, w1_max_slave_count, w1_max_slave_ttl, &w1_master_driver, &w1_master_device);
+	if (!dev) {
+		mutex_unlock(&w1_mlock);
 		return -ENOMEM;
+	}
 
 	retval =  w1_create_master_attributes(dev);
-	if (retval)
+	if (retval) {
+		mutex_unlock(&w1_mlock);
 		goto err_out_free_dev;
+	}
 
 	memcpy(dev->bus_master, master, sizeof(struct w1_bus_master));
 
@@ -140,10 +159,10 @@ int w1_add_master_device(struct w1_bus_master *master)
 		dev_err(&dev->dev,
 			 "Failed to create new kernel thread. err=%d\n",
 			 retval);
+		mutex_unlock(&w1_mlock);
 		goto err_out_rm_attr;
 	}
 
-	mutex_lock(&w1_mlock);
 	list_add(&dev->w1_master_entry, &w1_masters);
 	mutex_unlock(&w1_mlock);
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 20/35] W1: w1.c s/printk/dev_dbg/

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

w1.c 1.18
Code cleanup, more of a just because.
s/printk/dev_dbg/

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 32cfb6a..189b60b 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -82,10 +82,10 @@ static void w1_slave_release(struct device *dev)
 {
 	struct w1_slave *sl = dev_to_w1_slave(dev);
 
-	printk("%s: Releasing %s.\n", __func__, sl->name);
+	dev_dbg(dev, "%s: Releasing %s.\n", __func__, sl->name);
 
 	while (atomic_read(&sl->refcnt)) {
-		printk("Waiting for %s to become free: refcnt=%d.\n",
+		dev_dbg(dev, "Waiting for %s to become free: refcnt=%d.\n",
 				sl->name, atomic_read(&sl->refcnt));
 		if (msleep_interruptible(1000))
 			flush_signals(current);
@@ -881,7 +881,7 @@ void w1_search(struct w1_master *dev, u8 search_type, w1_slave_found_callback cb
 			rn |= (tmp64 << i);
 
 			if(kthread_should_stop()) {
-				printk(KERN_INFO "Abort w1_search (exiting)\n");
+				dev_dbg(&dev->dev, "Abort w1_search\n");
 				return;
 			}
 		}
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 21/35] W1: w1_io.c reset comments and msleep

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

w1_io.c 1.7
w1_reset_bus, added some comments about the timing and switched to
msleep for the later delay.  I don't have the hardware to test the
sleep after reset change.  The one wire doesn't have a timing                   requirement between commands so it is fine.  I do have the USB                  hardware and it would be in big trouble with 10ms interrupt transfers
to find that the reset completed.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/w1_io.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c
index 46c6994..9238758 100644
--- a/drivers/w1/w1_io.c
+++ b/drivers/w1/w1_io.c
@@ -291,12 +291,24 @@ int w1_reset_bus(struct w1_master *dev)
 		result = dev->bus_master->reset_bus(dev->bus_master->data) & 0x1;
 	else {
 		dev->bus_master->write_bit(dev->bus_master->data, 0);
+		/* minimum 480, max ? us
+		 * be nice and sleep, except 18b20 spec lists 960us maximum,
+		 * so until we can sleep with microsecond accuracy, spin.
+		 * Feel free to come up with some other way to give up the
+		 * cpu for such a short amount of time AND get it back in
+		 * the maximum amount of time.
+		 */
 		w1_delay(480);
 		dev->bus_master->write_bit(dev->bus_master->data, 1);
 		w1_delay(70);
 
 		result = dev->bus_master->read_bit(dev->bus_master->data) & 0x1;
-		w1_delay(410);
+		/* minmum 70 (above) + 410 = 480 us
+		 * There aren't any timing requirements between a reset and
+		 * the following transactions.  Sleeping is safe here.
+		 */
+		/* w1_delay(410); min required time */
+		msleep(1);
 	}
 
 	return result;
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 22/35] W1: ds1wm.c msleep for reset

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]

masters/ds1wm.c 1.4
Like the previous w1_io.c reset coments and msleep patch, I don't have
the hardware to verify the change, but I think it is safe.  It also
helps to see a comment like this in the code.
"We'll wait a bit longer just to be sure."
If they are going to calculate delaying 324.9us, but actually delay
500us, why not just give up the CPU and sleep?  This is designed for a
battery powered ARM system, avoiding busywaiting has to be good for
battery life.

I sent a request for testers March 7, 2008 to the Linux kernel mailing
list and two developers who have patches for ds1wm.c, but I didn't get
any respons.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds1wm.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/masters/ds1wm.c b/drivers/w1/masters/ds1wm.c
index ea894bf..29e144f 100644
--- a/drivers/w1/masters/ds1wm.c
+++ b/drivers/w1/masters/ds1wm.c
@@ -160,8 +160,10 @@ static int ds1wm_reset(struct ds1wm_data *ds1wm_data)
 	 *     625 us - 60 us - 240 us - 100 ns = 324.9 us
 	 *
 	 * We'll wait a bit longer just to be sure.
+	 * Was udelay(500), but if it is going to busywait the cpu that long,
+	 * might as well come back later.
 	 */
-	udelay(500);
+	msleep(1);
 
 	ds1wm_write_register(ds1wm_data, DS1WM_INT_EN,
 		DS1WM_INTEN_ERBF | DS1WM_INTEN_ETMT | DS1WM_INTEN_EPD |
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 23/35] W1: ds2490.c correct print message

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

ds2490.c 1.2
Corrected print message, it was writing not reading, this also prints
the endpoint used for the write instead of hardcoding it.
        Failed to write 1-wire data to ep0x%x: err=%d.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index b63b5e0..67a7f42 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -341,7 +341,8 @@ static int ds_send_data(struct ds_device *dev, unsigned char *buf, int len)
 	count = 0;
 	err = usb_bulk_msg(dev->udev, usb_sndbulkpipe(dev->udev, dev->ep[EP_DATA_OUT]), buf, len, &count, 1000);
 	if (err < 0) {
-		printk(KERN_ERR "Failed to read 1-wire data from 0x02: err=%d.\n", err);
+		printk(KERN_ERR "Failed to write 1-wire data to ep0x%x: err=%d.\n",
+			dev->ep[EP_DATA_OUT], err);
 		return err;
 	}
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 24/35] W1: ds2490.c add support for strong pullup

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 6067 bytes --]

ds2490.c had ds_start_pulse, but I think I decided that the pulse
wasn't being enabled, as the strong pullup wasn't enabled.  If it was
working it was being called some time after every byte or block was
written.  It should have been enabled in the write command instead of
some time later to benefit any device needing it.

ds_start_pulse was also calling mdelay(750), which would effectively
lock up the computer for 3/4 of a second four times per temperature
conversion.  This was immediately noticable just typing at a virtual
terminal.

The new set_pullup will only enable the strong pullup when requested,
which is expected to be the only write operation that will benefit
from a strong pullup.  Now msleep will be used for delays, resulting
in the reads and writes taking minimal system overhead.

ds2490.c 1.3
Add set_pullup.

ds2490.c 1.4
Enable existing ds_send_control to send the control command enable
strong pullup.

ds2490.c 1.6
Remove strong pullup enable bit when reading a byte.

ds2490.c 1.7
Add spu_sleep to avoid pulling the device status for the duration of
the strong pullup.

ds2490.c 1.17
Remove PULLUP_PULSE_DURATION macro, it is the delay used for a
temperature conversion which shouldn't be in the master driver anyway.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |   76 +++++++++++++++++++++---------------------
 1 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 67a7f42..ac27f89 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -98,11 +98,6 @@
 #define BRANCH_MAIN			0xCC
 #define BRANCH_AUX			0x33
 
-/*
- * Duration of the strong pull-up pulse in milliseconds.
- */
-#define PULLUP_PULSE_DURATION		750
-
 /* Status flags */
 #define ST_SPUA				0x01  /* Strong Pull-up is active */
 #define ST_PRGA				0x02  /* 12V programming pulse is being generated */
@@ -131,6 +126,11 @@ struct ds_device
 
 	int			ep[NUM_EP];
 
+	/* Strong PullUp
+	 * 0: pullup not active, else duration in milliseconds
+	 */
+	int			spu_sleep;
+
 	struct w1_bus_master	master;
 };
 
@@ -192,7 +192,7 @@ static int ds_send_control_cmd(struct ds_device *dev, u16 value, u16 index)
 
 	return err;
 }
-#if 0
+
 static int ds_send_control_mode(struct ds_device *dev, u16 value, u16 index)
 {
 	int err;
@@ -207,7 +207,7 @@ static int ds_send_control_mode(struct ds_device *dev, u16 value, u16 index)
 
 	return err;
 }
-#endif
+
 static int ds_send_control(struct ds_device *dev, u16 value, u16 index)
 {
 	int err;
@@ -294,14 +294,6 @@ static int ds_recv_status(struct ds_device *dev, struct ds_status *st)
 		if (count < 0)
 			return err;
 	}
-#if 0
-	if (st->status & ST_IDLE) {
-		printk(KERN_INFO "Resetting pulse after ST_IDLE.\n");
-		err = ds_start_pulse(dev, PULLUP_PULSE_DURATION);
-		if (err)
-			return err;
-	}
-#endif
 
 	return err;
 }
@@ -472,32 +464,26 @@ static int ds_set_speed(struct ds_device *dev, int speed)
 }
 #endif  /*  0  */
 
-static int ds_start_pulse(struct ds_device *dev, int delay)
+static int ds_set_pullup(struct ds_device *dev, int delay)
 {
 	int err;
 	u8 del = 1 + (u8)(delay >> 4);
-	struct ds_status st;
-
-#if 0
-	err = ds_stop_pulse(dev, 10);
-	if (err)
-		return err;
-
-	err = ds_send_control_mode(dev, MOD_PULSE_EN, PULSE_SPUE);
-	if (err)
-		return err;
-#endif
-	err = ds_send_control(dev, COMM_SET_DURATION | COMM_IM, del);
-	if (err)
-		return err;
 
-	err = ds_send_control(dev, COMM_PULSE | COMM_IM | COMM_F, 0);
+	dev->spu_sleep = 0;
+	err = ds_send_control_mode(dev, MOD_PULSE_EN, delay?PULSE_SPUE:0);
 	if (err)
 		return err;
 
-	mdelay(delay);
+	if(delay) {
+		err = ds_send_control(dev, COMM_SET_DURATION | COMM_IM, del);
+		if (err)
+			return err;
 
-	ds_wait_status(dev, &st);
+		/* Just storing delay would not get the trunication and
+		 * roundup.
+		 */
+		dev->spu_sleep = del<<4;
+	}
 
 	return err;
 }
@@ -558,6 +544,9 @@ static int ds_write_byte(struct ds_device *dev, u8 byte)
 	if (err)
 		return err;
 
+	if(dev->spu_sleep)
+		msleep(dev->spu_sleep);
+
 	err = ds_wait_status(dev, &st);
 	if (err)
 		return err;
@@ -566,8 +555,6 @@ static int ds_write_byte(struct ds_device *dev, u8 byte)
 	if (err < 0)
 		return err;
 
-	ds_start_pulse(dev, PULLUP_PULSE_DURATION);
-
 	return !(byte == rbyte);
 }
 
@@ -603,7 +590,7 @@ static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
 	if (err < 0)
 		return err;
 
-	err = ds_send_control(dev, COMM_BLOCK_IO | COMM_IM | COMM_SPU, len);
+	err = ds_send_control(dev, COMM_BLOCK_IO | COMM_IM, len);
 	if (err)
 		return err;
 
@@ -630,14 +617,15 @@ static int ds_write_block(struct ds_device *dev, u8 *buf, int len)
 	if (err)
 		return err;
 
+	if(dev->spu_sleep)
+		msleep(dev->spu_sleep);
+
 	ds_wait_status(dev, &st);
 
 	err = ds_recv_data(dev, buf, len);
 	if (err < 0)
 		return err;
 
-	ds_start_pulse(dev, PULLUP_PULSE_DURATION);
-
 	return !(err == len);
 }
 
@@ -803,6 +791,16 @@ static u8 ds9490r_reset(void *data)
 	return 0;
 }
 
+static u8 ds9490r_set_pullup(void *data, int delay)
+{
+	struct ds_device *dev = data;
+
+	if (ds_set_pullup(dev, delay))
+		return 1;
+
+	return 0;
+}
+
 static int ds_w1_init(struct ds_device *dev)
 {
 	memset(&dev->master, 0, sizeof(struct w1_bus_master));
@@ -816,6 +814,7 @@ static int ds_w1_init(struct ds_device *dev)
 	dev->master.read_block	= &ds9490r_read_block;
 	dev->master.write_block	= &ds9490r_write_block;
 	dev->master.reset_bus	= &ds9490r_reset;
+	dev->master.set_pullup	= &ds9490r_set_pullup;
 
 	return w1_add_master_device(&dev->master);
 }
@@ -839,6 +838,7 @@ static int ds_probe(struct usb_interface *intf,
 		printk(KERN_INFO "Failed to allocate new DS9490R structure.\n");
 		return -ENOMEM;
 	}
+	dev->spu_sleep=0;
 	dev->udev = usb_get_dev(udev);
 	if (!dev->udev) {
 		err = -ENOMEM;
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 25/35] W1: ds2490.c fix grouping error in ds_write_bit

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

ds2490.c 1.10

Bad grouping, fixed order of operations.  It would previously always
return COMM_D.
from
COMM_BIT_IO | COMM_IM | (bit) ? COMM_D : 0
to
COMM_BIT_IO | COMM_IM | (bit ? COMM_D : 0)

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index ac27f89..6778bd3 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -525,7 +525,7 @@ static int ds_write_bit(struct ds_device *dev, u8 bit)
 	int err;
 	struct ds_status st;
 
-	err = ds_send_control(dev, COMM_BIT_IO | COMM_IM | (bit) ? COMM_D : 0, 0);
+	err = ds_send_control(dev, COMM_BIT_IO | COMM_IM | (bit ? COMM_D : 0), 0);
 	if (err)
 		return err;
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 26/35] W1: ds2490.c ds_write_bit disable readback

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

ds2490.c 1.11
ds_write_bit doesn't read the input buffer, so add COMM_ICP and a
comment that it will no longer generate a read back data byte.  If
there is an extra data byte later on then it will cause an error and
discard what data was there.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 6778bd3..e0e9325 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -525,7 +525,12 @@ static int ds_write_bit(struct ds_device *dev, u8 bit)
 	int err;
 	struct ds_status st;
 
-	err = ds_send_control(dev, COMM_BIT_IO | COMM_IM | (bit ? COMM_D : 0), 0);
+	/* Set COMM_ICP to write without a readback.  Note, this will
+	 * produce one time slot, a down followed by an up with COMM_D
+	 * only determing the timing.
+	 */
+	err = ds_send_control(dev, COMM_BIT_IO | COMM_IM | COMM_ICP |
+		(bit ? COMM_D : 0), 0);
 	if (err)
 		return err;
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 27/35] W1: ds2490.c disable bit read and write

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]

Summary, don't export read and write bit operations, they didn't work,
they weren't used, and they can't be made to work.

ds2490.c 1.12
Comment out ds9490r_read_bit, ds9490r_write_bit, and ds_write_bit as
well as not exporting them to the one wire core.  As originally coded
ds_write_bit filled up the command fifo with invalid commands and left
data in the input buffer (both fixed in earlier patches).  The
underlying w1_io never even used these functions as the more advanced
byte, block, reset, and touch functions were available.

read_bit and write_bit in w1_bus_master are expected to set and sample
the line level.  For write_bit that means it is expected to set it to
that value and leave it there.  ds2490 only supports an individual
time slot at the lowest level.  The requirement from pulling the bus
state down to reading the state is 15us, something that isn't
realistic on the USB bus anyway.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index e0e9325..8c3d756 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -520,6 +520,7 @@ static int ds_touch_bit(struct ds_device *dev, u8 bit, u8 *tbit)
 	return 0;
 }
 
+#if 0
 static int ds_write_bit(struct ds_device *dev, u8 bit)
 {
 	int err;
@@ -538,6 +539,7 @@ static int ds_write_bit(struct ds_device *dev, u8 bit)
 
 	return 0;
 }
+#endif
 
 static int ds_write_byte(struct ds_device *dev, u8 byte)
 {
@@ -722,6 +724,7 @@ static u8 ds9490r_touch_bit(void *data, u8 bit)
 	return ret;
 }
 
+#if 0
 static void ds9490r_write_bit(void *data, u8 bit)
 {
 	struct ds_device *dev = data;
@@ -729,13 +732,6 @@ static void ds9490r_write_bit(void *data, u8 bit)
 	ds_write_bit(dev, bit);
 }
 
-static void ds9490r_write_byte(void *data, u8 byte)
-{
-	struct ds_device *dev = data;
-
-	ds_write_byte(dev, byte);
-}
-
 static u8 ds9490r_read_bit(void *data)
 {
 	struct ds_device *dev = data;
@@ -748,6 +744,14 @@ static u8 ds9490r_read_bit(void *data)
 
 	return bit & 1;
 }
+#endif
+
+static void ds9490r_write_byte(void *data, u8 byte)
+{
+	struct ds_device *dev = data;
+
+	ds_write_byte(dev, byte);
+}
 
 static u8 ds9490r_read_byte(void *data)
 {
@@ -812,8 +816,15 @@ static int ds_w1_init(struct ds_device *dev)
 
 	dev->master.data	= dev;
 	dev->master.touch_bit	= &ds9490r_touch_bit;
+	/* read_bit and write_bit in w1_bus_master are expected to set and
+	 * sample the line level.  For write_bit that means it is expected to
+	 * set it to that value and leave it there.  ds2490 only supports an
+	 * individual time slot at the lowest level.  The requirement from
+	 * pulling the bus state down to reading the state is 15us, something
+	 * that isn't realistic on the USB bus anyway.
 	dev->master.read_bit	= &ds9490r_read_bit;
 	dev->master.write_bit	= &ds9490r_write_bit;
+	*/
 	dev->master.read_byte	= &ds9490r_read_byte;
 	dev->master.write_byte	= &ds9490r_write_byte;
 	dev->master.read_block	= &ds9490r_read_block;
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 28/35] W1: ds2490.c simplify and fix ds_touch_bit

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

ds2490.c 1.15
Simplify and fix ds_touch_bit.  If a device is attached in the middle
of a bus search the status register will return more than the default
16 bytes.  The additional bytes indicate that it has detected a new
device.  The way ds_wait_status is coded, if it doesn't read 16 status
bytes it returns an error value.  ds_touch_bit then will detect that
error and return an error.  In that case it doesn't read the input
buffer and returns uninitialized data.  It doesn't stop there.  The
next transaction will not expect the extra byte in the input buffer
and the short read will cause an error and clear out both the old byte
and new data in the input buffer.

Just ignore the value of ds_wait_status.  It is still required to wait
until ds2490 is again idle and there is data to read when ds_recv_data
is called.  This also removes the while loop.  None of the other
commands wait and verify that the issued command is in the status
register.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |   21 ++++-----------------
 1 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 8c3d756..45e7f10 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -490,28 +490,15 @@ static int ds_set_pullup(struct ds_device *dev, int delay)
 
 static int ds_touch_bit(struct ds_device *dev, u8 bit, u8 *tbit)
 {
-	int err, count;
+	int err;
 	struct ds_status st;
-	u16 value = (COMM_BIT_IO | COMM_IM) | ((bit) ? COMM_D : 0);
-	u16 cmd;
 
-	err = ds_send_control(dev, value, 0);
+	err = ds_send_control(dev, COMM_BIT_IO | COMM_IM | (bit ? COMM_D : 0),
+		0);
 	if (err)
 		return err;
 
-	count = 0;
-	do {
-		err = ds_wait_status(dev, &st);
-		if (err)
-			return err;
-
-		cmd = st.command0 | (st.command1 << 8);
-	} while (cmd != value && ++count < 10);
-
-	if (err < 0 || count >= 10) {
-		printk(KERN_ERR "Failed to obtain status.\n");
-		return -EINVAL;
-	}
+	ds_wait_status(dev, &st);
 
 	err = ds_recv_data(dev, tbit, sizeof(*tbit));
 	if (err < 0)
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 29/35] W1: ds2490.c ds_dump_status rework

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 8833 bytes --]

add result register #defines
rename ds_dump_status to ds_print_msg
rename ds_recv_status to ds_dump_status
ds_dump_status prints the requested status and no longer reads the
status

ds_2490.c 1.16
rename ds_dump_status to ds_print_msg
ds_print_msg is more appropriate, it is only a wrapper around printk
rename ds_recv_status to ds_dump_status

ds_recv_status would first read a new status message, then print it.
The problem is ds_recv_status was only being called if there was an
error, so it could print the error status.  If the error was cleared,
then the good status message was dumped instead of the one in error.
ds_dump_status now is passed a status value to print.

ds_dump_status now will print the result register in addition to the
status buffer.  For now that will be limited to detecting new devices.
It could include detecting a short and other status indicators, but
the reset command would need to enable generating the result
information and more logic to only print the useful bits.  Printing
the extended result register also required being passed the status, as
a second status read would loose these bytes.

Resetting ST_EPOF was moved to ds_wait_status so it could be cleared
sooner as under normal operation there is never so many commands as to
fill up the command buffer.

ds_wait_status will not treat extra data in the status request as an
error, it will call ds_dump_status when there is extra data though.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |  129 +++++++++++++++++++++++++++----------------
 1 files changed, 81 insertions(+), 48 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 45e7f10..1e04ee4 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -107,6 +107,17 @@
 #define ST_IDLE				0x20  /* DS2490 is currently idle */
 #define ST_EPOF				0x80
 
+/* Result Register flags */
+#define RR_DETECT			0xA5 /* New device detected */
+#define RR_NRS				0x01 /* Reset no presence or ... */
+#define RR_SH				0x02 /* short on reset or set path */
+#define RR_APP				0x04 /* alarming presence on reset */
+#define RR_VPP				0x08 /* 12V expected not seen */
+#define RR_CMP				0x10 /* compare error */
+#define RR_CRC				0x20 /* CRC error detected */
+#define RR_RDP				0x40 /* redirected page */
+#define RR_EOS				0x80 /* end of search error */
+
 #define SPEED_NORMAL			0x00
 #define SPEED_FLEXIBLE			0x01
 #define SPEED_OVERDRIVE			0x02
@@ -164,7 +175,6 @@ MODULE_DEVICE_TABLE(usb, ds_id_table);
 static int ds_probe(struct usb_interface *, const struct usb_device_id *);
 static void ds_disconnect(struct usb_interface *);
 
-static inline void ds_dump_status(unsigned char *, unsigned char *, int);
 static int ds_send_control(struct ds_device *, u16, u16);
 static int ds_send_control_cmd(struct ds_device *, u16, u16);
 
@@ -223,11 +233,6 @@ static int ds_send_control(struct ds_device *dev, u16 value, u16 index)
 	return err;
 }
 
-static inline void ds_dump_status(unsigned char *buf, unsigned char *str, int off)
-{
-	printk("%45s: %8x\n", str, buf[off]);
-}
-
 static int ds_recv_status_nodump(struct ds_device *dev, struct ds_status *st,
 				 unsigned char *buf, int size)
 {
@@ -248,16 +253,14 @@ static int ds_recv_status_nodump(struct ds_device *dev, struct ds_status *st,
 	return count;
 }
 
-static int ds_recv_status(struct ds_device *dev, struct ds_status *st)
+static inline void ds_print_msg(unsigned char *buf, unsigned char *str, int off)
 {
-	unsigned char buf[64];
-	int count, err = 0, i;
-
-	memcpy(st, buf, sizeof(*st));
+	printk("%45s: %8x\n", str, buf[off]);
+}
 
-	count = ds_recv_status_nodump(dev, st, buf, sizeof(buf));
-	if (count < 0)
-		return err;
+static void ds_dump_status(struct ds_device *dev, unsigned char *buf, int count)
+{
+	int i;
 
 	printk("0x%x: count=%d, status: ", dev->ep[EP_STATUS], count);
 	for (i=0; i<count; ++i)
@@ -265,37 +268,46 @@ static int ds_recv_status(struct ds_device *dev, struct ds_status *st)
 	printk("\n");
 
 	if (count >= 16) {
-		ds_dump_status(buf, "enable flag", 0);
-		ds_dump_status(buf, "1-wire speed", 1);
-		ds_dump_status(buf, "strong pullup duration", 2);
-		ds_dump_status(buf, "programming pulse duration", 3);
-		ds_dump_status(buf, "pulldown slew rate control", 4);
-		ds_dump_status(buf, "write-1 low time", 5);
-		ds_dump_status(buf, "data sample offset/write-0 recovery time", 6);
-		ds_dump_status(buf, "reserved (test register)", 7);
-		ds_dump_status(buf, "device status flags", 8);
-		ds_dump_status(buf, "communication command byte 1", 9);
-		ds_dump_status(buf, "communication command byte 2", 10);
-		ds_dump_status(buf, "communication command buffer status", 11);
-		ds_dump_status(buf, "1-wire data output buffer status", 12);
-		ds_dump_status(buf, "1-wire data input buffer status", 13);
-		ds_dump_status(buf, "reserved", 14);
-		ds_dump_status(buf, "reserved", 15);
+		ds_print_msg(buf, "enable flag", 0);
+		ds_print_msg(buf, "1-wire speed", 1);
+		ds_print_msg(buf, "strong pullup duration", 2);
+		ds_print_msg(buf, "programming pulse duration", 3);
+		ds_print_msg(buf, "pulldown slew rate control", 4);
+		ds_print_msg(buf, "write-1 low time", 5);
+		ds_print_msg(buf, "data sample offset/write-0 recovery time", 6);
+		ds_print_msg(buf, "reserved (test register)", 7);
+		ds_print_msg(buf, "device status flags", 8);
+		ds_print_msg(buf, "communication command byte 1", 9);
+		ds_print_msg(buf, "communication command byte 2", 10);
+		ds_print_msg(buf, "communication command buffer status", 11);
+		ds_print_msg(buf, "1-wire data output buffer status", 12);
+		ds_print_msg(buf, "1-wire data input buffer status", 13);
+		ds_print_msg(buf, "reserved", 14);
+		ds_print_msg(buf, "reserved", 15);
 	}
-
-	memcpy(st, buf, sizeof(*st));
-
-	if (st->status & ST_EPOF) {
-		printk(KERN_INFO "Resetting device after ST_EPOF.\n");
-		err = ds_send_control_cmd(dev, CTL_RESET_DEVICE, 0);
-		if (err)
-			return err;
-		count = ds_recv_status_nodump(dev, st, buf, sizeof(buf));
-		if (count < 0)
-			return err;
+	for(i=16; i<count; ++i) {
+		if(buf[i]==RR_DETECT) {
+			ds_print_msg(buf, "new device detect", i);
+			continue;
+		}
+		ds_print_msg(buf, "Result Register Value: ", i);
+		if(buf[i] & RR_NRS)
+			printk("NRS: Reset no presence or ...\n");
+		if(buf[i] & RR_SH)
+			printk("SH: short on reset or set path\n");
+		if(buf[i] & RR_APP)
+			printk("APP: alarming presence on reset\n");
+		if(buf[i] & RR_VPP)
+			printk("VPP: 12V expected not seen\n");
+		if(buf[i] & RR_CMP)
+			printk("CMP: compare error\n");
+		if(buf[i] & RR_CRC)
+			printk("CRC: CRC error detected\n");
+		if(buf[i] & RR_RDP)
+			printk("RDP: redirected page\n");
+		if(buf[i] & RR_EOS)
+			printk("EOS: end of search error\n");
 	}
-
-	return err;
 }
 
 static int ds_recv_data(struct ds_device *dev, unsigned char *buf, int size)
@@ -307,9 +319,14 @@ static int ds_recv_data(struct ds_device *dev, unsigned char *buf, int size)
 	err = usb_bulk_msg(dev->udev, usb_rcvbulkpipe(dev->udev, dev->ep[EP_DATA_IN]),
 				buf, size, &count, 1000);
 	if (err < 0) {
+		u8 buf[0x20];
+		int count;
+
 		printk(KERN_INFO "Clearing ep0x%x.\n", dev->ep[EP_DATA_IN]);
 		usb_clear_halt(dev->udev, usb_rcvbulkpipe(dev->udev, dev->ep[EP_DATA_IN]));
-		ds_recv_status(dev, &st);
+
+		count=ds_recv_status_nodump(dev, &st, buf, sizeof(buf));
+		ds_dump_status(dev, buf, count);
 		return err;
 	}
 
@@ -390,7 +407,7 @@ int ds_detect(struct ds_device *dev, struct ds_status *st)
 	if (err)
 		return err;
 
-	err = ds_recv_status(dev, st);
+	err = ds_dump_status(dev, st);
 
 	return err;
 }
@@ -415,11 +432,27 @@ static int ds_wait_status(struct ds_device *dev, struct ds_status *st)
 #endif
 	} while(!(buf[0x08] & 0x20) && !(err < 0) && ++count < 100);
 
+	if(err>=16 && st->status & ST_EPOF) {
+		printk(KERN_INFO "Resetting device after ST_EPOF.\n");
+		ds_send_control_cmd(dev, CTL_RESET_DEVICE, 0);
+		/* Always dump the device status. */
+		count=101;
+	}
+
+	/* Dump the status for errors or if there is extended return data.
+	 * The extended status includes new device detection (maybe someone
+	 * can do something with it).
+	 */
+	if(err > 16 || count >= 100 || err < 0)
+		ds_dump_status(dev, buf, err);
 
-	if (((err > 16) && (buf[0x10] & 0x01)) || count >= 100 || err < 0) {
-		ds_recv_status(dev, st);
+	/* Extended data isn't an error.  Well, a short is, but the dump
+	 * would have already told the user that and we can't do anything
+	 * about it in software anyway.
+	 */
+	if(count >= 100 || err < 0)
 		return -1;
-	} else
+	else
 		return 0;
 }
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 30/35] W1: ds2490.c ds_reset remove ds_wait_status

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

ds_2490.c 1.16
ds_reset no longer calls ds_wait_status, the result wasn't used and it
would only delay the following data operations.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |   17 +++--------------
 1 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 1e04ee4..1a87979 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -456,23 +456,15 @@ static int ds_wait_status(struct ds_device *dev, struct ds_status *st)
 		return 0;
 }
 
-static int ds_reset(struct ds_device *dev, struct ds_status *st)
+static int ds_reset(struct ds_device *dev)
 {
 	int err;
 
-	//err = ds_send_control(dev, COMM_1_WIRE_RESET | COMM_F | COMM_IM | COMM_SE, SPEED_FLEXIBLE);
+	/*err = ds_send_control(dev, COMM_1_WIRE_RESET | COMM_F | COMM_IM | COMM_SE, SPEED_FLEXIBLE); */
 	err = ds_send_control(dev, 0x43, SPEED_NORMAL);
 	if (err)
 		return err;
 
-	ds_wait_status(dev, st);
-#if 0
-	if (st->command_buffer_status) {
-		printk(KERN_INFO "Short circuit.\n");
-		return -EIO;
-	}
-#endif
-
 	return 0;
 }
 
@@ -808,12 +800,9 @@ static u8 ds9490r_read_block(void *data, u8 *buf, int len)
 static u8 ds9490r_reset(void *data)
 {
 	struct ds_device *dev = data;
-	struct ds_status st;
 	int err;
 
-	memset(&st, 0, sizeof(st));
-
-	err = ds_reset(dev, &st);
+	err = ds_reset(dev);
 	if (err)
 		return 1;
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 31/35] W1: ds2490.c reset ds2490 in init

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 2522 bytes --]

ds2490.c 1.8
Reset the device in init as it can be in a bad state.  This is
necessary because a block write will wait for data to be placed in the
output buffer and block any later commands which will keep
accumulating and the device will not be idle.  Another case is
removing the ds2490 module while a bus search is in progress, somehow
a few commands get through, but the input transfers fail leaving data
in the input buffer.  This will cause the next read to fail see the
note in ds_recv_data.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 1a87979..2d1bdbc 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -315,6 +315,15 @@ static int ds_recv_data(struct ds_device *dev, unsigned char *buf, int size)
 	int count, err;
 	struct ds_status st;
 
+	/* Careful on size.  If size is less than what is available in
+	 * the input buffer, the device fails the bulk transfer and
+	 * clears the input buffer.  It could read the maximum size of
+	 * the data buffer, but then do you return the first, last, or
+	 * some set of the middle size bytes?  As long as the rest of
+	 * the code is correct there will be size bytes waiting.  A
+	 * call to ds_wait_status will wait until the device is idle
+	 * and any data to be received would have been available.
+	 */
 	count = 0;
 	err = usb_bulk_msg(dev->udev, usb_rcvbulkpipe(dev->udev, dev->ep[EP_DATA_IN]),
 				buf, size, &count, 1000);
@@ -823,6 +832,18 @@ static int ds_w1_init(struct ds_device *dev)
 {
 	memset(&dev->master, 0, sizeof(struct w1_bus_master));
 
+	/* Reset the device as it can be in a bad state.
+	 * This is necessary because a block write will wait for data
+	 * to be placed in the output buffer and block any later
+	 * commands which will keep accumulating and the device will
+	 * not be idle.  Another case is removing the ds2490 module
+	 * while a bus search is in progress, somehow a few commands
+	 * get through, but the input transfers fail leaving data in
+	 * the input buffer.  This will cause the next read to fail
+	 * see the note in ds_recv_data.
+	 */
+	ds_send_control_cmd(dev, CTL_RESET_DEVICE, 0);
+
 	dev->master.data	= dev;
 	dev->master.touch_bit	= &ds9490r_touch_bit;
 	/* read_bit and write_bit in w1_bus_master are expected to set and
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 32/35] W1: ds2490.c magic number work

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

ds2490.c 1.16
macro name typo
s/COMM_NTP/COMM_NTF/

ds2490.c 1.7
specify the commands in ds_reset
s/0x43/COMM_1_WIRE_RESET | COMM_IM/

ds2490.c 1.18
replace magic bitmask in ds_wait_status
s/0x20/ST_IDLE/

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 2d1bdbc..1468077 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -88,7 +88,7 @@
 #define COMM_DT				0x2000
 #define COMM_SPU			0x1000
 #define COMM_F				0x0800
-#define COMM_NTP			0x0400
+#define COMM_NTF			0x0400
 #define COMM_ICP			0x0200
 #define COMM_RST			0x0100
 
@@ -439,7 +439,7 @@ static int ds_wait_status(struct ds_device *dev, struct ds_status *st)
 			printk("\n");
 		}
 #endif
-	} while(!(buf[0x08] & 0x20) && !(err < 0) && ++count < 100);
+	} while(!(buf[0x08] & ST_IDLE) && !(err < 0) && ++count < 100);
 
 	if(err>=16 && st->status & ST_EPOF) {
 		printk(KERN_INFO "Resetting device after ST_EPOF.\n");
@@ -470,7 +470,11 @@ static int ds_reset(struct ds_device *dev)
 	int err;
 
 	/*err = ds_send_control(dev, COMM_1_WIRE_RESET | COMM_F | COMM_IM | COMM_SE, SPEED_FLEXIBLE); */
-	err = ds_send_control(dev, 0x43, SPEED_NORMAL);
+	/* Add COMM_NTF to return result status.  This could initiate
+	 * a bus search if a new presense is detected, but that isn't
+	 * supported.
+	 */
+	err = ds_send_control(dev, COMM_1_WIRE_RESET | COMM_IM, SPEED_NORMAL);
 	if (err)
 		return err;
 
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 33/35] W1: ds2490.c ds_write_block remove extra ds_wait_status

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

ds_wait_status isn't needed between writing data to ds2490 and issuing
the block I/O command to use the data.

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 1468077..cde99b7 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -643,8 +643,6 @@ static int ds_write_block(struct ds_device *dev, u8 *buf, int len)
 	if (err < 0)
 		return err;
 
-	ds_wait_status(dev, &st);
-
 	err = ds_send_control(dev, COMM_BLOCK_IO | COMM_IM | COMM_SPU, len);
 	if (err)
 		return err;
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 34/35] W1: Documentation/w1/masters/ds2490 update

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 4380 bytes --]

Provide some additional details about the status of the driver.

Signed-off-by: David Fries <david@fries.net>
---
 Documentation/w1/masters/ds2490 |   60 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/Documentation/w1/masters/ds2490 b/Documentation/w1/masters/ds2490
index 239f9ae..4b7b8ee 100644
--- a/Documentation/w1/masters/ds2490
+++ b/Documentation/w1/masters/ds2490
@@ -16,3 +16,63 @@ which allows to build USB <-> W1 bridges.
 DS9490(R) is a USB <-> W1 bus master device
 which has 0x81 family ID integrated chip and DS2490
 low-level operational chip.
+
+Notes and limitations.
+- The 5V strong pullup is supported.
+- While the ds2490 supports a hardware search the code doesn't take
+  advantage of it.
+- The hardware will detect when devices are attached to the bus on the
+  next bus (reset?) operation, however only a message is printed as
+  the core w1 code doesn't make use of the information.  Connecting
+  one device tends to give multiple new device notifications.
+- The number of USB bus transactions could be reduced if w1_reset_send
+  was added to the API.  The name is just a suggestion.  It would take
+  a write buffer and a read buffer (along with sizes) as arguments.
+  It would add match rom and rom to the send buffer, reset the bus,
+  and read the result.  The ds2490 block I/O command supports reset,
+  write, read, and strong pullup all in one command.  That would
+  reduce the time and overhead for a set of commands.
+
+  The core w1 functions required to do a temperature conversion are,
+  w1_reset_select_slave, w1_next_pullup, and w1_write_8, which turn
+  around and execute the master functions, reset_bus, write_block,
+  set_pullup, write_byte, set_pullup.  Each ds2490 function will have
+  multiple USB bus transactions, except set_pullup where it only
+  requires transactions if the if the time value is different.  The
+  conversion could be reduced to w1_next_pullup, w1_reset_send which
+  would call set_pullup, reset_send. The reset_send would enable reset
+  and strong pullup (if enabled), write the data buffer, execute the
+  block I/O command, read status, and get data.  Currently I count 11
+  transactions required, this would reduce it to 4.
+- The hardware supports normal, flexable, and overdrive bus
+  communication speeds, but only the normal is supported.
+- The registered w1_bus_master functions don't define error
+  conditions.  If a bus search is in progress and the ds2490 is
+  removed it can produce a good amount of error output before the bus
+  search finishes.
+- The hardware supports detecting some error conditions, such as
+  short, alarming presence on reset, and no presence on reset, but the
+  driver doesn't query those values.
+- The ds2490 specification doesn't cover short bulk in reads in
+  detail, but my observation is if fewer bytes are requested than are
+  available, the bulk read will return an error and the hardware will
+  clear the entire bulk in buffer.  It would be possible to read the
+  maximum buffer size to not run into this error condition, only extra
+  bytes in the buffer is a logic error in the driver.  The code should
+  should match reads and writes as well as data sizes.  Reads and
+  writes are serialized and the status verifies that the chip is idle
+  (and data is available) before the read is executed, so it should
+  not happen.
+- Running x86_64 2.6.24 UHCI under qemu 0.9.0 under x86_64 2.6.22-rc6
+  with a OHCI controller, ds2490 running in the guest would operate
+  normally the first time the module was loaded after qemu attached
+  the ds2490 hardware, but if the module was unloaded, then reloaded
+  most of the time one of the bulk out or in, and usually the bulk in
+  would fail.  qemu sets a 50ms timeout and the bulk in would timeout
+  even when the status shows data available.  A bulk out write would
+  show a successful completion, but the ds2490 status register would
+  show 0 bytes written.  Detaching qemu from the ds2490 hardware and
+  reattaching would clear the problem.  usbmon output in the guest and
+  host did not explain the problem.  My guess is a bug in either qemu
+  or the host OS and more likely the host OS.
+-- 03-06-2008 David Fries <David@Fries.net>
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 35/35] W1: ds2490.c optimize ds_set_pullup

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 6507 bytes --]

ds2490.c 1.20

Optimize the ds_set_pullup function.

Previously when ds_set_pullup was called with a time, the strong pullup
mode was enabled, and the delay was set, when the write command was
called the strong pullup bit was always set, then the strong pullup
mode would be disabled by ds_set_pullup after the write.

ds_send_control_mode(dev, MOD_PULSE_EN, delay?PULSE_SPUE:0);
ds_send_control(dev, COMM_SET_DURATION | COMM_IM, del);
	ds_send_control(dev, COMM_BYTE_IO | COMM_IM | COMM_SPU, byte);
ds_send_control_mode(dev, MOD_PULSE_EN, delay?PULSE_SPUE:0);

This change enables the strong pullup mode at initialization.
ds_send_control_mode(dev, MOD_PULSE_EN, PULSE_SPUE);
If ds_set_pullup was called with a time, the delay was updated only if
it had changed, and an internal variable set to enable the SPU bit on
the following write command.  The ds_set_pullup to disable the strong pullup
only updates the variable.
ds_send_control(dev, COMM_SET_DURATION | COMM_IM, del); // if required
	ds_send_control(dev, COMM_BYTE_IO | COMM_IM | dev->spu_bit, byte);

In the best case, after the first pullup, the requested duration doesn't
change, making three less control transfers, 2 before and one after the
strong pullup enabled transaction.

In the worst case, after the hardware is initialized, it is 2 less
control transfers, as the delay has to be changed between each strong
pullup transfer.

Here is the total time for seven temperature conversions, three with 900ms
and four with 750ms.  The conversion that includes the extra set delay
control takes a little longer.  The additional time is a little closer to
the noise level than this data set might imply.  USBMon puts it between
2ms and 3ms for a control transfer.  This patch would save two to three
times that.

			750ms request
real    0m1.002s        900ms request, update duration
real    0m0.994s        900ms request,
real    0m0.994s        900ms request,
real    0m0.842s        750ms request, update duration
real    0m0.834s        750ms request,
real    0m0.834s        750ms request,
real    0m0.834s        750ms request,

Signed-off-by: David Fries <david@fries.net>
---
 drivers/w1/masters/ds2490.c |   62 +++++++++++++++++++++++++++++-------------
 1 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index cde99b7..9b44205 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -141,6 +141,10 @@ struct ds_device
 	 * 0: pullup not active, else duration in milliseconds
 	 */
 	int			spu_sleep;
+	/* spu_bit contains COMM_SPU or 0 depending on if the strong pullup
+	 * should be active or not for writes.
+	 */
+	u16			spu_bit;
 
 	struct w1_bus_master	master;
 };
@@ -310,6 +314,23 @@ static void ds_dump_status(struct ds_device *dev, unsigned char *buf, int count)
 	}
 }
 
+static void ds_reset_device(struct ds_device *dev)
+{
+	ds_send_control_cmd(dev, CTL_RESET_DEVICE, 0);
+	/* Always allow strong pullup which allow individual writes to use
+	 * the strong pullup.
+	 */
+	if(ds_send_control_mode(dev, MOD_PULSE_EN, PULSE_SPUE))
+		printk("ds_reset_device: Error allowing string pullup\n");
+	/* Chip strong pullup time was cleared. */
+	if(dev->spu_sleep) {
+		/* lower 4 bits are 0, see ds_set_pullup */
+		u8 del=dev->spu_sleep>>4;
+		if(ds_send_control(dev, COMM_SET_DURATION | COMM_IM, del))
+			printk("ds_reset_device: Error setting duration\n");
+	}
+}
+
 static int ds_recv_data(struct ds_device *dev, unsigned char *buf, int size)
 {
 	int count, err;
@@ -443,7 +464,7 @@ static int ds_wait_status(struct ds_device *dev, struct ds_status *st)
 
 	if(err>=16 && st->status & ST_EPOF) {
 		printk(KERN_INFO "Resetting device after ST_EPOF.\n");
-		ds_send_control_cmd(dev, CTL_RESET_DEVICE, 0);
+		ds_reset_device(dev);
 		/* Always dump the device status. */
 		count=101;
 	}
@@ -504,24 +525,26 @@ static int ds_set_speed(struct ds_device *dev, int speed)
 
 static int ds_set_pullup(struct ds_device *dev, int delay)
 {
-	int err;
+	int err=0;
 	u8 del = 1 + (u8)(delay >> 4);
+	/* Just storing delay would not get the trunication and roundup. */
+	int ms = del<<4;
+
+	/* Enable spu_bit if a delay is set. */
+	dev->spu_bit=delay?COMM_SPU:0;
+	/* If delay is zero, it has already been disabled, if the time is
+	 * the same as the hardware was last programmed to, there is also
+	 * nothing more to do.  Compare with the recalculated value ms
+	 * rather than del or delay which can have a different value.
+	 */
+	if(delay==0 || ms==dev->spu_sleep)
+		return err;
 
-	dev->spu_sleep = 0;
-	err = ds_send_control_mode(dev, MOD_PULSE_EN, delay?PULSE_SPUE:0);
+	err = ds_send_control(dev, COMM_SET_DURATION | COMM_IM, del);
 	if (err)
 		return err;
 
-	if(delay) {
-		err = ds_send_control(dev, COMM_SET_DURATION | COMM_IM, del);
-		if (err)
-			return err;
-
-		/* Just storing delay would not get the trunication and
-		 * roundup.
-		 */
-		dev->spu_sleep = del<<4;
-	}
+	dev->spu_sleep = ms;
 
 	return err;
 }
@@ -572,11 +595,11 @@ static int ds_write_byte(struct ds_device *dev, u8 byte)
 	struct ds_status st;
 	u8 rbyte;
 
-	err = ds_send_control(dev, COMM_BYTE_IO | COMM_IM | COMM_SPU, byte);
+	err = ds_send_control(dev, COMM_BYTE_IO | COMM_IM | dev->spu_bit, byte);
 	if (err)
 		return err;
 
-	if(dev->spu_sleep)
+	if(dev->spu_bit)
 		msleep(dev->spu_sleep);
 
 	err = ds_wait_status(dev, &st);
@@ -643,11 +666,11 @@ static int ds_write_block(struct ds_device *dev, u8 *buf, int len)
 	if (err < 0)
 		return err;
 
-	err = ds_send_control(dev, COMM_BLOCK_IO | COMM_IM | COMM_SPU, len);
+	err = ds_send_control(dev, COMM_BLOCK_IO | COMM_IM | dev->spu_bit, len);
 	if (err)
 		return err;
 
-	if(dev->spu_sleep)
+	if(dev->spu_bit)
 		msleep(dev->spu_sleep);
 
 	ds_wait_status(dev, &st);
@@ -844,7 +867,7 @@ static int ds_w1_init(struct ds_device *dev)
 	 * the input buffer.  This will cause the next read to fail
 	 * see the note in ds_recv_data.
 	 */
-	ds_send_control_cmd(dev, CTL_RESET_DEVICE, 0);
+	ds_reset_device(dev);
 
 	dev->master.data	= dev;
 	dev->master.touch_bit	= &ds9490r_touch_bit;
@@ -887,6 +910,7 @@ static int ds_probe(struct usb_interface *intf,
 		return -ENOMEM;
 	}
 	dev->spu_sleep=0;
+	dev->spu_bit=0;
 	dev->udev = usb_get_dev(udev);
 	if (!dev->udev) {
 		err = -ENOMEM;
-- 
1.4.4.4

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 6/35] W1: w1_process, block when there's nothing to do

[Evgeniy Polyakov]

I'm ko with 1-6 patches, please combine them into one.

-- 
	Evgeniy Polyakov

Re: [PATCH 8/35] W1: feature, w1_therm.c use strong pullup

[Evgeniy Polyakov]

On Fri, Mar 28, 2008 at 07:25:10AM -0500, David Fries (david@fries.net) wrote:
> w1_therm.c 1.3 1.4 1.10
> Use the strong pullup not the sleep.  Note that the pullup goes before
> W1_CONVERT_TEMP not after.
> +w1_next_pullup(dev, tm);
>  w1_write_8(dev, W1_CONVERT_TEMP);
> -msleep(tm);

I'm ok with it, but believe it has to be turned off by default and be
able to be turned on by parameter.

Did you test it with multiple devices on the same bus?

-- 
	Evgeniy Polyakov

Re: [PATCH 7/35] W1: feature, enable hardware strong pullup

[Evgeniy Polyakov]

On Fri, Mar 28, 2008 at 07:25:04AM -0500, David Fries (david@fries.net) wrote:
> Add a strong pullup option to the w1 system.  This supplies extra
> power for parasite powered devices.

Ack.

-- 
	Evgeniy Polyakov

Re: [PATCH 9/35] W1: create documentation for the w1_therm slave device

[Evgeniy Polyakov]

On Fri, Mar 28, 2008 at 07:25:15AM -0500, David Fries (david@fries.net) wrote:
> New directory for temperature sensor documentation.
> 
> Documentation/w1/00-INDEX 1.2
> Added new subdirectory slaves
> 
> Documentation/w1/slaves/00-INDEX 1.1 1.2
> Added index and w1_therm entry.
> 
> Documentation/w1/slaves/w1_therm 1.1 1.2
> Initial entry for the temperature sensor.
> 
> Signed-off-by: David Fries <david@fries.net>

Definitely ack.
Whatever is being done, when it has documentation it was done right :)

-- 
	Evgeniy Polyakov

Re: [PATCH 10/35] W1: be able to manually add and remove slaves

[Evgeniy Polyakov]

On Fri, Mar 28, 2008 at 07:25:23AM -0500, David Fries (david@fries.net) wrote:
> w1.c 1.11 1.12 1.14
> I'm intending to be working with a static network and run without the
> bus searching, but bus searching is currently the only way to add
> slave devices.  This allows any slave to be added or removed.  There
> is one file for adding and another for removing.  If the files are
> read a short comment including the expected serial number format is
> returned.
> 
> Signed-off-by: David Fries <david@fries.net>

Ack.

-- 
	Evgeniy Polyakov

Re: [PATCH 13/35] W1: Document add, remove, and search_count.

[Evgeniy Polyakov]

Ack 11-13 patches, please combine them together.

On Fri, Mar 28, 2008 at 07:25:48AM -0500, David Fries (david@fries.net) wrote:
> Documentation/w1/w1.generic 1.2
> Document w1_master_add, w1_master_remove, and search_count.
> 
> Signed-off-by: David Fries <david@fries.net>

-- 
	Evgeniy Polyakov

Re: [PATCH 16/35] W1: w1_therm fix user buffer overflow and cat

[Evgeniy Polyakov]

14-16 patches are a bug fixes (for the same problem though), please
combine them into single patch.
Thank you.

On Fri, Mar 28, 2008 at 07:26:26AM -0500, David Fries (david@fries.net) wrote:
> This switches w1_therm from bin_attribute to device_attribute which
> fixes a buffer overflow and some bad behavior.

-- 
	Evgeniy Polyakov

Re: [PATCH 20/35] W1: w1.c s/printk/dev_dbg/

[Evgeniy Polyakov]

17-20 patches are non-critical cleanups, please combine them into single
patch. Thanks.

On Fri, Mar 28, 2008 at 07:26:50AM -0500, David Fries (david@fries.net) wrote:
> w1.c 1.18
> Code cleanup, more of a just because.
> s/printk/dev_dbg/

-- 
	Evgeniy Polyakov

Re: [PATCH 21/35] W1: w1_io.c reset comments and msleep

[Evgeniy Polyakov]

On Fri, Mar 28, 2008 at 07:26:55AM -0500, David Fries (david@fries.net) wrote:
> w1_io.c 1.7
> w1_reset_bus, added some comments about the timing and switched to
> msleep for the later delay.  I don't have the hardware to test the
> sleep after reset change.  The one wire doesn't have a timing                   requirement between commands so it is fine.  I do have the USB                  hardware and it would be in big trouble with 10ms interrupt transfers
> to find that the reset completed.

Ack.


-- 
	Evgeniy Polyakov

Re: [PATCH 22/35] W1: ds1wm.c msleep for reset

[Evgeniy Polyakov]

On Fri, Mar 28, 2008 at 07:27:00AM -0500, David Fries (david@fries.net) wrote:
> masters/ds1wm.c 1.4
> Like the previous w1_io.c reset coments and msleep patch, I don't have
> the hardware to verify the change, but I think it is safe.  It also
> helps to see a comment like this in the code.
> "We'll wait a bit longer just to be sure."
> If they are going to calculate delaying 324.9us, but actually delay
> 500us, why not just give up the CPU and sleep?  This is designed for a
> battery powered ARM system, avoiding busywaiting has to be good for
> battery life.

Ack, but would like to here from Matt Reimer.
Added to Cc:

> I sent a request for testers March 7, 2008 to the Linux kernel mailing
> list and two developers who have patches for ds1wm.c, but I didn't get
> any respons.
> 
> Signed-off-by: David Fries <david@fries.net>
> ---
>  drivers/w1/masters/ds1wm.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/w1/masters/ds1wm.c b/drivers/w1/masters/ds1wm.c
> index ea894bf..29e144f 100644
> --- a/drivers/w1/masters/ds1wm.c
> +++ b/drivers/w1/masters/ds1wm.c
> @@ -160,8 +160,10 @@ static int ds1wm_reset(struct ds1wm_data *ds1wm_data)
>  	 *     625 us - 60 us - 240 us - 100 ns = 324.9 us
>  	 *
>  	 * We'll wait a bit longer just to be sure.
> +	 * Was udelay(500), but if it is going to busywait the cpu that long,
> +	 * might as well come back later.
>  	 */
> -	udelay(500);
> +	msleep(1);
>  
>  	ds1wm_write_register(ds1wm_data, DS1WM_INT_EN,
>  		DS1WM_INTEN_ERBF | DS1WM_INTEN_ETMT | DS1WM_INTEN_EPD |
> -- 
> 1.4.4.4



-- 
	Evgeniy Polyakov

Re: [PATCH 34/35] W1: Documentation/w1/masters/ds2490 update

[Evgeniy Polyakov]

On Fri, Mar 28, 2008 at 07:27:59AM -0500, David Fries (david@fries.net) wrote:
> Provide some additional details about the status of the driver.
> 
> Signed-off-by: David Fries <david@fries.net>
> ---
>  Documentation/w1/masters/ds2490 |   60 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/w1/masters/ds2490 b/Documentation/w1/masters/ds2490
> index 239f9ae..4b7b8ee 100644
> --- a/Documentation/w1/masters/ds2490
> +++ b/Documentation/w1/masters/ds2490
> @@ -16,3 +16,63 @@ which allows to build USB <-> W1 bridges.
>  DS9490(R) is a USB <-> W1 bus master device
>  which has 0x81 family ID integrated chip and DS2490
>  low-level operational chip.
> +
> +Notes and limitations.
> +- The 5V strong pullup is supported.
> +- While the ds2490 supports a hardware search the code doesn't take
> +  advantage of it.

It does not actually - at least my hardware can only find first device
on the bus, so I dropped the implementation.

I generally ack whe whole ds2490 changes except pullup one - it has
to be turned off by default, presumably tested with multiple devices on
the same wire.

David, thanks a lot for your work on w1 drivers.

-- 
	Evgeniy Polyakov

Re: [PATCH 22/35] W1: ds1wm.c msleep for reset

[David Fries]

On Sun, Mar 30, 2008 at 03:38:06PM +0400, Evgeniy Polyakov wrote:
> On Fri, Mar 28, 2008 at 07:27:00AM -0500, David Fries (david@fries.net) wrote:
> > masters/ds1wm.c 1.4
> > Like the previous w1_io.c reset coments and msleep patch, I don't have
> > the hardware to verify the change, but I think it is safe.  It also
> > helps to see a comment like this in the code.
> > "We'll wait a bit longer just to be sure."
> > If they are going to calculate delaying 324.9us, but actually delay
> > 500us, why not just give up the CPU and sleep?  This is designed for a
> > battery powered ARM system, avoiding busywaiting has to be good for
> > battery life.
> 
> Ack, but would like to here from Matt Reimer.
> Added to Cc:

Yes, it would be nice to hear from someone with the hardware, but it
has been over three weeks without a response.

Date:   Fri, 7 Mar 2008 22:37:37 -0600
Subject: [W1] ds1wm: testers please, msleep not udelay
Cc: Anton Vorontsov <cbouatmailru@gmail.com>,
        Matt Reimer <mreimer@vpop.net>


> > I sent a request for testers March 7, 2008 to the Linux kernel mailing
> > list and two developers who have patches for ds1wm.c, but I didn't get
> > any respons.
> > 
> > Signed-off-by: David Fries <david@fries.net>
> > ---
> >  drivers/w1/masters/ds1wm.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/w1/masters/ds1wm.c b/drivers/w1/masters/ds1wm.c
> > index ea894bf..29e144f 100644
> > --- a/drivers/w1/masters/ds1wm.c
> > +++ b/drivers/w1/masters/ds1wm.c
> > @@ -160,8 +160,10 @@ static int ds1wm_reset(struct ds1wm_data *ds1wm_data)
> >  	 *     625 us - 60 us - 240 us - 100 ns = 324.9 us
> >  	 *
> >  	 * We'll wait a bit longer just to be sure.
> > +	 * Was udelay(500), but if it is going to busywait the cpu that long,
> > +	 * might as well come back later.
> >  	 */
> > -	udelay(500);
> > +	msleep(1);
> >  
> >  	ds1wm_write_register(ds1wm_data, DS1WM_INT_EN,
> >  		DS1WM_INTEN_ERBF | DS1WM_INTEN_ETMT | DS1WM_INTEN_EPD |
> > -- 
> > 1.4.4.4
> 
> 
> 
> -- 
> 	Evgeniy Polyakov

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)

Re: [PATCH 34/35] W1: Documentation/w1/masters/ds2490 update

[David Fries]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

On Sun, Mar 30, 2008 at 03:43:35PM +0400, Evgeniy Polyakov wrote:
> On Fri, Mar 28, 2008 at 07:27:59AM -0500, David Fries (david@fries.net) wrote:
> > +Notes and limitations.
> > +- The 5V strong pullup is supported.
> > +- While the ds2490 supports a hardware search the code doesn't take
> > +  advantage of it.
> 
> It does not actually - at least my hardware can only find first device
> on the bus, so I dropped the implementation.

ds_search, ds_match_access, and ds_set_path need a comment to that
effect then.

> I generally ack whe whole ds2490 changes except pullup one - it has
> to be turned off by default, presumably tested with multiple devices on
> the same wire.

The second set of patches include a module parameter for both the
w1_therm where a 5V strong pullup is requested and the one wire core
w1_io.c where the request makes calls into the master driver.  This
way there is also a sysfs file to enable and disable the 5V strong
pullup per master.

I have tested the 5V strong pullup with five devices on the bus.  It
works fine.  Two ds18b20 temperature sensors and two thermochron
iButton sensors.  I have maybe 20 feet of wire with a fork near the
ds2490.

It is the 12V programming pulse that is limited to one device and a
shorter wire.  This is the 5V strong pullup, and it doesn't have any
cautions.  The data wire is already working at 5V, it is just the
amount of current that can be supplied increases.

While I agree it is good to have an option to disable it, as it is
optional, what is your reasoning to opposed it?  The ds2490
specification describes in the active pullup section that when the bus
is low and the desired state is high it will start first by the weak
pullup, then when the voltage reaches 0.25V it will switch on the
strong pullup, and VD-1.75V to VDD -0.3V it will set a timer to
disable the strong pullup.  So it is already being used on the bus.

> David, thanks a lot for your work on w1 drivers.
> 
> -- 
> 	Evgeniy Polyakov

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]