Page MenuHomeFreeBSD
Differential D55541

nvme: Don't active memory space until all BARs are configured
ClosedPublic
Actions

Authored by ziaee on Thu, Feb 26, 7:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 22, 9:31 PM
Unknown Object (File)
Fri, Mar 20, 7:26 AM
Unknown Object (File)
Thu, Mar 19, 1:52 AM
Unknown Object (File)
Tue, Mar 17, 8:06 AM
Unknown Object (File)
Tue, Mar 17, 6:04 AM
Unknown Object (File)
Fri, Mar 13, 4:56 PM
Unknown Object (File)
Wed, Mar 11, 1:05 PM
Unknown Object (File)
Wed, Mar 11, 10:01 AM
View All 43 Files
Subscribers
bdrewery
cperciva
dab
fuz
jrtc27
kevans

Details

Reviewers
imp
glebius
gallatin
Commits
rGb3d9e5013f3e: nvme: Don't active memory space until all BARs are configured
Summary

In the current current behavior the 2nd and 3rd BARs can be activated
when they're configured with address zero. This change defers the
activation of all BARs until after they've all been configured with an
address.

Proxying on behalf of Matt Delco <delco@google.com>, I don't know if
phabricator will mangle the author. Tested by NetApp, I think this fixes
a bug where their FreeBSD based OS is crashing on GCE.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 71073
Build 67956: arc lint + arc unit

Event Timeline

ziaee created this revision.Thu, Feb 26, 7:47 PM
Herald added a subscriber: dab. · View Herald TranscriptThu, Feb 26, 7:47 PM
ziaee requested review of this revision.Thu, Feb 26, 7:47 PM
Harbormaster completed remote builds in B71035: Diff 172789.Thu, Feb 26, 7:47 PM
gallatin added a comment.Thu, Feb 26, 8:34 PM

This looks OK to me, but I defer to Warner.

imp added a comment.Fri, Feb 27, 1:19 AM

This will work as is, but I have a couple of comments to make it a bit better.

sys/dev/nvme/nvme_pci.c
156

These need to be bool.

244

The tag/handle doesn't change when we activate the resource, so these lines don't need to move, imho.

ziaee updated this revision to Diff 172879.Fri, Feb 27, 5:29 PM
ziaee added a subscriber: fuz.

attempt to convert to bool, thanks @fuz

Harbormaster completed remote builds in B71073: Diff 172879.Fri, Feb 27, 5:29 PM
cperciva added a subscriber: cperciva.Fri, Feb 27, 6:05 PM
cperciva added inline comments.
sys/dev/nvme/nvme_pci.c
153

You lost the opening { somehow.

jrtc27 added a subscriber: jrtc27.Fri, Feb 27, 6:08 PM
jrtc27 added inline comments.
sys/dev/nvme/nvme_pci.c
213

Given bus_activate_resource returns an error code it might be nice to print it?

216

Is there a reason to use these bools? Won't the bool be true if and only if the corresponding resource member of ctrlr is non-NULL?

ziaee updated this revision to Diff 172881.Fri, Feb 27, 6:15 PM

restore missing brace, thanks @cperciva!

Harbormaster completed remote builds in B71075: Diff 172881.Fri, Feb 27, 6:15 PM
ziaee updated this revision to Diff 172889.Fri, Feb 27, 8:08 PM

finish converting to bool

Harbormaster completed remote builds in B71078: Diff 172889.Fri, Feb 27, 8:08 PM
imp added a comment.Fri, Feb 27, 8:15 PM

I think @jrtc27 is right about the observation that we don't need the bools.

kevans added a subscriber: kevans.Fri, Feb 27, 8:15 PM
kevans added inline comments.
sys/dev/nvme/nvme_pci.c
34

This doesn't need to be here, sys/param.h includes sys/types.h (see style(9)) -- it would have been sorted higher if it was necessary, though

ziaee updated this revision to Diff 172890.Fri, Feb 27, 8:25 PM

attempt to restore ints

Harbormaster completed remote builds in B71079: Diff 172890.Fri, Feb 27, 8:25 PM
jrtc27 added a comment.Fri, Feb 27, 9:47 PM

attempt to restore ints

That's not what we meant by "don't need the bools"; rather you can instead check e.g. ctrlr->msix_table_resource != NULL instead of activate_msi_bar.

ziaee updated this revision to Diff 173184.Thu, Mar 5, 1:32 PM

okay, matt updated the patch

Harbormaster completed remote builds in B71193: Diff 173184.Thu, Mar 5, 1:32 PM
gallatin accepted this revision.Thu, Mar 5, 1:51 PM
This revision is now accepted and ready to land.Thu, Mar 5, 1:51 PM
imp accepted this revision.Thu, Mar 5, 6:16 PM

I think this has implemented the suggested changes to my suggested changes that @jrtc27 suggested.

ziaee updated this revision to Diff 173250.Fri, Mar 6, 2:01 AM

The last patch didn't work in testing on my laptop. Sorry for the noise. Here is an updated patch which I am currently booted from.

This revision now requires review to proceed.Fri, Mar 6, 2:01 AM
Harbormaster completed remote builds in B71215: Diff 173250.Fri, Mar 6, 2:01 AM
imp accepted this revision.Fri, Mar 6, 4:56 PM
This revision is now accepted and ready to land.Fri, Mar 6, 4:56 PM
gallatin accepted this revision.Fri, Mar 6, 5:25 PM
Closed by commit rGb3d9e5013f3e: nvme: Don't active memory space until all BARs are configured (authored by Matt Delco <delco@google.com>, committed by ziaee). · Explain WhyFri, Mar 6, 5:32 PM
This revision was automatically updated to reflect the committed changes.
ziaee added a commit: rGb3d9e5013f3e: nvme: Don't active memory space until all BARs are configured.
bdrewery added a subscriber: bdrewery.Fri, Mar 6, 5:32 PM

Commit message typo "current current"

ziaee added a comment.Fri, Mar 6, 5:33 PM

Thanks so much everyone!

bdrewery added a comment.Fri, Mar 6, 5:42 PM

It was pointed out to me that "current current" should be interpreted as "current CURRENT" which makes sense. So not a typo.

Revision Contents
Changeset List

PathSize
sys/
dev/
nvme/
35 lines

Diff 172879

View Options

sys/dev/nvme/nvme_pci.c

Loading...