Re: [PATCH v3] media: mtk-vpu: avoid unaligned access to DTCM buffer.

[Hsin-Yi Wang]

On Thu, Feb 27, 2020 at 5:50 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> 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 <hsinyi@xxxxxxxxxxxx>
> > ---
> > 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), ...
...