linux-kernel - Re: [PATCH v8 1/2] ASoC: hdmi: Add a transmitter interface to the HDMI CODEC

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 29 Dec 2014 17:08:18 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Jean-Francois Moine <moinejf@...e.fr>
Cc:	Russell King - ARM Linux <linux@....linux.org.uk>,
	Dave Airlie <airlied@...il.com>,
	Andrew Jackson <Andrew.Jackson@....com>,
	Jyri Sarha <jsarha@...com>, alsa-devel@...a-project.org,
	devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 1/2] ASoC: hdmi: Add a transmitter interface to the
 HDMI CODEC

On Thu, Oct 23, 2014 at 09:09:35AM +0200, Jean-Francois Moine wrote:

> +struct hdmi_data {
> +	int (*get_audio)(struct device *dev,
> +			 int *max_channels,
> +			 int *rate_mask,
> +			 int *fmt);
> +	void (*audio_switch)(struct device *dev,
> +			     int port_index,
> +			     unsigned sample_rate,
> +			     int sample_format);

I can't really tell from these function names what these functions are
supposed to do.  I think get_audio() should be get_audio_caps() or
similar since it reads the capabilities and audio_switch() is supposed
to be set_audio_params() or similar to set the settings for the running
stream.

> +	snd_pcm_hw_constraint_list(runtime, 0,
> +				   SNDRV_PCM_HW_PARAM_RATE,
> +				   rate_constraints);
> +
> +	formats = 0;
> +	if (fmt & 1)
> +		formats |= SNDRV_PCM_FMTBIT_S16_LE;
> +	if (fmt & 2)
> +		formats |= SNDRV_PCM_FMTBIT_S20_3LE;
> +	if (fmt & 4)
> +		formats |= SNDRV_PCM_FMTBIT_S24_LE |
> +			   SNDRV_PCM_FMTBIT_S24_3LE |
> +			   SNDRV_PCM_FMTBIT_S32_LE;

Magic numbers here - can't we have some constants defined?

> +	priv->hdmi_data.audio_switch(dai->dev, dai->id,
> +				     params_rate(params),
> +				     params_format(params));

I'd be happier if this were able to return an error; even if the
constraints are satisfied perhaps something changed or some operation
fails for some reason.

> +static void hdmi_shutdown(struct snd_pcm_substream *substream,
> +			  struct snd_soc_dai *dai)
> +{
> +	struct device *cdev;
> +	struct hdmi_priv *priv;
> +
> +	cdev = hdmi_get_cdev(dai->dev);
> +	if (!cdev)
> +		return;
> +	priv = dev_get_drvdata(cdev);
> +
> +	priv->hdmi_data.audio_switch(dai->dev, -1, 0, 0);	/* stop */
> +}

So the set parameters operation has to support these magic values as
being "off" - why not have an explicit shutdown operation here?

> +static int hdmi_codec_probe(struct snd_soc_codec *codec)
> +{
> +	struct hdmi_priv *priv;
> +	struct device *dev = codec->dev;	/* encoder device */
> +	struct device *cdev;			/* codec device */
> +
> +	cdev = hdmi_get_cdev(dev);
> +	if (!cdev)
> +		return -ENODEV;

This is (I think) only called for cases where the driver is being used
from a parent device with ops but the name makes it sound like it should
be called all the time so errors like that shouldn't happen.  This
should be clearer, or perhaps we should just have a separate device
(perhaps rename the existing one to say that it's for a dumb device with
no control?).

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