[PATCH for-4.12] gic-vgic: fix assert condition

[PATCH for-4.12] gic-vgic: fix assert condition

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---
 xen/arch/arm/gic-vgic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 48922f5..684f2d1 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -443,7 +443,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
     int ret = 0;
 
     /* "desc" is optional when we disconnect an IRQ. */
-    ASSERT(connect && desc);
+    ASSERT(connect || desc);
 
     /* We are taking to rank lock to prevent parallel connections. */
     vgic_lock_rank(v_target, rank, flags);
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[Nuernberger, Stefan]

On Thu, 2019-01-24 at 20:33 +0200, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>  xen/arch/arm/gic-vgic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 48922f5..684f2d1 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -443,7 +443,7 @@ int vgic_connect_hw_irq(struct domain *d, struct
> vcpu *v, unsigned int virq,
>      int ret = 0;
>  
>      /* "desc" is optional when we disconnect an IRQ. */
> -    ASSERT(connect && desc);
> +    ASSERT(connect || desc);

I assume it should be ASSERT(!connect || desc);

- Stefan
>  
>      /* We are taking to rank lock to prevent parallel connections.
> */
>      vgic_lock_rank(v_target, rank, flags);
-- 
Amazon Development Center Germany GmbH
c/o Stefan Nuernberger
Am Brauhaus 12
01099 Dresden
Germany

Amtsgericht Charlottenburg - HRB 149173 B - UStID DE 289 237 879
Managing Directors: Ralf Herbrich, Chris Schlaeger



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 1127 bytes --]

(sorry for the formatting)

On Thu, 24 Jan 2019, 18:35 Andrii Anisov, <andrii.anisov@gmail.com> wrote:

> From: Andrii Anisov <andrii_anisov@epam.com>
>

Empty commit message should only happen when the title provide enough
information.

In that case, you should explain how you hit the assert so the reviewers
can understand the change.


Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>  xen/arch/arm/gic-vgic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 48922f5..684f2d1 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -443,7 +443,7 @@ int vgic_connect_hw_irq(struct domain *d, storuct vcpu
> *v, unsigned int virq,
>      int ret = 0;
>
>      /* "desc" is optional when we disconnect an IRQ. */
> -    ASSERT(connect && desc);
> +    ASSERT(connect || desc);


This looks wrong to me. Now you allow desc to be NULL when connecting an
IRQ. This does not even match the comment above.

Without a meaningful commit message, this hard to understand the reasoning
behind the modification.

Cheers,

[-- Attachment #1.2: Type: text/html, Size: 1883 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[André Przywara]

On 24/01/2019 18:47, Nuernberger, Stefan wrote:
> On Thu, 2019-01-24 at 20:33 +0200, Andrii Anisov wrote:
>> From: Andrii Anisov <andrii_anisov@epam.com>
>>
>> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
>> ---
>>  xen/arch/arm/gic-vgic.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index 48922f5..684f2d1 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -443,7 +443,7 @@ int vgic_connect_hw_irq(struct domain *d, struct
>> vcpu *v, unsigned int virq,
>>      int ret = 0;
>>  
>>      /* "desc" is optional when we disconnect an IRQ. */
>> -    ASSERT(connect && desc);
>> +    ASSERT(connect || desc);
> 
> I assume it should be ASSERT(!connect || desc);

Drawing the truth table and throwing all my CS text book boolean
knowledge at it :-)  Yes, Stefan's version matches the comment.

Plus what Julien said about the commit message.

Cheers,
Andre

>>  
>>      /* We are taking to rank lock to prevent parallel connections.
>> */
>>      vgic_lock_rank(v_target, rank, flags);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[Andrii Anisov]

[-- Attachment #1.1: Type: text/plain, Size: 299 bytes --]

Hello Stefan,

You are absolutely correct.

Sent from my Android.

чт, 24 січ. 2019, 20:47 Nuernberger, Stefan користувач snu@amazon.de пише:

> -    ASSERT(connect && desc);
> +    ASSERT(connect || desc);

I assume it should be ASSERT(!connect || desc);

- Stefan

[-- Attachment #1.2: Type: text/html, Size: 921 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[Andrii Anisov]

[-- Attachment #1.1: Type: text/plain, Size: 1587 bytes --]

Dear Julien,



чт, 24 січ. 2019, 21:27 Julien Grall користувач julien.grall@gmail.com пише:

> (sorry for the formatting)
>
> On Thu, 24 Jan 2019, 18:35 Andrii Anisov, <andrii.anisov@gmail.com> wrote:
>
>> From: Andrii Anisov <andrii_anisov@epam.com>
>>
>
> Empty commit message should only happen when the title provide enough
> information.
>
> In that case, you should explain how you hit the assert so the reviewers
> can understand the change.
>

It was an extremely rare an totally unusual use-case, which nobody else
needs.
I did reboot a guest domain with a device passthroughed to it.


>
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
>> ---
>>  xen/arch/arm/gic-vgic.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index 48922f5..684f2d1 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -443,7 +443,7 @@ int vgic_connect_hw_irq(struct domain *d, storuct
>> vcpu *v, unsigned int virq,
>>      int ret = 0;
>>
>>      /* "desc" is optional when we disconnect an IRQ. */
>> -    ASSERT(connect && desc);
>> +    ASSERT(connect || desc);
>
>
> This looks wrong to me. Now you allow desc to be NULL when connecting an
> IRQ.
>
Yes, sure, the original check is much better: it doesn't allow
disconnecting at all.

This does not even match the comment above.
>
Being upset I mistakingly set another wrong condition. Stefan instead
suggested the right one.

Sent from my Android.

>

[-- Attachment #1.2: Type: text/html, Size: 3373 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[Andrii Anisov]

Stefan,

I hope you would not mind if I put your Suggested-by for v2?

On 25.01.19 08:55, Andrii Anisov wrote:

> You are absolutely correct.
-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[Stefano Stabellini]

Hi Andrii,

Please send an update soon as I would like to get it in 4.12.


Juergen,

I would like to have your release ack on this.


On Fri, 25 Jan 2019, Andrii Anisov wrote:
> Stefan,
> 
> I hope you would not mind if I put your Suggested-by for v2?
> 
> On 25.01.19 08:55, Andrii Anisov wrote:
> 
> > You are absolutely correct.
> -- 
> Sincerely,
> Andrii Anisov.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[Andrii Anisov]

Hello Stefano,

On 05.02.19 03:02, Stefano Stabellini wrote:
> Please send an update soon as I would like to get it in 4.12.

It is already there [1] [2].

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg02048.html
[2] https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=863549158129d326b2e5850f722bfda643264f2b

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.12] gic-vgic: fix assert condition

[Stefano Stabellini]

On Tue, 5 Feb 2019, Andrii Anisov wrote:
> Hello Stefano,
> 
> On 05.02.19 03:02, Stefano Stabellini wrote:
> > Please send an update soon as I would like to get it in 4.12.
> 
> It is already there [1] [2].
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg02048.html
> [2]
> https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=863549158129d326b2e5850f722bfda643264f2b

Ops, I missed it :-)  Even better.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel