Page MenuHomeFreeBSD

[2/2] mps(4): remove SATA ID command cancellation hack
ClosedPublic

Authored by cem on Dec 20 2018, 12:32 AM.

Details

Summary

Add a generic mechanism to override mps_wait_command's timeout behavior,
which continues to invoke reinit by default. Invokers who set
cm_timeout_handler may avoid automatic reinit and do their own handling.

Adapt mpssas_get_sata_identify to this mechanism and remove its callout
hack.

Test Plan

It compiles, but I have not tested it further in any way.

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

cem created this revision.Dec 20 2018, 12:32 AM
cem added inline comments.Dec 20 2018, 12:34 AM
sys/dev/mps/mps.c
3090 ↗(On Diff #52183)

Unrelated to this change, is this valid if !mtx_owned(sc->mps_mtx)? Line 3077 above suggests it may not be owned. If it is expected to be owned, maybe mtx_assert() is appropriate?

One small thing to consider is whether to create a new typedef for cm_timeout_handler, even if it's the same implementation is mps_command_callback_t for now. The reason is that we might want to evolve this into a different function call signature for the timeout handler. This might be useful for mps_scsiio_timeout().

Also, can you add the same changes for mpr?

cem added a comment.Dec 20 2018, 4:44 AM

One small thing to consider is whether to create a new typedef for cm_timeout_handler, even if it's the same implementation is mps_command_callback_t for now. The reason is that we might want to evolve this into a different function call signature for the timeout handler. This might be useful for mps_scsiio_timeout().

I figure we can just make the new definition if needed, if/when we cross that bridge. So I have a weak preference for just keeping it as-is. If you have a strong preference to do it now instead, I'm happy to change it.

Also, can you add the same changes for mpr?

Yeah, no problem. To avoid duplicating the same iterative changes during review, though, I'd prefer to wait until we get to a point where we're happy with the mps changes alone first, then copy them to mpr and confirm the mpr changes look ok as well. Is that ok, or would you prefer having both sets of changes sooner? I'm ok doing that, it's just more work for me.

Thanks for taking a look.

scottl accepted this revision.Dec 20 2018, 5:03 AM
This revision is now accepted and ready to land.Dec 20 2018, 5:03 AM
cem updated this revision to Diff 52208.Dec 21 2018, 12:30 AM

Copy to mpr(4) as well

This revision now requires review to proceed.Dec 21 2018, 12:30 AM
scottl accepted this revision.Dec 21 2018, 8:24 PM
This revision is now accepted and ready to land.Dec 21 2018, 8:24 PM
This revision was automatically updated to reflect the committed changes.