linux-kernel - Re: [PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f308d753cb68dc6afbfe4360c1bd4404@hardeman.nu>
Date:	Tue, 28 Oct 2014 09:42:30 +0100
From:	David Härdeman <david@...deman.nu>
To:	Tomas Melin <tomas.melin@....fi>
Cc:	LKML <linux-kernel@...r.kernel.org>, linux-media@...r.kernel.org,
	james.hogan@...tec.com,
	Antti Seppälä <a.seppala@...il.com>,
	Александр Берсенев <bay@...kerdom.ru>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	tomas.j.melin@...il.com
Subject: Re: [PATCH v2 1/2] [media] rc-core: fix protocol_change regression in ir_raw_event_register

On 2014-10-26 20:33, Tomas Melin wrote:
> On Sat, Oct 25, 2014 at 12:03 PM, David Härdeman <david@...deman.nu> 
> wrote:
>> Wouldn't something like this be a simpler way of achieving the same
>> result? (untested):
> 
> The idea was to remove the empty change_protocol function that had
> been added in the breaking commit.

The empty function was added for a reason? The presence of a 
change_protocol function implies that the receiver supports protocol 
changing (whether it's via the raw IR decoding or in hardware).

> IMHO, it would be better to not have functions that don't do anything.
> Actually, another problem with that empty function is that if the
> driver first sets up a "real" change_protocol function and related
> data, and then calls rc_register_device, the driver defined
> change_protocol function would be overwritten.

change_protocol is only set if it's a driver that does in-kernel 
decoding...meaning that it generates pulse/space timings...meaning that 
hardware protocol changes aren't necessary?

>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index a7991c7..d521f20 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -1421,6 +1421,9 @@ int rc_register_device(struct rc_dev *dev)
>> 
>>         if (dev->change_protocol) {
>>                 u64 rc_type = (1 << rc_map->rc_type);
>> +               if (dev->driver_type == RC_DRIVER_IR_RAW)
>> +                       rc_type |= RC_BIT_LIRC;
>> +
>>                 rc = dev->change_protocol(dev, &rc_type);
>>                 if (rc < 0)
>>                         goto out_raw;
> 
> But otherwise yes, your suggestion could work, with the addition that
> we still need to update enabled_protocols (and not init
> enabled_protocols anymore in ir_raw_event_register() ).

First, enabled_protocols is already taken care of in the above patch 
(the line after "goto out_raw" is "dev->enabled_protocols = rc_type;")?

Second, initializing enabled_protocols to some default in 
ir_raw_event_register() might not be strictly necessary but it also 
doesn't hurt since that happens before dev->change_protocol() is called 
in rc_register_device()?

> +               dev->enabled_protocols = (rc_type | RC_BIT_LIRC);
> 
> Please let me know your preferences on which you prefer, and, if
> needed, I'll make a new patch version.

I'd prefer the above, minimal, approach. But it's Mauro who decides in 
the end.

Regards,
David

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/