Re: [PATCH v2 4/7] sd: Create helper functions for read/write commands

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2019-01-15 10:23 p.m., Bart Van Assche wrote:
On 1/15/19 6:34 PM, Douglas Gilbert wrote:
On 2019-01-15 7:50 p.m., Bart Van Assche wrote:
+static blk_status_t sd_setup_rw6_cmnd(struct scsi_cmnd *cmd, bool write,
+                      sector_t lba, unsigned int nr_blocks,
+                      unsigned char flags)
+{
+    if (unlikely(flags & 0x8)) {
+        /*
+         * This happens only if this drive failed 10byte rw
+         * command with ILLEGAL_REQUEST during operation and
+         * thus turned off use_10_for_rw.
+         */
+        scmd_printk(KERN_ERR, cmd, "FUA write on READ/WRITE(6) drive\n");
+        return BLK_STS_IOERR;
+    }
+
+    cmd->cmd_len = 6;
+    cmd->cmnd[0] = write ? WRITE_6 : READ_6;
+    cmd->cmnd[1] = (lba >> 16) & 0x1f;
+    cmd->cmnd[2] = (lba >> 8) & 0xff;
+    cmd->cmnd[3] = lba & 0xff;
+    cmd->cmnd[4] = nr_blocks;

Since you have made this a separate function, observing the principle of least
surprise, you may want to return an error if nr_blocks==0 since in that case
the code as it stands will read or write 256 blocks!

Hi Doug,

Next to returning an error if nr_blocks == 0, how about triggering a kernel warning if nr_blocks == 0 is passed to this function? If nr_blocks == 0 there is something wrong. I don't think that any code in the kernel submits REQ_OP_READ or REQ_OP_WRITE requests with length zero.

Yes a WARN is fine, but you want to stop the operation, especially if it
happens to be a WRITE(6). Some other code may re-use this function in
the future.




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux