Page MenuHomeFreeBSD

ichsmb: defer smbus attach until interrupts are available
ClosedPublic

Authored by yuripv on Aug 28 2019, 12:59 PM.

Details

Summary

This fixes a "timed sleep before timers are working" panic seen while attaching jedec_dimm(4) instances too early in the boot.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sys/dev/ichsmb/ichsmb.c
642 ↗(On Diff #61397)

using hz here is wrong. timeout_us = 1000000 / 4;

sys/dev/ichsmb/ichsmb.c
649 ↗(On Diff #61397)

Without early AP startup option I doubt that the IRQ handler will run.
Should you call the IRQ handler from this function?

Can you upload full context?

Done, sorry about that.

If this driver doesn't need to started early have a look at this:

We could modify SI_SUB_DRIVERS in the macro below to load the driver at a later point in time!

#define EARLY_DRIVER_MODULE_ORDERED(name, busname, driver, devclass,    \
    evh, arg, order, pass)                                              \
                                                                        \
static struct driver_module_data name##_##busname##_driver_mod = {      \
        evh, arg,                                                       \
        #busname,                                                       \
        (kobj_class_t) &driver,                                         \
        &devclass,                                                      \
        pass                                                            \
};                                                                      \
                                                                        \
static moduledata_t name##_##busname##_mod = {                          \
        #busname "/" #name,                                             \
        driver_module_handler,                                          \
        &name##_##busname##_driver_mod                                  \
};                                                                      \
DECLARE_MODULE(name##_##busname, name##_##busname##_mod,                \
               SI_SUB_DRIVERS, order)

/sys/sys/bus.h:#define	DRIVER_MODULE_ORDERED(name, busname, driver, devclass, evh, arg,\

If this driver doesn't need to started early have a look at this:

We could modify SI_SUB_DRIVERS in the macro below to load the driver at a later point in time!

#define EARLY_DRIVER_MODULE_ORDERED(name, busname, driver, devclass,    \
    evh, arg, order, pass)                                              \
                                                                        \
static struct driver_module_data name##_##busname##_driver_mod = {      \
        evh, arg,                                                       \
        #busname,                                                       \
        (kobj_class_t) &driver,                                         \
        &devclass,                                                      \
        pass                                                            \
};                                                                      \
                                                                        \
static moduledata_t name##_##busname##_mod = {                          \
        #busname "/" #name,                                             \
        driver_module_handler,                                          \
        &name##_##busname##_driver_mod                                  \
};                                                                      \
DECLARE_MODULE(name##_##busname, name##_##busname##_mod,                \
               SI_SUB_DRIVERS, order)

/sys/sys/bus.h:#define	DRIVER_MODULE_ORDERED(name, busname, driver, devclass, evh, arg,\

Thanks, I didn't think about EARLY_STARTUP_APS being undefined and it doesn't seem to go away any time soon, so I'm now looking into this option making driver attach late.

IMO, the much easier fix is to change the call to bus_generic_attach() at the end of the ichsmb_attach() routine to:

config_intrhook_oneshot((ich_func_t)bus_generic_attach, dev);
return (0);

We really need to do it to all i2c/smb controller drivers. I've done most of the arm ones already.

yuripv retitled this revision from ichsmb: poll for cmd completion status instead of sleeping if cold booted to ichsmb: defer smbus attach until interrupts are available.
yuripv edited the summary of this revision. (Show Details)
yuripv marked 2 inline comments as done.
In D21452#466886, @ian wrote:

IMO, the much easier fix is to change the call to bus_generic_attach() at the end of the ichsmb_attach() routine to:

config_intrhook_oneshot((ich_func_t)bus_generic_attach, dev);
return (0);

We really need to do it to all i2c/smb controller drivers. I've done most of the arm ones already.

Thank you!

This revision is now accepted and ready to land.Aug 28 2019, 7:19 PM

@ian I'm not sure if I should put you in 'submitted by' or 'reviewed by' line, you did both :)

I never put either of those things in a commit message if there's a phab to cite, because it has all the details. But I think I'm in a minority about that. Anyway, no need to mention me in the commit at all. :)

Looks great to me too... Forgot to click Accept yesterday when I looked at it.