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

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

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

On 29/03/14 16:11, 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.
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/rc-main.c |   66 +++++++++++++++++++++++++++++---------------
>  include/media/rc-core.h    |    2 +
>  2 files changed, 45 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index ba955ac..8675e07 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -634,7 +634,6 @@ EXPORT_SYMBOL_GPL(rc_repeat);
>  static void ir_do_keydown(struct rc_dev *dev, enum rc_type protocol,
>  			  u32 scancode, u32 keycode, u8 toggle)
>  {
> -	struct rc_scancode_filter *filter;
>  	bool new_event = (!dev->keypressed		 ||
>  			  dev->last_protocol != protocol ||
>  			  dev->last_scancode != scancode ||
> @@ -643,11 +642,6 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_type protocol,
>  	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) {
> @@ -1017,14 +1011,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);
> @@ -1033,8 +1024,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 */
> @@ -1078,7 +1068,9 @@ static ssize_t show_filter(struct device *device,
>  		return -EINVAL;
>  
>  	mutex_lock(&dev->lock);
> -	if (fattr->mask)
> +	if (!dev->s_filter)
> +		val = 0;

I suspect this should take s_wakeup_filter into account depending on
fattr->type. It's probably quite common to have a wakeup filter but no
normal filter.

The rest looks reasonable, though it could easily have been a separate
patch (at least as long as the show/store callbacks don't assume the
presence of the callbacks they use).

Cheers
James

> +	else if (fattr->mask)
>  		val = dev->scancode_filters[fattr->type].mask;
>  	else
>  		val = dev->scancode_filters[fattr->type].data;
> @@ -1202,27 +1194,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 struct attribute *rc_dev_wakeup_filter_attrs[] = {
> +	&dev_attr_wakeup_filter.attr.attr,
> +	&dev_attr_wakeup_filter_mask.attr.attr,
> +	NULL,
>  };
>  
> -static const struct attribute_group *rc_dev_attr_groups[] = {
> -	&rc_dev_attr_grp,
> -	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,
>  };
> @@ -1279,7 +1289,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;
> @@ -1307,6 +1317,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 8c31e4a..2e97b98 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)
> @@ -118,6 +119,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;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]