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.
