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.
Details
Details
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
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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? |
| 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. |
| 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. |
| 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. |
| sys/dev/mpr/mpr_sas.c | ||
|---|---|---|
| 1036 ↗ | (On Diff #28687) |
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. |
| 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? |
| 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 |
| 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. |
| 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. |
| 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. |
| 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. |