On Fri, 8 Oct 2021 20:58:17 -0700 Bart Van Assche <bvanassche@xxxxxxx> wrote: > On 9/29/21 20:47, Yanling Song wrote: > > +#define dev_log_dbg(dev, fmt, ...) do { \ > > + if (unlikely(log_debug_switch)) \ > > + dev_info(dev, "[%s] [%d] " fmt, \ > > + __func__, __LINE__, ##__VA_ARGS__); > > \ +} while (0) > > Please use pr_debug() instead of introducing dev_log_dbg(). Ok. Will use pr_debug in the next version. > > > +static inline > > +struct spraid_admin_request *spraid_admin_req(struct request *req) > > +{ > > + return blk_mq_rq_to_pdu(req); > > +} > > Please read the section with the title "The inline disease" in > Documentation/process/coding-style.rst. Ok. Will use MACRO to replace inline in the next version. > > > +static inline bool spraid_is_rw_scmd(struct scsi_cmnd *scmd) > > +{ > > + switch (scmd->cmnd[0]) { > > + case READ_6: > > + case READ_10: > > + case READ_12: > > + case READ_16: > > + case READ_32: > > + case WRITE_6: > > + case WRITE_10: > > + case WRITE_12: > > + case WRITE_16: > > + case WRITE_32: > > + return true; > > + default: > > + return false; > > + } > > +} > > Please use op_is_write() instead of reimplementing it. op_is_write() does not meet our requirement: Both read and write commands have to be checked, not just write command. > > > + if (scmd->sc_data_direction == DMA_TO_DEVICE) { > > + rw->opcode = SPRAID_CMD_WRITE; > > + } else if (scmd->sc_data_direction == DMA_FROM_DEVICE) { > > + rw->opcode = SPRAID_CMD_READ; > > + } else { > > + dev_err(hdev->dev, "Invalid IO for unsupported > > data direction: %d\n", > > + scmd->sc_data_direction); > > + WARN_ON(1); > > + } > > + > > + /* 6-byte READ(0x08) or WRITE(0x0A) cdb */ > > + if (scmd->cmd_len == 6) { > > + datalength = (u32)(scmd->cmnd[4] == 0 ? > > + IO_6_DEFAULT_TX_LEN : > > scmd->cmnd[4]); > > + start_lba_lo = ((u32)scmd->cmnd[1] << 16) | > > + ((u32)scmd->cmnd[2] << 8) | > > (u32)scmd->cmnd[3]; + > > + start_lba_lo &= 0x1FFFFF; > > + } > > + > > + /* 10-byte READ(0x28) or WRITE(0x2A) cdb */ > > + else if (scmd->cmd_len == 10) { > > + datalength = (u32)scmd->cmnd[8] | > > ((u32)scmd->cmnd[7] << 8); > > + start_lba_lo = ((u32)scmd->cmnd[2] << 24) | > > + ((u32)scmd->cmnd[3] << 16) | > > + ((u32)scmd->cmnd[4] << 8) | > > (u32)scmd->cmnd[5]; + > > + if (scmd->cmnd[1] & FUA_MASK) > > + control |= SPRAID_RW_FUA; > > + } > > + > > + /* 12-byte READ(0xA8) or WRITE(0xAA) cdb */ > > + else if (scmd->cmd_len == 12) { > > + datalength = ((u32)scmd->cmnd[6] << 24) | > > + ((u32)scmd->cmnd[7] << 16) | > > + ((u32)scmd->cmnd[8] << 8) | > > (u32)scmd->cmnd[9]; > > + start_lba_lo = ((u32)scmd->cmnd[2] << 24) | > > + ((u32)scmd->cmnd[3] << 16) | > > + ((u32)scmd->cmnd[4] << 8) | > > (u32)scmd->cmnd[5]; + > > + if (scmd->cmnd[1] & FUA_MASK) > > + control |= SPRAID_RW_FUA; > > + } > > + /* 16-byte READ(0x88) or WRITE(0x8A) cdb */ > > + else if (scmd->cmd_len == 16) { > > + datalength = ((u32)scmd->cmnd[10] << 24) | > > + ((u32)scmd->cmnd[11] << 16) | > > + ((u32)scmd->cmnd[12] << 8) | > > (u32)scmd->cmnd[13]; > > + start_lba_lo = ((u32)scmd->cmnd[6] << 24) | > > + ((u32)scmd->cmnd[7] << 16) | > > + ((u32)scmd->cmnd[8] << 8) | > > (u32)scmd->cmnd[9]; > > + start_lba_hi = ((u32)scmd->cmnd[2] << 24) | > > + ((u32)scmd->cmnd[3] << 16) | > > + ((u32)scmd->cmnd[4] << 8) | > > (u32)scmd->cmnd[5]; + > > + if (scmd->cmnd[1] & FUA_MASK) > > + control |= SPRAID_RW_FUA; > > + } > > + /* 32-byte READ(0x88) or WRITE(0x8A) cdb */ > > + else if (scmd->cmd_len == 32) { > > + datalength = ((u32)scmd->cmnd[28] << 24) | > > + ((u32)scmd->cmnd[29] << 16) | > > + ((u32)scmd->cmnd[30] << 8) | > > (u32)scmd->cmnd[31]; > > + start_lba_lo = ((u32)scmd->cmnd[16] << 24) | > > + ((u32)scmd->cmnd[17] << 16) | > > + ((u32)scmd->cmnd[18] << 8) | > > (u32)scmd->cmnd[19]; > > + start_lba_hi = ((u32)scmd->cmnd[12] << 24) | > > + ((u32)scmd->cmnd[13] << 16) | > > + ((u32)scmd->cmnd[14] << 8) | > > (u32)scmd->cmnd[15]; + > > + if (scmd->cmnd[10] & FUA_MASK) > > + control |= SPRAID_RW_FUA; > > + } > > Please remove all of the above code and use blk_rq_pos(), > blk_rq_sectors() and rq->cmd_flags & REQ_FUA instead. I did not quite get your point. The above is commonly used in many similar use cases. For example: megasas_build_ldio() in megaraid_sas_base.c. What's the benefit to switch to another way: use blk_rq_pos(),blk_rq_sectors()? > > > + spraid_wq = alloc_workqueue("spraid-wq", WQ_UNBOUND | > > WQ_MEM_RECLAIM | WQ_SYSFS, 0); > > + if (!spraid_wq) > > + return -ENOMEM; > > Why does this driver create a workqueue? Why is system_wq not good > enough? In my opinion, there is not much difference by using system_wq or using a dedicated wq to execute a work. but the dedicated wq is must to execute some serial works. It is easy to add more serial works later if using a dedicated wq here. > > Thanks, > > Bart.