lkml.org 
[lkml]   [2017]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 3/5] staging: Introduce NVIDIA Tegra video decoder driver
On Sat, Nov 11, 2017 at 04:06:52PM +0200, Vladimir Zapolskiy wrote:
> > + if (!wait_dma)
> > + return 0;
> > +
> > + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
> > + !(value & BSE_DMA_BUSY), 1, 100);
> > + if (err) {
> > + dev_err(dev, "BSEV DMA timeout\n");
> > + return err;
> > + }
> > +
> > + return 0;
>
> if (err)
> dev_err(dev, "BSEV DMA timeout\n");
>
> return err;
>
> is two lines shorter.
>

This is fine, but just watch out because getting clever with a last if
statement is a common anti-pattern. For example, you often see it where
people do success handling instead of failure handling. And it leads
to static checker bugs, and makes the code slightly more subtle.

> > + err = tegra_vde_attach_dmabuf(dev, source->aux_fd,
> > + source->aux_offset, csize,
> > + &frame->aux_dmabuf_attachment,
> > + &frame->aux_addr,
> > + &frame->aux_sgt,
> > + NULL, dma_dir);
> > + if (err)
> > + goto err_release_cr;
> > + }
> > +
> > + return 0;
>
> if (!err)
> return 0;
>
> and then remove a check above.
>

Argh!!!! Success handling. Always do failure handling, never success
handling.

The rest of your comments I agree with, though.

regards,
dan carpenter


\
 
 \ /
  Last update: 2017-11-13 09:18    [W:0.183 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site