Re: [PATCH v4] Add support for SCT Write Same
From: Christoph Hellwig <hch@lst.de>
Date: 2016-08-01 09:41:53
Also in:
lkml
+ u8 unmap = cdb[1] & 0x8; + bool use_sct = unmap ? false : true;
I think this would be a bit easier to understand if the use_sct flag goes away, and we instead just key off "if (unmap)" below.
- /* for now we only support WRITE SAME with the unmap bit set */
- if (unlikely(!(cdb[1] & 0x8))) {
+ /*
+ * If UNMAP is not set and SCT write same is not available then fail.
+ */
+ if (use_sct && !ata_id_sct_write_same(dev->id)) {
fp = 1;
bp = 3;
goto invalid_fld;I don't think the comment adds much value. But now that we implement WRITE SAME for both the unmap and non-unmap case we'll also need a similar check here to reject a WRITE SAME with the unmap flag if TRIM isn't supported, don't we?
quoted hunk ↗ jump to hunk
@@ -3304,26 +3315,56 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) if (!scsi_sg_count(scmd)) goto invalid_param_len; - buf = page_address(sg_page(scsi_sglist(scmd))); - size = ata_set_lba_range_entries(buf, 512, block, n_block); + sg = scsi_sglist(scmd); + buf = page_address(sg_page(sg)) + sg->offset;
I still think that this is broken for arbitrary user space pass through command (and probably was broken before you touched the code). We really need something that does a kmap here. I think the safest think to do is to rewrite ata_set_lba_range_entries to use sg_copy_from_buffer ( and move it out of the header while we're at it). That should be a prep patch to the SCT support.
+ /*
+ * if we only have SCT then ignore the state of unmap request
+ * a zero the blocks.
+ */
+ if (use_sct) {The comment doesn't match what the code does. It only uses SCT if use_sct, aka !unmap is true.
/** + * ata_scsiop_maint_in - Simulate a subset of MAINTENANCE_IN + * @args: device MAINTENANCE_IN data / SCSI command of interest. + * @rbuf: Response buffer, to which simulated SCSI cmd output is sent. + * + * Yields a subset to satisfy scsi_report_opcode() + * + * LOCKING: + * spin_lock_irqsave(host lock)
+ switch (cdb[3]) {
+ case INQUIRY:
+ case FORMAT_UNIT:
+ case MODE_SENSE:
+ case MODE_SENSE_10:
+ case READ_CAPACITY:
+ case SERVICE_ACTION_IN_16:
+ case REPORT_LUNS:
+ case REQUEST_SENSE:
+ case SYNCHRONIZE_CACHE:
+ case REZERO_UNIT:
+ case SEEK_6:
+ case SEEK_10:
+ case TEST_UNIT_READY:
+ case SEND_DIAGNOSTIC:
+ supported = 3;
+ break;
+ case MAINTENANCE_IN:
+ supported = 3;
+ break;
+ case READ_6:
+ case READ_10:
+ case READ_16:
+ case WRITE_6:
+ case WRITE_10:
+ case WRITE_16:
+ supported = 3;
+ break;Please handle all the supported commands in a single fallthrough statement. Also please make supported a bool and do the conversion to the magic SCSI format in a single place at the end of the function.
+ case ZBC_IN: + case ZBC_OUT: + supported = 3; + break;
Should we report this for non-ZBC devices?