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
aprint_* used outside autoconfiguration step
Hi,
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:
Index: subr_prf.c
===
RCS file: /cvsroot/src/sys/kern/subr_prf.c,v
retrieving revision 1.176
diff -u -u -r1.176 subr_prf.c
--- subr_prf.c 14 Jan 2019 19:21:54 - 1.176
+++ subr_prf.c 19 Feb 2019 15:48:26 -
@@ -105,6 +105,7 @@
extern struct tty *constty;/* pointer to console "window" tty */
extern int log_open; /* subr_log: is /dev/klog open? */
extern krndsource_trnd_printf_source;
+extern int config_pending;
const char *panicstr; /* arg to first call to panic (used as a flag
to indicate that panic has already been called). */
struct cpu_info *paniccpu; /* cpu that first paniced */
@@ -865,7 +866,8 @@
if (prefix)
kprintf_internal("%s: ", flags, NULL, NULL, prefix);
- kprintf_internal("autoconfiguration error: ", TOLOG, NULL, NULL);
+ if (config_pending)
+ kprintf_internal("autoconfiguration error: ", TOLOG, NULL,
NULL);
kprintf(fmt, flags, NULL, NULL, ap);
kprintf_unlock();
Index: subr_autoconf.c
===
RCS file: /cvsroot/src/sys/kern/subr_autoconf.c,v
retrieving revision 1.265
diff -u -u -r1.265 subr_autoconf.c
--- subr_autoconf.c 1 Dec 2018 02:08:16 - 1.265
+++ subr_autoconf.c 19 Feb 2019 15:48:26 -
@@ -224,7 +224,7 @@
static int alldevs_nwrite = 0;
static bool alldevs_garbage = false;
-static int config_pending; /* semaphore for mountroot */
+int config_pending;/* semaphore for mountroot */
static kmutex_t config_misc_lock;
static kcondvar_t config_misc_cv;
christos
Re: Reserve device major numbers for pkgsrc
On 19.02.2019 08:17, Michael van Elst wrote: > n...@gmx.com (Kamil Rytarowski) writes: > >> =46rom end-user point of view major and minor numbers don't matter most o= >> f >> the time (are there exceptions?) and it might be picked by the kernel >> dynamically. > > When the kernel choses major numbers dynamically, it must also provide > a dynamically generated mapping to userland. > > Minor numbers usually translate 1:1 into device units, they cannot be > chosen dynamically. > > End-users are usually concerned with major/minor numbers in that they > don't change (e.g. for find -xdev) and sometimes don't change over > reboots or even re-installations (think about backup). > > The most simple method to assign major numbers is to assign them > during installation. Can be as simple as a file in /etc as a registry, > MAKEDEV to create /dev entries and a system call (probably sysctl) > that augments the device switches. Combining the system call with > the module loader is possible, but won't work for autoloaded modules > (not a problem for HAXM). > I feel corrected that non -U builds need it for other purposes, mostly file grants. As discussed internally an issue with devfs is that (according to my observation) that expectations raise too much and people start to demand functionality comparable to automounter. Something similar appeared in Linux with all the devfs, udev, sysfs etc.. Also touching major numbers can trigger temptation to change namespace of devices from numbers to strings (I don't really see win myself in exchange for compat breakage)... so better to leave it as it is. For cases such as HAXM the least bad solution is to add haxmfs and union mount over /dev (providing /dev/hax_vm*/* nodes).. however until it will become a blocker to ship only 8 VMs x 16 VCPUs I will keep using traditional mknod(2) approach. If there are no more concerns I will apply this patch: http://netbsd.org/~kamil/patch-00086-pkgsrc-majors.2.txt Probably with bumped major numbers as we are soon acquiring another major for KCOV. signature.asc Description: OpenPGP digital signature
