Sparc

GIT New David Miller 5 participants 9 messages Nov 5, 2015 – Nov 7, 2015

Subsystems (2)

Kernel janitors Checkpatch

lore

Share & tools

lore

David Miller

author

2015-11-05 21:39 UTC #0 lore raw

Just a couple of fixes/cleanups:

1) Correct NUMA latency calculations on sparc64, from Nitin Gupta.

2) ASI_ST_BLKINIT_MRU_S value was wrong, from Rob Gardner.

3) Fix non-faulting load handling of non-quad values, also from Rob Gardner.

4) Cleanup VISsave assembler, from Sam Ravnborg.

5) Fix iommu-common code so it doesn't emit rediculous warnings on some architectures, particularly ARM.

Please pull, thanks a lot!

The following changes since commit 49d7c6559bf2ab4f1d56be131ab9571a51fc71bd:

Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc (2015-08-07 05:28:24 +0300)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc.git

for you to fetch changes up to 52708d690b8be132ba9d294464625dbbdb9fa5df:

sparc64: Fix numa distance values (2015-11-04 12:14:49 -0800)

----------------------------------------------------------------
David S. Miller (1):
iommu-common: Fix error code used in iommu_tbl_range_{alloc,free}().

Nitin Gupta (1):
sparc64: Fix numa distance values

Rob Gardner (2):
sparc64: Fix incorrect ASI_ST_BLKINIT_MRU_S value
sparc64: Don't restrict fp regs for no-fault loads

Sam Ravnborg (1):
sparc64: use ENTRY/ENDPROC in VISsave

 arch/sparc/include/asm/topology_64.h |  3 +++
 arch/sparc/include/uapi/asm/asi.h    |  2 +-
 arch/sparc/kernel/iommu.c            | 12 ++++++------
 arch/sparc/kernel/ldc.c              |  2 +-
 arch/sparc/kernel/pci_sun4v.c        | 18 +++++++++---------
 arch/sparc/kernel/unaligned_64.c     | 22 +++++++++++++++-------
 arch/sparc/lib/VISsave.S             | 10 +++++-----
 arch/sparc/mm/init_64.c              | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/iommu-common.h         |  1 +
 lib/iommu-common.c                   | 10 +++-------
 10 files changed, 113 insertions(+), 37 deletions(-)

Linus Torvalds

2015-11-06 00:43 UTC #1 lore raw

Quoted 3 lines · David Miller

5) Fix iommu-common code so it doesn't emit rediculous warnings on some architectures, particularly ARM.

Heh. So looking at that patch, I can't but help to react that this:

  #define IOMMU_ERROR_CODE       (~(unsigned long) 0)

is still pretty ridiculous. Maybe just (-1ul), or for those people who don't understand C unsigned long arithmetic and think that a negative unsigned long looks odd, (~0ul)?

Not that this *matters*, but it's a bit odd to have to cast constants to perfectly regular C types.

Linus

Linus Torvalds

2015-11-06 00:56 UTC #2 lore raw

Quoted 3 lines · Linus Torvalds

Not that this *matters*, but it's a bit odd to have to cast constants to perfectly regular C types.

Looking around with "git grep", there's a few more of these.

- btrfs seems to like "(unsigned long)-1"

There's a few other users of that too, including more sparc uses.

- scsi/qla seems to like "(unsigned long)<hexnumber>"

- fmdrv_common.h seems to like "((unsigned long)1<<x)" for bitmaps

along with a smattering of random noise all over of "(unsigned long) n" where 'n' is some integer.

Apparently people aren't as aware of the normal "ul" postfix syntax as I would have expected. That said, it's a hundred-odd cases in all of the kernel, so it's still fairly rare.

Linus

Julian Calaby

2015-11-06 01:05 UTC #3 lore raw

Hi Linus,

Quoted 14 lines · Linus Torvalds

Quoted 3 lines · Linus Torvalds
Not that this *matters*, but it's a bit odd to have to cast constants to perfectly regular C types.
Looking around with "git grep", there's a few more of these.
- btrfs seems to like "(unsigned long)-1"
There's a few other users of that too, including more sparc uses.
- scsi/qla seems to like "(unsigned long)<hexnumber>"

Quoted 8 lines

- fmdrv_common.h seems to like "((unsigned long)1<<x)" for bitmaps
along with a smattering of random noise all over of "(unsigned long) n" where 'n' is some integer.
Apparently people aren't as aware of the normal "ul" postfix syntax as I would have expected. That said, it's a hundred-odd cases in all of the kernel, so it's still fairly rare.

Maybe this is something the kernel-janitors team should look at? (CC'd)

Thanks,

Signature · 4 lines

Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/

Julia Lawall

2015-11-06 06:44 UTC #4 lore raw

Quoted 20 lines · Julian Calaby

Hi Linus,
Quoted 16 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
Not that this *matters*, but it's a bit odd to have to cast constants to perfectly regular C types.
Looking around with "git grep", there's a few more of these.
- btrfs seems to like "(unsigned long)-1"
There's a few other users of that too, including more sparc uses.
- scsi/qla seems to like "(unsigned long)<hexnumber>"
- fmdrv_common.h seems to like "((unsigned long)1<<x)" for bitmaps

Quoted 8 lines

Quoted 6 lines
along with a smattering of random noise all over of "(unsigned long) n" where 'n' is some integer.
Apparently people aren't as aware of the normal "ul" postfix syntax as I would have expected. That said, it's a hundred-odd cases in all of the kernel, so it's still fairly rare.
Maybe this is something the kernel-janitors team should look at? (CC'd)

Do you have some concrete examples of the code that is undesirable?

thanks,
julia

Julian Calaby

2015-11-07 03:31 UTC #5 lore raw

Hi Julia,

Quoted 25 lines · Julia Lawall

Quoted 23 lines · Julian Calaby
Hi Linus,
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
Not that this *matters*, but it's a bit odd to have to cast constants to perfectly regular C types.
Looking around with "git grep", there's a few more of these.
- btrfs seems to like "(unsigned long)-1"
There's a few other users of that too, including more sparc uses.
- scsi/qla seems to like "(unsigned long)<hexnumber>"
- fmdrv_common.h seems to like "((unsigned long)1<<x)" for bitmaps
along with a smattering of random noise all over of "(unsigned long) n" where 'n' is some integer.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Apparently people aren't as aware of the normal "ul" postfix syntax as I would have expected. That said, it's a hundred-odd cases in all of the kernel, so it's still fairly rare.
Maybe this is something the kernel-janitors team should look at? (CC'd)
Do you have some concrete examples of the code that is undesirable?

Joe Perches produced a patch for checkpatch to check for this [1]

Linus was complaining [2] about people producing constants with casts to basic C types in them. For instance:

#define IOMMU_ERROR_CODE (~(unsigned long) 0)

This could be replaced with

#define IOMMU_ERROR_CODE (~0ul)

or potentially

#define IOMMU_ERROR_CODE (-1ul)

It's not overly common, but it looks wrong.

Thanks,

Julian Calaby

[1] https://lkml.org/lkml/2015/11/6/48
[2] https://lkml.org/lkml/2015/11/5/785

Signature · 4 lines

Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/

Julia Lawall

2015-11-07 06:04 UTC #6 lore raw

Quoted 45 lines · Julian Calaby

Hi Julia,
Quoted 25 lines · Julia Lawall
Quoted 23 lines · Julian Calaby
Hi Linus,
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
Not that this *matters*, but it's a bit odd to have to cast constants to perfectly regular C types.
Looking around with "git grep", there's a few more of these.
- btrfs seems to like "(unsigned long)-1"
There's a few other users of that too, including more sparc uses.
- scsi/qla seems to like "(unsigned long)<hexnumber>"
- fmdrv_common.h seems to like "((unsigned long)1<<x)" for bitmaps
along with a smattering of random noise all over of "(unsigned long) n" where 'n' is some integer.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Apparently people aren't as aware of the normal "ul" postfix syntax as I would have expected. That said, it's a hundred-odd cases in all of the kernel, so it's still fairly rare.
Maybe this is something the kernel-janitors team should look at? (CC'd)
Do you have some concrete examples of the code that is undesirable?
Joe Perches produced a patch for checkpatch to check for this [1]
Linus was complaining [2] about people producing constants with casts to basic C types in them. For instance:
#define IOMMU_ERROR_CODE (~(unsigned long) 0)
This could be replaced with

Quoted 7 lines

#define IOMMU_ERROR_CODE (~0ul)
or potentially
#define IOMMU_ERROR_CODE (-1ul)
It's not overly common, but it looks wrong.

Thanks. The checkpatch solution indeed looks fine in this case.

julia

Quoted 17 lines

Thanks,

Julian Calaby

[1] https://lkml.org/lkml/2015/11/6/48
[2] https://lkml.org/lkml/2015/11/5/785

Signature · 8 lines

Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Joe Perches

maintainer

2015-11-06 18:32 UTC #7 lore raw

PATCH 1/1 checkpatch: Warn when casting constants to c90 int or longer types

In reply to Linus Torvalds

Quoted 4 lines · Linus Torvalds

I can't but help to react that this:
#define IOMMU_ERROR_CODE (~(unsigned long) 0)
Not that this *matters*, but it's a bit odd to have to cast constants to perfectly regular C types.

So add a test that looks for constants that are cast to standard C90 int or longer types and suggest using C90 "6.4.4.1 Integer constants" integer-suffixes instead.

Miscellanea:

o Add a --fix option too

Signed-off-by Joe Perches <[email protected]>

---
 scripts/checkpatch.pl | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

scripts/checkpatch.pl +42 -0

index 2d88cbf9..24cdeb0 100755
@@ -431,6 +431,28 @@ our @typeList = (
 	qr{${Ident}_handler_fn},
 	@typeListMisordered,
 );
+
+our $C90_int_types = qr{(?x:
+	long\s+long\s+int\s+(?:un)?signed|
+	long\s+long\s+(?:un)?signed\s+int|
+	long\s+long\s+(?:un)?signed|
+	(?:(?:un)?signed\s+)?long\s+long\s+int|
+	(?:(?:un)?signed\s+)?long\s+long|
+	int\s+long\s+long\s+(?:un)?signed|
+	int\s+(?:(?:un)?signed\s+)?long\s+long|
+
+	long\s+int\s+(?:un)?signed|
+	long\s+(?:un)?signed\s+int|
+	long\s+(?:un)?signed|
+	(?:(?:un)?signed\s+)?long\s+int|
+	(?:(?:un)?signed\s+)?long|
+	int\s+long\s+(?:un)?signed|
+	int\s+(?:(?:un)?signed\s+)?long|
+
+	int\s+(?:un)?signed|
+	(?:(?:un)?signed\s+)?int
+)};
+
 our @typeListFile = ();
 our @typeListWithAttr = (
 	@typeList,

@@ -5240,6 +5262,26 @@ sub process {
 			}
 		}
 
+# check for cast of C90 native int or longer types constants
+		if ($line =~ /(\(\s*$C90_int_types\s*\)\s*)($Constant)\b/) {
+			my $cast = $1;
+			my $const = $2;
+			if (WARN("TYPECAST_INT_CONSTANT",
+				 "Unnecessary typecast of c90 int constant\n" . $herecurr) &&
+			    $fix) {
+				my $suffix = "";
+				my $newconst = $const;
+				$newconst =~ s/${Int_type}$//;
+				$suffix .= 'U' if ($cast =~ /\bunsigned\b/);
+				if ($cast =~ /\blong\s+long\b/) {
+					$suffix .= 'LL';
+				} elsif ($cast =~ /\blong\b/) {
+					$suffix .= 'L';
+				}
+				$fixed[$fixlinenr] =~ s/\Q$cast\E$const\b/$newconst$suffix/;
+			}
+		}
+
 # check for sizeof(&)
 		if ($line =~ /\bsizeof\s*\(\s*\&/) {
 			WARN("SIZEOF_ADDRESS",

David Miller

author

2015-11-06 05:14 UTC #8 lore raw

In reply to Linus Torvalds

From: Linus Torvalds <[email protected]>
Date: Thu, 5 Nov 2015 16:43:52 -0800

Quoted 5 lines · David Miller

Quoted 3 lines
5) Fix iommu-common code so it doesn't emit rediculous warnings on some architectures, particularly ARM.

Quoted 7 lines

Heh. So looking at that patch, I can't but help to react that this:
  #define IOMMU_ERROR_CODE       (~(unsigned long) 0)
is still pretty ridiculous. Maybe just (-1ul), or for those people who don't understand C unsigned long arithmetic and think that a negative unsigned long looks odd, (~0ul)?

Yeah I struggled with this, believe it or not.

The expression causing the problem was (~(dma_addr_t) 0) (AKA DMA_ERROR_CODE) being used as the return value.

So I just replicated that expression. A neuron did fire saying "hey pinhead, why not just use ~0UL" but I did not listen to the little voice in my head this time.