Re: [PATCHv4 10/17] pps: use BUG_ON for kernel API safety checks

[Alexander Gordeev]

Ð Sat, 20 Nov 2010 17:13:51 +0100
Rodolfo Giometti <giometti@xxxxxxxxxxxx> ÐÐÑÐÑ:

> On Thu, Nov 18, 2010 at 07:01:03PM +0300, Alexander Gordeev wrote:
> > This way less overhead is involved when running production kernel.
> > If you want to debug a pps client module please define DEBUG to enable
> > the checks.
> > 
> > Signed-off-by: Alexander Gordeev <lasaine@xxxxxxxxxxxxx>
> > ---
> >  drivers/pps/kapi.c |   33 ++++++++++-----------------------
> >  1 files changed, 10 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > index fe832aa..54261c4 100644
> > --- a/drivers/pps/kapi.c
> > +++ b/drivers/pps/kapi.c
> > @@ -81,25 +81,14 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> >  	int err;
> >  
> >  	/* Sanity checks */
> > -	if ((info->mode & default_params) != default_params) {
> > -		pr_err("pps: %s: unsupported default parameters\n",
> > -					info->name);
> > -		err = -EINVAL;
> > -		goto pps_register_source_exit;
> > -	}
> > -	if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> > -			info->echo == NULL) {
> > -		pr_err("pps: %s: echo function is not defined\n",
> > -					info->name);
> > -		err = -EINVAL;
> > -		goto pps_register_source_exit;
> > -	}
> > -	if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> > -		pr_err("pps: %s: unspecified time format\n",
> > -					info->name);
> > -		err = -EINVAL;
> > -		goto pps_register_source_exit;
> > -	}
> > +
> > +	/* default_params should be supported */
> > +	BUG_ON((info->mode & default_params) != default_params);
> > +	/* echo function should be defined if we are asked to call it */
> > +	BUG_ON((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> > +			info->echo == NULL);
> > +	/* time format should be specified */
> > +	BUG_ON((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0);
> 
> Nack.
> 
> If the userland gives us some wrong parameters this is not the same of
> a kernel bug (which BUG_ON is used for). The userland must be notified
> about the wrong input.

I agree with what you said completely but this is not a user-space API.
pps_register_source() can only be called from other kernel code.

> >  	/* Allocate memory for the new PPS source struct */
> >  	pps = kzalloc(sizeof(struct pps_device), GFP_KERNEL);
> > @@ -179,10 +168,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> >  	int captured = 0;
> >  	struct pps_ktime ts_real;
> >  
> > -	if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> > -		dev_err(pps->dev, "unknown event (%x)\n", event);
> > -		return;
> > -	}
> > +	/* check event type */
> > +	BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> 
> Ack.
> 
> This is a correct usage of BUG_ON. :)
> 
> >  	dev_dbg(pps->dev, "PPS event at %ld.%09ld\n",
> >  			ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
> > -- 
> > 1.7.2.3
> > 
> 
> Ciao,
> 
> Rodolfo
> 


-- 
  Alexander

Attachment: signature.asc
Description: PGP signature