Re: -10 panic during dbcool attach

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

> Date: Fri, 14 Feb 2025 13:21:46 +0100
> From: Edgar Fuß <ef%math.uni-bonn.de@localhost>
> 
> > Can you please file a PR
> Yes, sure, but ...
> 
> > explaining this use-case
> this sounds like I'm doing something unreasonable?

If you have a reason to do it it may not be unreasonable!  But
usually, if a machine's designers meant for the operating system to
reach into that part of the i2c address space, they would have
described it in some kind of firmware binding.

Often, the hardware interface is actually used by the firmware and
exposed to the operating system through some hardware-independent
interface like acpitz(4).  If the OS tries to use the hardware
interface directly, it might interfere with the firmware's
simultaneous use of it and confuse everyone.

If the hardware interface is meant to be used directly, usually there
is some enumerable firmware description of it -- and we should use
that instead of hard-coding magic numbers into the kernel config.
`acpidump -dt' might help find it if it's there.

It's possible that the machine's designers put the i2c device on the
bus and _didn't_ put any kind of firmware wrapping around it.  If that
happened, I would expect to find it at least described in some kind of
motherboard manual.  But I guess it's also possible that the designers
put extra sensor hardware in, wired it up to the i2c bus, and then
didn't tell anyone, yet you found it anyway.

> > and how you know it's safe on this board
> That once again sounds like I was doing something dangerous or unreasonable.
> I can't remember how I came to that configuration (probabably from some 
> "not configured" messages), but it gives me the data I want

If you can find those `not configured' messages I'd be curious to see
them.  The only way this will happen if there is some bus or firmware
enumeration (like ACPI) that describes the device to the OS -- and if
so, we should be using the bus's description, not a magic number in
the kernel config.

>                                                             (and doesn't 
> cause any harm on hardware where that chip is not present).

This is not a priori clear.  Some other device could be wired up to
the part of the i2c address space you've configured dbcool(4) to use.
Who knows how it will interpret the register reads and writes that
dbcool(4) issues?

> > Adding a null test is probably all this code needs
> Do you mean one guarding the prop_object_retain(sc->sc_prop) call?

That and the prop_dictionary_get call in dbcool_attach_sensor, yes.

> I would be afraid there's a complementing prop_object_release() call 
> somewhere, but I don't see one in dbcool_detach(). Most probably I'm 
> looking in the wrong place.

Yes, dbcool_detach should do prop_object_release -- also conditional
on whether sc_prop is null.