Re: [PATCH v3 11/15] ASoC: Intel: avs: Machine board registration - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Mark Brown <broonie@kernel.org>
Cc: pierre-louis.bossart@linux.intel.com,
	Cezary Rojewski <cezary.rojewski@intel.com>,
	upstream@semihalf.com, rad@semihalf.com,
	alsa-devel@alsa-project.org, tiwai@suse.com, hdegoede@redhat.com,
	lma@semihalf.com, amadeuszx.slawinski@linux.intel.com,
	cujomalainey@chromium.org, Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v3 11/15] ASoC: Intel: avs: Machine board registration
Date: Sun, 29 May 2022 07:48:07 +0200	[thread overview]
Message-ID: <20220529054807.uuqpsnt3w6dwlzuj@pengutronix.de> (raw)
In-Reply-To: <Yo+8hvwhvdx7tNGI@sirena.org.uk>

[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]

Hello Mark,

On Thu, May 26, 2022 at 06:44:38PM +0100, Mark Brown wrote:
> On Thu, May 26, 2022 at 09:24:43AM -0700, Guenter Roeck wrote:
> > On Mon, May 16, 2022 at 12:11:12PM +0200, Cezary Rojewski wrote:
> 
> > > +	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",
> 
> >    sound/soc/intel/avs/board_selection.c: In function 'avs_register_i2s_board':
> > >> sound/soc/intel/avs/board_selection.c:328:36: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'int' [-Wformat=]
> >      328 |                 dev_err(adev->dev, "Platform supports %d SSPs but board %s requires SSP%ld\n",
> >                                                                                                   ^^^
> > Reported by 0-day but still made it into mainline.
> 
> 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

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
