linux-kernel - Re: [RFC PATCH 02/06] input/rmi4: Core files

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]
Date:	Tue, 23 Oct 2012 16:46:28 -0700
From:	Christopher Heiny <cheiny@...aptics.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:	Linus Walleij <linus.walleij@...aro.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Jean Delvare <khali@...ux-fr.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Input <linux-input@...r.kernel.org>,
	Allie Xiong <axiong@...aptics.com>,
	Vivian Ly <vly@...aptics.com>,
	Daniel Rosenberg <daniel.rosenberg@...aptics.com>,
	Joerie de Gram <j.de.gram@...il.com>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Naveen Kumar Gaddipati <naveen.gaddipati@...ricsson.com>,
	Alexandra Chin <alexandra.chin@...synaptics.com>
Subject: Re: [RFC PATCH 02/06] input/rmi4: Core files

On 10/11/2012 01:13 AM, Dmitry Torokhov wrote:
> On Thu, Oct 11, 2012 at 04:15:56AM +0000, Christopher Heiny wrote:
>> On Thursday, October 11, 2012 02:21:53 AM you wrote:
>>> On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny <cheiny@...aptics.com> wrote:
>>
>>>
>>>> +
>>>> +/** This is here because all those casts made for some ugly code.
>>>> + */
>>>> +static void u8_and(u8 *dest, u8 *target1, u8 *target2, int nbits)
>>>> +{
>>>> +       bitmap_and((long unsigned int *) dest,
>>>> +                  (long unsigned int *) target1,
>>>> +                  (long unsigned int *) target2,
>>>> +                  nbits);
>>>> +}
>>>
>>> Hm, getting rid of unreadable casts is a valid case.
>>>
>>> I'll be OK with this but maybe the real solution is to introduce such
>>> helpers into <linux/bitmap.h>?
>>
>> Hmmm.  We'll give that some thought.  Thought I'd like to get the RMI4
>> driver nailed down, just to keep the area of change small.  Once we've
>> got all the kinks worked out here, we'll look at bitmap.h helpers.
>
> The question is why you are using u8 for bitmaps instead of doing
> DECALRE_BITMAP() and using it instead? Then you would not need silly
> wrappers around existing APIs.

OK, we'll look into that.  My big concern is whether the bit-order in 
bitmask.h will be the same as the bit order in the RMI4 sensor 
registers.  If that works out OK, we'll switch.


>>> (...)
>>>
>>>> +static int process_interrupt_requests(struct rmi_device *rmi_dev)
>>>> +{
>>>> +       struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
>>>> +       struct device *dev = &rmi_dev->dev;
>>>> +       struct rmi_function_container *entry;
>>>> +       u8 irq_status[data->num_of_irq_regs];
>>>
>>> Looking at this...
>>>
>>> What does the data->num_of_irq_regs actually contain?
>>>
>>> I just fear that it is something constant like always 2 or always 4,
>>> so there is actually, in reality, a 16 or 32 bit register hiding in there.
>>>
>>> In that case what you should do is to represent it as a u16 or u32 here,
>>> just or the bits into a status word, and then walk over that status
>>> word with something like ffs(bitword); ...
>>
>> Nope, it's not constant.  In theory, and RMI4 based sensor can have up
>> to 128 functions (in practice, it's far fewer), and each function can
>> have as many as 7 interrupts.  So the number of IRQ registers can vary
>> from RMI4 sensor to RMI4 sensor, and needs to be computed during the
>> scan of the product descriptor table.
>
> Is it a good idea to have it on stack then? Should it be part of
> rmi_device instead?

It's not coming off the stack.  We're allocating it via devm_kzalloc() 
in rmi_driver_probe().


>>>> +#define simple_show_union_struct(regtype, propname, fmt)\
>>>> +static ssize_t tricat(rmi_fn_, FNUM, _##propname##_show)(struct device
>>>> *dev,\ +                               struct device_attribute *attr,
>>>> char *buf) {\ +       struct rmi_function_container *fc;\
>>>> +       struct FUNCTION_DATA *data;\
>>>> +\
>>>> +       fc = to_rmi_function_container(dev);\
>>>> +       data = fc->data;\
>>>> +\
>>>> +       return snprintf(buf, PAGE_SIZE, fmt,\
>>>> +                       data->regtype.propname);\
>>>> +}
>>>
>>> OK I see the point, but is there really no other way to do this than
>>> to #define huge static inlines like these? Is it really not possible to
>>> create just generic functions instead of going this far?
>>>
>>> (same comment for all)
>>
>
>> We tried generic functions previously, and it wound up really really ugly.  We'd be willing to look at it again, though, since this isn't real beautiful either.  If you've got an example implementation in mind. a pointer would help a great deal.
>
> You just need to wrap around a custome structure around struct
> device_attribute and then you shoudl be able to use generics. If you
> look into trackpoint.c you should gett the idea.

That looks a lot tidier.  We'll work on it for sysfs (and maybe for 
debugfs, too).  It might not be applicable in all cases, but it promises 
to make a lot of things tidier.

Thanks!
--
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/