Skip to contentSkip to conversation

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

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore

Nuno Sá

author
#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
#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);
#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
maintainer Applied
#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!

#3 lore raw
In reply to Nuno Sá

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

#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

author
#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á