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.
WARNING: multiple messages have this Message-ID (diff)
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:38 UTC|newest] Thread overview: 16+ 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-11 14:18 ` Marc Gonzalez 2018-12-13 22:34 ` Winkler, Tomas 2018-12-13 22:34 ` Winkler, Tomas 2018-12-27 15:54 ` Marc Gonzalez 2018-12-27 15:54 ` Marc Gonzalez 2019-01-09 12:42 ` Marc Gonzalez 2019-01-09 12:42 ` Marc Gonzalez 2019-01-09 15:38 ` Jeffrey Hugo [this message] 2019-01-09 15:38 ` Jeffrey Hugo 2019-01-16 9:36 ` Marc Gonzalez 2019-01-16 9:36 ` Marc Gonzalez 2019-01-16 9:43 ` Winkler, Tomas 2019-01-16 9:43 ` Winkler, Tomas 2019-01-23 2:13 ` Martin K. Petersen 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.