diff mbox series

[v6,3/4] soundwire: qcom: Add set_channel_map api support

Message ID 20250206112225.3270400-4-quic_mohs@quicinc.com
State Accepted
Commit 7796c97df6b1b2206681a07f3c80f6023a6593d5
Headers show
Series Add static channel mapping between soundwire master and slave | expand

Commit Message

Mohammad Rafi Shaik Feb. 6, 2025, 11:22 a.m. UTC 
Added qcom_swrm_set_channel_map api to set the master channel mask for
TX and RX paths based on the provided slots.

Added a new field ch_mask to the qcom_swrm_port_config structure.
This field is used to store the master channel mask, which allows more
flexible to configure channel mask in runtime for specific active
soundwire ports.

Modified the qcom_swrm_port_enable function to configure master
channel mask. If the ch_mask is set to SWR_INVALID_PARAM or is zero,
the function will use the default channel mask.

Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
Acked-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/soundwire/qcom.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Yongqin Liu May 8, 2025, 7:18 a.m. UTC | #1 
Hi, Mohammad

On Mon, 10 Feb 2025 at 11:30, Jie Gan <jie.gan@oss.qualcomm.com> wrote:
>
>
>
> On 2/6/2025 7:22 PM, Mohammad Rafi Shaik wrote:
> > Added qcom_swrm_set_channel_map api to set the master channel mask for
> > TX and RX paths based on the provided slots.
> >
> > Added a new field ch_mask to the qcom_swrm_port_config structure.
> > This field is used to store the master channel mask, which allows more
> > flexible to configure channel mask in runtime for specific active
> > soundwire ports.
> >
> > Modified the qcom_swrm_port_enable function to configure master
> > channel mask. If the ch_mask is set to SWR_INVALID_PARAM or is zero,
> > the function will use the default channel mask.
> >
> > Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
> > Acked-by: Vinod Koul <vkoul@kernel.org>
> > ---

There is one "UBSAN: array index out of bounds" kernel panic reported
by one of our db845c Android builds,
the kernel panic is something like the following:
    [   34.478844][   T12] CPU: 1 UID: 0 PID: 12 Comm: kworker/u32:0
Tainted: G        W   E       6.15.0-rc5-mainline-g3d5bad71a798-4k #1
PREEMPT  6c0487950a65cef6b999d2d67be1f493cc9d9cb7
    [   34.495133][   T12] Tainted: [W]=WARN, [E]=UNSIGNED_MODULE
    [   34.500663][   T12] Hardware name: Thundercomm Dragonboard 845c (DT)
    [   34.507072][   T12] Workqueue: events_unbound deferred_probe_work_func
    [   34.513667][   T12] pstate: 20400005 (nzCv daif +PAN -UAO -TCO
-DIT -SSBS BTYPE=--)
    [   34.521392][   T12] pc : qcom_swrm_set_channel_map+0x208/0x210
[soundwire_qcom]
    [   34.528776][   T12] lr : snd_soc_dai_set_channel_map+0x50/0x8c
    [   34.534670][   T12] sp : ffffffc0800b3910
    [   34.538715][   T12] x29: ffffffc0800b3910 x28: ffffff808e65d090
x27: ffffffed1b8edec8
    [   34.546622][   T12] x26: ffffffed1b8edec8 x25: ffffff808c579c80
x24: ffffff808c6ff480
    [   34.554527][   T12] x23: 0000000000000002 x22: ffffff808cebea80
x21: ffffff808e639080
    [   34.562433][   T12] x20: ffffff808e63a880 x19: ffffff808cebea80
x18: ffffffc0800b50a0
    [   34.570334][   T12] x17: 00000000e068a532 x16: 00000000e068a532
x15: 00000000035fc49d
    [   34.578241][   T12] x14: 000000003b9243a6 x13: 00000000ec1dbcad
x12: ffffff93e1f38000
    [   34.586145][   T12] x11: 0000009b0000009a x10: 0000008100000080
x9 : 000000000000008e
    [   34.594051][   T12] x8 : ffffff808e463080 x7 : 0000000000000000
x6 : ffffffed1a0779c0
    [   34.601951][   T12] x5 : ffffff808034ba80 x4 : ffffffc0800b3970
x3 : 000000000000000d
    [   34.609857][   T12] x2 : ffffffc0800b3930 x1 : 0000000000000010
x0 : ffffff808cebea80
    [   34.617763][   T12] Call trace:
    [   34.620944][   T12]  qcom_swrm_set_channel_map+0x208/0x210
[soundwire_qcom e3a7c79ee66106e972319e98e81c50cf82a5307f] (P)
    [   34.631908][   T12]  sdm845_dai_init+0x1d8/0x2f4
[snd_soc_sdm845 1c888555a3e29ffd70a1064d1f53d421696609ba]
    [   34.641644][   T12]  snd_soc_link_init+0x48/0x88
    [   34.646314][   T12]  snd_soc_bind_card+0x734/0xbc4
    [   34.651157][   T12]  snd_soc_register_card+0xf8/0x110
    [   34.656261][   T12]  devm_snd_soc_register_card+0x54/0xa0
    [   34.661709][   T12]  sdm845_snd_platform_probe+0x13c/0x144
[snd_soc_sdm845 1c888555a3e29ffd70a1064d1f53d421696609ba]
    [   34.672314][   T12]  platform_probe+0xa8/0xe8
    [   34.676715][   T12]  really_probe+0x11c/0x45c
    [   34.681116][   T12]  __driver_probe_device+0xac/0x168
    [   34.686219][   T12]  driver_probe_device+0x44/0x1b4
    [   34.691150][   T12]  __device_attach_driver+0x108/0x184
    [   34.696426][   T12]  bus_for_each_drv+0x114/0x170
    [   34.701183][   T12]  __device_attach+0xc8/0x1a8
    [   34.705757][   T12]  device_initial_probe+0x1c/0x2c
    [   34.710678][   T12]  bus_probe_device+0x9c/0x128
    [   34.715341][   T12]  deferred_probe_work_func+0xc8/0x134
    [   34.720706][   T12]  process_one_work+0x26c/0x614
    [   34.725462][   T12]  worker_thread+0x268/0x3b8
    [   34.729954][   T12]  kthread+0x164/0x294
    [   34.733923][   T12]  ret_from_fork+0x10/0x20
    [   34.738251][   T12] Code: 39190109 54000061 2a1f03e0 d65f03c0 (d42aa240)
    [   34.745094][   T12] ---[ end trace 0000000000000000 ]---
    [   34.750456][   T12] Kernel panic - not syncing: UBSAN: array
index out of bounds: Fatal exception
    [   34.759398][   T12] SMP: stopping secondary CPUs
    [   34.964362][   T12] Kernel Offset: 0x2c99200000 from 0xffffffc080000000
    [   34.971037][   T12] PHYS_OFFSET: 0x80000000
    [   34.975258][   T12] CPU features: 0x0000,00000248,01002650,8200721b
    [   34.981581][   T12] Memory Limit: none

With my investigation, IIUC, it seems related to the following tree variables:
    #define QCOM_SDW_MAX_PORTS 14
    #define SLIM_MAX_TX_PORTS 16
    #define SLIM_MAX_RX_PORTS 13

QCOM_SDW_MAX_PORTS is used to declare the pconfig array,
SLIM_MAX_TX_PORTS and SLIM_MAX_RX_PORTS
are used to declare the rx_ch and tx_ch arrays in the sdm845_dai_init
function, which are finally passed
to qcom_swrm_set_channel_map as the rx_slot and tx_slot,

And I could confirm that the panic is not reported if this commit is reverted.
I also tried to add one debug line to print the value of the related
numbers in qcom_swrm_set_channel_map,
and here is the output
    [ 34.143174][ T115] drivers/soundwire/qcom.c 1287
qcom_swrm_set_channel_map tx_num=16, rx_num=13, QCOM_SDW_MAX_PORTS=14,
ARRAY_SIZE(ctrl->pconfig)=15

which could help to confirm the "array index out of bounds" error as well.

Could you please help to have a check, and give some suggestions?

Thanks,
Yongqin Liu


> >   drivers/soundwire/qcom.c | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> > index 0f45e3404756..295a46dc2be7 100644
> > --- a/drivers/soundwire/qcom.c
> > +++ b/drivers/soundwire/qcom.c
> > @@ -156,6 +156,7 @@ struct qcom_swrm_port_config {
> >       u8 word_length;
> >       u8 blk_group_count;
> >       u8 lane_control;
> > +     u8 ch_mask;
> >   };
> >
> >   /*
> > @@ -1048,9 +1049,13 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus,
> >   {
> >       u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
> >       struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> > +     struct qcom_swrm_port_config *pcfg;
> >       u32 val;
> >
> > +     pcfg = &ctrl->pconfig[enable_ch->port_num];
> >       ctrl->reg_read(ctrl, reg, &val);
> > +     if (pcfg->ch_mask != SWR_INVALID_PARAM && pcfg->ch_mask != 0)
> > +             enable_ch->ch_mask = pcfg->ch_mask;
> >
> >       if (enable_ch->enable)
> >               val |= (enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
> > @@ -1270,6 +1275,26 @@ static void *qcom_swrm_get_sdw_stream(struct snd_soc_dai *dai, int direction)
> >       return ctrl->sruntime[dai->id];
> >   }
> >
> > +static int qcom_swrm_set_channel_map(struct snd_soc_dai *dai,
> > +                                  unsigned int tx_num, const unsigned int *tx_slot,
> > +                                  unsigned int rx_num, const unsigned int *rx_slot)
> > +{
> > +     struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
> > +     int i;
> > +
> > +     if (tx_slot) {
> > +             for (i = 0; i < tx_num; i++)
> > +                     ctrl->pconfig[i].ch_mask = tx_slot[i];
> > +     }
> > +
> > +     if (rx_slot) {
> > +             for (i = 0; i < rx_num; i++)
> > +                     ctrl->pconfig[i].ch_mask = rx_slot[i];
> > +     }
> > +
> It looks like a hack.
> Consider the situation: if(tx_slot) is true and if(rx_slot) is true. So
> the ch_mask always overwritten by rx_slot?
>
> > +     return 0;
> I think you dont need the return value here. Just void is ok.
>
> Thanks,
> Jie
>
> > +}
> > +
> >   static int qcom_swrm_startup(struct snd_pcm_substream *substream,
> >                            struct snd_soc_dai *dai)
> >   {
> > @@ -1306,6 +1331,7 @@ static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = {
> >       .shutdown = qcom_swrm_shutdown,
> >       .set_stream = qcom_swrm_set_sdw_stream,
> >       .get_stream = qcom_swrm_get_sdw_stream,
> > +     .set_channel_map = qcom_swrm_set_channel_map,
> >   };
> >
> >   static const struct snd_soc_component_driver qcom_swrm_dai_component = {
>
>


--
Best Regards,
Yongqin Liu
---------------------------------------------------------------
#mailing list
linaro-android@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-android
diff mbox series

Patch

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 0f45e3404756..295a46dc2be7 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -156,6 +156,7 @@  struct qcom_swrm_port_config {
 	u8 word_length;
 	u8 blk_group_count;
 	u8 lane_control;
+	u8 ch_mask;
 };
 
 /*
@@ -1048,9 +1049,13 @@  static int qcom_swrm_port_enable(struct sdw_bus *bus,
 {
 	u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
 	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+	struct qcom_swrm_port_config *pcfg;
 	u32 val;
 
+	pcfg = &ctrl->pconfig[enable_ch->port_num];
 	ctrl->reg_read(ctrl, reg, &val);
+	if (pcfg->ch_mask != SWR_INVALID_PARAM && pcfg->ch_mask != 0)
+		enable_ch->ch_mask = pcfg->ch_mask;
 
 	if (enable_ch->enable)
 		val |= (enable_ch->ch_mask << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
@@ -1270,6 +1275,26 @@  static void *qcom_swrm_get_sdw_stream(struct snd_soc_dai *dai, int direction)
 	return ctrl->sruntime[dai->id];
 }
 
+static int qcom_swrm_set_channel_map(struct snd_soc_dai *dai,
+				     unsigned int tx_num, const unsigned int *tx_slot,
+				     unsigned int rx_num, const unsigned int *rx_slot)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+	int i;
+
+	if (tx_slot) {
+		for (i = 0; i < tx_num; i++)
+			ctrl->pconfig[i].ch_mask = tx_slot[i];
+	}
+
+	if (rx_slot) {
+		for (i = 0; i < rx_num; i++)
+			ctrl->pconfig[i].ch_mask = rx_slot[i];
+	}
+
+	return 0;
+}
+
 static int qcom_swrm_startup(struct snd_pcm_substream *substream,
 			     struct snd_soc_dai *dai)
 {
@@ -1306,6 +1331,7 @@  static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = {
 	.shutdown = qcom_swrm_shutdown,
 	.set_stream = qcom_swrm_set_sdw_stream,
 	.get_stream = qcom_swrm_get_sdw_stream,
+	.set_channel_map = qcom_swrm_set_channel_map,
 };
 
 static const struct snd_soc_component_driver qcom_swrm_dai_component = {