Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers

[Michael S. Tsirkin]

On Mon, Jan 04, 2016 at 02:15:50PM -0800, Joe Perches wrote:
> On Mon, 2016-01-04 at 22:45 +0200, Michael S. Tsirkin wrote:
> > On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
> > > On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> > > > SMP-only barriers were missing in checkpatch.pl
> > > > 
> > > > Refactor code slightly to make adding more variants easier.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin 
> > > > ---
> > > >  scripts/checkpatch.pl | 9 -
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > index 2b3c228..0245bbe 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -5116,7 +5116,14 @@ sub process {
> > > >     }
> > > >     }
> > > >  # check for memory barriers without a comment.
> > > > -   if ($line =~ 
> > > > /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/)
> > > >  {
> > > > +
> > > > +   my @barriers = ('mb', 'rmb', 'wmb', 
> > > > 'read_barrier_depends');
> > > > +   my @smp_barriers = ('smp_store_release', 
> > > > 'smp_load_acquire', 'smp_store_mb');
> > > > +
> > > > +   @smp_barriers = (@smp_barriers, map {"smp_" . $_} 
> > > > @barriers);
> > > 
> > > I think using map, which so far checkpatch doesn't use,
> > > makes smp_barriers harder to understand and it'd be
> > > better to enumerate them.
> > 
> > Okay - I'll rewrite using foreach.
> > 
> 
> I think using the qr form (like 'our $Attribute' and
> a bunch of others) is the general style that should
> be used for checkpatch.

Thanks - that's what I did in the new version (included in
v3 of the arch cleanup patchset now).

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers

[Michael S. Tsirkin]

On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
> On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> > SMP-only barriers were missing in checkpatch.pl
> > 
> > Refactor code slightly to make adding more variants easier.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> >  scripts/checkpatch.pl | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 2b3c228..0245bbe 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5116,7 +5116,14 @@ sub process {
> >     }
> >     }
> >  # check for memory barriers without a comment.
> > -   if ($line =~ 
> > /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/)
> >  {
> > +
> > +   my @barriers = ('mb', 'rmb', 'wmb', 'read_barrier_depends');
> > +   my @smp_barriers = ('smp_store_release', 'smp_load_acquire', 
> > 'smp_store_mb');
> > +
> > +   @smp_barriers = (@smp_barriers, map {"smp_" . $_} @barriers);
> 
> I think using map, which so far checkpatch doesn't use,
> makes smp_barriers harder to understand and it'd be
> better to enumerate them.

Okay - I'll rewrite using foreach.

> > +   my $all_barriers = join('|', (@barriers, @smp_barriers));
> > +
> > +   if ($line =~ /\b($all_barriers)\(/) {
> 
> It would be better to use /\b$all_barriers\s*\(/
> as there's no reason for the capture and there
> could be a space between the function and the
> open parenthesis.

That's the way it was - space before ( will trigger other
warnings. But sure, ok.

> 
> >     if (!ctx_has_comment($first_line, $linenr)) {
> >     WARN("MEMORY_BARRIER",
> >      "memory barrier without comment\n" . 
> > $herecurr);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 1/3] checkpatch.pl: add missing memory barriers

[Michael S. Tsirkin]

SMP-only barriers were missing in checkpatch.pl

Refactor code slightly to make adding more variants easier.

Signed-off-by: Michael S. Tsirkin 
---
 scripts/checkpatch.pl | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b3c228..0245bbe 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5116,7 +5116,14 @@ sub process {
}
}
 # check for memory barriers without a comment.
-   if ($line =~ 
/\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/)
 {
+
+   my @barriers = ('mb', 'rmb', 'wmb', 'read_barrier_depends');
+   my @smp_barriers = ('smp_store_release', 'smp_load_acquire', 
'smp_store_mb');
+
+   @smp_barriers = (@smp_barriers, map {"smp_" . $_} @barriers);
+   my $all_barriers = join('|', (@barriers, @smp_barriers));
+
+   if ($line =~ /\b($all_barriers)\(/) {
if (!ctx_has_comment($first_line, $linenr)) {
WARN("MEMORY_BARRIER",
 "memory barrier without comment\n" . 
$herecurr);
-- 
MST

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers

[Joe Perches]

On Mon, 2016-01-04 at 22:45 +0200, Michael S. Tsirkin wrote:
> On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
> > On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> > > SMP-only barriers were missing in checkpatch.pl
> > > 
> > > Refactor code slightly to make adding more variants easier.
> > > 
> > > Signed-off-by: Michael S. Tsirkin 
> > > ---
> > >  scripts/checkpatch.pl | 9 -
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 2b3c228..0245bbe 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -5116,7 +5116,14 @@ sub process {
> > >   }
> > >   }
> > >  # check for memory barriers without a comment.
> > > - if ($line =~ 
> > > /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/)
> > >  {
> > > +
> > > + my @barriers = ('mb', 'rmb', 'wmb', 'read_barrier_depends');
> > > + my @smp_barriers = ('smp_store_release', 'smp_load_acquire', 
> > > 'smp_store_mb');
> > > +
> > > + @smp_barriers = (@smp_barriers, map {"smp_" . $_} @barriers);
> > 
> > I think using map, which so far checkpatch doesn't use,
> > makes smp_barriers harder to understand and it'd be
> > better to enumerate them.
> 
> Okay - I'll rewrite using foreach.
> 

I think using the qr form (like 'our $Attribute' and
a bunch of others) is the general style that should
be used for checkpatch.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers

[Russell King - ARM Linux]

On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
> On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> > +   my $all_barriers = join('|', (@barriers, @smp_barriers));
> > +
> > +   if ($line =~ /\b($all_barriers)\(/) {
> 
> It would be better to use /\b$all_barriers\s*\(/
> as there's no reason for the capture and there
> could be a space between the function and the
> open parenthesis.

I think you mean

/\b(?:$all_barriers)\s*\(/

as 'all_barriers' will be:

mb|wmb|rmb|smp_mb|smp_wmb|smp_rmb

and putting that into your suggestion results in:

/\bmb|wmb|rmb|smp_mb|smp_wmb|smp_rmb\s*\(/

which is clearly wrong - the \b only applies to 'mb' and the \s*\( only
applies to smp_rmb.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers

[Joe Perches]

On Mon, 2016-01-04 at 16:11 +, Russell King - ARM Linux wrote:
> On Mon, Jan 04, 2016 at 08:07:40AM -0800, Joe Perches wrote:
> > On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> > > + my $all_barriers = join('|', (@barriers, @smp_barriers));
> > > +
> > > + if ($line =~ /\b($all_barriers)\(/) {
> > 
> > It would be better to use /\b$all_barriers\s*\(/
> > as there's no reason for the capture and there
> > could be a space between the function and the
> > open parenthesis.
> 
> I think you mean
> 
>   /\b(?:$all_barriers)\s*\(/
> 
> as 'all_barriers' will be:
> 
>   mb|wmb|rmb|smp_mb|smp_wmb|smp_rmb
> 
> and putting that into your suggestion results in:
> 
>   /\bmb|wmb|rmb|smp_mb|smp_wmb|smp_rmb\s*\(/
> 
> which is clearly wrong - the \b only applies to 'mb' and the \s*\( only
> applies to smp_rmb.

right, thanks.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] checkpatch.pl: add missing memory barriers

[Joe Perches]

On Mon, 2016-01-04 at 13:36 +0200, Michael S. Tsirkin wrote:
> SMP-only barriers were missing in checkpatch.pl
> 
> Refactor code slightly to make adding more variants easier.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  scripts/checkpatch.pl | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2b3c228..0245bbe 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5116,7 +5116,14 @@ sub process {
>   }
>   }
>  # check for memory barriers without a comment.
> - if ($line =~ 
> /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/)
>  {
> +
> + my @barriers = ('mb', 'rmb', 'wmb', 'read_barrier_depends');
> + my @smp_barriers = ('smp_store_release', 'smp_load_acquire', 
> 'smp_store_mb');
> +
> + @smp_barriers = (@smp_barriers, map {"smp_" . $_} @barriers);

I think using map, which so far checkpatch doesn't use,
makes smp_barriers harder to understand and it'd be
better to enumerate them.

> + my $all_barriers = join('|', (@barriers, @smp_barriers));
> +
> + if ($line =~ /\b($all_barriers)\(/) {

It would be better to use /\b$all_barriers\s*\(/
as there's no reason for the capture and there
could be a space between the function and the
open parenthesis.


>   if (!ctx_has_comment($first_line, $linenr)) {
>   WARN("MEMORY_BARRIER",
>    "memory barrier without comment\n" . 
> $herecurr);

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev