Page MenuHomeFreeBSD

Fix several problems with mapping code in the mpr(4) driver
ClosedPublic

Authored by slm on May 22 2017, 6:59 PM.

Details

Summary

mav@ pointed out a problem with the mapping code in the mps driver where devices were not getting mapped due to no space available. After looking into this, I found multiple problems with the mapping code, so this turned into quite a big change. I plan on getting this into 11.1 and it should also go into the mps driver. (This change is for mpr only.) I added a lot of comments which were missing so hopefully that helps in the review.

Test Plan

I couldn't test everything, but I did lots of testing with Enclosure/Slot mapping and Persistent mapping using 2 enclosures with 24 disks each, and using a direct-attached JBOD with 4 disks. I also tested with and without IR support with Low IDs and High IDs, depending on settings in IOC Config page 8.

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

slm created this revision.May 22 2017, 6:59 PM
ken requested changes to this revision.May 23 2017, 9:08 PM

This generally looks good, except for the printf format issues. Fix those and it'll be ready to go in.

sys/dev/mpr/mpr_mapping.c
1272 ↗(On Diff #28687)

Need to use %jx and case to uintmax_t here.

1423 ↗(On Diff #28687)

You need to use jx and case to uintmax_t here.

This revision now requires changes to proceed.May 23 2017, 9:08 PM
slm added inline comments.May 23 2017, 9:26 PM
sys/dev/mpr/mpr_mapping.c
1181 ↗(On Diff #28687)

sata_end_device is removed from latest - not used.

1272 ↗(On Diff #28687)

Fixed.

1423 ↗(On Diff #28687)

Fixed

sys/dev/mpr/mprvar.h
399 ↗(On Diff #28687)

num_channels is removed from latest - no used.

mav accepted this revision.May 23 2017, 9:50 PM

Thank you for working on this.

sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

Is there some good reason to return this particular target ID here, calculating it so verbosely? Does the HBA really occupy the last entry in the port mapping table, or just some arbitrary value outside of the allowed range can be reported here?

slm added inline comments.May 23 2017, 10:01 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

If IR volume IDs are kept at the end of the mapping table (a setting in IOC Page 8), the initiator_id will be put there if this check isn't done, and that causes problems with booting. I don't know what the initiator_id is used for, but I had to move it if volume IDs are at the end of the table. Volume IDs are kept either at the beginning of the mapping table or the end. Everything works fine if they are at the beginning but the end needs this check.

mav added inline comments.May 23 2017, 10:20 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

initiator_id AFAIK is used in two cases: in initiator mode it is excluded from scanning (to not make initiator scan itself), while in target mode it is used to bind target peripheral drivers to.

I am not sure what booting problem do you mean, but I can guess that old value could block detection/access of some device/volume if it unluckily get this specific ID assigned. From this perspective, for example, isp(4) driver just reports value out of supported target ID range there, since it is never accessed in initiator mode any way, and in target mode target IDs are handled specifically, not depending on port mapping table. I.e I guess "cpi->initiator_id = sassc->maxtargets;" could possibly do the same trick.

slm added inline comments.May 23 2017, 10:27 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

The boot problem I had was if I created a volume and its ID was kept at the end of the mapping table, the boot would hang because xpt_config was not able to set buses_to_config to 0. It always stopped at 1 and waited forever to get to 0 (until the system paniced). Moving the initiator_id's ID to just before the volume IDs fixed the problem. The volume IDs should be reserved only for volumes otherwise there will be things there that the driver doesn't expect.

mav added inline comments.May 23 2017, 11:08 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

The volume IDs should be reserved only for volumes otherwise there will be things there that the driver doesn't expect.

That is why I proposed to more initiator_id after them. I am not sure where reserving last target ID for initiator is a correct practice from the HBA point of view, or it is expected to be allowed to be used for target.

slm added inline comments.May 24 2017, 3:16 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

I didn't quite follow you before, but now I see what you're saying. There are several asserts in the driver that check that the ID is in the proper range. If I made the initiator_id = max_targets, where max_targets includes volumes, I would need to adjust those asserts to be plus one, right?

slm added inline comments.May 24 2017, 6:13 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

I did a little testing after setting initiator_id = sassc->maxtargets and it works fine. I added this comment. Is it OK?

initiator_id is set here to an ID outside of the set of valid
target IDs (including volumes). This ID is not expected to be
used, so all target ID asserts only check for one less than
this (0 thru maxtargets -1). If this ID is used, the assert will fail
and the system will panic.

mav added inline comments.May 24 2017, 8:02 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

As I have told, initiator_id is never addressed in initiator mode, and there is no target mode in the driver now any way.

slm added inline comments.May 24 2017, 8:08 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

So, do you think the comment is ok, or maybe unnecessary, or too much? I don't want to confuse anyone.

mav added inline comments.May 24 2017, 8:14 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

It is OK, for me if you wish it there. It may be called too verbose, but that probably depends on reviewer.

slm added inline comments.May 24 2017, 8:39 PM
sys/dev/mpr/mpr_sas.c
1036 ↗(On Diff #28687)

As long as you're good with the implementation, I might modify the comment before I commit. Thanks.

This revision was automatically updated to reflect the committed changes.