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@xxxxxxxxxx> >>> >>> 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@xxxxxxxxxx> >>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> >>> --- >>> 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@xxxxxxx> >> >> Cheers, >> >> Hannes > -- Damien Le Moal Western Digital Research
