linux-kernel - Re: [PATCH v4 3/5] staging: Introduce NVIDIA Tegra video decoder driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Nov 2017 11:16:41 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Vladimir Zapolskiy <vz@...ia.com>
Cc:     Dmitry Osipenko <digetx@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Stephen Warren <swarren@...dotorg.org>,
        devel@...verdev.osuosl.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-media@...r.kernel.org
Subject: Re: [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