Re: [PATCH v3 02/10] include: Move compat_timespec/ timeval to compat_time.h
On Mon, Jan 15, 2018 at 06:18:10PM -0800, Deepa Dinamani wrote: > All the current architecture specific defines for these > are the same. Refactor these common defines to a common > header file. > > The new common linux/compat_time.h is also useful as it > will eventually be used to hold all the defines that > are needed for compat time types that support non y2038 > safe types. New architectures need not have to define these > new types as they will only use new y2038 safe syscalls. > This file can be deleted after y2038 when we stop supporting > non y2038 safe syscalls. ... > arch/mips/include/asm/compat.h| 11 --- > arch/mips/kernel/signal32.c | 2 +- For MIPS: Acked-by: James Hogan <jho...@kernel.org> Cheers James signature.asc Description: Digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.
On Thu, Nov 30, 2017 at 03:09:33PM -0800, David Daney wrote:
> On 11/30/2017 02:56 PM, James Hogan wrote:
> > On Thu, Nov 30, 2017 at 01:49:43PM -0800, David Daney wrote:
> >> On 11/30/2017 01:36 PM, James Hogan wrote:
> >>> On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
> >>>> Signed-off-by: Carlos Munoz <cmu...@cavium.com>
> >>>> Signed-off-by: Steven J. Hill <steven.h...@cavium.com>
> >>>> Signed-off-by: David Daney <david.da...@cavium.com>
> >>>> ---
> >>>>arch/mips/cavium-octeon/setup.c | 6 ++
> >>>>arch/mips/include/asm/octeon/octeon.h | 12 ++--
> >>>>2 files changed, 16 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/mips/cavium-octeon/setup.c
> >>>> b/arch/mips/cavium-octeon/setup.c
> >>>> index a8034d0dcade..99e6a68bc652 100644
> >>>> --- a/arch/mips/cavium-octeon/setup.c
> >>>> +++ b/arch/mips/cavium-octeon/setup.c
> >>>> @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
> >>>>#else
> >>>> cvmmemctl.s.cvmsegenak = 0;
> >>>>#endif
> >>>> +if (OCTEON_IS_OCTEON3()) {
> >>>> +/* Enable LMTDMA */
> >>>> +cvmmemctl.s.lmtena = 1;
> >>>> +/* Scratch line to use for LMT operation */
> >>>> +cvmmemctl.s.lmtline = 2;
> >>>
> >>> Out of curiosity, is there significance to the value 2 and associated
> >>> virtual address 0x8100, or is it pretty arbitrary?
> >>
> >> Yes, there is significance.
> >>
> >> CPU local memory starts at 0x8000, each line is 0x80 bytes.
> >> so the 2nd line starts at 0x8100
> >
> > What I mean is, why is 2 chosen instead of any other value?
>
> That is explained in the change log of patch 5/8:
>
>
> 1st 128-bytes: Use by IOBDMA
> 2nd 128-bytes: Reserved by kernel for scratch/TLS emulation.
> 3rd 128-bytes: OCTEON-III LMTLINE
Ah yes. Perhaps it deserves a brief comment in the code, or even an
enum.
Cheers
James
signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.
On Thu, Nov 30, 2017 at 01:49:43PM -0800, David Daney wrote:
> On 11/30/2017 01:36 PM, James Hogan wrote:
> > On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
> >> Signed-off-by: Carlos Munoz <cmu...@cavium.com>
> >> Signed-off-by: Steven J. Hill <steven.h...@cavium.com>
> >> Signed-off-by: David Daney <david.da...@cavium.com>
> >> ---
> >> arch/mips/cavium-octeon/setup.c | 6 ++
> >> arch/mips/include/asm/octeon/octeon.h | 12 ++--
> >> 2 files changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/mips/cavium-octeon/setup.c
> >> b/arch/mips/cavium-octeon/setup.c
> >> index a8034d0dcade..99e6a68bc652 100644
> >> --- a/arch/mips/cavium-octeon/setup.c
> >> +++ b/arch/mips/cavium-octeon/setup.c
> >> @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
> >> #else
> >>cvmmemctl.s.cvmsegenak = 0;
> >> #endif
> >> + if (OCTEON_IS_OCTEON3()) {
> >> + /* Enable LMTDMA */
> >> + cvmmemctl.s.lmtena = 1;
> >> + /* Scratch line to use for LMT operation */
> >> + cvmmemctl.s.lmtline = 2;
> >
> > Out of curiosity, is there significance to the value 2 and associated
> > virtual address 0x8100, or is it pretty arbitrary?
>
> Yes, there is significance.
>
> CPU local memory starts at 0x8000, each line is 0x80 bytes.
> so the 2nd line starts at 0x8100
What I mean is, why is 2 chosen instead of any other value?
Cheers
James
signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.
On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
> From: Carlos Munoz <cmu...@cavium.com>
>
> LMTDMA/LMTST operations move data between cores and I/O devices:
>
> * LMTST operations can send an address and a variable length
> (up to 128 bytes) of data to an I/O device.
> * LMTDMA operations can send an address and a variable length
> (up to 128) of data to the I/O device and then return a
> variable length (up to 128 bytes) response from the IOI device.
Should that be "I/O"?
>
> Signed-off-by: Carlos Munoz <cmu...@cavium.com>
> Signed-off-by: Steven J. Hill <steven.h...@cavium.com>
> Signed-off-by: David Daney <david.da...@cavium.com>
> ---
> arch/mips/cavium-octeon/setup.c | 6 ++
> arch/mips/include/asm/octeon/octeon.h | 12 ++--
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
> index a8034d0dcade..99e6a68bc652 100644
> --- a/arch/mips/cavium-octeon/setup.c
> +++ b/arch/mips/cavium-octeon/setup.c
> @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
> #else
> cvmmemctl.s.cvmsegenak = 0;
> #endif
> + if (OCTEON_IS_OCTEON3()) {
> + /* Enable LMTDMA */
> + cvmmemctl.s.lmtena = 1;
> + /* Scratch line to use for LMT operation */
> + cvmmemctl.s.lmtline = 2;
Out of curiosity, is there significance to the value 2 and associated
virtual address 0x8100, or is it pretty arbitrary?
> + }
> /* R/W If set, CVMSEG is available for loads/stores in
>* supervisor mode. */
> cvmmemctl.s.cvmsegenas = 0;
> diff --git a/arch/mips/include/asm/octeon/octeon.h
> b/arch/mips/include/asm/octeon/octeon.h
> index c99c4b6a79f4..92a17d67c1fa 100644
> --- a/arch/mips/include/asm/octeon/octeon.h
> +++ b/arch/mips/include/asm/octeon/octeon.h
> @@ -179,7 +179,15 @@ union octeon_cvmemctl {
> /* RO 1 = BIST fail, 0 = BIST pass */
> __BITFIELD_FIELD(uint64_t wbfbist:1,
> /* Reserved */
> - __BITFIELD_FIELD(uint64_t reserved:17,
> + __BITFIELD_FIELD(uint64_t reserved_52_57:6,
> + /* When set, LMTDMA/LMTST operations are permitted */
> + __BITFIELD_FIELD(uint64_t lmtena:1,
> + /* Selects the CVMSEG LM cacheline used by LMTDMA
> + * LMTST and wide atomic store operations.
> + */
> + __BITFIELD_FIELD(uint64_t lmtline:6,
> + /* Reserved */
> + __BITFIELD_FIELD(uint64_t reserved_41_44:4,
> /* OCTEON II - TLB replacement policy: 0 = bitmask LRU; 1 = NLU.
>* This field selects between the TLB replacement policies:
>* bitmask LRU or NLU. Bitmask LRU maintains a mask of
> @@ -275,7 +283,7 @@ union octeon_cvmemctl {
> /* R/W Size of local memory in cache blocks, 54 (6912
>* bytes) is max legal value. */
> __BITFIELD_FIELD(uint64_t lmemsz:6,
> - ;)
> + ;
> } s;
> };
Regardless, the patch looks good to me.
Reviewed-by: James Hogan <jho...@kernel.org>
Cheers
James
signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 03/02/14 10:35, David Laight wrote:
From: James Hogan
On 03/02/14 10:05, David Laight wrote:
Architectures that define such alignment rules are a right PITA.
You either need to get the size to 2 without using 'packed', or
just not define such structures.
It is worth seeing if adding aligned(2) will change the size - I'm
not sure.
__aligned(2) alone doesn't seem to have any effect on sizeof() or
__alignof__() unless it is accompanied by __packed. x86_64 is similar in
that respect (it just packs sanely in the first place).
Combining __packed with __aligned(2) does the trick though (__packed
alone sets __aligned(1) which is obviously going to be suboptimal).
Compile some code for a cpu that doesn't support misaligned transfers
(probably one of sparc, arm, ppc) and see if the compiler generates a
single 16bit request or two 8 bits ones.
You don't want the compiler generating multiple byte-sized memory transfers.
Meta is also one of those arches, and according to my quick tests,
__packed alone does correctly make it fall back to byte loads/stores,
but with __packed __aligned(2) it uses 16bit loads/stores. I've also
confirmed that with an ARM toolchain (see below for example).
Cheers
James
input:
#define __aligned(x)__attribute__((aligned(x)))
#define __packed__attribute__((packed))
union a {
short x, y;
} __aligned(2) __packed;
struct b {
short x;
} __aligned(2) __packed;
unsigned int soa = sizeof(union a);
unsigned int aoa = __alignof__(union a);
unsigned int sob = sizeof(struct b);
unsigned int aob = __alignof__(struct b);
void t(struct b *x, union a *y)
{
++x-x;
++y-x;
}
ARM output (-O2):
.cpu arm10tdmi
.fpu softvfp
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 2
.eabi_attribute 30, 2
.eabi_attribute 34, 0
.eabi_attribute 18, 4
.file alignment4.c
.text
.align 2
.global t
.type t, %function
t:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
ldrhr3, [r0, #0]
add r3, r3, #1
strhr3, [r0, #0]@ movhi
ldrhr3, [r1, #0]
add r3, r3, #1
strhr3, [r1, #0]@ movhi
bx lr
.size t, .-t
.global aob
.global sob
.global aoa
.global soa
.data
.align 2
.type aob, %object
.size aob, 4
aob:
.word 2
.type sob, %object
.size sob, 4
sob:
.word 2
.type aoa, %object
.size aoa, 4
aoa:
.word 2
.type soa, %object
.size soa, 4
soa:
.word 2
.ident GCC: (GNU) 4.7.1 20120606 (Red Hat 4.7.1-0.1.20120606)
.section.note.GNU-stack,,%progbits
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
Hi Dan, On 20/01/14 21:13, Dan Carpenter wrote: I made a quick and dirty sparse patch to check for this. I don't think I will bother trying to send it to sparse upstream, but you can if you want to. It found 289 unions which might need a __packed added. The lustre unions were not in my allmodconfig so they're not listed. Thanks a lot for this, it seems to be useful. I'm adapting it to reduce false negatives (e.g. omitting the check if the struct/union is already packed), and I imagine it could be made to only warn about padded unpacked structs/unions which are used as nested members of packed structs/unions. It wouldn't catch everything but would probably catch a lot of cases that are most likely to be genuine since they would have been packed at the outer level for a reason. Perhaps there could be a command line option or a pragma so that unions will work in the kernel. We don't care about linking to outside libraries. We still interact with userland via structs and unions, so it would probably have to exclude anything in uapi/. Cheers James signature.asc Description: OpenPGP digital signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
Hi Chen,
On 19/01/14 10:07, Chen Gang wrote:
BTW: this patch is related with another patch which is discussing (so I
have cc that patch to you and Greg too): if we could sure that it is a
compiler's feature issue, we will skip this patch.
If you're referring to the #pragma pack portability issue then this
issue is unrelated since it doesn't use #pragma pack.
The issue is *not* that the compiler is defectively failing to pack
nested structs. Doing that would be utterly broken since it would change
the layout of the same struct depending on where it is placed in the
program, Consider this example (which uses a nested struct rather than
union to demonstrate the point):
struct a {
struct b {
unsigned int x;
unsigned short y;
} x;
unsigned short y;
} __packed;
Both ABIs behave the same here:
Archsizeof(struct b)sizeof(struct a)
x86_64 8 10
metag 8 10
If struct b is made __packed, again both ABIs behave differently in the
same way:
Archsizeof(struct b)sizeof(struct a)
x86_64 6 8
metag 6 8
The issue is that C compiler ABIs may (and unfortunately metag ABI does)
pack structs and unions to at least 4 bytes, even if no members of the
struct or union are that large, which means that the nested struct/union
should be __packed too to portably ensure it's the expected size.
Cheers
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: lustre: lustre: include: add __attribute__((packed)) for the related union
On 20/01/14 12:30, Dan Carpenter wrote:
Ah. From so metag is a new arch and not a compiler like the changelog
says.
On Mon, Jan 20, 2014 at 11:56:47AM +, James Hogan wrote:
struct a {
struct b {
unsigned int x;
unsigned short y;
} x;
unsigned short y;
} __packed;
This is not the code we are discussing. It should look like:
struct a {
union {
short x;
short y;
}
short z;
};
Any normal person would assume that sizeof(struct a) would be 4 but
apparently on metag it is 8. That totally defeats the point of using
a union in the first place. It's easy enough to add a __packed to the
lustre declaration but I expect this to cause an endless stream of bugs.
It it is really stupid.
I agree completely (and did request this be changed when I first found
out about it, but since it's an ABI issue it was really too late).
That's why I'm not actively pushing for every case to be fixed unless
it's in generic code that actually affects metag.
Cheers
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
