[PATCH V4 0/4] firmware: arm_scmi: Misc Fixes

[PATCH V4 0/4] firmware: arm_scmi: Misc Fixes

[Sibi Sankar]

The series addresses the kernel warnings reported by Johan at [1] and are
are required to X1E cpufreq device tree changes [2] to land.

[1] - https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
[2] - https://lore.kernel.org/lkml/20240612124056.39230-1-quic_sibis@quicinc.com/

The following warnings remain unadressed:
arm-scmi arm-scmi.0.auto: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
arm-scmi arm-scmi.0.auto: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16

They indicate that duplicate opps are reported by the SCP firmware and
they are seen during probe. They will get addressed by firmware updates. 

Duplicate levels:
arm-scmi arm-scmi.0.auto: Level 2976000 Power 218062 Latency 30us Ifreq 2976000 Index 10
arm-scmi arm-scmi.0.auto: Level 3206400 Power 264356 Latency 30us Ifreq 3206400 Index 11
arm-scmi arm-scmi.0.auto: Level 3417600 Power 314966 Latency 30us Ifreq 3417600 Index 12
arm-scmi arm-scmi.0.auto: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
arm-scmi arm-scmi.0.auto: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
arm-scmi arm-scmi.0.auto: Level 4012800 Power 528848 Latency 30us Ifreq 4012800 Index 15

^^ exist because SCP reports duplicate values for the highest sustainable
freq for perf domains 1 and 2. These are the only freqs that appear as
duplicates and will be fixed with a firmware update. FWIW the warnings
that we are addressing in this series will also get fixed by a firmware
update but they still have to land for devices already out in the wild.

V3:
* Pick up R-b, T-b from the list.
* Pick up the updated patch from Cristian for skipping opps.
* Update device names only when a name collision occurs [Dmitry/Ulf]
* Drop Johan's T-b from "fix debugfs node creation failure"
* Move scmi_protocol_msg_check to the top [Sudeep]

V2:
* Include the fix for do_xfer timeout
* Include the fix debugfs node creation failure
* Include Cristian's fix for skipping opp duplication

V1:
* add missing MSG_SUPPORTS_FASTCHANNEL definition.

Base branch: next-20241023

Cristian Marussi (1):
  firmware: arm_scmi: Skip opp duplicates

Sibi Sankar (3):
  firmware: arm_scmi: Ensure that the message-id supports fastchannel
  pmdomain: core: Fix debugfs node creation failure
  mailbox: qcom-cpucp: Mark the irq with IRQF_NO_SUSPEND flag

 drivers/firmware/arm_scmi/driver.c    | 72 +++++++++++++++------------
 drivers/firmware/arm_scmi/perf.c      | 40 +++++++++++----
 drivers/firmware/arm_scmi/protocols.h |  2 +
 drivers/mailbox/qcom-cpucp-mbox.c     |  2 +-
 drivers/pmdomain/core.c               | 65 ++++++++++++++++++------
 include/linux/pm_domain.h             |  1 +
 6 files changed, 123 insertions(+), 59 deletions(-)

-- 
2.34.1

[PATCH V4 1/4] firmware: arm_scmi: Ensure that the message-id supports fastchannel

[Sibi Sankar]

Currently the perf and powercap protocol relies on the protocol domain
attributes, which just ensures that one fastchannel per domain, before
instantiating fastchannels for all possible message-ids. Fix this by
ensuring that each message-id supports fastchannel before initialization.

Reported-by: Johan Hovold <johan+linaro@kernel.org>
Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support")
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Tested-by: Johan Hovold <johan+linaro@kernel.org>
---

v3:
* Pick up R-b, T-b from the list.
* Move scmi_protocol_msg_check to the top [Sudeep]

 drivers/firmware/arm_scmi/driver.c    | 72 +++++++++++++++------------
 drivers/firmware/arm_scmi/protocols.h |  2 +
 2 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 5bd4cc68a3e3..36195f177ade 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1705,6 +1705,39 @@ static int scmi_common_get_max_msg_size(const struct scmi_protocol_handle *ph)
 	return info->desc->max_msg_size;
 }
 
+/**
+ * scmi_protocol_msg_check  - Check protocol message attributes
+ *
+ * @ph: A reference to the protocol handle.
+ * @message_id: The ID of the message to check.
+ * @attributes: A parameter to optionally return the retrieved message
+ *		attributes, in case of Success.
+ *
+ * An helper to check protocol message attributes for a specific protocol
+ * and message pair.
+ *
+ * Return: 0 on SUCCESS
+ */
+static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,
+				   u32 message_id, u32 *attributes)
+{
+	int ret;
+	struct scmi_xfer *t;
+
+	ret = xfer_get_init(ph, PROTOCOL_MESSAGE_ATTRIBUTES,
+			    sizeof(__le32), 0, &t);
+	if (ret)
+		return ret;
+
+	put_unaligned_le32(message_id, t->tx.buf);
+	ret = do_xfer(ph, t);
+	if (!ret && attributes)
+		*attributes = get_unaligned_le32(t->rx.buf);
+	xfer_put(ph, t);
+
+	return ret;
+}
+
 /**
  * struct scmi_iterator  - Iterator descriptor
  * @msg: A reference to the message TX buffer; filled by @prepare_message with
@@ -1846,6 +1879,7 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
 	int ret;
 	u32 flags;
 	u64 phys_addr;
+	u32 attributes;
 	u8 size;
 	void __iomem *addr;
 	struct scmi_xfer *t;
@@ -1854,6 +1888,11 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
 	struct scmi_msg_resp_desc_fc *resp;
 	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
 
+	/* Check if the MSG_ID supports fastchannel */
+	ret = scmi_protocol_msg_check(ph, message_id, &attributes);
+	if (!ret && !MSG_SUPPORTS_FASTCHANNEL(attributes))
+		return;
+
 	if (!p_addr) {
 		ret = -EINVAL;
 		goto err_out;
@@ -1981,39 +2020,6 @@ static void scmi_common_fastchannel_db_ring(struct scmi_fc_db_info *db)
 #endif
 }
 
-/**
- * scmi_protocol_msg_check  - Check protocol message attributes
- *
- * @ph: A reference to the protocol handle.
- * @message_id: The ID of the message to check.
- * @attributes: A parameter to optionally return the retrieved message
- *		attributes, in case of Success.
- *
- * An helper to check protocol message attributes for a specific protocol
- * and message pair.
- *
- * Return: 0 on SUCCESS
- */
-static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,
-				   u32 message_id, u32 *attributes)
-{
-	int ret;
-	struct scmi_xfer *t;
-
-	ret = xfer_get_init(ph, PROTOCOL_MESSAGE_ATTRIBUTES,
-			    sizeof(__le32), 0, &t);
-	if (ret)
-		return ret;
-
-	put_unaligned_le32(message_id, t->tx.buf);
-	ret = do_xfer(ph, t);
-	if (!ret && attributes)
-		*attributes = get_unaligned_le32(t->rx.buf);
-	xfer_put(ph, t);
-
-	return ret;
-}
-
 static const struct scmi_proto_helpers_ops helpers_ops = {
 	.extended_name_get = scmi_common_extended_name_get,
 	.get_max_msg_size = scmi_common_get_max_msg_size,
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index aaee57cdcd55..d62c4469d1fd 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -31,6 +31,8 @@
 
 #define SCMI_PROTOCOL_VENDOR_BASE	0x80
 
+#define MSG_SUPPORTS_FASTCHANNEL(x)	((x) & BIT(0))
+
 enum scmi_common_cmd {
 	PROTOCOL_VERSION = 0x0,
 	PROTOCOL_ATTRIBUTES = 0x1,
-- 
2.34.1

Re: [PATCH V4 1/4] firmware: arm_scmi: Ensure that the message-id supports fastchannel

[Johan Hovold]

On Wed, Oct 23, 2024 at 03:51:45PM +0530, Sibi Sankar wrote:
> Currently the perf and powercap protocol relies on the protocol domain
> attributes, which just ensures that one fastchannel per domain, before
> instantiating fastchannels for all possible message-ids. Fix this by
> ensuring that each message-id supports fastchannel before initialization.

Again, perhaps you could include the error message I reported here so
that anyone searching for that error will find this fix more easily?
 
> Reported-by: Johan Hovold <johan+linaro@kernel.org>
> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
> Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support")

And, also again, should you add a CC-stable tag here to get this
backported?

> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> 
> v3:
> * Pick up R-b, T-b from the list.
> * Move scmi_protocol_msg_check to the top [Sudeep]

Johan

Re: [PATCH V4 1/4] firmware: arm_scmi: Ensure that the message-id supports fastchannel

[Sibi Sankar]

On 10/25/24 19:09, Johan Hovold wrote:
> On Wed, Oct 23, 2024 at 03:51:45PM +0530, Sibi Sankar wrote:
>> Currently the perf and powercap protocol relies on the protocol domain
>> attributes, which just ensures that one fastchannel per domain, before
>> instantiating fastchannels for all possible message-ids. Fix this by
>> ensuring that each message-id supports fastchannel before initialization.
> 
> Again, perhaps you could include the error message I reported here so
> that anyone searching for that error will find this fix more easily?

Ack

>   
>> Reported-by: Johan Hovold <johan+linaro@kernel.org>
>> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
>> Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support")
> 
> And, also again, should you add a CC-stable tag here to get this
> backported?

Ack

-Sibi

> 
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>> ---
>>
>> v3:
>> * Pick up R-b, T-b from the list.
>> * Move scmi_protocol_msg_check to the top [Sudeep]
> 
> Johan

[PATCH V4 2/4] firmware: arm_scmi: Skip opp duplicates

[Sibi Sankar]

From: Cristian Marussi <cristian.marussi@arm.com>

Buggy firmware can reply with duplicated PERF opps descriptors.

Ensure that the bad duplicates reported by the platform firmware doesn't
get added to the opp-tables.

Reported-by: Johan Hovold <johan+linaro@kernel.org>
Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Tested-by: Johan Hovold <johan+linaro@kernel.org>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---

v3:
* Pick up R-b, T-b from the list.
* Pick up the updated patch from Cristian for skipping opps.

 drivers/firmware/arm_scmi/perf.c | 40 ++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 2d77b5f40ca7..32f9a9acd3e9 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -373,7 +373,7 @@ static int iter_perf_levels_update_state(struct scmi_iterator_state *st,
 	return 0;
 }
 
-static inline void
+static inline int
 process_response_opp(struct device *dev, struct perf_dom_info *dom,
 		     struct scmi_opp *opp, unsigned int loop_idx,
 		     const struct scmi_msg_resp_perf_describe_levels *r)
@@ -386,12 +386,16 @@ process_response_opp(struct device *dev, struct perf_dom_info *dom,
 		le16_to_cpu(r->opp[loop_idx].transition_latency_us);
 
 	ret = xa_insert(&dom->opps_by_lvl, opp->perf, opp, GFP_KERNEL);
-	if (ret)
+	if (ret) {
 		dev_warn(dev, "Failed to add opps_by_lvl at %d for %s - ret:%d\n",
 			 opp->perf, dom->info.name, ret);
+		return ret;
+	}
+
+	return 0;
 }
 
-static inline void
+static inline int
 process_response_opp_v4(struct device *dev, struct perf_dom_info *dom,
 			struct scmi_opp *opp, unsigned int loop_idx,
 			const struct scmi_msg_resp_perf_describe_levels_v4 *r)
@@ -404,9 +408,11 @@ process_response_opp_v4(struct device *dev, struct perf_dom_info *dom,
 		le16_to_cpu(r->opp[loop_idx].transition_latency_us);
 
 	ret = xa_insert(&dom->opps_by_lvl, opp->perf, opp, GFP_KERNEL);
-	if (ret)
+	if (ret) {
 		dev_warn(dev, "Failed to add opps_by_lvl at %d for %s - ret:%d\n",
 			 opp->perf, dom->info.name, ret);
+		return ret;
+	}
 
 	/* Note that PERF v4 reports always five 32-bit words */
 	opp->indicative_freq = le32_to_cpu(r->opp[loop_idx].indicative_freq);
@@ -415,13 +421,21 @@ process_response_opp_v4(struct device *dev, struct perf_dom_info *dom,
 
 		ret = xa_insert(&dom->opps_by_idx, opp->level_index, opp,
 				GFP_KERNEL);
-		if (ret)
+		if (ret) {
 			dev_warn(dev,
 				 "Failed to add opps_by_idx at %d for %s - ret:%d\n",
 				 opp->level_index, dom->info.name, ret);
 
+			/* Cleanup by_lvl too */
+			xa_erase(&dom->opps_by_lvl, opp->perf);
+
+			return ret;
+		}
+
 		hash_add(dom->opps_by_freq, &opp->hash, opp->indicative_freq);
 	}
+
+	return 0;
 }
 
 static int
@@ -429,16 +443,22 @@ iter_perf_levels_process_response(const struct scmi_protocol_handle *ph,
 				  const void *response,
 				  struct scmi_iterator_state *st, void *priv)
 {
+	int ret;
 	struct scmi_opp *opp;
 	struct scmi_perf_ipriv *p = priv;
 
-	opp = &p->perf_dom->opp[st->desc_index + st->loop_idx];
+	opp = &p->perf_dom->opp[p->perf_dom->opp_count];
 	if (PROTOCOL_REV_MAJOR(p->version) <= 0x3)
-		process_response_opp(ph->dev, p->perf_dom, opp, st->loop_idx,
-				     response);
+		ret = process_response_opp(ph->dev, p->perf_dom, opp,
+					   st->loop_idx, response);
 	else
-		process_response_opp_v4(ph->dev, p->perf_dom, opp, st->loop_idx,
-					response);
+		ret = process_response_opp_v4(ph->dev, p->perf_dom, opp,
+					      st->loop_idx, response);
+
+	/* Skip BAD duplicates received from firmware */
+	if (ret)
+		return ret == -EBUSY ? 0 : ret;
+
 	p->perf_dom->opp_count++;
 
 	dev_dbg(ph->dev, "Level %d Power %d Latency %dus Ifreq %d Index %d\n",
-- 
2.34.1

[PATCH V4 3/4] pmdomain: core: Fix debugfs node creation failure

[Sibi Sankar]

The domain attributes returned by the perf protocol can end up
reporting identical names across domains, resulting in debugfs
node creation failure. Fix this failure by ensuring that pm domains
get a unique name using ida in pm_genpd_init.

Logs: [X1E reports 'NCC' for all its scmi perf domains]
debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
debugfs: Directory 'NCC' with parent 'pm_genpd' already present!

Reported-by: Johan Hovold <johan+linaro@kernel.org>
Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
Fixes: 718072ceb211 ("PM: domains: create debugfs nodes when adding power domains")
Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v3:
* Update device names only when a name collision occurs [Dmitry/Ulf]
* Drop Johan's T-b from "fix debugfs node creation failure"

 drivers/pmdomain/core.c   | 65 ++++++++++++++++++++++++++++++---------
 include/linux/pm_domain.h |  1 +
 2 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 76490f0bf1e2..756ed0975788 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -7,6 +7,7 @@
 #define pr_fmt(fmt) "PM: " fmt
 
 #include <linux/delay.h>
+#include <linux/idr.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>
@@ -23,6 +24,9 @@
 #include <linux/cpu.h>
 #include <linux/debugfs.h>
 
+/* Provides a unique ID for each genpd device */
+static DEFINE_IDA(genpd_ida);
+
 #define GENPD_RETRY_MAX_MS	250		/* Approximate */
 
 #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
@@ -189,7 +193,7 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
 
 	if (ret)
 		dev_warn_once(dev, "PM domain %s will not be powered off\n",
-				genpd->name);
+			      dev_name(&genpd->dev));
 
 	return ret;
 }
@@ -274,7 +278,7 @@ static void genpd_debug_remove(struct generic_pm_domain *genpd)
 	if (!genpd_debugfs_dir)
 		return;
 
-	debugfs_lookup_and_remove(genpd->name, genpd_debugfs_dir);
+	debugfs_lookup_and_remove(dev_name(&genpd->dev), genpd_debugfs_dir);
 }
 
 static void genpd_update_accounting(struct generic_pm_domain *genpd)
@@ -731,7 +735,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 	genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
 	genpd->gd->max_off_time_changed = true;
 	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
-		 genpd->name, "on", elapsed_ns);
+		 dev_name(&genpd->dev), "on", elapsed_ns);
 
 out:
 	raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL);
@@ -782,7 +786,7 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
 	genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
 	genpd->gd->max_off_time_changed = true;
 	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
-		 genpd->name, "off", elapsed_ns);
+		 dev_name(&genpd->dev), "off", elapsed_ns);
 
 out:
 	raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
@@ -1941,7 +1945,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb)
 
 	if (ret) {
 		dev_warn(dev, "failed to add notifier for PM domain %s\n",
-			 genpd->name);
+			 dev_name(&genpd->dev));
 		return ret;
 	}
 
@@ -1988,7 +1992,7 @@ int dev_pm_genpd_remove_notifier(struct device *dev)
 
 	if (ret) {
 		dev_warn(dev, "failed to remove notifier for PM domain %s\n",
-			 genpd->name);
+			 dev_name(&genpd->dev));
 		return ret;
 	}
 
@@ -2014,7 +2018,7 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
 	 */
 	if (!genpd_is_irq_safe(genpd) && genpd_is_irq_safe(subdomain)) {
 		WARN(1, "Parent %s of subdomain %s must be IRQ safe\n",
-				genpd->name, subdomain->name);
+		     dev_name(&genpd->dev), subdomain->name);
 		return -EINVAL;
 	}
 
@@ -2089,7 +2093,7 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 
 	if (!list_empty(&subdomain->parent_links) || subdomain->device_count) {
 		pr_warn("%s: unable to remove subdomain %s\n",
-			genpd->name, subdomain->name);
+			dev_name(&genpd->dev), subdomain->name);
 		ret = -EBUSY;
 		goto out;
 	}
@@ -2199,6 +2203,23 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
 	}
 }
 
+static bool genpd_name_present(const char *name)
+{
+	bool ret = false;
+	const struct generic_pm_domain *gpd;
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+		if (!strcmp(dev_name(&gpd->dev), name)) {
+			ret = true;
+			break;
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+
+	return ret;
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -2226,6 +2247,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
 	genpd->device_count = 0;
 	genpd->provider = NULL;
+	genpd->device_id = -ENXIO;
 	genpd->has_provider = false;
 	genpd->accounting_time = ktime_get_mono_fast_ns();
 	genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
@@ -2266,7 +2288,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 		return ret;
 
 	device_initialize(&genpd->dev);
-	dev_set_name(&genpd->dev, "%s", genpd->name);
+
+	if (!genpd_name_present(genpd->name)) {
+		dev_set_name(&genpd->dev, "%s", genpd->name);
+	} else {
+		ret = ida_alloc(&genpd_ida, GFP_KERNEL);
+		if (ret < 0) {
+			put_device(&genpd->dev);
+			return ret;
+		}
+		genpd->device_id = ret;
+		dev_set_name(&genpd->dev, "%s_%u", genpd->name, genpd->device_id);
+	}
 
 	mutex_lock(&gpd_list_lock);
 	list_add(&genpd->gpd_list_node, &gpd_list);
@@ -2288,13 +2321,13 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 
 	if (genpd->has_provider) {
 		genpd_unlock(genpd);
-		pr_err("Provider present, unable to remove %s\n", genpd->name);
+		pr_err("Provider present, unable to remove %s\n", dev_name(&genpd->dev));
 		return -EBUSY;
 	}
 
 	if (!list_empty(&genpd->parent_links) || genpd->device_count) {
 		genpd_unlock(genpd);
-		pr_err("%s: unable to remove %s\n", __func__, genpd->name);
+		pr_err("%s: unable to remove %s\n", __func__, dev_name(&genpd->dev));
 		return -EBUSY;
 	}
 
@@ -2308,9 +2341,11 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 	genpd_unlock(genpd);
 	genpd_debug_remove(genpd);
 	cancel_work_sync(&genpd->power_off_work);
+	if (genpd->device_id != -ENXIO)
+		ida_free(&genpd_ida, genpd->device_id);
 	genpd_free_data(genpd);
 
-	pr_debug("%s: removed %s\n", __func__, genpd->name);
+	pr_debug("%s: removed %s\n", __func__, dev_name(&genpd->dev));
 
 	return 0;
 }
@@ -3320,12 +3355,12 @@ static int genpd_summary_one(struct seq_file *s,
 	else
 		snprintf(state, sizeof(state), "%s",
 			 status_lookup[genpd->status]);
-	seq_printf(s, "%-30s  %-30s  %u", genpd->name, state, genpd->performance_state);
+	seq_printf(s, "%-30s  %-30s  %u", dev_name(&genpd->dev), state, genpd->performance_state);
 
 	/*
 	 * Modifications on the list require holding locks on both
 	 * parent and child, so we are safe.
-	 * Also genpd->name is immutable.
+	 * Also the device name is immutable.
 	 */
 	list_for_each_entry(link, &genpd->parent_links, parent_node) {
 		if (list_is_first(&link->parent_node, &genpd->parent_links))
@@ -3550,7 +3585,7 @@ static void genpd_debug_add(struct generic_pm_domain *genpd)
 	if (!genpd_debugfs_dir)
 		return;
 
-	d = debugfs_create_dir(genpd->name, genpd_debugfs_dir);
+	d = debugfs_create_dir(dev_name(&genpd->dev), genpd_debugfs_dir);
 
 	debugfs_create_file("current_state", 0444,
 			    d, genpd, &status_fops);
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 76775ab38898..1106b86de279 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -171,6 +171,7 @@ struct generic_pm_domain {
 	atomic_t sd_count;	/* Number of subdomains with power "on" */
 	enum gpd_status status;	/* Current state of the domain */
 	unsigned int device_count;	/* Number of devices */
+	unsigned int device_id;		/* unique device id */
 	unsigned int suspended_count;	/* System suspend device counter */
 	unsigned int prepared_count;	/* Suspend counter of prepared devices */
 	unsigned int performance_state;	/* Aggregated max performance state */
-- 
2.34.1

Re: [PATCH V4 3/4] pmdomain: core: Fix debugfs node creation failure

[Johan Hovold]

On Wed, Oct 23, 2024 at 03:51:47PM +0530, Sibi Sankar wrote:
> The domain attributes returned by the perf protocol can end up
> reporting identical names across domains, resulting in debugfs
> node creation failure. Fix this failure by ensuring that pm domains
> get a unique name using ida in pm_genpd_init.
> 
> Logs: [X1E reports 'NCC' for all its scmi perf domains]
> debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
> debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
> 
> Reported-by: Johan Hovold <johan+linaro@kernel.org>
> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
> Fixes: 718072ceb211 ("PM: domains: create debugfs nodes when adding power domains")
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v3:
> * Update device names only when a name collision occurs [Dmitry/Ulf]
> * Drop Johan's T-b from "fix debugfs node creation failure"

Also seems to do the trick:

Tested-by: Johan Hovold <johan+linaro@kernel.org>

But perhaps you could consider starting enumerating the duplicate
domains from 2 (or 1) instead of 0?:

NCC_1                           on                              0
NCC_0                           on                              0
NCC                             on                              0

Johan

Re: [PATCH V4 3/4] pmdomain: core: Fix debugfs node creation failure

[Sibi Sankar]

On 10/25/24 19:23, Johan Hovold wrote:
> On Wed, Oct 23, 2024 at 03:51:47PM +0530, Sibi Sankar wrote:
>> The domain attributes returned by the perf protocol can end up
>> reporting identical names across domains, resulting in debugfs
>> node creation failure. Fix this failure by ensuring that pm domains
>> get a unique name using ida in pm_genpd_init.
>>
>> Logs: [X1E reports 'NCC' for all its scmi perf domains]
>> debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
>> debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
>>
>> Reported-by: Johan Hovold <johan+linaro@kernel.org>
>> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
>> Fixes: 718072ceb211 ("PM: domains: create debugfs nodes when adding power domains")
>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> v3:
>> * Update device names only when a name collision occurs [Dmitry/Ulf]
>> * Drop Johan's T-b from "fix debugfs node creation failure"
> 
> Also seems to do the trick:
> 
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> 
> But perhaps you could consider starting enumerating the duplicate
> domains from 2 (or 1) instead of 0?:
> 
> NCC_1                           on                              0
> NCC_0                           on                              0
> NCC                             on                              0

We are just trying to make sure node names are unique and
can't ensure the pd-name correctness since ida starts its
number generation from 0 and I didn't want to shape the
fix just to cater to our specific case. The firmware fix
will be in charge of ensuring pd-name correctness.

-Sibi

> 
> Johan

Re: [PATCH V4 3/4] pmdomain: core: Fix debugfs node creation failure

[Johan Hovold]

On Fri, Oct 25, 2024 at 07:36:16PM +0530, Sibi Sankar wrote:
> On 10/25/24 19:23, Johan Hovold wrote:

> > Also seems to do the trick:
> > 
> > Tested-by: Johan Hovold <johan+linaro@kernel.org>
> > 
> > But perhaps you could consider starting enumerating the duplicate
> > domains from 2 (or 1) instead of 0?:
> > 
> > NCC_1                           on                              0
> > NCC_0                           on                              0
> > NCC                             on                              0
> 
> We are just trying to make sure node names are unique and
> can't ensure the pd-name correctness since ida starts its
> number generation from 0 and I didn't want to shape the
> fix just to cater to our specific case. The firmware fix
> will be in charge of ensuring pd-name correctness.

Ah, it's a global number space? I didn't really look at the
implementation...

Johan

Re: [PATCH V4 3/4] pmdomain: core: Fix debugfs node creation failure

[Sibi Sankar]

On 10/25/24 19:41, Johan Hovold wrote:
> On Fri, Oct 25, 2024 at 07:36:16PM +0530, Sibi Sankar wrote:
>> On 10/25/24 19:23, Johan Hovold wrote:
> 
>>> Also seems to do the trick:
>>>
>>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>>>
>>> But perhaps you could consider starting enumerating the duplicate
>>> domains from 2 (or 1) instead of 0?:
>>>
>>> NCC_1                           on                              0
>>> NCC_0                           on                              0
>>> NCC                             on                              0
>>
>> We are just trying to make sure node names are unique and
>> can't ensure the pd-name correctness since ida starts its
>> number generation from 0 and I didn't want to shape the
>> fix just to cater to our specific case. The firmware fix
>> will be in charge of ensuring pd-name correctness.
> 
> Ah, it's a global number space? I didn't really look at the
> implementation...

Thanks for testing out the patch. Yes it was a global number
space but we are changing the implementation again in the next
re-spin. Please try that out instead.

-Sibi

> 
> Johan

Re: [PATCH V4 3/4] pmdomain: core: Fix debugfs node creation failure

[Ulf Hansson]

On Wed, 23 Oct 2024 at 12:22, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>
> The domain attributes returned by the perf protocol can end up
> reporting identical names across domains, resulting in debugfs
> node creation failure. Fix this failure by ensuring that pm domains
> get a unique name using ida in pm_genpd_init.
>
> Logs: [X1E reports 'NCC' for all its scmi perf domains]
> debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
> debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
>
> Reported-by: Johan Hovold <johan+linaro@kernel.org>
> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
> Fixes: 718072ceb211 ("PM: domains: create debugfs nodes when adding power domains")
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
>
> v3:
> * Update device names only when a name collision occurs [Dmitry/Ulf]
> * Drop Johan's T-b from "fix debugfs node creation failure"
>
>  drivers/pmdomain/core.c   | 65 ++++++++++++++++++++++++++++++---------
>  include/linux/pm_domain.h |  1 +
>  2 files changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 76490f0bf1e2..756ed0975788 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -7,6 +7,7 @@
>  #define pr_fmt(fmt) "PM: " fmt
>
>  #include <linux/delay.h>
> +#include <linux/idr.h>
>  #include <linux/kernel.h>
>  #include <linux/io.h>
>  #include <linux/platform_device.h>
> @@ -23,6 +24,9 @@
>  #include <linux/cpu.h>
>  #include <linux/debugfs.h>
>
> +/* Provides a unique ID for each genpd device */
> +static DEFINE_IDA(genpd_ida);
> +
>  #define GENPD_RETRY_MAX_MS     250             /* Approximate */
>
>  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)         \
> @@ -189,7 +193,7 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
>
>         if (ret)
>                 dev_warn_once(dev, "PM domain %s will not be powered off\n",
> -                               genpd->name);
> +                             dev_name(&genpd->dev));
>
>         return ret;
>  }
> @@ -274,7 +278,7 @@ static void genpd_debug_remove(struct generic_pm_domain *genpd)
>         if (!genpd_debugfs_dir)
>                 return;
>
> -       debugfs_lookup_and_remove(genpd->name, genpd_debugfs_dir);
> +       debugfs_lookup_and_remove(dev_name(&genpd->dev), genpd_debugfs_dir);
>  }
>
>  static void genpd_update_accounting(struct generic_pm_domain *genpd)
> @@ -731,7 +735,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>         genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
>         genpd->gd->max_off_time_changed = true;
>         pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
> -                genpd->name, "on", elapsed_ns);
> +                dev_name(&genpd->dev), "on", elapsed_ns);
>
>  out:
>         raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL);
> @@ -782,7 +786,7 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
>         genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
>         genpd->gd->max_off_time_changed = true;
>         pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
> -                genpd->name, "off", elapsed_ns);
> +                dev_name(&genpd->dev), "off", elapsed_ns);
>
>  out:
>         raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
> @@ -1941,7 +1945,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb)
>
>         if (ret) {
>                 dev_warn(dev, "failed to add notifier for PM domain %s\n",
> -                        genpd->name);
> +                        dev_name(&genpd->dev));
>                 return ret;
>         }
>
> @@ -1988,7 +1992,7 @@ int dev_pm_genpd_remove_notifier(struct device *dev)
>
>         if (ret) {
>                 dev_warn(dev, "failed to remove notifier for PM domain %s\n",
> -                        genpd->name);
> +                        dev_name(&genpd->dev));
>                 return ret;
>         }
>
> @@ -2014,7 +2018,7 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>          */
>         if (!genpd_is_irq_safe(genpd) && genpd_is_irq_safe(subdomain)) {
>                 WARN(1, "Parent %s of subdomain %s must be IRQ safe\n",
> -                               genpd->name, subdomain->name);
> +                    dev_name(&genpd->dev), subdomain->name);
>                 return -EINVAL;
>         }
>
> @@ -2089,7 +2093,7 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>
>         if (!list_empty(&subdomain->parent_links) || subdomain->device_count) {
>                 pr_warn("%s: unable to remove subdomain %s\n",
> -                       genpd->name, subdomain->name);
> +                       dev_name(&genpd->dev), subdomain->name);
>                 ret = -EBUSY;
>                 goto out;
>         }
> @@ -2199,6 +2203,23 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
>         }
>  }
>
> +static bool genpd_name_present(const char *name)
> +{
> +       bool ret = false;
> +       const struct generic_pm_domain *gpd;
> +
> +       mutex_lock(&gpd_list_lock);
> +       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> +               if (!strcmp(dev_name(&gpd->dev), name)) {
> +                       ret = true;
> +                       break;
> +               }
> +       }
> +       mutex_unlock(&gpd_list_lock);
> +
> +       return ret;
> +}
> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -2226,6 +2247,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>         genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
>         genpd->device_count = 0;
>         genpd->provider = NULL;
> +       genpd->device_id = -ENXIO;
>         genpd->has_provider = false;
>         genpd->accounting_time = ktime_get_mono_fast_ns();
>         genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
> @@ -2266,7 +2288,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>                 return ret;
>
>         device_initialize(&genpd->dev);
> -       dev_set_name(&genpd->dev, "%s", genpd->name);
> +
> +       if (!genpd_name_present(genpd->name)) {
> +               dev_set_name(&genpd->dev, "%s", genpd->name);
> +       } else {
> +               ret = ida_alloc(&genpd_ida, GFP_KERNEL);
> +               if (ret < 0) {
> +                       put_device(&genpd->dev);
> +                       return ret;
> +               }
> +               genpd->device_id = ret;
> +               dev_set_name(&genpd->dev, "%s_%u", genpd->name, genpd->device_id);
> +       }

If we can't assume that the genpd->name is unique, I think we need to
hold the gpd_list_lock over this entire new section, until we have
added the new genpd in the gpd_list. I am not sure we really want this
as it could hurt (theoretically at least) boot/probing on systems
where a lot of genpds are being used.

That said, I would suggest we go for Dmitry's suggestion to make this
genpd provider specific. Let's add a new genpd flag that genpd
providers can set, if they need an ida to be tagged on to their
device-name. Then we should set that flag for SCMI perf/power domains.

[...]

Kind regards
Uffe

Re: [PATCH V4 3/4] pmdomain: core: Fix debugfs node creation failure

[Sibi Sankar]

On 10/28/24 18:58, Ulf Hansson wrote:
> On Wed, 23 Oct 2024 at 12:22, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>>
>> The domain attributes returned by the perf protocol can end up
>> reporting identical names across domains, resulting in debugfs
>> node creation failure. Fix this failure by ensuring that pm domains
>> get a unique name using ida in pm_genpd_init.
>>
>> Logs: [X1E reports 'NCC' for all its scmi perf domains]
>> debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
>> debugfs: Directory 'NCC' with parent 'pm_genpd' already present!
>>
>> Reported-by: Johan Hovold <johan+linaro@kernel.org>
>> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
>> Fixes: 718072ceb211 ("PM: domains: create debugfs nodes when adding power domains")
>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> v3:
>> * Update device names only when a name collision occurs [Dmitry/Ulf]
>> * Drop Johan's T-b from "fix debugfs node creation failure"
>>
>>   drivers/pmdomain/core.c   | 65 ++++++++++++++++++++++++++++++---------
>>   include/linux/pm_domain.h |  1 +
>>   2 files changed, 51 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
>> index 76490f0bf1e2..756ed0975788 100644
>> --- a/drivers/pmdomain/core.c
>> +++ b/drivers/pmdomain/core.c
>> @@ -7,6 +7,7 @@
>>   #define pr_fmt(fmt) "PM: " fmt
>>
>>   #include <linux/delay.h>
>> +#include <linux/idr.h>
>>   #include <linux/kernel.h>
>>   #include <linux/io.h>
>>   #include <linux/platform_device.h>
>> @@ -23,6 +24,9 @@
>>   #include <linux/cpu.h>
>>   #include <linux/debugfs.h>
>>
>> +/* Provides a unique ID for each genpd device */
>> +static DEFINE_IDA(genpd_ida);
>> +
>>   #define GENPD_RETRY_MAX_MS     250             /* Approximate */
>>
>>   #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)         \
>> @@ -189,7 +193,7 @@ static inline bool irq_safe_dev_in_sleep_domain(struct device *dev,
>>
>>          if (ret)
>>                  dev_warn_once(dev, "PM domain %s will not be powered off\n",
>> -                               genpd->name);
>> +                             dev_name(&genpd->dev));
>>
>>          return ret;
>>   }
>> @@ -274,7 +278,7 @@ static void genpd_debug_remove(struct generic_pm_domain *genpd)
>>          if (!genpd_debugfs_dir)
>>                  return;
>>
>> -       debugfs_lookup_and_remove(genpd->name, genpd_debugfs_dir);
>> +       debugfs_lookup_and_remove(dev_name(&genpd->dev), genpd_debugfs_dir);
>>   }
>>
>>   static void genpd_update_accounting(struct generic_pm_domain *genpd)
>> @@ -731,7 +735,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>>          genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
>>          genpd->gd->max_off_time_changed = true;
>>          pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
>> -                genpd->name, "on", elapsed_ns);
>> +                dev_name(&genpd->dev), "on", elapsed_ns);
>>
>>   out:
>>          raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL);
>> @@ -782,7 +786,7 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
>>          genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
>>          genpd->gd->max_off_time_changed = true;
>>          pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
>> -                genpd->name, "off", elapsed_ns);
>> +                dev_name(&genpd->dev), "off", elapsed_ns);
>>
>>   out:
>>          raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
>> @@ -1941,7 +1945,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb)
>>
>>          if (ret) {
>>                  dev_warn(dev, "failed to add notifier for PM domain %s\n",
>> -                        genpd->name);
>> +                        dev_name(&genpd->dev));
>>                  return ret;
>>          }
>>
>> @@ -1988,7 +1992,7 @@ int dev_pm_genpd_remove_notifier(struct device *dev)
>>
>>          if (ret) {
>>                  dev_warn(dev, "failed to remove notifier for PM domain %s\n",
>> -                        genpd->name);
>> +                        dev_name(&genpd->dev));
>>                  return ret;
>>          }
>>
>> @@ -2014,7 +2018,7 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>>           */
>>          if (!genpd_is_irq_safe(genpd) && genpd_is_irq_safe(subdomain)) {
>>                  WARN(1, "Parent %s of subdomain %s must be IRQ safe\n",
>> -                               genpd->name, subdomain->name);
>> +                    dev_name(&genpd->dev), subdomain->name);
>>                  return -EINVAL;
>>          }
>>
>> @@ -2089,7 +2093,7 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>>
>>          if (!list_empty(&subdomain->parent_links) || subdomain->device_count) {
>>                  pr_warn("%s: unable to remove subdomain %s\n",
>> -                       genpd->name, subdomain->name);
>> +                       dev_name(&genpd->dev), subdomain->name);
>>                  ret = -EBUSY;
>>                  goto out;
>>          }
>> @@ -2199,6 +2203,23 @@ static void genpd_lock_init(struct generic_pm_domain *genpd)
>>          }
>>   }
>>
>> +static bool genpd_name_present(const char *name)
>> +{
>> +       bool ret = false;
>> +       const struct generic_pm_domain *gpd;
>> +
>> +       mutex_lock(&gpd_list_lock);
>> +       list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
>> +               if (!strcmp(dev_name(&gpd->dev), name)) {
>> +                       ret = true;
>> +                       break;
>> +               }
>> +       }
>> +       mutex_unlock(&gpd_list_lock);
>> +
>> +       return ret;
>> +}
>> +
>>   /**
>>    * pm_genpd_init - Initialize a generic I/O PM domain object.
>>    * @genpd: PM domain object to initialize.
>> @@ -2226,6 +2247,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>>          genpd->status = is_off ? GENPD_STATE_OFF : GENPD_STATE_ON;
>>          genpd->device_count = 0;
>>          genpd->provider = NULL;
>> +       genpd->device_id = -ENXIO;
>>          genpd->has_provider = false;
>>          genpd->accounting_time = ktime_get_mono_fast_ns();
>>          genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
>> @@ -2266,7 +2288,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>>                  return ret;
>>
>>          device_initialize(&genpd->dev);
>> -       dev_set_name(&genpd->dev, "%s", genpd->name);
>> +
>> +       if (!genpd_name_present(genpd->name)) {
>> +               dev_set_name(&genpd->dev, "%s", genpd->name);
>> +       } else {
>> +               ret = ida_alloc(&genpd_ida, GFP_KERNEL);
>> +               if (ret < 0) {
>> +                       put_device(&genpd->dev);
>> +                       return ret;
>> +               }
>> +               genpd->device_id = ret;
>> +               dev_set_name(&genpd->dev, "%s_%u", genpd->name, genpd->device_id);
>> +       }
> 
> If we can't assume that the genpd->name is unique, I think we need to
> hold the gpd_list_lock over this entire new section, until we have
> added the new genpd in the gpd_list. I am not sure we really want this
> as it could hurt (theoretically at least) boot/probing on systems
> where a lot of genpds are being used.
> 
> That said, I would suggest we go for Dmitry's suggestion to make this
> genpd provider specific. Let's add a new genpd flag that genpd
> providers can set, if they need an ida to be tagged on to their
> device-name. Then we should set that flag for SCMI perf/power domains.

lol, will do ^^ in the next re-spin.

-Sibi

> 
> [...]
> 
> Kind regards
> Uffe

[PATCH V4 4/4] mailbox: qcom-cpucp: Mark the irq with IRQF_NO_SUSPEND flag

[Sibi Sankar]

The qcom-cpucp mailbox irq is expected to function during suspend-resume
cycle particularly when the scmi cpufreq driver can query the current
frequency using the get_level message after the cpus are brought up during
resume. Hence mark the irq with IRQF_NO_SUSPEND flag to fix the do_xfer
failures we see during resume.

Err Logs:
arm-scmi firmware:scmi: timed out in resp(caller:do_xfer+0x164/0x568)
cpufreq: cpufreq_online: ->get() failed

Reported-by: Johan Hovold <johan+linaro@kernel.org>
Closes: https://lore.kernel.org/lkml/ZtgFj1y5ggipgEOS@hovoldconsulting.com/
Fixes: 0e2a9a03106c ("mailbox: Add support for QTI CPUCP mailbox controller")
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Tested-by: Johan Hovold <johan+linaro@kernel.org>
---

V3:
* Pick up R-b, T-b from the list.

 drivers/mailbox/qcom-cpucp-mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
index e5437c294803..44f4ed15f818 100644
--- a/drivers/mailbox/qcom-cpucp-mbox.c
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -138,7 +138,7 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
 		return irq;
 
 	ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
-			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
+			       IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND, "apss_cpucp_mbox", cpucp);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
 
-- 
2.34.1

Re: [PATCH V4 0/4] firmware: arm_scmi: Misc Fixes

[Johan Hovold]

On Wed, Oct 23, 2024 at 03:51:44PM +0530, Sibi Sankar wrote:
> The series addresses the kernel warnings reported by Johan at [1] and are
> are required to X1E cpufreq device tree changes [2] to land.
> 
> [1] - https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
> [2] - https://lore.kernel.org/lkml/20240612124056.39230-1-quic_sibis@quicinc.com/
> 
> The following warnings remain unadressed:
> arm-scmi arm-scmi.0.auto: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> arm-scmi arm-scmi.0.auto: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> 
> They indicate that duplicate opps are reported by the SCP firmware and
> they are seen during probe. They will get addressed by firmware updates. 
> 
> Duplicate levels:
> arm-scmi arm-scmi.0.auto: Level 2976000 Power 218062 Latency 30us Ifreq 2976000 Index 10
> arm-scmi arm-scmi.0.auto: Level 3206400 Power 264356 Latency 30us Ifreq 3206400 Index 11
> arm-scmi arm-scmi.0.auto: Level 3417600 Power 314966 Latency 30us Ifreq 3417600 Index 12
> arm-scmi arm-scmi.0.auto: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> arm-scmi arm-scmi.0.auto: Failed to add opps_by_lvl at 3417600 for NCC - ret:-16
> arm-scmi arm-scmi.0.auto: Level 4012800 Power 528848 Latency 30us Ifreq 4012800 Index 15
> 
> ^^ exist because SCP reports duplicate values for the highest sustainable
> freq for perf domains 1 and 2. These are the only freqs that appear as
> duplicates and will be fixed with a firmware update. FWIW the warnings
> that we are addressing in this series will also get fixed by a firmware
> update but they still have to land for devices already out in the wild.

Thanks for sorting this out, Sibi!

I guess the above description is clear enough too as to why the
opps_by_lvl warnings should stay (in some form).

Johan