Re: [PATCH v8 4/4] misc: fastrpc: Add polling mode support for fastRPC driver

[Luben Tuikov <ltuikov89@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3463 bytes --]

On 2026-04-16 09:58, Ekansh Gupta wrote:
> On 16-04-2026 13:47, Luben Tuikov wrote:
>> Hi Ekansh,
>>
>> Good work. A couple of notes below:
--cut--->>> +static inline int fastrpc_wait_for_response(struct fastrpc_invoke_ctx *ctx,
>>> +					    u32 kernel)
>>
>> What is "kernel" and why is it a u32 when it is used as a "bool"? Perhaps a better name can be had?
> This reflects kernel message. As of now, just propagated the same that
> is used across the driver, maybe can address this as a separate patch.

I can see that its origin is internal to the driver, as a boolean. Perhaps "kernel_message" or "kmessage" or something descriptive like that. I think it's more important that it is a "message", rather than "kernel message".

Yes, a separate patch indeed makes sense for this.

--cut--->>> +static int fastrpc_wait_for_completion(struct fastrpc_invoke_ctx *ctx,
>>> +				       u32 kernel)
>>> +{
>>> +	int err;
>>> +
>>> +	do {
>>> +		if (ctx->is_polled) {
>>> +			err = poll_for_remote_response(ctx);
>>> +			/* If polling timed out, move to normal response mode */
>>> +			if (err)
>>> +				ctx->is_polled = false;
>>> +		} else {
>>> +			err = fastrpc_wait_for_response(ctx, kernel);
>>> +			if (err)
>>> +				return err;
>>> +		}
>>> +	} while (!ctx->is_work_done);
>>
>> Perhaps you want to also check "err" here to make the exit condition more explicit. (The invariant in do-while loops is generally directly determined by something within the loop and generally not implicit.)
> The reason to not keep "err" check is because the call should fallback
> to normal response(fastrpc_wait_for_response()) in case
> poll_for_remote_response() fails.
>>
>> Is it possible that in poll_for_remote_response() you get 0 as a poll result and val is not equal to FASTRCPC_POLL_RESPONSE? In such a case, this may hang. (Is a hang desired here?)
> That's actually a good point, let me try making it more robust, this
> condition might get encountered in case normal response is sent instead
> of poll memory update.

Right. We want to avoid this dependency. If the device hangs for whatever reason (defective device, cosmic ray, etc.) this should not result in a process or a kernel execution context hanging.

>> Is it possible that if polling is enabled, then you want to poll only once, and if unsuccessful, or successful but "!work_done", then transition to fastrpc_wait_for_response() and return, without looping? (since polling is looping after all...)
> This is correct, the intention is the poll until it returns, continue if
> successful and fallback to normal response if unsuccessful.

Right. So this was obvious by reading the contents of the do-while loop. If you prefer, you can remove the do-while loop, or at least take out the poll_for_remote_response() out, and only leave the fastrpc_wait_for_response() inside the loop, and decide how many time intervals you want to wait. If that is once, then you don't need the do-while loop. (Effectively, you've waited once in the poll and 2nd time in the fast_wait_for_response().)

We just want to avoid hangs.

> Thanks, Luben, for taking the time to review this change and for
> providing insightful comments.

Yes, no problem. Good work and thank you for your work and contribution!
> [1]
> https://lore.kernel.org/all/wipphezpxtuuxtwhpwamsmvhwgwuesexmy5ev5pcqb65vov5kz@vuzzyyqnu7ci/
Ah, thank you for this reference.
-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]