From: Jeffrey Hugo <jhugo@codeaurora.org>
To: Marc Gonzalez <marc.w.gonzalez@free.fr>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Andy Gross <andy.gross@linaro.org>
Cc: SCSI <linux-scsi@vger.kernel.org>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
Joao Pinto <jpinto@synopsys.com>,
MSM <linux-arm-msm@vger.kernel.org>,
Avri Altman <avri.altman@wdc.com>,
Tomas Winkler <tomas.winkler@intel.com>,
Vinayak Holikatti <vinholikatti@gmail.com>,
Robin Murphy <robin.murphy@arm.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v1] scsi: ufs: Use explicit access size in ufshcd_dump_regs
Date: Wed, 9 Jan 2019 08:38:57 -0700 [thread overview]
Message-ID: <4c367e20-3ae8-ba28-75dc-b07805e27ea8@codeaurora.org> (raw)
In-Reply-To: <214700f8-a381-5740-47c0-54923dae60b1@free.fr>
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
next prev parent reply other threads:[~2019-01-09 15:39 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2019-01-16 9:36 ` Marc Gonzalez
2019-01-16 9:43 ` Winkler, Tomas
2019-01-23 2:13 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4c367e20-3ae8-ba28-75dc-b07805e27ea8@codeaurora.org \
--to=jhugo@codeaurora.org \
--cc=andy.gross@linaro.org \
--cc=avri.altman@wdc.com \
--cc=bjorn.andersson@linaro.org \
--cc=jpinto@synopsys.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=marc.w.gonzalez@free.fr \
--cc=martin.petersen@oracle.com \
--cc=robin.murphy@arm.com \
--cc=tomas.winkler@intel.com \
--cc=vinholikatti@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).