Page MenuHomeFreeBSD

add mrsas_shutdown method
ClosedPublic

Authored by avg on Apr 4 2019, 6:51 AM.

Details

Summary

It should be safer to flush controller and disk caches on the shutdown.
And to gracefully shut down the controller as well.

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

avg created this revision.Apr 4 2019, 6:51 AM
avg updated this revision to Diff 55797.Apr 4 2019, 6:57 AM
  • update
imp added inline comments.Apr 4 2019, 5:17 PM
sys/dev/mrsas/mrsas.c
1197 ↗(On Diff #55797)

is there an upper bound here? And why only for panicstr != NULL? I get it's only while we're panicing, but why do we have to do this when we panic and not otherwise.

avg added inline comments.Apr 4 2019, 5:46 PM
sys/dev/mrsas/mrsas.c
1197 ↗(On Diff #55797)

I copied this code verbatim from mrsas_detach, so there is no upper limit in both places.

And it's actually the other way around, we do this only when we are not panic-ing. This code wakes another thread and waits for it to clear a flag. But the other thread, of course, cannot run if we are in panic.

avg added a reviewer: scottl.Apr 8 2019, 6:20 PM
avg updated this revision to Diff 55992.Apr 9 2019, 1:35 PM

remove a wrong panicstr check that was left by accident

imp accepted this revision.Apr 9 2019, 5:20 PM

This looks good to me, however I'd wait a few days to see if Scott can spot something that I can't.

sys/dev/mrsas/mrsas.c
1197 ↗(On Diff #55797)

I'm glad to see you got rid of the panicstr here, that's what I was really commenting about.

This revision is now accepted and ready to land.Apr 9 2019, 5:20 PM
avg added a comment.Apr 9 2019, 7:05 PM
In D19817#426388, @imp wrote:

This looks good to me, however I'd wait a few days to see if Scott can spot something that I can't.

I also sent a link to this review to Kashyap Desai.
Waiting for his / Broadcom's reply as well.

avg added inline comments.Apr 9 2019, 7:07 PM
sys/dev/mrsas/mrsas.c
1197 ↗(On Diff #55797)

I was so sure that I removed it that I just didn't see it.

scottl added inline comments.Apr 23 2019, 3:02 AM
sys/dev/mrsas/mrsas.c
1199 ↗(On Diff #55992)

This can be done with pps_ratecheck() instead.

1209 ↗(On Diff #55992)

This isn't incorrect, but I'm not comfortable with mesas_shutdown_ctlr() having an unbounded timeout.

This revision was automatically updated to reflect the committed changes.