Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
Hi Randy, On 02. 09. 19 18:52, Linus Torvalds wrote: > On Mon, Sep 2, 2019 at 6:17 AM Michal Simek wrote: >> >> Randy/Linus: Are you going create regular patch from this? > > Since I can't even test it, and Randy did most of the work (and that > last patch worked for him too), I'd suggest he just send it in as his. > > You can add my acked-by or sign-of depending on how you want to do it > (but I really don't need authorship credit, it might as well go to > Randy). Can you please send v4 on this one? Thanks, Michal signature.asc Description: OpenPGP digital signature
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On Mon, Sep 2, 2019 at 6:17 AM Michal Simek wrote: > > Randy/Linus: Are you going create regular patch from this? Since I can't even test it, and Randy did most of the work (and that last patch worked for him too), I'd suggest he just send it in as his. You can add my acked-by or sign-of depending on how you want to do it (but I really don't need authorship credit, it might as well go to Randy). Linus
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On 9/1/19 9:58 PM, Linus Torvalds wrote: > On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap wrote: >> >> I guess we need a way to coerce that to call get_user_1(), >> such as a typecast. This _seems_ to work (i.e., call get_user_1()): > > No, I oversimplified. > > Try this slightly modified patch instead. > Yes, that builds cleanly. thanks. -- ~Randy
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On 02. 09. 19 6:58, Linus Torvalds wrote: > On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap wrote: >> >> I guess we need a way to coerce that to call get_user_1(), >> such as a typecast. This _seems_ to work (i.e., call get_user_1()): > > No, I oversimplified. > > Try this slightly modified patch instead. This one looks good. I have also tested it on HW. I will run some LTP tests to see if there is any new error. If there is better testsuite to validate this please let me know. Randy/Linus: Are you going create regular patch from this? Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Xilinx Microblaze Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs signature.asc Description: OpenPGP digital signature
RE: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
From: Randy Dunlap > Sent: 01 September 2019 15:56 > arch/microblaze/ is missing support for get_user() of size 8 bytes, > so add it by using __copy_from_user(). Ugg Use get_user() for 4 bytes twice and combine the returned values. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On Sun, Sep 1, 2019 at 7:10 PM Randy Dunlap wrote:
>
> I guess we need a way to coerce that to call get_user_1(),
> such as a typecast. This _seems_ to work (i.e., call get_user_1()):
No, I oversimplified.
Try this slightly modified patch instead.
Linus
arch/microblaze/include/asm/uaccess.h | 42 ---
1 file changed, 9 insertions(+), 33 deletions(-)
diff --git a/arch/microblaze/include/asm/uaccess.h b/arch/microblaze/include/asm/uaccess.h
index bff2a71c828a..a1f206b90753 100644
--- a/arch/microblaze/include/asm/uaccess.h
+++ b/arch/microblaze/include/asm/uaccess.h
@@ -163,44 +163,15 @@ extern long __user_bad(void);
* Returns zero on success, or -EFAULT on error.
* On error, the variable @x is set to zero.
*/
-#define get_user(x, ptr) \
- __get_user_check((x), (ptr), sizeof(*(ptr)))
-
-#define __get_user_check(x, ptr, size) \
-({ \
- unsigned long __gu_val = 0; \
- const typeof(*(ptr)) __user *__gu_addr = (ptr); \
- int __gu_err = 0; \
- \
- if (access_ok(__gu_addr, size)) { \
- switch (size) { \
- case 1: \
- __get_user_asm("lbu", __gu_addr, __gu_val, \
- __gu_err); \
- break; \
- case 2: \
- __get_user_asm("lhu", __gu_addr, __gu_val, \
- __gu_err); \
- break; \
- case 4: \
- __get_user_asm("lw", __gu_addr, __gu_val, \
- __gu_err); \
- break; \
- default: \
- __gu_err = __user_bad(); \
- break; \
- } \
- } else { \
- __gu_err = -EFAULT; \
- }\
- x = (__force typeof(*(ptr)))__gu_val;\
- __gu_err; \
+#define get_user(x, ptr) ({\
+ const typeof(*(ptr)) __user *__gu_ptr = (ptr); \
+ access_ok(__gu_ptr, sizeof(*__gu_ptr)) ? \
+ __get_user(x, __gu_ptr) : -EFAULT; \
})
#define __get_user(x, ptr) \
({ \
unsigned long __gu_val = 0; \
- /*unsigned long __gu_ptr = (unsigned long)(ptr);*/ \
long __gu_err; \
switch (sizeof(*(ptr))) { \
case 1:\
@@ -212,6 +183,11 @@ extern long __user_bad(void);
case 4:\
__get_user_asm("lw", (ptr), __gu_val, __gu_err); \
break; \
+ case 8:\
+ __gu_err = __copy_from_user(&__gu_val, ptr, 8); \
+ if (__gu_err) \
+ __gu_err = -EFAULT;\
+ break; \
default: \
/* __gu_val = 0; __gu_err = -EINVAL;*/ __gu_err = __user_bad();\
}\
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On 9/1/19 12:10 PM, Randy Dunlap wrote:
> On 9/1/19 10:31 AM, Linus Torvalds wrote:
>> On Sun, Sep 1, 2019 at 10:07 AM Linus Torvalds
>> wrote:
>>>
>>> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ...
>
> It was just a response to the 0day build bot reporting build errors.
>
>
>> Ugh. As I was going to apply it, my code cleanliness conscience struck.
>>
>> I can't deal with that unnecessary duplication of code. Does something
>> like the attached patch work instead?
>>
>> Totally untested, but looks much cleaner.
>
> Hm, I'm getting one (confusing) build error, in block/scsi_ioctl.c:
>
> CC block/scsi_ioctl.o
> In file included from ../include/linux/uaccess.h:11,
> from ../include/linux/highmem.h:9,
> from ../include/linux/pagemap.h:11,
> from ../include/linux/blkdev.h:16,
> from ../block/scsi_ioctl.c:9:
> ../block/scsi_ioctl.c: In function 'sg_scsi_ioctl':
> ../arch/microblaze/include/asm/uaccess.h:167:25: error: invalid initializer
> typeof(ptr) __gu_ptr = (ptr); \
> ^
> ../block/scsi_ioctl.c:426:6: note: in expansion of macro 'get_user'
> if (get_user(opcode, sic->data))
> ^~~~
if (get_user(opcode, sic->data))
return -EFAULT;
where sic->data is unsigned char data[0] here:
typedef struct scsi_ioctl_command {
unsigned int inlen;
unsigned int outlen;
unsigned char data[0];
} Scsi_Ioctl_Command;
On x86_64 this builds as a call to get_user_1().
(cannot do objdump on arch/microblaze/, unknown arch/machine)
I guess we need a way to coerce that to call get_user_1(),
such as a typecast. This _seems_ to work (i.e., call get_user_1()):
if (get_user(opcode, (unsigned char *)(sic->data)))
return -EFAULT;
??
--
~Randy
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On 9/1/19 10:33 AM, Michal Simek wrote: > Hi, > > On 01. 09. 19 19:07, Linus Torvalds wrote: >> On Sun, Sep 1, 2019 at 7:55 AM Randy Dunlap wrote: >>> >>> What is a reasonable path for having this patch merged? >>> I have sent several emails to Micahl Simek but he seems to have >>> dropped active maintenance of arch/microblaze/. >> >> Yeah, I haven't gotten a pull request from him since March, and that >> was a trivial fixup. >> >> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ... > > I am still around. The reason why I didn't send any pull request was > simply there were no patches. For 5.4 there will be some patches. > > Randy: I found one email which was sent July and this thread when I had > vacation. I will take a look at both tmr. If there is something else > please resend. Good to hear that you are still around. I'll let you know if I find other lost patches. thanks. -- ~Randy
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On 9/1/19 10:31 AM, Linus Torvalds wrote: > On Sun, Sep 1, 2019 at 10:07 AM Linus Torvalds > wrote: >> >> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ... It was just a response to the 0day build bot reporting build errors. > Ugh. As I was going to apply it, my code cleanliness conscience struck. > > I can't deal with that unnecessary duplication of code. Does something > like the attached patch work instead? > > Totally untested, but looks much cleaner. Hm, I'm getting one (confusing) build error, in block/scsi_ioctl.c: CC block/scsi_ioctl.o In file included from ../include/linux/uaccess.h:11, from ../include/linux/highmem.h:9, from ../include/linux/pagemap.h:11, from ../include/linux/blkdev.h:16, from ../block/scsi_ioctl.c:9: ../block/scsi_ioctl.c: In function 'sg_scsi_ioctl': ../arch/microblaze/include/asm/uaccess.h:167:25: error: invalid initializer typeof(ptr) __gu_ptr = (ptr); \ ^ ../block/scsi_ioctl.c:426:6: note: in expansion of macro 'get_user' if (get_user(opcode, sic->data)) ^~~~ -- ~Randy
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
Hi, On 01. 09. 19 19:07, Linus Torvalds wrote: > On Sun, Sep 1, 2019 at 7:55 AM Randy Dunlap wrote: >> >> What is a reasonable path for having this patch merged? >> I have sent several emails to Micahl Simek but he seems to have >> dropped active maintenance of arch/microblaze/. > > Yeah, I haven't gotten a pull request from him since March, and that > was a trivial fixup. > > I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ... I am still around. The reason why I didn't send any pull request was simply there were no patches. For 5.4 there will be some patches. Randy: I found one email which was sent July and this thread when I had vacation. I will take a look at both tmr. If there is something else please resend. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Xilinx Microblaze Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs signature.asc Description: OpenPGP digital signature
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On Sun, Sep 1, 2019 at 10:07 AM Linus Torvalds
wrote:
>
> I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ...
Ugh. As I was going to apply it, my code cleanliness conscience struck.
I can't deal with that unnecessary duplication of code. Does something
like the attached patch work instead?
Totally untested, but looks much cleaner.
Linus
arch/microblaze/include/asm/uaccess.h | 42 ---
1 file changed, 9 insertions(+), 33 deletions(-)
diff --git a/arch/microblaze/include/asm/uaccess.h b/arch/microblaze/include/asm/uaccess.h
index bff2a71c828a..88bde06a41a3 100644
--- a/arch/microblaze/include/asm/uaccess.h
+++ b/arch/microblaze/include/asm/uaccess.h
@@ -163,44 +163,15 @@ extern long __user_bad(void);
* Returns zero on success, or -EFAULT on error.
* On error, the variable @x is set to zero.
*/
-#define get_user(x, ptr) \
- __get_user_check((x), (ptr), sizeof(*(ptr)))
-
-#define __get_user_check(x, ptr, size) \
-({ \
- unsigned long __gu_val = 0; \
- const typeof(*(ptr)) __user *__gu_addr = (ptr); \
- int __gu_err = 0; \
- \
- if (access_ok(__gu_addr, size)) { \
- switch (size) { \
- case 1: \
- __get_user_asm("lbu", __gu_addr, __gu_val, \
- __gu_err); \
- break; \
- case 2: \
- __get_user_asm("lhu", __gu_addr, __gu_val, \
- __gu_err); \
- break; \
- case 4: \
- __get_user_asm("lw", __gu_addr, __gu_val, \
- __gu_err); \
- break; \
- default: \
- __gu_err = __user_bad(); \
- break; \
- } \
- } else { \
- __gu_err = -EFAULT; \
- }\
- x = (__force typeof(*(ptr)))__gu_val;\
- __gu_err; \
+#define get_user(x, ptr) ({\
+ typeof(ptr) __gu_ptr = (ptr); \
+ access_ok(__gu_ptr, sizeof(*__gu_ptr)) ? \
+ __get_user(x, __gu_ptr) : -EFAULT; \
})
#define __get_user(x, ptr) \
({ \
unsigned long __gu_val = 0; \
- /*unsigned long __gu_ptr = (unsigned long)(ptr);*/ \
long __gu_err; \
switch (sizeof(*(ptr))) { \
case 1:\
@@ -212,6 +183,11 @@ extern long __user_bad(void);
case 4:\
__get_user_asm("lw", (ptr), __gu_val, __gu_err); \
break; \
+ case 8:\
+ __gu_err = __copy_from_user(&__gu_val, ptr, 8); \
+ if (__gu_err) \
+ __gu_err = -EFAULT;\
+ break; \
default: \
/* __gu_val = 0; __gu_err = -EINVAL;*/ __gu_err = __user_bad();\
}\
Re: [PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
On Sun, Sep 1, 2019 at 7:55 AM Randy Dunlap wrote: > > What is a reasonable path for having this patch merged? > I have sent several emails to Micahl Simek but he seems to have > dropped active maintenance of arch/microblaze/. Yeah, I haven't gotten a pull request from him since March, and that was a trivial fixup. I guess I'll apply it. I'm not sure why you _care_ about microblaze, but ... Linus
[PATCH v3] arch/microblaze: add support for get_user() of size 8 bytes
From: Randy Dunlap
arch/microblaze/ is missing support for get_user() of size 8 bytes,
so add it by using __copy_from_user().
Fixes these build errors:
drivers/infiniband/core/uverbs_main.o: In function `ib_uverbs_write':
drivers/infiniband/core/.tmp_gl_uverbs_main.o:(.text+0x13a4): undefined
reference to `__user_bad'
drivers/android/binder.o: In function `binder_thread_write':
drivers/android/.tmp_gl_binder.o:(.text+0xda6c): undefined reference to
`__user_bad'
drivers/android/.tmp_gl_binder.o:(.text+0xda98): undefined reference to
`__user_bad'
drivers/android/.tmp_gl_binder.o:(.text+0xdf10): undefined reference to
`__user_bad'
drivers/android/.tmp_gl_binder.o:(.text+0xe498): undefined reference to
`__user_bad'
drivers/android/binder.o:drivers/android/.tmp_gl_binder.o:(.text+0xea78):
more undefined references to `__user_bad' follow
'make allmodconfig' now builds successfully for arch/microblaze/.
Fixes: 538722ca3b76 ("microblaze: fix get_user/put_user side-effects")
Reported-by: kbuild test robot
Signed-off-by: Randy Dunlap
Cc: Al Viro
Cc: Steven J. Magnani
Cc: Michal Simek
Cc: Jason Gunthorpe
Cc: Leon Romanovsky
Cc: Andrew Morton
Cc: Doug Ledford
---
v3: fix return value in error case (comments from Linus).
What is a reasonable path for having this patch merged?
I have sent several emails to Micahl Simek but he seems to have
dropped active maintenance of arch/microblaze/.
arch/microblaze/include/asm/uaccess.h | 10 ++
1 file changed, 10 insertions(+)
--- lnx-53-rc6.orig/arch/microblaze/include/asm/uaccess.h
+++ lnx-53-rc6/arch/microblaze/include/asm/uaccess.h
@@ -186,6 +186,11 @@ extern long __user_bad(void);
__get_user_asm("lw", __gu_addr, __gu_val, \
__gu_err); \
break; \
+ case 8: \
+ __gu_err = __copy_from_user(&__gu_val, __gu_addr, 8);\
+ if (__gu_err) /* bytes remaining */ \
+ __gu_err = -EFAULT; \
+ break; \
default:\
__gu_err = __user_bad();\
break; \
@@ -212,6 +217,11 @@ extern long __user_bad(void);
case 4: \
__get_user_asm("lw", (ptr), __gu_val, __gu_err);\
break; \
+ case 8: \
+ __gu_err = __copy_from_user(&__gu_val, ptr, 8); \
+ if (__gu_err) /* bytes remaining */ \
+ __gu_err = -EFAULT; \
+ break; \
default:\
/* __gu_val = 0; __gu_err = -EINVAL;*/ __gu_err = __user_bad();\
} \
