Honor the FWUG value of some drives in nvmecontrol

[dab]

Summary

nvmecontrol tries to upload firmware in chunks as large as it thinks
the device permits. It fails to take into account the FWUG value used
by some drives to advertise the size and alignment limits for firmware
chunks.

• Add an ioctl to get the device's max transfer size
• Use the firwmare update granularity value from the identify-controller response to set the max transfer size.
• If the granularity is not reported or not restricted, fall back to the previously existing logic that calculates the max transfer size based on MDTS.
• Add firmware update granularity to the identify-controller output.

Test Plan

Tested in system at $JOB

[chuck]

sbin/nvmecontrol/firmware.c

175 ↗

(On Diff #76873)

Would it make sense to allocate max_xfer_size bytes for chunk?

179 ↗

(On Diff #76873)

Instead of doing this in two steps, does it make sense to simplify the two statements into:

size = (resid >= (int32_t)max_xfer_size) ?
		max_xfer_size : resid;

[mav]

sbin/nvmecontrol/firmware.c

173 ↗

(On Diff #76873)

I am not sure about this part. Specification does not tell what exactly granularity means in this context. Is it really a maximum size, not just an alignment that you can multiply? In the later case I would fetch max_xfer_size from kernel and round it down to fwug << 12.

182 ↗

(On Diff #76873)

Those two comparisons are excessive. max_xfer_size returned by kernel can not be bigger then NVME_MAX_XFER_SIZE. It can be decided before the loop.

[rramsden_isilon.com]

sbin/nvmecontrol/firmware.c

173 ↗

(On Diff #76873)

I think you're right. As I read the spec, granularity is a block size, not a maximum transfer size.

[dave_harder_dell.com]

sbin/nvmecontrol/firmware.c

173 ↗

(On Diff #76873)

I have encountered drives from at least one major vendor that have a maximum transfer size for f/w download that is smaller than MDTS.

The spec states that the NUMD field in the Firmware Image Download command has to adhere to FWUG requirements (not just the OFST field). This implies that FWUG defines both granularity and alignment, not just alignment.

Finally, I agree that the wording of the spec is not entirely clear (and conflicts with the reality of some drives). Since there is ambiguity, the safest (and most compatible) approach is to only transfer FWUG bytes of data with each Firmware Image Download command.

[imp]

Looks good to me. I'd be tempted to commit the kernel and non-kernel parts separately since the kernel bits aren't directly related to the firmware downloading change, but merely facilitate it (and possibly other) uses.

[rramsden_isilon.com]

sbin/nvmecontrol/firmware.c

173 ↗

(On Diff #76873)

OK. That makes sense.

One thing here that worries me is that if we override FWUG or MTDS with NVME_MAX_XFER_SIZE, this could make the firmware update fail -- because it could affect both the size and alignment of transfers. We do this silently. It seems like there should be a warning.

Note that I think this revision is OK as is.

[dab]

Combine statements that calculate the transfer size as suggested by review comments.

[dab]

sbin/nvmecontrol/firmware.c

175 ↗

(On Diff #76873)

That could be done. max_xfer_size isn't going to exceed NVME_MAX_XFER_SIZE, so using the latter size is OK; but I suppose it would make more sense to allocate using max_xfer_size since that becomes the limiting size factor for transfers in subsequent code.

Any strong feelings to the contrary?

[dab]

Allocate the buffer based on max_xfer_size as suggested by a review comment.

[chuck]

FWIW, I checked with our NVMe standards person who says:

I would say it should be treated as a unit. FWUG can be viewed as the smallest chunk that the firmware can be broken into. If it is 4K then you can download 64K chunks since that is a multiple of 4K but an attempt to download a 62K chunk might be rejected (or might be really slow). In that example NUMD would be 64K for every transfer and OFST would be some multiple of 64K (which is also a multiple of 4K). OFST would be 0, 64K, 128K, etc.

[chuck]

I'd love to see the addition of a warning, but am happy with these changes.

sbin/nvmecontrol/firmware.c

173 ↗

(On Diff #76873)

I also like the idea of warning if max_xfer_size is truncated but only in the case that fwug != 0 && fwug != 0xFF