[PATCH 0/2] platform/x86/intel-uncore-freq: Fix error handling

[PATCH 0/2] platform/x86/intel-uncore-freq: Fix error handling

[Srinivas Pandruvada]

While reviewing the driver code with AI models, identified a few corner
cases during the device probe and removal paths where error handling and
resource cleanup were insufficient or improperly ordered.

Srinivas Pandruvada (2):
  platform/x86/intel-uncore-freq: Don't remove entries which are not
    added
  platform/x86/intel-uncore-freq: Fix deadlock when removing attributes

 .../uncore-frequency/uncore-frequency-common.c | 18 +++++++++++++++---
 .../uncore-frequency/uncore-frequency-tpmi.c   |  3 ++-
 2 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.55.0

[PATCH 1/2] platform/x86/intel-uncore-freq: Don't remove entries which are not added

[Srinivas Pandruvada]

When uncore_probe() fails, remove_cluster_entries() removes the entries of
all clusters of every mapped power domain. But not all of them are added:

- When the allocation of cluster_infos fails, cluster_count is already
  set. So remove_cluster_entries() dereferences the NULL cluster_infos.

- When uncore_freq_add_entry() fails for a cluster, this cluster and all
  later clusters were not added. uncore_freq_remove_die_entry() still
  removes their attribute group, which doesn't exist. As domain_id of
  these clusters is not UNCORE_DOMAIN_ID_INVALID, it also frees their
  seqnum_id with ida_free(). The seqnum_id is 0 from allocation, which
  can be in use by an entry of another device.

Skip removing an entry in uncore_freq_remove_die_entry(), when it is not
valid, which means that it was not added or is already removed. Also
skip power domains without cluster_infos in remove_cluster_entries().

Fixes: 01c10f88c9b7 ("platform/x86/intel-uncore-freq: tpmi: Provide cluster level control")
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel/uncore-frequency/uncore-frequency-common.c     | 5 +++++
 .../x86/intel/uncore-frequency/uncore-frequency-tpmi.c       | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
index 4bcafedf68ef..6051aa040abf 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
@@ -312,12 +312,17 @@ EXPORT_SYMBOL_NS_GPL(uncore_freq_add_entry, "INTEL_UNCORE_FREQUENCY");
 void uncore_freq_remove_die_entry(struct uncore_data *data)
 {
 	mutex_lock(&uncore_lock);
+	/* Nothing to remove, if the entry was not added or already removed */
+	if (!data->valid)
+		goto uncore_unlock;
+
 	delete_attr_group(data, data->name);
 	data->control_cpu = -1;
 	data->valid = false;
 	if (data->domain_id != UNCORE_DOMAIN_ID_INVALID)
 		ida_free(&intel_uncore_ida, data->seqnum_id);
 
+uncore_unlock:
 	mutex_unlock(&uncore_lock);
 }
 EXPORT_SYMBOL_NS_GPL(uncore_freq_remove_die_entry, "INTEL_UNCORE_FREQUENCY");
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index 753ddfe8c759..73e0b0a94eaf 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -524,7 +524,8 @@ static void remove_cluster_entries(struct tpmi_uncore_struct *tpmi_uncore)
 		int j;
 
 		pd_info = &tpmi_uncore->pd_info[i];
-		if (!pd_info->uncore_base)
+		/* Probe may fail before cluster_infos is allocated */
+		if (!pd_info->uncore_base || !pd_info->cluster_infos)
 			continue;
 
 		for (j = 0; j < pd_info->cluster_count; ++j) {
-- 
2.55.0

[PATCH 2/2] platform/x86/intel-uncore-freq: Fix deadlock when removing attributes

[Srinivas Pandruvada]

uncore_freq_remove_die_entry() holds uncore_lock while removing the
attribute group of an entry. Removing attributes waits for active sysfs
callbacks of these attributes to finish. But the show and store callbacks
take uncore_lock, which results in an ABBA deadlock:

  CPU0                              CPU1
  ----                              ----
  kernfs_fop_read_iter()
    show_attr()                     uncore_freq_remove_die_entry()
                                      mutex_lock(&uncore_lock)
      mutex_lock(&uncore_lock)        delete_attr_group()
                                        sysfs_remove_group()
                                          waits for CPU0

Mark the entry as not valid with an invalid control CPU under
uncore_lock, and remove the attribute group after releasing it. Free
the seqnum_id after the attribute group is removed, so that the name
can't be used by a new entry while the old attributes still exist.

Adding and removing the same entry is serialized by the callers: CPU
hotplug callbacks in the MSR based driver, and device probe and remove
in the TPMI driver. Callbacks running before the attributes are removed
are safe: the MSR based driver returns -ENXIO for an invalid control CPU,
and the TPMI driver MMIO is unmapped only after the device is removed.

Fixes: ae7b2ce57851 ("platform/x86/intel/uncore-freq: Use sysfs API to create attributes")
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../uncore-frequency-common.c                 | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
index 6051aa040abf..22019099a857 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
@@ -313,17 +313,24 @@ void uncore_freq_remove_die_entry(struct uncore_data *data)
 {
 	mutex_lock(&uncore_lock);
 	/* Nothing to remove, if the entry was not added or already removed */
-	if (!data->valid)
-		goto uncore_unlock;
+	if (!data->valid) {
+		mutex_unlock(&uncore_lock);
+		return;
+	}
 
-	delete_attr_group(data, data->name);
 	data->control_cpu = -1;
 	data->valid = false;
+	mutex_unlock(&uncore_lock);
+
+	/*
+	 * Removing the attributes waits for active sysfs callbacks, which take
+	 * uncore_lock. So this can't be done while holding uncore_lock.
+	 */
+	delete_attr_group(data, data->name);
+
+	/* Free the ID after the attributes with the name using it are removed */
 	if (data->domain_id != UNCORE_DOMAIN_ID_INVALID)
 		ida_free(&intel_uncore_ida, data->seqnum_id);
-
-uncore_unlock:
-	mutex_unlock(&uncore_lock);
 }
 EXPORT_SYMBOL_NS_GPL(uncore_freq_remove_die_entry, "INTEL_UNCORE_FREQUENCY");
 
-- 
2.55.0