Re: [PATCH 3/3] rc-core: remove generic scancode filter

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 7835 bytes --]

On Saturday 05 April 2014 00:06:06 David Härdeman wrote:
> The generic scancode filtering has questionable value and makes it
> impossible to determine from userspace if there is an actual
> scancode hw filter present or not.
> 
> So revert the generic parts.
> 
> Based on a patch from James Hogan <james.hogan@imgtec.com>, but this
> version also makes sure that only the valid sysfs files are created
> in the first place.
> 
> v2: correct dev->s_filter check
> 
> v3: move some parts over from the previous patch
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>

Acked-by: James Hogan <james.hogan@imgtec.com>

Thanks
James

> ---
>  drivers/media/rc/rc-main.c |   88
> +++++++++++++++++++++++++++----------------- include/media/rc-core.h    |  
>  2 +
>  2 files changed, 55 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index ecbc20c..970b93d 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -633,19 +633,13 @@ EXPORT_SYMBOL_GPL(rc_repeat);
>  static void ir_do_keydown(struct rc_dev *dev, int scancode,
>  			  u32 keycode, u8 toggle)
>  {
> -	struct rc_scancode_filter *filter;
> -	bool new_event = !dev->keypressed ||
> -			 dev->last_scancode != scancode ||
> -			 dev->last_toggle != toggle;
> +	bool new_event = (!dev->keypressed		 ||
> +			  dev->last_scancode != scancode ||
> +			  dev->last_toggle != toggle);
> 
>  	if (new_event && dev->keypressed)
>  		ir_do_keyup(dev, false);
> 
> -	/* Generic scancode filtering */
> -	filter = &dev->scancode_filters[RC_FILTER_NORMAL];
> -	if (filter->mask && ((scancode ^ filter->data) & filter->mask))
> -		return;
> -
>  	input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode);
> 
>  	if (new_event && keycode != KEY_RESERVED) {
> @@ -1011,14 +1005,11 @@ static ssize_t store_protocols(struct device
> *device, set_filter = (fattr->type == RC_FILTER_NORMAL)
>  		? dev->s_filter : dev->s_wakeup_filter;
> 
> -	if (old_type != type && filter->mask) {
> +	if (set_filter && old_type != type && filter->mask) {
>  		local_filter = *filter;
>  		if (!type) {
>  			/* no protocol => clear filter */
>  			ret = -1;
> -		} else if (!set_filter) {
> -			/* generic filtering => accept any filter */
> -			ret = 0;
>  		} else {
>  			/* hardware filtering => try setting, otherwise clear */
>  			ret = set_filter(dev, &local_filter);
> @@ -1027,8 +1018,7 @@ static ssize_t store_protocols(struct device *device,
>  			/* clear the filter */
>  			local_filter.data = 0;
>  			local_filter.mask = 0;
> -			if (set_filter)
> -				set_filter(dev, &local_filter);
> +			set_filter(dev, &local_filter);
>  		}
> 
>  		/* commit the new filter */
> @@ -1072,7 +1062,10 @@ static ssize_t show_filter(struct device *device,
>  		return -EINVAL;
> 
>  	mutex_lock(&dev->lock);
> -	if (fattr->mask)
> +	if ((fattr->type == RC_FILTER_NORMAL && !dev->s_filter) ||
> +	    (fattr->type == RC_FILTER_WAKEUP && !dev->s_wakeup_filter))
> +		val = 0;
> +	else if (fattr->mask)
>  		val = dev->scancode_filters[fattr->type].mask;
>  	else
>  		val = dev->scancode_filters[fattr->type].data;
> @@ -1120,12 +1113,11 @@ static ssize_t store_filter(struct device *device,
>  	if (ret < 0)
>  		return ret;
> 
> +	/* Can the scancode filter be set? */
>  	set_filter = (fattr->type == RC_FILTER_NORMAL) ? dev->s_filter :
>  							 dev->s_wakeup_filter;
> -
> -	/* Scancode filter not supported (but still accept 0) */
> -	if (!set_filter && fattr->type == RC_FILTER_WAKEUP)
> -		return val ? -EINVAL : count;
> +	if (!set_filter)
> +		return -EINVAL;
> 
>  	mutex_lock(&dev->lock);
> 
> @@ -1143,11 +1135,9 @@ static ssize_t store_filter(struct device *device,
>  		goto unlock;
>  	}
> 
> -	if (set_filter) {
> -		ret = set_filter(dev, &local_filter);
> -		if (ret < 0)
> -			goto unlock;
> -	}
> +	ret = set_filter(dev, &local_filter);
> +	if (ret < 0)
> +		goto unlock;
> 
>  	/* Success, commit the new filter */
>  	*filter = local_filter;
> @@ -1199,27 +1189,45 @@ static RC_FILTER_ATTR(wakeup_filter,
> S_IRUGO|S_IWUSR, static RC_FILTER_ATTR(wakeup_filter_mask, S_IRUGO|S_IWUSR,
>  		      show_filter, store_filter, RC_FILTER_WAKEUP, true);
> 
> -static struct attribute *rc_dev_attrs[] = {
> +static struct attribute *rc_dev_protocol_attrs[] = {
>  	&dev_attr_protocols.attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group rc_dev_protocol_attr_grp = {
> +	.attrs	= rc_dev_protocol_attrs,
> +};
> +
> +static struct attribute *rc_dev_wakeup_protocol_attrs[] = {
>  	&dev_attr_wakeup_protocols.attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group rc_dev_wakeup_protocol_attr_grp = {
> +	.attrs	= rc_dev_wakeup_protocol_attrs,
> +};
> +
> +static struct attribute *rc_dev_filter_attrs[] = {
>  	&dev_attr_filter.attr.attr,
>  	&dev_attr_filter_mask.attr.attr,
> -	&dev_attr_wakeup_filter.attr.attr,
> -	&dev_attr_wakeup_filter_mask.attr.attr,
>  	NULL,
>  };
> 
> -static struct attribute_group rc_dev_attr_grp = {
> -	.attrs	= rc_dev_attrs,
> +static struct attribute_group rc_dev_filter_attr_grp = {
> +	.attrs	= rc_dev_filter_attrs,
>  };
> 
> -static const struct attribute_group *rc_dev_attr_groups[] = {
> -	&rc_dev_attr_grp,
> -	NULL
> +static struct attribute *rc_dev_wakeup_filter_attrs[] = {
> +	&dev_attr_wakeup_filter.attr.attr,
> +	&dev_attr_wakeup_filter_mask.attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group rc_dev_wakeup_filter_attr_grp = {
> +	.attrs	= rc_dev_wakeup_filter_attrs,
>  };
> 
>  static struct device_type rc_dev_type = {
> -	.groups		= rc_dev_attr_groups,
>  	.release	= rc_dev_release,
>  	.uevent		= rc_dev_uevent,
>  };
> @@ -1276,7 +1284,7 @@ int rc_register_device(struct rc_dev *dev)
>  	static bool raw_init = false; /* raw decoders loaded? */
>  	struct rc_map *rc_map;
>  	const char *path;
> -	int rc, devno;
> +	int rc, devno, attr = 0;
> 
>  	if (!dev || !dev->map_name)
>  		return -EINVAL;
> @@ -1304,6 +1312,16 @@ int rc_register_device(struct rc_dev *dev)
>  			return -ENOMEM;
>  	} while (test_and_set_bit(devno, ir_core_dev_number));
> 
> +	dev->dev.groups = dev->sysfs_groups;
> +	dev->sysfs_groups[attr++] = &rc_dev_protocol_attr_grp;
> +	if (dev->s_filter)
> +		dev->sysfs_groups[attr++] = &rc_dev_filter_attr_grp;
> +	if (dev->s_wakeup_filter)
> +		dev->sysfs_groups[attr++] = &rc_dev_wakeup_filter_attr_grp;
> +	if (dev->change_wakeup_protocol)
> +		dev->sysfs_groups[attr++] = &rc_dev_wakeup_protocol_attr_grp;
> +	dev->sysfs_groups[attr++] = NULL;
> +
>  	/*
>  	 * Take the lock here, as the device sysfs node will appear
>  	 * when device_add() is called, which may trigger an ir-keytable udev
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index 6dbc7c1..fde142e 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -60,6 +60,7 @@ enum rc_filter_type {
>  /**
>   * struct rc_dev - represents a remote control device
>   * @dev: driver model's view of this device
> + * @sysfs_groups: sysfs attribute groups
>   * @input_name: name of the input child device
>   * @input_phys: physical path to the input child device
>   * @input_id: id of the input child device (struct input_id)
> @@ -117,6 +118,7 @@ enum rc_filter_type {
>   */
>  struct rc_dev {
>  	struct device			dev;
> +	const struct attribute_group	*sysfs_groups[5];
>  	const char			*input_name;
>  	const char			*input_phys;
>  	struct input_id			input_id;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]