[Stgt-devel] [PATCH service_actions 1/1] RESEND Add infrastructure to handle cdb lengths and service_actions
FUJITA Tomonori
fujita.tomonori
Tue May 13 14:36:56 CEST 2008
On Sun, 11 May 2008 17:00:29 +1000
"ronnie sahlberg" <ronniesahlberg at gmail.com> wrote:
> Resend the patch.
> MMC and OSD does not implement MaintenanceIn so dont define them for
> those two emulation targets.
Thanks,
> List
>
> This patch is also attached in gzip format, if gmail once again
> decides to mangle it.
Can you attach a patch in plain text format next time?
> It adds infrastructure to handle commands that take service actions.
> It also adds the cdb length to all commands we have implemented and
> list in the .ops structure.
CDB_SIZE macro doesn't work?
> (at least when I try the ioctl() to do scsi passthrough, the devices
> I have gets really "unhappy" if i dont provide the correct/expected
> cdb length so this sould also benefit when passthrough is added)
>
> As an example of a "service action" command being implemented
> I implemented 0xa3/0x0c which is ReportSupportedOperationCodes
> other MaintenanceIn commands should be easy to add.
>
> please review/apply/discuss
>
> ronnie sahlberg
>
>
> >From a9b06135ac3805daa136bc39355b8a49759de6d8 Mon Sep 17 00:00:00 2001
> From: Ronnie Sahlberg <ronniesahlberg at gmail.com>
> Date: Sun, 11 May 2008 16:25:57 +1000
> Subject: [PATCH] Add infrastructure for MaintenanceIn and other
> commands taking service actions.
>
> Expand device_type_operations to also include the cdb length and an
> array of possible service actions for this command.
> We need both the list of service actions and also the cdb length in
> order to implement
> ReportSupportedOpcodes : ManagementIn:service action 0x0c
>
> Augment the .ops structure and provide the expected cdb length for all
> command implemented for all device types. This is required to support
> ReportSupportedOperationCodes as well as might be useful if/when we add
> a passthrough backend since at least the current backend becomes unhappy
> if the cdb length provided to the ioctl() is different from what the
> real device expects.
>
> Augment the .ops structure with a null terminated list of service action for
> the (now only 0xa3) opcodes that take service actions and which needs to
> be reported in ReportSupportedOpcodes.
>
> Implement MaintenanceIn and implement service action 0x0c for this command.
> the provided infrastructure should make it easier to add other MaintIn
> service actions, of which there are quite a few.
>
> there are many other opcodes that also take service actions and which
> could build on this framework:
> 0xa4 MaintOut
> 0x7f: variable length cdb
> 0x5e/0x5f persistent reservarion in/out
> 0xab
> 0x9e
> 0x9f
>
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg at gmail.com>
> ---
> usr/mmc.c | 52 +++++++++---------
> usr/osd.c | 10 ++--
> usr/sbc.c | 44 ++++++++--------
> usr/scc.c | 16 +++---
> usr/smc.c | 20 ++++----
> usr/spc.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> usr/spc.h | 4 ++
> usr/tgtd.h | 7 +++
> 8 files changed, 253 insertions(+), 71 deletions(-)
(snip)
Here are some minor comments,
> +static int report_opcodes_all(struct scsi_cmd *cmd, int rctd,
> +uint32_t alloc_len)
> +{
> + uint8_t buf[2048], *data;
> + struct device_type_operations *ops;
> + struct service_action *service_action;
> + int i;
> + uint32_t len;
> +
> + /* cant request RCTD for all descriptors */
> + if (rctd) {
> + scsi_set_in_resid_by_actual(cmd, 0);
> + sense_data_build(cmd, ILLEGAL_REQUEST,
> + ASC_INVALID_FIELD_IN_CDB);
> + return SAM_STAT_CHECK_CONDITION;
> + }
> +
> + memset(buf, 0, sizeof(buf));
> + data = &buf[4];
> +
> + ops = cmd->dev->dev_type_template.ops;
> + for (i = 0; i < 256; i++) {
> + if (ops[i].cdb_length == 0)
> + continue;
> +
> + /* this command does not take a service action, so just
> + report the opcode
> + */
> + if (ops[i].service_actions == NULL) {
Please use
if (!ops[i].service_actions)
It's Linux kernel coding style.
> + *data++ = i;
> +
> + /* reserved */
> + data++;
> +
> + /* service action */
> + data += 2;
> +
> + /* reserved */
> + data++;
> +
> + /* flags : no service action, no command descriptor */
> + data++;
> +
> + /* cdb length */
> + *data++ = (ops[i].cdb_length>>8) & 0xff;
Please put spaces around ">>":
*data++ = (ops[i].cdb_length >> 8) & 0xff;
> + *data++ = ops[i].cdb_length & 0xff;
> +
> + continue;
> + }
> +
> + for (service_action = ops[i].service_actions;
> + service_action->cmd_perform;
> + service_action++) {
> + /* opcode */
> + *data++ = i;
> +
> + /* reserved */
> + data++;
> +
> + /* service action */
> + *data++ = (service_action->service_action>>8) & 0xff;
> + *data++ = service_action->service_action & 0xff;
> +
> + /* reserved */
> + data++;
> +
> + /* flags : service action */
> + *data++ = 0x01;
> +
> + /* cdb length */
> + *data++ = (ops[i].cdb_length>>8) & 0xff;
> + *data++ = ops[i].cdb_length & 0xff;
> + }
> + }
> +
> + len = data - &buf[0];
> + len -= 4;
> + buf[0] = (len>>24) & 0xff;
> + buf[1] = (len>>16) & 0xff;
> + buf[2] = (len>>8) & 0xff;
> + buf[3] = len & 0xff;
> +
> + memcpy(scsi_get_in_buffer(cmd), buf,
> + min(scsi_get_in_length(cmd), len+4));
> +
> + scsi_set_in_resid_by_actual(cmd, len+4);
> +
> + return SAM_STAT_GOOD;
> +}
> +
> +int spc_report_supported_opcodes(int host_no, struct scsi_cmd *cmd)
> +{
> + uint8_t reporting_options;
> + uint8_t requested_opcode;
> + uint16_t requested_service_action;
> + uint32_t alloc_len;
> + int rctd;
> + int ret = SAM_STAT_GOOD;
> +
> + reporting_options = cmd->scb[2] & 0x07;
> +
> + requested_opcode = cmd->scb[3];
> +
> + requested_service_action = cmd->scb[4];
> + requested_service_action <<= 8;
> + requested_service_action |= cmd->scb[5];
> +
> + alloc_len = cmd->scb[6];
> + alloc_len <<= 8;
> + alloc_len |= cmd->scb[7];
> + alloc_len <<= 8;
> + alloc_len |= cmd->scb[8];
> + alloc_len <<= 8;
> + alloc_len |= cmd->scb[9];
Let's do this in one line:
alloc_len = (uint32_t)scb[6] << 24 | (uint32_t)scb[7] << 16 |
(uint32_t)scb[8] << 8 | (uint32_t)scb[9];
We have the same code in scsi_rw_offset and scsi_rw_count so adding
some helper functions to util.h would be nice.
And we need to check scsi_get_in_length(cmd) here?
> + rctd = cmd->scb[2] & 0x80;
> +
> + switch (reporting_options) {
> + case 0x00: /* report all */
> + ret = report_opcodes_all(cmd, rctd, alloc_len);
> + break;
> + case 0x01: /* report one no service action*/
> + case 0x02: /* report one service action */
> + default:
> + scsi_set_in_resid_by_actual(cmd, 0);
> + sense_data_build(cmd, ILLEGAL_REQUEST,
> + ASC_INVALID_FIELD_IN_CDB);
> + ret = SAM_STAT_CHECK_CONDITION;
> + }
> +
> + return ret;
> +}
> +
> +struct service_action maint_in_service_actions[] = {
> + {0x0c, spc_report_supported_opcodes},
> + {0, NULL}
> +};
> +
> +struct service_action *
> +find_service_action(struct service_action *service_action, uint32_t action)
> +{
> + while (service_action->cmd_perform) {
> + if (service_action->service_action == action)
> + return service_action;
> + service_action++;
> + }
> + return NULL;
> +}
> +
> +/**
> + * This functions emulates the various commands using the 0xa3 cdb opcode
> + */
> +int spc_maint_in(int host_no, struct scsi_cmd *cmd)
> +{
> + uint8_t action;
> + struct service_action *service_action;
> +
> + action = cmd->scb[1] & 0x1f;
> + service_action = find_service_action(maint_in_service_actions, action);
> +
> + if (service_action == NULL) {
> + scsi_set_in_resid_by_actual(cmd, 0);
> + sense_data_build(cmd, ILLEGAL_REQUEST,
> + ASC_INVALID_FIELD_IN_CDB);
> + return SAM_STAT_CHECK_CONDITION;
> + }
> +
> + return service_action->cmd_perform(host_no, cmd);
> +}
> +
> int spc_request_sense(int host_no, struct scsi_cmd *cmd)
> {
> scsi_set_in_resid_by_actual(cmd, 0);
> diff --git a/usr/spc.h b/usr/spc.h
> index a63436a..9147d4c 100644
> --- a/usr/spc.h
> +++ b/usr/spc.h
> @@ -1,6 +1,9 @@
> #ifndef __SPC_H
> #define __SPC_H
>
> +extern struct service_action maint_in_service_actions[];
> +extern int spc_maint_in(int host_no, struct scsi_cmd *cmd);
> +
> extern int spc_inquiry(int host_no, struct scsi_cmd *cmd);
> extern int spc_report_luns(int host_no, struct scsi_cmd *cmd);
> extern int spc_start_stop(int host_no, struct scsi_cmd *cmd);
> @@ -9,6 +12,7 @@ extern int spc_request_sense(int host_no, struct
> scsi_cmd *cmd);
> extern int spc_illegal_op(int host_no, struct scsi_cmd *cmd);
> extern int spc_lu_init(struct scsi_lu *lu);
>
> +
> typedef int (match_fn_t)(struct scsi_lu *lu, char *params);
> extern int lu_config(struct scsi_lu *lu, char *params, match_fn_t *);
> extern int spc_lu_config(struct scsi_lu *lu, char *params);
> diff --git a/usr/tgtd.h b/usr/tgtd.h
> index e516b0a..caff8d0 100644
> --- a/usr/tgtd.h
> +++ b/usr/tgtd.h
> @@ -80,8 +80,15 @@ struct it_nexus_lu_info {
> struct list_head pending_ua_sense_list;
> };
>
> +struct service_action {
> + uint32_t service_action;
> + int (*cmd_perform)(int host_no, struct scsi_cmd *cmd);
> +};
> +
> struct device_type_operations {
> int (*cmd_perform)(int host_no, struct scsi_cmd *cmd);
> + int cdb_length;
> + struct service_action *service_actions;
> };
>
> struct device_type_template {
> --
> 1.5.5
> _______________________________________________
> Stgt-devel mailing list
> Stgt-devel at lists.berlios.de
> https://lists.berlios.de/mailman/listinfo/stgt-devel
More information about the stgt
mailing list