Should 9aafabc7fece be reverted?

Should 9aafabc7fece be reverted?

[Andrew F. Davis]

Hello all,

Why do we need this patch? I removed giving each battery a number in its
name intentionally. It doesn't seem to be needed anymore, and only two
old drivers out of the ~70 power supply drivers do this.

Also I don't see this in my mailbox archive, was I CC'd on this patch?

Thanks,
Andrew

Re: Should 9aafabc7fece be reverted?

[Ivaylo Dimitrov]

Hi,

On 18.05.2016 19:58, Andrew F. Davis wrote:
> Hello all,
>
> Why do we need this patch? I removed giving each battery a number in its
> name intentionally. It doesn't seem to be needed anymore, and only two
> old drivers out of the ~70 power supply drivers do this.
>

The patch is needed because commit <703df6c09795> breaks existing 
userspace on Nokia N900, which counts on name numbering. Also, commit 
<703df6c09795> description says nothing about name changing, so I 
assumed it is an unintentional side-effect of splitting the code into 
modules.

> Also I don't see this in my mailbox archive, was I CC'd on this patch?
>

You were not, get_maintainer.pl on v4.4-rc7 (the tree I was working on 
by the time I've sent <9aafabc7fece>, iirc) gives:

Sebastian Reichel <xxx@xxx.org> (maintainer:POWER SUPPLY CLASS/SUBSYSTEM 
and DRIVERS)
Dmitry Eremin-Solenikov <xxx@xxx.org> (maintainer:POWER SUPPLY 
CLASS/SUBSYSTEM and DRIVERS)
David Woodhouse <xxx@xxx.org> (maintainer:POWER SUPPLY CLASS/SUBSYSTEM 
and DRIVERS)
"Pali Rohár" <xxx@xxx.org> (maintainer:NOKIA N900 POWER SUPPLY DRIVERS)
linux-pm@vger.kernel.org (open list:POWER SUPPLY CLASS/SUBSYSTEM and 
DRIVERS)
linux-kernel@vger.kernel.org (open list)
(mail addresses masked by me)

You are not listed and as I try to avoid making more noise than needed 
when sending patches, I didn't include you in CC. Apologies if I should 
have CC-ed you.

Ivo

Re: Should 9aafabc7fece be reverted?

[Pali Rohár]

On Thursday 19 May 2016 09:44:04 Ivaylo Dimitrov wrote:
> Hi,
> 
> On 18.05.2016 19:58, Andrew F. Davis wrote:
> >Hello all,
> >
> >Why do we need this patch? I removed giving each battery a number in its
> >name intentionally. It doesn't seem to be needed anymore, and only two
> >old drivers out of the ~70 power supply drivers do this.
> >
> 
> The patch is needed because commit <703df6c09795> breaks existing userspace
> on Nokia N900, which counts on name numbering. Also, commit <703df6c09795>
> description says nothing about name changing, so I assumed it is an
> unintentional side-effect of splitting the code into modules.

Right, changing name cause breaking userspace and that commit does not
describe anything about changing name. So also from my perspective it is
regression.

> You are not listed and as I try to avoid making more noise than needed when
> sending patches, I didn't include you in CC. Apologies if I should have
> CC-ed you.

Maybe in future it could make sense to CC author of commit which
introduced regression...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: Should 9aafabc7fece be reverted?

[Ivaylo Dimitrov]

On 19.05.2016 12:17, Pali Rohár wrote:
>
> Maybe in future it could make sense to CC author of commit which
> introduced regression...
>

Yeah, my fault, sorry for that.

Re: Should 9aafabc7fece be reverted?

[Andrew F. Davis]

On 05/19/2016 04:17 AM, Pali Rohár wrote:
> On Thursday 19 May 2016 09:44:04 Ivaylo Dimitrov wrote:
>> Hi,
>>
>> On 18.05.2016 19:58, Andrew F. Davis wrote:
>>> Hello all,
>>>
>>> Why do we need this patch? I removed giving each battery a number in its
>>> name intentionally. It doesn't seem to be needed anymore, and only two
>>> old drivers out of the ~70 power supply drivers do this.
>>>
>>
>> The patch is needed because commit <703df6c09795> breaks existing userspace
>> on Nokia N900, which counts on name numbering. Also, commit <703df6c09795>
>> description says nothing about name changing, so I assumed it is an
>> unintentional side-effect of splitting the code into modules.
> 
> Right, changing name cause breaking userspace and that commit does not
> describe anything about changing name. So also from my perspective it is
> regression.
> 

Fair enough, I was unaware of the userspace breakage, the patch message
only stated it was being put back because it used to exist.

Thanks for the clarification,
Andrew

P.S. The code is still a bit strange, I'll probably go grab one of the
N900s from our test farm and make sure my future cleanups don't break
this, but are we sure the *name* of a driver is an ABI? Sound like ABI
abuse by the N900 software to me, almost like telling Linus he can't
change the kernel version number because a userspace script depends on
"uname == v2.6.x". :)

Re: Should 9aafabc7fece be reverted?

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 397 bytes --]

On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
> P.S. The code is still a bit strange, I'll probably go grab one of
> the N900s from our test farm and make sure my future cleanups don't
> break this, but are we sure the *name* of a driver is an ABI?

It is not name of driver, but directory name of sysfs path where device 
is exported...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Should 9aafabc7fece be reverted?

[Andrew F. Davis]

On 05/19/2016 10:44 AM, Pali Rohár wrote:
> On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
>> P.S. The code is still a bit strange, I'll probably go grab one of
>> the N900s from our test farm and make sure my future cleanups don't
>> break this, but are we sure the *name* of a driver is an ABI?
> 
> It is not name of driver, but directory name of sysfs path where device 
> is exported...
> 

Which is named after the drivers name, so the same question remains. :/

Re: Should 9aafabc7fece be reverted?

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 678 bytes --]

On Thursday 19 May 2016 17:48:29 Andrew F. Davis wrote:
> On 05/19/2016 10:44 AM, Pali Rohár wrote:
> > On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
> >> P.S. The code is still a bit strange, I'll probably go grab one of
> >> the N900s from our test farm and make sure my future cleanups
> >> don't break this, but are we sure the *name* of a driver is an
> >> ABI?
> > 
> > It is not name of driver, but directory name of sysfs path where
> > device is exported...
> 
> Which is named after the drivers name, so the same question remains.
> :/

No, it is not driver name, but device name. That is different.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Should 9aafabc7fece be reverted?

[Andrew F. Davis]

On 05/19/2016 10:51 AM, Pali Rohár wrote:
> On Thursday 19 May 2016 17:48:29 Andrew F. Davis wrote:
>> On 05/19/2016 10:44 AM, Pali Rohár wrote:
>>> On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
>>>> P.S. The code is still a bit strange, I'll probably go grab one of
>>>> the N900s from our test farm and make sure my future cleanups
>>>> don't break this, but are we sure the *name* of a driver is an
>>>> ABI?
>>>
>>> It is not name of driver, but directory name of sysfs path where
>>> device is exported...
>>
>> Which is named after the drivers name, so the same question remains.
>> :/
> 
> No, it is not driver name, but device name. That is different.
> 

My bad, that's what I meant, device names should be dynamic, right?
Relying on them being static in software would then be buggy (like
relying on eth0 being the right NIC everytime)?

I'm not familiar with the N900 software, but IDR can give out different
numbers and may not always give the first battery #0 (it does now but is
that a guarantee in IDR?) and if not then what is the userspace response
to this changing?

Re: Should 9aafabc7fece be reverted?

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1417 bytes --]

On Thursday 19 May 2016 17:57:34 Andrew F. Davis wrote:
> On 05/19/2016 10:51 AM, Pali Rohár wrote:
> > On Thursday 19 May 2016 17:48:29 Andrew F. Davis wrote:
> >> On 05/19/2016 10:44 AM, Pali Rohár wrote:
> >>> On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
> >>>> P.S. The code is still a bit strange, I'll probably go grab one
> >>>> of the N900s from our test farm and make sure my future
> >>>> cleanups don't break this, but are we sure the *name* of a
> >>>> driver is an ABI?
> >>> 
> >>> It is not name of driver, but directory name of sysfs path where
> >>> device is exported...
> >> 
> >> Which is named after the drivers name, so the same question
> >> remains.
> >> 
> >> :/
> > 
> > No, it is not driver name, but device name. That is different.
> 
> My bad, that's what I meant, device names should be dynamic, right?
> Relying on them being static in software would then be buggy (like
> relying on eth0 being the right NIC everytime)?
> 
> I'm not familiar with the N900 software, but IDR can give out
> different numbers and may not always give the first battery #0 (it
> does now but is that a guarantee in IDR?) and if not then what is
> the userspace response to this changing?

In case N900 would have more bq27xxx batteries, then yes. But N900 has 
exactly *one* bq27200 battery, so there is no IDR problem.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Should 9aafabc7fece be reverted?

[Andrew F. Davis]

On 05/19/2016 11:06 AM, Pali Rohár wrote:
> On Thursday 19 May 2016 17:57:34 Andrew F. Davis wrote:
>> On 05/19/2016 10:51 AM, Pali Rohár wrote:
>>> On Thursday 19 May 2016 17:48:29 Andrew F. Davis wrote:
>>>> On 05/19/2016 10:44 AM, Pali Rohár wrote:
>>>>> On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
>>>>>> P.S. The code is still a bit strange, I'll probably go grab one
>>>>>> of the N900s from our test farm and make sure my future
>>>>>> cleanups don't break this, but are we sure the *name* of a
>>>>>> driver is an ABI?
>>>>>
>>>>> It is not name of driver, but directory name of sysfs path where
>>>>> device is exported...
>>>>
>>>> Which is named after the drivers name, so the same question
>>>> remains.
>>>>
>>>> :/
>>>
>>> No, it is not driver name, but device name. That is different.
>>
>> My bad, that's what I meant, device names should be dynamic, right?
>> Relying on them being static in software would then be buggy (like
>> relying on eth0 being the right NIC everytime)?
>>
>> I'm not familiar with the N900 software, but IDR can give out
>> different numbers and may not always give the first battery #0 (it
>> does now but is that a guarantee in IDR?) and if not then what is
>> the userspace response to this changing?
> 
> In case N900 would have more bq27xxx batteries, then yes. But N900 has 
> exactly *one* bq27200 battery, so there is no IDR problem.
> 

Right, but if IDR returns 42 instead of 0 would this then break its
userspace software?

How is this handled for other power-supply drivers when more that one
battery exists, they look to give static device names so I'm assuming
the framework handles giving them new folder names automatically.

Re: Should 9aafabc7fece be reverted?

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2571 bytes --]

On Thursday 19 May 2016 18:11:14 Andrew F. Davis wrote:
> On 05/19/2016 11:06 AM, Pali Rohár wrote:
> > On Thursday 19 May 2016 17:57:34 Andrew F. Davis wrote:
> >> On 05/19/2016 10:51 AM, Pali Rohár wrote:
> >>> On Thursday 19 May 2016 17:48:29 Andrew F. Davis wrote:
> >>>> On 05/19/2016 10:44 AM, Pali Rohár wrote:
> >>>>> On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
> >>>>>> P.S. The code is still a bit strange, I'll probably go grab
> >>>>>> one of the N900s from our test farm and make sure my future
> >>>>>> cleanups don't break this, but are we sure the *name* of a
> >>>>>> driver is an ABI?
> >>>>> 
> >>>>> It is not name of driver, but directory name of sysfs path
> >>>>> where device is exported...
> >>>> 
> >>>> Which is named after the drivers name, so the same question
> >>>> remains.
> >>>> 
> >>>> :/
> >>> 
> >>> No, it is not driver name, but device name. That is different.
> >> 
> >> My bad, that's what I meant, device names should be dynamic,
> >> right? Relying on them being static in software would then be
> >> buggy (like relying on eth0 being the right NIC everytime)?
> >> 
> >> I'm not familiar with the N900 software, but IDR can give out
> >> different numbers and may not always give the first battery #0 (it
> >> does now but is that a guarantee in IDR?) and if not then what is
> >> the userspace response to this changing?
> > 
> > In case N900 would have more bq27xxx batteries, then yes. But N900
> > has exactly *one* bq27200 battery, so there is no IDR problem.
> 
> Right, but if IDR returns 42 instead of 0 would this then break its
> userspace software?

I think it is insane to starts generating random numbers for IDR... Hard 
to decide what happen if such situation occur...

What I can say is that N900 charging software and other battery 
applications stops working. And people who develop kernel for N900 
starts to be angry, because without working battery charging it is not 
possible to develop and fix other kernel bugs on device.

> How is this handled for other power-supply drivers when more that one
> battery exists, they look to give static device names so I'm assuming
> the framework handles giving them new folder names automatically.

N900 software can be happy that this problem does not happen... :-)

But basically in case there will be more batteries of same type, then it 
depends if it is needed to distinguish between them or not. So this is 
depends on exact situation and usage...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Should 9aafabc7fece be reverted?

[Andrew F. Davis]

On 05/19/2016 11:18 AM, Pali Rohár wrote:
> On Thursday 19 May 2016 18:11:14 Andrew F. Davis wrote:
>> On 05/19/2016 11:06 AM, Pali Rohár wrote:
>>> On Thursday 19 May 2016 17:57:34 Andrew F. Davis wrote:
>>>> On 05/19/2016 10:51 AM, Pali Rohár wrote:
>>>>> On Thursday 19 May 2016 17:48:29 Andrew F. Davis wrote:
>>>>>> On 05/19/2016 10:44 AM, Pali Rohár wrote:
>>>>>>> On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
>>>>>>>> P.S. The code is still a bit strange, I'll probably go grab
>>>>>>>> one of the N900s from our test farm and make sure my future
>>>>>>>> cleanups don't break this, but are we sure the *name* of a
>>>>>>>> driver is an ABI?
>>>>>>>
>>>>>>> It is not name of driver, but directory name of sysfs path
>>>>>>> where device is exported...
>>>>>>
>>>>>> Which is named after the drivers name, so the same question
>>>>>> remains.
>>>>>>
>>>>>> :/
>>>>>
>>>>> No, it is not driver name, but device name. That is different.
>>>>
>>>> My bad, that's what I meant, device names should be dynamic,
>>>> right? Relying on them being static in software would then be
>>>> buggy (like relying on eth0 being the right NIC everytime)?
>>>>
>>>> I'm not familiar with the N900 software, but IDR can give out
>>>> different numbers and may not always give the first battery #0 (it
>>>> does now but is that a guarantee in IDR?) and if not then what is
>>>> the userspace response to this changing?
>>>
>>> In case N900 would have more bq27xxx batteries, then yes. But N900
>>> has exactly *one* bq27200 battery, so there is no IDR problem.
>>
>> Right, but if IDR returns 42 instead of 0 would this then break its
>> userspace software?
> 
> I think it is insane to starts generating random numbers for IDR... Hard 
> to decide what happen if such situation occur...
> 

That's kind of what I'm looking for though, if the userspace break is
caused by software relying on the battery to be named "bq27200-0", then
nothing short of hard-coding this exact name will 100% safely fix the
N900 userspace software. And so using IDR and hoping it gives 0 is just
a hacky work-around way of just naming the device "bq27200-0".

> What I can say is that N900 charging software and other battery 
> applications stops working. And people who develop kernel for N900 
> starts to be angry, because without working battery charging it is not 
> possible to develop and fix other kernel bugs on device.
> 
>> How is this handled for other power-supply drivers when more that one
>> battery exists, they look to give static device names so I'm assuming
>> the framework handles giving them new folder names automatically.
> 
> N900 software can be happy that this problem does not happen... :-)
> 
> But basically in case there will be more batteries of same type, then it 
> depends if it is needed to distinguish between them or not. So this is 
> depends on exact situation and usage...
> 

I'll test this and see what happens, but I'm assuming there is a more
standard way to differentiate parts than appending a number to the
device name.

Re: Should 9aafabc7fece be reverted?

[Pavel Machek]

Hi!

> >>>>> No, it is not driver name, but device name. That is different.
> >>>>
> >>>> My bad, that's what I meant, device names should be dynamic,
> >>>> right? Relying on them being static in software would then be
> >>>> buggy (like relying on eth0 being the right NIC everytime)?
> >>>>
> >>>> I'm not familiar with the N900 software, but IDR can give out
> >>>> different numbers and may not always give the first battery #0 (it
> >>>> does now but is that a guarantee in IDR?) and if not then what is
> >>>> the userspace response to this changing?
> >>>
> >>> In case N900 would have more bq27xxx batteries, then yes. But N900
> >>> has exactly *one* bq27200 battery, so there is no IDR problem.
> >>
> >> Right, but if IDR returns 42 instead of 0 would this then break its
> >> userspace software?
> > 
> > I think it is insane to starts generating random numbers for IDR... Hard 
> > to decide what happen if such situation occur...
> > 
> 
> That's kind of what I'm looking for though, if the userspace break is
> caused by software relying on the battery to be named "bq27200-0", then
> nothing short of hard-coding this exact name will 100% safely fix the
> N900 userspace software. And so using IDR and hoping it gives 0 is just
> a hacky work-around way of just naming the device "bq27200-0".

N900 userspace is not intended to be portable. (And actually current
kernel support does not allow writing portable userspace for a
phone. It presents _3_ power supplies to userspace. How is userspace
expected to deal with that?) 

And if you ever name the single battery bq27200-42 or bq27200-1337,
you'll have a flock of angry penguins at your doorstep.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Should 9aafabc7fece be reverted?

[Andrew F. Davis]

On 05/20/2016 04:29 AM, Pavel Machek wrote:
> Hi!
> 
>>>>>>> No, it is not driver name, but device name. That is different.
>>>>>>
>>>>>> My bad, that's what I meant, device names should be dynamic,
>>>>>> right? Relying on them being static in software would then be
>>>>>> buggy (like relying on eth0 being the right NIC everytime)?
>>>>>>
>>>>>> I'm not familiar with the N900 software, but IDR can give out
>>>>>> different numbers and may not always give the first battery #0 (it
>>>>>> does now but is that a guarantee in IDR?) and if not then what is
>>>>>> the userspace response to this changing?
>>>>>
>>>>> In case N900 would have more bq27xxx batteries, then yes. But N900
>>>>> has exactly *one* bq27200 battery, so there is no IDR problem.
>>>>
>>>> Right, but if IDR returns 42 instead of 0 would this then break its
>>>> userspace software?
>>>
>>> I think it is insane to starts generating random numbers for IDR... Hard 
>>> to decide what happen if such situation occur...
>>>
>>
>> That's kind of what I'm looking for though, if the userspace break is
>> caused by software relying on the battery to be named "bq27200-0", then
>> nothing short of hard-coding this exact name will 100% safely fix the
>> N900 userspace software. And so using IDR and hoping it gives 0 is just
>> a hacky work-around way of just naming the device "bq27200-0".
> 
> N900 userspace is not intended to be portable. (And actually current
> kernel support does not allow writing portable userspace for a
> phone. It presents _3_ power supplies to userspace. How is userspace
> expected to deal with that?) 
> 
> And if you ever name the single battery bq27200-42 or bq27200-1337,
> you'll have a flock of angry penguins at your doorstep.
> 

haha, wouldn't want that.

That's what I'm trying to prevent here. Right now n900 userspace looks
for "bq27200-0" and *only* that, but the 0 does not necessarily mean it
is the first battery, it doesn't mean anything, we could have it mean
the first bq27x battery if we keep a count, right now we though we
depend on IDR behavior, which does not make any order guarantee.

Really we are abusing IDR, which is meant to return a unique ID that we
can use to fetch an associated pointer, but we don't do that, we are
just using it as a number generator, and that number needs to be zero.

Looking at the code you posted it looks like "n900-battery" was renamed
to "rx51-battery", I think this would be the correct fix here for the
bq27x batteries, there is only one of each so just dropping the "-0" and
looking for "bq27200" should fix all this.

Or we could always call the battery "bq27200-0" and not use IDR to
generate the number zero for us :)

Thanks,
Andrew

> Best regards,
> 									Pavel
> 

Re: Should 9aafabc7fece be reverted?

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 3347 bytes --]

On Monday 23 May 2016 17:55:49 Andrew F. Davis wrote:
> On 05/20/2016 04:29 AM, Pavel Machek wrote:
> > Hi!
> > 
> >>>>>>> No, it is not driver name, but device name. That is
> >>>>>>> different.
> >>>>>> 
> >>>>>> My bad, that's what I meant, device names should be dynamic,
> >>>>>> right? Relying on them being static in software would then be
> >>>>>> buggy (like relying on eth0 being the right NIC everytime)?
> >>>>>> 
> >>>>>> I'm not familiar with the N900 software, but IDR can give out
> >>>>>> different numbers and may not always give the first battery #0
> >>>>>> (it does now but is that a guarantee in IDR?) and if not then
> >>>>>> what is the userspace response to this changing?
> >>>>> 
> >>>>> In case N900 would have more bq27xxx batteries, then yes. But
> >>>>> N900 has exactly *one* bq27200 battery, so there is no IDR
> >>>>> problem.
> >>>> 
> >>>> Right, but if IDR returns 42 instead of 0 would this then break
> >>>> its userspace software?
> >>> 
> >>> I think it is insane to starts generating random numbers for
> >>> IDR... Hard to decide what happen if such situation occur...
> >> 
> >> That's kind of what I'm looking for though, if the userspace break
> >> is caused by software relying on the battery to be named
> >> "bq27200-0", then nothing short of hard-coding this exact name
> >> will 100% safely fix the N900 userspace software. And so using
> >> IDR and hoping it gives 0 is just a hacky work-around way of just
> >> naming the device "bq27200-0".
> > 
> > N900 userspace is not intended to be portable. (And actually
> > current kernel support does not allow writing portable userspace
> > for a phone. It presents _3_ power supplies to userspace. How is
> > userspace expected to deal with that?)
> > 
> > And if you ever name the single battery bq27200-42 or bq27200-1337,
> > you'll have a flock of angry penguins at your doorstep.
> 
> haha, wouldn't want that.
> 
> That's what I'm trying to prevent here. Right now n900 userspace
> looks for "bq27200-0" and *only* that, but the 0 does not
> necessarily mean it is the first battery, it doesn't mean anything,
> we could have it mean the first bq27x battery if we keep a count,
> right now we though we depend on IDR behavior, which does not make
> any order guarantee.
> 
> Really we are abusing IDR, which is meant to return a unique ID that
> we can use to fetch an associated pointer, but we don't do that, we
> are just using it as a number generator, and that number needs to be
> zero.
> 
> Looking at the code you posted it looks like "n900-battery" was
> renamed to "rx51-battery", I think this would be the correct fix
> here for the bq27x batteries, there is only one of each so just
> dropping the "-0" and looking for "bq27200" should fix all this.
> 
> Or we could always call the battery "bq27200-0" and not use IDR to
> generate the number zero for us :)
> 
> Thanks,
> Andrew

I think that IDR is used here to allow more bq27xxx batteries to be 
registered into power_supply subsystem. You cannot have two batteries 
with same name, as all are exported by sysfs...

Basically, if you store name into driver, device tree or IDR... I do not 
care, what I want is to have working code and working battery charging.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Should 9aafabc7fece be reverted?

[Andrew F. Davis]

On 05/23/2016 11:10 AM, Pali Rohár wrote:
> On Monday 23 May 2016 17:55:49 Andrew F. Davis wrote:
>> On 05/20/2016 04:29 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>>>> No, it is not driver name, but device name. That is
>>>>>>>>> different.
>>>>>>>>
>>>>>>>> My bad, that's what I meant, device names should be dynamic,
>>>>>>>> right? Relying on them being static in software would then be
>>>>>>>> buggy (like relying on eth0 being the right NIC everytime)?
>>>>>>>>
>>>>>>>> I'm not familiar with the N900 software, but IDR can give out
>>>>>>>> different numbers and may not always give the first battery #0
>>>>>>>> (it does now but is that a guarantee in IDR?) and if not then
>>>>>>>> what is the userspace response to this changing?
>>>>>>>
>>>>>>> In case N900 would have more bq27xxx batteries, then yes. But
>>>>>>> N900 has exactly *one* bq27200 battery, so there is no IDR
>>>>>>> problem.
>>>>>>
>>>>>> Right, but if IDR returns 42 instead of 0 would this then break
>>>>>> its userspace software?
>>>>>
>>>>> I think it is insane to starts generating random numbers for
>>>>> IDR... Hard to decide what happen if such situation occur...
>>>>
>>>> That's kind of what I'm looking for though, if the userspace break
>>>> is caused by software relying on the battery to be named
>>>> "bq27200-0", then nothing short of hard-coding this exact name
>>>> will 100% safely fix the N900 userspace software. And so using
>>>> IDR and hoping it gives 0 is just a hacky work-around way of just
>>>> naming the device "bq27200-0".
>>>
>>> N900 userspace is not intended to be portable. (And actually
>>> current kernel support does not allow writing portable userspace
>>> for a phone. It presents _3_ power supplies to userspace. How is
>>> userspace expected to deal with that?)
>>>
>>> And if you ever name the single battery bq27200-42 or bq27200-1337,
>>> you'll have a flock of angry penguins at your doorstep.
>>
>> haha, wouldn't want that.
>>
>> That's what I'm trying to prevent here. Right now n900 userspace
>> looks for "bq27200-0" and *only* that, but the 0 does not
>> necessarily mean it is the first battery, it doesn't mean anything,
>> we could have it mean the first bq27x battery if we keep a count,
>> right now we though we depend on IDR behavior, which does not make
>> any order guarantee.
>>
>> Really we are abusing IDR, which is meant to return a unique ID that
>> we can use to fetch an associated pointer, but we don't do that, we
>> are just using it as a number generator, and that number needs to be
>> zero.
>>
>> Looking at the code you posted it looks like "n900-battery" was
>> renamed to "rx51-battery", I think this would be the correct fix
>> here for the bq27x batteries, there is only one of each so just
>> dropping the "-0" and looking for "bq27200" should fix all this.
>>
>> Or we could always call the battery "bq27200-0" and not use IDR to
>> generate the number zero for us :)
>>
>> Thanks,
>> Andrew
> 
> I think that IDR is used here to allow more bq27xxx batteries to be 
> registered into power_supply subsystem. You cannot have two batteries 
> with same name, as all are exported by sysfs...
> 

Then this is a problem with the framework as I see very few power-supply
driver doing this unique naming thing. I can add framework support if
the need ever arises.

> Basically, if you store name into driver, device tree or IDR... I do not 
> care, what I want is to have working code and working battery charging.
> 

I would like this as well, hopefully these fixes can make things more
robust and prevent future breakages.

Andrew

Re: Should 9aafabc7fece be reverted?

[Pavel Machek]

Hi!

> > I think it is insane to starts generating random numbers for IDR... Hard 
> > to decide what happen if such situation occur...
> > 
> 
> That's kind of what I'm looking for though, if the userspace break is
> caused by software relying on the battery to be named "bq27200-0", then
> nothing short of hard-coding this exact name will 100% safely fix the
> N900 userspace software. And so using IDR and hoping it gives 0 is just
> a hacky work-around way of just naming the device "bq27200-0".

BTW proposals how to fix that userspace would be welcome.

We have three "power supplies", but userspace does not know how they
are connecting, resulting in strange results. For now, I have
hardware-specific layer in userspace.

https://gitlab.com/tui/tui/blob/master/ofone/hardware.py

Kernel does not really give me other choice.
									
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Should 9aafabc7fece be reverted?

[Pali Rohár]

On Friday 20 May 2016 11:34:32 Pavel Machek wrote:
> Hi!
> 
> > > I think it is insane to starts generating random numbers for IDR... Hard 
> > > to decide what happen if such situation occur...
> > > 
> > 
> > That's kind of what I'm looking for though, if the userspace break is
> > caused by software relying on the battery to be named "bq27200-0", then
> > nothing short of hard-coding this exact name will 100% safely fix the
> > N900 userspace software. And so using IDR and hoping it gives 0 is just
> > a hacky work-around way of just naming the device "bq27200-0".
> 
> BTW proposals how to fix that userspace would be welcome.
> 
> We have three "power supplies", but userspace does not know how they
> are connecting, resulting in strange results. For now, I have
> hardware-specific layer in userspace.
> 
> https://gitlab.com/tui/tui/blob/master/ofone/hardware.py
> 
> Kernel does not really give me other choice.

I have in my mind idea of "merged" power supply devices.

For Nokia N900 we have:
* bq27200 monitoring chip
* bq24150a charging chip
* isp1707 usb phy chargin chip (wallcharger detection + enable gpio)
* rx51_battery driver (export ADC values from battery pins: temperature + design capacity)

And each part export own power supply device in /sys/class/power_supply/
So there are 4 power supply devices!

In ideal world kernel should provide into userspace just two devices:
* power supply charger device (merged values from isp1707 and bq24150a)
* power supply battery device (merged values from rx51_battery and bq27200)

Problem is that e.g. design capacity value provide both drivers. But
bq27xxx just return constant and incorrect value from bq eeprom. Correct
design capacity return only rx51_battery (which read it via ADC, third
battery pin).

That should fix detection and discovery problem. On Nokia N900 there is
just one battery and one charger device.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: Should 9aafabc7fece be reverted?

[Andrew F. Davis]

On 05/20/2016 05:35 AM, Pali Rohár wrote:
> On Friday 20 May 2016 11:34:32 Pavel Machek wrote:
>> Hi!
>>
>>>> I think it is insane to starts generating random numbers for IDR... Hard 
>>>> to decide what happen if such situation occur...
>>>>
>>>
>>> That's kind of what I'm looking for though, if the userspace break is
>>> caused by software relying on the battery to be named "bq27200-0", then
>>> nothing short of hard-coding this exact name will 100% safely fix the
>>> N900 userspace software. And so using IDR and hoping it gives 0 is just
>>> a hacky work-around way of just naming the device "bq27200-0".
>>
>> BTW proposals how to fix that userspace would be welcome.
>>
>> We have three "power supplies", but userspace does not know how they
>> are connecting, resulting in strange results. For now, I have
>> hardware-specific layer in userspace.
>>
>> https://gitlab.com/tui/tui/blob/master/ofone/hardware.py
>>
>> Kernel does not really give me other choice.
> 
> I have in my mind idea of "merged" power supply devices.
> 
> For Nokia N900 we have:
> * bq27200 monitoring chip
> * bq24150a charging chip
> * isp1707 usb phy chargin chip (wallcharger detection + enable gpio)
> * rx51_battery driver (export ADC values from battery pins: temperature + design capacity)
> 
> And each part export own power supply device in /sys/class/power_supply/
> So there are 4 power supply devices!
> 
> In ideal world kernel should provide into userspace just two devices:
> * power supply charger device (merged values from isp1707 and bq24150a)
> * power supply battery device (merged values from rx51_battery and bq27200)
> 
> Problem is that e.g. design capacity value provide both drivers. But
> bq27xxx just return constant and incorrect value from bq eeprom. Correct
> design capacity return only rx51_battery (which read it via ADC, third
> battery pin).
> 
> That should fix detection and discovery problem. On Nokia N900 there is
> just one battery and one charger device.
> 

Right, so there is no need to append each type of power-supply device
with a number, as there is only one of each type of power-supply, they
can still be uniquely identified. And if ever a product comes out that
has two or more of the same power-supply then they can be identified by
the I2C address they are attached to.

Number them in the name would not even work as it would change depending
on probe order and not based on which battery is which.

Andrew

Re: Should 9aafabc7fece be reverted?

[Pavel Machek]

On Thu 2016-05-19 10:48:29, Andrew F. Davis wrote:
> On 05/19/2016 10:44 AM, Pali Rohár wrote:
> > On Thursday 19 May 2016 17:38:14 Andrew F. Davis wrote:
> >> P.S. The code is still a bit strange, I'll probably go grab one of
> >> the N900s from our test farm and make sure my future cleanups don't
> >> break this, but are we sure the *name* of a driver is an ABI?
> > 
> > It is not name of driver, but directory name of sysfs path where device 
> > is exported...
> > 
> 
> Which is named after the drivers name, so the same question remains. :/

We don't break userspace. And yes, phone software is somewhat more
tightly coupled than is usual in PC world.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html