* [bug report] ASoC: mediatek: mt8188: support etdm in platform driver
@ 2023-01-25 12:13 Dan Carpenter
2023-01-30 9:25 ` Trevor Wu (吳文良)
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-01-25 12:13 UTC (permalink / raw)
To: trevor.wu; +Cc: linux-mediatek
Hello Trevor Wu,
The patch 2babb4777489: "ASoC: mediatek: mt8188: support etdm in
platform driver" from Jan 16, 2023, leads to the following Smatch
static checker warning:
sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2487 mt8188_dai_etdm_parse_of()
warn: 'ret' returned from snprintf() might be larger than 48
sound/soc/mediatek/mt8188/mt8188-dai-etdm.c
2455 static void mt8188_dai_etdm_parse_of(struct mtk_base_afe *afe)
2456 {
2457 const struct device_node *of_node = afe->dev->of_node;
2458 struct mt8188_afe_private *afe_priv = afe->platform_priv;
2459 struct mtk_dai_etdm_priv *etdm_data;
2460 char prop[48];
2461 u8 disable_chn[MT8188_ETDM_MAX_CHANNELS];
2462 int max_chn = MT8188_ETDM_MAX_CHANNELS;
2463 unsigned int sync_id;
2464 u32 sel;
2465 int ret;
2466 int dai_id;
2467 int i, j;
2468 struct {
2469 const char *name;
2470 const unsigned int sync_id;
2471 } of_afe_etdms[MT8188_AFE_IO_ETDM_NUM] = {
2472 {"etdm-in1", ETDM_SYNC_FROM_IN1},
2473 {"etdm-in2", ETDM_SYNC_FROM_IN2},
2474 {"etdm-out1", ETDM_SYNC_FROM_OUT1},
2475 {"etdm-out2", ETDM_SYNC_FROM_OUT2},
2476 {"etdm-out3", ETDM_SYNC_FROM_OUT3},
2477 };
2478
2479 for (i = 0; i < MT8188_AFE_IO_ETDM_NUM; i++) {
2480 dai_id = ETDM_TO_DAI_ID(i);
2481 etdm_data = afe_priv->dai_priv[dai_id];
2482
2483 ret = snprintf(prop, sizeof(prop),
2484 "mediatek,%s-multi-pin-mode",
2485 of_afe_etdms[i].name);
2486 if (ret < 0) {
--> 2487 dev_err(afe->dev, "%s snprintf err=%d\n",
2488 __func__, ret);
I guess the reason behind this warning is that we checked something but
we didn't check if "ret >= sizeof(prop)" as one would normally suspect.
In user space snprintf() can return negatives but in the kernel it never
does. Changing that in the future is impossible because it would
introduce a ton of security issues, so it never will.
I guess I suggest just removing the dead code.
2489 return;
2490 }
2491 etdm_data->data_mode = of_property_read_bool(of_node, prop);
2492
2493 ret = snprintf(prop, sizeof(prop),
2494 "mediatek,%s-cowork-source",
2495 of_afe_etdms[i].name);
2496 if (ret < 0) {
Same
2497 dev_err(afe->dev, "%s snprintf err=%d\n",
2498 __func__, ret);
2499 return;
2500 }
2501 ret = of_property_read_u32(of_node, prop, &sel);
2502 if (ret == 0) {
2503 if (sel >= MT8188_AFE_IO_ETDM_NUM) {
2504 dev_err(afe->dev, "%s invalid id=%d\n",
2505 __func__, sel);
2506 etdm_data->cowork_source_id = COWORK_ETDM_NONE;
2507 } else {
2508 sync_id = of_afe_etdms[sel].sync_id;
2509 etdm_data->cowork_source_id =
2510 sync_to_dai_id(sync_id);
2511 }
2512 } else {
2513 etdm_data->cowork_source_id = COWORK_ETDM_NONE;
2514 }
2515 }
2516
2517 /* etdm in only */
2518 for (i = 0; i < 2; i++) {
2519 ret = snprintf(prop, sizeof(prop),
2520 "mediatek,%s-chn-disabled",
2521 of_afe_etdms[i].name);
2522 if (ret < 0) {
Same.
2523 dev_err(afe->dev, "%s snprintf err=%d\n",
2524 __func__, ret);
2525 return;
2526 }
2527 ret = of_property_read_variable_u8_array(of_node, prop,
2528 disable_chn,
2529 1, max_chn);
2530 if (ret < 0)
2531 continue;
2532
2533 for (j = 0; j < ret; j++) {
2534 if (disable_chn[j] >= MT8188_ETDM_MAX_CHANNELS)
2535 dev_err(afe->dev, "%s [%d] invalid chn %u\n",
2536 __func__, j, disable_chn[j]);
2537 else
2538 etdm_data->in_disable_ch[disable_chn[j]] = true;
2539 }
2540 }
2541 mt8188_etdm_update_sync_info(afe);
2542 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] ASoC: mediatek: mt8188: support etdm in platform driver
2023-01-25 12:13 [bug report] ASoC: mediatek: mt8188: support etdm in platform driver Dan Carpenter
@ 2023-01-30 9:25 ` Trevor Wu (吳文良)
2023-01-30 11:40 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Trevor Wu (吳文良) @ 2023-01-30 9:25 UTC (permalink / raw)
To: error27@gmail.com; +Cc: linux-mediatek@lists.infradead.org
Hi Dan,
Thank you for catching that.
I didn't notice that snprintf() never returns a negative value in
kernel code.
It is possible that I misunderstood the coverity error message, but I
can't find the original coverity scan result.
If I just remove the dead code, coverity will report some errors in the
future.
As a result, I prefer to keep the line and add another check "ret >=
sizeof(prop)", so that we can avoid the problem reported by coverity
and Smatch.
Thanks,
Trevor
On Wed, 2023-01-25 at 15:13 +0300, Dan Carpenter wrote:
> Hello Trevor Wu,
>
> The patch 2babb4777489: "ASoC: mediatek: mt8188: support etdm in
> platform driver" from Jan 16, 2023, leads to the following Smatch
> static checker warning:
>
> sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2487
> mt8188_dai_etdm_parse_of()
> warn: 'ret' returned from snprintf() might be larger than 48
>
> sound/soc/mediatek/mt8188/mt8188-dai-etdm.c
> 2455 static void mt8188_dai_etdm_parse_of(struct mtk_base_afe
> *afe)
> 2456 {
> 2457 const struct device_node *of_node = afe->dev-
> >of_node;
> 2458 struct mt8188_afe_private *afe_priv = afe-
> >platform_priv;
> 2459 struct mtk_dai_etdm_priv *etdm_data;
> 2460 char prop[48];
> 2461 u8 disable_chn[MT8188_ETDM_MAX_CHANNELS];
> 2462 int max_chn = MT8188_ETDM_MAX_CHANNELS;
> 2463 unsigned int sync_id;
> 2464 u32 sel;
> 2465 int ret;
> 2466 int dai_id;
> 2467 int i, j;
> 2468 struct {
> 2469 const char *name;
> 2470 const unsigned int sync_id;
> 2471 } of_afe_etdms[MT8188_AFE_IO_ETDM_NUM] = {
> 2472 {"etdm-in1", ETDM_SYNC_FROM_IN1},
> 2473 {"etdm-in2", ETDM_SYNC_FROM_IN2},
> 2474 {"etdm-out1", ETDM_SYNC_FROM_OUT1},
> 2475 {"etdm-out2", ETDM_SYNC_FROM_OUT2},
> 2476 {"etdm-out3", ETDM_SYNC_FROM_OUT3},
> 2477 };
> 2478
> 2479 for (i = 0; i < MT8188_AFE_IO_ETDM_NUM; i++) {
> 2480 dai_id = ETDM_TO_DAI_ID(i);
> 2481 etdm_data = afe_priv->dai_priv[dai_id];
> 2482
> 2483 ret = snprintf(prop, sizeof(prop),
> 2484 "mediatek,%s-multi-pin-mode",
> 2485 of_afe_etdms[i].name);
> 2486 if (ret < 0) {
> --> 2487 dev_err(afe->dev, "%s snprintf
> err=%d\n",
> 2488 __func__, ret);
>
>
> I guess the reason behind this warning is that we checked something
> but
> we didn't check if "ret >= sizeof(prop)" as one would normally
> suspect.
>
> In user space snprintf() can return negatives but in the kernel it
> never
> does. Changing that in the future is impossible because it would
> introduce a ton of security issues, so it never will.
>
> I guess I suggest just removing the dead code.
>
> 2489 return;
> 2490 }
> 2491 etdm_data->data_mode =
> of_property_read_bool(of_node, prop);
> 2492
> 2493 ret = snprintf(prop, sizeof(prop),
> 2494 "mediatek,%s-cowork-source",
> 2495 of_afe_etdms[i].name);
> 2496 if (ret < 0) {
>
> Same
>
> 2497 dev_err(afe->dev, "%s snprintf
> err=%d\n",
> 2498 __func__, ret);
> 2499 return;
> 2500 }
> 2501 ret = of_property_read_u32(of_node, prop,
> &sel);
> 2502 if (ret == 0) {
> 2503 if (sel >= MT8188_AFE_IO_ETDM_NUM) {
> 2504 dev_err(afe->dev, "%s
> invalid id=%d\n",
> 2505 __func__, sel);
> 2506 etdm_data->cowork_source_id
> = COWORK_ETDM_NONE;
> 2507 } else {
> 2508 sync_id =
> of_afe_etdms[sel].sync_id;
> 2509 etdm_data->cowork_source_id
> =
> 2510 sync_to_dai_id(sync_
> id);
> 2511 }
> 2512 } else {
> 2513 etdm_data->cowork_source_id =
> COWORK_ETDM_NONE;
> 2514 }
> 2515 }
> 2516
> 2517 /* etdm in only */
> 2518 for (i = 0; i < 2; i++) {
> 2519 ret = snprintf(prop, sizeof(prop),
> 2520 "mediatek,%s-chn-disabled",
> 2521 of_afe_etdms[i].name);
> 2522 if (ret < 0) {
>
> Same.
>
> 2523 dev_err(afe->dev, "%s snprintf
> err=%d\n",
> 2524 __func__, ret);
> 2525 return;
> 2526 }
> 2527 ret =
> of_property_read_variable_u8_array(of_node, prop,
> 2528 dis
> able_chn,
> 2529 1,
> max_chn);
> 2530 if (ret < 0)
> 2531 continue;
> 2532
> 2533 for (j = 0; j < ret; j++) {
> 2534 if (disable_chn[j] >=
> MT8188_ETDM_MAX_CHANNELS)
> 2535 dev_err(afe->dev, "%s [%d]
> invalid chn %u\n",
> 2536 __func__, j,
> disable_chn[j]);
> 2537 else
> 2538 etdm_data-
> >in_disable_ch[disable_chn[j]] = true;
> 2539 }
> 2540 }
> 2541 mt8188_etdm_update_sync_info(afe);
> 2542 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] ASoC: mediatek: mt8188: support etdm in platform driver
2023-01-30 9:25 ` Trevor Wu (吳文良)
@ 2023-01-30 11:40 ` Dan Carpenter
2023-01-31 2:26 ` Trevor Wu (吳文良)
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-01-30 11:40 UTC (permalink / raw)
To: Trevor Wu (吳文良); +Cc: linux-mediatek@lists.infradead.org
On Mon, Jan 30, 2023 at 09:25:52AM +0000, Trevor Wu (吳文良) wrote:
> Hi Dan,
>
> Thank you for catching that.
> I didn't notice that snprintf() never returns a negative value in
> kernel code.
> It is possible that I misunderstood the coverity error message, but I
> can't find the original coverity scan result.
>
> If I just remove the dead code, coverity will report some errors in the
> future.
> As a result, I prefer to keep the line and add another check "ret >=
> sizeof(prop)", so that we can avoid the problem reported by coverity
> and Smatch.
Generally, when static checkers print nonsense warnings you should just
ignore them instead of adding dead code so I would encourage to delete
the dead code even though Coverity complains. Fix what is broken
instead of just working around it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] ASoC: mediatek: mt8188: support etdm in platform driver
2023-01-30 11:40 ` Dan Carpenter
@ 2023-01-31 2:26 ` Trevor Wu (吳文良)
0 siblings, 0 replies; 4+ messages in thread
From: Trevor Wu (吳文良) @ 2023-01-31 2:26 UTC (permalink / raw)
To: error27@gmail.com; +Cc: linux-mediatek@lists.infradead.org
On Mon, 2023-01-30 at 14:40 +0300, Dan Carpenter wrote:
> On Mon, Jan 30, 2023 at 09:25:52AM +0000, Trevor Wu (吳文良) wrote:
> > Hi Dan,
> >
> > Thank you for catching that.
> > I didn't notice that snprintf() never returns a negative value in
> > kernel code.
> > It is possible that I misunderstood the coverity error message, but
> > I
> > can't find the original coverity scan result.
> >
> > If I just remove the dead code, coverity will report some errors in
> > the
> > future.
> > As a result, I prefer to keep the line and add another check "ret
> > >=
> > sizeof(prop)", so that we can avoid the problem reported by
> > coverity
> > and Smatch.
>
> Generally, when static checkers print nonsense warnings you should
> just
> ignore them instead of adding dead code so I would encourage to
> delete
> the dead code even though Coverity complains. Fix what is broken
> instead of just working around it.
>
> regards,
> dan carpenter
>
Got it. I will prepare a patch and remove the dead code.
Thanks,
Trevor
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-31 2:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-25 12:13 [bug report] ASoC: mediatek: mt8188: support etdm in platform driver Dan Carpenter
2023-01-30 9:25 ` Trevor Wu (吳文良)
2023-01-30 11:40 ` Dan Carpenter
2023-01-31 2:26 ` Trevor Wu (吳文良)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox