> + 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?
> @@ -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?
--
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
