Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1406197 > unrolled thread
| Started by | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| First post | 2016-05-24 16:10 +0200 |
| Last post | 2016-05-25 18:00 +0200 |
| Articles | 2 on this page of 22 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-24 16:10 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Rhyland Klein <rklein@nvidia.com> - 2016-05-24 21:10 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-25 13:00 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-25 13:10 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Thierry Reding <treding@nvidia.com> - 2016-05-25 17:50 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-25 18:00 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Rhyland Klein <rklein@nvidia.com> - 2016-05-25 18:00 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-25 18:20 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-25 18:30 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-25 18:40 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Rhyland Klein <rklein@nvidia.com> - 2016-05-25 18:40 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-25 19:30 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Rhyland Klein <rklein@nvidia.com> - 2016-05-25 21:50 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-26 12:40 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-27 10:40 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-27 11:30 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-27 12:30 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-27 13:50 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-27 14:20 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-05-27 15:00 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-31 19:30 +0200
Re: [PATCH] arm64: defconfig: Enable cros-ec and battery driver Rhyland Klein <rklein@nvidia.com> - 2016-05-25 18:00 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| Date | 2016-05-31 19:30 +0200 |
| Message-ID | <rEVEn-5CE-51@gated-at.bofh.it> |
| In reply to | #1408063 |
On 27/05/16 13:55, Krzysztof Kozlowski wrote: > On 05/27/2016 02:17 PM, Jon Hunter wrote: >> >> On 27/05/16 12:46, Krzysztof Kozlowski wrote: >>> On 05/27/2016 12:28 PM, Jon Hunter wrote: >>>> Hi Krzysztof, >>>> >>>> On 27/05/16 09:37, Krzysztof Kozlowski wrote: >>>> >>>> ... >>>> >>>>> Indeed I was struggling with similar issue in bq27x00_battery. The issue >>>>> was introduced by... me :( when moving the ownership of power supply >>>>> structure from driver to the core. However IMHO my change exposed the >>>>> fundamental problem with power supply. >>>>> >>>>> Anyway a fix for this issue was: >>>>> 7f1a57fdd6cb6e7b (power_supply: Fix possible NULL pointer dereference on >>>>> early uevent) >>>>> AFAIU, this fix no longer fixes all the issues, right? >>>>> >>>>> As for the fundamental problem, the power supply core should not call >>>>> back the driver (get_property()) until the probe ends. Even if the >>>>> di->bat was initialized, some other fields of driver could not be set >>>>> yet. In general, the probe did not end so we should avoid calling driver >>>>> internal functions. >>>> >>>> For my understanding, can you elaborate why the power-supply core should >>>> not call back to the drivers ->get_property() before the probe ends? I >>>> assume that registering the power-supply should be the last thing done >>>> in the probe and so the power-supply should be configured at that point. >>> >>> It is not only about power supply but other resources allocated by the >>> driver. If the power_supply_register() is a last call, then no problem. >>> But if not, then these resources won't be available. >>> >>> Actually I exaggerated a little bit as a fundamental problem as this is >>> quite common pattern. When driver provides something (like power supply) >>> then after registration it should be ready for calls coming from the >>> core or user space. It does not have to be power supply. It might be >>> exposing sysfs entries or file operations (exposed before calling >>> power_supply_register()). >> >> Right, exactly when you register with the power-supply core the device >> better be ready so that handle any incoming calls. > > Yes, the unusual thing here is that the device is called back directly > from the power_supply_register() call. > >> >>>> The problems with the bq27xxx seem to stem from the periodic update of >>>> the bq27xxx status and so it is not clear to me that this is a generic >>>> problem for all power-supply devices. >>> >>> Initially, the generic problem was that the core would call back the >>> driver from power_supply_register() in a synchronous way through >>> power_supply_changed(). The commit 7f1a57fdd6c changed it to an >>> asynchronous call. Here it looks like the same problem - the >>> power_supply_register() calls thermal which calls >>> thermal_zone_device_update() and we are back at the driver... before >>> finishing power_supply_register() call. >> >> So I am still not convinced this is a generic problem but a problem with >> the bq27xxx. In fact, I think that commit 7f1a57fdd6c could be avoided >> if we did something like ... >> >> http://marc.info/?l=linux-kernel&m=146425896332433&w=2 >> >> AFAICT in most cases, in ->get_property() you should have no need to >> access a driver's equivalent of di->bat, because you have already been >> passed a pointer to this via the *psy argument. > > I agree that get_property() shouldn't access di->bat. However if it is > not forbidden (at least by documentation) then someone might just do it > because he does not know about such requirement. In that case, shouldn't the driver should check that di->bat is valid before anyone attempts to dereference it? However, if you and/or Rhyland have a generic fix for preventing this, please go ahead and propose it. Cheers Jon -- nvpublic
[toc] | [prev] | [next] | [standalone]
| From | Rhyland Klein <rklein@nvidia.com> |
|---|---|
| Date | 2016-05-25 18:00 +0200 |
| Message-ID | <rCJnY-2Kq-7@gated-at.bofh.it> |
| In reply to | #1406841 |
On 5/25/2016 7:03 AM, Jon Hunter wrote:
>
> On 25/05/16 11:58, Jon Hunter wrote:
>
> ...
I am aware of the splat, and I was considering the proper place for
working around that.
>
>> Looking at this a bit more I am wondering if we should prevent the
>> battery for being polled before the registration has completed ...
>>
>> diff --git a/drivers/power/bq27xxx_battery.c
>> b/drivers/power/bq27xxx_battery.c
>> index 45f6ebf88df6..32649183ecd9 100644
>> --- a/drivers/power/bq27xxx_battery.c
>> +++ b/drivers/power/bq27xxx_battery.c
>> @@ -871,12 +871,14 @@ static int bq27xxx_battery_get_property(struct
>> power_supply *psy,
>> int ret = 0;
>> struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
>>
>> - mutex_lock(&di->lock);
>> - if (time_is_before_jiffies(di->last_update + 5 * HZ)) {
>> - cancel_delayed_work_sync(&di->work);
>> - bq27xxx_battery_poll(&di->work.work);
>> + if (di->bat) {
>> + mutex_lock(&di->lock);
>> + if (time_is_before_jiffies(di->last_update + 5 * HZ)) {
>> + cancel_delayed_work_sync(&di->work);
>> + bq27xxx_battery_poll(&di->work.work);
>> + }
>> + mutex_unlock(&di->lock);
>> }
>> - mutex_unlock(&di->lock);
>
> Alternatively, maybe the following is simpler ...
>
> diff --git a/drivers/power/bq27xxx_battery.c
> b/drivers/power/bq27xxx_battery.c
> index 45f6ebf88df6..8a713b52e9f6 100644
> --- a/drivers/power/bq27xxx_battery.c
> +++ b/drivers/power/bq27xxx_battery.c
> @@ -733,7 +733,8 @@ static void bq27xxx_battery_poll(struct work_struct
> *work)
> container_of(work, struct bq27xxx_device_info,
> work.work);
>
> - bq27xxx_battery_update(di);
> + if (di->bat)
> + bq27xxx_battery_update(di);
>
While that might get around the problem, I don't think the fix should be
inside the bq27xxx driver. The problem is that the core is calling :
__power_supply_register->
psy_register_thermal()->
thermal_zone_device_register()->
thermal_zone_device_update()->
thermal_zone_get_temp()->
power_supply_read_temp()
then power_supply_read_temp() will attempt to use the driver's callback
get_property method passing it uncompletely initialized struct.
If you notice, there are already other places inside power_supply_core.c
where use_cnt is used to block calls that would reach back to the
get_property callbacks. I don't think it would be bad to have sanity
checks in those callbacks for NULL pointers, but the main problem is
that in this path, the core should know not to call a get_property
callback during registration (before use_cnt is incremented).
This is closely related to this patch in the power_supply_core.c
commit 7f1a57fdd6cb6e7be2ed31878a34655df38e1861
Author: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Date: Tue May 19 16:13:02 2015 +0900
power_supply: Fix possible NULL pointer dereference on early uevent
Don't call the power_supply_changed() from power_supply_register() when
parent is still probing because it may lead to accessing parent too
early.
...
Its just another situation where get_property is called prematurely.
-rhyland
--
nvpublic
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web