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

yuripv created this revision.Aug 28 2019, 12:59 PM
hselasky added inline comments.Aug 28 2019, 1:47 PM
sys/dev/ichsmb/ichsmb.c
642 ↗(On Diff #61397)

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

hselasky added inline comments.Aug 28 2019, 1:48 PM
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?

yuripv updated this revision to Diff 61398.Aug 28 2019, 1:52 PM

Can you upload full context?

Done, sorry about that.

hselasky added a comment.EditedAug 28 2019, 1:59 PM

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.

ian added a subscriber: ian.Aug 28 2019, 3:14 PM

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 updated this revision to Diff 61415.Aug 28 2019, 7:07 PM
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!

ian accepted this revision.Aug 28 2019, 7:19 PM
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 :)

ian added a comment.Aug 29 2019, 1:02 AM

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. :)

hselasky accepted this revision.Aug 29 2019, 7:16 AM
This revision was automatically updated to reflect the committed changes.
imp added a comment.Aug 29 2019, 2:20 PM

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