Bug 210686 - NCQ_TRIM_BROKEN quirk mismatch
Summary: NCQ_TRIM_BROKEN quirk mismatch
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-06-29 07:38 UTC by allg
Modified: 2019-01-21 19:13 UTC (History)
7 users (show)

See Also:


Attachments
patch for ata_da.c (2.37 KB, text/plain)
2016-06-29 07:38 UTC, allg
no flags Details
Fix regex code in cam_strmatch (3.31 KB, patch)
2016-08-18 04:13 UTC, Warner Losh
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description allg 2016-06-29 07:38:29 UTC
Created attachment 171939 [details]
patch for ata_da.c

Crucial MX200 is not blacklisted, but NCQ_TRIM_BROKEN quirk is enabled.

> ada0 at ahcich0 bus 0 scbus0 target 0 lun 0
> ada0: <Crucial CT250MX200SSD3 MU03> ACS-3 ATA SATA 3.x device
> ada0: Serial Number xx
> ada0: 600.000MB/s transfers (SATA 3.x, UDMA6, PIO 8192bytes)
> ada0: Command Queueing enabled
> ada0: 238475MB (488397168 512 byte sectors)
> ada0: quirks=0x2<NCQ_TRIM_BROKEN>

Mismatched quirk entry is:

> 		/*
> 		 * Crucial M500 SSDs all other firmware
> 		 * NCQ Trim doesn't work
> 		 */
> 		{ T_DIRECT, SIP_MEDIA_FIXED, "*", "Crucial CT*M500*", "*" },
> 		/*quirks*/ADA_Q_NCQ_TRIM_BROKEN

"Crucial CT*M500*" is equivalent to "Crucial CT*".
(wildcard '*' in pattern matches rest of name (in cam.c))

I wrote a patch. This patch also includes fix for Micron M510/M550.
Comment 1 Tomoaki AOKI 2016-08-17 06:15:57 UTC
Your patch worked for my Crucial M550 1TB SSD. Thanks.

  stable/11 at r304234, amd64.
  ada0: <Crucial CT1024M550SSD1 MU02> ACS-2 ATA SATA 3.x device

Fixing insufficient wildcard processing and restore original codes here
would be the best way, but for now, your patch is a excellent workaround.
Thanks again!
Comment 2 Warner Losh freebsd_committer freebsd_triage 2016-08-18 04:13:39 UTC
Created attachment 173807 [details]
Fix regex code in cam_strmatch

Maybe this will work better than the ata_da.c patch
Comment 3 Tomoaki AOKI 2016-08-18 10:57:56 UTC
(In reply to Warner Losh from comment #2)

Tried and worked fine. This would be the better fix for the future.
Hope this included in 11.0-RELEASE. Thanks!


% camcontrol devlist
camcontrol devlist
<Crucial CT1024M550SSD1 MU02>      at scbus0 target 0 lun 0 (ada0,pass0)
<Samsung SSD 850 EVO 250GB EMT01B6Q>  at scbus1 target 0 lun 0 (ada1,pass1)
<AHCI SGPIO Enclosure 1.00 0001>   at scbus4 target 0 lun 0 (ses0,pass2)


% dmesg (related part only)
ses0 at ahciem0 bus 0 scbus4 target 0 lun 0
ses0: <AHCI SGPIO Enclosure 1.00 0001> SEMB S-E-S 2.00 device
ses0: SEMB SES Device
ada0 at ahcich0 bus 0 scbus0 target 0 lun 0
ada0: <Crucial CT1024M550SSD1 MU02> ACS-2 ATA SATA 3.x device
ada0: Serial Number ************
ada0: 600.000MB/s transfers (SATA 3.x, UDMA6, PIO 8192bytes)
ada0: Command Queueing enabled
ada0: 976762MB (2000409264 512 byte sectors)
Steering write from 0 kBps to 300000 kBps
ada1 at ahcich1 bus 0 scbus1 target 0 lun 0
ada1: <Samsung SSD 850 EVO 250GB EMT01B6Q> ACS-2 ATA SATA 3.x device
ada1: Serial Number ***************
ada1: 600.000MB/s transfers (SATA 3.x, UDMA6, PIO 512bytes)
ada1: Command Queueing enabled
ada1: 238475MB (488397168 512 byte sectors)
ada1: quirks=0x3<4K,NCQ_TRIM_BROKEN>
Steering write from 0 kBps to 300000 kBps
Comment 4 allg 2016-08-19 02:01:58 UTC
(In reply to Warner Losh from comment #2)

Worked well on my drive. Thanks!

> ada0 at ahcich0 bus 0 scbus0 target 0 lun 0
> ada0: <Crucial CT250MX200SSD3 MU04> ACS-3 ATA SATA 3.x device
> ada0: Serial Number xx
> ada0: 600.000MB/s transfers (SATA 3.x, UDMA6, PIO 8192bytes)
> ada0: Command Queueing enabled
> ada0: 238475MB (488397168 512 byte sectors)
Comment 5 commit-hook freebsd_committer freebsd_triage 2016-08-19 04:30:36 UTC
A commit references this bug:

Author: imp
Date: Fri Aug 19 04:30:30 UTC 2016
New revision: 304443
URL: https://svnweb.freebsd.org/changeset/base/304443

Log:
  Improve the pattern matching so that internal *'s work, as well as
  [set] notation. This fixes pattern matching for recently added drives
  that would set the NCQ Trim being broken incorrectly.

  PR: 210686
  Tested-by: Tomoaki AOKI
  MFC After: 3 days

Changes:
  head/sys/cam/cam.c
Comment 6 Warner Losh freebsd_committer freebsd_triage 2016-08-19 04:32:46 UTC
I'll see about getting it into 11.0R, but the hour is quite late.
Comment 7 Tomoaki AOKI 2016-08-19 13:56:25 UTC
It should be best getting in 11.0-R but it's not mandatory
as the workaround is quite easy (just 1 line in /boot/loader.conf).

But if not got in, announcement of workaround would be needed in RELNOTES
(not only 20160414 entry of /usr/src/UPDATING) not to be missed for newbies.

Thanks.
Comment 8 Tomoaki AOKI 2017-01-04 11:54:08 UTC
(In reply to commit-hook from comment #5)

11.0-RELEASE is already out and there's no objection for more than 4 months.
Any chance for MFC to stable/11?
Comment 9 commit-hook freebsd_committer freebsd_triage 2017-12-11 20:47:55 UTC
A commit references this bug:

Author: asomers
Date: Mon Dec 11 20:47:27 UTC 2017
New revision: 326782
URL: https://svnweb.freebsd.org/changeset/base/326782

Log:
  MFC r304443, r326034, r326065

  r304443 by imp:
  Improve the pattern matching so that internal *'s work, as well as
  [set] notation. This fixes pattern matching for recently added drives
  that would set the NCQ Trim being broken incorrectly.

  PR: 210686
  Tested-by: Tomoaki AOKI

  r326034:
  Fix multiple bugs in cam_strmatch

  * Wrongly matches strings that are shorter than the pattern
  * Fails to match negative character sets
  * Fails to match character sets that aren't at the end of the pattern
  * Fails to match character ranges

  Reviewed by:	imp
  Sponsored by:	Spectra Logic Corp
  Differential Revision:	https://reviews.freebsd.org/D13173

  r326065:
  Fix uninitialized variable from 326034

  Reported by:	Coverity
  CID:		1382887
  X-MFC-With:	326034
  Sponsored by:	Spectra Logic Corp

Changes:
_U  stable/11/
  stable/11/lib/libcam/tests/Makefile
  stable/11/lib/libcam/tests/cam_test.c
  stable/11/lib/libcam/tests/libcam_test.c
  stable/11/sys/cam/cam.c
Comment 10 Vladimir 2017-12-18 14:44:06 UTC
 Tomoaki AOKI, is there workaround without patching the system? I saw something you have posted about loader.conf need 1 line to be added, but you did not provide  information of what exactly it should be. Thanks.
Comment 11 Tomoaki AOKI 2017-12-19 09:48:42 UTC
(In reply to Vladimir from comment #10)

As shown on commit hook (comment 9), this patch is MFC'ed to stable/11.
But if you're on 11.1-RELEASE or 11-RELEASE, you should add below on /boot/loader.conf.

 If no other quirks is required:
    kern.cam.ada.0.quirks="0x0"

 If you need 4k quirks but want to drop NCQ_TRIM one:
    kern.cam.ada.0.quirks="0x1"

      *4k one is bit0 (0x1), and NCQ_TRIM one is bit1 (0x2).

The example above assumes the affected drive is recognized as ada0.
You should change "ada.0" to whatever appropreate.
Comment 12 Vladimir 2017-12-19 16:07:39 UTC
Thank you very much  Tomoaki AOKI!
kern.cam.ada.0.quirks="0x1" solved my problem on 11.1 RELEASE branch.
Comment 13 Oleksandr Tymoshenko freebsd_committer freebsd_triage 2019-01-21 19:13:16 UTC
There is a commit referencing this PR, but it's still not closed and has been inactive for some time. Closing the PR as fixed but feel free to re-open it if the issue hasn't been completely resolved.

Thanks