linux-kernel - Re: [PATCH 2/5] soundwire: stream: add helper to startup/shutdown streams

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: <5123471d-92f3-8f49-7a2c-d7b70bf442af@linux.intel.com>
Date:   Tue, 30 Jun 2020 11:58:41 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Vinod Koul <vkoul@...nel.org>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>
Cc:     alsa-devel@...a-project.org, tiwai@...e.de,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        ranjani.sridharan@...ux.intel.com, hui.wang@...onical.com,
        broonie@...nel.org, srinivas.kandagatla@...aro.org,
        jank@...ence.com, mengdong.lin@...el.com,
        slawomir.blauciak@...el.com, sanyog.r.kale@...el.com,
        rander.wang@...ux.intel.com, bard.liao@...el.com
Subject: Re: [PATCH 2/5] soundwire: stream: add helper to startup/shutdown
 streams

Thanks Vinod for the review.

>> --- a/drivers/soundwire/stream.c
>> +++ b/drivers/soundwire/stream.c
>> @@ -13,6 +13,9 @@
>>   #include <linux/slab.h>
>>   #include <linux/soundwire/sdw_registers.h>
>>   #include <linux/soundwire/sdw.h>
>> +#include <sound/core.h>
> 
> Do we really need core header?

No we don't, the only thing needed in sound/soc.h it seems.

>> +#include <sound/pcm.h>
>> +#include <sound/soc.h>
>>   #include "bus.h"
>>   
>>   /*
>> @@ -1826,3 +1829,90 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL(sdw_deprepare_stream);
>> +
>> +static int set_stream(struct snd_pcm_substream *substream,
>> +		      struct sdw_stream_runtime *sdw_stream)
> 
> sdw_set_stream() please

it's an internal helper not exported, the choice of not adding a prefix 
was intentional to avoid confusion with exported ones.

>> +{
>> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +	struct snd_soc_dai *dai;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	/* Set stream pointer on all DAIs */
>> +	for_each_rtd_dais(rtd, i, dai) {
>> +		ret = snd_soc_dai_set_sdw_stream(dai, sdw_stream,
>> +						 substream->stream);
>> +		if (ret < 0) {
>> +			dev_err(rtd->dev,
>> +				"failed to set stream pointer on dai %s",
>> +				dai->name);
> 
> lets use one line and shiny new 100 char limit, would make code read
> better!

yes

>> +			break;
> 
> So on error should unset of stream pointer be done?

it's already done below, see [1]

>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +int sdw_startup_stream(void *sdw_substream)
> 
> Can we have kernel doc style Documentation for exported APIs?

yes, that's a miss indeed.

Though if we follow the existing examples it's not going to be very 
informative, e.g.

/**
  * sdw_disable_stream() - Disable SoundWire stream
  *
  * @stream: Soundwire stream
  *
  * Documentation/driver-api/soundwire/stream.rst explains this API in 
detail
  */

>> +{
>> +	struct snd_pcm_substream *substream = sdw_substream;
>> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +	struct sdw_stream_runtime *sdw_stream;
>> +	char *name;
>> +	int ret;
>> +
>> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +		name = kasprintf(GFP_KERNEL, "%s-Playback", substream->name);
>> +	else
>> +		name = kasprintf(GFP_KERNEL, "%s-Capture", substream->name);
>> +
>> +	if (!name)
>> +		return -ENOMEM;
>> +
>> +	sdw_stream = sdw_alloc_stream(name);
>> +	if (!sdw_stream) {
>> +		dev_err(rtd->dev, "alloc stream failed for substream DAI %s",
>> +			substream->name);
>> +		ret = -ENOMEM;
>> +		goto error;
>> +	}
>> +
>> +	ret = set_stream(substream, sdw_stream);
>> +	if (ret < 0)
>> +		goto release_stream;

[1]

>> +	return 0;
>> +
>> +release_stream:
>> +	sdw_release_stream(sdw_stream);
>> +	set_stream(substream, NULL);
>> +error:
>> +	kfree(name);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(sdw_startup_stream);