w1: small type cleanup in sysfs

On 64 bit systems, a large value for "tmp" could be truncated so we still end up with a ->max_slave_count which is less than one despite the "tmp < 1" check.

This is more of a problem for static checkers than a real life issue, but it's simple enough to fix.

index 9eb816b2ea5e..b96f61b15dc6 100644
@@ -320,10 +320,10 @@ static ssize_t w1_master_attribute_show_timeout(struct device *dev, struct devic
 static ssize_t w1_master_attribute_store_max_slave_count(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count)
 {
-	long tmp;
+	int tmp;
 	struct w1_master *md = dev_to_w1_master(dev);
 
-	if (kstrtol(buf, 0, &tmp) = -EINVAL || tmp < 1)
+	if (kstrtoint(buf, 0, &tmp) = -EINVAL || tmp < 1)
 		return -EINVAL;
 
 	mutex_lock(&md->mutex);

Dan,

I have some other changes in work, how automated is your checkers? How much work is it for me to give a github repository and branch and find out if I introduced any problems before submitting them?

I didn't get how you could get a less than one after a check for less than one from the description or patch until I looked at the rest of the source code. Looks good if the description mentions max_slave_count is an int.

How about wording it,

On 64 bit systems, a large value for "long tmp" is truncated when assigning to "int md->max_slave_count" so we still end up with a value less than one despite the "tmp < 1" check.

David Fries <[email protected]>    PGP pub CB1EE8F0
http://fries.net/~david/

On 64 bit systems, a large value for "long tmp" is truncated when assigning to "int md->max_slave_count" so we still end up with a value less than one despite the "tmp < 1" check.

This is more of a problem for static checkers than a real life issue, but it's simple enough to fix.

---
v2: clarified the commit message

index 9eb816b2ea5e..b96f61b15dc6 100644
@@ -320,10 +320,10 @@ static ssize_t w1_master_attribute_show_timeout(struct device *dev, struct devic
 static ssize_t w1_master_attribute_store_max_slave_count(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count)
 {
-	long tmp;
+	int tmp;
 	struct w1_master *md = dev_to_w1_master(dev);
 
-	if (kstrtol(buf, 0, &tmp) = -EINVAL || tmp < 1)
+	if (kstrtoint(buf, 0, &tmp) = -EINVAL || tmp < 1)
 		return -EINVAL;
 
 	mutex_lock(&md->mutex);

This is a Smatch thing I'm working on but haven't pushed yet. Eventually it will be a part of Fengguang Wu's automated kbuild zero day testing process.

Sure. That's clearer. I have resent it.

regards,
dan carpenter

Hi

12.02.2014, 01:08, "Dan Carpenter" <[email protected]>:

Looks good to me, thank you

A program can bundle multiple messages and commands together when making one wire requests, which is going to be much more efficient than sending lots of little packets one write at a time. With this change the kernel will then bundle responses to requests that were bundled, where it's probably even more important because programs will typically execute select or a poll operation between reading packets, which takes additional context switches.

Without doing any bundling, reading 14 temperature sensors using slave commands requires the application to send 14 packets for the conversion with one command each, and then later for reading it takes 14 packets with two commands each, one to request reading the scratchpad, and another to read the data back in. The kernel will then generate 14 replies with data, and then one status for each of the three commands per sensor totaling 56 packets or 84 total packets.

That's a lot of context switches and not very efficient on the one wire bus. Using master commands allows for merging writes, such as match rom, rom id, and convert temperature into sending one write to the hardware. But that means sending individual commands for reset and write (for convert), or reset, write, read (for reading the result) which would be 28 packets without bundling or 2 packets when bundling. It requires two packets because it must wait for the conversion before reading results. Without this change the kernel would send one read result, and five command status results, so 6*14 = 84 packets. With this set of changes it would send three, a set of status results for the first convert commands, and another status result along with the data read for the second set.

This also allows an application to get all the results from a bundle (reading the temperature sensors) at the same time, instead of them being spread out in time as the one wire bus takes time to execute and read each response.

Due to the data structure limitation this patch will return packets in a different order. For a given bundle all the replies will be sent first, followed by the status messages. This is because currently the connector takes a cn_msg, and the difference between a reply and status is the ack field of cn_msg. I could add a second cn_netlink_send_nlh, to take a nlmsghdr or maybe a length so multiple cn_msg can be bundled into one packet, then all the messages could be returned in the current order.

---
 drivers/w1/w1.h         |    8 -
 drivers/w1/w1_netlink.c |  531 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 340 insertions(+), 199 deletions(-)

index 734dab7..56a49ba 100644
@@ -203,7 +203,6 @@ enum w1_master_flags {
  * @search_id:		allows continuing a search
  * @refcnt:		reference count
  * @priv:		private data storage
- * @priv_size:		size allocated
  * @enable_pullup:	allows a strong pullup
  * @pullup_duration:	time for the next strong pullup
  * @flags:		one of w1_master_flags

@@ -214,7 +213,6 @@ enum w1_master_flags {
  * @dev:		sysfs device
  * @bus_master:		io operations available
  * @seq:		sequence number used for netlink broadcasts
- * @portid:		destination for the current netlink command
  */
 struct w1_master
 {

@@ -241,7 +239,6 @@ struct w1_master
 	atomic_t		refcnt;
 
 	void			*priv;
-	int			priv_size;
 
 	/** 5V strong pullup enabled flag, 1 enabled, zero disabled. */
 	int			enable_pullup;

@@ -260,11 +257,6 @@ struct w1_master
 	struct w1_bus_master	*bus_master;
 
 	u32			seq;
-	/* port id to send netlink responses to.  The value is temporarily
-	 * stored here while processing a message, set after locking the
-	 * mutex, zero before unlocking the mutex.
-	 */
-	u32			portid;
 };
 
 /**

1.7.10.4

Hi

19.02.2014, 11:19, "David Fries" <[email protected]>:

Your approach and patch seem correct, but I worry about how old commands are processed. Do I get it right, that replies to old non-bundle commands are queued back instead of sending immediately, but since it is not bundle but single command, queued reply is being sent 'almost' immediately? Basically, nothing changed to old clients, but there is new way to send batch requests now?

And please make indent less paranoid about line length, it is really hard to read some lines, which were broken into pieces just because of that.