linux-kernel - Re: [PATCH 5/7] ASoC: imx-pcm-rpmsg: Add platform driver for audio base on rpmsg

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]

Message-ID: <20210205145816.GD4720@sirena.org.uk>
Date:   Fri, 5 Feb 2021 14:58:16 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Shengjiu Wang <shengjiu.wang@....com>
Cc:     lgirdwood@...il.com, perex@...ex.cz, tiwai@...e.com,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        timur@...nel.org, nicoleotsuka@...il.com, Xiubo.Lee@...il.com,
        festevam@...il.com, linuxppc-dev@...ts.ozlabs.org,
        robh+dt@...nel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 5/7] ASoC: imx-pcm-rpmsg: Add platform driver for audio
 base on rpmsg

On Fri, Feb 05, 2021 at 02:57:28PM +0800, Shengjiu Wang wrote:

> +	if (params_format(params) == SNDRV_PCM_FORMAT_S16_LE)
> +		msg->s_msg.param.format   = RPMSG_S16_LE;
> +	else if (params_format(params) == SNDRV_PCM_FORMAT_S24_LE)

Again this should be a switch statement.

> +	if (params_channels(params) == 1)
> +		msg->s_msg.param.channels = RPMSG_CH_LEFT;
> +	else
> +		msg->s_msg.param.channels = RPMSG_CH_STEREO;

Shouldn't this be reporting an error if the number of channels is more
than 2?

> +		/*
> +		 * if the data in the buffer is less than one period
> +		 * send message immediately.
> +		 * if there is more than one period data, delay one
> +		 * period (timer) to send the message.
> +		 */
> +		if ((avail - writen_num * period_size) <= period_size) {
> +			imx_rpmsg_insert_workqueue(substream, msg, info);
> +		} else if (rpmsg->force_lpa && !timer_pending(timer)) {
> +			int time_msec;
> +
> +			time_msec = (int)(runtime->period_size * 1000 / runtime->rate);
> +			mod_timer(timer, jiffies + msecs_to_jiffies(time_msec));
> +		}

The comment here is at least confusing - why would we not send a full
buffer immediately if we have one?  This sounds like it's the opposite
way round to what we'd do if we were trying to cut down the number of
messages.  It might help to say which buffer and where?

> +	/**
> +	 * Every work in the work queue, first we check if there

/** comments are only for kerneldoc.

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