[v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
diff mbox series

Message ID 3bf7ea8e-4cf2-1f9b-7f46-15609dfcad19@free.fr
State New
Headers show
Series
  • [v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
Related show

Commit Message

Marc Gonzalez Dec. 11, 2018, 2:18 p.m. UTC
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(-)

Comments

Winkler, Tomas Dec. 13, 2018, 10:34 p.m. UTC | #1
> 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
Marc Gonzalez Dec. 27, 2018, 3:54 p.m. UTC | #2
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.
Marc Gonzalez Jan. 9, 2019, 12:42 p.m. UTC | #3
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);
>
Jeffrey Hugo Jan. 9, 2019, 3:38 p.m. UTC | #4
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>
Marc Gonzalez Jan. 16, 2019, 9:36 a.m. UTC | #5
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.
Winkler, Tomas Jan. 16, 2019, 9:43 a.m. UTC | #6
> 
> 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

Patch
diff mbox series

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);