Ro volatile byte kcontrol #2368

RDharageswari · 2020-08-15T04:23:14Z

This patch set allows a user to create Readonly and writeonly kcontrols. It also adds the snd_sof_bytes_ext_get_pm IO control to get the "actual values" of the requested parameters from DSP. These patches mainly help for the tuning purpose.

bardliao

Some comments based on SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE = (SNDRV_CTL_ELEM_ACCESS_TLV_READ|SNDRV_CTL_ELEM_ACCESS_TLV_WRITE), and checkpatch.pl errors

sound/soc/soc-topology.c

sound/soc/sof/sof-audio.h

sound/soc/sof/control.c

sound/soc/soc-topology.c

sound/soc/sof/control.c

ranj063 · 2020-08-17T23:00:53Z

sound/soc/sof/control.c

+					    SOF_CTRL_TYPE_DATA_GET, scontrol->cmd, false);
+
+	/* Prevent read of other kernel data or possibly corrupt response */
+	data_size = cdata->data->size + sizeof(const struct sof_abi_hdr);


should you also include the size of tlv header in this calculation?

Hi Ranjani,
I dont think TLV header is needed here. For get params IPC, fw sends the response as sof_abi_hdr + model data.
cdata->data->size is just the model data size. So, for the condition check here, cdata->data->size + sizeof(const struct sof_abi_hdr) becomes mandatory(data_size > be->max) be->max is from topology the max control size.

data_size will just hold the actual data size from the firmware(ABI_header + model_data).
The application reading data while allocating the buffer to read will add the size for tlv header.
eg: sof-ctl:
/*
* Allocate buffer for tlv write/read. The buffer needs a two
* words header with tag (SOF_CTRL_CMD_BINARY) and size in bytes.
*/
buffer_size = ctl_data->ctrl_size + 2 * sizeof(unsigned int);

thanks for the explanation @RDharageswari .In that case the check looks fine.

don't know how paranoid we want to be here, but in principle you have an overflow possibility here. If cdata->data->size is returned by the FW as a small negative number (-1), it'll pass the test, but obviously that's invalid.

kv2019i

Thanks @RDharageswari A couple of notes/questions inline.

sound/soc/soc-topology.c

sound/soc/sof/control.c

ranj063 · 2020-08-20T13:21:04Z

sound/soc/soc-topology.c


 	/* vendor specific handlers found ? */
-	if (k->put && k->get && k->info)
+	/* RW controls need to have both get/put IO handlers */


@RDharageswari these checks are for the io_ops and not for the ext_ops right? Do we really need to check these here?

Removed the handling here

sound/soc/sof/control.c

This patch adds support for write-only and read-only TLV byte kcontrols by checking for appropriate get/put IO handlers. Signed-off-by: Dharageswari R <dharageswari.r@intel.com>

ranj063

Looks good now @RDharageswari . Thanks!

kv2019i

Looks good now, thanks @RDharageswari ! One CI fail, but that seems unrelated (BDW version not found -> need to debug this separately as a CI issue).

ranj063 · 2020-08-21T17:09:20Z

@bardliao you good with the changes now?

lyakh · 2020-08-24T11:12:41Z

sound/soc/sof/control.c

+					    SOF_CTRL_TYPE_DATA_GET, scontrol->cmd, false);
+
+	/* Prevent read of other kernel data or possibly corrupt response */
+	data_size = cdata->data->size + sizeof(const struct sof_abi_hdr);


don't know how paranoid we want to be here, but in principle you have an overflow possibility here. If cdata->data->size is returned by the FW as a small negative number (-1), it'll pass the test, but obviously that's invalid.

lyakh · 2020-08-24T11:15:34Z

sound/soc/sof/control.c

+	struct sof_ipc_ctrl_data *cdata = scontrol->control_data;
+	struct snd_ctl_tlv header;
+	struct snd_ctl_tlv __user *tlvd = (struct snd_ctl_tlv __user *)binary_data;
+	int data_size;


lyakh · 2020-08-25T08:09:45Z

sound/soc/sof/control.c

+	data_size = cdata->data->size + sizeof(const struct sof_abi_hdr);
+
+	/* check data size doesn't exceed max coming from topology */
+	if (data_size > be->max) {


the point I was trying to make above is, that this test is incomplete. You're writing, that you want to check for corrupted data, but if, as I suggested, cdata->data->size == (uint32_t)-1, which is invalid, you won't catch it. Please have a look at #2383

This patch implements the snd_sof_bytes_ext_volatile_get() to read the actual parameters from DSP by sending the SOF_IPC_COMP_GET_DATA IPC for the kcontrol of type SOF_TPLG_KCTL_BYTES_VOLATILE_RO. Signed-off-by: Dharageswari R <dharageswari.r@intel.com>

RDharageswari requested review from bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, lyakh, plbossart, ranj063 and singalsu as code owners August 15, 2020 04:23

RDharageswari assigned sathya-nujella and unassigned sathya-nujella Aug 15, 2020

RDharageswari force-pushed the RO_volatile_byte_kcontrol branch from 0b5c5b7 to 63e526d Compare August 17, 2020 07:11

bardliao requested changes Aug 17, 2020

View reviewed changes

sound/soc/soc-topology.c Outdated Show resolved Hide resolved

sound/soc/sof/sof-audio.h Outdated Show resolved Hide resolved

sound/soc/sof/control.c Outdated Show resolved Hide resolved