Re: [PATCH v1] ASoC: Intel: Skylake: Switch to modern UUID API

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On Wed, 19 Jun 2019 18:02:13 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Switch the driver to use modern UUID API, i.e. guid_t type and
> accompanying functions, such as guid_equal().
> 
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  sound/soc/intel/skylake/skl-pcm.c       | 12 ++++++------
>  sound/soc/intel/skylake/skl-sst-dsp.h   |  6 +++---
>  sound/soc/intel/skylake/skl-sst-utils.c | 23 +++++++----------------
>  sound/soc/intel/skylake/skl-sst.c       |  4 +---
>  sound/soc/intel/skylake/skl-topology.c  | 24 ++++++++++++------------
>  sound/soc/intel/skylake/skl-topology.h  |  6 +++---
>  6 files changed, 32 insertions(+), 43 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl-pcm.c
> b/sound/soc/intel/skylake/skl-pcm.c index 8b7232d3ffee..b2b9958605d1
> 100644 --- a/sound/soc/intel/skylake/skl-pcm.c
> +++ b/sound/soc/intel/skylake/skl-pcm.c
> @@ -1310,12 +1310,12 @@ static int skl_get_module_info(struct skl
> *skl, struct skl_module_cfg *mconfig) {
>  	struct skl_sst *ctx = skl->skl_sst;
>  	struct skl_module_inst_id *pin_id;
> -	uuid_le *uuid_mod, *uuid_tplg;
> +	guid_t *uuid_mod, *uuid_tplg;
>  	struct skl_module *skl_module;
>  	struct uuid_module *module;
>  	int i, ret = -EIO;
>  
> -	uuid_mod = (uuid_le *)mconfig->guid;
> +	uuid_mod = (guid_t *)mconfig->guid;
>  
>  	if (list_empty(&ctx->uuid_list)) {
>  		dev_err(ctx->dev, "Module list is empty\n");
> @@ -1323,7 +1323,7 @@ static int skl_get_module_info(struct skl *skl,
> struct skl_module_cfg *mconfig) }
>  
>  	list_for_each_entry(module, &ctx->uuid_list, list) {
> -		if (uuid_le_cmp(*uuid_mod, module->uuid) == 0) {
> +		if (guid_equal(uuid_mod, &module->uuid)) {
>  			mconfig->id.module_id = module->id;
>  			if (mconfig->module)
>  				mconfig->module->loadable =
> module->is_loadable; @@ -1340,7 +1340,7 @@ static int
> skl_get_module_info(struct skl *skl, struct skl_module_cfg *mconfig)
> for (i = 0; i < skl->nr_modules; i++) { skl_module = skl->modules[i];
>  		uuid_tplg = &skl_module->uuid;
> -		if (!uuid_le_cmp(*uuid_mod, *uuid_tplg)) {
> +		if (guid_equal(uuid_mod, uuid_tplg)) {
>  			mconfig->module = skl_module;
>  			ret = 0;
>  			break;
> @@ -1352,13 +1352,13 @@ static int skl_get_module_info(struct skl
> *skl, struct skl_module_cfg *mconfig) list_for_each_entry(module,
> &ctx->uuid_list, list) { for (i = 0; i < MAX_IN_QUEUE; i++) {
>  			pin_id = &mconfig->m_in_pin[i].id;
> -			if (!uuid_le_cmp(pin_id->mod_uuid,
> module->uuid))
> +			if (guid_equal(&pin_id->mod_uuid,
> &module->uuid)) pin_id->module_id = module->id;
>  		}
>  
>  		for (i = 0; i < MAX_OUT_QUEUE; i++) {
>  			pin_id = &mconfig->m_out_pin[i].id;
> -			if (!uuid_le_cmp(pin_id->mod_uuid,
> module->uuid))
> +			if (guid_equal(&pin_id->mod_uuid,
> &module->uuid)) pin_id->module_id = module->id;
>  		}
>  	}
> diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h
> b/sound/soc/intel/skylake/skl-sst-dsp.h index
> e1d6f6719f7e..cbc7a93d56c2 100644 ---
> a/sound/soc/intel/skylake/skl-sst-dsp.h +++
> b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -177,7 +177,7 @@ struct
> skl_dsp_loader_ops { #define MAX_INSTANCE_BUFF 2
>  
>  struct uuid_module {
> -	uuid_le uuid;
> +	guid_t uuid;
>  	int id;
>  	int is_loadable;
>  	int max_instance;
> @@ -241,8 +241,8 @@ void bxt_sst_dsp_cleanup(struct device *dev,
> struct skl_sst *ctx); 
>  int snd_skl_parse_uuids(struct sst_dsp *ctx, const struct firmware
> *fw, unsigned int offset, int index);
> -int skl_get_pvt_id(struct skl_sst *ctx, uuid_le *uuid_mod, int
> instance_id); -int skl_put_pvt_id(struct skl_sst *ctx, uuid_le
> *uuid_mod, int *pvt_id); +int skl_get_pvt_id(struct skl_sst *ctx,
> guid_t *uuid_mod, int instance_id); +int skl_put_pvt_id(struct
> skl_sst *ctx, guid_t *uuid_mod, int *pvt_id); int
> skl_get_pvt_instance_id_map(struct skl_sst *ctx, int module_id, int
> instance_id); void skl_freeup_uuid_list(struct skl_sst *ctx);
> diff --git a/sound/soc/intel/skylake/skl-sst-utils.c
> b/sound/soc/intel/skylake/skl-sst-utils.c index
> 2ae405617876..85551321c35b 100644 ---
> a/sound/soc/intel/skylake/skl-sst-utils.c +++
> b/sound/soc/intel/skylake/skl-sst-utils.c @@ -21,17 +21,11 @@
>  #include "../common/sst-dsp-priv.h"
>  #include "skl-sst-ipc.h"
>  
> -
> -#define UUID_STR_SIZE 37
>  #define DEFAULT_HASH_SHA256_LEN 32
>  
>  /* FW Extended Manifest Header id = $AE1 */
>  #define SKL_EXT_MANIFEST_HEADER_MAGIC   0x31454124
>  
> -struct UUID {
> -	u8 id[16];
> -};
> -
>  union seg_flags {
>  	u32 ul;
>  	struct {
> @@ -65,7 +59,7 @@ struct module_type {
>  struct adsp_module_entry {
>  	u32 struct_id;
>  	u8  name[8];
> -	struct UUID uuid;
> +	u8  uuid[16];

guid_t uuid;

>  	struct module_type type;
>  	u8  hash1[DEFAULT_HASH_SHA256_LEN];
>  	u32 entry_point;
> @@ -184,13 +178,13 @@ static inline int skl_pvtid_128(struct
> uuid_module *module)
>   * This generates a 128 bit private unique id for a module TYPE so
> that
>   * module instance is unique
>   */
> -int skl_get_pvt_id(struct skl_sst *ctx, uuid_le *uuid_mod, int
> instance_id) +int skl_get_pvt_id(struct skl_sst *ctx, guid_t
> *uuid_mod, int instance_id) {
>  	struct uuid_module *module;
>  	int pvt_id;
>  
>  	list_for_each_entry(module, &ctx->uuid_list, list) {
> -		if (uuid_le_cmp(*uuid_mod, module->uuid) == 0) {
> +		if (guid_equal(uuid_mod, &module->uuid)) {
>  
>  			pvt_id = skl_pvtid_128(module);
>  			if (pvt_id >= 0) {
> @@ -214,13 +208,13 @@ EXPORT_SYMBOL_GPL(skl_get_pvt_id);
>   *
>   * This frees a 128 bit private unique id previously generated
>   */
> -int skl_put_pvt_id(struct skl_sst *ctx, uuid_le *uuid_mod, int
> *pvt_id) +int skl_put_pvt_id(struct skl_sst *ctx, guid_t *uuid_mod,
> int *pvt_id) {
>  	int i;
>  	struct uuid_module *module;
>  
>  	list_for_each_entry(module, &ctx->uuid_list, list) {
> -		if (uuid_le_cmp(*uuid_mod, module->uuid) == 0) {
> +		if (guid_equal(uuid_mod, &module->uuid)) {
>  
>  			if (*pvt_id != 0)
>  				i = (*pvt_id) / 64;
> @@ -247,7 +241,6 @@ int snd_skl_parse_uuids(struct sst_dsp *ctx,
> const struct firmware *fw, struct adsp_fw_hdr *adsp_hdr;
>  	struct adsp_module_entry *mod_entry;
>  	int i, num_entry, size;
> -	uuid_le *uuid_bin;
>  	const char *buf;
>  	struct skl_sst *skl = ctx->thread_context;
>  	struct uuid_module *module;
> @@ -279,8 +272,7 @@ int snd_skl_parse_uuids(struct sst_dsp *ctx,
> const struct firmware *fw, return -EINVAL;
>  	}
>  
> -	mod_entry = (struct adsp_module_entry *)
> -		(buf + offset + adsp_hdr->len);
> +	mod_entry = (struct adsp_module_entry *)(buf + offset +
> adsp_hdr->len); 
>  	num_entry = adsp_hdr->num_modules;
>  
> @@ -307,8 +299,7 @@ int snd_skl_parse_uuids(struct sst_dsp *ctx,
> const struct firmware *fw, goto free_uuid_list;
>  		}
>  
> -		uuid_bin = (uuid_le *)mod_entry->uuid.id;
> -		memcpy(&module->uuid, uuid_bin,
> sizeof(module->uuid));
> +		guid_copy(&module->uuid, (guid_t *)&mod_entry->uuid);
>  
>  		module->id = (i | (index << 12));
>  		module->is_loadable = mod_entry->type.load_type;
> diff --git a/sound/soc/intel/skylake/skl-sst.c
> b/sound/soc/intel/skylake/skl-sst.c index 5951bbdf1f1a..13c636dece56
> 100644 --- a/sound/soc/intel/skylake/skl-sst.c
> +++ b/sound/soc/intel/skylake/skl-sst.c
> @@ -420,11 +420,9 @@ static int skl_load_module(struct sst_dsp *ctx,
> u16 mod_id, u8 *guid) struct skl_module_table *module_entry = NULL;
>  	int ret = 0;
>  	char mod_name[64]; /* guid str = 32 chars + 4 hyphens */
> -	uuid_le *uuid_mod;
>  
> -	uuid_mod = (uuid_le *)guid;
>  	snprintf(mod_name, sizeof(mod_name), "%s%pUL%s",
> -				"intel/dsp_fw_", uuid_mod, ".bin");
> +					     "intel/dsp_fw_", guid,
> ".bin"); 
>  	module_entry = skl_module_get_from_id(ctx, mod_id);
>  	if (module_entry == NULL) {
> diff --git a/sound/soc/intel/skylake/skl-topology.c
> b/sound/soc/intel/skylake/skl-topology.c index
> c69d999d7bf1..9fd756bcc740 100644 ---
> a/sound/soc/intel/skylake/skl-topology.c +++
> b/sound/soc/intel/skylake/skl-topology.c @@ -580,7 +580,7 @@
> skl_tplg_init_pipe_modules(struct skl *skl, struct skl_pipe *pipe)
> int ret = 0; 
>  	list_for_each_entry(w_module, &pipe->w_list, node) {
> -		uuid_le *uuid_mod;
> +		guid_t *uuid_mod;
>  		w = w_module->w;
>  		mconfig = w->priv;
>  
> @@ -588,7 +588,7 @@ skl_tplg_init_pipe_modules(struct skl *skl,
> struct skl_pipe *pipe) if (mconfig->id.module_id < 0) {
>  			dev_err(skl->skl_sst->dev,
>  					"module %pUL id not
> populated\n",
> -					(uuid_le *)mconfig->guid);
> +					(guid_t *)mconfig->guid);
>  			return -EIO;
>  		}
>  
> @@ -622,7 +622,7 @@ skl_tplg_init_pipe_modules(struct skl *skl,
> struct skl_pipe *pipe)
>  		 * FE/BE params
>  		 */
>  		skl_tplg_update_module_params(w, ctx);
> -		uuid_mod = (uuid_le *)mconfig->guid;
> +		uuid_mod = (guid_t *)mconfig->guid;
>  		mconfig->id.pvt_id = skl_get_pvt_id(ctx, uuid_mod,
>  						mconfig->id.instance_id);
>  		if (mconfig->id.pvt_id < 0)
> @@ -661,9 +661,9 @@ static int skl_tplg_unload_pipe_modules(struct
> skl_sst *ctx, struct skl_module_cfg *mconfig = NULL;
>  
>  	list_for_each_entry(w_module, &pipe->w_list, node) {
> -		uuid_le *uuid_mod;
> +		guid_t *uuid_mod;
>  		mconfig  = w_module->w->priv;
> -		uuid_mod = (uuid_le *)mconfig->guid;
> +		uuid_mod = (guid_t *)mconfig->guid;
>  
>  		if (mconfig->module->loadable &&
> ctx->dsp->fw_ops.unload_mod && mconfig->m_state > SKL_MODULE_UNINIT) {
> @@ -918,12 +918,12 @@ static int
> skl_tplg_set_module_bind_params(struct snd_soc_dapm_widget *w, return
> 0; }
>  
> -static int skl_get_module_id(struct skl_sst *ctx, uuid_le *uuid)
> +static int skl_get_module_id(struct skl_sst *ctx, guid_t *uuid)
>  {
>  	struct uuid_module *module;
>  
>  	list_for_each_entry(module, &ctx->uuid_list, list) {
> -		if (uuid_le_cmp(*uuid, module->uuid) == 0)
> +		if (guid_equal(uuid, &module->uuid))
>  			return module->id;
>  	}
>  
> @@ -2121,11 +2121,11 @@ static int skl_tplg_add_pipe(struct device
> *dev, return 0;
>  }
>  
> -static int skl_tplg_get_uuid(struct device *dev, u8 *guid,
> +static int skl_tplg_get_uuid(struct device *dev, guid_t *guid,
>  	      struct snd_soc_tplg_vendor_uuid_elem *uuid_tkn)
>  {
>  	if (uuid_tkn->token == SKL_TKN_UUID) {
> -		memcpy(guid, &uuid_tkn->uuid, 16);
> +		guid_copy(guid, (guid_t *)&uuid_tkn->uuid);
>  		return 0;
>  	}
>  
> @@ -2151,7 +2151,7 @@ static int skl_tplg_fill_pin(struct device *dev,
>  		break;
>  
>  	case SKL_TKN_UUID:
> -		ret = skl_tplg_get_uuid(dev,
> m_pin[pin_index].id.mod_uuid.b,
> +		ret = skl_tplg_get_uuid(dev,
> &m_pin[pin_index].id.mod_uuid, (struct snd_soc_tplg_vendor_uuid_elem
> *)tkn_elem); if (ret < 0)
>  			return ret;
> @@ -2667,7 +2667,7 @@ static int skl_tplg_get_tokens(struct device
> *dev, 
>  		case SND_SOC_TPLG_TUPLE_TYPE_UUID:
>  			if (is_module_guid) {
> -				ret = skl_tplg_get_uuid(dev,
> mconfig->guid,
> +				ret = skl_tplg_get_uuid(dev, (guid_t
> *)mconfig->guid, array->uuid);
>  				is_module_guid = false;
>  			} else {
> @@ -3486,7 +3486,7 @@ static int skl_tplg_get_manifest_uuid(struct
> device *dev, 
>  	if (uuid_tkn->token == SKL_TKN_UUID) {
>  		mod = skl->modules[ref_count];
> -		memcpy(&mod->uuid, &uuid_tkn->uuid,
> sizeof(uuid_tkn->uuid));
> +		guid_copy(&mod->uuid, (guid_t *)&uuid_tkn->uuid);
>  		ref_count++;
>  	} else {
>  		dev_err(dev, "Not an UUID token tkn %d\n",
> uuid_tkn->token); diff --git a/sound/soc/intel/skylake/skl-topology.h
> b/sound/soc/intel/skylake/skl-topology.h index
> b66e3a728853..5d2047114db0 100644 ---
> a/sound/soc/intel/skylake/skl-topology.h +++
> b/sound/soc/intel/skylake/skl-topology.h @@ -215,7 +215,7 @@ struct
> skl_mod_inst_map { struct skl_uuid_inst_map {
>  	u16 inst_id;
>  	u16 reserved;
> -	uuid_le mod_uuid;
> +	guid_t mod_uuid;
>  } __packed;
>  
>  struct skl_kpb_params {
> @@ -227,7 +227,7 @@ struct skl_kpb_params {
>  };
>  
>  struct skl_module_inst_id {
> -	uuid_le mod_uuid;
> +	guid_t mod_uuid;
>  	int module_id;
>  	u32 instance_id;
>  	int pvt_id;
> @@ -360,7 +360,7 @@ struct skl_module_res {
>  };
>  
>  struct skl_module {
> -	uuid_le uuid;
> +	guid_t uuid;
>  	u8 loadable;
>  	u8 input_pin_type;
>  	u8 output_pin_type;


I would also add:

struct skl_module_cfg {
-	u8 guid[16];
+	guid_t uuid;

should get rid of few casts above. Overall I would expect no casts at
all.