[PATCH v4 00/15] Refactor SDEI client driver

[PATCH v4 00/15] Refactor SDEI client driver

[Gavin Shan]

This series bases on 5.8-rc7 and refactors the SDEI client driver.
It's the preparatory work of virtualizing SDEI afterwords. The series
is organized as below:

   * PATCH[1-13] refactors and clean up the code. They shouldn't cause
     any functional changes.
   * PATCH[14-15] exposes "struct sdei_event", which will be dereferenced
     on virtualizing SDEI. After it's applied, the SDEI event is identified
     by the instance of "struct sdei_event" instead of event number.

The series can be checked out from:

    git@github.com:gwshan/linux.git
    (branch: "sdei_client")

Testing
=======
I have the SDEI virtualization code implemented as part of KVM module.
With that, the SDEI event can be registered/unregistered/enabled/disabled.
Also, the SDEI event can be injected from host and the guest handler runs
properly.

The code can be found from:

   git@github.com:gwshan/linux.git
   (branch: "sdei")

Changelog
=========
v4:
   Rebase to last upstream kernel                                  (Gavin)
   Use @event_el for SDEI internal event and use @event to cache
   SDEI event if needed                                            (Jonathan)
   Rename @se to @event for APIs                                   (Jonathan)
v3:
   Rebase to 5.8.rc7                                               (Gavin)
   Pick rbs from Jonathan                                          (Gavin)
   Correct spellings in commit logs                                (Jonathan)
   Rename "out" to "unlock" tag                                    (Jonathan)
   Keep the empty line in sdei_event_unregister()                  (Jonathan)
   Drop tabs between type and field for struct sdei_crosscall_args (Jonathan)
   Use smp_call_func_t for @fn argument in CPU callbacks           (Jonathan)
   Split struct sdei_event into struct sdei_{internal,}_event      (Jonathan)
   Remove last two patches and get it reviewed later               (Jonathan)
v2:
   Rebase to 5.8.rc6                                               (Gavin)
   Improved changelog                                              (James/Gavin)
   Split patches for easy review                                   (Gavin)
   Drop changes to reorder variables                               (James)
   Drop unnecessary (@regs removal) cleanup in sdei_event_create() (James)
   Fix broken case for device-tree in sdei_init()                  (James)

Gavin Shan (15):
  drivers/firmware/sdei: Remove sdei_is_err()
  drivers/firmware/sdei: Common block for failing path in
    sdei_event_create()
  drivers/firmware/sdei: Retrieve event number from event instance
  drivers/firmware/sdei: Avoid nested statements in sdei_init()
  drivers/firmware/sdei: Unregister driver on error in sdei_init()
  drivers/firmware/sdei: Remove duplicate check in sdei_get_conduit()
  drivers/firmware/sdei: Remove Drop redundant error message in
    sdei_probe()
  drivers/firmware/sdei: Remove while loop in sdei_event_register()
  drivers/firmware/sdei: Remove while loop in sdei_event_unregister()
  drivers/firmware/sdei: Cleanup on cross call function
  drivers/firmware/sdei: Introduce sdei_do_local_call()
  drivers/firmware/sdei: Remove _sdei_event_register()
  drivers/firmware/sdei: Remove _sdei_event_unregister()
  drivers/firmware/sdei: Expose struct sdei_event
  drivers/firmware/sdei: Identify event by struct sdei_event

 drivers/firmware/arm_sdei.c | 459 +++++++++++++++++-------------------
 include/linux/arm_sdei.h    |  16 +-
 2 files changed, 231 insertions(+), 244 deletions(-)

-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 01/15] drivers/firmware/sdei: Remove sdei_is_err()

[Gavin Shan]

sdei_is_err() is only called by sdei_to_linux_errno(). The logic of
checking on the error number is common to them. They can be combined
finely.

This removes sdei_is_err() and its logic is combined to the function
sdei_to_linux_errno(). Also, the assignment of @err to zero is also
dropped in invoke_sdei_fn() because it's always overridden afterwards.
This shouldn't cause functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: James Morse <james.morse@arm.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index e7e36aab2386..be5367f839f2 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -114,26 +114,7 @@ static int sdei_to_linux_errno(unsigned long sdei_err)
 		return -ENOMEM;
 	}
 
-	/* Not an error value ... */
-	return sdei_err;
-}
-
-/*
- * If x0 is any of these values, then the call failed, use sdei_to_linux_errno()
- * to translate.
- */
-static int sdei_is_err(struct arm_smccc_res *res)
-{
-	switch (res->a0) {
-	case SDEI_NOT_SUPPORTED:
-	case SDEI_INVALID_PARAMETERS:
-	case SDEI_DENIED:
-	case SDEI_PENDING:
-	case SDEI_OUT_OF_RESOURCE:
-		return true;
-	}
-
-	return false;
+	return 0;
 }
 
 static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
@@ -141,14 +122,13 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
 			  unsigned long arg3, unsigned long arg4,
 			  u64 *result)
 {
-	int err = 0;
+	int err;
 	struct arm_smccc_res res;
 
 	if (sdei_firmware_call) {
 		sdei_firmware_call(function_id, arg0, arg1, arg2, arg3, arg4,
 				   &res);
-		if (sdei_is_err(&res))
-			err = sdei_to_linux_errno(res.a0);
+		err = sdei_to_linux_errno(res.a0);
 	} else {
 		/*
 		 * !sdei_firmware_call means we failed to probe or called
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 02/15] drivers/firmware/sdei: Common block for failing path in sdei_event_create()

[Gavin Shan]

There are multiple calls of kfree(event) in the failing path of
sdei_event_create() to free the SDEI event. It means we need to
call it again when adding more code in the failing path. It's
prone to miss doing that and introduce memory leakage.

This introduces common block for failing path in sdei_event_create()
to resolve the issue. This shouldn't cause functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index be5367f839f2..67da02b3a06e 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -190,33 +190,31 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 	lockdep_assert_held(&sdei_events_lock);
 
 	event = kzalloc(sizeof(*event), GFP_KERNEL);
-	if (!event)
-		return ERR_PTR(-ENOMEM);
+	if (!event) {
+		err = -ENOMEM;
+		goto fail;
+	}
 
 	INIT_LIST_HEAD(&event->list);
 	event->event_num = event_num;
 
 	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
 				      &result);
-	if (err) {
-		kfree(event);
-		return ERR_PTR(err);
-	}
+	if (err)
+		goto fail;
 	event->priority = result;
 
 	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_TYPE,
 				      &result);
-	if (err) {
-		kfree(event);
-		return ERR_PTR(err);
-	}
+	if (err)
+		goto fail;
 	event->type = result;
 
 	if (event->type == SDEI_EVENT_TYPE_SHARED) {
 		reg = kzalloc(sizeof(*reg), GFP_KERNEL);
 		if (!reg) {
-			kfree(event);
-			return ERR_PTR(-ENOMEM);
+			err = -ENOMEM;
+			goto fail;
 		}
 
 		reg->event_num = event_num;
@@ -231,8 +229,8 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 
 		regs = alloc_percpu(struct sdei_registered_event);
 		if (!regs) {
-			kfree(event);
-			return ERR_PTR(-ENOMEM);
+			err = -ENOMEM;
+			goto fail;
 		}
 
 		for_each_possible_cpu(cpu) {
@@ -252,6 +250,10 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 	spin_unlock(&sdei_list_lock);
 
 	return event;
+
+fail:
+	kfree(event);
+	return ERR_PTR(err);
 }
 
 static void sdei_event_destroy_llocked(struct sdei_event *event)
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 03/15] drivers/firmware/sdei: Retrieve event number from event instance

[Gavin Shan]

In sdei_event_create(), the event number is retrieved from the
variable @event_num for the shared event. The event number was
stored in the event instance. So we can fetch it from the event
instance, similar to what we're doing for the private event.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 67da02b3a06e..86ab878d1198 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -217,7 +217,7 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 			goto fail;
 		}
 
-		reg->event_num = event_num;
+		reg->event_num = event->event_num;
 		reg->priority = event->priority;
 
 		reg->callback = cb;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 04/15] drivers/firmware/sdei: Avoid nested statements in sdei_init()

[Gavin Shan]

In sdei_init(), the nested statements can be avoided by bailing
on error from platform_driver_register() or absent ACPI SDEI table.
With it, the code looks a bit more readable.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 86ab878d1198..c9f2bedfb6b5 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -1081,17 +1081,18 @@ static bool __init sdei_present_acpi(void)
 
 static int __init sdei_init(void)
 {
-	int ret = platform_driver_register(&sdei_driver);
-
-	if (!ret && sdei_present_acpi()) {
-		struct platform_device *pdev;
-
-		pdev = platform_device_register_simple(sdei_driver.driver.name,
-						       0, NULL, 0);
-		if (IS_ERR(pdev))
-			pr_info("Failed to register ACPI:SDEI platform device %ld\n",
-				PTR_ERR(pdev));
-	}
+	struct platform_device *pdev;
+	int ret;
+
+	ret = platform_driver_register(&sdei_driver);
+	if (ret || !sdei_present_acpi())
+		return ret;
+
+	pdev = platform_device_register_simple(sdei_driver.driver.name,
+					       0, NULL, 0);
+	if (IS_ERR(pdev))
+		pr_info("Failed to register ACPI:SDEI platform device %ld\n",
+			PTR_ERR(pdev));
 
 	return ret;
 }
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 05/15] drivers/firmware/sdei: Unregister driver on error in sdei_init()

[Gavin Shan]

The platform driver needs to be unregistered on error to create the
platform device, so that the state is restored to original one.

Besides, the errno (@ret) should be updated acccording in that case.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index c9f2bedfb6b5..909f27abf8a7 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -1090,9 +1090,12 @@ static int __init sdei_init(void)
 
 	pdev = platform_device_register_simple(sdei_driver.driver.name,
 					       0, NULL, 0);
-	if (IS_ERR(pdev))
-		pr_info("Failed to register ACPI:SDEI platform device %ld\n",
-			PTR_ERR(pdev));
+	if (IS_ERR(pdev)) {
+		ret = PTR_ERR(pdev);
+		platform_driver_unregister(&sdei_driver);
+		pr_info("Failed to register ACPI:SDEI platform device %d\n",
+			ret);
+	}
 
 	return ret;
 }
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 06/15] drivers/firmware/sdei: Remove duplicate check in sdei_get_conduit()

[Gavin Shan]

The following two checks are duplicate because @acpi_disabled doesn't
depend on CONFIG_ACPI. So the duplicate check (IS_ENABLED(CONFIG_ACPI))
can be dropped. More details is provided to keep the commit log complete:

   * @acpi_disabled is defined in arch/arm64/kernel/acpi.c when
     CONFIG_ACPI is enabled.
   * @acpi_disabled in defined in include/acpi.h when CONFIG_ACPI
     is disabled.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 909f27abf8a7..240c06ae7bfe 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -958,7 +958,7 @@ static int sdei_get_conduit(struct platform_device *pdev)
 		}
 
 		pr_warn("invalid \"method\" property: %s\n", method);
-	} else if (IS_ENABLED(CONFIG_ACPI) && !acpi_disabled) {
+	} else if (!acpi_disabled) {
 		if (acpi_psci_use_hvc()) {
 			sdei_firmware_call = &sdei_smccc_hvc;
 			return SMCCC_CONDUIT_HVC;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 07/15] drivers/firmware/sdei: Remove Drop redundant error message in sdei_probe()

[Gavin Shan]

This removes the redundant error message in sdei_probe() because
the case can be identified from the errno in next error message.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 240c06ae7bfe..03b1179da9b4 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -982,8 +982,6 @@ static int sdei_probe(struct platform_device *pdev)
 		return 0;
 
 	err = sdei_api_get_version(&ver);
-	if (err == -EOPNOTSUPP)
-		pr_err("advertised but not implemented in platform firmware\n");
 	if (err) {
 		pr_err("Failed to get SDEI version: %d\n", err);
 		sdei_mark_interface_broken();
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 08/15] drivers/firmware/sdei: Remove while loop in sdei_event_register()

[Gavin Shan]

This removes the unnecessary while loop in sdei_event_register().
This shouldn't cause any functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 52 ++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 03b1179da9b4..d04329f98922 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -590,36 +590,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	WARN_ON(in_nmi());
 
 	mutex_lock(&sdei_events_lock);
-	do {
-		if (sdei_event_find(event_num)) {
-			pr_warn("Event %u already registered\n", event_num);
-			err = -EBUSY;
-			break;
-		}
+	if (sdei_event_find(event_num)) {
+		pr_warn("Event %u already registered\n", event_num);
+		err = -EBUSY;
+		goto unlock;
+	}
 
-		event = sdei_event_create(event_num, cb, arg);
-		if (IS_ERR(event)) {
-			err = PTR_ERR(event);
-			pr_warn("Failed to create event %u: %d\n", event_num,
-				err);
-			break;
-		}
+	event = sdei_event_create(event_num, cb, arg);
+	if (IS_ERR(event)) {
+		err = PTR_ERR(event);
+		pr_warn("Failed to create event %u: %d\n", event_num, err);
+		goto unlock;
+	}
 
-		cpus_read_lock();
-		err = _sdei_event_register(event);
-		if (err) {
-			sdei_event_destroy(event);
-			pr_warn("Failed to register event %u: %d\n", event_num,
-				err);
-		} else {
-			spin_lock(&sdei_list_lock);
-			event->reregister = true;
-			spin_unlock(&sdei_list_lock);
-		}
-		cpus_read_unlock();
-	} while (0);
-	mutex_unlock(&sdei_events_lock);
+	cpus_read_lock();
+	err = _sdei_event_register(event);
+	if (err) {
+		sdei_event_destroy(event);
+		pr_warn("Failed to register event %u: %d\n", event_num, err);
+		goto cpu_unlock;
+	}
 
+	spin_lock(&sdei_list_lock);
+	event->reregister = true;
+	spin_unlock(&sdei_list_lock);
+cpu_unlock:
+	cpus_read_unlock();
+unlock:
+	mutex_unlock(&sdei_events_lock);
 	return err;
 }
 
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 09/15] drivers/firmware/sdei: Remove while loop in sdei_event_unregister()

[Gavin Shan]

This removes the unnecessary while loop in sdei_event_unregister().
This shouldn't cause any functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index d04329f98922..c8e894098c56 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -491,24 +491,23 @@ int sdei_event_unregister(u32 event_num)
 
 	mutex_lock(&sdei_events_lock);
 	event = sdei_event_find(event_num);
-	do {
-		if (!event) {
-			pr_warn("Event %u not registered\n", event_num);
-			err = -ENOENT;
-			break;
-		}
+	if (!event) {
+		pr_warn("Event %u not registered\n", event_num);
+		err = -ENOENT;
+		goto unlock;
+	}
 
-		spin_lock(&sdei_list_lock);
-		event->reregister = false;
-		event->reenable = false;
-		spin_unlock(&sdei_list_lock);
+	spin_lock(&sdei_list_lock);
+	event->reregister = false;
+	event->reenable = false;
+	spin_unlock(&sdei_list_lock);
 
-		err = _sdei_event_unregister(event);
-		if (err)
-			break;
+	err = _sdei_event_unregister(event);
+	if (err)
+		goto unlock;
 
-		sdei_event_destroy(event);
-	} while (0);
+	sdei_event_destroy(event);
+unlock:
 	mutex_unlock(&sdei_events_lock);
 
 	return err;
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 10/15] drivers/firmware/sdei: Cleanup on cross call function

[Gavin Shan]

This applies cleanup on the cross call functions, no functional
changes are introduced:

   * Refactor CROSSCALL_INIT to use "do { ... } while (0)" to be
     compatible with scripts/checkpatch.pl
   * Use smp_call_func_t for @fn argument in sdei_do_cross_call()
   * Remove unnecessary space before @event in sdei_do_cross_call()

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index c8e894098c56..5560c8880631 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -78,11 +78,15 @@ struct sdei_crosscall_args {
 	int first_error;
 };
 
-#define CROSSCALL_INIT(arg, event)	(arg.event = event, \
-					 arg.first_error = 0, \
-					 atomic_set(&arg.errors, 0))
-
-static inline int sdei_do_cross_call(void *fn, struct sdei_event * event)
+#define CROSSCALL_INIT(arg, event)		\
+	do {					\
+		arg.event = event;		\
+		arg.first_error = 0;		\
+		atomic_set(&arg.errors, 0);	\
+	} while (0)
+
+static inline int sdei_do_cross_call(smp_call_func_t fn,
+				     struct sdei_event *event)
 {
 	struct sdei_crosscall_args arg;
 
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 11/15] drivers/firmware/sdei: Introduce sdei_do_local_call()

[Gavin Shan]

During the CPU hotplug, the private events are registered, enabled
or unregistered on the specific CPU. It repeats the same steps:
initializing cross call argument, make function call on local CPU,
check the returned error.

This introduces sdei_do_local_call() to cover the first steps. The
other benefit is to make CROSSCALL_INIT and struct sdei_crosscall_args
are only visible to sddi_do_{cross, local}_call().

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 41 ++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 5560c8880631..7f4309039513 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -85,6 +85,17 @@ struct sdei_crosscall_args {
 		atomic_set(&arg.errors, 0);	\
 	} while (0)
 
+static inline int sdei_do_local_call(smp_call_func_t fn,
+				     struct sdei_event *event)
+{
+	struct sdei_crosscall_args arg;
+
+	CROSSCALL_INIT(arg, event);
+	fn(&arg);
+
+	return arg.first_error;
+}
+
 static inline int sdei_do_cross_call(smp_call_func_t fn,
 				     struct sdei_event *event)
 {
@@ -677,7 +688,7 @@ static int sdei_reregister_shared(void)
 static int sdei_cpuhp_down(unsigned int cpu)
 {
 	struct sdei_event *event;
-	struct sdei_crosscall_args arg;
+	int err;
 
 	/* un-register private events */
 	spin_lock(&sdei_list_lock);
@@ -685,12 +696,11 @@ static int sdei_cpuhp_down(unsigned int cpu)
 		if (event->type == SDEI_EVENT_TYPE_SHARED)
 			continue;
 
-		CROSSCALL_INIT(arg, event);
-		/* call the cross-call function locally... */
-		_local_event_unregister(&arg);
-		if (arg.first_error)
+		err = sdei_do_local_call(_local_event_unregister, event);
+		if (err) {
 			pr_err("Failed to unregister event %u: %d\n",
-			       event->event_num, arg.first_error);
+			       event->event_num, err);
+		}
 	}
 	spin_unlock(&sdei_list_lock);
 
@@ -700,7 +710,7 @@ static int sdei_cpuhp_down(unsigned int cpu)
 static int sdei_cpuhp_up(unsigned int cpu)
 {
 	struct sdei_event *event;
-	struct sdei_crosscall_args arg;
+	int err;
 
 	/* re-register/enable private events */
 	spin_lock(&sdei_list_lock);
@@ -709,20 +719,19 @@ static int sdei_cpuhp_up(unsigned int cpu)
 			continue;
 
 		if (event->reregister) {
-			CROSSCALL_INIT(arg, event);
-			/* call the cross-call function locally... */
-			_local_event_register(&arg);
-			if (arg.first_error)
+			err = sdei_do_local_call(_local_event_register, event);
+			if (err) {
 				pr_err("Failed to re-register event %u: %d\n",
-				       event->event_num, arg.first_error);
+				       event->event_num, err);
+			}
 		}
 
 		if (event->reenable) {
-			CROSSCALL_INIT(arg, event);
-			_local_event_enable(&arg);
-			if (arg.first_error)
+			err = sdei_do_local_call(_local_event_enable, event);
+			if (err) {
 				pr_err("Failed to re-enable event %u: %d\n",
-				       event->event_num, arg.first_error);
+				       event->event_num, err);
+			}
 		}
 	}
 	spin_unlock(&sdei_list_lock);
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 12/15] drivers/firmware/sdei: Remove _sdei_event_register()

[Gavin Shan]

The function _sdei_event_register() is called by sdei_event_register()
and sdei_device_thaw() as the following functional call chain shows.
_sdei_event_register() covers the shared and private events, but
sdei_device_thaw() only covers the shared events. So the logic to
cover the private events in _sdei_event_register() isn't needed by
sdei_device_thaw().

Similarly, sdei_reregister_event_llocked() covers the shared and
private events in the regard of reenablement. The logic to cover
the private events isn't needed by sdei_device_thaw() either.

   sdei_event_register          sdei_device_thaw
      _sdei_event_register         sdei_reregister_shared
                                      sdei_reregister_event_llocked
                                         _sdei_event_register

This removes _sdei_event_register() and sdei_reregister_event_llocked().
Their logic is moved to sdei_event_register() and sdei_reregister_shared().
This shouldn't cause any logicial changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 77 ++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 49 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 7f4309039513..749f1619e652 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -577,25 +577,6 @@ static void _local_event_register(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-static int _sdei_event_register(struct sdei_event *event)
-{
-	int err;
-
-	lockdep_assert_held(&sdei_events_lock);
-
-	if (event->type == SDEI_EVENT_TYPE_SHARED)
-		return sdei_api_event_register(event->event_num,
-					       sdei_entry_point,
-					       event->registered,
-					       SDEI_EVENT_REGISTER_RM_ANY, 0);
-
-	err = sdei_do_cross_call(_local_event_register, event);
-	if (err)
-		sdei_do_cross_call(_local_event_unregister, event);
-
-	return err;
-}
-
 int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 {
 	int err;
@@ -618,7 +599,17 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	}
 
 	cpus_read_lock();
-	err = _sdei_event_register(event);
+	if (event->type == SDEI_EVENT_TYPE_SHARED) {
+		err = sdei_api_event_register(event->event_num,
+					      sdei_entry_point,
+					      event->registered,
+					      SDEI_EVENT_REGISTER_RM_ANY, 0);
+	} else {
+		err = sdei_do_cross_call(_local_event_register, event);
+		if (err)
+			sdei_do_cross_call(_local_event_unregister, event);
+	}
+
 	if (err) {
 		sdei_event_destroy(event);
 		pr_warn("Failed to register event %u: %d\n", event_num, err);
@@ -635,33 +626,6 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	return err;
 }
 
-static int sdei_reregister_event_llocked(struct sdei_event *event)
-{
-	int err;
-
-	lockdep_assert_held(&sdei_events_lock);
-	lockdep_assert_held(&sdei_list_lock);
-
-	err = _sdei_event_register(event);
-	if (err) {
-		pr_err("Failed to re-register event %u\n", event->event_num);
-		sdei_event_destroy_llocked(event);
-		return err;
-	}
-
-	if (event->reenable) {
-		if (event->type == SDEI_EVENT_TYPE_SHARED)
-			err = sdei_api_event_enable(event->event_num);
-		else
-			err = sdei_do_cross_call(_local_event_enable, event);
-	}
-
-	if (err)
-		pr_err("Failed to re-enable event %u\n", event->event_num);
-
-	return err;
-}
-
 static int sdei_reregister_shared(void)
 {
 	int err = 0;
@@ -674,9 +638,24 @@ static int sdei_reregister_shared(void)
 			continue;
 
 		if (event->reregister) {
-			err = sdei_reregister_event_llocked(event);
-			if (err)
+			err = sdei_api_event_register(event->event_num,
+					sdei_entry_point, event->registered,
+					SDEI_EVENT_REGISTER_RM_ANY, 0);
+			if (err) {
+				sdei_event_destroy_llocked(event);
+				pr_err("Failed to re-register event %u\n",
+				       event->event_num);
 				break;
+			}
+		}
+
+		if (event->reenable) {
+			err = sdei_api_event_enable(event->event_num);
+			if (err) {
+				pr_err("Failed to re-enable event %u\n",
+				       event->event_num);
+				break;
+			}
 		}
 	}
 	spin_unlock(&sdei_list_lock);
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 13/15] drivers/firmware/sdei: Remove _sdei_event_unregister()

[Gavin Shan]

_sdei_event_unregister() is called by sdei_event_unregister() and
sdei_device_freeze(). _sdei_event_unregister() covers the shared
and private events, but sdei_device_freeze() only covers the shared
events. So the logic to cover the private events isn't needed by
sdei_device_freeze().

   sdei_event_unregister        sdei_device_freeze
      _sdei_event_unregister       sdei_unregister_shared
                                     _sdei_event_unregister

This removes _sdei_event_unregister(). Its logic is moved to its
callers accordingly. This shouldn't cause any logicial changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 749f1619e652..2678475940e6 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -487,16 +487,6 @@ static void _local_event_unregister(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-static int _sdei_event_unregister(struct sdei_event *event)
-{
-	lockdep_assert_held(&sdei_events_lock);
-
-	if (event->type == SDEI_EVENT_TYPE_SHARED)
-		return sdei_api_event_unregister(event->event_num);
-
-	return sdei_do_cross_call(_local_event_unregister, event);
-}
-
 int sdei_event_unregister(u32 event_num)
 {
 	int err;
@@ -517,7 +507,11 @@ int sdei_event_unregister(u32 event_num)
 	event->reenable = false;
 	spin_unlock(&sdei_list_lock);
 
-	err = _sdei_event_unregister(event);
+	if (event->type == SDEI_EVENT_TYPE_SHARED)
+		err = sdei_api_event_unregister(event->event_num);
+	else
+		err = sdei_do_cross_call(_local_event_unregister, event);
+
 	if (err)
 		goto unlock;
 
@@ -543,7 +537,7 @@ static int sdei_unregister_shared(void)
 		if (event->type != SDEI_EVENT_TYPE_SHARED)
 			continue;
 
-		err = _sdei_event_unregister(event);
+		err = sdei_api_event_unregister(event->event_num);
 		if (err)
 			break;
 	}
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 14/15] drivers/firmware/sdei: Expose struct sdei_event

[Gavin Shan]

This splits struct sdei_event into two structs: sdei_event and
sdei_internal_event. The former one can be dereferenced from external
module like arm64/kvm when SDEI virtualization is supported. The later
one is used by the client driver only.

This also renames local variables from @event to @event_el if their
type is "struct sdei_internal_event" to make the variable name a bit
meaningful. Note that it's not applied to sdei_do_{local,cross}_call()
because CROSSCALL_INIT stops us doing this unless much more changes
are needed. I'm avoiding to do that.

Besides, @event is added as "struct sdei_event" to cache @event_el->event
if it's accessed for multiple times in the particular function to reduce
the code change size.

This shouldn't cause functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v4: Rename variable @event to @event_el if it's internal event
    Use @event to cache @event_el->event if it's used for multiple times
---
 drivers/firmware/arm_sdei.c | 160 ++++++++++++++++++++----------------
 include/linux/arm_sdei.h    |   6 ++
 2 files changed, 94 insertions(+), 72 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 2678475940e6..f9827c096275 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -44,16 +44,14 @@ static asmlinkage void (*sdei_firmware_call)(unsigned long function_id,
 /* entry point from firmware to arch asm code */
 static unsigned long sdei_entry_point;
 
-struct sdei_event {
+struct sdei_internal_event {
+	struct sdei_event	event;
+
 	/* These three are protected by the sdei_list_lock */
 	struct list_head	list;
 	bool			reregister;
 	bool			reenable;
 
-	u32			event_num;
-	u8			type;
-	u8			priority;
-
 	/* This pointer is handed to firmware as the event argument. */
 	union {
 		/* Shared events */
@@ -73,7 +71,7 @@ static LIST_HEAD(sdei_list);
 
 /* Private events are registered/enabled via IPI passing one of these */
 struct sdei_crosscall_args {
-	struct sdei_event *event;
+	struct sdei_internal_event *event;
 	atomic_t errors;
 	int first_error;
 };
@@ -86,7 +84,7 @@ struct sdei_crosscall_args {
 	} while (0)
 
 static inline int sdei_do_local_call(smp_call_func_t fn,
-				     struct sdei_event *event)
+				     struct sdei_internal_event *event)
 {
 	struct sdei_crosscall_args arg;
 
@@ -97,7 +95,7 @@ static inline int sdei_do_local_call(smp_call_func_t fn,
 }
 
 static inline int sdei_do_cross_call(smp_call_func_t fn,
-				     struct sdei_event *event)
+				     struct sdei_internal_event *event)
 {
 	struct sdei_crosscall_args arg;
 
@@ -162,16 +160,16 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
 }
 NOKPROBE_SYMBOL(invoke_sdei_fn);
 
-static struct sdei_event *sdei_event_find(u32 event_num)
+static struct sdei_internal_event *sdei_event_find(u32 event_num)
 {
-	struct sdei_event *e, *found = NULL;
+	struct sdei_internal_event *event_el, *found = NULL;
 
 	lockdep_assert_held(&sdei_events_lock);
 
 	spin_lock(&sdei_list_lock);
-	list_for_each_entry(e, &sdei_list, list) {
-		if (e->event_num == event_num) {
-			found = e;
+	list_for_each_entry(event_el, &sdei_list, list) {
+		if (event_el->event.event_num == event_num) {
+			found = event_el;
 			break;
 		}
 	}
@@ -193,24 +191,26 @@ static int sdei_api_event_get_info(u32 event, u32 info, u64 *result)
 			      0, 0, result);
 }
 
-static struct sdei_event *sdei_event_create(u32 event_num,
-					    sdei_event_callback *cb,
-					    void *cb_arg)
+static struct sdei_internal_event *sdei_event_create(u32 event_num,
+						     sdei_event_callback *cb,
+						     void *cb_arg)
 {
 	int err;
 	u64 result;
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 	struct sdei_registered_event *reg;
 
 	lockdep_assert_held(&sdei_events_lock);
 
-	event = kzalloc(sizeof(*event), GFP_KERNEL);
-	if (!event) {
+	event_el = kzalloc(sizeof(*event_el), GFP_KERNEL);
+	if (!event_el) {
 		err = -ENOMEM;
 		goto fail;
 	}
 
-	INIT_LIST_HEAD(&event->list);
+	event = &event_el->event;
+	INIT_LIST_HEAD(&event_el->list);
 	event->event_num = event_num;
 
 	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
@@ -237,7 +237,7 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 
 		reg->callback = cb;
 		reg->callback_arg = cb_arg;
-		event->registered = reg;
+		event_el->registered = reg;
 	} else {
 		int cpu;
 		struct sdei_registered_event __percpu *regs;
@@ -257,39 +257,39 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 			reg->callback_arg = cb_arg;
 		}
 
-		event->private_registered = regs;
+		event_el->private_registered = regs;
 	}
 
 	spin_lock(&sdei_list_lock);
-	list_add(&event->list, &sdei_list);
+	list_add(&event_el->list, &sdei_list);
 	spin_unlock(&sdei_list_lock);
 
-	return event;
+	return event_el;
 
 fail:
-	kfree(event);
+	kfree(event_el);
 	return ERR_PTR(err);
 }
 
-static void sdei_event_destroy_llocked(struct sdei_event *event)
+static void sdei_event_destroy_llocked(struct sdei_internal_event *event_el)
 {
 	lockdep_assert_held(&sdei_events_lock);
 	lockdep_assert_held(&sdei_list_lock);
 
-	list_del(&event->list);
+	list_del(&event_el->list);
 
-	if (event->type == SDEI_EVENT_TYPE_SHARED)
-		kfree(event->registered);
+	if (event_el->event.type == SDEI_EVENT_TYPE_SHARED)
+		kfree(event_el->registered);
 	else
-		free_percpu(event->private_registered);
+		free_percpu(event_el->private_registered);
 
-	kfree(event);
+	kfree(event_el);
 }
 
-static void sdei_event_destroy(struct sdei_event *event)
+static void sdei_event_destroy(struct sdei_internal_event *event_el)
 {
 	spin_lock(&sdei_list_lock);
-	sdei_event_destroy_llocked(event);
+	sdei_event_destroy_llocked(event_el);
 	spin_unlock(&sdei_list_lock);
 }
 
@@ -392,7 +392,7 @@ static void _local_event_enable(void *data)
 
 	WARN_ON_ONCE(preemptible());
 
-	err = sdei_api_event_enable(arg->event->event_num);
+	err = sdei_api_event_enable(arg->event->event.event_num);
 
 	sdei_cross_call_return(arg, err);
 }
@@ -400,25 +400,27 @@ static void _local_event_enable(void *data)
 int sdei_event_enable(u32 event_num)
 {
 	int err = -EINVAL;
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 
 	mutex_lock(&sdei_events_lock);
-	event = sdei_event_find(event_num);
-	if (!event) {
+	event_el = sdei_event_find(event_num);
+	if (!event_el) {
 		mutex_unlock(&sdei_events_lock);
 		return -ENOENT;
 	}
 
 
 	cpus_read_lock();
+	event = &event_el->event;
 	if (event->type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_enable(event->event_num);
 	else
-		err = sdei_do_cross_call(_local_event_enable, event);
+		err = sdei_do_cross_call(_local_event_enable, event_el);
 
 	if (!err) {
 		spin_lock(&sdei_list_lock);
-		event->reenable = true;
+		event_el->reenable = true;
 		spin_unlock(&sdei_list_lock);
 	}
 	cpus_read_unlock();
@@ -438,7 +440,7 @@ static void _ipi_event_disable(void *data)
 	int err;
 	struct sdei_crosscall_args *arg = data;
 
-	err = sdei_api_event_disable(arg->event->event_num);
+	err = sdei_api_event_disable(arg->event->event.event_num);
 
 	sdei_cross_call_return(arg, err);
 }
@@ -446,23 +448,25 @@ static void _ipi_event_disable(void *data)
 int sdei_event_disable(u32 event_num)
 {
 	int err = -EINVAL;
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 
 	mutex_lock(&sdei_events_lock);
-	event = sdei_event_find(event_num);
-	if (!event) {
+	event_el = sdei_event_find(event_num);
+	if (!event_el) {
 		mutex_unlock(&sdei_events_lock);
 		return -ENOENT;
 	}
 
 	spin_lock(&sdei_list_lock);
-	event->reenable = false;
+	event_el->reenable = false;
 	spin_unlock(&sdei_list_lock);
 
+	event = &event_el->event;
 	if (event->type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_disable(event->event_num);
 	else
-		err = sdei_do_cross_call(_ipi_event_disable, event);
+		err = sdei_do_cross_call(_ipi_event_disable, event_el);
 	mutex_unlock(&sdei_events_lock);
 
 	return err;
@@ -482,7 +486,7 @@ static void _local_event_unregister(void *data)
 
 	WARN_ON_ONCE(preemptible());
 
-	err = sdei_api_event_unregister(arg->event->event_num);
+	err = sdei_api_event_unregister(arg->event->event.event_num);
 
 	sdei_cross_call_return(arg, err);
 }
@@ -490,32 +494,34 @@ static void _local_event_unregister(void *data)
 int sdei_event_unregister(u32 event_num)
 {
 	int err;
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 
 	WARN_ON(in_nmi());
 
 	mutex_lock(&sdei_events_lock);
-	event = sdei_event_find(event_num);
-	if (!event) {
+	event_el = sdei_event_find(event_num);
+	if (!event_el) {
 		pr_warn("Event %u not registered\n", event_num);
 		err = -ENOENT;
 		goto unlock;
 	}
 
 	spin_lock(&sdei_list_lock);
-	event->reregister = false;
-	event->reenable = false;
+	event_el->reregister = false;
+	event_el->reenable = false;
 	spin_unlock(&sdei_list_lock);
 
+	event = &event_el->event;
 	if (event->type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_unregister(event->event_num);
 	else
-		err = sdei_do_cross_call(_local_event_unregister, event);
+		err = sdei_do_cross_call(_local_event_unregister, event_el);
 
 	if (err)
 		goto unlock;
 
-	sdei_event_destroy(event);
+	sdei_event_destroy(event_el);
 unlock:
 	mutex_unlock(&sdei_events_lock);
 
@@ -529,11 +535,13 @@ int sdei_event_unregister(u32 event_num)
 static int sdei_unregister_shared(void)
 {
 	int err = 0;
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 
 	mutex_lock(&sdei_events_lock);
 	spin_lock(&sdei_list_lock);
-	list_for_each_entry(event, &sdei_list, list) {
+	list_for_each_entry(event_el, &sdei_list, list) {
+		event = &event_el->event;
 		if (event->type != SDEI_EVENT_TYPE_SHARED)
 			continue;
 
@@ -565,8 +573,8 @@ static void _local_event_register(void *data)
 	WARN_ON(preemptible());
 
 	reg = per_cpu_ptr(arg->event->private_registered, smp_processor_id());
-	err = sdei_api_event_register(arg->event->event_num, sdei_entry_point,
-				      reg, 0, 0);
+	err = sdei_api_event_register(arg->event->event.event_num,
+				      sdei_entry_point, reg, 0, 0);
 
 	sdei_cross_call_return(arg, err);
 }
@@ -574,6 +582,7 @@ static void _local_event_register(void *data)
 int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 {
 	int err;
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 
 	WARN_ON(in_nmi());
@@ -585,33 +594,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 		goto unlock;
 	}
 
-	event = sdei_event_create(event_num, cb, arg);
-	if (IS_ERR(event)) {
-		err = PTR_ERR(event);
+	event_el = sdei_event_create(event_num, cb, arg);
+	if (IS_ERR(event_el)) {
+		err = PTR_ERR(event_el);
 		pr_warn("Failed to create event %u: %d\n", event_num, err);
 		goto unlock;
 	}
 
 	cpus_read_lock();
+	event = &event_el->event;
 	if (event->type == SDEI_EVENT_TYPE_SHARED) {
 		err = sdei_api_event_register(event->event_num,
 					      sdei_entry_point,
-					      event->registered,
+					      event_el->registered,
 					      SDEI_EVENT_REGISTER_RM_ANY, 0);
 	} else {
-		err = sdei_do_cross_call(_local_event_register, event);
+		err = sdei_do_cross_call(_local_event_register, event_el);
 		if (err)
-			sdei_do_cross_call(_local_event_unregister, event);
+			sdei_do_cross_call(_local_event_unregister, event_el);
 	}
 
 	if (err) {
-		sdei_event_destroy(event);
+		sdei_event_destroy(event_el);
 		pr_warn("Failed to register event %u: %d\n", event_num, err);
 		goto cpu_unlock;
 	}
 
 	spin_lock(&sdei_list_lock);
-	event->reregister = true;
+	event_el->reregister = true;
 	spin_unlock(&sdei_list_lock);
 cpu_unlock:
 	cpus_read_unlock();
@@ -623,27 +633,29 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 static int sdei_reregister_shared(void)
 {
 	int err = 0;
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 
 	mutex_lock(&sdei_events_lock);
 	spin_lock(&sdei_list_lock);
-	list_for_each_entry(event, &sdei_list, list) {
+	list_for_each_entry(event_el, &sdei_list, list) {
+		event = &event_el->event;
 		if (event->type != SDEI_EVENT_TYPE_SHARED)
 			continue;
 
-		if (event->reregister) {
+		if (event_el->reregister) {
 			err = sdei_api_event_register(event->event_num,
-					sdei_entry_point, event->registered,
+					sdei_entry_point, event_el->registered,
 					SDEI_EVENT_REGISTER_RM_ANY, 0);
 			if (err) {
-				sdei_event_destroy_llocked(event);
+				sdei_event_destroy_llocked(event_el);
 				pr_err("Failed to re-register event %u\n",
 				       event->event_num);
 				break;
 			}
 		}
 
-		if (event->reenable) {
+		if (event_el->reenable) {
 			err = sdei_api_event_enable(event->event_num);
 			if (err) {
 				pr_err("Failed to re-enable event %u\n",
@@ -660,16 +672,18 @@ static int sdei_reregister_shared(void)
 
 static int sdei_cpuhp_down(unsigned int cpu)
 {
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 	int err;
 
 	/* un-register private events */
 	spin_lock(&sdei_list_lock);
-	list_for_each_entry(event, &sdei_list, list) {
+	list_for_each_entry(event_el, &sdei_list, list) {
+		event = &event_el->event;
 		if (event->type == SDEI_EVENT_TYPE_SHARED)
 			continue;
 
-		err = sdei_do_local_call(_local_event_unregister, event);
+		err = sdei_do_local_call(_local_event_unregister, event_el);
 		if (err) {
 			pr_err("Failed to unregister event %u: %d\n",
 			       event->event_num, err);
@@ -682,25 +696,27 @@ static int sdei_cpuhp_down(unsigned int cpu)
 
 static int sdei_cpuhp_up(unsigned int cpu)
 {
+	struct sdei_internal_event *event_el;
 	struct sdei_event *event;
 	int err;
 
 	/* re-register/enable private events */
 	spin_lock(&sdei_list_lock);
-	list_for_each_entry(event, &sdei_list, list) {
+	list_for_each_entry(event_el, &sdei_list, list) {
+		event = &event_el->event;
 		if (event->type == SDEI_EVENT_TYPE_SHARED)
 			continue;
 
-		if (event->reregister) {
-			err = sdei_do_local_call(_local_event_register, event);
+		if (event_el->reregister) {
+			err = sdei_do_local_call(_local_event_register, event_el);
 			if (err) {
 				pr_err("Failed to re-register event %u: %d\n",
 				       event->event_num, err);
 			}
 		}
 
-		if (event->reenable) {
-			err = sdei_do_local_call(_local_event_enable, event);
+		if (event_el->reenable) {
+			err = sdei_do_local_call(_local_event_enable, event_el);
 			if (err) {
 				pr_err("Failed to re-enable event %u: %d\n",
 				       event->event_num, err);
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index 0a241c5c911d..d2464a18b6ff 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -22,6 +22,12 @@
  */
 typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
 
+struct sdei_event {
+	u32			event_num;
+	u8			type;
+	u8			priority;
+};
+
 /*
  * Register your callback to claim an event. The event must be described
  * by firmware.
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 15/15] drivers/firmware/sdei: Identify event by struct sdei_event

[Gavin Shan]

There are 4 APIs exported by the driver as below. They are using
the event number as the identifier to the event. It's conflicting
with the requirement to dereference the event by struct sdei_event
instance when SDEI virtualization is supported. So this reworks on
the APIs accordingly to dereference the event by struct sdei_event
instance:

   * sdei_event_register() returns the struct sdei_event instance.
   * sdei_event_unregister() and sdei_event_{enable, disable}()
     accepts struct sdei_event instance as the parameter.
   * Rework sdei_{register,unregister}_ghes() to use the modified
     APIs.

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v4: Rename @se to @event for APIs
---
 drivers/firmware/arm_sdei.c | 76 ++++++++++++++++++-------------------
 include/linux/arm_sdei.h    | 10 +++--
 2 files changed, 42 insertions(+), 44 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index f9827c096275..9c7a6a7c9527 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -397,22 +397,14 @@ static void _local_event_enable(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_enable(u32 event_num)
+int sdei_event_enable(struct sdei_event *event)
 {
 	int err = -EINVAL;
-	struct sdei_internal_event *event_el;
-	struct sdei_event *event;
+	struct sdei_internal_event *event_el =
+		container_of(event, struct sdei_internal_event, event);
 
 	mutex_lock(&sdei_events_lock);
-	event_el = sdei_event_find(event_num);
-	if (!event_el) {
-		mutex_unlock(&sdei_events_lock);
-		return -ENOENT;
-	}
-
-
 	cpus_read_lock();
-	event = &event_el->event;
 	if (event->type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_enable(event->event_num);
 	else
@@ -445,24 +437,17 @@ static void _ipi_event_disable(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_disable(u32 event_num)
+int sdei_event_disable(struct sdei_event *event)
 {
 	int err = -EINVAL;
-	struct sdei_internal_event *event_el;
-	struct sdei_event *event;
+	struct sdei_internal_event *event_el =
+		container_of(event, struct sdei_internal_event, event);
 
 	mutex_lock(&sdei_events_lock);
-	event_el = sdei_event_find(event_num);
-	if (!event_el) {
-		mutex_unlock(&sdei_events_lock);
-		return -ENOENT;
-	}
-
 	spin_lock(&sdei_list_lock);
 	event_el->reenable = false;
 	spin_unlock(&sdei_list_lock);
 
-	event = &event_el->event;
 	if (event->type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_disable(event->event_num);
 	else
@@ -491,28 +476,20 @@ static void _local_event_unregister(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_unregister(u32 event_num)
+int sdei_event_unregister(struct sdei_event *event)
 {
 	int err;
-	struct sdei_internal_event *event_el;
-	struct sdei_event *event;
+	struct sdei_internal_event *event_el =
+		container_of(event, struct sdei_internal_event, event);
 
 	WARN_ON(in_nmi());
 
 	mutex_lock(&sdei_events_lock);
-	event_el = sdei_event_find(event_num);
-	if (!event_el) {
-		pr_warn("Event %u not registered\n", event_num);
-		err = -ENOENT;
-		goto unlock;
-	}
-
 	spin_lock(&sdei_list_lock);
 	event_el->reregister = false;
 	event_el->reenable = false;
 	spin_unlock(&sdei_list_lock);
 
-	event = &event_el->event;
 	if (event->type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_unregister(event->event_num);
 	else
@@ -579,7 +556,9 @@ static void _local_event_register(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
+struct sdei_event *sdei_event_register(u32 event_num,
+				       sdei_event_callback *cb,
+				       void *arg)
 {
 	int err;
 	struct sdei_internal_event *event_el;
@@ -590,13 +569,14 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	mutex_lock(&sdei_events_lock);
 	if (sdei_event_find(event_num)) {
 		pr_warn("Event %u already registered\n", event_num);
-		err = -EBUSY;
+		event = ERR_PTR(-EBUSY);
 		goto unlock;
 	}
 
 	event_el = sdei_event_create(event_num, cb, arg);
 	if (IS_ERR(event_el)) {
 		err = PTR_ERR(event_el);
+		event = ERR_PTR(err);
 		pr_warn("Failed to create event %u: %d\n", event_num, err);
 		goto unlock;
 	}
@@ -615,11 +595,13 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	}
 
 	if (err) {
+		event = ERR_PTR(err);
 		sdei_event_destroy(event_el);
 		pr_warn("Failed to register event %u: %d\n", event_num, err);
 		goto cpu_unlock;
 	}
 
+	event = &event_el->event;
 	spin_lock(&sdei_list_lock);
 	event_el->reregister = true;
 	spin_unlock(&sdei_list_lock);
@@ -627,7 +609,7 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	cpus_read_unlock();
 unlock:
 	mutex_unlock(&sdei_events_lock);
-	return err;
+	return event;
 }
 
 static int sdei_reregister_shared(void)
@@ -876,6 +858,7 @@ int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
 	u64 result;
 	u32 event_num;
 	sdei_event_callback *cb;
+	struct sdei_event *event;
 
 	if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES))
 		return -EOPNOTSUPP;
@@ -899,9 +882,13 @@ int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
 	else
 		cb = normal_cb;
 
-	err = sdei_event_register(event_num, cb, ghes);
-	if (!err)
-		err = sdei_event_enable(event_num);
+	event = sdei_event_register(event_num, cb, ghes);
+	if (IS_ERR(event)) {
+		err = PTR_ERR(event);
+		return err;
+	}
+
+	err = sdei_event_enable(event);
 
 	return err;
 }
@@ -911,22 +898,31 @@ int sdei_unregister_ghes(struct ghes *ghes)
 	int i;
 	int err;
 	u32 event_num = ghes->generic->notify.vector;
+	struct sdei_internal_event *event_el;
+	struct sdei_event *event;
 
 	might_sleep();
 
 	if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES))
 		return -EOPNOTSUPP;
 
+	mutex_lock(&sdei_events_lock);
+	event_el = sdei_event_find(event_num);
+	mutex_unlock(&sdei_events_lock);
+	if (!event_el)
+		return -ENOENT;
+
 	/*
 	 * The event may be running on another CPU. Disable it
 	 * to stop new events, then try to unregister a few times.
 	 */
-	err = sdei_event_disable(event_num);
+	event = &event_el->event;
+	err = sdei_event_disable(event);
 	if (err)
 		return err;
 
 	for (i = 0; i < 3; i++) {
-		err = sdei_event_unregister(event_num);
+		err = sdei_event_unregister(event);
 		if (err != -EINPROGRESS)
 			break;
 
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index d2464a18b6ff..2723a99937f3 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -32,16 +32,18 @@ struct sdei_event {
  * Register your callback to claim an event. The event must be described
  * by firmware.
  */
-int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg);
+struct sdei_event *sdei_event_register(u32 event_num,
+				       sdei_event_callback *cb,
+				       void *arg);
 
 /*
  * Calls to sdei_event_unregister() may return EINPROGRESS. Keep calling
  * it until it succeeds.
  */
-int sdei_event_unregister(u32 event_num);
+int sdei_event_unregister(struct sdei_event *event);
 
-int sdei_event_enable(u32 event_num);
-int sdei_event_disable(u32 event_num);
+int sdei_event_enable(struct sdei_event *event);
+int sdei_event_disable(struct sdei_event *event);
 
 /* GHES register/unregister helpers */
 int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
-- 
2.23.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 00/15] Refactor SDEI client driver

[Gavin Shan]

Hi James,

On 7/30/20 11:45 AM, Gavin Shan wrote:
> This series bases on 5.8-rc7 and refactors the SDEI client driver.
> It's the preparatory work of virtualizing SDEI afterwords. The series
> is organized as below:
> 
>     * PATCH[1-13] refactors and clean up the code. They shouldn't cause
>       any functional changes.
>     * PATCH[14-15] exposes "struct sdei_event", which will be dereferenced
>       on virtualizing SDEI. After it's applied, the SDEI event is identified
>       by the instance of "struct sdei_event" instead of event number.
> 
> The series can be checked out from:
> 
>      git@github.com:gwshan/linux.git
>      (branch: "sdei_client")
> 

I'm not sure if you need to review it as Jonathan almost finishes the
review. It would be nice to get this merged in 5.9.rc1/rc2 since the
subsequent series to support SDEI virtualization, which depends on this
series.

Thanks in advance for your time and comments :)

> Testing
> =======
> I have the SDEI virtualization code implemented as part of KVM module.
> With that, the SDEI event can be registered/unregistered/enabled/disabled.
> Also, the SDEI event can be injected from host and the guest handler runs
> properly.
> 
> The code can be found from:
> 
>     git@github.com:gwshan/linux.git
>     (branch: "sdei")
> 
> Changelog
> =========
> v4:
>     Rebase to last upstream kernel                                  (Gavin)
>     Use @event_el for SDEI internal event and use @event to cache
>     SDEI event if needed                                            (Jonathan)
>     Rename @se to @event for APIs                                   (Jonathan)
> v3:
>     Rebase to 5.8.rc7                                               (Gavin)
>     Pick rbs from Jonathan                                          (Gavin)
>     Correct spellings in commit logs                                (Jonathan)
>     Rename "out" to "unlock" tag                                    (Jonathan)
>     Keep the empty line in sdei_event_unregister()                  (Jonathan)
>     Drop tabs between type and field for struct sdei_crosscall_args (Jonathan)
>     Use smp_call_func_t for @fn argument in CPU callbacks           (Jonathan)
>     Split struct sdei_event into struct sdei_{internal,}_event      (Jonathan)
>     Remove last two patches and get it reviewed later               (Jonathan)
> v2:
>     Rebase to 5.8.rc6                                               (Gavin)
>     Improved changelog                                              (James/Gavin)
>     Split patches for easy review                                   (Gavin)
>     Drop changes to reorder variables                               (James)
>     Drop unnecessary (@regs removal) cleanup in sdei_event_create() (James)
>     Fix broken case for device-tree in sdei_init()                  (James)
> 
> Gavin Shan (15):
>    drivers/firmware/sdei: Remove sdei_is_err()
>    drivers/firmware/sdei: Common block for failing path in
>      sdei_event_create()
>    drivers/firmware/sdei: Retrieve event number from event instance
>    drivers/firmware/sdei: Avoid nested statements in sdei_init()
>    drivers/firmware/sdei: Unregister driver on error in sdei_init()
>    drivers/firmware/sdei: Remove duplicate check in sdei_get_conduit()
>    drivers/firmware/sdei: Remove Drop redundant error message in
>      sdei_probe()
>    drivers/firmware/sdei: Remove while loop in sdei_event_register()
>    drivers/firmware/sdei: Remove while loop in sdei_event_unregister()
>    drivers/firmware/sdei: Cleanup on cross call function
>    drivers/firmware/sdei: Introduce sdei_do_local_call()
>    drivers/firmware/sdei: Remove _sdei_event_register()
>    drivers/firmware/sdei: Remove _sdei_event_unregister()
>    drivers/firmware/sdei: Expose struct sdei_event
>    drivers/firmware/sdei: Identify event by struct sdei_event
> 
>   drivers/firmware/arm_sdei.c | 459 +++++++++++++++++-------------------
>   include/linux/arm_sdei.h    |  16 +-
>   2 files changed, 231 insertions(+), 244 deletions(-)
> 

Thanks,
Gavin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 14/15] drivers/firmware/sdei: Expose struct sdei_event

[Jonathan Cameron]

On Thu, 30 Jul 2020 11:45:29 +1000
Gavin Shan <gshan@redhat.com> wrote:

> This splits struct sdei_event into two structs: sdei_event and
> sdei_internal_event. The former one can be dereferenced from external
> module like arm64/kvm when SDEI virtualization is supported. The later
> one is used by the client driver only.
> 
> This also renames local variables from @event to @event_el if their
> type is "struct sdei_internal_event" to make the variable name a bit
> meaningful. Note that it's not applied to sdei_do_{local,cross}_call()
> because CROSSCALL_INIT stops us doing this unless much more changes
> are needed. I'm avoiding to do that.
> 
> Besides, @event is added as "struct sdei_event" to cache @event_el->event
> if it's accessed for multiple times in the particular function to reduce
> the code change size.
> 
> This shouldn't cause functional changes.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> v4: Rename variable @event to @event_el if it's internal event
Looks like a few cases were missed.  

>     Use @event to cache @event_el->event if it's used for multiple times

I'm not totally sure this splitting of the structure was worth the pain :(
(even though I suggested it :)
It perhaps makes things easier in the long run though.  Until we get the
user in place, it does look a bit over architected!

I'm fine with it, but others may well not be!

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>

> ---
>  drivers/firmware/arm_sdei.c | 160 ++++++++++++++++++++----------------
>  include/linux/arm_sdei.h    |   6 ++
>  2 files changed, 94 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 2678475940e6..f9827c096275 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -44,16 +44,14 @@ static asmlinkage void (*sdei_firmware_call)(unsigned long function_id,
>  /* entry point from firmware to arch asm code */
>  static unsigned long sdei_entry_point;
>  
> -struct sdei_event {
> +struct sdei_internal_event {
> +	struct sdei_event	event;
> +
>  	/* These three are protected by the sdei_list_lock */
>  	struct list_head	list;
>  	bool			reregister;
>  	bool			reenable;
>  
> -	u32			event_num;
> -	u8			type;
> -	u8			priority;
> -
>  	/* This pointer is handed to firmware as the event argument. */
>  	union {
>  		/* Shared events */
> @@ -73,7 +71,7 @@ static LIST_HEAD(sdei_list);
>  
>  /* Private events are registered/enabled via IPI passing one of these */
>  struct sdei_crosscall_args {
> -	struct sdei_event *event;
> +	struct sdei_internal_event *event;

Even though it is a bit painful should probably aim for consistent naming so
this and the other cases related to crosscall should be event_el;

>  	atomic_t errors;
>  	int first_error;
>  };
> @@ -86,7 +84,7 @@ struct sdei_crosscall_args {
>  	} while (0)
>  
>  static inline int sdei_do_local_call(smp_call_func_t fn,
> -				     struct sdei_event *event)
> +				     struct sdei_internal_event *event)
>  {
>  	struct sdei_crosscall_args arg;
>  
> @@ -97,7 +95,7 @@ static inline int sdei_do_local_call(smp_call_func_t fn,
>  }
>  
>  static inline int sdei_do_cross_call(smp_call_func_t fn,
> -				     struct sdei_event *event)
> +				     struct sdei_internal_event *event)
>  {
>  	struct sdei_crosscall_args arg;
>  
> @@ -162,16 +160,16 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
>  }
>  NOKPROBE_SYMBOL(invoke_sdei_fn);
>  
> -static struct sdei_event *sdei_event_find(u32 event_num)
> +static struct sdei_internal_event *sdei_event_find(u32 event_num)
>  {
> -	struct sdei_event *e, *found = NULL;
> +	struct sdei_internal_event *event_el, *found = NULL;
>  
>  	lockdep_assert_held(&sdei_events_lock);
>  
>  	spin_lock(&sdei_list_lock);
> -	list_for_each_entry(e, &sdei_list, list) {
> -		if (e->event_num == event_num) {
> -			found = e;
> +	list_for_each_entry(event_el, &sdei_list, list) {
> +		if (event_el->event.event_num == event_num) {
> +			found = event_el;
>  			break;
>  		}
>  	}
> @@ -193,24 +191,26 @@ static int sdei_api_event_get_info(u32 event, u32 info, u64 *result)
>  			      0, 0, result);
>  }
>  
> -static struct sdei_event *sdei_event_create(u32 event_num,
> -					    sdei_event_callback *cb,
> -					    void *cb_arg)
> +static struct sdei_internal_event *sdei_event_create(u32 event_num,
> +						     sdei_event_callback *cb,
> +						     void *cb_arg)
>  {
>  	int err;
>  	u64 result;
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;
>  	struct sdei_registered_event *reg;
>  
>  	lockdep_assert_held(&sdei_events_lock);
>  
> -	event = kzalloc(sizeof(*event), GFP_KERNEL);
> -	if (!event) {
> +	event_el = kzalloc(sizeof(*event_el), GFP_KERNEL);
> +	if (!event_el) {
>  		err = -ENOMEM;
>  		goto fail;
>  	}
>  
> -	INIT_LIST_HEAD(&event->list);
> +	event = &event_el->event;
> +	INIT_LIST_HEAD(&event_el->list);
>  	event->event_num = event_num;
>  
>  	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
> @@ -237,7 +237,7 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>  
>  		reg->callback = cb;
>  		reg->callback_arg = cb_arg;
> -		event->registered = reg;
> +		event_el->registered = reg;
>  	} else {
>  		int cpu;
>  		struct sdei_registered_event __percpu *regs;
> @@ -257,39 +257,39 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>  			reg->callback_arg = cb_arg;
>  		}
>  
> -		event->private_registered = regs;
> +		event_el->private_registered = regs;
>  	}
>  
>  	spin_lock(&sdei_list_lock);
> -	list_add(&event->list, &sdei_list);
> +	list_add(&event_el->list, &sdei_list);
>  	spin_unlock(&sdei_list_lock);
>  
> -	return event;
> +	return event_el;
>  
>  fail:
> -	kfree(event);
> +	kfree(event_el);
>  	return ERR_PTR(err);
>  }
>  
> -static void sdei_event_destroy_llocked(struct sdei_event *event)
> +static void sdei_event_destroy_llocked(struct sdei_internal_event *event_el)
>  {
>  	lockdep_assert_held(&sdei_events_lock);
>  	lockdep_assert_held(&sdei_list_lock);
>  
> -	list_del(&event->list);
> +	list_del(&event_el->list);
>  
> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
> -		kfree(event->registered);
> +	if (event_el->event.type == SDEI_EVENT_TYPE_SHARED)
> +		kfree(event_el->registered);
>  	else
> -		free_percpu(event->private_registered);
> +		free_percpu(event_el->private_registered);
>  
> -	kfree(event);
> +	kfree(event_el);
>  }
>  
> -static void sdei_event_destroy(struct sdei_event *event)
> +static void sdei_event_destroy(struct sdei_internal_event *event_el)
>  {
>  	spin_lock(&sdei_list_lock);
> -	sdei_event_destroy_llocked(event);
> +	sdei_event_destroy_llocked(event_el);
>  	spin_unlock(&sdei_list_lock);
>  }
>  
> @@ -392,7 +392,7 @@ static void _local_event_enable(void *data)
>  
>  	WARN_ON_ONCE(preemptible());
>  
> -	err = sdei_api_event_enable(arg->event->event_num);
> +	err = sdei_api_event_enable(arg->event->event.event_num);
>  
>  	sdei_cross_call_return(arg, err);
>  }
> @@ -400,25 +400,27 @@ static void _local_event_enable(void *data)
>  int sdei_event_enable(u32 event_num)
>  {
>  	int err = -EINVAL;
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;
>  
>  	mutex_lock(&sdei_events_lock);
> -	event = sdei_event_find(event_num);
> -	if (!event) {
> +	event_el = sdei_event_find(event_num);
> +	if (!event_el) {
>  		mutex_unlock(&sdei_events_lock);
>  		return -ENOENT;
>  	}
>  
>  
>  	cpus_read_lock();
> +	event = &event_el->event;
>  	if (event->type == SDEI_EVENT_TYPE_SHARED)
>  		err = sdei_api_event_enable(event->event_num);
>  	else
> -		err = sdei_do_cross_call(_local_event_enable, event);
> +		err = sdei_do_cross_call(_local_event_enable, event_el);
>  
>  	if (!err) {
>  		spin_lock(&sdei_list_lock);
> -		event->reenable = true;
> +		event_el->reenable = true;
>  		spin_unlock(&sdei_list_lock);
>  	}
>  	cpus_read_unlock();
> @@ -438,7 +440,7 @@ static void _ipi_event_disable(void *data)
>  	int err;
>  	struct sdei_crosscall_args *arg = data;
>  
> -	err = sdei_api_event_disable(arg->event->event_num);
> +	err = sdei_api_event_disable(arg->event->event.event_num);
>  
>  	sdei_cross_call_return(arg, err);
>  }
> @@ -446,23 +448,25 @@ static void _ipi_event_disable(void *data)
>  int sdei_event_disable(u32 event_num)
>  {
>  	int err = -EINVAL;
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;
>  
>  	mutex_lock(&sdei_events_lock);
> -	event = sdei_event_find(event_num);
> -	if (!event) {
> +	event_el = sdei_event_find(event_num);
> +	if (!event_el) {
>  		mutex_unlock(&sdei_events_lock);
>  		return -ENOENT;
>  	}
>  
>  	spin_lock(&sdei_list_lock);
> -	event->reenable = false;
> +	event_el->reenable = false;
>  	spin_unlock(&sdei_list_lock);
>  
> +	event = &event_el->event;
>  	if (event->type == SDEI_EVENT_TYPE_SHARED)
>  		err = sdei_api_event_disable(event->event_num);
>  	else
> -		err = sdei_do_cross_call(_ipi_event_disable, event);
> +		err = sdei_do_cross_call(_ipi_event_disable, event_el);
>  	mutex_unlock(&sdei_events_lock);
>  
>  	return err;
> @@ -482,7 +486,7 @@ static void _local_event_unregister(void *data)
>  
>  	WARN_ON_ONCE(preemptible());
>  
> -	err = sdei_api_event_unregister(arg->event->event_num);
> +	err = sdei_api_event_unregister(arg->event->event.event_num);
>  
>  	sdei_cross_call_return(arg, err);
>  }
> @@ -490,32 +494,34 @@ static void _local_event_unregister(void *data)
>  int sdei_event_unregister(u32 event_num)
>  {
>  	int err;
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;
>  
>  	WARN_ON(in_nmi());
>  
>  	mutex_lock(&sdei_events_lock);
> -	event = sdei_event_find(event_num);
> -	if (!event) {
> +	event_el = sdei_event_find(event_num);
> +	if (!event_el) {
>  		pr_warn("Event %u not registered\n", event_num);
>  		err = -ENOENT;
>  		goto unlock;
>  	}
>  
>  	spin_lock(&sdei_list_lock);
> -	event->reregister = false;
> -	event->reenable = false;
> +	event_el->reregister = false;
> +	event_el->reenable = false;
>  	spin_unlock(&sdei_list_lock);
>  
> +	event = &event_el->event;
>  	if (event->type == SDEI_EVENT_TYPE_SHARED)
>  		err = sdei_api_event_unregister(event->event_num);
>  	else
> -		err = sdei_do_cross_call(_local_event_unregister, event);
> +		err = sdei_do_cross_call(_local_event_unregister, event_el);
>  
>  	if (err)
>  		goto unlock;
>  
> -	sdei_event_destroy(event);
> +	sdei_event_destroy(event_el);
>  unlock:
>  	mutex_unlock(&sdei_events_lock);
>  
> @@ -529,11 +535,13 @@ int sdei_event_unregister(u32 event_num)
>  static int sdei_unregister_shared(void)
>  {
>  	int err = 0;
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;
>  
>  	mutex_lock(&sdei_events_lock);
>  	spin_lock(&sdei_list_lock);
> -	list_for_each_entry(event, &sdei_list, list) {
> +	list_for_each_entry(event_el, &sdei_list, list) {
> +		event = &event_el->event;
>  		if (event->type != SDEI_EVENT_TYPE_SHARED)
>  			continue;
>  
> @@ -565,8 +573,8 @@ static void _local_event_register(void *data)
>  	WARN_ON(preemptible());
>  
>  	reg = per_cpu_ptr(arg->event->private_registered, smp_processor_id());
> -	err = sdei_api_event_register(arg->event->event_num, sdei_entry_point,
> -				      reg, 0, 0);
> +	err = sdei_api_event_register(arg->event->event.event_num,
> +				      sdei_entry_point, reg, 0, 0);
>  
>  	sdei_cross_call_return(arg, err);
>  }
> @@ -574,6 +582,7 @@ static void _local_event_register(void *data)
>  int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  {
>  	int err;
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;
>  
>  	WARN_ON(in_nmi());
> @@ -585,33 +594,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  		goto unlock;
>  	}
>  
> -	event = sdei_event_create(event_num, cb, arg);
> -	if (IS_ERR(event)) {
> -		err = PTR_ERR(event);
> +	event_el = sdei_event_create(event_num, cb, arg);
> +	if (IS_ERR(event_el)) {
> +		err = PTR_ERR(event_el);
>  		pr_warn("Failed to create event %u: %d\n", event_num, err);
>  		goto unlock;
>  	}
>  
>  	cpus_read_lock();
> +	event = &event_el->event;
>  	if (event->type == SDEI_EVENT_TYPE_SHARED) {
>  		err = sdei_api_event_register(event->event_num,
>  					      sdei_entry_point,
> -					      event->registered,
> +					      event_el->registered,
>  					      SDEI_EVENT_REGISTER_RM_ANY, 0);
>  	} else {
> -		err = sdei_do_cross_call(_local_event_register, event);
> +		err = sdei_do_cross_call(_local_event_register, event_el);
>  		if (err)
> -			sdei_do_cross_call(_local_event_unregister, event);
> +			sdei_do_cross_call(_local_event_unregister, event_el);
>  	}
>  
>  	if (err) {
> -		sdei_event_destroy(event);
> +		sdei_event_destroy(event_el);
>  		pr_warn("Failed to register event %u: %d\n", event_num, err);
>  		goto cpu_unlock;
>  	}
>  
>  	spin_lock(&sdei_list_lock);
> -	event->reregister = true;
> +	event_el->reregister = true;
>  	spin_unlock(&sdei_list_lock);
>  cpu_unlock:
>  	cpus_read_unlock();
> @@ -623,27 +633,29 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  static int sdei_reregister_shared(void)
>  {
>  	int err = 0;
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;

Could reduce the scope by moving this in the loop.  Not that important though.

>  
>  	mutex_lock(&sdei_events_lock);
>  	spin_lock(&sdei_list_lock);
> -	list_for_each_entry(event, &sdei_list, list) {
> +	list_for_each_entry(event_el, &sdei_list, list) {
> +		event = &event_el->event;
>  		if (event->type != SDEI_EVENT_TYPE_SHARED)
>  			continue;
>  
> -		if (event->reregister) {
> +		if (event_el->reregister) {
>  			err = sdei_api_event_register(event->event_num,
> -					sdei_entry_point, event->registered,
> +					sdei_entry_point, event_el->registered,
>  					SDEI_EVENT_REGISTER_RM_ANY, 0);
>  			if (err) {
> -				sdei_event_destroy_llocked(event);
> +				sdei_event_destroy_llocked(event_el);
>  				pr_err("Failed to re-register event %u\n",
>  				       event->event_num);
>  				break;
>  			}
>  		}
>  
> -		if (event->reenable) {
> +		if (event_el->reenable) {
>  			err = sdei_api_event_enable(event->event_num);
>  			if (err) {
>  				pr_err("Failed to re-enable event %u\n",
> @@ -660,16 +672,18 @@ static int sdei_reregister_shared(void)
>  
>  static int sdei_cpuhp_down(unsigned int cpu)
>  {
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;
>  	int err;
>  
>  	/* un-register private events */
>  	spin_lock(&sdei_list_lock);
> -	list_for_each_entry(event, &sdei_list, list) {
> +	list_for_each_entry(event_el, &sdei_list, list) {
> +		event = &event_el->event;
>  		if (event->type == SDEI_EVENT_TYPE_SHARED)
>  			continue;
>  
> -		err = sdei_do_local_call(_local_event_unregister, event);
> +		err = sdei_do_local_call(_local_event_unregister, event_el);
>  		if (err) {
>  			pr_err("Failed to unregister event %u: %d\n",
>  			       event->event_num, err);
> @@ -682,25 +696,27 @@ static int sdei_cpuhp_down(unsigned int cpu)
>  
>  static int sdei_cpuhp_up(unsigned int cpu)
>  {
> +	struct sdei_internal_event *event_el;
>  	struct sdei_event *event;
>  	int err;
>  
>  	/* re-register/enable private events */
>  	spin_lock(&sdei_list_lock);
> -	list_for_each_entry(event, &sdei_list, list) {
> +	list_for_each_entry(event_el, &sdei_list, list) {
> +		event = &event_el->event;
>  		if (event->type == SDEI_EVENT_TYPE_SHARED)
>  			continue;
>  
> -		if (event->reregister) {
> -			err = sdei_do_local_call(_local_event_register, event);
> +		if (event_el->reregister) {
> +			err = sdei_do_local_call(_local_event_register, event_el);
>  			if (err) {
>  				pr_err("Failed to re-register event %u: %d\n",
>  				       event->event_num, err);
>  			}
>  		}
>  
> -		if (event->reenable) {
> -			err = sdei_do_local_call(_local_event_enable, event);
> +		if (event_el->reenable) {
> +			err = sdei_do_local_call(_local_event_enable, event_el);
>  			if (err) {
>  				pr_err("Failed to re-enable event %u: %d\n",
>  				       event->event_num, err);
> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> index 0a241c5c911d..d2464a18b6ff 100644
> --- a/include/linux/arm_sdei.h
> +++ b/include/linux/arm_sdei.h
> @@ -22,6 +22,12 @@
>   */
>  typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>  
> +struct sdei_event {
> +	u32			event_num;
> +	u8			type;
> +	u8			priority;
> +};
> +
>  /*
>   * Register your callback to claim an event. The event must be described
>   * by firmware.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 14/15] drivers/firmware/sdei: Expose struct sdei_event

[Gavin Shan]

Hi Jonathan,

On 7/30/20 8:54 PM, Jonathan Cameron wrote:
> On Thu, 30 Jul 2020 11:45:29 +1000
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> This splits struct sdei_event into two structs: sdei_event and
>> sdei_internal_event. The former one can be dereferenced from external
>> module like arm64/kvm when SDEI virtualization is supported. The later
>> one is used by the client driver only.
>>
>> This also renames local variables from @event to @event_el if their
>> type is "struct sdei_internal_event" to make the variable name a bit
>> meaningful. Note that it's not applied to sdei_do_{local,cross}_call()
>> because CROSSCALL_INIT stops us doing this unless much more changes
>> are needed. I'm avoiding to do that.
>>
>> Besides, @event is added as "struct sdei_event" to cache @event_el->event
>> if it's accessed for multiple times in the particular function to reduce
>> the code change size.
>>
>> This shouldn't cause functional changes.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> v4: Rename variable @event to @event_el if it's internal event
> Looks like a few cases were missed.
> 
>>      Use @event to cache @event_el->event if it's used for multiple times
> 
> I'm not totally sure this splitting of the structure was worth the pain :(
> (even though I suggested it :)
> It perhaps makes things easier in the long run though.  Until we get the
> user in place, it does look a bit over architected!
> 

Yeah, more changes are introduced because of this. However, I do think
it's a good idea to hide these fields and prevent them being exposed
in long run. So I think the effort is still worthy.

> I'm fine with it, but others may well not be!
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>
> 

Thank you for your comments and time on this. I think you've finished
the review on all the patches :)

>> ---
>>   drivers/firmware/arm_sdei.c | 160 ++++++++++++++++++++----------------
>>   include/linux/arm_sdei.h    |   6 ++
>>   2 files changed, 94 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 2678475940e6..f9827c096275 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -44,16 +44,14 @@ static asmlinkage void (*sdei_firmware_call)(unsigned long function_id,
>>   /* entry point from firmware to arch asm code */
>>   static unsigned long sdei_entry_point;
>>   
>> -struct sdei_event {
>> +struct sdei_internal_event {
>> +	struct sdei_event	event;
>> +
>>   	/* These three are protected by the sdei_list_lock */
>>   	struct list_head	list;
>>   	bool			reregister;
>>   	bool			reenable;
>>   
>> -	u32			event_num;
>> -	u8			type;
>> -	u8			priority;
>> -
>>   	/* This pointer is handed to firmware as the event argument. */
>>   	union {
>>   		/* Shared events */
>> @@ -73,7 +71,7 @@ static LIST_HEAD(sdei_list);
>>   
>>   /* Private events are registered/enabled via IPI passing one of these */
>>   struct sdei_crosscall_args {
>> -	struct sdei_event *event;
>> +	struct sdei_internal_event *event;
> 
> Even though it is a bit painful should probably aim for consistent naming so
> this and the other cases related to crosscall should be event_el;
> 

Agree, I will put this into my TODO list and post a patch to address it
later. I don't want introduce more changes into this patch, considering
the change size.

>>   	atomic_t errors;
>>   	int first_error;
>>   };
>> @@ -86,7 +84,7 @@ struct sdei_crosscall_args {
>>   	} while (0)
>>   
>>   static inline int sdei_do_local_call(smp_call_func_t fn,
>> -				     struct sdei_event *event)
>> +				     struct sdei_internal_event *event)
>>   {
>>   	struct sdei_crosscall_args arg;
>>   
>> @@ -97,7 +95,7 @@ static inline int sdei_do_local_call(smp_call_func_t fn,
>>   }
>>   
>>   static inline int sdei_do_cross_call(smp_call_func_t fn,
>> -				     struct sdei_event *event)
>> +				     struct sdei_internal_event *event)
>>   {
>>   	struct sdei_crosscall_args arg;
>>   
>> @@ -162,16 +160,16 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
>>   }
>>   NOKPROBE_SYMBOL(invoke_sdei_fn);
>>   
>> -static struct sdei_event *sdei_event_find(u32 event_num)
>> +static struct sdei_internal_event *sdei_event_find(u32 event_num)
>>   {
>> -	struct sdei_event *e, *found = NULL;
>> +	struct sdei_internal_event *event_el, *found = NULL;
>>   
>>   	lockdep_assert_held(&sdei_events_lock);
>>   
>>   	spin_lock(&sdei_list_lock);
>> -	list_for_each_entry(e, &sdei_list, list) {
>> -		if (e->event_num == event_num) {
>> -			found = e;
>> +	list_for_each_entry(event_el, &sdei_list, list) {
>> +		if (event_el->event.event_num == event_num) {
>> +			found = event_el;
>>   			break;
>>   		}
>>   	}
>> @@ -193,24 +191,26 @@ static int sdei_api_event_get_info(u32 event, u32 info, u64 *result)
>>   			      0, 0, result);
>>   }
>>   
>> -static struct sdei_event *sdei_event_create(u32 event_num,
>> -					    sdei_event_callback *cb,
>> -					    void *cb_arg)
>> +static struct sdei_internal_event *sdei_event_create(u32 event_num,
>> +						     sdei_event_callback *cb,
>> +						     void *cb_arg)
>>   {
>>   	int err;
>>   	u64 result;
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
>>   	struct sdei_registered_event *reg;
>>   
>>   	lockdep_assert_held(&sdei_events_lock);
>>   
>> -	event = kzalloc(sizeof(*event), GFP_KERNEL);
>> -	if (!event) {
>> +	event_el = kzalloc(sizeof(*event_el), GFP_KERNEL);
>> +	if (!event_el) {
>>   		err = -ENOMEM;
>>   		goto fail;
>>   	}
>>   
>> -	INIT_LIST_HEAD(&event->list);
>> +	event = &event_el->event;
>> +	INIT_LIST_HEAD(&event_el->list);
>>   	event->event_num = event_num;
>>   
>>   	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
>> @@ -237,7 +237,7 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>>   
>>   		reg->callback = cb;
>>   		reg->callback_arg = cb_arg;
>> -		event->registered = reg;
>> +		event_el->registered = reg;
>>   	} else {
>>   		int cpu;
>>   		struct sdei_registered_event __percpu *regs;
>> @@ -257,39 +257,39 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>>   			reg->callback_arg = cb_arg;
>>   		}
>>   
>> -		event->private_registered = regs;
>> +		event_el->private_registered = regs;
>>   	}
>>   
>>   	spin_lock(&sdei_list_lock);
>> -	list_add(&event->list, &sdei_list);
>> +	list_add(&event_el->list, &sdei_list);
>>   	spin_unlock(&sdei_list_lock);
>>   
>> -	return event;
>> +	return event_el;
>>   
>>   fail:
>> -	kfree(event);
>> +	kfree(event_el);
>>   	return ERR_PTR(err);
>>   }
>>   
>> -static void sdei_event_destroy_llocked(struct sdei_event *event)
>> +static void sdei_event_destroy_llocked(struct sdei_internal_event *event_el)
>>   {
>>   	lockdep_assert_held(&sdei_events_lock);
>>   	lockdep_assert_held(&sdei_list_lock);
>>   
>> -	list_del(&event->list);
>> +	list_del(&event_el->list);
>>   
>> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
>> -		kfree(event->registered);
>> +	if (event_el->event.type == SDEI_EVENT_TYPE_SHARED)
>> +		kfree(event_el->registered);
>>   	else
>> -		free_percpu(event->private_registered);
>> +		free_percpu(event_el->private_registered);
>>   
>> -	kfree(event);
>> +	kfree(event_el);
>>   }
>>   
>> -static void sdei_event_destroy(struct sdei_event *event)
>> +static void sdei_event_destroy(struct sdei_internal_event *event_el)
>>   {
>>   	spin_lock(&sdei_list_lock);
>> -	sdei_event_destroy_llocked(event);
>> +	sdei_event_destroy_llocked(event_el);
>>   	spin_unlock(&sdei_list_lock);
>>   }
>>   
>> @@ -392,7 +392,7 @@ static void _local_event_enable(void *data)
>>   
>>   	WARN_ON_ONCE(preemptible());
>>   
>> -	err = sdei_api_event_enable(arg->event->event_num);
>> +	err = sdei_api_event_enable(arg->event->event.event_num);
>>   
>>   	sdei_cross_call_return(arg, err);
>>   }
>> @@ -400,25 +400,27 @@ static void _local_event_enable(void *data)
>>   int sdei_event_enable(u32 event_num)
>>   {
>>   	int err = -EINVAL;
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
>>   
>>   	mutex_lock(&sdei_events_lock);
>> -	event = sdei_event_find(event_num);
>> -	if (!event) {
>> +	event_el = sdei_event_find(event_num);
>> +	if (!event_el) {
>>   		mutex_unlock(&sdei_events_lock);
>>   		return -ENOENT;
>>   	}
>>   
>>   
>>   	cpus_read_lock();
>> +	event = &event_el->event;
>>   	if (event->type == SDEI_EVENT_TYPE_SHARED)
>>   		err = sdei_api_event_enable(event->event_num);
>>   	else
>> -		err = sdei_do_cross_call(_local_event_enable, event);
>> +		err = sdei_do_cross_call(_local_event_enable, event_el);
>>   
>>   	if (!err) {
>>   		spin_lock(&sdei_list_lock);
>> -		event->reenable = true;
>> +		event_el->reenable = true;
>>   		spin_unlock(&sdei_list_lock);
>>   	}
>>   	cpus_read_unlock();
>> @@ -438,7 +440,7 @@ static void _ipi_event_disable(void *data)
>>   	int err;
>>   	struct sdei_crosscall_args *arg = data;
>>   
>> -	err = sdei_api_event_disable(arg->event->event_num);
>> +	err = sdei_api_event_disable(arg->event->event.event_num);
>>   
>>   	sdei_cross_call_return(arg, err);
>>   }
>> @@ -446,23 +448,25 @@ static void _ipi_event_disable(void *data)
>>   int sdei_event_disable(u32 event_num)
>>   {
>>   	int err = -EINVAL;
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
>>   
>>   	mutex_lock(&sdei_events_lock);
>> -	event = sdei_event_find(event_num);
>> -	if (!event) {
>> +	event_el = sdei_event_find(event_num);
>> +	if (!event_el) {
>>   		mutex_unlock(&sdei_events_lock);
>>   		return -ENOENT;
>>   	}
>>   
>>   	spin_lock(&sdei_list_lock);
>> -	event->reenable = false;
>> +	event_el->reenable = false;
>>   	spin_unlock(&sdei_list_lock);
>>   
>> +	event = &event_el->event;
>>   	if (event->type == SDEI_EVENT_TYPE_SHARED)
>>   		err = sdei_api_event_disable(event->event_num);
>>   	else
>> -		err = sdei_do_cross_call(_ipi_event_disable, event);
>> +		err = sdei_do_cross_call(_ipi_event_disable, event_el);
>>   	mutex_unlock(&sdei_events_lock);
>>   
>>   	return err;
>> @@ -482,7 +486,7 @@ static void _local_event_unregister(void *data)
>>   
>>   	WARN_ON_ONCE(preemptible());
>>   
>> -	err = sdei_api_event_unregister(arg->event->event_num);
>> +	err = sdei_api_event_unregister(arg->event->event.event_num);
>>   
>>   	sdei_cross_call_return(arg, err);
>>   }
>> @@ -490,32 +494,34 @@ static void _local_event_unregister(void *data)
>>   int sdei_event_unregister(u32 event_num)
>>   {
>>   	int err;
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
>>   
>>   	WARN_ON(in_nmi());
>>   
>>   	mutex_lock(&sdei_events_lock);
>> -	event = sdei_event_find(event_num);
>> -	if (!event) {
>> +	event_el = sdei_event_find(event_num);
>> +	if (!event_el) {
>>   		pr_warn("Event %u not registered\n", event_num);
>>   		err = -ENOENT;
>>   		goto unlock;
>>   	}
>>   
>>   	spin_lock(&sdei_list_lock);
>> -	event->reregister = false;
>> -	event->reenable = false;
>> +	event_el->reregister = false;
>> +	event_el->reenable = false;
>>   	spin_unlock(&sdei_list_lock);
>>   
>> +	event = &event_el->event;
>>   	if (event->type == SDEI_EVENT_TYPE_SHARED)
>>   		err = sdei_api_event_unregister(event->event_num);
>>   	else
>> -		err = sdei_do_cross_call(_local_event_unregister, event);
>> +		err = sdei_do_cross_call(_local_event_unregister, event_el);
>>   
>>   	if (err)
>>   		goto unlock;
>>   
>> -	sdei_event_destroy(event);
>> +	sdei_event_destroy(event_el);
>>   unlock:
>>   	mutex_unlock(&sdei_events_lock);
>>   
>> @@ -529,11 +535,13 @@ int sdei_event_unregister(u32 event_num)
>>   static int sdei_unregister_shared(void)
>>   {
>>   	int err = 0;
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
>>   
>>   	mutex_lock(&sdei_events_lock);
>>   	spin_lock(&sdei_list_lock);
>> -	list_for_each_entry(event, &sdei_list, list) {
>> +	list_for_each_entry(event_el, &sdei_list, list) {
>> +		event = &event_el->event;
>>   		if (event->type != SDEI_EVENT_TYPE_SHARED)
>>   			continue;
>>   
>> @@ -565,8 +573,8 @@ static void _local_event_register(void *data)
>>   	WARN_ON(preemptible());
>>   
>>   	reg = per_cpu_ptr(arg->event->private_registered, smp_processor_id());
>> -	err = sdei_api_event_register(arg->event->event_num, sdei_entry_point,
>> -				      reg, 0, 0);
>> +	err = sdei_api_event_register(arg->event->event.event_num,
>> +				      sdei_entry_point, reg, 0, 0);
>>   
>>   	sdei_cross_call_return(arg, err);
>>   }
>> @@ -574,6 +582,7 @@ static void _local_event_register(void *data)
>>   int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   {
>>   	int err;
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
>>   
>>   	WARN_ON(in_nmi());
>> @@ -585,33 +594,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   		goto unlock;
>>   	}
>>   
>> -	event = sdei_event_create(event_num, cb, arg);
>> -	if (IS_ERR(event)) {
>> -		err = PTR_ERR(event);
>> +	event_el = sdei_event_create(event_num, cb, arg);
>> +	if (IS_ERR(event_el)) {
>> +		err = PTR_ERR(event_el);
>>   		pr_warn("Failed to create event %u: %d\n", event_num, err);
>>   		goto unlock;
>>   	}
>>   
>>   	cpus_read_lock();
>> +	event = &event_el->event;
>>   	if (event->type == SDEI_EVENT_TYPE_SHARED) {
>>   		err = sdei_api_event_register(event->event_num,
>>   					      sdei_entry_point,
>> -					      event->registered,
>> +					      event_el->registered,
>>   					      SDEI_EVENT_REGISTER_RM_ANY, 0);
>>   	} else {
>> -		err = sdei_do_cross_call(_local_event_register, event);
>> +		err = sdei_do_cross_call(_local_event_register, event_el);
>>   		if (err)
>> -			sdei_do_cross_call(_local_event_unregister, event);
>> +			sdei_do_cross_call(_local_event_unregister, event_el);
>>   	}
>>   
>>   	if (err) {
>> -		sdei_event_destroy(event);
>> +		sdei_event_destroy(event_el);
>>   		pr_warn("Failed to register event %u: %d\n", event_num, err);
>>   		goto cpu_unlock;
>>   	}
>>   
>>   	spin_lock(&sdei_list_lock);
>> -	event->reregister = true;
>> +	event_el->reregister = true;
>>   	spin_unlock(&sdei_list_lock);
>>   cpu_unlock:
>>   	cpus_read_unlock();
>> @@ -623,27 +633,29 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   static int sdei_reregister_shared(void)
>>   {
>>   	int err = 0;
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
> 
> Could reduce the scope by moving this in the loop.  Not that important though.
> 

Yeah, but it's fine not to move it.

>>   
>>   	mutex_lock(&sdei_events_lock);
>>   	spin_lock(&sdei_list_lock);
>> -	list_for_each_entry(event, &sdei_list, list) {
>> +	list_for_each_entry(event_el, &sdei_list, list) {
>> +		event = &event_el->event;
>>   		if (event->type != SDEI_EVENT_TYPE_SHARED)
>>   			continue;
>>   
>> -		if (event->reregister) {
>> +		if (event_el->reregister) {
>>   			err = sdei_api_event_register(event->event_num,
>> -					sdei_entry_point, event->registered,
>> +					sdei_entry_point, event_el->registered,
>>   					SDEI_EVENT_REGISTER_RM_ANY, 0);
>>   			if (err) {
>> -				sdei_event_destroy_llocked(event);
>> +				sdei_event_destroy_llocked(event_el);
>>   				pr_err("Failed to re-register event %u\n",
>>   				       event->event_num);
>>   				break;
>>   			}
>>   		}
>>   
>> -		if (event->reenable) {
>> +		if (event_el->reenable) {
>>   			err = sdei_api_event_enable(event->event_num);
>>   			if (err) {
>>   				pr_err("Failed to re-enable event %u\n",
>> @@ -660,16 +672,18 @@ static int sdei_reregister_shared(void)
>>   
>>   static int sdei_cpuhp_down(unsigned int cpu)
>>   {
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
>>   	int err;
>>   
>>   	/* un-register private events */
>>   	spin_lock(&sdei_list_lock);
>> -	list_for_each_entry(event, &sdei_list, list) {
>> +	list_for_each_entry(event_el, &sdei_list, list) {
>> +		event = &event_el->event;
>>   		if (event->type == SDEI_EVENT_TYPE_SHARED)
>>   			continue;
>>   
>> -		err = sdei_do_local_call(_local_event_unregister, event);
>> +		err = sdei_do_local_call(_local_event_unregister, event_el);
>>   		if (err) {
>>   			pr_err("Failed to unregister event %u: %d\n",
>>   			       event->event_num, err);
>> @@ -682,25 +696,27 @@ static int sdei_cpuhp_down(unsigned int cpu)
>>   
>>   static int sdei_cpuhp_up(unsigned int cpu)
>>   {
>> +	struct sdei_internal_event *event_el;
>>   	struct sdei_event *event;
>>   	int err;
>>   
>>   	/* re-register/enable private events */
>>   	spin_lock(&sdei_list_lock);
>> -	list_for_each_entry(event, &sdei_list, list) {
>> +	list_for_each_entry(event_el, &sdei_list, list) {
>> +		event = &event_el->event;
>>   		if (event->type == SDEI_EVENT_TYPE_SHARED)
>>   			continue;
>>   
>> -		if (event->reregister) {
>> -			err = sdei_do_local_call(_local_event_register, event);
>> +		if (event_el->reregister) {
>> +			err = sdei_do_local_call(_local_event_register, event_el);
>>   			if (err) {
>>   				pr_err("Failed to re-register event %u: %d\n",
>>   				       event->event_num, err);
>>   			}
>>   		}
>>   
>> -		if (event->reenable) {
>> -			err = sdei_do_local_call(_local_event_enable, event);
>> +		if (event_el->reenable) {
>> +			err = sdei_do_local_call(_local_event_enable, event_el);
>>   			if (err) {
>>   				pr_err("Failed to re-enable event %u: %d\n",
>>   				       event->event_num, err);
>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
>> index 0a241c5c911d..d2464a18b6ff 100644
>> --- a/include/linux/arm_sdei.h
>> +++ b/include/linux/arm_sdei.h
>> @@ -22,6 +22,12 @@
>>    */
>>   typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>>   
>> +struct sdei_event {
>> +	u32			event_num;
>> +	u8			type;
>> +	u8			priority;
>> +};
>> +
>>   /*
>>    * Register your callback to claim an event. The event must be described
>>    * by firmware.

Thanks,
Gavin
  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 00/15] Refactor SDEI client driver

[Gavin Shan]

Hi James,

On 7/30/20 6:03 PM, Gavin Shan wrote:
> On 7/30/20 11:45 AM, Gavin Shan wrote:
>> This series bases on 5.8-rc7 and refactors the SDEI client driver.
>> It's the preparatory work of virtualizing SDEI afterwords. The series
>> is organized as below:
>>
>>     * PATCH[1-13] refactors and clean up the code. They shouldn't cause
>>       any functional changes.
>>     * PATCH[14-15] exposes "struct sdei_event", which will be dereferenced
>>       on virtualizing SDEI. After it's applied, the SDEI event is identified
>>       by the instance of "struct sdei_event" instead of event number.
>>
>> The series can be checked out from:
>>
>>      git@github.com:gwshan/linux.git
>>      (branch: "sdei_client")
>>
> 
> I'm not sure if you need to review it as Jonathan almost finishes the
> review. It would be nice to get this merged in 5.9.rc1/rc2 since the
> subsequent series to support SDEI virtualization, which depends on this
> series.
> 
> Thanks in advance for your time and comments :)
> 

A kindly ping for this series :)

[...]

Thanks,
Gavin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/15] drivers/firmware/sdei: Common block for failing path in sdei_event_create()

[James Morse]

Hi Gavin,

(Suject Nit: almost all of the commits to this file begin 'firmware: arm_sdei:')

On 30/07/2020 02:45, Gavin Shan wrote:
> There are multiple calls of kfree(event) in the failing path of
> sdei_event_create() to free the SDEI event.

> It means we need to
> call it again when adding more code in the failing path. It's
> prone to miss doing that and introduce memory leakage.

If there is another things that needs adding here, sure.
(otherwise there is no unwinding that needs to happen.)


> This introduces common block for failing path in sdei_event_create()
> to resolve the issue. This shouldn't cause functional changes.

Acked-by: James Morse <james.morse@arm.com>



Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/15] drivers/firmware/sdei: Retrieve event number from event instance

[James Morse]

On 30/07/2020 02:45, Gavin Shan wrote:
> In sdei_event_create(), the event number is retrieved from the
> variable @event_num for the shared event. The event number was
> stored in the event instance. So we can fetch it from the event
> instance, similar to what we're doing for the private event.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

(I don't think patches like this are worth the effort)

Acked-by: James Morse <james.morse@arm.com>


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 04/15] drivers/firmware/sdei: Avoid nested statements in sdei_init()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> In sdei_init(), the nested statements can be avoided by bailing
> on error from platform_driver_register() or absent ACPI SDEI table.
> With it, the code looks a bit more readable.

This is how the code ended up after the DT pattern was fixed out of existence.

Thanks for the cleanup.
Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/15] drivers/firmware/sdei: Unregister driver on error in sdei_init()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> The platform driver needs to be unregistered on error to create the
> platform device, so that the state is restored to original one.

Why is this important? Is it just symmetry? 'needs' causes me to look at this as a bug-fix.

DT systems leave a dangling platform device in this case. Is that a problem?
See commit 3aa0582fdb82 ("of: platform: populate /firmware/ node from
of_platform_default_populate_init()").

On DT systems, sdei_init() doesn't create the platform device, so it doesn't remove it
either. ACPI leaves it dangling because ... why should ACPI behave differently?


> Besides, the errno (@ret) should be updated acccording in that case.

(according)


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index c9f2bedfb6b5..909f27abf8a7 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -1090,9 +1090,12 @@ static int __init sdei_init(void)
>  
>  	pdev = platform_device_register_simple(sdei_driver.driver.name,
>  					       0, NULL, 0);
> -	if (IS_ERR(pdev))
> -		pr_info("Failed to register ACPI:SDEI platform device %ld\n",
> -			PTR_ERR(pdev));
> +	if (IS_ERR(pdev)) {
> +		ret = PTR_ERR(pdev);
> +		platform_driver_unregister(&sdei_driver);
> +		pr_info("Failed to register ACPI:SDEI platform device %d\n",
> +			ret);
> +	}
>  
>  	return ret;
>  }

If the argument here is about symmetry, sure. Please stuff sneak the word symmetry into
the commit message - I keep reading this as if its a bug. Its probably worth a comment in
the commit message that its only like this for ACPI. Previously the motivation was to keep
these things as similar as possible.

With that:
Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 06/15] drivers/firmware/sdei: Remove duplicate check in sdei_get_conduit()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> The following two checks are duplicate because @acpi_disabled doesn't
> depend on CONFIG_ACPI. So the duplicate check (IS_ENABLED(CONFIG_ACPI))
> can be dropped. More details is provided to keep the commit log complete:
> 
>    * @acpi_disabled is defined in arch/arm64/kernel/acpi.c when
>      CONFIG_ACPI is enabled.
>    * @acpi_disabled in defined in include/acpi.h when CONFIG_ACPI
>      is disabled.

Acked-by: James Morse <james.morse@arm.com>


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/15] drivers/firmware/sdei: Remove Drop redundant error message in sdei_probe()

[James Morse]

Hi Gavin,

(Subject nit: Drop should have a lower case d)

On 30/07/2020 02:45, Gavin Shan wrote:
> This removes the redundant error message in sdei_probe() because
> the case can be identified from the errno in next error message.

... assuming anyone knows what -95 means. This is meant to be helpful, to tell people they
have incomplete firmware.


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 240c06ae7bfe..03b1179da9b4 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -982,8 +982,6 @@ static int sdei_probe(struct platform_device *pdev)
>  		return 0;
>  
>  	err = sdei_api_get_version(&ver);
> -	if (err == -EOPNOTSUPP)
> -		pr_err("advertised but not implemented in platform firmware\n");
>  	if (err) {
>  		pr_err("Failed to get SDEI version: %d\n", err);
>  		sdei_mark_interface_broken();
> 

Given the firmware implementation is upstream in ATF, I guess no-one will create their own.

Acked-by: James Morse <james.morse@arm.com>


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 08/15] drivers/firmware/sdei: Remove while loop in sdei_event_register()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> This removes the unnecessary while loop in sdei_event_register().

Did you notice how this code doesn't need to unwind anything to clean up?
It only unlocks the mutex, and the indentation tells you it always does that.


> This shouldn't cause any functional changes.

Why is it better? This complicates the flow by jumping around with goto.

If the unwinding were necessary, I agree this would be clearer, but as its not ... why do
we need to make this harder to read?


Thanks,

James


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 03b1179da9b4..d04329f98922 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -590,36 +590,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  	WARN_ON(in_nmi());
>  
>  	mutex_lock(&sdei_events_lock);
> -	do {
> -		if (sdei_event_find(event_num)) {
> -			pr_warn("Event %u already registered\n", event_num);
> -			err = -EBUSY;
> -			break;
> -		}
> +	if (sdei_event_find(event_num)) {
> +		pr_warn("Event %u already registered\n", event_num);
> +		err = -EBUSY;
> +		goto unlock;
> +	}
>  
> -		event = sdei_event_create(event_num, cb, arg);
> -		if (IS_ERR(event)) {
> -			err = PTR_ERR(event);
> -			pr_warn("Failed to create event %u: %d\n", event_num,
> -				err);
> -			break;
> -		}
> +	event = sdei_event_create(event_num, cb, arg);
> +	if (IS_ERR(event)) {
> +		err = PTR_ERR(event);
> +		pr_warn("Failed to create event %u: %d\n", event_num, err);
> +		goto unlock;
> +	}
>  
> -		cpus_read_lock();
> -		err = _sdei_event_register(event);
> -		if (err) {
> -			sdei_event_destroy(event);
> -			pr_warn("Failed to register event %u: %d\n", event_num,
> -				err);
> -		} else {
> -			spin_lock(&sdei_list_lock);
> -			event->reregister = true;
> -			spin_unlock(&sdei_list_lock);
> -		}
> -		cpus_read_unlock();
> -	} while (0);
> -	mutex_unlock(&sdei_events_lock);
> +	cpus_read_lock();
> +	err = _sdei_event_register(event);
> +	if (err) {
> +		sdei_event_destroy(event);
> +		pr_warn("Failed to register event %u: %d\n", event_num, err);
> +		goto cpu_unlock;
> +	}
>  
> +	spin_lock(&sdei_list_lock);
> +	event->reregister = true;
> +	spin_unlock(&sdei_list_lock);
> +cpu_unlock:
> +	cpus_read_unlock();
> +unlock:
> +	mutex_unlock(&sdei_events_lock);
>  	return err;
>  }
>  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 10/15] drivers/firmware/sdei: Cleanup on cross call function

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> This applies cleanup on the cross call functions, no functional
> changes are introduced:


>    * Refactor CROSSCALL_INIT to use "do { ... } while (0)" to be
>      compatible with scripts/checkpatch.pl

What does this mean?


>    * Use smp_call_func_t for @fn argument in sdei_do_cross_call()
>    * Remove unnecessary space before @event in sdei_do_cross_call()


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index c8e894098c56..5560c8880631 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -78,11 +78,15 @@ struct sdei_crosscall_args {
>  	int first_error;
>  };
>  
> -#define CROSSCALL_INIT(arg, event)	(arg.event = event, \
> -					 arg.first_error = 0, \
> -					 atomic_set(&arg.errors, 0))
> -
> -static inline int sdei_do_cross_call(void *fn, struct sdei_event * event)
> +#define CROSSCALL_INIT(arg, event)		\
> +	do {					\
> +		arg.event = event;		\
> +		arg.first_error = 0;		\
> +		atomic_set(&arg.errors, 0);	\
> +	} while (0)

Huh, I'm surprised I didn't write it like that the first time!


> +static inline int sdei_do_cross_call(smp_call_func_t fn,
> +				     struct sdei_event *event)
>  {
>  	struct sdei_crosscall_args arg;


With some kind of motivation for the change, instead of "checkpatch made me do it":
Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/15] drivers/firmware/sdei: Introduce sdei_do_local_call()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> During the CPU hotplug, the private events are registered, enabled
> or unregistered on the specific CPU. It repeats the same steps:
> initializing cross call argument, make function call on local CPU,
> check the returned error.
> 
> This introduces sdei_do_local_call() to cover the first steps. The
> other benefit is to make CROSSCALL_INIT and struct sdei_crosscall_args
> are only visible to sddi_do_{cross, local}_call().

(sdei)

These things are abusing the IPI call, I agree it makes sense to wrap this weird boiler
plate up in a helper.

Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 12/15] drivers/firmware/sdei: Remove _sdei_event_register()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> The function _sdei_event_register() is called by sdei_event_register()
> and sdei_device_thaw() as the following functional call chain shows.
> _sdei_event_register() covers the shared and private events, but
> sdei_device_thaw() only covers the shared events. So the logic to
> cover the private events in _sdei_event_register() isn't needed by
> sdei_device_thaw().
> 
> Similarly, sdei_reregister_event_llocked() covers the shared and
> private events in the regard of reenablement. The logic to cover
> the private events isn't needed by sdei_device_thaw() either.

These were added for code-reuse instead of duplication, but from the diff-stat it looks
like it cut-in too high up and would have been simpler not to. (I remember cpuhp being a
nightmare)


>    sdei_event_register          sdei_device_thaw
>       _sdei_event_register         sdei_reregister_shared
>                                       sdei_reregister_event_llocked
>                                          _sdei_event_register
> 
> This removes _sdei_event_register() and sdei_reregister_event_llocked().
> Their logic is moved to sdei_event_register() and sdei_reregister_shared().
> This shouldn't cause any logicial changes.

(logical)


>  drivers/firmware/arm_sdei.c | 77 ++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 49 deletions(-)

>  static int sdei_reregister_shared(void)
>  {
>  	int err = 0;
> @@ -674,9 +638,24 @@ static int sdei_reregister_shared(void)
>  			continue;
>  
>  		if (event->reregister) {
> -			err = sdei_reregister_event_llocked(event);
> -			if (err)
> +			err = sdei_api_event_register(event->event_num,
> +					sdei_entry_point, event->registered,
> +					SDEI_EVENT_REGISTER_RM_ANY, 0);
> +			if (err) {
> +				sdei_event_destroy_llocked(event);

> +				pr_err("Failed to re-register event %u\n",
> +				       event->event_num);

sdei_event_destroy_llocked() just free()'d event. You need to print the error first.


>  				break;
> +			}
> +		}
> +
> +		if (event->reenable) {
> +			err = sdei_api_event_enable(event->event_num);
> +			if (err) {
> +				pr_err("Failed to re-enable event %u\n",
> +				       event->event_num);
> +				break;
> +			}
>  		}
>  	}
>  	spin_unlock(&sdei_list_lock);


With the use-after-free fixed:
Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 13/15] drivers/firmware/sdei: Remove _sdei_event_unregister()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> _sdei_event_unregister() is called by sdei_event_unregister() and
> sdei_device_freeze(). _sdei_event_unregister() covers the shared
> and private events, but sdei_device_freeze() only covers the shared
> events. So the logic to cover the private events isn't needed by
> sdei_device_freeze().
> 
>    sdei_event_unregister        sdei_device_freeze
>       _sdei_event_unregister       sdei_unregister_shared
>                                      _sdei_event_unregister
> 
> This removes _sdei_event_unregister(). Its logic is moved to its
> callers accordingly. This shouldn't cause any logicial changes.

(logical)


Reviewed-by: James Morse <james.morse@arm.com>


Thanks

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 14/15] drivers/firmware/sdei: Expose struct sdei_event

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> This splits struct sdei_event into two structs: sdei_event and
> sdei_internal_event. The former one can be dereferenced from external
> module like arm64/kvm when SDEI virtualization is supported. The later
> one is used by the client driver only.

This should be part of the series that need its.

I don't see any reason to share this as it risks muddling events the OS has as a client,
with those its offering as a hypervisor.

I doubt any KVM support would need to have anything to do with the client support.

The same argument goes for patch 15.


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/15] drivers/firmware/sdei: Unregister driver on error in sdei_init()

[Gavin Shan]

Hi James,

On 9/19/20 2:12 AM, James Morse wrote:
> On 30/07/2020 02:45, Gavin Shan wrote:
>> The platform driver needs to be unregistered on error to create the
>> platform device, so that the state is restored to original one.
> 
> Why is this important? Is it just symmetry? 'needs' causes me to look at this as a bug-fix.
> 
> DT systems leave a dangling platform device in this case. Is that a problem?
> See commit 3aa0582fdb82 ("of: platform: populate /firmware/ node from
> of_platform_default_populate_init()").
> 
> On DT systems, sdei_init() doesn't create the platform device, so it doesn't remove it
> either. ACPI leaves it dangling because ... why should ACPI behave differently?
> 

It's all about symmetry because the platform driver has the owner,
but platform device doesn't have it. It means it's fine to keep
dangling device, but it's not reasonable to keep dangling driver.
However, this patch isn't a big deal though.

> 
>> Besides, the errno (@ret) should be updated acccording in that case.
> 
> (according)
> 

Thanks. It's fixed to "accordingly in this case" in next revision.

> 
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index c9f2bedfb6b5..909f27abf8a7 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -1090,9 +1090,12 @@ static int __init sdei_init(void)
>>   
>>   	pdev = platform_device_register_simple(sdei_driver.driver.name,
>>   					       0, NULL, 0);
>> -	if (IS_ERR(pdev))
>> -		pr_info("Failed to register ACPI:SDEI platform device %ld\n",
>> -			PTR_ERR(pdev));
>> +	if (IS_ERR(pdev)) {
>> +		ret = PTR_ERR(pdev);
>> +		platform_driver_unregister(&sdei_driver);
>> +		pr_info("Failed to register ACPI:SDEI platform device %d\n",
>> +			ret);
>> +	}
>>   
>>   	return ret;
>>   }
> 
> If the argument here is about symmetry, sure. Please stuff sneak the word symmetry into
> the commit message - I keep reading this as if its a bug. Its probably worth a comment in
> the commit message that its only like this for ACPI. Previously the motivation was to keep
> these things as similar as possible.
> 
> With that:
> Reviewed-by: James Morse <james.morse@arm.com>
> 

Sure. I will have something like below in the change log:

---

The SDEI platform device is created from device-tree node or ACPI
(SDEI) table. For the later case, the platform device is created
explicitly by this module. It'd better to unregister the driver on
failure to create the device to keep the symmetry. The driver, owned
by this module, isn't needed if the device isn't existing.

Besides, the errno (@ret) should be updated accordingly in this
case.

Cheers,
Gavin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 08/15] drivers/firmware/sdei: Remove while loop in sdei_event_register()

[Gavin Shan]

Hi James,

On 9/19/20 2:13 AM, James Morse wrote:
> On 30/07/2020 02:45, Gavin Shan wrote:
>> This removes the unnecessary while loop in sdei_event_register().
> 
> Did you notice how this code doesn't need to unwind anything to clean up?
> It only unlocks the mutex, and the indentation tells you it always does that.
> 

Yes, I noticed it.

> 
>> This shouldn't cause any functional changes.
> 
> Why is it better? This complicates the flow by jumping around with goto.
> 
> If the unwinding were necessary, I agree this would be clearer, but as its not ... why do
> we need to make this harder to read?
> 

I intended to avoid the unnecessary nested statements, caused
by the "do { ... } while (0)". With that, the code looks a bit
cleaner. It might make the code a bit harder to read, but it's
fine to me. Besides, the unnecessary "do { ... } while (0)" looks
a bit strange to me because the block is executed for once.

So I think it'd better to keep the changes if you don't object
strongly. Otherwise, I can drop this one.

Cheers,
Gavin

> 
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 03b1179da9b4..d04329f98922 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -590,36 +590,34 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   	WARN_ON(in_nmi());
>>   
>>   	mutex_lock(&sdei_events_lock);
>> -	do {
>> -		if (sdei_event_find(event_num)) {
>> -			pr_warn("Event %u already registered\n", event_num);
>> -			err = -EBUSY;
>> -			break;
>> -		}
>> +	if (sdei_event_find(event_num)) {
>> +		pr_warn("Event %u already registered\n", event_num);
>> +		err = -EBUSY;
>> +		goto unlock;
>> +	}
>>   
>> -		event = sdei_event_create(event_num, cb, arg);
>> -		if (IS_ERR(event)) {
>> -			err = PTR_ERR(event);
>> -			pr_warn("Failed to create event %u: %d\n", event_num,
>> -				err);
>> -			break;
>> -		}
>> +	event = sdei_event_create(event_num, cb, arg);
>> +	if (IS_ERR(event)) {
>> +		err = PTR_ERR(event);
>> +		pr_warn("Failed to create event %u: %d\n", event_num, err);
>> +		goto unlock;
>> +	}
>>   
>> -		cpus_read_lock();
>> -		err = _sdei_event_register(event);
>> -		if (err) {
>> -			sdei_event_destroy(event);
>> -			pr_warn("Failed to register event %u: %d\n", event_num,
>> -				err);
>> -		} else {
>> -			spin_lock(&sdei_list_lock);
>> -			event->reregister = true;
>> -			spin_unlock(&sdei_list_lock);
>> -		}
>> -		cpus_read_unlock();
>> -	} while (0);
>> -	mutex_unlock(&sdei_events_lock);
>> +	cpus_read_lock();
>> +	err = _sdei_event_register(event);
>> +	if (err) {
>> +		sdei_event_destroy(event);
>> +		pr_warn("Failed to register event %u: %d\n", event_num, err);
>> +		goto cpu_unlock;
>> +	}
>>   
>> +	spin_lock(&sdei_list_lock);
>> +	event->reregister = true;
>> +	spin_unlock(&sdei_list_lock);
>> +cpu_unlock:
>> +	cpus_read_unlock();
>> +unlock:
>> +	mutex_unlock(&sdei_events_lock);
>>   	return err;
>>   }
>>   
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 14/15] drivers/firmware/sdei: Expose struct sdei_event

[Gavin Shan]

Hi James,

On 9/19/20 2:15 AM, James Morse wrote:
> On 30/07/2020 02:45, Gavin Shan wrote:
>> This splits struct sdei_event into two structs: sdei_event and
>> sdei_internal_event. The former one can be dereferenced from external
>> module like arm64/kvm when SDEI virtualization is supported. The later
>> one is used by the client driver only.
> 
> This should be part of the series that need its.
> 
> I don't see any reason to share this as it risks muddling events the OS has as a client,
> with those its offering as a hypervisor.
> 
> I doubt any KVM support would need to have anything to do with the client support.
> 
> The same argument goes for patch 15.
> 

I'll drop PATCH[14/15] and PATCH[15/15] from this series and fold them
into the series of "Support SDEI virtualization". Lets have more discussion
when that series is reviewed.

The primary reason is to export "struct sdei_event" to KVM so that the
information about the SDEI event, like priority/signaled etc, can be
fetched from the struct. It's only meaningful if the corresponding
SDEI event is exported by underly firmware and passed through to the
guest.

Cheers,
Gavin



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel