[PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Prarit Bhargava]

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 <prarit@redhat.com>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc: Luca Coelho <luciano.coelho@intel.com>
Cc: Intel Linux Wireless <linuxwifi@intel.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Chaya Rachel Ivgi <chaya.rachel.ivgi@intel.com>
Cc: Sara Sharon <sara.sharon@intel.com>
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(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
index 58fc7b3c711c..64802659711f 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
@@ -634,11 +634,6 @@ static int iwl_mvm_tzone_get_temp(struct thermal_zone_device *device,
 
 	mutex_lock(&mvm->mutex);
 
-	if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR)) {
-		ret = -EIO;
-		goto out;
-	}
-
 	ret = iwl_mvm_get_temp(mvm, &temp);
 	if (ret)
 		goto out;
@@ -684,11 +679,6 @@ static int iwl_mvm_tzone_set_trip_temp(struct thermal_zone_device *device,
 
 	mutex_lock(&mvm->mutex);
 
-	if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR)) {
-		ret = -EIO;
-		goto out;
-	}
-
 	if (trip < 0 || trip >= IWL_MAX_DTS_TRIPS) {
 		ret = -EINVAL;
 		goto out;
@@ -750,6 +740,9 @@ static void iwl_mvm_thermal_zone_register(struct iwl_mvm *mvm)
 		return;
 	}
 
+	if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR))
+		return;
+
 	BUILD_BUG_ON(ARRAY_SIZE(name) >= THERMAL_NAME_LENGTH);
 
 	mvm->tz_device.tzone = thermal_zone_device_register(name,
-- 
1.7.9.3

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Coelho, Luciano]

T24gTW9uLCAyMDE2LTA3LTExIGF0IDExOjE4IC0wNDAwLCBQcmFyaXQgQmhhcmdhdmEgd3JvdGU6
DQo+IERpZG4ndCBnZXQgYW55IGZlZWRiYWNrIG9yIHJldmlldyBjb21tZW50cyBvbiB0aGlzIHBh
dGNoLsKgwqBSZXNlbmRpbmcNCj4gLi4uDQo+IA0KPiBQLg0KDQpTb3JyeSwgdGhpcyBnb3QgZmxv
b2RlZCBkb3duIG15IGluYm94Lg0KDQoNCj4gLS0tODwtLS0NCj4gDQo+IFRoZSBpd2x3aWZpIGRy
aXZlciBpbXBsZW1lbnRzIGEgdGhlcm1hbCB6b25lIGFuZCBod21vbiBkZXZpY2UsIGJ1dA0KPiBy
ZXR1cm5zIC1FSU8gb24gdGVtcGVyYXR1cmUgcmVhZHMgaWYgdGhlIGZpcm13YXJlIGlzbid0IGxv
YWRlZC7CoMKgVGhpcw0KPiByZXN1bHRzIGluIHRoZSBlcnJvcg0KPiANCj4gaXdsd2lmaS12aXJ0
dWFsLTANCj4gQWRhcHRlcjogVmlydHVhbCBkZXZpY2UNCj4gRVJST1I6IENhbid0IGdldCB2YWx1
ZSBvZiBzdWJmZWF0dXJlIHRlbXAxX2lucHV0OiBJL08gZXJyb3INCj4gdGVtcDE6wqDCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgTi9BDQo+IA0KPiBiZWluZyBvdXRwdXQgd2hlbiB1c2luZyBzZW5zb3Jz
IGZyb20gdGhlIGxtLXNlbnNvcnMgcGFja2FnZS7CoMKgU2luY2UNCj4gdGhlIHRlbXBlcmF0dXJl
IGNhbm5vdCBiZSByZWFkIHVubGVzcyB0aGUgdWNvZGUgaXMgbG9hZGVkIHRoZXJlIGlzIG5vDQo+
IHJlYXNvbiB0byBhZGQgdGhlIGludGVyZmFjZSBvbmx5IHRvIGhhdmUgaXQgcmV0dXJuIGFuIGVy
cm9yIDEwMCUgb2YNCj4gdGhlIHRpbWUuDQo+IA0KPiBUaGlzIHBhdGNoIG1vdmVzIHRoZSBmaXJt
d2FyZSBjaGVjayB0bw0KPiBpd2xfbXZtX3RoZXJtYWxfem9uZV9yZWdpc3RlcigpIGFuZA0KPiBz
dG9wcyB0aGUgdGhlcm1hbCB6b25lIGZyb20gYmVpbmcgY3JlYXRlZCBpZiB0aGUgdWNvZGUgaGFz
bid0IGJlZW4NCj4gbG9hZGVkLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogUHJhcml0IEJoYXJnYXZh
IDxwcmFyaXRAcmVkaGF0LmNvbT4NCj4gQ2M6IEpvaGFubmVzIEJlcmcgPGpvaGFubmVzLmJlcmdA
aW50ZWwuY29tPg0KPiBDYzogRW1tYW51ZWwgR3J1bWJhY2ggPGVtbWFudWVsLmdydW1iYWNoQGlu
dGVsLmNvbT4NCj4gQ2M6IEx1Y2EgQ29lbGhvIDxsdWNpYW5vLmNvZWxob0BpbnRlbC5jb20+DQo+
IENjOiBJbnRlbCBMaW51eCBXaXJlbGVzcyA8bGludXh3aWZpQGludGVsLmNvbT4NCj4gQ2M6IEth
bGxlIFZhbG8gPGt2YWxvQGNvZGVhdXJvcmEub3JnPg0KPiBDYzogQ2hheWEgUmFjaGVsIEl2Z2kg
PGNoYXlhLnJhY2hlbC5pdmdpQGludGVsLmNvbT4NCj4gQ2M6IFNhcmEgU2hhcm9uIDxzYXJhLnNo
YXJvbkBpbnRlbC5jb20+DQo+IENjOiBsaW51eC13aXJlbGVzc0B2Z2VyLmtlcm5lbC5vcmcNCj4g
Q2M6IG5ldGRldkB2Z2VyLmtlcm5lbC5vcmcNCj4gLS0tDQoNCkkgaGF2ZSBub3cgc2VudCBpdCBm
b3IgcmV2aWV3IG9uIG91ciBpbnRlcm5hbCB0cmVlLg0KDQotLQ0KTHVjYS4=

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Coelho, Luciano]

On Mon, 2016-07-11 at 11:18 -0400, Prarit Bhargava wrote:
> Didn't get any feedback or review comments on this patch.  Resending
> ...
> 
> P.

Sorry, this got flooded down my inbox.


> ---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 <prarit@redhat.com>
> Cc: Johannes Berg <johannes.berg@intel.com>
> Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> Cc: Luca Coelho <luciano.coelho@intel.com>
> Cc: Intel Linux Wireless <linuxwifi@intel.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: Chaya Rachel Ivgi <chaya.rachel.ivgi@intel.com>
> Cc: Sara Sharon <sara.sharon@intel.com>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---

I have now sent it for review on our internal tree.

--
Luca.

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Prarit Bhargava]

On 07/11/2016 12:07 PM, Coelho, Luciano wrote:
> On Mon, 2016-07-11 at 11:18 -0400, Prarit Bhargava wrote:
>> 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.

> 
> 
>> ---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 <prarit@redhat.com>
>> Cc: Johannes Berg <johannes.berg@intel.com>
>> Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
>> Cc: Luca Coelho <luciano.coelho@intel.com>
>> Cc: Intel Linux Wireless <linuxwifi@intel.com>
>> Cc: Kalle Valo <kvalo@codeaurora.org>
>> Cc: Chaya Rachel Ivgi <chaya.rachel.ivgi@intel.com>
>> Cc: Sara Sharon <sara.sharon@intel.com>
>> Cc: linux-wireless@vger.kernel.org
>> Cc: netdev@vger.kernel.org
>> ---
> 
> I have now sent it for review on our internal tree.
> 
> --
> Luca.
> 

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Emmanuel Grumbach]

On Mon, Jul 11, 2016 at 6:18 PM, Prarit Bhargava <prarit@redhat.com> 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.

>
> ---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 <prarit@redhat.com>
> Cc: Johannes Berg <johannes.berg@intel.com>
> Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> Cc: Luca Coelho <luciano.coelho@intel.com>
> Cc: Intel Linux Wireless <linuxwifi@intel.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: Chaya Rachel Ivgi <chaya.rachel.ivgi@intel.com>
> Cc: Sara Sharon <sara.sharon@intel.com>
> 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(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> index 58fc7b3c711c..64802659711f 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> @@ -634,11 +634,6 @@ static int iwl_mvm_tzone_get_temp(struct thermal_zone_device *device,
>
>         mutex_lock(&mvm->mutex);
>
> -       if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR)) {
> -               ret = -EIO;
> -               goto out;
> -       }
> -
>         ret = iwl_mvm_get_temp(mvm, &temp);
>         if (ret)
>                 goto out;
> @@ -684,11 +679,6 @@ static int iwl_mvm_tzone_set_trip_temp(struct thermal_zone_device *device,
>
>         mutex_lock(&mvm->mutex);
>
> -       if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR)) {
> -               ret = -EIO;
> -               goto out;
> -       }
> -
>         if (trip < 0 || trip >= IWL_MAX_DTS_TRIPS) {
>                 ret = -EINVAL;
>                 goto out;
> @@ -750,6 +740,9 @@ static void iwl_mvm_thermal_zone_register(struct iwl_mvm *mvm)
>                 return;
>         }
>
> +       if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR))
> +               return;
> +
>         BUILD_BUG_ON(ARRAY_SIZE(name) >= THERMAL_NAME_LENGTH);
>
>         mvm->tz_device.tzone = thermal_zone_device_register(name,
> --
> 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

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Prarit Bhargava]

On 07/11/2016 02:00 PM, Emmanuel Grumbach wrote:
> On Mon, Jul 11, 2016 at 6:18 PM, Prarit Bhargava <prarit@redhat.com> 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.

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.


> 
>>
>> ---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 <prarit@redhat.com>
>> Cc: Johannes Berg <johannes.berg@intel.com>
>> Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
>> Cc: Luca Coelho <luciano.coelho@intel.com>
>> Cc: Intel Linux Wireless <linuxwifi@intel.com>
>> Cc: Kalle Valo <kvalo@codeaurora.org>
>> Cc: Chaya Rachel Ivgi <chaya.rachel.ivgi@intel.com>
>> Cc: Sara Sharon <sara.sharon@intel.com>
>> 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(-)
>>
>> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
>> index 58fc7b3c711c..64802659711f 100644
>> --- a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
>> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
>> @@ -634,11 +634,6 @@ static int iwl_mvm_tzone_get_temp(struct thermal_zone_device *device,
>>
>>         mutex_lock(&mvm->mutex);
>>
>> -       if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR)) {
>> -               ret = -EIO;
>> -               goto out;
>> -       }
>> -
>>         ret = iwl_mvm_get_temp(mvm, &temp);
>>         if (ret)
>>                 goto out;
>> @@ -684,11 +679,6 @@ static int iwl_mvm_tzone_set_trip_temp(struct thermal_zone_device *device,
>>
>>         mutex_lock(&mvm->mutex);
>>
>> -       if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR)) {
>> -               ret = -EIO;
>> -               goto out;
>> -       }
>> -
>>         if (trip < 0 || trip >= IWL_MAX_DTS_TRIPS) {
>>                 ret = -EINVAL;
>>                 goto out;
>> @@ -750,6 +740,9 @@ static void iwl_mvm_thermal_zone_register(struct iwl_mvm *mvm)
>>                 return;
>>         }
>>
>> +       if (!mvm->ucode_loaded || !(mvm->cur_ucode == IWL_UCODE_REGULAR))
>> +               return;
>> +
>>         BUILD_BUG_ON(ARRAY_SIZE(name) >= THERMAL_NAME_LENGTH);
>>
>>         mvm->tz_device.tzone = thermal_zone_device_register(name,
>> --
>> 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

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Grumbach, Emmanuel]

T24gTW9uLCAyMDE2LTA3LTExIGF0IDE0OjE5IC0wNDAwLCBQcmFyaXQgQmhhcmdhdmEgd3JvdGU6
DQo+IA0KPiBPbiAwNy8xMS8yMDE2IDAyOjAwIFBNLCBFbW1hbnVlbCBHcnVtYmFjaCB3cm90ZToN
Cj4gPiBPbiBNb24sIEp1bCAxMSwgMjAxNiBhdCA2OjE4IFBNLCBQcmFyaXQgQmhhcmdhdmEgPHBy
YXJpdEByZWRoYXQuY29tDQo+ID4gPiB3cm90ZToNCj4gPiA+IA0KPiA+ID4gRGlkbid0IGdldCBh
bnkgZmVlZGJhY2sgb3IgcmV2aWV3IGNvbW1lbnRzIG9uIHRoaXMgcGF0Y2guIA0KPiA+ID4gIFJl
c2VuZGluZyAuLi4NCj4gPiA+IA0KPiA+ID4gUC4NCj4gPiANCj4gPiBUaGlzIGNoYW5nZSBpcyBv
YnZpb3VzbHkgY29tcGxldGVseSBicm9rZW4uIEl0IHNpbXBseSBkaXNhYmxlcyB0aGUNCj4gPiBy
ZWdpc3RyYXRpb24gdG8gdGhlcm1hbCB6b25lIGNvcmUuDQo+IA0KPiBObyBpdCBpcyBub3QgYnJv
a2VuLCBhbmQgeWVzLCB0aGF0IGlzIGV4YWN0bHkgd2hhdCBzaG91bGQgaGFwcGVuIElNTy4NCj4g
DQo+IFRoZSBwcm9ibGVtIGlzIHRoYXQgdGhlIGl3bHdpZmkgZHJpdmVyIGltcGxlbWVudHMgdGhl
IHRoZXJtYWwgem9uZQ0KPiBldmVuIHdoZW4gdGhlDQo+IGRldmljZSBkb2Vzbid0IHN1cHBvcnQg
aXQuDQoNCldlIGltcGxlbWVudCB0aGVybWFsIHpvbmUgYmVjYXVzZSB3ZSBkbyBzdXBwb3J0IGl0
LCBidXQgdGhlIHByb2JsZW0gaXMNCnRoYXQgd2UgbmVlZCB0aGUgZmlybXdhcmUgdG8gYmUgbG9h
ZGVkIGZvciB0aGF0LiBTbyB5b3UgY2FuIGFyZ3VlIHRoYXQNCndlIHNob3VsZCByZWdpc3RlciAq
bGF0ZXIqIHdoZW4gdGhlIGZpcm13YXJlIGlzIGxvYWRlZC4gQnV0IHRoaXMgaXMNCnJlYWxseSBu
b3QgaGVscGluZyBhbGwgdGhhdCBtdWNoIGJlY2F1c2UgdGhlIGZpcm13YXJlIGNhbiBhbHNvIGJl
DQpzdG9wcGVkIGF0IGFueSB0aW1lLiBTbyB5b3UnZCB3YW50IHVzIHRvIHJlZ2lzdGVyIC8gdW5y
ZWdpc3RlciB0aGUNCnRoZXJtYWwgem9uZSBhbnl0aW1lIHRoZSBmaXJtd2FyZSBpcyBsb2FkZWQg
LyB1bmxvYWRlZD8NCkkgZ3Vlc3MgdGhhdCB3b3JrcywgYnV0IGl0IHNlZW1zIHdyb25nIHRvIG1l
LiBVc3VhbGx5LCByZWdpc3RyYXRpb24NCnNob3VsZCBoYXBwZW4gb25seSB1cG9uIElOSVQsIGFu
ZCB5ZXMsIGF0IHRoYXQgdGltZSB0aGUgZmlybXdhcmUgaXMgbm90DQpyZWFkeSB0byBwcm92aWRl
IHRoZSBpbmZvcm1hdGlvbiB5ZXQuDQpNYXliZSByZXR1cm5pbmcgLUVCVVNZIHdvdWxkIGhlbHAg
bG0tc2Vuc29ycyBub3QgdG8gZ2V0IGNvbmZ1c2VkPw0KDQo+IA0KPiBBcyBjYW4gYmUgc2VlbiBp
biB0aGUgY3VycmVudCBjb2RlIGJhc2UsIGl3bF9tdm1fdHpvbmVfZ2V0X3RlbXAoKQ0KPiB3aWxs
IHJldHVybg0KPiAtRUlPIDEwMCUgb2YgdGhlIHRpbWUgd2hlbiB0aGUgZmlybXdhcmUgZG9lc24n
dCBzdXBwb3J0IHJlYWRpbmcgdGhlDQo+IHRlbXBlcmF0dXJlWzFdLiAgSW4gdGhpcyBjYXNlIGEg
cmVhZCBvZiBzeXNmcyB3aWxsIHJlc3VsdCBpbiBhIHJldHVybg0KPiBvZiAtRUlPLA0KPiBhbmQg
dGhpcyBicmVha3MgZXhpc3RpbmcgdXNlcnNwYWNlIHByb2dyYW1zIHN1Y2ggYXMgbG0tc2Vuc29y
cyAod2hpY2gNCj4gYnkgYWxsDQo+IGFjY291bnRzIGlzIGJhZCB0byBkbykuDQoNClJpZ2h0LCBi
dXQgSSBkb24ndCB1bmRlcnN0YW5kIHdoeSB0aGUgdXNlcnNwYWNlIGlzIGJyb2tlbiBiZWNhdXNl
IG9mDQp0aGF0PyBVbmxlc3Mgd2UgcmVnaXN0ZXIgLyB1bnJlZ2lzdGVyIGFueXRpbWUgdGhlIGZp
cm13YXJlIGlzIGxvYWRlZCwgSQ0KZG9uJ3Qgc2VlIGFueSBwcm9wZXIgd2F5IHRvIGZpeCB0aGlz
LiBBbmQgeWVzLCBJJ2QgZXhwZWN0IHRoZSB1c2Vyc3BhY2UNCnRvIGhhbmRsZSBncmFjZWZ1bGx5
IGZhaWx1cmVzIGluIGl0cyByZXF1ZXN0cy4NCg0KPiANCj4gTm90ZSB0aGF0IGluIG15IHBhdGNo
IEkgaGF2ZSByZW1vdmVkIHRoZSAtRUlPIHJldHVybiBpbiBmYXZvciBvZiBub3QNCj4gcmVnaXN0
ZXJpbmcNCj4gdGhlIG5vbi1leGlzdGVudCB0aGVybWFsIHpvbmUuICBJJ20gbm90IHJlbW92aW5n
IGFueSBmdW5jdGlvbmFsaXR5IGJ5DQo+IGNoYW5naW5nDQo+IHRoaXMsIG5vciBhbSBJIGFkZGlu
ZyBmdW5jdGlvbmFsaXR5LiAgSW4gYm90aCBjYXNlcyB0aGUgdGhlcm1hbCB6b25lDQo+IGlzIG5v
dA0KPiBmdW5jdGlvbmFsLCBhbmQgd2l0aCBteSBwYXRjaCB1c2Vyc3BhY2UgY29udGludWVzIHRv
IHdvcmsuDQoNCllvdSBhcmUgcmVtb3ZpbmcgdGhlIHRoZXJtYWwgem9uZSBmdW5jdGlvbmFsaXR5
IHNpbmNlIGV2ZW4gd2hlbiB0aGUNCmZpcm13YXJlIHdpbGwgYmUgbG9hZGVkICh3aGljaCB0eXBp
Y2FsbHkgaGFwcGVucyBmYWlybHkgcXVpY2tseSksDQp0aGVybWFsIHpvbmUgd29uJ3Qgd29yay4N
Cg0KPiANCj4gUC4NCj4gDQo+IFsxXSBpd2xfbXZtX3R6b25lX3NldF90cmlwX3RlbXAoKSBhbHNv
IHJldHVybnMgLUVJTywgc28gc2V0dGluZyBhbmQNCj4gZ2V0dGluZyBvZg0KPiB0aGUgdGVtcGVy
YXR1cmUgaXMgbm9uLWZ1bmN0aW9uYWwuDQo+IA0KPiANCj4gPiANCj4gPiA+IA0KPiA+ID4gLS0t
ODwtLS0NCj4gPiA+IA0KPiA+ID4gVGhlIGl3bHdpZmkgZHJpdmVyIGltcGxlbWVudHMgYSB0aGVy
bWFsIHpvbmUgYW5kIGh3bW9uIGRldmljZSwNCj4gPiA+IGJ1dA0KPiA+ID4gcmV0dXJucyAtRUlP
IG9uIHRlbXBlcmF0dXJlIHJlYWRzIGlmIHRoZSBmaXJtd2FyZSBpc24ndCBsb2FkZWQuIA0KPiA+
ID4gIFRoaXMNCj4gPiA+IHJlc3VsdHMgaW4gdGhlIGVycm9yDQo+ID4gPiANCj4gPiA+IGl3bHdp
ZmktdmlydHVhbC0wDQo+ID4gPiBBZGFwdGVyOiBWaXJ0dWFsIGRldmljZQ0KPiA+ID4gRVJST1I6
IENhbid0IGdldCB2YWx1ZSBvZiBzdWJmZWF0dXJlIHRlbXAxX2lucHV0OiBJL08gZXJyb3INCj4g
PiA+IHRlbXAxOiAgICAgICAgICAgIE4vQQ0KPiA+ID4gDQo+ID4gPiBiZWluZyBvdXRwdXQgd2hl
biB1c2luZyBzZW5zb3JzIGZyb20gdGhlIGxtLXNlbnNvcnMgcGFja2FnZS4gDQo+ID4gPiAgU2lu
Y2UNCj4gPiA+IHRoZSB0ZW1wZXJhdHVyZSBjYW5ub3QgYmUgcmVhZCB1bmxlc3MgdGhlIHVjb2Rl
IGlzIGxvYWRlZCB0aGVyZQ0KPiA+ID4gaXMgbm8NCj4gPiA+IHJlYXNvbiB0byBhZGQgdGhlIGlu
dGVyZmFjZSBvbmx5IHRvIGhhdmUgaXQgcmV0dXJuIGFuIGVycm9yIDEwMCUNCj4gPiA+IG9mDQo+
ID4gPiB0aGUgdGltZS4NCj4gPiA+IA0KPiA+ID4gVGhpcyBwYXRjaCBtb3ZlcyB0aGUgZmlybXdh
cmUgY2hlY2sgdG8NCj4gPiA+IGl3bF9tdm1fdGhlcm1hbF96b25lX3JlZ2lzdGVyKCkgYW5kDQo+
ID4gPiBzdG9wcyB0aGUgdGhlcm1hbCB6b25lIGZyb20gYmVpbmcgY3JlYXRlZCBpZiB0aGUgdWNv
ZGUgaGFzbid0DQo+ID4gPiBiZWVuIGxvYWRlZC4NCj4gPiA+IA0KPiA+ID4gU2lnbmVkLW9mZi1i
eTogUHJhcml0IEJoYXJnYXZhIDxwcmFyaXRAcmVkaGF0LmNvbT4NCj4gPiA+IENjOiBKb2hhbm5l
cyBCZXJnIDxqb2hhbm5lcy5iZXJnQGludGVsLmNvbT4NCj4gPiA+IENjOiBFbW1hbnVlbCBHcnVt
YmFjaCA8ZW1tYW51ZWwuZ3J1bWJhY2hAaW50ZWwuY29tPg0KPiA+ID4gQ2M6IEx1Y2EgQ29lbGhv
IDxsdWNpYW5vLmNvZWxob0BpbnRlbC5jb20+DQo+ID4gPiBDYzogSW50ZWwgTGludXggV2lyZWxl
c3MgPGxpbnV4d2lmaUBpbnRlbC5jb20+DQo+ID4gPiBDYzogS2FsbGUgVmFsbyA8a3ZhbG9AY29k
ZWF1cm9yYS5vcmc+DQo+ID4gPiBDYzogQ2hheWEgUmFjaGVsIEl2Z2kgPGNoYXlhLnJhY2hlbC5p
dmdpQGludGVsLmNvbT4NCj4gPiA+IENjOiBTYXJhIFNoYXJvbiA8c2FyYS5zaGFyb25AaW50ZWwu
Y29tPg0KPiA+ID4gQ2M6IGxpbnV4LXdpcmVsZXNzQHZnZXIua2VybmVsLm9yZw0KPiA+ID4gQ2M6
IG5ldGRldkB2Z2VyLmtlcm5lbC5vcmcNCj4gPiA+IC0tLQ0KPiA+ID4gIGRyaXZlcnMvbmV0L3dp
cmVsZXNzL2ludGVsL2l3bHdpZmkvbXZtL3R0LmMgfCAgIDEzICsrKy0tLS0tLS0tLS0NCj4gPiA+
ICAxIGZpbGUgY2hhbmdlZCwgMyBpbnNlcnRpb25zKCspLCAxMCBkZWxldGlvbnMoLSkNCj4gPiA+
IA0KPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbmV0L3dpcmVsZXNzL2ludGVsL2l3bHdpZmkv
bXZtL3R0LmMNCj4gPiA+IGIvZHJpdmVycy9uZXQvd2lyZWxlc3MvaW50ZWwvaXdsd2lmaS9tdm0v
dHQuYw0KPiA+ID4gaW5kZXggNThmYzdiM2M3MTFjLi42NDgwMjY1OTcxMWYgMTAwNjQ0DQo+ID4g
PiAtLS0gYS9kcml2ZXJzL25ldC93aXJlbGVzcy9pbnRlbC9pd2x3aWZpL212bS90dC5jDQo+ID4g
PiArKysgYi9kcml2ZXJzL25ldC93aXJlbGVzcy9pbnRlbC9pd2x3aWZpL212bS90dC5jDQo+ID4g
PiBAQCAtNjM0LDExICs2MzQsNiBAQCBzdGF0aWMgaW50IGl3bF9tdm1fdHpvbmVfZ2V0X3RlbXAo
c3RydWN0DQo+ID4gPiB0aGVybWFsX3pvbmVfZGV2aWNlICpkZXZpY2UsDQo+ID4gPiANCj4gPiA+
ICAgICAgICAgbXV0ZXhfbG9jaygmbXZtLT5tdXRleCk7DQo+ID4gPiANCj4gPiA+IC0gICAgICAg
aWYgKCFtdm0tPnVjb2RlX2xvYWRlZCB8fCAhKG12bS0+Y3VyX3Vjb2RlID09DQo+ID4gPiBJV0xf
VUNPREVfUkVHVUxBUikpIHsNCj4gPiA+IC0gICAgICAgICAgICAgICByZXQgPSAtRUlPOw0KPiA+
ID4gLSAgICAgICAgICAgICAgIGdvdG8gb3V0Ow0KPiA+ID4gLSAgICAgICB9DQo+ID4gPiAtDQo+
ID4gPiAgICAgICAgIHJldCA9IGl3bF9tdm1fZ2V0X3RlbXAobXZtLCAmdGVtcCk7DQo+ID4gPiAg
ICAgICAgIGlmIChyZXQpDQo+ID4gPiAgICAgICAgICAgICAgICAgZ290byBvdXQ7DQo+ID4gPiBA
QCAtNjg0LDExICs2NzksNiBAQCBzdGF0aWMgaW50DQo+ID4gPiBpd2xfbXZtX3R6b25lX3NldF90
cmlwX3RlbXAoc3RydWN0IHRoZXJtYWxfem9uZV9kZXZpY2UgKmRldmljZSwNCj4gPiA+IA0KPiA+
ID4gICAgICAgICBtdXRleF9sb2NrKCZtdm0tPm11dGV4KTsNCj4gPiA+IA0KPiA+ID4gLSAgICAg
ICBpZiAoIW12bS0+dWNvZGVfbG9hZGVkIHx8ICEobXZtLT5jdXJfdWNvZGUgPT0NCj4gPiA+IElX
TF9VQ09ERV9SRUdVTEFSKSkgew0KPiA+ID4gLSAgICAgICAgICAgICAgIHJldCA9IC1FSU87DQo+
ID4gPiAtICAgICAgICAgICAgICAgZ290byBvdXQ7DQo+ID4gPiAtICAgICAgIH0NCj4gPiA+IC0N
Cj4gPiA+ICAgICAgICAgaWYgKHRyaXAgPCAwIHx8IHRyaXAgPj0gSVdMX01BWF9EVFNfVFJJUFMp
IHsNCj4gPiA+ICAgICAgICAgICAgICAgICByZXQgPSAtRUlOVkFMOw0KPiA+ID4gICAgICAgICAg
ICAgICAgIGdvdG8gb3V0Ow0KPiA+ID4gQEAgLTc1MCw2ICs3NDAsOSBAQCBzdGF0aWMgdm9pZA0K
PiA+ID4gaXdsX212bV90aGVybWFsX3pvbmVfcmVnaXN0ZXIoc3RydWN0IGl3bF9tdm0gKm12bSkN
Cj4gPiA+ICAgICAgICAgICAgICAgICByZXR1cm47DQo+ID4gPiAgICAgICAgIH0NCj4gPiA+IA0K
PiA+ID4gKyAgICAgICBpZiAoIW12bS0+dWNvZGVfbG9hZGVkIHx8ICEobXZtLT5jdXJfdWNvZGUg
PT0NCj4gPiA+IElXTF9VQ09ERV9SRUdVTEFSKSkNCj4gPiA+ICsgICAgICAgICAgICAgICByZXR1
cm47DQo+ID4gPiArDQo+ID4gPiAgICAgICAgIEJVSUxEX0JVR19PTihBUlJBWV9TSVpFKG5hbWUp
ID49IFRIRVJNQUxfTkFNRV9MRU5HVEgpOw0KPiA+ID4gDQo+ID4gPiAgICAgICAgIG12bS0+dHpf
ZGV2aWNlLnR6b25lID0gdGhlcm1hbF96b25lX2RldmljZV9yZWdpc3RlcihuYW1lLA0KPiA+ID4g
LS0NCj4gPiA+IDEuNy45LjMNCj4gPiA+IA0KPiA+ID4gLS0NCj4gPiA+IFRvIHVuc3Vic2NyaWJl
IGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eA0KPiA+ID4g
LXdpcmVsZXNzIiBpbg0KPiA+ID4gdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2
Z2VyLmtlcm5lbC5vcmcNCj4gPiA+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIA0KPiA+ID4gaHR0
cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5odG1s

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Grumbach, Emmanuel]

On Mon, 2016-07-11 at 14:19 -0400, Prarit Bhargava wrote:
> 
> On 07/11/2016 02:00 PM, Emmanuel Grumbach wrote:
> > On Mon, Jul 11, 2016 at 6:18 PM, Prarit Bhargava <prarit@redhat.com
> > > 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?
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?

> 
> 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? 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.

> 
> 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.

> 
> P.
> 
> [1] iwl_mvm_tzone_set_trip_temp() also returns -EIO, so setting and
> getting of
> the temperature is non-functional.
> 
> 
> > 
> > > 
> > > ---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 <prarit@redhat.com>
> > > Cc: Johannes Berg <johannes.berg@intel.com>
> > > Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> > > Cc: Luca Coelho <luciano.coelho@intel.com>
> > > Cc: Intel Linux Wireless <linuxwifi@intel.com>
> > > Cc: Kalle Valo <kvalo@codeaurora.org>
> > > Cc: Chaya Rachel Ivgi <chaya.rachel.ivgi@intel.com>
> > > Cc: Sara Sharon <sara.sharon@intel.com>
> > > 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(-)
> > > 
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> > > b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> > > index 58fc7b3c711c..64802659711f 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tt.c
> > > @@ -634,11 +634,6 @@ static int iwl_mvm_tzone_get_temp(struct
> > > thermal_zone_device *device,
> > > 
> > >         mutex_lock(&mvm->mutex);
> > > 
> > > -       if (!mvm->ucode_loaded || !(mvm->cur_ucode ==
> > > IWL_UCODE_REGULAR)) {
> > > -               ret = -EIO;
> > > -               goto out;
> > > -       }
> > > -
> > >         ret = iwl_mvm_get_temp(mvm, &temp);
> > >         if (ret)
> > >                 goto out;
> > > @@ -684,11 +679,6 @@ static int
> > > iwl_mvm_tzone_set_trip_temp(struct thermal_zone_device *device,
> > > 
> > >         mutex_lock(&mvm->mutex);
> > > 
> > > -       if (!mvm->ucode_loaded || !(mvm->cur_ucode ==
> > > IWL_UCODE_REGULAR)) {
> > > -               ret = -EIO;
> > > -               goto out;
> > > -       }
> > > -
> > >         if (trip < 0 || trip >= IWL_MAX_DTS_TRIPS) {
> > >                 ret = -EINVAL;
> > >                 goto out;
> > > @@ -750,6 +740,9 @@ static void
> > > iwl_mvm_thermal_zone_register(struct iwl_mvm *mvm)
> > >                 return;
> > >         }
> > > 
> > > +       if (!mvm->ucode_loaded || !(mvm->cur_ucode ==
> > > IWL_UCODE_REGULAR))
> > > +               return;
> > > +
> > >         BUILD_BUG_ON(ARRAY_SIZE(name) >= THERMAL_NAME_LENGTH);
> > > 
> > >         mvm->tz_device.tzone = thermal_zone_device_register(name,
> > > --
> > > 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

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Prarit Bhargava]

On 07/11/2016 02:27 PM, Grumbach, Emmanuel wrote:
> On Mon, 2016-07-11 at 14:19 -0400, Prarit Bhargava wrote:
>>
>> On 07/11/2016 02:00 PM, Emmanuel Grumbach wrote:
>>> On Mon, Jul 11, 2016 at 6:18 PM, Prarit Bhargava <prarit@redhat.com
>>>> 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.

> 
>>
>> 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.

> 
>>
>> 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.

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Kalle Valo]

Prarit Bhargava <prarit@redhat.com> writes:

>> 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.

But I suspect that the iwlwifi firmware is loaded during interface up
(and unloaded during interface down) and in that case
register/unregister would be happening all the time. That doesn't sound
like a good idea. I would rather try to fix the thermal interface to
handle the cases when the measurement is not available.

-- 
Kalle Valo

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Kalle Valo]

Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:

>> 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.

But I suspect that the iwlwifi firmware is loaded during interface up
(and unloaded during interface down) and in that case
register/unregister would be happening all the time. That doesn't sound
like a good idea. I would rather try to fix the thermal interface to
handle the cases when the measurement is not available.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Luca Coelho]

On Wed, 2016-07-13 at 09:50 +0300, Kalle Valo wrote:
> Prarit Bhargava <prarit@redhat.com> writes:
> 
> > > 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.
> 
> But I suspect that the iwlwifi firmware is loaded during interface up
> (and unloaded during interface down) and in that case
> register/unregister would be happening all the time. That doesn't
> sound
> like a good idea. I would rather try to fix the thermal interface to
> handle the cases when the measurement is not available.

I totally agree with Emmanuel and Kalle.  We should not change this.
 It is a design decision to return an error when the interface is down,
this is very common with other subsystems as well.  The userspace
should be able to handle errors and report something like "unavailable"
when this kind of error is returned.

I'm not sure EIO is the best we can have, but for me that's exactly
what it is.  The thermal zone *is* there, but cannot be accessed
because the firmware is not available.  I'm okay to change it to EBUSY,
if that would help userspace, but I think that's a bit misleading.  The
device is not busy, on the contrary, it's not even running at all.

Furthermore, I don't think this is "breaking userspace" in the sense of
being a regression.  The userspace API has always been implemented with
the possibility of returning errors.  It's not a good design if a
single device returning an error causes all the other devices to also
fail.

--
Cheers,
Luca.

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Luca Coelho]

On Wed, 2016-07-13 at 09:50 +0300, Kalle Valo wrote:
> Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> 
> > > 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.
> 
> But I suspect that the iwlwifi firmware is loaded during interface up
> (and unloaded during interface down) and in that case
> register/unregister would be happening all the time. That doesn't
> sound
> like a good idea. I would rather try to fix the thermal interface to
> handle the cases when the measurement is not available.

I totally agree with Emmanuel and Kalle.  We should not change this.
 It is a design decision to return an error when the interface is down,
this is very common with other subsystems as well.  The userspace
should be able to handle errors and report something like "unavailable"
when this kind of error is returned.

I'm not sure EIO is the best we can have, but for me that's exactly
what it is.  The thermal zone *is* there, but cannot be accessed
because the firmware is not available.  I'm okay to change it to EBUSY,
if that would help userspace, but I think that's a bit misleading.  The
device is not busy, on the contrary, it's not even running at all.

Furthermore, I don't think this is "breaking userspace" in the sense of
being a regression.  The userspace API has always been implemented with
the possibility of returning errors.  It's not a good design if a
single device returning an error causes all the other devices to also
fail.

--
Cheers,
Luca.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Prarit Bhargava]

On 07/13/2016 02:50 AM, Kalle Valo wrote:
> Prarit Bhargava <prarit@redhat.com> writes:
> 
>>> 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.
> 
> But I suspect that the iwlwifi firmware is loaded during interface up
> (and unloaded during interface down) and in that case
> register/unregister would be happening all the time. 

You make it sound like the interface is coming and going a 1000 times a second.
 Maybe this happens once during runtime & during suspend/resume cycles?  What
about the cases when the firmware isn't present (and that's what lead me to this
bug)?

That doesn't sound
> like a good idea. I would rather try to fix the thermal interface to
> handle the cases when the measurement is not available.
> 

Userspace is broken because of this change.  I've had to make another horrible
change to cpufreq for a similar change so I don't see the argument here to just
blame userspace and ignore the outcome of the patch.

P.

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Prarit Bhargava]

On 07/13/2016 03:24 AM, Luca Coelho wrote:
> On Wed, 2016-07-13 at 09:50 +0300, Kalle Valo wrote:
>> Prarit Bhargava <prarit@redhat.com> writes:
>>
>>>> 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.
>>
>> But I suspect that the iwlwifi firmware is loaded during interface up
>> (and unloaded during interface down) and in that case
>> register/unregister would be happening all the time. That doesn't
>> sound
>> like a good idea. I would rather try to fix the thermal interface to
>> handle the cases when the measurement is not available.
> 
> I totally agree with Emmanuel and Kalle.  We should not change this.
>  It is a design decision to return an error when the interface is down,
> this is very common with other subsystems as well.  

Please show me another subsystem or driver that does this.  I've looked around
the kernel but cannot find one that updates the firmware and implements new
features on the fly like this.  I have come across several drivers that allow
for an update, but they do not implement new features based on the firmware.

Additionally, what happens when someone back revs firmware versions (which
happens far more than you and I would expect)?  Does that mean I now go from a
functional system to a non-functional system wrt to userspace?

The userspace
> should be able to handle errors and report something like "unavailable"
> when this kind of error is returned.
> 

I myself have made the same arguments wrt to cpufreq code & bad userspace
choices.  I just went through this a few months back with what went from a
simple patch and turned out to be a hideous patch in cpufreq.  You cannot break
userspace like this.

See commit 51443fbf3d2c ("cpufreq: intel_pstate: Fix intel_pstate powersave
min_perf_pct value").  What should have been a trivial change resulted in a
massive change because of broken userspace.

> I'm not sure EIO is the best we can have, but for me that's exactly
> what it is.  The thermal zone *is* there, but cannot be accessed
> because the firmware is not available.  I'm okay to change it to EBUSY,
> if that would help userspace, but I think that's a bit misleading.  The
> device is not busy, on the contrary, it's not even running at all.
> 

I understand that, but by returning -EIO we end up with an error.

> Furthermore, I don't think this is "breaking userspace" in the sense of
> being a regression.  

I run (let's say 4.5 kernel).  sensors works.  I update to 4.7.  sensors doesn't
work.  How is that not a regression?  That's _exactly_ what it should be
reported as.

The userspace API has always been implemented with
> the possibility of returning errors.  It's not a good design if a
> single device returning an error causes all the other devices to also
> fail.
> 

If that were the case we would never have to worry about "breaking userspace"?
For any kernel change I could just say that the userspace design was bad and be
done with it.  Why fix anything then?

I don't see any harm in waiting to register the sysfs files for hwmon until the
firmware has been validated.  IIUC, the up/down'ing of the device doesn't happen
that often (during initial boot, and suspend/resume, switching wifi connections,
shutdown?).  This would make the iwlwifi community happy (IMO) and sensors would
still work.  At the same time I could write a patch for lm-sensors to fix this
issue if it comes up in future versions.  [Aside: I'm going to have the
reproducing system available today and will test this out.  It looks like just
moving some code around.]

The bottom line is that lm-sensors is currently broken with this change in
iwlwifi.  AFAICT, no other thermal device returns an error this way, and IMO
that means the iwlwifi driver is doing something new and unexpected wrt to
userspace.

P.


> --
> Cheers,
> Luca.
> 

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Kalle Valo]

Prarit Bhargava <prarit@redhat.com> writes:

> On 07/13/2016 02:50 AM, Kalle Valo wrote:
>> Prarit Bhargava <prarit@redhat.com> writes:
>> 
>>>> 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.
>> 
>> But I suspect that the iwlwifi firmware is loaded during interface up
>> (and unloaded during interface down) and in that case
>> register/unregister would be happening all the time. 
>
> You make it sound like the interface is coming and going a 1000 times a second.
>  Maybe this happens once during runtime & during suspend/resume
>  cycles?

Of course it doesn't happen 1000 times a second but it depends on user
space behaviour. In some cases, when the wlan interface is always up,
the firmware is loaded only once. But in some cases the user space might
change the interface state more frequently.

More so registering services like thermal zone should happen during
driver probe time, not during interface up event.

> What about the cases when the firmware isn't present (and that's what
> lead me to this bug)?

In that case the kernel could return a predefined error value like
-EGAIN or -ENOTDOWN so that the user space knows that a value is not
available at this time (but might be available later).

-- 
Kalle Valo

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Kalle Valo]

Prarit Bhargava <prarit@redhat.com> writes:

> On 07/13/2016 03:24 AM, Luca Coelho wrote:
>
>> I totally agree with Emmanuel and Kalle. We should not change this.
>> It is a design decision to return an error when the interface is
>> down, this is very common with other subsystems as well.
>
> Please show me another subsystem or driver that does this.  I've looked around
> the kernel but cannot find one that updates the firmware and implements new
> features on the fly like this.  I have come across several drivers that allow
> for an update, but they do not implement new features based on the
> firmware.
>
> Additionally, what happens when someone back revs firmware versions (which
> happens far more than you and I would expect)?  Does that mean I now go from a
> functional system to a non-functional system wrt to userspace?

I'm not following, what do you mean exactly? Why are you talking
updating the firmware?

So when we talk about "loading firmware" we mean that the driver pushes
the firmware image to to the chipset. And then the interface is down the
chipset is powered down and the RAM on it will be erased. That's the
general idea anyway, I haven't checked how iwlwifi exactly works in this
case but Luca or Emmanuel can correct me.

>> The userspace should be able to handle errors and report something
>> like "unavailable" when this kind of error is returned.
>
> I myself have made the same arguments wrt to cpufreq code & bad userspace
> choices.  I just went through this a few months back with what went from a
> simple patch and turned out to be a hideous patch in cpufreq.  You cannot break
> userspace like this.

Don't get me wrong, I'm a strong supporter of stable user space
interfaces and I always try to adher to that. But there's a limit for
everything. If I'm understanding correctly, what you mean is that the
kernel should never return an error because an application doesn't
handle errors gracefully. Sorry, but that doesn't make sense to me.

> See commit 51443fbf3d2c ("cpufreq: intel_pstate: Fix intel_pstate powersave
> min_perf_pct value").  What should have been a trivial change resulted in a
> massive change because of broken userspace.

In that cpufreq case I understand, it was about a combination of
configuration values which broke the user space. But here we are just
dealing with a simple error value, nothing fancy.

>> I'm not sure EIO is the best we can have, but for me that's exactly
>> what it is.  The thermal zone *is* there, but cannot be accessed
>> because the firmware is not available.  I'm okay to change it to EBUSY,
>> if that would help userspace, but I think that's a bit misleading.  The
>> device is not busy, on the contrary, it's not even running at all.
>> 
>
> I understand that, but by returning -EIO we end up with an error.
>
>> Furthermore, I don't think this is "breaking userspace" in the sense of
>> being a regression.  
>
> I run (let's say 4.5 kernel).  sensors works.  I update to 4.7.  sensors doesn't
> work.  How is that not a regression?  That's _exactly_ what it should be
> reported as.

Sure, it's a regression in a way. But that's how the user space app you
are using is implemented, the same problem would happen with any driver
returning errors.

>> The userspace API has always been implemented with
>> the possibility of returning errors.  It's not a good design if a
>> single device returning an error causes all the other devices to also
>> fail.
>> 
>
> If that were the case we would never have to worry about "breaking userspace"?
> For any kernel change I could just say that the userspace design was bad and be
> done with it.  Why fix anything then?

Because we are talking about a simple error value.

> I don't see any harm in waiting to register the sysfs files for hwmon until the
> firmware has been validated.

I'm against of that because it's bad software design. It's standard
practise in Linux that drivers register their capabilities during driver
probe time so that user space can query them whenever needed. I assume a
properly behaving user space app would want to know about all the
available sensors once the driver is initialised and your suggestion
would break that.

> IIUC, the up/down'ing of the device doesn't happen that often (during
> initial boot, and suspend/resume, switching wifi connections,
> shutdown?).

Basically it can happen anytime, this is fully controlled by user space.
There's no point of trying to make any assumptions as they won't hold
anyway.

> This would make the iwlwifi community happy (IMO) and
> sensors would still work. At the same time I could write a patch for
> lm-sensors to fix this issue if it comes up in future versions.
> [Aside: I'm going to have the reproducing system available today and
> will test this out. It looks like just moving some code around.]

Another option, but still a bad one I don't like, is that you change the
kernel interface to ignore all errors from drivers (like iwlwifi). This
way drivers don't need to make ugly workarounds.

> The bottom line is that lm-sensors is currently broken with this change in
> iwlwifi.  AFAICT, no other thermal device returns an error this way, and IMO
> that means the iwlwifi driver is doing something new and unexpected wrt to
> userspace.

I haven't checked but I suspect ath10k has a similar problem when
interface is down.

-- 
Kalle Valo

RE: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Grumbach, Emmanuel]

> 
> Prarit Bhargava <prarit@redhat.com> writes:
> 
> > On 07/13/2016 03:24 AM, Luca Coelho wrote:
> >
> >> I totally agree with Emmanuel and Kalle. We should not change this.
> >> It is a design decision to return an error when the interface is
> >> down, this is very common with other subsystems as well.
> >
> > Please show me another subsystem or driver that does this.  I've
> > looked around the kernel but cannot find one that updates the firmware
> > and implements new features on the fly like this.  I have come across
> > several drivers that allow for an update, but they do not implement
> > new features based on the firmware.
> >
> > Additionally, what happens when someone back revs firmware versions
> > (which happens far more than you and I would expect)?  Does that mean
> > I now go from a functional system to a non-functional system wrt to
> userspace?
> 
> I'm not following, what do you mean exactly? Why are you talking updating
> the firmware?
> 
> So when we talk about "loading firmware" we mean that the driver pushes
> the firmware image to to the chipset. And then the interface is down the
> chipset is powered down and the RAM on it will be erased. That's the general
> idea anyway, I haven't checked how iwlwifi exactly works in this case but
> Luca or Emmanuel can correct me.

This is correct.


> 
> >> The userspace should be able to handle errors and report something
> >> like "unavailable" when this kind of error is returned.
> >
> > I myself have made the same arguments wrt to cpufreq code & bad
> > userspace choices.  I just went through this a few months back with
> > what went from a simple patch and turned out to be a hideous patch in
> > cpufreq.  You cannot break userspace like this.
> 
> Don't get me wrong, I'm a strong supporter of stable user space interfaces
> and I always try to adher to that. But there's a limit for everything. If I'm
> understanding correctly, what you mean is that the kernel should never
> return an error because an application doesn't handle errors gracefully.
> Sorry, but that doesn't make sense to me.
> 
> > See commit 51443fbf3d2c ("cpufreq: intel_pstate: Fix intel_pstate
> > powersave min_perf_pct value").  What should have been a trivial
> > change resulted in a massive change because of broken userspace.
> 
> In that cpufreq case I understand, it was about a combination of
> configuration values which broke the user space. But here we are just
> dealing with a simple error value, nothing fancy.
> 
> >> I'm not sure EIO is the best we can have, but for me that's exactly
> >> what it is.  The thermal zone *is* there, but cannot be accessed
> >> because the firmware is not available.  I'm okay to change it to
> >> EBUSY, if that would help userspace, but I think that's a bit
> >> misleading.  The device is not busy, on the contrary, it's not even running
> at all.
> >>
> >
> > I understand that, but by returning -EIO we end up with an error.
> >
> >> Furthermore, I don't think this is "breaking userspace" in the sense
> >> of being a regression.
> >
> > I run (let's say 4.5 kernel).  sensors works.  I update to 4.7.
> > sensors doesn't work.  How is that not a regression?  That's _exactly_
> > what it should be reported as.
> 
> Sure, it's a regression in a way. But that's how the user space app you are
> using is implemented, the same problem would happen with any driver
> returning errors.
> 
> >> The userspace API has always been implemented with the possibility of
> >> returning errors.  It's not a good design if a single device
> >> returning an error causes all the other devices to also fail.
> >>
> >
> > If that were the case we would never have to worry about "breaking
> userspace"?
> > For any kernel change I could just say that the userspace design was
> > bad and be done with it.  Why fix anything then?
> 
> Because we are talking about a simple error value.
> 
> > I don't see any harm in waiting to register the sysfs files for hwmon
> > until the firmware has been validated.
> 
> I'm against of that because it's bad software design. It's standard practise in
> Linux that drivers register their capabilities during driver probe time so that
> user space can query them whenever needed. I assume a properly behaving
> user space app would want to know about all the available sensors once the
> driver is initialised and your suggestion would break that.
> 
> > IIUC, the up/down'ing of the device doesn't happen that often (during
> > initial boot, and suspend/resume, switching wifi connections,
> > shutdown?).
> 
> Basically it can happen anytime, this is fully controlled by user space.
> There's no point of trying to make any assumptions as they won't hold
> anyway.
> 
> > This would make the iwlwifi community happy (IMO) and sensors would
> > still work. At the same time I could write a patch for lm-sensors to
> > fix this issue if it comes up in future versions.
> > [Aside: I'm going to have the reproducing system available today and
> > will test this out. It looks like just moving some code around.]
> 
> Another option, but still a bad one I don't like, is that you change the kernel
> interface to ignore all errors from drivers (like iwlwifi). This way drivers don't
> need to make ugly workarounds.
> 
> > The bottom line is that lm-sensors is currently broken with this
> > change in iwlwifi.  AFAICT, no other thermal device returns an error
> > this way, and IMO that means the iwlwifi driver is doing something new
> > and unexpected wrt to userspace.
> 
> I haven't checked but I suspect ath10k has a similar problem when interface
> is down.
> 
> --
> Kalle Valo

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Stanislaw Gruszka]

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>
> > 
> > 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

RE: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Grumbach, Emmanuel]

> 
> 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>
> > >
> > > 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 :)

> 
> Stanislaw

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Stanislaw Gruszka]

On Thu, Jul 14, 2016 at 09:44:22AM +0000, Grumbach, Emmanuel wrote:
> > 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

Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Prarit Bhargava]

On 07/15/2016 07:25 AM, Stanislaw Gruszka wrote:
> On Thu, Jul 14, 2016 at 09:44:22AM +0000, Grumbach, Emmanuel wrote:
>>> 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.
> 
> Stanislaw
> 

RE: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded

[Grumbach, Emmanuel]

> On 07/15/2016 07:25 AM, Stanislaw Gruszka wrote:
> > On Thu, Jul 14, 2016 at 09:44:22AM +0000, Grumbach, Emmanuel wrote:
> >>> 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.
> 

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.