Re: [PATCH v2 09/24] input: Port hid-dr to ff-memless-next

[Michal Malý]

On Wednesday 23 of April 2014 15:41:03 Oliver Neukum wrote:
> On Tue, 2014-04-22 at 15:59 +0200, Michal Malý wrote:
> >  static int drff_play(struct input_dev *dev, void *data,
> > 
> > -                                struct ff_effect *effect)
> > +                       const struct mlnx_effect_command *command)
> > 
> >  {
> >  
> >         struct hid_device *hid = input_get_drvdata(dev);
> >         struct drff_device *drff = data;
> > 
> > +       const struct mlnx_rumble_force *rumble_force =
> > &command->u.rumble_force;
> > 
> >         int strong, weak;
> > 
> > -       strong = effect->u.rumble.strong_magnitude;
> > -       weak = effect->u.rumble.weak_magnitude;
> > +       strong = rumble_force->strong;
> > +       weak = rumble_force->weak;
> > 
> >         dbg_hid("called with 0x%04x 0x%04x", strong, weak);
> > 
> > -       if (strong || weak) {
> > -               strong = strong * 0xff / 0xffff;
> > -               weak = weak * 0xff / 0xffff;
> > -
> > -               /* While reverse engineering this device, I found that
> > when
> > -                  this value is set, it causes the strong rumble to
> > function
> > -                  at a near maximum speed, so we'll bypass it. */
> > -               if (weak == 0x0a)
> > -                       weak = 0x0b;
> > -
> > -               drff->report->field[0]->value[0] = 0x51;
> > -               drff->report->field[0]->value[1] = 0x00;
> > -               drff->report->field[0]->value[2] = weak;
> > -               drff->report->field[0]->value[4] = strong;
> > -               hid_hw_request(hid, drff->report, HID_REQ_SET_REPORT);
> > -
> > -               drff->report->field[0]->value[0] = 0xfa;
> > -               drff->report->field[0]->value[1] = 0xfe;
> > -       } else {
> > +       switch (command->cmd) {
> > +       case MLNX_START_RUMBLE:
> > +               if (strong || weak) {
> > +                       strong = strong * 0xff / 0xffff;
> > +                       weak = weak * 0xff / 0xffff;
> > +
> > +                       /* While reverse engineering this device, I
> > found that when
> > +                       this value is set, it causes the strong rumble
> > to function
> > +                       at a near maximum speed, so we'll bypass it.
> > */
> > +                       if (weak == 0x0a)
> > +                               weak = 0x0b;
> > +
> > +                       drff->report->field[0]->value[0] = 0x51;
> > +                       drff->report->field[0]->value[1] = 0x00;
> > +                       drff->report->field[0]->value[2] = weak;
> > +                       drff->report->field[0]->value[4] = strong;
> 
> This looks like an endianness bug.

I don't have a big endian machine to check but why would this be an endianness 
issue? We're dealing with values all the time here, not addresses so I'd 
expect the 'weak' and 'strong' values to be truncated if they won't fit into 
byte. Division done beforehand makes sure that the values are within <0; 255> 
range. As far as I can see this is quite common in the HID and Input code. Am 
I missing something here?

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