re: aprint_* used outside autoconfiguration step
On Feb 20, 8:44am, m...@eterna.com.au (matthew green) wrote: -- Subject: re: aprint_* used outside autoconfiguration step | i don't like that patch for two reasons: | | - if config_pending must be exposed, do it properly, not with a | another extern that isn't seen by the definition. subr_prf.c | is already gross enough in that way. however, i think it is | an abuse of it to use it this way, early autoconfig can happen | without ever increasing config_pending -- it all depends upon | what devices you have configured and what are present. Yes, I don't like that either (using this variable). But introducing another variable is more intrusive and at this point I would prefer to re-evaluate how driver messaging is done. | - no prefix at all seems worse. at least i know it was an error | before, but no there is no context, just a message. | | however, the biggest problem, IMO, is the presence of the API | aprint_error() -- it takes no "device" parameter for a name, and | thus is prefixless in messages making them confusing. | | can we eliminate this one entirely while we are at it? it's | unfortunately used a *lot*. There is aprint_debug() aprint_naive() and aprint_normal() too that are prefix-less. Perhaps we should enforce a prefix name in case where there is no ethernet interface or device involved, something like the "driver name". | > 2. We don't have a non-autoconfig-related family of printf | >calls to handle errors outside autoconfiguration. | | we have device_printf(9). perhaps a device_printf_error()? I would rename that to: printf_dev(), printf_error_dev() and printf_error_ifnet() to be orthogonal; then again it is going to be confusing with the semantics of LOG/CONS in the different aprint variants (it is implicit with the variant where the data goes). It also does not handle the debug/naive/normal cases. This is a mess. christos
Re: aprint_* used outside autoconfiguration step
> On Feb 19, 2019, at 1:44 PM, matthew green wrote: > > can we eliminate this one entirely while we are at it? it's > unfortunately used a *lot*. We should probably just get rid of aprint_naive(), too. > >> 2. We don't have a non-autoconfig-related family of printf >> calls to handle errors outside autoconfiguration. > > we have device_printf(9). perhaps a device_printf_error()? > > > .mrg. -- thorpej
Re: aprint_* used outside autoconfiguration step
On Feb 19, 10:19am, buh...@nfbcal.org (Brian Buhrow) wrote:
-- Subject: Re: aprint_* used outside autoconfiguration step
| hello. Is it sufficient to do something like:
|
| if (cold) {
| aprint_error("...");
| } else {
| printf("...");
| }
|
| or, is that what your new pending variable is supposed to help with?
The "config_pending" variable is not new, and "cold" is too restrictive
since a lot of autoconfiguration is now happening after cold is unset.
christos
Re: aprint_* used outside autoconfiguration step
On Feb 19, 9:39am, buh...@nfbcal.org (Brian Buhrow) wrote:
-- Subject: Re: aprint_* used outside autoconfiguration step
| Hello. I'm a little confused by the goal of this change? If there
| are are errors worth printing messages about, shouldn't the author of the
| error message have a reasonable expectation that their message will get
| printed via the available kernel printing facilities? I certainly expect
| if I'm writing a kernel module, that if I call a print routine, my message
| should turn up rather than being ignored.
I think that with the current aprint_ framework we run into two issues:
1. There is code that is called both during autoconfiguration and during
regular operations. For example the network interface initialization
code; when there is a need to reload firmware for example, most of
the drivers use aprint_error_dev() so we end up with an:
autoconfiguration error: Can't load firmware.
There is also the behavioral side-effect of using printf() vs.
using aprint*(): That the AB_* flags affect where it goes.
I think that the right fix is to not call aprint*() for those
errors, since they can also happen during regular operations.
The question still remains though, how does one know that some
routine is only called during autoconfiguration and they should
use aprint*() vs. regular operation/both and they should use printf()?
It is hard to code review and get it right.
2. The aprint*() family has many useful variants, which are missing from
the regular print*(). like:
aprint_{normal,verbose,naive,debug}{,_dev,_ifnet}()
so people end up using those outside the autoconfiguration
process, because they are convenient (and some people don't
understand what the "a" in the name stands for). I am not sure
if we should grow another parallel set of functions... Perhaps
it is simpler to just unify them all and have a single set of
functions. It is not clear to me either, if encoding the level in
the function name is desirable as opposed to having a separate level
argument for it (like we traditionally had in the past).
christos
Re: aprint_* used outside autoconfiguration step
On Tue, Feb 19, 2019 at 10:49:04AM -0500, Christos Zoulas wrote: > I've noticed that now the "autoconfiguration print" error messages > are printing "autoconfiguration error: " we have a situation where > aprint_ calls are happening outside the autoconfiguration process. > This is confusing. What makes this difficult to fix is: > > 1. Some of the calls can happen during the autoconfiguration > phase and also later (during normal operations). For example > the iffoo_init() routine and its children calls. > 2. We don't have a non-autoconfig-related family of printf > calls to handle errors outside autoconfiguration. > > I propose to go for the simplest fix for now, which is to only print > "autoconfiguration error: " during the autoconfiguration process (i.e. > once autoconfiguration is done, to skip printing it. This again is > wrong in some cases (hotplug), but something simplistic such as the > following might take care of the majority of the cases: While the primary reason I added this was to expose which parts of the dmesg were actually errors, a secondary reason was to help curb the abuse of aprint_error at runtime. While I'm of the opinion that aprint_* should be used iff within the autoconf(9) paths, I'm also seeing now that if you want a perfectly silent AB_SILENT boot, you can't have plain printfs making a mess. So, somewhat reluctantly I accept the general idea of your proposal as an immediate solution. Jonathan Kollasch
