From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-io0-f194.google.com ([209.85.223.194]:35413 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751962AbeEEC53 (ORCPT ); Fri, 4 May 2018 22:57:29 -0400 Cc: andresx7@gmail.com, linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org, alexdeucher@gmail.com, christian.koenig@amd.com, kvalo@codeaurora.org, arend.vanspriel@broadcom.com, linux-wireless@vger.kernel.org, ath10k@lists.infradead.org, hdegoede@redhat.com, Kees Cook , Mimi Zohar Subject: Re: [PATCH 6/9] firmware: print firmware name on fallback path To: "Luis R. Rodriguez" References: <20180423201205.20533-1-andresx7@gmail.com> <20180423201205.20533-7-andresx7@gmail.com> <20180503234235.GX27853@wotan.suse.de> From: Andres Rodriguez Message-ID: (sfid-20180505_045743_692881_99B470F3) Date: Fri, 4 May 2018 22:57:26 -0400 MIME-Version: 1.0 In-Reply-To: <20180503234235.GX27853@wotan.suse.de> Content-Type: text/plain; charset=utf-8; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 2018-05-03 07:42 PM, Luis R. Rodriguez wrote: > On Mon, Apr 23, 2018 at 04:12:02PM -0400, Andres Rodriguez wrote: >> Previously, one could assume the firmware name from the preceding >> message: "Direct firmware load for {name} failed with error %d". >> >> However, with the new firmware_request_nowarn() entrypoint, the message >> outlined above will not always be printed. > > I though the whole point was to not print an error message. What if > we want later to disable this error message? This would prove a bit > pointless. > > Let's discuss the exact semantics desired here. Why would only the > fallback be desirable here? > > Andres, Kalle? > > After we address this I'll address resubmitting this lat patch > along with the last one. For now I'll skip it. You are correct. I initially thought it would be useful to know that the usermode fallback was being triggered. And for that message to be useful we would need a fw name. But now that you point it out, this behaviour is inconsistent with the _nowarn() definition. We shouldn't have a message in the first place. So it might be better to instead have: if (!(opt_flags & FW_OPT_NO_WARN) ) dev_warn(device, "Falling back to user helper\n"); No need to add the firmware name, cause we either: a) FW_OPT_NO_WARN is set and no messages are printed, or b) FW_OPT_NO_WARN is not set and we get both messages. Yay, nay? Regards, Andres > > Luis > >> Therefore, we add the firmware name to the fallback path spew in order >> to associate it with the appropriate request. >> >> Signed-off-by: Andres Rodriguez >> --- >> drivers/base/firmware_loader/fallback.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c >> index e75928458489..1a47ddc70c31 100644 >> --- a/drivers/base/firmware_loader/fallback.c >> +++ b/drivers/base/firmware_loader/fallback.c >> @@ -669,6 +669,6 @@ int fw_sysfs_fallback(struct firmware *fw, const char *name, >> if (!fw_run_sysfs_fallback(opt_flags)) >> return ret; >> >> - dev_warn(device, "Falling back to user helper\n"); >> + dev_warn(device, "Falling back to user helper for %s\n", name); >> return fw_load_from_user_helper(fw, name, device, opt_flags); >> } >> -- >> 2.14.1 >> >> > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-io0-x242.google.com ([2607:f8b0:4001:c06::242]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1fEnOH-0003Xx-BY for ath10k@lists.infradead.org; Sat, 05 May 2018 02:57:43 +0000 Received: by mail-io0-x242.google.com with SMTP id a10-v6so27791758ioc.9 for ; Fri, 04 May 2018 19:57:30 -0700 (PDT) Subject: Re: [PATCH 6/9] firmware: print firmware name on fallback path References: <20180423201205.20533-1-andresx7@gmail.com> <20180423201205.20533-7-andresx7@gmail.com> <20180503234235.GX27853@wotan.suse.de> From: Andres Rodriguez Message-ID: Date: Fri, 4 May 2018 22:57:26 -0400 MIME-Version: 1.0 In-Reply-To: <20180503234235.GX27853@wotan.suse.de> Content-Language: en-US List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: "Luis R. Rodriguez" Cc: arend.vanspriel@broadcom.com, Kees Cook , gregkh@linuxfoundation.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, ath10k@lists.infradead.org, hdegoede@redhat.com, andresx7@gmail.com, alexdeucher@gmail.com, Mimi Zohar , christian.koenig@amd.com, kvalo@codeaurora.org On 2018-05-03 07:42 PM, Luis R. Rodriguez wrote: > On Mon, Apr 23, 2018 at 04:12:02PM -0400, Andres Rodriguez wrote: >> Previously, one could assume the firmware name from the preceding >> message: "Direct firmware load for {name} failed with error %d". >> >> However, with the new firmware_request_nowarn() entrypoint, the message >> outlined above will not always be printed. > > I though the whole point was to not print an error message. What if > we want later to disable this error message? This would prove a bit > pointless. > > Let's discuss the exact semantics desired here. Why would only the > fallback be desirable here? > > Andres, Kalle? > > After we address this I'll address resubmitting this lat patch > along with the last one. For now I'll skip it. You are correct. I initially thought it would be useful to know that the usermode fallback was being triggered. And for that message to be useful we would need a fw name. But now that you point it out, this behaviour is inconsistent with the _nowarn() definition. We shouldn't have a message in the first place. So it might be better to instead have: if (!(opt_flags & FW_OPT_NO_WARN) ) dev_warn(device, "Falling back to user helper\n"); No need to add the firmware name, cause we either: a) FW_OPT_NO_WARN is set and no messages are printed, or b) FW_OPT_NO_WARN is not set and we get both messages. Yay, nay? Regards, Andres > > Luis > >> Therefore, we add the firmware name to the fallback path spew in order >> to associate it with the appropriate request. >> >> Signed-off-by: Andres Rodriguez >> --- >> drivers/base/firmware_loader/fallback.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c >> index e75928458489..1a47ddc70c31 100644 >> --- a/drivers/base/firmware_loader/fallback.c >> +++ b/drivers/base/firmware_loader/fallback.c >> @@ -669,6 +669,6 @@ int fw_sysfs_fallback(struct firmware *fw, const char *name, >> if (!fw_run_sysfs_fallback(opt_flags)) >> return ret; >> >> - dev_warn(device, "Falling back to user helper\n"); >> + dev_warn(device, "Falling back to user helper for %s\n", name); >> return fw_load_from_user_helper(fw, name, device, opt_flags); >> } >> -- >> 2.14.1 >> >> > _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k