ASoC: mediatek: mt8188: support etdm in platform driver

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-

Quoted 1 line

of_node;

    2458         struct mt8188_afe_private *afe_priv = afe-

Quoted 1 line

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-

Quoted 1 line

in_disable_ch[disable_chn[j]] = true;

    2539                 }
    2540         }
    2541         mt8188_etdm_update_sync_info(afe);
    2542 }

regards,
dan carpenter

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.

Quoted 9 lines · Trevor Wu (吳文良)

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.

Quoted 8 lines

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

Quoted 1 line

=

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