w1: unlock correct lock on error in w1_seq_show()
Subsystems (2)
PATCH 2/2 w1: unlock correct lock on error in w1_seq_show()
Smatch complains that we don't unlock "master->mutex" on this error path. It looks like it is a typo and we unlock ->bus_mutext where ->mutex was intended.
drivers/w1/slaves/w1_therm.c +1 -1
@@ -412,7 +412,7 @@ static ssize_t w1_seq_show(struct device *device, c -=snprintf (buf +PAGE_SIZE - c, c,"%d\n" , seq);return PAGE_SIZE - c; error: -mutex_unlock (&sl->master->bus_mutex); +mutex_unlock (&sl->master->mutex);return -EIO ; }
Thanks for including me, I'm going to have to nack this though, it is an error, but bus_mutex is correct, mutex is incorrect, so the other two lock/unlock in that routine are the ones that need to be changed.
w1.h
* @mutex: protect most of w1_master
* @bus_mutex: pretect concurrent bus access
Quoted 20 lines · Dan Carpenter
Smatch complains that we don't unlock "
master->mutex" on this error path. It looks like it is a typo and we unlock ->bus_mutext where ->mutex was intended.Fixes d9411e57dc7f ('w1: Add support for DS28EA00 sequence to w1-therm')Signed-off-by Dan Carpenter <[email protected]>drivers/w1/slaves/w1_therm.c +1
-1@@ -412,7 +412,7 @@ static ssize_t w1_seq_show(struct device *device, c -=snprintf (buf +PAGE_SIZE - c, c,"%d\n" , seq);return PAGE_SIZE - c; error: -mutex_unlock (&sl->master->bus_mutex); +mutex_unlock (&sl->master->mutex);return -EIO ; }
Signature · 1 line
David Fries <[email protected]>
A message that is not in the archive (sent off-list or not yet received): CA+gYwTGSBAjSPyUSUwknOmm8SoFzHFdJmoYPVA9yN61aWO7sdA@mail.gmail.com
The source code is as good as it gets, that's why I posted those two lines from the w1.h file. Maybe it could be expanded upon, mutex is more for adding or removing slaves and the like modifications to w1_master data structure, where bus_mutex is what make sure something else doesn't try to talk on the bus while the current device is using it. What really matters is how other code is using the locks. For instance if you look at w1_therm.c bus_mutex is the only one used. Then you look at what your routine is doing, and what the existing routine is doing, they are both reading/writing from the one wire bus, and then return their output to the user, so then use the same lock. If it's the wrong lock then it's at least consistent and easier to cleanup when someone figures out it's the wrong lock. If you think it's the wrong lock raise a flag and ask.
My patch review comments.
The temperature sensor uses a strong pullup or delay of 750 ms to do the temperature conversion. A quick read through of this sensor has temperature conversion and EEPROM write as the only operations requiring a significant time delay, everything else is in the us, so I would expect this to not require any delay.
Read the comments to w1_next_pullup in w1_io.c as pullup_duration will always be 0 in msleep(sl->master->pullup_duration); which isn't likely what you expect. Apparently it is working for you so that further reinforces my previous point.
Where did the 64 come from? I have 14 temperature sensors in my single family residence right now, and it would take another 7 to cover each area/room. I could imagine an actual industrial setting to far exceed the 64 count, but I don't work in that area so I don't know the practical limits. How long does it take to read if it loops through 64 times without finding a match? What is reg_num value if a sensor doesn't reply? If it is all 00's or all ff's would that be a better test for the end of the line? How long would it take to loop 16384 times? Maybe you stop if it reaches 16384 or if the end of line value is read 64 times. I also assume this is pretty much a static answer, so you aren't going to be querying it except once per device, once per program setup and rarely or infrequently after that so the time shouldn't matter too much. Maybe the program queries again if a sensor is detected or removed.
I would like to see some documentation (comment) at the top of w1_seq_show as to what the sysfs number returns means. I can see the variable is named seq, and it returns it as a signed number. What does a negative value mean? Are they expected? Will the first device in the chain be 0 or 1, and what defines what end that is? What kind of value is returned if the pins are connected as GPIO and not in the direct sequence configuration? Maybe it needs a comment that all sensors of this type must be in the direct sequence configuration for this routine to give meaningful values. "Place all devices in CHAIN state" that will address all devices including other types of devices, would you expect this to cause a problem with other devices?
Line up the constants after #define to the same tab location before w1_seq_show.
Do you plan to send in a mutex fixup patch? In my experience gregkh's tree only accepts patches to what is already there, he won't swap out a patch he already has (he could as it isn't in Linus's tree, but he doesn't).
Quoted 43 lines · Matt Campbell
David, not arguing just curious how you decide which lock to use? Is there a standard documented?
--m
Matt Campbell
[email protected]Quoted 33 lines · David Fries
Thanks for including me, I'm going to have to nack this though, it is an error, but
bus_mutexis correct, mutex is incorrect, so the other two lock/unlock in that routine are the ones that need to be changed.
w1.h
* @mutex: protect most ofw1_master
* @bus_mutex: pretect concurrent bus accessQuoted 12 lines · Dan Carpenter
Smatch complains that we don't unlock "
master->mutex" on this error path. It looks like it is a typo and we unlock ->bus_mutext where ->mutex was intended.Fixes d9411e57dc7f ('w1: Add support for DS28EA00 sequence to w1-therm')Signed-off-by Dan Carpenter <[email protected]>drivers/w1/slaves/w1_therm.c +0
-0drivers/w1/slaves/w1_therm.c static ssize_t w1_seq_show(struct device *device, · line 418
Quoted 8 lines
drivers/w1/slaves/w1_therm.c +1
-1@@ -412,7 +412,7 @@ static ssize_t w1_seq_show(struct device *device, c -=snprintf (buf +PAGE_SIZE - c, c,"%d\n" , seq);return PAGE_SIZE - c; error: -mutex_unlock (&sl->master->bus_mutex); +mutex_unlock (&sl->master->mutex);return -EIO ; }--
David Fries <[email protected]>
Signature · 1 line
David Fries <[email protected]>
Thanks for the response, I will try to answer your questions.
The other function besides temp conversion and EEPROM write that requires a delay is the initial CHAIN on command. The datasheet says to "Wait for chain to charge" but it does not offer ANY indication on how to do so. I did discover that my sleep was a sleep(0) long after I submitted the patch, but as you observed it was not hurting anything.
The 64 was, in all honesty, a starting point that I picked for lack of finding a better option. I only currently have 10 sensors available for me to test with, so I was not able to do any significant tests to see what would happen if I raised that limit and the loop did not exit properly at the end of the chain. I was also not able to find an edge case that would cause the loop to not find a match. The w1_seq file only shows for the one type sensor that supports it so in the absence of a bus error, a match is guaranteed.
The output of the function in the event the pins are hooked up as GPIO and not a chain is undefined. Since the chips are very tiny, prototyping this function would require me to design and order some boards that I never was able to use. To my knowledge it is not possible to determine that things are wired this way in the software.
Apologies, this is my first foray in submitting any sort of kernel code so I am not sure of exactly how I would submit a fixup patch for gregkh's tree. If someone wants to walk me through that off list I would be most appreciative!
--m
Matt Campbell
[email protected]
Quoted 109 lines · David Fries
The source code is as good as it gets, that's why I posted those two lines from the
w1.hfile. Maybe it could be expanded upon, mutex is more for adding or removing slaves and the like modifications tow1_masterdata structure, wherebus_mutexis what make sure something else doesn't try to talk on the bus while the current device is using it. What really matters is how other code is using the locks. For instance if you look atw1_therm.cbus_mutexis the only one used. Then you look at what your routine is doing, and what the existing routine is doing, they are both reading/writing from the one wire bus, and then return their output to the user, so then use the same lock. If it's the wrong lock then it's at least consistent and easier to cleanup when someone figures out it's the wrong lock. If you think it's the wrong lock raise a flag and ask.My patch review comments.
The temperature sensor uses a strong pullup or delay of 750 ms to do the temperature conversion. A quick read through of this sensor has temperature conversion and EEPROM write as the only operations requiring a significant time delay, everything else is in the us, so I would expect this to not require any delay.
Read the comments to
w1_next_pullupinw1_io.caspullup_durationwill always be 0 inmsleep(sl->master->pullup_duration); which isn't likely what you expect. Apparently it is working for you so that further reinforces my previous point.Where did the 64 come from? I have 14 temperature sensors in my single family residence right now, and it would take another 7 to cover each area/room. I could imagine an actual industrial setting to far exceed the 64 count, but I don't work in that area so I don't know the practical limits. How long does it take to read if it loops through 64 times without finding a match? What is
reg_numvalue if a sensor doesn't reply? If it is all 00's or all ff's would that be a better test for the end of the line? How long would it take to loop 16384 times? Maybe you stop if it reaches 16384 or if the end of line value is read 64 times. I also assume this is pretty much a static answer, so you aren't going to be querying it except once per device, once per program setup and rarely or infrequently after that so the time shouldn't matter too much. Maybe the program queries again if a sensor is detected or removed.I would like to see some documentation (comment) at the top of
w1_seq_showas to what the sysfs number returns means. I can see the variable is named seq, and it returns it as a signed number. What does a negative value mean? Are they expected? Will the first device in the chain be 0 or 1, and what defines what end that is? What kind of value is returned if the pins are connected as GPIO and not in the direct sequence configuration? Maybe it needs a comment that all sensors of this type must be in the direct sequence configuration for this routine to give meaningful values. "Place all devices in CHAIN state" that will address all devices including other types of devices, would you expect this to cause a problem with other devices?Line up the constants after #define to the same tab location before
w1_seq_show.Do you plan to send in a mutex fixup patch? In my experience gregkh's tree only accepts patches to what is already there, he won't swap out a patch he already has (he could as it isn't in Linus's tree, but he doesn't).
Quoted 43 lines · Matt Campbell
David, not arguing just curious how you decide which lock to use? Is there a standard documented?
--m
Matt Campbell
[email protected]Quoted 33 lines · David Fries
Thanks for including me, I'm going to have to nack this though, it is an error, but
bus_mutexis correct, mutex is incorrect, so the other two lock/unlock in that routine are the ones that need to be changed.
w1.h
* @mutex: protect most ofw1_master
* @bus_mutex: pretect concurrent bus accessQuoted 12 lines · Dan Carpenter
Smatch complains that we don't unlock "
master->mutex" on this error path. It looks like it is a typo and we unlock ->bus_mutext where ->mutex was intended.Fixes d9411e57dc7f ('w1: Add support for DS28EA00 sequence to w1-therm')Signed-off-by Dan Carpenter <[email protected]>drivers/w1/slaves/w1_therm.c +0
-0drivers/w1/slaves/w1_therm.c static ssize_t w1_seq_show(struct device *device, · line 418
Quoted 8 lines
drivers/w1/slaves/w1_therm.c +1
-1@@ -412,7 +412,7 @@ static ssize_t w1_seq_show(struct device *device, c -=snprintf (buf +PAGE_SIZE - c, c,"%d\n" , seq);return PAGE_SIZE - c; error: -mutex_unlock (&sl->master->bus_mutex); +mutex_unlock (&sl->master->mutex);return -EIO ; }--
David Fries <[email protected]>--
David Fries <[email protected]>
Quoted 8 lines · Matt Campbell
Thanks for the response, I will try to answer your questions.
The other function besides temp conversion and EEPROM write that requires a delay is the initial CHAIN on command. The datasheet says to "Wait for chain to charge" but it does not offer ANY indication on how to do so. I did discover that my sleep was a sleep(0) long after I submitted the patch, but as you observed it was not hurting anything.
Then it should be removed as it gives the wrong impression and that variable shouldn't be used in the slave code anyway.
Quoted 8 lines
The 64 was, in all honesty, a starting point that I picked for lack of finding a better option. I only currently have 10 sensors available for me to test with, so I was not able to do any significant tests to see what would happen if I raised that limit and the loop did not exit properly at the end of the chain. I was also not able to find an edge case that would cause the loop to not find a match. The
w1_seqfile only shows for the one type sensor that supports it so in the absence of a bus error, a match is guaranteed.
That's easy enough to test, comment out your if...break; in the loop.
Is there a good reason that the loop doesn't terminate when the device you're after sl->reg_num.id is found? By making your loop terminate on a different condition, W1_42_FINISHED_BYTE, if the slave you are after isn't found you'll still return seq of 0. I'm wondering if seq should be initialized to -1.
Do this, make up the right id of the correct family code and manually tell the kernel it is there. That would be the same as if you removed a chip before the kernel's search timed it out. Then do your sequence scan on your devices including this new one which isn't there. cd /sys/devices/w1_bus_master1
echo 42-000001234567 > w1_master_add
The devices return the rom numbers, I wonder about returning the rom list (which is in the sequence), instead of the sequence number for a device if that would be more or less useful to users. You would be able to do it in one pass.
Quoted 5 lines
The output of the function in the event the pins are hooked up as GPIO and not a chain is undefined. Since the chips are very tiny, prototyping this function would require me to design and order some boards that I never was able to use. To my knowledge it is not possible to determine that things are wired this way in the software.
Something like "Output is undefined if all direct sequence sensors on this one wire bus aren't part of one chain. GPIO control of the two pins is not support by this driver." for the comment above w1_seq_show, in addition to the other comments I was asking about. I see now you have some documentation in Documentation/w1/slaves/w1_therm already that I didn't realize earlier.
Quoted 4 lines
Apologies, this is my first foray in submitting any sort of kernel code so I am not sure of exactly how I would submit a fixup patch for gregkh's tree. If someone wants to walk me through that off list I would be most appreciative!
git remote add gregkh_char-misc git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
git fetch gregkh_char-misc
git rebase remotes/gregkh_char-misc/char-misc-next
and add a new commit from there.
Quoted 114 lines
Matt Campbell
[email protected]Quoted 109 lines · David Fries
The source code is as good as it gets, that's why I posted those two lines from the
w1.hfile. Maybe it could be expanded upon, mutex is more for adding or removing slaves and the like modifications tow1_masterdata structure, wherebus_mutexis what make sure something else doesn't try to talk on the bus while the current device is using it. What really matters is how other code is using the locks. For instance if you look atw1_therm.cbus_mutexis the only one used. Then you look at what your routine is doing, and what the existing routine is doing, they are both reading/writing from the one wire bus, and then return their output to the user, so then use the same lock. If it's the wrong lock then it's at least consistent and easier to cleanup when someone figures out it's the wrong lock. If you think it's the wrong lock raise a flag and ask.My patch review comments.
The temperature sensor uses a strong pullup or delay of 750 ms to do the temperature conversion. A quick read through of this sensor has temperature conversion and EEPROM write as the only operations requiring a significant time delay, everything else is in the us, so I would expect this to not require any delay.
Read the comments to
w1_next_pullupinw1_io.caspullup_durationwill always be 0 inmsleep(sl->master->pullup_duration); which isn't likely what you expect. Apparently it is working for you so that further reinforces my previous point.Where did the 64 come from? I have 14 temperature sensors in my single family residence right now, and it would take another 7 to cover each area/room. I could imagine an actual industrial setting to far exceed the 64 count, but I don't work in that area so I don't know the practical limits. How long does it take to read if it loops through 64 times without finding a match? What is
reg_numvalue if a sensor doesn't reply? If it is all 00's or all ff's would that be a better test for the end of the line? How long would it take to loop 16384 times? Maybe you stop if it reaches 16384 or if the end of line value is read 64 times. I also assume this is pretty much a static answer, so you aren't going to be querying it except once per device, once per program setup and rarely or infrequently after that so the time shouldn't matter too much. Maybe the program queries again if a sensor is detected or removed.I would like to see some documentation (comment) at the top of
w1_seq_showas to what the sysfs number returns means. I can see the variable is named seq, and it returns it as a signed number. What does a negative value mean? Are they expected? Will the first device in the chain be 0 or 1, and what defines what end that is? What kind of value is returned if the pins are connected as GPIO and not in the direct sequence configuration? Maybe it needs a comment that all sensors of this type must be in the direct sequence configuration for this routine to give meaningful values. "Place all devices in CHAIN state" that will address all devices including other types of devices, would you expect this to cause a problem with other devices?Line up the constants after #define to the same tab location before
w1_seq_show.Do you plan to send in a mutex fixup patch? In my experience gregkh's tree only accepts patches to what is already there, he won't swap out a patch he already has (he could as it isn't in Linus's tree, but he doesn't).
Quoted 43 lines · Matt Campbell
David, not arguing just curious how you decide which lock to use? Is there a standard documented?
--m
Matt Campbell
[email protected]Quoted 33 lines · David Fries
Thanks for including me, I'm going to have to nack this though, it is an error, but
bus_mutexis correct, mutex is incorrect, so the other two lock/unlock in that routine are the ones that need to be changed.
w1.h
* @mutex: protect most ofw1_master
* @bus_mutex: pretect concurrent bus accessQuoted 12 lines · Dan Carpenter
Smatch complains that we don't unlock "
master->mutex" on this error path. It looks like it is a typo and we unlock ->bus_mutext where ->mutex was intended.Fixes d9411e57dc7f ('w1: Add support for DS28EA00 sequence to w1-therm')Signed-off-by Dan Carpenter <[email protected]>drivers/w1/slaves/w1_therm.c +0
-0drivers/w1/slaves/w1_therm.c static ssize_t w1_seq_show(struct device *device, · line 418
Quoted 8 lines
drivers/w1/slaves/w1_therm.c +1
-1@@ -412,7 +412,7 @@ static ssize_t w1_seq_show(struct device *device, c -=snprintf (buf +PAGE_SIZE - c, c,"%d\n" , seq);return PAGE_SIZE - c; error: -mutex_unlock (&sl->master->bus_mutex); +mutex_unlock (&sl->master->mutex);return -EIO ; }--
David Fries <[email protected]>--
David Fries <[email protected]>
Signature · 1 line
David Fries <[email protected]>
PATCH v2 2/2 w1: use correct lock on error in w1_seq_show()
In reply to David FriesI noticed there was a problem here because Smatch complained:
drivers/w1/slaves/w1_therm.c:416 w1_seq_show() warn: inconsistent returns 'mutex:&sl->master->mutex'. Locked on: line 416 Unlocked on: line 413
The problem is that we lock ->mutex but we unlock ->bus_mutex on error. David Fries says that ->bus_mutex is correct and ->mutex is incorrect.
---
v2: In the first version I changed ->bus_mutext to ->mutex instead of
the other way around.
drivers/w1/slaves/w1_therm.c +2 -2
@@ -355,7 +355,7 @@ static ssize_t w1_seq_show(struct device *device,struct w1_reg_num *reg_num;int seq =0 ; -mutex_lock (&sl->master->mutex); +mutex_lock (&sl->master->bus_mutex);/* Place all devices in CHAIN state */ if (w1_reset_bus (sl->master)) goto error;
@@ -407,7 +407,7 @@ static ssize_t w1_seq_show(struct device *device, ack =w1_read_8 (sl->master);if (ack !=W1_42_SUCCESS_CONFIRM_BYTE ) goto error; -mutex_unlock (&sl->master->mutex); +mutex_unlock (&sl->master->bus_mutex); c -=snprintf (buf +PAGE_SIZE - c, c,"%d\n" , seq);return PAGE_SIZE - c;
Re: [patch] [patch 2/2 v2] w1: use correct lock on error in w1_seq_show()
Hi
04.06.2015, 12:04, "Dan Carpenter" <[email protected]>:
Quoted 7 lines
I noticed there was a problem here because Smatch complained:
ššššššššdrivers/w1/slaves/w1_therm.c:416
w1_seq_show()warn:
ššššššššinconsistent returns 'mutex:&sl->master->mutex'. ššššššššššLocked on: line 416
ššššššššššUnlocked on: line 413
Quoted 5 lines
The problem is that we lock ->mutex but we unlock ->bus_mutex on error. David Fries says that ->bus_mutex is correct and ->mutex is incorrect.
Fixes d9411e57dc7f ('w1: Add support for DS28EA00 sequence to w1-therm')Signed-off-by Dan Carpenter <[email protected]>
Looks good to me, Greg please pull this serie into your tree, if you hadn't yet. Am I right that this is a stable tree material too?
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [patch 2/2 v2] w1: use correct lock on error in w1_seq_show()
Quoted 10 lines · Evgeniy Polyakov
Hi
04.06.2015, 12:04, "Dan Carpenter" <[email protected]>:
Quoted 7 lines
I noticed there was a problem here because Smatch complained:
drivers/w1/slaves/w1_therm.c:416
w1_seq_show()warn:
inconsistent returns 'mutex:&sl->master->mutex'. Locked on: line 416
Unlocked on: line 413
Quoted 8 lines
Quoted 5 lines
The problem is that we lock ->mutex but we unlock ->bus_mutex on error. David Fries says that ->bus_mutex is correct and ->mutex is incorrect.
Fixes d9411e57dc7f ('w1: Add support for DS28EA00 sequence to w1-therm')Signed-off-by Dan Carpenter <[email protected]>Looks good to me, Greg please pull this serie into your tree, if you hadn't yet. Am I right that this is a stable tree material too?
I would expect the answer to be no. This is a fix to a new feature that is in gregkh/char-misc but not yet in Linus's tree.
Signature · 5 lines
David Fries <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html