| 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
| ||
|
Date: Tue, 20 Feb 2024 07:32:43 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Shenghao Ding <shenghao-ding@...com>, broonie@...nel.org
Cc: andriy.shevchenko@...ux.intel.com, lgirdwood@...il.com, perex@...ex.cz,
13916275206@....com, alsa-devel@...a-project.org,
linux-kernel@...r.kernel.org, liam.r.girdwood@...el.com,
bard.liao@...el.com, mengdong.lin@...el.com,
yung-chuan.liao@...ux.intel.com, baojun.xu@...com, kevin-lu@...com,
tiwai@...e.de, soyer@....hu
Subject: Re: [PATCH v8] ASoc: tas2783: Add tas2783 codec driver
It's starting to look good, but the use of the dev_num is conceptually
broken. The only way to reliably identify a device is the combination of
link_id and unique_id. The information is present for both ACPI and DT
systems. See comments below.
-Pierre
> +static void tas2783_apply_calib(struct tasdevice_priv *tas_dev,
> + unsigned int *cali_data)
> +{
> + struct regmap *map = tas_dev->regmap;
> + u8 *cali_start;
> + u16 dev_num;
> + int ret;
> +
> + if (!tas_dev->sdw_peripheral) {
> + dev_err(tas_dev->dev, "%s: peripheral doesn't exist.\n",
> + __func__);
> + return;
> + }
> +
> + dev_num = clamp(tas_dev->sdw_peripheral->dev_num, 1, 4) - 1;
Not following what restrictions you are trying to enforce on the device
number. That's a value selected by the manager. You absolutely cannot
assume the value is between 1 and 4, the max value is 11.
> + /*
> + * The area saving tas2783 calibrated data is specified by its
> + * dev_num. cali_start is the first address of current tas2783's
> + * calibrated data.
> + */
> + cali_start = (u8 *)(cali_data + dev_num * sizeof(tas2783_cali_reg));
> + for (int i = 0; i < ARRAY_SIZE(tas2783_cali_reg); i++) {
> + ret = regmap_bulk_write(map, tas2783_cali_reg[i],
> + &cali_start[4 * i], 4);
> + if (ret) {
> + dev_err(tas_dev->dev, "Cali failed %x:%d\n",
> + tas2783_cali_reg[i], ret);
> + break;
> + }
> + }
> +}
> +
> +/*
> + * Load the calibration data, including speaker impedance, f0, etc.
> + * Calibration is done by the manufacturer in the factory. The calibration
> + * data are used by the algorithm for calculating the speaker temperature,
> + * speaker membrane excursion and f0 in real time during playback.
> + * The DSP will work with default data values if calibrated data are
> + * missing or are invalid.
> + * Layout of calibrated Data in UEFI:
> + * Calibrated Data of Dev 0 (20 bytes)
> + * Calibrated Data of Dev 1 (20 bytes)
> + * Calibrated Data of Dev 2 (20 bytes)
> + * Calibrated Data of Dev 3 (20 bytes)
You will have a hard-time matching those device indices with the
dev_num, which depends on the enumeration order and the bus allocation.
The only stable board-specific value is to use a combination of link_id
and unique_id (possibly controller id as well).
> + * CRC (4 bytes)
> + */
> +static int tasdevice_comp_probe(struct snd_soc_component *comp)
> +{
> + struct tasdevice_priv *tas_dev = snd_soc_component_get_drvdata(comp);
> + acpi_handle handle = ACPI_HANDLE(tas_dev->dev);
> + const struct firmware *fw_entry = NULL;
> + const char *sub = NULL;
> + int ret, value_sdw;
> +
> + if (handle) {
> + sub = acpi_get_subsystem_id(handle);
> + if (IS_ERR(sub))
> + sub = NULL;
> + }
> +
> + tas_dev->component = comp;
> +
> + /*
> + * Each tas2783 in the system has its own dspfw.
> + */
> + if (comp->name_prefix) {
> + /*
> + * name_prefix.bin stores the dsp firmware including speaker
> + * protection algorithm, audio acoustic algorithm, speaker
> + * characters and algorithm params, it must be copied into
> + * firmware folder.
> + */
> + scnprintf(tas_dev->dspfw_binaryname,
> + TAS2783_DSPFW_FILENAME_LEN, "%s-tas2783.bin",
> + comp->name_prefix);
> + } else {
> + /* Compatible with the previous naming rule */
> + if (sub) {
> + /*
> + * subsystem_id-link_id[0,1,...,N]-dev_num[1,...,4].bin stores
> + * the dsp firmware including speaker protection algorithm,
> + * audio acoustic algorithm, speaker characters and algorithm
> + * params, it must be copied into firmware folder.
no the dev_num cannot be used. It's only used for host-device
communication and cannot be used to identify a device position.
The link_id+unique_unique is the only way to go.
But in addition you want want to consider a platform-specific prefix
which contains the OEM name or device SKU.
> + */
> + scnprintf(tas_dev->dspfw_binaryname,
> + TAS2783_DSPFW_FILENAME_LEN,
> + "%s-%d-%d.bin", sub,
> + tas_dev->sdw_peripheral->bus->link_id,
> + tas_dev->sdw_peripheral->dev_num);
> + } else {
> + /*
> + * tas2783-link_id[0,1,...,N]-dev_num[1,...,4].bin stores
> + * the dsp firmware including speaker protection algorithm,
> + * audio acoustic algorithm, speaker characters and algorithm
> + * params, it must be copied into firmware folder.
> + */
> + scnprintf(tas_dev->dspfw_binaryname,
> + TAS2783_DSPFW_FILENAME_LEN,
> + "tas2783-%d-%d.bin",
> + tas_dev->sdw_peripheral->bus->link_id,
> + tas_dev->sdw_peripheral->dev_num);
> + }
> + }
> +
> + ret = request_firmware(&fw_entry, tas_dev->dspfw_binaryname,
> + tas_dev->dev);
> + if (ret) {
> + dev_err(tas_dev->dev,
> + "%s: request_firmware %x open status: %d.\n", __func__,
> + tas_dev->sdw_peripheral->id.unique_id, ret);
> + goto out;
> + }
> +
> + tasdevice_dspfw_ready(fw_entry, tas_dev);
> +
> + /* Select left/right channel based on device number. */
> + value_sdw = 0x1a;
> + value_sdw |= (tas_dev->sdw_peripheral->dev_num & BIT(0)) << 4;
> + dev_dbg(tas_dev->dev, "%s: dev_num = %u", __func__,
> + tas_dev->sdw_peripheral->dev_num);
> + regmap_write(tas_dev->regmap, TAS2783_REG_TDM_RX_CFG, value_sdw);
> + if (ret != 0)
> + dev_warn(tas_dev->dev, "%s: L/R setting failed: %d.\n",
> + __func__, ret);
> +
> +out:
> + if (fw_entry)
> + release_firmware(fw_entry);
> + return 0;
> +}
> +static int tasdevice_io_init(struct device *dev,
> + struct sdw_slave *slave)
> +{
> + struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> + int ret;
> +
> + regcache_cache_only(tas_priv->regmap, false);
> +
> + if (tas_priv->first_hw_init) {
> + regcache_cache_bypass(tas_priv->regmap, true);
> + } else {
> + /*
> + * PM runtime is only enabled when a Slave reports as Attached
> + * Update count of parent 'active' children
> + */
weird indentation for comments in multiple places.
> + pm_runtime_set_active(&slave->dev);
> + }
> +
> + /* sw reset */
> + ret = regmap_write(tas_priv->regmap, TAS2873_REG_SWRESET,
> + TAS2873_REG_SWRESET_RESET);
> + if (ret) {
> + dev_err(tas_priv->dev, "Reset failed.\n");
> + goto out;
> + }
> +
> + if (tas_priv->first_hw_init) {
> + regcache_cache_bypass(tas_priv->regmap, false);
> + regcache_mark_dirty(tas_priv->regmap);
> + }
> +
> + tas_priv->first_hw_init = true;
> + tas_priv->hw_init = true;
> +
> +out:
> + return ret;
> +}
Powered by blists - more mailing lists