>-----Original Message-----
>From: Jan Kiszka <[email protected]>
>Sent: Wednesday, June 15, 2022 5:28 AM
>To: Chen, Hongzhan <[email protected]>; [email protected]
>Subject: Re: [Cobalt Xenoami3.1 PATCH 1/2] process: update clockfreq when
>receive corresponding event.
>
>On 27.05.22 08:22, Hongzhan Chen via Xenomai wrote:
>> 1. When there is clockfreq param passed down via command line, we
>> do not update clockfreq even if we receive event of updating clockfreq.
>> Or else, we update the clockfreq with notified value.
>> 2. At the same time, we would like to update clockfreq param showing
>> in sys filesystem after apply updated clockfreq.
>>
>> Signed-off-by: Hongzhan Chen <[email protected]>
>>
>> diff --git a/include/cobalt/kernel/init.h b/include/cobalt/kernel/init.h
>> index 41dd531a8..43144443b 100644
>> --- a/include/cobalt/kernel/init.h
>> +++ b/include/cobalt/kernel/init.h
>> @@ -51,4 +51,6 @@ void cobalt_remove_state_chain(struct notifier_block *nb);
>>
>> void cobalt_call_state_chain(enum cobalt_run_states newstate);
>>
>> +void cobalt_update_clockfreq_arg(unsigned long updatedclockfreq);
>
>Just "freq" is sufficient as parameter name.
>
>> +
>> #endif /* !_COBALT_KERNEL_INIT_H_ */
>> diff --git a/kernel/cobalt/include/asm-generic/xenomai/machine.h
>> b/kernel/cobalt/include/asm-generic/xenomai/machine.h
>> index 25764f989..aaa3edc97 100644
>> --- a/kernel/cobalt/include/asm-generic/xenomai/machine.h
>> +++ b/kernel/cobalt/include/asm-generic/xenomai/machine.h
>> @@ -61,6 +61,7 @@ struct cobalt_pipeline {
>> #ifdef CONFIG_SMP
>> cpumask_t supported_cpus;
>> #endif
>> + unsigned int passed_clockfreq;
>
>bool
>
>But I would rather make this a private flag in cobalt/init.c, see below.
Sorry , I can not find your comments which related to private flag you
mentioned in the below.
>
>> };
>>
>> extern struct cobalt_pipeline cobalt_pipeline;
>> diff --git a/kernel/cobalt/init.c b/kernel/cobalt/init.c
>> index dbe321c3b..a6cfc1e06 100644
>> --- a/kernel/cobalt/init.c
>> +++ b/kernel/cobalt/init.c
>> @@ -53,6 +53,19 @@ module_param_named(timerfreq, timerfreq_arg, ulong, 0444);
>> static unsigned long clockfreq_arg;
>> module_param_named(clockfreq, clockfreq_arg, ulong, 0444);
>>
>> +void cobalt_update_clockfreq_arg(unsigned long updatedclockfreq)
>> +{
>> + spl_t s;
>> +
>> + xnlock_get_irqsave(&nklock, s);
>> +
>> + clockfreq_arg = updatedclockfreq;
>> +
>> + xnlock_put_irqrestore(&nklock, s);
>
>How is synchronization supposed to work here?
>
>And when is clockfreq_arg supposed to be consumed after mach_setup? By
>whom? /sys/module/xenomai/parameters/?
Yes , /sys/module/xenomai/parameters/clockfreq would use consume it.
>
>To make this function more useful, it could perform the "was passed"
>check and also set clockfreq_arg and call xnclock_update_freq() if it
>was not defined via the command line.
>
>> +
>> +}
>> +EXPORT_SYMBOL_GPL(cobalt_update_clockfreq_arg);
>
>Why exporting this internal helper? Xenomai is always built into the kernel.
>
>> +
>> #ifdef CONFIG_SMP
>> static unsigned long supported_cpus_arg = -1;
>> module_param_named(supported_cpus, supported_cpus_arg, ulong, 0444);
>> @@ -148,8 +161,11 @@ static int __init mach_setup(void)
>> if (timerfreq_arg == 0)
>> timerfreq_arg = sysinfo.sys_hrtimer_freq;
>>
>> - if (clockfreq_arg == 0)
>> + if (clockfreq_arg == 0) {
>> clockfreq_arg = sysinfo.sys_hrclock_freq;
>> + cobalt_pipeline.passed_clockfreq = 0;
>
>Global variables are already zero-initialized.
>
>> + } else
>> + cobalt_pipeline.passed_clockfreq = 1;
>
>cobalt_pipeline.passed_clockfreq = clockfreq_arg != 0;
>
>>
>> if (clockfreq_arg == 0) {
>> printk(XENO_ERR "null clock frequency? Aborting.\n");
>> diff --git a/kernel/cobalt/posix/process.c b/kernel/cobalt/posix/process.c
>> index 6d1c1c427..41ea47b0d 100644
>> --- a/kernel/cobalt/posix/process.c
>> +++ b/kernel/cobalt/posix/process.c
>> @@ -38,6 +38,7 @@
>> #include <linux/kallsyms.h>
>> #include <linux/ipipe.h>
>> #include <linux/ipipe_tickdev.h>
>> +#include <cobalt/kernel/init.h>
>> #include <cobalt/kernel/sched.h>
>> #include <cobalt/kernel/heap.h>
>> #include <cobalt/kernel/synch.h>
>> @@ -1382,7 +1383,13 @@ static inline int handle_clockfreq_event(unsigned int
>> *p)
>> {
>> unsigned int newfreq = *p;
>>
>> - xnclock_update_freq(newfreq);
>> + /* when there is no para in commandline
>> + * passed down to set clockfreq
>
>Comment does not tell anything that isn't in the code already.
>
>> + */
>> + if (!cobalt_pipeline.passed_clockfreq) {
>> + xnclock_update_freq(newfreq);
>> + cobalt_update_clockfreq_arg(newfreq);
>> + }
>
>See my remark above: If you push xnclock_update_freq into
>cobalt_update_clockfreq, you can simplify to logic here, just call
>cobalt_update_clockfreq unconditionally.
>
>>
>> return KEVENT_PROPAGATE;
>> }
>
>And now I understand that none of this is needed for 3.2, only the
>I-pipe patch to send the kevent.
Yes, only I-pipe-based would have such issue. But actually 3.2 dropped
/sys/module/xenomai/parameters/clockfreq.
/sys/module/xenomai/parameters/clockfreq
scheme can keep the way for user at least from debug view to quickly validate
such issue via
passing clockfreq through commandline and know what clockfreq it is using and
is useful for I-PIPE-based.
Do we need to revert /sys/module/xenomai/parameters/clockfreq for I-PIPE in 3.2?
Regards
Hongzhan Chen
>
>Jan
>
>--
>Siemens AG, Technology
>Competence Center Embedded Linux
>
>
>
