On Mon, 3 Jun 2024 at 09:38, Jeuk Kim <jeuk20....@gmail.com> wrote:
>
> From: Minwoo Im <minwoo...@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...@samsung.com>
> Reviewed-by: Jeuk Kim <jeuk20....@samsung.com>
> Message-Id: <20240528023106.856777-3-minwoo...@samsung.com>
> Signed-off-by: Jeuk Kim <jeuk20....@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
