Re: [PATCH 3/6] scsi: target: core: dynamic opcode support in RSOC

Mike Christie
2022-08-12T03:43:15Z

On 7/18/22 7:01 AM, Dmitry Bogdanov wrote:
> Report supported opcodes depending on a dynamic device configuration
>
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> ---
> drivers/target/target_core_spc.c | 118 ++++++++++++++++++++++++++++--
> include/target/target_core_base.h | 1 +
> 2 files changed, 114 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index 506e28b14e5a..cf516136b933 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -1424,6 +1424,13 @@ static struct target_opcode_descriptor tcm_opcode_xdwriteread32 = {
> 0xff, 0xff, 0xff, 0xff},
> };
>
> +static bool tcm_is_ws_enabled(struct se_cmd *cmd)
> +{
> + struct se_device *dev = cmd->se_dev;
> +
> + return dev->dev_attrib.emulate_tpws;
> +}
> +
> static struct target_opcode_descriptor tcm_opcode_write_same32 = {
> .support = SCSI_SUPPORT_FULL,
> .serv_action_valid = 1,
> @@ -1438,8 +1445,16 @@ static struct target_opcode_descriptor tcm_opcode_write_same32 = {
> 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00,
> 0xff, 0xff, 0xff, 0xff},
> + .enabled = tcm_is_ws_enabled,
> };

I'm not sure what's incorrect. I think your patch is correct but the write
same code is wrong.

If emulate_tpws is 0, we will still execute the command. We actually only fail
with TCM_UNSUPPORTED_SCSI_OPCODE if it's a WRITE_SAME with the UNMAP bit = 1
and emulate_tpws=0.

If it's just a normal WRITE_SAME we maybe go by if by max_write_same_len is
greater than zero? Maybe that was a mistake and sbc_setup_write_same needs
a emulate_tpws check.

> static struct target_opcode_descriptor tcm_opcode_sync_cache = {
> @@ -1502,6 +1533,14 @@ static struct target_opcode_descriptor tcm_opcode_sync_cache16 = {
> 0xff, 0xff, SCSI_GROUP_NUMBER_MASK, SCSI_CONTROL_MASK},
> };
>
> +static bool tcm_is_unmap_enabled(struct se_cmd *cmd)
> +{
> + struct sbc_ops *ops = cmd->protocol_data;
> + struct se_device *dev = cmd->se_dev;
> +
> + return ops->execute_unmap && dev->dev_attrib.emulate_tpu;
> +}

Just a trivial nit. You had an extra space there.