On Mon, Jan 23, 2017 at 02:32:48PM +0000, Bryan O'Donoghue wrote:
> On 23/01/17 12:04, Johan Hovold wrote:
>
> > +static void gb_operation_timeout(unsigned long arg)
> > +{
> > + struct gb_operation *operation = (void *)arg;
> > +
> > + if (gb_operation_result_set(operation, -ETIMEDOUT)) {
> > + /*
> > + * A stuck request message will be cancelled from the
> > + * workqueue.
> > + */
> > + queue_work(gb_operation_completion_wq, &operation->work);
> > + }
> > +}
> > +
>
> If queue_work fails, you will never wake up the waiter.
How could queue_work fail here? The operation result is always set
exactly once before being queued so this is fine.
> > -
> > - ret = wait_for_completion_interruptible_timeout(&operation->completion,
> > - timeout_jiffies);
> > + ret = wait_for_completion_interruptible(&operation->completion);
>
> Note, that's a case I explicitly handle (failure to queue the work) in
> the async set I sent out.
Yep, I noticed that, but that was for queueing your timeout work at
submission. And you queued unconditionally, so you could potentially
fail to queue if an operation was submitted twice, even if that would in
itself be a driver bug.
> On gbsim at any rate, with a sufficient number of outstanding operations
> in-flight it's possible to cause queue_work() to fail. Since you're in a
> timer callback you're atomic so you can't call gb_operation_cancel()
> here directly, those two reasons are why I did it in a work-queue as
> opposed to a timer.
Yeah, there are some non-trivial issues to deal with here (connection
tear down is another). I'm quite sure you won't be able to see
queue_work failing with this patch though.
> That's why I trapped that error at the send() stage of the operation -
> because testing it out on gbsim showed messages getting lost @ the
> queue_work stage.
Yeah, (with some minor modifications) the patch you ended up submitting
would have worked, just not as efficiently.
Thanks,
Johan
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev
|