* [PATCH v2 00/17] Refactor SDEI client driver
@ 2020-07-22 9:57 Gavin Shan
2020-07-22 9:57 ` [PATCH v2 01/17] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
` (16 more replies)
0 siblings, 17 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
This series bases on 5.8-rc6 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.
* PATCH[16] retrieves the signaled property of the SDEI event when it's
populated. It's needed by SDEI virtualization afterwards.
* PATCH[17] exposes API (sdei_event_get_info()), which is needed by SDEI
virtualization either.
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.
Changelog
=========
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 (17):
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: Move struct sdei_event to header file
drivers/firmware/sdei: Identify event by struct sdei_event
drivers/firmware/sdei: Retrieve event signaled property on
registration
drivers/firmware/sdei: Add sdei_event_get_info()
drivers/firmware/arm_sdei.c | 394 ++++++++++++++++--------------------
include/linux/arm_sdei.h | 73 ++++---
2 files changed, 224 insertions(+), 243 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] 40+ messages in thread
* [PATCH v2 01/17] drivers/firmware/sdei: Remove sdei_is_err()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 02/17] drivers/firmware/sdei: Common block for failing path in sdei_event_create() Gavin Shan
` (15 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 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(). 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>
---
v2: Remove the initializer for @err in invoke_sdei_fn()
---
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 02/17] drivers/firmware/sdei: Common block for failing path in sdei_event_create()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
2020-07-22 9:57 ` [PATCH v2 01/17] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 03/17] drivers/firmware/sdei: Retrieve event number from event instance Gavin Shan
` (14 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
v2: Improved commit log to make the movtivation explicit
Drop the part of changes to reorder the variables
---
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 03/17] drivers/firmware/sdei: Retrieve event number from event instance
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
2020-07-22 9:57 ` [PATCH v2 01/17] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
2020-07-22 9:57 ` [PATCH v2 02/17] drivers/firmware/sdei: Common block for failing path in sdei_event_create() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 04/17] drivers/firmware/sdei: Avoid nested statements in sdei_init() Gavin Shan
` (13 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
v2: Improved commit log
Dropped changes to remove @regs
Dropped changes to reorder variables
---
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 04/17] drivers/firmware/sdei: Avoid nested statements in sdei_init()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (2 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 03/17] drivers/firmware/sdei: Retrieve event number from event instance Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 05/17] drivers/firmware/sdei: Unregister driver on error " Gavin Shan
` (12 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
v2: Derived from "drivers/firmware/sdei: Rework sdei_init()"
---
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 05/17] drivers/firmware/sdei: Unregister driver on error in sdei_init()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (3 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 04/17] drivers/firmware/sdei: Avoid nested statements in sdei_init() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 06/17] drivers/firmware/sdei: Remove duplicate check in sdei_get_conduit() Gavin Shan
` (11 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
v2: Derived from "drivers/firmware/sdei: Rework sdei_init()"
Unregister platform driver instead of platform device
Remove duplicate ACPI enablement checker
Update errno (@ret) accordingly
---
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 06/17] drivers/firmware/sdei: Remove duplicate check in sdei_get_conduit()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (4 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 05/17] drivers/firmware/sdei: Unregister driver on error " Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 07/17] drivers/firmware/sdei: Remove Drop redundant error message in sdei_probe() Gavin Shan
` (10 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
v2: Derived from "drivers/firmware/sdei: Remove sdei_get_conduit()"
---
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 07/17] drivers/firmware/sdei: Remove Drop redundant error message in sdei_probe()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (5 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 06/17] drivers/firmware/sdei: Remove duplicate check in sdei_get_conduit() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 08/17] drivers/firmware/sdei: Remove while loop in sdei_event_register() Gavin Shan
` (9 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
v2: Remove changes to reorder variables
---
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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 08/17] drivers/firmware/sdei: Remove while loop in sdei_event_register()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (6 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 07/17] drivers/firmware/sdei: Remove Drop redundant error message in sdei_probe() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 09/17] drivers/firmware/sdei: Remove while loop in sdei_event_unregister() Gavin Shan
` (8 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
v2: Derived from "drivers/firmware/sdei: Drop unnecessary while loop"
---
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..ab1088fbdd37 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 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] 40+ messages in thread
* [PATCH v2 09/17] drivers/firmware/sdei: Remove while loop in sdei_event_unregister()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (7 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 08/17] drivers/firmware/sdei: Remove while loop in sdei_event_register() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-23 15:51 ` Jonathan Cameron
2020-07-22 9:57 ` [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function Gavin Shan
` (7 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
v2: Derived from "drivers/firmware/sdei: Drop unnecessary while loop"
---
drivers/firmware/arm_sdei.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index ab1088fbdd37..d03371161aaf 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -491,26 +491,24 @@ 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);
+ sdei_event_destroy(event);
+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] 40+ messages in thread
* [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (8 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 09/17] drivers/firmware/sdei: Remove while loop in sdei_event_unregister() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-23 15:52 ` Jonathan Cameron
2020-07-22 9:57 ` [PATCH v2 11/17] drivers/firmware/sdei: Introduce sdei_do_local_call() Gavin Shan
` (6 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 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
* Refactor CROSSCALL_INIT to use "do { ... } while (0)" to be
compatible with scripts/checkpatch.pl
* Remove unnecessary space before @event in sdei_do_cross_call()
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v2: Drop changes to sdei_cross_call_return()
Remove unnecessary space before @event in sdei_do_cross_call()
---
drivers/firmware/arm_sdei.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index d03371161aaf..04dc144a8f31 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -73,14 +73,17 @@ 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 int sdei_do_cross_call(void *fn, 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] 40+ messages in thread
* [PATCH v2 11/17] drivers/firmware/sdei: Introduce sdei_do_local_call()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (9 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-23 15:25 ` Jonathan Cameron
2020-07-22 9:57 ` [PATCH v2 12/17] drivers/firmware/sdei: Remove _sdei_event_register() Gavin Shan
` (5 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
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>
---
drivers/firmware/arm_sdei.c | 43 ++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 04dc144a8f31..da0e0d5591a8 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -85,7 +85,18 @@ struct sdei_crosscall_args {
atomic_set(&arg.errors, 0); \
} while (0)
-static inline int sdei_do_cross_call(void *fn, struct sdei_event * event)
+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 inline int sdei_do_cross_call(void *fn, struct sdei_event *event)
{
struct sdei_crosscall_args arg;
@@ -675,7 +686,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 +694,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 +708,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 +717,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] 40+ messages in thread
* [PATCH v2 12/17] drivers/firmware/sdei: Remove _sdei_event_register()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (10 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 11/17] drivers/firmware/sdei: Introduce sdei_do_local_call() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-23 15:25 ` Jonathan Cameron
2020-07-22 9:57 ` [PATCH v2 13/17] drivers/firmware/sdei: Remove _sdei_event_unregister() Gavin Shan
` (4 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 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.
_sdei_event_register() covers the shared and privte 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>
---
v2: Improved commit log
Drop changes to reorder the variables
---
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 da0e0d5591a8..f4cd9791242f 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -575,25 +575,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;
@@ -616,7 +597,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);
@@ -633,33 +624,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;
@@ -672,9 +636,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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 13/17] drivers/firmware/sdei: Remove _sdei_event_unregister()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (11 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 12/17] drivers/firmware/sdei: Remove _sdei_event_register() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file Gavin Shan
` (3 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
_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>
---
v2: Improved change log
---
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 f4cd9791242f..a52dcff59a20 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -486,16 +486,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;
@@ -516,7 +506,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;
@@ -541,7 +535,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] 40+ messages in thread
* [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (12 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 13/17] drivers/firmware/sdei: Remove _sdei_event_unregister() Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-23 15:19 ` Jonathan Cameron
2020-07-22 9:57 ` [PATCH v2 15/17] drivers/firmware/sdei: Identify event by struct sdei_event Gavin Shan
` (2 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 UTC (permalink / raw)
To: linux-arm-kernel
Cc: mark.rutland, will, james.morse, shan.gavin, catalin.marinas
This moves struct sdei_event to the header file so that it can be
dereferenced by external modules. This is needed by the code to
virtualize SDEI functionality, as part of the arm64/kvm.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v2: Derived from "drivers/firmware/sdei: Identify event by struct"
---
drivers/firmware/arm_sdei.c | 20 ------------
include/linux/arm_sdei.h | 61 ++++++++++++++++++++++++-------------
2 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index a52dcff59a20..bdd2de0149c0 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);
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index 0a241c5c911d..fdc2f868d84b 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -22,6 +22,46 @@
*/
typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
+/*
+ * This struct represents an event that has been registered. The driver
+ * maintains a list of all events, and which ones are registered. (Private
+ * events have one entry in the list, but are registered on each CPU).
+ * A pointer to this struct is passed to firmware, and back to the event
+ * handler. The event handler can then use this to invoke the registered
+ * callback, without having to walk the list.
+ *
+ * For CPU private events, this structure is per-cpu.
+ */
+struct sdei_registered_event {
+ /* For use by arch code: */
+ struct pt_regs interrupted_regs;
+
+ sdei_event_callback *callback;
+ void *callback_arg;
+ u32 event_num;
+ 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;
+ };
+};
+
/*
* Register your callback to claim an event. The event must be described
* by firmware.
@@ -51,27 +91,6 @@ 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
- * events have one entry in the list, but are registered on each CPU).
- * A pointer to this struct is passed to firmware, and back to the event
- * handler. The event handler can then use this to invoke the registered
- * callback, without having to walk the list.
- *
- * For CPU private events, this structure is per-cpu.
- */
-struct sdei_registered_event {
- /* For use by arch code: */
- struct pt_regs interrupted_regs;
-
- sdei_event_callback *callback;
- void *callback_arg;
- u32 event_num;
- u8 priority;
-};
-
/* 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] 40+ messages in thread
* [PATCH v2 15/17] drivers/firmware/sdei: Identify event by struct sdei_event
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (13 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration Gavin Shan
2020-07-22 9:57 ` [PATCH v2 17/17] drivers/firmware/sdei: Add sdei_event_get_info() Gavin Shan
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 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 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>
---
v2: Derived from "drivers/firmware/sdei: Identify event by struct"
---
drivers/firmware/arm_sdei.c | 60 +++++++++++++++++--------------------
include/linux/arm_sdei.h | 9 +++---
2 files changed, 32 insertions(+), 37 deletions(-)
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index bdd2de0149c0..cf10fec57f2a 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -376,18 +376,11 @@ 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)
@@ -422,17 +415,11 @@ 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;
@@ -466,20 +453,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;
@@ -549,7 +529,8 @@ 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_event *event;
@@ -559,14 +540,14 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg)
mutex_lock(&sdei_events_lock);
if (sdei_event_find(event_num)) {
pr_warn("Event %u already registered\n", event_num);
- err = -EBUSY;
+ event = ERR_PTR(-EBUSY);
goto 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);
+ pr_warn("Failed to create event %u: %ld\n",
+ event_num, PTR_ERR(event));
goto out;
}
@@ -584,6 +565,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;
}
@@ -595,7 +577,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)
@@ -838,6 +820,7 @@ int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
u64 result;
u32 event_num;
sdei_event_callback *cb;
+ struct sdei_event *event;
if (!IS_ENABLED(CONFIG_ACPI_APEI_GHES))
return -EOPNOTSUPP;
@@ -861,9 +844,13 @@ int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
else
cb = normal_cb;
- err = sdei_event_register(event_num, cb, ghes);
- if (!err)
- err = sdei_event_enable(event_num);
+ event = sdei_event_register(event_num, cb, ghes);
+ if (IS_ERR(event)) {
+ err = PTR_ERR(event);
+ return err;
+ }
+
+ err = sdei_event_enable(event);
return err;
}
@@ -873,22 +860,29 @@ int sdei_unregister_ghes(struct ghes *ghes)
int i;
int err;
u32 event_num = ghes->generic->notify.vector;
+ struct sdei_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);
if (err)
return err;
for (i = 0; i < 3; i++) {
- err = sdei_event_unregister(event_num);
+ err = sdei_event_unregister(event);
if (err != -EINPROGRESS)
break;
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index fdc2f868d84b..11af6410dd52 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -66,16 +66,17 @@ struct sdei_event {
* Register your callback to claim an event. The event must be described
* by firmware.
*/
-int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg);
+struct sdei_event *sdei_event_register(u32 event_num, sdei_event_callback *cb,
+ void *arg);
/*
* Calls to sdei_event_unregister() may return EINPROGRESS. Keep calling
* it until it succeeds.
*/
-int sdei_event_unregister(u32 event_num);
+int sdei_event_unregister(struct sdei_event *event);
-int sdei_event_enable(u32 event_num);
-int sdei_event_disable(u32 event_num);
+int sdei_event_enable(struct sdei_event *event);
+int sdei_event_disable(struct sdei_event *event);
/* GHES register/unregister helpers */
int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (14 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 15/17] drivers/firmware/sdei: Identify event by struct sdei_event Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
2020-07-23 15:24 ` Jonathan Cameron
2020-07-22 9:57 ` [PATCH v2 17/17] drivers/firmware/sdei: Add sdei_event_get_info() Gavin Shan
16 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 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 cf10fec57f2a..7518d3febf53 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -204,6 +204,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) {
reg = kzalloc(sizeof(*reg), GFP_KERNEL);
if (!reg) {
diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
index 11af6410dd52..7f3ed7e4b680 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] 40+ messages in thread
* [PATCH v2 17/17] drivers/firmware/sdei: Add sdei_event_get_info()
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
` (15 preceding siblings ...)
2020-07-22 9:57 ` [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration Gavin Shan
@ 2020-07-22 9:57 ` Gavin Shan
16 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-22 9:57 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 | 2 ++
2 files changed, 15 insertions(+)
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 7518d3febf53..cb7f8c6849a1 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -172,6 +172,19 @@ static int sdei_api_event_get_info(u32 event, u32 info, u64 *result)
0, 0, result);
}
+int sdei_event_get_info(u32 event_num, u32 info, u64 *result)
+{
+ int err;
+
+ mutex_lock(&sdei_events_lock);
+
+ err = sdei_api_event_get_info(event_num, 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 7f3ed7e4b680..93e372a1454c 100644
--- a/include/linux/arm_sdei.h
+++ b/include/linux/arm_sdei.h
@@ -63,6 +63,8 @@ struct sdei_event {
};
};
+int sdei_event_get_info(u32 event_num, u32 info, u64 *result);
+
/*
* 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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file
2020-07-22 9:57 ` [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file Gavin Shan
@ 2020-07-23 15:19 ` Jonathan Cameron
2020-07-27 0:46 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-23 15:19 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Wed, 22 Jul 2020 19:57:37 +1000
Gavin Shan <gshan@redhat.com> wrote:
> This moves struct sdei_event to the header file so that it can be
> dereferenced by external modules. This is needed by the code to
> virtualize SDEI functionality, as part of the arm64/kvm.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
Hi Gavin,
One question inline.
Jonathan
> ---
> v2: Derived from "drivers/firmware/sdei: Identify event by struct"
> ---
> drivers/firmware/arm_sdei.c | 20 ------------
> include/linux/arm_sdei.h | 61 ++++++++++++++++++++++++-------------
> 2 files changed, 40 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index a52dcff59a20..bdd2de0149c0 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);
>
> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> index 0a241c5c911d..fdc2f868d84b 100644
> --- a/include/linux/arm_sdei.h
> +++ b/include/linux/arm_sdei.h
> @@ -22,6 +22,46 @@
> */
> typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>
> +/*
> + * This struct represents an event that has been registered. The driver
> + * maintains a list of all events, and which ones are registered. (Private
> + * events have one entry in the list, but are registered on each CPU).
> + * A pointer to this struct is passed to firmware, and back to the event
> + * handler. The event handler can then use this to invoke the registered
> + * callback, without having to walk the list.
> + *
> + * For CPU private events, this structure is per-cpu.
> + */
> +struct sdei_registered_event {
> + /* For use by arch code: */
> + struct pt_regs interrupted_regs;
> +
> + sdei_event_callback *callback;
> + void *callback_arg;
> + u32 event_num;
> + u8 priority;
> +};
> +
> +struct sdei_event {
> + /* These three are protected by the sdei_list_lock */
As this patch leaves the sdei_list_lock as local to arm_sdei.c, is this comment still valid?
> + 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;
> + };
> +};
> +
> /*
> * Register your callback to claim an event. The event must be described
> * by firmware.
> @@ -51,27 +91,6 @@ 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
> - * events have one entry in the list, but are registered on each CPU).
> - * A pointer to this struct is passed to firmware, and back to the event
> - * handler. The event handler can then use this to invoke the registered
> - * callback, without having to walk the list.
> - *
> - * For CPU private events, this structure is per-cpu.
> - */
> -struct sdei_registered_event {
> - /* For use by arch code: */
> - struct pt_regs interrupted_regs;
> -
> - sdei_event_callback *callback;
> - void *callback_arg;
> - u32 event_num;
> - u8 priority;
> -};
> -
> /* 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);
_______________________________________________
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] 40+ messages in thread
* Re: [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration
2020-07-22 9:57 ` [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration Gavin Shan
@ 2020-07-23 15:24 ` Jonathan Cameron
2020-07-27 0:53 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-23 15:24 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Wed, 22 Jul 2020 19:57:39 +1000
Gavin Shan <gshan@redhat.com> wrote:
> 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>
These last two patches are probably fine but hard to tell without a user.
Jonathan
> ---
> 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 cf10fec57f2a..7518d3febf53 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -204,6 +204,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) {
> reg = kzalloc(sizeof(*reg), GFP_KERNEL);
> if (!reg) {
> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> index 11af6410dd52..7f3ed7e4b680 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 {
_______________________________________________
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] 40+ messages in thread
* Re: [PATCH v2 11/17] drivers/firmware/sdei: Introduce sdei_do_local_call()
2020-07-22 9:57 ` [PATCH v2 11/17] drivers/firmware/sdei: Introduce sdei_do_local_call() Gavin Shan
@ 2020-07-23 15:25 ` Jonathan Cameron
2020-07-27 0:41 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-23 15:25 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Wed, 22 Jul 2020 19:57:34 +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>
One comment inline.
Jonathan
> ---
> drivers/firmware/arm_sdei.c | 43 ++++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 04dc144a8f31..da0e0d5591a8 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -85,7 +85,18 @@ struct sdei_crosscall_args {
> atomic_set(&arg.errors, 0); \
> } while (0)
>
> -static inline int sdei_do_cross_call(void *fn, struct sdei_event * event)
> +static inline int sdei_do_local_call(void *fn, struct sdei_event *event)
Can we now give the function parameter it's full type and avoid the nasty
looking cast below?
> +{
> + struct sdei_crosscall_args arg;
> + void (*func)(void *) = fn;
> +
> + CROSSCALL_INIT(arg, event);
> + func(&arg);
> +
> + return arg.first_error;
> +}
> +
> +static inline int sdei_do_cross_call(void *fn, struct sdei_event *event)
> {
> struct sdei_crosscall_args arg;
>
> @@ -675,7 +686,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 +694,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 +708,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 +717,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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 12/17] drivers/firmware/sdei: Remove _sdei_event_register()
2020-07-22 9:57 ` [PATCH v2 12/17] drivers/firmware/sdei: Remove _sdei_event_register() Gavin Shan
@ 2020-07-23 15:25 ` Jonathan Cameron
2020-07-27 0:42 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-23 15:25 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Wed, 22 Jul 2020 19:57:35 +1000
Gavin Shan <gshan@redhat.com> wrote:
> The function _sdei_event_register() is called by sdei_event_register()
> and sdei_device_thaw() as the following functional call chain shows.
> _sdei_event_register() covers the shared and privte events, but
private
> 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>
Otherwise looks good to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v2: Improved commit log
> Drop changes to reorder the variables
> ---
> 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 da0e0d5591a8..f4cd9791242f 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -575,25 +575,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;
> @@ -616,7 +597,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);
> @@ -633,33 +624,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;
> @@ -672,9 +636,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);
_______________________________________________
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] 40+ messages in thread
* Re: [PATCH v2 09/17] drivers/firmware/sdei: Remove while loop in sdei_event_unregister()
2020-07-22 9:57 ` [PATCH v2 09/17] drivers/firmware/sdei: Remove while loop in sdei_event_unregister() Gavin Shan
@ 2020-07-23 15:51 ` Jonathan Cameron
2020-07-27 0:22 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-23 15:51 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Wed, 22 Jul 2020 19:57:32 +1000
Gavin Shan <gshan@redhat.com> wrote:
> 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>
> ---
> v2: Derived from "drivers/firmware/sdei: Drop unnecessary while loop"
> ---
> drivers/firmware/arm_sdei.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index ab1088fbdd37..d03371161aaf 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -491,26 +491,24 @@ 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);
> + sdei_event_destroy(event);
> +out:
As in previous I'd give the label a name to indicate there is still work
to be done.
> mutex_unlock(&sdei_events_lock);
> -
That whitespace is arguably slightly useful from a readability point of view.
I would leave it here.
> return err;
> }
>
_______________________________________________
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] 40+ messages in thread
* Re: [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function
2020-07-22 9:57 ` [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function Gavin Shan
@ 2020-07-23 15:52 ` Jonathan Cameron
2020-07-27 0:33 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-23 15:52 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Wed, 22 Jul 2020 19:57:33 +1000
Gavin Shan <gshan@redhat.com> wrote:
> This applies cleanup on the corss call functions, no functional
cross
> changes is introduced:
changes are introduced:
>
> * Cleanup struct sdei_crosscall_arg to use tab between fields
> and their types
Hmm. I guess there is a consistency argument for doing this but generally
that one doesn't look like it actually has much benefit.
> * Refactor CROSSCALL_INIT to use "do { ... } while (0)" to be
> compatible with scripts/checkpatch.pl
Good to tidy that up.
> * Remove unnecessary space before @event in sdei_do_cross_call()
Good change, but seems to have gotten lost in this patch.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> v2: Drop changes to sdei_cross_call_return()
> Remove unnecessary space before @event in sdei_do_cross_call()
> ---
> drivers/firmware/arm_sdei.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index d03371161aaf..04dc144a8f31 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -73,14 +73,17 @@ 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 int sdei_do_cross_call(void *fn, struct sdei_event * event)
> {
_______________________________________________
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] 40+ messages in thread
* Re: [PATCH v2 09/17] drivers/firmware/sdei: Remove while loop in sdei_event_unregister()
2020-07-23 15:51 ` Jonathan Cameron
@ 2020-07-27 0:22 ` Gavin Shan
0 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 0:22 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/24/20 1:51 AM, Jonathan Cameron wrote:
> On Wed, 22 Jul 2020 19:57:32 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
>> 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>
>> ---
>> v2: Derived from "drivers/firmware/sdei: Drop unnecessary while loop"
>> ---
>> drivers/firmware/arm_sdei.c | 30 ++++++++++++++----------------
>> 1 file changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index ab1088fbdd37..d03371161aaf 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -491,26 +491,24 @@ 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);
>> + sdei_event_destroy(event);
>> +out:
> As in previous I'd give the label a name to indicate there is still work
> to be done.
>
Sure, I will rename this to "unlock" in v3. However, I would hold
v3 for a while until v2 is fully reviewed. Please take a look on
the left patches if you have more time, thanks for your time on
this series.
>> mutex_unlock(&sdei_events_lock);
>> -
> That whitespace is arguably slightly useful from a readability point of view.
> I would leave it here.
>
Ok. I will keep this in v3 then.
>> return err;
>> }
>>
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] 40+ messages in thread
* Re: [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function
2020-07-23 15:52 ` Jonathan Cameron
@ 2020-07-27 0:33 ` Gavin Shan
2020-07-27 8:58 ` Jonathan Cameron
0 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 0:33 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/24/20 1:52 AM, Jonathan Cameron wrote:
> On Wed, 22 Jul 2020 19:57:33 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
>> This applies cleanup on the corss call functions, no functional
>
> cross
>
It will be fixed in v3.
>> changes is introduced:
>
> changes are introduced:
>
It will be fixed in v3.
>>
>> * Cleanup struct sdei_crosscall_arg to use tab between fields
>> and their types
> Hmm. I guess there is a consistency argument for doing this but generally
> that one doesn't look like it actually has much benefit.
>
Sorry, I guess you're talking about "./scripts/cleanpatch"? As stated
in the head of the script, the result would be destructive. So I think
manual changes would be more reliable. Lets keep this in v3 if you agree.
Extracted from "scripts/cleanpatch":
# Clean a patch file -- or directory of patch files -- of stealth whitespace.
# WARNING: this can be a highly destructive operation. Use with caution.
#
>> * Refactor CROSSCALL_INIT to use "do { ... } while (0)" to be
>> compatible with scripts/checkpatch.pl
> Good to tidy that up.
>
Thanks.
>> * Remove unnecessary space before @event in sdei_do_cross_call()
>
> Good change, but seems to have gotten lost in this patch.
>
Thanks.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> v2: Drop changes to sdei_cross_call_return()
>> Remove unnecessary space before @event in sdei_do_cross_call()
>> ---
>> drivers/firmware/arm_sdei.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index d03371161aaf..04dc144a8f31 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -73,14 +73,17 @@ 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 int sdei_do_cross_call(void *fn, struct sdei_event * event)
>> {
>
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] 40+ messages in thread
* Re: [PATCH v2 11/17] drivers/firmware/sdei: Introduce sdei_do_local_call()
2020-07-23 15:25 ` Jonathan Cameron
@ 2020-07-27 0:41 ` Gavin Shan
0 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 0:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/24/20 1:25 AM, Jonathan Cameron wrote:
> On Wed, 22 Jul 2020 19:57:34 +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>
> One comment inline.
>
> Jonathan
>
>> ---
>> drivers/firmware/arm_sdei.c | 43 ++++++++++++++++++++++---------------
>> 1 file changed, 26 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 04dc144a8f31..da0e0d5591a8 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -85,7 +85,18 @@ struct sdei_crosscall_args {
>> atomic_set(&arg.errors, 0); \
>> } while (0)
>>
>> -static inline int sdei_do_cross_call(void *fn, struct sdei_event * event)
>> +static inline int sdei_do_local_call(void *fn, struct sdei_event *event)
>
> Can we now give the function parameter it's full type and avoid the nasty
> looking cast below?
>
Nice idea. I will use smp_call_func_t defined as below in include/linux/smp.h
typedef void (*smp_call_func_t)(void *info);
>> +{
>> + struct sdei_crosscall_args arg;
>> + void (*func)(void *) = fn;
>> +
>> + CROSSCALL_INIT(arg, event);
>> + func(&arg);
>> +
>> + return arg.first_error;
>> +}
>> +
>> +static inline int sdei_do_cross_call(void *fn, struct sdei_event *event)
>> {
>> struct sdei_crosscall_args arg;
>>
>> @@ -675,7 +686,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 +694,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 +708,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 +717,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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 12/17] drivers/firmware/sdei: Remove _sdei_event_register()
2020-07-23 15:25 ` Jonathan Cameron
@ 2020-07-27 0:42 ` Gavin Shan
0 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 0:42 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/24/20 1:25 AM, Jonathan Cameron wrote:
> On Wed, 22 Jul 2020 19:57:35 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
>> The function _sdei_event_register() is called by sdei_event_register()
>> and sdei_device_thaw() as the following functional call chain shows.
>> _sdei_event_register() covers the shared and privte events, but
>
> private
>
Will be fixed in v3.
>> 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>
>
> Otherwise looks good to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Will be picked up in v3.
>> ---
>> v2: Improved commit log
>> Drop changes to reorder the variables
>> ---
>> 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 da0e0d5591a8..f4cd9791242f 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -575,25 +575,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;
>> @@ -616,7 +597,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);
>> @@ -633,33 +624,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;
>> @@ -672,9 +636,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);
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] 40+ messages in thread
* Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file
2020-07-23 15:19 ` Jonathan Cameron
@ 2020-07-27 0:46 ` Gavin Shan
2020-07-27 9:02 ` Jonathan Cameron
0 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 0:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/24/20 1:19 AM, Jonathan Cameron wrote:
> On Wed, 22 Jul 2020 19:57:37 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
>> This moves struct sdei_event to the header file so that it can be
>> dereferenced by external modules. This is needed by the code to
>> virtualize SDEI functionality, as part of the arm64/kvm.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
[...]
>> ---
>> v2: Derived from "drivers/firmware/sdei: Identify event by struct"
>> ---
>> drivers/firmware/arm_sdei.c | 20 ------------
>> include/linux/arm_sdei.h | 61 ++++++++++++++++++++++++-------------
>> 2 files changed, 40 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index a52dcff59a20..bdd2de0149c0 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);
>>
>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
>> index 0a241c5c911d..fdc2f868d84b 100644
>> --- a/include/linux/arm_sdei.h
>> +++ b/include/linux/arm_sdei.h
>> @@ -22,6 +22,46 @@
>> */
>> typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>>
>> +/*
>> + * This struct represents an event that has been registered. The driver
>> + * maintains a list of all events, and which ones are registered. (Private
>> + * events have one entry in the list, but are registered on each CPU).
>> + * A pointer to this struct is passed to firmware, and back to the event
>> + * handler. The event handler can then use this to invoke the registered
>> + * callback, without having to walk the list.
>> + *
>> + * For CPU private events, this structure is per-cpu.
>> + */
>> +struct sdei_registered_event {
>> + /* For use by arch code: */
>> + struct pt_regs interrupted_regs;
>> +
>> + sdei_event_callback *callback;
>> + void *callback_arg;
>> + u32 event_num;
>> + u8 priority;
>> +};
>> +
>> +struct sdei_event {
>> + /* These three are protected by the sdei_list_lock */
>
> As this patch leaves the sdei_list_lock as local to arm_sdei.c, is this comment still valid?
>
Yes, the comment is still valid. @sdei_list_lock is used to protect
the linked list (@sdei_list) and all elements (@event) in the list.
For example, the lock is taken before updating @event->reenabled in
function sdei_event_enable().
>> + 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;
>> + };
>> +};
>> +
>> /*
>> * Register your callback to claim an event. The event must be described
>> * by firmware.
>> @@ -51,27 +91,6 @@ 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
>> - * events have one entry in the list, but are registered on each CPU).
>> - * A pointer to this struct is passed to firmware, and back to the event
>> - * handler. The event handler can then use this to invoke the registered
>> - * callback, without having to walk the list.
>> - *
>> - * For CPU private events, this structure is per-cpu.
>> - */
>> -struct sdei_registered_event {
>> - /* For use by arch code: */
>> - struct pt_regs interrupted_regs;
>> -
>> - sdei_event_callback *callback;
>> - void *callback_arg;
>> - u32 event_num;
>> - u8 priority;
>> -};
>> -
>> /* 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);
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] 40+ messages in thread
* Re: [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration
2020-07-23 15:24 ` Jonathan Cameron
@ 2020-07-27 0:53 ` Gavin Shan
2020-07-27 9:04 ` Jonathan Cameron
0 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 0:53 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/24/20 1:24 AM, Jonathan Cameron wrote:
> On Wed, 22 Jul 2020 19:57:39 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
>> 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>
>
> These last two patches are probably fine but hard to tell without a user.
>
Good question. Let me explain the background and please let me know
if you have more questions. SDEI was suggested by Marc to deliver
the notification during the asynchronous page fault, so that the
process can be rescheduled in guest. Unfortunately, we don't have
SDEI (or virtualized SDEI) supported yet. So the additional event
information is needed when SDEI virtualization is supported.
The code of SDEI virtualization can be checked out from github:
https://github.com/gwshan/linux/tree/sdei (branch: "sdei")
>> ---
>> 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 cf10fec57f2a..7518d3febf53 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -204,6 +204,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) {
>> reg = kzalloc(sizeof(*reg), GFP_KERNEL);
>> if (!reg) {
>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
>> index 11af6410dd52..7f3ed7e4b680 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 {
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] 40+ messages in thread
* Re: [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function
2020-07-27 0:33 ` Gavin Shan
@ 2020-07-27 8:58 ` Jonathan Cameron
2020-07-27 9:45 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-27 8:58 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Mon, 27 Jul 2020 10:33:46 +1000
Gavin Shan <gshan@redhat.com> wrote:
> Hi Jonathan,
>
> On 7/24/20 1:52 AM, Jonathan Cameron wrote:
> > On Wed, 22 Jul 2020 19:57:33 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >
> >> This applies cleanup on the corss call functions, no functional
> >
> > cross
> >
>
> It will be fixed in v3.
>
> >> changes is introduced:
> >
> > changes are introduced:
> >
>
> It will be fixed in v3.
>
> >>
> >> * Cleanup struct sdei_crosscall_arg to use tab between fields
> >> and their types
> > Hmm. I guess there is a consistency argument for doing this but generally
> > that one doesn't look like it actually has much benefit.
> >
>
> Sorry, I guess you're talking about "./scripts/cleanpatch"? As stated
> in the head of the script, the result would be destructive. So I think
> manual changes would be more reliable. Lets keep this in v3 if you agree.
>
> Extracted from "scripts/cleanpatch":
No I was talking about doing this change at all. Generally I'd say don't
bother making changes like this to existing code. They just add noise.
>
> # Clean a patch file -- or directory of patch files -- of stealth whitespace.
> # WARNING: this can be a highly destructive operation. Use with caution.
> #
>
>
> >> * Refactor CROSSCALL_INIT to use "do { ... } while (0)" to be
> >> compatible with scripts/checkpatch.pl
> > Good to tidy that up.
> >
>
> Thanks.
>
> >> * Remove unnecessary space before @event in sdei_do_cross_call()
> >
> > Good change, but seems to have gotten lost in this patch.
> >
>
> Thanks.
>
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >> v2: Drop changes to sdei_cross_call_return()
> >> Remove unnecessary space before @event in sdei_do_cross_call()
> >> ---
> >> drivers/firmware/arm_sdei.c | 15 +++++++++------
> >> 1 file changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> >> index d03371161aaf..04dc144a8f31 100644
> >> --- a/drivers/firmware/arm_sdei.c
> >> +++ b/drivers/firmware/arm_sdei.c
> >> @@ -73,14 +73,17 @@ 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 int sdei_do_cross_call(void *fn, struct sdei_event * event)
> >> {
> >
>
> 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] 40+ messages in thread
* Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file
2020-07-27 0:46 ` Gavin Shan
@ 2020-07-27 9:02 ` Jonathan Cameron
2020-07-27 9:59 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-27 9:02 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Mon, 27 Jul 2020 10:46:52 +1000
Gavin Shan <gshan@redhat.com> wrote:
> Hi Jonathan,
>
> On 7/24/20 1:19 AM, Jonathan Cameron wrote:
> > On Wed, 22 Jul 2020 19:57:37 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >
> >> This moves struct sdei_event to the header file so that it can be
> >> dereferenced by external modules. This is needed by the code to
> >> virtualize SDEI functionality, as part of the arm64/kvm.
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
>
> [...]
>
> >> ---
> >> v2: Derived from "drivers/firmware/sdei: Identify event by struct"
> >> ---
> >> drivers/firmware/arm_sdei.c | 20 ------------
> >> include/linux/arm_sdei.h | 61 ++++++++++++++++++++++++-------------
> >> 2 files changed, 40 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> >> index a52dcff59a20..bdd2de0149c0 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);
> >>
> >> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> >> index 0a241c5c911d..fdc2f868d84b 100644
> >> --- a/include/linux/arm_sdei.h
> >> +++ b/include/linux/arm_sdei.h
> >> @@ -22,6 +22,46 @@
> >> */
> >> typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
> >>
> >> +/*
> >> + * This struct represents an event that has been registered. The driver
> >> + * maintains a list of all events, and which ones are registered. (Private
> >> + * events have one entry in the list, but are registered on each CPU).
> >> + * A pointer to this struct is passed to firmware, and back to the event
> >> + * handler. The event handler can then use this to invoke the registered
> >> + * callback, without having to walk the list.
> >> + *
> >> + * For CPU private events, this structure is per-cpu.
> >> + */
> >> +struct sdei_registered_event {
> >> + /* For use by arch code: */
> >> + struct pt_regs interrupted_regs;
> >> +
> >> + sdei_event_callback *callback;
> >> + void *callback_arg;
> >> + u32 event_num;
> >> + u8 priority;
> >> +};
> >> +
> >> +struct sdei_event {
> >> + /* These three are protected by the sdei_list_lock */
> >
> > As this patch leaves the sdei_list_lock as local to arm_sdei.c, is this comment still valid?
> >
>
> Yes, the comment is still valid. @sdei_list_lock is used to protect
> the linked list (@sdei_list) and all elements (@event) in the list.
> For example, the lock is taken before updating @event->reenabled in
> function sdei_event_enable().
OK. I assume your new KVM code will simply not touch the list.
That's a bit messy from a 'scope' point of view, but I guess it's not
worth doing something like:
struct sdei_event_opaque {
struct list_head list;
// Whatever else the kvm code doesn't need
struct sdei_event {
// The bits that you want to expose more widely (i.e. use in the
// kvm code. + you ensure that code only ever sees this internal structure.
};
}
>
> >> + 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;
> >> + };
> >> +};
> >> +
> >> /*
> >> * Register your callback to claim an event. The event must be described
> >> * by firmware.
> >> @@ -51,27 +91,6 @@ 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
> >> - * events have one entry in the list, but are registered on each CPU).
> >> - * A pointer to this struct is passed to firmware, and back to the event
> >> - * handler. The event handler can then use this to invoke the registered
> >> - * callback, without having to walk the list.
> >> - *
> >> - * For CPU private events, this structure is per-cpu.
> >> - */
> >> -struct sdei_registered_event {
> >> - /* For use by arch code: */
> >> - struct pt_regs interrupted_regs;
> >> -
> >> - sdei_event_callback *callback;
> >> - void *callback_arg;
> >> - u32 event_num;
> >> - u8 priority;
> >> -};
> >> -
> >> /* 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);
>
> 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] 40+ messages in thread
* Re: [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration
2020-07-27 0:53 ` Gavin Shan
@ 2020-07-27 9:04 ` Jonathan Cameron
2020-07-27 10:03 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-27 9:04 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Mon, 27 Jul 2020 10:53:27 +1000
Gavin Shan <gshan@redhat.com> wrote:
> Hi Jonathan,
>
> On 7/24/20 1:24 AM, Jonathan Cameron wrote:
> > On Wed, 22 Jul 2020 19:57:39 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >
> >> 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>
> >
> > These last two patches are probably fine but hard to tell without a user.
> >
>
> Good question. Let me explain the background and please let me know
> if you have more questions. SDEI was suggested by Marc to deliver
> the notification during the asynchronous page fault, so that the
> process can be rescheduled in guest. Unfortunately, we don't have
> SDEI (or virtualized SDEI) supported yet. So the additional event
> information is needed when SDEI virtualization is supported.
>
> The code of SDEI virtualization can be checked out from github:
>
> https://github.com/gwshan/linux/tree/sdei (branch: "sdei")
Thanks.
I'd be tempted to move these two patches to the next series
that includes the users.
I forgot to say, I'm fine with all the patches I didn't comment on.
Jonathan
>
>
> >> ---
> >> 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 cf10fec57f2a..7518d3febf53 100644
> >> --- a/drivers/firmware/arm_sdei.c
> >> +++ b/drivers/firmware/arm_sdei.c
> >> @@ -204,6 +204,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) {
> >> reg = kzalloc(sizeof(*reg), GFP_KERNEL);
> >> if (!reg) {
> >> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> >> index 11af6410dd52..7f3ed7e4b680 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 {
>
> 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] 40+ messages in thread
* Re: [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function
2020-07-27 8:58 ` Jonathan Cameron
@ 2020-07-27 9:45 ` Gavin Shan
0 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 9:45 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/27/20 6:58 PM, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 10:33:46 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>> On 7/24/20 1:52 AM, Jonathan Cameron wrote:
>>> On Wed, 22 Jul 2020 19:57:33 +1000
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> This applies cleanup on the corss call functions, no functional
>>>
>>> cross
>>>
>>
>> It will be fixed in v3.
>>
>>>> changes is introduced:
>>>
>>> changes are introduced:
>>>
>>
>> It will be fixed in v3.
>>
>>>>
>>>> * Cleanup struct sdei_crosscall_arg to use tab between fields
>>>> and their types
>>> Hmm. I guess there is a consistency argument for doing this but generally
>>> that one doesn't look like it actually has much benefit.
>>>
>>
>> Sorry, I guess you're talking about "./scripts/cleanpatch"? As stated
>> in the head of the script, the result would be destructive. So I think
>> manual changes would be more reliable. Lets keep this in v3 if you agree.
>>
>> Extracted from "scripts/cleanpatch":
>
> No I was talking about doing this change at all. Generally I'd say don't
> bother making changes like this to existing code. They just add noise.
>
I will drop the particular changes in v3 then :)
>>
>> # Clean a patch file -- or directory of patch files -- of stealth whitespace.
>> # WARNING: this can be a highly destructive operation. Use with caution.
>> #
>>
>>
>>>> * Refactor CROSSCALL_INIT to use "do { ... } while (0)" to be
>>>> compatible with scripts/checkpatch.pl
>>> Good to tidy that up.
>>>
>>
>> Thanks.
>>
>>>> * Remove unnecessary space before @event in sdei_do_cross_call()
>>>
>>> Good change, but seems to have gotten lost in this patch.
>>>
>>
>> Thanks.
>>
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>>> ---
>>>> v2: Drop changes to sdei_cross_call_return()
>>>> Remove unnecessary space before @event in sdei_do_cross_call()
>>>> ---
>>>> drivers/firmware/arm_sdei.c | 15 +++++++++------
>>>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>>>> index d03371161aaf..04dc144a8f31 100644
>>>> --- a/drivers/firmware/arm_sdei.c
>>>> +++ b/drivers/firmware/arm_sdei.c
>>>> @@ -73,14 +73,17 @@ 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 int sdei_do_cross_call(void *fn, struct sdei_event * event)
>>>> {
>>>
>>
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] 40+ messages in thread
* Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file
2020-07-27 9:02 ` Jonathan Cameron
@ 2020-07-27 9:59 ` Gavin Shan
2020-07-27 13:50 ` Jonathan Cameron
0 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 9:59 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/27/20 7:02 PM, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 10:46:52 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>> On 7/24/20 1:19 AM, Jonathan Cameron wrote:
>>> On Wed, 22 Jul 2020 19:57:37 +1000
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> This moves struct sdei_event to the header file so that it can be
>>>> dereferenced by external modules. This is needed by the code to
>>>> virtualize SDEI functionality, as part of the arm64/kvm.
>>>>
>>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>
>> [...]
>>
>>>> ---
>>>> v2: Derived from "drivers/firmware/sdei: Identify event by struct"
>>>> ---
>>>> drivers/firmware/arm_sdei.c | 20 ------------
>>>> include/linux/arm_sdei.h | 61 ++++++++++++++++++++++++-------------
>>>> 2 files changed, 40 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>>>> index a52dcff59a20..bdd2de0149c0 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);
>>>>
>>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
>>>> index 0a241c5c911d..fdc2f868d84b 100644
>>>> --- a/include/linux/arm_sdei.h
>>>> +++ b/include/linux/arm_sdei.h
>>>> @@ -22,6 +22,46 @@
>>>> */
>>>> typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>>>>
>>>> +/*
>>>> + * This struct represents an event that has been registered. The driver
>>>> + * maintains a list of all events, and which ones are registered. (Private
>>>> + * events have one entry in the list, but are registered on each CPU).
>>>> + * A pointer to this struct is passed to firmware, and back to the event
>>>> + * handler. The event handler can then use this to invoke the registered
>>>> + * callback, without having to walk the list.
>>>> + *
>>>> + * For CPU private events, this structure is per-cpu.
>>>> + */
>>>> +struct sdei_registered_event {
>>>> + /* For use by arch code: */
>>>> + struct pt_regs interrupted_regs;
>>>> +
>>>> + sdei_event_callback *callback;
>>>> + void *callback_arg;
>>>> + u32 event_num;
>>>> + u8 priority;
>>>> +};
>>>> +
>>>> +struct sdei_event {
>>>> + /* These three are protected by the sdei_list_lock */
>>>
>>> As this patch leaves the sdei_list_lock as local to arm_sdei.c, is this comment still valid?
>>>
>>
>> Yes, the comment is still valid. @sdei_list_lock is used to protect
>> the linked list (@sdei_list) and all elements (@event) in the list.
>> For example, the lock is taken before updating @event->reenabled in
>> function sdei_event_enable().
> OK. I assume your new KVM code will simply not touch the list.
> That's a bit messy from a 'scope' point of view, but I guess it's not
> worth doing something like:
>
> struct sdei_event_opaque {
> struct list_head list;
> // Whatever else the kvm code doesn't need
> struct sdei_event {
> // The bits that you want to expose more widely (i.e. use in the
> // kvm code. + you ensure that code only ever sees this internal structure.
>
> };
>
> }
Yes, your assumption is correct. The list is still managed by
drivers/firmware/arm_sdei.c and it's invisible to the new KVM
code for SDEI virtualization.
It's worthy to hide those fields in "struct sdei_event" from
external by introducing another struct, from the point of "scope".
But it's not free to maintain another struct in this case. I would
say lets avoid introducing another struct if you agree.
>>
>>>> + 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;
>>>> + };
>>>> +};
>>>> +
>>>> /*
>>>> * Register your callback to claim an event. The event must be described
>>>> * by firmware.
>>>> @@ -51,27 +91,6 @@ 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
>>>> - * events have one entry in the list, but are registered on each CPU).
>>>> - * A pointer to this struct is passed to firmware, and back to the event
>>>> - * handler. The event handler can then use this to invoke the registered
>>>> - * callback, without having to walk the list.
>>>> - *
>>>> - * For CPU private events, this structure is per-cpu.
>>>> - */
>>>> -struct sdei_registered_event {
>>>> - /* For use by arch code: */
>>>> - struct pt_regs interrupted_regs;
>>>> -
>>>> - sdei_event_callback *callback;
>>>> - void *callback_arg;
>>>> - u32 event_num;
>>>> - u8 priority;
>>>> -};
>>>> -
>>>> /* 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);
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] 40+ messages in thread
* Re: [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration
2020-07-27 9:04 ` Jonathan Cameron
@ 2020-07-27 10:03 ` Gavin Shan
2020-07-27 13:56 ` Jonathan Cameron
0 siblings, 1 reply; 40+ messages in thread
From: Gavin Shan @ 2020-07-27 10:03 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/27/20 7:04 PM, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 10:53:27 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>> On 7/24/20 1:24 AM, Jonathan Cameron wrote:
>>> On Wed, 22 Jul 2020 19:57:39 +1000
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> 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>
>>>
>>> These last two patches are probably fine but hard to tell without a user.
>>>
>>
>> Good question. Let me explain the background and please let me know
>> if you have more questions. SDEI was suggested by Marc to deliver
>> the notification during the asynchronous page fault, so that the
>> process can be rescheduled in guest. Unfortunately, we don't have
>> SDEI (or virtualized SDEI) supported yet. So the additional event
>> information is needed when SDEI virtualization is supported.
>>
>> The code of SDEI virtualization can be checked out from github:
>>
>> https://github.com/gwshan/linux/tree/sdei (branch: "sdei")
> Thanks.
>
> I'd be tempted to move these two patches to the next series
> that includes the users.
>
> I forgot to say, I'm fine with all the patches I didn't comment on.
>
Yes, it's fine to move the last two patches to where we need
them. Thanks for your review and comments. May I have your
reviewed-by on those patches you didn't comment on? I would
like to pick the reviwed-by in v3 :)
>>
>>>> ---
>>>> 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 cf10fec57f2a..7518d3febf53 100644
>>>> --- a/drivers/firmware/arm_sdei.c
>>>> +++ b/drivers/firmware/arm_sdei.c
>>>> @@ -204,6 +204,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) {
>>>> reg = kzalloc(sizeof(*reg), GFP_KERNEL);
>>>> if (!reg) {
>>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
>>>> index 11af6410dd52..7f3ed7e4b680 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 {
>>
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] 40+ messages in thread
* Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file
2020-07-27 9:59 ` Gavin Shan
@ 2020-07-27 13:50 ` Jonathan Cameron
2020-07-28 2:52 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-27 13:50 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Mon, 27 Jul 2020 19:59:24 +1000
Gavin Shan <gshan@redhat.com> wrote:
> Hi Jonathan,
>
> On 7/27/20 7:02 PM, Jonathan Cameron wrote:
> > On Mon, 27 Jul 2020 10:46:52 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >> On 7/24/20 1:19 AM, Jonathan Cameron wrote:
> >>> On Wed, 22 Jul 2020 19:57:37 +1000
> >>> Gavin Shan <gshan@redhat.com> wrote:
> >>>
> >>>> This moves struct sdei_event to the header file so that it can be
> >>>> dereferenced by external modules. This is needed by the code to
> >>>> virtualize SDEI functionality, as part of the arm64/kvm.
> >>>>
> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >>
> >> [...]
> >>
> >>>> ---
> >>>> v2: Derived from "drivers/firmware/sdei: Identify event by struct"
> >>>> ---
> >>>> drivers/firmware/arm_sdei.c | 20 ------------
> >>>> include/linux/arm_sdei.h | 61 ++++++++++++++++++++++++-------------
> >>>> 2 files changed, 40 insertions(+), 41 deletions(-)
> >>>>
> >>>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> >>>> index a52dcff59a20..bdd2de0149c0 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);
> >>>>
> >>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> >>>> index 0a241c5c911d..fdc2f868d84b 100644
> >>>> --- a/include/linux/arm_sdei.h
> >>>> +++ b/include/linux/arm_sdei.h
> >>>> @@ -22,6 +22,46 @@
> >>>> */
> >>>> typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
> >>>>
> >>>> +/*
> >>>> + * This struct represents an event that has been registered. The driver
> >>>> + * maintains a list of all events, and which ones are registered. (Private
> >>>> + * events have one entry in the list, but are registered on each CPU).
> >>>> + * A pointer to this struct is passed to firmware, and back to the event
> >>>> + * handler. The event handler can then use this to invoke the registered
> >>>> + * callback, without having to walk the list.
> >>>> + *
> >>>> + * For CPU private events, this structure is per-cpu.
> >>>> + */
> >>>> +struct sdei_registered_event {
> >>>> + /* For use by arch code: */
> >>>> + struct pt_regs interrupted_regs;
> >>>> +
> >>>> + sdei_event_callback *callback;
> >>>> + void *callback_arg;
> >>>> + u32 event_num;
> >>>> + u8 priority;
> >>>> +};
> >>>> +
> >>>> +struct sdei_event {
> >>>> + /* These three are protected by the sdei_list_lock */
> >>>
> >>> As this patch leaves the sdei_list_lock as local to arm_sdei.c, is this comment still valid?
> >>>
> >>
> >> Yes, the comment is still valid. @sdei_list_lock is used to protect
> >> the linked list (@sdei_list) and all elements (@event) in the list.
> >> For example, the lock is taken before updating @event->reenabled in
> >> function sdei_event_enable().
> > OK. I assume your new KVM code will simply not touch the list.
> > That's a bit messy from a 'scope' point of view, but I guess it's not
> > worth doing something like:
> >
> > struct sdei_event_opaque {
> > struct list_head list;
> > // Whatever else the kvm code doesn't need
> > struct sdei_event {
> > // The bits that you want to expose more widely (i.e. use in the
> > // kvm code. + you ensure that code only ever sees this internal structure.
> >
> > };
> >
> > }
>
> Yes, your assumption is correct. The list is still managed by
> drivers/firmware/arm_sdei.c and it's invisible to the new KVM
> code for SDEI virtualization.
>
> It's worthy to hide those fields in "struct sdei_event" from
> external by introducing another struct, from the point of "scope".
> But it's not free to maintain another struct in this case. I would
> say lets avoid introducing another struct if you agree.
I'm fine either way.
Jonathan
>
> >>
> >>>> + 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;
> >>>> + };
> >>>> +};
> >>>> +
> >>>> /*
> >>>> * Register your callback to claim an event. The event must be described
> >>>> * by firmware.
> >>>> @@ -51,27 +91,6 @@ 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
> >>>> - * events have one entry in the list, but are registered on each CPU).
> >>>> - * A pointer to this struct is passed to firmware, and back to the event
> >>>> - * handler. The event handler can then use this to invoke the registered
> >>>> - * callback, without having to walk the list.
> >>>> - *
> >>>> - * For CPU private events, this structure is per-cpu.
> >>>> - */
> >>>> -struct sdei_registered_event {
> >>>> - /* For use by arch code: */
> >>>> - struct pt_regs interrupted_regs;
> >>>> -
> >>>> - sdei_event_callback *callback;
> >>>> - void *callback_arg;
> >>>> - u32 event_num;
> >>>> - u8 priority;
> >>>> -};
> >>>> -
> >>>> /* 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);
>
> 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] 40+ messages in thread
* Re: [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration
2020-07-27 10:03 ` Gavin Shan
@ 2020-07-27 13:56 ` Jonathan Cameron
2020-07-28 2:56 ` Gavin Shan
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2020-07-27 13:56 UTC (permalink / raw)
To: Gavin Shan
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
On Mon, 27 Jul 2020 20:03:32 +1000
Gavin Shan <gshan@redhat.com> wrote:
> Hi Jonathan,
>
> On 7/27/20 7:04 PM, Jonathan Cameron wrote:
> > On Mon, 27 Jul 2020 10:53:27 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >> On 7/24/20 1:24 AM, Jonathan Cameron wrote:
> >>> On Wed, 22 Jul 2020 19:57:39 +1000
> >>> Gavin Shan <gshan@redhat.com> wrote:
> >>>
> >>>> 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>
> >>>
> >>> These last two patches are probably fine but hard to tell without a user.
> >>>
> >>
> >> Good question. Let me explain the background and please let me know
> >> if you have more questions. SDEI was suggested by Marc to deliver
> >> the notification during the asynchronous page fault, so that the
> >> process can be rescheduled in guest. Unfortunately, we don't have
> >> SDEI (or virtualized SDEI) supported yet. So the additional event
> >> information is needed when SDEI virtualization is supported.
> >>
> >> The code of SDEI virtualization can be checked out from github:
> >>
> >> https://github.com/gwshan/linux/tree/sdei (branch: "sdei")
> > Thanks.
> >
> > I'd be tempted to move these two patches to the next series
> > that includes the users.
> >
> > I forgot to say, I'm fine with all the patches I didn't comment on.
> >
>
> Yes, it's fine to move the last two patches to where we need
> them. Thanks for your review and comments. May I have your
> reviewed-by on those patches you didn't comment on? I would
> like to pick the reviwed-by in v3 :)
>
Sure FWIW (I'm far from an expert in this area!)
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
for patches
1-7,13,15 as is
8-10 with trivial changes as discussed.
12 already given
Given postponing 16 and 17, that just leaves 11 and 14 that I'd
like to take a quick look at in v3.
Jonathan
> >>
> >>>> ---
> >>>> 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 cf10fec57f2a..7518d3febf53 100644
> >>>> --- a/drivers/firmware/arm_sdei.c
> >>>> +++ b/drivers/firmware/arm_sdei.c
> >>>> @@ -204,6 +204,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) {
> >>>> reg = kzalloc(sizeof(*reg), GFP_KERNEL);
> >>>> if (!reg) {
> >>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
> >>>> index 11af6410dd52..7f3ed7e4b680 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 {
> >>
>
> 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] 40+ messages in thread
* Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file
2020-07-27 13:50 ` Jonathan Cameron
@ 2020-07-28 2:52 ` Gavin Shan
0 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-28 2:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/27/20 11:50 PM, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 19:59:24 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>> On 7/27/20 7:02 PM, Jonathan Cameron wrote:
>>> On Mon, 27 Jul 2020 10:46:52 +1000
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>> On 7/24/20 1:19 AM, Jonathan Cameron wrote:
>>>>> On Wed, 22 Jul 2020 19:57:37 +1000
>>>>> Gavin Shan <gshan@redhat.com> wrote:
[...]
>>>>>>
>>>>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
>>>>>> index 0a241c5c911d..fdc2f868d84b 100644
>>>>>> --- a/include/linux/arm_sdei.h
>>>>>> +++ b/include/linux/arm_sdei.h
>>>>>> @@ -22,6 +22,46 @@
>>>>>> */
>>>>>> typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg);
>>>>>>
>>>>>> +/*
>>>>>> + * This struct represents an event that has been registered. The driver
>>>>>> + * maintains a list of all events, and which ones are registered. (Private
>>>>>> + * events have one entry in the list, but are registered on each CPU).
>>>>>> + * A pointer to this struct is passed to firmware, and back to the event
>>>>>> + * handler. The event handler can then use this to invoke the registered
>>>>>> + * callback, without having to walk the list.
>>>>>> + *
>>>>>> + * For CPU private events, this structure is per-cpu.
>>>>>> + */
>>>>>> +struct sdei_registered_event {
>>>>>> + /* For use by arch code: */
>>>>>> + struct pt_regs interrupted_regs;
>>>>>> +
>>>>>> + sdei_event_callback *callback;
>>>>>> + void *callback_arg;
>>>>>> + u32 event_num;
>>>>>> + u8 priority;
>>>>>> +};
>>>>>> +
>>>>>> +struct sdei_event {
>>>>>> + /* These three are protected by the sdei_list_lock */
>>>>>
>>>>> As this patch leaves the sdei_list_lock as local to arm_sdei.c, is this comment still valid?
>>>>>
>>>>
>>>> Yes, the comment is still valid. @sdei_list_lock is used to protect
>>>> the linked list (@sdei_list) and all elements (@event) in the list.
>>>> For example, the lock is taken before updating @event->reenabled in
>>>> function sdei_event_enable().
>>> OK. I assume your new KVM code will simply not touch the list.
>>> That's a bit messy from a 'scope' point of view, but I guess it's not
>>> worth doing something like:
>>>
>>> struct sdei_event_opaque {
>>> struct list_head list;
>>> // Whatever else the kvm code doesn't need
>>> struct sdei_event {
>>> // The bits that you want to expose more widely (i.e. use in the
>>> // kvm code. + you ensure that code only ever sees this internal structure.
>>>
>>> };
>>>
>>> }
>>
>> Yes, your assumption is correct. The list is still managed by
>> drivers/firmware/arm_sdei.c and it's invisible to the new KVM
>> code for SDEI virtualization.
>>
>> It's worthy to hide those fields in "struct sdei_event" from
>> external by introducing another struct, from the point of "scope".
>> But it's not free to maintain another struct in this case. I would
>> say lets avoid introducing another struct if you agree.
>
> I'm fine either way.
>
Thanks again for your feedback. I think about it again and it's
really nice idea to narrow the scopes of the fields in the struct.
I will split original struct sdei_event into struct sdei_event and
struct sdei_internal_event in v3, which will be posted shortly.
As the naems indicate, struct sdei_event can be dereferenced by
external modules like arm64/kvm in the future, while struct
sdei_internal_event is used by the SDEI client driver only :)
>
>>
>>>>
>>>>>> + 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;
>>>>>> + };
>>>>>> +};
>>>>>> +
[...]
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] 40+ messages in thread
* Re: [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration
2020-07-27 13:56 ` Jonathan Cameron
@ 2020-07-28 2:56 ` Gavin Shan
0 siblings, 0 replies; 40+ messages in thread
From: Gavin Shan @ 2020-07-28 2:56 UTC (permalink / raw)
To: Jonathan Cameron
Cc: mark.rutland, catalin.marinas, james.morse, shan.gavin, will,
linux-arm-kernel
Hi Jonathan,
On 7/27/20 11:56 PM, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 20:03:32 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>> On 7/27/20 7:04 PM, Jonathan Cameron wrote:
>>> On Mon, 27 Jul 2020 10:53:27 +1000
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>> On 7/24/20 1:24 AM, Jonathan Cameron wrote:
>>>>> On Wed, 22 Jul 2020 19:57:39 +1000
>>>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>>
>>>>>> 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>
>>>>>
>>>>> These last two patches are probably fine but hard to tell without a user.
>>>>>
>>>>
>>>> Good question. Let me explain the background and please let me know
>>>> if you have more questions. SDEI was suggested by Marc to deliver
>>>> the notification during the asynchronous page fault, so that the
>>>> process can be rescheduled in guest. Unfortunately, we don't have
>>>> SDEI (or virtualized SDEI) supported yet. So the additional event
>>>> information is needed when SDEI virtualization is supported.
>>>>
>>>> The code of SDEI virtualization can be checked out from github:
>>>>
>>>> https://github.com/gwshan/linux/tree/sdei (branch: "sdei")
>>> Thanks.
>>>
>>> I'd be tempted to move these two patches to the next series
>>> that includes the users.
>>>
>>> I forgot to say, I'm fine with all the patches I didn't comment on.
>>>
>>
>> Yes, it's fine to move the last two patches to where we need
>> them. Thanks for your review and comments. May I have your
>> reviewed-by on those patches you didn't comment on? I would
>> like to pick the reviwed-by in v3 :)
>>
> Sure FWIW (I'm far from an expert in this area!)
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> for patches
> 1-7,13,15 as is
> 8-10 with trivial changes as discussed.
> 12 already given
>
> Given postponing 16 and 17, that just leaves 11 and 14 that I'd
> like to take a quick look at in v3.
>
Thanks for your confirmation. Except PATCH[11] and PATCH[14], all
other patches in v3 will include your r-b. Thanks again for your
time and comments :)
>>>>
>>>>>> ---
>>>>>> 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 cf10fec57f2a..7518d3febf53 100644
>>>>>> --- a/drivers/firmware/arm_sdei.c
>>>>>> +++ b/drivers/firmware/arm_sdei.c
>>>>>> @@ -204,6 +204,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) {
>>>>>> reg = kzalloc(sizeof(*reg), GFP_KERNEL);
>>>>>> if (!reg) {
>>>>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h
>>>>>> index 11af6410dd52..7f3ed7e4b680 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 {
>>>>
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] 40+ messages in thread
end of thread, other threads:[~2020-07-28 2:57 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22 9:57 [PATCH v2 00/17] Refactor SDEI client driver Gavin Shan
2020-07-22 9:57 ` [PATCH v2 01/17] drivers/firmware/sdei: Remove sdei_is_err() Gavin Shan
2020-07-22 9:57 ` [PATCH v2 02/17] drivers/firmware/sdei: Common block for failing path in sdei_event_create() Gavin Shan
2020-07-22 9:57 ` [PATCH v2 03/17] drivers/firmware/sdei: Retrieve event number from event instance Gavin Shan
2020-07-22 9:57 ` [PATCH v2 04/17] drivers/firmware/sdei: Avoid nested statements in sdei_init() Gavin Shan
2020-07-22 9:57 ` [PATCH v2 05/17] drivers/firmware/sdei: Unregister driver on error " Gavin Shan
2020-07-22 9:57 ` [PATCH v2 06/17] drivers/firmware/sdei: Remove duplicate check in sdei_get_conduit() Gavin Shan
2020-07-22 9:57 ` [PATCH v2 07/17] drivers/firmware/sdei: Remove Drop redundant error message in sdei_probe() Gavin Shan
2020-07-22 9:57 ` [PATCH v2 08/17] drivers/firmware/sdei: Remove while loop in sdei_event_register() Gavin Shan
2020-07-22 9:57 ` [PATCH v2 09/17] drivers/firmware/sdei: Remove while loop in sdei_event_unregister() Gavin Shan
2020-07-23 15:51 ` Jonathan Cameron
2020-07-27 0:22 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 10/17] drivers/firmware/sdei: Cleanup on cross call function Gavin Shan
2020-07-23 15:52 ` Jonathan Cameron
2020-07-27 0:33 ` Gavin Shan
2020-07-27 8:58 ` Jonathan Cameron
2020-07-27 9:45 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 11/17] drivers/firmware/sdei: Introduce sdei_do_local_call() Gavin Shan
2020-07-23 15:25 ` Jonathan Cameron
2020-07-27 0:41 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 12/17] drivers/firmware/sdei: Remove _sdei_event_register() Gavin Shan
2020-07-23 15:25 ` Jonathan Cameron
2020-07-27 0:42 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 13/17] drivers/firmware/sdei: Remove _sdei_event_unregister() Gavin Shan
2020-07-22 9:57 ` [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file Gavin Shan
2020-07-23 15:19 ` Jonathan Cameron
2020-07-27 0:46 ` Gavin Shan
2020-07-27 9:02 ` Jonathan Cameron
2020-07-27 9:59 ` Gavin Shan
2020-07-27 13:50 ` Jonathan Cameron
2020-07-28 2:52 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 15/17] drivers/firmware/sdei: Identify event by struct sdei_event Gavin Shan
2020-07-22 9:57 ` [PATCH v2 16/17] drivers/firmware/sdei: Retrieve event signaled property on registration Gavin Shan
2020-07-23 15:24 ` Jonathan Cameron
2020-07-27 0:53 ` Gavin Shan
2020-07-27 9:04 ` Jonathan Cameron
2020-07-27 10:03 ` Gavin Shan
2020-07-27 13:56 ` Jonathan Cameron
2020-07-28 2:56 ` Gavin Shan
2020-07-22 9:57 ` [PATCH v2 17/17] drivers/firmware/sdei: Add sdei_event_get_info() Gavin Shan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).