All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/14] Refactor SDEI client driver
@ 2020-07-06  5:47 Gavin Shan
  2020-07-06  5:47 ` [PATCH 01/14] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
                   ` (14 more replies)
  0 siblings, 15 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

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

   * PATCH[1-11] refactors and clean up the code. They shouldn't cause
     any functional changes.
   * PATCH[12] 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.
   * PATCH[13] retrieves the signaled property of the SDEI event when it's
     populated. It's needed by SDEI virtualization afterwards.
   * PATCH[14] exposes API (sdei_event_get_info()), which is needed by SDEI
     virtualization either.

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.

Gavin Shan (14):
  drivers/firmware/sdei: Remove sdei_is_err()
  drivers/firmware/sdei: Common block for failing path in
    sdei_event_create()
  drivers/firmware/sdei: Dereference SDEI event parameter directly
  drivers/firmware/sdei: Rework sdei_init()
  drivers/firmware/sdei: Remove sdei_get_conduit()
  drivers/firmware/sdei: Drop redundant error message in sdei_probe()
  drivers/firmware/sdei: Drop unnecessary while loop
  drivers/firmware/sdei: Cleanup on cross call functions
  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: Identify event by struct sdei_event
  drivers/firmware/sdei: Retrieve event signaled property on creation
  drivers/firmware/sdei: Add sdei_event_get_info()

 drivers/firmware/arm_sdei.c | 536 ++++++++++++++++--------------------
 include/linux/arm_sdei.h    |  73 +++--
 2 files changed, 284 insertions(+), 325 deletions(-)

-- 
2.23.0


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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* [PATCH 01/14] drivers/firmware/sdei: Remove sdei_is_err()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-21 20:39   ` James Morse
  2020-07-06  5:47 ` [PATCH 02/14] drivers/firmware/sdei: Common block for failing path in sdei_event_create() Gavin Shan
                   ` (13 subsequent siblings)
  14 siblings, 1 reply; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

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(). This shouldn't cause functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index e7e36aab2386..415c243a8e65 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,
@@ -147,8 +128,7 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
 	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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 02/14] drivers/firmware/sdei: Common block for failing path in sdei_event_create()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
  2020-07-06  5:47 ` [PATCH 01/14] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-21 20:40   ` James Morse
  2020-07-06  5:47 ` [PATCH 03/14] drivers/firmware/sdei: Dereference SDEI event parameter directly Gavin Shan
                   ` (12 subsequent siblings)
  14 siblings, 1 reply; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

The failing path in sdei_event_create() is to free SDEI event and
return the corresponding error. This introduces common block of
code for that to avoid duplicated logics. By the way, the function
scoped variables are also reordered according to their importance.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 415c243a8e65..a75212a743d3 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -182,41 +182,39 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 					    sdei_event_callback *cb,
 					    void *cb_arg)
 {
-	int err;
-	u64 result;
 	struct sdei_event *event;
 	struct sdei_registered_event *reg;
+	u64 result;
+	int err;
 
 	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,9 @@ 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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 03/14] drivers/firmware/sdei: Dereference SDEI event parameter directly
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
  2020-07-06  5:47 ` [PATCH 01/14] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
  2020-07-06  5:47 ` [PATCH 02/14] drivers/firmware/sdei: Common block for failing path in sdei_event_create() Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-21 20:41   ` James Morse
  2020-07-06  5:47 ` [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init() Gavin Shan
                   ` (11 subsequent siblings)
  14 siblings, 1 reply; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

The SDEI parameter is dereferenced by @sdei_event->registered or
@sdei_event->private_registered, depending on the event type. They
can be dereferenced directly so that the intermediate variable
(@regs) can be removed. It makes the code looks a bit simplified.
Also, @event->event_num instead of @event_num is used for retrieving
the event number for the shared event, which makes the code similar
to the case of private event. Besides, the local scoped variables
are reordered according to their importance by the way.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index a75212a743d3..35a319e7e1e6 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -211,38 +211,35 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 	event->type = result;
 
 	if (event->type == SDEI_EVENT_TYPE_SHARED) {
-		reg = kzalloc(sizeof(*reg), GFP_KERNEL);
-		if (!reg) {
+		event->registered = kzalloc(sizeof(*reg), GFP_KERNEL);
+		if (!event->registered) {
 			err = -ENOMEM;
 			goto fail;
 		}
 
-		reg->event_num = event_num;
-		reg->priority = event->priority;
-
-		reg->callback = cb;
-		reg->callback_arg = cb_arg;
-		event->registered = reg;
+		event->registered->event_num = event->event_num;
+		event->registered->priority = event->priority;
+		event->registered->callback = cb;
+		event->registered->callback_arg = cb_arg;
 	} else {
 		int cpu;
-		struct sdei_registered_event __percpu *regs;
+		struct sdei_registered_event *reg;
 
-		regs = alloc_percpu(struct sdei_registered_event);
-		if (!regs) {
+		event->private_registered =
+			alloc_percpu(struct sdei_registered_event);
+		if (!event->private_registered) {
 			err = -ENOMEM;
 			goto fail;
 		}
 
 		for_each_possible_cpu(cpu) {
-			reg = per_cpu_ptr(regs, cpu);
+			reg = per_cpu_ptr(event->private_registered, cpu);
 
 			reg->event_num = event->event_num;
 			reg->priority = event->priority;
 			reg->callback = cb;
 			reg->callback_arg = cb_arg;
 		}
-
-		event->private_registered = regs;
 	}
 
 	spin_lock(&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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (2 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 03/14] drivers/firmware/sdei: Dereference SDEI event parameter directly Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-21 20:42   ` James Morse
  2020-07-06  5:47 ` [PATCH 05/14] drivers/firmware/sdei: Remove sdei_get_conduit() Gavin Shan
                   ` (10 subsequent siblings)
  14 siblings, 1 reply; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

This reworks sdei_init()

   * The function follows the steps in sequence: check ACPI existence,
     register platform device, register platform driver.
   * The corresponding error numbers are returned in failing paths.
   * The platform device is deleted if the driver can't be registered.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 35a319e7e1e6..7e7b26b1f91b 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -1058,7 +1058,7 @@ static bool __init sdei_present_acpi(void)
 	acpi_status status;
 	struct acpi_table_header *sdei_table_header;
 
-	if (acpi_disabled)
+	if (!IS_ENABLED(CONFIG_ACPI) || acpi_disabled)
 		return false;
 
 	status = acpi_get_table(ACPI_SIG_SDEI, 0, &sdei_table_header);
@@ -1077,19 +1077,27 @@ static bool __init sdei_present_acpi(void)
 
 static int __init sdei_init(void)
 {
-	int ret = platform_driver_register(&sdei_driver);
+	struct platform_device *pdev;
+	int ret;
 
-	if (!ret && sdei_present_acpi()) {
-		struct platform_device *pdev;
+	if (!sdei_present_acpi())
+		return -EPERM;
 
-		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));
+	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 -ENOMEM;
 	}
 
-	return ret;
+	ret = platform_driver_register(&sdei_driver);
+	if (ret) {
+		platform_device_del(pdev);
+		return ret;
+	}
+
+	return 0;
 }
 
 /*
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 05/14] drivers/firmware/sdei: Remove sdei_get_conduit()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (3 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init() Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-21 20:42   ` James Morse
  2020-07-06  5:47 ` [PATCH 06/14] drivers/firmware/sdei: Drop redundant error message in sdei_probe() Gavin Shan
                   ` (9 subsequent siblings)
  14 siblings, 1 reply; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

There are some logics in sdei_get_conduit() can be safely dropped:

   * There are no associated device node with the platform device,
     so it's pointless to check on it.
   * ACPI functionality has been verified when the platform device
     is added in sdei_init(). So it's unnecessary to recheck.

With above logics dropped, sdei_get_conduit() is quite simple and
it's not worthy to maintain a separate function. The logic is merged
to sdei_probe() as it's the only caller.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 40 +++----------------------------------
 1 file changed, 3 insertions(+), 37 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 7e7b26b1f91b..ca0e3f5eb907 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -933,49 +933,15 @@ int sdei_unregister_ghes(struct ghes *ghes)
 	return err;
 }
 
-static int sdei_get_conduit(struct platform_device *pdev)
-{
-	const char *method;
-	struct device_node *np = pdev->dev.of_node;
-
-	sdei_firmware_call = NULL;
-	if (np) {
-		if (of_property_read_string(np, "method", &method)) {
-			pr_warn("missing \"method\" property\n");
-			return SMCCC_CONDUIT_NONE;
-		}
-
-		if (!strcmp("hvc", method)) {
-			sdei_firmware_call = &sdei_smccc_hvc;
-			return SMCCC_CONDUIT_HVC;
-		} else if (!strcmp("smc", method)) {
-			sdei_firmware_call = &sdei_smccc_smc;
-			return SMCCC_CONDUIT_SMC;
-		}
-
-		pr_warn("invalid \"method\" property: %s\n", method);
-	} else if (IS_ENABLED(CONFIG_ACPI) && !acpi_disabled) {
-		if (acpi_psci_use_hvc()) {
-			sdei_firmware_call = &sdei_smccc_hvc;
-			return SMCCC_CONDUIT_HVC;
-		} else {
-			sdei_firmware_call = &sdei_smccc_smc;
-			return SMCCC_CONDUIT_SMC;
-		}
-	}
-
-	return SMCCC_CONDUIT_NONE;
-}
-
 static int sdei_probe(struct platform_device *pdev)
 {
 	int err;
 	u64 ver = 0;
 	int conduit;
 
-	conduit = sdei_get_conduit(pdev);
-	if (!sdei_firmware_call)
-		return 0;
+	conduit = acpi_psci_use_hvc() ? SMCCC_CONDUIT_HVC : SMCCC_CONDUIT_SMC;
+	sdei_firmware_call = acpi_psci_use_hvc() ?
+			     &sdei_smccc_hvc : &sdei_smccc_smc;
 
 	err = sdei_api_get_version(&ver);
 	if (err == -EOPNOTSUPP)
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 06/14] drivers/firmware/sdei: Drop redundant error message in sdei_probe()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (4 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 05/14] drivers/firmware/sdei: Remove sdei_get_conduit() Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-06  5:47 ` [PATCH 07/14] drivers/firmware/sdei: Drop unnecessary while loop Gavin Shan
                   ` (8 subsequent siblings)
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

This drops the redundant error message in sdei_probe() because the
situation can be indicated by the error number from next error
message.

The function scoped variables are reorder according to their
importance by the way.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index ca0e3f5eb907..28bc712ba71d 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -935,17 +935,15 @@ int sdei_unregister_ghes(struct ghes *ghes)
 
 static int sdei_probe(struct platform_device *pdev)
 {
-	int err;
-	u64 ver = 0;
 	int conduit;
+	u64 ver = 0;
+	int err;
 
 	conduit = acpi_psci_use_hvc() ? SMCCC_CONDUIT_HVC : SMCCC_CONDUIT_SMC;
 	sdei_firmware_call = acpi_psci_use_hvc() ?
 			     &sdei_smccc_hvc : &sdei_smccc_smc;
 
 	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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 07/14] drivers/firmware/sdei: Drop unnecessary while loop
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (5 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 06/14] drivers/firmware/sdei: Drop redundant error message in sdei_probe() Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-06  5:47 ` [PATCH 08/14] drivers/firmware/sdei: Cleanup on cross call functions Gavin Shan
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

There are two unnecessary "do { ... } while (0);" in the function
sdei_event_{register, unregister} separately. This just removes them
to avoid them to make the code a bit cleaner. This shouldn't cause
any logical changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 86 ++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 28bc712ba71d..e3efea40df88 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -487,26 +487,25 @@ 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 out;
+	}
 
-		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 out;
 
-		sdei_event_destroy(event);
-	} while (0);
-	mutex_unlock(&sdei_events_lock);
+	sdei_event_destroy(event);
 
+out:
+	mutex_unlock(&sdei_events_lock);
 	return err;
 }
 
@@ -586,36 +585,37 @@ 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 out;
+	}
 
-		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 out;
+	}
 
-		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();
+out:
+	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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 08/14] drivers/firmware/sdei: Cleanup on cross call functions
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (6 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 07/14] drivers/firmware/sdei: Drop unnecessary while loop Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-06  5:47 ` [PATCH 09/14] drivers/firmware/sdei: Introduce sdei_do_local_call() Gavin Shan
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

This applies cleanup on the corss call functions, no functional
changes is introduced:

   * Cleanup struct sdei_crosscall_arg to use tab between fields
     and their types.
   * Move sdei_cross_call_return() ahead of sdei_do_cross_call().
   * Refactor CROSSCALL_INIT to use "do { ... } while (0)".

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index e3efea40df88..3393f1650b20 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -73,14 +73,24 @@ static LIST_HEAD(sdei_list);
 
 /* Private events are registered/enabled via IPI passing one of these */
 struct sdei_crosscall_args {
-	struct sdei_event *event;
-	atomic_t errors;
-	int first_error;
+	struct sdei_event	*event;
+	atomic_t		errors;
+	int			first_error;
 };
 
-#define CROSSCALL_INIT(arg, event)	(arg.event = event, \
-					 arg.first_error = 0, \
-					 atomic_set(&arg.errors, 0))
+#define CROSSCALL_INIT(arg, event)		\
+	do {					\
+		arg.event = event;		\
+		arg.first_error = 0;		\
+		atomic_set(&arg.errors, 0);	\
+	} while (0)
+
+static inline void
+sdei_cross_call_return(struct sdei_crosscall_args *arg, int err)
+{
+	if (err && (atomic_inc_return(&arg->errors) == 1))
+		arg->first_error = err;
+}
 
 static inline int sdei_do_cross_call(void *fn, struct sdei_event * event)
 {
@@ -92,13 +102,6 @@ static inline int sdei_do_cross_call(void *fn, struct sdei_event * event)
 	return arg.first_error;
 }
 
-static inline void
-sdei_cross_call_return(struct sdei_crosscall_args *arg, int err)
-{
-	if (err && (atomic_inc_return(&arg->errors) == 1))
-		arg->first_error = err;
-}
-
 static int sdei_to_linux_errno(unsigned long sdei_err)
 {
 	switch (sdei_err) {
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 09/14] drivers/firmware/sdei: Introduce sdei_do_local_call()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (7 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 08/14] drivers/firmware/sdei: Cleanup on cross call functions Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-06  5:47 ` [PATCH 10/14] drivers/firmware/sdei: Remove _sdei_event_register() Gavin Shan
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

For the private events, there are codes to repeat the same steps:
initializing cross call argument, make function call on local CPU,
check the returned error.

This introduces sdei_do_local_call() helper to cover the first two
steps. Another benefit is to make CROSSCALL_INIT and sdei_crosscall_args
only visible to sddi_do_{cross, local}_call().

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 42 +++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 3393f1650b20..6a583eb34222 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -102,6 +102,18 @@ static inline int sdei_do_cross_call(void *fn, struct sdei_event * event)
 	return arg.first_error;
 }
 
+static inline int sdei_do_local_call(void *fn,
+				     struct sdei_event *event)
+{
+	struct sdei_crosscall_args arg;
+	void (*func)(void *) = fn;
+
+	CROSSCALL_INIT(arg, event);
+	func(&arg);
+
+	return arg.first_error;
+}
+
 static int sdei_to_linux_errno(unsigned long sdei_err)
 {
 	switch (sdei_err) {
@@ -675,7 +687,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);
@@ -683,12 +695,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);
 
@@ -698,7 +709,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);
@@ -707,20 +718,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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 10/14] drivers/firmware/sdei: Remove _sdei_event_register()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (8 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 09/14] drivers/firmware/sdei: Introduce sdei_do_local_call() Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-06  5:47 ` [PATCH 11/14] drivers/firmware/sdei: Remove _sdei_event_unregister() Gavin Shan
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

The function _sdei_event_register() is called by sdei_event_register()
and sdei_device_thaw() as the following functional call chain shows.
The different set of reregistration and reenablement cpu mask will be
updted once it's going to be supported. Also, the shared events are
only taken cared by sdei_device_thaw(), but _sdei_event_register()
covers both privte and shared events. So the logic about the private
events in _sdei_event_register() is useless to sdei_device_thaw().

   sdei_event_register          sdei_device_thaw
      _sdei_event_register         sdei_reregister_shared
                                      sdei_reregister_event_llocked
                                         _sdei_event_register

This removes _sdei_event_register(). After it's removed, another
function (sdei_reregister_event_llocked()) is also removed as its
only caller is sdei_reregister_shared(). This shouldn't cause any
logicial changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 81 ++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 50 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 6a583eb34222..2530b8565267 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -573,25 +573,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;
@@ -615,7 +596,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,
@@ -634,50 +625,40 @@ 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;
 	struct sdei_event *event;
+	int err = 0;
 
 	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)
 			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);
 	mutex_unlock(&sdei_events_lock);
 
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 11/14] drivers/firmware/sdei: Remove _sdei_event_unregister()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (9 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 10/14] drivers/firmware/sdei: Remove _sdei_event_register() Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-06  5:47 ` [PATCH 12/14] drivers/firmware/sdei: Identify event by struct sdei_event Gavin Shan
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

The function is called like the following function call chain shows.
The shared events are unregistered only by sdei_device_freeze().
So the logic to unregister the private events in _sdei_event_unregister()
is useless to sdei_device_freeze().

   sdei_event_unregister        sdei_device_freeze
      _sdei_event_unregister       sdei_unregister_shared
                                     _sdei_event_unregister

This removes _sdei_event_unregister(), which is similar to what we
did for _sdei_event_register(). It shouldn't cause logicial changes.

Signed-off-by: Gavin Shan <gshan@redhat.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 2530b8565267..8f53bef88379 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -483,16 +483,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;
@@ -513,7 +503,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 out;
 
@@ -539,7 +533,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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 12/14] drivers/firmware/sdei: Identify event by struct sdei_event
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (10 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 11/14] drivers/firmware/sdei: Remove _sdei_event_unregister() Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-06  5:47 ` [PATCH 13/14] drivers/firmware/sdei: Retrieve event signaled property on creation Gavin Shan
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

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 according 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>
---
 drivers/firmware/arm_sdei.c | 145 +++++++++++++++++-------------------
 include/linux/arm_sdei.h    |  71 ++++++++++--------
 2 files changed, 108 insertions(+), 108 deletions(-)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 8f53bef88379..8e5f6683c155 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -44,26 +44,6 @@ 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 {
-	/* 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 */
-		struct sdei_registered_event *registered;
-
-		/* CPU private events */
-		struct sdei_registered_event __percpu *private_registered;
-	};
-};
-
 /* Take the mutex for any API call or modification. Take the mutex first. */
 static DEFINE_MUTEX(sdei_events_lock);
 
@@ -393,30 +373,26 @@ static void _local_event_enable(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_enable(u32 event_num)
+int sdei_event_enable(struct sdei_event *event)
 {
 	int err = -EINVAL;
-	struct sdei_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->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) {
-		spin_lock(&sdei_list_lock);
-		event->reenable = true;
-		spin_unlock(&sdei_list_lock);
-	}
+	if (err)
+		goto cpu_unlock;
+
+	spin_lock(&sdei_list_lock);
+	event->reenable = true;
+	spin_unlock(&sdei_list_lock);
+
+cpu_unlock:
 	cpus_read_unlock();
 	mutex_unlock(&sdei_events_lock);
 
@@ -439,28 +415,26 @@ static void _ipi_event_disable(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_disable(u32 event_num)
+int sdei_event_disable(struct sdei_event *event)
 {
 	int err = -EINVAL;
-	struct sdei_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);
 
 	if (event->type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_disable(event->event_num);
 	else
 		err = sdei_do_cross_call(_ipi_event_disable, event);
-	mutex_unlock(&sdei_events_lock);
 
+	if (err)
+		goto out;
+
+	spin_lock(&sdei_list_lock);
+	event->reenable = false;
+	spin_unlock(&sdei_list_lock);
+
+out:
+	mutex_unlock(&sdei_events_lock);
 	return err;
 }
 
@@ -483,25 +457,13 @@ static void _local_event_unregister(void *data)
 	sdei_cross_call_return(arg, err);
 }
 
-int sdei_event_unregister(u32 event_num)
+int sdei_event_unregister(struct sdei_event *event)
 {
 	int err;
-	struct sdei_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 out;
-	}
-
-	spin_lock(&sdei_list_lock);
-	event->reregister = false;
-	event->reenable = false;
-	spin_unlock(&sdei_list_lock);
 
 	if (event->type == SDEI_EVENT_TYPE_SHARED)
 		err = sdei_api_event_unregister(event->event_num);
@@ -511,6 +473,11 @@ int sdei_event_unregister(u32 event_num)
 	if (err)
 		goto out;
 
+	spin_lock(&sdei_list_lock);
+	event->reregister = false;
+	event->reenable = false;
+	spin_unlock(&sdei_list_lock);
+
 	sdei_event_destroy(event);
 
 out:
@@ -567,17 +534,18 @@ 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)
 {
+	struct sdei_event *event = NULL;
 	int err;
-	struct sdei_event *event;
 
 	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;
+		event = ERR_PTR(-EBUSY);
 		goto out;
 	}
 
@@ -603,6 +571,7 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 
 	if (err) {
 		sdei_event_destroy(event);
+		event = ERR_PTR(err);
 		pr_warn("Failed to register event %u: %d\n", event_num,
 			err);
 		goto cpu_unlock;
@@ -616,7 +585,7 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
 	cpus_read_unlock();
 out:
 	mutex_unlock(&sdei_events_lock);
-	return err;
+	return event;
 }
 
 static int sdei_reregister_shared(void)
@@ -857,13 +826,16 @@ NOKPROBE_SYMBOL(sdei_smccc_hvc);
 int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
 		       sdei_event_callback *critical_cb)
 {
-	int err;
+	struct sdei_event *event;
 	u64 result;
 	u32 event_num;
 	sdei_event_callback *cb;
+	int err;
 
-	if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES))
-		return -EOPNOTSUPP;
+	if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES)) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
 
 	event_num = ghes->generic->notify.vector;
 	if (event_num == 0) {
@@ -871,53 +843,70 @@ int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
 		 * Event 0 is reserved by the specification for
 		 * SDEI_EVENT_SIGNAL.
 		 */
-		return -EINVAL;
+		err = -EINVAL;
+		goto out;
 	}
 
 	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_PRIORITY,
 				      &result);
 	if (err)
-		return err;
+		goto out;
 
 	if (result == SDEI_EVENT_PRIORITY_CRITICAL)
 		cb = critical_cb;
 	else
 		cb = normal_cb;
 
-	err = sdei_event_register(event_num, cb, ghes);
-	if (!err)
-		err = sdei_event_enable(event_num);
+	event = sdei_event_register(event_num, cb, ghes);
+	if (IS_ERR(event)) {
+		err = PTR_ERR(event);
+		goto out;
+	}
 
+	err = sdei_event_enable(event);
+
+out:
 	return err;
 }
 
 int sdei_unregister_ghes(struct ghes *ghes)
 {
-	int i;
-	int err;
+	struct sdei_event *event;
 	u32 event_num = ghes->generic->notify.vector;
+	int err, i;
 
 	might_sleep();
 
-	if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES))
-		return -EOPNOTSUPP;
+	if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES)) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	mutex_lock(&sdei_events_lock);
+	event = sdei_event_find(event_num);
+	mutex_unlock(&sdei_events_lock);
+	if (!event) {
+		err = -ENOENT;
+		goto out;
+	}
 
 	/*
 	 * 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);
 	if (err)
-		return err;
+		goto out;
 
 	for (i = 0; i < 3; i++) {
-		err = sdei_event_unregister(event_num);
+		err = sdei_event_unregister(event);
 		if (err != -EINPROGRESS)
 			break;
 
 		schedule();
 	}
 
+out:
 	return err;
 }
 
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index 0a241c5c911d..fb6aa455e51d 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -22,36 +22,6 @@
  */
 typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
 
-/*
- * 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);
-
-/*
- * Calls to sdei_event_unregister() may return EINPROGRESS. Keep calling
- * it until it succeeds.
- */
-int sdei_event_unregister(u32 event_num);
-
-int sdei_event_enable(u32 event_num);
-int sdei_event_disable(u32 event_num);
-
-/* GHES register/unregister helpers */
-int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
-		       sdei_event_callback *critical_cb);
-int sdei_unregister_ghes(struct ghes *ghes);
-
-#ifdef CONFIG_ARM_SDE_INTERFACE
-/* For use by arch code when CPU hotplug notifiers are not appropriate. */
-int sdei_mask_local_cpu(void);
-int sdei_unmask_local_cpu(void);
-#else
-static inline int sdei_mask_local_cpu(void) { return 0; }
-static inline int sdei_unmask_local_cpu(void) { return 0; }
-#endif /* CONFIG_ARM_SDE_INTERFACE */
-
-
 /*
  * This struct represents an event that has been registered. The driver
  * maintains a list of all events, and which ones are registered. (Private
@@ -72,6 +42,47 @@ struct sdei_registered_event {
 	u8			 priority;
 };
 
+struct sdei_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 */
+		struct sdei_registered_event *registered;
+
+		/* CPU private events */
+		struct sdei_registered_event __percpu *private_registered;
+	};
+};
+
+/* APIs */
+struct sdei_event *sdei_event_register(u32 event_num,
+				       sdei_event_callback *cb, void *arg);
+int sdei_event_unregister(struct sdei_event *event);
+int sdei_event_enable(struct sdei_event *event);
+int sdei_event_disable(struct sdei_event *event);
+
+/* GHES register/unregister helpers */
+int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
+		       sdei_event_callback *critical_cb);
+int sdei_unregister_ghes(struct ghes *ghes);
+
+#ifdef CONFIG_ARM_SDE_INTERFACE
+/* For use by arch code when CPU hotplug notifiers are not appropriate. */
+int sdei_mask_local_cpu(void);
+int sdei_unmask_local_cpu(void);
+#else
+static inline int sdei_mask_local_cpu(void) { return 0; }
+static inline int sdei_unmask_local_cpu(void) { return 0; }
+#endif /* CONFIG_ARM_SDE_INTERFACE */
+
 /* The arch code entry point should then call this when an event arrives. */
 int notrace sdei_event_handler(struct pt_regs *regs,
 			       struct sdei_registered_event *arg);
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 13/14] drivers/firmware/sdei: Retrieve event signaled property on creation
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (11 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 12/14] drivers/firmware/sdei: Identify event by struct sdei_event Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-06  5:47 ` [PATCH 14/14] drivers/firmware/sdei: Add sdei_event_get_info() Gavin Shan
  2020-07-21  9:44 ` [PATCH 00/14] Refactor SDEI client driver Gavin Shan
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

This retrieves the event signaled property when it's created for the
first time. The property will be needed when SDEI virtualization is
supported.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 6 ++++++
 include/linux/arm_sdei.h    | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 8e5f6683c155..c6bd43914ea0 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -205,6 +205,12 @@ static struct sdei_event *sdei_event_create(u32 event_num,
 		goto fail;
 	event->type = result;
 
+	err = sdei_api_event_get_info(event_num, SDEI_EVENT_INFO_EV_SIGNALED,
+				      &result);
+	if (err)
+		goto fail;
+	event->signaled = result;
+
 	if (event->type == SDEI_EVENT_TYPE_SHARED) {
 		event->registered = kzalloc(sizeof(*reg), GFP_KERNEL);
 		if (!event->registered) {
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index fb6aa455e51d..36003751e26e 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -51,6 +51,7 @@ struct sdei_event {
 	u32			event_num;
 	u8			type;
 	u8			priority;
+	u8			signaled;
 
 	/* This pointer is handed to firmware as the event argument. */
 	union {
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* [PATCH 14/14] drivers/firmware/sdei: Add sdei_event_get_info()
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (12 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 13/14] drivers/firmware/sdei: Retrieve event signaled property on creation Gavin Shan
@ 2020-07-06  5:47 ` Gavin Shan
  2020-07-21  9:44 ` [PATCH 00/14] Refactor SDEI client driver Gavin Shan
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-06  5:47 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas

This adds API sdei_event_get_info(), to be used when virtualized
SDEI is supported to retrieve the information about the specified
event.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 drivers/firmware/arm_sdei.c | 13 +++++++++++++
 include/linux/arm_sdei.h    |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index c6bd43914ea0..d5aadf99d0c1 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -173,6 +173,19 @@ static int sdei_api_event_get_info(u32 event, u32 info, u64 *result)
 			      0, 0, result);
 }
 
+int sdei_event_get_info(u32 event, u32 info, u64 *result)
+{
+	int err;
+
+	mutex_lock(&sdei_events_lock);
+
+	err = sdei_api_event_get_info(event, info, result);
+
+	mutex_unlock(&sdei_events_lock);
+
+	return err;
+}
+
 static struct sdei_event *sdei_event_create(u32 event_num,
 					    sdei_event_callback *cb,
 					    void *cb_arg)
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index 36003751e26e..11dd66273209 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -64,6 +64,7 @@ struct sdei_event {
 };
 
 /* APIs */
+int sdei_event_get_info(u32 event, u32 info, u64 *result);
 struct sdei_event *sdei_event_register(u32 event_num,
 				       sdei_event_callback *cb, void *arg);
 int sdei_event_unregister(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

^ permalink raw reply related	[flat|nested] 26+ messages in thread
* Re: [PATCH 00/14] Refactor SDEI client driver
  2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
                   ` (13 preceding siblings ...)
  2020-07-06  5:47 ` [PATCH 14/14] drivers/firmware/sdei: Add sdei_event_get_info() Gavin Shan
@ 2020-07-21  9:44 ` Gavin Shan
  14 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-21  9:44 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: mark.rutland, james.morse, will, shan.gavin, catalin.marinas

On 7/6/20 3:47 PM, Gavin Shan wrote:
> This series bases on 5.8-rc4 and refactors the SDEI client driver.
> It's the preparatory work of virtualizing SDEI afterwords. The series
> is organized as below:
> 
>     * PATCH[1-11] refactors and clean up the code. They shouldn't cause
>       any functional changes.
>     * PATCH[12] 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.
>     * PATCH[13] retrieves the signaled property of the SDEI event when it's
>       populated. It's needed by SDEI virtualization afterwards.
>     * PATCH[14] exposes API (sdei_event_get_info()), which is needed by SDEI
>       virtualization either.
> 

The code can be checked out from github:

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

Thanks,
Gavin

> 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.
> 
> Gavin Shan (14):
>    drivers/firmware/sdei: Remove sdei_is_err()
>    drivers/firmware/sdei: Common block for failing path in
>      sdei_event_create()
>    drivers/firmware/sdei: Dereference SDEI event parameter directly
>    drivers/firmware/sdei: Rework sdei_init()
>    drivers/firmware/sdei: Remove sdei_get_conduit()
>    drivers/firmware/sdei: Drop redundant error message in sdei_probe()
>    drivers/firmware/sdei: Drop unnecessary while loop
>    drivers/firmware/sdei: Cleanup on cross call functions
>    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: Identify event by struct sdei_event
>    drivers/firmware/sdei: Retrieve event signaled property on creation
>    drivers/firmware/sdei: Add sdei_event_get_info()
> 
>   drivers/firmware/arm_sdei.c | 536 ++++++++++++++++--------------------
>   include/linux/arm_sdei.h    |  73 +++--
>   2 files changed, 284 insertions(+), 325 deletions(-)
> 


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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 01/14] drivers/firmware/sdei: Remove sdei_is_err()
  2020-07-06  5:47 ` [PATCH 01/14] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
@ 2020-07-21 20:39   ` James Morse
  2020-07-22  2:04     ` Gavin Shan
  0 siblings, 1 reply; 26+ messages in thread
From: James Morse @ 2020-07-21 20:39 UTC (permalink / raw)
  To: Gavin Shan
  Cc: mark.rutland, catalin.marinas, will, shan.gavin, linux-arm-kernel

Hi Gavin,

On 06/07/2020 06:47, Gavin Shan wrote:
> sdei_is_err() is only called by sdei_to_linux_errno().

... so it is! There were a lot more of these out of tree when it was being used to test
the firmware.


> 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(). 

> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index e7e36aab2386..415c243a8e65 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,
> @@ -147,8 +128,7 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,

Please also remove the now-pointless initialiser at the top of the function.


>  	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
> 

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


Thanks,

James

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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 02/14] drivers/firmware/sdei: Common block for failing path in sdei_event_create()
  2020-07-06  5:47 ` [PATCH 02/14] drivers/firmware/sdei: Common block for failing path in sdei_event_create() Gavin Shan
@ 2020-07-21 20:40   ` James Morse
  2020-07-22  2:12     ` Gavin Shan
  0 siblings, 1 reply; 26+ messages in thread
From: James Morse @ 2020-07-21 20:40 UTC (permalink / raw)
  To: Gavin Shan
  Cc: mark.rutland, catalin.marinas, will, shan.gavin, linux-arm-kernel

Hi Gavin,

On 06/07/2020 06:47, Gavin Shan wrote:
> The failing path in sdei_event_create() is to free SDEI event and
> return the corresponding error. This introduces common block of
> code for that to avoid duplicated logics.

By replacing it with slightly different duplicated logic?
If there were some kind of state to unwind here, I'd agree a single block to return from
is less likely to have bugs. Until then, this change is just churn.


> By the way, the function
> scoped variables are also reordered according to their importance.

Please: Never do this.

The 'order' is usually to make it easier to resolve merge conflicts. If two people add an
entry, at the same time. Its normally a 'fir' tree, and all that really matters is its
consistent. This change is just churn.


No Thanks,

James

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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 03/14] drivers/firmware/sdei: Dereference SDEI event parameter directly
  2020-07-06  5:47 ` [PATCH 03/14] drivers/firmware/sdei: Dereference SDEI event parameter directly Gavin Shan
@ 2020-07-21 20:41   ` James Morse
  2020-07-22  2:38     ` Gavin Shan
  0 siblings, 1 reply; 26+ messages in thread
From: James Morse @ 2020-07-21 20:41 UTC (permalink / raw)
  To: Gavin Shan
  Cc: mark.rutland, catalin.marinas, will, shan.gavin, linux-arm-kernel

Hi Gavin,

On 06/07/2020 06:47, Gavin Shan wrote:
> The SDEI parameter is dereferenced by @sdei_event->registered or
> @sdei_event->private_registered, depending on the event type.

> They
> can be dereferenced directly so that the intermediate variable
> (@regs) can be removed.

Unless its there to make it obvious that the shared/private blocks of this are doing the
same work.


> It makes the code looks a bit simplified.

I disagree.


> Also, @event->event_num instead of @event_num is used for retrieving
> the event number for the shared event, which makes the code similar
> to the case of private event.

Sure, this is an an inconsistency that could be changed. But I don't think its worth the
effort!


> Besides, the local scoped variables
> are reordered according to their importance by the way.

Please: never do this.


Thanks,

James

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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init()
  2020-07-06  5:47 ` [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init() Gavin Shan
@ 2020-07-21 20:42   ` James Morse
  2020-07-22  3:34     ` Gavin Shan
  0 siblings, 1 reply; 26+ messages in thread
From: James Morse @ 2020-07-21 20:42 UTC (permalink / raw)
  To: Gavin Shan
  Cc: mark.rutland, catalin.marinas, will, shan.gavin, linux-arm-kernel

Hi Gavin,

On 06/07/2020 06:47, Gavin Shan wrote:
> This reworks sdei_init()
> 
>    * The function follows the steps in sequence: check ACPI existence,
>      register platform device, register platform driver.
>    * The corresponding error numbers are returned in failing paths.
>    * The platform device is deleted if the driver can't be registered.

What is your motivation for the change?

The commit message should describe the problem you are solving, and why you are doing it
this way.


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 35a319e7e1e6..7e7b26b1f91b 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -1058,7 +1058,7 @@ static bool __init sdei_present_acpi(void)
>  	acpi_status status;
>  	struct acpi_table_header *sdei_table_header;
>  
> -	if (acpi_disabled)
> +	if (!IS_ENABLED(CONFIG_ACPI) || acpi_disabled)
>  		return false;
>  

This was already covered. From include/linux/acpi.h:
| #else	/* !CONFIG_ACPI */
|
| #define acpi_disabled 1


>  	status = acpi_get_table(ACPI_SIG_SDEI, 0, &sdei_table_header);
> @@ -1077,19 +1077,27 @@ static bool __init sdei_present_acpi(void)
>  
>  static int __init sdei_init(void)
>  {
> -	int ret = platform_driver_register(&sdei_driver);
> +	struct platform_device *pdev;
> +	int ret;
>  
> -	if (!ret && sdei_present_acpi()) {
> -		struct platform_device *pdev;
> +	if (!sdei_present_acpi())
> +		return -EPERM;
>  
> -		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));

> +	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 -ENOMEM;
>  	}

This will break DT platforms. Not everything in the world is ACPI.


> -	return ret;
> +	ret = platform_driver_register(&sdei_driver);
> +	if (ret) {
> +		platform_device_del(pdev);

The platform device is not unregistered on APCI platforms because it does not get
unregistered on DT platforms either. When using DT its created by the OF core code when it
probes the '/firmware' node of the DT.


> +		return ret;
> +	}
> +
> +	return 0;
>  }

Without a motivation in the commit message, I can't work out if there is something here,
or its just churn.



Thanks,

James

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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 05/14] drivers/firmware/sdei: Remove sdei_get_conduit()
  2020-07-06  5:47 ` [PATCH 05/14] drivers/firmware/sdei: Remove sdei_get_conduit() Gavin Shan
@ 2020-07-21 20:42   ` James Morse
  2020-07-22  3:50     ` Gavin Shan
  0 siblings, 1 reply; 26+ messages in thread
From: James Morse @ 2020-07-21 20:42 UTC (permalink / raw)
  To: Gavin Shan, linux-arm-kernel
  Cc: mark.rutland, catalin.marinas, will, shan.gavin

Hi Gavin,

On 06/07/2020 06:47, Gavin Shan wrote:
> There are some logics in sdei_get_conduit() can be safely dropped:
> 
>    * There are no associated device node with the platform device,
>      so it's pointless to check on it.

This is for DT. Its looking up the conduit in the binding. See
Documentation/devicetree/bindings/arm/firmware/sdei.txt.


>    * ACPI functionality has been verified when the platform device
>      is added in sdei_init(). So it's unnecessary to recheck.

This is so that a kernel built without ACPI support can have all that code removed at
compile time. The check appears repeatedly to ensure the compiler knows this is dead code.


Thanks,

James

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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 01/14] drivers/firmware/sdei: Remove sdei_is_err()
  2020-07-21 20:39   ` James Morse
@ 2020-07-22  2:04     ` Gavin Shan
  0 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-22  2:04 UTC (permalink / raw)
  To: James Morse
  Cc: mark.rutland, catalin.marinas, will, shan.gavin, linux-arm-kernel

Hi James,

On 7/22/20 6:39 AM, James Morse wrote:
> On 06/07/2020 06:47, Gavin Shan wrote:
>> sdei_is_err() is only called by sdei_to_linux_errno().
> 
> ... so it is! There were a lot more of these out of tree when it was being used to test
> the firmware.
> 

Ok, thanks for the explanation.

> 
>> 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().
> 
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index e7e36aab2386..415c243a8e65 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,
>> @@ -147,8 +128,7 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
> 
> Please also remove the now-pointless initialiser at the top of the function.
> 

Sure, the assignment of @err to zero will be dropped in v2.

> 
>>   	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
>>
> 
> With that:
> Reviewed-by: James Morse <james.morse@arm.com>
> 

Thanks for your review.

Thanks,
Gavin



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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 02/14] drivers/firmware/sdei: Common block for failing path in sdei_event_create()
  2020-07-21 20:40   ` James Morse
@ 2020-07-22  2:12     ` Gavin Shan
  0 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-22  2:12 UTC (permalink / raw)
  To: James Morse
  Cc: mark.rutland, catalin.marinas, will, shan.gavin, linux-arm-kernel

Hi James,

On 7/22/20 6:40 AM, James Morse wrote:
> On 06/07/2020 06:47, Gavin Shan wrote:
>> The failing path in sdei_event_create() is to free SDEI event and
>> return the corresponding error. This introduces common block of
>> code for that to avoid duplicated logics.
> 
> By replacing it with slightly different duplicated logic?
> If there were some kind of state to unwind here, I'd agree a single block to return from
> is less likely to have bugs. Until then, this change is just churn.
> 

The problem is there are multiple calls to kfree(event) in sdei_event_create().
It's prone to cause memory leakage when more code is added to the failing
path. To have common/single block to free @event and return errno can avoid
the situation as you mentioned. I will adjust the commit log to make it more
explicit in v2.

> 
>> By the way, the function
>> scoped variables are also reordered according to their importance.
> 
> Please: Never do this.
> 
> The 'order' is usually to make it easier to resolve merge conflicts. If two people add an
> entry, at the same time. Its normally a 'fir' tree, and all that really matters is its
> consistent. This change is just churn.
> 
> 
> No Thanks,
> 

Ok. I will drop this part of change in v2.

Thanks,
Gavin


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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 03/14] drivers/firmware/sdei: Dereference SDEI event parameter directly
  2020-07-21 20:41   ` James Morse
@ 2020-07-22  2:38     ` Gavin Shan
  0 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-22  2:38 UTC (permalink / raw)
  To: James Morse
  Cc: mark.rutland, catalin.marinas, will, shan.gavin, linux-arm-kernel

Hi James,

On 7/22/20 6:41 AM, James Morse wrote:
> On 06/07/2020 06:47, Gavin Shan wrote:
>> The SDEI parameter is dereferenced by @sdei_event->registered or
>> @sdei_event->private_registered, depending on the event type.
> 
>> They
>> can be dereferenced directly so that the intermediate variable
>> (@regs) can be removed.
> 
> Unless its there to make it obvious that the shared/private blocks of this are doing the
> same work.
> 
> 
>> It makes the code looks a bit simplified.
> 
> I disagree.
> 

Well, @regs can be removed and the scope of @reg is narrowed.
However, it might be not worthy for the effort. I will drop
this part of changes in v2.

> 
>> Also, @event->event_num instead of @event_num is used for retrieving
>> the event number for the shared event, which makes the code similar
>> to the case of private event.
> 
> Sure, this is an an inconsistency that could be changed. But I don't think its worth the
> effort!
> 

Yes, but I think it's still worthy to make the code consistent.
Lets keep this change in v2 only. All other changes will be
dropped.

> 
>> Besides, the local scoped variables
>> are reordered according to their importance by the way.
> 
> Please: never do this.
> 

Sure.

Thanks,
Gavin


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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init()
  2020-07-21 20:42   ` James Morse
@ 2020-07-22  3:34     ` Gavin Shan
  0 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-22  3:34 UTC (permalink / raw)
  To: James Morse
  Cc: mark.rutland, catalin.marinas, will, shan.gavin, linux-arm-kernel

Hi James,

On 7/22/20 6:42 AM, James Morse wrote:
> On 06/07/2020 06:47, Gavin Shan wrote:
>> This reworks sdei_init()
>>
>>     * The function follows the steps in sequence: check ACPI existence,
>>       register platform device, register platform driver.
>>     * The corresponding error numbers are returned in failing paths.
>>     * The platform device is deleted if the driver can't be registered.
> 
> What is your motivation for the change?
> 
> The commit message should describe the problem you are solving, and why you are doing it
> this way.
> 

Yep, the commit log isn't obvious. I will improve it in v2. I also
think it'd better to split the patch in v2.

drivers/firmware/sdei: Avoid nested statements in sdei_init()
drivers/firmware/sdei: Unregister driver on error in sdei_init()

There are 3 issues to be resolved and will be addressed in v2
commit log explicitly:

(1) The nest statements in sdei_init() can be avoided by bailing
     on error returned from platform_driver_register() or absent
     ACPI SDEI table.
(2) The platform driver isn't unregistered on failing to create
     the platform device. It means we're not restored to original
     state with this error. It's only true when ACPI is enabled,
     meaning we don't have the issue for DT case.
(3) The errno isn't updated accordingly on failing to create the
     platform device.

> 
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 35a319e7e1e6..7e7b26b1f91b 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -1058,7 +1058,7 @@ static bool __init sdei_present_acpi(void)
>>   	acpi_status status;
>>   	struct acpi_table_header *sdei_table_header;
>>   
>> -	if (acpi_disabled)
>> +	if (!IS_ENABLED(CONFIG_ACPI) || acpi_disabled)
>>   		return false;
>>   
> 
> This was already covered. From include/linux/acpi.h:
> | #else	/* !CONFIG_ACPI */
> |
> | #define acpi_disabled 1
> 

Yeah, I was thinking @acpi_disable isn't defined when CONFIG_ACPI
is off. Thanks for the linker :)

> 
>>   	status = acpi_get_table(ACPI_SIG_SDEI, 0, &sdei_table_header);
>> @@ -1077,19 +1077,27 @@ static bool __init sdei_present_acpi(void)
>>   
>>   static int __init sdei_init(void)
>>   {
>> -	int ret = platform_driver_register(&sdei_driver);
>> +	struct platform_device *pdev;
>> +	int ret;
>>   
>> -	if (!ret && sdei_present_acpi()) {
>> -		struct platform_device *pdev;
>> +	if (!sdei_present_acpi())
>> +		return -EPERM;
>>   
>> -		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));
> 
>> +	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 -ENOMEM;
>>   	}
> 
> This will break DT platforms. Not everything in the world is ACPI.
> 

Correct, I will split this patch into two patches in v2. The
issue will be fixed there.

> 
>> -	return ret;
>> +	ret = platform_driver_register(&sdei_driver);
>> +	if (ret) {
>> +		platform_device_del(pdev);
> 
> The platform device is not unregistered on APCI platforms because it does not get
> unregistered on DT platforms either. When using DT its created by the OF core code when it
> probes the '/firmware' node of the DT.
> 

Yes. I think the platform driver needs to be unregistered on
error to create the platform device when ACPI is enabled. With
it, we're restored to original state: no platform driver and
device.

For DT case, we needn't worry about the platform device because
it's not managed by the driver (arm_sdei.c).

> 
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>   }
> 
> Without a motivation in the commit message, I can't work out if there is something here,
> or its just churn.
> 

Sure, I will improve the commit log in v2 :)

Thanks,
Gavin


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

^ permalink raw reply	[flat|nested] 26+ messages in thread
* Re: [PATCH 05/14] drivers/firmware/sdei: Remove sdei_get_conduit()
  2020-07-21 20:42   ` James Morse
@ 2020-07-22  3:50     ` Gavin Shan
  0 siblings, 0 replies; 26+ messages in thread
From: Gavin Shan @ 2020-07-22  3:50 UTC (permalink / raw)
  To: James Morse, linux-arm-kernel
  Cc: mark.rutland, catalin.marinas, will, shan.gavin

Hi James,

On 7/22/20 6:42 AM, James Morse wrote:
> On 06/07/2020 06:47, Gavin Shan wrote:
>> There are some logics in sdei_get_conduit() can be safely dropped:
>>
>>     * There are no associated device node with the platform device,
>>       so it's pointless to check on it.
> 
> This is for DT. Its looking up the conduit in the binding. See
> Documentation/devicetree/bindings/arm/firmware/sdei.txt.
> 

Yeah, I obviously missed the DT case.

> 
>>     * ACPI functionality has been verified when the platform device
>>       is added in sdei_init(). So it's unnecessary to recheck.
> 
> This is so that a kernel built without ACPI support can have all that code removed at
> compile time. The check appears repeatedly to ensure the compiler knows this is dead code.
> 

Yes, this patch isn't meaningful and should be dropped except to
remove the duplicate checker on ACPI enablement in sdei_get_conduit()

     if (np) {
         :
     } else if (IS_ENABLED(CONFIG_ACPI) && !acpi_disabled) {
         :
     }

I will have new patch to replace this one, to remove the duplicate
checker in v2.

Thanks,
Gavin


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

^ permalink raw reply	[flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-07-22  3:52 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-06  5:47 [PATCH 00/14] Refactor SDEI client driver Gavin Shan
2020-07-06  5:47 ` [PATCH 01/14] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
2020-07-21 20:39   ` James Morse
2020-07-22  2:04     ` Gavin Shan
2020-07-06  5:47 ` [PATCH 02/14] drivers/firmware/sdei: Common block for failing path in sdei_event_create() Gavin Shan
2020-07-21 20:40   ` James Morse
2020-07-22  2:12     ` Gavin Shan
2020-07-06  5:47 ` [PATCH 03/14] drivers/firmware/sdei: Dereference SDEI event parameter directly Gavin Shan
2020-07-21 20:41   ` James Morse
2020-07-22  2:38     ` Gavin Shan
2020-07-06  5:47 ` [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init() Gavin Shan
2020-07-21 20:42   ` James Morse
2020-07-22  3:34     ` Gavin Shan
2020-07-06  5:47 ` [PATCH 05/14] drivers/firmware/sdei: Remove sdei_get_conduit() Gavin Shan
2020-07-21 20:42   ` James Morse
2020-07-22  3:50     ` Gavin Shan
2020-07-06  5:47 ` [PATCH 06/14] drivers/firmware/sdei: Drop redundant error message in sdei_probe() Gavin Shan
2020-07-06  5:47 ` [PATCH 07/14] drivers/firmware/sdei: Drop unnecessary while loop Gavin Shan
2020-07-06  5:47 ` [PATCH 08/14] drivers/firmware/sdei: Cleanup on cross call functions Gavin Shan
2020-07-06  5:47 ` [PATCH 09/14] drivers/firmware/sdei: Introduce sdei_do_local_call() Gavin Shan
2020-07-06  5:47 ` [PATCH 10/14] drivers/firmware/sdei: Remove _sdei_event_register() Gavin Shan
2020-07-06  5:47 ` [PATCH 11/14] drivers/firmware/sdei: Remove _sdei_event_unregister() Gavin Shan
2020-07-06  5:47 ` [PATCH 12/14] drivers/firmware/sdei: Identify event by struct sdei_event Gavin Shan
2020-07-06  5:47 ` [PATCH 13/14] drivers/firmware/sdei: Retrieve event signaled property on creation Gavin Shan
2020-07-06  5:47 ` [PATCH 14/14] drivers/firmware/sdei: Add sdei_event_get_info() Gavin Shan
2020-07-21  9:44 ` [PATCH 00/14] Refactor SDEI client driver Gavin Shan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.