Re: [PATCH v4 04/32] usb: host: xhci-mem: Cleanup pending secondary event ring events

Re: [PATCH v4 04/32] usb: host: xhci-mem: Cleanup pending secondary event ring events

[Mathias Nyman]

On 25.7.2023 5.33, Wesley Cheng wrote:
> As part of xHCI bus suspend, the XHCI is halted.  However, if there are
> pending events in the secondary event ring, it is observed that the xHCI
> controller stops responding to further commands upon host or device
> initiated bus resume.  Iterate through all pending events and updating the
> dequeue pointer to the last pending event trb.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>   drivers/usb/host/xhci-mem.c | 74 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 69 insertions(+), 5 deletions(-)

This sounds more like ring handling code.
Maybe xhci-ring.c would be a better place

> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index c51150af22f2..6b01d56c176f 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1799,17 +1799,85 @@ int xhci_alloc_erst(struct xhci_hcd *xhci,
>   	return 0;
>   }
>   
> +static void xhci_handle_sec_intr_events(struct xhci_hcd *xhci,
> +	struct xhci_ring *ring,	struct xhci_intr_reg __iomem *ir_set,
> +	struct xhci_erst *erst)
> +{

The function name is a bit misleading as it doesn't handle
any of the pending events, it just marks them all handled.

> +	union xhci_trb *erdp_trb, *current_trb;
> +	struct xhci_segment	*seg;
> +	u64 erdp_reg;
> +	u32 iman_reg;
> +	dma_addr_t deq;
> +	unsigned long segment_offset;
> +
> +	/* disable irq, ack pending interrupt and ack all pending events */
> +	iman_reg = readl_relaxed(&ir_set->irq_pending);
> +	iman_reg &= ~IMAN_IE;
> +	writel_relaxed(iman_reg, &ir_set->irq_pending);
> +	iman_reg = readl_relaxed(&ir_set->irq_pending);
> +	if (iman_reg & IMAN_IP)
> +		writel_relaxed(iman_reg, &ir_set->irq_pending);

maybe use xhci_disable_interrupter() helper, it does most of this already.

> +
> +	/* last acked event trb is in erdp reg  */
> +	erdp_reg = xhci_read_64(xhci, &ir_set->erst_dequeue);
> +	deq = (dma_addr_t)(erdp_reg & ~ERST_PTR_MASK);
> +	if (!deq) {
> +		xhci_dbg(xhci, "event ring handling not required\n");
> +		return;
> +	}
> +
> +	seg = ring->first_seg;
> +	segment_offset = deq - seg->dma;
> +
> +	/* find out virtual address of the last acked event trb */
> +	erdp_trb = current_trb = &seg->trbs[0] +
> +				(segment_offset/sizeof(*current_trb));
> +
> +	/* read cycle state of the last acked trb to find out CCS */
> +	ring->cycle_state = le32_to_cpu(current_trb->event_cmd.flags) & TRB_CYCLE;
> +
> +	while (1) {
> +		/* last trb of the event ring: toggle cycle state */
> +		if (current_trb == &seg->trbs[TRBS_PER_SEGMENT - 1]) {
> +			ring->cycle_state ^= 1;
> +			current_trb = &seg->trbs[0];
> +		} else {
> +			current_trb++;
> +		}
> +
> +		/* cycle state transition */
> +		if ((le32_to_cpu(current_trb->event_cmd.flags) & TRB_CYCLE) !=
> +		    ring->cycle_state)
> +			break;
> +	}
> +
> +	if (erdp_trb != current_trb) {
> +		deq = xhci_trb_virt_to_dma(ring->deq_seg, current_trb);
> +		if (deq == 0)
> +			xhci_warn(xhci,
> +				"WARN invalid SW event ring dequeue ptr.\n");
> +		/* Update HC event ring dequeue pointer */
> +		erdp_reg &= ERST_PTR_MASK;
> +		erdp_reg |= ((u64) deq & (u64) ~ERST_PTR_MASK);
> +	}
> +
> +	/* Clear the event handler busy flag (RW1C); event ring is empty. */
> +	erdp_reg |= ERST_EHB;
> +	xhci_write_64(xhci, erdp_reg, &ir_set->erst_dequeue);

There are some helpers like inc_deq() and  xhci_update_erst_dequeue()
that could be used here.

> +}
> +
>   static void
>   xhci_free_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
>   {
>   	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
>   	size_t erst_size;
> -	u64 tmp64;
>   	u32 tmp;
>   
>   	if (!ir)
>   		return;
>   
> +	xhci_handle_sec_intr_events(xhci, ir->event_ring, ir->ir_set, &ir->erst);

Cleaning up the interrupter event ring should be called earlier.
  
Probably from xhci_remove_secondary_interrupter(), before it calls xhci_free_interrupter()

Thanks
-Mathias



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 08/32] ASoC: qcom: qdsp6: Introduce USB AFE port to q6dsp

[Wesley Cheng]

Hi Pierre,

On 7/25/2023 1:27 AM, Pierre-Louis Bossart wrote:
> 
>> +static const struct snd_soc_dai_ops q6usb_ops = {
>> +	.prepare	= q6afe_dai_prepare,
>> +	.hw_params	= q6usb_hw_params,
>> +	.shutdown	= q6afe_dai_shutdown,
> 
> it's a bit odd to see a .shutdown without a .startup?
> 
> Is this intentional and should a comment be added?
> 
> 

That is correct, because the Q6AFE port start command needs to also pass 
in the PCM params, so we wait for the hw_params() callback before 
actually starting the port.  I will add a comment.

>> +/* device token of actual end USB aduio device */
> 
> audio
> 
>> +	u32                  dev_token;
>> +/* endianness of this interface */
>> +	u32                   endian;
>> +/* service interval */
>> +	u32                  service_interval;
>> +} __packed;
>> +
>> +/**
>> + * struct afe_param_id_usb_audio_dev_params
>> + * @cfg_minor_version: Minor version used for tracking USB audio device
>> + * configuration.
>> + * Supported values:
>> + *     AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG
>> + * @dev_token: device token of actual end USB aduio device
> 
> audio. please run a spell-checker.
> 
> 

Will fix the typos.

>> +	svc_int.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> +	svc_int.svc_interval = pcfg->usb_cfg.service_interval;
>> +	ret = q6afe_port_set_param_v2(port, &svc_int,
>> +				      AFE_PARAM_ID_USB_AUDIO_SVC_INTERVAL,
>> +				      AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(svc_int));
>> +	if (ret) {
>> +		dev_err(port->afe->dev, "%s: AFE device param cmd svc_interval failed %d\n",
>> +			__func__, ret);
>> +		ret = -EINVAL;
> 
> why do you override the return value?
> 
>> +		goto exit;
> 
> not necessary, this is a jump to the next line. Looks like copy-paste ...
> 

Thanks, will fix.

>> +	}
>> +exit:
>> +	return ret;
>> +}
>> +
>> +/**
>> + * q6afe_usb_port_prepare() - Prepare usb afe port.
>> + *
>> + * @port: Instance of afe port
>> + * @cfg: USB configuration for the afe port
>> + *
>> + */
>> +void q6afe_usb_port_prepare(struct q6afe_port *port,
>> +			     struct q6afe_usb_cfg *cfg)
>> +{
>> +	union afe_port_config *pcfg = &port->port_cfg;
>> +
>> +	pcfg->usb_cfg.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> +	pcfg->usb_cfg.sample_rate = cfg->sample_rate;
>> +	pcfg->usb_cfg.num_channels = cfg->num_channels;
>> +	pcfg->usb_cfg.bit_width = cfg->bit_width;
>> +
>> +	afe_port_send_usb_dev_param(port, cfg);
>> +}
>> +EXPORT_SYMBOL_GPL(q6afe_usb_port_prepare);
>> +
>>   /**
>>    * q6afe_hdmi_port_prepare() - Prepare hdmi afe port.
>>    *
>> @@ -1611,7 +1791,10 @@ struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id)
>>   		break;
>>   	case AFE_PORT_ID_WSA_CODEC_DMA_RX_0 ... AFE_PORT_ID_RX_CODEC_DMA_RX_7:
>>   		cfg_type = AFE_PARAM_ID_CODEC_DMA_CONFIG;
>> -	break;
>> +		break;
>> +	case AFE_PORT_ID_USB_RX:
>> +		cfg_type = AFE_PARAM_ID_USB_AUDIO_CONFIG;
>> +		break;
>>   	default:
>>   		dev_err(dev, "Invalid port id 0x%x\n", port_id);
>>   		return ERR_PTR(-EINVAL);
>> diff --git a/sound/soc/qcom/qdsp6/q6afe.h b/sound/soc/qcom/qdsp6/q6afe.h
>> index 30fd77e2f458..e098a3e15135 100644
>> --- a/sound/soc/qcom/qdsp6/q6afe.h
>> +++ b/sound/soc/qcom/qdsp6/q6afe.h
>> @@ -5,7 +5,7 @@
>>   
>>   #include <dt-bindings/sound/qcom,q6afe.h>
>>   
>> -#define AFE_PORT_MAX		129
>> +#define AFE_PORT_MAX		130
>>   
>>   #define MSM_AFE_PORT_TYPE_RX 0
>>   #define MSM_AFE_PORT_TYPE_TX 1
>> @@ -205,6 +205,47 @@ struct q6afe_cdc_dma_cfg {
>>   	u16	active_channels_mask;
>>   };
>>   
>> +/**
>> + * struct q6afe_usb_cfg
>> + * @cfg_minor_version: Minor version used for tracking USB audio device
>> + * configuration.
>> + * Supported values:
>> + *     AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG
>> + * @sample_rate: Sampling rate of the port
>> + *    Supported values:
>> + *      AFE_PORT_SAMPLE_RATE_8K
>> + *      AFE_PORT_SAMPLE_RATE_11025
>> + *      AFE_PORT_SAMPLE_RATE_12K
>> + *      AFE_PORT_SAMPLE_RATE_16K
>> + *      AFE_PORT_SAMPLE_RATE_22050
>> + *      AFE_PORT_SAMPLE_RATE_24K
>> + *      AFE_PORT_SAMPLE_RATE_32K
>> + *      AFE_PORT_SAMPLE_RATE_44P1K
>> + *      AFE_PORT_SAMPLE_RATE_48K
>> + *      AFE_PORT_SAMPLE_RATE_96K
>> + *      AFE_PORT_SAMPLE_RATE_192K
>> + * @bit_width: Bit width of the sample.
>> + *    Supported values: 16, 24
>> + * @num_channels: Number of channels
>> + *    Supported values: 1, 2
>> + * @data_format: Data format supported by the USB
>> + *    Supported values: 0
>> + * @reserved: this field must be 0
>> + * @dev_token: device token of actual end USB audio device
>> + * @endian: endianness of this interface
>> + * @service_interval: service interval
>> + **/
>> +struct q6afe_usb_cfg {
>> +	u32	cfg_minor_version;
>> +	u32     sample_rate;
>> +	u16	bit_width;
>> +	u16	num_channels;
>> +	u16	data_format;
>> +	u16	reserved;
>> +	u32	dev_token;
>> +	u32	endian;
>> +	u32	service_interval;
>> +};
> 
> this definition looks exactly the same as
> struct afe_param_id_usb_cfg
> ??
> 
> 

I'll remove some of the params that we aren't utilizing.

Thanks
Wesley Cheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 10/32] ASoC: qcom: Add USB backend ASoC driver for Q6

[Wesley Cheng]

Hi Pierre,

On 7/25/2023 1:45 AM, Pierre-Louis Bossart wrote:
> 
>> +struct q6usb_port_data {
>> +	struct q6afe_usb_cfg usb_cfg;
>> +	struct snd_soc_usb *usb;
>> +	struct q6usb_offload priv;
>> +	int active_idx;
> 
> what is an 'active_idx' ?
> 
> 

active_idx carries the USB sound card we're going to be offloading.

>> +static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>> +			int connected)
>> +{
>> +	struct snd_soc_dapm_context *dapm;
>> +	struct q6usb_port_data *data;
>> +
>> +	dapm = snd_soc_component_get_dapm(usb->component);
>> +	data = dev_get_drvdata(usb->component->dev);
> 
> shouldn't you test that 'dapm' and 'data' are not NULL ?
> 

q6usb_component_probe() would be the one that registers to SOC USB to 
add this callback.  At that time, the component's dev and dapm 
references should be populated, so that should ensure that those are 
valid.  However, we could see that usb->component to be NULL, as that 
assignment happens after adding the port.  Instead I will add a check 
for usb->component before attempting to access the dapm/data params.

Another thing I will modify is to add a component removal callback, 
which will remove the SOC USB port.  That will ensure that no 
connection_cb() events are issued, so we don't run into any NULL pointer 
issues during the remove path.

>> +
>> +	if (connected) {
> 
> this goes back to my earlier comment that you treat 'connected' as a
> boolean.
> 

Done, changed to boolean.

>> +		snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>> +		/* We only track the latest USB headset plugged in */
>> +		data->active_idx = card_idx;
>> +	} else {
>> +		snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> +	}
>> +	snd_soc_dapm_sync(dapm);
>> +
>> +	return 0;
>> +}
>> +
>> +static int q6usb_component_probe(struct snd_soc_component *component)
>> +{
>> +	struct q6usb_port_data *data = dev_get_drvdata(component->dev);
>> +	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
>> +
>> +	snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> +	snd_soc_dapm_sync(dapm);
>> +
>> +	data->usb = snd_soc_usb_add_port(component->dev, &data->priv, q6usb_alsa_connection_cb);
>> +	if (IS_ERR(data->usb)) {
>> +		dev_err(component->dev, "failed to add usb port\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	data->usb->component = component;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct snd_soc_component_driver q6usb_dai_component = {
>> +	.probe = q6usb_component_probe,
> 
> erm, if you have a .probe that adds a port, don't you need a remove that
> removes the same port, and sets the pin state as well?
> 

Will add this as mentioned above.

>> +	.name = "q6usb-dai-component",
>> +	.dapm_widgets = q6usb_dai_widgets,
>> +	.num_dapm_widgets = ARRAY_SIZE(q6usb_dai_widgets),
>> +	.dapm_routes = q6usb_dapm_routes,
>> +	.num_dapm_routes = ARRAY_SIZE(q6usb_dapm_routes),
>> +	.of_xlate_dai_name = q6usb_audio_ports_of_xlate_dai_name,
>> +};
>> +
>> +static int q6usb_dai_dev_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct q6usb_port_data *data;
>> +	struct device *dev = &pdev->dev;
>> +	struct of_phandle_args args;
>> +	int ret;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_u32(node, "qcom,usb-audio-intr-num",
>> +				&data->priv.intr_num);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to read intr num.\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_parse_phandle_with_fixed_args(node, "iommus", 1, 0, &args);
>> +	if (ret < 0)
>> +		data->priv.sid = -1;
>> +	else
>> +		data->priv.sid = args.args[0] & SID_MASK;
>> +
>> +	data->priv.domain = iommu_get_domain_for_dev(&pdev->dev);
>> +
>> +	data->priv.dev = dev;
>> +	dev_set_drvdata(dev, data);
>> +
>> +	ret = devm_snd_soc_register_component(dev, &q6usb_dai_component,
>> +					q6usb_be_dais, ARRAY_SIZE(q6usb_be_dais));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> return devm_snd_soc_register_component
> 
>> +}
>> +
>> +static int q6usb_dai_dev_remove(struct platform_device *pdev)
>> +{
>> +	snd_soc_usb_remove_port(&pdev->dev);
> 
> that seems wrong, the port is added in the component probe, not the
> platform device probe.
> 

Will fix this.

Thanks
Wesley Cheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 26/32] sound: Pass USB SND card and PCM information to SOC USB

[Wesley Cheng]

Hi Pierre,

On 7/25/2023 1:59 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 7/25/23 04:34, Wesley Cheng wrote:
>> Currently, only the index to the USB SND card array is passed to the USB
>> backend.  Pass through more information, specifically the USB SND card
>> number and the number of PCM devices available.  The USB backend should
>> know about which sound resources are being shared between the ASoC and USB
>> SND paths.  This can be utilized to properly select and maintain the
>> offloading devices.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   include/sound/soc-usb.h           |  9 +++++----
>>   sound/soc/qcom/qdsp6/q6usb.c      | 20 ++++++++++++++++++--
>>   sound/soc/soc-usb.c               | 12 +++++++-----
>>   sound/usb/qcom/qc_audio_offload.c |  9 +++++----
>>   4 files changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h
>> index 71e6e75e600a..606128332044 100644
>> --- a/include/sound/soc-usb.h
>> +++ b/include/sound/soc-usb.h
>> @@ -19,20 +19,21 @@ struct snd_soc_usb {
>>   	struct device *dev;
>>   	struct snd_soc_component *component;
>>   	int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx,
>> -				int connected);
>> +				int chip_idx, int num_pcm, int connected);
> 
> I don't know what 'chip_idx' is.
> 

chip_idx is the index to the USB SND chip array which carries 
information about each udev (USB device) connected to USB SND)

> The 'num_pcm' sounds problematic if there are different devices for
> playback and capture. I would guess this is for playback only, but this
> doesn't scale.
> 

I agree.  My plan is to address this by having another SND SOC USB 
structure that is created by the USB class driver, ie qc_usb_offload, 
which would populate the required parameters.

Main thing I wanted to avoid is to have to expose the main USB SND 
structure if we're only going to only have a few variables to check for.

>>   	void *priv_data;
>>   };
> 
>> +struct q6usb_status {
>> +	unsigned int num_pcm;
>> +	unsigned int chip_index;
>> +	unsigned int pcm_index;
>> +};
>> +
>>   struct q6usb_port_data {
>>   	struct q6afe_usb_cfg usb_cfg;
>>   	struct snd_soc_usb *usb;
>>   	struct q6usb_offload priv;
>> +	unsigned long available_card_slot;
> 
> what is a card slot?
> 

Idea was just to have a bitmapped value of all offload capable audio 
devices that are currently connected.  Then we can take the necessary 
actions when everything has been disconnected.

>> +	struct q6usb_status status[SNDRV_CARDS];
>>   	int active_idx;
>>   };
>>   
>> @@ -97,7 +105,7 @@ static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone
>>   }
>>   
>>   static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>> -			int connected)
>> +			int chip_idx, int num_pcm, int connected)
>>   {
>>   	struct snd_soc_dapm_context *dapm;
>>   	struct q6usb_port_data *data;
>> @@ -109,8 +117,16 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>>   		snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>>   		/* We only track the latest USB headset plugged in */
>>   		data->active_idx = card_idx;
>> +
>> +		set_bit(card_idx, &data->available_card_slot);
>> +		data->status[card_idx].num_pcm = num_pcm;
>> +		data->status[card_idx].chip_index = chip_idx;
>>   	} else {
>> -		snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> +		clear_bit(card_idx, &data->available_card_slot);
>> +		data->status[card_idx].num_pcm = 0;
>> +		data->status[card_idx].chip_index = 0;
>> +		if (!data->available_card_slot)
>> +			snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
> 
> not able to follow what this does, this patch is rather unclear and
> lacks comments.
> 

I will add comments, but it basically will populate some of the limits 
that we'll end up using for controlling the ksndcontrols.

Thanks
Wesley Cheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 29/32] sound: soc: qcom: q6usb: Add headphone jack for offload connection status

[Wesley Cheng]

Hi Pierre,

On 7/25/2023 2:10 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 7/25/23 04:34, Wesley Cheng wrote:
>> The headphone jack framework has a well defined intrastructure for
> 
> infrastructure
> 
>> notifying userspace entities through input devices.  Expose a jack device
>> that carries information about if an offload capable device is connected.
>> Applications can further identify specific offloading information through
>> other SND kcontrols.
> 
> What if you connect a set of USB speakers? Would they show as a
> headphone/headset?
> 

For now, let me modify the patch to send a HEADPHONE event.  We don't 
support the capture/record path as of now, so it doesn't make sense to 
generate a HEADSET event (which exposes both MIC and HEADPHONE).

When you plug in any USB audio device we'd generate this snd jack event. 
  Main purpose was to notify that the offload path is potentially available.

Thanks
Wesley Cheng

>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   sound/soc/qcom/qdsp6/q6usb.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c
>> index e4ccb9d912b0..860dff8c1438 100644
>> --- a/sound/soc/qcom/qdsp6/q6usb.c
>> +++ b/sound/soc/qcom/qdsp6/q6usb.c
>> @@ -20,6 +20,7 @@
>>   #include <sound/pcm_params.h>
>>   #include <sound/asound.h>
>>   #include <sound/q6usboffload.h>
>> +#include <sound/jack.h>
>>   
>>   #include "q6dsp-lpass-ports.h"
>>   #include "q6afe.h"
>> @@ -37,6 +38,7 @@ struct q6usb_status {
>>   struct q6usb_port_data {
>>   	struct q6afe_usb_cfg usb_cfg;
>>   	struct snd_soc_usb *usb;
>> +	struct snd_soc_jack hs_jack;
>>   	struct q6usb_offload priv;
>>   	struct mutex mutex;
>>   	unsigned long available_card_slot;
>> @@ -279,6 +281,7 @@ static const struct snd_kcontrol_new q6usb_offload_control = {
>>   /* Build a mixer control for a UAC connector control (jack-detect) */
>>   static void q6usb_connector_control_init(struct snd_soc_component *component)
>>   {
>> +	struct q6usb_port_data *data = dev_get_drvdata(component->dev);
>>   	int ret;
>>   
>>   	ret = snd_ctl_add(component->card->snd_card,
>> @@ -290,6 +293,11 @@ static void q6usb_connector_control_init(struct snd_soc_component *component)
>>   				snd_ctl_new1(&q6usb_offload_dev_ctrl, component));
>>   	if (ret < 0)
>>   		return;
>> +
>> +	ret = snd_soc_card_jack_new(component->card, "USB offload",
>> +					SND_JACK_HEADSET, &data->hs_jack);
> 
> not all USB devices are headsets...
> 
>> +	if (ret)
>> +		return;
>>   }
>>   
>>   static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *component,
>> @@ -322,7 +330,10 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>>   
>>   	mutex_lock(&data->mutex);
>>   	if (connected) {
>> -		snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>> +		if (!data->available_card_slot) {
>> +			snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>> +			snd_jack_report(data->hs_jack.jack, 1);
>> +		}
>>   		/*
>>   		 * Update the latest USB headset plugged in, if session is
>>   		 * idle.
>> @@ -338,8 +349,10 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>>   		clear_bit(card_idx, &data->available_card_slot);
>>   		data->status[card_idx].num_pcm = 0;
>>   		data->status[card_idx].chip_index = 0;
>> -		if (!data->available_card_slot)
>> +		if (!data->available_card_slot) {
>>   			snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> +			snd_jack_report(data->hs_jack.jack, 0);
>> +		}
>>   	}
>>   	snd_soc_dapm_sync(dapm);
>>   	mutex_unlock(&data->mutex);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 31/32] sound: usb: card: Allow for rediscovery of connected USB SND devices

[Wesley Cheng]

Hi Pierre,

On 7/25/2023 2:15 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 7/25/23 04:34, Wesley Cheng wrote:
>> In case of notifying SND platform drivers of connection events, some of
>> these use cases, such as offloading, require an ASoC USB backend device to
>> be initialized before the events can be handled.  If the USB backend device
>> has not yet been probed, this leads to missing initial USB audio device
>> connection events.
>>
>> Expose an API that traverses the usb_chip array for connected devices, and
>> to call the respective connection callback registered to the SND platform
>> driver.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   sound/usb/card.c | 19 +++++++++++++++++++
>>   sound/usb/card.h |  2 ++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>> index 365f6d978608..27a89aaa0bf3 100644
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
>> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
>>   }
>>   EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream);
>>   
>> +/*
>> + * in case the platform driver was not ready at the time of USB SND
>> + * device connect, expose an API to discover all connected USB devices
>> + * so it can populate any dependent resources/structures.
>> + */
>> +void snd_usb_rediscover_devices(void)
>> +{
>> +	int i;
>> +
>> +	mutex_lock(&register_mutex);
>> +	for (i = 0; i < SNDRV_CARDS; i++) {
>> +		if (usb_chip[i])
>> +			if (platform_ops && platform_ops->connect_cb)
>> +				platform_ops->connect_cb(usb_chip[i]);
> 
> what happens if the USB device is removed while the platform device adds
> a port?
> 
> This sounds super-racy to me. It's the same set of problems we're having
> between audio and display/DRM, I would be surprised if this function
> dealt with all corner cases of insertion/removal, bind/unbind.
> 

The chip array entries are all populated and removed while under the 
register_mutex, so going over your race condition, we should see:

Thread#1:
q6usb_component_probe()
--> snd_soc_usb_add_port()
   --> snd_usb_rediscover_devices()
     --> mutex_lock(register_mutex)

Thread#2
--> usb_audio_disconnect()
   --> mutex_lock(register_mutex)

So either thread#1 or thread#2 will complete first.  If

Thread#1 completes before thread#2:
   SOC USB will notify DPCM backend of the device connection.  Shortly 
after, once thread#2 runs, we will get a disconnect event for the 
connected device.

Thread#2 completes before thread#1:
   Then during snd_usb_rediscover_devices() we won't notify of any 
connection for that particular chip index.

Thanks
Wesley Cheng


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 31/32] sound: usb: card: Allow for rediscovery of connected USB SND devices

[Pierre-Louis Bossart]

On 8/15/23 20:38, Wesley Cheng wrote:
> Hi Pierre,
> 
> On 7/25/2023 2:15 AM, Pierre-Louis Bossart wrote:
>>
>>
>> On 7/25/23 04:34, Wesley Cheng wrote:
>>> In case of notifying SND platform drivers of connection events, some of
>>> these use cases, such as offloading, require an ASoC USB backend
>>> device to
>>> be initialized before the events can be handled.  If the USB backend
>>> device
>>> has not yet been probed, this leads to missing initial USB audio device
>>> connection events.
>>>
>>> Expose an API that traverses the usb_chip array for connected
>>> devices, and
>>> to call the respective connection callback registered to the SND
>>> platform
>>> driver.
>>>
>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>> ---
>>>   sound/usb/card.c | 19 +++++++++++++++++++
>>>   sound/usb/card.h |  2 ++
>>>   2 files changed, 21 insertions(+)
>>>
>>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>>> index 365f6d978608..27a89aaa0bf3 100644
>>> --- a/sound/usb/card.c
>>> +++ b/sound/usb/card.c
>>> @@ -170,6 +170,25 @@ struct snd_usb_stream
>>> *snd_usb_find_suppported_substream(int card_idx,
>>>   }
>>>   EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream);
>>>   +/*
>>> + * in case the platform driver was not ready at the time of USB SND
>>> + * device connect, expose an API to discover all connected USB devices
>>> + * so it can populate any dependent resources/structures.
>>> + */
>>> +void snd_usb_rediscover_devices(void)
>>> +{
>>> +    int i;
>>> +
>>> +    mutex_lock(&register_mutex);
>>> +    for (i = 0; i < SNDRV_CARDS; i++) {
>>> +        if (usb_chip[i])
>>> +            if (platform_ops && platform_ops->connect_cb)
>>> +                platform_ops->connect_cb(usb_chip[i]);
>>
>> what happens if the USB device is removed while the platform device adds
>> a port?
>>
>> This sounds super-racy to me. It's the same set of problems we're having
>> between audio and display/DRM, I would be surprised if this function
>> dealt with all corner cases of insertion/removal, bind/unbind.
>>
> 
> The chip array entries are all populated and removed while under the
> register_mutex, so going over your race condition, we should see:
> 
> Thread#1:
> q6usb_component_probe()
> --> snd_soc_usb_add_port()
>   --> snd_usb_rediscover_devices()
>     --> mutex_lock(register_mutex)
> 
> Thread#2
> --> usb_audio_disconnect()
>   --> mutex_lock(register_mutex)
> 
> So either thread#1 or thread#2 will complete first.  If
> 
> Thread#1 completes before thread#2:
>   SOC USB will notify DPCM backend of the device connection.  Shortly
> after, once thread#2 runs, we will get a disconnect event for the
> connected device.
> 
> Thread#2 completes before thread#1:
>   Then during snd_usb_rediscover_devices() we won't notify of any
> connection for that particular chip index.

You really want to capture this locking model as part of the commit
messages or code, it'll help reviewers figure things out.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 31/32] sound: usb: card: Allow for rediscovery of connected USB SND devices

[Wesley Cheng]

Hi Takashi,

On 7/25/2023 2:27 AM, Takashi Iwai wrote:
> On Tue, 25 Jul 2023 11:15:11 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>
>> On 7/25/23 04:34, Wesley Cheng wrote:
>>> In case of notifying SND platform drivers of connection events, some of
>>> these use cases, such as offloading, require an ASoC USB backend device to
>>> be initialized before the events can be handled.  If the USB backend device
>>> has not yet been probed, this leads to missing initial USB audio device
>>> connection events.
>>>
>>> Expose an API that traverses the usb_chip array for connected devices, and
>>> to call the respective connection callback registered to the SND platform
>>> driver.
>>>
>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>> ---
>>>   sound/usb/card.c | 19 +++++++++++++++++++
>>>   sound/usb/card.h |  2 ++
>>>   2 files changed, 21 insertions(+)
>>>
>>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>>> index 365f6d978608..27a89aaa0bf3 100644
>>> --- a/sound/usb/card.c
>>> +++ b/sound/usb/card.c
>>> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
>>>   }
>>>   EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream);
>>>   
>>> +/*
>>> + * in case the platform driver was not ready at the time of USB SND
>>> + * device connect, expose an API to discover all connected USB devices
>>> + * so it can populate any dependent resources/structures.
>>> + */
>>> +void snd_usb_rediscover_devices(void)
>>> +{
>>> +	int i;
>>> +
>>> +	mutex_lock(&register_mutex);
>>> +	for (i = 0; i < SNDRV_CARDS; i++) {
>>> +		if (usb_chip[i])
>>> +			if (platform_ops && platform_ops->connect_cb)
>>> +				platform_ops->connect_cb(usb_chip[i]);
>>
>> what happens if the USB device is removed while the platform device adds
>> a port?
> 
> That should be protected by the register_mutex.  But there can be
> other races (see below :)
> 
>> This sounds super-racy to me. It's the same set of problems we're having
>> between audio and display/DRM, I would be surprised if this function
>> dealt with all corner cases of insertion/removal, bind/unbind.
> 
> Yes, we need to be more careful about binding.
> 
> For example, in the current patch set, I see no way to prevent
> unloading snd-usb-audio-qmi module, and it allows user to cut off the
> stuff during operation, which may break things while the kernel is
> running the code of the unloaded module.  You need to have a proper
> module refcount management for avoiding such a scenario.  Most of
> drivers don't need it because ALSA core part already takes care of
> it.  But in this case, it requires a manual adjustment.
> 

Sorry for the delayed response...this was routed to another folder, and 
missed it.

I see, that is a valid situation, so I think the best way to address it 
is to do something like the following:

static void __exit qc_usb_audio_offload_exit(void)
{
...
	snd_usb_unregister_platform_ops();
	for (idx = 0; idx < SNDRV_CARDS; idx++)
		qc_usb_audio_offload_disconnect(uadev[idx].chip);

We'll first unregister the platform ops, so that we get no further 
connect/disconnect CBs, and then we'll forcefully ensure that all 
devices are removed/cleaned.  Part of the USB offload disconnect 
sequence will also forcefully stop the offload path on the external DSP 
by issuing a disconnect indication QMI message.

Then we can safely clean up the rest of the resources.  We do have 
refcounting in place for several of the other structures, but I think in 
the module exit case, we need to ensure the offload path is stopped fully.

Thanks
Wesley Cheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 13/32] dt-bindings: usb: dwc3: Add snps,num-hc-interrupters definition

[Krzysztof Kozlowski]

On 25/07/2023 04:33, Wesley Cheng wrote:
> Add a new definition for specifying how many XHCI secondary interrupters
> can be allocated.  XHCI in general can potentially support up to 1024
> interrupters, which some uses may want to limit depending on how many
> users utilize the interrupters.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml          | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index 50edc4da780e..cc6012e922e0 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -376,6 +376,19 @@ properties:
>      items:
>        enum: [1, 4, 8, 16, 32, 64, 128, 256]
>  
> +  snps,num-hc-interrupters:
> +    description:
> +      Defines the maximum number of XHCI host controller interrupters that can
> +      be supported.  The XHCI host controller has support to allocate multiple
> +      event rings, which can be assigned to different clients/users.  The DWC3
> +      controller has a maximum of 8 interrupters.  If this is not defined then
> +      the value will be defaulted to 1.  This parameter is used only when
> +      operating in host mode.
> +    $ref: /schemas/types.yaml#/definitions/uint8

You did not respond to Rob's comments before sending this patch. :(

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 31/32] sound: usb: card: Allow for rediscovery of connected USB SND devices

[Takashi Iwai]

On Mon, 28 Aug 2023 23:25:14 +0200,
Wesley Cheng wrote:
> 
> Hi Takashi,
> 
> On 7/25/2023 2:27 AM, Takashi Iwai wrote:
> > On Tue, 25 Jul 2023 11:15:11 +0200,
> > Pierre-Louis Bossart wrote:
> >> 
> >> 
> >> 
> >> On 7/25/23 04:34, Wesley Cheng wrote:
> >>> In case of notifying SND platform drivers of connection events, some of
> >>> these use cases, such as offloading, require an ASoC USB backend device to
> >>> be initialized before the events can be handled.  If the USB backend device
> >>> has not yet been probed, this leads to missing initial USB audio device
> >>> connection events.
> >>> 
> >>> Expose an API that traverses the usb_chip array for connected devices, and
> >>> to call the respective connection callback registered to the SND platform
> >>> driver.
> >>> 
> >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> >>> ---
> >>>   sound/usb/card.c | 19 +++++++++++++++++++
> >>>   sound/usb/card.h |  2 ++
> >>>   2 files changed, 21 insertions(+)
> >>> 
> >>> diff --git a/sound/usb/card.c b/sound/usb/card.c
> >>> index 365f6d978608..27a89aaa0bf3 100644
> >>> --- a/sound/usb/card.c
> >>> +++ b/sound/usb/card.c
> >>> @@ -170,6 +170,25 @@ struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
> >>>   }
> >>>   EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream);
> >>>   +/*
> >>> + * in case the platform driver was not ready at the time of USB SND
> >>> + * device connect, expose an API to discover all connected USB devices
> >>> + * so it can populate any dependent resources/structures.
> >>> + */
> >>> +void snd_usb_rediscover_devices(void)
> >>> +{
> >>> +	int i;
> >>> +
> >>> +	mutex_lock(&register_mutex);
> >>> +	for (i = 0; i < SNDRV_CARDS; i++) {
> >>> +		if (usb_chip[i])
> >>> +			if (platform_ops && platform_ops->connect_cb)
> >>> +				platform_ops->connect_cb(usb_chip[i]);
> >> 
> >> what happens if the USB device is removed while the platform device adds
> >> a port?
> > 
> > That should be protected by the register_mutex.  But there can be
> > other races (see below :)
> > 
> >> This sounds super-racy to me. It's the same set of problems we're having
> >> between audio and display/DRM, I would be surprised if this function
> >> dealt with all corner cases of insertion/removal, bind/unbind.
> > 
> > Yes, we need to be more careful about binding.
> > 
> > For example, in the current patch set, I see no way to prevent
> > unloading snd-usb-audio-qmi module, and it allows user to cut off the
> > stuff during operation, which may break things while the kernel is
> > running the code of the unloaded module.  You need to have a proper
> > module refcount management for avoiding such a scenario.  Most of
> > drivers don't need it because ALSA core part already takes care of
> > it.  But in this case, it requires a manual adjustment.
> > 
> 
> Sorry for the delayed response...this was routed to another folder,
> and missed it.
> 
> I see, that is a valid situation, so I think the best way to address
> it is to do something like the following:
> 
> static void __exit qc_usb_audio_offload_exit(void)
> {
> ...
> 	snd_usb_unregister_platform_ops();
> 	for (idx = 0; idx < SNDRV_CARDS; idx++)
> 		qc_usb_audio_offload_disconnect(uadev[idx].chip);
> 
> We'll first unregister the platform ops, so that we get no further
> connect/disconnect CBs, and then we'll forcefully ensure that all
> devices are removed/cleaned.  Part of the USB offload disconnect
> sequence will also forcefully stop the offload path on the external
> DSP by issuing a disconnect indication QMI message.
> 
> Then we can safely clean up the rest of the resources.  We do have
> refcounting in place for several of the other structures, but I think
> in the module exit case, we need to ensure the offload path is stopped
> fully.

Yes, the proper disconnection procedure is mandatory.
In addition, you can have a module refcount increment upon the card
binding, too.  This may prevent the unexpected exit (although it's
often still possible to unbind via sysfs).


thanks,

Takashi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel