of: overlay: do not break notify on NOTIFY_OK

PATCH Applied Nuno Sá 5 participants 8 messages Apr 4, 2022 – Apr 19, 2022 Device tree

Share & tools

Nuno Sá

author

2022-04-04 07:40 UTC #0 lore raw

PATCH 1/1 of: overlay: do not break notify on NOTIFY_OK

We should not break overlay notifications on NOTIFY_OK otherwise we might break on the first fragment. As NOTIFY_OK is not zero, we need to account for that when looking for errors.

Fixes a1d19bd4cf1fe ("of: overlay: pr_err from return NOTIFY_OK to overlay apply/remove")

Signed-off-by Nuno Sá <[email protected]>

---
 drivers/of/overlay.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/of/overlay.c +2 -2

index d80160cf34bb..0b2d47598cfb 100644
@@ -170,9 +170,9 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 
 		ret = blocking_notifier_call_chain(&overlay_notify_chain,
 						   action, &nd);
-		if (ret == NOTIFY_OK || ret == NOTIFY_STOP)
+		if (ret == NOTIFY_STOP)
 			return 0;
-		if (ret) {
+		if (ret && ret != NOTIFY_OK) {
 			ret = notifier_to_errno(ret);
 			pr_err("overlay changeset %s notifier error %d, target: %pOF\n",
 			       of_overlay_action_name[action], ret, nd.target);

Signature · 1 line

2.35.1

Frank Rowand

2022-04-04 21:17 UTC #1 lore raw

Quoted 3 lines · Nuno Sá

We should not break overlay notifications on NOTIFY_OK otherwise we might break on the first fragment. As NOTIFY_OK is not zero, we need to account for that when looking for errors.

It's been a long time since I've looked at notifiers, it will take me some time to review this.

-Frank

Quoted 23 lines

Fixes a1d19bd4cf1fe ("of: overlay: pr_err from return NOTIFY_OK to overlay apply/remove")

Signed-off-by Nuno Sá <[email protected]>

---
 drivers/of/overlay.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/of/overlay.c +2 -2

index d80160cf34bb..0b2d47598cfb 100644
@@ -170,9 +170,9 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 
 		ret = blocking_notifier_call_chain(&overlay_notify_chain,
 						   action, &nd);
-		if (ret == NOTIFY_OK || ret == NOTIFY_STOP)
+		if (ret == NOTIFY_STOP)
 			return 0;
-		if (ret) {
+		if (ret && ret != NOTIFY_OK) {
 			ret = notifier_to_errno(ret);
 			pr_err("overlay changeset %s notifier error %d, target: %pOF\n",
 			       of_overlay_action_name[action], ret, nd.target);

Nuno Sá

2022-04-05 07:20 UTC #2 lore raw

Quoted 6 lines · Frank Rowand

Quoted 4 lines · Nuno Sá
We should not break overlay notifications on NOTIFY_OK otherwise we might
break on the first fragment. As NOTIFY_OK is not zero, we need to account for that when looking for errors.

Quoted 7 lines

It's been a long time since I've looked at notifiers, it will take me some time to
review this.
-Frank
Quoted 1 line

Yeah, it was also my first time looking at of dynamic code. But it just didn't felt right to stop fragmment notifications if someone returns NOTIFY_OK. In fact, I'm starting to think that even if someone wants to NOTIFY_STOP on the current fragment, that should not mean we should not send notifications for the remaining ones. So, maybe the right patch is actually something like:

ret = blocking_notifier_call_chain()
if (notifier_to_errno(ret))

    return notifier_to_errno(ret);

This would also be more in line (not totally identical) with '__of_changeset_revert_notify()'.

- Nuno Sá

Quoted 2 lines

Frank Rowand

2022-04-19 14:32 UTC #3 lore raw

Hi Rob, Nuno,

(adding cc: Alan)

You just applied the patch at the root of this email thread.

Please either revert it and accept the alternate that Nuno suggests below, or if you do not want to follow that path, then Nuno please add a follow on patch that does what you suggest below.

-Frank

Quoted 34 lines · Nuno Sá

Quoted 6 lines · Frank Rowand
Quoted 4 lines · Nuno Sá
We should not break overlay notifications on NOTIFY_OK otherwise we might
break on the first fragment. As NOTIFY_OK is not zero, we need to account for that when looking for errors.
Quoted 7 lines
It's been a long time since I've looked at notifiers, it will take me some time to
review this.
-Frank
Quoted 1 line
Yeah, it was also my first time looking at of dynamic code. But it just didn't felt right to stop fragmment notifications if someone returns NOTIFY_OK. In fact, I'm starting to think that even if someone wants to NOTIFY_STOP on the current fragment, that should not mean we should not send notifications for the remaining ones. So, maybe the right patch is actually something like:
ret = blocking_notifier_call_chain()
if (notifier_to_errno(ret))
    return notifier_to_errno(ret);
This would also be more in line (not totally identical) with '__of_changeset_revert_notify()'.
- Nuno Sá
Quoted 2 lines

Rob Herring

2022-04-19 14:48 UTC #4 lore raw

Quoted 4 lines · Frank Rowand

Hi Rob, Nuno,
(adding cc: Alan)

Alan is not active and Altera doesn't exist.

Quoted 1 line

You just applied the patch at the root of this email thread.

Well, no one had commented further after 2 weeks and the patch looked like a move in the right direction as-is.

Quoted 4 lines

Please either revert it and accept the alternate that Nuno suggests below, or if you do not want to follow that path, then Nuno please add a follow on patch that does what you suggest below.

Okay, Dropped as it's not pushed out.

Rob

Frank Rowand

2022-04-19 15:20 UTC #5 lore raw

Hi Rob, Nuno,

Quoted 4 lines · Rob Herring

Quoted 3 lines · Frank Rowand
Hi Rob, Nuno,

Quoted 8 lines

Quoted 1 line
(adding cc: Alan)
Alan is not active and Altera doesn't exist.
Quoted 1 line
You just applied the patch at the root of this email thread.
Well, no one had commented further after 2 weeks and the patch looked like a move in the right direction as-is.

Agreed.

Quoted 7 lines

Quoted 5 lines
Please either revert it and accept the alternate that Nuno suggests below, or if you do not want to follow that path, then Nuno please add a follow on patch that does what you suggest below.
Okay, Dropped as it's not pushed out.

Thanks. I thought that might be possible...

Nuno, can you please create the alternate patch you suggested? It also fixes the additional problem that you noted.

-Frank

Quoted 2 lines

Rob

Sa, Nuno

author

2022-04-19 15:50 UTC #6 lore raw

Quoted 40 lines

-----Original Message-----
From: Frank Rowand <[email protected]>
Sent: Tuesday, April 19, 2022 5:21 PM
To: Rob Herring <[email protected]>
Cc: Nuno Sá <[email protected]>; Sa, Nuno <[email protected]>; [email protected]; Pantelis Antoniou <[email protected]>; Alan Tull <[email protected]>; Frank Rowand <[email protected]>
Subject: Re: [PATCH] of: overlay: do not break notify on NOTIFY_OK
[External]
Hi Rob, Nuno,
Quoted 1 line · Rob Herring
On Tue, Apr 19, 2022 at 9:32 AM Frank Rowand
Quoted 10 lines · <[email protected]> wrote:
Quoted 4 lines
Hi Rob, Nuno,
(adding cc: Alan)
Alan is not active and Altera doesn't exist.
Quoted 1 line
You just applied the patch at the root of this email thread.
Well, no one had commented further after 2 weeks and the patch
looked
Quoted 1 line
like a move in the right direction as-is.
Agreed.
Quoted 6 lines
Quoted 5 lines
Please either revert it and accept the alternate that Nuno suggests below, or if you do not want to follow that path, then Nuno please add a follow on patch that does what you suggest below.

Quoted 8 lines

Quoted 1 line
Okay, Dropped as it's not pushed out.
Thanks. I thought that might be possible...
Nuno, can you please create the alternate patch you suggested? It also fixes the additional problem that you noted.

Sure, I will send a v2 tomorrow...

- Nuno Sá

Rob Herring

maintainer Applied

2022-04-19 13:02 UTC #7 lore raw

In reply to Nuno Sá

Quoted 10 lines · Nuno Sá

We should not break overlay notifications on NOTIFY_OK otherwise we might break on the first fragment. As NOTIFY_OK is not zero, we need to account for that when looking for errors.
Fixes a1d19bd4cf1fe ("of: overlay: pr_err from return NOTIFY_OK to overlay apply/remove")
Signed-off-by Nuno Sá <[email protected]>
---
 drivers/of/overlay.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks!