* [bug report] misc: fastrpc: Add support for context Invoke method
@ 2020-05-11 14:51 Dan Carpenter
2020-05-11 16:07 ` Srinivas Kandagatla
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2020-05-11 14:51 UTC (permalink / raw)
To: srinivas.kandagatla; +Cc: linux-media, linaro-mm-sig
Hello Srinivas Kandagatla,
The patch c68cfb718c8f: "misc: fastrpc: Add support for context
Invoke method" from Feb 8, 2019, leads to the following static
checker warning:
drivers/misc/fastrpc.c:990 fastrpc_internal_invoke()
warn: 'ctx->refcount.refcount.ref.counter' not decremented on lines: 990.
drivers/misc/fastrpc.c
925 static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel,
926 u32 handle, u32 sc,
927 struct fastrpc_invoke_args *args)
928 {
929 struct fastrpc_invoke_ctx *ctx = NULL;
930 int err = 0;
931
932 if (!fl->sctx)
933 return -EINVAL;
934
935 if (!fl->cctx->rpdev)
936 return -EPIPE;
937
938 ctx = fastrpc_context_alloc(fl, kernel, sc, args);
refcount is 1.
939 if (IS_ERR(ctx))
940 return PTR_ERR(ctx);
941
942 if (ctx->nscalars) {
943 err = fastrpc_get_args(kernel, ctx);
944 if (err)
945 goto bail;
^^^^^^^^^
Still holding one refcount.
946 }
947
948 /* make sure that all CPU memory writes are seen by DSP */
949 dma_wmb();
950 /* Send invoke buffer to remote dsp */
951 err = fastrpc_invoke_send(fl->sctx, ctx, kernel, handle);
^^^^^^^^^^^^^^^^^^^
Takes a reference count. Refcount is now 2.
952 if (err)
953 goto bail;
954
955 if (kernel) {
956 if (!wait_for_completion_timeout(&ctx->work, 10 * HZ))
957 err = -ETIMEDOUT;
958 } else {
959 err = wait_for_completion_interruptible(&ctx->work);
This drops a refcount.
960 }
961
962 if (err)
963 goto bail;
964
965 /* Check the response from remote dsp */
966 err = ctx->retval;
967 if (err)
968 goto bail;
969
970 if (ctx->nscalars) {
971 /* make sure that all memory writes by DSP are seen by CPU */
972 dma_rmb();
973 /* populate all the output buffers with results */
974 err = fastrpc_put_args(ctx, kernel);
975 if (err)
976 goto bail;
977 }
978
979 bail:
980 if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
981 /* We are done with this compute context */
982 spin_lock(&fl->lock);
983 list_del(&ctx->node);
984 spin_unlock(&fl->lock);
985 fastrpc_context_put(ctx);
If we're holding two refcounts then I think this leaks.
986 }
987 if (err)
988 dev_dbg(fl->sctx->dev, "Error: Invoke Failed %d\n", err);
989
990 return err;
991 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] misc: fastrpc: Add support for context Invoke method
2020-05-11 14:51 [bug report] misc: fastrpc: Add support for context Invoke method Dan Carpenter
@ 2020-05-11 16:07 ` Srinivas Kandagatla
0 siblings, 0 replies; 2+ messages in thread
From: Srinivas Kandagatla @ 2020-05-11 16:07 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media, linaro-mm-sig
Thanks Dan for reporting this,
On 11/05/2020 15:51, Dan Carpenter wrote:
> Hello Srinivas Kandagatla,
>
> The patch c68cfb718c8f: "misc: fastrpc: Add support for context
> Invoke method" from Feb 8, 2019, leads to the following static
> checker warning:
>
> drivers/misc/fastrpc.c:990 fastrpc_internal_invoke()
> warn: 'ctx->refcount.refcount.ref.counter' not decremented on lines: 990.
>
> drivers/misc/fastrpc.c
> 925 static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel,
> 926 u32 handle, u32 sc,
> 927 struct fastrpc_invoke_args *args)
> 928 {
> 929 struct fastrpc_invoke_ctx *ctx = NULL;
> 930 int err = 0;
> 931
> 932 if (!fl->sctx)
> 933 return -EINVAL;
> 934
> 935 if (!fl->cctx->rpdev)
> 936 return -EPIPE;
> 937
> 938 ctx = fastrpc_context_alloc(fl, kernel, sc, args);
>
> refcount is 1.
>
> 939 if (IS_ERR(ctx))
> 940 return PTR_ERR(ctx);
> 941
> 942 if (ctx->nscalars) {
> 943 err = fastrpc_get_args(kernel, ctx);
> 944 if (err)
> 945 goto bail;
> ^^^^^^^^^
> Still holding one refcount.
>
> 946 }
> 947
> 948 /* make sure that all CPU memory writes are seen by DSP */
> 949 dma_wmb();
> 950 /* Send invoke buffer to remote dsp */
> 951 err = fastrpc_invoke_send(fl->sctx, ctx, kernel, handle);
> ^^^^^^^^^^^^^^^^^^^
> Takes a reference count. Refcount is now 2.
>
> 952 if (err)
> 953 goto bail;
> 954
> 955 if (kernel) {
> 956 if (!wait_for_completion_timeout(&ctx->work, 10 * HZ))
> 957 err = -ETIMEDOUT;
> 958 } else {
> 959 err = wait_for_completion_interruptible(&ctx->work);
>
> This drops a refcount.
>
> 960 }
> 961
> 962 if (err)
> 963 goto bail;
> 964
> 965 /* Check the response from remote dsp */
> 966 err = ctx->retval;
> 967 if (err)
> 968 goto bail;
> 969
> 970 if (ctx->nscalars) {
> 971 /* make sure that all memory writes by DSP are seen by CPU */
> 972 dma_rmb();
> 973 /* populate all the output buffers with results */
> 974 err = fastrpc_put_args(ctx, kernel);
> 975 if (err)
> 976 goto bail;
> 977 }
> 978
> 979 bail:
> 980 if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
> 981 /* We are done with this compute context */
> 982 spin_lock(&fl->lock);
> 983 list_del(&ctx->node);
> 984 spin_unlock(&fl->lock);
> 985 fastrpc_context_put(ctx);
>
> If we're holding two refcounts then I think this leaks.
Code can enter with two refcounts only an error other than -ERESTARTSYS
&& err != -ETIMEDOUT. This is only possible rpmsg_send() fails for some
reason.
Let me send a patch to fix this!
thanks,
srini
>
> 986 }
> 987 if (err)
> 988 dev_dbg(fl->sctx->dev, "Error: Invoke Failed %d\n", err);
> 989
> 990 return err;
> 991 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-05-11 16:08 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-11 14:51 [bug report] misc: fastrpc: Add support for context Invoke method Dan Carpenter
2020-05-11 16:07 ` Srinivas Kandagatla
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).