* [PATCH v2 0/5] libata.force improvements
@ 2022-04-25 1:34 Damien Le Moal
2022-04-25 1:34 ` [PATCH v2 1/5] ata: libata-core: cleanup ata_device_blacklist Damien Le Moal
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 1:34 UTC (permalink / raw)
To: linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
This patch series improves the usefulness of the libata.force kernel
boot parameters to facilitate field debugging of ata drives and adapter
issues by extending the range of horkage flags and link flags that can
be controlled.
The first patch is a simple cleanup of the drive blacklist array.
Patch 2 refactors the declaration of the force_tbl array defining the
possible values that libata.force can take. Patch 3 and 4 extend this
array adding most horkage flags and one link flag. Finally, patch 5
updates the kernel documentation reflecting these changes.
Changes from v1:
* Changed force_lflag_on() macro name to force_lflag() in patch 2.
Patch 3 introduce the on & onoff naming of the macro.
* Updated the commit message of patch 2 and 3.
* Removed underscores in the onoff horkage option names in patch 4.
* Added review tags.
Damien Le Moal (5):
ata: libata-core: cleanup ata_device_blacklist
ata: libata-core: Refactor force_tbl definition
ata: libata-core: Improve link flags forced settings
ata: libata-core: Allow forcing most horkage flags
doc: admin-guide: Update libata kernel parameters
.../admin-guide/kernel-parameters.txt | 71 +++--
drivers/ata/libata-core.c | 271 +++++++++++-------
2 files changed, 217 insertions(+), 125 deletions(-)
--
2.35.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/5] ata: libata-core: cleanup ata_device_blacklist
2022-04-25 1:34 [PATCH v2 0/5] libata.force improvements Damien Le Moal
@ 2022-04-25 1:34 ` Damien Le Moal
2022-04-25 5:49 ` Hannes Reinecke
2022-04-25 1:34 ` [PATCH v2 2/5] ata: libata-core: Refactor force_tbl definition Damien Le Moal
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 1:34 UTC (permalink / raw)
To: linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
Remove the unneeded comma after the last field of the array entries.
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
drivers/ata/libata-core.c | 96 +++++++++++++++++++--------------------
1 file changed, 48 insertions(+), 48 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cceedde51126..bc59c77b99b6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3898,7 +3898,7 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
/* Devices where NCQ should be avoided */
/* NCQ is slow */
{ "WDC WD740ADFD-00", NULL, ATA_HORKAGE_NONCQ },
- { "WDC WD740ADFD-00NLR1", NULL, ATA_HORKAGE_NONCQ, },
+ { "WDC WD740ADFD-00NLR1", NULL, ATA_HORKAGE_NONCQ },
/* http://thread.gmane.org/gmane.linux.ide/14907 */
{ "FUJITSU MHT2060BH", NULL, ATA_HORKAGE_NONCQ },
/* NCQ is broken */
@@ -3924,23 +3924,23 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
/* drives which fail FPDMA_AA activation (some may freeze afterwards)
the ST disks also have LPM issues */
{ "ST1000LM024 HN-M101MBB", NULL, ATA_HORKAGE_BROKEN_FPDMA_AA |
- ATA_HORKAGE_NOLPM, },
+ ATA_HORKAGE_NOLPM },
{ "VB0250EAVER", "HPG7", ATA_HORKAGE_BROKEN_FPDMA_AA },
/* Blacklist entries taken from Silicon Image 3124/3132
Windows driver .inf file - also several Linux problem reports */
- { "HTS541060G9SA00", "MB3OC60D", ATA_HORKAGE_NONCQ, },
- { "HTS541080G9SA00", "MB4OC60D", ATA_HORKAGE_NONCQ, },
- { "HTS541010G9SA00", "MBZOC60D", ATA_HORKAGE_NONCQ, },
+ { "HTS541060G9SA00", "MB3OC60D", ATA_HORKAGE_NONCQ },
+ { "HTS541080G9SA00", "MB4OC60D", ATA_HORKAGE_NONCQ },
+ { "HTS541010G9SA00", "MBZOC60D", ATA_HORKAGE_NONCQ },
/* https://bugzilla.kernel.org/show_bug.cgi?id=15573 */
- { "C300-CTFDDAC128MAG", "0001", ATA_HORKAGE_NONCQ, },
+ { "C300-CTFDDAC128MAG", "0001", ATA_HORKAGE_NONCQ },
/* Sandisk SD7/8/9s lock up hard on large trims */
- { "SanDisk SD[789]*", NULL, ATA_HORKAGE_MAX_TRIM_128M, },
+ { "SanDisk SD[789]*", NULL, ATA_HORKAGE_MAX_TRIM_128M },
/* devices which puke on READ_NATIVE_MAX */
- { "HDS724040KLSA80", "KFAOA20N", ATA_HORKAGE_BROKEN_HPA, },
+ { "HDS724040KLSA80", "KFAOA20N", ATA_HORKAGE_BROKEN_HPA },
{ "WDC WD3200JD-00KLB0", "WD-WCAMR1130137", ATA_HORKAGE_BROKEN_HPA },
{ "WDC WD2500JD-00HBB0", "WD-WMAL71490727", ATA_HORKAGE_BROKEN_HPA },
{ "MAXTOR 6L080L4", "A93.0500", ATA_HORKAGE_BROKEN_HPA },
@@ -3949,22 +3949,22 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
{ "OCZ-VERTEX", "1.30", ATA_HORKAGE_BROKEN_HPA },
/* Devices which report 1 sector over size HPA */
- { "ST340823A", NULL, ATA_HORKAGE_HPA_SIZE, },
- { "ST320413A", NULL, ATA_HORKAGE_HPA_SIZE, },
- { "ST310211A", NULL, ATA_HORKAGE_HPA_SIZE, },
+ { "ST340823A", NULL, ATA_HORKAGE_HPA_SIZE },
+ { "ST320413A", NULL, ATA_HORKAGE_HPA_SIZE },
+ { "ST310211A", NULL, ATA_HORKAGE_HPA_SIZE },
/* Devices which get the IVB wrong */
- { "QUANTUM FIREBALLlct10 05", "A03.0900", ATA_HORKAGE_IVB, },
+ { "QUANTUM FIREBALLlct10 05", "A03.0900", ATA_HORKAGE_IVB },
/* Maybe we should just blacklist TSSTcorp... */
- { "TSSTcorp CDDVDW SH-S202[HJN]", "SB0[01]", ATA_HORKAGE_IVB, },
+ { "TSSTcorp CDDVDW SH-S202[HJN]", "SB0[01]", ATA_HORKAGE_IVB },
/* Devices that do not need bridging limits applied */
- { "MTRON MSP-SATA*", NULL, ATA_HORKAGE_BRIDGE_OK, },
- { "BUFFALO HD-QSU2/R5", NULL, ATA_HORKAGE_BRIDGE_OK, },
+ { "MTRON MSP-SATA*", NULL, ATA_HORKAGE_BRIDGE_OK },
+ { "BUFFALO HD-QSU2/R5", NULL, ATA_HORKAGE_BRIDGE_OK },
/* Devices which aren't very happy with higher link speeds */
- { "WD My Book", NULL, ATA_HORKAGE_1_5_GBPS, },
- { "Seagate FreeAgent GoFlex", NULL, ATA_HORKAGE_1_5_GBPS, },
+ { "WD My Book", NULL, ATA_HORKAGE_1_5_GBPS },
+ { "Seagate FreeAgent GoFlex", NULL, ATA_HORKAGE_1_5_GBPS },
/*
* Devices which choke on SETXFER. Applies only if both the
@@ -3982,54 +3982,54 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
/* 512GB MX100 with MU01 firmware has both queued TRIM and LPM issues */
{ "Crucial_CT512MX100*", "MU01", ATA_HORKAGE_NO_NCQ_TRIM |
ATA_HORKAGE_ZERO_AFTER_TRIM |
- ATA_HORKAGE_NOLPM, },
+ ATA_HORKAGE_NOLPM },
/* 512GB MX100 with newer firmware has only LPM issues */
{ "Crucial_CT512MX100*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM |
- ATA_HORKAGE_NOLPM, },
+ ATA_HORKAGE_NOLPM },
/* 480GB+ M500 SSDs have both queued TRIM and LPM issues */
{ "Crucial_CT480M500*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
ATA_HORKAGE_ZERO_AFTER_TRIM |
- ATA_HORKAGE_NOLPM, },
+ ATA_HORKAGE_NOLPM },
{ "Crucial_CT960M500*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
ATA_HORKAGE_ZERO_AFTER_TRIM |
- ATA_HORKAGE_NOLPM, },
+ ATA_HORKAGE_NOLPM },
/* These specific Samsung models/firmware-revs do not handle LPM well */
- { "SAMSUNG MZMPC128HBFU-000MV", "CXM14M1Q", ATA_HORKAGE_NOLPM, },
- { "SAMSUNG SSD PM830 mSATA *", "CXM13D1Q", ATA_HORKAGE_NOLPM, },
- { "SAMSUNG MZ7TD256HAFV-000L9", NULL, ATA_HORKAGE_NOLPM, },
- { "SAMSUNG MZ7TE512HMHP-000L1", "EXT06L0Q", ATA_HORKAGE_NOLPM, },
+ { "SAMSUNG MZMPC128HBFU-000MV", "CXM14M1Q", ATA_HORKAGE_NOLPM },
+ { "SAMSUNG SSD PM830 mSATA *", "CXM13D1Q", ATA_HORKAGE_NOLPM },
+ { "SAMSUNG MZ7TD256HAFV-000L9", NULL, ATA_HORKAGE_NOLPM },
+ { "SAMSUNG MZ7TE512HMHP-000L1", "EXT06L0Q", ATA_HORKAGE_NOLPM },
/* devices that don't properly handle queued TRIM commands */
{ "Micron_M500IT_*", "MU01", ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
{ "Micron_M500_*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
{ "Crucial_CT*M500*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
{ "Micron_M5[15]0_*", "MU01", ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
{ "Crucial_CT*M550*", "MU01", ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
{ "Crucial_CT*MX100*", "MU01", ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
{ "Samsung SSD 840*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
{ "Samsung SSD 850*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
{ "Samsung SSD 860*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
ATA_HORKAGE_ZERO_AFTER_TRIM |
- ATA_HORKAGE_NO_NCQ_ON_ATI, },
+ ATA_HORKAGE_NO_NCQ_ON_ATI },
{ "Samsung SSD 870*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
ATA_HORKAGE_ZERO_AFTER_TRIM |
- ATA_HORKAGE_NO_NCQ_ON_ATI, },
+ ATA_HORKAGE_NO_NCQ_ON_ATI },
{ "FCCT*M500*", NULL, ATA_HORKAGE_NO_NCQ_TRIM |
- ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ ATA_HORKAGE_ZERO_AFTER_TRIM },
/* devices that don't properly handle TRIM commands */
- { "SuperSSpeed S238*", NULL, ATA_HORKAGE_NOTRIM, },
- { "M88V29*", NULL, ATA_HORKAGE_NOTRIM, },
+ { "SuperSSpeed S238*", NULL, ATA_HORKAGE_NOTRIM },
+ { "M88V29*", NULL, ATA_HORKAGE_NOTRIM },
/*
* As defined, the DRAT (Deterministic Read After Trim) and RZAT
@@ -4047,16 +4047,16 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
* The intel 510 drive has buggy DRAT/RZAT. Explicitly exclude
* that model before whitelisting all other intel SSDs.
*/
- { "INTEL*SSDSC2MH*", NULL, 0, },
-
- { "Micron*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
- { "Crucial*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
- { "INTEL*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
- { "SSD*INTEL*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
- { "Samsung*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
- { "SAMSUNG*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
- { "SAMSUNG*MZ7KM*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
- { "ST[1248][0248]0[FH]*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM, },
+ { "INTEL*SSDSC2MH*", NULL, 0 },
+
+ { "Micron*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM },
+ { "Crucial*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM },
+ { "INTEL*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM },
+ { "SSD*INTEL*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM },
+ { "Samsung*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM },
+ { "SAMSUNG*SSD*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM },
+ { "SAMSUNG*MZ7KM*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM },
+ { "ST[1248][0248]0[FH]*", NULL, ATA_HORKAGE_ZERO_AFTER_TRIM },
/*
* Some WD SATA-I drives spin up and down erratically when the link
--
2.35.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/5] ata: libata-core: Refactor force_tbl definition
2022-04-25 1:34 [PATCH v2 0/5] libata.force improvements Damien Le Moal
2022-04-25 1:34 ` [PATCH v2 1/5] ata: libata-core: cleanup ata_device_blacklist Damien Le Moal
@ 2022-04-25 1:34 ` Damien Le Moal
2022-04-25 5:51 ` Hannes Reinecke
2022-04-25 1:34 ` [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings Damien Le Moal
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 1:34 UTC (permalink / raw)
To: linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
Introduce the macro definitions force_cbl(), force_spd_limit(),
force_xfer(), force_lflag(), force_horkage_on() and
force_horkage_onoff() to define with a more compact syntax the struct
ata_force_param entries in the force_tbl array defined in the function
ata_parse_force_one().
To reduce the indentation of the array declaration, force_tbl definition
is also moved out of the ata_parse_force_one() function. The entries are
also reordered to group them by type of the quirck that is applied.
Finally, fix a comment in ata_parse_force_param() incorrectly
referencing force_tbl instead of ata_force_tbl.
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
drivers/ata/libata-core.c | 139 ++++++++++++++++++++++----------------
1 file changed, 81 insertions(+), 58 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index bc59c77b99b6..dfdb23c2bbd6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6143,67 +6143,90 @@ int ata_platform_remove_one(struct platform_device *pdev)
EXPORT_SYMBOL_GPL(ata_platform_remove_one);
#ifdef CONFIG_ATA_FORCE
+
+#define force_cbl(name, flag) \
+ { #name, .cbl = (flag) }
+
+#define force_spd_limit(spd, val) \
+ { #spd, .spd_limit = (val) }
+
+#define force_xfer(mode, shift) \
+ { #mode, .xfer_mask = (1UL << (shift)) }
+
+#define force_lflag(name, flags) \
+ { #name, .lflags = (flags) }
+
+#define force_horkage_on(name, flag) \
+ { #name, .horkage_on = (flag) }
+
+#define force_horkage_onoff(name, flag) \
+ { "no" #name, .horkage_on = (flag) }, \
+ { #name, .horkage_off = (flag) }
+
+static const struct ata_force_param force_tbl[] __initconst = {
+ force_cbl(40c, ATA_CBL_PATA40),
+ force_cbl(80c, ATA_CBL_PATA80),
+ force_cbl(short40c, ATA_CBL_PATA40_SHORT),
+ force_cbl(unk, ATA_CBL_PATA_UNK),
+ force_cbl(ign, ATA_CBL_PATA_IGN),
+ force_cbl(sata, ATA_CBL_SATA),
+
+ force_spd_limit(1.5Gbps, 1),
+ force_spd_limit(3.0Gbps, 2),
+
+ force_xfer(pio0, ATA_SHIFT_PIO + 0),
+ force_xfer(pio1, ATA_SHIFT_PIO + 1),
+ force_xfer(pio2, ATA_SHIFT_PIO + 2),
+ force_xfer(pio3, ATA_SHIFT_PIO + 3),
+ force_xfer(pio4, ATA_SHIFT_PIO + 4),
+ force_xfer(pio5, ATA_SHIFT_PIO + 5),
+ force_xfer(pio6, ATA_SHIFT_PIO + 6),
+ force_xfer(mwdma0, ATA_SHIFT_MWDMA + 0),
+ force_xfer(mwdma1, ATA_SHIFT_MWDMA + 1),
+ force_xfer(mwdma2, ATA_SHIFT_MWDMA + 2),
+ force_xfer(mwdma3, ATA_SHIFT_MWDMA + 3),
+ force_xfer(mwdma4, ATA_SHIFT_MWDMA + 4),
+ force_xfer(udma0, ATA_SHIFT_UDMA + 0),
+ force_xfer(udma16, ATA_SHIFT_UDMA + 0),
+ force_xfer(udma/16, ATA_SHIFT_UDMA + 0),
+ force_xfer(udma1, ATA_SHIFT_UDMA + 1),
+ force_xfer(udma25, ATA_SHIFT_UDMA + 1),
+ force_xfer(udma/25, ATA_SHIFT_UDMA + 1),
+ force_xfer(udma2, ATA_SHIFT_UDMA + 2),
+ force_xfer(udma33, ATA_SHIFT_UDMA + 2),
+ force_xfer(udma/33, ATA_SHIFT_UDMA + 2),
+ force_xfer(udma3, ATA_SHIFT_UDMA + 3),
+ force_xfer(udma44, ATA_SHIFT_UDMA + 3),
+ force_xfer(udma/44, ATA_SHIFT_UDMA + 3),
+ force_xfer(udma4, ATA_SHIFT_UDMA + 4),
+ force_xfer(udma66, ATA_SHIFT_UDMA + 4),
+ force_xfer(udma/66, ATA_SHIFT_UDMA + 4),
+ force_xfer(udma5, ATA_SHIFT_UDMA + 5),
+ force_xfer(udma100, ATA_SHIFT_UDMA + 5),
+ force_xfer(udma/100, ATA_SHIFT_UDMA + 5),
+ force_xfer(udma6, ATA_SHIFT_UDMA + 6),
+ force_xfer(udma133, ATA_SHIFT_UDMA + 6),
+ force_xfer(udma/133, ATA_SHIFT_UDMA + 6),
+ force_xfer(udma7, ATA_SHIFT_UDMA + 7),
+
+ force_lflag(nohrst, ATA_LFLAG_NO_HRST),
+ force_lflag(nosrst, ATA_LFLAG_NO_SRST),
+ force_lflag(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
+ force_lflag(rstonce, ATA_LFLAG_RST_ONCE),
+
+ force_horkage_onoff(ncq, ATA_HORKAGE_NONCQ),
+ force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
+ force_horkage_onoff(ncqati, ATA_HORKAGE_NO_NCQ_ON_ATI),
+
+ force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
+ force_horkage_on(atapi_dmadir, ATA_HORKAGE_ATAPI_DMADIR),
+ force_horkage_on(disable, ATA_HORKAGE_DISABLE)
+};
+
static int __init ata_parse_force_one(char **cur,
struct ata_force_ent *force_ent,
const char **reason)
{
- static const struct ata_force_param force_tbl[] __initconst = {
- { "40c", .cbl = ATA_CBL_PATA40 },
- { "80c", .cbl = ATA_CBL_PATA80 },
- { "short40c", .cbl = ATA_CBL_PATA40_SHORT },
- { "unk", .cbl = ATA_CBL_PATA_UNK },
- { "ign", .cbl = ATA_CBL_PATA_IGN },
- { "sata", .cbl = ATA_CBL_SATA },
- { "1.5Gbps", .spd_limit = 1 },
- { "3.0Gbps", .spd_limit = 2 },
- { "noncq", .horkage_on = ATA_HORKAGE_NONCQ },
- { "ncq", .horkage_off = ATA_HORKAGE_NONCQ },
- { "noncqtrim", .horkage_on = ATA_HORKAGE_NO_NCQ_TRIM },
- { "ncqtrim", .horkage_off = ATA_HORKAGE_NO_NCQ_TRIM },
- { "noncqati", .horkage_on = ATA_HORKAGE_NO_NCQ_ON_ATI },
- { "ncqati", .horkage_off = ATA_HORKAGE_NO_NCQ_ON_ATI },
- { "dump_id", .horkage_on = ATA_HORKAGE_DUMP_ID },
- { "pio0", .xfer_mask = 1 << (ATA_SHIFT_PIO + 0) },
- { "pio1", .xfer_mask = 1 << (ATA_SHIFT_PIO + 1) },
- { "pio2", .xfer_mask = 1 << (ATA_SHIFT_PIO + 2) },
- { "pio3", .xfer_mask = 1 << (ATA_SHIFT_PIO + 3) },
- { "pio4", .xfer_mask = 1 << (ATA_SHIFT_PIO + 4) },
- { "pio5", .xfer_mask = 1 << (ATA_SHIFT_PIO + 5) },
- { "pio6", .xfer_mask = 1 << (ATA_SHIFT_PIO + 6) },
- { "mwdma0", .xfer_mask = 1 << (ATA_SHIFT_MWDMA + 0) },
- { "mwdma1", .xfer_mask = 1 << (ATA_SHIFT_MWDMA + 1) },
- { "mwdma2", .xfer_mask = 1 << (ATA_SHIFT_MWDMA + 2) },
- { "mwdma3", .xfer_mask = 1 << (ATA_SHIFT_MWDMA + 3) },
- { "mwdma4", .xfer_mask = 1 << (ATA_SHIFT_MWDMA + 4) },
- { "udma0", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 0) },
- { "udma16", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 0) },
- { "udma/16", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 0) },
- { "udma1", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 1) },
- { "udma25", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 1) },
- { "udma/25", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 1) },
- { "udma2", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 2) },
- { "udma33", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 2) },
- { "udma/33", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 2) },
- { "udma3", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 3) },
- { "udma44", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 3) },
- { "udma/44", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 3) },
- { "udma4", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 4) },
- { "udma66", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 4) },
- { "udma/66", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 4) },
- { "udma5", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 5) },
- { "udma100", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 5) },
- { "udma/100", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 5) },
- { "udma6", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 6) },
- { "udma133", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 6) },
- { "udma/133", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 6) },
- { "udma7", .xfer_mask = 1 << (ATA_SHIFT_UDMA + 7) },
- { "nohrst", .lflags = ATA_LFLAG_NO_HRST },
- { "nosrst", .lflags = ATA_LFLAG_NO_SRST },
- { "norst", .lflags = ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST },
- { "rstonce", .lflags = ATA_LFLAG_RST_ONCE },
- { "atapi_dmadir", .horkage_on = ATA_HORKAGE_ATAPI_DMADIR },
- { "disable", .horkage_on = ATA_HORKAGE_DISABLE },
- };
char *start = *cur, *p = *cur;
char *id, *val, *endp;
const struct ata_force_param *match_fp = NULL;
@@ -6285,7 +6308,7 @@ static void __init ata_parse_force_param(void)
int last_port = -1, last_device = -1;
char *p, *cur, *next;
- /* calculate maximum number of params and allocate force_tbl */
+ /* Calculate maximum number of params and allocate ata_force_tbl */
for (p = ata_force_param_buf; *p; p++)
if (*p == ',')
size++;
--
2.35.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings
2022-04-25 1:34 [PATCH v2 0/5] libata.force improvements Damien Le Moal
2022-04-25 1:34 ` [PATCH v2 1/5] ata: libata-core: cleanup ata_device_blacklist Damien Le Moal
2022-04-25 1:34 ` [PATCH v2 2/5] ata: libata-core: Refactor force_tbl definition Damien Le Moal
@ 2022-04-25 1:34 ` Damien Le Moal
2022-04-25 5:56 ` Hannes Reinecke
2022-04-25 1:34 ` [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags Damien Le Moal
2022-04-25 1:34 ` [PATCH v2 5/5] doc: admin-guide: Update libata kernel parameters Damien Le Moal
4 siblings, 1 reply; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 1:34 UTC (permalink / raw)
To: linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
Similarly to the horkage flags, introduce the force_lflag_onoff() macro
to define struct ata_force_param entries of the force_tbl array that
allow turning on or off a link flag using the libata.force boot
parameter. To be consistent with naming, the macro force_lflag() is
renamed to force_lflag_on().
Using force_lflag_onoff(), define a new force_tbl entry for the
ATA_LFLAG_NO_DEBOUNCE_DELAY link flag, thus allowing testing if an
adapter requires a link debounce delay or not.
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
drivers/ata/libata-core.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index dfdb23c2bbd6..e5a0e73b39d3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -96,7 +96,8 @@ struct ata_force_param {
unsigned long xfer_mask;
unsigned int horkage_on;
unsigned int horkage_off;
- u16 lflags;
+ u16 lflags_on;
+ u16 lflags_off;
};
struct ata_force_ent {
@@ -386,11 +387,17 @@ static void ata_force_link_limits(struct ata_link *link)
}
/* let lflags stack */
- if (fe->param.lflags) {
- link->flags |= fe->param.lflags;
+ if (fe->param.lflags_on) {
+ link->flags |= fe->param.lflags_on;
ata_link_notice(link,
"FORCE: link flag 0x%x forced -> 0x%x\n",
- fe->param.lflags, link->flags);
+ fe->param.lflags_on, link->flags);
+ }
+ if (fe->param.lflags_off) {
+ link->flags &= ~fe->param.lflags_off;
+ ata_link_notice(link,
+ "FORCE: link flag 0x%x cleared -> 0x%x\n",
+ fe->param.lflags_off, link->flags);
}
}
}
@@ -6153,8 +6160,12 @@ EXPORT_SYMBOL_GPL(ata_platform_remove_one);
#define force_xfer(mode, shift) \
{ #mode, .xfer_mask = (1UL << (shift)) }
-#define force_lflag(name, flags) \
- { #name, .lflags = (flags) }
+#define force_lflag_on(name, flags) \
+ { #name, .lflags_on = (flags) }
+
+#define force_lflag_onoff(name, flags) \
+ { "no" #name, .lflags_on = (flags) }, \
+ { #name, .lflags_off = (flags) }
#define force_horkage_on(name, flag) \
{ #name, .horkage_on = (flag) }
@@ -6209,10 +6220,11 @@ static const struct ata_force_param force_tbl[] __initconst = {
force_xfer(udma/133, ATA_SHIFT_UDMA + 6),
force_xfer(udma7, ATA_SHIFT_UDMA + 7),
- force_lflag(nohrst, ATA_LFLAG_NO_HRST),
- force_lflag(nosrst, ATA_LFLAG_NO_SRST),
- force_lflag(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
- force_lflag(rstonce, ATA_LFLAG_RST_ONCE),
+ force_lflag_on(nohrst, ATA_LFLAG_NO_HRST),
+ force_lflag_on(nosrst, ATA_LFLAG_NO_SRST),
+ force_lflag_on(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
+ force_lflag_on(rstonce, ATA_LFLAG_RST_ONCE),
+ force_lflag_onoff(dbdelay, ATA_LFLAG_NO_DEBOUNCE_DELAY),
force_horkage_onoff(ncq, ATA_HORKAGE_NONCQ),
force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
--
2.35.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags
2022-04-25 1:34 [PATCH v2 0/5] libata.force improvements Damien Le Moal
` (2 preceding siblings ...)
2022-04-25 1:34 ` [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings Damien Le Moal
@ 2022-04-25 1:34 ` Damien Le Moal
2022-04-25 6:00 ` Hannes Reinecke
2022-04-25 1:34 ` [PATCH v2 5/5] doc: admin-guide: Update libata kernel parameters Damien Le Moal
4 siblings, 1 reply; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 1:34 UTC (permalink / raw)
To: linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
To facilitate debugging of drive issues in the field without kernel
changes (e.g. temporary test patches), add an entry for most horkage
flags in the force_tbl array to allow controlling these horkage
settings with the libata.force kernel boot parameter.
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
drivers/ata/libata-core.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e5a0e73b39d3..f68cb5639ec4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6230,9 +6230,27 @@ static const struct ata_force_param force_tbl[] __initconst = {
force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
force_horkage_onoff(ncqati, ATA_HORKAGE_NO_NCQ_ON_ATI),
- force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
+ force_horkage_onoff(trim, ATA_HORKAGE_NOTRIM),
+ force_horkage_on(trim_zero, ATA_HORKAGE_ZERO_AFTER_TRIM),
+ force_horkage_on(max_trim_128m, ATA_HORKAGE_MAX_TRIM_128M),
+
+ force_horkage_onoff(dma, ATA_HORKAGE_NODMA),
force_horkage_on(atapi_dmadir, ATA_HORKAGE_ATAPI_DMADIR),
- force_horkage_on(disable, ATA_HORKAGE_DISABLE)
+ force_horkage_on(atapi_mod16_dma, ATA_HORKAGE_ATAPI_MOD16_DMA),
+
+ force_horkage_onoff(dmalog, ATA_HORKAGE_NO_DMA_LOG),
+ force_horkage_onoff(iddevlog, ATA_HORKAGE_NO_ID_DEV_LOG),
+ force_horkage_onoff(logdir, ATA_HORKAGE_NO_LOG_DIR),
+
+ force_horkage_on(max_sec_128, ATA_HORKAGE_MAX_SEC_128),
+ force_horkage_on(max_sec_1024, ATA_HORKAGE_MAX_SEC_1024),
+ force_horkage_on(max_sec_lba48, ATA_HORKAGE_MAX_SEC_LBA48),
+
+ force_horkage_onoff(lpm, ATA_HORKAGE_NOLPM),
+ force_horkage_onoff(setxfer, ATA_HORKAGE_NOSETXFER),
+ force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
+
+ force_horkage_on(disable, ATA_HORKAGE_DISABLE),
};
static int __init ata_parse_force_one(char **cur,
--
2.35.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 5/5] doc: admin-guide: Update libata kernel parameters
2022-04-25 1:34 [PATCH v2 0/5] libata.force improvements Damien Le Moal
` (3 preceding siblings ...)
2022-04-25 1:34 ` [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags Damien Le Moal
@ 2022-04-25 1:34 ` Damien Le Moal
2022-04-25 1:37 ` Damien Le Moal
2022-04-25 6:01 ` Hannes Reinecke
4 siblings, 2 replies; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 1:34 UTC (permalink / raw)
To: linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
Cleanup the text text describing the libata.force boot parameter and
update the list of the values to include all supported horkage and link
flag that can be forced.
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
.../admin-guide/kernel-parameters.txt | 71 ++++++++++++++-----
1 file changed, 55 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 3f1cc5e317ed..00fb37cab649 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2622,14 +2622,14 @@
when set.
Format: <int>
- libata.force= [LIBATA] Force configurations. The format is comma-
- separated list of "[ID:]VAL" where ID is
- PORT[.DEVICE]. PORT and DEVICE are decimal numbers
- matching port, link or device. Basically, it matches
- the ATA ID string printed on console by libata. If
- the whole ID part is omitted, the last PORT and DEVICE
- values are used. If ID hasn't been specified yet, the
- configuration applies to all ports, links and devices.
+ libata.force= [LIBATA] Force configurations. The format is a comma-
+ separated list of "[ID:]VAL" where ID is PORT[.DEVICE].
+ PORT and DEVICE are decimal numbers matching port, link
+ or device. Basically, it matches the ATA ID string
+ printed on console by libata. If the whole ID part is
+ omitted, the last PORT and DEVICE values are used. If
+ ID hasn't been specified yet, the configuration applies
+ to all ports, links and devices.
If only DEVICE is omitted, the parameter applies to
the port and all links and devices behind it. DEVICE
@@ -2639,7 +2639,7 @@
host link and device attached to it.
The VAL specifies the configuration to force. As long
- as there's no ambiguity shortcut notation is allowed.
+ as there is no ambiguity, shortcut notation is allowed.
For example, both 1.5 and 1.5G would work for 1.5Gbps.
The following configurations can be forced.
@@ -2652,19 +2652,58 @@
udma[/][16,25,33,44,66,100,133] notation is also
allowed.
+ * nohrst, nosrst, norst: suppress hard, soft and both
+ resets.
+
+ * rstonce: only attempt one reset during hot-unplug
+ link recovery.
+
+ * [no]dbdelay: Enable or disable the extra 200ms delay
+ before debouncing a link PHY and device presence
+ detection.
+
* [no]ncq: Turn on or off NCQ.
- * [no]ncqtrim: Turn off queued DSM TRIM.
+ * [no]ncqtrim: Enable or disable queued DSM TRIM.
+
+ * [no]ncqati: Enable or disable NCQ trim on ATI chipset.
+
+ * [no]trim: Enable or disable (unqueued) TRIM.
+
+ * trim_zero: Indicate that TRIM command zeroes data.
+
+ * max_trim_128m: Set 128M maximum trim size limit.
+
+ * [no]dma: Turn on or off DMA transfers.
+
+ * atapi_dmadir: Enable ATAPI DMADIR bridge support.
+
+ * atapi_mod16_dma: Enable the use of ATAPI DMA for
+ commands that are not a multiple of 16 bytes.
+
+ * [no]dmalog: Enable or disable the use of the
+ READ LOG DMA EXT command to access logs.
+
+ * [no]iddevlog: Enable or disable access to the
+ identify device data log.
+
+ * [no]logdir: Enable or disable access to the general
+ purpose log directory.
+
+ * max_sec_128: Set transfer size limit to 128 sectors.
+
+ * max_sec_1024: Set or clear transfer size limit to
+ 1024 sectors.
- * nohrst, nosrst, norst: suppress hard, soft
- and both resets.
+ * max_sec_lba48: Set or clear transfer size limit to
+ 65535 sectors.
- * rstonce: only attempt one reset during
- hot-unplug link recovery
+ * [no]lpm: Enable or disable link power management.
- * dump_id: dump IDENTIFY data.
+ * [no]setxfer: Indicate if transfer speed mode setting
+ should be skipped.
- * atapi_dmadir: Enable ATAPI DMADIR bridge support
+ * dump_id: Dump IDENTIFY data.
* disable: Disable this device.
--
2.35.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] doc: admin-guide: Update libata kernel parameters
2022-04-25 1:34 ` [PATCH v2 5/5] doc: admin-guide: Update libata kernel parameters Damien Le Moal
@ 2022-04-25 1:37 ` Damien Le Moal
2022-04-25 6:01 ` Hannes Reinecke
1 sibling, 0 replies; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 1:37 UTC (permalink / raw)
To: linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 10:34, Damien Le Moal wrote:
> Cleanup the text text describing the libata.force boot parameter and
> update the list of the values to include all supported horkage and link
> flag that can be forced.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
> .../admin-guide/kernel-parameters.txt | 71 ++++++++++++++-----
> 1 file changed, 55 insertions(+), 16 deletions(-)
Jonathan,
Are you OK with these changes ? Will you take this patch or should I keep
it with the series in the libata tree ? Either way work for me.
Thanks !
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 3f1cc5e317ed..00fb37cab649 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2622,14 +2622,14 @@
> when set.
> Format: <int>
>
> - libata.force= [LIBATA] Force configurations. The format is comma-
> - separated list of "[ID:]VAL" where ID is
> - PORT[.DEVICE]. PORT and DEVICE are decimal numbers
> - matching port, link or device. Basically, it matches
> - the ATA ID string printed on console by libata. If
> - the whole ID part is omitted, the last PORT and DEVICE
> - values are used. If ID hasn't been specified yet, the
> - configuration applies to all ports, links and devices.
> + libata.force= [LIBATA] Force configurations. The format is a comma-
> + separated list of "[ID:]VAL" where ID is PORT[.DEVICE].
> + PORT and DEVICE are decimal numbers matching port, link
> + or device. Basically, it matches the ATA ID string
> + printed on console by libata. If the whole ID part is
> + omitted, the last PORT and DEVICE values are used. If
> + ID hasn't been specified yet, the configuration applies
> + to all ports, links and devices.
>
> If only DEVICE is omitted, the parameter applies to
> the port and all links and devices behind it. DEVICE
> @@ -2639,7 +2639,7 @@
> host link and device attached to it.
>
> The VAL specifies the configuration to force. As long
> - as there's no ambiguity shortcut notation is allowed.
> + as there is no ambiguity, shortcut notation is allowed.
> For example, both 1.5 and 1.5G would work for 1.5Gbps.
> The following configurations can be forced.
>
> @@ -2652,19 +2652,58 @@
> udma[/][16,25,33,44,66,100,133] notation is also
> allowed.
>
> + * nohrst, nosrst, norst: suppress hard, soft and both
> + resets.
> +
> + * rstonce: only attempt one reset during hot-unplug
> + link recovery.
> +
> + * [no]dbdelay: Enable or disable the extra 200ms delay
> + before debouncing a link PHY and device presence
> + detection.
> +
> * [no]ncq: Turn on or off NCQ.
>
> - * [no]ncqtrim: Turn off queued DSM TRIM.
> + * [no]ncqtrim: Enable or disable queued DSM TRIM.
> +
> + * [no]ncqati: Enable or disable NCQ trim on ATI chipset.
> +
> + * [no]trim: Enable or disable (unqueued) TRIM.
> +
> + * trim_zero: Indicate that TRIM command zeroes data.
> +
> + * max_trim_128m: Set 128M maximum trim size limit.
> +
> + * [no]dma: Turn on or off DMA transfers.
> +
> + * atapi_dmadir: Enable ATAPI DMADIR bridge support.
> +
> + * atapi_mod16_dma: Enable the use of ATAPI DMA for
> + commands that are not a multiple of 16 bytes.
> +
> + * [no]dmalog: Enable or disable the use of the
> + READ LOG DMA EXT command to access logs.
> +
> + * [no]iddevlog: Enable or disable access to the
> + identify device data log.
> +
> + * [no]logdir: Enable or disable access to the general
> + purpose log directory.
> +
> + * max_sec_128: Set transfer size limit to 128 sectors.
> +
> + * max_sec_1024: Set or clear transfer size limit to
> + 1024 sectors.
>
> - * nohrst, nosrst, norst: suppress hard, soft
> - and both resets.
> + * max_sec_lba48: Set or clear transfer size limit to
> + 65535 sectors.
>
> - * rstonce: only attempt one reset during
> - hot-unplug link recovery
> + * [no]lpm: Enable or disable link power management.
>
> - * dump_id: dump IDENTIFY data.
> + * [no]setxfer: Indicate if transfer speed mode setting
> + should be skipped.
>
> - * atapi_dmadir: Enable ATAPI DMADIR bridge support
> + * dump_id: Dump IDENTIFY data.
>
> * disable: Disable this device.
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/5] ata: libata-core: cleanup ata_device_blacklist
2022-04-25 1:34 ` [PATCH v2 1/5] ata: libata-core: cleanup ata_device_blacklist Damien Le Moal
@ 2022-04-25 5:49 ` Hannes Reinecke
0 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2022-04-25 5:49 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 03:34, Damien Le Moal wrote:
> Remove the unneeded comma after the last field of the array entries.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> drivers/ata/libata-core.c | 96 +++++++++++++++++++--------------------
> 1 file changed, 48 insertions(+), 48 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] ata: libata-core: Refactor force_tbl definition
2022-04-25 1:34 ` [PATCH v2 2/5] ata: libata-core: Refactor force_tbl definition Damien Le Moal
@ 2022-04-25 5:51 ` Hannes Reinecke
0 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2022-04-25 5:51 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 03:34, Damien Le Moal wrote:
> Introduce the macro definitions force_cbl(), force_spd_limit(),
> force_xfer(), force_lflag(), force_horkage_on() and
> force_horkage_onoff() to define with a more compact syntax the struct
> ata_force_param entries in the force_tbl array defined in the function
> ata_parse_force_one().
>
> To reduce the indentation of the array declaration, force_tbl definition
> is also moved out of the ata_parse_force_one() function. The entries are
> also reordered to group them by type of the quirck that is applied.
>
> Finally, fix a comment in ata_parse_force_param() incorrectly
> referencing force_tbl instead of ata_force_tbl.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> drivers/ata/libata-core.c | 139 ++++++++++++++++++++++----------------
> 1 file changed, 81 insertions(+), 58 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings
2022-04-25 1:34 ` [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings Damien Le Moal
@ 2022-04-25 5:56 ` Hannes Reinecke
2022-04-25 6:08 ` Damien Le Moal
0 siblings, 1 reply; 16+ messages in thread
From: Hannes Reinecke @ 2022-04-25 5:56 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 03:34, Damien Le Moal wrote:
> Similarly to the horkage flags, introduce the force_lflag_onoff() macro
> to define struct ata_force_param entries of the force_tbl array that
> allow turning on or off a link flag using the libata.force boot
> parameter. To be consistent with naming, the macro force_lflag() is
> renamed to force_lflag_on().
>
> Using force_lflag_onoff(), define a new force_tbl entry for the
> ATA_LFLAG_NO_DEBOUNCE_DELAY link flag, thus allowing testing if an
> adapter requires a link debounce delay or not.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> drivers/ata/libata-core.c | 32 ++++++++++++++++++++++----------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index dfdb23c2bbd6..e5a0e73b39d3 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -96,7 +96,8 @@ struct ata_force_param {
> unsigned long xfer_mask;
> unsigned int horkage_on;
> unsigned int horkage_off;
> - u16 lflags;
> + u16 lflags_on;
> + u16 lflags_off;
> };
>
> struct ata_force_ent {
> @@ -386,11 +387,17 @@ static void ata_force_link_limits(struct ata_link *link)
> }
>
> /* let lflags stack */
> - if (fe->param.lflags) {
> - link->flags |= fe->param.lflags;
> + if (fe->param.lflags_on) {
> + link->flags |= fe->param.lflags_on;
> ata_link_notice(link,
> "FORCE: link flag 0x%x forced -> 0x%x\n",
> - fe->param.lflags, link->flags);
> + fe->param.lflags_on, link->flags);
> + }
> + if (fe->param.lflags_off) {
> + link->flags &= ~fe->param.lflags_off;
> + ata_link_notice(link,
> + "FORCE: link flag 0x%x cleared -> 0x%x\n",
> + fe->param.lflags_off, link->flags);
> }
> }
> }
> @@ -6153,8 +6160,12 @@ EXPORT_SYMBOL_GPL(ata_platform_remove_one);
> #define force_xfer(mode, shift) \
> { #mode, .xfer_mask = (1UL << (shift)) }
>
> -#define force_lflag(name, flags) \
> - { #name, .lflags = (flags) }
> +#define force_lflag_on(name, flags) \
> + { #name, .lflags_on = (flags) }
> +
> +#define force_lflag_onoff(name, flags) \
> + { "no" #name, .lflags_on = (flags) }, \
> + { #name, .lflags_off = (flags) }
>
> #define force_horkage_on(name, flag) \
> { #name, .horkage_on = (flag) }
> @@ -6209,10 +6220,11 @@ static const struct ata_force_param force_tbl[] __initconst = {
> force_xfer(udma/133, ATA_SHIFT_UDMA + 6),
> force_xfer(udma7, ATA_SHIFT_UDMA + 7),
>
> - force_lflag(nohrst, ATA_LFLAG_NO_HRST),
> - force_lflag(nosrst, ATA_LFLAG_NO_SRST),
> - force_lflag(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
> - force_lflag(rstonce, ATA_LFLAG_RST_ONCE),
> + force_lflag_on(nohrst, ATA_LFLAG_NO_HRST),
> + force_lflag_on(nosrst, ATA_LFLAG_NO_SRST),
> + force_lflag_on(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
> + force_lflag_on(rstonce, ATA_LFLAG_RST_ONCE),
> + force_lflag_onoff(dbdelay, ATA_LFLAG_NO_DEBOUNCE_DELAY),
>
> force_horkage_onoff(ncq, ATA_HORKAGE_NONCQ),
> force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
Hmm.
Personally, I'm not a fan of distinct 'flags_on' and 'flags_off'; I
always wonder what does happen if one sets the same value for both ...
And do we really need this? All settings above are just simple flags
which can be set or unset.
Sure, some flags are inverted, but still they are flags.
So why do we need the separation here?
Isn't it rather cosmetical?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags
2022-04-25 1:34 ` [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags Damien Le Moal
@ 2022-04-25 6:00 ` Hannes Reinecke
2022-04-25 6:15 ` Damien Le Moal
0 siblings, 1 reply; 16+ messages in thread
From: Hannes Reinecke @ 2022-04-25 6:00 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 03:34, Damien Le Moal wrote:
> To facilitate debugging of drive issues in the field without kernel
> changes (e.g. temporary test patches), add an entry for most horkage
> flags in the force_tbl array to allow controlling these horkage
> settings with the libata.force kernel boot parameter.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> drivers/ata/libata-core.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e5a0e73b39d3..f68cb5639ec4 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6230,9 +6230,27 @@ static const struct ata_force_param force_tbl[] __initconst = {
> force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
> force_horkage_onoff(ncqati, ATA_HORKAGE_NO_NCQ_ON_ATI),
>
> - force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
> + force_horkage_onoff(trim, ATA_HORKAGE_NOTRIM),
> + force_horkage_on(trim_zero, ATA_HORKAGE_ZERO_AFTER_TRIM),
> + force_horkage_on(max_trim_128m, ATA_HORKAGE_MAX_TRIM_128M),
> +
> + force_horkage_onoff(dma, ATA_HORKAGE_NODMA),
> force_horkage_on(atapi_dmadir, ATA_HORKAGE_ATAPI_DMADIR),
> - force_horkage_on(disable, ATA_HORKAGE_DISABLE)
> + force_horkage_on(atapi_mod16_dma, ATA_HORKAGE_ATAPI_MOD16_DMA),
> +
> + force_horkage_onoff(dmalog, ATA_HORKAGE_NO_DMA_LOG),
> + force_horkage_onoff(iddevlog, ATA_HORKAGE_NO_ID_DEV_LOG),
> + force_horkage_onoff(logdir, ATA_HORKAGE_NO_LOG_DIR),
> +
> + force_horkage_on(max_sec_128, ATA_HORKAGE_MAX_SEC_128),
> + force_horkage_on(max_sec_1024, ATA_HORKAGE_MAX_SEC_1024),
> + force_horkage_on(max_sec_lba48, ATA_HORKAGE_MAX_SEC_LBA48),
> +
> + force_horkage_onoff(lpm, ATA_HORKAGE_NOLPM),
> + force_horkage_onoff(setxfer, ATA_HORKAGE_NOSETXFER),
> + force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
> +
> + force_horkage_on(disable, ATA_HORKAGE_DISABLE),
... and this exemplifies my concerns with the 'onoff' mechanism:
Why is 'disable' just marked as 'on' ?
Sure we can set it to 'off' (we have to, otherwise that flag would
always be set). And if we can set it to 'off', where's the different to
'onoff' ?
Style-differences apart it looks good.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] doc: admin-guide: Update libata kernel parameters
2022-04-25 1:34 ` [PATCH v2 5/5] doc: admin-guide: Update libata kernel parameters Damien Le Moal
2022-04-25 1:37 ` Damien Le Moal
@ 2022-04-25 6:01 ` Hannes Reinecke
1 sibling, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2022-04-25 6:01 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 03:34, Damien Le Moal wrote:
> Cleanup the text text describing the libata.force boot parameter and
> update the list of the values to include all supported horkage and link
> flag that can be forced.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
> .../admin-guide/kernel-parameters.txt | 71 ++++++++++++++-----
> 1 file changed, 55 insertions(+), 16 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings
2022-04-25 5:56 ` Hannes Reinecke
@ 2022-04-25 6:08 ` Damien Le Moal
2022-04-25 6:10 ` Hannes Reinecke
0 siblings, 1 reply; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 6:08 UTC (permalink / raw)
To: Hannes Reinecke, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 14:56, Hannes Reinecke wrote:
> On 4/25/22 03:34, Damien Le Moal wrote:
>> Similarly to the horkage flags, introduce the force_lflag_onoff() macro
>> to define struct ata_force_param entries of the force_tbl array that
>> allow turning on or off a link flag using the libata.force boot
>> parameter. To be consistent with naming, the macro force_lflag() is
>> renamed to force_lflag_on().
>>
>> Using force_lflag_onoff(), define a new force_tbl entry for the
>> ATA_LFLAG_NO_DEBOUNCE_DELAY link flag, thus allowing testing if an
>> adapter requires a link debounce delay or not.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> drivers/ata/libata-core.c | 32 ++++++++++++++++++++++----------
>> 1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index dfdb23c2bbd6..e5a0e73b39d3 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -96,7 +96,8 @@ struct ata_force_param {
>> unsigned long xfer_mask;
>> unsigned int horkage_on;
>> unsigned int horkage_off;
>> - u16 lflags;
>> + u16 lflags_on;
>> + u16 lflags_off;
>> };
>>
>> struct ata_force_ent {
>> @@ -386,11 +387,17 @@ static void ata_force_link_limits(struct ata_link *link)
>> }
>>
>> /* let lflags stack */
>> - if (fe->param.lflags) {
>> - link->flags |= fe->param.lflags;
>> + if (fe->param.lflags_on) {
>> + link->flags |= fe->param.lflags_on;
>> ata_link_notice(link,
>> "FORCE: link flag 0x%x forced -> 0x%x\n",
>> - fe->param.lflags, link->flags);
>> + fe->param.lflags_on, link->flags);
>> + }
>> + if (fe->param.lflags_off) {
>> + link->flags &= ~fe->param.lflags_off;
>> + ata_link_notice(link,
>> + "FORCE: link flag 0x%x cleared -> 0x%x\n",
>> + fe->param.lflags_off, link->flags);
>> }
>> }
>> }
>> @@ -6153,8 +6160,12 @@ EXPORT_SYMBOL_GPL(ata_platform_remove_one);
>> #define force_xfer(mode, shift) \
>> { #mode, .xfer_mask = (1UL << (shift)) }
>>
>> -#define force_lflag(name, flags) \
>> - { #name, .lflags = (flags) }
>> +#define force_lflag_on(name, flags) \
>> + { #name, .lflags_on = (flags) }
>> +
>> +#define force_lflag_onoff(name, flags) \
>> + { "no" #name, .lflags_on = (flags) }, \
>> + { #name, .lflags_off = (flags) }
>>
>> #define force_horkage_on(name, flag) \
>> { #name, .horkage_on = (flag) }
>> @@ -6209,10 +6220,11 @@ static const struct ata_force_param force_tbl[] __initconst = {
>> force_xfer(udma/133, ATA_SHIFT_UDMA + 6),
>> force_xfer(udma7, ATA_SHIFT_UDMA + 7),
>>
>> - force_lflag(nohrst, ATA_LFLAG_NO_HRST),
>> - force_lflag(nosrst, ATA_LFLAG_NO_SRST),
>> - force_lflag(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
>> - force_lflag(rstonce, ATA_LFLAG_RST_ONCE),
>> + force_lflag_on(nohrst, ATA_LFLAG_NO_HRST),
>> + force_lflag_on(nosrst, ATA_LFLAG_NO_SRST),
>> + force_lflag_on(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
>> + force_lflag_on(rstonce, ATA_LFLAG_RST_ONCE),
>> + force_lflag_onoff(dbdelay, ATA_LFLAG_NO_DEBOUNCE_DELAY),
>>
>> force_horkage_onoff(ncq, ATA_HORKAGE_NONCQ),
>> force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
>
> Hmm.
>
> Personally, I'm not a fan of distinct 'flags_on' and 'flags_off'; I
> always wonder what does happen if one sets the same value for both ...
Problem is that when parsing the options and gathering the horkages/flags
about the drive, nothing is known about it yet, so we do not have the link
flags to directly modify based on the option name. So we have to keep the
information about which flag to set and which to reset. Generating a
"flags_mask" variable with all the correct flags set based on the option
is easy, but we still need to know the operation to do with that mask on
the device/link flags.
>
> And do we really need this? All settings above are just simple flags
> which can be set or unset.
> Sure, some flags are inverted, but still they are flags.
> So why do we need the separation here?
> Isn't it rather cosmetical?
For the existing libata.force option, yes, all this is cosmetic.
For the new flags, we still have to deal with the reversed flags (the
ATA_XXX_NO_...). And for these, the onoff is definitely reversed from the
non reversed flags. And given that some of the reversed flags can already
be set with libata.force, I find this way the cleanest to add new flags
without breaking the manipulation of existing ones.
>
> Cheers,
>
> Hannes
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings
2022-04-25 6:08 ` Damien Le Moal
@ 2022-04-25 6:10 ` Hannes Reinecke
0 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2022-04-25 6:10 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 08:08, Damien Le Moal wrote:
> On 4/25/22 14:56, Hannes Reinecke wrote:
>> On 4/25/22 03:34, Damien Le Moal wrote:
>>> Similarly to the horkage flags, introduce the force_lflag_onoff() macro
>>> to define struct ata_force_param entries of the force_tbl array that
>>> allow turning on or off a link flag using the libata.force boot
>>> parameter. To be consistent with naming, the macro force_lflag() is
>>> renamed to force_lflag_on().
>>>
>>> Using force_lflag_onoff(), define a new force_tbl entry for the
>>> ATA_LFLAG_NO_DEBOUNCE_DELAY link flag, thus allowing testing if an
>>> adapter requires a link debounce delay or not.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>> drivers/ata/libata-core.c | 32 ++++++++++++++++++++++----------
>>> 1 file changed, 22 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index dfdb23c2bbd6..e5a0e73b39d3 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -96,7 +96,8 @@ struct ata_force_param {
>>> unsigned long xfer_mask;
>>> unsigned int horkage_on;
>>> unsigned int horkage_off;
>>> - u16 lflags;
>>> + u16 lflags_on;
>>> + u16 lflags_off;
>>> };
>>>
>>> struct ata_force_ent {
>>> @@ -386,11 +387,17 @@ static void ata_force_link_limits(struct ata_link *link)
>>> }
>>>
>>> /* let lflags stack */
>>> - if (fe->param.lflags) {
>>> - link->flags |= fe->param.lflags;
>>> + if (fe->param.lflags_on) {
>>> + link->flags |= fe->param.lflags_on;
>>> ata_link_notice(link,
>>> "FORCE: link flag 0x%x forced -> 0x%x\n",
>>> - fe->param.lflags, link->flags);
>>> + fe->param.lflags_on, link->flags);
>>> + }
>>> + if (fe->param.lflags_off) {
>>> + link->flags &= ~fe->param.lflags_off;
>>> + ata_link_notice(link,
>>> + "FORCE: link flag 0x%x cleared -> 0x%x\n",
>>> + fe->param.lflags_off, link->flags);
>>> }
>>> }
>>> }
>>> @@ -6153,8 +6160,12 @@ EXPORT_SYMBOL_GPL(ata_platform_remove_one);
>>> #define force_xfer(mode, shift) \
>>> { #mode, .xfer_mask = (1UL << (shift)) }
>>>
>>> -#define force_lflag(name, flags) \
>>> - { #name, .lflags = (flags) }
>>> +#define force_lflag_on(name, flags) \
>>> + { #name, .lflags_on = (flags) }
>>> +
>>> +#define force_lflag_onoff(name, flags) \
>>> + { "no" #name, .lflags_on = (flags) }, \
>>> + { #name, .lflags_off = (flags) }
>>>
>>> #define force_horkage_on(name, flag) \
>>> { #name, .horkage_on = (flag) }
>>> @@ -6209,10 +6220,11 @@ static const struct ata_force_param force_tbl[] __initconst = {
>>> force_xfer(udma/133, ATA_SHIFT_UDMA + 6),
>>> force_xfer(udma7, ATA_SHIFT_UDMA + 7),
>>>
>>> - force_lflag(nohrst, ATA_LFLAG_NO_HRST),
>>> - force_lflag(nosrst, ATA_LFLAG_NO_SRST),
>>> - force_lflag(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
>>> - force_lflag(rstonce, ATA_LFLAG_RST_ONCE),
>>> + force_lflag_on(nohrst, ATA_LFLAG_NO_HRST),
>>> + force_lflag_on(nosrst, ATA_LFLAG_NO_SRST),
>>> + force_lflag_on(norst, ATA_LFLAG_NO_HRST | ATA_LFLAG_NO_SRST),
>>> + force_lflag_on(rstonce, ATA_LFLAG_RST_ONCE),
>>> + force_lflag_onoff(dbdelay, ATA_LFLAG_NO_DEBOUNCE_DELAY),
>>>
>>> force_horkage_onoff(ncq, ATA_HORKAGE_NONCQ),
>>> force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
>>
>> Hmm.
>>
>> Personally, I'm not a fan of distinct 'flags_on' and 'flags_off'; I
>> always wonder what does happen if one sets the same value for both ...
>
> Problem is that when parsing the options and gathering the horkages/flags
> about the drive, nothing is known about it yet, so we do not have the link
> flags to directly modify based on the option name. So we have to keep the
> information about which flag to set and which to reset. Generating a
> "flags_mask" variable with all the correct flags set based on the option
> is easy, but we still need to know the operation to do with that mask on
> the device/link flags.
>
>>
>> And do we really need this? All settings above are just simple flags
>> which can be set or unset.
>> Sure, some flags are inverted, but still they are flags.
>> So why do we need the separation here?
>> Isn't it rather cosmetical?
>
> For the existing libata.force option, yes, all this is cosmetic.
> For the new flags, we still have to deal with the reversed flags (the
> ATA_XXX_NO_...). And for these, the onoff is definitely reversed from the
> non reversed flags. And given that some of the reversed flags can already
> be set with libata.force, I find this way the cleanest to add new flags
> without breaking the manipulation of existing ones.
>
Ok.
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags
2022-04-25 6:00 ` Hannes Reinecke
@ 2022-04-25 6:15 ` Damien Le Moal
2022-04-25 6:44 ` Hannes Reinecke
0 siblings, 1 reply; 16+ messages in thread
From: Damien Le Moal @ 2022-04-25 6:15 UTC (permalink / raw)
To: Hannes Reinecke, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 15:00, Hannes Reinecke wrote:
> On 4/25/22 03:34, Damien Le Moal wrote:
>> To facilitate debugging of drive issues in the field without kernel
>> changes (e.g. temporary test patches), add an entry for most horkage
>> flags in the force_tbl array to allow controlling these horkage
>> settings with the libata.force kernel boot parameter.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> drivers/ata/libata-core.c | 22 ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index e5a0e73b39d3..f68cb5639ec4 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -6230,9 +6230,27 @@ static const struct ata_force_param force_tbl[] __initconst = {
>> force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
>> force_horkage_onoff(ncqati, ATA_HORKAGE_NO_NCQ_ON_ATI),
>>
>> - force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
>> + force_horkage_onoff(trim, ATA_HORKAGE_NOTRIM),
>> + force_horkage_on(trim_zero, ATA_HORKAGE_ZERO_AFTER_TRIM),
>> + force_horkage_on(max_trim_128m, ATA_HORKAGE_MAX_TRIM_128M),
>> +
>> + force_horkage_onoff(dma, ATA_HORKAGE_NODMA),
>> force_horkage_on(atapi_dmadir, ATA_HORKAGE_ATAPI_DMADIR),
>> - force_horkage_on(disable, ATA_HORKAGE_DISABLE)
>> + force_horkage_on(atapi_mod16_dma, ATA_HORKAGE_ATAPI_MOD16_DMA),
>> +
>> + force_horkage_onoff(dmalog, ATA_HORKAGE_NO_DMA_LOG),
>> + force_horkage_onoff(iddevlog, ATA_HORKAGE_NO_ID_DEV_LOG),
>> + force_horkage_onoff(logdir, ATA_HORKAGE_NO_LOG_DIR),
>> +
>> + force_horkage_on(max_sec_128, ATA_HORKAGE_MAX_SEC_128),
>> + force_horkage_on(max_sec_1024, ATA_HORKAGE_MAX_SEC_1024),
>> + force_horkage_on(max_sec_lba48, ATA_HORKAGE_MAX_SEC_LBA48),
>> +
>> + force_horkage_onoff(lpm, ATA_HORKAGE_NOLPM),
>> + force_horkage_onoff(setxfer, ATA_HORKAGE_NOSETXFER),
>> + force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
>> +
>> + force_horkage_on(disable, ATA_HORKAGE_DISABLE),
>
> ... and this exemplifies my concerns with the 'onoff' mechanism:
> Why is 'disable' just marked as 'on' ?
Yeah, I can add the off side of it too. Fairly useless though as these are
off by default, except for the few cases where we already know that the
flag is needed, in which case turning it off would be a bad idea. So I do
not allow it by having the "on" only.
> Sure we can set it to 'off' (we have to, otherwise that flag would
> always be set). And if we can set it to 'off', where's the different to
> 'onoff' ?
Because of the reversed definition of the flag. E.g. nodmalog means *set*
ATA_HORKAGE_NO_DMA_LOG flags. so the "no" option means set. If we add the
off version for non reversed flags, then the "no" option would clear the
flag, not set it. It is a mess. That is the cleanest way I could think of
without making things even more messy.
At best, we can allow everything to be set/cleared using 2 macros:
onoff and offon, depending on the flag meaning polarity (i.e. a NO flag or
not).
>
> Style-differences apart it looks good.
>
> Cheers,
>
> Hannes
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags
2022-04-25 6:15 ` Damien Le Moal
@ 2022-04-25 6:44 ` Hannes Reinecke
0 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2022-04-25 6:44 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Jonathan Corbet, linux-doc; +Cc: Sergey Shtylyov
On 4/25/22 08:15, Damien Le Moal wrote:
> On 4/25/22 15:00, Hannes Reinecke wrote:
>> On 4/25/22 03:34, Damien Le Moal wrote:
>>> To facilitate debugging of drive issues in the field without kernel
>>> changes (e.g. temporary test patches), add an entry for most horkage
>>> flags in the force_tbl array to allow controlling these horkage
>>> settings with the libata.force kernel boot parameter.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>> drivers/ata/libata-core.c | 22 ++++++++++++++++++++--
>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index e5a0e73b39d3..f68cb5639ec4 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -6230,9 +6230,27 @@ static const struct ata_force_param force_tbl[] __initconst = {
>>> force_horkage_onoff(ncqtrim, ATA_HORKAGE_NO_NCQ_TRIM),
>>> force_horkage_onoff(ncqati, ATA_HORKAGE_NO_NCQ_ON_ATI),
>>>
>>> - force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
>>> + force_horkage_onoff(trim, ATA_HORKAGE_NOTRIM),
>>> + force_horkage_on(trim_zero, ATA_HORKAGE_ZERO_AFTER_TRIM),
>>> + force_horkage_on(max_trim_128m, ATA_HORKAGE_MAX_TRIM_128M),
>>> +
>>> + force_horkage_onoff(dma, ATA_HORKAGE_NODMA),
>>> force_horkage_on(atapi_dmadir, ATA_HORKAGE_ATAPI_DMADIR),
>>> - force_horkage_on(disable, ATA_HORKAGE_DISABLE)
>>> + force_horkage_on(atapi_mod16_dma, ATA_HORKAGE_ATAPI_MOD16_DMA),
>>> +
>>> + force_horkage_onoff(dmalog, ATA_HORKAGE_NO_DMA_LOG),
>>> + force_horkage_onoff(iddevlog, ATA_HORKAGE_NO_ID_DEV_LOG),
>>> + force_horkage_onoff(logdir, ATA_HORKAGE_NO_LOG_DIR),
>>> +
>>> + force_horkage_on(max_sec_128, ATA_HORKAGE_MAX_SEC_128),
>>> + force_horkage_on(max_sec_1024, ATA_HORKAGE_MAX_SEC_1024),
>>> + force_horkage_on(max_sec_lba48, ATA_HORKAGE_MAX_SEC_LBA48),
>>> +
>>> + force_horkage_onoff(lpm, ATA_HORKAGE_NOLPM),
>>> + force_horkage_onoff(setxfer, ATA_HORKAGE_NOSETXFER),
>>> + force_horkage_on(dump_id, ATA_HORKAGE_DUMP_ID),
>>> +
>>> + force_horkage_on(disable, ATA_HORKAGE_DISABLE),
>>
>> ... and this exemplifies my concerns with the 'onoff' mechanism:
>> Why is 'disable' just marked as 'on' ?
>
> Yeah, I can add the off side of it too. Fairly useless though as these are
> off by default, except for the few cases where we already know that the
> flag is needed, in which case turning it off would be a bad idea. So I do
> not allow it by having the "on" only.
>
>> Sure we can set it to 'off' (we have to, otherwise that flag would
>> always be set). And if we can set it to 'off', where's the different to
>> 'onoff' ?
>
> Because of the reversed definition of the flag. E.g. nodmalog means *set*
> ATA_HORKAGE_NO_DMA_LOG flags. so the "no" option means set. If we add the
> off version for non reversed flags, then the "no" option would clear the
> flag, not set it. It is a mess. That is the cleanest way I could think of
> without making things even more messy.
>
> At best, we can allow everything to be set/cleared using 2 macros:
> onoff and offon, depending on the flag meaning polarity (i.e. a NO flag or
> not).
>
Hmm. Yeah, I see. Ugly, but probably the easiest for now.
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-04-25 6:44 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-25 1:34 [PATCH v2 0/5] libata.force improvements Damien Le Moal
2022-04-25 1:34 ` [PATCH v2 1/5] ata: libata-core: cleanup ata_device_blacklist Damien Le Moal
2022-04-25 5:49 ` Hannes Reinecke
2022-04-25 1:34 ` [PATCH v2 2/5] ata: libata-core: Refactor force_tbl definition Damien Le Moal
2022-04-25 5:51 ` Hannes Reinecke
2022-04-25 1:34 ` [PATCH v2 3/5] ata: libata-core: Improve link flags forced settings Damien Le Moal
2022-04-25 5:56 ` Hannes Reinecke
2022-04-25 6:08 ` Damien Le Moal
2022-04-25 6:10 ` Hannes Reinecke
2022-04-25 1:34 ` [PATCH v2 4/5] ata: libata-core: Allow forcing most horkage flags Damien Le Moal
2022-04-25 6:00 ` Hannes Reinecke
2022-04-25 6:15 ` Damien Le Moal
2022-04-25 6:44 ` Hannes Reinecke
2022-04-25 1:34 ` [PATCH v2 5/5] doc: admin-guide: Update libata kernel parameters Damien Le Moal
2022-04-25 1:37 ` Damien Le Moal
2022-04-25 6:01 ` Hannes Reinecke
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.