Re: [RFC PATCH v3 15/16] drivers: coresight: Add generic CoreSight cpu power notifications.

Hi,

On Wed, Jun 05, 2019 at 08:03:12PM +0100, Mike Leach wrote:

Currently each driver type sets up own CPU power notifications such as CPU hotplug etc. Need a common mechanism for all driver types to use.

Adds a notification mechanism for CPU power events such as CPU hotplug as a general notification mechnism in CoreSight common code to be used by all CPU bound CS devices.

Provides a per-device registration mechanism with callback fn and event type notifier. Core code now manages the CPU hotplug registrations and calls back any registered device when events occur.

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/coresight-priv.h | 26 +++ drivers/hwtracing/coresight/coresight.c | 167 ++++++++++++++++++- 2 files changed, 192 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 9c4fd7eb56eb..5ea2f86f71e0 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -214,4 +214,30 @@ void coresight_release_platform_data(struct coresight_platform_data *pdata); int coresight_device_fwnode_match(struct device *dev, void *fwnode); +/*

• • cpu power management - allow any CPU bound Coresight device
• • to use a common registration infrastructure for notifications.
• */

+/* cpu pm event types */ +enum cs_cpu_pm_events {

• CS_CPUPM_SMP_ON_REG_CPU, /* smp call on cpu at registration */
• CS_CPUPM_CPUHP_CS_START, /* hotplug CoreSight starting CPU */
• CS_CPUPM_CPUHP_CS_STOP, /* hotplug CoreSight stopping CPU */
• CS_CPUPM_CPUHP_AP_DYN_ONLINE, /* hotplug AP dynamic online CPU */
• CS_CPUPM_CPUHP_AP_DYN_OFFLINE, /* hotplug AP dynamic offline CPU */

+};

+/* callback function type */ +typedef int (*p_cs_cpu_pm_event)(struct coresight_device *csdev,

• 		 unsigned int cpuid,
  

• 		 enum cs_cpu_pm_events event);
  

+/* registration functions */ +int coresight_cpupm_register(struct coresight_device *csdev,

• 	     unsigned int cpuid,
  

• 	     p_cs_cpu_pm_event callback);
  

+void coresight_cpupm_unregister(struct coresight_device *csdev,

• 	       unsigned int cpuid);
  

#endif diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 74661fabe098..509fc0b8329d 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -1347,7 +1347,6 @@ void coresight_unregister(struct coresight_device *csdev) } EXPORT_SYMBOL_GPL(coresight_unregister);

/*

• coresight_search_device_idx - Search the fwnode handle of a device
• in the given dev_idx list. Must be called with the coresight_mutex held.

@@ -1404,3 +1403,169 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict, return name; } EXPORT_SYMBOL_GPL(coresight_alloc_device_name);

+/* cs general CPU bound device pm notification */ +struct coresight_cpupm_item {

• struct coresight_device *csdev;
• p_cs_cpu_pm_event callback;
• struct list_head node;

+};

+/* CS cpu pm notifier lists per cpu */ +static DEFINE_PER_CPU(struct list_head *, cs_cpupm_notifier);

+static enum cpuhp_state cscpu_hp_online; +static int cscpu_dev_reg_cnt;

I do not really see the advantage of adding all this code. To me it is like shadowing an infrastructure the CPUhotplug subsystem is alreading giving us. And looking at function cti_cs_cpupm_event() in the next patch the code to handle 3 events is grouped into a single function. To me we are not gaining anything other than replicating code that already exists.

To me it is much easier to have a percpu instance of the cti_drvdata (I think Suzuki already hinted at that in his review) and access the right one using the CPU parameter provided by the individual callbacks.

Let me know if the above is not handling a scenario that this patch is.

+static int coresight_iterate_cpu_event_list(unsigned int cpu,

• 			    enum cs_cpu_pm_events event)
  

+{

• struct list_head *cpu_list;
• struct coresight_cpupm_item *item;
• mutex_lock(&coresight_mutex);
• cpu_list = per_cpu(cs_cpupm_notifier, cpu);
• if (!list_empty(cpu_list)) {

• list_for_each_entry(item, cpu_list, node) {
  

• 	item->callback(item->csdev, cpu, event);
  

• }
  

• }
• mutex_unlock(&coresight_mutex);
• return 0;

+}

+static int coresight_hp_starting_cpu(unsigned int cpu) +{

• return coresight_iterate_cpu_event_list(cpu, CS_CPUPM_CPUHP_CS_START);

+}

+static int coresight_hp_stopping_cpu(unsigned int cpu) +{

• return coresight_iterate_cpu_event_list(cpu, CS_CPUPM_CPUHP_CS_STOP);

+}

+static int coresight_hp_dyn_online_cpu(unsigned int cpu) +{

• return coresight_iterate_cpu_event_list(cpu,

• 				CS_CPUPM_CPUHP_AP_DYN_ONLINE);
  

+}

+static int coresight_hp_dyn_offline_cpu(unsigned int cpu) +{

• return coresight_iterate_cpu_event_list(cpu,

• 				CS_CPUPM_CPUHP_AP_DYN_OFFLINE);
  

+}

+struct cs_smp_reg_cb_info {

• unsigned int cpu;
• struct coresight_cpupm_item *item;

+};

+static void coresight_smp_onreg_callback(void *info) +{

• struct cs_smp_reg_cb_info *cb_info = info;
• cb_info->item->callback(cb_info->item->csdev, cb_info->cpu,

• 		CS_CPUPM_SMP_ON_REG_CPU);
  

+}

+int coresight_cpupm_register(struct coresight_device *csdev,

• 	     unsigned int cpuid,
  

• 	     p_cs_cpu_pm_event callback)
  

+{

• struct list_head *cpu_list;
• struct coresight_cpupm_item *item;
• int err = 0;
• struct cs_smp_reg_cb_info on_reg_cb_info;
• mutex_lock(&coresight_mutex);
• cpu_list = per_cpu(cs_cpupm_notifier, cpuid);
• /* init list if no head yet */
• if (!cpu_list) {

• cpu_list = kzalloc(sizeof(struct list_head), GFP_KERNEL);
  

• if (!cpu_list)
  

• 	goto cpupm_reg_done;
  

• else
  

• 	INIT_LIST_HEAD(cpu_list);
  

• }
• /* create a list item */
• item = kzalloc(sizeof(struct coresight_cpupm_item), GFP_KERNEL);
• if (!item) {

• err = -ENOMEM;
  

• goto cpupm_reg_done;
  

• }
• item->csdev = csdev;
• item->callback = callback;
• /* init the cpu hotplug notification */
• cpus_read_lock();
• if (!cscpu_dev_reg_cnt) {

• cpuhp_setup_state_nocalls_cpuslocked(
  

• 	CPUHP_AP_ARM_CORESIGHT_STARTING,
  

• 	"arm/coresightcpu:starting",
  

• 	coresight_hp_starting_cpu,
  

• 	coresight_hp_stopping_cpu);
  

• cscpu_hp_online = cpuhp_setup_state_nocalls_cpuslocked(
  

• 	CPUHP_AP_ONLINE_DYN, "arm/coresightcpu:online",
  

• 	coresight_hp_dyn_online_cpu,
  

• 	coresight_hp_dyn_offline_cpu);
  

• if (cscpu_hp_online < 0) {
  

• 	err = cscpu_hp_online;
  

• 	cpuhp_remove_state_nocalls(
  

• 		CPUHP_AP_ARM_CORESIGHT_STARTING);
  

• 	kfree(item);
  

• 	cscpu_hp_online = 0;
  

• 	goto cpupm_hp_done;
  

• }
  

• }
• cscpu_dev_reg_cnt++;
• list_add(&item->node, cpu_list);
• /* smp callback on dev cpu @ registration */
• on_reg_cb_info.item = item;
• on_reg_cb_info.cpu = cpuid;
• smp_call_function_single(cpuid, coresight_smp_onreg_callback,

• 		 &on_reg_cb_info, 1);
  

+cpupm_hp_done:

• cpus_read_unlock();

+cpupm_reg_done:

• mutex_unlock(&coresight_mutex);
• return err;

+} +EXPORT_SYMBOL_GPL(coresight_cpupm_register);

+void coresight_cpupm_unregister(struct coresight_device *csdev,

• 		unsigned int cpuid)
  

+{

• struct list_head *cpu_list;
• struct coresight_cpupm_item *item, *tmp;
• mutex_lock(&coresight_mutex);
• cpu_list = per_cpu(cs_cpupm_notifier, cpuid);
• if (!list_empty(cpu_list)) {

• list_for_each_entry_safe(item, tmp, cpu_list, node) {
  

• 	if (item->csdev == csdev) {
  

• 		list_del(&item->node);
  

• 		cscpu_dev_reg_cnt--;
  

• 		kfree(item);
  

• 		goto cs_cpupm_item_removed;
  

• 	}
  

• }
  

• }

+cs_cpupm_item_removed:

• if (!cscpu_dev_reg_cnt) {

• cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
  

• if (cscpu_hp_online)
  

• 	cpuhp_remove_state_nocalls(cscpu_hp_online);
  

• }
• mutex_unlock(&coresight_mutex);

+}

+EXPORT_SYMBOL_GPL(coresight_cpupm_unregister);

2.20.1

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight