Hi Eduardo, Douglas pointed out to me that I hadn't reviewed this series yet. That was mostly because it's pretty good as far as I'm concerned :-) I do think one small thing should change: On Monday 27 July 2009 17:12:08 Eduardo Valentin wrote: > diff --git a/linux/drivers/media/radio/si4713-i2c.c b/linux/drivers/media/radio/si4713-i2c.c <snip> > +/* write string property */ > +static int si4713_write_econtrol_string(struct si4713_device *sdev, > + struct v4l2_ext_control *control) > +{ > + struct v4l2_queryctrl vqc; > + int len; > + s32 rval = 0; > + > + vqc.id = control->id; > + rval = si4713_queryctrl(&sdev->sd, &vqc); > + if (rval < 0) > + goto exit; > + > + switch (control->id) { > + case V4L2_CID_RDS_TX_PS_NAME: { > + char ps_name[MAX_RDS_PS_NAME + 1]; > + > + len = control->size - 1; > + if (len > MAX_RDS_PS_NAME) > + len = MAX_RDS_PS_NAME; > + rval = copy_from_user(ps_name, control->string, len); > + if (rval < 0) > + goto exit; > + ps_name[len] = '\0'; > + > + if (strlen(ps_name) % vqc.step) { > + rval = -EINVAL; I think we should return -ERANGE instead. It makes more sense than -EINVAL, since it is the string length that is out of bounds. -ERANGE is the expected error code when the control boundary checks fail. I know I said EINVAL before, but after thinking about it some more I've reconsidered. > + goto exit; > + } > + > + rval = si4713_set_rds_ps_name(sdev, ps_name); > + } > + break; > + > + case V4L2_CID_RDS_TX_RADIO_TEXT:{ > + char radio_text[MAX_RDS_RADIO_TEXT + 1]; > + > + len = control->size - 1; > + if (len > MAX_RDS_RADIO_TEXT) > + len = MAX_RDS_RADIO_TEXT; > + rval = copy_from_user(radio_text, control->string, len); > + if (rval < 0) > + goto exit; > + radio_text[len] = '\0'; > + > + if (strlen(radio_text) % vqc.step) { > + rval = -EINVAL; Ditto. > + goto exit; > + } > + > + rval = si4713_set_rds_radio_text(sdev, radio_text); > + } > + break; > + > + default: > + rval = -EINVAL; > + break; > + }; > + > +exit: > + return rval; > +} Just change this and you can add Reviewed-by: Hans Verkuil <hverkuil@xxxxxxxxx> for the whole series. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html