Received-SPF: pass (google.com: domain of linux-kernel+bounces-21474-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 9 Jan 2024 14:31:18 -0800
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: James Ogletree <James.Ogletree@cirrus.com>
Cc: James Ogletree <jogletre@opensource.cirrus.com>,
	Fred Treven <Fred.Treven@cirrus.com>,
	Ben Bright <Ben.Bright@cirrus.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Simon Trimmer <simont@opensource.cirrus.com>,
	Charles Keepax <ckeepax@opensource.cirrus.com>,
	Richard Fitzgerald <rf@opensource.cirrus.com>,
	Lee Jones <lee@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
	Takashi Iwai <tiwai@suse.com>,
	James Schulman <James.Schulman@cirrus.com>,
	David Rhodes <David.Rhodes@cirrus.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Peng Fan <peng.fan@nxp.com>, Jeff LaBundy <jeff@labundy.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	Jacky Bai <ping.bai@nxp.com>,
	Weidong Wang <wangweidong.a@awinic.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Herve Codina <herve.codina@bootlin.com>,
	Shuming Fan <shumingf@realtek.com>,
	Shenghao Ding <13916275206@139.com>,
	Ryan Lee <ryans.lee@analog.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	"open list:CIRRUS LOGIC HAPTIC DRIVERS" <patches@opensource.cirrus.com>,
	"open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..." <linux-input@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	"open list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." <linux-sound@vger.kernel.org>,
	"moderated list:CIRRUS LOGIC AUDIO CODEC DRIVERS" <alsa-devel@alsa-project.org>
Subject: Re: [PATCH v5 4/5] Input: cs40l50 - Add support for the CS40L50
 haptic driver
Message-ID: <ZZ3JNuLp-ZfGOQRF@google.com>
References: <20240104223643.876292-1-jogletre@opensource.cirrus.com>
 <20240104223643.876292-5-jogletre@opensource.cirrus.com>
 <ZZoFUwOEF6NByIp2@google.com>
 <564A2601-4933-4BD7-B4E6-C973A585CA61@cirrus.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <564A2601-4933-4BD7-B4E6-C973A585CA61@cirrus.com>

On Tue, Jan 09, 2024 at 10:03:02PM +0000, James Ogletree wrote:
> Hi Dmitry,
> 
> Thank you for your excellent review. Just a few questions.
> 
> > On Jan 6, 2024, at 7:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > On Thu, Jan 04, 2024 at 10:36:37PM +0000, James Ogletree wrote:
> 
> >> +
> >> + info->add_effect.u.periodic.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
> >> + if (!info->add_effect.u.periodic.custom_data)
> >> + return -ENOMEM;
> >> +
> >> + if (copy_from_user(info->add_effect.u.periodic.custom_data,
> >> +   effect->u.periodic.custom_data, sizeof(s16) * len)) {
> >> + info->add_error = -EFAULT;
> >> + goto out_free;
> >> + }
> >> +
> >> + queue_work(info->vibe_wq, &info->add_work);
> >> + flush_work(&info->add_work);
> > 
> > I do not understand the need of scheduling a work here. You are
> > obviously in a sleeping context (otherwise you would not be able to
> > execute flush_work()) so you should be able to upload the effect right
> > here.
> 
> Scheduling work here is to ensure its ordering with “playback" worker
> items, which themselves are called in atomic context and so need
> deferred work. I think this explains why we need a workqueue as well,
> but please correct me.
> 
> > 
> >> +
> >> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
> >> +{
> >> + struct vibra_info *info = input_get_drvdata(dev);
> >> +
> >> + if (val > 0) {
> > 
> > value is supposed to signal how many times an effect should be repeated.
> > It looks like you are not handling this at all.
> 
> For playbacks, we mandate that the input_event value field is set to either 1

I am sorry, who is "we"?

> or 0 to command either a start playback or stop playback respectively.
> Values other than that should be rejected, so in the next version I will fix this
> to explicitly check for 1 or 0.

No, please implement the API properly.

> 
> > 
> >> + info->start_effect = &dev->ff->effects[effect_id];
> >> + queue_work(info->vibe_wq, &info->vibe_start_work);
> > 
> > The API allows playback of several effects at once, the way you have it
> > done here if multiple requests come at same time only one will be
> > handled.
> 
> I think I may need some clarification on this point. Why would concurrent
> start/stop playback commands get dropped? It seems they would all be
> added to the workqueue and executed eventually.

You only have one instance of vibe_start_work, as well as only one
"slot" to hold the effect you want to start. So if you issue 2 request
back to back to play effect 1 and 2 you are likely to end with
info->start_effect == 2 and that is what vibe_start_work handler will
observe, effectively dropping request to play effect 1 on the floor.

> 
> > 
> >> + } else {
> >> + queue_work(info->vibe_wq, &info->vibe_stop_work);
> > 
> > Which effect are you stopping? All of them? You need to stop a
> > particular one.
> 
> Our implementation of “stop” stops all effects in flight which is intended.
> That is probably unusual so I will add a comment here in the next
> version.

Again, please implement the driver properly, not define your own
carveouts for the expected behavior.

Thanks.

-- 
Dmitry