Messages in this thread | | | From | "Henrik Rydberg" <> | Date | Wed, 19 Sep 2012 20:32:31 +0200 | Subject | Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons |
| |
Hi Kevin,
Thanks for looking to this.
> Hmm. I hadn't noticed that the other drivers are returning a static > structure. In that case, it seems that report_fixup itself is broken > from a memory perspective, in that it returns pointers to > inconsistent storage types depending on the driver.
The driver can either modify the existing buffer, of return a pointer to a buffer managed by the driver. The former requires a kmemdup before, the latter a kmemdup after.
> 1. Ugly workaround: make a temporary copy of the dev_rdesc, give it > to report_fixup, make a copy of the return, store that copy in > rdesc, free the temporary copy. Though ugly, this would at least > involve the smallest diff.
Yes, it is correct and ugly, in no particular order.
> 2. Standardize the behavior of the drivers' report_fixup > implementations. Given that some of them need to change the size of > the descriptor, modifying the passed structure is not an option. > Probably all of them should return a newly allocated structure, > either a modified copy of the input or a copy of their static, that > can then be stored directly in rdesc. Especially since report_fixup > is only ever called right before a copy is going to be taken anyway.
Relying on the returned pointer to be properly alloc'd is not a good idea, in particular since it changes semantics rather drastically.
Since the current function performs two different things, perhaps there should be two different functions instead.
> (Adding constness to a parameter isn't considered a severe ABI > break, is it?)
Inside the kernel there is no ABI. Go wild.
Thanks, Henrik
| |