Page MenuHomeFreeBSD

ahci: extend init timeouts for Marvell 88SE9220 (Dell BOSS-S1) controller
Needs ReviewPublic

Authored by dch on Dec 1 2022, 3:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
May 9 2024, 2:20 AM
Unknown Object (File)
Apr 29 2024, 4:54 AM
Unknown Object (File)
Apr 27 2024, 7:57 AM
Unknown Object (File)
Apr 26 2024, 6:54 AM
Unknown Object (File)
Apr 26 2024, 12:18 AM
Unknown Object (File)
Apr 26 2024, 12:17 AM
Unknown Object (File)
Apr 25 2024, 11:31 PM
Unknown Object (File)
Apr 23 2024, 11:57 AM
Subscribers

Details

Reviewers
imp
mav
Summary
Test Plan

Works On My Machine: yes
Submitted By: Peter Eriksson <pen@lysator.liu.se>
Tested By: Lorenzo Perone <lopez.on.the.lists@yellowspace.net>

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48596
Build 45482: arc lint + arc unit

Event Timeline

dch requested review of this revision.Dec 1 2022, 3:47 PM
dch created this revision.

fix 2 errors during patch trimming

emaste added inline comments.
sys/dev/ahci/ahci.c
2688–2689

whitespace changes

sys/dev/ahci/ahci.h
624

We don't seem to use this quirk?

sys/dev/ahci/ahci_pci.c
1

Unrelated change?

/*- is used (in some files) to indicate a preformatted block of text that should not be rewrapped IIRC, most commonly on copyright/license blocks.

294

This line just adds a new device that does not need the extended timeouts?

This change looks like it still has half of the debugging code in it. That should really be cleaned up. It obscures that the real fix is

sys/dev/ahci/ahci.c
2364

This really isn't timeout, but retry count since you might be reschedule. If you want the time bounded, you should use ab sbtime instead.

2378–2392

this code is pointless. We don't ever use last_cr, etc, they are write-only variables.

2655

Same comment about the retry count vs elapsed time. Here it likely matters more since we're waiting for a device to connect and rescheduling could lead to rather longer delays which can be more of a problem in the > 10ms raange. though the original code did also suffer from that problem a bit.

I also don't like the structure here. Why do we need to increase the timeout only after the first status? that makes no sense, why not have a ?: assignment for timeout_full_limit here?

2660

Again, last_status is write-only

2668

This especially suggests you'd want to restructure to use a sbtime.

2671

We never set this quirk on any device, so let's drop this change until we have one.