Hi Dmitry,
> On Apr 16, 2024, at 6:28 PM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
>
> On Mon, Apr 08, 2024 at 03:32:13PM +0000, James Ogletree wrote:
>> Introduce support for Cirrus Logic Device CS40L50: a
>> haptic driver with waveform memory, integrated DSP,
>> and closed-loop algorithms.
>>
>> The input driver provides the interface for control of
>> haptic effects through the device.
>>
>> Signed-off-by: James Ogletree <jogletre@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> v10:
>> Minor concern that playback stop is still misused with respect to not
>> specifying an effect ID. The device can only play one effect at a
>> time, but setting max effects for the EVIOCGEFFECTS ioctl to 1 would
>> restrict the number of uploads to 1 as well.
>
> Sorry for a long delay on my part.
Not to worry at all. I am very grateful for the thorough review.
>
> Unfortunately this is not a minor concern, because it breaks the API
> that we so far been presenting to userspace. EVIOCGEFFECTS ioctl which returns
> input->ff->max_effects is described as "Report number of effects
> playable at the same time".
>
> I suggest you limit number of effects to 1 so that we can merge the
> driver with such limitation, and then try to figure out how to expand
> the API. We will probably have to split the notion of number of playable
> effects vs number of uploadable effects, and only accept uploads of
> effects that exceed number of playable effects if they all have the same
> owner, so that different processes would not be able to affect each
> other in case when number of simultaneously playable effects is smaller.
I understand. I will do just that for this submission.
>> +/* Describes an area in DSP memory populated by effects */
>> +struct cs40l50_bank {
>> + enum cs40l50_bank_type type;
>> + u32 base_index;
>> + u32 max_index;
>
> This looks like it is written to the device, so I think this needs
> proper endianness annotation. Is there any concern about padding between
> the type and base_index?
The structure as a whole is not written, so there is no concern about padding.
Only base_index is written, and the endianness conversion is handled by regmap.
“cs40l50_owt_header” would be an example of a struct which is written, and there
care is taken for padding, and there also regmap handles the endianness
conversion.
>> +static int cs40l50_upload_owt(struct cs40l50_work *work_data)
>> +{
>> + u32 len = 2 * work_data->custom_len, wt_offset, wt_size;
>> + struct cs40l50_vibra *vibra = work_data->vibra;
>> + struct cs40l50_owt_header header;
>> + u8 *out_data;
>> + int error;
>> +
>> + error = regmap_read(vibra->regmap, vibra->dsp.owt_size_reg, &wt_size);
>> + if (error)
>> + return error;
>> +
>> + if ((wt_size * sizeof(u32)) < sizeof(header) + len) {
>> + dev_err(vibra->dev, "No space in OWT bank for effect\n");
>> + return -ENOSPC;
>> + }
>> +
>> + out_data = kzalloc(sizeof(header) + len, GFP_KERNEL);
>
> You can make this as
>
> u8 *out_data __free(kfree) = kzalloc(...);
>
> and then you do not need to explicitly kfree() it.
>
> Also, I wonder if you have to zero it and can't use kmalloc() given that
> you copy over the entire thing.
Ack. I will declare it as:
u8 *out_data __free(kfree) = NULL;
And then initialize it in the same place with kmalloc().
This will allow me to keep reverse Christmas tree style.
>> +static int cs40l50_add(struct input_dev *dev, struct ff_effect *effect,
>> + struct ff_effect *old)
>> +{
>> + struct ff_periodic_effect *periodic = &effect->u.periodic;
>> + struct cs40l50_vibra *vibra = input_get_drvdata(dev);
>> + u32 len = effect->u.periodic.custom_len;
>> + struct cs40l50_work work_data;
>> +
>> + if (effect->type != FF_PERIODIC || periodic->waveform != FF_CUSTOM) {
>> + dev_err(vibra->dev, "Type (%#X) or waveform (%#X) unsupported\n",
>> + effect->type, periodic->waveform);
>> + return -EINVAL;
>> + }
>> +
>> + work_data.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
>> + if (!work_data.custom_data)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(work_data.custom_data, effect->u.periodic.custom_data,
>> + sizeof(s16) * len)) {
>> + work_data.error = -EFAULT;
>> + goto err_free;
>> + }
>
> work_data.custom_data = memdup_array_user(effect->u.periodic.custom_data,
> len, sizeof(s16));
> if (IS_ERR(work_data.custom_data))
> return PTR_ERR(work_data.custom_data);
Ack.
>> +static void cs40l50_start_worker(struct work_struct *work)
>> +{
>> + struct cs40l50_work *work_data = container_of(work, struct cs40l50_work, work);
>> + struct cs40l50_vibra *vibra = work_data->vibra;
>> + struct cs40l50_effect *start_effect;
>> +
>> + if (pm_runtime_resume_and_get(vibra->dev) < 0)
>> + goto err_free;
>> +
>> + mutex_lock(&vibra->lock);
>> +
>> + start_effect = cs40l50_find_effect(work_data->effect->id, &vibra->effect_head);
>> + if (start_effect) {
>> + while (--work_data->count >= 0) {
>> + vibra->dsp.write(vibra->dev, vibra->regmap, start_effect->index);
>> + usleep_range(work_data->effect->replay.length,
>> + work_data->effect->replay.length + 100);
>
> If (I am reading this right you are spinning here playing the effect. It
> would be much better if you tracked outstanding number of replays for an
> effect and had a work re-scheduled that would play an instance.
> Similarly to what code in ff-memless.c is doing.
I think you are reading it right.
My concern with the ff-memless.c method is that it seems to risk out of order
executions when adopted to our driver, because of the use of the workqueue.
If a playback is issued with a high enough repeat value, it seems very plausible
that another operation, e.g. erase, could arrive in the middle of the chain of
re-scheduling and disrupt the playbacks which have yet to be queued. Currently,
the driver handles all repeats in the same work item, so it is guaranteed to repeat
the effect for the specified number of times without being interrupted.
Please let me know what you think.
>> +static int cs40l50_erase(struct input_dev *dev, int effect_id)
>> +{
>> + struct cs40l50_vibra *vibra = input_get_drvdata(dev);
>> + struct cs40l50_work work_data;
>> +
>> + work_data.vibra = vibra;
>> + work_data.effect = &dev->ff->effects[effect_id];
>> +
>> + INIT_WORK(&work_data.work, cs40l50_erase_worker);
>> +
>> + /* Push to workqueue to serialize with playbacks */
>> + queue_work(vibra->vibe_wq, &work_data.work);
>> + flush_work(&work_data.work);
>
> You already take the lock when you play, upload or erase an effect. Why
> do we need additional single-thread workqueue for this? Why do we need
> additional ordering and synchronization?
I think there is unnecessary serialization here, but I think it is the mutexes
which are excessive.
Without the workqueue I think there will be out of order operations. The only
reason the workqueue is necessary is because playback blocks (via
pm_runtime_resume_and_get), and so playback must defer work.
Now upload and erase are not called in atomic context. But without queueing
those executions, they could cut in front of any playbacks waiting in the
workqueue. This is the meaning of the comment "Push to workqueue to
serialize with playbacks”.
But if they are already serialized in this way, then I don’t see the need
for the locks. That’s my thinking, let me know if it is sound.
>> + return input_register_device(vibra->input);
>
> Please no this kind of short hands when there are multiple exists from a
> function.
>
> error = input_register_device(vibra->input);
> if (error)
> return error;
>
> return 0;
>
> This way it is much easier to move the code around when needed.
Ack.
Best,
James
