On Sun, May 29, 2022 at 03:24:48PM +0200, Cezary Rojewski wrote:
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.
Hmm, I doubt this (i.e. unify the return value of __fls) will be merged during this cycle. I'd want that to cook a bit in next before it hits mainline, there might be similar problems introduced by changing __fls to the one under discussion here.
A more short-term fix would be:
diff --git a/sound/soc/intel/avs/board_selection.c b/sound/soc/intel/avs/board_selection.c --- 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)); + num_ssps, mach->drv_name, (unsigned long)__fls(mach->mach_params.i2s_link_mask)); return -ENODEV; }
i.e. explicitly cast the return value of __fls to unsigned long.
Best regards Uwe