Re: -10 panic during dbcool attach

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Thu, 13 Feb 2025 18:28:09 +0100
> From: Edgar Fuß <ef%math.uni-bonn.de@localhost>
> 
> A (custom) -10/amd64 kernel panics on certain hardware during dbcool attach:
> 
> ...
> [  1.0438364] dbcool0 at iic0 addr 0x2euvm_fault (0xffffffff810f0340, 0x0, 2) -> e

This is a null pointer dereference.

> db{0}> bt
> atomic_inc32_nv() at netbsd:_atomic inc 32 nv+0x5
> config_attach_internal() at netbsd:config_attach_internal+0x214
> ...

It probably happened inside dbcool_attach, in a leaf subroutine that
didn't create a stack frame but called atomic_inc_32_nv (or possibly
its alias atomic_inc_uint_nv).

> A (custom) -8 kernel attaches dbcool on that hardware:
> 
> ...
> dbcool0 at iic0 addr 0x2e
> dbcool0: ADT7476 dBCool(tm) Controller (rev 0x0069)

It happened _before_ the identification string was printed.  So that
narrows it down to this:

    778 	sc->sc_prop = args->ia_prop;
    779 	prop_object_retain(sc->sc_prop);

https://nxr.netbsd.org/xref/src/sys/dev/i2c/dbcool.c?r=1.64#778

I bet args->ia_prop is null in this case.  What prop_object does is
atomic_inc_32_nv, unconditionally:

   1000 	_PROP_ATOMIC_INC32_NV(&po->po_refcnt, ncnt);

https://nxr.netbsd.org/xref/src/common/lib/libprop/prop_object.c?r=1.35#1000

> > A (custom) -10/amd64 kernel
> The relevant config entry I added is
> 	dbcool* at iic? addr 0x2E
> but that works under -8.

This use-case was probably not tested when macallan@ added logic to
get sensor names from i2c properties if available and thorpej@ added
the call to prop_object_retain:

https://mail-index.netbsd.org/source-changes/2020/07/12/msg119271.html
https://mail-index.netbsd.org/source-changes/2021/01/30/msg126423.html

Can you please file a PR explaining this use-case (including details
of why you are using this dbcool attachment instead of, say, getting
the sensor data through an ACPI or FDT firmware binding, and how you
know it's safe on this board) and all the symptoms you just shared?

Adding a null test is probably all this code needs but it's not clear
to me a priori that this static configuration is sensible, so I'd just
like to make sure of that first.  (The PR will help to track pullups.)