From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AACA0C27C52 for ; Tue, 4 Jun 2024 15:13:24 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 15E6FE7D; Tue, 4 Jun 2024 17:13:12 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 15E6FE7D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1717514002; bh=hSqtnWWeq83bRrdJrf3MfAQNHnxLuRaoyE2ioCIEo0M=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=tKy9burwXTS5KSQHACVGpiz4iDoqeSLBRPDJJoMNqD0mj1Qig7ub3BEgxFWP72MqJ 9CSkfaMpLwS/6opK4amQbaNTwwIMFyILRCaUhC0Y0D3BCKJyAlRynnwVBV4aLH9T+a 6uAMAo3FJZdmWon9rUl0+TOCGGdl+DXRpEaNJj9g= Received: by alsa1.perex.cz (Postfix, from userid 50401) id 46BD9F805D3; Tue, 4 Jun 2024 17:12:40 +0200 (CEST) Received: from mailman-core.alsa-project.org (mailman-core.alsa-project.org [10.254.200.10]) by alsa1.perex.cz (Postfix) with ESMTP id 061EAF805D3; Tue, 4 Jun 2024 17:12:39 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id D6E9BF802DB; Tue, 4 Jun 2024 17:12:32 +0200 (CEST) Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.8]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id AE8EBF8003A for ; Tue, 4 Jun 2024 17:12:27 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz AE8EBF8003A Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key, unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=dZeHVCgT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1717513949; x=1749049949; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=hSqtnWWeq83bRrdJrf3MfAQNHnxLuRaoyE2ioCIEo0M=; b=dZeHVCgTx8DToee3hokfCey/Gsw+6topOQaikk6wRW+i7ZG+7lCgF3BK AgB3qfeUw9wakEDKdHRN9jKFs8YW1EhUu5ghMD1N8ayp3CjyrSVD9fFhJ Fa5Q8ZrM6ovz44nh+kmiSrSwYPmb4ADerRtVW2icU0v+Bs5OFESkH3XHY WH1iPWDWlxa0X/v8OYkWtQGJr0z93aeOpdo14G/Ts6fiV/xE2+VmrCAWs jCIdjUolawzVFoyptvylVy1Xp7M9UL/V/TwH8hVFApWKYtXpZLrGfSr4A HzC2BRKO7pv3qCIXyL+z19fCgwmYFzKWHjCmkgF5qRykSdtXAbP5NobWo w==; X-CSE-ConnectionGUID: pYXj1fHMTO2reXizNuxwjQ== X-CSE-MsgGUID: pNXpf7VDS5q2MlbbrkrO/w== X-IronPort-AV: E=McAfee;i="6600,9927,11093"; a="31602500" X-IronPort-AV: E=Sophos;i="6.08,214,1712646000"; d="scan'208";a="31602500" Received: from fmviesa007.fm.intel.com ([10.60.135.147]) by fmvoesa102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Jun 2024 08:12:09 -0700 X-CSE-ConnectionGUID: XIeU217HRTuXyOfBAck/OQ== X-CSE-MsgGUID: Oe+hRJJRQT+iPoQlktO7RQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.08,214,1712646000"; d="scan'208";a="37139630" Received: from fdefranc-mobl3.ger.corp.intel.com (HELO [10.245.246.143]) ([10.245.246.143]) by fmviesa007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Jun 2024 08:12:07 -0700 Message-ID: <0d15954f-0158-4a56-afef-f0d043135146@linux.intel.com> Date: Tue, 4 Jun 2024 17:07:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 3/4] soundwire: bus: clean up probe warnings To: Johan Hovold Cc: Johan Hovold , Vinod Koul , Bard Liao , Sanyog Kale , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org References: <20240604075213.20815-1-johan+linaro@kernel.org> <20240604075213.20815-4-johan+linaro@kernel.org> <8dd7cadc-138c-4ef5-b06f-7177550b1215@linux.intel.com> <970501b1-09ae-4f2c-a078-2b4f23fe460e@linux.intel.com> Content-Language: en-US From: Pierre-Louis Bossart In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Message-ID-Hash: VYP6IJ4DYUXQQVD7HBSVTTQDZ5PQEWBK X-Message-ID-Hash: VYP6IJ4DYUXQQVD7HBSVTTQDZ5PQEWBK X-MailFrom: pierre-louis.bossart@linux.intel.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-alsa-devel.alsa-project.org-0; header-match-alsa-devel.alsa-project.org-1; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.9 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: \ >>>>> @@ -123,7 +123,7 @@ static int sdw_drv_probe(struct device *dev) >>>>> /* init the dynamic sysfs attributes we need */ >>>>> ret = sdw_slave_sysfs_dpn_init(slave); >>>>> if (ret < 0) >>>>> - dev_warn(dev, "Slave sysfs init failed:%d\n", ret); >>>>> + dev_warn(dev, "failed to initialise sysfs: %d\n", ret); >>>>> >>>>> /* >>>>> * Check for valid clk_stop_timeout, use DisCo worst case value of >>>>> @@ -147,7 +147,7 @@ static int sdw_drv_probe(struct device *dev) >>>>> if (drv->ops && drv->ops->update_status) { >>>>> ret = drv->ops->update_status(slave, slave->status); >>>>> if (ret < 0) >>>>> - dev_warn(dev, "%s: update_status failed with status %d\n", __func__, ret); >>>>> + dev_warn(dev, "failed to update status: %d\n", ret); >>>> >>>> the __func__ does help IMHO, 'failed to update status' is way too general... >>> >>> Error messages printed with dev_warn will include the device and driver >>> names so this message will be quite specific still. >> >> The goal isn't to be 'quite specific' but rather 'completely >> straightforward'. Everyone can lookup a function name in a xref tool and >> quickly find out what happened. Doing 'git grep' on message logs isn't >> great really, and over time logs tend to be copy-pasted. Just look at >> the number of patches where we had to revisit the dev_err logs to make >> then really unique/useful. > > Error message should be self-contained and give user's some idea of what > went wrong and not leak implementation details like function names (and > be greppable, which "%s:" is not). "Failed to update status" doesn't sound terribly self-contained to me. It's actually a great example of making the logs less clear with good intentions. How many people know that the SoundWire bus exposes an 'update_status' callback, and that callback can be invoked from two completely different places (probe or on device attachment)? /* Ensure driver knows that peripheral unattached */ ret = sdw_update_slave_status(slave, status[i]); if (ret < 0) dev_warn(&slave->dev, "Update Slave status failed:%d\n", ret); You absolutely want to know which of these two cases failed, but with your changes they now look rather identical except for the order of words. one would be 'failed to update status' and the other 'update status failed'. What is much better is to know WHEN this failure happens, then folks looking at logs to fix a problem don't need to worry about precise wording or word order. It's a constant battle to get meaningful messages that are useful for validation/integration folks, and my take is that it's a windmill-fighting endeavor. The function name is actually more useful, it's not an implementation detail, it's what you're looking for when reverse-engineering problematic sequences from a series of CI logs. >>>> Replacing 'with status' by ":" is fine, but do we really care about 10 >>>> chars in a log? >>> >>> It's not primarily about the numbers of characters but about consistency. >> >> I am advocating for inclusion of __func__ everywhere...It's simpler for >> remote support and bug chasing. I meant everywhere in SoundWire. Other subsystems may have different views and different observability tools, that's fine. > A quick grep seems to suggest you're in a small minority here with some > 5k of 65k dev_err() including __func__. > > [ And there's only 55 out of 750 dev_err() like that in > drivers/soundwire, which is inconsistent at best. ] As you mentioned yourself, the asynchronous nature of the SoundWire probe/attachment/interrupts makes it difficult to reverse-engineer, and we want to err on the side of MORE information. Also not all dev_err() are equal, most are part of paranoid checks and never used. An example above is the sysfs log, we've never seen it happen. That's different to changes that impact probe and interrupts which will fail at some point on new platforms. It's not an academic statement, I've spent most of my day chasing two such issues.