Re: [PATCH] spraid: initial commit of Ramaxel spraid driver

[Bart Van Assche <bvanassche@xxxxxxx>]

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().

> +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.

> +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.

> +	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.

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

Thanks,

Bart.