* [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
@ 2018-12-11 14:18 Marc Gonzalez
2018-12-13 22:34 ` Winkler, Tomas
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Marc Gonzalez @ 2018-12-11 14:18 UTC (permalink / raw)
To: Vinayak Holikatti, Joao Pinto, Tomas Winkler, Avri Altman,
Martin K. Petersen, Robin Murphy
Cc: MSM, SCSI, Linux ARM
memcpy_fromio() doesn't provide any control over access size.
For example, on arm64, it is implemented using readb and readq.
This may trigger a synchronous external abort:
[ 3.729943] Internal error: synchronous external abort: 96000210 [#1] PREEMPT SMP
[ 3.737000] Modules linked in:
[ 3.744371] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 4.20.0-rc4 #16
[ 3.747413] Hardware name: Qualcomm Technologies, Inc. MSM8998 v1 MTP (DT)
[ 3.755295] pstate: 00000005 (nzcv daif -PAN -UAO)
[ 3.761978] pc : __memcpy_fromio+0x68/0x80
[ 3.766718] lr : ufshcd_dump_regs+0x50/0xb0
[ 3.770767] sp : ffff00000807ba00
[ 3.774830] x29: ffff00000807ba00 x28: 00000000fffffffb
[ 3.778344] x27: ffff0000089db068 x26: ffff8000f6e58000
[ 3.783728] x25: 000000000000000e x24: 0000000000000800
[ 3.789023] x23: ffff8000f6e587c8 x22: 0000000000000800
[ 3.794319] x21: ffff000008908368 x20: ffff8000f6e1ab80
[ 3.799615] x19: 000000000000006c x18: ffffffffffffffff
[ 3.804910] x17: 0000000000000000 x16: 0000000000000000
[ 3.810206] x15: ffff000009199648 x14: ffff000089244187
[ 3.815502] x13: ffff000009244195 x12: ffff0000091ab000
[ 3.820797] x11: 0000000005f5e0ff x10: ffff0000091998a0
[ 3.826093] x9 : 0000000000000000 x8 : ffff8000f6e1ac00
[ 3.831389] x7 : 0000000000000000 x6 : 0000000000000068
[ 3.836676] x5 : ffff8000f6e1abe8 x4 : 0000000000000000
[ 3.841971] x3 : ffff00000928c868 x2 : ffff8000f6e1abec
[ 3.847267] x1 : ffff00000928c868 x0 : ffff8000f6e1abe8
[ 3.852567] Process swapper/0 (pid: 1, stack limit = 0x(____ptrval____))
[ 3.857900] Call trace:
[ 3.864473] __memcpy_fromio+0x68/0x80
[ 3.866683] ufs_qcom_dump_dbg_regs+0x1c0/0x370
[ 3.870522] ufshcd_print_host_regs+0x168/0x190
[ 3.874946] ufshcd_init+0xd4c/0xde0
[ 3.879459] ufshcd_pltfrm_init+0x3c8/0x550
[ 3.883264] ufs_qcom_probe+0x24/0x60
[ 3.887188] platform_drv_probe+0x50/0xa0
Assuming aligned 32-bit registers, let's use readl, after making sure
that 'offset' and 'len' are indeed multiples of 4.
Fixes: ba80917d9932d ("scsi: ufs: ufshcd_dump_regs to use memcpy_fromio")
Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
---
drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 535180c01ce8..320bbd9849bc 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -112,13 +112,19 @@
int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
const char *prefix)
{
- u8 *regs;
+ u32 *regs;
+ size_t pos;
+
+ if (offset % 4 != 0 || len % 4 != 0) /* keep readl happy */
+ return -EINVAL;
regs = kzalloc(len, GFP_KERNEL);
if (!regs)
return -ENOMEM;
- memcpy_fromio(regs, hba->mmio_base + offset, len);
+ for (pos = 0; pos < len; pos += 4)
+ regs[pos / 4] = ufshcd_readl(hba, offset + pos);
+
ufshcd_hex_dump(prefix, regs, len);
kfree(regs);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 8+ messages in thread* RE: [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
2018-12-11 14:18 [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs Marc Gonzalez
@ 2018-12-13 22:34 ` Winkler, Tomas
2018-12-27 15:54 ` Marc Gonzalez
2019-01-09 12:42 ` Marc Gonzalez
2019-01-23 2:13 ` Martin K. Petersen
2 siblings, 1 reply; 8+ messages in thread
From: Winkler, Tomas @ 2018-12-13 22:34 UTC (permalink / raw)
To: Marc Gonzalez, Vinayak Holikatti, Joao Pinto, Avri Altman,
Martin K. Petersen, Robin Murphy
Cc: MSM, SCSI, Linux ARM
> memcpy_fromio() doesn't provide any control over access size.
> For example, on arm64, it is implemented using readb and readq.
> This may trigger a synchronous external abort:
>
> [ 3.729943] Internal error: synchronous external abort: 96000210 [#1]
> PREEMPT SMP
> [ 3.737000] Modules linked in:
> [ 3.744371] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 4.20.0-rc4
> #16
> [ 3.747413] Hardware name: Qualcomm Technologies, Inc. MSM8998 v1
> MTP (DT)
> [ 3.755295] pstate: 00000005 (nzcv daif -PAN -UAO)
> [ 3.761978] pc : __memcpy_fromio+0x68/0x80
> [ 3.766718] lr : ufshcd_dump_regs+0x50/0xb0
> [ 3.770767] sp : ffff00000807ba00
> [ 3.774830] x29: ffff00000807ba00 x28: 00000000fffffffb
> [ 3.778344] x27: ffff0000089db068 x26: ffff8000f6e58000
> [ 3.783728] x25: 000000000000000e x24: 0000000000000800
> [ 3.789023] x23: ffff8000f6e587c8 x22: 0000000000000800
> [ 3.794319] x21: ffff000008908368 x20: ffff8000f6e1ab80
> [ 3.799615] x19: 000000000000006c x18: ffffffffffffffff
> [ 3.804910] x17: 0000000000000000 x16: 0000000000000000
> [ 3.810206] x15: ffff000009199648 x14: ffff000089244187
> [ 3.815502] x13: ffff000009244195 x12: ffff0000091ab000
> [ 3.820797] x11: 0000000005f5e0ff x10: ffff0000091998a0
> [ 3.826093] x9 : 0000000000000000 x8 : ffff8000f6e1ac00
> [ 3.831389] x7 : 0000000000000000 x6 : 0000000000000068
> [ 3.836676] x5 : ffff8000f6e1abe8 x4 : 0000000000000000
> [ 3.841971] x3 : ffff00000928c868 x2 : ffff8000f6e1abec
> [ 3.847267] x1 : ffff00000928c868 x0 : ffff8000f6e1abe8
> [ 3.852567] Process swapper/0 (pid: 1, stack limit = 0x(____ptrval____))
> [ 3.857900] Call trace:
> [ 3.864473] __memcpy_fromio+0x68/0x80
> [ 3.866683] ufs_qcom_dump_dbg_regs+0x1c0/0x370
> [ 3.870522] ufshcd_print_host_regs+0x168/0x190
> [ 3.874946] ufshcd_init+0xd4c/0xde0
> [ 3.879459] ufshcd_pltfrm_init+0x3c8/0x550
> [ 3.883264] ufs_qcom_probe+0x24/0x60
> [ 3.887188] platform_drv_probe+0x50/0xa0
>
> Assuming aligned 32-bit registers, let's use readl, after making sure that 'offset'
> and 'len' are indeed multiples of 4.
>
> Fixes: ba80917d9932d ("scsi: ufs: ufshcd_dump_regs to use memcpy_fromio")
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
LGTM
Thanks
Tomas
> ---
> drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 535180c01ce8..320bbd9849bc 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -112,13 +112,19 @@
> int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
> const char *prefix)
> {
> - u8 *regs;
> + u32 *regs;
> + size_t pos;
> +
> + if (offset % 4 != 0 || len % 4 != 0) /* keep readl happy */
> + return -EINVAL;
>
> regs = kzalloc(len, GFP_KERNEL);
> if (!regs)
> return -ENOMEM;
>
> - memcpy_fromio(regs, hba->mmio_base + offset, len);
> + for (pos = 0; pos < len; pos += 4)
> + regs[pos / 4] = ufshcd_readl(hba, offset + pos);
> +
> ufshcd_hex_dump(prefix, regs, len);
> kfree(regs);
>
> --
> 2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
2018-12-13 22:34 ` Winkler, Tomas
@ 2018-12-27 15:54 ` Marc Gonzalez
0 siblings, 0 replies; 8+ messages in thread
From: Marc Gonzalez @ 2018-12-27 15:54 UTC (permalink / raw)
To: Vinayak Holikatti, Joao Pinto
Cc: SCSI, Martin K. Petersen, MSM, Avri Altman, Winkler, Tomas,
Robin Murphy, Linux ARM
On 13/12/2018 23:34, Winkler, Tomas wrote:
> On 11/12/2018 15:18, Marc Gonzalez wrote:
>
>> Fixes: ba80917d9932d ("scsi: ufs: ufshcd_dump_regs to use memcpy_fromio")
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>
> LGTM
Vinayak, Joao,
What do you think?
Regards.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
2018-12-11 14:18 [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs Marc Gonzalez
2018-12-13 22:34 ` Winkler, Tomas
@ 2019-01-09 12:42 ` Marc Gonzalez
2019-01-09 15:38 ` Jeffrey Hugo
2019-01-23 2:13 ` Martin K. Petersen
2 siblings, 1 reply; 8+ messages in thread
From: Marc Gonzalez @ 2019-01-09 12:42 UTC (permalink / raw)
To: Bjorn Andersson, Andy Gross, Jeffrey Hugo
Cc: SCSI, Martin K. Petersen, Joao Pinto, MSM, Avri Altman,
Tomas Winkler, Vinayak Holikatti, Robin Murphy, Linux ARM
Bjorn, Andy, Jeffrey,
What do you think about the patch below?
Regards.
On 11/12/2018 15:18, Marc Gonzalez wrote:
> memcpy_fromio() doesn't provide any control over access size.
> For example, on arm64, it is implemented using readb and readq.
> This may trigger a synchronous external abort:
>
> [ 3.729943] Internal error: synchronous external abort: 96000210 [#1] PREEMPT SMP
> [ 3.737000] Modules linked in:
> [ 3.744371] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 4.20.0-rc4 #16
> [ 3.747413] Hardware name: Qualcomm Technologies, Inc. MSM8998 v1 MTP (DT)
> [ 3.755295] pstate: 00000005 (nzcv daif -PAN -UAO)
> [ 3.761978] pc : __memcpy_fromio+0x68/0x80
> [ 3.766718] lr : ufshcd_dump_regs+0x50/0xb0
> [ 3.770767] sp : ffff00000807ba00
> [ 3.774830] x29: ffff00000807ba00 x28: 00000000fffffffb
> [ 3.778344] x27: ffff0000089db068 x26: ffff8000f6e58000
> [ 3.783728] x25: 000000000000000e x24: 0000000000000800
> [ 3.789023] x23: ffff8000f6e587c8 x22: 0000000000000800
> [ 3.794319] x21: ffff000008908368 x20: ffff8000f6e1ab80
> [ 3.799615] x19: 000000000000006c x18: ffffffffffffffff
> [ 3.804910] x17: 0000000000000000 x16: 0000000000000000
> [ 3.810206] x15: ffff000009199648 x14: ffff000089244187
> [ 3.815502] x13: ffff000009244195 x12: ffff0000091ab000
> [ 3.820797] x11: 0000000005f5e0ff x10: ffff0000091998a0
> [ 3.826093] x9 : 0000000000000000 x8 : ffff8000f6e1ac00
> [ 3.831389] x7 : 0000000000000000 x6 : 0000000000000068
> [ 3.836676] x5 : ffff8000f6e1abe8 x4 : 0000000000000000
> [ 3.841971] x3 : ffff00000928c868 x2 : ffff8000f6e1abec
> [ 3.847267] x1 : ffff00000928c868 x0 : ffff8000f6e1abe8
> [ 3.852567] Process swapper/0 (pid: 1, stack limit = 0x(____ptrval____))
> [ 3.857900] Call trace:
> [ 3.864473] __memcpy_fromio+0x68/0x80
> [ 3.866683] ufs_qcom_dump_dbg_regs+0x1c0/0x370
> [ 3.870522] ufshcd_print_host_regs+0x168/0x190
> [ 3.874946] ufshcd_init+0xd4c/0xde0
> [ 3.879459] ufshcd_pltfrm_init+0x3c8/0x550
> [ 3.883264] ufs_qcom_probe+0x24/0x60
> [ 3.887188] platform_drv_probe+0x50/0xa0
>
> Assuming aligned 32-bit registers, let's use readl, after making sure
> that 'offset' and 'len' are indeed multiples of 4.
>
> Fixes: ba80917d9932d ("scsi: ufs: ufshcd_dump_regs to use memcpy_fromio")
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> ---
> drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 535180c01ce8..320bbd9849bc 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -112,13 +112,19 @@
> int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
> const char *prefix)
> {
> - u8 *regs;
> + u32 *regs;
> + size_t pos;
> +
> + if (offset % 4 != 0 || len % 4 != 0) /* keep readl happy */
> + return -EINVAL;
>
> regs = kzalloc(len, GFP_KERNEL);
> if (!regs)
> return -ENOMEM;
>
> - memcpy_fromio(regs, hba->mmio_base + offset, len);
> + for (pos = 0; pos < len; pos += 4)
> + regs[pos / 4] = ufshcd_readl(hba, offset + pos);
> +
> ufshcd_hex_dump(prefix, regs, len);
> kfree(regs);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
2019-01-09 12:42 ` Marc Gonzalez
@ 2019-01-09 15:38 ` Jeffrey Hugo
2019-01-16 9:36 ` Marc Gonzalez
0 siblings, 1 reply; 8+ messages in thread
From: Jeffrey Hugo @ 2019-01-09 15:38 UTC (permalink / raw)
To: Marc Gonzalez, Bjorn Andersson, Andy Gross
Cc: SCSI, Martin K. Petersen, Joao Pinto, MSM, Avri Altman,
Tomas Winkler, Vinayak Holikatti, Robin Murphy, Linux ARM
On 1/9/2019 5:42 AM, Marc Gonzalez wrote:
> Bjorn, Andy, Jeffrey,
>
> What do you think about the patch below?
>
> Regards.
>
> On 11/12/2018 15:18, Marc Gonzalez wrote:
>
>> memcpy_fromio() doesn't provide any control over access size.
>> For example, on arm64, it is implemented using readb and readq.
>> This may trigger a synchronous external abort:
>>
>> [ 3.729943] Internal error: synchronous external abort: 96000210 [#1] PREEMPT SMP
>> [ 3.737000] Modules linked in:
>> [ 3.744371] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 4.20.0-rc4 #16
>> [ 3.747413] Hardware name: Qualcomm Technologies, Inc. MSM8998 v1 MTP (DT)
>> [ 3.755295] pstate: 00000005 (nzcv daif -PAN -UAO)
>> [ 3.761978] pc : __memcpy_fromio+0x68/0x80
>> [ 3.766718] lr : ufshcd_dump_regs+0x50/0xb0
>> [ 3.770767] sp : ffff00000807ba00
>> [ 3.774830] x29: ffff00000807ba00 x28: 00000000fffffffb
>> [ 3.778344] x27: ffff0000089db068 x26: ffff8000f6e58000
>> [ 3.783728] x25: 000000000000000e x24: 0000000000000800
>> [ 3.789023] x23: ffff8000f6e587c8 x22: 0000000000000800
>> [ 3.794319] x21: ffff000008908368 x20: ffff8000f6e1ab80
>> [ 3.799615] x19: 000000000000006c x18: ffffffffffffffff
>> [ 3.804910] x17: 0000000000000000 x16: 0000000000000000
>> [ 3.810206] x15: ffff000009199648 x14: ffff000089244187
>> [ 3.815502] x13: ffff000009244195 x12: ffff0000091ab000
>> [ 3.820797] x11: 0000000005f5e0ff x10: ffff0000091998a0
>> [ 3.826093] x9 : 0000000000000000 x8 : ffff8000f6e1ac00
>> [ 3.831389] x7 : 0000000000000000 x6 : 0000000000000068
>> [ 3.836676] x5 : ffff8000f6e1abe8 x4 : 0000000000000000
>> [ 3.841971] x3 : ffff00000928c868 x2 : ffff8000f6e1abec
>> [ 3.847267] x1 : ffff00000928c868 x0 : ffff8000f6e1abe8
>> [ 3.852567] Process swapper/0 (pid: 1, stack limit = 0x(____ptrval____))
>> [ 3.857900] Call trace:
>> [ 3.864473] __memcpy_fromio+0x68/0x80
>> [ 3.866683] ufs_qcom_dump_dbg_regs+0x1c0/0x370
>> [ 3.870522] ufshcd_print_host_regs+0x168/0x190
>> [ 3.874946] ufshcd_init+0xd4c/0xde0
>> [ 3.879459] ufshcd_pltfrm_init+0x3c8/0x550
>> [ 3.883264] ufs_qcom_probe+0x24/0x60
>> [ 3.887188] platform_drv_probe+0x50/0xa0
>>
>> Assuming aligned 32-bit registers, let's use readl, after making sure
>> that 'offset' and 'len' are indeed multiples of 4.
>>
>> Fixes: ba80917d9932d ("scsi: ufs: ufshcd_dump_regs to use memcpy_fromio")
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 535180c01ce8..320bbd9849bc 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -112,13 +112,19 @@
>> int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
>> const char *prefix)
>> {
>> - u8 *regs;
>> + u32 *regs;
>> + size_t pos;
>> +
>> + if (offset % 4 != 0 || len % 4 != 0) /* keep readl happy */
>> + return -EINVAL;
Hmm. It seems like these cases could be handled, but I guess we cannot
necessarily assume that reading past the bounds specified by the client
is safe, so this seems reasonable.
>>
>> regs = kzalloc(len, GFP_KERNEL);
>> if (!regs)
>> return -ENOMEM;
>>
>> - memcpy_fromio(regs, hba->mmio_base + offset, len);
>> + for (pos = 0; pos < len; pos += 4)
>> + regs[pos / 4] = ufshcd_readl(hba, offset + pos);
>> +
>> ufshcd_hex_dump(prefix, regs, len);
>> kfree(regs);
>>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
2019-01-09 15:38 ` Jeffrey Hugo
@ 2019-01-16 9:36 ` Marc Gonzalez
2019-01-16 9:43 ` Winkler, Tomas
0 siblings, 1 reply; 8+ messages in thread
From: Marc Gonzalez @ 2019-01-16 9:36 UTC (permalink / raw)
To: Vinayak Holikatti, Martin K. Petersen, James Bottomley
Cc: SCSI, Avri Altman, Jeffrey Hugo, MSM, Bjorn Andersson,
Joao Pinto, Andy Gross, Tomas Winkler, Robin Murphy, Linux ARM
On 09/01/2019 16:38, Jeffrey Hugo wrote:
>> On 11/12/2018 15:18, Marc Gonzalez wrote:
>>
>>> memcpy_fromio() doesn't provide any control over access size.
>>> For example, on arm64, it is implemented using readb and readq.
>>> This may trigger a synchronous external abort:
>>>
>>> [ 3.729943] Internal error: synchronous external abort: 96000210 [#1] PREEMPT SMP
>>> [ 3.737000] Modules linked in:
>>> [ 3.744371] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 4.20.0-rc4 #16
>>> [ 3.747413] Hardware name: Qualcomm Technologies, Inc. MSM8998 v1 MTP (DT)
>>> [ 3.755295] pstate: 00000005 (nzcv daif -PAN -UAO)
>>> [ 3.761978] pc : __memcpy_fromio+0x68/0x80
>>> [ 3.766718] lr : ufshcd_dump_regs+0x50/0xb0
>>> [ 3.770767] sp : ffff00000807ba00
>>> [ 3.774830] x29: ffff00000807ba00 x28: 00000000fffffffb
>>> [ 3.778344] x27: ffff0000089db068 x26: ffff8000f6e58000
>>> [ 3.783728] x25: 000000000000000e x24: 0000000000000800
>>> [ 3.789023] x23: ffff8000f6e587c8 x22: 0000000000000800
>>> [ 3.794319] x21: ffff000008908368 x20: ffff8000f6e1ab80
>>> [ 3.799615] x19: 000000000000006c x18: ffffffffffffffff
>>> [ 3.804910] x17: 0000000000000000 x16: 0000000000000000
>>> [ 3.810206] x15: ffff000009199648 x14: ffff000089244187
>>> [ 3.815502] x13: ffff000009244195 x12: ffff0000091ab000
>>> [ 3.820797] x11: 0000000005f5e0ff x10: ffff0000091998a0
>>> [ 3.826093] x9 : 0000000000000000 x8 : ffff8000f6e1ac00
>>> [ 3.831389] x7 : 0000000000000000 x6 : 0000000000000068
>>> [ 3.836676] x5 : ffff8000f6e1abe8 x4 : 0000000000000000
>>> [ 3.841971] x3 : ffff00000928c868 x2 : ffff8000f6e1abec
>>> [ 3.847267] x1 : ffff00000928c868 x0 : ffff8000f6e1abe8
>>> [ 3.852567] Process swapper/0 (pid: 1, stack limit = 0x(____ptrval____))
>>> [ 3.857900] Call trace:
>>> [ 3.864473] __memcpy_fromio+0x68/0x80
>>> [ 3.866683] ufs_qcom_dump_dbg_regs+0x1c0/0x370
>>> [ 3.870522] ufshcd_print_host_regs+0x168/0x190
>>> [ 3.874946] ufshcd_init+0xd4c/0xde0
>>> [ 3.879459] ufshcd_pltfrm_init+0x3c8/0x550
>>> [ 3.883264] ufs_qcom_probe+0x24/0x60
>>> [ 3.887188] platform_drv_probe+0x50/0xa0
>>>
>>> Assuming aligned 32-bit registers, let's use readl, after making sure
>>> that 'offset' and 'len' are indeed multiples of 4.
>>>
>>> Fixes: ba80917d9932d ("scsi: ufs: ufshcd_dump_regs to use memcpy_fromio")
>>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>>> ---
>>> drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index 535180c01ce8..320bbd9849bc 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -112,13 +112,19 @@
>>> int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
>>> const char *prefix)
>>> {
>>> - u8 *regs;
>>> + u32 *regs;
>>> + size_t pos;
>>> +
>>> + if (offset % 4 != 0 || len % 4 != 0) /* keep readl happy */
>>> + return -EINVAL;
>
> Hmm. It seems like these cases could be handled, but I guess we cannot
> necessarily assume that reading past the bounds specified by the client
> is safe, so this seems reasonable.
>
>>>
>>> regs = kzalloc(len, GFP_KERNEL);
>>> if (!regs)
>>> return -ENOMEM;
>>>
>>> - memcpy_fromio(regs, hba->mmio_base + offset, len);
>>> + for (pos = 0; pos < len; pos += 4)
>>> + regs[pos / 4] = ufshcd_readl(hba, offset + pos);
>>> +
>>> ufshcd_hex_dump(prefix, regs, len);
>>> kfree(regs);
>>>
>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
James, Martin,
I haven't heard back from Vinayak. (In fact, his last review dates back
to 2016. Should MAINTAINERS be updated? I can send a patch.)
Would you mind taking this patch through the SCSI tree?
Tomas, does "LGTM" translate to Acked-by? :-D
Regards.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
2019-01-16 9:36 ` Marc Gonzalez
@ 2019-01-16 9:43 ` Winkler, Tomas
0 siblings, 0 replies; 8+ messages in thread
From: Winkler, Tomas @ 2019-01-16 9:43 UTC (permalink / raw)
To: Marc Gonzalez, Vinayak Holikatti, Martin K. Petersen, James Bottomley
Cc: SCSI, Avri Altman, Jeffrey Hugo, MSM, Bjorn Andersson,
Joao Pinto, Andy Gross, Robin Murphy, Linux ARM
>
> On 09/01/2019 16:38, Jeffrey Hugo wrote:
>
> >> On 11/12/2018 15:18, Marc Gonzalez wrote:
> >>
> >>> memcpy_fromio() doesn't provide any control over access size.
> >>> For example, on arm64, it is implemented using readb and readq.
> >>> This may trigger a synchronous external abort:
> >>>
> >>> [ 3.729943] Internal error: synchronous external abort: 96000210 [#1]
> PREEMPT SMP
> >>> [ 3.737000] Modules linked in:
> >>> [ 3.744371] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G S 4.20.0-
> rc4 #16
> >>> [ 3.747413] Hardware name: Qualcomm Technologies, Inc. MSM8998 v1
> MTP (DT)
> >>> [ 3.755295] pstate: 00000005 (nzcv daif -PAN -UAO)
> >>> [ 3.761978] pc : __memcpy_fromio+0x68/0x80
> >>> [ 3.766718] lr : ufshcd_dump_regs+0x50/0xb0
> >>> [ 3.770767] sp : ffff00000807ba00
> >>> [ 3.774830] x29: ffff00000807ba00 x28: 00000000fffffffb
> >>> [ 3.778344] x27: ffff0000089db068 x26: ffff8000f6e58000
> >>> [ 3.783728] x25: 000000000000000e x24: 0000000000000800
> >>> [ 3.789023] x23: ffff8000f6e587c8 x22: 0000000000000800
> >>> [ 3.794319] x21: ffff000008908368 x20: ffff8000f6e1ab80
> >>> [ 3.799615] x19: 000000000000006c x18: ffffffffffffffff
> >>> [ 3.804910] x17: 0000000000000000 x16: 0000000000000000
> >>> [ 3.810206] x15: ffff000009199648 x14: ffff000089244187
> >>> [ 3.815502] x13: ffff000009244195 x12: ffff0000091ab000
> >>> [ 3.820797] x11: 0000000005f5e0ff x10: ffff0000091998a0
> >>> [ 3.826093] x9 : 0000000000000000 x8 : ffff8000f6e1ac00
> >>> [ 3.831389] x7 : 0000000000000000 x6 : 0000000000000068
> >>> [ 3.836676] x5 : ffff8000f6e1abe8 x4 : 0000000000000000
> >>> [ 3.841971] x3 : ffff00000928c868 x2 : ffff8000f6e1abec
> >>> [ 3.847267] x1 : ffff00000928c868 x0 : ffff8000f6e1abe8
> >>> [ 3.852567] Process swapper/0 (pid: 1, stack limit = 0x(____ptrval____))
> >>> [ 3.857900] Call trace:
> >>> [ 3.864473] __memcpy_fromio+0x68/0x80
> >>> [ 3.866683] ufs_qcom_dump_dbg_regs+0x1c0/0x370
> >>> [ 3.870522] ufshcd_print_host_regs+0x168/0x190
> >>> [ 3.874946] ufshcd_init+0xd4c/0xde0
> >>> [ 3.879459] ufshcd_pltfrm_init+0x3c8/0x550
> >>> [ 3.883264] ufs_qcom_probe+0x24/0x60
> >>> [ 3.887188] platform_drv_probe+0x50/0xa0
> >>>
> >>> Assuming aligned 32-bit registers, let's use readl, after making
> >>> sure that 'offset' and 'len' are indeed multiples of 4.
> >>>
> >>> Fixes: ba80917d9932d ("scsi: ufs: ufshcd_dump_regs to use
> >>> memcpy_fromio")
> >>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> >>> ---
> >>> drivers/scsi/ufs/ufshcd.c | 10 ++++++++--
> >>> 1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >>> index 535180c01ce8..320bbd9849bc 100644
> >>> --- a/drivers/scsi/ufs/ufshcd.c
> >>> +++ b/drivers/scsi/ufs/ufshcd.c
> >>> @@ -112,13 +112,19 @@
> >>> int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
> >>> const char *prefix)
> >>> {
> >>> - u8 *regs;
> >>> + u32 *regs;
> >>> + size_t pos;
> >>> +
> >>> + if (offset % 4 != 0 || len % 4 != 0) /* keep readl happy */
> >>> + return -EINVAL;
> >
> > Hmm. It seems like these cases could be handled, but I guess we
> > cannot necessarily assume that reading past the bounds specified by
> > the client is safe, so this seems reasonable.
> >
> >>>
> >>> regs = kzalloc(len, GFP_KERNEL);
> >>> if (!regs)
> >>> return -ENOMEM;
> >>>
> >>> - memcpy_fromio(regs, hba->mmio_base + offset, len);
> >>> + for (pos = 0; pos < len; pos += 4)
> >>> + regs[pos / 4] = ufshcd_readl(hba, offset + pos);
> >>> +
> >>> ufshcd_hex_dump(prefix, regs, len);
> >>> kfree(regs);
> >>>
> >
> > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
>
> James, Martin,
>
> I haven't heard back from Vinayak. (In fact, his last review dates back to 2016.
> Should MAINTAINERS be updated? I can send a patch.)
>
> Would you mind taking this patch through the SCSI tree?
>
> Tomas, does "LGTM" translate to Acked-by? :-D
Yes, you can add my Ack.
Thanks
Tomas
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
2018-12-11 14:18 [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs Marc Gonzalez
2018-12-13 22:34 ` Winkler, Tomas
2019-01-09 12:42 ` Marc Gonzalez
@ 2019-01-23 2:13 ` Martin K. Petersen
2 siblings, 0 replies; 8+ messages in thread
From: Martin K. Petersen @ 2019-01-23 2:13 UTC (permalink / raw)
To: Marc Gonzalez
Cc: SCSI, Martin K. Petersen, Joao Pinto, MSM, Avri Altman,
Tomas Winkler, Vinayak Holikatti, Robin Murphy, Linux ARM
Marc,
> memcpy_fromio() doesn't provide any control over access size.
> For example, on arm64, it is implemented using readb and readq.
> This may trigger a synchronous external abort:
Applied to 5.0/scsi-fixes, thanks.
--
Martin K. Petersen Oracle Linux Engineering
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-23 2:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-11 14:18 [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs Marc Gonzalez
2018-12-13 22:34 ` Winkler, Tomas
2018-12-27 15:54 ` Marc Gonzalez
2019-01-09 12:42 ` Marc Gonzalez
2019-01-09 15:38 ` Jeffrey Hugo
2019-01-16 9:36 ` Marc Gonzalez
2019-01-16 9:43 ` Winkler, Tomas
2019-01-23 2:13 ` Martin K. Petersen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).