nvme_xpt: Tidy nvme_announce_periph for fabrics support.

[jhb]

Summary

• Read the version from cts.protocol_version.

• Only check xport_specific.nvme for PCI-e info for XPORT_NVME.

[imp]

Ah, this explains an earlier comment.

Will fabrics have a different protocol specific field for the cts? If not, why bother being careful here? I've not seen that code so I don't know, but it would seem a bit odd if it did, though most of nvme_xpt doesn't care... the only other place is in nvme_device_transport which sets the .valid fields to 0 before calling XPT_SET_TRAN_SETTINGS

Also, side note: I think we should be setting max_xfer where I tagged before and printing it here, but that's beyond this scope of this change...

[chuck]

If the transport variable mirrors the Fabric types (i.e., PCIe, RDMA, TCP, FC), this makes sense to me as there are differences (like shown here with the PCIe lane count / speeds) between the -oF transports.

[jhb]

FYI, the hash will change so this link will go dead as I keep rebasing the branch, but here's the current XPORT for Fabrics: https://github.com/freebsd/freebsd-src/commit/8f0f6268974edd44695fec83ea527cbf3af5e8a5

I don't think it makes sense to merge until the actual Fabrics host is closer to landing though.

[jhb]

In D40618#924842, @imp wrote:

Ah, this explains an earlier comment.

Will fabrics have a different protocol specific field for the cts? If not, why bother being careful here? I've not seen that code so I don't know, but it would seem a bit odd if it did, though most of nvme_xpt doesn't care... the only other place is in nvme_device_transport which sets the .valid fields to 0 before calling XPT_SET_TRAN_SETTINGS

It has a different transport settings field. However, this also moves towards pulling the version from protocol_version instead of storing it separately in the structure. See my comment on the earlier review about what I think makes sense longer term, that is splitting ccb_trans_settings_nvme into separate structures: one for the protocol and one for the xport (right now we reuse the same structure for both which is a bit confusing)

Also, side note: I think we should be setting max_xfer where I tagged before and printing it here, but that's beyond this scope of this change...

[imp]

In D40618#925264, @jhb wrote:

In D40618#924842, @imp wrote:

Ah, this explains an earlier comment.

Will fabrics have a different protocol specific field for the cts? If not, why bother being careful here? I've not seen that code so I don't know, but it would seem a bit odd if it did, though most of nvme_xpt doesn't care... the only other place is in nvme_device_transport which sets the .valid fields to 0 before calling XPT_SET_TRAN_SETTINGS

It has a different transport settings field. However, this also moves towards pulling the version from protocol_version instead of storing it separately in the structure. See my comment on the earlier review about what I think makes sense longer term, that is splitting ccb_trans_settings_nvme into separate structures: one for the protocol and one for the xport (right now we reuse the same structure for both which is a bit confusing)

Yea, that likely makes sense.