| 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
| ||
|
Message-ID: <8ae1928e-4b38-e7e1-162b-144a07efde9f@quicinc.com>
Date: Tue, 25 Jul 2023 16:17:57 -0700
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
<agross@...nel.org>, <andersson@...nel.org>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<catalin.marinas@....com>, <will@...nel.org>,
<mathias.nyman@...el.com>, <gregkh@...uxfoundation.org>,
<lgirdwood@...il.com>, <broonie@...nel.org>, <perex@...ex.cz>,
<tiwai@...e.com>, <srinivas.kandagatla@...aro.org>,
<bgoswami@...cinc.com>, <Thinh.Nguyen@...opsys.com>
CC: <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-usb@...r.kernel.org>, <alsa-devel@...a-project.org>,
<quic_jackp@...cinc.com>, <oneukum@...e.com>,
<albertccwang@...gle.com>, <o-takashi@...amocchi.jp>
Subject: Re: [PATCH v4 06/32] ASoC: Add SOC USB APIs for adding an USB backend
Hi Pierre,
On 7/25/2023 1:10 AM, Pierre-Louis Bossart wrote:
>
>> +/**
>> + * struct snd_soc_usb
>> + * @list - list head for SND SOC struct list
>> + * @dev - USB backend device reference
>> + * @component - reference to DAPM component
>
> ASoC component, not DAPM.
>
Thanks for the detailed review. Will fix this.
>> + * @connection_status_cb - callback to notify connection events
>> + * @priv_data - driver data
>> + **/
>> +struct snd_soc_usb {
>> + struct list_head list;
>> + struct device *dev;
>> + struct snd_soc_component *component;
>> + int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx,
>> + int connected);
>
> It's not clear what 'connected' really refers to, is this a boolean
> really or is this a "connection_event?
>
>
I'll change it to boolean for now, since its currently only
connected/disconnected. If more states are required, then we can add
that in the future.
>> + void *priv_data;
>> +};
>> +
>> +int snd_soc_usb_connect(struct device *usbdev, int card_idx);
>> +int snd_soc_usb_disconnect(struct device *usbdev);
>> +void snd_soc_usb_set_priv_data(struct device *dev, void *priv);
>
> this function is not part of this patch, is this intentional to have a
> get but not a set?
>
>> +void *snd_soc_usb_get_priv_data(struct device *usbdev);
>
> you are using 'usbdev' and 'dev' for the same type of parameters, why
> not align on one set of definition with a consistent naming.
>
>
I'll take a look at this and see what happened. I think Greg mentioned
something similar and I made the change to remove the set priv, and
moved it elsewhere.
>> +static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev)
>> +{
>> + struct device_node *node;
>> + struct snd_soc_usb *ctx = NULL;
>
> this init doesn't seem required?
>
Yes, not needed.
>> +
>> + node = snd_soc_find_phandle(dev);
>> + if (IS_ERR(node))
>> + return NULL;
>> +
>> + mutex_lock(&ctx_mutex);
>> + list_for_each_entry(ctx, &usb_ctx_list, list) {
>> + if (ctx->dev->of_node == node) {
>> + of_node_put(node);
>> + mutex_unlock(&ctx_mutex);
>> + return ctx;
>> + }
>> + }
>> + of_node_put(node);
>> + mutex_unlock(&ctx_mutex);
>> +
>> + return NULL;
>> +}
>> +
>> +/**
>> + * snd_soc_usb_get_priv_data() - Retrieve private data stored
>> + * @dev: device reference
>> + *
>> + * Fetch the private data stored in the USB SND SOC structure.
>> + *
>> + */
>> +void *snd_soc_usb_get_priv_data(struct device *dev)
>> +{
>> + struct snd_soc_usb *ctx;
>> +
>> + ctx = snd_soc_find_usb_ctx(dev);
>
> so in this function you walk through the usb_ctx_list with locking...
>
>> + if (!ctx) {
>> + /* Check if backend device */
>> + list_for_each_entry(ctx, &usb_ctx_list, list) {
>
> ... and here you walk again through the list without locking.
>
> Something's weird here, if this was intentional you need to add comments.
>
Yes, needs a lock here.
>> + if (dev->of_node == ctx->dev->of_node)
>> + goto out;
>> + }
>> + ctx = NULL;
>> + }
>> +out:
>> + return ctx ? ctx->priv_data : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_get_priv_data);
>> +
>> +/**
>> + * snd_soc_usb_add_port() - Add a USB backend port
>> + * @dev: USB backend device
>> + * @priv: private data
>> + * @connection_cb: connection status callback
>> + *
>> + * Register a USB backend device to the SND USB SOC framework. Memory is
>> + * allocated as part of the USB backend device.
>> + *
>> + */
>> +struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv,
>> + int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
>> + int connected))
>> +{
>> + struct snd_soc_usb *usb;
>> +
>> + usb = devm_kzalloc(dev, sizeof(*usb), GFP_KERNEL);
>> + if (!usb)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + usb->connection_status_cb = connection_cb;
>> + usb->dev = dev;
>> + usb->priv_data = priv;
>
> back to my comment above, you don't seem to need the set_priv_data() ?
>
And yes, this is where I moved the priv data setting.
>> +
>> + mutex_lock(&ctx_mutex);
>> + list_add_tail(&usb->list, &usb_ctx_list);
>> + mutex_unlock(&ctx_mutex);
>> +
>> + return usb;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port);
>
>> +/**
>> + * snd_soc_usb_connect() - Notification of USB device connection
>> + * @usbdev: USB bus device
>> + * @card_idx: USB SND card instance
>> + *
>> + * Notify of a new USB SND device connection. The card_idx can be used to
>> + * handle how the USB backend selects, which device to enable offloading on.
>
> "USB backend" is confusing, not sure if this is the same concept as DPCM
> "backend" or something else. Please try to avoid overloaded terms.
>
Sure, I meant DPCM backend.
>> + *
>> + */
>> +int snd_soc_usb_connect(struct device *usbdev, int card_idx)
>> +{
>> + struct snd_soc_usb *ctx;
>> +
>> + if (!usbdev)
>> + return -ENODEV;
>> +
>> + ctx = snd_soc_find_usb_ctx(usbdev);
>> + if (!ctx)
>> + return -ENODEV;
>> +
>> + if (ctx->connection_status_cb)
>> + ctx->connection_status_cb(ctx, card_idx, 1);
>
> so either the 'connected' value is 1...
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_usb_connect);
>> +
>> +/**
>> + * snd_soc_usb_disconnect() - Notification of USB device disconnection
>> + * @usbdev: USB bus device
>> + *
>> + * Notify of a new USB SND device disconnection to the USB backend.
>> + *
>> + */
>> +int snd_soc_usb_disconnect(struct device *usbdev)
>> +{
>> + struct snd_soc_usb *ctx;
>> +
>> + if (!usbdev)
>> + return -ENODEV;
>> +
>> + ctx = snd_soc_find_usb_ctx(usbdev);
>> + if (!ctx)
>> + return -ENODEV;
>> +
>> + if (ctx->connection_status_cb)
>> + ctx->connection_status_cb(ctx, -1, 0);
>
> ...and here it's zero.
>
> should this 'connected' parameter be a boolean then with true/false
> value, or do you want to add enums/defines for more flexibility down the
> road?
>
As mentioned above, will switch to boolean for now. I don't see a need
to have other enums.
Thanks
Wesley Cheng
Powered by blists - more mailing lists