* [bug report] media: platform: mtk-mdp3: add MediaTek MDP3 driver
@ 2022-09-01 9:33 Dan Carpenter
2022-09-02 6:38 ` moudy ho
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-09-01 9:33 UTC (permalink / raw)
To: moudy.ho; +Cc: linux-media, linux-mediatek
Hello Moudy Ho,
The patch 61890ccaefaf: "media: platform: mtk-mdp3: add MediaTek MDP3
driver" from Aug 23, 2022, leads to the following Smatch static
checker warning:
drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c:292 mdp_probe()
error: we previously assumed 'mdp' could be null (see line 188)
My blog entry gives a good overview of how to write Free the Last Thing
Style error handling. Let's take a look at it as it applies to
mdp_probe().
drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
180 static int mdp_probe(struct platform_device *pdev)
181 {
182 struct device *dev = &pdev->dev;
183 struct mdp_dev *mdp;
184 struct platform_device *mm_pdev;
185 int ret, i;
186
187 mdp = kzalloc(sizeof(*mdp), GFP_KERNEL);
188 if (!mdp) {
189 ret = -ENOMEM;
190 goto err_return;
There is no Last Thing to free. This should be "return -ENOMEM;".
This goto will crash. The Last thing is now "mdp".
191 }
192
193 mdp->pdev = pdev;
194 mdp->mdp_data = of_device_get_match_data(&pdev->dev);
195
196 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_MMSYS);
197 if (!mm_pdev) {
198 ret = -ENODEV;
199 goto err_return;
This should be "goto err_free_mdp;". This goto will trigger a series
of WARN_ON() stack traces. The Last Thing is now mdp->mdp_mmsys.
200 }
201 mdp->mdp_mmsys = &mm_pdev->dev;
202
203 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_MUTEX);
204 if (WARN_ON(!mm_pdev)) {
205 ret = -ENODEV;
206 goto err_return;
This goto should call put on mdp->mdp_mmsys dev_something. It instead
leaks that. But it does call mtk_mutex_put() and each call will leads
to a series of stack traces. The Last Thing is now mm_pdev. That
doesn't seem to be stored anywhere so it's going to be complicated to
free it... :/
207 }
208 for (i = 0; i < MDP_PIPE_MAX; i++) {
209 mdp->mdp_mutex[i] = mtk_mutex_get(&mm_pdev->dev);
210 if (!mdp->mdp_mutex[i]) {
211 ret = -ENODEV;
212 goto err_return;
If this fails it should unwind the successful allocations:
while (--i >= 0) {
But instead it does unwinds everything. Stack traces.
213 }
214 }
215
216 ret = mdp_comp_config(mdp);
217 if (ret) {
218 dev_err(dev, "Failed to config mdp components\n");
219 goto err_return;
Alright. Good. This will leak the "mm_pdev" references but it will not
print any stack traces. The mdp_comp_config() function uses One Magic
Free Function style error handling so it is buggy. The Last thing is
now comp_config.
220 }
221
222 mdp->job_wq = alloc_workqueue(MDP_MODULE_NAME, WQ_FREEZABLE, 0);
223 if (!mdp->job_wq) {
224 dev_err(dev, "Unable to create job workqueue\n");
225 ret = -ENOMEM;
226 goto err_deinit_comp;
Hooray! This label has a good name and frees the Last Thing. The new
last thing is the ->job_wq.
227 }
228
229 mdp->clock_wq = alloc_workqueue(MDP_MODULE_NAME "-clock", WQ_FREEZABLE,
230 0);
231 if (!mdp->clock_wq) {
232 dev_err(dev, "Unable to create clock workqueue\n");
233 ret = -ENOMEM;
234 goto err_destroy_job_wq;
Hooray!
235 }
236
237 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_SCP);
238 if (WARN_ON(!mm_pdev)) {
239 dev_err(&pdev->dev, "Could not get scp device\n");
240 ret = -ENODEV;
241 goto err_destroy_clock_wq;
Hooray!
242 }
243 mdp->scp = platform_get_drvdata(mm_pdev);
244 mdp->rproc_handle = scp_get_rproc(mdp->scp);
245 dev_dbg(&pdev->dev, "MDP rproc_handle: %pK", mdp->rproc_handle);
246
247 mutex_init(&mdp->vpu_lock);
248 mutex_init(&mdp->m2m_lock);
249
250 mdp->cmdq_clt = cmdq_mbox_create(dev, 0);
251 if (IS_ERR(mdp->cmdq_clt)) {
252 ret = PTR_ERR(mdp->cmdq_clt);
253 goto err_put_scp;
Ugh... The mm_pdev name changed to scp. It took a while to figure that
out. This unwinds the __get_pdev_by_id(). Fair enough. The Last thing
is now cmdq_clt.
254 }
255
256 init_waitqueue_head(&mdp->callback_wq);
257 ida_init(&mdp->mdp_ida);
258 platform_set_drvdata(pdev, mdp);
259
260 vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
261
262 ret = v4l2_device_register(dev, &mdp->v4l2_dev);
263 if (ret) {
264 dev_err(dev, "Failed to register v4l2 device\n");
265 ret = -EINVAL;
266 goto err_mbox_destroy;
The cmdq_clt is an mbox. Fair enough. Good name, label does what we
expected.
267 }
268
269 ret = mdp_m2m_device_register(mdp);
270 if (ret) {
271 v4l2_err(&mdp->v4l2_dev, "Failed to register m2m device\n");
272 goto err_unregister_device;
Good.
273 }
274
275 dev_dbg(dev, "mdp-%d registered successfully\n", pdev->id);
276 return 0;
Summary:
BUG1: NULL dereference
BUG2: Stack traces calling mtk_mutex_put().
BUG3&4: Two __get_pdev_by_id() which need a matching put (reference leaks).
277
278 err_unregister_device:
279 v4l2_device_unregister(&mdp->v4l2_dev);
280 err_mbox_destroy:
281 cmdq_mbox_destroy(mdp->cmdq_clt);
282 err_put_scp:
283 scp_put(mdp->scp);
284 err_destroy_clock_wq:
285 destroy_workqueue(mdp->clock_wq);
286 err_destroy_job_wq:
287 destroy_workqueue(mdp->job_wq);
288 err_deinit_comp:
289 mdp_comp_destroy(mdp);
290 err_return:
291 for (i = 0; i < MDP_PIPE_MAX; i++)
--> 292 mtk_mutex_put(mdp->mdp_mutex[i]);
293 kfree(mdp);
294 dev_dbg(dev, "Errno %d\n", ret);
295 return ret;
296 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] media: platform: mtk-mdp3: add MediaTek MDP3 driver
2022-09-01 9:33 [bug report] media: platform: mtk-mdp3: add MediaTek MDP3 driver Dan Carpenter
@ 2022-09-02 6:38 ` moudy ho
0 siblings, 0 replies; 3+ messages in thread
From: moudy ho @ 2022-09-02 6:38 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media, linux-mediatek
Hi Dan,
Thanks for your time and all advices on mdp3 driver!
Regarding note 2 you listed, I tried using a queue event to notify that
the suspend process will not execute until the CMQ send task done.
ref: function "mdp_suspend" in mtk-mdp3-core.cmdq
Although I've added a patch (as below)to fix the smatch bugs and
warnings you mentioned, , and it is less complete than your fix
Message ID = 20220831102853.6843-1-moudy.ho@mediatek.com/
I'd like to confirm whether I should send the v3 patch as per your
suggestion or just drop it and let you deal with it.
Regards,
Moudy
On Thu, 2022-09-01 at 12:33 +0300, Dan Carpenter wrote:
> Hello Moudy Ho,
>
> The patch 61890ccaefaf: "media: platform: mtk-mdp3: add MediaTek MDP3
> driver" from Aug 23, 2022, leads to the following Smatch static
> checker warning:
>
> drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c:292
> mdp_probe()
> error: we previously assumed 'mdp' could be null (see line 188)
>
> My blog entry gives a good overview of how to write Free the Last
> Thing
> Style error handling. Let's take a look at it as it applies to
> mdp_probe().
>
> drivers/media/platform/mediatek/mdp3/mtk-mdp3-core.c
> 180 static int mdp_probe(struct platform_device *pdev)
> 181 {
> 182 struct device *dev = &pdev->dev;
> 183 struct mdp_dev *mdp;
> 184 struct platform_device *mm_pdev;
> 185 int ret, i;
> 186
> 187 mdp = kzalloc(sizeof(*mdp), GFP_KERNEL);
> 188 if (!mdp) {
> 189 ret = -ENOMEM;
> 190 goto err_return;
>
> There is no Last Thing to free. This should be "return -ENOMEM;".
> This goto will crash. The Last thing is now "mdp".
>
> 191 }
> 192
> 193 mdp->pdev = pdev;
> 194 mdp->mdp_data = of_device_get_match_data(&pdev->dev);
> 195
> 196 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_MMSYS);
> 197 if (!mm_pdev) {
> 198 ret = -ENODEV;
> 199 goto err_return;
>
> This should be "goto err_free_mdp;". This goto will trigger a series
> of WARN_ON() stack traces. The Last Thing is now mdp->mdp_mmsys.
>
> 200 }
> 201 mdp->mdp_mmsys = &mm_pdev->dev;
> 202
> 203 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_MUTEX);
> 204 if (WARN_ON(!mm_pdev)) {
> 205 ret = -ENODEV;
> 206 goto err_return;
>
> This goto should call put on mdp->mdp_mmsys dev_something. It
> instead
> leaks that. But it does call mtk_mutex_put() and each call will
> leads
> to a series of stack traces. The Last Thing is now mm_pdev. That
> doesn't seem to be stored anywhere so it's going to be complicated to
> free it... :/
>
> 207 }
> 208 for (i = 0; i < MDP_PIPE_MAX; i++) {
> 209 mdp->mdp_mutex[i] = mtk_mutex_get(&mm_pdev-
> >dev);
> 210 if (!mdp->mdp_mutex[i]) {
> 211 ret = -ENODEV;
> 212 goto err_return;
>
> If this fails it should unwind the successful allocations:
>
> while (--i >= 0) {
>
> But instead it does unwinds everything. Stack traces.
>
> 213 }
> 214 }
> 215
> 216 ret = mdp_comp_config(mdp);
> 217 if (ret) {
> 218 dev_err(dev, "Failed to config mdp
> components\n");
> 219 goto err_return;
>
> Alright. Good. This will leak the "mm_pdev" references but it will
> not
> print any stack traces. The mdp_comp_config() function uses One
> Magic
> Free Function style error handling so it is buggy. The Last thing is
> now comp_config.
>
> 220 }
> 221
> 222 mdp->job_wq = alloc_workqueue(MDP_MODULE_NAME,
> WQ_FREEZABLE, 0);
> 223 if (!mdp->job_wq) {
> 224 dev_err(dev, "Unable to create job
> workqueue\n");
> 225 ret = -ENOMEM;
> 226 goto err_deinit_comp;
>
> Hooray! This label has a good name and frees the Last Thing. The
> new
> last thing is the ->job_wq.
>
> 227 }
> 228
> 229 mdp->clock_wq = alloc_workqueue(MDP_MODULE_NAME "-
> clock", WQ_FREEZABLE,
> 230 0);
> 231 if (!mdp->clock_wq) {
> 232 dev_err(dev, "Unable to create clock
> workqueue\n");
> 233 ret = -ENOMEM;
> 234 goto err_destroy_job_wq;
>
> Hooray!
>
> 235 }
> 236
> 237 mm_pdev = __get_pdev_by_id(pdev, MDP_INFRA_SCP);
> 238 if (WARN_ON(!mm_pdev)) {
> 239 dev_err(&pdev->dev, "Could not get scp
> device\n");
> 240 ret = -ENODEV;
> 241 goto err_destroy_clock_wq;
>
> Hooray!
>
> 242 }
> 243 mdp->scp = platform_get_drvdata(mm_pdev);
> 244 mdp->rproc_handle = scp_get_rproc(mdp->scp);
> 245 dev_dbg(&pdev->dev, "MDP rproc_handle: %pK", mdp-
> >rproc_handle);
> 246
> 247 mutex_init(&mdp->vpu_lock);
> 248 mutex_init(&mdp->m2m_lock);
> 249
> 250 mdp->cmdq_clt = cmdq_mbox_create(dev, 0);
> 251 if (IS_ERR(mdp->cmdq_clt)) {
> 252 ret = PTR_ERR(mdp->cmdq_clt);
> 253 goto err_put_scp;
>
> Ugh... The mm_pdev name changed to scp. It took a while to figure
> that
> out. This unwinds the __get_pdev_by_id(). Fair enough. The Last
> thing
> is now cmdq_clt.
>
> 254 }
> 255
> 256 init_waitqueue_head(&mdp->callback_wq);
> 257 ida_init(&mdp->mdp_ida);
> 258 platform_set_drvdata(pdev, mdp);
> 259
> 260 vb2_dma_contig_set_max_seg_size(&pdev->dev,
> DMA_BIT_MASK(32));
> 261
> 262 ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> 263 if (ret) {
> 264 dev_err(dev, "Failed to register v4l2
> device\n");
> 265 ret = -EINVAL;
> 266 goto err_mbox_destroy;
>
> The cmdq_clt is an mbox. Fair enough. Good name, label does what we
> expected.
>
> 267 }
> 268
> 269 ret = mdp_m2m_device_register(mdp);
> 270 if (ret) {
> 271 v4l2_err(&mdp->v4l2_dev, "Failed to register
> m2m device\n");
> 272 goto err_unregister_device;
>
> Good.
>
> 273 }
> 274
> 275 dev_dbg(dev, "mdp-%d registered successfully\n",
> pdev->id);
> 276 return 0;
>
> Summary:
> BUG1: NULL dereference
> BUG2: Stack traces calling mtk_mutex_put().
> BUG3&4: Two __get_pdev_by_id() which need a matching put (reference
> leaks).
>
> 277
> 278 err_unregister_device:
> 279 v4l2_device_unregister(&mdp->v4l2_dev);
> 280 err_mbox_destroy:
> 281 cmdq_mbox_destroy(mdp->cmdq_clt);
> 282 err_put_scp:
> 283 scp_put(mdp->scp);
> 284 err_destroy_clock_wq:
> 285 destroy_workqueue(mdp->clock_wq);
> 286 err_destroy_job_wq:
> 287 destroy_workqueue(mdp->job_wq);
> 288 err_deinit_comp:
> 289 mdp_comp_destroy(mdp);
> 290 err_return:
> 291 for (i = 0; i < MDP_PIPE_MAX; i++)
> --> 292 mtk_mutex_put(mdp->mdp_mutex[i]);
> 293 kfree(mdp);
> 294 dev_dbg(dev, "Errno %d\n", ret);
> 295 return ret;
> 296 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* [bug report] media: platform: mtk-mdp3: add MediaTek MDP3 driver
@ 2022-09-01 9:02 Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-09-01 9:02 UTC (permalink / raw)
To: moudy.ho; +Cc: linux-media, linux-mediatek
Hello Moudy Ho,
The patch 61890ccaefaf: "media: platform: mtk-mdp3: add MediaTek MDP3
driver" from Aug 23, 2022, leads to the following Smatch static
checker warning:
drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c:460 mdp_cmdq_send()
error: we previously assumed 'cmd' could be null (see line 369)
The error handling in mdp_cmdq_send() has a number of bugs.
Dereferencing uninitialized pointers, a double free, stack traces, and
not propagating the negative error code, and returning a positive
number instead of a negative error codes. See below for details and
potential fixes (not complete).
Read my blog for more Error Handling Tips!
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/
diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
index 29f6c1cd3de7..0cdb62c27b58 100644
--- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
+++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-cmdq.c
@@ -252,10 +252,9 @@ static int mdp_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *pkt,
dma_addr_t dma_addr;
pkt->va_base = kzalloc(size, GFP_KERNEL);
- if (!pkt->va_base) {
- kfree(pkt);
NOTE1: Freeing here lead to a double free
+ if (!pkt->va_base)
return -ENOMEM;
- }
+
pkt->buf_size = size;
pkt->cl = (void *)client;
@@ -368,25 +367,24 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param)
cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
if (!cmd) {
ret = -ENOMEM;
- goto err_cmdq_data;
+ goto err_decrement;
}
- if (mdp_cmdq_pkt_create(mdp->cmdq_clt, &cmd->pkt, SZ_16K)) {
- ret = -ENOMEM;
+ ret = mdp_cmdq_pkt_create(mdp->cmdq_clt, &cmd->pkt, SZ_16K);
+ if (ret)
goto err_cmdq_data;
- }
comps = kcalloc(param->config->num_components, sizeof(*comps),
GFP_KERNEL);
if (!comps) {
ret = -ENOMEM;
- goto err_cmdq_data;
+ goto err_destroy_pkt;
}
path = kzalloc(sizeof(*path), GFP_KERNEL);
if (!path) {
ret = -ENOMEM;
- goto err_cmdq_data;
+ goto err_free_comps;
}
path->mdp_dev = mdp;
@@ -406,7 +404,7 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param)
ret = mdp_path_ctx_init(mdp, path);
if (ret) {
dev_err(dev, "mdp_path_ctx_init error\n");
- goto err_cmdq_data;
+ goto err_free_path;
}
mtk_mutex_prepare(mdp->mdp_mutex[MDP_PIPE_RDMA0]);
@@ -414,7 +412,7 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param)
ret = mdp_path_config(mdp, cmd, path);
if (ret) {
dev_err(dev, "mdp_path_config error\n");
- goto err_cmdq_data;
+ goto err_free_path;
}
cmdq_pkt_finalize(&cmd->pkt);
@@ -433,7 +431,7 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param)
ret = mdp_comp_clocks_on(&mdp->pdev->dev, cmd->comps, cmd->num_comps);
if (ret) {
dev_err(dev, "comp %d failed to enable clock!\n", ret);
- goto err_clock_off;
+ goto err_free_path;
}
dma_sync_single_for_device(mdp->cmdq_clt->chan->mbox->dev,
@@ -453,14 +451,20 @@ int mdp_cmdq_send(struct mdp_dev *mdp, struct mdp_cmdq_param *param)
mtk_mutex_unprepare(mdp->mdp_mutex[MDP_PIPE_RDMA0]);
mdp_comp_clocks_off(&mdp->pdev->dev, cmd->comps,
cmd->num_comps);
-err_cmdq_data:
+// FIXME: what to do here?
+// err_cmdq_data:
+// wake_up(&mdp->callback_wq);
NOTE2: not sure how why we call wake_up(&mdp->callback_wq);
+err_free_path:
kfree(path);
- atomic_dec(&mdp->job_count);
- wake_up(&mdp->callback_wq);
- if (cmd->pkt.buf_size > 0)
- mdp_cmdq_pkt_destroy(&cmd->pkt);
+err_free_comps:
kfree(comps);
+err_destroy_pkt:
+ mdp_cmdq_pkt_destroy(&cmd->pkt);
+err_free_cmd:
kfree(cmd);
+err_decrement:
+ atomic_dec(&mdp->job_count);
+
return ret;
}
EXPORT_SYMBOL_GPL(mdp_cmdq_send);
diff --git a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c
index e62abf3587bf..a18ac10269a6 100644
--- a/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c
+++ b/drivers/media/platform/mediatek/mdp3/mtk-mdp3-comp.c
@@ -699,11 +699,22 @@ int mdp_comp_clock_on(struct device *dev, struct mdp_comp *comp)
dev_err(dev,
"Failed to enable clk %d. type:%d id:%d\n",
i, comp->type, comp->id);
- return ret;
+ goto err_unwind;
}
}
return 0;
NOTE3: This function *must* clean up after itself. Calling
clk_disable_unprepare() on an uninitialized clk will trigger a stack
trace.
+
+err_unwind:
+ while (--i >= 0) {
+ if (IS_ERR_OR_NULL(comp->clks[i]))
+ continue;
+ clk_disable_unprepare(comp->clks[i]);
+ }
+ if (comp->comp_dev)
+ pm_runtime_put_sync(comp->comp_dev);
+
+ return ret;
}
void mdp_comp_clock_off(struct device *dev, struct mdp_comp *comp)
@@ -722,13 +733,21 @@ void mdp_comp_clock_off(struct device *dev, struct mdp_comp *comp)
int mdp_comp_clocks_on(struct device *dev, struct mdp_comp *comps, int num)
{
- int i;
+ int ret, i;
- for (i = 0; i < num; i++)
- if (mdp_comp_clock_on(dev, &comps[i]) != 0)
- return ++i;
NOTE4: This function is supposed to return a negative error code.
+ for (i = 0; i < num; i++) {
+ ret = mdp_comp_clock_on(dev, &comps[i]);
+ if (ret)
+ goto err_unwind;
+ }
return 0;
+
NOTE5: Same bug here.
+err_unwind:
+ while (--i >= 0)
+ mdp_comp_clock_off(dev, &comps[i]);
+
+ return ret;
}
void mdp_comp_clocks_off(struct device *dev, struct mdp_comp *comps, int num)
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-02 6:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-01 9:33 [bug report] media: platform: mtk-mdp3: add MediaTek MDP3 driver Dan Carpenter
2022-09-02 6:38 ` moudy ho
-- strict thread matches above, loose matches on Subject: below --
2022-09-01 9:02 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.