| Message ID | 1441818286-31400-1-git-send-email-arnaud.pouliquen@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 09 Sep 2015 19:04:46 +0200, Arnaud Pouliquen wrote: > > Add check on of_property_read_string return to avoid compilation warning. The purpose of the patch isn't to avoid compilation warning. It's for fixing the bugs in the code. Fixing compile warning is just a consequence, not the goal. Please rewrite the subject and the change log text appropriately. Some review comments below: > - of_property_read_u32(pnode, "version", &player->ver); > - if (player->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN) { > + if ((of_property_read_u32(pnode, "version", &player->ver)) || Better to drop superfluous parentheses. > + (player->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN)) { > dev_err(dev, "Unknown uniperipheral version "); > return -EINVAL; > } In general, such a sanity check should be rather checking the valid value ranges, something like: if (ver < VER_MIN || ver > VER_MAX) error(); In short, you can't trust the value passed by DT, so you need to check it strictly (if needed). But this doesn't have to be covered by this patch. Better to be in another fix patch. > @@ -998,19 +998,25 @@ static int uni_player_parse_dt(struct platform_device *pdev, > if (player->ver >= SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0) > info->underflow_enabled = 1; > > - of_property_read_u32(pnode, "uniperiph-id", &info->id); > + if (of_property_read_u32(pnode, "uniperiph-id", &info->id)) { > + dev_err(dev, "uniperipheral id not defined"); > + return -EINVAL; > + } > > /* Read the device mode property */ > - of_property_read_string(pnode, "mode", &mode); > - > - if (strcasecmp(mode, "hdmi") == 0) > - info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_HDMI; > - else if (strcasecmp(mode, "pcm") == 0) > - info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_PCM; > - else if (strcasecmp(mode, "spdif") == 0) > - info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_SPDIF; > - else > - info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_NONE; > + if (!of_property_read_string(pnode, "mode", &mode)) { > + if (strcasecmp(mode, "hdmi") == 0) > + info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_HDMI; > + else if (strcasecmp(mode, "pcm") == 0) > + info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_PCM; > + else if (strcasecmp(mode, "spdif") == 0) > + info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_SPDIF; > + else > + info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_NONE; > + } else { > + dev_err(dev, "uniperipheral mode not defined"); > + return -EINVAL; > + } A form like below would be clearer: if (of_property_read_string(pnode, "mode", &mode)) { dev_err(dev, "uniperipheral mode not defined"); return -EINVAL; } if (strcasecmp(mode, "hdmi") == 0) info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_HDMI; .... thanks, Takashi
diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c index f6eefe1..d90a3fb 100644 --- a/sound/soc/sti/uniperif_player.c +++ b/sound/soc/sti/uniperif_player.c @@ -989,8 +989,8 @@ static int uni_player_parse_dt(struct platform_device *pdev, if (!info) return -ENOMEM; - of_property_read_u32(pnode, "version", &player->ver); - if (player->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN) { + if ((of_property_read_u32(pnode, "version", &player->ver)) || + (player->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN)) { dev_err(dev, "Unknown uniperipheral version "); return -EINVAL; } @@ -998,19 +998,25 @@ static int uni_player_parse_dt(struct platform_device *pdev, if (player->ver >= SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0) info->underflow_enabled = 1; - of_property_read_u32(pnode, "uniperiph-id", &info->id); + if (of_property_read_u32(pnode, "uniperiph-id", &info->id)) { + dev_err(dev, "uniperipheral id not defined"); + return -EINVAL; + } /* Read the device mode property */ - of_property_read_string(pnode, "mode", &mode); - - if (strcasecmp(mode, "hdmi") == 0) - info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_HDMI; - else if (strcasecmp(mode, "pcm") == 0) - info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_PCM; - else if (strcasecmp(mode, "spdif") == 0) - info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_SPDIF; - else - info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_NONE; + if (!of_property_read_string(pnode, "mode", &mode)) { + if (strcasecmp(mode, "hdmi") == 0) + info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_HDMI; + else if (strcasecmp(mode, "pcm") == 0) + info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_PCM; + else if (strcasecmp(mode, "spdif") == 0) + info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_SPDIF; + else + info->player_type = SND_ST_UNIPERIF_PLAYER_TYPE_NONE; + } else { + dev_err(dev, "uniperipheral mode not defined"); + return -EINVAL; + } /* Save the info structure */ player->info = info; diff --git a/sound/soc/sti/uniperif_reader.c b/sound/soc/sti/uniperif_reader.c index c502626..7ddbd14 100644 --- a/sound/soc/sti/uniperif_reader.c +++ b/sound/soc/sti/uniperif_reader.c @@ -316,7 +316,11 @@ static int uni_reader_parse_dt(struct platform_device *pdev, if (!info) return -ENOMEM; - of_property_read_u32(node, "version", &reader->ver); + if ((of_property_read_u32(node, "version", &reader->ver)) || + (reader->ver == SND_ST_UNIPERIF_VERSION_UNKNOWN)) { + dev_err(&pdev->dev, "Unknown uniperipheral version "); + return -EINVAL; + } /* Save the info structure */ reader->info = info;
Add check on of_property_read_string return to avoid compilation warning. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> --- sound/soc/sti/uniperif_player.c | 32 +++++++++++++++++++------------- sound/soc/sti/uniperif_reader.c | 6 +++++- 2 files changed, 24 insertions(+), 14 deletions(-)