Page MenuHomeFreeBSD

Honor the FWUG value of some drives in nvmecontrol
ClosedPublic

Authored by dab on Thu, Sep 10, 1:11 PM.

Details

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

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

dab created this revision.Thu, Sep 10, 1:11 PM
dab requested review of this revision.Thu, Sep 10, 1:11 PM
chuck added inline comments.Thu, Sep 10, 2:05 PM
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 added inline comments.Thu, Sep 10, 2:17 PM
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 added inline comments.
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.

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 accepted this revision.Mon, Sep 14, 5:39 PM

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.

This revision is now accepted and ready to land.Mon, Sep 14, 5:39 PM
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 updated this revision to Diff 77096.Wed, Sep 16, 1:25 PM

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

This revision now requires review to proceed.Wed, Sep 16, 1:25 PM
dab marked 2 inline comments as done.Wed, Sep 16, 1:30 PM
dab added inline comments.
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 updated this revision to Diff 77184.Fri, Sep 18, 2:43 PM

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

dab marked an inline comment as done.Fri, Sep 18, 2:44 PM
chuck added a comment.Fri, Sep 18, 4:34 PM

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 accepted this revision.Fri, Sep 18, 4:43 PM

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

This revision is now accepted and ready to land.Fri, Sep 18, 4:43 PM
This revision was automatically updated to reflect the committed changes.