| Message ID | 20180423201205.20533-7-andresx7@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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. 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 <andresx7@gmail.com> > --- > 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 > >
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 <andresx7@gmail.com> >> --- >> 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 >> >> >
On Fri, May 04, 2018 at 10:57:26PM -0400, Andres Rodriguez wrote: > 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? I welcome such a new warning but not for any of the reasons stated. It make sense if FW_OPT_NO_WARN is not set and only because the fallback mechanism can fail for a slew of different firmware files, and just getting informed a failure with a fallback occurred does not tell us for which file it failed for. I'll add such a patch to my queue and send it off soon prior to your own new API nowarn call. Luis
(sorry for the delay, this got buried in my inbox) "Luis R. Rodriguez" <mcgrof@kernel.org> writes: > 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? So from ath10k point of view we do not want to have any messages printed when calling firmware_request_nowarn(). The warnings get users really confused when ath10k is checking if an optional firmware file is available or not.
On Sat, May 12, 2018 at 11:03:52AM +0300, Kalle Valo wrote: > (sorry for the delay, this got buried in my inbox) > > "Luis R. Rodriguez" <mcgrof@kernel.org> writes: > > > 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? > > So from ath10k point of view we do not want to have any messages printed > when calling firmware_request_nowarn(). The warnings get users really > confused when ath10k is checking if an optional firmware file is > available or not. I figured, that is the intended functionality now. Luis
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); }
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. 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 <andresx7@gmail.com> --- drivers/base/firmware_loader/fallback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)