linux-kernel - Re: [RESEND] Input: support pre-stored effects

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: <5CD75646-02A6-4175-9176-A8DCD059085F@cirrus.com>
Date:   Thu, 15 Jun 2023 18:12:20 +0000
From:   James Ogletree <James.Ogletree@...rus.com>
To:     Jeff LaBundy <jeff@...undy.com>
CC:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND] Input: support pre-stored effects



> On Jun 12, 2023, at 8:25 PM, Jeff LaBundy <jeff@...undy.com> wrote:
> 
> Hi James,
> 
> On Mon, Jun 12, 2023 at 07:43:57PM +0000, James Ogletree wrote:
>> At present, the best way to define effects that
>> pre-exist in device memory is by utilizing
>> the custom_data field, which it was not intended
>> for, and requires arbitrary interpretation by
>> the driver to make meaningful.
>> 
>> Provide option for defining pre-stored effects in
>> device memory.
>> 
>> Signed-off-by: James Ogletree <james.ogletree@...rus.com>
>> ---
>> include/uapi/linux/input.h | 32 ++++++++++++++++++++++----------
>> 1 file changed, 22 insertions(+), 10 deletions(-)
>> 
>> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
>> index 2557eb7b0561..689e5fa10647 100644
>> --- a/include/uapi/linux/input.h
>> +++ b/include/uapi/linux/input.h
>> @@ -428,17 +428,27 @@ struct ff_rumble_effect {
>> __u16 weak_magnitude;
>> };
>> 
>> +/**
>> + * struct ff_prestored_effect - defines parameters of a pre-stored force-feedback effect
>> + * @index: index of effect
>> + * @bank: memory bank of effect
>> + */
>> +struct ff_prestored_effect {
>> + __u16 index;
>> + __u16 bank;
>> +};
> 
> This seems like a good start; I do wonder if it's useful to review recent
> customer vibrator HAL implementations and decide whether you want to pack
> any additional members here such as magnitude, etc. as is done for periodic
> effects?
> 
> Back in L25 days, some customers would assign click or tap effects to one
> or more entries in the wavetable and then use a separate digital volume
> control (at that time exposed through sysfs) to create a few discrete
> amplitude levels. Perhaps it would be handy to bundle this information as
> part of the same call?
> 
> It's just a suggestion; I'll defer to your much more recent expertise.
> 

My thinking is that ff_prestored_effect ought to be for effects being used
“off-the-shelf”, and in such cases it would seem appropriate to defer to
firmware for the effect design. I think this fits nicely as-is with the other
structures as it serves a clear and distinct use-case. Otherwise one might
just add these two members to ff_periodic_effect (or every kind of effect).

I think the current predominant method for setting "magnitude" for these
pre-stored effects is by using the FF_GAIN event code as a separate write
call, so I think adding a magnitude member would go unused, if I understand
you correctly.

Thanks,
James