Didn't get any feedback or review comments on this patch. Resending ...
P.
---8<---
The iwlwifi driver implements a thermal zone and hwmon device, but
returns -EIO on temperature reads if the firmware isn't loaded. This
results in the error
iwlwifi-virtual-0
Adapter: Virtual device
ERROR: Can't get value of subfeature temp1_input: I/O error
temp1: N/A
being output when using sensors from the lm-sensors package. Since
the temperature cannot be read unless the ucode is loaded there is no
reason to add the interface only to have it return an error 100% of
the time.
This patch moves the firmware check to iwl_mvm_thermal_zone_register() and
stops the thermal zone from being created if the ucode hasn't been loaded.
Signed-off-by: Prarit Bhargava <redacted>
Cc: Johannes Berg <redacted>
Cc: Emmanuel Grumbach <redacted>
Cc: Luca Coelho <redacted>
Cc: Intel Linux Wireless <redacted>
Cc: Kalle Valo <redacted>
Cc: Chaya Rachel Ivgi <redacted>
Cc: Sara Sharon <redacted>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
On Mon, 2016-07-11 at 11:18 -0400, Prarit Bhargava wrote:
quoted
Didn't get any feedback or review comments on this patch. Resending
...
P.
Sorry, this got flooded down my inbox.
NP, Luciano -- My worry was that it hadn't been seen or didn't make it out to
the list.
I'm being a bit impatient too ;)
P.
quoted
---8<---
The iwlwifi driver implements a thermal zone and hwmon device, but
returns -EIO on temperature reads if the firmware isn't loaded. This
results in the error
iwlwifi-virtual-0
Adapter: Virtual device
ERROR: Can't get value of subfeature temp1_input: I/O error
temp1: N/A
being output when using sensors from the lm-sensors package. Since
the temperature cannot be read unless the ucode is loaded there is no
reason to add the interface only to have it return an error 100% of
the time.
This patch moves the firmware check to
iwl_mvm_thermal_zone_register() and
stops the thermal zone from being created if the ucode hasn't been
loaded.
Signed-off-by: Prarit Bhargava <redacted>
Cc: Johannes Berg <redacted>
Cc: Emmanuel Grumbach <redacted>
Cc: Luca Coelho <redacted>
Cc: Intel Linux Wireless <redacted>
Cc: Kalle Valo <redacted>
Cc: Chaya Rachel Ivgi <redacted>
Cc: Sara Sharon <redacted>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
I have now sent it for review on our internal tree.
--
Luca.
On Mon, Jul 11, 2016 at 6:18 PM, Prarit Bhargava [off-list ref] wrote:
Didn't get any feedback or review comments on this patch. Resending ...
P.
This change is obviously completely broken. It simply disables the
registration to thermal zone core.
quoted hunk
---8<---
The iwlwifi driver implements a thermal zone and hwmon device, but
returns -EIO on temperature reads if the firmware isn't loaded. This
results in the error
iwlwifi-virtual-0
Adapter: Virtual device
ERROR: Can't get value of subfeature temp1_input: I/O error
temp1: N/A
being output when using sensors from the lm-sensors package. Since
the temperature cannot be read unless the ucode is loaded there is no
reason to add the interface only to have it return an error 100% of
the time.
This patch moves the firmware check to iwl_mvm_thermal_zone_register() and
stops the thermal zone from being created if the ucode hasn't been loaded.
Signed-off-by: Prarit Bhargava <redacted>
Cc: Johannes Berg <redacted>
Cc: Emmanuel Grumbach <redacted>
Cc: Luca Coelho <redacted>
Cc: Intel Linux Wireless <redacted>
Cc: Kalle Valo <redacted>
Cc: Chaya Rachel Ivgi <redacted>
Cc: Sara Sharon <redacted>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
1.7.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 11, 2016 at 6:18 PM, Prarit Bhargava [off-list ref] wrote:
quoted
Didn't get any feedback or review comments on this patch. Resending ...
P.
This change is obviously completely broken. It simply disables the
registration to thermal zone core.
No it is not broken, and yes, that is exactly what should happen IMO.
The problem is that the iwlwifi driver implements the thermal zone even when the
device doesn't support it.
As can be seen in the current code base, iwl_mvm_tzone_get_temp() will return
-EIO 100% of the time when the firmware doesn't support reading the
temperature[1]. In this case a read of sysfs will result in a return of -EIO,
and this breaks existing userspace programs such as lm-sensors (which by all
accounts is bad to do).
Note that in my patch I have removed the -EIO return in favor of not registering
the non-existent thermal zone. I'm not removing any functionality by changing
this, nor am I adding functionality. In both cases the thermal zone is not
functional, and with my patch userspace continues to work.
P.
[1] iwl_mvm_tzone_set_trip_temp() also returns -EIO, so setting and getting of
the temperature is non-functional.
quoted
---8<---
The iwlwifi driver implements a thermal zone and hwmon device, but
returns -EIO on temperature reads if the firmware isn't loaded. This
results in the error
iwlwifi-virtual-0
Adapter: Virtual device
ERROR: Can't get value of subfeature temp1_input: I/O error
temp1: N/A
being output when using sensors from the lm-sensors package. Since
the temperature cannot be read unless the ucode is loaded there is no
reason to add the interface only to have it return an error 100% of
the time.
This patch moves the firmware check to iwl_mvm_thermal_zone_register() and
stops the thermal zone from being created if the ucode hasn't been loaded.
Signed-off-by: Prarit Bhargava <redacted>
Cc: Johannes Berg <redacted>
Cc: Emmanuel Grumbach <redacted>
Cc: Luca Coelho <redacted>
Cc: Intel Linux Wireless <redacted>
Cc: Kalle Valo <redacted>
Cc: Chaya Rachel Ivgi <redacted>
Cc: Sara Sharon <redacted>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
1.7.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2016-07-11 at 14:19 -0400, Prarit Bhargava wrote:
quoted
On 07/11/2016 02:00 PM, Emmanuel Grumbach wrote:
quoted
On Mon, Jul 11, 2016 at 6:18 PM, Prarit Bhargava <prarit@redhat.com
quoted
wrote:
Didn't get any feedback or review comments on this patch.
Resending ...
P.
This change is obviously completely broken. It simply disables the
registration to thermal zone core.
No it is not broken, and yes, that is exactly what should happen IMO.
The problem is that the iwlwifi driver implements the thermal zone
even when the
device doesn't support it.
We implement thermal zone because we do support it, but the problem is
that we need the firmware to be loaded for that. So you can argue that
we should register *later* when the firmware is loaded. But this is
really not helping all that much because the firmware can also be
stopped at any time. So you'd want us to register / unregister the
thermal zone anytime the firmware is loaded / unloaded?
You might have to do that. I think that if the firmware enables a feature then
the act of loading the firmware should run the code that enables the feature.
IMO of course.
I guess that works, but it seems wrong to me. Usually, registration
should happen only upon INIT, and yes, at that time the firmware is not
ready to provide the information yet.
Maybe returning -EBUSY would help lm-sensors not to get confused?
I'll give that a shot, but I expect that won't work either as an error message
will still be displayed.
quoted
As can be seen in the current code base, iwl_mvm_tzone_get_temp()
will return
-EIO 100% of the time when the firmware doesn't support reading the
temperature[1]. In this case a read of sysfs will result in a return
of -EIO,
and this breaks existing userspace programs such as lm-sensors (which
by all
accounts is bad to do).
Right, but I don't understand why the userspace is broken because of
that?
Before the iwlwifi change, sensors successfully returned. Now, because of the
error, it doesn't.
Unless we register / unregister anytime the firmware is loaded, I
don't see any proper way to fix this. And yes, I'd expect the userspace
to handle gracefully failures in its requests.
I agree with you in principle *and there's a great many things I wish userspace
would do gracefully* but updating the kernel shouldn't result in userspace
programs failing.
quoted
Note that in my patch I have removed the -EIO return in favor of not
registering
the non-existent thermal zone. I'm not removing any functionality by
changing
this, nor am I adding functionality. In both cases the thermal zone
is not
functional, and with my patch userspace continues to work.
You are removing the thermal zone functionality since even when the
firmware will be loaded (which typically happens fairly quickly),
thermal zone won't work.
Then I agree with your suggestion above that you need to enable the thermal zone
on a successful load of the firmware. [Aside: I wonder what other drivers do in
this situation? While this does seem like an odd case, I can't believe that the
iwlwifi driver is the only driver to enable features based on firmware.]
P.
On Mon, Jul 11, 2016 at 06:27:30PM +0000, Grumbach, Emmanuel wrote:
I guess that works, but it seems wrong to me. Usually, registration
should happen only upon INIT, and yes, at that time the firmware is not
ready to provide the information yet.
<snip>
quoted
As can be seen in the current code base, iwl_mvm_tzone_get_temp()
will return
-EIO 100% of the time when the firmware doesn't support reading the
If I understad correctly this error happen 100% of the time, not only
during init. Hence seems there is an issue here, i.e. cur_ucode is not
marked correctly as IWL_UCODE_REGULAR or iwl_mvm_get_temp() fail
100% of the time (iwl_mvm_is_tt_in_fw() incorrecly return true on
Prarit device ? ).
BTW, you implement thermal_zone device, but do you also need hwmon
device? Perhaps using theramal_zone_params no_hwmon option would be
proper here?
Stanislaw
On Mon, Jul 11, 2016 at 06:27:30PM +0000, Grumbach, Emmanuel wrote:
quoted
I guess that works, but it seems wrong to me. Usually, registration
should happen only upon INIT, and yes, at that time the firmware is
not ready to provide the information yet.
<snip>
quoted
quoted
As can be seen in the current code base, iwl_mvm_tzone_get_temp()
will return -EIO 100% of the time when the firmware doesn't support
reading the
If I understad correctly this error happen 100% of the time, not only during
init. Hence seems there is an issue here, i.e. cur_ucode is not marked
correctly as IWL_UCODE_REGULAR or iwl_mvm_get_temp() fail 100% of the
time (iwl_mvm_is_tt_in_fw() incorrecly return true on Prarit device ? ).
Cur_ucode will not be IWL_UCODE_REGULAR until you load the firmware which
will happen upon ifup.
BTW, you implement thermal_zone device, but do you also need hwmon
device? Perhaps using theramal_zone_params no_hwmon option would be
proper here?
That's an interesting direction. I'd have to check, but TBH, I am not familiar with
that code. Luca was very involved during the development but he is not available
right now. I will be back more the less when the merge window will close :)
On Thu, Jul 14, 2016 at 09:44:22AM +0000, Grumbach, Emmanuel wrote:
quoted
If I understad correctly this error happen 100% of the time, not only during
init. Hence seems there is an issue here, i.e. cur_ucode is not marked
correctly as IWL_UCODE_REGULAR or iwl_mvm_get_temp() fail 100% of the
time (iwl_mvm_is_tt_in_fw() incorrecly return true on Prarit device ? ).
Cur_ucode will not be IWL_UCODE_REGULAR until you load the firmware which
will happen upon ifup.
Then creating thermal_device on ifup looks more reasonable to me.
Otherwise we can create device that can be non-functional virtually
forever, i.e. when soft RFKILL is enabled. However I admit that
creating thermal_device when HW is detected has some advantages
too.
Stanislaw
On Thu, Jul 14, 2016 at 09:44:22AM +0000, Grumbach, Emmanuel wrote:
quoted
quoted
If I understad correctly this error happen 100% of the time, not only during
init. Hence seems there is an issue here, i.e. cur_ucode is not marked
correctly as IWL_UCODE_REGULAR or iwl_mvm_get_temp() fail 100% of the
time (iwl_mvm_is_tt_in_fw() incorrecly return true on Prarit device ? ).
Cur_ucode will not be IWL_UCODE_REGULAR until you load the firmware which
will happen upon ifup.
Then creating thermal_device on ifup looks more reasonable to me.
Otherwise we can create device that can be non-functional virtually
forever, i.e. when soft RFKILL is enabled. However I admit that
creating thermal_device when HW is detected has some advantages
too.
That's my plan right now. Unfortunately something else in the kernel seems
recently broken and is preventing me from testing. I will get back to this
early next week.
P.
On Thu, Jul 14, 2016 at 09:44:22AM +0000, Grumbach, Emmanuel wrote:
quoted
quoted
If I understad correctly this error happen 100% of the time, not
only during init. Hence seems there is an issue here, i.e. cur_ucode
is not marked correctly as IWL_UCODE_REGULAR or
iwl_mvm_get_temp()
quoted
quoted
quoted
fail 100% of the time (iwl_mvm_is_tt_in_fw() incorrecly return true on
Prarit device ? ).
quoted
quoted
Cur_ucode will not be IWL_UCODE_REGULAR until you load the firmware
which will happen upon ifup.
Then creating thermal_device on ifup looks more reasonable to me.
Otherwise we can create device that can be non-functional virtually
forever, i.e. when soft RFKILL is enabled. However I admit that
creating thermal_device when HW is detected has some advantages too.
That's my plan right now. Unfortunately something else in the kernel seems
recently broken and is preventing me from testing. I will get back to this
early next week.
But we already said that this won't work since you may have the device enabled upon boot and then disabled. So unless you unregister the thermal zone subsystem upon wifi disable, you won't solve the problem. Kalle and Luca already refused that solution.
I glanced (again) at the thermal zone API and since it allows to return an int, the subsystem itself should handle the failures and / or the userspace problems. The API itself is awful, it has no documentation whatsoever, even not variable names, but only types... You can't really blame the subsystem users to assume that a method that can return an int can't fail where the out values is passed by a pointer. Of course, you have to guess that this is the expected behavior, since you don't have any hint about the meaning of the parameters.
I think that the right place to "fix" this problem is to fix the subsystem. This way, you will fix it for iwlwifi and for any (future) other users that may fall into the trap opened by the API itself.