| Message ID | 1465453967-11085-2-git-send-email-shaun@tancheff.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
> + if (ata_id_sct_write_same(dev->id)) > + sdev->sct_write_same = 1; > + What's the point of this flag? It should simply clear the no_write_same flag for this device. Due to the way how we have both a per-host and per-device flag that might not be completely trivial, but untangling that mess might be a good idea anyway. > @@ -3305,6 +3308,37 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) > goto invalid_param_len; > > buf = page_address(sg_page(scsi_sglist(scmd))); > + > + if (ata_id_sct_write_same(dev->id)) { Various comments: - The plain page_address above looks harmful, how do we know that the page is mapped into kernel memory? This might actually be broken already, though. - Why is this below the check that rejects non-unmap WRITE SAME commands? - Shouldn't we still translate discard command to TRIM? Maybe we need a check of the operation in the request structure.. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 9, 2016 at 4:22 AM, Christoph Hellwig <hch@infradead.org> wrote: >> + if (ata_id_sct_write_same(dev->id)) >> + sdev->sct_write_same = 1; >> + > > What's the point of this flag? It should simply clear the no_write_same > flag for this device. Due to the way how we have both a per-host and > per-device flag that might not be completely trivial, but untangling > that mess might be a good idea anyway. Agreed. I looks like clearing the no_write_same flag mostly works but the queue limits don't make any sense because they get cleared. I'll see if I can untangle it. >> @@ -3305,6 +3308,37 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >> goto invalid_param_len; >> >> buf = page_address(sg_page(scsi_sglist(scmd))); >> + >> + if (ata_id_sct_write_same(dev->id)) { > > Various comments: > > - The plain page_address above looks harmful, how do we know that > the page is mapped into kernel memory? This might actually be broken > already, though. I think it just happens to work because it's always used with a recently allocated page. Fixing it to include the (possible) offset is just a good thing. > - Why is this below the check that rejects non-unmap WRITE SAME > commands? Yeah, tunnel vision. For some reason I was thinking that it was either WRITE SAME or SCT. But this case is actually TRIM and/or SCT which makes the demuxing what the user expects a little less clear. Now I am thinking that the cleanest method is to try and honor the unmap flag to pick the command path. If trim is available and unmap is set then you get the current behavior. else if SCT is available follow the SCT path else fail with the current error (unmap is not set). In this way if you device supports both TRIM and SCT then you can WRITE SAME or TRIM. > - Shouldn't we still translate discard command to TRIM? Maybe we > need a check of the operation in the request structure.. >
On Thu, Jun 09, 2016 at 10:24:40PM -0500, Shaun Tancheff wrote: > > - The plain page_address above looks harmful, how do we know that > > the page is mapped into kernel memory? This might actually be broken > > already, though. > > I think it just happens to work because it's always used with a recently > allocated page. Fixing it to include the (possible) offset is just a good thing. I haven't spent much time with libata recently, but what prevents an arbitrary user page ending up here through SG_IO? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index bfec66f..b73eace 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1204,6 +1204,9 @@ static int ata_scsi_dev_config(struct scsi_device *sdev, if (!ata_id_has_unload(dev->id)) dev->flags |= ATA_DFLAG_NO_UNLOAD; + if (ata_id_sct_write_same(dev->id)) + sdev->sct_write_same = 1; + /* configure max sectors */ blk_queue_max_hw_sectors(q, dev->max_sectors); @@ -3305,6 +3308,37 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) goto invalid_param_len; buf = page_address(sg_page(scsi_sglist(scmd))); + + if (ata_id_sct_write_same(dev->id)) { + u16 *sctpg = buf; + + put_unaligned_le16(0x0002, &sctpg[0]); /* SCT_ACT_WRITE_SAME */ + put_unaligned_le16(0x0101, &sctpg[1]); /* WRITE PTRN FG */ + put_unaligned_le64(block, &sctpg[2]); + put_unaligned_le64(n_block, &sctpg[6]); + put_unaligned_le32(0u, &sctpg[10]); + + tf->hob_feature = 0; + tf->feature = 0; + tf->hob_nsect = 0; + tf->nsect = 1; + tf->lbah = 0; + tf->lbam = 0; + tf->lbal = ATA_CMD_STANDBYNOW1; + tf->hob_lbah = 0; + tf->hob_lbam = 0; + tf->hob_lbal = 0; + tf->device = ATA_CMD_STANDBYNOW1; + tf->protocol = ATA_PROT_DMA; + tf->command = ATA_CMD_WRITE_LOG_DMA_EXT; + tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | + ATA_TFLAG_LBA48 | ATA_TFLAG_WRITE; + + ata_qc_set_pc_nbytes(qc); + + return 0; + } + size = ata_set_lba_range_entries(buf, 512, block, n_block); if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) { diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index f459dff..b5ffcd3 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -794,7 +794,7 @@ static void sd_config_write_same(struct scsi_disk *sdkp) struct request_queue *q = sdkp->disk->queue; unsigned int logical_block_size = sdkp->device->sector_size; - if (sdkp->device->no_write_same) { + if (sdkp->device->no_write_same && !sdkp->device->sct_write_same) { sdkp->max_ws_blocks = 0; goto out; } diff --git a/include/linux/ata.h b/include/linux/ata.h index 99346be..4132de3 100644 --- a/include/linux/ata.h +++ b/include/linux/ata.h @@ -104,6 +104,7 @@ enum { ATA_ID_CFA_KEY_MGMT = 162, ATA_ID_CFA_MODES = 163, ATA_ID_DATA_SET_MGMT = 169, + ATA_ID_SCT_CMD_XPORT = 206, ATA_ID_ROT_SPEED = 217, ATA_ID_PIO4 = (1 << 1), @@ -778,6 +779,48 @@ static inline bool ata_id_sense_reporting_enabled(const u16 *id) } /** + * + * Word: 206 - SCT Command Transport + * 15:12 - Vendor Specific + * 11:6 - Reserved + * 5 - SCT Command Transport Data Tables supported + * 4 - SCT Command Transport Features Control supported + * 3 - SCT Command Transport Error Recovery Control supported + * 2 - SCT Command Transport Write Same supported + * 1 - SCT Command Transport Long Sector Access supported + * 0 - SCT Command Transport supported + */ +static inline bool ata_id_sct_data_tables(const u16 *id) +{ + return id[ATA_ID_SCT_CMD_XPORT] & (1 << 5) ? true : false; +} + +static inline bool ata_id_sct_features_ctrl(const u16 *id) +{ + return id[ATA_ID_SCT_CMD_XPORT] & (1 << 4) ? true : false; +} + +static inline bool ata_id_sct_error_recovery_ctrl(const u16 *id) +{ + return id[ATA_ID_SCT_CMD_XPORT] & (1 << 3) ? true : false; +} + +static inline bool ata_id_sct_write_same(const u16 *id) +{ + return id[ATA_ID_SCT_CMD_XPORT] & (1 << 2) ? true : false; +} + +static inline bool ata_id_sct_long_sector_access(const u16 *id) +{ + return id[ATA_ID_SCT_CMD_XPORT] & (1 << 1) ? true : false; +} + +static inline bool ata_id_sct_supported(const u16 *id) +{ + return id[ATA_ID_SCT_CMD_XPORT] & (1 << 0) ? true : false; +} + +/** * ata_id_major_version - get ATA level of drive * @id: Identify data * diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index a6c346d..66f5af7 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -157,6 +157,7 @@ struct scsi_device { unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */ unsigned no_report_opcodes:1; /* no REPORT SUPPORTED OPERATION CODES */ unsigned no_write_same:1; /* no WRITE SAME command */ + unsigned sct_write_same:1; /* Has WRITE SAME via SCT Command */ unsigned use_16_for_rw:1; /* Use read/write(16) over read/write(10) */ unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08 */ unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f */
SATA drives may support write same via SCT. This is useful for setting the drive contents to a specific pattern (0's). Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com> --- v2: - Remove fugly ata hacking from sd.c --- drivers/ata/libata-scsi.c | 34 ++++++++++++++++++++++++++++++++++ drivers/scsi/sd.c | 2 +- include/linux/ata.h | 43 +++++++++++++++++++++++++++++++++++++++++++ include/scsi/scsi_device.h | 1 + 4 files changed, 79 insertions(+), 1 deletion(-)