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

[Prarit Bhargava <prarit@redhat.com>]

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.