linux-kernel - Re: [PATCH v2] Staging: vc04_services: Cleanup in ctrl_set

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 23 Apr 2019 08:36:05 -0500
From:   Madhumitha Prabakaran <madhumithabiw@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     eric@...olt.net, stefan.wahren@...e.com, f.fainelli@...il.com,
        rjui@...adcom.com, sbranden@...adcom.com,
        bcm-kernel-feedback-list@...adcom.com,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Staging: vc04_services: Cleanup in ctrl_set_bitrate()

On 04/20  :51, Greg KH wrote:
> On Sat, Apr 20, 2019 at 11:56:44AM -0500, Madhumitha Prabakaran wrote:
> > Remove unnecessary variable and replace return type.
> > In addition to that align the function parameters with parentheses to
> > maintain Linux coding style.
> > 
> > Issue suggested by Coccinelle.
> > 
> > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@...il.com>
> > 
> > ---
> > Changes in v2
> > 
> > 1) Fix my own error of removing function call from the variable.
> > 2) Align the function parameters with parentheses.
> > 
> > ---
> > ---
> >  .../staging/vc04_services/bcm2835-camera/controls.c    | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/vc04_services/bcm2835-camera/controls.c b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > index e39ab5fae724..71eb349c3138 100644
> > --- a/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > +++ b/drivers/staging/vc04_services/bcm2835-camera/controls.c
> > @@ -607,18 +607,16 @@ static int ctrl_set_bitrate(struct bm2835_mmal_dev *dev,
> >  			    struct v4l2_ctrl *ctrl,
> >  			    const struct bm2835_mmal_v4l2_ctrl *mmal_ctrl)
> >  {
> > -	int ret;
> >  	struct vchiq_mmal_port *encoder_out;
> >  
> >  	dev->capture.encode_bitrate = ctrl->val;
> >  
> >  	encoder_out = &dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->output[0];
> >  
> > -	ret = vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
> > -					    mmal_ctrl->mmal_id,
> > -					    &ctrl->val, sizeof(ctrl->val));
> > -	ret = 0;
> > -	return ret;
> > +	vchiq_mmal_port_parameter_set(dev->instance, encoder_out,
> > +				      mmal_ctrl->mmal_id, &ctrl->val,
> > +				      sizeof(ctrl->val));
> > +	return 0;
> >  }
> 
> Why does this function even return anything at all if it can not fail?

I looked at the function vchiq_mmal_port_parameter_set, which calls
another function for setting the port parameters, which inturns sends an
synchronous message. If it is able to send the message successfully, it
returns 0. It can also fail and can return any other value. Also the
function ctrl_set_bitrate is not evaluated anywhere for true or false,
so it is safe to return the value we get from
vchiq_mmal_port_parameter_set instead of returning 0 always.


> 
> Why not return the value that vchiq_mmal_port_parameter_set() returned?
> 

I will change the code to return the value from
vchiq_mmal_port_parameter_set directly.

> thanks,
> 
> greg k-h