Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 5 Mar 2019 09:53:18 +0200
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     "Cao, Bingbu" <bingbu.cao@intel.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Zhi, Yong" <yong.zhi@intel.com>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
Message-ID: <20190305075317.4t32uyyhzftuoebp@kekkonen.localdomain>
References: <20190304202758.1802417-1-arnd@arndb.de>
 <EE45BB6704246A4E914B70E8B61FB42A15C131D5@SHSMSX104.ccr.corp.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <EE45BB6704246A4E914B70E8B61FB42A15C131D5@SHSMSX104.ccr.corp.intel.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Bingbu, Arnd,

On Tue, Mar 05, 2019 at 12:25:18AM +0000, Cao, Bingbu wrote:
...
> > @@ -1744,7 +1745,7 @@ int imgu_css_fmt_try(struct imgu_css *css,
> >  	struct v4l2_rect *const bds = &r[IPU3_CSS_RECT_BDS];
> >  	struct v4l2_rect *const env = &r[IPU3_CSS_RECT_ENVELOPE];
> >  	struct v4l2_rect *const gdc = &r[IPU3_CSS_RECT_GDC];
> > -	struct imgu_css_queue q[IPU3_CSS_QUEUES];
> > +	struct imgu_css_queue *q = kcalloc(IPU3_CSS_QUEUES, sizeof(struct
> > +imgu_css_queue), GFP_KERNEL);
> 
> Could you use the devm_kcalloc()? 

No, because this is not related to the device, called instead on
e.g. VIDIOC_TRY_FMT.

> >  	struct v4l2_pix_format_mplane *const in =
> >  					&q[IPU3_CSS_QUEUE_IN].fmt.mpix;
> >  	struct v4l2_pix_format_mplane *const out = @@ -1753,6 +1754,11 @@
> > int imgu_css_fmt_try(struct imgu_css *css,
> >  					&q[IPU3_CSS_QUEUE_VF].fmt.mpix;
> >  	int i, s, ret;
> > 
> > +	if (!q) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> [Cao, Bingbu] 
> The goto here is wrong, you can just report an error, and I prefer it is next to the alloc.

I agree, the goto is just not needed.

> > +
> >  	/* Adjust all formats, get statistics buffer sizes and formats */
> >  	for (i = 0; i < IPU3_CSS_QUEUES; i++) {
> >  		if (fmts[i])
> > @@ -1766,7 +1772,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> >  					IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> >  			dev_notice(css->dev, "can not initialize queue %s\n",
> >  				   qnames[i]);
> > -			return -EINVAL;
> > +			ret = -EINVAL;
> > +			goto out;
> >  		}
> >  	}
> >  	for (i = 0; i < IPU3_CSS_RECTS; i++) { @@ -1788,7 +1795,8 @@ int
> > imgu_css_fmt_try(struct imgu_css *css,
> >  	if (!imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_IN]) ||
> >  	    !imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_OUT])) {
> >  		dev_warn(css->dev, "required queues are disabled\n");
> > -		return -EINVAL;
> > +		ret = -EINVAL;
> > +		goto out;
> >  	}
> > 
> >  	if (!imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_OUT])) { @@ -1829,7
> > +1837,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> >  	ret = imgu_css_find_binary(css, pipe, q, r);
> >  	if (ret < 0) {
> >  		dev_err(css->dev, "failed to find suitable binary\n");
> > -		return -EINVAL;
> > +		ret = -EINVAL;
> > +		goto out;
> >  	}
> >  	css->pipes[pipe].bindex = ret;
> > 
> > @@ -1843,7 +1852,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> >  						IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> >  				dev_err(css->dev,
> >  					"final resolution adjustment failed\n");
> > -				return -EINVAL;
> > +				ret = -EINVAL;
> > +				goto out;
> >  			}
> >  			*fmts[i] = q[i].fmt.mpix;
> >  		}
> > @@ -1859,7 +1869,10 @@ int imgu_css_fmt_try(struct imgu_css *css,
> >  		 bds->width, bds->height, gdc->width, gdc->height,
> >  		 out->width, out->height, vf->width, vf->height);
> > 
> > -	return 0;
> > +	ret = 0;
> > +out:
> > +	kfree(q);
> > +	return ret;
> >  }
> > 
> >  int imgu_css_fmt_set(struct imgu_css *css,

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com