All of lore.kernel.org
 help / color / mirror / Atom feed
From: Douglas Gilbert <dgilbert@interlog.com>
To: "Maciej W. Rozycki" <macro@orcam.me.uk>,
	Khalid Aziz <khalid@gonehiking.org>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>, Nix <nix@esperi.org.uk>,
	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page
Date: Sun, 2 Jan 2022 23:09:24 -0500	[thread overview]
Message-ID: <d9eaa1f8-7abb-645b-d624-5069205c6983@interlog.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2201020030130.56863@angie.orcam.me.uk>

On 2022-01-02 6:23 p.m., Maciej W. Rozycki wrote:
> Set the allocation length to 255 for the ATA Information VPD page
> requested in the WRITE SAME handler, so as not to limit information
> examined by `scsi_get_vpd_page' in the supported vital product data
> pages unnecessarily.
> 
> Originally it was thought that Areca hardware may have issues with a
> valid allocation length supplied for a VPD inquiry, however older SCSI
> standard revisions[1] consider 255 the maximum length allowed and what
> has later become the high order byte is considered reserved and must be
> zero with the INQUIRY command.  Therefore it was unnecessary to reduce
> the amount of data requested from 512 as far down as to 64, arbitrarily
> chosen, and 255 would as well do.

Not arbitrary, 64 bytes would get all the fields less the 512 byte ATA
DEVICE IDENTIFY data field.

> With commit b3ae8780b429 ("[SCSI] Add EVPD page 0x83 and 0x80 to sysfs")
> we have since got the SCSI_VPD_PG_LEN macro, so use that instead.
> 
> References:
> 
> [1] "Information technology - Small Computer System Interface - 2",
>      WORKING DRAFT, X3T9.2, Project 375D, Revision 10L, 7-SEP-93, Section
>      8.2.5 "INQUIRY command", pp.104-108

Yes, 1992, long withdrawn and only used by several billion USB keys.

But the ATA Information VPD page first appeared in SAT around 2006 and the
length of that page was (and still is in SAT-5 drafts) "238h" (572 bytes
long (when the 4 byte header is considered)). So it needs 16 bits to
represent that length. SPC-3 (2005) bumped the allocation length field in
the INQUIRY command from 8 to 16 bits.

Finally SAT-1 in its approved references [2.2] says:
ISO/IEC 14776-453, SCSI Primary Commands - 3 (SPC-3) [ANSI INCITS 408-2005]

So any SAT layer that supplies the ATA Information VPD page should also
support (translating) an INQUIRY with a 16 bit allocation field.

How does your problem arise? Could USB mass storage be involved?

And this patch solves your problem by returning part of the ATA DEVICE
IDENTIFY data (which is 512 bytes long)? If so, why not say so.

And what about using 0x2ff as the INQUIRY allocation length? If the
broken device ignores the top byte, you get 255 bytes back. If a
correct device takes both bytes it should return 0x23c bytes after
resid is taken into account.



Not related to this patch:
sd_read_write_same() seems a strange name for a function given that
it is checking on WRITE SAME support. How about s/read/report/ ?
And calling scsi_report_opcode() on INQUIRY seems a weird time waster
(it actually checks if the SCSI version is < SPC-3 or does the check
on a _mandatory_ command). And for modern disks scsi_report_opcode()
is called 5 times. Why not call the REPORT SUPPORTED OPERATION CODES
command once and cache its result? It would save 4 commands in every
disk setup (or revalidation).

Doug Gilbert


> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> Fixes: af73623f5f10 ("[SCSI] sd: Reduce buffer size for vpd request")
> Tested-by: Nick Alcock <nick.alcock@oracle.com>
> ---
> Changes from v2:
> 
> - Add Nick's Tested-by annotation.
> 
> No changes from v1.
> ---
>   drivers/scsi/sd.c |    5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> linux-scsi-write-same-vpd-buffer.diff
> Index: linux-macro/drivers/scsi/sd.c
> ===================================================================
> --- linux-macro.orig/drivers/scsi/sd.c
> +++ linux-macro/drivers/scsi/sd.c
> @@ -3101,16 +3101,13 @@ static void sd_read_write_same(struct sc
>   	}
>   
>   	if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, INQUIRY) < 0) {
> -		/* too large values might cause issues with arcmsr */
> -		int vpd_buf_len = 64;
> -
>   		sdev->no_report_opcodes = 1;
>   
>   		/* Disable WRITE SAME if REPORT SUPPORTED OPERATION
>   		 * CODES is unsupported and the device has an ATA
>   		 * Information VPD page (SAT).
>   		 */
> -		if (!scsi_get_vpd_page(sdev, 0x89, buffer, vpd_buf_len))
> +		if (!scsi_get_vpd_page(sdev, 0x89, buffer, SCSI_VPD_PG_LEN))
>   			sdev->no_write_same = 1;
>   	}
>   
> 


  reply	other threads:[~2022-01-03  4:16 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-02 23:23 [PATCH v3 0/3] Bring the BusLogic host bus adapter driver up to Y2021 Maciej W. Rozycki
2022-01-02 23:23 ` [PATCH v3 1/3] scsi: Provide for avoiding trailing allocation length with VPD inquiries Maciej W. Rozycki
2022-01-03  8:23   ` Christoph Hellwig
2022-01-04 16:39     ` Khalid Aziz and Shuah Khan
2022-01-04 17:20     ` Maciej W. Rozycki
2022-01-02 23:23 ` [PATCH v3 2/3] scsi: Avoid using reserved length byte " Maciej W. Rozycki
2022-01-02 23:23 ` [PATCH v3 3/3] scsi: Set allocation length to 255 for ATA Information VPD page Maciej W. Rozycki
2022-01-03  4:09   ` Douglas Gilbert [this message]
2022-01-03 21:06     ` Maciej W. Rozycki
2022-01-03 21:28       ` Martin K. Petersen
2022-01-04 13:52         ` Maciej W. Rozycki
2022-01-04 17:57           ` Martin K. Petersen
2022-01-06  4:13             ` Martin K. Petersen
2022-01-06  5:21               ` Damien Le Moal
2022-01-07 14:07                 ` Martin K. Petersen
2022-01-07 10:36               ` Maciej W. Rozycki
2022-01-07 14:03                 ` Martin K. Petersen
2022-01-10 12:00                   ` Maciej W. Rozycki
2022-01-10 15:48                     ` Martin K. Petersen
2022-01-03 21:37     ` Martin K. Petersen

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=d9eaa1f8-7abb-645b-d624-5069205c6983@interlog.com \
    --to=dgilbert@interlog.com \
    --cc=hch@lst.de \
    --cc=jejb@linux.ibm.com \
    --cc=khalid@gonehiking.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=macro@orcam.me.uk \
    --cc=martin.petersen@oracle.com \
    --cc=nix@esperi.org.uk \
    /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.