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

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

On Thu, Aug 11, 2022 at 10:43:05PM -0500, Mike Christie wrote:
> «Внимание! Данное письмо от внешнего адресата!»
> 
> 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.
Looks like emulate_tpws was introduced exaclty for WS+UNMAP bit case
and it can not be used in tcm_is_ws_enabled as only check. Because of
WS is actually two different commands selected by UNMAP bit it is
unable somehow to differentiate them in RSOC. So I will reformulate
the check in tcm_is_ws_enabled to be true if some of cases is
supported by the backstore device.
+	return (dev->dev_attrib.emulate_tpws && !!ops->execute_unmap) ||
+	       !!ops->execute_write_same;

> 
> >  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.
yep, will fix