linux-kernel - Re: [PATCH v5 11/32] sound: usb: card: Introduce USB SND platform op callbacks

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: <c32e6983-4641-1849-cb25-d29da27bff15@quicinc.com>
Date:   Mon, 11 Sep 2023 10:57:38 -0700
From:   Wesley Cheng <quic_wcheng@...cinc.com>
To:     Takashi Iwai <tiwai@...e.de>
CC:     <srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
        <perex@...ex.cz>, <lgirdwood@...il.com>, <andersson@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <gregkh@...uxfoundation.org>,
        <Thinh.Nguyen@...opsys.com>, <broonie@...nel.org>,
        <bgoswami@...cinc.com>, <tiwai@...e.com>, <robh+dt@...nel.org>,
        <agross@...nel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <alsa-devel@...a-project.org>,
        <devicetree@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <quic_jackp@...cinc.com>, <quic_plai@...cinc.com>
Subject: Re: [PATCH v5 11/32] sound: usb: card: Introduce USB SND platform op
 callbacks

Hi Takashi,

On 9/7/2023 8:36 AM, Takashi Iwai wrote:
> On Tue, 29 Aug 2023 23:06:36 +0200,
> Wesley Cheng wrote:
>>
>> Allow for different platforms to be notified on USB SND connect/disconnect
>> seqeunces.  This allows for platform USB SND modules to properly initialize
>> and populate internal structures with references to the USB SND chip
>> device.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
>> ---
>>   sound/usb/card.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   sound/usb/card.h |  9 +++++++++
>>   2 files changed, 54 insertions(+)
>>
>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>> index 1b2edc0fd2e9..067a1e82f4bf 100644
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
>> @@ -118,6 +118,34 @@ MODULE_PARM_DESC(skip_validation, "Skip unit descriptor validation (default: no)
>>   static DEFINE_MUTEX(register_mutex);
>>   static struct snd_usb_audio *usb_chip[SNDRV_CARDS];
>>   static struct usb_driver usb_audio_driver;
>> +static struct snd_usb_platform_ops *platform_ops;
>> +
>> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&register_mutex);
>> +	if (platform_ops) {
>> +		ret = -EEXIST;
>> +		goto out;
>> +	}
>> +
>> +	platform_ops = ops;
>> +out:
>> +	mutex_unlock(&register_mutex);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
> 
> For adding this kind of API, please give the proper comment.
> Especially this API is special and need a caution, to mention that it
> can be used only for a single instance.
> 
> Also, it should be mentioned that all callbacks are exclusive under
> the global register_mutex.
> 

Thanks for taking the time to review.  Sure, I'll add some comments in 
these new APIs to document what they are used for and how they are 
protected and limited.

>> @@ -910,7 +938,11 @@ static int usb_audio_probe(struct usb_interface *intf,
>>   	chip->num_interfaces++;
>>   	usb_set_intfdata(intf, chip);
>>   	atomic_dec(&chip->active);
>> +
>> +	if (platform_ops && platform_ops->connect_cb)
>> +		platform_ops->connect_cb(chip);
>>   	mutex_unlock(&register_mutex);
> 
> One uncertain thing is the argument for connect_cb and disconnect_cb.
> Those take snd_usb_audio object, but the callback gets called per
> interface at each probe and disconnect.  How does the callee handle
> multiple calls?

I guess it should depend on how the platform driver wants to handle it? 
  I haven't run into a device with multiple UAC interfaces before, so 
I'll need to mimic this configuration on a device, so I can see how it 
exposes itself.

Will investigate this a bit more on my end and come back with my findings.

> 
> Last but not least, the patch subject should be with "ALSA:" prefix,
> and in this case, at best "ALSA: usb-audio: xxx".
> 
> 

Got it, thanks!

Thanks
Wesley Cheng