[PATCH v7 03/14] ASoC: Intel: catpt: Add IPC message handlers

[Andy Shevchenko]

On Mon, Sep 21, 2020 at 06:13:59PM +0000, Rojewski, Cezary wrote:
> On 2020-09-21 2:59 PM, Andy Shevchenko wrote:
> > On Mon, Sep 21, 2020 at 01:54:13PM +0200, Cezary Rojewski wrote:

...

> >> +	for (i = j = 0; i < FW_INFO_SIZE_MAX; i++)
> >> +		if (cdev->ipc.config.fw_info[i] == ' ')
> >> +			if (++j == 4)
> >> +				break;
> > 
> >> +	for (j = ++i; j < FW_INFO_SIZE_MAX && j - i < 20; j++) {
> > 
> > This should have static_assert() at the place where you define both constants
> > (2nd is mentioned above 20).
> > 
> >> +		if (cdev->ipc.config.fw_info[j] == ' ')
> >> +			break;
> >> +		*(pos + j - i) = cdev->ipc.config.fw_info[j];
> >> +	}
> >> +	pos += 20;
> > 
> > These two for-loops should have some comment to explain what's going on.
> > 
> 
> Actually, after poking my FW friends again I realized that it's just
> dumping 20chars from "hash" segment of fw_info (struct catpt_fw_ready,
> field: fw_info[]).
> 
> So, this could be replaced by:
> 

> 	/* navigate to fifth info segment (fw hash) */
> 	for (i = j = 0; i < FW_INFO_SIZE_MAX; i++)
> 		/* info segments are separated by space each */
> 		if (cdev->ipc.config.fw_info[i] == ' ')
> 			if (++j == 4)
> 				break;

...and this is repeating strnchr() / strnchrnul().

With the questions "what if...":
 - nul in the middle of this?
 - less than 4 spaces found?

> 	memcpy(pos, &cdev->ipc.config.fw_info[++i], CATPT_DUMP_HASH_SIZE);
> 	pos += CATPT_DUMP_HASH_SIZE;
> 
> 
> Existing for-loops were based on internal solution. Half of the code
> isn't needed afterall..

-- 
With Best Regards,
Andy Shevchenko