linux-kernel - Re: [PATCH v3 14/25] ASoC: qcom: qdsp6: Add support to q6afe dai driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Mar 2018 12:50:54 +0000
From:   Mark Brown <broonie@...nel.org>
To:     srinivas.kandagatla@...aro.org
Cc:     andy.gross@...aro.org, linux-arm-msm@...r.kernel.org,
        alsa-devel@...a-project.org, david.brown@...aro.org,
        robh+dt@...nel.org, mark.rutland@....com, lgirdwood@...il.com,
        plai@...eaurora.org, bgoswami@...eaurora.org, perex@...ex.cz,
        tiwai@...e.com, linux-soc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, rohkumar@....qualcomm.com,
        spatakok@....qualcomm.com
Subject: Re: [PATCH v3 14/25] ASoC: qcom: qdsp6: Add support to q6afe dai
 driver

On Tue, Feb 13, 2018 at 04:58:26PM +0000, srinivas.kandagatla@...aro.org wrote:

> +static int q6hdmi_format_put(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct q6afe_dai_data *dai_data = kcontrol->private_data;
> +	int value = ucontrol->value.integer.value[0];
> +
> +	dai_data->port_config[AFE_PORT_HDMI_RX].hdmi.datatype = value;
> +
> +	return 0;
> +}

No validation, and do we not need to tell a currently running stream if
the format changed on it (or block such changes if they're not going to
work, which seems more likely)?

> +static int q6hdmi_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)
> +{
> +	struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
> +	int channels = params_channels(params);
> +	struct q6afe_hdmi_cfg *hdmi = &dai_data->port_config[dai->id].hdmi;
> +
> +	hdmi->sample_rate = params_rate(params);
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		hdmi->bit_width = 16;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		hdmi->bit_width = 24;
> +		break;
> +	}

This silently accepts invalid values.

> +	/*refer to HDMI spec CEA-861-E: Table 28 Audio InfoFrame Data Byte 4*/

Coding style, spaces around the /* */.

> +static int q6afe_dai_startup(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
> +
> +	dai_data->is_port_started[dai->id] = false;
> +
> +	return 0;
> +}

If this is needed it makes me a bit worried that we've got some kind of
bug with not shutting things down properly somewhere - what's going on
here?

> +static void q6afe_dai_shutdown(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct q6afe_dai_data *dai_data = q6afe_get_dai_data(dai->dev);
> +	int rc;
> +
> +	rc = q6afe_port_stop(dai_data->port[dai->id]);
> +	if (rc < 0)
> +		dev_err(dai->dev, "fail to close AFE port\n");

Better to print the error code so users have more information to debug
the problem.

> +			.stream_name = "HDMI Playback",
> +			.rates = SNDRV_PCM_RATE_48000 |
> +				 SNDRV_PCM_RATE_96000 |
> +			 SNDRV_PCM_RATE_192000,

Indentation again.

> +static int q6afe_of_xlate_dai_name(struct snd_soc_component *component,
> +				   struct of_phandle_args *args,
> +				   const char **dai_name)
> +{
> +	int id = args->args[0];
> +	int i, ret = -EINVAL;

Coding style, don't mix initialization in with other variable
declarations on the same line like this.

> +int q6afe_dai_dev_remove(struct device *dev)
> +{
> +	return 0;
> +}

Remove empty functions, if they can't be removed it's probably not OK
for them to be empty either.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)