* [PATCH 0/6] USB-gadget: mass-storage: Support DVD-like images.
@ 2021-06-26 21:18 Igor Kononenko
2021-06-26 21:18 ` [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function Igor Kononenko
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Igor Kononenko @ 2021-06-26 21:18 UTC (permalink / raw)
Cc: openbmc, Igor Kononenko
The following patches contain several fixes of implementation
'usb-gadget:mass-storage' and bring a support DVD/BR media-types.
The type of MMC device will be calculating relevant to backend-file
image size.
Igor Kononenko (6):
usb:gadget:mass-storage: Improve the signature of SCSI handler
function
usb:gadget:mass-storage: refactoring the SCSI command handling
fms: Add TOC/PMA/ATIP DVD-ROM capabilities
fms: Support the DVD/BD images size over 2.1Gb
FMS: Add the SCSI Get Configuration command.
FMS: Add SCSI Read Disc Information command.
drivers/ata/libata-zpodd.c | 8 +-
drivers/usb/gadget/function/f_mass_storage.c | 942 ++++++++++----
drivers/usb/gadget/function/storage_common.c | 19 +-
drivers/usb/gadget/function/storage_common.h | 5 +
include/scsi/scsi_proto.h | 2 +
include/uapi/linux/cdrom.h | 1216 +++++++++++++++++-
6 files changed, 1854 insertions(+), 338 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function
2021-06-26 21:18 [PATCH 0/6] USB-gadget: mass-storage: Support DVD-like images Igor Kononenko
@ 2021-06-26 21:18 ` Igor Kononenko
2021-06-27 14:18 ` Alan Stern
2021-06-26 21:18 ` [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling Igor Kononenko
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Igor Kononenko @ 2021-06-26 21:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman
Cc: openbmc, linux-usb, Igor Kononenko, linux-kernel
SCSI command handlers currently have an ambiguous return value. This
return value may indicate the length of the data written to the response
buffer and the command's processing status. Thus, the understanding of
command handling may be implicit.
After this patch, the output buffer's size will be set in the
'data_size_to_handle' field of 'struct fsg_common', and the command
handler's return value indicates only the processing status.
Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN
BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the
USBGadget MassStorage debug print showed all sent commands works
properly.
Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
---
drivers/usb/gadget/function/f_mass_storage.c | 176 ++++++++++---------
1 file changed, 95 insertions(+), 81 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 4a4703634a2a..e9a7f87b4df3 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -296,6 +296,7 @@ struct fsg_common {
enum data_direction data_dir;
u32 data_size;
u32 data_size_from_cmnd;
+ u32 data_size_to_handle;
u32 tag;
u32 residue;
u32 usb_amount_left;
@@ -1066,7 +1067,8 @@ static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)
memset(buf, 0, 36);
buf[0] = TYPE_NO_LUN; /* Unsupported, no device-type */
buf[4] = 31; /* Additional length */
- return 36;
+ common->data_size_to_handle = 36;
+ return 0;
}
buf[0] = curlun->cdrom ? TYPE_ROM : TYPE_DISK;
@@ -1083,7 +1085,8 @@ static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)
else
memcpy(buf + 8, common->inquiry_string,
sizeof(common->inquiry_string));
- return 36;
+ common->data_size_to_handle = 36;
+ return 0;
}
static int do_request_sense(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1136,7 +1139,8 @@ static int do_request_sense(struct fsg_common *common, struct fsg_buffhd *bh)
buf[7] = 18 - 8; /* Additional sense length */
buf[12] = ASC(sd);
buf[13] = ASCQ(sd);
- return 18;
+ common->data_size_to_handle = 18;
+ return 0;
}
static int do_read_capacity(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1155,7 +1159,8 @@ static int do_read_capacity(struct fsg_common *common, struct fsg_buffhd *bh)
put_unaligned_be32(curlun->num_sectors - 1, &buf[0]);
/* Max logical block */
put_unaligned_be32(curlun->blksize, &buf[4]);/* Block length */
- return 8;
+ common->data_size_to_handle = 8;
+ return 0;
}
static int do_read_header(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1177,7 +1182,8 @@ static int do_read_header(struct fsg_common *common, struct fsg_buffhd *bh)
memset(buf, 0, 8);
buf[0] = 0x01; /* 2048 bytes of user data, rest is EC */
store_cdrom_address(&buf[4], msf, lba);
- return 8;
+ common->data_size_to_handle = 8;
+ return 0;
}
static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1204,7 +1210,8 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
buf[13] = 0x16; /* Lead-out track is data */
buf[14] = 0xAA; /* Lead-out track number */
store_cdrom_address(&buf[16], msf, curlun->num_sectors);
- return 20;
+ common->data_size_to_handle = 20;
+ return 0;
}
static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1290,7 +1297,8 @@ static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
buf0[0] = len - 1;
else
put_unaligned_be16(len - 2, buf0);
- return len;
+ common->data_size_to_handle = len;
+ return 0;
}
static int do_start_stop(struct fsg_common *common)
@@ -1381,7 +1389,8 @@ static int do_read_format_capacities(struct fsg_common *common,
/* Number of blocks */
put_unaligned_be32(curlun->blksize, &buf[4]);/* Block length */
buf[4] = 0x02; /* Current capacity */
- return 12;
+ common->data_size_to_handle = 12;
+ return 0;
}
static int do_mode_select(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1796,7 +1805,7 @@ static int do_scsi_command(struct fsg_common *common)
{
struct fsg_buffhd *bh;
int rc;
- int reply = -EINVAL;
+ int status = -EINVAL;
int i;
static char unknown[16];
@@ -1813,104 +1822,107 @@ static int do_scsi_command(struct fsg_common *common)
common->short_packet_received = 0;
down_read(&common->filesem); /* We're using the backing file */
+ /* flash all unhandled data */
+ common->data_size_to_handle = 0;
+
switch (common->cmnd[0]) {
case INQUIRY:
common->data_size_from_cmnd = common->cmnd[4];
- reply = check_command(common, 6, DATA_DIR_TO_HOST,
+ status = check_command(common, 6, DATA_DIR_TO_HOST,
(1<<4), 0,
"INQUIRY");
- if (reply == 0)
- reply = do_inquiry(common, bh);
+ if (status == 0)
+ status = do_inquiry(common, bh);
break;
case MODE_SELECT:
common->data_size_from_cmnd = common->cmnd[4];
- reply = check_command(common, 6, DATA_DIR_FROM_HOST,
+ status = check_command(common, 6, DATA_DIR_FROM_HOST,
(1<<1) | (1<<4), 0,
"MODE SELECT(6)");
- if (reply == 0)
- reply = do_mode_select(common, bh);
+ if (status == 0)
+ status = do_mode_select(common, bh);
break;
case MODE_SELECT_10:
common->data_size_from_cmnd =
get_unaligned_be16(&common->cmnd[7]);
- reply = check_command(common, 10, DATA_DIR_FROM_HOST,
+ status = check_command(common, 10, DATA_DIR_FROM_HOST,
(1<<1) | (3<<7), 0,
"MODE SELECT(10)");
- if (reply == 0)
- reply = do_mode_select(common, bh);
+ if (status == 0)
+ status = do_mode_select(common, bh);
break;
case MODE_SENSE:
common->data_size_from_cmnd = common->cmnd[4];
- reply = check_command(common, 6, DATA_DIR_TO_HOST,
+ status = check_command(common, 6, DATA_DIR_TO_HOST,
(1<<1) | (1<<2) | (1<<4), 0,
"MODE SENSE(6)");
- if (reply == 0)
- reply = do_mode_sense(common, bh);
+ if (status == 0)
+ status = do_mode_sense(common, bh);
break;
case MODE_SENSE_10:
common->data_size_from_cmnd =
get_unaligned_be16(&common->cmnd[7]);
- reply = check_command(common, 10, DATA_DIR_TO_HOST,
+ status = check_command(common, 10, DATA_DIR_TO_HOST,
(1<<1) | (1<<2) | (3<<7), 0,
"MODE SENSE(10)");
- if (reply == 0)
- reply = do_mode_sense(common, bh);
+ if (status == 0)
+ status = do_mode_sense(common, bh);
break;
case ALLOW_MEDIUM_REMOVAL:
common->data_size_from_cmnd = 0;
- reply = check_command(common, 6, DATA_DIR_NONE,
+ status = check_command(common, 6, DATA_DIR_NONE,
(1<<4), 0,
"PREVENT-ALLOW MEDIUM REMOVAL");
- if (reply == 0)
- reply = do_prevent_allow(common);
+ if (status == 0)
+ status = do_prevent_allow(common);
break;
case READ_6:
i = common->cmnd[4];
common->data_size_from_cmnd = (i == 0) ? 256 : i;
- reply = check_command_size_in_blocks(common, 6,
+ status = check_command_size_in_blocks(common, 6,
DATA_DIR_TO_HOST,
(7<<1) | (1<<4), 1,
"READ(6)");
- if (reply == 0)
- reply = do_read(common);
+ if (status == 0)
+ status = do_read(common);
break;
case READ_10:
common->data_size_from_cmnd =
get_unaligned_be16(&common->cmnd[7]);
- reply = check_command_size_in_blocks(common, 10,
+ status = check_command_size_in_blocks(common, 10,
DATA_DIR_TO_HOST,
(1<<1) | (0xf<<2) | (3<<7), 1,
"READ(10)");
- if (reply == 0)
- reply = do_read(common);
+ if (status == 0)
+ status = do_read(common);
break;
case READ_12:
common->data_size_from_cmnd =
get_unaligned_be32(&common->cmnd[6]);
- reply = check_command_size_in_blocks(common, 12,
+ status = check_command_size_in_blocks(common, 12,
DATA_DIR_TO_HOST,
(1<<1) | (0xf<<2) | (0xf<<6), 1,
"READ(12)");
- if (reply == 0)
- reply = do_read(common);
+ if (status == 0)
+ status = do_read(common);
break;
case READ_CAPACITY:
common->data_size_from_cmnd = 8;
- reply = check_command(common, 10, DATA_DIR_TO_HOST,
+ status = check_command(common, 10, DATA_DIR_TO_HOST,
(0xf<<2) | (1<<8), 1,
"READ CAPACITY");
- if (reply == 0)
- reply = do_read_capacity(common, bh);
+ if (status == 0)
+ status = do_read_capacity(common, bh);
break;
case READ_HEADER:
@@ -1918,11 +1930,11 @@ static int do_scsi_command(struct fsg_common *common)
goto unknown_cmnd;
common->data_size_from_cmnd =
get_unaligned_be16(&common->cmnd[7]);
- reply = check_command(common, 10, DATA_DIR_TO_HOST,
+ status = check_command(common, 10, DATA_DIR_TO_HOST,
(3<<7) | (0x1f<<1), 1,
"READ HEADER");
- if (reply == 0)
- reply = do_read_header(common, bh);
+ if (status == 0)
+ status = do_read_header(common, bh);
break;
case READ_TOC:
@@ -1930,53 +1942,53 @@ static int do_scsi_command(struct fsg_common *common)
goto unknown_cmnd;
common->data_size_from_cmnd =
get_unaligned_be16(&common->cmnd[7]);
- reply = check_command(common, 10, DATA_DIR_TO_HOST,
+ status = check_command(common, 10, DATA_DIR_TO_HOST,
(7<<6) | (1<<1), 1,
"READ TOC");
- if (reply == 0)
- reply = do_read_toc(common, bh);
+ if (status == 0)
+ status = do_read_toc(common, bh);
break;
case READ_FORMAT_CAPACITIES:
common->data_size_from_cmnd =
get_unaligned_be16(&common->cmnd[7]);
- reply = check_command(common, 10, DATA_DIR_TO_HOST,
+ status = check_command(common, 10, DATA_DIR_TO_HOST,
(3<<7), 1,
"READ FORMAT CAPACITIES");
- if (reply == 0)
- reply = do_read_format_capacities(common, bh);
+ if (status == 0)
+ status = do_read_format_capacities(common, bh);
break;
case REQUEST_SENSE:
common->data_size_from_cmnd = common->cmnd[4];
- reply = check_command(common, 6, DATA_DIR_TO_HOST,
+ status = check_command(common, 6, DATA_DIR_TO_HOST,
(1<<4), 0,
"REQUEST SENSE");
- if (reply == 0)
- reply = do_request_sense(common, bh);
+ if (status == 0)
+ status = do_request_sense(common, bh);
break;
case START_STOP:
common->data_size_from_cmnd = 0;
- reply = check_command(common, 6, DATA_DIR_NONE,
+ status = check_command(common, 6, DATA_DIR_NONE,
(1<<1) | (1<<4), 0,
"START-STOP UNIT");
- if (reply == 0)
- reply = do_start_stop(common);
+ if (status == 0)
+ status = do_start_stop(common);
break;
case SYNCHRONIZE_CACHE:
common->data_size_from_cmnd = 0;
- reply = check_command(common, 10, DATA_DIR_NONE,
+ status = check_command(common, 10, DATA_DIR_NONE,
(0xf<<2) | (3<<7), 1,
"SYNCHRONIZE CACHE");
- if (reply == 0)
- reply = do_synchronize_cache(common);
+ if (status == 0)
+ status = do_synchronize_cache(common);
break;
case TEST_UNIT_READY:
common->data_size_from_cmnd = 0;
- reply = check_command(common, 6, DATA_DIR_NONE,
+ status = check_command(common, 6, DATA_DIR_NONE,
0, 1,
"TEST UNIT READY");
break;
@@ -1987,44 +1999,44 @@ static int do_scsi_command(struct fsg_common *common)
*/
case VERIFY:
common->data_size_from_cmnd = 0;
- reply = check_command(common, 10, DATA_DIR_NONE,
+ status = check_command(common, 10, DATA_DIR_NONE,
(1<<1) | (0xf<<2) | (3<<7), 1,
"VERIFY");
- if (reply == 0)
- reply = do_verify(common);
+ if (status == 0)
+ status = do_verify(common);
break;
case WRITE_6:
i = common->cmnd[4];
common->data_size_from_cmnd = (i == 0) ? 256 : i;
- reply = check_command_size_in_blocks(common, 6,
+ status = check_command_size_in_blocks(common, 6,
DATA_DIR_FROM_HOST,
(7<<1) | (1<<4), 1,
"WRITE(6)");
- if (reply == 0)
- reply = do_write(common);
+ if (status == 0)
+ status = do_write(common);
break;
case WRITE_10:
common->data_size_from_cmnd =
get_unaligned_be16(&common->cmnd[7]);
- reply = check_command_size_in_blocks(common, 10,
+ status = check_command_size_in_blocks(common, 10,
DATA_DIR_FROM_HOST,
(1<<1) | (0xf<<2) | (3<<7), 1,
"WRITE(10)");
- if (reply == 0)
- reply = do_write(common);
+ if (status == 0)
+ status = do_write(common);
break;
case WRITE_12:
common->data_size_from_cmnd =
get_unaligned_be32(&common->cmnd[6]);
- reply = check_command_size_in_blocks(common, 12,
+ status = check_command_size_in_blocks(common, 12,
DATA_DIR_FROM_HOST,
(1<<1) | (0xf<<2) | (0xf<<6), 1,
"WRITE(12)");
- if (reply == 0)
- reply = do_write(common);
+ if (status == 0)
+ status = do_write(common);
break;
/*
@@ -2042,27 +2054,29 @@ static int do_scsi_command(struct fsg_common *common)
unknown_cmnd:
common->data_size_from_cmnd = 0;
sprintf(unknown, "Unknown x%02x", common->cmnd[0]);
- reply = check_command(common, common->cmnd_size,
+ status = check_command(common, common->cmnd_size,
DATA_DIR_UNKNOWN, ~0, 0, unknown);
- if (reply == 0) {
+ if (status == 0) {
common->curlun->sense_data = SS_INVALID_COMMAND;
- reply = -EINVAL;
+ status = -EINVAL;
}
break;
}
up_read(&common->filesem);
- if (reply == -EINTR || signal_pending(current))
+ if (status == -EINTR || signal_pending(current))
return -EINTR;
- /* Set up the single reply buffer for finish_reply() */
- if (reply == -EINVAL)
- reply = 0; /* Error reply length */
- if (reply >= 0 && common->data_dir == DATA_DIR_TO_HOST) {
- reply = min((u32)reply, common->data_size_from_cmnd);
- bh->inreq->length = reply;
+ /* Set up the single status buffer for finish_reply() */
+ if (status == -EINVAL)
+ status = 0; /* Error reply length */
+ if (status == 0 && common->data_dir == DATA_DIR_TO_HOST) {
+ common->data_size_to_handle =
+ min_t(u32, common->data_size_to_handle,
+ common->data_size_from_cmnd);
+ bh->inreq->length = common->data_size_to_handle;
bh->state = BUF_STATE_FULL;
- common->residue -= reply;
+ common->residue -= common->data_size_to_handle;
} /* Otherwise it's already set */
return 0;
--
2.32.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling
2021-06-26 21:18 [PATCH 0/6] USB-gadget: mass-storage: Support DVD-like images Igor Kononenko
2021-06-26 21:18 ` [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function Igor Kononenko
@ 2021-06-26 21:18 ` Igor Kononenko
2021-06-26 23:29 ` kernel test robot
2021-06-27 14:23 ` Alan Stern
2021-06-26 21:18 ` [PATCH 3/6] fms: Add TOC/PMA/ATIP DVD-ROM capabilities Igor Kononenko
` (3 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Igor Kononenko @ 2021-06-26 21:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman
Cc: openbmc, linux-usb, Igor Kononenko, linux-kernel
Implements a universal way to define SCSI commands and configure
precheck handlers.
Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN
BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the
USBGadget MassStorage debug print showed all sent commands works
properly.
Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
---
drivers/usb/gadget/function/f_mass_storage.c | 540 +++++++++++--------
drivers/usb/gadget/function/storage_common.h | 5 +
2 files changed, 310 insertions(+), 235 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index e9a7f87b4df3..c3fddee21b53 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -237,6 +237,137 @@ static const char fsg_string_interface[] = "Mass Storage";
#include "storage_common.h"
#include "f_mass_storage.h"
+
+/*------------------------------------------------------------------------*/
+
+/**
+ * @brief The handler of incoming CDB command
+ * @param cmd - SCSI command number
+ * @param callback - The callback of handle the incoming command
+ */
+#define CDB_REG_HANDLER(cmd, callback) \
+ .command = (cmd), .do_command = (callback), \
+ .type = CDB_HANDLER_COMMON, .name = (#cmd)
+
+/**
+ * @brief The handler of incoming CDB command
+ * @param cmd - SCSI command nubmer with fsg buffhd
+ * @param callback - The callback of handle the incoming command
+ */
+#define CDB_REG_HANDLER_BUFFHD(cmd, callback) \
+ .command = (cmd), .do_command_with_buffhd = (callback), \
+ .type = CDB_HANDLER_FSG_BUFFHD, .name = (#cmd)
+
+/**
+ * @see CDB_REG_CHECKER_DS
+ * @details Register CDB command without additional check handler.
+ */
+#define CDB_REG_NO_CHECKER(cmd, si, dir, req) \
+ .command = (cmd), .direction = (dir), .size_index = (si), \
+ .medium_required = (req), .do_check_command = NULL,
+
+/**
+ * @brief Register the CDB command checker, which checks an incoming command
+ * by specified criteria.
+ * This validator will take care of the specified data size (DS)
+ *
+ * @param cmd - SCSI command nubmer
+ * @param s - CDB command size in bytes
+ * @param si - The CDB command might have the recommended response size.
+ * This field indicates the size field index in the input CDB command
+ * buffer
+ * @param dir - Direction of data transfer of requested CDB command
+ * @param mask - Mask of relevant bytes in the input command buffer.
+ * The ordinal number of a bit in the mask indicates that a byte in the
+ * CDB command buffer might be present.
+ * If that ordinal number bit equals zero, only a zero value must be
+ * present in this original byte.
+ * @param req - Indicates that medium MUST be present or might be optional
+ * @param ds - If @param SI member is equal to @enum CDB_SIZE_MANUAL, than this
+ * field indicates the custom response buffer size
+ */
+#define CDB_REG_CHECKER_DS(cmd, s, si, dir, mask, req, ds) \
+ .command = (cmd), .size = (s), .size_index = (si), .direction = (dir), \
+ .valid_bytes_bitmask = (mask), .medium_required = (req), \
+ .data_size_manual = (ds), .do_check_command = &check_command
+
+/**
+ * @see CDB_REG_CHECKER_DS
+ * @details The data size is zero.
+ * This macro can't be used with the @enum CDB_SIZE_MANUAL
+ */
+#define CDB_REG_CHECKER(cmd, s, si, dir, mask, req) \
+ CDB_REG_CHECKER_DS(cmd, s, si, dir, mask, req, 0)
+
+/**
+ * @see CDB_REG_CHECKER_DS
+ * @details The checker which registried by this macros will validate the input
+ * data size in blocks.
+ * Block size specified by MSF interface type, in the curlun->blksize.
+ */
+#define CDB_REG_CHECKER_BLK(cmd, s, si, dir, mask, req) \
+ CDB_REG_CHECKER_DS(cmd, s, si, dir, mask, req, 0), \
+ .do_check_command = &check_command_size_in_blocks
+
+/**
+ * @brief Field index of possible data length of output buffer size, which
+ * contains in the input CDB command buffer
+ */
+enum cdb_data_size_field {
+ CDB_SIZE_MANUAL = -2,
+ CDB_NO_SIZE_FIELD = -1,
+ CDB_SIZE_FIELD_4 = 4,
+ CDB_SIZE_FIELD_6 = 6,
+ CDB_SIZE_FIELD_7 = 7,
+};
+
+/* Type of CDB command checker with associated data to check */
+struct cdb_command_check {
+ /* SCSI command number */
+ u8 command;
+ /* CDB command size */
+ size_t size;
+ /* Size field index in the input CDB command buffer */
+ enum cdb_data_size_field size_index;
+ /* CDB command data direction, @enum data_direction */
+ u8 direction;
+ /* Mask of expected meaningfull bytes in input CDB command buffer */
+ u32 valid_bytes_bitmask;
+ /* Is medium must be present or not */
+ u8 medium_required;
+ /* If data size is custom (the size_index is equal to CDB_SIZE_MANUAL),
+ * then this field indicates the output data size
+ */
+ u8 data_size_manual;
+ /* the CDB command checker */
+ int (*do_check_command)(struct fsg_common *common, int size,
+ enum data_direction direction,
+ unsigned int mask, int needs_medium,
+ const char *name);
+};
+
+/* CDB command hundler metadata */
+struct cdb_handler {
+ /* SCSI command number */
+ u8 command;
+ /**
+ * @brief the CDB command hundler
+ * @param fsg_common - FSG instance
+ */
+ int (*do_command)(struct fsg_common *common);
+ /**
+ * @brief The CDB command hundler with a fsg_buffhd specified
+ */
+ int (*do_command_with_buffhd)(struct fsg_common *common,
+ struct fsg_buffhd *bufhd);
+ /* CDB handler type to pick a relevant callback */
+ enum { CDB_HANDLER_COMMON, CDB_HANDLER_FSG_BUFFHD } type;
+ /* SCSI command ASCII name */
+ char *name;
+};
+
+/*------------------------------------------------------------------------*/
+
/* Static strings, in UTF-8 (for simplicity we use only ASCII characters) */
static struct usb_string fsg_strings[] = {
{FSG_STRING_INTERFACE, fsg_string_interface},
@@ -1801,6 +1932,97 @@ static int check_command_size_in_blocks(struct fsg_common *common,
mask, needs_medium, name);
}
+static struct cdb_command_check cdb_checker_table[] = {
+ { CDB_REG_CHECKER(INQUIRY, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
+ 0x0010, MEDIUM_OPTIONAL) },
+ { CDB_REG_CHECKER(MODE_SELECT, 6, CDB_SIZE_FIELD_4, DATA_DIR_FROM_HOST,
+ 0x0012, MEDIUM_OPTIONAL) },
+ { CDB_REG_CHECKER(MODE_SELECT_10, 10, CDB_SIZE_FIELD_7,
+ DATA_DIR_FROM_HOST, 0x0182, MEDIUM_OPTIONAL) },
+ { CDB_REG_CHECKER(MODE_SENSE, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
+ 0x0016, MEDIUM_OPTIONAL) },
+ { CDB_REG_CHECKER(MODE_SENSE_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
+ 0x186, MEDIUM_OPTIONAL) },
+ { CDB_REG_CHECKER(ALLOW_MEDIUM_REMOVAL, 6, CDB_NO_SIZE_FIELD,
+ DATA_DIR_NONE, 0x0010, MEDIUM_OPTIONAL) },
+ { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
+ 0x001E, MEDIUM_REQUIRED) },
+ { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
+ 0x01BE, MEDIUM_REQUIRED) },
+ { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
+ 0x03FE, MEDIUM_REQUIRED) },
+ { CDB_REG_CHECKER_DS(READ_CAPACITY, 10, CDB_SIZE_MANUAL,
+ DATA_DIR_TO_HOST, 0x011E, MEDIUM_REQUIRED, 8) },
+ { CDB_REG_CHECKER(READ_HEADER, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
+ 0x01BE, MEDIUM_REQUIRED) },
+ { CDB_REG_CHECKER(READ_TOC, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
+ 0x03C7, MEDIUM_REQUIRED) },
+ { CDB_REG_CHECKER(READ_FORMAT_CAPACITIES, 10, CDB_SIZE_FIELD_7,
+ DATA_DIR_TO_HOST, 0x0180, MEDIUM_REQUIRED) },
+
+ { CDB_REG_CHECKER(REQUEST_SENSE, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
+ 0x0010, MEDIUM_OPTIONAL) },
+ { CDB_REG_CHECKER(START_STOP, 6, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
+ 0x0012, MEDIUM_OPTIONAL) },
+ { CDB_REG_CHECKER(SYNCHRONIZE_CACHE, 10, CDB_NO_SIZE_FIELD,
+ DATA_DIR_NONE, 0x01BC, MEDIUM_REQUIRED) },
+
+ { CDB_REG_CHECKER(TEST_UNIT_READY, 6, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
+ 0x0000, MEDIUM_REQUIRED) },
+
+ { CDB_REG_CHECKER_BLK(VERIFY, 10, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
+ 0x0000, MEDIUM_REQUIRED) },
+ { CDB_REG_CHECKER_BLK(WRITE_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_FROM_HOST,
+ 0x001E, MEDIUM_REQUIRED) },
+ { CDB_REG_CHECKER_BLK(WRITE_10, 10, CDB_SIZE_FIELD_7,
+ DATA_DIR_FROM_HOST, 0x01BE, MEDIUM_REQUIRED) },
+ { CDB_REG_CHECKER_BLK(WRITE_12, 12, CDB_SIZE_FIELD_6,
+ DATA_DIR_FROM_HOST, 0x03FE, MEDIUM_REQUIRED) },
+};
+
+static struct cdb_handler cdb_handlers_table[] = {
+ { CDB_REG_HANDLER_BUFFHD(INQUIRY, &do_inquiry) },
+ { CDB_REG_HANDLER_BUFFHD(MODE_SELECT, &do_mode_select) },
+ { CDB_REG_HANDLER_BUFFHD(MODE_SELECT_10, &do_mode_select) },
+ { CDB_REG_HANDLER_BUFFHD(MODE_SENSE, &do_mode_sense) },
+ { CDB_REG_HANDLER_BUFFHD(MODE_SENSE_10, &do_mode_sense) },
+ { CDB_REG_HANDLER(ALLOW_MEDIUM_REMOVAL, &do_prevent_allow) },
+ { CDB_REG_HANDLER(READ_6, &do_read) },
+ { CDB_REG_HANDLER(READ_10, &do_read) },
+ { CDB_REG_HANDLER(READ_12, &do_read) },
+ { CDB_REG_HANDLER_BUFFHD(READ_CAPACITY, &do_read_capacity) },
+ { CDB_REG_HANDLER_BUFFHD(READ_HEADER, &do_read_header) },
+ { CDB_REG_HANDLER_BUFFHD(READ_TOC, &do_read_toc) },
+ { CDB_REG_HANDLER_BUFFHD(READ_FORMAT_CAPACITIES, &do_read_format_capacities) },
+
+ { CDB_REG_HANDLER_BUFFHD(REQUEST_SENSE, &do_request_sense) },
+ { CDB_REG_HANDLER(START_STOP, &do_start_stop) },
+ { CDB_REG_HANDLER(SYNCHRONIZE_CACHE, &do_synchronize_cache) },
+ { CDB_REG_HANDLER(TEST_UNIT_READY, NULL) },
+
+ /*
+ * Although optional, this command is used by MS-Windows. We
+ * support a minimal version: BytChk must be 0.
+ */
+
+ { CDB_REG_HANDLER(VERIFY, do_verify) },
+ { CDB_REG_HANDLER(WRITE_6, do_write) },
+ { CDB_REG_HANDLER(WRITE_10, do_write) },
+ { CDB_REG_HANDLER(WRITE_12, do_write) },
+ /*
+ * Some mandatory commands that we recognize but don't implement.
+ * They don't mean much in this setting. It's left as an exercise
+ * for anyone interested to implement RESERVE and RELEASE in terms
+ * of Posix locks.
+ *
+ * Commands:
+ * FORMAT_UNIT
+ * RELEASE
+ * RESERVE
+ * SEND_DIAGNOSTIC
+ */
+};
+
static int do_scsi_command(struct fsg_common *common)
{
struct fsg_buffhd *bh;
@@ -1811,6 +2033,14 @@ static int do_scsi_command(struct fsg_common *common)
dump_cdb(common);
+ /* The size of both handlers and SCSI-checkers tables must be equal */
+ if (WARN(ARRAY_SIZE(cdb_checker_table) !=
+ ARRAY_SIZE(cdb_handlers_table),
+ "The checkers and handlers tables length are not matched!\n")) {
+ pr_err("Invalid cdb handlers initialization.\n");
+ return status;
+ }
+
/* Wait for the next buffer to become available for data or status */
bh = common->next_buffhd_to_fill;
common->next_buffhd_to_drain = bh;
@@ -1825,243 +2055,84 @@ static int do_scsi_command(struct fsg_common *common)
/* flash all unhandled data */
common->data_size_to_handle = 0;
- switch (common->cmnd[0]) {
+ for (i = 0; i < ARRAY_SIZE(cdb_checker_table); ++i) {
+ const struct cdb_command_check *curr_check =
+ &cdb_checker_table[i];
+ const struct cdb_handler *curr_handler = &cdb_handlers_table[i];
- case INQUIRY:
- common->data_size_from_cmnd = common->cmnd[4];
- status = check_command(common, 6, DATA_DIR_TO_HOST,
- (1<<4), 0,
- "INQUIRY");
- if (status == 0)
- status = do_inquiry(common, bh);
- break;
-
- case MODE_SELECT:
- common->data_size_from_cmnd = common->cmnd[4];
- status = check_command(common, 6, DATA_DIR_FROM_HOST,
- (1<<1) | (1<<4), 0,
- "MODE SELECT(6)");
- if (status == 0)
- status = do_mode_select(common, bh);
- break;
-
- case MODE_SELECT_10:
- common->data_size_from_cmnd =
- get_unaligned_be16(&common->cmnd[7]);
- status = check_command(common, 10, DATA_DIR_FROM_HOST,
- (1<<1) | (3<<7), 0,
- "MODE SELECT(10)");
- if (status == 0)
- status = do_mode_select(common, bh);
- break;
-
- case MODE_SENSE:
- common->data_size_from_cmnd = common->cmnd[4];
- status = check_command(common, 6, DATA_DIR_TO_HOST,
- (1<<1) | (1<<2) | (1<<4), 0,
- "MODE SENSE(6)");
- if (status == 0)
- status = do_mode_sense(common, bh);
- break;
-
- case MODE_SENSE_10:
- common->data_size_from_cmnd =
- get_unaligned_be16(&common->cmnd[7]);
- status = check_command(common, 10, DATA_DIR_TO_HOST,
- (1<<1) | (1<<2) | (3<<7), 0,
- "MODE SENSE(10)");
- if (status == 0)
- status = do_mode_sense(common, bh);
- break;
-
- case ALLOW_MEDIUM_REMOVAL:
- common->data_size_from_cmnd = 0;
- status = check_command(common, 6, DATA_DIR_NONE,
- (1<<4), 0,
- "PREVENT-ALLOW MEDIUM REMOVAL");
- if (status == 0)
- status = do_prevent_allow(common);
- break;
-
- case READ_6:
- i = common->cmnd[4];
- common->data_size_from_cmnd = (i == 0) ? 256 : i;
- status = check_command_size_in_blocks(common, 6,
- DATA_DIR_TO_HOST,
- (7<<1) | (1<<4), 1,
- "READ(6)");
- if (status == 0)
- status = do_read(common);
- break;
-
- case READ_10:
- common->data_size_from_cmnd =
- get_unaligned_be16(&common->cmnd[7]);
- status = check_command_size_in_blocks(common, 10,
- DATA_DIR_TO_HOST,
- (1<<1) | (0xf<<2) | (3<<7), 1,
- "READ(10)");
- if (status == 0)
- status = do_read(common);
- break;
-
- case READ_12:
- common->data_size_from_cmnd =
- get_unaligned_be32(&common->cmnd[6]);
- status = check_command_size_in_blocks(common, 12,
- DATA_DIR_TO_HOST,
- (1<<1) | (0xf<<2) | (0xf<<6), 1,
- "READ(12)");
- if (status == 0)
- status = do_read(common);
- break;
-
- case READ_CAPACITY:
- common->data_size_from_cmnd = 8;
- status = check_command(common, 10, DATA_DIR_TO_HOST,
- (0xf<<2) | (1<<8), 1,
- "READ CAPACITY");
- if (status == 0)
- status = do_read_capacity(common, bh);
- break;
-
- case READ_HEADER:
- if (!common->curlun || !common->curlun->cdrom)
- goto unknown_cmnd;
- common->data_size_from_cmnd =
- get_unaligned_be16(&common->cmnd[7]);
- status = check_command(common, 10, DATA_DIR_TO_HOST,
- (3<<7) | (0x1f<<1), 1,
- "READ HEADER");
- if (status == 0)
- status = do_read_header(common, bh);
- break;
-
- case READ_TOC:
- if (!common->curlun || !common->curlun->cdrom)
- goto unknown_cmnd;
- common->data_size_from_cmnd =
- get_unaligned_be16(&common->cmnd[7]);
- status = check_command(common, 10, DATA_DIR_TO_HOST,
- (7<<6) | (1<<1), 1,
- "READ TOC");
- if (status == 0)
- status = do_read_toc(common, bh);
- break;
-
- case READ_FORMAT_CAPACITIES:
- common->data_size_from_cmnd =
- get_unaligned_be16(&common->cmnd[7]);
- status = check_command(common, 10, DATA_DIR_TO_HOST,
- (3<<7), 1,
- "READ FORMAT CAPACITIES");
- if (status == 0)
- status = do_read_format_capacities(common, bh);
- break;
-
- case REQUEST_SENSE:
- common->data_size_from_cmnd = common->cmnd[4];
- status = check_command(common, 6, DATA_DIR_TO_HOST,
- (1<<4), 0,
- "REQUEST SENSE");
- if (status == 0)
- status = do_request_sense(common, bh);
- break;
-
- case START_STOP:
- common->data_size_from_cmnd = 0;
- status = check_command(common, 6, DATA_DIR_NONE,
- (1<<1) | (1<<4), 0,
- "START-STOP UNIT");
- if (status == 0)
- status = do_start_stop(common);
- break;
-
- case SYNCHRONIZE_CACHE:
- common->data_size_from_cmnd = 0;
- status = check_command(common, 10, DATA_DIR_NONE,
- (0xf<<2) | (3<<7), 1,
- "SYNCHRONIZE CACHE");
- if (status == 0)
- status = do_synchronize_cache(common);
- break;
-
- case TEST_UNIT_READY:
- common->data_size_from_cmnd = 0;
- status = check_command(common, 6, DATA_DIR_NONE,
- 0, 1,
- "TEST UNIT READY");
- break;
-
- /*
- * Although optional, this command is used by MS-Windows. We
- * support a minimal version: BytChk must be 0.
- */
- case VERIFY:
- common->data_size_from_cmnd = 0;
- status = check_command(common, 10, DATA_DIR_NONE,
- (1<<1) | (0xf<<2) | (3<<7), 1,
- "VERIFY");
- if (status == 0)
- status = do_verify(common);
- break;
-
- case WRITE_6:
- i = common->cmnd[4];
- common->data_size_from_cmnd = (i == 0) ? 256 : i;
- status = check_command_size_in_blocks(common, 6,
- DATA_DIR_FROM_HOST,
- (7<<1) | (1<<4), 1,
- "WRITE(6)");
- if (status == 0)
- status = do_write(common);
- break;
-
- case WRITE_10:
- common->data_size_from_cmnd =
- get_unaligned_be16(&common->cmnd[7]);
- status = check_command_size_in_blocks(common, 10,
- DATA_DIR_FROM_HOST,
- (1<<1) | (0xf<<2) | (3<<7), 1,
- "WRITE(10)");
- if (status == 0)
- status = do_write(common);
- break;
-
- case WRITE_12:
- common->data_size_from_cmnd =
- get_unaligned_be32(&common->cmnd[6]);
- status = check_command_size_in_blocks(common, 12,
- DATA_DIR_FROM_HOST,
- (1<<1) | (0xf<<2) | (0xf<<6), 1,
- "WRITE(12)");
- if (status == 0)
- status = do_write(common);
- break;
-
- /*
- * Some mandatory commands that we recognize but don't implement.
- * They don't mean much in this setting. It's left as an exercise
- * for anyone interested to implement RESERVE and RELEASE in terms
- * of Posix locks.
- */
- case FORMAT_UNIT:
- case RELEASE:
- case RESERVE:
- case SEND_DIAGNOSTIC:
-
- default:
-unknown_cmnd:
- common->data_size_from_cmnd = 0;
- sprintf(unknown, "Unknown x%02x", common->cmnd[0]);
- status = check_command(common, common->cmnd_size,
- DATA_DIR_UNKNOWN, ~0, 0, unknown);
- if (status == 0) {
- common->curlun->sense_data = SS_INVALID_COMMAND;
- status = -EINVAL;
+ if (common->cmnd[0] != curr_check->command)
+ continue;
+ if (WARN(curr_check->command != curr_handler->command,
+ "Invalid CDB handlers initialization. Command not matches\n")) {
+ goto end;
}
- break;
+
+ // The command was matched. Go to processing
+ switch (curr_check->size_index) {
+ case CDB_NO_SIZE_FIELD:
+ common->data_size_from_cmnd = 0;
+ break;
+ case CDB_SIZE_FIELD_4:
+ common->data_size_from_cmnd =
+ (common->cmnd[CDB_SIZE_FIELD_4] == 0) ?
+ 0xFF :
+ common->cmnd[CDB_SIZE_FIELD_4];
+ break;
+ case CDB_SIZE_FIELD_6:
+ common->data_size_from_cmnd =
+ get_unaligned_be32(&common->cmnd[CDB_SIZE_FIELD_6]);
+ break;
+ case CDB_SIZE_FIELD_7:
+ common->data_size_from_cmnd =
+ get_unaligned_be16(&common->cmnd[CDB_SIZE_FIELD_7]);
+ break;
+ case CDB_SIZE_MANUAL:
+ common->data_size_from_cmnd =
+ curr_check->data_size_manual;
+ break;
+ default:
+ // should never happen
+ pr_err("error of get kind size field\n");
+ goto end;
+ }
+
+ if (curr_check->do_check_command) {
+ status = curr_check->do_check_command(common,
+ curr_check->size, curr_check->direction,
+ curr_check->valid_bytes_bitmask,
+ curr_check->medium_required,
+ curr_handler->name);
+ } else {
+ DBG(common, "SCSI command: %s\n", curr_handler->name);
+ status = 0;
+ }
+
+ if (status == 0) {
+ if (curr_handler->type == CDB_HANDLER_COMMON &&
+ curr_handler->do_command) {
+ status = curr_handler->do_command(common);
+ } else if (curr_handler->type ==
+ CDB_HANDLER_FSG_BUFFHD &&
+ curr_handler->do_command_with_buffhd !=
+ NULL) {
+ status =
+ curr_handler->do_command_with_buffhd(common, bh);
+ }
+ }
+
+ goto end;
}
+
+ common->data_size_from_cmnd = 0;
+ sprintf(unknown, "Unknown %02Xh", common->cmnd[0]);
+ status = check_command(common, common->cmnd_size, DATA_DIR_UNKNOWN, ~0,
+ MEDIUM_OPTIONAL, unknown);
+ if (status == 0) {
+ common->curlun->sense_data = SS_INVALID_COMMAND;
+ status = -EINVAL;
+ }
+
+end:
up_read(&common->filesem);
if (status == -EINTR || signal_pending(current))
@@ -2082,7 +2153,6 @@ static int do_scsi_command(struct fsg_common *common)
return 0;
}
-
/*-------------------------------------------------------------------------*/
static int received_cbw(struct fsg_dev *fsg, struct fsg_buffhd *bh)
diff --git a/drivers/usb/gadget/function/storage_common.h b/drivers/usb/gadget/function/storage_common.h
index bdeb1e233fc9..84f5d2ffd7d8 100644
--- a/drivers/usb/gadget/function/storage_common.h
+++ b/drivers/usb/gadget/function/storage_common.h
@@ -172,6 +172,11 @@ enum data_direction {
DATA_DIR_NONE
};
+enum medium_required_values {
+ MEDIUM_OPTIONAL = 0,
+ MEDIUM_REQUIRED
+};
+
static inline struct fsg_lun *fsg_lun_from_dev(struct device *dev)
{
return container_of(dev, struct fsg_lun, dev);
--
2.32.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/6] fms: Add TOC/PMA/ATIP DVD-ROM capabilities
2021-06-26 21:18 [PATCH 0/6] USB-gadget: mass-storage: Support DVD-like images Igor Kononenko
2021-06-26 21:18 ` [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function Igor Kononenko
2021-06-26 21:18 ` [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling Igor Kononenko
@ 2021-06-26 21:18 ` Igor Kononenko
2021-06-27 14:29 ` Alan Stern
2021-06-26 21:18 ` [PATCH 4/6] fms: Support the DVD/BD images size over 2.1Gb Igor Kononenko
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Igor Kononenko @ 2021-06-26 21:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, Jens Axboe
Cc: openbmc, linux-usb, Igor Kononenko, linux-kernel
The DVD-ROM required the SCSI 6.25 READ TOC/PMA/ATIP Command formats:
* Response Format 0000b: Formatted TOC
* Response Format 0001b: Multi-session Information
(MMC-6 Specification).
This patch adds an implementation of that described above formats.
Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
---
drivers/usb/gadget/function/f_mass_storage.c | 99 +++++++++--
drivers/usb/gadget/function/storage_common.c | 10 +-
include/uapi/linux/cdrom.h | 177 +++++++++++++++++++
3 files changed, 261 insertions(+), 25 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index c3fddee21b53..4865937799aa 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -223,6 +223,7 @@
#include <linux/usb/composite.h>
#include <linux/nospec.h>
+#include <linux/cdrom.h>
#include "configfs.h"
@@ -1319,29 +1320,93 @@ static int do_read_header(struct fsg_common *common, struct fsg_buffhd *bh)
static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
{
- struct fsg_lun *curlun = common->curlun;
- int msf = common->cmnd[1] & 0x02;
- int start_track = common->cmnd[6];
- u8 *buf = (u8 *)bh->buf;
+ struct fsg_lun *curlun = common->curlun;
+ struct cdb_read_toc_pma_atip *cdb =
+ (struct cdb_read_toc_pma_atip *)common->cmnd;
+ struct read_tpa_header *header = (struct read_tpa_header *)bh->buf;
+ struct read_tpa_toc_formatted *data =
+ (struct read_tpa_toc_formatted *)((u8 *)bh->buf +
+ sizeof(*header));
+ size_t data_size = sizeof(*header);
- if ((common->cmnd[1] & ~0x02) != 0 || /* Mask away MSF */
- start_track > 1) {
+ if (cdb->format == 0) {
+ if (cdb->control == READ_TPA_CTRL_MAGIC_SESS) {
+ LDBG(curlun,
+ "The MMC-3 specifies format a control byte. Using Multi-Session info\n");
+ cdb->format = CDB_TPA_MULTI_SESS_INFO;
+ }
+ if (cdb->control == READ_TPA_CTRL_MAGIC_RAW) {
+ LDBG(curlun,
+ "The MMC-3 specifies format a control byte. Using RAW TOC\n");
+ cdb->format = CDB_TPA_RAW_TOC;
+ }
+ }
+
+ /* Currently support response format 0000b: Formatted TOC only */
+ if (cdb->format > CDB_TPA_MULTI_SESS_INFO) {
+ LDBG(curlun, "Unsupported TOC/PMA/ATIP format: %02Xh\n",
+ cdb->format);
curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
return -EINVAL;
}
- memset(buf, 0, 20);
- buf[1] = (20-2); /* TOC data length */
- buf[2] = 1; /* First track number */
- buf[3] = 1; /* Last track number */
- buf[5] = 0x16; /* Data track, copying allowed */
- buf[6] = 0x01; /* Only track is number 1 */
- store_cdrom_address(&buf[8], msf, 0);
+ /*
+ * We only support one track per disk.
+ * We also needs to indicate the number of the last track
+ */
+ if (cdb->number > 1 && cdb->number != READ_TPA_LEADOUT_TRACK) {
+ curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
+ return -EINVAL;
+ }
- buf[13] = 0x16; /* Lead-out track is data */
- buf[14] = 0xAA; /* Lead-out track number */
- store_cdrom_address(&buf[16], msf, curlun->num_sectors);
- common->data_size_to_handle = 20;
+ /*
+ * MULTI-SESSIOIN information must be reported only for first track.
+ */
+ if (cdb->format == CDB_TPA_MULTI_SESS_INFO && cdb->number > 1) {
+ curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
+ return -EINVAL;
+ }
+
+ memset(header, 0, sizeof(*header));
+ header->n_first_stf = 1;
+ header->n_last_stf = 1;
+
+ memset(data, 0, sizeof(*data));
+ data->addr = 1;
+ data->control = TPA_SECTOR_MODE2_MIXED;
+ data->track_number = cdb->number;
+ data_size += sizeof(*data);
+
+ /*
+ * We have too case:
+ * 1) The request Track Number == 1.
+ * We shall set 2 descriptors: First Track, Lead-Out Track
+ * 2) The requested Track Number == 0xAA
+ * Only Lead-Out descriptor shall be set
+ */
+ if (cdb->number == 1) {
+ DBG(common, "Fill first track addr\n");
+ store_cdrom_address((u8 *)&data->start_addr_track, cdb->msf, 0);
+
+ data += 1; /* Add one more descriptor */
+ data_size += sizeof(*data);
+ memset(data, 0, sizeof(*data));
+ /* setting the lead-out track info. First part of data*/
+ data->addr = 1;
+ data->control = TPA_SECTOR_MODE2_MIXED;
+ data->track_number = READ_TPA_LEADOUT_TRACK;
+ }
+
+ /*
+ * Lead-out track must be set anyway.
+ * If 0xAA Track is requested - the first part of data is already set.
+ */
+ DBG(common, "Fill last track addr\n");
+ store_cdrom_address((u8 *)&data->start_addr_track,
+ cdb->msf, curlun->num_sectors);
+
+ header->length = cpu_to_be16(data_size - sizeof(header->length));
+ common->data_size_to_handle = data_size;
return 0;
}
diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
index b859a158a414..b883b8b7b05b 100644
--- a/drivers/usb/gadget/function/storage_common.c
+++ b/drivers/usb/gadget/function/storage_common.c
@@ -24,6 +24,7 @@
#include <linux/file.h>
#include <linux/fs.h>
#include <linux/usb/composite.h>
+#include <linux/cdrom.h>
#include "storage_common.h"
@@ -295,14 +296,7 @@ void store_cdrom_address(u8 *dest, int msf, u32 addr)
{
if (msf) {
/* Convert to Minutes-Seconds-Frames */
- addr >>= 2; /* Convert to 2048-byte frames */
- addr += 2*75; /* Lead-in occupies 2 seconds */
- dest[3] = addr % 75; /* Frames */
- addr /= 75;
- dest[2] = addr % 60; /* Seconds */
- addr /= 60;
- dest[1] = addr; /* Minutes */
- dest[0] = 0; /* Reserved */
+ lba_to_msf(addr, &dest[1], &dest[2], &dest[3]);
} else {
/* Absolute sector */
put_unaligned_be32(addr, dest);
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 6c34f6e2f1f7..1d7b4333c3aa 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -947,4 +947,181 @@ struct rm_feature_desc {
__u8 reserved4;
};
+/**
+ * The READ TOC/PMA/ATIP format field values
+ */
+enum cdb_read_tpa_format {
+ /**
+ * The Track/Session Number field specifies starting track number
+ * for which the data is returned. For multi-session discs, TOC
+ * data is returned for all sessions. Track number AAh is reported
+ * only for the Lead-out area of the last complete session.
+ */
+ CDB_TPA_FORMATTED_TOC,
+ /**
+ * This format returns the first complete session number, last
+ * complete session number and last complete session starting address.
+ * In this format, the Track/Session Number field is reserved and
+ * should be set to 00h.
+ * NOTE: This format provides the Host access to the last closed
+ * session starting address quickly.
+ */
+ CDB_TPA_MULTI_SESS_INFO,
+ /**
+ * This format returns all Q sub-code data in the Lead-In (TOC) areas
+ * starting from a session number as specified in the Number
+ * Track/Session Number field.
+ * In this mode, the Drive shall support Q Sub-channel POINT field
+ * value of A0h, A1h, A2h, Track numbers, B0h, B1h, B2h, B3h, B4h, C0h,
+ * and C1h.
+ * There is no defined LBA addressing and MSF bit shall be set to one.
+ */
+ CDB_TPA_RAW_TOC,
+ /**
+ * This format returns Q sub-channel data in the PMA area. In this
+ * format, the Track/Session Number field is reserved and shall be
+ * set to 00h. There is no defined LBA addressing and MSF bit
+ * shall be set to one.
+ */
+ CDB_TPA_PMA,
+ /**
+ * This format returns ATIP data. In this format, the Track/Session
+ * Number field is reserved and shall be set to 00h. There is no
+ * defined LBA addressing and MSF bit shall be set to one.
+ */
+ CDB_TPA_ATIP,
+ /**
+ * This format returns CD-TEXT information that is recorded in the
+ * Lead-in area as R-W Sub-channel Data.
+ */
+ CDB_TPA_CD_TEXT,
+};
+
+#define TPA_SECTOR_MODE0 (0x00)
+#define TPA_SECTOR_AUDIO (0x01)
+#define TPA_SECTOR_MODE1 (0x02)
+#define TPA_SECTOR_MODE2 (0x03)
+#define TPA_SECTOR_MODE2_FORM1 (0x04)
+#define TPA_SECTOR_MODE2_FORM2 (0x05)
+#define TPA_SECTOR_MODE2_MIXED (TPA_SECTOR_MODE1 | TPA_SECTOR_MODE2_FORM1)
+#define TPA_SECTOR_RAW (0x07)
+#define TPA_SECTOR_RAW_SCRAMBLED (0x08)
+
+/**
+ * @brief The READ TOC/PMA/ATIP CDB (43h)
+ * The READ TOC/PMA/ATIP command requests that the Drive read data from a
+ * Table of Contents, the Program Memory Area (PMA), or the Absolute Time
+ * in Pre-Grove (ATIP) from CD media, format according to CDB parameters
+ * and transfer the result to the Host.
+ */
+struct cdb_read_toc_pma_atip {
+ __u8 code;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved1 : 6;
+ /**
+ * When MSF is set to zero, the address fields in some returned data
+ * formats shall be in LBA form. When MSF is set to one, the address
+ * fields in some returned data formats shall be in MSF form
+ */
+ __u8 msf : 1;
+ __u8 reserved2 : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 reserved2 : 1;
+ __u8 msf : 1;
+ __u8 reserved1 : 6;
+#endif
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved3 : 4;
+ /**
+ * The Format field is used to select specific returned data format
+ * according to @enum cdb_read_tpa_format
+ */
+ __u8 format : 4;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 format : 4;
+ __u8 reserved3 : 4;
+#endif
+
+ __u8 reserved4[3];
+ /**
+ * The Track/Session Number field provides a method to restrict the
+ * returned of some data formats to a specific session or a track range
+ */
+ __u8 number;
+
+ /**
+ * The Allocation Length field specifies the maximum number of bytes that
+ * may be returned by the Drive.
+ * An Allocation Length field of zero shall not be considered an error
+ */
+ __be16 length;
+
+ __u8 control;
+} __packed;
+
+#define READ_TPA_LEADOUT_TRACK (0xAAU)
+/*
+ * Control magic byte
+ * Some legacy media recorder implementations set the control byte,
+ * helping determine the relevant TOC/PMA/ATIP formats.
+ * We should support this as well.
+ */
+#define READ_TPA_CTRL_MAGIC_SESS (0x40U)
+#define READ_TPA_CTRL_MAGIC_RAW (0x80U)
+
+/**
+ * @brief READ TOC/PMA/ATIP Data list header
+ * The response data list shows the general description of the response data
+ * to the Read TOC/PMA/ATIP command.
+ */
+struct read_tpa_header {
+ __be16 length;
+ /* First Track/Session/Reserved Field */
+ __u8 n_first_stf;
+ /* Last Track/Session/Reserved Field */
+ __u8 n_last_stf;
+} __packed;
+
+/**
+ * @brief Response Format 0000b: Formatted TOC
+ * The response data consist of four header bytes and zero or more track
+ * descriptors.
+ */
+struct read_tpa_toc_formatted {
+ __u8 reserved1;
+#if defined(__BIG_ENDIAN_BITFIELD)
+ /**
+ * The ADR field gives the type of information encoded in the Q Sub-channel
+ * of the block where this TOC entry was found.
+ */
+ __u8 addr : 4;
+ /**
+ * The CONTROL Field indicates the attributes of the track.
+ */
+ __u8 control : 4;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 control : 4;
+ __u8 addr : 4;
+#endif
+ /**
+ * The Track Start Address contains the address of the first block with user
+ * information for that track number as read from the Table of Contents.
+ * A MSF bit of zero indicates that the Track Start Address field shall contain
+ * a logical block address.
+ * A MSF bit of one indicates the Logical Block Address field shall contain a
+ * MSF address
+ */
+ __u8 track_number;
+ __u8 reserved2;
+ /**
+ * The Track Number field indicates the track number for that the data in the
+ * TOC track descriptor is valid. A track number of READ_TPA_LEADOUT_TRACK
+ * indicates that the track descriptor is for the start of the Lead-out area.
+ */
+ __be32 start_addr_track;
+} __packed;
+
+
#endif /* _UAPI_LINUX_CDROM_H */
--
2.32.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/6] fms: Support the DVD/BD images size over 2.1Gb
2021-06-26 21:18 [PATCH 0/6] USB-gadget: mass-storage: Support DVD-like images Igor Kononenko
` (2 preceding siblings ...)
2021-06-26 21:18 ` [PATCH 3/6] fms: Add TOC/PMA/ATIP DVD-ROM capabilities Igor Kononenko
@ 2021-06-26 21:18 ` Igor Kononenko
2021-06-26 21:18 ` [PATCH 5/6] FMS: Add the SCSI Get Configuration command Igor Kononenko
2021-06-26 21:18 ` [PATCH 6/6] FMS: Add SCSI Read Disc Information command Igor Kononenko
5 siblings, 0 replies; 22+ messages in thread
From: Igor Kononenko @ 2021-06-26 21:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman
Cc: openbmc, linux-usb, Igor Kononenko, linux-kernel
Adds the ability to use the FMS image size greater than 2.1Gb. This
limitation is due to the maximum number of available frames on the
CD-ROM media.
An incoming implementation of additional media formats (DVD-ROM, BD-ROM)
should support both formats' possible capacity.
End-user-impact: Now, the FMS able to use a medium-image backend file,
which size is more significant than 2.1Gb
Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
---
drivers/usb/gadget/function/storage_common.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/storage_common.c b/drivers/usb/gadget/function/storage_common.c
index b883b8b7b05b..468f7622b11d 100644
--- a/drivers/usb/gadget/function/storage_common.c
+++ b/drivers/usb/gadget/function/storage_common.c
@@ -242,15 +242,8 @@ int fsg_lun_open(struct fsg_lun *curlun, const char *filename)
num_sectors = size >> blkbits; /* File size in logic-block-size blocks */
min_sectors = 1;
- if (curlun->cdrom) {
+ if (curlun->cdrom)
min_sectors = 300; /* Smallest track is 300 frames */
- if (num_sectors >= 256*60*75) {
- num_sectors = 256*60*75 - 1;
- LINFO(curlun, "file too big: %s\n", filename);
- LINFO(curlun, "using only first %d blocks\n",
- (int) num_sectors);
- }
- }
if (num_sectors < min_sectors) {
LINFO(curlun, "file too small: %s\n", filename);
rc = -ETOOSMALL;
--
2.32.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/6] FMS: Add the SCSI Get Configuration command.
2021-06-26 21:18 [PATCH 0/6] USB-gadget: mass-storage: Support DVD-like images Igor Kononenko
` (3 preceding siblings ...)
2021-06-26 21:18 ` [PATCH 4/6] fms: Support the DVD/BD images size over 2.1Gb Igor Kononenko
@ 2021-06-26 21:18 ` Igor Kononenko
2021-06-27 0:44 ` kernel test robot
` (2 more replies)
2021-06-26 21:18 ` [PATCH 6/6] FMS: Add SCSI Read Disc Information command Igor Kononenko
5 siblings, 3 replies; 22+ messages in thread
From: Igor Kononenko @ 2021-06-26 21:18 UTC (permalink / raw)
To: Jens Axboe, Felipe Balbi, Greg Kroah-Hartman,
James E.J. Bottomley, Martin K. Petersen
Cc: Igor Kononenko, linux-scsi, openbmc, linux-usb, linux-kernel, linux-ide
The SCSI Get Configuration command is required to obtain information
about a CD/DVD/BL device (MMC-6, 6.5 GET CONFIGURATION Command).
The aforementioned ability will be expected by several multimedia
consumers, such as OS MS Windows, OS ESXi, etc., for selecting the
appropriate FS driver.
End-user-impact: The FMS devices consumers can now retrieve their
capabilities.
FMS supports the ISO-13346(UDF) multimedia file systems
Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
---
drivers/ata/libata-zpodd.c | 8 +-
drivers/usb/gadget/function/f_mass_storage.c | 222 +++++
include/scsi/scsi_proto.h | 1 +
include/uapi/linux/cdrom.h | 844 ++++++++++++++++++-
4 files changed, 1041 insertions(+), 34 deletions(-)
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index eefda51f97d3..e49ff795a506 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -54,7 +54,7 @@ static enum odd_mech_type zpodd_get_mech_type(struct ata_device *dev)
{
char *buf;
unsigned int ret;
- struct rm_feature_desc *desc;
+ struct cdf_removable_medium *desc;
struct ata_taskfile tf;
static const char cdb[ATAPI_CDB_LEN] = { GPCMD_GET_CONFIGURATION,
2, /* only 1 feature descriptor requested */
@@ -82,15 +82,15 @@ static enum odd_mech_type zpodd_get_mech_type(struct ata_device *dev)
return ODD_MECH_TYPE_UNSUPPORTED;
}
- if (be16_to_cpu(desc->feature_code) != 3) {
+ if (be16_to_cpu(desc->code) != 3) {
kfree(buf);
return ODD_MECH_TYPE_UNSUPPORTED;
}
- if (desc->mech_type == 0 && desc->load == 0 && desc->eject == 1) {
+ if (desc->mechanism == 0 && desc->load == 0 && desc->eject == 1) {
kfree(buf);
return ODD_MECH_TYPE_SLOT;
- } else if (desc->mech_type == 1 && desc->load == 0 &&
+ } else if (desc->mechanism == 1 && desc->load == 0 &&
desc->eject == 1) {
kfree(buf);
return ODD_MECH_TYPE_DRAWER;
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 4865937799aa..7e736e5594f9 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -366,6 +366,87 @@ struct cdb_handler {
/* SCSI command ASCII name */
char *name;
};
+/*-------------------------------------------------------------------------*/
+
+#define CDF_PROFILES_COUNT (ARRAY_SIZE(cdf_supported_profiles))
+
+/* List of supported profiles */
+static struct mmc_profile cdf_supported_profiles[] = {
+ { .profile = cpu_to_be16(MMC_PROFILE_BD_ROM) },
+ { .profile = cpu_to_be16(MMC_PROFILE_DVD_ROM) },
+ { .profile = cpu_to_be16(MMC_PROFILE_CD_ROM) },
+};
+
+struct cdf_profile_list_custom {
+ struct cdf_profile_list header;
+ /* We support several profiles, whose indices are declared in the
+ * enum above
+ */
+ struct mmc_profile profiles[CDF_PROFILES_COUNT];
+} __packed;
+
+/**
+ * Type to allocate of all supported features
+ * @param populate - callback to fill the specified feature data which
+ * is depended by medium
+ */
+struct cdr_features;
+struct cdr_features {
+ union {
+ struct cdf_profile_list_custom profile_list;
+ struct cdf_core core;
+ struct cdf_morphing morphing;
+ struct cdf_removable_medium removable;
+ struct cdf_random_readable random_readable;
+ struct cdf_multi_read multi_read;
+ struct cdf_cd_read cd_read;
+ struct cdf_dvd_read dvd_read;
+ struct cdf_rt_streaming rt_streaming;
+ } __packed feature;
+ void (*populate)(struct fsg_common *common,
+ struct cdr_features **features);
+};
+
+/**
+ * @brief Adjust the Profile List members of actual data which is depended
+ * on the inserted medium image
+ *
+ * @param common - the FSG instance
+ * @param feature - a list of profile descriptors which to be configured
+ */
+static void cdf_populate_profile_list(struct fsg_common *common,
+ struct cdr_features **feature);
+
+#define CDF_SET_VPC(v, p, c) .vpc = { .ver = (v), .per = (p), .cur = (c) }
+
+#define CDF_FT_SIZE(member) \
+ ((sizeof(((struct cdr_features *)0)->feature.member)) - \
+ sizeof(struct cdb_ft_generic))
+
+#define CDR_FT_ITEM(item, c, ...) \
+ CDR_FT_ITEM_S(item, c, CDF_FT_SIZE(item), __VA_ARGS__)
+
+#define CDR_FT_ITEM_S(item, c, s, ...) \
+ .feature.item = { .code = cpu_to_be16(c), .length = (s), __VA_ARGS__ }
+
+static struct cdr_features features_table[] = {
+ { CDR_FT_ITEM_S(profile_list.header, CDF_PROFILE_LIST_CODE,
+ CDF_FT_SIZE(profile_list), CDF_SET_VPC(0, 1, 1)),
+ .populate = &cdf_populate_profile_list },
+ { CDR_FT_ITEM(core, CDF_CORE, CDF_SET_VPC(2, 1, 1), .dbevent = 1,
+ .interface = cpu_to_be32(CF_PIS_USB)) },
+ { CDR_FT_ITEM(morphing, CDF_MORPHING_CODE, CDF_SET_VPC(1, 1, 1),
+ .ocevent = 1) },
+ { CDR_FT_ITEM(removable, CDF_REMOVEBLE_MEDIA, CDF_SET_VPC(2, 1, 1),
+ .mechanism = CDF_LMT__TRAY_TYPE, .eject = 1, .lock = 1) },
+ { CDR_FT_ITEM(random_readable, CDF_RANDOM_READ, CDF_SET_VPC(0, 0, 1),
+ .block_size = cpu_to_be32(CD_FRAMESIZE),
+ .blocking = cpu_to_be16(0x10), .pp = 1) },
+ { CDR_FT_ITEM(dvd_read, CDF_DVD_READ, CDF_SET_VPC(2, 1, 1),
+ .multi110 = 1, .dualr = 1) },
+ { CDR_FT_ITEM(rt_streaming, CDF_REAL_TIME_STREAM, CDF_SET_VPC(5, 0, 1),
+ .rbcb = 1, .scs = 1, .mp2a = 1, .wspd = 1, .sw = 1) },
+};
/*------------------------------------------------------------------------*/
@@ -1851,6 +1932,143 @@ static void send_status(struct fsg_common *common)
return;
}
+/**
+ * Attempts to guess medium type by looking at the length of the disc layout.
+ */
+static inline __be16 cdr_guess_medium_type(struct fsg_common *common)
+{
+ struct fsg_lun *curlun = common->curlun;
+ size_t length = curlun->num_sectors;
+
+ if (length <= CD_MAX_FRAMES) {
+ LDBG(curlun, "Disc layout size implies CD-ROM image\n");
+ return MMC_PROFILE_CD_ROM;
+ } else if (length <= CD_DVD_MAX_FRAMES) {
+ LDBG(curlun,
+ "Disc layout size implies single-layer DVD-ROM image\n");
+ return MMC_PROFILE_DVD_ROM;
+ } else if (length <= CD_DVDDL_MAX_FRAMES) {
+ LDBG(curlun,
+ "Disc layout size implies dual-layer DVD-ROM image\n");
+ return MMC_PROFILE_DVD_ROM;
+ } else if (length <= CD_BD_MAX_FRAMES) {
+ LDBG(curlun,
+ "Disc layout size implies single-layer BD-ROM image\n");
+ return MMC_PROFILE_BD_ROM;
+ } else if (length <= CD_BDDL_MAX_FRAMES) {
+ LDBG(curlun,
+ "Disc layout size implies dual-layer BD-ROM image\n");
+ return MMC_PROFILE_BD_ROM;
+ }
+
+ LDBG(curlun,
+ "Disc layout size (%u) exceeds all known media types, assuming BD - ROM !\n",
+ length);
+ return MMC_PROFILE_BD_ROM;
+}
+
+/* Adjust current profile which depended on an inserted medium */
+static inline void cdf_populate_profile_list(struct fsg_common *common,
+ struct cdr_features **feature)
+{
+ __be16 current_media_type = cdr_guess_medium_type(common);
+ struct mmc_profile *profiles =
+ (*feature)->feature.profile_list.profiles;
+ int i;
+
+ /* copy profile list to the response buffer */
+ memcpy(profiles, cdf_supported_profiles,
+ sizeof(cdf_supported_profiles));
+ for (i = 0; i < CDF_PROFILES_COUNT; ++i) {
+ /*
+ * Reset the current profile bit,
+ * because it might be set from the previous one
+ */
+ profiles[i].current_p = 0;
+ if (be16_to_cpu(profiles[i].profile) == current_media_type) {
+ DBG(common, "Fill current profile: curr=(%04Xh)\n",
+ be16_to_cpu(profiles[i].profile));
+ profiles[i].current_p = 1;
+ }
+ }
+}
+
+static int do_get_configuration(struct fsg_common *common,
+ struct fsg_buffhd *bh)
+{
+ struct fsg_lun *curlun = common->curlun;
+ int i;
+ struct cdb_get_configuration *cdb =
+ (struct cdb_get_configuration *)common->cmnd;
+ size_t buffer_size = sizeof(struct feature_header);
+ size_t generic_desc_size = sizeof(struct cdb_ft_generic);
+ struct feature_header *ret_header = (struct feature_header *)bh->buf;
+ u8 *ret_data = ((u8 *)ret_header) + buffer_size;
+
+ LDBG(curlun, "Requesting features from 0x%04X, with RT flag 0x%02X\n",
+ be16_to_cpu(cdb->sfn), cdb->rt);
+
+ if (!common->curlun || !common->curlun->cdrom)
+ return -EINVAL;
+
+ /* Go over *all* features, and copy them according to RT value */
+ for (i = 0; i < ARRAY_SIZE(features_table); ++i) {
+ struct cdb_ft_generic *generic =
+ (struct cdb_ft_generic *)&features_table[i];
+ struct cdr_features *feature = &features_table[i];
+
+ if (feature->populate != NULL)
+ feature->populate(common, &feature);
+
+ // a) RT is 0x00 and feature's code >= SFN
+ // b) RT is 0x01, feature's code >= SFN and feature has 'current' bit set
+ // c) RT is 0x02 and feature's code == SFN
+
+ if (be16_to_cpu(generic->code) >= be16_to_cpu(cdb->sfn)) {
+ if ((cdb->rt == CDR_CFG_RT_FULL) ||
+ (cdb->rt == CDR_CFG_RT_CURRENT &&
+ generic->vpc.cur) ||
+ (cdb->rt == CDR_CFG_RT_SPECIFIED_SFN &&
+ be16_to_cpu(generic->code) ==
+ be16_to_cpu(cdb->sfn))) {
+ LDBG(curlun, "Copying feature 0x%04X\n",
+ be16_to_cpu(generic->code));
+
+ memset(ret_data, 0,
+ (generic->length + generic_desc_size));
+ /* Copy feature */
+ memcpy(ret_data, feature,
+ (generic->length + generic_desc_size));
+ buffer_size +=
+ (generic->length + generic_desc_size);
+ ret_data +=
+ (generic->length + generic_desc_size);
+
+ /* Break the loop if RT is CDR_CFG_RT_SPECIFIED_SFN */
+ if (cdb->rt == CDR_CFG_RT_SPECIFIED_SFN) {
+ LDBG(curlun,
+ "Got the feature we wanted (0x%04X), breaking the loop\n",
+ be16_to_cpu(cdb->sfn));
+ break;
+ }
+ }
+ }
+ }
+
+ memset(ret_header, 0, sizeof(struct feature_header));
+ /* Header */
+ ret_header->data_len = cpu_to_be32(buffer_size - generic_desc_size);
+ ret_header->curr_profile =
+ cpu_to_be16(cdr_guess_medium_type(common));
+
+ dump_msg(common, "feature header", (u8 *)ret_header,
+ sizeof(struct feature_header));
+
+ dump_msg(common, "features table", (u8 *)bh->buf, buffer_size);
+
+ common->data_size_to_handle = buffer_size;
+ return 0;
+}
/*-------------------------------------------------------------------------*/
@@ -2035,6 +2253,9 @@ static struct cdb_command_check cdb_checker_table[] = {
{ CDB_REG_CHECKER(TEST_UNIT_READY, 6, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
0x0000, MEDIUM_REQUIRED) },
+ { CDB_REG_NO_CHECKER(GET_CONFIGURATION, CDB_SIZE_FIELD_7,
+ DATA_DIR_TO_HOST, MEDIUM_REQUIRED) },
+
{ CDB_REG_CHECKER_BLK(VERIFY, 10, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
0x0000, MEDIUM_REQUIRED) },
{ CDB_REG_CHECKER_BLK(WRITE_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_FROM_HOST,
@@ -2065,6 +2286,7 @@ static struct cdb_handler cdb_handlers_table[] = {
{ CDB_REG_HANDLER(SYNCHRONIZE_CACHE, &do_synchronize_cache) },
{ CDB_REG_HANDLER(TEST_UNIT_READY, NULL) },
+ { CDB_REG_HANDLER_BUFFHD(GET_CONFIGURATION, &do_get_configuration) },
/*
* Although optional, this command is used by MS-Windows. We
* support a minimal version: BytChk must be 0.
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index c36860111932..6b2a8ee1f0a3 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -73,6 +73,7 @@
#define UNMAP 0x42
#define READ_TOC 0x43
#define READ_HEADER 0x44
+#define GET_CONFIGURATION 0x46
#define GET_EVENT_STATUS_NOTIFICATION 0x4a
#define LOG_SELECT 0x4c
#define LOG_SENSE 0x4d
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 1d7b4333c3aa..442693fdc059 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -349,6 +349,12 @@ struct cdrom_generic_command
/* most drives don't deliver everything: */
#define CD_FRAMESIZE_RAW1 (CD_FRAMESIZE_RAW-CD_SYNC_SIZE) /*2340*/
#define CD_FRAMESIZE_RAW0 (CD_FRAMESIZE_RAW-CD_SYNC_SIZE-CD_HEAD_SIZE) /*2336*/
+/* total frames on the specific medium-disk format */
+#define CD_MAX_FRAMES (CD_MINS * CD_SECS * CD_FRAMES)
+#define CD_DVD_MAX_FRAMES (2295104)
+#define CD_DVDDL_MAX_FRAMES (4173824)
+#define CD_BD_MAX_FRAMES (12219392)
+#define CD_BDDL_MAX_FRAMES (24438784)
#define CD_XA_HEAD (CD_HEAD_SIZE+CD_SUBHEAD_SIZE) /* "before data" part of raw XA frame */
#define CD_XA_TAIL (CD_EDC_SIZE+CD_ECC_SIZE) /* "after data" part of raw XA frame */
@@ -896,12 +902,173 @@ typedef struct {
__be32 last_rec_address;
} track_information;
+/* CDB Get Configuration command */
+
+/**
+ * The Drive shall return the Feature Header and all Feature Descriptors supported by the
+ * Drive without regard to currency
+ */
+#define CDR_CFG_RT_FULL 0x00
+/**
+ * The Drive shall return the Feature Header and only those Feature Descriptors in which
+ * the Current bit is set to one.
+ */
+#define CDR_CFG_RT_CURRENT 0x01
+/**
+ * The Feature Header and the Feature Descriptor identified by Starting Feature Number
+ * shall be returned. If the Drive does not support the specified feature, only the Feature
+ * Header shall be returned.
+ */
+#define CDR_CFG_RT_SPECIFIED_SFN 0x02
+#define CDR_CFG_RT_RESERVED 0x03
+
+/**
+ * @brief GET CONFIGURATION Command
+ * The GET CONFIGURATION command provides a Host with information about Drive capabilities;
+ * both current and potential.
+ *
+ * @note The command shall not return a CHECK CONDITION Status due to a pending
+ * UNIT ATTENTION Condition.
+ */
+struct cdb_get_configuration {
+ __u8 code;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved1 : 6;
+ /* The RT field identifies the type of data to be returned by the Drive */
+ __u8 rt : 2;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 rt : 2;
+ __u8 reserved1 : 6;
+#endif
+ /**
+ * The Starting Feature Number field indicates the first Feature number to be returned.
+ * All supported Feature numbers higher than the Starting Feature Number shall be
+ * returned.
+ */
+ __be16 sfn;
+ __u8 reserved2[3];
+ /**
+ * The Allocation Length field specifies the maximum length in bytes of the
+ * Get Configuration response data. An Allocation Length field of zero indicates that no
+ * data shall be transferred
+ */
+ __be16 length;
+ __u8 control;
+
+} __packed;
+
+/* Features */
+
+/* Feature and Profile Descriptors*/
+
+/**
+ * @brief The Version, Persisten and Current byte.
+ * This structure is required for many CDB features.
+ */
+struct cdb_ft_vpc_byte {
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved1 : 2;
+ /**
+ * The Version field is reserved and shall be set to zero unless otherwise specified
+ * within the Feature Description
+ */
+ __u8 ver : 4;
+ /**
+ * The Persistent bit, when set to zero, shall indicate that this Feature may change
+ * its current status.
+ * When set to one, shall indicate that this Feature is always active.
+ * The Drive shall not set this bit to one if the Current bit is, or may become, zero.
+ */
+ __u8 per : 1;
+ /**
+ * The Current bit, when set to zero, indicates that this Feature is not currently
+ * active and that the Feature Dependent Data may not be valid.
+ * When set to one, this Feature is currently active and the Feature Dependent Data is
+ * valid.
+ */
+ __u8 cur : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 cur : 1;
+ __u8 per : 1;
+ __u8 ver : 4;
+ __u8 reserved1 : 2;
+#endif
+} __packed;
+
+/**
+ * @brief Feature Descriptor generic
+ * A Feature Descriptor shall describe each Feature supported by a Drive. All
+ * Feature descriptors shall be a multiple of four bytes
+ */
+struct cdb_ft_generic {
+ /**
+ * The Feature Code field shall identify a Feature supported by the Drive
+ */
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ /**
+ * The Additional Length field indicates the number of Feature specific
+ * bytes that follow this header. This field shall be an integral multiple
+ * of 4
+ */
+ __u8 length;
+} __packed;
+
+/* Profile list */
+#define MMC_PROFILE_NONE 0x0000
+#define MMC_PROFILE_CD_ROM 0x0008
+#define MMC_PROFILE_CD_R 0x0009
+#define MMC_PROFILE_CD_RW 0x000A
+#define MMC_PROFILE_DVD_ROM 0x0010
+#define MMC_PROFILE_DVD_R_SR 0x0011
+#define MMC_PROFILE_DVD_RAM 0x0012
+#define MMC_PROFILE_DVD_RW_RO 0x0013
+#define MMC_PROFILE_DVD_RW_SR 0x0014
+#define MMC_PROFILE_DVD_R_DL_SR 0x0015
+#define MMC_PROFILE_DVD_R_DL_JR 0x0016
+#define MMC_PROFILE_DVD_RW_DL 0x0017
+#define MMC_PROFILE_DVD_DDR 0x0018
+#define MMC_PROFILE_DVD_PLUS_RW 0x001A
+#define MMC_PROFILE_DVD_PLUS_R 0x001B
+#define MMC_PROFILE_DVD_PLUS_RW_DL 0x002A
+#define MMC_PROFILE_DVD_PLUS_R_DL 0x002B
+#define MMC_PROFILE_BD_ROM 0x0040
+#define MMC_PROFILE_BD_R_SRM 0x0041
+#define MMC_PROFILE_BD_R_RRM 0x0042
+#define MMC_PROFILE_BD_RE 0x0043
+#define MMC_PROFILE_HDDVD_ROM 0x0050
+#define MMC_PROFILE_HDDVD_R 0x0051
+#define MMC_PROFILE_HDDVD_RAM 0x0052
+#define MMC_PROFILE_HDDVD_RW 0x0053
+#define MMC_PROFILE_HDDVD_R_DL 0x0058
+#define MMC_PROFILE_HDDVD_RW_DL 0x005A
+#define MMC_PROFILE_INVALID 0xFFFF
+
+/**
+ * @brief The CDB Feature Header
+ * Response data consists of a header field and zero or more variable length
+ * Feature descriptors
+ */
struct feature_header {
+ /**
+ * The Data Length field indicates the amount of data available given a
+ * sufficient allocation length following this field.
+ * This length shall not be truncated due to an insufficient Allocation
+ * Length
+ */
__u32 data_len;
__u8 reserved1;
__u8 reserved2;
+ /**
+ * The Current Profile field shall identify one of the profiles from the
+ * Profile List Feature. If there are no Profiles currently active, this
+ * field shall contain zero.
+ */
__u16 curr_profile;
-};
+} __packed;
struct mode_page_header {
__be16 mode_data_length;
@@ -912,41 +1079,658 @@ struct mode_page_header {
__be16 desc_length;
};
-/* removable medium feature descriptor */
-struct rm_feature_desc {
- __be16 feature_code;
+/**
+ * @brief Profile descriptors are returned in the order of preferred
+ * operation – most desirable to least desirable. e.g., a DVD-ROM
+ * that is also able to read a CD-ROM should list the DVD-ROM
+ * Profile first and the CD-ROM Profile second.
+ */
+struct mmc_profile {
+ /* The Profile Number identifies a Profile */
+ __be16 profile;
+
#if defined(__BIG_ENDIAN_BITFIELD)
- __u8 reserved1:2;
- __u8 feature_version:4;
- __u8 persistent:1;
- __u8 curr:1;
+ __u8 reserved1 : 7;
+ /**
+ * The current_p bit, when set to one, shall indicate that this
+ * Profile is currently active.
+ */
+ __u8 current_p : 1;
#elif defined(__LITTLE_ENDIAN_BITFIELD)
- __u8 curr:1;
- __u8 persistent:1;
- __u8 feature_version:4;
- __u8 reserved1:2;
-#endif
- __u8 add_len;
-#if defined(__BIG_ENDIAN_BITFIELD)
- __u8 mech_type:3;
- __u8 load:1;
- __u8 eject:1;
- __u8 pvnt_jmpr:1;
- __u8 dbml:1;
- __u8 lock:1;
-#elif defined(__LITTLE_ENDIAN_BITFIELD)
- __u8 lock:1;
- __u8 dbml:1;
- __u8 pvnt_jmpr:1;
- __u8 eject:1;
- __u8 load:1;
- __u8 mech_type:3;
+ __u8 current_p : 1;
+ __u8 reserved1 : 7;
#endif
+
__u8 reserved2;
- __u8 reserved3;
- __u8 reserved4;
+} __packed;
+
+/**
+ * @brief Profile List Feature (0000h)
+ *
+ * This Feature identifies Profiles supported by the Drive.
+ * Profiles are defined as collections of Features and provide a method
+ * to quickly determine the Drive’s type.
+ */
+struct cdf_profile_list {
+ /* The Feature Code */
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+ /**
+ * The Additional Length field shall be set
+ * to ((number of Profile Descriptors) * 4).
+ */
+ __u8 length;
+} __packed;
+
+/**
+ * @brief The core feature: phisycal interface standards
+ */
+enum cdf_cf_pis {
+ CF_PIS_UNSPECIFIED = 0x00000000U,
+ CF_PIS_SCSI_FAMILY,
+ CF_PIS_ATAPI,
+ CF_PIS_IEEE_1394_1995,
+ CF_PIS_IEEE_1394A,
+ CF_PIS_FIBRE_CHANNEL,
+ CF_PIS_IEEE_1394_B,
+ CF_PIS_USB,
+ CF_PIS_RESERVED,
+ CF_PIS_DEF_INCITS = 0x00010000U,
+ CF_PIS_DEF_SFF = 0x00020000U,
+ CF_PIS_DEF_IEEE = 0x00030000U,
+ CF_PIS_DEF_RESERVED = 0x00040000U
};
+/**
+ * @brief Core Feature (0001h)
+ * This Feature identifies a Drive that supports functionality common
+ * to all devices.
+ */
+struct cdf_core {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ /* The Additional Length field shall be set to 8. */
+ __u8 length;
+ /**
+ * The Physical Interface Standard field shall be set to a value
+ * selected from @enum cdf_cf_pis
+ * It is possible that more than one physical interface exists
+ * between the Host and Drive, e.g., an IEEE1394 Host connecting
+ * to an ATAPI bridge to an ATAPI Drive. The Drive may not be aware
+ * of interfaces beyond the ATAPI.
+ */
+ __be32 interface;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved2 : 6;
+ /**
+ * The INQ2 bit permits the Drive to indicate support for certain
+ * features of the INQUIRY command. If INQ2 is set to one, the
+ * Drive shall support validation of EVPD, Page Code, and the
+ * 16-bit Allocation Length fields
+ */
+ __u8 inq2 : 1;
+ /**
+ * DBE (Device Busy Event) shall be set to one.
+ */
+ __u8 dbevent : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 dbevent : 1;
+ __u8 inq2 : 1;
+ __u8 reserved2 : 6;
+#endif
+
+ __u8 reserved3[3];
+} __packed;
+
+/**
+ * @brief Morphing Feature (0002h)
+ * This Feature identifies the ability of the Drive to notify
+ * A Host about operational changes and accept Host requests to
+ * prevent operational changes.
+ */
+struct cdf_morphing {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved2 : 6;
+ __u8 ocevent : 1;
+ __u8 async : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 async : 1;
+ __u8 ocevent : 1;
+ __u8 reserved2 : 6;
+#endif
+
+ __u8 reserved3[3];
+} __packed;
+
+/**
+ * @brief Removable Medium: Loading Mechanism Types
+ */
+enum cdf_removable_media_lmt {
+ CDF_LMT__CADDY_SLOT_TYPE,
+ CDF_LMT__TRAY_TYPE,
+ CDF_LMT__POP_UP_TYPE,
+ CDF_LMT__RESERVED1,
+ CDF_LMT__EMBEDDED_INDIVIDUALLY,
+ CDF_LMT__EMBEDDED_MAGAZINE,
+ CDF_LMT__RESERVED2,
+};
+
+/**
+ * @brief Removable Medium Feature (0003h)
+ *
+ * This Feature identifies a Drive that has a medium that is removable.
+ * Media shall be considered removable if it is possible to remove it
+ * from the loaded position, i.e., a single mechanism changer, even if
+ * the media is captive to the changer.
+ *
+ * The Drive shall generate Events for media changes.
+ * Event Notification Class 4 (Media Events) shall be supported. This
+ * includes reporting user requests to load/eject the medium.
+ */
+struct cdf_removable_medium {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+ /* The Additional Length field shall be set to 4. */
+ __u8 length;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ /**
+ * The Loading Mechanism Type field shall be set according to
+ * @enum cdf_removable_media_lmt
+ */
+ __u8 mechanism : 3;
+ /**
+ * If the Load bit is set to zero, the Drive is unable to load
+ * the medium or cartridge via the START STOP UNIT command with
+ * the LoEj bit set to one, e.g. the tray type loading mechanism
+ * that is found in many portable PCs.
+ * If the Load bit is set to one, the Drive is able to load the
+ * medium or cartridge.
+ */
+ __u8 load : 1;
+ /**
+ * The Eject bit, when set to zero, indicates that the device is
+ * unable to eject the medium or magazine via the normal START STOP UNIT
+ * command with the LoEj bit set.
+ * When set to one, indicates that the device is able to eject
+ * the medium or magazine.
+ */
+ __u8 eject : 1;
+ /**
+ * The Pvnt Jmpr bit, when set to zero, shall indicate that the
+ * Prevent Jumper is present.
+ * When set to one, the Prevent Jumper is not present.
+ * The Pvnt Jmpr bit shall not change state, even if the physical
+ * jumper is added or removed during operation.
+ */
+ __u8 prvnt_jmp : 1;
+ __u8 reserved2 : 1;
+ /**
+ * If Lock is set to zero, there is no locking mechanism for locking
+ * the medium into the Drive. If Lock is set to one, the Drive is
+ * capable of locking the media into the Drive.
+ */
+ __u8 lock : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 lock : 1;
+ __u8 reserved2 : 1;
+ __u8 prvnt_jmp : 1;
+ __u8 eject : 1;
+ __u8 load : 1;
+ __u8 mechanism : 3;
+#endif
+
+ __u8 reserved3[3];
+} __packed;
+
+/**
+ * @brief Random Readable Feature (0010h)
+ *
+ * This Feature identifies a Drive that is able to read data from logical
+ * blocks referenced by Logical Block Addresses, but not requiring that
+ * either the addresses or the read sequences occur in any particular order.
+ */
+struct cdf_random_readable {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+ /* The Additional Length field shall be set to 8. */
+ __u8 length;
+ /**
+ * The Logical Block Size shall be set to the number of bytes per
+ * logical block.
+ */
+ __be32 block_size;
+ /**
+ * The Blocking field shall indicate the number of logical blocks per
+ * device readable unit.
+ * If there is more than one Blocking on the medium possible,
+ * the Blocking field shall be set to zero.
+ */
+ __be16 blocking;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved2 : 7;
+ /**
+ * The PP (Page Present) bit, when set to zero, shall indicate that
+ * the Read/Write Error Recovery mode page may not be present.
+ * When set to one, shall indicate that the Read/Write Error Recovery
+ * mode page is present.
+ */
+ __u8 pp : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 pp : 1;
+ __u8 reserved2 : 7;
+#endif
+
+ __u8 reserved3;
+} __packed;
+
+/*
+ * Multi-read Feature (001Dh)
+ * The Drive shall conform to the OSTA Multi-Read
+ * specification 1.00, with the exception of CD Play
+ * capability (the CD Audio Feature is not required).
+ */
+struct cdf_multi_read {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+} __packed;
+
+/*
+ * CD Read Feature (001Eh)
+ * This Feature identifies a Drive that is able to read
+ * CD specific information from the media and is able
+ * to read user data from all types of CD sectors.
+ */
+struct cdf_cd_read {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+#if defined(__BIG_ENDIAN_BITFIELD)
+
+ /*
+ * If DAP is set to one, the READ CD and READ CD MSF
+ * commands support the DAP bit in bit 1, byte 1
+ * of the CDB.
+ */
+ __u8 dap : 1;
+ __u8 reserved2 : 5;
+
+ /*
+ * The C2 Flags, when set to one, indicates the Drive
+ * supports the C2 Error Pointers.
+ * When set to zero the Drive does not support
+ * C2 Error Pointers.
+ */
+ __u8 c2flags : 1;
+ /*
+ * The CD-Text bit, when set to one, indicates the Drive
+ * supports Format Code 5h of the READ TOC/PMA/ATIP
+ * command.
+ * When set to zero, CD-Text is not supported.
+ */
+ __u8 cdtext : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 cdtext : 1;
+ __u8 c2flags : 1;
+ __u8 reserved2 : 5;
+ __u8 dap : 1;
+#endif
+
+ __u8 reserved3[3];
+} __packed;
+
+/*
+ * DVD Read Feature (001Fh)
+ * This Feature identifies a Drive that is able to read DVD
+ * specific information from the media.
+ */
+struct cdf_dvd_read {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved2 : 7;
+ /*
+ * If MULTI110 is set to one, the Drive shall
+ * be compliant with the DVD Multi Drive Read-only
+ * specifications as defined in [DVD-Ref8].
+ */
+ __u8 multi110 : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 multi110 : 1;
+ __u8 reserved2 : 7;
+#endif
+ __u8 reserved3;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved4 : 6;
+ /*
+ * If the DVD-RW Dual Layer (Dual-RW) bit is set to one,
+ * the Drive is able to read DVD-RW DL media that has the
+ * Complete state.
+ * If the Dual-RW bit is set to zero, the Drive is unable
+ * to read the DVD-RW DL media.
+ */
+ __u8 dualrw : 1;
+ /*
+ * If the DVD-R Dual Layer (Dual-R) bit is set to one,
+ * the Drive shall support reading all recording modes
+ * (i.e., Sequential recording and Layer Jump recording modes)
+ * of DVD-R DL discs.
+ * The Drive shall support Remapping on DVD-R DL discs.
+ */
+ __u8 dualr : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 dualr : 1;
+ __u8 dualrw : 1;
+ __u8 reserved4 : 6;
+#endif
+
+ __u8 reserved5;
+} __packed;
+
+/**
+ * @brief DVD+R Feature (002Bh)
+ * The presence of the DVD+R Feature indicates that the Drive is
+ * capable of reading a recorded DVD+R disc that is written according
+ * to [DVD+Ref1].
+ * Specifically, this includes the capability of reading DCBs.
+ */
+struct cdf_dvd_plus_r {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved2 : 7;
+ /**
+ * If the Write bit is set to one, then the Drive is also capable
+ * of writing DVD+R discs according to [DVD+Ref1].
+ */
+ __u8 write : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 write : 1;
+ __u8 reserved2 : 7;
+#endif
+
+ __u8 reserved3[3];
+} __packed;
+
+/**
+ * @brief CD Track at Once Feature (002Dh)
+ * This Feature identifies a Drive that is able to write data to
+ * a CD track.
+ */
+struct cdf_cd_track_at_once {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved2 : 1;
+ /**
+ * The BUF bit, if set to 1, shall indicate that the Drive
+ * is capable of zero loss linking.
+ */
+ __u8 buf : 1;
+ __u8 reserved1 : 1;
+ /**
+ * The R-W Raw bit, if set to 1, shall indicate that the Drive
+ * supports writing R-W Sub code in the Raw mode.
+ * The R-W Sub-code bit shall be set if this bit is set.
+ */
+ __u8 rw_raw : 1;
+ /**
+ * The R-W Pack bit, if set to 1, shall indicate that the Drive
+ * supports writing R-W Sub code in the Packed mode.
+ * The R-W Sub-code bit shall be set if this bit is set.
+ */
+ __u8 rw_pack : 1;
+ /**
+ * The Test Write bit indicates that the Drive is able to
+ * perform test writes.
+ */
+ __u8 test_write : 1;
+ /**
+ * The CD-RW bit indicates support for overwriting a Track at
+ * Once track with another.
+ */
+ __u8 cd_rw : 1;
+ /**
+ * The R-W Sub-code bit indicates that the Drive is able to
+ * record the R-W Sub-channels with user supplied data.
+ */
+ __u8 rw_subcode : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 rw_subcode : 1;
+ __u8 cd_rw : 1;
+ __u8 test_write : 1;
+ __u8 rw_pack : 1;
+ __u8 rw_raw : 1;
+ __u8 reserved3 : 1;
+ __u8 buf : 1;
+ __u8 reserved2 : 1;
+#endif
+
+ __u8 reserved4;
+ /**
+ * The data type references to the
+ * "Incremental Streaming Writable Feature"
+ */
+ __be16 data_type_supported;
+} __packed;
+
+/**
+ * @brief BD Read Feature (0040h)
+ * This Feature identifies a Drive that is able to read control
+ * structures and user data from the BD disc.
+ */
+struct cdf_bd_read {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+
+ __u8 reserved2[4];
+ /**
+ * If the Version K bit (K = 0...15) of the Class M (M = 0...3)
+ * bit map is set to zero, the Drive claims no read capabilities
+ * for BD-R(E)(ROM) discs of Class M and Version K.
+ * If the Version K bit of Class M is set to one, the Drive is
+ * able to read BD-RE discs of class M and Version K.
+ *
+ */
+
+ /* Class M (M = 0..3) BD-RE Read Support */
+ __be16 class0_bdre_read_support;
+ __be16 class1_bdre_read_support;
+ __be16 class2_bdre_read_support;
+ __be16 class3_bdre_read_support;
+ /* Class M (M = 0..3) BD-R Read Support */
+ __be16 class0_bdr_read_support;
+ __be16 class1_bdr_read_support;
+ __be16 class2_bdr_read_support;
+ __be16 class3_bdr_read_support;
+ /* Class M (M = 0..3) BD-ROM Read Support */
+ __be16 class0_bdrom_read_support;
+ __be16 class1_bdrom_read_support;
+ __be16 class2_bdrom_read_support;
+ __be16 class3_bdrom_read_support;
+} __packed;
+
+/**
+ * @brief Power Management Feature (0100h)
+ * This Feature identifies a Drive that is able to perform Host and
+ * Drive directed power management.
+ */
+struct cdf_power_mgmt {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+} __packed;
+
+/**
+ * @brief Real Time Streaming Feature (0107h)
+ * This Feature identifies a Drive that is able to perform reading
+ * and writing within Host specified (and Drive verified) performance
+ * ranges. This Feature also indicates whether the Drive supports the
+ * Stream playback operation.
+ */
+struct cdf_rt_streaming {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ __u8 length;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved2 : 3;
+ /**
+ * The Read Buffer Capacity Block (RBCB) bit indicates that the
+ * Drive supports the READ_BUFFER_CAPACITY command and its Block
+ * bit.
+ */
+ __u8 rbcb : 1;
+ /**
+ * The Set CD Speed (SCS) bit of one indicates that the Drive
+ * supports the SET_CD_SPEED command. Otherwise, the Drive does not
+ * support the SET_CD_SPEED command.
+ */
+ __u8 scs : 1;
+ /**
+ * The mode page 2A (MP2A) bit of one indicates that the MM
+ * Capabilities & Mechanical Status mode page (2Ah) with the Drive
+ * Write Speed Performance Descriptor Blocks is supported.
+ * Otherwise, the MM Capabilities & Mechanical Status mode
+ * page (2Ah), with the Drive Write Speed Performance Descriptor
+ * Blocks are not supported by the Drive.
+ */
+ __u8 mp2a : 1;
+ /**
+ * A Write Speed Performance Descriptor (WSPD) bit of one indicates
+ * that the Drive supports the Write Speed (Type field = 03h) data
+ * of GET PERFORMANCE command and the WRC field of SET STREAMING
+ * command. This bit shall be set to one, if Drive supports writing
+ * speed selection.
+ */
+ __u8 wspd : 1;
+ /**
+ * A Stream Writing (SW) bit of one indicates that the Drive
+ * supports the Stream recording operation. A SW bit of zero
+ * indicates that the Drive may not support the Stream recording
+ * operation.
+ */
+ __u8 sw : 1;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 sw : 1;
+ __u8 wspd : 1;
+ __u8 mp2a : 1;
+ __u8 scs : 1;
+ __u8 rbcb : 1;
+ __u8 reserved2 : 3;
+#endif
+
+ __u8 reserved3[3];
+} __packed;
+
+/**
+ * @brief Disc Control Blocks (DCBs) Feature (010Ah)
+ *
+ * This Feature identifies a Drive that is able to read and/or write
+ * DCBs from or to the media.
+ */
+struct cdf_dcbs {
+ __be16 code;
+
+ struct cdb_ft_vpc_byte vpc;
+
+ /**
+ * The Additional Length field shall be set to N * 4, where n is
+ * the number of Supported DCB entries. The Supported DCB entry
+ * n fields shall each contain the Content Descriptor of a
+ * supported DCB.
+ * Entries shall be sorted in ascending order.
+ */
+ __u8 length;
+
+ /**
+ * Non supported read and/or write the DCBs blocks.
+ */
+ __be32 supported_dcb_entry[0];
+};
+
+/*
+ * feature codes list
+ */
+
+/* A list of all Profiles supported by the Drive*/
+#define CDF_PROFILE_LIST_CODE 0x0000
+/* Mandatory behavior for all devices */
+#define CDF_CORE 0x0001
+
+#define CDF_MORPHING_CODE 0x0002
+/* The medium may be removed from the device */
+#define CDF_REMOVEBLE_MEDIA 0x0003
+#define CDF_RANDOM_READ 0x0010
+/* The Drive is able to read all CD media types; based on OSTA MultiRead */
+#define CDF_MULTI_READ 0x001D
+/* The ability to read CD specific structures */
+#define CDF_CD_READ 0x001E
+/* The ability to read DVD specific structures*/
+#define CDF_DVD_READ 0x001F
+/* Write support for randomly addressed writes */
+#define CDF_RWRT 0x0020
+/* Write support for sequential recording */
+#define CDF_INC_STREAM_WR 0x0021
+/* Hardware Defect Management */
+#define CDF_HWDM 0x0024
+/* The ability to recognize and read and optionally write MRW formatted media */
+#define CDF_MRW 0x0028
+/* The ability to read DVD+R recorded media formats */
+#define CDF_DVD_R 0x002B
+/* Ability to write CD with Track at Once recording */
+#define CDF_CD_TRACK_ONCE 0x002D
+/* The ability to read control structures and user data from a BD disc */
+#define CDF_BD_READ 0x0040
+/* The ability to write control structures and user data to certain BD discs */
+#define CDF_BD_WRITE 0x0041
+/* Host and device directed power management */
+#define CDF_POWER_MGMT 0x0100
+/* Ability to perform DVD CSS/CPPM authentication and RPC */
+#define CDF_DVD_CSS 0x0106
+/* Ability to read and write using Host requested performance parameters */
+#define CDF_REAL_TIME_STREAM 0x0107
+/* The ability to read and/or write DCBs*/
+#define CDF_DCBS 0x010A
+
/**
* The READ TOC/PMA/ATIP format field values
*/
--
2.32.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/6] FMS: Add SCSI Read Disc Information command.
2021-06-26 21:18 [PATCH 0/6] USB-gadget: mass-storage: Support DVD-like images Igor Kononenko
` (4 preceding siblings ...)
2021-06-26 21:18 ` [PATCH 5/6] FMS: Add the SCSI Get Configuration command Igor Kononenko
@ 2021-06-26 21:18 ` Igor Kononenko
5 siblings, 0 replies; 22+ messages in thread
From: Igor Kononenko @ 2021-06-26 21:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, James E.J. Bottomley,
Martin K. Petersen, Jens Axboe
Cc: openbmc, linux-usb, Igor Kononenko, linux-kernel, linux-scsi
Adds the SCSI Read Disc Information command, which should be given for
multimedia device consumers to retrieve the DVD/BD disk information
about:
* Total tracks contained at the disc Total and active sessions Border
* status(incomplete, damaged, etc.)
This information is wanted for supporting the DVD-ROM and BD-ROM
devices.
End-user-impact: Now, multimedia device consumers have a way to retrieve
the multimedia disk information.
Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
---
drivers/usb/gadget/function/f_mass_storage.c | 37 ++++
include/scsi/scsi_proto.h | 1 +
include/uapi/linux/cdrom.h | 195 +++++++++++++++++--
3 files changed, 212 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 7e736e5594f9..d3d8a806b5e6 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -1932,6 +1932,40 @@ static void send_status(struct fsg_common *common)
return;
}
+static int do_read_disc_info(struct fsg_common *common, struct fsg_buffhd *bh)
+{
+ struct fsg_lun *curlun = common->curlun;
+ struct cdb_disc_info *cdb = (struct cdb_disc_info *)common->cmnd;
+ disc_information *info = (disc_information *)bh->buf;
+
+ if (cdb->type != DISC_TYPE_STANDARD) {
+ LERROR(curlun,
+ "Unsupported disc information type(%02Xh) requested\n",
+ cdb->type);
+ return -EINVAL;
+ }
+ memset(info, 0, sizeof(disc_information));
+ info->disc_information_length = cpu_to_be16(
+ sizeof(*info) - sizeof(info->disc_information_length));
+
+ info->border_status = DISC_LAST_SESS_COMPLETE;
+ info->disc_status = DISC_STATUS_FINALIZED;
+
+ /* We only support one session per disk */
+ info->n_first_track = 1;
+ info->n_sessions_lsb = 1;
+ info->first_track_lsb = 1;
+ info->last_track_lsb = 1;
+
+ /* Setting the unrestricted use because we only support (CD/DVD/BD)-ROM */
+ info->uru = 1;
+
+ info->disc_type = DISC_FIELD_DA_ROM;
+
+ common->data_size_to_handle = sizeof(*info);
+ return 0;
+}
+
/**
* Attempts to guess medium type by looking at the length of the disc layout.
*/
@@ -2253,6 +2287,8 @@ static struct cdb_command_check cdb_checker_table[] = {
{ CDB_REG_CHECKER(TEST_UNIT_READY, 6, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
0x0000, MEDIUM_REQUIRED) },
+ { CDB_REG_NO_CHECKER(READ_DISC_INFORMATION, CDB_SIZE_FIELD_7,
+ DATA_DIR_TO_HOST, MEDIUM_REQUIRED) },
{ CDB_REG_NO_CHECKER(GET_CONFIGURATION, CDB_SIZE_FIELD_7,
DATA_DIR_TO_HOST, MEDIUM_REQUIRED) },
@@ -2286,6 +2322,7 @@ static struct cdb_handler cdb_handlers_table[] = {
{ CDB_REG_HANDLER(SYNCHRONIZE_CACHE, &do_synchronize_cache) },
{ CDB_REG_HANDLER(TEST_UNIT_READY, NULL) },
+ { CDB_REG_HANDLER_BUFFHD(READ_DISC_INFORMATION, &do_read_disc_info) },
{ CDB_REG_HANDLER_BUFFHD(GET_CONFIGURATION, &do_get_configuration) },
/*
* Although optional, this command is used by MS-Windows. We
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index 6b2a8ee1f0a3..6728fcbd73e4 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -77,6 +77,7 @@
#define GET_EVENT_STATUS_NOTIFICATION 0x4a
#define LOG_SELECT 0x4c
#define LOG_SENSE 0x4d
+#define READ_DISC_INFORMATION 0x51
#define XDWRITEREAD_10 0x53
#define MODE_SELECT_10 0x55
#define RESERVE_10 0x56
diff --git a/include/uapi/linux/cdrom.h b/include/uapi/linux/cdrom.h
index 442693fdc059..460377e1a532 100644
--- a/include/uapi/linux/cdrom.h
+++ b/include/uapi/linux/cdrom.h
@@ -816,51 +816,204 @@ struct rwrt_feature_desc {
__u8 reserved3;
};
+/* Disc Information Data Types */
+#define DISC_TYPE_STANDARD (0x00U)
+#define DISC_TYPE_TRACK (0x01U)
+#define DISC_TYPE_POW (0x02U)
+
+/* Disc Status */
+#define DISC_STATUS_EMPTY (0x00U)
+#define DISC_STATUS_INCOMPLETE (0x01U)
+#define DISC_STATUS_FINALIZED (0x02U)
+#define DISC_STATUS_OTHER (0x03U)
+
+/* State of Last Session */
+#define DISC_LAST_SESS_EMPTY (0x00U)
+#define DISC_LAST_SESS_INCOMPLETE (0x01U)
+#define DISC_LAST_SESS_DAMAGED (0x02U)
+#define DISC_LAST_SESS_COMPLETE (0x03U)
+
+/* Background Format Status Codes */
+#define DISC_BACK_FMT_NEITHER (0x00U)
+#define DISC_BACK_FMT_STARTED (0x01U)
+#define DISC_BACK_FMT_PROGRESS (0x02U)
+#define DISC_BACK_FMT_COMPLETED (0x03U)
+
+/* Disc Type Field */
+#define DISC_FIELD_DA_ROM (0x00U)
+#define DISC_FIELD_I (0x10U)
+#define DISC_FIELD_ROM_XA (0x20U)
+#define DISC_FIELD_UNDEF (0xFFU)
+
+/**
+ * @brief The READ DISC INFORMATION CDB(0051h)
+ * The READ DISC INFORMATION command allows the Host to request information about
+ * the currently mounted MM disc.
+ */
+struct cdb_disc_info {
+ __u8 code;
+
+#if defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved1 : 5;
+ /**
+ * When a disc is present, Data Type defines the specific information requested
+ */
+ __u8 type : 3;
+#elif defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 type : 3;
+ __u8 reserved1 : 5;
+#endif
+
+ __u8 reserved2[5];
+
+ __be16 length;
+
+ __u8 control;
+} __packed;
+
typedef struct {
__be16 disc_information_length;
#if defined(__BIG_ENDIAN_BITFIELD)
- __u8 reserved1 : 3;
- __u8 erasable : 1;
- __u8 border_status : 2;
- __u8 disc_status : 2;
+ /**
+ * The Disc Information Data Type field shall be set to the reported
+ * Disc Information Type
+ */
+ __u8 info_data_type : 3;
+ /**
+ * The Erasable bit, when set to one, indicates that CD-RW, DVD-RAM, DVD-RW, DVD+RW,
+ * HD DVD-RAM, or BD-RE media is present and the Drive is capable of writing the media.
+ * If the Erasable bit is set to zero, then either the medium is not erasable or the
+ * Drive is unable to write the media.
+ */
+ __u8 erasable : 1;
+ /**
+ * The State of Last Session field specifies the recorded state of the last
+ * session, regardless of the number of sessions on the disc.
+ */
+ __u8 border_status : 2;
+ /* The Disc Status field indicates the recorded status of the disc */
+ __u8 disc_status : 2;
#elif defined(__LITTLE_ENDIAN_BITFIELD)
- __u8 disc_status : 2;
- __u8 border_status : 2;
- __u8 erasable : 1;
- __u8 reserved1 : 3;
+ __u8 disc_status : 2;
+ __u8 border_status : 2;
+ __u8 erasable : 1;
+ __u8 info_data_type : 3;
#else
#error "Please fix <asm/byteorder.h>"
#endif
+ /**
+ * The Number of First Track on Disc is the track number of the Logical Track that
+ * contains LBA 0
+ */
__u8 n_first_track;
__u8 n_sessions_lsb;
+ /**
+ * First Track Number in Last Session (bytes 5 & 10) is the track number of the
+ * first Logical Track in the last session.
+ * This includes the incomplete logical track.
+ */
__u8 first_track_lsb;
+ /**
+ * Last Track Number in Last Session (bytes 6 & 11) is the track number of the last
+ * Logical Track in the last session.
+ * This includes the incomplete logical track.
+ */
__u8 last_track_lsb;
#if defined(__BIG_ENDIAN_BITFIELD)
- __u8 did_v : 1;
- __u8 dbc_v : 1;
- __u8 uru : 1;
- __u8 reserved2 : 2;
- __u8 dbit : 1;
- __u8 mrw_status : 2;
+ /**
+ * The DID_V (Disc ID Valid) bit, when set to one, indicates that the Disc
+ * Identification field is valid
+ */
+ __u8 did_v : 1;
+ /**
+ * The DBC_V (Disc Bar Code Valid bit, when set to one, indicates that the Disc Bar
+ * Code field (bytes 24 through 31) is valid
+ */
+ __u8 dbc_v : 1;
+ /**
+ * The URU (Unrestricted Use Disc) bit may be zero for special use CD-R, CD-RW,
+ * or DVD-R, medium.
+ * For all other media types, URU shall be set to one. When URU is zero, the mounted
+ * disc is defined for restricted use.
+ */
+ __u8 uru : 1;
+ /**
+ * DAC_V indicates the validity of the Disc Application Code in byte 32. If DAC_V is
+ * set to zero, then the Disc Application Code is not valid. If DAC_V is set to one,
+ * the Disc Application Code is valid.
+ */
+ __u8 dac_v: 1;
+ __u8 reserved2 : 1;
+ /**
+ * If the disc is MRW formatted or MRW formatting (state = 01b, 10b, or 11b),
+ * then bit 2 of byte 7 (Dbit) is a copy of the “dirty bit” from the defect table.
+ * If Dbit is set to zero, then the MRW structures are current.
+ * If Dbit is set to one, then the MRW structures may not be current.
+ * When BG format status = 00b, Dbit shall be set to zero.
+ */
+ __u8 dbit : 1;
+ /**
+ * The BG format status is the background format status of the mounted disc.
+ * Drives that report the Formattable Feature and either the MRW Feature or the DVD+RW
+ * Feature, or both are required to implement Background format.
+ * For all other Drives, this field shall be @param DISC_BACK_FMT_NEITHER.
+ */
+ __u8 mrw_status : 2;
#elif defined(__LITTLE_ENDIAN_BITFIELD)
- __u8 mrw_status : 2;
- __u8 dbit : 1;
- __u8 reserved2 : 2;
- __u8 uru : 1;
- __u8 dbc_v : 1;
- __u8 did_v : 1;
+ __u8 mrw_status : 2;
+ __u8 dbit : 1;
+ __u8 reserved2 : 1;
+ __u8 dac_v: 1;
+ __u8 uru : 1;
+ __u8 dbc_v : 1;
+ __u8 did_v : 1;
#endif
+ /**
+ * The Disc Type field is associated only with CD media type
+ */
__u8 disc_type;
__u8 n_sessions_msb;
__u8 first_track_msb;
__u8 last_track_msb;
+
+ /**
+ * For CD-R/RW, the Disc Identification number recorded in the PMA is returned.
+ * The Disc Identification Number is recorded in the PMA as a six-digit BCD number.
+ * It is returned in the Disc Information Block as a 32 bit binary integer.
+ * This value should be zero filled for all other media types.
+ */
__u32 disc_id;
+ /**
+ * The Last Session Lead-in Start Address field is dependent on medium and
+ * recorded status.
+ */
__u32 lead_in;
+ /**
+ * The Last Possible Lead-out Start Address field is dependent on medium and
+ * recorded status.
+ */
__u32 lead_out;
+ /**
+ * For CD, the Disc Bar Code field contains the hexadecimal value of the bar code
+ * if the Drive has the ability to read Disc Bar Code and a bar code is present.
+ * For all other media this field should be set to zeros.
+ */
__u8 disc_bar_code[8];
+ /**
+ *
+ */
__u8 reserved3;
+ /**
+ * The Number of OPC Tables field is the number of OPC tables that follow this field.
+ * If OPC has not been determined for the currently mounted medium, the Number of
+ * OPC Tables field is set to zero.
+ * The Number of OPC Tables represents the number of disc speeds for which the OPC
+ * values are known.
+ * Since each OPC Table is 8 bytes in length, then the number of bytes that follow
+ * the Number of OPC Tables field is 8 x Number of OPC Tables.
+ */
__u8 n_opc;
-} disc_information;
+} __packed disc_information;
typedef struct {
__be16 track_information_length;
--
2.32.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling
2021-06-26 21:18 ` [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling Igor Kononenko
@ 2021-06-26 23:29 ` kernel test robot
2021-06-27 14:23 ` Alan Stern
1 sibling, 0 replies; 22+ messages in thread
From: kernel test robot @ 2021-06-26 23:29 UTC (permalink / raw)
To: Igor Kononenko, Felipe Balbi, Greg Kroah-Hartman
Cc: Igor Kononenko, openbmc, linux-usb, kbuild-all, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 17714 bytes --]
Hi Igor,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next linus/master v5.13-rc7 next-20210625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Igor-Kononenko/usb-gadget-mass-storage-Improve-the-signature-of-SCSI-handler-function/20210627-061851
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/92c07dc68c51fab87517c2453d8f249c2565deed
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Igor-Kononenko/usb-gadget-mass-storage-Improve-the-signature-of-SCSI-handler-function/20210627-061851
git checkout 92c07dc68c51fab87517c2453d8f249c2565deed
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1948:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1948 | { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[6].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1948:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1948 | { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1950:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1950 | { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[7].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1950:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1950 | { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1952:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1952 | { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[8].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1952:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1952 | { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1973:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1973 | { CDB_REG_CHECKER_BLK(VERIFY, 10, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[17].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1973:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1973 | { CDB_REG_CHECKER_BLK(VERIFY, 10, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1975:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1975 | { CDB_REG_CHECKER_BLK(WRITE_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_FROM_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[18].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1975:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1975 | { CDB_REG_CHECKER_BLK(WRITE_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_FROM_HOST,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1977:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1977 | { CDB_REG_CHECKER_BLK(WRITE_10, 10, CDB_SIZE_FIELD_7,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[19].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1977:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1977 | { CDB_REG_CHECKER_BLK(WRITE_10, 10, CDB_SIZE_FIELD_7,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1979:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1979 | { CDB_REG_CHECKER_BLK(WRITE_12, 12, CDB_SIZE_FIELD_6,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[20].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1979:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1979 | { CDB_REG_CHECKER_BLK(WRITE_12, 12, CDB_SIZE_FIELD_6,
| ^~~~~~~~~~~~~~~~~~~
--
drivers/usb/gadget/function/f_mass_storage.c: In function 'invalidate_sub':
drivers/usb/gadget/function/f_mass_storage.c:1084:16: warning: variable 'rc' set but not used [-Wunused-but-set-variable]
1084 | unsigned long rc;
| ^~
drivers/usb/gadget/function/f_mass_storage.c: At top level:
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1948:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1948 | { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[6].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1948:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1948 | { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1950:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1950 | { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[7].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1950:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1950 | { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1952:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1952 | { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[8].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1952:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1952 | { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1973:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1973 | { CDB_REG_CHECKER_BLK(VERIFY, 10, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[17].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1973:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1973 | { CDB_REG_CHECKER_BLK(VERIFY, 10, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1975:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1975 | { CDB_REG_CHECKER_BLK(WRITE_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_FROM_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[18].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1975:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1975 | { CDB_REG_CHECKER_BLK(WRITE_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_FROM_HOST,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1977:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1977 | { CDB_REG_CHECKER_BLK(WRITE_10, 10, CDB_SIZE_FIELD_7,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[19].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1977:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1977 | { CDB_REG_CHECKER_BLK(WRITE_10, 10, CDB_SIZE_FIELD_7,
| ^~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/function/f_mass_storage.c:310:23: warning: initialized field overwritten [-Woverride-init]
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1979:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1979 | { CDB_REG_CHECKER_BLK(WRITE_12, 12, CDB_SIZE_FIELD_6,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:310:23: note: (near initialization for 'cdb_checker_table[20].do_check_command')
310 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:1979:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
1979 | { CDB_REG_CHECKER_BLK(WRITE_12, 12, CDB_SIZE_FIELD_6,
| ^~~~~~~~~~~~~~~~~~~
vim +310 drivers/usb/gadget/function/f_mass_storage.c
242
243 /**
244 * @brief The handler of incoming CDB command
245 * @param cmd - SCSI command number
246 * @param callback - The callback of handle the incoming command
247 */
248 #define CDB_REG_HANDLER(cmd, callback) \
249 .command = (cmd), .do_command = (callback), \
250 .type = CDB_HANDLER_COMMON, .name = (#cmd)
251
252 /**
253 * @brief The handler of incoming CDB command
254 * @param cmd - SCSI command nubmer with fsg buffhd
255 * @param callback - The callback of handle the incoming command
256 */
257 #define CDB_REG_HANDLER_BUFFHD(cmd, callback) \
258 .command = (cmd), .do_command_with_buffhd = (callback), \
259 .type = CDB_HANDLER_FSG_BUFFHD, .name = (#cmd)
260
261 /**
262 * @see CDB_REG_CHECKER_DS
263 * @details Register CDB command without additional check handler.
264 */
265 #define CDB_REG_NO_CHECKER(cmd, si, dir, req) \
266 .command = (cmd), .direction = (dir), .size_index = (si), \
267 .medium_required = (req), .do_check_command = NULL,
268
269 /**
270 * @brief Register the CDB command checker, which checks an incoming command
271 * by specified criteria.
272 * This validator will take care of the specified data size (DS)
273 *
274 * @param cmd - SCSI command nubmer
275 * @param s - CDB command size in bytes
276 * @param si - The CDB command might have the recommended response size.
277 * This field indicates the size field index in the input CDB command
278 * buffer
279 * @param dir - Direction of data transfer of requested CDB command
280 * @param mask - Mask of relevant bytes in the input command buffer.
281 * The ordinal number of a bit in the mask indicates that a byte in the
282 * CDB command buffer might be present.
283 * If that ordinal number bit equals zero, only a zero value must be
284 * present in this original byte.
285 * @param req - Indicates that medium MUST be present or might be optional
286 * @param ds - If @param SI member is equal to @enum CDB_SIZE_MANUAL, than this
287 * field indicates the custom response buffer size
288 */
289 #define CDB_REG_CHECKER_DS(cmd, s, si, dir, mask, req, ds) \
290 .command = (cmd), .size = (s), .size_index = (si), .direction = (dir), \
291 .valid_bytes_bitmask = (mask), .medium_required = (req), \
292 .data_size_manual = (ds), .do_check_command = &check_command
293
294 /**
295 * @see CDB_REG_CHECKER_DS
296 * @details The data size is zero.
297 * This macro can't be used with the @enum CDB_SIZE_MANUAL
298 */
299 #define CDB_REG_CHECKER(cmd, s, si, dir, mask, req) \
300 CDB_REG_CHECKER_DS(cmd, s, si, dir, mask, req, 0)
301
302 /**
303 * @see CDB_REG_CHECKER_DS
304 * @details The checker which registried by this macros will validate the input
305 * data size in blocks.
306 * Block size specified by MSF interface type, in the curlun->blksize.
307 */
308 #define CDB_REG_CHECKER_BLK(cmd, s, si, dir, mask, req) \
309 CDB_REG_CHECKER_DS(cmd, s, si, dir, mask, req, 0), \
> 310 .do_check_command = &check_command_size_in_blocks
311
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60468 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] FMS: Add the SCSI Get Configuration command.
2021-06-26 21:18 ` [PATCH 5/6] FMS: Add the SCSI Get Configuration command Igor Kononenko
@ 2021-06-27 0:44 ` kernel test robot
2021-06-27 4:42 ` kernel test robot
2021-06-28 9:53 ` Christoph Hellwig
2 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2021-06-27 0:44 UTC (permalink / raw)
To: Igor Kononenko, Jens Axboe, Felipe Balbi, Greg Kroah-Hartman,
James E.J. Bottomley, Martin K. Petersen
Cc: Igor Kononenko, openbmc, linux-usb, linux-kernel, linux-ide, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 16918 bytes --]
Hi Igor,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next linus/master balbi-usb/testing/next v5.13-rc7 next-20210625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Igor-Kononenko/usb-gadget-mass-storage-Improve-the-signature-of-SCSI-handler-function/20210627-061851
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: powerpc-randconfig-r035-20210627 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/db2ec6f1e52293817f380a4875e01c36a4195c19
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Igor-Kononenko/usb-gadget-mass-storage-Improve-the-signature-of-SCSI-handler-function/20210627-061851
git checkout db2ec6f1e52293817f380a4875e01c36a4195c19
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:7,
from include/linux/kernel.h:17,
from include/asm-generic/bug.h:20,
from arch/powerpc/include/asm/bug.h:109,
from include/linux/bug.h:5,
from arch/powerpc/include/asm/mmu.h:147,
from arch/powerpc/include/asm/paca.h:18,
from arch/powerpc/include/asm/current.h:13,
from include/linux/sched.h:12,
from include/linux/blkdev.h:5,
from drivers/usb/gadget/function/f_mass_storage.c:201:
drivers/usb/gadget/function/f_mass_storage.c: In function 'cdr_guess_medium_type':
>> include/linux/kern_levels.h:5:18: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/kern_levels.h:15:20: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
| ^~~~~~~~
include/linux/printk.h:427:9: note: in expansion of macro 'KERN_DEBUG'
427 | printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~
drivers/usb/gadget/function/storage_common.h:24:4: note: in expansion of macro 'pr_debug'
24 | func("%s/%s: " fmt, *(lun)->name_pfx, \
| ^~~~
drivers/usb/gadget/function/storage_common.h:30:34: note: in expansion of macro '_LMSG'
30 | #define LDBG(lun, fmt, args...) _LMSG(pr_debug, lun, fmt, ## args)
| ^~~~~
drivers/usb/gadget/function/f_mass_storage.c:1964:2: note: in expansion of macro 'LDBG'
1964 | LDBG(curlun,
| ^~~~
drivers/usb/gadget/function/f_mass_storage.c:1965:27: note: format string is defined here
1965 | "Disc layout size (%u) exceeds all known media types, assuming BD - ROM !\n",
| ~^
| |
| unsigned int
| %lu
In file included from include/linux/printk.h:7,
from include/linux/kernel.h:17,
from include/asm-generic/bug.h:20,
from arch/powerpc/include/asm/bug.h:109,
from include/linux/bug.h:5,
from arch/powerpc/include/asm/mmu.h:147,
from arch/powerpc/include/asm/paca.h:18,
from arch/powerpc/include/asm/current.h:13,
from include/linux/sched.h:12,
from include/linux/blkdev.h:5,
from drivers/usb/gadget/function/f_mass_storage.c:201:
include/linux/kern_levels.h:5:18: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/kern_levels.h:15:20: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
| ^~~~~~~~
include/linux/printk.h:427:9: note: in expansion of macro 'KERN_DEBUG'
427 | printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~
drivers/usb/gadget/function/storage_common.h:27:4: note: in expansion of macro 'pr_debug'
27 | func("%s: " fmt, (lun)->name, ## args); \
| ^~~~
drivers/usb/gadget/function/storage_common.h:30:34: note: in expansion of macro '_LMSG'
30 | #define LDBG(lun, fmt, args...) _LMSG(pr_debug, lun, fmt, ## args)
| ^~~~~
drivers/usb/gadget/function/f_mass_storage.c:1964:2: note: in expansion of macro 'LDBG'
1964 | LDBG(curlun,
| ^~~~
drivers/usb/gadget/function/f_mass_storage.c:1965:27: note: format string is defined here
1965 | "Disc layout size (%u) exceeds all known media types, assuming BD - ROM !\n",
| ~^
| |
| unsigned int
| %lu
drivers/usb/gadget/function/f_mass_storage.c: At top level:
drivers/usb/gadget/function/f_mass_storage.c:311:23: warning: initialized field overwritten [-Woverride-init]
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2231:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2231 | { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: note: (near initialization for 'cdb_checker_table[6].do_check_command')
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2231:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2231 | { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: warning: initialized field overwritten [-Woverride-init]
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2233:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2233 | { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: note: (near initialization for 'cdb_checker_table[7].do_check_command')
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2233:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2233 | { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: warning: initialized field overwritten [-Woverride-init]
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2235:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2235 | { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: note: (near initialization for 'cdb_checker_table[8].do_check_command')
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2235:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2235 | { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: warning: initialized field overwritten [-Woverride-init]
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2259:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2259 | { CDB_REG_CHECKER_BLK(VERIFY, 10, CDB_NO_SIZE_FIELD, DATA_DIR_NONE,
--
drivers/usb/gadget/function/f_mass_storage.c: In function 'invalidate_sub':
drivers/usb/gadget/function/f_mass_storage.c:1166:16: warning: variable 'rc' set but not used [-Wunused-but-set-variable]
1166 | unsigned long rc;
| ^~
In file included from include/linux/kernel.h:17,
from include/asm-generic/bug.h:20,
from arch/powerpc/include/asm/bug.h:109,
from include/linux/bug.h:5,
from arch/powerpc/include/asm/mmu.h:147,
from arch/powerpc/include/asm/paca.h:18,
from arch/powerpc/include/asm/current.h:13,
from include/linux/sched.h:12,
from include/linux/blkdev.h:5,
from drivers/usb/gadget/function/f_mass_storage.c:201:
drivers/usb/gadget/function/f_mass_storage.c: In function 'cdr_guess_medium_type':
>> include/linux/kern_levels.h:5:18: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:140:10: note: in definition of macro 'no_printk'
140 | printk(fmt, ##__VA_ARGS__); \
| ^~~
include/linux/kern_levels.h:15:20: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
| ^~~~~~~~
include/linux/printk.h:430:12: note: in expansion of macro 'KERN_DEBUG'
430 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~
drivers/usb/gadget/function/storage_common.h:24:4: note: in expansion of macro 'pr_debug'
24 | func("%s/%s: " fmt, *(lun)->name_pfx, \
| ^~~~
drivers/usb/gadget/function/storage_common.h:30:34: note: in expansion of macro '_LMSG'
30 | #define LDBG(lun, fmt, args...) _LMSG(pr_debug, lun, fmt, ## args)
| ^~~~~
drivers/usb/gadget/function/f_mass_storage.c:1964:2: note: in expansion of macro 'LDBG'
1964 | LDBG(curlun,
| ^~~~
drivers/usb/gadget/function/f_mass_storage.c:1965:27: note: format string is defined here
1965 | "Disc layout size (%u) exceeds all known media types, assuming BD - ROM !\n",
| ~^
| |
| unsigned int
| %lu
In file included from include/linux/kernel.h:17,
from include/asm-generic/bug.h:20,
from arch/powerpc/include/asm/bug.h:109,
from include/linux/bug.h:5,
from arch/powerpc/include/asm/mmu.h:147,
from arch/powerpc/include/asm/paca.h:18,
from arch/powerpc/include/asm/current.h:13,
from include/linux/sched.h:12,
from include/linux/blkdev.h:5,
from drivers/usb/gadget/function/f_mass_storage.c:201:
include/linux/kern_levels.h:5:18: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:140:10: note: in definition of macro 'no_printk'
140 | printk(fmt, ##__VA_ARGS__); \
| ^~~
include/linux/kern_levels.h:15:20: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
| ^~~~~~~~
include/linux/printk.h:430:12: note: in expansion of macro 'KERN_DEBUG'
430 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~
drivers/usb/gadget/function/storage_common.h:27:4: note: in expansion of macro 'pr_debug'
27 | func("%s: " fmt, (lun)->name, ## args); \
| ^~~~
drivers/usb/gadget/function/storage_common.h:30:34: note: in expansion of macro '_LMSG'
30 | #define LDBG(lun, fmt, args...) _LMSG(pr_debug, lun, fmt, ## args)
| ^~~~~
drivers/usb/gadget/function/f_mass_storage.c:1964:2: note: in expansion of macro 'LDBG'
1964 | LDBG(curlun,
| ^~~~
drivers/usb/gadget/function/f_mass_storage.c:1965:27: note: format string is defined here
1965 | "Disc layout size (%u) exceeds all known media types, assuming BD - ROM !\n",
| ~^
| |
| unsigned int
| %lu
drivers/usb/gadget/function/f_mass_storage.c: At top level:
drivers/usb/gadget/function/f_mass_storage.c:311:23: warning: initialized field overwritten [-Woverride-init]
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2231:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2231 | { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: note: (near initialization for 'cdb_checker_table[6].do_check_command')
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2231:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2231 | { CDB_REG_CHECKER_BLK(READ_6, 6, CDB_SIZE_FIELD_4, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: warning: initialized field overwritten [-Woverride-init]
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2233:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2233 | { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: note: (near initialization for 'cdb_checker_table[7].do_check_command')
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2233:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2233 | { CDB_REG_CHECKER_BLK(READ_10, 10, CDB_SIZE_FIELD_7, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: warning: initialized field overwritten [-Woverride-init]
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2235:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2235 | { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/gadget/function/f_mass_storage.c:311:23: note: (near initialization for 'cdb_checker_table[8].do_check_command')
311 | .do_check_command = &check_command_size_in_blocks
| ^
drivers/usb/gadget/function/f_mass_storage.c:2235:4: note: in expansion of macro 'CDB_REG_CHECKER_BLK'
2235 | { CDB_REG_CHECKER_BLK(READ_12, 12, CDB_SIZE_FIELD_6, DATA_DIR_TO_HOST,
| ^~~~~~~~~~~~~~~~~~~
vim +5 include/linux/kern_levels.h
314ba3520e513a Joe Perches 2012-07-30 4
04d2c8c83d0e3a Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c83d0e3a Joe Perches 2012-07-30 7
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44957 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] FMS: Add the SCSI Get Configuration command.
2021-06-26 21:18 ` [PATCH 5/6] FMS: Add the SCSI Get Configuration command Igor Kononenko
2021-06-27 0:44 ` kernel test robot
@ 2021-06-27 4:42 ` kernel test robot
2021-06-28 9:53 ` Christoph Hellwig
2 siblings, 0 replies; 22+ messages in thread
From: kernel test robot @ 2021-06-27 4:42 UTC (permalink / raw)
To: Igor Kononenko, Jens Axboe, Felipe Balbi, Greg Kroah-Hartman,
James E.J. Bottomley, Martin K. Petersen
Cc: Igor Kononenko, openbmc, linux-usb, linux-kernel, linux-ide, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 9125 bytes --]
Hi Igor,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next linus/master balbi-usb/testing/next v5.13-rc7 next-20210625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Igor-Kononenko/usb-gadget-mass-storage-Improve-the-signature-of-SCSI-handler-function/20210627-061851
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: i386-randconfig-s002-20210627 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-341-g8af24329-dirty
# https://github.com/0day-ci/linux/commit/db2ec6f1e52293817f380a4875e01c36a4195c19
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Igor-Kononenko/usb-gadget-mass-storage-Improve-the-signature-of-SCSI-handler-function/20210627-061851
git checkout db2ec6f1e52293817f380a4875e01c36a4195c19
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/usb/gadget/function/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/usb/gadget/function/f_mass_storage.c:1988:57: sparse: sparse: restricted __be16 degrades to integer
>> drivers/usb/gadget/function/f_mass_storage.c:2060:30: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] data_len @@ got restricted __be32 [usertype] @@
drivers/usb/gadget/function/f_mass_storage.c:2060:30: sparse: expected unsigned int [usertype] data_len
drivers/usb/gadget/function/f_mass_storage.c:2060:30: sparse: got restricted __be32 [usertype]
>> drivers/usb/gadget/function/f_mass_storage.c:2062:17: sparse: sparse: cast from restricted __be16
>> drivers/usb/gadget/function/f_mass_storage.c:2061:34: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned short [usertype] curr_profile @@ got restricted __be16 [usertype] @@
drivers/usb/gadget/function/f_mass_storage.c:2061:34: sparse: expected unsigned short [usertype] curr_profile
drivers/usb/gadget/function/f_mass_storage.c:2061:34: sparse: got restricted __be16 [usertype]
drivers/usb/gadget/function/f_mass_storage.c:2231:11: sparse: sparse: Initializer entry defined twice
drivers/usb/gadget/function/f_mass_storage.c:2231:11: sparse: also defined here
drivers/usb/gadget/function/f_mass_storage.c:2233:11: sparse: sparse: Initializer entry defined twice
drivers/usb/gadget/function/f_mass_storage.c:2233:11: sparse: also defined here
drivers/usb/gadget/function/f_mass_storage.c:2235:11: sparse: sparse: Initializer entry defined twice
drivers/usb/gadget/function/f_mass_storage.c:2235:11: sparse: also defined here
drivers/usb/gadget/function/f_mass_storage.c:2259:11: sparse: sparse: Initializer entry defined twice
drivers/usb/gadget/function/f_mass_storage.c:2259:11: sparse: also defined here
drivers/usb/gadget/function/f_mass_storage.c:2261:11: sparse: sparse: Initializer entry defined twice
drivers/usb/gadget/function/f_mass_storage.c:2261:11: sparse: also defined here
drivers/usb/gadget/function/f_mass_storage.c:2263:11: sparse: sparse: Initializer entry defined twice
drivers/usb/gadget/function/f_mass_storage.c:2263:11: sparse: also defined here
drivers/usb/gadget/function/f_mass_storage.c:2265:11: sparse: sparse: Initializer entry defined twice
drivers/usb/gadget/function/f_mass_storage.c:2265:11: sparse: also defined here
drivers/usb/gadget/function/f_mass_storage.c: note: in included file (through include/linux/rcuwait.h, include/linux/percpu-rwsem.h, include/linux/fs.h, ...):
include/linux/sched/signal.h:285:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
include/linux/sched/signal.h:285:28: sparse: expected struct spinlock [usertype] *lock
include/linux/sched/signal.h:285:28: sparse: got struct spinlock [noderef] __rcu *
include/linux/sched/signal.h:287:30: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
include/linux/sched/signal.h:287:30: sparse: expected struct spinlock [usertype] *lock
include/linux/sched/signal.h:287:30: sparse: got struct spinlock [noderef] __rcu *
vim +1988 drivers/usb/gadget/function/f_mass_storage.c
1969
1970 /* Adjust current profile which depended on an inserted medium */
1971 static inline void cdf_populate_profile_list(struct fsg_common *common,
1972 struct cdr_features **feature)
1973 {
1974 __be16 current_media_type = cdr_guess_medium_type(common);
1975 struct mmc_profile *profiles =
1976 (*feature)->feature.profile_list.profiles;
1977 int i;
1978
1979 /* copy profile list to the response buffer */
1980 memcpy(profiles, cdf_supported_profiles,
1981 sizeof(cdf_supported_profiles));
1982 for (i = 0; i < CDF_PROFILES_COUNT; ++i) {
1983 /*
1984 * Reset the current profile bit,
1985 * because it might be set from the previous one
1986 */
1987 profiles[i].current_p = 0;
> 1988 if (be16_to_cpu(profiles[i].profile) == current_media_type) {
1989 DBG(common, "Fill current profile: curr=(%04Xh)\n",
1990 be16_to_cpu(profiles[i].profile));
1991 profiles[i].current_p = 1;
1992 }
1993 }
1994 }
1995
1996 static int do_get_configuration(struct fsg_common *common,
1997 struct fsg_buffhd *bh)
1998 {
1999 struct fsg_lun *curlun = common->curlun;
2000 int i;
2001 struct cdb_get_configuration *cdb =
2002 (struct cdb_get_configuration *)common->cmnd;
2003 size_t buffer_size = sizeof(struct feature_header);
2004 size_t generic_desc_size = sizeof(struct cdb_ft_generic);
2005 struct feature_header *ret_header = (struct feature_header *)bh->buf;
2006 u8 *ret_data = ((u8 *)ret_header) + buffer_size;
2007
2008 LDBG(curlun, "Requesting features from 0x%04X, with RT flag 0x%02X\n",
2009 be16_to_cpu(cdb->sfn), cdb->rt);
2010
2011 if (!common->curlun || !common->curlun->cdrom)
2012 return -EINVAL;
2013
2014 /* Go over *all* features, and copy them according to RT value */
2015 for (i = 0; i < ARRAY_SIZE(features_table); ++i) {
2016 struct cdb_ft_generic *generic =
2017 (struct cdb_ft_generic *)&features_table[i];
2018 struct cdr_features *feature = &features_table[i];
2019
2020 if (feature->populate != NULL)
2021 feature->populate(common, &feature);
2022
2023 // a) RT is 0x00 and feature's code >= SFN
2024 // b) RT is 0x01, feature's code >= SFN and feature has 'current' bit set
2025 // c) RT is 0x02 and feature's code == SFN
2026
2027 if (be16_to_cpu(generic->code) >= be16_to_cpu(cdb->sfn)) {
2028 if ((cdb->rt == CDR_CFG_RT_FULL) ||
2029 (cdb->rt == CDR_CFG_RT_CURRENT &&
2030 generic->vpc.cur) ||
2031 (cdb->rt == CDR_CFG_RT_SPECIFIED_SFN &&
2032 be16_to_cpu(generic->code) ==
2033 be16_to_cpu(cdb->sfn))) {
2034 LDBG(curlun, "Copying feature 0x%04X\n",
2035 be16_to_cpu(generic->code));
2036
2037 memset(ret_data, 0,
2038 (generic->length + generic_desc_size));
2039 /* Copy feature */
2040 memcpy(ret_data, feature,
2041 (generic->length + generic_desc_size));
2042 buffer_size +=
2043 (generic->length + generic_desc_size);
2044 ret_data +=
2045 (generic->length + generic_desc_size);
2046
2047 /* Break the loop if RT is CDR_CFG_RT_SPECIFIED_SFN */
2048 if (cdb->rt == CDR_CFG_RT_SPECIFIED_SFN) {
2049 LDBG(curlun,
2050 "Got the feature we wanted (0x%04X), breaking the loop\n",
2051 be16_to_cpu(cdb->sfn));
2052 break;
2053 }
2054 }
2055 }
2056 }
2057
2058 memset(ret_header, 0, sizeof(struct feature_header));
2059 /* Header */
> 2060 ret_header->data_len = cpu_to_be32(buffer_size - generic_desc_size);
> 2061 ret_header->curr_profile =
> 2062 cpu_to_be16(cdr_guess_medium_type(common));
2063
2064 dump_msg(common, "feature header", (u8 *)ret_header,
2065 sizeof(struct feature_header));
2066
2067 dump_msg(common, "features table", (u8 *)bh->buf, buffer_size);
2068
2069 common->data_size_to_handle = buffer_size;
2070 return 0;
2071 }
2072
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47122 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function
2021-06-26 21:18 ` [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function Igor Kononenko
@ 2021-06-27 14:18 ` Alan Stern
2021-06-27 15:32 ` i.kononenko
0 siblings, 1 reply; 22+ messages in thread
From: Alan Stern @ 2021-06-27 14:18 UTC (permalink / raw)
To: Igor Kononenko
Cc: Felipe Balbi, openbmc, linux-usb, linux-kernel, Greg Kroah-Hartman
On Sun, Jun 27, 2021 at 12:18:14AM +0300, Igor Kononenko wrote:
> SCSI command handlers currently have an ambiguous return value. This
(I dislike very much this way of writing patch descriptions. Unless
the reader has already looked at the email subject line and remembers
that this patch affects the mass-storage gadget, he will think the
sentence above is talking about command handlers in the SCSI core -- a
completely different part of the kernel. When writing patch
descriptions, please do not assume that the reader already knows what
the patch is about.)
> return value may indicate the length of the data written to the response
> buffer and the command's processing status. Thus, the understanding of
> command handling may be implicit.
The return value is _not_ ambiguous. If the value is >= 0 then it is
a data length, otherwise it is a status. Yes, this is implicit, but it
is a very common pattern used throughout the kernel and everyone
understands it.
> After this patch, the output buffer's size will be set in the
> 'data_size_to_handle' field of 'struct fsg_common', and the command
> handler's return value indicates only the processing status.
What is the reason for making this change? Does it fix any problems
or prepare the way for any future patches? It seems like this is
completely unnecessary.
Alan Stern
> Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN
> BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the
> USBGadget MassStorage debug print showed all sent commands works
> properly.
>
> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling
2021-06-26 21:18 ` [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling Igor Kononenko
2021-06-26 23:29 ` kernel test robot
@ 2021-06-27 14:23 ` Alan Stern
2021-06-27 17:14 ` i.kononenko
1 sibling, 1 reply; 22+ messages in thread
From: Alan Stern @ 2021-06-27 14:23 UTC (permalink / raw)
To: Igor Kononenko
Cc: Felipe Balbi, openbmc, linux-usb, linux-kernel, Greg Kroah-Hartman
On Sun, Jun 27, 2021 at 12:18:15AM +0300, Igor Kononenko wrote:
> Implements a universal way to define SCSI commands and configure
> precheck handlers.
What is the reason for doing this?
At first glance, it appears you have added a great deal of complexity
to the driver. The patch replaces a large amount of easily understood
(albeit rather repetitious) code with an approximately equal amount
of rather complicated code. This does not seem like an improvement.
Furthermore, the code you removed is flexible; it easily allows for
small variations as neede by some command handlers. But the code you
added is all table-driven, which does not easily permit arbitrary
variations.
> Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN
> BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the
> USBGadget MassStorage debug print showed all sent commands works
> properly.
>
> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 540 +++++++++++--------
> drivers/usb/gadget/function/storage_common.h | 5 +
> 2 files changed, 310 insertions(+), 235 deletions(-)
I don't see the point of adding 75 lines to the kernel source if they
don't accomplish anything new.
Alan Stern
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] fms: Add TOC/PMA/ATIP DVD-ROM capabilities
2021-06-26 21:18 ` [PATCH 3/6] fms: Add TOC/PMA/ATIP DVD-ROM capabilities Igor Kononenko
@ 2021-06-27 14:29 ` Alan Stern
2021-06-27 18:45 ` i.kononenko
0 siblings, 1 reply; 22+ messages in thread
From: Alan Stern @ 2021-06-27 14:29 UTC (permalink / raw)
To: Igor Kononenko
Cc: Jens Axboe, Felipe Balbi, Greg Kroah-Hartman, openbmc, linux-usb,
linux-kernel
On Sun, Jun 27, 2021 at 12:18:16AM +0300, Igor Kononenko wrote:
> The DVD-ROM required the SCSI 6.25 READ TOC/PMA/ATIP Command formats:
> * Response Format 0000b: Formatted TOC
> * Response Format 0001b: Multi-session Information
> (MMC-6 Specification).
>
> This patch adds an implementation of that described above formats.
I will sum up the last four patches of this series by saying that they
add features for emulating DVD-ROM and BD devices. Doing so increases
the size of the f_mass_storage driver by a considerable amount and
also adds a large amount of new material to Jens Axboe's
include/uapi/linux/cdrom.h.
Is any of this really needed? What usage scenarios require
f_mass_storage to emulate a DVD-ROM that couldn't use f_tcm instead?
Alan Stern
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function
2021-06-27 14:18 ` Alan Stern
@ 2021-06-27 15:32 ` i.kononenko
2021-06-27 16:39 ` Alan Stern
0 siblings, 1 reply; 22+ messages in thread
From: i.kononenko @ 2021-06-27 15:32 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, openbmc, linux-usb, linux-kernel, Greg Kroah-Hartman
Good morning, Alan!
First of all, thank you for your time to review my first patchset for
the Linux Kernel and valuable advice on the right way of patchwriting!
On 27.06.2021 17:18, Alan Stern wrote:
> On Sun, Jun 27, 2021 at 12:18:14AM +0300, Igor Kononenko wrote:
>> SCSI command handlers currently have an ambiguous return value. This
>
> (I dislike very much this way of writing patch descriptions. Unless
> the reader has already looked at the email subject line and remembers
> that this patch affects the mass-storage gadget, he will think the
> sentence above is talking about command handlers in the SCSI core -- a
> completely different part of the kernel. When writing patch
> descriptions, please do not assume that the reader already knows what
> the patch is about.)
>
>> return value may indicate the length of the data written to the response
>> buffer and the command's processing status. Thus, the understanding of
>> command handling may be implicit.
First of all, thank you for your time to review my first patchset for the
Linux Kernel and valuable advice on the right way of patchwriting!
I noticed that the status/datasize return value pattern is pervasive for
Linux and used through many subsystems. But for the f_mass_storage.c,
such approach use case is not documented anywhere, and implementation has
too many magic-constant, e.g.
```
static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)
{
....
return 36;
}
```
IMHO, this way is not giving the developer an explicit understanding of
'what is the 36' and its origin.
If moving to the suggested way is unwanted, I'd keep the implementation
as is with additional documentation for each function where uses this
approach.
Additionally, I guess, define clarify macros of return value instead of
magic numbers is required.
>
> The return value is _not_ ambiguous. If the value is >= 0 then it is
> a data length, otherwise it is a status. Yes, this is implicit, but it
> is a very common pattern used throughout the kernel and everyone
> understands it.
>
>> After this patch, the output buffer's size will be set in the
>> 'data_size_to_handle' field of 'struct fsg_common', and the command
>> handler's return value indicates only the processing status.
>
> What is the reason for making this change? Does it fix any problems
> or prepare the way for any future patches? It seems like this is
> completely unnecessary.
Yes, the patch uses as part of the incoming implementation of refactoring
'usb:gadget:mass-storage:scsi' command handling.
I believed the suggested improvement would be useful for the community as
an improvement of code.
>
> Alan Stern
>
>> Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN
>> BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the
>> USBGadget MassStorage debug print showed all sent commands works
>> properly.
>>
>> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
--
Best regards,
Igor Kononenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function
2021-06-27 15:32 ` i.kononenko
@ 2021-06-27 16:39 ` Alan Stern
0 siblings, 0 replies; 22+ messages in thread
From: Alan Stern @ 2021-06-27 16:39 UTC (permalink / raw)
To: i.kononenko
Cc: Felipe Balbi, openbmc, linux-usb, linux-kernel, Greg Kroah-Hartman
On Sun, Jun 27, 2021 at 06:32:03PM +0300, i.kononenko wrote:
> Good morning, Alan!
>
> First of all, thank you for your time to review my first patchset for
> the Linux Kernel and valuable advice on the right way of patchwriting!
>
> On 27.06.2021 17:18, Alan Stern wrote:
> > On Sun, Jun 27, 2021 at 12:18:14AM +0300, Igor Kononenko wrote:
> >> SCSI command handlers currently have an ambiguous return value. This
> >
> > (I dislike very much this way of writing patch descriptions. Unless
> > the reader has already looked at the email subject line and remembers
> > that this patch affects the mass-storage gadget, he will think the
> > sentence above is talking about command handlers in the SCSI core -- a
> > completely different part of the kernel. When writing patch
> > descriptions, please do not assume that the reader already knows what
> > the patch is about.)
> >
> >> return value may indicate the length of the data written to the response
> >> buffer and the command's processing status. Thus, the understanding of
> >> command handling may be implicit.
>
> First of all, thank you for your time to review my first patchset for the
> Linux Kernel and valuable advice on the right way of patchwriting!
>
> I noticed that the status/datasize return value pattern is pervasive for
> Linux and used through many subsystems. But for the f_mass_storage.c,
> such approach use case is not documented anywhere, and implementation has
> too many magic-constant, e.g.
> ```
> static int do_inquiry(struct fsg_common *common, struct fsg_buffhd *bh)
> {
> ....
> return 36;
> }
> ```
> IMHO, this way is not giving the developer an explicit understanding of
> 'what is the 36' and its origin.
> If moving to the suggested way is unwanted, I'd keep the implementation
> as is with additional documentation for each function where uses this
> approach.
Since every one of the command handler functions uses this convention,
it would be wasteful to have separate documentation of the return value
for each function. A single documentation comment that covers all the
command handlers would be acceptable.
> Additionally, I guess, define clarify macros of return value instead of
> magic numbers is required.
If you want, okay. That should go in a separate patch from the
documentation patch.
Also, since the return values are different for each command handler, I
suggest that the macro definitions be placed along with the handler
functions and not in a separate header file. Having a separate file for
these macros would not make any sense, because the values do not need to
be shared across multiple functions or source files.
> > The return value is _not_ ambiguous. If the value is >= 0 then it is
> > a data length, otherwise it is a status. Yes, this is implicit, but it
> > is a very common pattern used throughout the kernel and everyone
> > understands it.
> >
> >> After this patch, the output buffer's size will be set in the
> >> 'data_size_to_handle' field of 'struct fsg_common', and the command
> >> handler's return value indicates only the processing status.
> >
> > What is the reason for making this change? Does it fix any problems
> > or prepare the way for any future patches? It seems like this is
> > completely unnecessary.
>
> Yes, the patch uses as part of the incoming implementation of refactoring
> 'usb:gadget:mass-storage:scsi' command handling.
That incoming implementation uses the refactored command handling but
doesn't depend on the refactoring. It could just as easily use the
existing command handling.
> I believed the suggested improvement would be useful for the community as
> an improvement of code.
Unless you can provide a convincing reason for this change, it doesn't
seem like an improvement to me. It's no easier to read or understand,
and it doesn't improve execution speed on a critical pathway. It just
seems like pointless code churn.
Alan Stern
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling
2021-06-27 14:23 ` Alan Stern
@ 2021-06-27 17:14 ` i.kononenko
2021-06-28 1:06 ` Alan Stern
0 siblings, 1 reply; 22+ messages in thread
From: i.kononenko @ 2021-06-27 17:14 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, openbmc, linux-usb, linux-kernel, Greg Kroah-Hartman
On 27.06.2021 17:23, Alan Stern wrote:
> On Sun, Jun 27, 2021 at 12:18:15AM +0300, Igor Kononenko wrote:
>> Implements a universal way to define SCSI commands and configure
>> precheck handlers.
>
> What is the reason for doing this?
I have started implementing a way to specify a backend-file of
mass-storage images greater than 2.1Gb for cdrom-like mediums.
I notice the implementation of each scsi-command handler uses too
many magic-constant, hardcoded indexes and shifts. I decided to
define structures that contained appropriate SCSI-defined fields
and constant-values to clarify the code.
Additionally, I noticed, many kernel subsystems use the 'separate
data and logic' approach, making a code more explicit and readable.
This looks reasonable to me, and a code looks more clearly, at
least - we don't need to examine each magic constant and its purpose.
>
> At first glance, it appears you have added a great deal of complexity
> to the driver. The patch replaces a large amount of easily understood
> (albeit rather repetitious) code with an approximately equal amount
> of rather complicated code. This does not seem like an improvement.
The SCSI-commands table is defined as unifying a way to specify the
SCSI-command handler, with corresponding required data instead pass
it to each repeatedly switch-case block, which makes code more readable
to me. If there isn't, I can keep the definition of SCSI-handlers as is,
but the SCSI-data structures with their constant-values are still
required, in my opinion.
>
> Furthermore, the code you removed is flexible; it easily allows for
> small variations as neede by some command handlers. But the code you
> added is all table-driven, which does not easily permit arbitrary
> variations.
>
I don't think that the SCSI-command handlers table is an obstacle to
define variation into a specific handler because the current patch has
helper macros, which can specify a behavior for each requirement of
handler.
Anyway, the definition of the scsi-command handlers table may be discarded,
because this work done to helping developers who will work the
'usb:gadget:mass-storage' subsystem in the future.
>> Tested: By probing the USBGadget Mass-Storage on the YADRO VEGMAN
>> BMC(AST2500) sample, each SCSI command was sent through HOST->BMC; the
>> USBGadget MassStorage debug print showed all sent commands works
>> properly.
>>
>> Signed-off-by: Igor Kononenko <i.kononenko@yadro.com>
>> ---
>> drivers/usb/gadget/function/f_mass_storage.c | 540 +++++++++++--------
>> drivers/usb/gadget/function/storage_common.h | 5 +
>> 2 files changed, 310 insertions(+), 235 deletions(-)
>
> I don't see the point of adding 75 lines to the kernel source if they
> don't accomplish anything new.
>
> Alan Stern
>
--
Best regards,
Igor Kononenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] fms: Add TOC/PMA/ATIP DVD-ROM capabilities
2021-06-27 14:29 ` Alan Stern
@ 2021-06-27 18:45 ` i.kononenko
2021-06-28 14:31 ` Alan Stern
0 siblings, 1 reply; 22+ messages in thread
From: i.kononenko @ 2021-06-27 18:45 UTC (permalink / raw)
To: Alan Stern
Cc: Jens Axboe, Felipe Balbi, Greg Kroah-Hartman, openbmc, linux-usb,
linux-kernel
On 27.06.2021 17:29, Alan Stern wrote:
> On Sun, Jun 27, 2021 at 12:18:16AM +0300, Igor Kononenko wrote:
>> The DVD-ROM required the SCSI 6.25 READ TOC/PMA/ATIP Command formats:
>> * Response Format 0000b: Formatted TOC
>> * Response Format 0001b: Multi-session Information
>> (MMC-6 Specification).
>>
>> This patch adds an implementation of that described above formats.
>
> I will sum up the last four patches of this series by saying that they
> add features for emulating DVD-ROM and BD devices. Doing so increases
> the size of the f_mass_storage driver by a considerable amount and
> also adds a large amount of new material to Jens Axboe's
> include/uapi/linux/cdrom.h.
The `include/uapi/linux/cdrom.h` already includes the definition of the
MMC-(2/3) SCSI data structures, and I believe this uses in many cdrom-api
consumers. The current patchset extends this declaration with additional
structures set and clarifies each. Besides, the described above
SCSI structures are used to implement a mass_storage SCSI-command handler
to make implementation clearer and avoid the use of magic constants.
>
> Is any of this really needed? What usage scenarios require
> f_mass_storage to emulate a DVD-ROM that couldn't use f_tcm instead?
I can't see any impediments to supplement the already existing
implementation of MMC-(2/3) specification of multimedia devices to
represent the DVD/BD features. If the kernel presents the CD-ROM SCSI
commands, why the mass_storage:usb-gadget-function still doesn't include
that for DVD/BD?
Many modern embedded systems (e.g., BMC, OpenBMC) implements their
required features, e.g., Virtual Media Device, which is based on the
usb:gadget:mass-storage.
The purpose of that features is extensive, and their use the mass-storage
not only as a cdrom-device.
The required features of such systems might expect image back-end files
that size is significant than 2.1Gb, but such medium is not the CD-ROM
device. USB-gadget consumers can incorrectly interpret such device by
loading the wrong driver. I believe that should be the DVD-medium device,
at least.
Additionally, please note the current patch also fixes the incorrect
implementation of retrieving TOC/PMA/ATIP data, which is required for the
CD-ROM. One system might correct works with retrieving first with the
last session together, but for some systems, e.g., OS ESXi, OS Windows,
should retrieving first and last border sessions in separate SCSI-request.
>
> Alan Stern
>
--
Best regards,
Igor Kononenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling
2021-06-27 17:14 ` i.kononenko
@ 2021-06-28 1:06 ` Alan Stern
2021-06-28 10:38 ` i.kononenko
0 siblings, 1 reply; 22+ messages in thread
From: Alan Stern @ 2021-06-28 1:06 UTC (permalink / raw)
To: i.kononenko
Cc: Felipe Balbi, openbmc, linux-usb, linux-kernel, Greg Kroah-Hartman
On Sun, Jun 27, 2021 at 08:14:48PM +0300, i.kononenko wrote:
>
>
> On 27.06.2021 17:23, Alan Stern wrote:
> > On Sun, Jun 27, 2021 at 12:18:15AM +0300, Igor Kononenko wrote:
> >> Implements a universal way to define SCSI commands and configure
> >> precheck handlers.
> >
> > What is the reason for doing this?
>
> I have started implementing a way to specify a backend-file of
> mass-storage images greater than 2.1Gb for cdrom-like mediums.
> I notice the implementation of each scsi-command handler uses too
> many magic-constant, hardcoded indexes and shifts. I decided to
> define structures that contained appropriate SCSI-defined fields
> and constant-values to clarify the code.
>
> Additionally, I noticed, many kernel subsystems use the 'separate
> data and logic' approach, making a code more explicit and readable.
> This looks reasonable to me, and a code looks more clearly, at
> least - we don't need to examine each magic constant and its purpose.
>
> >
> > At first glance, it appears you have added a great deal of complexity
> > to the driver. The patch replaces a large amount of easily understood
> > (albeit rather repetitious) code with an approximately equal amount
> > of rather complicated code. This does not seem like an improvement.
>
> The SCSI-commands table is defined as unifying a way to specify the
> SCSI-command handler, with corresponding required data instead pass
> it to each repeatedly switch-case block, which makes code more readable
> to me. If there isn't, I can keep the definition of SCSI-handlers as is,
> but the SCSI-data structures with their constant-values are still
> required, in my opinion.
>
> >
> > Furthermore, the code you removed is flexible; it easily allows for
> > small variations as neede by some command handlers. But the code you
> > added is all table-driven, which does not easily permit arbitrary
> > variations.
> >
>
> I don't think that the SCSI-command handlers table is an obstacle to
> define variation into a specific handler because the current patch has
> helper macros, which can specify a behavior for each requirement of
> handler.
>
> Anyway, the definition of the scsi-command handlers table may be discarded,
> because this work done to helping developers who will work the
> 'usb:gadget:mass-storage' subsystem in the future.
Can you submit a patch that adds only the data structures without the
commands table?
Alan Stern
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] FMS: Add the SCSI Get Configuration command.
2021-06-26 21:18 ` [PATCH 5/6] FMS: Add the SCSI Get Configuration command Igor Kononenko
2021-06-27 0:44 ` kernel test robot
2021-06-27 4:42 ` kernel test robot
@ 2021-06-28 9:53 ` Christoph Hellwig
2021-06-28 10:34 ` i.kononenko
2 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2021-06-28 9:53 UTC (permalink / raw)
To: Igor Kononenko
Cc: Jens Axboe, Felipe Balbi, linux-scsi, Martin K. Petersen,
Greg Kroah-Hartman, James E.J. Bottomley, linux-usb,
linux-kernel, linux-ide, openbmc
What is FMS? And why do only patches 5 and 6 show up on the list?
And why does this mix changes to the SCSI layer, libata, usb-gadget and
the CDROM UAPI in a single patch?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] FMS: Add the SCSI Get Configuration command.
2021-06-28 9:53 ` Christoph Hellwig
@ 2021-06-28 10:34 ` i.kononenko
0 siblings, 0 replies; 22+ messages in thread
From: i.kononenko @ 2021-06-28 10:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Felipe Balbi, linux-scsi, Martin K. Petersen,
Greg Kroah-Hartman, James E.J. Bottomley, linux-usb,
linux-kernel, linux-ide, openbmc
On 28.06.2021 12:53, Christoph Hellwig wrote:
> What is FMS? And why do only patches 5 and 6 show up on the list?
> And why does this mix changes to the SCSI layer, libata, usb-gadget and
> the CDROM UAPI in a single patch?
>
It looks like my 'git email-send' has appended up the separate respondents
for each PATCH of the patchset. It is my first experience publishing
patches for the community. Sorry, I will be careful in the future.
Several patches have already been reviewed, and these need to be improved.
I will prepare the new version of the patchset and will publish it with
the appropriate list of respondents for each one.
From your letter, I noticed the commit messages also need to improve and
should be more clear. The descriptions will be updated either.
--
Best regards,
Igor Kononenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling
2021-06-28 1:06 ` Alan Stern
@ 2021-06-28 10:38 ` i.kononenko
0 siblings, 0 replies; 22+ messages in thread
From: i.kononenko @ 2021-06-28 10:38 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, openbmc, linux-usb, linux-kernel, Greg Kroah-Hartman
On 28.06.2021 04:06, Alan Stern wrote:
> On Sun, Jun 27, 2021 at 08:14:48PM +0300, i.kononenko wrote:
>>
>>
>> On 27.06.2021 17:23, Alan Stern wrote:
>>> On Sun, Jun 27, 2021 at 12:18:15AM +0300, Igor Kononenko wrote:
>>>> Implements a universal way to define SCSI commands and configure
>>>> precheck handlers.
>>>
>>> What is the reason for doing this?
>>
>> I have started implementing a way to specify a backend-file of
>> mass-storage images greater than 2.1Gb for cdrom-like mediums.
>> I notice the implementation of each scsi-command handler uses too
>> many magic-constant, hardcoded indexes and shifts. I decided to
>> define structures that contained appropriate SCSI-defined fields
>> and constant-values to clarify the code.
>>
>> Additionally, I noticed, many kernel subsystems use the 'separate
>> data and logic' approach, making a code more explicit and readable.
>> This looks reasonable to me, and a code looks more clearly, at
>> least - we don't need to examine each magic constant and its purpose.
>>
>>>
>>> At first glance, it appears you have added a great deal of complexity
>>> to the driver. The patch replaces a large amount of easily understood
>>> (albeit rather repetitious) code with an approximately equal amount
>>> of rather complicated code. This does not seem like an improvement.
>>
>> The SCSI-commands table is defined as unifying a way to specify the
>> SCSI-command handler, with corresponding required data instead pass
>> it to each repeatedly switch-case block, which makes code more readable
>> to me. If there isn't, I can keep the definition of SCSI-handlers as is,
>> but the SCSI-data structures with their constant-values are still
>> required, in my opinion.
>>
>>>
>>> Furthermore, the code you removed is flexible; it easily allows for
>>> small variations as neede by some command handlers. But the code you
>>> added is all table-driven, which does not easily permit arbitrary
>>> variations.
>>>
>>
>> I don't think that the SCSI-command handlers table is an obstacle to
>> define variation into a specific handler because the current patch has
>> helper macros, which can specify a behavior for each requirement of
>> handler.
>>
>> Anyway, the definition of the scsi-command handlers table may be discarded,
>> because this work done to helping developers who will work the
>> 'usb:gadget:mass-storage' subsystem in the future.
>
> Can you submit a patch that adds only the data structures without the
> commands table?
Yes, I will remove the mentioned SCSI-handlers table. Thank you!
>
> Alan Stern
>
--
Best regards,
Igor Kononenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] fms: Add TOC/PMA/ATIP DVD-ROM capabilities
2021-06-27 18:45 ` i.kononenko
@ 2021-06-28 14:31 ` Alan Stern
0 siblings, 0 replies; 22+ messages in thread
From: Alan Stern @ 2021-06-28 14:31 UTC (permalink / raw)
To: i.kononenko
Cc: Jens Axboe, Felipe Balbi, Greg Kroah-Hartman, openbmc, linux-usb,
linux-kernel
On Sun, Jun 27, 2021 at 09:45:07PM +0300, i.kononenko wrote:
>
>
> On 27.06.2021 17:29, Alan Stern wrote:
> > Is any of this really needed? What usage scenarios require
> > f_mass_storage to emulate a DVD-ROM that couldn't use f_tcm instead?
>
> I can't see any impediments to supplement the already existing
> implementation of MMC-(2/3) specification of multimedia devices to
> represent the DVD/BD features. If the kernel presents the CD-ROM SCSI
> commands, why the mass_storage:usb-gadget-function still doesn't include
> that for DVD/BD?
>
> Many modern embedded systems (e.g., BMC, OpenBMC) implements their
> required features, e.g., Virtual Media Device, which is based on the
> usb:gadget:mass-storage.
> The purpose of that features is extensive, and their use the mass-storage
> not only as a cdrom-device.
>
> The required features of such systems might expect image back-end files
> that size is significant than 2.1Gb, but such medium is not the CD-ROM
> device. USB-gadget consumers can incorrectly interpret such device by
> loading the wrong driver. I believe that should be the DVD-medium device,
> at least.
You should include this information in the patch description, so that
people will understand why you wrote the patch.
> Additionally, please note the current patch also fixes the incorrect
> implementation of retrieving TOC/PMA/ATIP data, which is required for the
> CD-ROM. One system might correct works with retrieving first with the
> last session together, but for some systems, e.g., OS ESXi, OS Windows,
> should retrieving first and last border sessions in separate SCSI-request.
What's wrong with the existing implementation? Are you talking about
the do_read_toc function? The driver only supports one session in any
case.
In general, fixes to existing code and additions of new code should go
in separate patches.
Alan Stern
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-06-29 3:26 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-26 21:18 [PATCH 0/6] USB-gadget: mass-storage: Support DVD-like images Igor Kononenko
2021-06-26 21:18 ` [PATCH 1/6] usb:gadget:mass-storage: Improve the signature of SCSI handler function Igor Kononenko
2021-06-27 14:18 ` Alan Stern
2021-06-27 15:32 ` i.kononenko
2021-06-27 16:39 ` Alan Stern
2021-06-26 21:18 ` [PATCH 2/6] usb:gadget:mass-storage: refactoring the SCSI command handling Igor Kononenko
2021-06-26 23:29 ` kernel test robot
2021-06-27 14:23 ` Alan Stern
2021-06-27 17:14 ` i.kononenko
2021-06-28 1:06 ` Alan Stern
2021-06-28 10:38 ` i.kononenko
2021-06-26 21:18 ` [PATCH 3/6] fms: Add TOC/PMA/ATIP DVD-ROM capabilities Igor Kononenko
2021-06-27 14:29 ` Alan Stern
2021-06-27 18:45 ` i.kononenko
2021-06-28 14:31 ` Alan Stern
2021-06-26 21:18 ` [PATCH 4/6] fms: Support the DVD/BD images size over 2.1Gb Igor Kononenko
2021-06-26 21:18 ` [PATCH 5/6] FMS: Add the SCSI Get Configuration command Igor Kononenko
2021-06-27 0:44 ` kernel test robot
2021-06-27 4:42 ` kernel test robot
2021-06-28 9:53 ` Christoph Hellwig
2021-06-28 10:34 ` i.kononenko
2021-06-26 21:18 ` [PATCH 6/6] FMS: Add SCSI Read Disc Information command Igor Kononenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).