From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E7797C34022 for ; Thu, 27 Feb 2020 15:13:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BEFA6246A1 for ; Thu, 27 Feb 2020 15:13:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="G2r3t7rr" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730445AbgB0PNn (ORCPT ); Thu, 27 Feb 2020 10:13:43 -0500 Received: from mail-il1-f196.google.com ([209.85.166.196]:40146 "EHLO mail-il1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729819AbgB0PNl (ORCPT ); Thu, 27 Feb 2020 10:13:41 -0500 Received: by mail-il1-f196.google.com with SMTP id i7so2719422ilr.7 for ; Thu, 27 Feb 2020 07:13:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=m+kylmRZdQ7kHJRK9lKNLqwlSVhHwyYol2gsDgRlfLA=; b=G2r3t7rraJO/jHNvyP5CFwnplNk9lJtpT9K3gnayhn4rcbOA3gSXoz8oCLeE81E/Wt no0Zh7DuWczVpOtO+5F+bZ/zXJJtqem4r07fo5SkxDWfHb9mDBp6nKik1l4JDM8KN/al qqemDCNP9bkaEgHme33rJac7E7n0rUJSoO+8E= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=m+kylmRZdQ7kHJRK9lKNLqwlSVhHwyYol2gsDgRlfLA=; b=PttHz4C34jh8IuHMYZWkqeXRR6cRoCzvAUbvoFNFv3GDGM6p01w0eAZQAHKY4VFjkt fxsZKuLm/sssnzhFlaAbQxQzKlVyvIJlH1HqTFMcbQAmqYWr1mwm+PLghfg375SjXenq UIjAa25SrU5+BYXrBLvNJMPTLxcDwOXK39tE+hqy/y5YZqWCaOy+EEPS9E7NcDyGNt7R mYEKGEWw4iCSzBaSxgMpOQ8ZEX1S+qtJpoksUpzyu4g2plt+zDPXtCvXvF/bENam6Q0l qjJsA9cT22HwUKuTP4PHpTvIVCVUQ/Qon4Ro+1m9bx1bw4I4iuz03ihiz/6HgEm9/Bv+ Ecyw== X-Gm-Message-State: APjAAAVSw7djtD8Ou3+i/Vlx1Ic8c/DCxJFAHRY9u4wZZzYRJXR3x/IY 6ACXSV0J1E+j+5Ynb3p+EqwWhv9kV80ME9/gwR/k4w== X-Google-Smtp-Source: APXvYqx6ZTgm/JnuJem8YlsSHDER8b5Wil6I0Af4SkhwFk++vp7lU3TdAvSta4JoAfspzedl//Jug2AVYKDt3UXKMdc= X-Received: by 2002:a92:d610:: with SMTP id w16mr5875950ilm.283.1582816420405; Thu, 27 Feb 2020 07:13:40 -0800 (PST) MIME-Version: 1.0 References: <20200225172437.106679-1-hsinyi@chromium.org> <6986e879-cf35-13a5-baae-9ab09ba1a0d7@xs4all.nl> In-Reply-To: <6986e879-cf35-13a5-baae-9ab09ba1a0d7@xs4all.nl> From: Hsin-Yi Wang Date: Thu, 27 Feb 2020 23:13:14 +0800 Message-ID: Subject: Re: [PATCH v3] media: mtk-vpu: avoid unaligned access to DTCM buffer. To: Hans Verkuil Cc: "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" , Minghsiu Tsai , Houlong Wei , Andrew-CT Chen , Tiffany Lin , Mauro Carvalho Chehab , Matthias Brugger , Enric Balletbo i Serra , linux-media@vger.kernel.org, linux-mediatek@lists.infradead.org, lkml Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 27, 2020 at 5:50 PM Hans Verkuil wrote: > > On 2/25/20 6:24 PM, Hsin-Yi Wang wrote: > > struct vpu_run *run in vpu_init_ipi_handler() is an ioremapped DTCM (Data > > Tightly Coupled Memory) buffer shared with AP. It's not able to do > > unaligned access. Otherwise kernel would crash due to unable to handle > > kernel paging request. > > > > struct vpu_run { > > u32 signaled; > > char fw_ver[VPU_FW_VER_LEN]; > > unsigned int dec_capability; > > unsigned int enc_capability; > > wait_queue_head_t wq; > > }; > > > > fw_ver starts at 4 byte boundary. If system enables > > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, strscpy() will do > > read_word_at_a_time(), which tries to read 8-byte: *(unsigned long *)addr > > > > Copy the string by memcpy_fromio() for this buffer to avoid unaligned > > access. > > > > Fixes: 85709cbf1524 ("media: replace strncpy() by strscpy()") > > Signed-off-by: Hsin-Yi Wang > > --- > > Change in v3: > > - fix sparse warnings. > > Change in v2: > > - fix sparse warnings. > > --- > > drivers/media/platform/mtk-vpu/mtk_vpu.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c b/drivers/media/platform/mtk-vpu/mtk_vpu.c > > index a768707abb94..e3fd2d1814f3 100644 > > --- a/drivers/media/platform/mtk-vpu/mtk_vpu.c > > +++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c > > @@ -603,12 +603,14 @@ EXPORT_SYMBOL_GPL(vpu_load_firmware); > > static void vpu_init_ipi_handler(void *data, unsigned int len, void *priv) > > { > > struct mtk_vpu *vpu = (struct mtk_vpu *)priv; > > - struct vpu_run *run = (struct vpu_run *)data; > > - > > - vpu->run.signaled = run->signaled; > > - strscpy(vpu->run.fw_ver, run->fw_ver, sizeof(vpu->run.fw_ver)); > > - vpu->run.dec_capability = run->dec_capability; > > - vpu->run.enc_capability = run->enc_capability; > > + struct vpu_run __iomem *run = (struct vpu_run __iomem __force *)data; > > The use of __force is generally a bad sign. Shouldn't the 'void *data' be a > 'void __iomem *data'? And vpu->recv_buf should be 'struct share_obj __iomem *recv_buf;'. > Probably send_buf as well. > > In other words, the __iomem attribute should be wired up correctly throughout the > driver code, and not forcibly applied in one place. That is asking for trouble in > the future. Also, sparse only works well in detecting problems if such attributes > are applied at the right level. > > Regards, > > Hans > Thanks for your comments. I should check the whole code more thoroughly. I do see that vpu->recv_buf is forced cast from void __iomem *tcm: vpu->recv_buf = (__force struct share_obj *)(vpu->reg.tcm +VPU_DTCM_OFFSET); I'll use struct share_obj __iomem *recv_buf; as you suggested. Thanks Since all handlers (vpu_init_ipi_handler, vpu_enc_ipi_handler, vpu_dec_ipi_handler, and mtk_mdp_vpu_ipi_handler) only do read access to this buffer, I think we can also change 'void *data' as 'const void *data', and pass another buffer copied from vpu->recv_buf->share_buf to handler. In this way we don't have to change to use iomem APIs in those handlers. static void vpu_ipi_handler(struct mtk_vpu *vpu) { - struct share_obj *rcv_obj = vpu->recv_buf; + struct share_obj __iomem *rcv_obj = vpu->recv_buf; struct vpu_ipi_desc *ipi_desc = vpu->ipi_desc; - - if (rcv_obj->id < IPI_MAX && ipi_desc[rcv_obj->id].handler) { - ipi_desc[rcv_obj->id].handler(rcv_obj->share_buf,... ... + unsigned char data[SHARE_BUF_SIZE]; + s32 id = readl(&rcv_obj->id); + + memcpy_fromio(data, rcv_obj->share_buf, sizeof(data)); + if (id < IPI_MAX && ipi_desc[id].handler) { + ipi_desc[id].handler(data, readl(&rcv_obj->len), ... ...