[PATCH v2 0/3] Add comment to memory barrier

[PATCH v2 0/3] Add comment to memory barrier

[Evan Chime]

Include outreachy mailing list in "cc" list. No change

Evan Chime (3):
  Staging: media: omap4iss: Add comment to memory barrier
  Staging: media: omap4iss: Add comment to memory barrier
  Staging: media: omap4iss: Add comment to memory barrier

 drivers/staging/media/omap4iss/iss.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] Staging: media: omap4iss: Add comment to memory barrier

[Evan Chime]

Fix "WARNING: memory barrier without comment" from checkpath. Add comment
to memory barrier smp_wmb() on line 635

Signed-off-by: Evan Chime <chime.evan.dri.devel@gmail.com>
---
 drivers/staging/media/omap4iss/iss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index 1a966cb2f3a6..5156ad8e4b96 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -632,7 +632,7 @@ int omap4iss_module_sync_idle(struct media_entity *me, wait_queue_head_t *wait,
 	 * scenario. We'll call it here to avoid race conditions.
 	 */
 	atomic_set(stopping, 1);
-	smp_wmb();
+	smp_wmb(); /* Prevents stores from being reordered across the barrier */
 
 	/*
 	 * If module is the last one, it's writing to memory. In this case,
-- 
2.17.1

[PATCH v2 2/3] Staging: media: omap4iss: Add comment to memory barrier

[Evan Chime]

Fix "WARNING: memory barrier without comment" from checkpath. Add comment
to memory barrier smp_wmb() on line 653

Signed-off-by: Evan Chime <chime.evan.dri.devel@gmail.com>
---
 drivers/staging/media/omap4iss/iss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index 5156ad8e4b96..ce34c19874c7 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -650,7 +650,7 @@ int omap4iss_module_sync_idle(struct media_entity *me, wait_queue_head_t *wait,
 	if (video->dmaqueue_flags & ISS_VIDEO_DMAQUEUE_UNDERRUN) {
 		spin_unlock_irqrestore(&video->qlock, flags);
 		atomic_set(stopping, 0);
-		smp_wmb();
+		smp_wmb(); /* Prevent stores reordering across the barrier */
 		return 0;
 	}
 	spin_unlock_irqrestore(&video->qlock, flags);
-- 
2.17.1

[PATCH v2 3/3] Staging: media: omap4iss: Add comment to memory barrier

[Evan Chime]

Fix "WARNING: memory barrier without comment" from checkpath. Add comment
to memory barrier smp_wmb() on line 660

Signed-off-by: Evan Chime <chime.evan.dri.devel@gmail.com>
---
 drivers/staging/media/omap4iss/iss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index ce34c19874c7..85b0d9cbaa14 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -657,7 +657,7 @@ int omap4iss_module_sync_idle(struct media_entity *me, wait_queue_head_t *wait,
 	if (!wait_event_timeout(*wait, !atomic_read(stopping),
 				msecs_to_jiffies(1000))) {
 		atomic_set(stopping, 0);
-		smp_wmb();
+		smp_wmb(); /* Prevent stores reordering across the barrier */
 		return -ETIMEDOUT;
 	}
 
-- 
2.17.1

Re: [Outreachy kernel] [PATCH v2 3/3] Staging: media: omap4iss: Add comment to memory barrier

[Julia Lawall]

On Sun, 3 Nov 2019, Evan Chime wrote:

> Fix "WARNING: memory barrier without comment" from checkpath. Add comment
> to memory barrier smp_wmb() on line 660
>
> Signed-off-by: Evan Chime <chime.evan.dri.devel@gmail.com>
> ---
>  drivers/staging/media/omap4iss/iss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index ce34c19874c7..85b0d9cbaa14 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -657,7 +657,7 @@ int omap4iss_module_sync_idle(struct media_entity *me, wait_queue_head_t *wait,
>  	if (!wait_event_timeout(*wait, !atomic_read(stopping),
>  				msecs_to_jiffies(1000))) {
>  		atomic_set(stopping, 0);
> -		smp_wmb();
> +		smp_wmb(); /* Prevent stores reordering across the barrier */

Which stores?

julia

>  		return -ETIMEDOUT;
>  	}
>
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/62b034d416bc0b0f2fc2a10c225bcf1441b4eecc.1572797322.git.chime.evan.dri.devel%40gmail.com.
>

Re: [Outreachy kernel] [PATCH v2 0/3] Add comment to memory barrier

[Julia Lawall]

On Sun, 3 Nov 2019, Evan Chime wrote:

> Include outreachy mailing list in "cc" list. No change
>
> Evan Chime (3):
>   Staging: media: omap4iss: Add comment to memory barrier
>   Staging: media: omap4iss: Add comment to memory barrier
>   Staging: media: omap4iss: Add comment to memory barrier

Each patch should have a different subject.  If you end up with patches
with the same subject, you should make the subjects more specific or merge
them.

In this case, though, I think that this is an issue that only someone with
a deep knowledge of the code could really address.  The question is not
what a memory barrier does, but why one is needed at a particular place in
the code.

julia

>
>  drivers/staging/media/omap4iss/iss.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/cover.1572797322.git.chime.evan.dri.devel%40gmail.com.
>

Re: [Outreachy kernel] [PATCH v2 3/3] Staging: media: omap4iss: Add comment to memory barrier

[evan chime]

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]

On Sun, 3 Nov 2019 at 16:16, Julia Lawall <julia.lawall@lip6.fr> wrote:

>
>
> On Sun, 3 Nov 2019, Evan Chime wrote:
>
> > Fix "WARNING: memory barrier without comment" from checkpath. Add comment
> > to memory barrier smp_wmb() on line 660
> >
> > Signed-off-by: Evan Chime <chime.evan.dri.devel@gmail.com>
> > ---
> >  drivers/staging/media/omap4iss/iss.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/omap4iss/iss.c
> b/drivers/staging/media/omap4iss/iss.c
> > index ce34c19874c7..85b0d9cbaa14 100644
> > --- a/drivers/staging/media/omap4iss/iss.c
> > +++ b/drivers/staging/media/omap4iss/iss.c
> > @@ -657,7 +657,7 @@ int omap4iss_module_sync_idle(struct media_entity
> *me, wait_queue_head_t *wait,
> >       if (!wait_event_timeout(*wait, !atomic_read(stopping),
> >                               msecs_to_jiffies(1000))) {
> >               atomic_set(stopping, 0);
> > -             smp_wmb();
> > +             smp_wmb(); /* Prevent stores reordering across the barrier
> */
>
> Which stores?
>
> julia
>

Memory writes

Kind regards
Evan



> >               return -ETIMEDOUT;
> >       }
> >
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/62b034d416bc0b0f2fc2a10c225bcf1441b4eecc.1572797322.git.chime.evan.dri.devel%40gmail.com
> .
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.1911031716020.2557%40hadrien
> .
>

[-- Attachment #2: Type: text/html, Size: 3424 bytes --]

Re: [Outreachy kernel] [PATCH v2 0/3] Add comment to memory barrier

[evan chime]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

On Sun, 3 Nov 2019 at 16:18, Julia Lawall <julia.lawall@lip6.fr> wrote:

>
>
> On Sun, 3 Nov 2019, Evan Chime wrote:
>
> > Include outreachy mailing list in "cc" list. No change
> >
> > Evan Chime (3):
> >   Staging: media: omap4iss: Add comment to memory barrier
> >   Staging: media: omap4iss: Add comment to memory barrier
> >   Staging: media: omap4iss: Add comment to memory barrier
>
> Each patch should have a different subject.  If you end up with patches
> with the same subject, you should make the subjects more specific or merge
> them.
>
> In this case, though, I think that this is an issue that only someone with
> a deep knowledge of the code could really address.  The question is not
> what a memory barrier does, but why one is needed at a particular place in
> the code.
>
> julia


Thanks for the feedback Julia

Kind Regards
Evan



>
> >
> >  drivers/staging/media/omap4iss/iss.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/cover.1572797322.git.chime.evan.dri.devel%40gmail.com
> .
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.1911031716390.2557%40hadrien
> .
>

[-- Attachment #2: Type: text/html, Size: 2991 bytes --]

Re: [PATCH v2 1/3] Staging: media: omap4iss: Add comment to memory barrier

[Greg KH]

On Sun, Nov 03, 2019 at 04:14:31PM +0000, Evan Chime wrote:
> Fix "WARNING: memory barrier without comment" from checkpath. Add comment
> to memory barrier smp_wmb() on line 635
> 
> Signed-off-by: Evan Chime <chime.evan.dri.devel@gmail.com>
> ---
>  drivers/staging/media/omap4iss/iss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index 1a966cb2f3a6..5156ad8e4b96 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -632,7 +632,7 @@ int omap4iss_module_sync_idle(struct media_entity *me, wait_queue_head_t *wait,
>  	 * scenario. We'll call it here to avoid race conditions.
>  	 */
>  	atomic_set(stopping, 1);
> -	smp_wmb();
> +	smp_wmb(); /* Prevents stores from being reordered across the barrier */

We all know what smp_wmb() does, what you need to document is what
exactly it is here for.  What is it preventing from happening to what
data that is being touched elsewhere where?

If you don't know the code, solving something like this might be very
difficult, sorry.

greg k-h

Re: [PATCH v2 1/3] Staging: media: omap4iss: Add comment to memory barrier

[evan chime]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

On Sun, 3 Nov 2019 at 17:07, Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sun, Nov 03, 2019 at 04:14:31PM +0000, Evan Chime wrote:
> > Fix "WARNING: memory barrier without comment" from checkpath. Add comment
> > to memory barrier smp_wmb() on line 635
> >
> > Signed-off-by: Evan Chime <chime.evan.dri.devel@gmail.com>
> > ---
> >  drivers/staging/media/omap4iss/iss.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/omap4iss/iss.c
> b/drivers/staging/media/omap4iss/iss.c
> > index 1a966cb2f3a6..5156ad8e4b96 100644
> > --- a/drivers/staging/media/omap4iss/iss.c
> > +++ b/drivers/staging/media/omap4iss/iss.c
> > @@ -632,7 +632,7 @@ int omap4iss_module_sync_idle(struct media_entity
> *me, wait_queue_head_t *wait,
> >        * scenario. We'll call it here to avoid race conditions.
> >        */
> >       atomic_set(stopping, 1);
> > -     smp_wmb();
> > +     smp_wmb(); /* Prevents stores from being reordered across the
> barrier */
>
> We all know what smp_wmb() does, what you need to document is what
> exactly it is here for.  What is it preventing from happening to what
> data that is being touched elsewhere where?
>
> If you don't know the code, solving something like this might be very
> difficult, sorry.
>
> greg k-h


Thanks for your feedback Greg. I will have a look at the code

Kind regards
Evan



>

[-- Attachment #2: Type: text/html, Size: 2187 bytes --]

Re: [Outreachy kernel] [PATCH v2 0/3] Add comment to memory barrier

[evan chime]

[-- Attachment #1: Type: text/plain, Size: 2969 bytes --]

On Sun, 3 Nov 2019 at 16:27, evan chime <chime.evan.dri.devel@gmail.com>
wrote:

>
> On Sun, 3 Nov 2019 at 16:18, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>>
>>
>> On Sun, 3 Nov 2019, Evan Chime wrote:
>>
>> > Include outreachy mailing list in "cc" list. No change
>> >
>> > Evan Chime (3):
>> >   Staging: media: omap4iss: Add comment to memory barrier
>> >   Staging: media: omap4iss: Add comment to memory barrier
>> >   Staging: media: omap4iss: Add comment to memory barrier
>>
>> Each patch should have a different subject.  If you end up with patches
>> with the same subject, you should make the subjects more specific or merge
>> them.
>>
>> In this case, though, I think that this is an issue that only someone with
>> a deep knowledge of the code could really address.  The question is not
>> what a memory barrier does, but why one is needed at a particular place in
>> the code.
>>
>> julia
>
>
> Thanks for the feedback Julia
>
> Kind Regards
> Evan
>

Please I have studied the function. From what I understand, the function
checks if ISS submodule needs to wait for next interrupt. If yes, makes the
caller to sleep while waiting for such event.


The atomic_set(stopping, 1) indicates whether the submodule needs to wait,
where stopping is the flag, and atomic_set(stopping, 0) indicates no need
to wait


According to the comment before the first atomic_set() call and the
smp_wmb() call that follows which says:

/* Atomic set() doesn’t include memory barrier on ARM platform for SMP
scenario. We’ll call it here to avoid race conditions */, the smp_wmb()
call is I think, to avoid reordering of the atomic_set() operation


So can I use one of the following, as the comment


A) prevent reordering of atomic_set()


B) sync module with it’s idle state(from the comment that introduced the
function)


C) ensures *stopping is persistent


D) none of the above :)




>
>
>>
>> >
>> >  drivers/staging/media/omap4iss/iss.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > --
>> > 2.17.1
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups "outreachy-kernel" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> > To view this discussion on the web visit
>> https://groups.google.com/d/msgid/outreachy-kernel/cover.1572797322.git.chime.evan.dri.devel%40gmail.com
>> .
>> >
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.1911031716390.2557%40hadrien
>> .
>>
>

[-- Attachment #2: Type: text/html, Size: 8716 bytes --]

Re: [Outreachy kernel] [PATCH v2 0/3] Add comment to memory barrier

[evan chime]

[-- Attachment #1: Type: text/plain, Size: 4279 bytes --]

On Mon, 4 Nov 2019 at 13:26, evan chime <chime.evan.dri.devel@gmail.com>
wrote:

>
>
> On Sun, 3 Nov 2019 at 16:27, evan chime <chime.evan.dri.devel@gmail.com>
> wrote:
>
>>
>> On Sun, 3 Nov 2019 at 16:18, Julia Lawall <julia.lawall@lip6.fr> wrote:
>>
>>>
>>>
>>> On Sun, 3 Nov 2019, Evan Chime wrote:
>>>
>>> > Include outreachy mailing list in "cc" list. No change
>>> >
>>> > Evan Chime (3):
>>> >   Staging: media: omap4iss: Add comment to memory barrier
>>> >   Staging: media: omap4iss: Add comment to memory barrier
>>> >   Staging: media: omap4iss: Add comment to memory barrier
>>>
>>> Each patch should have a different subject.  If you end up with patches
>>> with the same subject, you should make the subjects more specific or
>>> merge
>>> them.
>>>
>>> In this case, though, I think that this is an issue that only someone
>>> with
>>> a deep knowledge of the code could really address.  The question is not
>>> what a memory barrier does, but why one is needed at a particular place
>>> in
>>> the code.
>>>
>>> julia
>>
>>
>> Thanks for the feedback Julia
>>
>> Kind Regards
>> Evan
>>
>
> Please I have studied the function. From what I understand, the function
> checks if ISS submodule needs to wait for next interrupt. If yes, makes the
> caller to sleep while waiting for such event.
>
>
> The atomic_set(stopping, 1) indicates whether the submodule needs to wait,
> where stopping is the flag, and atomic_set(stopping, 0) indicates no need
> to wait
>
>
> According to the comment before the first atomic_set() call and the
> smp_wmb() call that follows which says:
>
> /* Atomic set() doesn’t include memory barrier on ARM platform for SMP
> scenario. We’ll call it here to avoid race conditions */, the smp_wmb()
> call is I think, to avoid reordering of the atomic_set() operation
>
>
> So can I use one of the following, as the comment
>
>
> A) prevent reordering of atomic_set()
>
>
> B) sync module with it’s idle state(from the comment that introduced the
> function)
>
>
> C) ensures *stopping is persistent
>
>
> D) none of the above :)
>

Please forgive me for the first email, just noticed I didn’t say who it was
coming from at the end.


I have studied the function. From what I understand, the function checks if
ISS submodule needs to wait for next interrupt. If yes, makes the caller to
sleep while waiting for such event.


The atomic_set(stopping, 1) indicates whether the submodule needs to wait,
where stopping is the flag, and atomic_set(stopping, 0) indicates no need
to wait


According to the comment before the first atomic_set() call and the
smp_wmb() call that follows which says:

/* Atomic set() doesn’t include memory barrier on ARM platform for SMP
scenario. We’ll call it here to avoid race conditions */, the smp_wmb()
call is I think, to avoid reordering of the atomic_set() operation


So can I use one of the following, as the comment


A) prevent reordering of atomic_set()


B) sync module with it’s idle state(from the comment that introduced the
function)


C) ensures *stopping is persistent


D) none of the above :)


Kind regards

Evan



>
>
>>
>>
>>>
>>> >
>>> >  drivers/staging/media/omap4iss/iss.c | 6 +++---
>>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>>> >
>>> > --
>>> > 2.17.1
>>> >
>>> > --
>>> > You received this message because you are subscribed to the Google
>>> Groups "outreachy-kernel" group.
>>> > To unsubscribe from this group and stop receiving emails from it, send
>>> an email to outreachy-kernel+unsubscribe@googlegroups.com.
>>> > To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/outreachy-kernel/cover.1572797322.git.chime.evan.dri.devel%40gmail.com
>>> .
>>> >
>>>
>>> --
>>> You received this message because you are subscribed to the Google
>>> Groups "outreachy-kernel" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to outreachy-kernel+unsubscribe@googlegroups.com.
>>> To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.1911031716390.2557%40hadrien
>>> .
>>>
>>

[-- Attachment #2: Type: text/html, Size: 16317 bytes --]

[PATCH v2 0/3] Add comment to memory barrier

[ledepiele]

[-- Attachment #1: Type: text/plain, Size: 15 bytes --]

Comment