On Mon, 3 Jun 2024 at 09:38, Jeuk Kim <jeuk20.kim@gmail.com> wrote:
From: Minwoo Im <minwoo.im@samsung.com>
This patch adds support for MCQ defined in UFSHCI 4.0. This patch
utilized the legacy I/O codes as much as possible to support MCQ.
MCQ operation & runtime register is placed at 0x1000 offset of UFSHCI
register statically with no spare space among four registers (48B):
UfsMcqSqReg, UfsMcqSqIntReg, UfsMcqCqReg, UfsMcqCqIntReg
The maxinum number of queue is 32 as per spec, and the default
MAC(Multiple Active Commands) are 32 in the device.
Example:
-device ufs,serial=foo,id=ufs0,mcq=true,mcq-maxq=8
Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
Reviewed-by: Jeuk Kim <jeuk20.kim@samsung.com>
Message-Id: <20240528023106.856777-3-minwoo.im@samsung.com>
Signed-off-by: Jeuk Kim <jeuk20.kim@samsung.com>
Hi; Coverity reported a potential issue with this code.
I don't think it's an actual bug, but it would be nice to
clean it up and keep Coverity happy. (CID 1546866).
static uint64_t ufs_mmio_read(void *opaque, hwaddr addr, unsigned size)
{
UfsHc *u = (UfsHc *)opaque;
- uint8_t *ptr = (uint8_t *)&u->reg;
+ uint8_t *ptr;
uint64_t value;
-
- if (addr > sizeof(u->reg) - size) {
Before this change, we checked addr against (sizeof(u->reg) - size).
+ uint64_t offset;
+
+ if (addr < sizeof(u->reg)) {
Now we changed to check it against sizeof(u->reg).
That means Coverity thinks it's possible that we could
have addr = sizeof(u->reg) - 1...
+ offset = addr;
+ ptr = (uint8_t *)&u->reg;
+ } else if (ufs_is_mcq_reg(u, addr)) {
+ offset = addr - ufs_mcq_reg_addr(u, 0);
+ ptr = (uint8_t *)&u->mcq_reg;
+ } else if (ufs_is_mcq_op_reg(u, addr)) {
+ offset = addr - ufs_mcq_op_reg_addr(u, 0);
+ ptr = (uint8_t *)&u->mcq_op_reg;
+ } else {
trace_ufs_err_invalid_register_offset(addr);
return 0;
}
- value = *(uint32_t *)(ptr + addr);
+ value = *(uint32_t *)(ptr + offset);
...so Coverity thinks that this write of a 32-bit value
might overrun the end of the array.
trace_ufs_mmio_read(addr, value, size);
return value;
Side note: why use uint8_t* for the type of "ptr" in these
functions? We know it must be a uint32_t* (it comes either from
the u->reg or from one of these u_mcq_reg or u->mcq_op_reg
fields, and they must all be uint32_t), and using the right
type would reduce the number of casts you need to do.
It also helps the reader a little, because using a uint8_t
implies that you're indexing into an array-of-bytes, and
if you were doing that it would be a bug (because both of
it not handling endianness correctly and because of it
not handling alignment correctly).
}
@@ -423,13 +802,17 @@ static void ufs_mmio_write(void *opaque, hwaddr addr,
uint64_t data,
{
UfsHc *u = (UfsHc *)opaque;
- if (addr > sizeof(u->reg) - size) {
+ trace_ufs_mmio_write(addr, data, size);
+
+ if (addr < sizeof(u->reg)) {
Similarly here we changed the bounds check we were doing.
+ ufs_write_reg(u, addr, data, size);
+ } else if (ufs_is_mcq_reg(u, addr)) {
+ ufs_write_mcq_reg(u, addr - ufs_mcq_reg_addr(u, 0), data, size);
+ } else if (ufs_is_mcq_op_reg(u, addr)) {
+ ufs_write_mcq_op_reg(u, addr - ufs_mcq_op_reg_addr(u, 0), data, size);
+ } else {
trace_ufs_err_invalid_register_offset(addr);
- return;
}
-
- trace_ufs_mmio_write(addr, data, size);
- ufs_write_reg(u, addr, data, size);
thanks
-- PMM