[PATCH v3 11/15] ASoC: Intel: avs: Machine board registration

[Cezary Rojewski]

On 2022-05-29 7:48 AM, Uwe Kleine-König wrote:
> Hello Mark,
> 
> On Thu, May 26, 2022 at 06:44:38PM +0100, Mark Brown wrote:

>> FWIW given how hard the 0-day reports have become to read I'd not rely
>> on people paying attention to things that are clearly pure build
>> coverage based off a 0-day report alone.
> 
> I'm unsure if I understand your sentiment correctly. Are you saying it
> doesn't matter if a patch breaks the build on some arch using a
> randconfig?
> 
> My position is: The commit under discussion (i.e. beed983621fb ("ASoC:
> Intel: avs: Machine board registration")) breaks an allmodconfig build
> on all platforms where __fls doesn't return a long int (i.e. arc, m68k,
> and sparc). This actively hurts people who do build tests using
> allmodconfig (or allyesconfig) for their patch series (e.g. me).
> 
> I agree that some reports by the 0-day bot are hard to parse. But still,
> if there is a build problem someone should look into that. If you (with
> your maintainer hat on) don't have the resource to do that, that's IMHO
> fine. But I'd wish you'd push back on the patch submitter in that case
> and don't apply the patch until the problem is resolved. If this is a
> corner case randconfig issue, applying might be fine despite the build
> error but breaking allmodconfig on 3 archs is bad.
> 
> The fix would be


Hello Uwe,

I don't believe anyone here wanted to break the build-configurations you 
did mention above. I also believe it's important to mention that below 
is not a fix but a W/A. Developer should be able to use __fls() if 
required. Value returned by fls() differs from one returned by __fls(), 
and using fls() in below context is misleading (hurts the debug-ability).

Yes, the faulty print should be devoid of __fls() until the function is 
fixed for all the archs. It's too late for that and I'm sorry for any 
inconvenience caused by the change.
To my knowledge the real fix has been provided on LKML by Amadeo [1] and 
is under review since Friday. I'd kindly appreciate helping fix the root 
cause of the problem. If there's anything missing in the series let us know.


[1]: 
https://lore.kernel.org/lkml/20220527115345.2588775-3-amadeuszx.slawinski@linux.intel.com/T/


Regards,
Czarek


> diff --git a/sound/soc/intel/avs/board_selection.c b/sound/soc/intel/avs/board_selection.c
> index 80cb0164678a..f189f71b8e1e 100644
> --- a/sound/soc/intel/avs/board_selection.c
> +++ b/sound/soc/intel/avs/board_selection.c
> @@ -325,8 +325,8 @@ static int avs_register_i2s_board(struct avs_dev *adev, struct snd_soc_acpi_mach
>   
>   	num_ssps = adev->hw_cfg.i2s_caps.ctrl_count;
>   	if (fls(mach->mach_params.i2s_link_mask) > num_ssps) {
> -		dev_err(adev->dev, "Platform supports %d SSPs but board %s requires SSP%ld\n",
> -			num_ssps, mach->drv_name, __fls(mach->mach_params.i2s_link_mask));
> +		dev_err(adev->dev, "Platform supports %d SSPs but board %s requires SSP%d\n",
> +			num_ssps, mach->drv_name, fls(mach->mach_params.i2s_link_mask));
>   		return -ENODEV;
>   	}
>   
> which uses fls instead of __fls (as is done in the test triggering the
> error). The former returns an int on all platforms.
> 
> Tell me if you don't want to squash this into beed983621fb and prefer a
> formal patch.
> 
> Best regards
> Uwe
>