[PATCH] of: overlay: do not break notify on NOTIFY_OK

[PATCH] of: overlay: do not break notify on NOTIFY_OK

[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á <nuno.sa@analog.com>
---
 drivers/of/overlay.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index d80160cf34bb..0b2d47598cfb 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -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.35.1

Re: [PATCH] of: overlay: do not break notify on NOTIFY_OK

[Frank Rowand]

On 4/4/22 02:40, Nuno Sá wrote:
> 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

> 
> Fixes: a1d19bd4cf1fe ("of: overlay: pr_err from return NOTIFY_OK to overlay apply/remove")
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/of/overlay.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index d80160cf34bb..0b2d47598cfb 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -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);

Re: [PATCH] of: overlay: do not break notify on NOTIFY_OK

[Nuno Sá]

On Mon, 2022-04-04 at 13:10 -0500, Frank Rowand wrote:
> On 4/4/22 02:40, Nuno Sá wrote:
> > 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
> 
> > 

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á

> 
> 

Re: [PATCH] of: overlay: do not break notify on NOTIFY_OK

[Frank Rowand]

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


On 4/5/22 02:19, Nuno Sá wrote:
> On Mon, 2022-04-04 at 13:10 -0500, Frank Rowand wrote:
>> On 4/4/22 02:40, Nuno Sá wrote:
>>> 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
>>
>>>
> 
> 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á
> 
>>
>>
> 

Re: [PATCH] of: overlay: do not break notify on NOTIFY_OK

[Rob Herring]

On Tue, Apr 19, 2022 at 9:32 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> Hi Rob, Nuno,
>
> (adding cc: Alan)

Alan is not active and Altera doesn't exist.

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

> 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

Re: [PATCH] of: overlay: do not break notify on NOTIFY_OK

[Frank Rowand]

Hi Rob, Nuno,

On 4/19/22 09:48, Rob Herring wrote:
> On Tue, Apr 19, 2022 at 9:32 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> Hi Rob, Nuno,
>>
>> (adding cc: Alan)
> 
> Alan is not active and Altera doesn't exist.
> 
>> 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.

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

> 
> Rob

RE: [PATCH] of: overlay: do not break notify on NOTIFY_OK

[Sa, Nuno]

> -----Original Message-----
> From: Frank Rowand <frowand.list@gmail.com>
> Sent: Tuesday, April 19, 2022 5:21 PM
> To: Rob Herring <robh+dt@kernel.org>
> Cc: Nuno Sá <noname.nuno@gmail.com>; Sa, Nuno
> <Nuno.Sa@analog.com>; devicetree@vger.kernel.org; Pantelis
> Antoniou <pantelis.antoniou@konsulko.com>; Alan Tull
> <atull@kernel.org>; Frank Rowand <frowand.list@gmail.com>
> Subject: Re: [PATCH] of: overlay: do not break notify on NOTIFY_OK
> 
> [External]
> 
> Hi Rob, Nuno,
> 
> On 4/19/22 09:48, Rob Herring wrote:
> > On Tue, Apr 19, 2022 at 9:32 AM Frank Rowand
> <frowand.list@gmail.com> wrote:
> >>
> >> Hi Rob, Nuno,
> >>
> >> (adding cc: Alan)
> >
> > Alan is not active and Altera doesn't exist.
> >
> >> 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.
> 
> >
> >> 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.
> 
>

Sure, I will send a v2 tomorrow...
 
- Nuno Sá

Re: [PATCH] of: overlay: do not break notify on NOTIFY_OK

[Rob Herring]

On Mon, 04 Apr 2022 09:40:55 +0200, Nuno Sá wrote:
> 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á <nuno.sa@analog.com>
> ---
>  drivers/of/overlay.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Applied, thanks!