From: Viresh Kumar <viresh.kumar@linaro.org> To: Saravana Kannan <skannan@codeaurora.org> Cc: "Srivatsa S. Bhat" <srivatsa@mit.edu>, "Rafael J . Wysocki" <rjw@rjwysocki.net>, Todd Poynor <toddpoynor@google.com>, "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Stephen Boyd <sboyd@codeaurora.org> Subject: Re: [PATCH v3 1/2] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Date: Thu, 17 Jul 2014 11:21:25 +0530 [thread overview] Message-ID: <CAKohponX1Wkk0ZOpV6C4i5pQr35b=zMN0L7mjfHVZDwq_A4ocw@mail.gmail.com> (raw) In-Reply-To: <53C6D90A.1000402@codeaurora.org> On 17 July 2014 01:26, Saravana Kannan <skannan@codeaurora.org> wrote: > No it's not. All the cpu*/ directories for all possible CPUs will be there > whether a CPU is online/offline. Which is why I also weed out impossible > CPUs, but you said the driver shouldn't be passing impossible CPUs anyway. > I'm just picking one directory to put the real cpufreq directory under. So, > the code as-is is definitely not broken. I may be wrong, and that's why I checked it with Srivatsa. He is quite familiar with hotplug code. Let me show the example again, its a bit tricky. I agree with you that sysfs nodes for CPUs stay as is with offline events, but we aren't talking about that here. On boot when we are trying to bring CPUs online, some of them may fail to come. And in that case, as confirmed by Srivatsa, there are no sysfs directories. This doesn't happen normally and is a very corner case. Still think we are wrong? > Sure, I can pick the first cpu that comes online to decide where to put the > real sysfs cpufreq directory, but then I have to keep track of that in a > separate field when it's time to remove it when the cpufreq driver is > unregistered. It works this way right now and I don't think we maintain any separate field here. Subsys-interface takes care of the order in which CPUs are added/ removed. And we don't have to handle that here. Just fix policy->cpu at first cpufreq_add_dev(). > And no, we > shouldn't be moving sysfs directory to stay with only an online directory. > That's the thing this patch is trying to simplify. Ahh, I really missed it in reviews. So, that's why you are looking at first cpu of related_cpus.. Hmm, so it is quite possible that we would end up in a case where policy->cpu wouldn't have sysfs directory created for it. Not sure if that might cause some hickups. @Srivatsa: ?? > So, I think using the first cpu in related CPUs will always work. If there's > any disagreement, I think it's purely a personal preference over adding > another field vs calling cpumask_first() That's what the problem with this patch was, just too big to miss important things :) I now understood why you had these extra cpumask_first() calls. But having said that, I still don't see a need to change the current behavior. The important point is kobject and links are added just once, no movement. And so, I would still like to add it to policy->cpu, i.e. the cpu which comes first. And this happens only once while we register a driver, so no side effects probably. Not every platform is going through hotplug/suspend/resume and keeping policy->cpu and sysfs node aligned atleast for them might not be that bad. Though it will work for any cpu.
WARNING: multiple messages have this Message-ID (diff)
From: viresh.kumar@linaro.org (Viresh Kumar) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 1/2] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Date: Thu, 17 Jul 2014 11:21:25 +0530 [thread overview] Message-ID: <CAKohponX1Wkk0ZOpV6C4i5pQr35b=zMN0L7mjfHVZDwq_A4ocw@mail.gmail.com> (raw) In-Reply-To: <53C6D90A.1000402@codeaurora.org> On 17 July 2014 01:26, Saravana Kannan <skannan@codeaurora.org> wrote: > No it's not. All the cpu*/ directories for all possible CPUs will be there > whether a CPU is online/offline. Which is why I also weed out impossible > CPUs, but you said the driver shouldn't be passing impossible CPUs anyway. > I'm just picking one directory to put the real cpufreq directory under. So, > the code as-is is definitely not broken. I may be wrong, and that's why I checked it with Srivatsa. He is quite familiar with hotplug code. Let me show the example again, its a bit tricky. I agree with you that sysfs nodes for CPUs stay as is with offline events, but we aren't talking about that here. On boot when we are trying to bring CPUs online, some of them may fail to come. And in that case, as confirmed by Srivatsa, there are no sysfs directories. This doesn't happen normally and is a very corner case. Still think we are wrong? > Sure, I can pick the first cpu that comes online to decide where to put the > real sysfs cpufreq directory, but then I have to keep track of that in a > separate field when it's time to remove it when the cpufreq driver is > unregistered. It works this way right now and I don't think we maintain any separate field here. Subsys-interface takes care of the order in which CPUs are added/ removed. And we don't have to handle that here. Just fix policy->cpu at first cpufreq_add_dev(). > And no, we > shouldn't be moving sysfs directory to stay with only an online directory. > That's the thing this patch is trying to simplify. Ahh, I really missed it in reviews. So, that's why you are looking at first cpu of related_cpus.. Hmm, so it is quite possible that we would end up in a case where policy->cpu wouldn't have sysfs directory created for it. Not sure if that might cause some hickups. @Srivatsa: ?? > So, I think using the first cpu in related CPUs will always work. If there's > any disagreement, I think it's purely a personal preference over adding > another field vs calling cpumask_first() That's what the problem with this patch was, just too big to miss important things :) I now understood why you had these extra cpumask_first() calls. But having said that, I still don't see a need to change the current behavior. The important point is kobject and links are added just once, no movement. And so, I would still like to add it to policy->cpu, i.e. the cpu which comes first. And this happens only once while we register a driver, so no side effects probably. Not every platform is going through hotplug/suspend/resume and keeping policy->cpu and sysfs node aligned atleast for them might not be that bad. Though it will work for any cpu.
next prev parent reply other threads:[~2014-07-17 5:51 UTC|newest] Thread overview: 205+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-07-10 2:37 [PATCH] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Saravana Kannan 2014-07-10 2:37 ` Saravana Kannan 2014-07-11 4:18 ` [PATCH v2] " Saravana Kannan 2014-07-11 4:18 ` Saravana Kannan 2014-07-11 6:19 ` Viresh Kumar 2014-07-11 6:19 ` Viresh Kumar 2014-07-11 6:19 ` Viresh Kumar 2014-07-11 9:59 ` skannan 2014-07-11 9:59 ` skannan at codeaurora.org 2014-07-11 9:59 ` skannan 2014-07-11 10:07 ` skannan 2014-07-11 10:07 ` skannan at codeaurora.org 2014-07-11 10:07 ` skannan 2014-07-11 10:52 ` Viresh Kumar 2014-07-11 10:52 ` Viresh Kumar 2014-07-11 10:52 ` Viresh Kumar 2014-07-12 2:44 ` Saravana Kannan 2014-07-12 2:44 ` Saravana Kannan 2014-07-12 2:44 ` Saravana Kannan 2014-07-14 6:09 ` Viresh Kumar 2014-07-14 6:09 ` Viresh Kumar 2014-07-14 6:09 ` Viresh Kumar 2014-07-14 19:08 ` Saravana Kannan 2014-07-14 19:08 ` Saravana Kannan 2014-07-14 19:08 ` Saravana Kannan 2014-07-15 4:35 ` Viresh Kumar 2014-07-15 4:35 ` Viresh Kumar 2014-07-15 4:35 ` Viresh Kumar 2014-07-15 5:36 ` Saravana Kannan 2014-07-15 5:36 ` Saravana Kannan 2014-07-15 5:36 ` Saravana Kannan 2014-07-15 5:52 ` Viresh Kumar 2014-07-15 5:52 ` Viresh Kumar 2014-07-15 5:52 ` Viresh Kumar 2014-07-15 6:58 ` Srivatsa S. Bhat 2014-07-15 6:58 ` Srivatsa S. Bhat 2014-07-15 6:58 ` Srivatsa S. Bhat 2014-07-15 17:35 ` skannan 2014-07-15 17:35 ` skannan at codeaurora.org 2014-07-15 17:35 ` skannan 2014-07-16 7:44 ` Srivatsa S. Bhat 2014-07-16 7:44 ` Srivatsa S. Bhat 2014-07-16 7:44 ` Srivatsa S. Bhat 2014-07-16 5:44 ` Viresh Kumar 2014-07-16 5:44 ` Viresh Kumar 2014-07-16 5:44 ` Viresh Kumar 2014-07-16 7:49 ` Srivatsa S. Bhat 2014-07-16 7:49 ` Srivatsa S. Bhat 2014-07-16 7:49 ` Srivatsa S. Bhat 2014-07-12 3:06 ` Saravana Kannan 2014-07-12 3:06 ` Saravana Kannan 2014-07-12 3:06 ` Saravana Kannan 2014-07-14 6:13 ` Viresh Kumar 2014-07-14 6:13 ` Viresh Kumar 2014-07-14 6:13 ` Viresh Kumar 2014-07-14 19:10 ` Saravana Kannan 2014-07-14 19:10 ` Saravana Kannan 2014-07-14 19:10 ` Saravana Kannan 2014-07-11 7:43 ` Srivatsa S. Bhat 2014-07-11 7:43 ` Srivatsa S. Bhat 2014-07-11 10:02 ` skannan 2014-07-11 10:02 ` skannan at codeaurora.org 2014-07-15 22:47 ` [PATCH v3 0/2] Simplify hotplug/suspend handling Saravana Kannan 2014-07-15 22:47 ` Saravana Kannan 2014-07-15 22:47 ` [PATCH v3 1/2] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Saravana Kannan 2014-07-15 22:47 ` Saravana Kannan 2014-07-16 0:28 ` Saravana Kannan 2014-07-16 0:28 ` Saravana Kannan 2014-07-16 8:30 ` Viresh Kumar 2014-07-16 8:30 ` Viresh Kumar 2014-07-16 8:30 ` Viresh Kumar 2014-07-16 19:19 ` Saravana Kannan 2014-07-16 19:19 ` Saravana Kannan 2014-07-16 19:19 ` Saravana Kannan 2014-07-16 8:24 ` Viresh Kumar 2014-07-16 8:24 ` Viresh Kumar 2014-07-16 8:24 ` Viresh Kumar 2014-07-16 11:16 ` Srivatsa S. Bhat 2014-07-16 11:16 ` Srivatsa S. Bhat 2014-07-16 11:16 ` Srivatsa S. Bhat 2014-07-16 13:13 ` Viresh Kumar 2014-07-16 13:13 ` Viresh Kumar 2014-07-16 13:13 ` Viresh Kumar 2014-07-16 18:04 ` Srivatsa S. Bhat 2014-07-16 18:04 ` Srivatsa S. Bhat 2014-07-16 18:04 ` Srivatsa S. Bhat 2014-07-16 19:56 ` Saravana Kannan 2014-07-16 19:56 ` Saravana Kannan 2014-07-16 19:56 ` Saravana Kannan 2014-07-17 5:51 ` Viresh Kumar [this message] 2014-07-17 5:51 ` Viresh Kumar 2014-07-17 5:51 ` Viresh Kumar 2014-07-16 19:56 ` Saravana Kannan 2014-07-16 19:56 ` Saravana Kannan 2014-07-16 19:56 ` Saravana Kannan 2014-07-17 5:35 ` Viresh Kumar 2014-07-17 5:35 ` Viresh Kumar 2014-07-17 5:35 ` Viresh Kumar 2014-07-18 3:25 ` Saravana Kannan 2014-07-18 3:25 ` Saravana Kannan 2014-07-18 3:25 ` Saravana Kannan 2014-07-18 4:19 ` Viresh Kumar 2014-07-18 4:19 ` Viresh Kumar 2014-07-18 4:19 ` Viresh Kumar 2014-07-16 20:25 ` Saravana Kannan 2014-07-16 20:25 ` Saravana Kannan 2014-07-16 20:25 ` Saravana Kannan 2014-07-16 21:45 ` Saravana Kannan 2014-07-16 21:45 ` Saravana Kannan 2014-07-16 21:45 ` Saravana Kannan 2014-07-17 6:24 ` Viresh Kumar 2014-07-17 6:24 ` Viresh Kumar 2014-07-17 6:24 ` Viresh Kumar 2014-07-16 14:29 ` Dirk Brandewie 2014-07-16 14:29 ` Dirk Brandewie 2014-07-16 15:28 ` Viresh Kumar 2014-07-16 15:28 ` Viresh Kumar 2014-07-16 15:28 ` Viresh Kumar 2014-07-16 19:42 ` Saravana Kannan 2014-07-16 19:42 ` Saravana Kannan 2014-07-16 19:42 ` Saravana Kannan 2014-07-15 22:47 ` [PATCH v3 2/2] cpufreq: Simplify and fix mutual exclusion with hotplug Saravana Kannan 2014-07-15 22:47 ` Saravana Kannan 2014-07-16 8:48 ` Viresh Kumar 2014-07-16 8:48 ` Viresh Kumar 2014-07-16 8:48 ` Viresh Kumar 2014-07-16 19:34 ` Saravana Kannan 2014-07-16 19:34 ` Saravana Kannan 2014-07-16 19:34 ` Saravana Kannan 2014-07-25 1:07 ` [PATCH v4 0/5] Simplify hotplug/suspend handling Saravana Kannan 2014-07-25 1:07 ` Saravana Kannan 2014-07-25 1:07 ` [PATCH v4 1/5] cpufreq: Don't wait for CPU to going offline to restart governor Saravana Kannan 2014-07-25 1:07 ` Saravana Kannan 2014-07-31 20:47 ` Saravana Kannan 2014-07-31 20:47 ` Saravana Kannan 2014-07-25 1:07 ` [PATCH v4 2/5] cpufreq: Keep track of which CPU owns the kobj/sysfs nodes separately Saravana Kannan 2014-07-25 1:07 ` Saravana Kannan 2014-08-07 9:02 ` Viresh Kumar 2014-08-07 9:02 ` Viresh Kumar 2014-08-07 9:02 ` Viresh Kumar 2014-07-25 1:07 ` [PATCH v4 3/5] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Saravana Kannan 2014-07-25 1:07 ` Saravana Kannan 2014-07-31 21:56 ` Rafael J. Wysocki 2014-07-31 21:56 ` Rafael J. Wysocki 2014-07-31 22:15 ` Saravana Kannan 2014-07-31 22:15 ` Saravana Kannan 2014-07-31 23:48 ` Saravana Kannan 2014-07-31 23:48 ` Saravana Kannan 2014-07-31 23:48 ` Saravana Kannan 2014-08-07 10:51 ` Viresh Kumar 2014-08-07 10:51 ` Viresh Kumar 2014-08-07 10:51 ` Viresh Kumar 2014-08-12 9:17 ` Viresh Kumar 2014-08-12 9:17 ` Viresh Kumar 2014-08-12 9:17 ` Viresh Kumar 2014-08-07 10:48 ` Viresh Kumar 2014-08-07 10:48 ` Viresh Kumar 2014-08-07 10:48 ` Viresh Kumar 2014-08-11 22:13 ` Saravana Kannan 2014-08-11 22:13 ` Saravana Kannan 2014-08-11 22:13 ` Saravana Kannan 2014-08-12 8:51 ` Viresh Kumar 2014-08-12 8:51 ` Viresh Kumar 2014-08-12 8:51 ` Viresh Kumar 2014-07-25 1:07 ` [PATCH v4 4/5] cpufreq: Properly handle physical CPU hot-add/hot-remove Saravana Kannan 2014-07-25 1:07 ` Saravana Kannan 2014-07-25 1:07 ` Saravana Kannan 2014-08-07 11:02 ` Viresh Kumar 2014-08-07 11:02 ` Viresh Kumar 2014-08-07 11:02 ` Viresh Kumar 2014-08-11 22:15 ` Saravana Kannan 2014-08-11 22:15 ` Saravana Kannan 2014-08-11 22:15 ` Saravana Kannan 2014-07-25 1:07 ` [PATCH v4 5/5] cpufreq: Delete dead code related to policy save/restore Saravana Kannan 2014-07-25 1:07 ` Saravana Kannan 2014-08-07 11:06 ` Viresh Kumar 2014-08-07 11:06 ` Viresh Kumar 2014-08-07 11:06 ` Viresh Kumar 2014-07-29 5:52 ` [PATCH v4 0/5] Simplify hotplug/suspend handling skannan 2014-07-29 5:52 ` skannan at codeaurora.org 2014-07-29 5:52 ` skannan 2014-07-30 0:29 ` Rafael J. Wysocki 2014-07-30 0:29 ` Rafael J. Wysocki 2014-07-31 20:25 ` Saravana Kannan 2014-07-31 20:25 ` Saravana Kannan 2014-08-07 6:04 ` skannan 2014-08-07 6:04 ` skannan at codeaurora.org 2014-10-16 8:53 ` Viresh Kumar 2014-10-16 8:53 ` Viresh Kumar 2014-10-16 8:53 ` Viresh Kumar 2014-10-23 21:41 ` Saravana Kannan 2014-10-23 21:41 ` Saravana Kannan 2014-10-23 21:41 ` Saravana Kannan 2014-07-16 22:02 ` [PATCH] cpufreq: Don't destroy/realloc policy/sysfs on hotplug/suspend Rafael J. Wysocki 2014-07-16 22:02 ` Rafael J. Wysocki 2014-07-16 22:35 ` Saravana Kannan 2014-07-16 22:35 ` Saravana Kannan 2014-07-24 3:02 ` Saravana Kannan 2014-07-24 3:02 ` Saravana Kannan 2014-07-24 5:04 ` Viresh Kumar 2014-07-24 5:04 ` Viresh Kumar 2014-07-24 5:04 ` Viresh Kumar 2014-07-24 9:12 ` skannan 2014-07-24 9:12 ` skannan at codeaurora.org 2014-07-24 9:12 ` skannan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAKohponX1Wkk0ZOpV6C4i5pQr35b=zMN0L7mjfHVZDwq_A4ocw@mail.gmail.com' \ --to=viresh.kumar@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=rjw@rjwysocki.net \ --cc=sboyd@codeaurora.org \ --cc=skannan@codeaurora.org \ --cc=srivatsa@mit.edu \ --cc=toddpoynor@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.