Hi Sergei, Thanks for your review comments. On Sun, Feb 5, 2012 at 5:35 PM, Sergei Shtylyov <sshtylyov@xxxxxxxxxx> 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@xxxxxxxxxxx> >> Signed-off-by: Namjae Jeon<namjae.jeon@xxxxxxxxxxx> > > > [...] > > >> 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-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
