From: Alan Stern <stern@rowland.harvard.edu>
To: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc: Felipe Balbi <balbi@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
Petr Nechaev <petr.nechaev@cogentembedded.com>
Subject: Re: [PATCH] usb: gadget: storage: add support for media larger than 2T
Date: Tue, 14 Sep 2021 11:13:29 -0400 [thread overview]
Message-ID: <20210914151329.GD155245@rowland.harvard.edu> (raw)
In-Reply-To: <20210914052728.23369-1-nikita.yoush@cogentembedded.com>
On Tue, Sep 14, 2021 at 08:27:28AM +0300, Nikita Yushchenko wrote:
> This adds support for READ_CAPACITY(16), READ(16) and WRITE(16)
> commands, and fixes READ_CAPACITY command to return 0xffffffff if
> media size does not fit in 32 bits.
>
> This makes f_mass_storage to export a 16T disk array correctly.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 70 ++++++++++++++++++--
> 1 file changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 7c96c4665178..b79737c19750 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -619,7 +619,7 @@ static int sleep_thread(struct fsg_common *common, bool can_freeze,
> static int do_read(struct fsg_common *common)
> {
> struct fsg_lun *curlun = common->curlun;
> - u32 lba;
> + u64 lba;
> struct fsg_buffhd *bh;
> int rc;
> u32 amount_left;
> @@ -634,7 +634,10 @@ static int do_read(struct fsg_common *common)
> if (common->cmnd[0] == READ_6)
> lba = get_unaligned_be24(&common->cmnd[1]);
> else {
> - lba = get_unaligned_be32(&common->cmnd[2]);
> + if (common->cmnd[0] == READ_16)
> + lba = get_unaligned_be64(&common->cmnd[2]);
> + else
It would be good to put a comment here:
else /* READ_10 or READ_12 */
Same for the WRITE_10/WRITE_12 case below.
> + lba = get_unaligned_be32(&common->cmnd[2]);
>
> /*
> * We allow DPO (Disable Page Out = don't save data in the
> @@ -747,7 +750,7 @@ static int do_read(struct fsg_common *common)
> static int do_write(struct fsg_common *common)
> {
> struct fsg_lun *curlun = common->curlun;
> - u32 lba;
> + u64 lba;
> struct fsg_buffhd *bh;
> int get_some_more;
> u32 amount_left_to_req, amount_left_to_write;
> @@ -771,7 +774,10 @@ static int do_write(struct fsg_common *common)
> if (common->cmnd[0] == WRITE_6)
> lba = get_unaligned_be24(&common->cmnd[1]);
> else {
> - lba = get_unaligned_be32(&common->cmnd[2]);
> + if (common->cmnd[0] == WRITE_16)
> + lba = get_unaligned_be64(&common->cmnd[2]);
> + else
> + lba = get_unaligned_be32(&common->cmnd[2]);
>
> /*
> * We allow DPO (Disable Page Out = don't save data in the
> @@ -1146,6 +1152,7 @@ static int do_read_capacity(struct fsg_common *common, struct fsg_buffhd *bh)
> u32 lba = get_unaligned_be32(&common->cmnd[2]);
> int pmi = common->cmnd[8];
> u8 *buf = (u8 *)bh->buf;
> + u32 mlb;
The Linux kernel does not suffer from a shortage of letters; you don't
have to struggle to conserve them. This variable should be named
max_lba or something similar. (Note: To readers in the US, "mlb" stands
for "Major League Baseball"!)
>
> /* Check the PMI and LBA fields */
> if (pmi > 1 || (pmi == 0 && lba != 0)) {
> @@ -1153,12 +1160,28 @@ static int do_read_capacity(struct fsg_common *common, struct fsg_buffhd *bh)
> return -EINVAL;
> }
>
> - put_unaligned_be32(curlun->num_sectors - 1, &buf[0]);
> - /* Max logical block */
> - put_unaligned_be32(curlun->blksize, &buf[4]);/* Block length */
> + if (curlun->num_sectors < 0x100000000ULL)
> + mlb = curlun->num_sectors - 1;
> + else
> + mlb = 0xffffffff;
> + put_unaligned_be32(mlb, &buf[0]); /* Max logical block */
> + put_unaligned_be32(curlun->blksize, &buf[4]); /* Block length */
> return 8;
> }
>
> +static int do_read_capacity_16(struct fsg_common *common, struct fsg_buffhd *bh)
> +{
> + struct fsg_lun *curlun = common->curlun;
> + u8 *buf = (u8 *)bh->buf;
> +
> + put_unaligned_be64(curlun->num_sectors - 1, &buf[0]);
> + /* Max logical block */
> + put_unaligned_be32(curlun->blksize, &buf[8]); /* Block length */
> + /* It is safe to keep other fields zeroed */
Blank line preceding the comment, please.
> + memset(&buf[12], 0, 32 - 12);
> + return 32;
> +}
> +
> static int do_read_header(struct fsg_common *common, struct fsg_buffhd *bh)
> {
> struct fsg_lun *curlun = common->curlun;
> @@ -1905,6 +1928,17 @@ static int do_scsi_command(struct fsg_common *common)
> reply = do_read(common);
> break;
>
> + case READ_16:
> + common->data_size_from_cmnd =
> + get_unaligned_be32(&common->cmnd[10]);
> + reply = check_command_size_in_blocks(common, 16,
> + DATA_DIR_TO_HOST,
> + (1<<1) | (0xff<<2) | (0xf<<10), 1,
> + "READ(16)");
> + if (reply == 0)
> + reply = do_read(common);
> + break;
> +
> case READ_CAPACITY:
> common->data_size_from_cmnd = 8;
> reply = check_command(common, 10, DATA_DIR_TO_HOST,
> @@ -1914,6 +1948,17 @@ static int do_scsi_command(struct fsg_common *common)
> reply = do_read_capacity(common, bh);
> break;
>
> + case SERVICE_ACTION_IN_16:
I realize that this is the code for READ_CAPACITY_16, but the commands
should be kept in alphabetical order by symbol name. So this belongs
lower down, after REQUEST_SENSE.
> + if ((common->cmnd[1] & 0x1f) != SAI_READ_CAPACITY_16)
> + goto unknown_cmnd;
> + common->data_size_from_cmnd = 32;
This should be get_unaligned_be32(&common->cmnd[10]).
> + reply = check_command(common, 14, DATA_DIR_TO_HOST,
16, not 14.
> + (1<<1) | (1<<13), 1,
Do you want to disallow nonzero values for the LBA field? Then
this should be (1<<1) | (0xf<<10) | (1<<14). If you also want to
disallow nonzero values for the PMI bit, omit the (1<<14) part.
> + "READ CAPACITY(16)");
> + if (reply == 0)
> + reply = do_read_capacity_16(common, bh);
> + break;
> +
> case READ_HEADER:
> if (!common->curlun || !common->curlun->cdrom)
> goto unknown_cmnd;
> @@ -2028,6 +2073,17 @@ static int do_scsi_command(struct fsg_common *common)
> reply = do_write(common);
> break;
>
> + case WRITE_16:
> + common->data_size_from_cmnd =
> + get_unaligned_be32(&common->cmnd[10]);
> + reply = check_command_size_in_blocks(common, 16,
> + DATA_DIR_FROM_HOST,
> + (1<<1) | (0xff<<2) | (0xf<<10), 1,
> + "WRITE(16)");
> + if (reply == 0)
> + reply = 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
Please resubmit the patch with these changes.
Alan Stern
next prev parent reply other threads:[~2021-09-14 15:13 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-14 5:27 [PATCH] usb: gadget: storage: add support for media larger than 2T Nikita Yushchenko
2021-09-14 15:13 ` Alan Stern [this message]
2021-09-14 20:09 ` [PATCH v2] " Nikita Yushchenko
2021-09-14 20:32 ` Alan Stern
2021-09-21 14:29 ` Greg Kroah-Hartman
2021-09-21 14:59 ` [PATCH v3] " Nikita Yushchenko
2021-10-05 11:07 ` Greg Kroah-Hartman
2021-10-05 12:07 ` Felipe Balbi
2021-10-05 14:19 ` Alan Stern
2021-10-10 13:08 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210914151329.GD155245@rowland.harvard.edu \
--to=stern@rowland.harvard.edu \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=nikita.yoush@cogentembedded.com \
--cc=petr.nechaev@cogentembedded.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.