Re: [PATCH v3 0/3] cpufreq: avoid redundant driver calls in schedutil

[Pingbo Wen]

On Thursday, July 14, 2016 04:25 AM, Steve Muckle wrote:
> Invoking the cpufreq driver to set a frequency can be expensive. On platforms
> with a cpufreq driver that does not support fast switching a thread must be
> woken to complete the operation. IPIs will also occur if and when support to
> process remote task wakeups is added in schedutil.
> 
> Currently schedutil calculates a raw frequency request from scheduler
> utilization data. This raw frequency request does not correlate to supported
> cpufreq driver frequencies aside from being capped by the CPU's maximum
> frequency. Consequently, there may be many consecutive requests for different
> raw frequency values which all translate to the same driver-supported
> frequency. For example on a platform with 3 supported frequencies 200MHz,
> 400MHz, and 600MHz, raw requests for 257MHz, 389MHz, and 307MHz all map to a
> driver-supported frequency of 400MHz in schedutil. Assuming these requests 
> were
> consecutive and there were no changes in policy limits (min/max), there is no
> need to issue the second or third request.
> 
> In order to resolve a raw frequency request to a driver-supported one a new
> cpufreq API is added, cpufreq_driver_resolve_freq(). This API relies on a new
> cpufreq driver callback in the case of ->target() style drivers. Otherwise it
> uses the existing frequency table operations.
> 
> Lookups are cached both in cpufreq_driver_resolve_freq() (for the benefit of 
> the
> driver) and in schedutil.
> 
> Changes since v2:
> - incorporated feedback from Viresh to use resolve_freq driver callbacks
>   only for ->target() style drivers, to use cpufreq's freq table operations,
>   and move freq mapping caching into cpufreq policy
> Changes since v1:
> - incorporated feedback from Rafael to avoid referencing freq_table from
>   schedutil by introducing a new cpufreq API
> 
> Steve Muckle (3):
>   cpufreq: add cpufreq_driver_resolve_freq()
>   cpufreq: schedutil: map raw required frequency to driver frequency

Tested the first two patches on db410c, only waking up irq_work 53
times, while previous was 257 times(79% decrease) in Android home idle
for 5 minutes.

>   cpufreq: acpi-cpufreq: use cached frequency mapping when possible
> 

Pingbo

Re: [PATCH v3 0/3] cpufreq: avoid redundant driver calls in schedutil

[Pingbo Wen]

On Thursday, July 14, 2016 04:25 AM, Steve Muckle wrote:
> Invoking the cpufreq driver to set a frequency can be expensive. On platforms
> with a cpufreq driver that does not support fast switching a thread must be
> woken to complete the operation. IPIs will also occur if and when support to
> process remote task wakeups is added in schedutil.
> 
> Currently schedutil calculates a raw frequency request from scheduler
> utilization data. This raw frequency request does not correlate to supported
> cpufreq driver frequencies aside from being capped by the CPU's maximum
> frequency. Consequently, there may be many consecutive requests for different
> raw frequency values which all translate to the same driver-supported
> frequency. For example on a platform with 3 supported frequencies 200MHz,
> 400MHz, and 600MHz, raw requests for 257MHz, 389MHz, and 307MHz all map to a
> driver-supported frequency of 400MHz in schedutil. Assuming these requests 
> were
> consecutive and there were no changes in policy limits (min/max), there is no
> need to issue the second or third request.
> 
> In order to resolve a raw frequency request to a driver-supported one a new
> cpufreq API is added, cpufreq_driver_resolve_freq(). This API relies on a new
> cpufreq driver callback in the case of ->target() style drivers. Otherwise it
> uses the existing frequency table operations.
> 
> Lookups are cached both in cpufreq_driver_resolve_freq() (for the benefit of 
> the
> driver) and in schedutil.
> 
> Changes since v2:
> - incorporated feedback from Viresh to use resolve_freq driver callbacks
>   only for ->target() style drivers, to use cpufreq's freq table operations,
>   and move freq mapping caching into cpufreq policy
> Changes since v1:
> - incorporated feedback from Rafael to avoid referencing freq_table from
>   schedutil by introducing a new cpufreq API
> 
> Steve Muckle (3):
>   cpufreq: add cpufreq_driver_resolve_freq()
>   cpufreq: schedutil: map raw required frequency to driver frequency

Tested the first two patches on db410c, only waking up irq_work 53
times, while previous was 257 times(79% decrease) in Android home idle
for 5 minutes.

>   cpufreq: acpi-cpufreq: use cached frequency mapping when possible
> 

Pingbo

Re: [RFC PATCH] regulator: introduce boot protection flag

[Pingbo Wen]

Hi, Mark

On Friday, June 17, 2016 07:42 PM, Mark Brown wrote:
> On Fri, Jun 17, 2016 at 11:34:25AM +0800, Pingbo Wen wrote:
>> On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote:
> 
>>> Having the consumer driver know that it's "critical" seems wrong since
>>> different systems may have different ideas about that, it's probably
>>> better to hook this in with the device model so that when the device
>>> finishes probing that kicks things off.
> 
>> That will imply the protection would be end when the specific device has
>> probed, and consumers should take their place at the same time. But
>> there have some other devices, which will set the consumer in a IRQ
>> event, or after some other events, can't be covered.
> 
> I don't understand what this means, sorry.
> 

I mean maybe there's some consumer driver only do partial initialization
during probing.

>> We can set the protection flag easily, but it's hard to tell whether a
>> consumer is well initialized, the end of protection, since regulator
>> consumer is not initialized within one call.
> 
> If the driver is not initializing itself during probe the driver is
> doing something wrong and needs to be fixed anyway.
> 

OK, if all driver have full initialized during probing, and we need
insert a hook after driver probing. I think we can add a function in
driver/base/dd.c:driver_probe_device() as this:

ret = really_probe(dev, drv);
...
if (!ret)
regulator_clean_up(dev);

And in regulator_clean_up(), we can iterate all regulator deivce, and
call a regulator_clear_boot_protection function:

if (!rdev->constraints->boot_protection)
return 0;

if (strcmp(rdev->constraints->critical_consumer, dev_name(dev)))
return 0;

rdev->constraints->boot_protection = 0;
...
-real clean stuffs-

the critical_consumer can be specified in devicetree.

Add a callback in driver_probe_device() is not so good, but it's fine
for me.

Pingbo

Re: [RFC PATCH] regulator: introduce boot protection flag

[Pingbo Wen]

Hi, Mark

On Friday, June 17, 2016 07:42 PM, Mark Brown wrote:
> On Fri, Jun 17, 2016 at 11:34:25AM +0800, Pingbo Wen wrote:
>> On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote:
> 
>>> Having the consumer driver know that it's "critical" seems wrong since
>>> different systems may have different ideas about that, it's probably
>>> better to hook this in with the device model so that when the device
>>> finishes probing that kicks things off.
> 
>> That will imply the protection would be end when the specific device has
>> probed, and consumers should take their place at the same time. But
>> there have some other devices, which will set the consumer in a IRQ
>> event, or after some other events, can't be covered.
> 
> I don't understand what this means, sorry.
> 

I mean maybe there's some consumer driver only do partial initialization
during probing.

>> We can set the protection flag easily, but it's hard to tell whether a
>> consumer is well initialized, the end of protection, since regulator
>> consumer is not initialized within one call.
> 
> If the driver is not initializing itself during probe the driver is
> doing something wrong and needs to be fixed anyway.
> 

OK, if all driver have full initialized during probing, and we need
insert a hook after driver probing. I think we can add a function in
driver/base/dd.c:driver_probe_device() as this:

ret = really_probe(dev, drv);
...
if (!ret)
regulator_clean_up(dev);

And in regulator_clean_up(), we can iterate all regulator deivce, and
call a regulator_clear_boot_protection function:

if (!rdev->constraints->boot_protection)
return 0;

if (strcmp(rdev->constraints->critical_consumer, dev_name(dev)))
return 0;

rdev->constraints->boot_protection = 0;
...
-real clean stuffs-

the critical_consumer can be specified in devicetree.

Add a callback in driver_probe_device() is not so good, but it's fine
for me.

Pingbo

Re: [RFC PATCH] regulator: introduce boot protection flag

[Pingbo Wen]

On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote:
> On Wed, Jun 15, 2016 at 08:05:13PM +0800, Pingbo Wen wrote:
>> On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:
>
>> If we take modules under consideration, and to make this patch more
>> universal, I think what we really need is adding a flag to protect a
>> regulator from registration to a specific consumer(not the first
>> consumer). The regulator driver gives the initial state, and the
>> specific consumer need to clear this flag while finishing regulator
>> setting(by calling a function like regulator_clear_protect()). And what
>> the regulator core need to do is staging all operations during
>> protection. And that will cover all consumers probing order, whenever
>> the regulator is registered.
> 
> Having the consumer driver know that it's "critical" seems wrong since
> different systems may have different ideas about that, it's probably
> better to hook this in with the device model so that when the device
> finishes probing that kicks things off.
> 

That will imply the protection would be end when the specific device has
probed, and consumers should take their place at the same time. But
there have some other devices, which will set the consumer in a IRQ
event, or after some other events, can't be covered.

We can set the protection flag easily, but it's hard to tell whether a
consumer is well initialized, the end of protection, since regulator
consumer is not initialized within one call.

Pingbo

Re: [RFC PATCH] regulator: introduce boot protection flag

[Pingbo Wen]

On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote:
> On Wed, Jun 15, 2016 at 08:05:13PM +0800, Pingbo Wen wrote:
>> On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:
>
>> If we take modules under consideration, and to make this patch more
>> universal, I think what we really need is adding a flag to protect a
>> regulator from registration to a specific consumer(not the first
>> consumer). The regulator driver gives the initial state, and the
>> specific consumer need to clear this flag while finishing regulator
>> setting(by calling a function like regulator_clear_protect()). And what
>> the regulator core need to do is staging all operations during
>> protection. And that will cover all consumers probing order, whenever
>> the regulator is registered.
> 
> Having the consumer driver know that it's "critical" seems wrong since
> different systems may have different ideas about that, it's probably
> better to hook this in with the device model so that when the device
> finishes probing that kicks things off.
> 

That will imply the protection would be end when the specific device has
probed, and consumers should take their place at the same time. But
there have some other devices, which will set the consumer in a IRQ
event, or after some other events, can't be covered.

We can set the protection flag easily, but it's hard to tell whether a
consumer is well initialized, the end of protection, since regulator
consumer is not initialized within one call.

Pingbo

Re: [RFC PATCH] regulator: introduce boot protection flag

[Pingbo Wen]

Hi, Mark

On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:
> On Mon, May 09, 2016 at 03:05:08PM +0800, WEN Pingbo wrote:
> 
>> And regulator core will postpone all operations until all consumers
>> have taked their place.
> 
> It doesn't, it postpones them until late_initacall().  This is both
> after the consumers have loaded if they are built in and before any
> consumers built as modules come up.

Yes, this patch only protects a regulator from regulator
registration(built in) to late_initcall(). But, IMO, if a regulator is
critical, it's weird to build as a module. Maybe I was thoughtless here.

If we take modules under consideration, and to make this patch more
universal, I think what we really need is adding a flag to protect a
regulator from registration to a specific consumer(not the first
consumer). The regulator driver gives the initial state, and the
specific consumer need to clear this flag while finishing regulator
setting(by calling a function like regulator_clear_protect()). And what
the regulator core need to do is staging all operations during
protection. And that will cover all consumers probing order, whenever
the regulator is registered.

Any idea?

> 
>> The boot_protection flag only work before late_initicall. And as other
>> constraints liked, you can specify this flag in a board file, or in
>> dts file.
> 
> Anything added to the DT ABI needs a binding.
> 

I will add bindings in next version.

>> +/* constraints check has already done */
>> +if (rdev->boot_mode)
>> +rdev->desc->ops->set_mode(rdev, rdev->boot_mode);
> 
> This whole sequence of code ignores errors - that's not great.  We
> should at least log them.
> 

OK.

>> +mutex_unlock(>mutex);
>> +
>> +if (regulator)
>> +regulator_set_voltage(regulator, regulator->min_uV,
>> +regulator->max_uV);
> 
> That's...  exciting.  There's a couple of issues here.  One is that
> this is not operating on the rdev but rather on a consumer regulator
> device, the other is that we drop out of the lock before doing the
> update which tends to be a warning sign that something fun is going on
> and at least an internal function should be used.  These two most likely
> come down to the same issue.
> 

OK, some bugs here. I will use a unlock version.

Pingbo

Re: [RFC PATCH] regulator: introduce boot protection flag

[Pingbo Wen]

Hi, Mark

On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:
> On Mon, May 09, 2016 at 03:05:08PM +0800, WEN Pingbo wrote:
> 
>> And regulator core will postpone all operations until all consumers
>> have taked their place.
> 
> It doesn't, it postpones them until late_initacall().  This is both
> after the consumers have loaded if they are built in and before any
> consumers built as modules come up.

Yes, this patch only protects a regulator from regulator
registration(built in) to late_initcall(). But, IMO, if a regulator is
critical, it's weird to build as a module. Maybe I was thoughtless here.

If we take modules under consideration, and to make this patch more
universal, I think what we really need is adding a flag to protect a
regulator from registration to a specific consumer(not the first
consumer). The regulator driver gives the initial state, and the
specific consumer need to clear this flag while finishing regulator
setting(by calling a function like regulator_clear_protect()). And what
the regulator core need to do is staging all operations during
protection. And that will cover all consumers probing order, whenever
the regulator is registered.

Any idea?

> 
>> The boot_protection flag only work before late_initicall. And as other
>> constraints liked, you can specify this flag in a board file, or in
>> dts file.
> 
> Anything added to the DT ABI needs a binding.
> 

I will add bindings in next version.

>> +/* constraints check has already done */
>> +if (rdev->boot_mode)
>> +rdev->desc->ops->set_mode(rdev, rdev->boot_mode);
> 
> This whole sequence of code ignores errors - that's not great.  We
> should at least log them.
> 

OK.

>> +mutex_unlock(>mutex);
>> +
>> +if (regulator)
>> +regulator_set_voltage(regulator, regulator->min_uV,
>> +regulator->max_uV);
> 
> That's...  exciting.  There's a couple of issues here.  One is that
> this is not operating on the rdev but rather on a consumer regulator
> device, the other is that we drop out of the lock before doing the
> update which tends to be a warning sign that something fun is going on
> and at least an internal function should be used.  These two most likely
> come down to the same issue.
> 

OK, some bugs here. I will use a unlock version.

Pingbo

Re: [RFC PATCH 2/2] regulator: add boot protection flag

[Pingbo Wen]

Hi, Mark

> 在 2016年4月25日，06:51，Mark Brown  写道：
> 
> On Sat, Apr 23, 2016 at 03:11:06PM +0800, WEN Pingbo wrote:
> 
>> This patch try to add a boot_protection flag in regulator constraints.
>> So the regulator core will prevent the specified operation during kernel
>> booting.
> 
>> The boot_protection flag only work before late_initicall. And as other
>> constraints liked, you can specify this flag in a board file, or in
>> dts file. By default, all operations of this regulator will be rejected
>> during kernel booting, if you add this flag in a regulator. But you
>> still have a chance to change this, by modifying boot_valid_ops_mask.
> 
> This is still a complete hack which is going to break as soon as things
> are built modular, it's definitely *not* something that should ever
> appear in DT since it depends so heavily on implementation details.  If
> you need some driver to start early work on getting that sorted.
> 

I think this patch can handle the case you mentioned. I have add a
regulator_has_booted flag, and it will set in regulator_init_complete()
late_initcall hook. The regulator_ops_is_valid() will ignore boot
protection if this flag is set.

> This is also going to interact badly with any other drivers that are
> trying to configure things at runtime, if they've done enables and
> disables (or especially an enable without a matching disable) their
> refcounts are going to be wrong and if they've tried to do anything with
> setting voltages we'll have completely ignored whatever they asked for
> or told them that they can't change voltages.  If we were doing anything
> like this it would need to be a lot more transparent to other
> regulators sharing the supplies (which are presumably what's causing
> problems here).

Ok, I have to admit that the boot_protection didn’t cover this. If other
consumer try to configure during booting, it will get some error code.
And the consumers need to re-configure the regulator state after
late_initcall.

If we need to hold the state of other consumer, I prefer using a
dummy-consumer to hold this. And this is my next try.

> 
>> @@ -868,7 +877,7 @@ static void print_constraints(struct regulator_dev *rdev)
>>  rdev_dbg(rdev, "%s\n", buf);
>> 
>>  if ((constraints->min_uV != constraints->max_uV) &&
>> -!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE))
>> +!(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE))
>>  rdev_warn(rdev,
>>"Voltage range but no REGULATOR_CHANGE_VOLTAGE\n");
>> }
> 
> This appears to be unrelated?
> 
>> +if (constraints->boot_protection) {
>> +if (of_property_read_bool(np, "boot-allow-set-voltage"))
>> +constraints->boot_valid_ops_mask |=
>> +REGULATOR_CHANGE_VOLTAGE;
> 
> We were factoring things out a minute ago…

Actually, the two part are only want to check the regulator operation
capacity, if we include the boot_protect checking, it will get error.

Pingbo

Re: [RFC PATCH 2/2] regulator: add boot protection flag

[Pingbo Wen]

Hi, Mark

> 在 2016年4月25日，06:51，Mark Brown  写道：
> 
> On Sat, Apr 23, 2016 at 03:11:06PM +0800, WEN Pingbo wrote:
> 
>> This patch try to add a boot_protection flag in regulator constraints.
>> So the regulator core will prevent the specified operation during kernel
>> booting.
> 
>> The boot_protection flag only work before late_initicall. And as other
>> constraints liked, you can specify this flag in a board file, or in
>> dts file. By default, all operations of this regulator will be rejected
>> during kernel booting, if you add this flag in a regulator. But you
>> still have a chance to change this, by modifying boot_valid_ops_mask.
> 
> This is still a complete hack which is going to break as soon as things
> are built modular, it's definitely *not* something that should ever
> appear in DT since it depends so heavily on implementation details.  If
> you need some driver to start early work on getting that sorted.
> 

I think this patch can handle the case you mentioned. I have add a
regulator_has_booted flag, and it will set in regulator_init_complete()
late_initcall hook. The regulator_ops_is_valid() will ignore boot
protection if this flag is set.

> This is also going to interact badly with any other drivers that are
> trying to configure things at runtime, if they've done enables and
> disables (or especially an enable without a matching disable) their
> refcounts are going to be wrong and if they've tried to do anything with
> setting voltages we'll have completely ignored whatever they asked for
> or told them that they can't change voltages.  If we were doing anything
> like this it would need to be a lot more transparent to other
> regulators sharing the supplies (which are presumably what's causing
> problems here).

Ok, I have to admit that the boot_protection didn’t cover this. If other
consumer try to configure during booting, it will get some error code.
And the consumers need to re-configure the regulator state after
late_initcall.

If we need to hold the state of other consumer, I prefer using a
dummy-consumer to hold this. And this is my next try.

> 
>> @@ -868,7 +877,7 @@ static void print_constraints(struct regulator_dev *rdev)
>>  rdev_dbg(rdev, "%s\n", buf);
>> 
>>  if ((constraints->min_uV != constraints->max_uV) &&
>> -!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE))
>> +!(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE))
>>  rdev_warn(rdev,
>>"Voltage range but no REGULATOR_CHANGE_VOLTAGE\n");
>> }
> 
> This appears to be unrelated?
> 
>> +if (constraints->boot_protection) {
>> +if (of_property_read_bool(np, "boot-allow-set-voltage"))
>> +constraints->boot_valid_ops_mask |=
>> +REGULATOR_CHANGE_VOLTAGE;
> 
> We were factoring things out a minute ago…

Actually, the two part are only want to check the regulator operation
capacity, if we include the boot_protect checking, it will get error.

Pingbo

Re: [RFC] shared regulator initialization and protection

[Pingbo Wen]

Hi, Mark

On Tuesday, April 12, 2016 11:48 AM, Mark Brown wrote:
> On Tue, Apr 12, 2016 at 10:44:11AM +0800, Pingbo Wen wrote:
> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.

Ok, I have re-send another pre-formated email, but you replied to the bad
one. Anyway, sorry for this.

> 
>> Those day, I get some fuzz idea in a proxy-consumer regulator driver
>> [1] from Qcom kernel tree. The driver is simple, that let some
>> critical regulator, which specified in DT tree, initializing in a
>> pre-defined state, and hold a regulator consumer during system boot,
>> to provide some protection between multi-consumers.
> 
>> And I think this driver is a hint that our upstream code lack of some
>> regulator intialization and protection. We already have some regulator
> 
> Given how poorly most of the Qualcom code uses the regulator API I'd
> take anything they've got out of tree with a pinch of salt.  At best
> this looks like a hack for some system specific issues but since it's
> just some undocumented code it's hard to tell.
> 

It's hard to tell how many Qualcom code uses the regulator API, I didn't
do such investigation. Maybe the 'proxy' consumer driver is just a
work-around code, but I don't think it's a reason to prevent us to get
some hints from it.

>> If a regulator only used by one consumer, current code will work fine.
>> But for the regulator which shared between multi-consumers, we can not
>> make sure that the regulator will work in a defined-state, during
>> system boot, since the device driver can probe in any order, and set
>> some conflict attributes.
> 
> What is the actual problem here?  Every driver is responsible for
> ensuring that regulators are in a good state before it starts using the
> hardware, if some other device did something before why do we care?
> 

Yes, I agree the driver should set the regulator before do the real work,
when the device life time is started from driver probing. But if the device
life time is started before Linux kernel loading, the regulator core should
do the regulator initialization and protection to power the device properly,
during kernel booting, until the consumer driver probed.

> 
> Anything based on doing things at initcall levels is fundamentally
> broken, as soon as things are built modular like most things in a distro
> kernel then it'll stop working.  If the system integrator needs some
> devices to start early to provide continuity (the main case here is
> display) they need to deal with that at the system integration level.
> 

Adding a initcall to do the consumer removing is not a decent method.
We can do some hack in regulator_get(), removing agent consumer until
specified device call regulator_get(), or the first regulator_get().

Pingbo

Re: [RFC] shared regulator initialization and protection

[Pingbo Wen]

Hi, Mark

On Tuesday, April 12, 2016 11:48 AM, Mark Brown wrote:
> On Tue, Apr 12, 2016 at 10:44:11AM +0800, Pingbo Wen wrote:
> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.

Ok, I have re-send another pre-formated email, but you replied to the bad
one. Anyway, sorry for this.

> 
>> Those day, I get some fuzz idea in a proxy-consumer regulator driver
>> [1] from Qcom kernel tree. The driver is simple, that let some
>> critical regulator, which specified in DT tree, initializing in a
>> pre-defined state, and hold a regulator consumer during system boot,
>> to provide some protection between multi-consumers.
> 
>> And I think this driver is a hint that our upstream code lack of some
>> regulator intialization and protection. We already have some regulator
> 
> Given how poorly most of the Qualcom code uses the regulator API I'd
> take anything they've got out of tree with a pinch of salt.  At best
> this looks like a hack for some system specific issues but since it's
> just some undocumented code it's hard to tell.
> 

It's hard to tell how many Qualcom code uses the regulator API, I didn't
do such investigation. Maybe the 'proxy' consumer driver is just a
work-around code, but I don't think it's a reason to prevent us to get
some hints from it.

>> If a regulator only used by one consumer, current code will work fine.
>> But for the regulator which shared between multi-consumers, we can not
>> make sure that the regulator will work in a defined-state, during
>> system boot, since the device driver can probe in any order, and set
>> some conflict attributes.
> 
> What is the actual problem here?  Every driver is responsible for
> ensuring that regulators are in a good state before it starts using the
> hardware, if some other device did something before why do we care?
> 

Yes, I agree the driver should set the regulator before do the real work,
when the device life time is started from driver probing. But if the device
life time is started before Linux kernel loading, the regulator core should
do the regulator initialization and protection to power the device properly,
during kernel booting, until the consumer driver probed.

> 
> Anything based on doing things at initcall levels is fundamentally
> broken, as soon as things are built modular like most things in a distro
> kernel then it'll stop working.  If the system integrator needs some
> devices to start early to provide continuity (the main case here is
> display) they need to deal with that at the system integration level.
> 

Adding a initcall to do the consumer removing is not a decent method.
We can do some hack in regulator_get(), removing agent consumer until
specified device call regulator_get(), or the first regulator_get().

Pingbo

[RFC] shared regulator initialization and protection

[Pingbo Wen]

(Resend, previous one rejected due to html link code)

Hi,

Those day, I get some fuzz idea in a proxy-consumer regulator driver [1] from 
Qcom kernel tree.
The driver is simple, that let some critical regulator, which specified in DT 
tree, initializing 
in a pre-defined state, and hold a regulator consumer during system boot, to 
provide some 
protection between multi-consumers.

And I think this driver is a hint that our upstream code lack of some regulator 
initialization protection.
We already have some regulator flags liked always_on, boot_on in the upstream 
code. The boot_on 
flag can do the enable work in boot time, the regulator_get_exclusive can 
provide some protection
from other consumers, and the regulator consumers itself will filter out some 
illegal and duplicated
operation.

If a regulator only used by one consumer, current code will work fine. But for 
the regulator which
shared between multi-consumers, we can not make sure that the regulator will 
work in a defined-state,
during system boot, since the device driver can probe in any order, and set 
some conflict attributes.

Currently, I didn’t have much solution for this problem, but I’m work on that. 
The simplest solution is
to create an agent regulator driver to do the first ‘regulator_get’, just like 
Qcom proxy consumer driver.
Or we define a master device of a regulator, only the master device can set the 
regulator attributes,
other device can only do the enable operation.

Any idea?

(Hope I have cc-ed the people right)

Pingbo

- [1]  
https://codeaurora.org/cgit/quic/la/kernel/msm-3.18/tree/drivers/regulator/proxy-consumer.c?h=rel/msm-3.18

[RFC] shared regulator initialization and protection

[Pingbo Wen]

(Resend, previous one rejected due to html link code)

Hi,

Those day, I get some fuzz idea in a proxy-consumer regulator driver [1] from 
Qcom kernel tree.
The driver is simple, that let some critical regulator, which specified in DT 
tree, initializing 
in a pre-defined state, and hold a regulator consumer during system boot, to 
provide some 
protection between multi-consumers.

And I think this driver is a hint that our upstream code lack of some regulator 
initialization protection.
We already have some regulator flags liked always_on, boot_on in the upstream 
code. The boot_on 
flag can do the enable work in boot time, the regulator_get_exclusive can 
provide some protection
from other consumers, and the regulator consumers itself will filter out some 
illegal and duplicated
operation.

If a regulator only used by one consumer, current code will work fine. But for 
the regulator which
shared between multi-consumers, we can not make sure that the regulator will 
work in a defined-state,
during system boot, since the device driver can probe in any order, and set 
some conflict attributes.

Currently, I didn’t have much solution for this problem, but I’m work on that. 
The simplest solution is
to create an agent regulator driver to do the first ‘regulator_get’, just like 
Qcom proxy consumer driver.
Or we define a master device of a regulator, only the master device can set the 
regulator attributes,
other device can only do the enable operation.

Any idea?

(Hope I have cc-ed the people right)

Pingbo

- [1]  
https://codeaurora.org/cgit/quic/la/kernel/msm-3.18/tree/drivers/regulator/proxy-consumer.c?h=rel/msm-3.18

Re: [Eas-dev] [PATCH] cpufreq_sched: set governor_data before waking up kschedfreq

[Pingbo Wen]

Hi, Juri

On Monday, February 22, 2016 06:53 PM, Juri Lelli wrote:
> Hi,
> 
> On 20/02/16 19:32, Pingbo Wen wrote:
>> Fix null pointer dereference error liked below. This BUG can be easily
>> re-produced by 'monkey --throttle 50' in android 6.0.
>>
> 
> I'm not sure which code base you are looking at here, but I think this
> problem has already been noticed and fixed by Ricky. Thanks for sharing
> your fix, though.

I pulled git://linux-arm.org/linux-power.git origin/energy_model_rfc_v5.2,
but didn't get any new commits about this bug. Where can I find the latest
branch?

Pingbo

Re: [Eas-dev] [PATCH] cpufreq_sched: set governor_data before waking up kschedfreq

[Pingbo Wen]

Hi, Juri

On Monday, February 22, 2016 06:53 PM, Juri Lelli wrote:
> Hi,
> 
> On 20/02/16 19:32, Pingbo Wen wrote:
>> Fix null pointer dereference error liked below. This BUG can be easily
>> re-produced by 'monkey --throttle 50' in android 6.0.
>>
> 
> I'm not sure which code base you are looking at here, but I think this
> problem has already been noticed and fixed by Ricky. Thanks for sharing
> your fix, though.

I pulled git://linux-arm.org/linux-power.git origin/energy_model_rfc_v5.2,
but didn't get any new commits about this bug. Where can I find the latest
branch?

Pingbo

[PATCH] cpufreq_sched: set governor_data before waking up kschedfreq

[Pingbo Wen]

Fix null pointer dereference error liked below. This BUG can be easily
re-produced by 'monkey --throttle 50' in android 6.0.

Unable to handle kernel NULL pointer dereference at virtual address 0010
[KERN Warning] check backtrace:
CPU: 0 PID: 10714 Comm: kschedfreq:0 Tainted:
Call trace:
[] dump_backtrace+0x0/0x15c
[] show_stack+0x10/0x1c
[] dump_stack+0x74/0xb8
[] debug_locks_off+0x4c/0x7c
[] oops_enter+0xc/0x28
[] die+0x2c/0x1a4
[] __do_kernel_fault.part.5+0x70/0x84
[] do_page_fault+0x344/0x348
[] do_translation_fault+0xbc/0xf0
[] do_mem_abort+0x38/0x9c
[] el1_da+0x14/0x80
[] kthread+0xd4/0xec

Signed-off-by: Pingbo Wen <pingbo@linaro.org>
---
 kernel/sched/cpufreq_sched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/cpufreq_sched.c b/kernel/sched/cpufreq_sched.c
index e1d208e..93731ef 100644
--- a/kernel/sched/cpufreq_sched.c
+++ b/kernel/sched/cpufreq_sched.c
@@ -265,6 +265,8 @@ static int cpufreq_sched_policy_init(struct cpufreq_policy 
*policy)
pr_debug("%s: throttle threshold = %u [ns]\n",
  __func__, gd->throttle_nsec);
 
+   policy->governor_data = gd;
+
if (cpufreq_driver_is_slow()) {
cpufreq_driver_slow = true;
gd->task = kthread_create(cpufreq_sched_thread, policy,
@@ -281,7 +283,6 @@ static int cpufreq_sched_policy_init(struct cpufreq_policy 
*policy)
init_irq_work(>irq_work, cpufreq_sched_irq_work);
}
 
-   policy->governor_data = gd;
set_sched_freq();
 
return 0;
-- 
1.9.1

[PATCH] cpufreq_sched: set governor_data before waking up kschedfreq

[Pingbo Wen]

Fix null pointer dereference error liked below. This BUG can be easily
re-produced by 'monkey --throttle 50' in android 6.0.

Unable to handle kernel NULL pointer dereference at virtual address 0010
[KERN Warning] check backtrace:
CPU: 0 PID: 10714 Comm: kschedfreq:0 Tainted:
Call trace:
[] dump_backtrace+0x0/0x15c
[] show_stack+0x10/0x1c
[] dump_stack+0x74/0xb8
[] debug_locks_off+0x4c/0x7c
[] oops_enter+0xc/0x28
[] die+0x2c/0x1a4
[] __do_kernel_fault.part.5+0x70/0x84
[] do_page_fault+0x344/0x348
[] do_translation_fault+0xbc/0xf0
[] do_mem_abort+0x38/0x9c
[] el1_da+0x14/0x80
[] kthread+0xd4/0xec

Signed-off-by: Pingbo Wen 
---
 kernel/sched/cpufreq_sched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/cpufreq_sched.c b/kernel/sched/cpufreq_sched.c
index e1d208e..93731ef 100644
--- a/kernel/sched/cpufreq_sched.c
+++ b/kernel/sched/cpufreq_sched.c
@@ -265,6 +265,8 @@ static int cpufreq_sched_policy_init(struct cpufreq_policy 
*policy)
pr_debug("%s: throttle threshold = %u [ns]\n",
  __func__, gd->throttle_nsec);
 
+   policy->governor_data = gd;
+
if (cpufreq_driver_is_slow()) {
cpufreq_driver_slow = true;
gd->task = kthread_create(cpufreq_sched_thread, policy,
@@ -281,7 +283,6 @@ static int cpufreq_sched_policy_init(struct cpufreq_policy 
*policy)
init_irq_work(>irq_work, cpufreq_sched_irq_work);
}
 
-   policy->governor_data = gd;
set_sched_freq();
 
return 0;
-- 
1.9.1

Re: [PATCH 0/3] introduce new evdev interface type

[Pingbo Wen]

> 在 2015年12月1日，18:47，Arnd Bergmann  写道：
> 
> On Tuesday 01 December 2015 16:34:00 Pingbo Wen wrote:
>> 
>> 
>> What kinds of changes in time_t? Extending it to 64-bits in both kernel
>> and userspace? Ok, I get confused here, if there are some sample codes
>> or use-cases here, it will help me a lot.
> 
> We don't change time_t in the kernel, we just try to replace it
> with time64_t, or ktime_t where appropriate.
> 
> What I meant is the problem when glibc defines their time_t to
> be 'long long' so that user space can be built that runs after
> 2038. This changes the timeval and timespec definitions, so
> a process that tries to use 'struct input_event' has a different
> layout compared to what the kernel has.

Ok, I didn’t reach this point. Thanks to clarify.

>> 
>> It seems we are mismatch here.
>> 
>> Actually input_composite_event has the similar structure with input_event,
>> but with a nicer definition, which can take both monotonic and non-monotonic
>> timestamps safely.
>> 
>> What I assumed here, is that leaving EV_IF_LEGACY as a untouched, deprecated
>> interface. If userspace try to adapt to new libc and kernel, it should move
>> to new interface. The userspace can do a lazy update, keep the code 
>> untouched,
>> but suffer the y2038 problem by itself.
> 
> Forcing the move to the new API is very ugly, because you can't do it
> in a way that works on old kernels as well, unless you then try to support
> both APIs at runtime.
> 
> Just requiring user space to switch to monotonic time is easily done,
> as it can likely work either way.

It seems I’m more aggressive:) But the legacy binary can still live 30 years 
longer
without any changes.

> 
>> We can force kernel using monotonic time in EV_IF_LEGACY interface, and
>> making input_event independent from time_t(after evdev has converted to
>> input_value, it’s easy to do that), but that also imply userspace
>> must change their code to fit this change. If changing userspace code is
>> a mandatory option, why not to force them do a complete conversion?
> 
> Most user space programs won't care, as they don't even look at the tv_sec
> portion, and the goal is to avoid having to change them.
> 
> There is still an open question to how exactly we want to get user space
> to change.
> 
> We could do some compile-time trick by having a union in struct input_event
> and mark the existing timeval part as deprecated, so we flag any use of the
> 32-bit tv_sec member, such as:
> 
> struct input_event {
> #if !defined(__KERNEL__) && __TIME_T_BITS == __BITS_PER_LONG
>struct timeval time;

> #else
>   struct {
>   union {
>   __u32 tv_sec __attribute__((deprecated));
>   __u32 tv_sec_monotonic;
>   };
>   __s32 tv_usec;
>   } time;
> #endif
>__u16 type;
>__u16 code;
>__s32 value;
> };

I have one question here, if userspace use this structure, all helper functions
of timeval will not work. And userspace need to write extra helper function for
this fake timeval. This just create an another urgly time structure.

And this method also forces most of old binaries to compile with new libc, 
adjust
their codes with new fake time structure.

Besides, I get an idea to combine your structure with input_composite_event:

union {
struct {
__s32 tv_usec;
__s32 tv_sec;
};
__s64 time;
} time;

I prefer to use a single s64 timestamp, if our goal is to remove timeval from 
kernel.

Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] introduce new evdev interface type

[Pingbo Wen]

> 在 2015年12月1日，18:47，Arnd Bergmann <a...@arndb.de> 写道：
> 
> On Tuesday 01 December 2015 16:34:00 Pingbo Wen wrote:
>> 
>> 
>> What kinds of changes in time_t? Extending it to 64-bits in both kernel
>> and userspace? Ok, I get confused here, if there are some sample codes
>> or use-cases here, it will help me a lot.
> 
> We don't change time_t in the kernel, we just try to replace it
> with time64_t, or ktime_t where appropriate.
> 
> What I meant is the problem when glibc defines their time_t to
> be 'long long' so that user space can be built that runs after
> 2038. This changes the timeval and timespec definitions, so
> a process that tries to use 'struct input_event' has a different
> layout compared to what the kernel has.

Ok, I didn’t reach this point. Thanks to clarify.

>> 
>> It seems we are mismatch here.
>> 
>> Actually input_composite_event has the similar structure with input_event,
>> but with a nicer definition, which can take both monotonic and non-monotonic
>> timestamps safely.
>> 
>> What I assumed here, is that leaving EV_IF_LEGACY as a untouched, deprecated
>> interface. If userspace try to adapt to new libc and kernel, it should move
>> to new interface. The userspace can do a lazy update, keep the code 
>> untouched,
>> but suffer the y2038 problem by itself.
> 
> Forcing the move to the new API is very ugly, because you can't do it
> in a way that works on old kernels as well, unless you then try to support
> both APIs at runtime.
> 
> Just requiring user space to switch to monotonic time is easily done,
> as it can likely work either way.

It seems I’m more aggressive:) But the legacy binary can still live 30 years 
longer
without any changes.

> 
>> We can force kernel using monotonic time in EV_IF_LEGACY interface, and
>> making input_event independent from time_t(after evdev has converted to
>> input_value, it’s easy to do that), but that also imply userspace
>> must change their code to fit this change. If changing userspace code is
>> a mandatory option, why not to force them do a complete conversion?
> 
> Most user space programs won't care, as they don't even look at the tv_sec
> portion, and the goal is to avoid having to change them.
> 
> There is still an open question to how exactly we want to get user space
> to change.
> 
> We could do some compile-time trick by having a union in struct input_event
> and mark the existing timeval part as deprecated, so we flag any use of the
> 32-bit tv_sec member, such as:
> 
> struct input_event {
> #if !defined(__KERNEL__) && __TIME_T_BITS == __BITS_PER_LONG
>struct timeval time;

> #else
>   struct {
>   union {
>   __u32 tv_sec __attribute__((deprecated));
>   __u32 tv_sec_monotonic;
>   };
>   __s32 tv_usec;
>   } time;
> #endif
>__u16 type;
>__u16 code;
>__s32 value;
> };

I have one question here, if userspace use this structure, all helper functions
of timeval will not work. And userspace need to write extra helper function for
this fake timeval. This just create an another urgly time structure.

And this method also forces most of old binaries to compile with new libc, 
adjust
their codes with new fake time structure.

Besides, I get an idea to combine your structure with input_composite_event:

union {
struct {
__s32 tv_usec;
__s32 tv_sec;
};
__s64 time;
} time;

I prefer to use a single s64 timestamp, if our goal is to remove timeval from 
kernel.

Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] introduce new evdev interface type

[Pingbo Wen]

Hi, Arnd

 
 The patch series add three evdev interface type:
 
 - EV_IF_LEGACY
 send event by input_event. This is the default option, keep kernel
 backward compatible.
>>> 
>>> The problem I see with this approach is that it still breaks any
>>> legacy source code that is compiled with a new libc that uses 64-bit
>>> time_t. If we are requiring source code changes for building users
>>> of input devices with a new libc, we can easily get them to handle
>>> the overflow (they normally only care about the microsecond portion
>>> anyway, so it doesn't matter in most cases), or to use monotonic time.
>> 
>> I don’t think so.
>> 
>> Actually, from the view of userspace, EV_IF_LEGACY interface is work
>> exactly the same as old evdev. We didn’t change anything in input_event
>> and input_event_compat. And the problem you said will still be there,
>> even without those patches.
> 
> I think we're still talking past one another. I thought we had established
> that
> 
> 1. the current interface is broken when time_t changes in user space

What kinds of changes in time_t? Extending it to 64-bits in both kernel
and userspace? Ok, I get confused here, if there are some sample codes
or use-cases here, it will help me a lot.

> 2. we can fix it by redefining struct input_event in a way that
>   is independent of time_t
> 3. once both user space and kernel are using the same ABI independent
>   of time_t, we can look improving the timestamps so they don't
>   overflow
> 4. the monotonic timestamp interface already avoids the overflow, so
>   it would be sufficient as a solution for 3.
> 
> Where did I lose you here? Did you find any other facts that I
> was missing? I don't know whether the two new event structures make
> the interface better in some other way, but it seems mostly unrelated
> to either of the two problems we already have with time_t (the
> ABI mismatch, and the use of non-monotonic timestamps).

It seems we are mismatch here.

Actually input_composite_event has the similar structure with input_event,
but with a nicer definition, which can take both monotonic and non-monotonic
timestamps safely.

What I assumed here, is that leaving EV_IF_LEGACY as a untouched, deprecated
interface. If userspace try to adapt to new libc and kernel, it should move
to new interface. The userspace can do a lazy update, keep the code untouched,
but suffer the y2038 problem by itself.

We can force kernel using monotonic time in EV_IF_LEGACY interface, and
making input_event independent from time_t(after evdev has converted to
input_value, it’s easy to do that), but that also imply userspace
must change their code to fit this change. If changing userspace code is
a mandatory option, why not to force them do a complete conversion?

Pingbo--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] introduce new evdev interface type

[Pingbo Wen]

Hi, Arnd

 
 The patch series add three evdev interface type:
 
 - EV_IF_LEGACY
 send event by input_event. This is the default option, keep kernel
 backward compatible.
>>> 
>>> The problem I see with this approach is that it still breaks any
>>> legacy source code that is compiled with a new libc that uses 64-bit
>>> time_t. If we are requiring source code changes for building users
>>> of input devices with a new libc, we can easily get them to handle
>>> the overflow (they normally only care about the microsecond portion
>>> anyway, so it doesn't matter in most cases), or to use monotonic time.
>> 
>> I don’t think so.
>> 
>> Actually, from the view of userspace, EV_IF_LEGACY interface is work
>> exactly the same as old evdev. We didn’t change anything in input_event
>> and input_event_compat. And the problem you said will still be there,
>> even without those patches.
> 
> I think we're still talking past one another. I thought we had established
> that
> 
> 1. the current interface is broken when time_t changes in user space

What kinds of changes in time_t? Extending it to 64-bits in both kernel
and userspace? Ok, I get confused here, if there are some sample codes
or use-cases here, it will help me a lot.

> 2. we can fix it by redefining struct input_event in a way that
>   is independent of time_t
> 3. once both user space and kernel are using the same ABI independent
>   of time_t, we can look improving the timestamps so they don't
>   overflow
> 4. the monotonic timestamp interface already avoids the overflow, so
>   it would be sufficient as a solution for 3.
> 
> Where did I lose you here? Did you find any other facts that I
> was missing? I don't know whether the two new event structures make
> the interface better in some other way, but it seems mostly unrelated
> to either of the two problems we already have with time_t (the
> ABI mismatch, and the use of non-monotonic timestamps).

It seems we are mismatch here.

Actually input_composite_event has the similar structure with input_event,
but with a nicer definition, which can take both monotonic and non-monotonic
timestamps safely.

What I assumed here, is that leaving EV_IF_LEGACY as a untouched, deprecated
interface. If userspace try to adapt to new libc and kernel, it should move
to new interface. The userspace can do a lazy update, keep the code untouched,
but suffer the y2038 problem by itself.

We can force kernel using monotonic time in EV_IF_LEGACY interface, and
making input_event independent from time_t(after evdev has converted to
input_value, it’s easy to do that), but that also imply userspace
must change their code to fit this change. If changing userspace code is
a mandatory option, why not to force them do a complete conversion?

Pingbo--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] introduce new evdev interface type

[Pingbo Wen]

Hi, Arnd

 
 The patch series add three evdev interface type:
 
 - EV_IF_LEGACY
 send event by input_event. This is the default option, keep kernel
 backward compatible.
>>> 
>>> The problem I see with this approach is that it still breaks any
>>> legacy source code that is compiled with a new libc that uses 64-bit
>>> time_t. If we are requiring source code changes for building users
>>> of input devices with a new libc, we can easily get them to handle
>>> the overflow (they normally only care about the microsecond portion
>>> anyway, so it doesn't matter in most cases), or to use monotonic time.
>> 
>> I don’t think so.
>> 
>> Actually, from the view of userspace, EV_IF_LEGACY interface is work
>> exactly the same as old evdev. We didn’t change anything in input_event
>> and input_event_compat. And the problem you said will still be there,
>> even without those patches.
> 
> I think we're still talking past one another. I thought we had established
> that
> 
> 1. the current interface is broken when time_t changes in user space

What kinds of changes in time_t? Extending it to 64-bits in both kernel
and userspace? Ok, I get confused here, if there are some sample codes
or use-cases here, it will help me a lot.

> 2. we can fix it by redefining struct input_event in a way that
>   is independent of time_t
> 3. once both user space and kernel are using the same ABI independent
>   of time_t, we can look improving the timestamps so they don't
>   overflow
> 4. the monotonic timestamp interface already avoids the overflow, so
>   it would be sufficient as a solution for 3.
> 
> Where did I lose you here? Did you find any other facts that I
> was missing? I don't know whether the two new event structures make
> the interface better in some other way, but it seems mostly unrelated
> to either of the two problems we already have with time_t (the
> ABI mismatch, and the use of non-monotonic timestamps).

It seems we are mismatch here.

Actually input_composite_event has the similar structure with input_event,
but with a nicer definition, which can take both monotonic and non-monotonic
timestamps safely.

What I assumed here, is that leaving EV_IF_LEGACY as a untouched, deprecated
interface. If userspace try to adapt to new libc and kernel, it should move
to new interface. The userspace can do a lazy update, keep the code untouched,
but suffer the y2038 problem by itself.

We can force kernel using monotonic time in EV_IF_LEGACY interface, and
making input_event independent from time_t(after evdev has converted to
input_value, it’s easy to do that), but that also imply userspace
must change their code to fit this change. If changing userspace code is
a mandatory option, why not to force them do a complete conversion?

Pingbo--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] introduce new evdev interface type

[Pingbo Wen]

Hi, Arnd

 
 The patch series add three evdev interface type:
 
 - EV_IF_LEGACY
 send event by input_event. This is the default option, keep kernel
 backward compatible.
>>> 
>>> The problem I see with this approach is that it still breaks any
>>> legacy source code that is compiled with a new libc that uses 64-bit
>>> time_t. If we are requiring source code changes for building users
>>> of input devices with a new libc, we can easily get them to handle
>>> the overflow (they normally only care about the microsecond portion
>>> anyway, so it doesn't matter in most cases), or to use monotonic time.
>> 
>> I don’t think so.
>> 
>> Actually, from the view of userspace, EV_IF_LEGACY interface is work
>> exactly the same as old evdev. We didn’t change anything in input_event
>> and input_event_compat. And the problem you said will still be there,
>> even without those patches.
> 
> I think we're still talking past one another. I thought we had established
> that
> 
> 1. the current interface is broken when time_t changes in user space

What kinds of changes in time_t? Extending it to 64-bits in both kernel
and userspace? Ok, I get confused here, if there are some sample codes
or use-cases here, it will help me a lot.

> 2. we can fix it by redefining struct input_event in a way that
>   is independent of time_t
> 3. once both user space and kernel are using the same ABI independent
>   of time_t, we can look improving the timestamps so they don't
>   overflow
> 4. the monotonic timestamp interface already avoids the overflow, so
>   it would be sufficient as a solution for 3.
> 
> Where did I lose you here? Did you find any other facts that I
> was missing? I don't know whether the two new event structures make
> the interface better in some other way, but it seems mostly unrelated
> to either of the two problems we already have with time_t (the
> ABI mismatch, and the use of non-monotonic timestamps).

It seems we are mismatch here.

Actually input_composite_event has the similar structure with input_event,
but with a nicer definition, which can take both monotonic and non-monotonic
timestamps safely.

What I assumed here, is that leaving EV_IF_LEGACY as a untouched, deprecated
interface. If userspace try to adapt to new libc and kernel, it should move
to new interface. The userspace can do a lazy update, keep the code untouched,
but suffer the y2038 problem by itself.

We can force kernel using monotonic time in EV_IF_LEGACY interface, and
making input_event independent from time_t(after evdev has converted to
input_value, it’s easy to do that), but that also imply userspace
must change their code to fit this change. If changing userspace code is
a mandatory option, why not to force them do a complete conversion?

Pingbo--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] input: evdev: add new ioctl EVIOCSIFTYPE / EVIOCGIFTYPE

[Pingbo Wen]

> 在 2015年11月28日，00:59，Arnd Bergmann  写道：
> 
> On Friday 27 November 2015 18:00:31 WEN Pingbo wrote:
>> This patch depends on 'introduce new evdev interface'.
>> 
>> Userspace cat set / get evdev interface type via the two ioctl
>> commands. And default interface type is EV_IF_LEGACY, so the old binary
>> will work normal with new kernel. Maybe we should change this default
>> option to encourage people to move to new interface.
>> 
>> And since all events are stored as input_value in evdev, there are no
>> need to flush evdev_client's buffer if we change clk_type and if_type.
> 
> I would split out the change to evdev_set_clk_type into a separate patch.

Agreed.

> 
>> +case EVIOCSIFTYPE:
>> +if (get_user(if_type, ip))
>> +return -EFAULT;
>> +
>> +return evdev_set_if_type(client, if_type);
>> +case EVIOCGIFTYPE:
>> +return put_user(client->if_type, ip);
>>  }
> 
> This look asymmetric: EVIOCSIFTYPE uses a EVDEV_* constant, while
> EVIOCGIFTYPE returns a EV_IF_* constant. Should those just
> be the same constants anyway?

Yes, thanks for pointing it out. I need add evdev_get_if_type() here.

Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] introduce new evdev interface type

[Pingbo Wen]

Hi, Arnd

> 在 2015年11月28日，00:58，Arnd Bergmann  写道：
> 
> On Friday 27 November 2015 18:00:29 WEN Pingbo wrote:
>> To solve the y2038 problem in input_event, I had some attempts before [1],
>> and this is the second one.
>> 
>> We can force userspace to use monotonic time in event timestamp, so the
>> 'struct timeval' is enough to keep y2038-safe, as Arnd suggested. But we
>> can not find a way to make kernel compatible with old binaries, which use
>> realtime, and there are still some devices, which depend on realtime.
>> 
>> So I get a idea to add a new evdev interface, which is y2038 safe. And
>> userspace can switch between the old and new interface via ioctl.
>> 
>> The patch series add three evdev interface type:
>> 
>> - EV_IF_LEGACY
>>  send event by input_event. This is the default option, keep kernel
>>  backward compatible.
> 
> The problem I see with this approach is that it still breaks any
> legacy source code that is compiled with a new libc that uses 64-bit
> time_t. If we are requiring source code changes for building users
> of input devices with a new libc, we can easily get them to handle
> the overflow (they normally only care about the microsecond portion
> anyway, so it doesn't matter in most cases), or to use monotonic time.

I don’t think so.

Actually, from the view of userspace, EV_IF_LEGACY interface is work
exactly the same as old evdev. We didn’t change anything in input_event
and input_event_compat. And the problem you said will still be there,
even without those patches.

Pingbo--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/3] introduce new evdev interface type

[Pingbo Wen]

Hi, Arnd

> 在 2015年11月28日，00:58，Arnd Bergmann  写道：
> 
> On Friday 27 November 2015 18:00:29 WEN Pingbo wrote:
>> To solve the y2038 problem in input_event, I had some attempts before [1],
>> and this is the second one.
>> 
>> We can force userspace to use monotonic time in event timestamp, so the
>> 'struct timeval' is enough to keep y2038-safe, as Arnd suggested. But we
>> can not find a way to make kernel compatible with old binaries, which use
>> realtime, and there are still some devices, which depend on realtime.
>> 
>> So I get a idea to add a new evdev interface, which is y2038 safe. And
>> userspace can switch between the old and new interface via ioctl.
>> 
>> The patch series add three evdev interface type:
>> 
>> - EV_IF_LEGACY
>>  send event by input_event. This is the default option, keep kernel
>>  backward compatible.
> 
> The problem I see with this approach is that it still breaks any
> legacy source code that is compiled with a new libc that uses 64-bit
> time_t. If we are requiring source code changes for building users
> of input devices with a new libc, we can easily get them to handle
> the overflow (they normally only care about the microsecond portion
> anyway, so it doesn't matter in most cases), or to use monotonic time.

I don’t think so.

Actually, from the view of userspace, EV_IF_LEGACY interface is work
exactly the same as old evdev. We didn’t change anything in input_event
and input_event_compat. And the problem you said will still be there,
even without those patches.

Pingbo--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] input: evdev: add new ioctl EVIOCSIFTYPE / EVIOCGIFTYPE

[Pingbo Wen]

> 在 2015年11月28日，00:59，Arnd Bergmann  写道：
> 
> On Friday 27 November 2015 18:00:31 WEN Pingbo wrote:
>> This patch depends on 'introduce new evdev interface'.
>> 
>> Userspace cat set / get evdev interface type via the two ioctl
>> commands. And default interface type is EV_IF_LEGACY, so the old binary
>> will work normal with new kernel. Maybe we should change this default
>> option to encourage people to move to new interface.
>> 
>> And since all events are stored as input_value in evdev, there are no
>> need to flush evdev_client's buffer if we change clk_type and if_type.
> 
> I would split out the change to evdev_set_clk_type into a separate patch.

Agreed.

> 
>> +case EVIOCSIFTYPE:
>> +if (get_user(if_type, ip))
>> +return -EFAULT;
>> +
>> +return evdev_set_if_type(client, if_type);
>> +case EVIOCGIFTYPE:
>> +return put_user(client->if_type, ip);
>>  }
> 
> This look asymmetric: EVIOCSIFTYPE uses a EVDEV_* constant, while
> EVIOCGIFTYPE returns a EV_IF_* constant. Should those just
> be the same constants anyway?

Yes, thanks for pointing it out. I need add evdev_get_if_type() here.

Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64

[Pingbo Wen]

> 在 2015年10月23日，17:45，Arnd Bergmann  写道：
> 
> On Friday 23 October 2015 17:12:38 Pingbo Wen wrote:
>> On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
>>>> -do_gettimeofday();
>>>> -
> Handling the jiffies overflow is trivially done through the time_before()
> and time_after() helpers, like
> 
> 
>   start = jiffies;
>   ...
>   now = jiffies;
>   timeout = start + HZ * timeout_usec / USEC_PER_SEC;
>   if (time_after(now, start + timeout_jiffies)
>   timeout();
>   else
>   mod_timer(timer, start + timeout_jiffies);
> 
> The time_after function works because unsigned overflow is well-defined
> in C (unlike signed overflow).
> 

Make sense, I will try this in next version.

Thanks,
Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH V2] hil_mlc: convert timeval to ktime_t

[Pingbo Wen]

> 在 2015年10月23日，17:55，Arnd Bergmann  写道：
> 
> On Friday 23 October 2015 17:24:59 WEN Pingbo wrote:
>> Using struct timeval will cause time overflow in 2038, replacing it with
>> ktime_t. And we don't need to handle sec and nsec separately.
>> 
> 
> This part looks good now, but as I commented in version 1, it should
> really be a separate patch rather than combined with the rest that
> is dealing with another use of timeval.

Ok, I will split it in next version.

>> -do_gettimeofday();
>> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
>> -tv.tv_usec -= mlc->instart.tv_usec;
>> -if (tv.tv_usec >= mlc->intimeout) goto sched;
>> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
>> -if (!tv.tv_usec) goto sched;
>> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
>> +if (tmp.tv64 >= (mlc->intimeout * NSEC_PER_USEC))
>> +goto sched;
>> +tmp.tv64 = mlc->intimeout * NSEC_PER_USEC - tmp.tv64;
>> +if (tmp.tv64 < NSEC_PER_USEC)
>> +goto sched;
>> +mod_timer(_mlcs_kicker,
>> +jiffies + nsecs_to_jiffies(tmp.tv64));
>>  break;
>>  sched:
>>  tasklet_schedule(_mlcs_tasklet);
> 
> If I read this right, the code is executed one for each input event such
> as a keypress or mouse movement. In the latter case, doing nsecs_to_jiffies()
> here is actually a bit expensive, and I stil think it can be avoided
> by just using jiffies.
> 
> For the (tmp.tv64 < NSEC_PER_USEC) part, did you just do that because
> I said this, or did you actually prove that it is required? I'm still
> confused about what the driver is trying to achieve here.

More explanation here:)
the judgement here is to prevent mod_timer with zero delta. I can not make sure 
whether the module
have nanosecond precise, so just keep same.

> 
>> diff --git a/drivers/input/serio/hp_sdc_mlc.c 
>> b/drivers/input/serio/hp_sdc_mlc.c
>> index d50f067..0a27b89 100644
>> --- a/drivers/input/serio/hp_sdc_mlc.c
>> +++ b/drivers/input/serio/hp_sdc_mlc.c
>> @@ -149,7 +149,8 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t 
>> timeout)
>> 
>>  /* Try to down the semaphore */
>>  if (down_trylock(>isem)) {
>> -struct timeval tv;
>> +ktime_t tmp = ktime_sub(ktime_get(), mlc->instart);
>> +
>>  if (priv->emtestmode) {
>>  mlc->ipacket[0] =
>>  HIL_ERR_INT | (mlc->opacket &
> 
> Maybe give the variable a more useful name than 'tmp'?
> 
> You could also remove the variable entirely if you slightly restructure
> the condition below.

I see, thanks for point out.

Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V3] hp_sdc: convert struct timeval to ktime_t

[Pingbo Wen]

> 在 2015年10月23日，19:45，Arnd Bergmann  写道：
> 
> On Friday 23 October 2015 19:29:39 WEN Pingbo wrote:
>> 1. struct timeval is not y2038 safe, convert it to ktime_t, and there is no 
>> need to handle sec and usec separately
>> 
>> 
> 
> The patch looks good now, but the changelog still needs a tiny bit of
> work. First of all, your line wrapping is off, please start a new line
> after around 70 characters as you do in an email.
> 

Well, little fault:)
Will be fixed in next version.

> Also, we don't normally have enumerated lists in a changelog, just use
> normal text. The best changelogs typically have three paragraphs:
> 
> The first paragraph describes what the driver currently does. For really
> obvious cases, this can be combined with the second paragraph.
> 
> The second paragraph explains why that is bad. This is where you can
> mention the monotonic time vs real time issue and say whether we
> just want the timeval removed to fix the kernel in general or whether
> this particular driver is broken.
> 
> The third paragraph explains what the patch does to resolve the issue
> described in the second one. This is also where you can list other
> approaches that would have solved the problem, and why you picked on
> over the others.

Do we really need this in ChangeLog? Commit msg already states this. I think
the purpose of ChangeLog is let people know the main difference of two
version patch at a glance, and the ‘what’ and ‘why’ should be placed in
commit msg.

Pingbo--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH V2] hp_sdc: fixed y2038 problem

[Pingbo Wen]

On Friday, October 23, 2015 05:25 PM, Arnd Bergmann wrote:
> On Friday 23 October 2015 16:53:26 WEN Pingbo wrote:
>> 1. Converting timeval to ktime_t, and there is no need to handle sec and
>> usec separately
>>
>> 2. hp_sdc.rtv is only used for time diff, monotonic time is better here
>>
>> Signed-off-by: WEN Pingbo 
>> ---
> 
> This version is still correct and looks better than the first version, but
> I'd still like you to improve some details:
> 
> - read some other changelogs and follow the common style. In particular,
>   in the subject line say /what/ you do ("e.g. use ktime_get instead of
>   do_gettimeofday",  or "avoid using struct timespec") instead of /why/,
>   but then explain in the changelog text what is wrong with the current
>   version and why it gets changed like this.
> 
> - Below the '---', add a short list of things you have changed since
>   the previous versions. This part will not get copied into the git
>   history.
> 

Ok, I will fix this in the next version.

>> -do_gettimeofday();
>> -if (tv.tv_sec > hp_sdc.rtv.tv_sec)
>> -tv.tv_usec += USEC_PER_SEC;
>> -
>> -if (tv.tv_usec - hp_sdc.rtv.tv_usec > HP_SDC_MAX_REG_DELAY) {
>> +if (time_diff.tv64 > HP_SDC_MAX_REG_DELAY) {
>>  hp_sdc_transaction *curr;
>>  uint8_t tmp;
>>  
>> -printk(KERN_WARNING PREFIX "read timeout (%ius)!\n",
>> -   (int)(tv.tv_usec - hp_sdc.rtv.tv_usec));
>> +printk(KERN_WARNING PREFIX "read timeout (%llins)!\n",
>> +   time_diff.tv64);
>>  curr->idx += hp_sdc.rqty;
>>  hp_sdc.rqty = 0;
>>  tmp = curr->seq[curr->actidx];
> 
> A tiny style comment here: please use ktime_to_ns() to extract the
> nanoseconds out of the ktime_t type rather than accessing the tv64
> member directly.

Same here.

Thank you
Pingbo

> 
>   Arnd
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64

[Pingbo Wen]

On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
> On Sunday 18 October 2015 17:45:19 WEN Pingbo wrote:
>> Using struct timeval will cause time overflow in 2038, replacing it with
>> a 64bit version.
>>
>> In addition, the origin driver try to covert usec to jiffies manually in
>> hilse_donode(). This is not a universal and safe way, using
>> nsecs_to_jiffies() to fix that.
>>
>> Signed-off-by: WEN Pingbo 
> 
> You should mention somewhere that you are also converting from real
> time to monotonic time, and why this is done.
> 
>> ---
>>  drivers/input/serio/hil_mlc.c| 31 +--
>>  drivers/input/serio/hp_sdc_mlc.c | 10 ++
>>  include/linux/hil_mlc.h  |  4 ++--
>>  3 files changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
>> index 65605e4..4e3b926 100644
>> --- a/drivers/input/serio/hil_mlc.c
>> +++ b/drivers/input/serio/hil_mlc.c
>> @@ -274,14 +274,14 @@ static int hilse_match(hil_mlc *mlc, int unused)
>>  /* An LCV used to prevent runaway loops, forces 5 second sleep when reset. 
>> */
>>  static int hilse_init_lcv(hil_mlc *mlc, int unused)
>>  {
>> -struct timeval tv;
>> +struct timespec64 ts64;
>>  
>> -do_gettimeofday();
>> +ktime_get_ts64();
>>  
>> -if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5)
>> +if (mlc->lcv && (ts64.tv_sec - mlc->lcv_ts64.tv_sec) < 5)
>>  return -1;
>>  
>> -mlc->lcv_tv = tv;
>> +mlc->lcv_ts64 = ts64;
>>  mlc->lcv = 0;
> 
> No need to rename the two variables here. Also, it seems we never access the
> tv_nsec portion at all, so this could use the simpler ktime_get_seconds()
> or even 'jiffies' instead.
> 
>> @@ -605,7 +605,7 @@ static inline void hilse_setup_input(hil_mlc *mlc, const 
>> struct hilse_node *node
>>  }
>>  mlc->istarted = 1;
>>  mlc->intimeout = node->arg;
>> -do_gettimeofday(&(mlc->instart));
>> +ktime_get_ts64(&(mlc->instart));
>>  mlc->icount = 15;
>>  memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
>>  BUG_ON(down_trylock(>isem));
> 
> This looks unrelated to the change above, so I would suggest making separate 
> patches.
> 
>> @@ -710,7 +710,7 @@ static int hilse_donode(hil_mlc *mlc)
>>  break;
>>  }
>>  mlc->ostarted = 0;
>> -do_gettimeofday(&(mlc->instart));
>> +ktime_get_ts64(&(mlc->instart));
>>  write_unlock_irqrestore(>lock, flags);
>>  nextidx = HILSEN_NEXT;
>>  break;
>> @@ -731,18 +731,21 @@ static int hilse_donode(hil_mlc *mlc)
>>  #endif
>>  
>>  while (nextidx & HILSEN_SCHED) {
>> -struct timeval tv;
>> +struct timespec64 ts64;
>>  
>>  if (!sched_long)
>>  goto sched;
>>  
>> -do_gettimeofday();
>> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
>> -tv.tv_usec -= mlc->instart.tv_usec;
>> -if (tv.tv_usec >= mlc->intimeout) goto sched;
>> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
>> -if (!tv.tv_usec) goto sched;
>> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
>> +ktime_get_ts64();
>> +ts64.tv_nsec += NSEC_PER_SEC *
>> +(ts64.tv_sec - mlc->instart.tv_sec);
>> +ts64.tv_nsec -= mlc->instart.tv_nsec;
> 
> tv_nsec will overflow here for any timeout over 4.3 seconds, where it
> used to overflow after 4294 seconds. This is almost certainly a bug.
> 
> You could work around that by using ktime_get_ns() to get a nanosecond
> value right away, but a 64-bit number is more expensive to convert to
> jiffies.

You are right, I didn't notice that tv_nsec is a 32bit variable. Maybe
we should use ktime_t here, so that handling sec and nsec separately is
needless.

Using jiffies here will need to take more codes to handle jiffies overflow
carefully. I think coverting 64bit number to jiffies is the price we must 
take, if we use 64bit version here.

> 
>> +if (ts64.tv_nsec >= (mlc->intimeout * NSEC_PER_USEC))
>> +goto sched;
>> +ts64.tv_nsec = mlc->intimeout * NSEC_PER_USEC - ts64.tv_nsec;
>> +if (!ts64.tv_nsec) goto sched;
> 
> As you are modifying the line, you should also fix the coding style to
> write
> 
>   if (!ts64.tv_nsec)
>   goto sched;
> 
> I also notice that you modify the behavior here, by changing from
> microsecond to nanosecond resolution, the equivalent of the original
> would have been
> 
>   if (ts64.tv_nsec < NSECS_PER_USEC)
> 
> 
> Your current version looks like it will practically never be true (meaning
> you hit the exact nanosecond). Is this conditional actually needed at all
> then? If it is, what is the intention and what should it be?
> 

Yes, compare to NSEC_PER_USEC is more safe, 

Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64

[Pingbo Wen]

On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
> On Sunday 18 October 2015 17:45:19 WEN Pingbo wrote:
>> Using struct timeval will cause time overflow in 2038, replacing it with
>> a 64bit version.
>>
>> In addition, the origin driver try to covert usec to jiffies manually in
>> hilse_donode(). This is not a universal and safe way, using
>> nsecs_to_jiffies() to fix that.
>>
>> Signed-off-by: WEN Pingbo 
> 
> You should mention somewhere that you are also converting from real
> time to monotonic time, and why this is done.
> 
>> ---
>>  drivers/input/serio/hil_mlc.c| 31 +--
>>  drivers/input/serio/hp_sdc_mlc.c | 10 ++
>>  include/linux/hil_mlc.h  |  4 ++--
>>  3 files changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
>> index 65605e4..4e3b926 100644
>> --- a/drivers/input/serio/hil_mlc.c
>> +++ b/drivers/input/serio/hil_mlc.c
>> @@ -274,14 +274,14 @@ static int hilse_match(hil_mlc *mlc, int unused)
>>  /* An LCV used to prevent runaway loops, forces 5 second sleep when reset. 
>> */
>>  static int hilse_init_lcv(hil_mlc *mlc, int unused)
>>  {
>> -struct timeval tv;
>> +struct timespec64 ts64;
>>  
>> -do_gettimeofday();
>> +ktime_get_ts64();
>>  
>> -if (mlc->lcv && (tv.tv_sec - mlc->lcv_tv.tv_sec) < 5)
>> +if (mlc->lcv && (ts64.tv_sec - mlc->lcv_ts64.tv_sec) < 5)
>>  return -1;
>>  
>> -mlc->lcv_tv = tv;
>> +mlc->lcv_ts64 = ts64;
>>  mlc->lcv = 0;
> 
> No need to rename the two variables here. Also, it seems we never access the
> tv_nsec portion at all, so this could use the simpler ktime_get_seconds()
> or even 'jiffies' instead.
> 
>> @@ -605,7 +605,7 @@ static inline void hilse_setup_input(hil_mlc *mlc, const 
>> struct hilse_node *node
>>  }
>>  mlc->istarted = 1;
>>  mlc->intimeout = node->arg;
>> -do_gettimeofday(&(mlc->instart));
>> +ktime_get_ts64(&(mlc->instart));
>>  mlc->icount = 15;
>>  memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
>>  BUG_ON(down_trylock(>isem));
> 
> This looks unrelated to the change above, so I would suggest making separate 
> patches.
> 
>> @@ -710,7 +710,7 @@ static int hilse_donode(hil_mlc *mlc)
>>  break;
>>  }
>>  mlc->ostarted = 0;
>> -do_gettimeofday(&(mlc->instart));
>> +ktime_get_ts64(&(mlc->instart));
>>  write_unlock_irqrestore(>lock, flags);
>>  nextidx = HILSEN_NEXT;
>>  break;
>> @@ -731,18 +731,21 @@ static int hilse_donode(hil_mlc *mlc)
>>  #endif
>>  
>>  while (nextidx & HILSEN_SCHED) {
>> -struct timeval tv;
>> +struct timespec64 ts64;
>>  
>>  if (!sched_long)
>>  goto sched;
>>  
>> -do_gettimeofday();
>> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
>> -tv.tv_usec -= mlc->instart.tv_usec;
>> -if (tv.tv_usec >= mlc->intimeout) goto sched;
>> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
>> -if (!tv.tv_usec) goto sched;
>> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
>> +ktime_get_ts64();
>> +ts64.tv_nsec += NSEC_PER_SEC *
>> +(ts64.tv_sec - mlc->instart.tv_sec);
>> +ts64.tv_nsec -= mlc->instart.tv_nsec;
> 
> tv_nsec will overflow here for any timeout over 4.3 seconds, where it
> used to overflow after 4294 seconds. This is almost certainly a bug.
> 
> You could work around that by using ktime_get_ns() to get a nanosecond
> value right away, but a 64-bit number is more expensive to convert to
> jiffies.

You are right, I didn't notice that tv_nsec is a 32bit variable. Maybe
we should use ktime_t here, so that handling sec and nsec separately is
needless.

Using jiffies here will need to take more codes to handle jiffies overflow
carefully. I think coverting 64bit number to jiffies is the price we must 
take, if we use 64bit version here.

> 
>> +if (ts64.tv_nsec >= (mlc->intimeout * NSEC_PER_USEC))
>> +goto sched;
>> +ts64.tv_nsec = mlc->intimeout * NSEC_PER_USEC - ts64.tv_nsec;
>> +if (!ts64.tv_nsec) goto sched;
> 
> As you are modifying the line, you should also fix the coding style to
> write
> 
>   if (!ts64.tv_nsec)
>   goto sched;
> 
> I also notice that you modify the behavior here, by changing from
> microsecond to nanosecond resolution, the equivalent of the original
> would have been
> 
>   if (ts64.tv_nsec < NSECS_PER_USEC)
> 
> 
> Your current version looks like it will practically never be true (meaning
> you hit the exact nanosecond). Is this conditional actually needed at all
> then? If it is, what is the intention and what should it be?
> 

Yes, compare to 

Re: [Y2038] [PATCH V2] hp_sdc: fixed y2038 problem

[Pingbo Wen]

On Friday, October 23, 2015 05:25 PM, Arnd Bergmann wrote:
> On Friday 23 October 2015 16:53:26 WEN Pingbo wrote:
>> 1. Converting timeval to ktime_t, and there is no need to handle sec and
>> usec separately
>>
>> 2. hp_sdc.rtv is only used for time diff, monotonic time is better here
>>
>> Signed-off-by: WEN Pingbo 
>> ---
> 
> This version is still correct and looks better than the first version, but
> I'd still like you to improve some details:
> 
> - read some other changelogs and follow the common style. In particular,
>   in the subject line say /what/ you do ("e.g. use ktime_get instead of
>   do_gettimeofday",  or "avoid using struct timespec") instead of /why/,
>   but then explain in the changelog text what is wrong with the current
>   version and why it gets changed like this.
> 
> - Below the '---', add a short list of things you have changed since
>   the previous versions. This part will not get copied into the git
>   history.
> 

Ok, I will fix this in the next version.

>> -do_gettimeofday();
>> -if (tv.tv_sec > hp_sdc.rtv.tv_sec)
>> -tv.tv_usec += USEC_PER_SEC;
>> -
>> -if (tv.tv_usec - hp_sdc.rtv.tv_usec > HP_SDC_MAX_REG_DELAY) {
>> +if (time_diff.tv64 > HP_SDC_MAX_REG_DELAY) {
>>  hp_sdc_transaction *curr;
>>  uint8_t tmp;
>>  
>> -printk(KERN_WARNING PREFIX "read timeout (%ius)!\n",
>> -   (int)(tv.tv_usec - hp_sdc.rtv.tv_usec));
>> +printk(KERN_WARNING PREFIX "read timeout (%llins)!\n",
>> +   time_diff.tv64);
>>  curr->idx += hp_sdc.rqty;
>>  hp_sdc.rqty = 0;
>>  tmp = curr->seq[curr->actidx];
> 
> A tiny style comment here: please use ktime_to_ns() to extract the
> nanoseconds out of the ktime_t type rather than accessing the tv64
> member directly.

Same here.

Thank you
Pingbo

> 
>   Arnd
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH V2] hil_mlc: convert timeval to ktime_t

[Pingbo Wen]

> 在 2015年10月23日，17:55，Arnd Bergmann  写道：
> 
> On Friday 23 October 2015 17:24:59 WEN Pingbo wrote:
>> Using struct timeval will cause time overflow in 2038, replacing it with
>> ktime_t. And we don't need to handle sec and nsec separately.
>> 
> 
> This part looks good now, but as I commented in version 1, it should
> really be a separate patch rather than combined with the rest that
> is dealing with another use of timeval.

Ok, I will split it in next version.

>> -do_gettimeofday();
>> -tv.tv_usec += USEC_PER_SEC * (tv.tv_sec - mlc->instart.tv_sec);
>> -tv.tv_usec -= mlc->instart.tv_usec;
>> -if (tv.tv_usec >= mlc->intimeout) goto sched;
>> -tv.tv_usec = (mlc->intimeout - tv.tv_usec) * HZ / USEC_PER_SEC;
>> -if (!tv.tv_usec) goto sched;
>> -mod_timer(_mlcs_kicker, jiffies + tv.tv_usec);
>> +if (tmp.tv64 >= (mlc->intimeout * NSEC_PER_USEC))
>> +goto sched;
>> +tmp.tv64 = mlc->intimeout * NSEC_PER_USEC - tmp.tv64;
>> +if (tmp.tv64 < NSEC_PER_USEC)
>> +goto sched;
>> +mod_timer(_mlcs_kicker,
>> +jiffies + nsecs_to_jiffies(tmp.tv64));
>>  break;
>>  sched:
>>  tasklet_schedule(_mlcs_tasklet);
> 
> If I read this right, the code is executed one for each input event such
> as a keypress or mouse movement. In the latter case, doing nsecs_to_jiffies()
> here is actually a bit expensive, and I stil think it can be avoided
> by just using jiffies.
> 
> For the (tmp.tv64 < NSEC_PER_USEC) part, did you just do that because
> I said this, or did you actually prove that it is required? I'm still
> confused about what the driver is trying to achieve here.

More explanation here:)
the judgement here is to prevent mod_timer with zero delta. I can not make sure 
whether the module
have nanosecond precise, so just keep same.

> 
>> diff --git a/drivers/input/serio/hp_sdc_mlc.c 
>> b/drivers/input/serio/hp_sdc_mlc.c
>> index d50f067..0a27b89 100644
>> --- a/drivers/input/serio/hp_sdc_mlc.c
>> +++ b/drivers/input/serio/hp_sdc_mlc.c
>> @@ -149,7 +149,8 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t 
>> timeout)
>> 
>>  /* Try to down the semaphore */
>>  if (down_trylock(>isem)) {
>> -struct timeval tv;
>> +ktime_t tmp = ktime_sub(ktime_get(), mlc->instart);
>> +
>>  if (priv->emtestmode) {
>>  mlc->ipacket[0] =
>>  HIL_ERR_INT | (mlc->opacket &
> 
> Maybe give the variable a more useful name than 'tmp'?
> 
> You could also remove the variable entirely if you slightly restructure
> the condition below.

I see, thanks for point out.

Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH] hil_mlc: convert timeval to timespec64

[Pingbo Wen]

> 在 2015年10月23日，17:45，Arnd Bergmann <a...@arndb.de> 写道：
> 
> On Friday 23 October 2015 17:12:38 Pingbo Wen wrote:
>> On Monday, October 19, 2015 04:58 PM, Arnd Bergmann wrote:
>>>> -do_gettimeofday();
>>>> -
> Handling the jiffies overflow is trivially done through the time_before()
> and time_after() helpers, like
> 
> 
>   start = jiffies;
>   ...
>   now = jiffies;
>   timeout = start + HZ * timeout_usec / USEC_PER_SEC;
>   if (time_after(now, start + timeout_jiffies)
>   timeout();
>   else
>   mod_timer(timer, start + timeout_jiffies);
> 
> The time_after function works because unsigned overflow is well-defined
> in C (unlike signed overflow).
> 

Make sense, I will try this in next version.

Thanks,
Pingbo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V3] hp_sdc: convert struct timeval to ktime_t

[Pingbo Wen]

> 在 2015年10月23日，19:45，Arnd Bergmann  写道：
> 
> On Friday 23 October 2015 19:29:39 WEN Pingbo wrote:
>> 1. struct timeval is not y2038 safe, convert it to ktime_t, and there is no 
>> need to handle sec and usec separately
>> 
>> 
> 
> The patch looks good now, but the changelog still needs a tiny bit of
> work. First of all, your line wrapping is off, please start a new line
> after around 70 characters as you do in an email.
> 

Well, little fault:)
Will be fixed in next version.

> Also, we don't normally have enumerated lists in a changelog, just use
> normal text. The best changelogs typically have three paragraphs:
> 
> The first paragraph describes what the driver currently does. For really
> obvious cases, this can be combined with the second paragraph.
> 
> The second paragraph explains why that is bad. This is where you can
> mention the monotonic time vs real time issue and say whether we
> just want the timeval removed to fix the kernel in general or whether
> this particular driver is broken.
> 
> The third paragraph explains what the patch does to resolve the issue
> described in the second one. This is also where you can list other
> approaches that would have solved the problem, and why you picked on
> over the others.

Do we really need this in ChangeLog? Commit msg already states this. I think
the purpose of ChangeLog is let people know the main difference of two
version patch at a glance, and the ‘what’ and ‘why’ should be placed in
commit msg.

Pingbo--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH] hp_sdc: fixed y2038 problem

[Pingbo Wen]

On Monday, October 19, 2015 05:01 PM, Arnd Bergmann wrote:
> On Sunday 18 October 2015 17:44:19 WEN Pingbo wrote:
>> Two replacements happened in this patch:
>> 1. using timespec64 to prevent time overflow in 2038
>> 2. using ktime_get_ts64 to avoid wall time issues(leap second, etc)
>>
>> Signed-off-by: WEN Pingbo 
>>
> 
> The patch looks correct, but I think this could be written a little
> simpler and clearer using ktime_get() or ktime_getns().
> 

That make sense, Maybe it can save a sec shift operation. I will check it later.

Pingbo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH] hp_sdc: fixed y2038 problem

[Pingbo Wen]

On Monday, October 19, 2015 05:01 PM, Arnd Bergmann wrote:
> On Sunday 18 October 2015 17:44:19 WEN Pingbo wrote:
>> Two replacements happened in this patch:
>> 1. using timespec64 to prevent time overflow in 2038
>> 2. using ktime_get_ts64 to avoid wall time issues(leap second, etc)
>>
>> Signed-off-by: WEN Pingbo 
>>
> 
> The patch looks correct, but I think this could be written a little
> simpler and clearer using ktime_get() or ktime_getns().
> 

That make sense, Maybe it can save a sec shift operation. I will check it later.

Pingbo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] dummy_hcd: replace timeval with timespec64

[Pingbo Wen]

On Thursday, September 17, 2015 05:59 PM, Peter Stuge wrote:
> WEN Pingbo wrote:
>> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
>> @@ -833,10 +833,10 @@ static const struct usb_ep_ops dummy_ep_ops = {
>>  /* there are both host and device side versions of this call ... */
>>  static int dummy_g_get_frame(struct usb_gadget *_gadget)
>>  {
>> -struct timeval  tv;
>> +struct timespec64 tv;
> 
> tv is very often used for timeval structs.
> 
> I suggest also changing the variable name, for example to ts, to
> avoid some possible confusion.
> 

Hi Peter,

Thanks for pointing it out, I will fix it in next version.

Pingbo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] dummy_hcd: replace timeval with timespec64

[Pingbo Wen]

On Thursday, September 17, 2015 05:59 PM, Peter Stuge wrote:
> WEN Pingbo wrote:
>> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
>> @@ -833,10 +833,10 @@ static const struct usb_ep_ops dummy_ep_ops = {
>>  /* there are both host and device side versions of this call ... */
>>  static int dummy_g_get_frame(struct usb_gadget *_gadget)
>>  {
>> -struct timeval  tv;
>> +struct timespec64 tv;
> 
> tv is very often used for timeval structs.
> 
> I suggest also changing the variable name, for example to ts, to
> avoid some possible confusion.
> 

Hi Peter,

Thanks for pointing it out, I will fix it in next version.

Pingbo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] dummy_hcd: replace timeval with timespec64

[Pingbo Wen]

add Alan Stern

On Thursday, September 17, 2015 11:09 AM, WEN Pingbo wrote:
> The millisecond of the last second will be normal if tv_sec is
> overflowed. But for y2038 consistency and demonstration purpose,
> and avoiding further risks, we need to remove 'timeval' in this
> driver, to avoid similair problems.
> 
> V2 Updates:
> - using monotonic time here by replacing getnstimeofday() with
>   ktime_get_ts64(), to avoid leap second issues. The frame time in USB
>   is always 1ms, no matter what speed, so ktime_get_ts64() have enough
>   resolution to cover this.
> - using NSEC_PER_MSEC instead of hard code.
> 
> Signed-off-by: Pingbo Wen 
> Cc: Y2038 
> Cc: linux-kernel@vger.kernel.org
> Cc: Arnd Bergmann 
> Cc: Felipe Balbi 
> Signed-off-by: WEN Pingbo 
> ---
>  drivers/usb/gadget/udc/dummy_hcd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
> b/drivers/usb/gadget/udc/dummy_hcd.c
> index 1379ad4..6d1ed35 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -833,10 +833,10 @@ static const struct usb_ep_ops dummy_ep_ops = {
>  /* there are both host and device side versions of this call ... */
>  static int dummy_g_get_frame(struct usb_gadget *_gadget)
>  {
> - struct timeval  tv;
> + struct timespec64 tv;
>  
> - do_gettimeofday();
> - return tv.tv_usec / 1000;
> + ktime_get_ts64();
> + return tv.tv_nsec / NSEC_PER_MSEC;
>  }
>  
>  static int dummy_wakeup(struct usb_gadget *_gadget)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2] dummy_hcd: replace timeval with timespec64

[Pingbo Wen]

add Alan Stern

On Thursday, September 17, 2015 11:09 AM, WEN Pingbo wrote:
> The millisecond of the last second will be normal if tv_sec is
> overflowed. But for y2038 consistency and demonstration purpose,
> and avoiding further risks, we need to remove 'timeval' in this
> driver, to avoid similair problems.
> 
> V2 Updates:
> - using monotonic time here by replacing getnstimeofday() with
>   ktime_get_ts64(), to avoid leap second issues. The frame time in USB
>   is always 1ms, no matter what speed, so ktime_get_ts64() have enough
>   resolution to cover this.
> - using NSEC_PER_MSEC instead of hard code.
> 
> Signed-off-by: Pingbo Wen <pingbo@linaro.org>
> Cc: Y2038 <y2...@lists.linaro.org>
> Cc: linux-kernel@vger.kernel.org
> Cc: Arnd Bergmann <a...@arndb.de>
> Cc: Felipe Balbi <ba...@ti.com>
> Signed-off-by: WEN Pingbo <pingbo@linaro.org>
> ---
>  drivers/usb/gadget/udc/dummy_hcd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
> b/drivers/usb/gadget/udc/dummy_hcd.c
> index 1379ad4..6d1ed35 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -833,10 +833,10 @@ static const struct usb_ep_ops dummy_ep_ops = {
>  /* there are both host and device side versions of this call ... */
>  static int dummy_g_get_frame(struct usb_gadget *_gadget)
>  {
> - struct timeval  tv;
> + struct timespec64 tv;
>  
> - do_gettimeofday();
> - return tv.tv_usec / 1000;
> + ktime_get_ts64();
> + return tv.tv_nsec / NSEC_PER_MSEC;
>  }
>  
>  static int dummy_wakeup(struct usb_gadget *_gadget)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH] dummy_hcd: replace timeval with timespec64

[Pingbo Wen]

On Tuesday, September 15, 2015 09:14 PM, Arnd Bergmann wrote:
> On Tuesday 15 September 2015 20:56:15 WEN Pingbo wrote:
>> The millisecond of the last second will be normal if tv_sec is
>> overflowed. But for y2038 consistency and demonstration purpose,
>> and avoiding further risks, we still need to fix it here,
>> to avoid similair problems.
>>
>> Signed-off-by: Pingbo Wen 
>> Cc: Y2038 
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Arnd Bergmann 
>> Cc: Felipe Balbi 
> 
> I just replied to the thread of the first mail, and it's good to see you had
> the same thought about adding the usb folks to the Cc list.
> 
> When sending a new version of a patch you have already sent before, it's
> common practice to use [PATCH v2] in the subject, so please do that for the
> next version.
> 

Ok, I thought previous thread is just draft, so I renewed one. Losing some 
history,
sorry for that.

> Your changelog text is better than the first version, but can still be 
> improved.
> I would at least mention that we want to remove all uses of 'timeval' from
> device drivers in order to better scan for y2038 problems in the drivers that
> still use them.
> 
> Also, you don't mention at all the discussion we had about real time vs.
> monotonic time for this driver. I think using monotonic time 
> (ktime_get_ts64())
> would be more appropriate here, but whichever you choose, you should explain
> in the changelog why you think that one is better than the other.
> Most of the time, we end up changing from real time to monotonic time in the
> same patch when converting a driver to avoid 32-bit time_t, so even you don't
> change it, you should explain why not.

ktime_get_ts64() is another choice. As we discussed in previous thread, only 
using
jiffies can not keep the precise, and we should also handle the jiffies 
overflowing
case, so I kept the old implementation.

> 
>>  drivers/usb/gadget/udc/dummy_hcd.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
>> b/drivers/usb/gadget/udc/dummy_hcd.c
>> index 1379ad4..7be721dad 100644
>> --- a/drivers/usb/gadget/udc/dummy_hcd.c
>> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
>> @@ -833,10 +833,10 @@ static const struct usb_ep_ops dummy_ep_ops = {
>>  /* there are both host and device side versions of this call ... */
>>  static int dummy_g_get_frame(struct usb_gadget *_gadget)
>>  {
>> -struct timeval  tv;
>> +struct timespec64 tv;
>>  
>> -do_gettimeofday();
>> -return tv.tv_usec / 1000;
>> +getnstimeofday64();
>> +return tv.tv_nsec / 100L;
>>  }
>>  
> 
> As in your other patch, I think the use of NSEC_PER_MSEC would make this
> slightly more understandable.
> 
>   Arnd
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Y2038] [PATCH] dummy_hcd: replace timeval with timespec64

[Pingbo Wen]

On Tuesday, September 15, 2015 09:14 PM, Arnd Bergmann wrote:
> On Tuesday 15 September 2015 20:56:15 WEN Pingbo wrote:
>> The millisecond of the last second will be normal if tv_sec is
>> overflowed. But for y2038 consistency and demonstration purpose,
>> and avoiding further risks, we still need to fix it here,
>> to avoid similair problems.
>>
>> Signed-off-by: Pingbo Wen <pingbo@linaro.org>
>> Cc: Y2038 <y2...@lists.linaro.org>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Arnd Bergmann <a...@arndb.de>
>> Cc: Felipe Balbi <ba...@ti.com>
> 
> I just replied to the thread of the first mail, and it's good to see you had
> the same thought about adding the usb folks to the Cc list.
> 
> When sending a new version of a patch you have already sent before, it's
> common practice to use [PATCH v2] in the subject, so please do that for the
> next version.
> 

Ok, I thought previous thread is just draft, so I renewed one. Losing some 
history,
sorry for that.

> Your changelog text is better than the first version, but can still be 
> improved.
> I would at least mention that we want to remove all uses of 'timeval' from
> device drivers in order to better scan for y2038 problems in the drivers that
> still use them.
> 
> Also, you don't mention at all the discussion we had about real time vs.
> monotonic time for this driver. I think using monotonic time 
> (ktime_get_ts64())
> would be more appropriate here, but whichever you choose, you should explain
> in the changelog why you think that one is better than the other.
> Most of the time, we end up changing from real time to monotonic time in the
> same patch when converting a driver to avoid 32-bit time_t, so even you don't
> change it, you should explain why not.

ktime_get_ts64() is another choice. As we discussed in previous thread, only 
using
jiffies can not keep the precise, and we should also handle the jiffies 
overflowing
case, so I kept the old implementation.

> 
>>  drivers/usb/gadget/udc/dummy_hcd.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
>> b/drivers/usb/gadget/udc/dummy_hcd.c
>> index 1379ad4..7be721dad 100644
>> --- a/drivers/usb/gadget/udc/dummy_hcd.c
>> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
>> @@ -833,10 +833,10 @@ static const struct usb_ep_ops dummy_ep_ops = {
>>  /* there are both host and device side versions of this call ... */
>>  static int dummy_g_get_frame(struct usb_gadget *_gadget)
>>  {
>> -struct timeval  tv;
>> +struct timespec64 tv;
>>  
>> -do_gettimeofday();
>> -return tv.tv_usec / 1000;
>> +getnstimeofday64();
>> +return tv.tv_nsec / 100L;
>>  }
>>  
> 
> As in your other patch, I think the use of NSEC_PER_MSEC would make this
> slightly more understandable.
> 
>   Arnd
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/