linux-kernel - Re: [PATCH v2 1/1] HID: shield: Initial driver implementation with Thunderstrike support

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:   Fri, 14 Apr 2023 00:09:32 -0700
From:   Rahul Rameshbabu <rrameshbabu@...dia.com>
To:     Jiri Kosina <jikos@...nel.org>
Cc:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] HID: shield: Initial driver implementation with
 Thunderstrike support

Hi Jiri,

Thanks for the feedback.

On Thu, 13 Apr, 2023 17:07:00 +0200 Jiri Kosina <jikos@...nel.org> wrote:
> On Mon, 10 Apr 2023, Rahul Rameshbabu wrote:
>
>> Supports the Thunderstrike (SHIELD 2017) controller. Implements support for
>> the Thunderstrike HOSTCMD firmware interface. Adds sysfs attributes about a
>> SHIELD device and introduces haptics support for controllers.
>> 
>> Signed-off-by: Rahul Rameshbabu <rrameshbabu@...dia.com>
>
> Thanks a lot for submitting the driver. I have one minor question:
>
> [ ... snip ... ]
>> +thunderstrike_update_haptics(struct thunderstrike *ts,
>> +			     struct thunderstrike_hostcmd_haptics *motors)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&ts->haptics_update_lock, flags);
>> +	ts->haptics_val = *motors;
>> +	spin_unlock_irqrestore(&ts->haptics_update_lock, flags);
>
> Do we really have to disable interrupts when taking haptics_update_lock? 
> Is it ever being taken from interrupt context?

The reason why I disable interrupts is because I call
thunderstrike_update_haptics from the play_effect callback I pass to
input_ff_create_memless. From there, it is used in ml_effect_timer
drivers/input/ff-memless.c, which is a timer interrupt context so we
should disable interrupts when taking haptics_update_lock.

  static void ml_effect_timer(struct timer_list *t)
  {
    struct ml_device *ml = from_timer(ml, t, timer);
    struct input_dev *dev = ml->dev;
    unsigned long flags;

    pr_debug("timer: updating effects\n");

    spin_lock_irqsave(&dev->event_lock, flags);
    ml_play_effects(ml);
    spin_unlock_irqrestore(&dev->event_lock, flags);
  }

Let me know if this seems off.

>
> Thanks,

-- Rahul Rameshbabu