[PATCH v3 00/15] Refactor SDEI client driver

[PATCH v3 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
=========
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 | 440 +++++++++++++++++-------------------
 include/linux/arm_sdei.h    |  16 +-
 2 files changed, 215 insertions(+), 241 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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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>
---
v3: Rename "out" to "unlock" tag in sdei_event_register()
---
 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 v3 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>
---
v3: Rename "out" to "unlock" tag in sdei_event_unregister()
    Keep the empty line before returning errno in sdei_event_unregister()
---
 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 v3 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>
---
v3: Corrected spellings in changelog
    Drop tabs between type and field for struct sdei_crosscall_args
    Use smp_call_func_t for @fn argument in sdei_do_cross_call()
---
 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 v3 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>
---
v3: Use smp_call_func_t for @fn argument in sdei_do_local_call()
---
 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 v3 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>
---
v3: s/pivate/private in changelog
---
 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 v3 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 v3 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 shouldn't cause functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v3: Split struct sdei_event into struct sdei_{internal,}_event and
    struct sdei_internal_event is used by client driver only
---
 drivers/firmware/arm_sdei.c | 116 ++++++++++++++++++------------------
 include/linux/arm_sdei.h    |   6 ++
 2 files changed, 63 insertions(+), 59 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 2678475940e6..c80085e30db1 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,15 +160,15 @@ 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 *e, *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) {
+		if (e->event.event_num == event_num) {
 			found = e;
 			break;
 		}
@@ -193,13 +191,13 @@ 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_event *event;
+	struct sdei_internal_event *event;
 	struct sdei_registered_event *reg;
 
 	lockdep_assert_held(&sdei_events_lock);
@@ -211,29 +209,29 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 	}
 
 	INIT_LIST_HEAD(&event->list);
-	event->event_num = event_num;
+	event->event.event_num = event_num;
 
 	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
 				      &result);
 	if (err)
 		goto fail;
-	event->priority = result;
+	event->event.priority = result;
 
 	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_TYPE,
 				      &result);
 	if (err)
 		goto fail;
-	event->type = result;
+	event->event.type = result;
 
-	if (event->type == SDEI_EVENT_TYPE_SHARED) {
+	if (event->event.type == SDEI_EVENT_TYPE_SHARED) {
 		reg = kzalloc(sizeof(*reg), GFP_KERNEL);
 		if (!reg) {
 			err = -ENOMEM;
 			goto fail;
 		}
 
-		reg->event_num = event->event_num;
-		reg->priority = event->priority;
+		reg->event_num = event->event.event_num;
+		reg->priority = event->event.priority;
 
 		reg->callback = cb;
 		reg->callback_arg = cb_arg;
@@ -251,8 +249,8 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 		for_each_possible_cpu(cpu) {
 			reg = per_cpu_ptr(regs, cpu);
 
-			reg->event_num = event->event_num;
-			reg->priority = event->priority;
+			reg->event_num = event->event.event_num;
+			reg->priority = event->event.priority;
 			reg->callback = cb;
 			reg->callback_arg = cb_arg;
 		}
@@ -271,14 +269,14 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 	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)
 {
 	lockdep_assert_held(&sdei_events_lock);
 	lockdep_assert_held(&sdei_list_lock);
 
 	list_del(&event->list);
 
-	if (event->type == SDEI_EVENT_TYPE_SHARED)
+	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
 		kfree(event->registered);
 	else
 		free_percpu(event->private_registered);
@@ -286,7 +284,7 @@ static void sdei_event_destroy_llocked(struct sdei_event *event)
 	kfree(event);
 }
 
-static void sdei_event_destroy(struct sdei_event *event)
+static void sdei_event_destroy(struct sdei_internal_event *event)
 {
 	spin_lock(&sdei_list_lock);
 	sdei_event_destroy_llocked(event);
@@ -392,7 +390,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,7 +398,7 @@ static void _local_event_enable(void *data)
 int sdei_event_enable(u32 event_num)
 {
 	int err = -EINVAL;
-	struct sdei_event *event;
+	struct sdei_internal_event *event;
 
 	mutex_lock(&sdei_events_lock);
 	event = sdei_event_find(event_num);
@@ -411,8 +409,8 @@ int sdei_event_enable(u32 event_num)
 
 
 	cpus_read_lock();
-	if (event->type == SDEI_EVENT_TYPE_SHARED)
-		err = sdei_api_event_enable(event->event_num);
+	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
+		err = sdei_api_event_enable(event->event.event_num);
 	else
 		err = sdei_do_cross_call(_local_event_enable, event);
 
@@ -438,7 +436,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,7 +444,7 @@ static void _ipi_event_disable(void *data)
 int sdei_event_disable(u32 event_num)
 {
 	int err = -EINVAL;
-	struct sdei_event *event;
+	struct sdei_internal_event *event;
 
 	mutex_lock(&sdei_events_lock);
 	event = sdei_event_find(event_num);
@@ -459,8 +457,8 @@ int sdei_event_disable(u32 event_num)
 	event->reenable = false;
 	spin_unlock(&sdei_list_lock);
 
-	if (event->type == SDEI_EVENT_TYPE_SHARED)
-		err = sdei_api_event_disable(event->event_num);
+	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
+		err = sdei_api_event_disable(event->event.event_num);
 	else
 		err = sdei_do_cross_call(_ipi_event_disable, event);
 	mutex_unlock(&sdei_events_lock);
@@ -482,7 +480,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,7 +488,7 @@ static void _local_event_unregister(void *data)
 int sdei_event_unregister(u32 event_num)
 {
 	int err;
-	struct sdei_event *event;
+	struct sdei_internal_event *event;
 
 	WARN_ON(in_nmi());
 
@@ -507,8 +505,8 @@ int sdei_event_unregister(u32 event_num)
 	event->reenable = false;
 	spin_unlock(&sdei_list_lock);
 
-	if (event->type == SDEI_EVENT_TYPE_SHARED)
-		err = sdei_api_event_unregister(event->event_num);
+	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
+		err = sdei_api_event_unregister(event->event.event_num);
 	else
 		err = sdei_do_cross_call(_local_event_unregister, event);
 
@@ -529,15 +527,15 @@ int sdei_event_unregister(u32 event_num)
 static int sdei_unregister_shared(void)
 {
 	int err = 0;
-	struct sdei_event *event;
+	struct sdei_internal_event *event;
 
 	mutex_lock(&sdei_events_lock);
 	spin_lock(&sdei_list_lock);
 	list_for_each_entry(event, &sdei_list, list) {
-		if (event->type != SDEI_EVENT_TYPE_SHARED)
+		if (event->event.type != SDEI_EVENT_TYPE_SHARED)
 			continue;
 
-		err = sdei_api_event_unregister(event->event_num);
+		err = sdei_api_event_unregister(event->event.event_num);
 		if (err)
 			break;
 	}
@@ -565,8 +563,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,7 +572,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_event *event;
+	struct sdei_internal_event *event;
 
 	WARN_ON(in_nmi());
 
@@ -593,8 +591,8 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	}
 
 	cpus_read_lock();
-	if (event->type == SDEI_EVENT_TYPE_SHARED) {
-		err = sdei_api_event_register(event->event_num,
+	if (event->event.type == SDEI_EVENT_TYPE_SHARED) {
+		err = sdei_api_event_register(event->event.event_num,
 					      sdei_entry_point,
 					      event->registered,
 					      SDEI_EVENT_REGISTER_RM_ANY, 0);
@@ -623,31 +621,31 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 static int sdei_reregister_shared(void)
 {
 	int err = 0;
-	struct sdei_event *event;
+	struct sdei_internal_event *event;
 
 	mutex_lock(&sdei_events_lock);
 	spin_lock(&sdei_list_lock);
 	list_for_each_entry(event, &sdei_list, list) {
-		if (event->type != SDEI_EVENT_TYPE_SHARED)
+		if (event->event.type != SDEI_EVENT_TYPE_SHARED)
 			continue;
 
 		if (event->reregister) {
-			err = sdei_api_event_register(event->event_num,
+			err = sdei_api_event_register(event->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);
+				       event->event.event_num);
 				break;
 			}
 		}
 
 		if (event->reenable) {
-			err = sdei_api_event_enable(event->event_num);
+			err = sdei_api_event_enable(event->event.event_num);
 			if (err) {
 				pr_err("Failed to re-enable event %u\n",
-				       event->event_num);
+				       event->event.event_num);
 				break;
 			}
 		}
@@ -660,19 +658,19 @@ static int sdei_reregister_shared(void)
 
 static int sdei_cpuhp_down(unsigned int cpu)
 {
-	struct sdei_event *event;
+	struct sdei_internal_event *event;
 	int err;
 
 	/* un-register private events */
 	spin_lock(&sdei_list_lock);
 	list_for_each_entry(event, &sdei_list, list) {
-		if (event->type == SDEI_EVENT_TYPE_SHARED)
+		if (event->event.type == SDEI_EVENT_TYPE_SHARED)
 			continue;
 
 		err = sdei_do_local_call(_local_event_unregister, event);
 		if (err) {
 			pr_err("Failed to unregister event %u: %d\n",
-			       event->event_num, err);
+			       event->event.event_num, err);
 		}
 	}
 	spin_unlock(&sdei_list_lock);
@@ -682,20 +680,20 @@ static int sdei_cpuhp_down(unsigned int cpu)
 
 static int sdei_cpuhp_up(unsigned int cpu)
 {
-	struct sdei_event *event;
+	struct sdei_internal_event *event;
 	int err;
 
 	/* re-register/enable private events */
 	spin_lock(&sdei_list_lock);
 	list_for_each_entry(event, &sdei_list, list) {
-		if (event->type == SDEI_EVENT_TYPE_SHARED)
+		if (event->event.type == SDEI_EVENT_TYPE_SHARED)
 			continue;
 
 		if (event->reregister) {
 			err = sdei_do_local_call(_local_event_register, event);
 			if (err) {
 				pr_err("Failed to re-register event %u: %d\n",
-				       event->event_num, err);
+				       event->event.event_num, err);
 			}
 		}
 
@@ -703,7 +701,7 @@ static int sdei_cpuhp_up(unsigned int cpu)
 			err = sdei_do_local_call(_local_event_enable, event);
 			if (err) {
 				pr_err("Failed to re-enable event %u: %d\n",
-				       event->event_num, err);
+				       event->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 v3 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>
---
v3: Rename parameter @event to @se to avoid conflicts
---
 drivers/firmware/arm_sdei.c | 69 +++++++++++++++++++------------------
 include/linux/arm_sdei.h    | 10 +++---
 2 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index c80085e30db1..a2068c14a50f 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -395,19 +395,13 @@ 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 *se)
 {
 	int err = -EINVAL;
-	struct sdei_internal_event *event;
+	struct sdei_internal_event *event =
+		container_of(se, struct sdei_internal_event, event);
 
 	mutex_lock(&sdei_events_lock);
-	event = sdei_event_find(event_num);
-	if (!event) {
-		mutex_unlock(&sdei_events_lock);
-		return -ENOENT;
-	}
-
-
 	cpus_read_lock();
 	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_enable(event->event.event_num);
@@ -441,18 +435,13 @@ 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 *se)
 {
 	int err = -EINVAL;
-	struct sdei_internal_event *event;
+	struct sdei_internal_event *event =
+		container_of(se, struct sdei_internal_event, event);
 
 	mutex_lock(&sdei_events_lock);
-	event = sdei_event_find(event_num);
-	if (!event) {
-		mutex_unlock(&sdei_events_lock);
-		return -ENOENT;
-	}
-
 	spin_lock(&sdei_list_lock);
 	event->reenable = false;
 	spin_unlock(&sdei_list_lock);
@@ -485,21 +474,15 @@ 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 *se)
 {
 	int err;
-	struct sdei_internal_event *event;
+	struct sdei_internal_event *event =
+		container_of(se, struct sdei_internal_event, event);
 
 	WARN_ON(in_nmi());
 
 	mutex_lock(&sdei_events_lock);
-	event = sdei_event_find(event_num);
-	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;
@@ -569,23 +552,27 @@ 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;
+	struct sdei_event *se;
 
 	WARN_ON(in_nmi());
 
 	mutex_lock(&sdei_events_lock);
 	if (sdei_event_find(event_num)) {
 		pr_warn("Event %u already registered\n", event_num);
-		err = -EBUSY;
+		se = ERR_PTR(-EBUSY);
 		goto unlock;
 	}
 
 	event = sdei_event_create(event_num, cb, arg);
 	if (IS_ERR(event)) {
 		err = PTR_ERR(event);
+		se = ERR_PTR(err);
 		pr_warn("Failed to create event %u: %d\n", event_num, err);
 		goto unlock;
 	}
@@ -603,11 +590,13 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	}
 
 	if (err) {
+		se = ERR_PTR(err);
 		sdei_event_destroy(event);
 		pr_warn("Failed to register event %u: %d\n", event_num, err);
 		goto cpu_unlock;
 	}
 
+	se = &event->event;
 	spin_lock(&sdei_list_lock);
 	event->reregister = true;
 	spin_unlock(&sdei_list_lock);
@@ -615,7 +604,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 se;
 }
 
 static int sdei_reregister_shared(void)
@@ -858,6 +847,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 *se;
 
 	if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES))
 		return -EOPNOTSUPP;
@@ -881,9 +871,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);
+	se = sdei_event_register(event_num, cb, ghes);
+	if (IS_ERR(se)) {
+		err = PTR_ERR(se);
+		return err;
+	}
+
+	err = sdei_event_enable(se);
 
 	return err;
 }
@@ -893,22 +887,29 @@ int sdei_unregister_ghes(struct ghes *ghes)
 	int i;
 	int err;
 	u32 event_num = ghes->generic->notify.vector;
+	struct sdei_internal_event *event;
 
 	might_sleep();
 
 	if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES))
 		return -EOPNOTSUPP;
 
+	mutex_lock(&sdei_events_lock);
+	event = sdei_event_find(event_num);
+	mutex_unlock(&sdei_events_lock);
+	if (!event)
+		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);
+	err = sdei_event_disable(&event->event);
 	if (err)
 		return err;
 
 	for (i = 0; i < 3; i++) {
-		err = sdei_event_unregister(event_num);
+		err = sdei_event_unregister(&event->event);
 		if (err != -EINPROGRESS)
 			break;
 
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index d2464a18b6ff..984adc6cdec4 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 *se);
 
-int sdei_event_enable(u32 event_num);
-int sdei_event_disable(u32 event_num);
+int sdei_event_enable(struct sdei_event *se);
+int sdei_event_disable(struct sdei_event *se);
 
 /* 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 v3 11/15] drivers/firmware/sdei: Introduce sdei_do_local_call()

[Jonathan Cameron]

On Tue, 28 Jul 2020 12:59:51 +1000
Gavin Shan <gshan@redhat.com> 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().
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Hi Gavin,

I'm not totally convinced with the use of smp_call_func_t here.
It sort of feels a little confusing as type naming goes. It gets
used in similar places as smp_call_func_t would be, but not
really smp paths perhaps?

Still I'm not that bothered hence if everyone else is happy it's
fine by me.

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

> ---
> v3: Use smp_call_func_t for @fn argument in sdei_do_local_call()
> ---
>  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);



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

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

[Jonathan Cameron]

On Tue, 28 Jul 2020 12:59:54 +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 shouldn't cause functional changes.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Hi Gavin,

This ended up cleaner than I expected :)

You may have already tried it, but if not it might be worth experimenting with
a bit of naming changing and local variables.

In most places you are operating only on either the
struct sdei_event, or the wrapping struct sdei_internal_event

It may help both readability and size of patch to give naming such as

struct sdei_internal_event *event_el =...;
struct sdei_event *event = &event_el->event;

From eyeballing the code I'm not sure if that's actually beneficial or not.
It is tidier in the various loops over the list, but requires a bunch of
renames elsewhere.

What do you think?

Thanks,

Jonathan



> ---
> v3: Split struct sdei_event into struct sdei_{internal,}_event and
>     struct sdei_internal_event is used by client driver only
> ---
>  drivers/firmware/arm_sdei.c | 116 ++++++++++++++++++------------------
>  include/linux/arm_sdei.h    |   6 ++
>  2 files changed, 63 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 2678475940e6..c80085e30db1 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,15 +160,15 @@ 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 *e, *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) {
> +		if (e->event.event_num == event_num) {
>  			found = e;
>  			break;
>  		}
> @@ -193,13 +191,13 @@ 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_event *event;
> +	struct sdei_internal_event *event;
>  	struct sdei_registered_event *reg;
>  
>  	lockdep_assert_held(&sdei_events_lock);
> @@ -211,29 +209,29 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>  	}
>  
>  	INIT_LIST_HEAD(&event->list);
> -	event->event_num = event_num;
> +	event->event.event_num = event_num;
>  
>  	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
>  				      &result);
>  	if (err)
>  		goto fail;
> -	event->priority = result;
> +	event->event.priority = result;
>  
>  	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_TYPE,
>  				      &result);
>  	if (err)
>  		goto fail;
> -	event->type = result;
> +	event->event.type = result;
>  
> -	if (event->type == SDEI_EVENT_TYPE_SHARED) {
> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED) {
>  		reg = kzalloc(sizeof(*reg), GFP_KERNEL);
>  		if (!reg) {
>  			err = -ENOMEM;
>  			goto fail;
>  		}
>  
> -		reg->event_num = event->event_num;
> -		reg->priority = event->priority;
> +		reg->event_num = event->event.event_num;
> +		reg->priority = event->event.priority;
>  
>  		reg->callback = cb;
>  		reg->callback_arg = cb_arg;
> @@ -251,8 +249,8 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>  		for_each_possible_cpu(cpu) {
>  			reg = per_cpu_ptr(regs, cpu);
>  
> -			reg->event_num = event->event_num;
> -			reg->priority = event->priority;
> +			reg->event_num = event->event.event_num;
> +			reg->priority = event->event.priority;
>  			reg->callback = cb;
>  			reg->callback_arg = cb_arg;
>  		}
> @@ -271,14 +269,14 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>  	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)
>  {
>  	lockdep_assert_held(&sdei_events_lock);
>  	lockdep_assert_held(&sdei_list_lock);
>  
>  	list_del(&event->list);
>  
> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>  		kfree(event->registered);
>  	else
>  		free_percpu(event->private_registered);
> @@ -286,7 +284,7 @@ static void sdei_event_destroy_llocked(struct sdei_event *event)
>  	kfree(event);
>  }
>  
> -static void sdei_event_destroy(struct sdei_event *event)
> +static void sdei_event_destroy(struct sdei_internal_event *event)
>  {
>  	spin_lock(&sdei_list_lock);
>  	sdei_event_destroy_llocked(event);
> @@ -392,7 +390,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,7 +398,7 @@ static void _local_event_enable(void *data)
>  int sdei_event_enable(u32 event_num)
>  {
>  	int err = -EINVAL;
> -	struct sdei_event *event;
> +	struct sdei_internal_event *event;
>  
>  	mutex_lock(&sdei_events_lock);
>  	event = sdei_event_find(event_num);
> @@ -411,8 +409,8 @@ int sdei_event_enable(u32 event_num)
>  
>  
>  	cpus_read_lock();
> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
> -		err = sdei_api_event_enable(event->event_num);
> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
> +		err = sdei_api_event_enable(event->event.event_num);
>  	else
>  		err = sdei_do_cross_call(_local_event_enable, event);
>  
> @@ -438,7 +436,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,7 +444,7 @@ static void _ipi_event_disable(void *data)
>  int sdei_event_disable(u32 event_num)
>  {
>  	int err = -EINVAL;
> -	struct sdei_event *event;
> +	struct sdei_internal_event *event;
>  
>  	mutex_lock(&sdei_events_lock);
>  	event = sdei_event_find(event_num);
> @@ -459,8 +457,8 @@ int sdei_event_disable(u32 event_num)
>  	event->reenable = false;
>  	spin_unlock(&sdei_list_lock);
>  
> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
> -		err = sdei_api_event_disable(event->event_num);
> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
> +		err = sdei_api_event_disable(event->event.event_num);
>  	else
>  		err = sdei_do_cross_call(_ipi_event_disable, event);
>  	mutex_unlock(&sdei_events_lock);
> @@ -482,7 +480,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,7 +488,7 @@ static void _local_event_unregister(void *data)
>  int sdei_event_unregister(u32 event_num)
>  {
>  	int err;
> -	struct sdei_event *event;
> +	struct sdei_internal_event *event;
>  
>  	WARN_ON(in_nmi());
>  
> @@ -507,8 +505,8 @@ int sdei_event_unregister(u32 event_num)
>  	event->reenable = false;
>  	spin_unlock(&sdei_list_lock);
>  
> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
> -		err = sdei_api_event_unregister(event->event_num);
> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
> +		err = sdei_api_event_unregister(event->event.event_num);
>  	else
>  		err = sdei_do_cross_call(_local_event_unregister, event);
>  
> @@ -529,15 +527,15 @@ int sdei_event_unregister(u32 event_num)
>  static int sdei_unregister_shared(void)
>  {
>  	int err = 0;
> -	struct sdei_event *event;
> +	struct sdei_internal_event *event;
>  
>  	mutex_lock(&sdei_events_lock);
>  	spin_lock(&sdei_list_lock);
>  	list_for_each_entry(event, &sdei_list, list) {
> -		if (event->type != SDEI_EVENT_TYPE_SHARED)
> +		if (event->event.type != SDEI_EVENT_TYPE_SHARED)
>  			continue;
>  
> -		err = sdei_api_event_unregister(event->event_num);
> +		err = sdei_api_event_unregister(event->event.event_num);
>  		if (err)
>  			break;
>  	}
> @@ -565,8 +563,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,7 +572,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_event *event;
> +	struct sdei_internal_event *event;
>  
>  	WARN_ON(in_nmi());
>  
> @@ -593,8 +591,8 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  	}
>  
>  	cpus_read_lock();
> -	if (event->type == SDEI_EVENT_TYPE_SHARED) {
> -		err = sdei_api_event_register(event->event_num,
> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED) {
> +		err = sdei_api_event_register(event->event.event_num,
>  					      sdei_entry_point,
>  					      event->registered,
>  					      SDEI_EVENT_REGISTER_RM_ANY, 0);
> @@ -623,31 +621,31 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>  static int sdei_reregister_shared(void)
>  {
>  	int err = 0;
> -	struct sdei_event *event;
> +	struct sdei_internal_event *event;
>  
>  	mutex_lock(&sdei_events_lock);
>  	spin_lock(&sdei_list_lock);
>  	list_for_each_entry(event, &sdei_list, list) {

Perhaps worth a local variable for event->event?
If you were to do
list_for_each_entry(event_el, &sdei_list, list) {
	sdei_event *event = &event_el->event;

Would reduce the patch size and might give slightly more readable code.

The issue is that you'd then need to be very careful with naming of
event vs event_el (for element of list) to keep it consistent.
It's possible it will cause more mess than the current approach!

> -		if (event->type != SDEI_EVENT_TYPE_SHARED)
> +		if (event->event.type != SDEI_EVENT_TYPE_SHARED)
>  			continue;
>  
>  		if (event->reregister) {
> -			err = sdei_api_event_register(event->event_num,
> +			err = sdei_api_event_register(event->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);
> +				       event->event.event_num);
>  				break;
>  			}
>  		}
>  
>  		if (event->reenable) {
> -			err = sdei_api_event_enable(event->event_num);
> +			err = sdei_api_event_enable(event->event.event_num);
>  			if (err) {
>  				pr_err("Failed to re-enable event %u\n",
> -				       event->event_num);
> +				       event->event.event_num);
>  				break;
>  			}
>  		}
> @@ -660,19 +658,19 @@ static int sdei_reregister_shared(void)
>  
>  static int sdei_cpuhp_down(unsigned int cpu)
>  {
> -	struct sdei_event *event;
> +	struct sdei_internal_event *event;
>  	int err;
>  
>  	/* un-register private events */
>  	spin_lock(&sdei_list_lock);
>  	list_for_each_entry(event, &sdei_list, list) {
> -		if (event->type == SDEI_EVENT_TYPE_SHARED)
> +		if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>  			continue;
>  
>  		err = sdei_do_local_call(_local_event_unregister, event);
>  		if (err) {
>  			pr_err("Failed to unregister event %u: %d\n",
> -			       event->event_num, err);
> +			       event->event.event_num, err);
>  		}
>  	}
>  	spin_unlock(&sdei_list_lock);
> @@ -682,20 +680,20 @@ static int sdei_cpuhp_down(unsigned int cpu)
>  
>  static int sdei_cpuhp_up(unsigned int cpu)
>  {
> -	struct sdei_event *event;
> +	struct sdei_internal_event *event;
>  	int err;
>  
>  	/* re-register/enable private events */
>  	spin_lock(&sdei_list_lock);
>  	list_for_each_entry(event, &sdei_list, list) {
> -		if (event->type == SDEI_EVENT_TYPE_SHARED)
> +		if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>  			continue;
>  
>  		if (event->reregister) {
>  			err = sdei_do_local_call(_local_event_register, event);
>  			if (err) {
>  				pr_err("Failed to re-register event %u: %d\n",
> -				       event->event_num, err);
> +				       event->event.event_num, err);
>  			}
>  		}
>  
> @@ -703,7 +701,7 @@ static int sdei_cpuhp_up(unsigned int cpu)
>  			err = sdei_do_local_call(_local_event_enable, event);
>  			if (err) {
>  				pr_err("Failed to re-enable event %u: %d\n",
> -				       event->event_num, err);
> +				       event->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 v3 11/15] drivers/firmware/sdei: Introduce sdei_do_local_call()

[Gavin Shan]

Hi Jonathan,

On 7/29/20 1:32 AM, Jonathan Cameron wrote:
> On Tue, 28 Jul 2020 12:59:51 +1000
> Gavin Shan <gshan@redhat.com> 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().
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> Hi Gavin,
> 
> I'm not totally convinced with the use of smp_call_func_t here.
> It sort of feels a little confusing as type naming goes. It gets
> used in similar places as smp_call_func_t would be, but not
> really smp paths perhaps?
> 

Yeah, it's true it's only called on local CPU, but it was derived
from sdei_do_cross_call() because they are sharing same function
prototype :)


> Still I'm not that bothered hence if everyone else is happy it's
> fine by me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Thanks, I will pick it up and put to v4.

>> ---
>> v3: Use smp_call_func_t for @fn argument in sdei_do_local_call()
>> ---
>>   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);

Thanks,
Gavin


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

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

[Gavin Shan]

Hi Jonathan,

On 7/29/20 1:48 AM, Jonathan Cameron wrote:
> On Tue, 28 Jul 2020 12:59:54 +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 shouldn't cause functional changes.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> Hi Gavin,
> 
> This ended up cleaner than I expected :)
> 
> You may have already tried it, but if not it might be worth experimenting with
> a bit of naming changing and local variables.
> 
> In most places you are operating only on either the
> struct sdei_event, or the wrapping struct sdei_internal_event
> 
> It may help both readability and size of patch to give naming such as
> 
> struct sdei_internal_event *event_el =...;
> struct sdei_event *event = &event_el->event;
> 
>>From eyeballing the code I'm not sure if that's actually beneficial or not.
> It is tidier in the various loops over the list, but requires a bunch of
> renames elsewhere.
> 
> What do you think?
> 

I was thinking of this sort of changes and more code changes are needed
to do so. However, I think it's fine to use @event_el to dereference
the "struct sdei_internal_event" instance and @event for "struct sdei_event".
It makes the variable names meaningful and I think the effort is worthy.

So lets have more changes, combined to this patch in v4:

    (1) Use @event_el to dereference SDEI internal event. Note that
        we can't do this for sdei_do_{local,cross}_call() because
        CROSSCALL_INIT stops us from doing this.
    (2) Add @event in function where the SDEI event is deferrenced
        for multiple times in the particular function. Otherwise,
        @event won't be added.

By the way, here is the code change statistics without/with the renaming.
I think it's fine to do the renaming :)

  drivers/firmware/arm_sdei.c | 116 ++++++++++++++++++------------------
  include/linux/arm_sdei.h    |   6 ++
  2 files changed, 63 insertions(+), 59 deletions(-)

  drivers/firmware/arm_sdei.c | 158 ++++++++++++++++++++++++++++++++---------------------------
  include/linux/arm_sdei.h    |   6 +++
  2 files changed, 93 insertions(+), 71 deletions(-)

> 
> 
>> ---
>> v3: Split struct sdei_event into struct sdei_{internal,}_event and
>>      struct sdei_internal_event is used by client driver only
>> ---
>>   drivers/firmware/arm_sdei.c | 116 ++++++++++++++++++------------------
>>   include/linux/arm_sdei.h    |   6 ++
>>   2 files changed, 63 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 2678475940e6..c80085e30db1 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,15 +160,15 @@ 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 *e, *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) {
>> +		if (e->event.event_num == event_num) {
>>   			found = e;
>>   			break;
>>   		}
>> @@ -193,13 +191,13 @@ 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_event *event;
>> +	struct sdei_internal_event *event;
>>   	struct sdei_registered_event *reg;
>>   
>>   	lockdep_assert_held(&sdei_events_lock);
>> @@ -211,29 +209,29 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>>   	}
>>   
>>   	INIT_LIST_HEAD(&event->list);
>> -	event->event_num = event_num;
>> +	event->event.event_num = event_num;
>>   
>>   	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
>>   				      &result);
>>   	if (err)
>>   		goto fail;
>> -	event->priority = result;
>> +	event->event.priority = result;
>>   
>>   	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_TYPE,
>>   				      &result);
>>   	if (err)
>>   		goto fail;
>> -	event->type = result;
>> +	event->event.type = result;
>>   
>> -	if (event->type == SDEI_EVENT_TYPE_SHARED) {
>> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED) {
>>   		reg = kzalloc(sizeof(*reg), GFP_KERNEL);
>>   		if (!reg) {
>>   			err = -ENOMEM;
>>   			goto fail;
>>   		}
>>   
>> -		reg->event_num = event->event_num;
>> -		reg->priority = event->priority;
>> +		reg->event_num = event->event.event_num;
>> +		reg->priority = event->event.priority;
>>   
>>   		reg->callback = cb;
>>   		reg->callback_arg = cb_arg;
>> @@ -251,8 +249,8 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>>   		for_each_possible_cpu(cpu) {
>>   			reg = per_cpu_ptr(regs, cpu);
>>   
>> -			reg->event_num = event->event_num;
>> -			reg->priority = event->priority;
>> +			reg->event_num = event->event.event_num;
>> +			reg->priority = event->event.priority;
>>   			reg->callback = cb;
>>   			reg->callback_arg = cb_arg;
>>   		}
>> @@ -271,14 +269,14 @@ static struct sdei_event *sdei_event_create(u32 event_num,
>>   	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)
>>   {
>>   	lockdep_assert_held(&sdei_events_lock);
>>   	lockdep_assert_held(&sdei_list_lock);
>>   
>>   	list_del(&event->list);
>>   
>> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
>> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>>   		kfree(event->registered);
>>   	else
>>   		free_percpu(event->private_registered);
>> @@ -286,7 +284,7 @@ static void sdei_event_destroy_llocked(struct sdei_event *event)
>>   	kfree(event);
>>   }
>>   
>> -static void sdei_event_destroy(struct sdei_event *event)
>> +static void sdei_event_destroy(struct sdei_internal_event *event)
>>   {
>>   	spin_lock(&sdei_list_lock);
>>   	sdei_event_destroy_llocked(event);
>> @@ -392,7 +390,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,7 +398,7 @@ static void _local_event_enable(void *data)
>>   int sdei_event_enable(u32 event_num)
>>   {
>>   	int err = -EINVAL;
>> -	struct sdei_event *event;
>> +	struct sdei_internal_event *event;
>>   
>>   	mutex_lock(&sdei_events_lock);
>>   	event = sdei_event_find(event_num);
>> @@ -411,8 +409,8 @@ int sdei_event_enable(u32 event_num)
>>   
>>   
>>   	cpus_read_lock();
>> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
>> -		err = sdei_api_event_enable(event->event_num);
>> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> +		err = sdei_api_event_enable(event->event.event_num);
>>   	else
>>   		err = sdei_do_cross_call(_local_event_enable, event);
>>   
>> @@ -438,7 +436,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,7 +444,7 @@ static void _ipi_event_disable(void *data)
>>   int sdei_event_disable(u32 event_num)
>>   {
>>   	int err = -EINVAL;
>> -	struct sdei_event *event;
>> +	struct sdei_internal_event *event;
>>   
>>   	mutex_lock(&sdei_events_lock);
>>   	event = sdei_event_find(event_num);
>> @@ -459,8 +457,8 @@ int sdei_event_disable(u32 event_num)
>>   	event->reenable = false;
>>   	spin_unlock(&sdei_list_lock);
>>   
>> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
>> -		err = sdei_api_event_disable(event->event_num);
>> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> +		err = sdei_api_event_disable(event->event.event_num);
>>   	else
>>   		err = sdei_do_cross_call(_ipi_event_disable, event);
>>   	mutex_unlock(&sdei_events_lock);
>> @@ -482,7 +480,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,7 +488,7 @@ static void _local_event_unregister(void *data)
>>   int sdei_event_unregister(u32 event_num)
>>   {
>>   	int err;
>> -	struct sdei_event *event;
>> +	struct sdei_internal_event *event;
>>   
>>   	WARN_ON(in_nmi());
>>   
>> @@ -507,8 +505,8 @@ int sdei_event_unregister(u32 event_num)
>>   	event->reenable = false;
>>   	spin_unlock(&sdei_list_lock);
>>   
>> -	if (event->type == SDEI_EVENT_TYPE_SHARED)
>> -		err = sdei_api_event_unregister(event->event_num);
>> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>> +		err = sdei_api_event_unregister(event->event.event_num);
>>   	else
>>   		err = sdei_do_cross_call(_local_event_unregister, event);
>>   
>> @@ -529,15 +527,15 @@ int sdei_event_unregister(u32 event_num)
>>   static int sdei_unregister_shared(void)
>>   {
>>   	int err = 0;
>> -	struct sdei_event *event;
>> +	struct sdei_internal_event *event;
>>   
>>   	mutex_lock(&sdei_events_lock);
>>   	spin_lock(&sdei_list_lock);
>>   	list_for_each_entry(event, &sdei_list, list) {
>> -		if (event->type != SDEI_EVENT_TYPE_SHARED)
>> +		if (event->event.type != SDEI_EVENT_TYPE_SHARED)
>>   			continue;
>>   
>> -		err = sdei_api_event_unregister(event->event_num);
>> +		err = sdei_api_event_unregister(event->event.event_num);
>>   		if (err)
>>   			break;
>>   	}
>> @@ -565,8 +563,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,7 +572,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_event *event;
>> +	struct sdei_internal_event *event;
>>   
>>   	WARN_ON(in_nmi());
>>   
>> @@ -593,8 +591,8 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   	}
>>   
>>   	cpus_read_lock();
>> -	if (event->type == SDEI_EVENT_TYPE_SHARED) {
>> -		err = sdei_api_event_register(event->event_num,
>> +	if (event->event.type == SDEI_EVENT_TYPE_SHARED) {
>> +		err = sdei_api_event_register(event->event.event_num,
>>   					      sdei_entry_point,
>>   					      event->registered,
>>   					      SDEI_EVENT_REGISTER_RM_ANY, 0);
>> @@ -623,31 +621,31 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
>>   static int sdei_reregister_shared(void)
>>   {
>>   	int err = 0;
>> -	struct sdei_event *event;
>> +	struct sdei_internal_event *event;
>>   
>>   	mutex_lock(&sdei_events_lock);
>>   	spin_lock(&sdei_list_lock);
>>   	list_for_each_entry(event, &sdei_list, list) {
> 
> Perhaps worth a local variable for event->event?
> If you were to do
> list_for_each_entry(event_el, &sdei_list, list) {
> 	sdei_event *event = &event_el->event;
> 
> Would reduce the patch size and might give slightly more readable code.
> 
> The issue is that you'd then need to be very careful with naming of
> event vs event_el (for element of list) to keep it consistent.
> It's possible it will cause more mess than the current approach!
> 

Yep, will do in v4 :)


>> -		if (event->type != SDEI_EVENT_TYPE_SHARED)
>> +		if (event->event.type != SDEI_EVENT_TYPE_SHARED)
>>   			continue;
>>   
>>   		if (event->reregister) {
>> -			err = sdei_api_event_register(event->event_num,
>> +			err = sdei_api_event_register(event->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);
>> +				       event->event.event_num);
>>   				break;
>>   			}
>>   		}
>>   
>>   		if (event->reenable) {
>> -			err = sdei_api_event_enable(event->event_num);
>> +			err = sdei_api_event_enable(event->event.event_num);
>>   			if (err) {
>>   				pr_err("Failed to re-enable event %u\n",
>> -				       event->event_num);
>> +				       event->event.event_num);
>>   				break;
>>   			}
>>   		}
>> @@ -660,19 +658,19 @@ static int sdei_reregister_shared(void)
>>   
>>   static int sdei_cpuhp_down(unsigned int cpu)
>>   {
>> -	struct sdei_event *event;
>> +	struct sdei_internal_event *event;
>>   	int err;
>>   
>>   	/* un-register private events */
>>   	spin_lock(&sdei_list_lock);
>>   	list_for_each_entry(event, &sdei_list, list) {
>> -		if (event->type == SDEI_EVENT_TYPE_SHARED)
>> +		if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>>   			continue;
>>   
>>   		err = sdei_do_local_call(_local_event_unregister, event);
>>   		if (err) {
>>   			pr_err("Failed to unregister event %u: %d\n",
>> -			       event->event_num, err);
>> +			       event->event.event_num, err);
>>   		}
>>   	}
>>   	spin_unlock(&sdei_list_lock);
>> @@ -682,20 +680,20 @@ static int sdei_cpuhp_down(unsigned int cpu)
>>   
>>   static int sdei_cpuhp_up(unsigned int cpu)
>>   {
>> -	struct sdei_event *event;
>> +	struct sdei_internal_event *event;
>>   	int err;
>>   
>>   	/* re-register/enable private events */
>>   	spin_lock(&sdei_list_lock);
>>   	list_for_each_entry(event, &sdei_list, list) {
>> -		if (event->type == SDEI_EVENT_TYPE_SHARED)
>> +		if (event->event.type == SDEI_EVENT_TYPE_SHARED)
>>   			continue;
>>   
>>   		if (event->reregister) {
>>   			err = sdei_do_local_call(_local_event_register, event);
>>   			if (err) {
>>   				pr_err("Failed to re-register event %u: %d\n",
>> -				       event->event_num, err);
>> +				       event->event.event_num, err);
>>   			}
>>   		}
>>   
>> @@ -703,7 +701,7 @@ static int sdei_cpuhp_up(unsigned int cpu)
>>   			err = sdei_do_local_call(_local_event_enable, event);
>>   			if (err) {
>>   				pr_err("Failed to re-enable event %u: %d\n",
>> -				       event->event_num, err);
>> +				       event->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