Page MenuHomeFreeBSD
Differential D55541

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

Authored by ziaee on Feb 26 2026, 7:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Sep 8, 12:37 AM
Unknown Object (File)
Mon, Sep 7, 11:51 PM
Unknown Object (File)
Mon, Sep 7, 9:15 PM
Unknown Object (File)
Mon, Sep 7, 4:38 PM
Unknown Object (File)
Mon, Sep 7, 2:45 PM
Unknown Object (File)
Mon, Sep 7, 2:15 PM
Unknown Object (File)
Mon, Sep 7, 8:40 AM
Unknown Object (File)
Sun, Sep 6, 6:18 PM
View All Files
Subscribers
bdrewery
cperciva
dab
fuz
jrtc27
kevans

Details

Reviewers
imp
glebius
gallatin
Commits
rG925387f94432: nvme: Don't active memory space until all BARs are configured
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 71193
Build 68076: arc lint + arc unit

Event Timeline

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

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

imp added a comment.Feb 27 2026, 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
155

These need to be bool.

225

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.Feb 27 2026, 5:29 PM
ziaee added a subscriber: fuz.

attempt to convert to bool, thanks @fuz

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

You lost the opening { somehow.

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

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

213

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.Feb 27 2026, 6:15 PM

restore missing brace, thanks @cperciva!

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

finish converting to bool

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

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

kevans added a subscriber: kevans.Feb 27 2026, 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.Feb 27 2026, 8:25 PM

attempt to restore ints

Harbormaster completed remote builds in B71079: Diff 172890.Feb 27 2026, 8:25 PM
jrtc27 added a comment.Feb 27 2026, 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.Mar 5 2026, 1:32 PM

okay, matt updated the patch

Harbormaster completed remote builds in B71193: Diff 173184.Mar 5 2026, 1:32 PM
gallatin accepted this revision.Mar 5 2026, 1:51 PM
This revision is now accepted and ready to land.Mar 5 2026, 1:51 PM
imp accepted this revision.Mar 5 2026, 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.Mar 6 2026, 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.Mar 6 2026, 2:01 AM
Harbormaster completed remote builds in B71215: Diff 173250.Mar 6 2026, 2:01 AM
imp accepted this revision.Mar 6 2026, 4:56 PM
This revision is now accepted and ready to land.Mar 6 2026, 4:56 PM
gallatin accepted this revision.Mar 6 2026, 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 WhyMar 6 2026, 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.Mar 6 2026, 5:32 PM

Commit message typo "current current"

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

Thanks so much everyone!

bdrewery added a comment.Mar 6 2026, 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.

ziaee added a commit: rG925387f94432: nvme: Don't active memory space until all BARs are configured.Apr 13 2026, 7:55 PM

Revision Contents
Changeset List

PathSize
sys/
dev/
nvme/
36 lines

Diff 173184

View Options

sys/dev/nvme/nvme_pci.c

Loading...