diff mbox series

[v5,07/17] soc: qcom: minidump: Add pending region registration

Message ID 1694290578-17733-8-git-send-email-quic_mojha@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add Qualcomm Minidump kernel driver related support | expand

Commit Message

Mukesh Ojha Sept. 9, 2023, 8:16 p.m. UTC
Pending regions are those apss minidump regions which came for
registration before minidump was initialized or ready to do
register the region.

We can add regions to pending region list and register all of
them from apss minidump driver probe in one go.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/soc/qcom/qcom_minidump.c | 140 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 130 insertions(+), 10 deletions(-)

Comments

Krzysztof Kozlowski Sept. 11, 2023, 11:08 a.m. UTC | #1
On 09/09/2023 22:16, Mukesh Ojha wrote:
>  static int qcom_apss_minidump_probe(struct platform_device *pdev)
>  {
>  	struct minidump_global_toc *mdgtoc;
> @@ -571,7 +688,10 @@ static int qcom_apss_minidump_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	mutex_lock(&md_plist.plock);
>  	platform_set_drvdata(pdev, md);

Why this is locked?

> +	qcom_apss_register_pending_regions(md);

Why this one is locked? It seems ordering of your operations is not
correct if you need to lock the providers probe().

> +	mutex_unlock(&md_plist.plock);


Best regards,
Krzysztof
Jeff Johnson Sept. 11, 2023, 6:59 p.m. UTC | #2
On 9/9/2023 1:16 PM, Mukesh Ojha wrote:
> +/**
> + * struct minidump_pregion - Minidump pending region
> + * @list       : Pending region list pointer
> + * @region     : APSS minidump client region

does kernel-doc parse this correctly? should not be whitespace between 
@ID and ":"

refer to 
<https://static.lwn.net/kerneldoc/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation>
diff mbox series

Patch

diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
index 86f4d09a7b4e..4ce36f154e89 100644
--- a/drivers/soc/qcom/qcom_minidump.c
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -13,6 +13,7 @@ 
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -63,6 +64,33 @@  struct minidump {
 	struct mutex			md_lock;
 };
 
+/**
+ * struct minidump_pregion - Minidump pending region
+ * @list       : Pending region list pointer
+ * @region     : APSS minidump client region
+ */
+struct minidump_pregion {
+	struct list_head	     list;
+	struct qcom_minidump_region  region;
+};
+
+/**
+ * struct minidump_plist - Minidump pending region list
+ * @plist	: List of pending region to be registered
+ * @pregion_cnt	: Count of the pending region to be registered
+ */
+struct minidump_plist {
+	struct list_head  plist;
+	int 		  pregion_cnt;
+	struct mutex	  plock;
+};
+
+static struct minidump_plist md_plist = {
+	.plist = LIST_HEAD_INIT(md_plist.plist),
+	.pregion_cnt = 0,
+	.plock = __MUTEX_INITIALIZER(md_plist.plock),
+};
+
 /*
  * In some of the Old Qualcomm devices, boot firmware statically allocates 300
  * as total number of supported region (including all co-processors) in
@@ -336,6 +364,26 @@  static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region
 		IS_ALIGNED(region->size, 4);
 }
 
+static struct minidump_pregion *
+check_region_in_plist(const struct qcom_minidump_region *region)
+{
+	struct minidump_pregion *md_pregion;
+	struct minidump_pregion *tmp;
+	bool found = false;
+
+	list_for_each_entry_safe(md_pregion, tmp, &md_plist.plist, list) {
+		struct qcom_minidump_region *md_region;
+
+		md_region = &md_pregion->region;
+		if (!strcmp(md_region->name, region->name)) {
+			found = true;
+			break;
+		}
+	}
+
+	return found ? md_pregion : NULL;
+}
+
 /**
  * qcom_minidump_region_register() - Register region in APSS Minidump table.
  * @region: minidump region.
@@ -344,16 +392,44 @@  static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region
  */
 int qcom_minidump_region_register(const struct qcom_minidump_region *region)
 {
+	struct minidump_pregion *md_pregion;
 	struct minidump *md;
-	int ret;
-
-	md = qcom_smem_minidump_ready();
-	if (!md)
-		return -EPROBE_DEFER;
+	int ret = 0;
 
 	if (!qcom_minidump_valid_region(region))
 		return -EINVAL;
 
+	mutex_lock(&md_plist.plock);
+	md = qcom_smem_minidump_ready();
+	if (!md) {
+		if (md_plist.pregion_cnt >= MAX_NUM_ENTRIES - 1) {
+			pr_err("maximum region limit %u reached\n", md_plist.pregion_cnt);
+			ret = -ENOSPC;
+			goto unlock_plock;
+		}
+
+		md_pregion = check_region_in_plist(region);
+		if (md_pregion) {
+			pr_info("%s region is already exist\n", region->name);
+			ret = -EEXIST;
+			goto unlock_plock;
+		}
+		/*
+		 * Maintain a list of client regions which came before
+		 * minidump driver was ready and once it is ready,
+		 * register them in one go from minidump probe function.
+		 */
+		md_pregion = kzalloc(sizeof(*md_pregion), GFP_KERNEL);
+		if (!md_pregion) {
+			ret = -ENOMEM;
+			goto unlock_plock;
+		}
+		md_pregion->region = *region;
+		list_add_tail(&md_pregion->list, &md_plist.plist);
+		md_plist.pregion_cnt++;
+		goto unlock_plock;
+	}
+
 	mutex_lock(&md->md_lock);
 	ret = qcom_md_region_register(md, region);
 	if (ret)
@@ -362,6 +438,10 @@  int qcom_minidump_region_register(const struct qcom_minidump_region *region)
 	qcom_md_update_elfheader(md, region);
 unlock:
 	mutex_unlock(&md->md_lock);
+	return 0;
+
+unlock_plock:
+	mutex_unlock(&md_plist.plock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
@@ -374,16 +454,28 @@  EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
  */
 int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
 {
+	struct minidump_pregion *md_pregion;
 	struct minidump *md;
-	int ret;
-
-	md = qcom_smem_minidump_ready();
-	if (!md)
-		return -EPROBE_DEFER;
+	int ret = 0;
 
 	if (!qcom_minidump_valid_region(region))
 		return -EINVAL;
 
+	mutex_lock(&md_plist.plock);
+	md = qcom_smem_minidump_ready();
+	if (!md) {
+		md_pregion = check_region_in_plist(region);
+		if (!md_pregion) {
+			ret = -ENOENT;
+			goto unlock_plock;
+		}
+
+		list_del(&md_pregion->list);
+		kfree(md_pregion);
+		md_plist.pregion_cnt--;
+		goto unlock_plock;
+	}
+
 	mutex_lock(&md->md_lock);
 	ret = qcom_md_region_unregister(md, region);
 	if (ret)
@@ -393,6 +485,10 @@  int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
 unlock:
 	mutex_unlock(&md->md_lock);
 	return ret;
+
+unlock_plock:
+	mutex_unlock(&md_plist.plock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(qcom_minidump_region_unregister);
 
@@ -532,6 +628,27 @@  static int qcom_apss_md_table_init(struct minidump *md,
 	return 0;
 }
 
+void qcom_apss_register_pending_regions(struct minidump *md)
+{
+	struct minidump_ss_data *mdss_data = md->apss_data;
+	struct minidump_pregion *md_pregion;
+	struct minidump_pregion *tmp;
+
+	list_for_each_entry_safe(md_pregion, tmp, &md_plist.plist, list) {
+		struct qcom_minidump_region *region;
+
+		region = &md_pregion->region;
+		mutex_lock(&md->md_lock);
+		qcom_md_add_region(mdss_data, region);
+		qcom_md_update_elfheader(md, region);
+		mutex_unlock(&md->md_lock);
+
+		list_del(&md_pregion->list);
+		kfree(md_pregion);
+		md_plist.pregion_cnt--;
+	}
+}
+
 static int qcom_apss_minidump_probe(struct platform_device *pdev)
 {
 	struct minidump_global_toc *mdgtoc;
@@ -571,7 +688,10 @@  static int qcom_apss_minidump_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	mutex_lock(&md_plist.plock);
 	platform_set_drvdata(pdev, md);
+	qcom_apss_register_pending_regions(md);
+	mutex_unlock(&md_plist.plock);
 
 	return ret;
 }