linux-kernel - Re: [RFC PATCH v2 3/5] ASoC: apple: Add MCA platform driver for Apple SoCs

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, 6 Jun 2022 22:35:20 +0200
From:   Martin Povišer <povik+lin@...ebit.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mark Kettenis <kettenis@...nbsd.org>,
        Hector Martin <marcan@...can.st>,
        Sven Peter <sven@...npeter.dev>, asahi@...ts.linux.dev
Subject: Re: [RFC PATCH v2 3/5] ASoC: apple: Add MCA platform driver for Apple
 SoCs


> On 6. 6. 2022, at 22:17, Mark Brown <broonie@...nel.org> wrote:
> 
> On Mon, Jun 06, 2022 at 09:19:08PM +0200, Martin Povišer wrote:
> 
>> +++ b/sound/soc/apple/mca.c
>> @@ -0,0 +1,1122 @@
>> +/*
>> + * Apple SoCs MCA driver
> 
> Please add SPDX headers to all your files.
> 
>> +		mca_modify(cl, serdes_conf,
>> +			SERDES_CONF_SOME_RST, SERDES_CONF_SOME_RST);
>> +		(void) readl_relaxed(cl->base + serdes_conf);
> 
> Please drop the cast, casts to/from void are generally a warning sign as
> they're unneeded in C.  If you want to document the barrier use a
> comment or wrapper function.
> 
>> +	/*
>> +	 * Codecs require clocks at time of umute with the 'mute_stream' op.
>> +	 * We need to enable them here at the latest (frontend prepare would
>> +	 * be too late).
>> +	 */
>> +	if (!mca_fe_clocks_in_use(fe_cl)) {
>> +		ret = mca_fe_enable_clocks(fe_cl);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
> 
> This requirement is CODEC specific.  It's fine to bodge around to
> satisfy it though, especially given the restricted set of platforms this
> can be used with.
> 
>> +	fe_cl = &mca->clusters[cl->port_driver];
>> +	if (!mca_fe_clocks_in_use(fe_cl))
>> +		return 0; /* Nothing to do */
>> +
>> +	cl->clocks_in_use[substream->stream] = false;
>> +
>> +	if (!mca_fe_clocks_in_use(fe_cl))
>> +		mca_fe_disable_clocks(fe_cl);
> 
> Are you sure this doesn't need locking?

I am not sure. I need to study what locking is already done by ALSA/ASoC.
I assume the two stream directions here don’t share a lock already...