Re: reverted battery current conversion fix

Re: reverted battery current conversion fix

[Alexey Starikovskiy]

Ferenc Wagner wrote:
> Hi Alex,
> 
> I sadly noticed that your fix 558073dd was overridden by aaad0776 and
> later reverted altogether by f10a3a32.  I haven't found any discussion
> of the latter two patches, but I agree that Rafael's fix was wrong, as
> it converted the current into A instead of uA, since voltage_now is in
> mV and current_now is in mW on the new code path, and mW/mV=A.  I
> skimmed the code, but didn't find a place where the kernel provided
> time estimates, so the wild values Rafael experienced probably came
> from some application, which previously worked around the wrong sysfs
> current value (supposedly correctly interpreting it as power) and thus
> got broken by the fix.  But all of this is guesswork, I'm hunting for
> hard data.  Until I get around compiling and testing a new kernel with
> /proc/acpi, can you provide some insight?  I'm not subscribed to the
> relevant MLs, so maybe just didn't find all the pieces in the archives.
Yes, the application is kpowersaved and it is written with the assumption
that it could get remaining time by dividing either energy_now or charge_now 
by current_now.

Regards,
Alex.

Re: reverted battery current conversion fix

[Ferenc Wagner]

Alexey Starikovskiy <astarikovskiy@suse.de> writes:

> Ferenc Wagner wrote:
>
>> the wild values probably came from some application, which
>> previously worked around the wrong sysfs current value (supposedly
>> correctly interpreting it as power) and thus got broken by the fix.
>
> Yes, the application is kpowersaved and it is written with the
> assumption that it could get remaining time by dividing either
> energy_now or charge_now by current_now.

So the first choice depends on the current buggy kernel behaviour.
What's the plan of action in such cases?  I found some notes that
kpowersaved can or could use HAL for getting this information, in
which case HAL also should be fixed.  At least their bug tracker[1] on
SF doesn't contain any such issue, maybe one should be added by
somebody actually using it if the kernel is to be fixed.

[1]  http://sourceforge.net/tracker/?atid=700009&group_id=124576&func=browse
-- 
Cheers,
Feri.

Re: reverted battery current conversion fix

[Alexey Starikovskiy]

Ferenc Wagner wrote:
> Alexey Starikovskiy <astarikovskiy@suse.de> writes:
> 
>> Ferenc Wagner wrote:
>>
>>> the wild values probably came from some application, which
>>> previously worked around the wrong sysfs current value (supposedly
>>> correctly interpreting it as power) and thus got broken by the fix.
>> Yes, the application is kpowersaved and it is written with the
>> assumption that it could get remaining time by dividing either
>> energy_now or charge_now by current_now.
> 
> So the first choice depends on the current buggy kernel behaviour.
> What's the plan of action in such cases?  I found some notes that
> kpowersaved can or could use HAL for getting this information, in
> which case HAL also should be fixed.  At least their bug tracker[1] on
> SF doesn't contain any such issue, maybe one should be added by
> somebody actually using it if the kernel is to be fixed.
Currently, Rafael suggests, that it is too late to fix the kernel...

Regards,
Alex.
> 
> [1]  http://sourceforge.net/tracker/?atid=700009&group_id=124576&func=browse

Re: reverted battery current conversion fix

[Ferenc Wagner]

Alexey Starikovskiy <astarikovskiy@suse.de> writes:

> Ferenc Wagner wrote:
>
>> Alexey Starikovskiy <astarikovskiy@suse.de> writes:
>>
>>> Ferenc Wagner wrote:
>>>
>>>> the wild values probably came from some application, which
>>>> previously worked around the wrong sysfs current value (supposedly
>>>> correctly interpreting it as power) and thus got broken by the fix.
>>>
>>> Yes, the application is kpowersaved and it is written with the
>>> assumption that it could get remaining time by dividing either
>>> energy_now or charge_now by current_now.
>>
>> So the first choice depends on the current buggy kernel behaviour.
>> What's the plan of action in such cases?  I found some notes that
>> kpowersaved can or could use HAL for getting this information, in
>> which case HAL also should be fixed.  At least their bug tracker[1] on
>> SF doesn't contain any such issue, maybe one should be added by
>> somebody actually using it if the kernel is to be fixed.
>
> Currently, Rafael suggests, that it is too late to fix the kernel...

Too late for 2.6.28 or too late for ever?  The ACPI sysfs interface
appeared about one year ago, if I read the git log right,
documentation followed this spring.  If the supposedly clean sysfs
interface can't live up to its very precise and well thought out
documentation, that should be documented at least.  :(  What a pity.
-- 
Regards,
Feri.

Re: reverted battery current conversion fix

[Henrique de Moraes Holschuh]

On Tue, 16 Dec 2008, Ferenc Wagner wrote:
> Alexey Starikovskiy <astarikovskiy@suse.de> writes:
> > Ferenc Wagner wrote:
> >> Alexey Starikovskiy <astarikovskiy@suse.de> writes:
> >>> Ferenc Wagner wrote:
> >>>> the wild values probably came from some application, which
> >>>> previously worked around the wrong sysfs current value (supposedly
> >>>> correctly interpreting it as power) and thus got broken by the fix.
> >>>
> >>> Yes, the application is kpowersaved and it is written with the
> >>> assumption that it could get remaining time by dividing either
> >>> energy_now or charge_now by current_now.
> >>
> >> So the first choice depends on the current buggy kernel behaviour.
> >> What's the plan of action in such cases?  I found some notes that
> >> kpowersaved can or could use HAL for getting this information, in
> >> which case HAL also should be fixed.  At least their bug tracker[1] on
> >> SF doesn't contain any such issue, maybe one should be added by
> >> somebody actually using it if the kernel is to be fixed.
> >
> > Currently, Rafael suggests, that it is too late to fix the kernel...
> 
> Too late for 2.6.28 or too late for ever?  The ACPI sysfs interface
> appeared about one year ago, if I read the git log right,
> documentation followed this spring.  If the supposedly clean sysfs
> interface can't live up to its very precise and well thought out
> documentation, that should be documented at least.  :(  What a pity.

I sure hope it is "too late for 2.6.28" :-)

I can tell you what *I* do on thinkpad-acpi: I fix it, but I warn the users
beforehand, and in the release notes, and in the commit message.  And I have
documented that fact in the rules of engagement for the thinkpad-acpi sysfs
interface, to make it extremely clear to all parties involved.

Note that "broken" != "not as neat as we'd like".  In this case, it *is*
clearly broken, so what is happening is not a gratuitous ABI change.  And if
someone in userspace worked around the broken crap, IT IS THEIR FAULT for
doing it in the first place instead of demanding that we fix the mess when
they noticed it existed.

PS: a little foresight can help wonders.  I suggest adding a version
read-only attribute to the sysfs interface, and increase it when you do any
ABI change that userspace could notice (be them fixes or something else).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: reverted battery current conversion fix

[Rafael J. Wysocki]

On Tuesday, 16 of December 2008, Henrique de Moraes Holschuh wrote:
> On Tue, 16 Dec 2008, Ferenc Wagner wrote:
> > Alexey Starikovskiy <astarikovskiy@suse.de> writes:
> > > Ferenc Wagner wrote:
> > >> Alexey Starikovskiy <astarikovskiy@suse.de> writes:
> > >>> Ferenc Wagner wrote:
> > >>>> the wild values probably came from some application, which
> > >>>> previously worked around the wrong sysfs current value (supposedly
> > >>>> correctly interpreting it as power) and thus got broken by the fix.
> > >>>
> > >>> Yes, the application is kpowersaved and it is written with the
> > >>> assumption that it could get remaining time by dividing either
> > >>> energy_now or charge_now by current_now.
> > >>
> > >> So the first choice depends on the current buggy kernel behaviour.
> > >> What's the plan of action in such cases?  I found some notes that
> > >> kpowersaved can or could use HAL for getting this information, in
> > >> which case HAL also should be fixed.  At least their bug tracker[1] on
> > >> SF doesn't contain any such issue, maybe one should be added by
> > >> somebody actually using it if the kernel is to be fixed.
> > >
> > > Currently, Rafael suggests, that it is too late to fix the kernel...

Actually no, I didn't say that.

What I said is that it was too late to change the interpretation of
'current_now' in the case when energy units were used.

> > Too late for 2.6.28 or too late for ever?  The ACPI sysfs interface
> > appeared about one year ago, if I read the git log right,
> > documentation followed this spring.  If the supposedly clean sysfs
> > interface can't live up to its very precise and well thought out
> > documentation, that should be documented at least.  :(  What a pity.
> 
> I sure hope it is "too late for 2.6.28" :-)

For 2.6.28 it's obviously too late, but I think it generally is too late to
change whatever is reported via 'current_now', because the userland already
started to rely on that.

> I can tell you what *I* do on thinkpad-acpi: I fix it, but I warn the users
> beforehand, and in the release notes, and in the commit message.  And I have
> documented that fact in the rules of engagement for the thinkpad-acpi sysfs
> interface, to make it extremely clear to all parties involved.
> 
> Note that "broken" != "not as neat as we'd like".  In this case, it *is*
> clearly broken, so what is happening is not a gratuitous ABI change.

That only is a semantic difference.  The breakage is actually the same.

> And if someone in userspace worked around the broken crap, IT IS THEIR FAULT
> for doing it in the first place instead of demanding that we fix the mess when
> they noticed it existed.

I think they didn't understand the interface and found the working settings by
experimentation.  It's not their fault that had to do that.

> PS: a little foresight can help wonders.  I suggest adding a version
> read-only attribute to the sysfs interface, and increase it when you do any
> ABI change that userspace could notice (be them fixes or something else).

Good idea, but it doesn't help in this particular case.

Thanks,
Rafael