Re: [PATCH] ASoC: Intel: Skylake: Fix struct definition

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2023-02-15 12:17 AM, Kees Cook wrote:
> On Mon, Feb 13, 2023 at 09:52:23PM +0100, Amadeusz Sławiński wrote:

...

>> diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
>> index 6db0fd7bad49..30a0977af943 100644
>> --- a/sound/soc/intel/skylake/skl-topology.h
>> +++ b/sound/soc/intel/skylake/skl-topology.h
>> @@ -115,7 +115,10 @@ struct skl_cpr_gtw_cfg {
>>   	u32 dma_buffer_size;
>>   	u32 config_length;
>>   	/* not mandatory; required only for DMIC/I2S */
>> -	u32 config_data[1];
>> +	struct {
>> +		u32 gtw_attrs;
>> +		u32 data[];
>> +	} config_data;
>>   } __packed;
> 
> I recommend leaving the original memcpy() as it was, and instead
> creating an anonymous union in place of "config_data":
> 
> 	union {
> 		u32 gtw_attrs;
> 		DECLARE_FLEX_ARRAY(u32, data);
> 	};


I appreciate the input. The reason we're sticking to struct is purely 
for readability/maintenance reasons. In the past AudioDSP drivers were 
declaring structs that take part in the IPC communication differently 
than the base AudioDSP firmware did (i.e.: the other participant of the 
IPC). Over the years this resulted in communication issues between the 
software and the firmware teams. Thus, for quite some time now we're 
sticking to same naming/layout convention wherever possible so there is 
no confusion. Works out pretty well if you ask me.

Now, in regard to this very case. 'config_data' consists of field: 
'gateway attributes' and an optional BLOB (hardware configuration) that 
follows it. Often this config is taken directly from one of the ACPI 
tables - NHLT. The NHLT stores hardware configuration bits for I2S/DMIC 
audio endpoints. Unfortunately, each entry within NLHT _is_ the entire 
config, not just the BLOB part.

To make situation clear within the code, config is described with a 
struct, not an union. Given handler may access the entire config through 
&config_data when memcpying, attributes alone through 
config_data.gtw_attrs or the BLOB with config_data.blob. Again, readability.


Kind regards,
Czarek