* [PATCH 0/3] Improve libata support for FUA
@ 2022-10-21 5:38 Damien Le Moal
2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 5:38 UTC (permalink / raw)
To: linux-ide, Maciej S . Szmigiero; +Cc: Hannes Reinecke
These patches cleanup and improve libata support for the FUA device
feature. Patch 3 enables FUA support by default for any drive that
reports supporting the feature.
Damien Le Moal (2):
ata: libata: cleanup fua handling
ata: libata: Enable fua support by default
Maciej S. Szmigiero (1):
ata: libata: allow toggling fua parameter at runtime
.../admin-guide/kernel-parameters.txt | 3 ++
drivers/ata/libata-core.c | 35 ++++++++++++++++---
drivers/ata/libata-scsi.c | 30 ++--------------
include/linux/libata.h | 8 +++--
4 files changed, 41 insertions(+), 35 deletions(-)
--
2.37.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/3] ata: libata: cleanup fua handling 2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal @ 2022-10-21 5:38 ` Damien Le Moal 2022-10-21 6:20 ` Hannes Reinecke 2022-10-21 5:38 ` [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime Damien Le Moal ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: Damien Le Moal @ 2022-10-21 5:38 UTC (permalink / raw) To: linux-ide, Maciej S . Szmigiero; +Cc: Hannes Reinecke Move the detection of a device FUA support from ata_scsiop_mode_sense()/ata_dev_supports_fua() to device scan time in ata_dev_configure(). The function ata_dev_config_fua() is introduced to detect a device FUA support and this support is indicated using the new device flag ATA_DFLAG_FUA. In order to blacklist known buggy devices, the horkage flag ATA_HORKAGE_NO_FUA is introduced. Similarly to other horkage flags, the arguments fua and nofua are also introduced to allow a user to control this horkage flag through the "force" libata module parameter. The ATA_DFLAG_FUA device flag is set only and only if all the following conditions are met: * libata.fua module parameter is set to 1 * The device is not marked with the ATA_HORKAGE_NO_FUA flag, either from the blacklist or set by the user with libata.force=nofua * The device advertizes support for the WRITE DMA FUA EXT command, * The device supports LBA48 and is not restricted to single block PIO Note: Enabling or diabling libata fua support for all devices by default can now by done using either the "fua" module parameter or the "force=[port[.device]][no]fua" module parameter when libata.fua==1. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- .../admin-guide/kernel-parameters.txt | 3 ++ drivers/ata/libata-core.c | 29 +++++++++++++++++- drivers/ata/libata-scsi.c | 30 ++----------------- include/linux/libata.h | 8 +++-- 4 files changed, 38 insertions(+), 32 deletions(-) diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index a465d5242774..f9724642c703 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -2786,6 +2786,9 @@ * [no]setxfer: Indicate if transfer speed mode setting should be skipped. + * [no]fua: Disable or enable FUA (Force Unit Access) + support for devices supporting this feature. + * dump_id: Dump IDENTIFY data. * disable: Disable this device. diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 884ae73b11ea..6008f7ed1c42 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -2420,6 +2420,27 @@ static void ata_dev_config_chs(struct ata_device *dev) dev->heads, dev->sectors); } +static void ata_dev_config_fua(struct ata_device *dev) +{ + /* Ignore FUA support if its use is globally disabled */ + if (!libata_fua) + goto nofua; + + /* Ignore devices without support and known bad devices */ + if (!ata_id_has_fua(dev->id) || (dev->horkage & ATA_HORKAGE_NO_FUA)) + goto nofua; + + /* Limit FUA support to LBA48 without PIO restriction */ + if ((dev->flags & ATA_DFLAG_LBA48) && + (!(dev->flags & ATA_DFLAG_PIO) || dev->multi_count)) { + dev->flags |= ATA_DFLAG_FUA; + return; + } + +nofua: + dev->flags &= ~ATA_DFLAG_FUA; +} + static void ata_dev_config_devslp(struct ata_device *dev) { u8 *sata_setting = dev->link->ap->sector_buf; @@ -2508,7 +2529,8 @@ static void ata_dev_print_features(struct ata_device *dev) return; ata_dev_info(dev, - "Features:%s%s%s%s%s%s\n", + "Features:%s%s%s%s%s%s%s\n", + dev->flags & ATA_DFLAG_FUA ? " FUA" : "", dev->flags & ATA_DFLAG_TRUSTED ? " Trust" : "", dev->flags & ATA_DFLAG_DA ? " Dev-Attention" : "", dev->flags & ATA_DFLAG_DEVSLP ? " Dev-Sleep" : "", @@ -2669,6 +2691,7 @@ int ata_dev_configure(struct ata_device *dev) ata_dev_config_chs(dev); } + ata_dev_config_fua(dev); ata_dev_config_devslp(dev); ata_dev_config_sense_reporting(dev); ata_dev_config_zac(dev); @@ -4103,6 +4126,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { */ { "SATADOM-ML 3ME", NULL, ATA_HORKAGE_NO_LOG_DIR }, + /* Buggy FUA */ + { "Maxtor", "BANC1G10", ATA_HORKAGE_NO_FUA }, + /* End Marker */ { } }; @@ -6214,6 +6240,7 @@ static const struct ata_force_param force_tbl[] __initconst = { 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_onoff(fua, ATA_HORKAGE_NO_FUA), force_horkage_on(disable, ATA_HORKAGE_DISABLE), }; diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 4cb914103382..69948e2a8f6d 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2240,30 +2240,6 @@ static unsigned int ata_msense_rw_recovery(u8 *buf, bool changeable) return sizeof(def_rw_recovery_mpage); } -/* - * We can turn this into a real blacklist if it's needed, for now just - * blacklist any Maxtor BANC1G10 revision firmware - */ -static int ata_dev_supports_fua(u16 *id) -{ - unsigned char model[ATA_ID_PROD_LEN + 1], fw[ATA_ID_FW_REV_LEN + 1]; - - if (!libata_fua) - return 0; - if (!ata_id_has_fua(id)) - return 0; - - ata_id_c_string(id, model, ATA_ID_PROD, sizeof(model)); - ata_id_c_string(id, fw, ATA_ID_FW_REV, sizeof(fw)); - - if (strcmp(model, "Maxtor")) - return 1; - if (strcmp(fw, "BANC1G10")) - return 1; - - return 0; /* blacklisted */ -} - /** * ata_scsiop_mode_sense - Simulate MODE SENSE 6, 10 commands * @args: device IDENTIFY data / SCSI command of interest. @@ -2287,7 +2263,7 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf) }; u8 pg, spg; unsigned int ebd, page_control, six_byte; - u8 dpofua, bp = 0xff; + u8 dpofua = 0, bp = 0xff; u16 fp; six_byte = (scsicmd[0] == MODE_SENSE); @@ -2350,9 +2326,7 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf) goto invalid_fld; } - dpofua = 0; - if (ata_dev_supports_fua(args->id) && (dev->flags & ATA_DFLAG_LBA48) && - (!(dev->flags & ATA_DFLAG_PIO) || dev->multi_count)) + if (dev->flags & ATA_DFLAG_FUA) dpofua = 1 << 4; if (six_byte) { diff --git a/include/linux/libata.h b/include/linux/libata.h index af4953b95f76..81d863d751e1 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -91,6 +91,7 @@ enum { ATA_DFLAG_AN = (1 << 7), /* AN configured */ ATA_DFLAG_TRUSTED = (1 << 8), /* device supports trusted send/recv */ ATA_DFLAG_DMADIR = (1 << 10), /* device requires DMADIR */ + ATA_DFLAG_FUA = (1 << 11), /* device supports FUA */ ATA_DFLAG_CFG_MASK = (1 << 12) - 1, ATA_DFLAG_PIO = (1 << 12), /* device limited to PIO mode */ @@ -113,9 +114,9 @@ enum { ATA_DFLAG_D_SENSE = (1 << 29), /* Descriptor sense requested */ ATA_DFLAG_ZAC = (1 << 30), /* ZAC device */ - ATA_DFLAG_FEATURES_MASK = ATA_DFLAG_TRUSTED | ATA_DFLAG_DA | \ - ATA_DFLAG_DEVSLP | ATA_DFLAG_NCQ_SEND_RECV | \ - ATA_DFLAG_NCQ_PRIO, + ATA_DFLAG_FEATURES_MASK = (ATA_DFLAG_TRUSTED | ATA_DFLAG_DA | \ + ATA_DFLAG_DEVSLP | ATA_DFLAG_NCQ_SEND_RECV | \ + ATA_DFLAG_NCQ_PRIO | ATA_DFLAG_FUA), ATA_DEV_UNKNOWN = 0, /* unknown device */ ATA_DEV_ATA = 1, /* ATA device */ @@ -381,6 +382,7 @@ enum { ATA_HORKAGE_NO_NCQ_ON_ATI = (1 << 27), /* Disable NCQ on ATI chipset */ ATA_HORKAGE_NO_ID_DEV_LOG = (1 << 28), /* Identify device log missing */ ATA_HORKAGE_NO_LOG_DIR = (1 << 29), /* Do not read log directory */ + ATA_HORKAGE_NO_FUA = (1 << 30), /* Do not use FUA */ /* DMA mask for user DMA control: User visible values; DO NOT renumber */ -- 2.37.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] ata: libata: cleanup fua handling 2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal @ 2022-10-21 6:20 ` Hannes Reinecke 0 siblings, 0 replies; 15+ messages in thread From: Hannes Reinecke @ 2022-10-21 6:20 UTC (permalink / raw) To: Damien Le Moal, linux-ide, Maciej S . Szmigiero On 10/21/22 07:38, Damien Le Moal wrote: > Move the detection of a device FUA support from > ata_scsiop_mode_sense()/ata_dev_supports_fua() to device scan time in > ata_dev_configure(). > > The function ata_dev_config_fua() is introduced to detect a device FUA > support and this support is indicated using the new device flag > ATA_DFLAG_FUA. In order to blacklist known buggy devices, the horkage > flag ATA_HORKAGE_NO_FUA is introduced. Similarly to other horkage flags, > the arguments fua and nofua are also introduced to allow a user to > control this horkage flag through the "force" libata module parameter. > > The ATA_DFLAG_FUA device flag is set only and only if all the following > conditions are met: > * libata.fua module parameter is set to 1 > * The device is not marked with the ATA_HORKAGE_NO_FUA flag, either from > the blacklist or set by the user with libata.force=nofua > * The device advertizes support for the WRITE DMA FUA EXT command, > * The device supports LBA48 and is not restricted to single block PIO > > Note: Enabling or diabling libata fua support for all devices by default > can now by done using either the "fua" module parameter or the > "force=[port[.device]][no]fua" module parameter when libata.fua==1. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > .../admin-guide/kernel-parameters.txt | 3 ++ > drivers/ata/libata-core.c | 29 +++++++++++++++++- > drivers/ata/libata-scsi.c | 30 ++----------------- > include/linux/libata.h | 8 +++-- > 4 files changed, 38 insertions(+), 32 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: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime 2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal 2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal @ 2022-10-21 5:38 ` Damien Le Moal 2022-10-21 6:21 ` Hannes Reinecke 2022-10-21 5:38 ` [PATCH 3/3] ata: libata: Enable fua support by default Damien Le Moal 2022-10-21 21:02 ` [PATCH 0/3] Improve libata support for FUA Maciej S. Szmigiero 3 siblings, 1 reply; 15+ messages in thread From: Damien Le Moal @ 2022-10-21 5:38 UTC (permalink / raw) To: linux-ide, Maciej S . Szmigiero; +Cc: Hannes Reinecke From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> Currently, the libata.fua parameter isn't runtime-writable, so a system restart is required in order to toggle it. This unnecessarily complicates testing how drives behave with FUA on and off. Let's make this parameter R/W instead, like many others in the kernel. Example usage: Disable the parameter: echo 0 >/sys/module/libata/parameters/fua Revalidate disk cache settings: F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F [Damien] Enabling fua support by setting libata.fua to 1 will have no effect if the libata module is loaded with libata.force=[ID]nofua, which disables fua support for the ata device(s) identified with ID or all ata devices if no ID is specified. Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/ata/libata-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 6008f7ed1c42..1bb9616b10d9 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -128,7 +128,7 @@ module_param(atapi_passthru16, int, 0444); MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])"); int libata_fua = 0; -module_param_named(fua, libata_fua, int, 0444); +module_param_named(fua, libata_fua, int, 0644); MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)"); static int ata_ignore_hpa; -- 2.37.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime 2022-10-21 5:38 ` [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime Damien Le Moal @ 2022-10-21 6:21 ` Hannes Reinecke 2022-10-21 6:50 ` Damien Le Moal 0 siblings, 1 reply; 15+ messages in thread From: Hannes Reinecke @ 2022-10-21 6:21 UTC (permalink / raw) To: Damien Le Moal, linux-ide, Maciej S . Szmigiero On 10/21/22 07:38, Damien Le Moal wrote: > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > Currently, the libata.fua parameter isn't runtime-writable, so a > system restart is required in order to toggle it. > This unnecessarily complicates testing how drives behave with FUA on and > off. > > Let's make this parameter R/W instead, like many others in the kernel. > > Example usage: > Disable the parameter: > echo 0 >/sys/module/libata/parameters/fua > > Revalidate disk cache settings: > F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F > > [Damien] > Enabling fua support by setting libata.fua to 1 will have no effect if > the libata module is loaded with libata.force=[ID]nofua, which disables > fua support for the ata device(s) identified with ID or all ata devices > if no ID is specified. > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/ata/libata-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 6008f7ed1c42..1bb9616b10d9 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -128,7 +128,7 @@ module_param(atapi_passthru16, int, 0444); > MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])"); > > int libata_fua = 0; > -module_param_named(fua, libata_fua, int, 0444); > +module_param_named(fua, libata_fua, int, 0644); > MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)"); > > static int ata_ignore_hpa; Hmm. I guess you'll need to revalidate the drive when changing that; but this can be done in a later patch. 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: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime 2022-10-21 6:21 ` Hannes Reinecke @ 2022-10-21 6:50 ` Damien Le Moal 2022-10-21 8:00 ` Damien Le Moal 0 siblings, 1 reply; 15+ messages in thread From: Damien Le Moal @ 2022-10-21 6:50 UTC (permalink / raw) To: Hannes Reinecke, linux-ide, Maciej S . Szmigiero On 10/21/22 15:21, Hannes Reinecke wrote: > On 10/21/22 07:38, Damien Le Moal wrote: >> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >> >> Currently, the libata.fua parameter isn't runtime-writable, so a >> system restart is required in order to toggle it. >> This unnecessarily complicates testing how drives behave with FUA on and >> off. >> >> Let's make this parameter R/W instead, like many others in the kernel. >> >> Example usage: >> Disable the parameter: >> echo 0 >/sys/module/libata/parameters/fua >> >> Revalidate disk cache settings: >> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F >> >> [Damien] >> Enabling fua support by setting libata.fua to 1 will have no effect if >> the libata module is loaded with libata.force=[ID]nofua, which disables >> fua support for the ata device(s) identified with ID or all ata devices >> if no ID is specified. >> >> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/ata/libata-core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index 6008f7ed1c42..1bb9616b10d9 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -128,7 +128,7 @@ module_param(atapi_passthru16, int, 0444); >> MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])"); >> >> int libata_fua = 0; >> -module_param_named(fua, libata_fua, int, 0444); >> +module_param_named(fua, libata_fua, int, 0644); >> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)"); >> >> static int ata_ignore_hpa; > Hmm. I guess you'll need to revalidate the drive when changing that; but > this can be done in a later patch. Well, this is not sysfs, we cannot do this automatically easily... And thinking about it now that you mention it, going from fua=1 to fua=0 can actually cause problems. The reverse not, since scsi side would still see fua=0 until revalidation. So... Unless we find a way to link the param write to reavlidation, we should actually not allow this. Maciej ? Thoughts ? > > Reviewed-by: Hannes Reinecke <hare@suse.de> > > Cheers, > > Hannes -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime 2022-10-21 6:50 ` Damien Le Moal @ 2022-10-21 8:00 ` Damien Le Moal 2022-10-21 8:45 ` Hannes Reinecke 0 siblings, 1 reply; 15+ messages in thread From: Damien Le Moal @ 2022-10-21 8:00 UTC (permalink / raw) To: Hannes Reinecke, linux-ide, Maciej S . Szmigiero On 10/21/22 15:50, Damien Le Moal wrote: > On 10/21/22 15:21, Hannes Reinecke wrote: >> On 10/21/22 07:38, Damien Le Moal wrote: >>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >>> >>> Currently, the libata.fua parameter isn't runtime-writable, so a >>> system restart is required in order to toggle it. >>> This unnecessarily complicates testing how drives behave with FUA on and >>> off. >>> >>> Let's make this parameter R/W instead, like many others in the kernel. >>> >>> Example usage: >>> Disable the parameter: >>> echo 0 >/sys/module/libata/parameters/fua >>> >>> Revalidate disk cache settings: >>> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F >>> >>> [Damien] >>> Enabling fua support by setting libata.fua to 1 will have no effect if >>> the libata module is loaded with libata.force=[ID]nofua, which disables >>> fua support for the ata device(s) identified with ID or all ata devices >>> if no ID is specified. >>> >>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> >>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >>> --- >>> drivers/ata/libata-core.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>> index 6008f7ed1c42..1bb9616b10d9 100644 >>> --- a/drivers/ata/libata-core.c >>> +++ b/drivers/ata/libata-core.c >>> @@ -128,7 +128,7 @@ module_param(atapi_passthru16, int, 0444); >>> MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])"); >>> >>> int libata_fua = 0; >>> -module_param_named(fua, libata_fua, int, 0444); >>> +module_param_named(fua, libata_fua, int, 0644); >>> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)"); >>> >>> static int ata_ignore_hpa; >> Hmm. I guess you'll need to revalidate the drive when changing that; but >> this can be done in a later patch. > > Well, this is not sysfs, we cannot do this automatically easily... > And thinking about it now that you mention it, going from fua=1 to fua=0 > can actually cause problems. The reverse not, since scsi side would still > see fua=0 until revalidation. > > So... Unless we find a way to link the param write to reavlidation, we > should actually not allow this. > Maciej ? Thoughts ? I looked at this a little more. We could define the operations (struct kernel_param_ops) manually together with the fua parameter declaration, but that would be really ugly... Given that we are switching to fua=1 by default, do you still need a dynamic argument ? I am now thinking that this patch should be dropped. > >> >> Reviewed-by: Hannes Reinecke <hare@suse.de> >> >> Cheers, >> >> Hannes > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime 2022-10-21 8:00 ` Damien Le Moal @ 2022-10-21 8:45 ` Hannes Reinecke 2022-10-21 8:48 ` Damien Le Moal 0 siblings, 1 reply; 15+ messages in thread From: Hannes Reinecke @ 2022-10-21 8:45 UTC (permalink / raw) To: Damien Le Moal, linux-ide, Maciej S . Szmigiero On 10/21/22 10:00, Damien Le Moal wrote: > On 10/21/22 15:50, Damien Le Moal wrote: >> On 10/21/22 15:21, Hannes Reinecke wrote: >>> On 10/21/22 07:38, Damien Le Moal wrote: >>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >>>> >>>> Currently, the libata.fua parameter isn't runtime-writable, so a >>>> system restart is required in order to toggle it. >>>> This unnecessarily complicates testing how drives behave with FUA on and >>>> off. >>>> >>>> Let's make this parameter R/W instead, like many others in the kernel. >>>> >>>> Example usage: >>>> Disable the parameter: >>>> echo 0 >/sys/module/libata/parameters/fua >>>> >>>> Revalidate disk cache settings: >>>> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F >>>> >>>> [Damien] >>>> Enabling fua support by setting libata.fua to 1 will have no effect if >>>> the libata module is loaded with libata.force=[ID]nofua, which disables >>>> fua support for the ata device(s) identified with ID or all ata devices >>>> if no ID is specified. >>>> >>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> >>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >>>> --- >>>> drivers/ata/libata-core.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>>> index 6008f7ed1c42..1bb9616b10d9 100644 >>>> --- a/drivers/ata/libata-core.c >>>> +++ b/drivers/ata/libata-core.c >>>> @@ -128,7 +128,7 @@ module_param(atapi_passthru16, int, 0444); >>>> MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])"); >>>> >>>> int libata_fua = 0; >>>> -module_param_named(fua, libata_fua, int, 0444); >>>> +module_param_named(fua, libata_fua, int, 0644); >>>> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)"); >>>> >>>> static int ata_ignore_hpa; >>> Hmm. I guess you'll need to revalidate the drive when changing that; but >>> this can be done in a later patch. >> >> Well, this is not sysfs, we cannot do this automatically easily... >> And thinking about it now that you mention it, going from fua=1 to fua=0 >> can actually cause problems. The reverse not, since scsi side would still >> see fua=0 until revalidation. >> >> So... Unless we find a way to link the param write to reavlidation, we >> should actually not allow this. >> Maciej ? Thoughts ? > > I looked at this a little more. We could define the operations (struct > kernel_param_ops) manually together with the fua parameter declaration, > but that would be really ugly... > > Given that we are switching to fua=1 by default, do you still need a > dynamic argument ? I am now thinking that this patch should be dropped. > I'd kill it, and let users it handle via blacklist flags only. 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] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime 2022-10-21 8:45 ` Hannes Reinecke @ 2022-10-21 8:48 ` Damien Le Moal 0 siblings, 0 replies; 15+ messages in thread From: Damien Le Moal @ 2022-10-21 8:48 UTC (permalink / raw) To: Hannes Reinecke, linux-ide, Maciej S . Szmigiero On 10/21/22 17:45, Hannes Reinecke wrote: > On 10/21/22 10:00, Damien Le Moal wrote: >> On 10/21/22 15:50, Damien Le Moal wrote: >>> On 10/21/22 15:21, Hannes Reinecke wrote: >>>> On 10/21/22 07:38, Damien Le Moal wrote: >>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >>>>> >>>>> Currently, the libata.fua parameter isn't runtime-writable, so a >>>>> system restart is required in order to toggle it. >>>>> This unnecessarily complicates testing how drives behave with FUA on and >>>>> off. >>>>> >>>>> Let's make this parameter R/W instead, like many others in the kernel. >>>>> >>>>> Example usage: >>>>> Disable the parameter: >>>>> echo 0 >/sys/module/libata/parameters/fua >>>>> >>>>> Revalidate disk cache settings: >>>>> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F >>>>> >>>>> [Damien] >>>>> Enabling fua support by setting libata.fua to 1 will have no effect if >>>>> the libata module is loaded with libata.force=[ID]nofua, which disables >>>>> fua support for the ata device(s) identified with ID or all ata devices >>>>> if no ID is specified. >>>>> >>>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> >>>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >>>>> --- >>>>> drivers/ata/libata-core.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >>>>> index 6008f7ed1c42..1bb9616b10d9 100644 >>>>> --- a/drivers/ata/libata-core.c >>>>> +++ b/drivers/ata/libata-core.c >>>>> @@ -128,7 +128,7 @@ module_param(atapi_passthru16, int, 0444); >>>>> MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])"); >>>>> >>>>> int libata_fua = 0; >>>>> -module_param_named(fua, libata_fua, int, 0444); >>>>> +module_param_named(fua, libata_fua, int, 0644); >>>>> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)"); >>>>> >>>>> static int ata_ignore_hpa; >>>> Hmm. I guess you'll need to revalidate the drive when changing that; but >>>> this can be done in a later patch. >>> >>> Well, this is not sysfs, we cannot do this automatically easily... >>> And thinking about it now that you mention it, going from fua=1 to fua=0 >>> can actually cause problems. The reverse not, since scsi side would still >>> see fua=0 until revalidation. >>> >>> So... Unless we find a way to link the param write to reavlidation, we >>> should actually not allow this. >>> Maciej ? Thoughts ? >> >> I looked at this a little more. We could define the operations (struct >> kernel_param_ops) manually together with the fua parameter declaration, >> but that would be really ugly... >> >> Given that we are switching to fua=1 by default, do you still need a >> dynamic argument ? I am now thinking that this patch should be dropped. >> > I'd kill it, and let users it handle via blacklist flags only. Yep, with the default set to 1 that is the goal. I kept the fua module parameter for backward compatibility, in case some setups out there use it. But the force=[ID]nofua or force=[ID]fua module parameters should be the preferred way to control this now. > > Cheers, > > Hannes -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] ata: libata: Enable fua support by default 2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal 2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal 2022-10-21 5:38 ` [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime Damien Le Moal @ 2022-10-21 5:38 ` Damien Le Moal 2022-10-21 6:22 ` Hannes Reinecke 2022-10-21 21:02 ` [PATCH 0/3] Improve libata support for FUA Maciej S. Szmigiero 3 siblings, 1 reply; 15+ messages in thread From: Damien Le Moal @ 2022-10-21 5:38 UTC (permalink / raw) To: linux-ide, Maciej S . Szmigiero; +Cc: Hannes Reinecke Change the default value of the fua module parameter to 1 to enable fua support by default for all devices supporting it. This parameter description is also updated to indicate it is deprecated and that libata.force=[no]fua should be used to control fua support. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/ata/libata-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 1bb9616b10d9..140e7e7d7833 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -127,9 +127,9 @@ int atapi_passthru16 = 1; module_param(atapi_passthru16, int, 0444); MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])"); -int libata_fua = 0; +int libata_fua = 1; module_param_named(fua, libata_fua, int, 0644); -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)"); +MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])"); static int ata_ignore_hpa; module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644); -- 2.37.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] ata: libata: Enable fua support by default 2022-10-21 5:38 ` [PATCH 3/3] ata: libata: Enable fua support by default Damien Le Moal @ 2022-10-21 6:22 ` Hannes Reinecke 0 siblings, 0 replies; 15+ messages in thread From: Hannes Reinecke @ 2022-10-21 6:22 UTC (permalink / raw) To: Damien Le Moal, linux-ide, Maciej S . Szmigiero On 10/21/22 07:38, Damien Le Moal wrote: > Change the default value of the fua module parameter to 1 to enable fua > support by default for all devices supporting it. This parameter > description is also updated to indicate it is deprecated and that > libata.force=[no]fua should be used to control fua support. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/ata/libata-core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 1bb9616b10d9..140e7e7d7833 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -127,9 +127,9 @@ int atapi_passthru16 = 1; > module_param(atapi_passthru16, int, 0444); > MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])"); > > -int libata_fua = 0; > +int libata_fua = 1; > module_param_named(fua, libata_fua, int, 0644); > -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)"); > +MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])"); > > static int ata_ignore_hpa; > module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644); 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: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Improve libata support for FUA 2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal ` (2 preceding siblings ...) 2022-10-21 5:38 ` [PATCH 3/3] ata: libata: Enable fua support by default Damien Le Moal @ 2022-10-21 21:02 ` Maciej S. Szmigiero 2022-10-21 22:45 ` Damien Le Moal 3 siblings, 1 reply; 15+ messages in thread From: Maciej S. Szmigiero @ 2022-10-21 21:02 UTC (permalink / raw) To: Damien Le Moal; +Cc: Hannes Reinecke, linux-ide On 21.10.2022 07:38, Damien Le Moal wrote: > These patches cleanup and improve libata support for the FUA device > feature. Patch 3 enables FUA support by default for any drive that > reports supporting the feature. > > Damien Le Moal (2): > ata: libata: cleanup fua handling > ata: libata: Enable fua support by default > > Maciej S. Szmigiero (1): > ata: libata: allow toggling fua parameter at runtime > Thanks Damien for the series! I've looked at the code changes and have basically two points: 1) There seems to be no way to revalidate the FUA setting for an existing disk, since it is now only taken into account in ata_dev_config_fua(). As far as I can see, this function is only called on probe paths (and during exception handling), so if the "libata.fua" parameter is toggled the new setting would only affect newly (re-)attached disks. Previously, this parameter was read directly in ata_scsiop_mode_sense() (specifically in ata_dev_supports_fua() called from there), which could be called to re-compute the FUA setting for an existing disk by re-writing the "cache_type" sysfs attribute (as described in my commit message). If that's indeed the case this severely limits the usefulness of having this parameter runtime-writable, and I agree with your discussion with Hannes that it isn't probably needed now (after all, probably nobody has an explicit "libata.fua=0" in their kernel command line, since this was the default setting anyway). 2) It would be good to collect known-broken disks from the similar FUA enabling attempt in 2012 [1] and add them to the blacklist upfront, so these users won't have to report them again. The problematic disks reported in that thread were: > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > ata1.00: ATA-7: WDC WD2500JS-41MVB1, 10.02E01, max UDMA/133 > ata1.00: 488397168 sectors, multi 16: LBA48 > ata1.00: configured for UDMA/133 > scsi 0:0:0:0: Direct-Access ATA WDC WD2500JS-41M 10.0 PQ: 0 ANSI: 5 > [ 2.845750] ata1.00: ATA-9: OCZ-VERTEX3 MI, 2.06, max UDMA/133 > [ 2.845754] ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 31/32), AA > [ 2.865726] ata1.00: configured for UDMA/133 > [ 2.865955] scsi 0:0:0:0: Direct-Access ATA OCZ-VERTEX3 MI 2.06 PQ: 0 ANSI: 5 > [ 2.866722] sd 0:0:0:0: [sda] 234441648 512-byte logical blocks: (120 GB/111 GiB) > [ 3.934157] ata1.00: ATA-9: INTEL SSDSC2CT120A3, 300i, max UDMA/133 > [ 3.934266] ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 0/32) > [ 3.954145] ata1.00: configured for UDMA/133 > [ 3.954441] scsi 0:0:0:0: Direct-Access ATA INTEL SSDSC2CT12 > 300i PQ: 0 ANSI: 5 > [ 3.955233] sd 0:0:0:0: [sda] 234441648 512-byte logical blocks: (120 > GB/111 GiB) Thanks, Maciej [1]: https://lore.kernel.org/lkml/CA+6av4=uxu_q5U_46HtpUt=FSgbh3pZuAEY54J5_xK=MKWq-YQ@mail.gmail.com/ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Improve libata support for FUA 2022-10-21 21:02 ` [PATCH 0/3] Improve libata support for FUA Maciej S. Szmigiero @ 2022-10-21 22:45 ` Damien Le Moal 2022-10-22 13:50 ` Hannes Reinecke 0 siblings, 1 reply; 15+ messages in thread From: Damien Le Moal @ 2022-10-21 22:45 UTC (permalink / raw) To: Maciej S. Szmigiero; +Cc: Hannes Reinecke, linux-ide On 10/22/22 06:02, Maciej S. Szmigiero wrote: > On 21.10.2022 07:38, Damien Le Moal wrote: >> These patches cleanup and improve libata support for the FUA device >> feature. Patch 3 enables FUA support by default for any drive that >> reports supporting the feature. >> >> Damien Le Moal (2): >> ata: libata: cleanup fua handling >> ata: libata: Enable fua support by default >> >> Maciej S. Szmigiero (1): >> ata: libata: allow toggling fua parameter at runtime >> > > Thanks Damien for the series! > > I've looked at the code changes and have basically two points: > 1) There seems to be no way to revalidate the FUA setting for an existing > disk, since it is now only taken into account in ata_dev_config_fua(). > > As far as I can see, this function is only called on probe paths > (and during exception handling), so if the "libata.fua" parameter is > toggled the new setting would only affect newly (re-)attached disks. Yes. Indeed. Forcing an ATA revalidation needs some more trickery as the regular sd_revalidate() does not lead to ata_dev_configure() being called again. > Previously, this parameter was read directly in ata_scsiop_mode_sense() > (specifically in ata_dev_supports_fua() called from there), which could > be called to re-compute the FUA setting for an existing disk by > re-writing the "cache_type" sysfs attribute (as described in my commit > message). > > If that's indeed the case this severely limits the usefulness of having > this parameter runtime-writable, and I agree with your discussion with > Hannes that it isn't probably needed now (after all, probably nobody > has an explicit "libata.fua=0" in their kernel command line, since this > was the default setting anyway). OK. Then I will drop your patch. Safer that way. > 2) It would be good to collect known-broken disks from the similar FUA > enabling attempt in 2012 [1] and add them to the blacklist upfront, so > these users won't have to report them again. The code only had one Maxtor drive blacklisted for FUA. Patch one adds it to the horkage table. > > The problematic disks reported in that thread were: >> ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) >> ata1.00: ATA-7: WDC WD2500JS-41MVB1, 10.02E01, max UDMA/133 >> ata1.00: 488397168 sectors, multi 16: LBA48 >> ata1.00: configured for UDMA/133 >> scsi 0:0:0:0: Direct-Access ATA WDC WD2500JS-41M 10.0 PQ: 0 ANSI: 5 > >> [ 2.845750] ata1.00: ATA-9: OCZ-VERTEX3 MI, 2.06, max UDMA/133 >> [ 2.845754] ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 31/32), AA >> [ 2.865726] ata1.00: configured for UDMA/133 >> [ 2.865955] scsi 0:0:0:0: Direct-Access ATA OCZ-VERTEX3 MI 2.06 PQ: 0 ANSI: 5 >> [ 2.866722] sd 0:0:0:0: [sda] 234441648 512-byte logical blocks: (120 GB/111 GiB) > >> [ 3.934157] ata1.00: ATA-9: INTEL SSDSC2CT120A3, 300i, max UDMA/133 >> [ 3.934266] ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 0/32) >> [ 3.954145] ata1.00: configured for UDMA/133 >> [ 3.954441] scsi 0:0:0:0: Direct-Access ATA INTEL SSDSC2CT12 >> 300i PQ: 0 ANSI: 5 >> [ 3.955233] sd 0:0:0:0: [sda] 234441648 512-byte logical blocks: (120 >> GB/111 GiB) OK. I will check that thread and add these drives to the horkage list. Thanks ! > > Thanks, > Maciej > > [1]: https://lore.kernel.org/lkml/CA+6av4=uxu_q5U_46HtpUt=FSgbh3pZuAEY54J5_xK=MKWq-YQ@mail.gmail.com/ > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Improve libata support for FUA 2022-10-21 22:45 ` Damien Le Moal @ 2022-10-22 13:50 ` Hannes Reinecke 2022-10-23 0:27 ` Damien Le Moal 0 siblings, 1 reply; 15+ messages in thread From: Hannes Reinecke @ 2022-10-22 13:50 UTC (permalink / raw) To: Damien Le Moal, Maciej S. Szmigiero; +Cc: linux-ide On 10/22/22 00:45, Damien Le Moal wrote: > On 10/22/22 06:02, Maciej S. Szmigiero wrote: >> On 21.10.2022 07:38, Damien Le Moal wrote: >>> These patches cleanup and improve libata support for the FUA device >>> feature. Patch 3 enables FUA support by default for any drive that >>> reports supporting the feature. >>> >>> Damien Le Moal (2): >>> ata: libata: cleanup fua handling >>> ata: libata: Enable fua support by default >>> >>> Maciej S. Szmigiero (1): >>> ata: libata: allow toggling fua parameter at runtime >>> >> >> Thanks Damien for the series! >> >> I've looked at the code changes and have basically two points: >> 1) There seems to be no way to revalidate the FUA setting for an existing >> disk, since it is now only taken into account in ata_dev_config_fua(). >> >> As far as I can see, this function is only called on probe paths >> (and during exception handling), so if the "libata.fua" parameter is >> toggled the new setting would only affect newly (re-)attached disks. > > Yes. Indeed. Forcing an ATA revalidation needs some more trickery as the > regular sd_revalidate() does not lead to ata_dev_configure() being called > again. > But shouldn't we rather fix that? After Johns series of pre-allocating the SCSI devices we should be able to call ata_dev_configure() from sd_revalidate() ... 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: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Improve libata support for FUA 2022-10-22 13:50 ` Hannes Reinecke @ 2022-10-23 0:27 ` Damien Le Moal 0 siblings, 0 replies; 15+ messages in thread From: Damien Le Moal @ 2022-10-23 0:27 UTC (permalink / raw) To: Hannes Reinecke, Maciej S. Szmigiero; +Cc: linux-ide On 10/22/22 22:50, Hannes Reinecke wrote: > On 10/22/22 00:45, Damien Le Moal wrote: >> On 10/22/22 06:02, Maciej S. Szmigiero wrote: >>> On 21.10.2022 07:38, Damien Le Moal wrote: >>>> These patches cleanup and improve libata support for the FUA device >>>> feature. Patch 3 enables FUA support by default for any drive that >>>> reports supporting the feature. >>>> >>>> Damien Le Moal (2): >>>> ata: libata: cleanup fua handling >>>> ata: libata: Enable fua support by default >>>> >>>> Maciej S. Szmigiero (1): >>>> ata: libata: allow toggling fua parameter at runtime >>>> >>> >>> Thanks Damien for the series! >>> >>> I've looked at the code changes and have basically two points: >>> 1) There seems to be no way to revalidate the FUA setting for an existing >>> disk, since it is now only taken into account in ata_dev_config_fua(). >>> >>> As far as I can see, this function is only called on probe paths >>> (and during exception handling), so if the "libata.fua" parameter is >>> toggled the new setting would only affect newly (re-)attached disks. >> >> Yes. Indeed. Forcing an ATA revalidation needs some more trickery as the >> regular sd_revalidate() does not lead to ata_dev_configure() being called >> again. >> > But shouldn't we rather fix that? > After Johns series of pre-allocating the SCSI devices we should be able > to call ata_dev_configure() from sd_revalidate() ... Yes, that should work. Though I am not sure if we really want to call ata_dev_configure() every time sd_revalidate() is called, given the performance impact of going to EH to revalidate an ATA drive. On an average distro, there are quite a lot of revalidate going on... For this particular case though, changing libata fua module parameter value at libata run-time should trigger a revalidate of *all* ata drives, which is different from the regular per-device revalidate driven by events or the user changing a drive config through sysfs. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-10-23 0:27 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal 2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal 2022-10-21 6:20 ` Hannes Reinecke 2022-10-21 5:38 ` [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime Damien Le Moal 2022-10-21 6:21 ` Hannes Reinecke 2022-10-21 6:50 ` Damien Le Moal 2022-10-21 8:00 ` Damien Le Moal 2022-10-21 8:45 ` Hannes Reinecke 2022-10-21 8:48 ` Damien Le Moal 2022-10-21 5:38 ` [PATCH 3/3] ata: libata: Enable fua support by default Damien Le Moal 2022-10-21 6:22 ` Hannes Reinecke 2022-10-21 21:02 ` [PATCH 0/3] Improve libata support for FUA Maciej S. Szmigiero 2022-10-21 22:45 ` Damien Le Moal 2022-10-22 13:50 ` Hannes Reinecke 2022-10-23 0:27 ` Damien Le Moal
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.