| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADDb1s2xX6kzeTO6pjmE9LLcd_NxcZncE99E=roi1DfZGto4vA@mail.gmail.com>
Date: Mon, 6 Feb 2012 11:10:22 +0530
From: Amit Sahrawat <amit.sahrawat83@...il.com>
To: Sergei Shtylyov <sshtylyov@...sta.com>
Cc: Jeff Garzik <jgarzik@...ox.com>,
"James E.J. Bottomley" <JBottomley@...allels.com>,
Nam-Jae Jeon <namjae.jeon@...sung.com>,
Amit Sahrawat <a.sahrawat@...sung.com>,
linux-ide@...r.kernel.org, linux-scsi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] scsi: retrieve cache mode using ATA_16 if normal
routine fails
Hi Sergei,
Thanks for your review comments.
On Sun, Feb 5, 2012 at 5:35 PM, Sergei Shtylyov <sshtylyov@...sta.com> wrote:
> Hello.
>
>
> On 03-02-2012 16:59, Amit Sahrawat wrote:
>
>> It has been observed that a number of USB HDD's do not respond correctly
>
>
> Why are you working around this in libata-scsi.c then if it's USB HDD?
There has been a history to this problem. It has been a long pending
problem with SCSI layer and USB HDD. We observed when we encountered
various Filesystem corruption issues on USB HDD and on analysis - we
found that there is no SYNCHRONIZE_CACHE command going to a number of
USB HDD even though - they do seem to possess WRITE CACHE.
So, drivers/scsi/sd.c - was where we found that WCE is being set for
these kind of devices which will in-turn take care of setting the
correct 'cache_type - Writeback' and also take care of proper working
(by managing correct order in requests queue).
We first did a change by introducing - mechanism to retrieve cache
bit in sd_read_cache_type, alongwith this we provided an interface
using 'syfs' if we can change the cache type from that place.
But, we received review comments in this regards - that the layer on
which we are doing changes is not the proper place(and also ATA_16
seems to crash some old HDD's, and were advised to look out for
changes in libata-scsi instead).(Original link -
http://lkml.org/lkml/2011/12/12/164)
So, we went for changes in libata-scsi.
Please let me know in case more information is needed in this regards,
or may be if you could provide more inputs to these sort of changes.
>
>
>> to SCSI mode sense command(retrieve caching pages) which results in their
>> Write Cache being discarded by queue requests i.e., WCE if left set to
>> '0'(disabled).
>> This results in a number of Filesystem corruptions, when the device
>> is unplugged abruptly.
>
>
>> So, in order to identify the devices correctly - give it
>> a last try using ATA_16 after failure from normal routine.
>
>
> Shouldn't this be done in drivers/usb/storage somewhere?
>
>
>> Signed-off-by: Amit Sahrawat<a.sahrawat@...sung.com>
>> Signed-off-by: Namjae Jeon<namjae.jeon@...sung.com>
>
>
> [...]
>
>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 508a60b..d5b00e6 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -562,6 +562,57 @@ error:
>> }
>>
>> /**
>> + * ata_get_cachestatus - Handler for to get WriteCache Status
>> + * using ATA_16 scsi command
>> + * @scsidev: Device to which we are issuing command
>> + *
>> + * LOCKING:
>> + * Defined by the SCSI layer. We don't really care.
>> + *
>> + * RETURNS:
>> + * '0' for Write Cache Disabled and on any error.
>> + * '1' for Write Cache enabled
>> + */
>> +int ata_get_cachestatus(struct scsi_device *scsidev)
>> +{
>> + int rc = 0;
>> + u8 scsi_cmd[MAX_COMMAND_SIZE] = {0};
>> + u8 *sensebuf = NULL, *argbuf = NULL;
>
>
> No need to initialize them.
Tried to maintain the uniformity in the code for this file, at other
places it is being done like.
>
>
>> + enum dma_data_direction data_dir;
>> +
>> + sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
>> + if (!sensebuf)
>> + return rc;
>> +
>> + argbuf = kmalloc(ATA_SECT_SIZE, GFP_KERNEL);
>> + if (argbuf == NULL)
>
>
> Could you use an uniform NULL-check, either '!x' or 'x == NULL'?
Agreed, need to stick with one type of comparision.
>
>
>> + goto error;
>> +
>> + scsi_cmd[0] = ATA_16;
>> + scsi_cmd[1] = (4<< 1); /* PIO Data-in */
>> + scsi_cmd[2] = 0x0e; /* no off.line or cc, read from dev,
>> + block count in sector count field */
>> + data_dir = DMA_FROM_DEVICE;
>> + scsi_cmd[14] = ATA_IDENTIFY_DEV;
>> +
>> + if (!scsi_execute(scsidev, scsi_cmd, data_dir, argbuf,
>> ATA_SECT_SIZE,
>> + sensebuf, (10*HZ), 5, 0, NULL)) {
>> + /*
>> + * '6th' Bit in Word 85 Corresponds to Write Cache
>
>
> No need for apostrophes here.
Tried to highlight the 6th bit, if this is not the convention, I will
remove this.
>
>> + * being Enabled/disabled, Word 85 represnets the
>> + * features supported
>> + */
>> + if (le16_to_cpu(((unsigned short *)argbuf)[85])& 0x20)
>>
>> + rc = 1;
>> + }
>> +
>> +error:
>> + kfree(sensebuf);
>> + kfree(argbuf);
>> + return rc;
>> +}
>> +
>> +/**
>> * ata_task_ioctl - Handler for HDIO_DRIVE_TASK ioctl
>> * @scsidev: Device to which we are issuing command
>> * @arg: User provided data for issuing command
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index c691fb5..a6b887d 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -50,6 +50,11 @@
>> #include<linux/string_helpers.h>
>> #include<linux/async.h>
>> #include<linux/slab.h>
>> +
>> +#ifdef CONFIG_ATA
>
>
> #ifdef not necessary here.
Yes, but just wanted to specifiy for code readability, as there is no
purpose to include - libata header in sd.c
>
>
>> +#include<linux/libata.h>
>> +#endif
>> +
>> #include<linux/pm_runtime.h>
>> #include<asm/uaccess.h>
>> #include<asm/unaligned.h>
>> @@ -2129,7 +2134,11 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned
>> char *buffer)
>> if (modepage == 0x3F) {
>> sd_printk(KERN_ERR, sdkp, "No Caching mode page "
>> "present\n");
>> +#ifdef CONFIG_ATA
>> + goto WCE_USING_ATA;
>
>
> Yikes.
>
>
>> +#else
>> goto defaults;
>> +#endif
>> } else if ((buffer[offset]& 0x3f) != modepage) {
>> sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
>> goto defaults;
>> @@ -2149,6 +2158,14 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned
>> char *buffer)
>> "Uses READ/WRITE(6), disabling FUA\n");
>> sdkp->DPOFUA = 0;
>> }
>> +#ifdef CONFIG_ATA
>> +WCE_USING_ATA:
>> + if (!sdp->removable && !sdkp->WCE) {
>> + sd_printk(KERN_NOTICE, sdkp, "Try to check write
>> cache "
>> + "enable/disable using ATA command\n");
>> + sdkp->WCE = ata_get_cachestatus(sdp);
>> + }
>> +#endif
>>
>> if (sdkp->first_scan || old_wce != sdkp->WCE ||
>> old_rcd != sdkp->RCD || old_dpofua != sdkp->DPOFUA)
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index cafc09a..33fc73f 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -84,6 +84,8 @@
>> } \
>> })
>>
>> +#define ATA_IDENTIFY_DEV 0xEC
>> +
>
>
> This is alerady #defined in <linux/ata.h> as ATA_CMD_ID_ATA.
Yes, will include that instead of redifining.
>
> MBR, Sergei
Please share your inputs to this.
Thanks & Regards,
Amit Sahrawat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists