| Message ID | alpine.DEB.2.21.2201020030130.56863@angie.orcam.me.uk |
|---|---|
| State | New |
| Headers | show |
| Series | Bring the BusLogic host bus adapter driver up to Y2021 | expand |
On Sun, 2 Jan 2022, Douglas Gilbert wrote: > > 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. That may well be the case, however there is no justification given for the particular size of 64 bytes chosen either in the comment nearby or the change description associated with the commit referred this arrangement has originated from. At the time of my original submission I examined the relevant thread of discussion[1] including the patch submission itself[2], and just to be sure I have double-checked it now and there is no mention as to why this value was chosen. There is no associated macro that could give some explanation and which good coding style would expect rather than a magic number inline. So I do have all the reasons to conclude this value has indeed been arbitrarily chosen, don't I? > > 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. Well, this has surfaced in a setup where devices dated 199x are used, so I guess they have all the rights to use whatever standard was most recent, or say second most recent at the time as we need to factor in design lead times. > How does your problem arise? Could USB mass storage be involved? This command does crash the HBA involved where 1/3 and 2/3 have not been applied. No USB involved, just these proper SCSI (SPI) targets: scsi 0:0:0:0: Direct-Access IBM DDYS-T18350M SA5A PQ: 0 ANSI: 3 scsi 0:0:1:0: Direct-Access SEAGATE ST336607LW 0006 PQ: 0 ANSI: 3 scsi 0:0:5:0: Direct-Access IOMEGA ZIP 100 E.08 PQ: 0 ANSI: 2 as noted with 1/3 and 2/3. Not noted here as not directly relevant though, and this is because this change is a clean-up only, to have the buffer size consistent across the various `scsi_get_vpd_page' calls, by using the SCSI_VPD_PG_LEN macro defined meanwhile, that sets the maximum supported by older SCSI standard revisions (which can therefore be safely used without asking the device how much data it can/wants to actually return) and consequently devices implementing them. I noted in the original submission[3]: > Nix, > > I can see you're still around. Would you therefore please be so kind > as to verify this change with your Areca hardware if you still have it? > > It looks to me like you were thinking in the right direction with: > <https://lore.kernel.org/linux-scsi/87vc3nuipg.fsf@spindle.srvr.nix/>. > Sadly nobody seemed to have paid attention to your observation and neither > were different buffer sizes considered (or at least it wasn't mentioned in > the discussion). > > Maciej -- and Nix was kind enough to verify my proposal works just fine with the piece of hardware the commit referred addressed a problem with, so the replacement buffer size is as good as the original one while making code consistent. As you can see I did observe right away that the buffer size was not discussed. If you insist that the value of 64 stay, then please come up with a suitable macro name to define along with SCSI_VPD_PG_LEN that reflects the meaning of 64 in this context and I'll be happy to update 3/3 accordingly, but please explain why the value of 64 is any better than 255 here and the inconsistency with the buffer size justified. > 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. As I noted above, this is for consistency with other `scsi_get_vpd_page' calls and to avoid an inline magic number. If you think that it is not stated clearly enough in my change description and the change is otherwise acceptable, then I can update the explanation accordingly. > 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. I have verified (some of) the devices listed above to correctly reject `scsi_get_vpd_page' requests with allocation length exceeding 255, as required by the SCSI standard revision at their time. I can't speak of the INQUIRY command, as I haven't checked it in this context. Does my explanation clear your concerns? If so, then please advise how to proceed with this change. Thank you for your review. References: [1] "3.10.2 or 3.10.3: arcmsr failure at bootup / early userspace transition", <https://lore.kernel.org/linux-scsi/87r4ehfzhf.fsf@spindle.srvr.nix/> [2] "scsi disk: Use its own buffer for the vpd request", <https://lore.kernel.org/linux-scsi/51FA71E2.6010501@fastmail.fm/> [3] "scsi: Set allocation length to 255 for ATA Information VPD page", <https://lore.kernel.org/linux-scsi/alpine.DEB.2.21.2104141306130.44318@angie.orcam.me.uk/> Maciej
Maciej, > So I do have all the reasons to conclude this value has indeed been > arbitrarily chosen, don't I? The SAT spec defines the contents of the first part of the page. The trailing 512 bytes are defined in the ATA spec. > If you insist that the value of 64 stay, then please come up with a > suitable macro name to define along with SCSI_VPD_PG_LEN that reflects > the meaning of 64 in this context and I'll be happy to update 3/3 > accordingly, but please explain why the value of 64 is any better than > 255 here and the inconsistency with the buffer size justified. Can please you try the following patch?
Doug, > 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/ ? It was chosen to be consistent with all the other sd_read_$VPD() functions. sd_read_cache_type(), sd_read_block_limits(), etc. > 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). The call to validate INQUIRY is really to check whether REPORT SUPPORTED OPERATION CODES command is supported. > 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). I have some patches that clean up discovery and start using cached VPDs. I hadn't thought of caching the RSOC output. Will look into that. I held this series back since I was concerned about them clashing with Christoph's recent revalidate changes. I'll get them sent out shortly.
Martin, > > So I do have all the reasons to conclude this value has indeed been > > arbitrarily chosen, don't I? > > The SAT spec defines the contents of the first part of the page. The > trailing 512 bytes are defined in the ATA spec. I think that would best be reflected in code somehow as lone `64' doesn't say anything really. > > If you insist that the value of 64 stay, then please come up with a > > suitable macro name to define along with SCSI_VPD_PG_LEN that reflects > > the meaning of 64 in this context and I'll be happy to update 3/3 > > accordingly, but please explain why the value of 64 is any better than > > 255 here and the inconsistency with the buffer size justified. > > Can please you try the following patch? I have tried it and it's neutral, that is with 1/3 applied the HBA still works and with 1/3 removed it still breaks (2/3 and 3/3 obviously don't build anymore). Unsurprisingly, as it's the call to `scsi_get_vpd_page' rather than `scsi_get_vpd_buf' that causes an issue here. I think the latter function isn't called in my setup, and it's not clear to me anymore after so long why I didn't poke at it. It looks to me like the `retry_pg' code there can be gone now with your update in place as it duplicates buffer sizing, and with that included it'll be a nice clean-up. NB you'll need to adjust drivers/scsi/mpt3sas/mpt3sas_scsih.c accordingly if we are to move forward with this change, as it's another user of the SCSI_VPD_PG_LEN macro. Given what has been said in the discussion so far would you consider 2/3 and 3/3 unnecessary? In the course of verifying your change I have looked through our code again and found that inline magic numbers are there in several though not all places where `scsi_get_vpd_page' gets called, so I think it would make sense to get rid of them all at once with a single self-contained change. Thank you for your input and the extra fix. Maciej
Maciej, > I have tried it and it's neutral, that is with 1/3 applied the HBA still > works and with 1/3 removed it still breaks (2/3 and 3/3 obviously don't > build anymore). Unsurprisingly, as it's the call to `scsi_get_vpd_page' > rather than `scsi_get_vpd_buf' that causes an issue here. Oh, you'll also need a follow-on patch that uses the cached ATA Information VPD page. I'll try to get my full series out today. > NB you'll need to adjust drivers/scsi/mpt3sas/mpt3sas_scsih.c accordingly > if we are to move forward with this change, as it's another user of the > SCSI_VPD_PG_LEN macro. That'll also use cached information in my series.
Martin, > > Oh, you'll also need a follow-on patch that uses the cached ATA > > Information VPD page. I'll try to get my full series out today. > > I would really appreciate it if you would be willing give this a whirl: > > https://git.kernel.org/mkp/h/5.18/discovery I have tried your tree and it does not clobber the HBA anymore, however partitions (of the MS-DOS type) are not recognised with any of the disks including one holding the root device, so the system fails to mount the root filesystem and therefore does not complete booting: VFS: Cannot open root device "802" or unknown-block(8,2): error -6 Please append a correct "root=" boot option; here are the available partitions: 0800 17921835 sda driver: sd 0810 35843686 sdb driver: sd 0830 239816 sdd driver: sd 0b00 1048575 sr0 driver: sr Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(8,2) -- is that expected? Here's the relevant part of the boot log: scsi host0: BusLogic BT-958 scsi 0:0:0:0: Direct-Access IBM DDYS-T18350M SA5A PQ: 0 ANSI: 3 scsi 0:0:1:0: Direct-Access SEAGATE ST336607LW 0006 PQ: 0 ANSI: 3 scsi 0:0:4:0: Sequential-Access HP C5683A C908 PQ: 0 ANSI: 2 scsi 0:0:5:0: Direct-Access IOMEGA ZIP 100 E.08 PQ: 0 ANSI: 2 st: Version 20160209, fixed bufsize 32768, s/g segs 256 st 0:0:4:0: Attached scsi tape st0 st 0:0:4:0: st0: try direct i/o: yes (alignment 4 B) sd 0:0:0:0: [sda] 35843670 512-byte logical blocks: (18.4 GB/17.1 GiB) sd 0:0:5:0: [sdc] Media removed, stopped polling sd 0:0:1:0: [sdb] 71687372 512-byte logical blocks: (36.7 GB/34.2 GiB) sd 0:0:5:0: [sdc] Attached SCSI removable disk sd 0:0:0:0: [sda] Write Protect is off sd 0:0:1:0: [sdb] Write Protect is off sd 0:0:0:0: [sda] Mode Sense: cb 00 00 08 sd 0:0:1:0: [sdb] Mode Sense: ab 00 10 08 sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA sda: detected capacity change from 0 to 35843670 sd 0:0:0:0: [sda] Attached SCSI disk sd 0:0:1:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA sdb: detected capacity change from 0 to 71687372 sd 0:0:1:0: [sdb] Attached SCSI disk sd 0:0:0:0: Attached scsi generic sg0 type 0 sd 0:0:1:0: Attached scsi generic sg1 type 0 st 0:0:4:0: Attached scsi generic sg2 type 1 sd 0:0:5:0: Attached scsi generic sg3 type 0 while upon a succesful boot with the upstream kernel (and my patch(es) applied) it looks like: scsi host0: BusLogic BT-958 scsi 0:0:0:0: Direct-Access IBM DDYS-T18350M SA5A PQ: 0 ANSI: 3 scsi 0:0:1:0: Direct-Access SEAGATE ST336607LW 0006 PQ: 0 ANSI: 3 scsi 0:0:4:0: Sequential-Access HP C5683A C908 PQ: 0 ANSI: 2 scsi 0:0:5:0: Direct-Access IOMEGA ZIP 100 E.08 PQ: 0 ANSI: 2 st: Version 20160209, fixed bufsize 32768, s/g segs 256 st 0:0:4:0: Attached scsi tape st0 st 0:0:4:0: st0: try direct i/o: yes (alignment 4 B) sd 0:0:0:0: [sda] 35843670 512-byte logical blocks: (18.4 GB/17.1 GiB) sd 0:0:5:0: [sdc] Media removed, stopped polling sd 0:0:1:0: [sdb] 71687372 512-byte logical blocks: (36.7 GB/34.2 GiB) sd 0:0:0:0: [sda] Write Protect is off sd 0:0:1:0: [sdb] Write Protect is off sd 0:0:0:0: [sda] Mode Sense: cb 00 00 08 sd 0:0:1:0: [sdb] Mode Sense: ab 00 10 08 sd 0:0:5:0: [sdc] Attached SCSI removable disk sd 0:0:1:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA sdb: sdb1 sdb2 sd 0:0:1:0: [sdb] Attached SCSI disk sda: sda1 sda2 sda3 sda4 < sda5 sda6 sda7 sda8 sda9 sda10 > sd 0:0:0:0: [sda] Attached SCSI disk sd 0:0:0:0: Attached scsi generic sg0 type 0 sd 0:0:1:0: Attached scsi generic sg1 type 0 st 0:0:4:0: Attached scsi generic sg2 type 1 sd 0:0:5:0: Attached scsi generic sg3 type 0 The failure is not specific to this HBA as `hdd' is a PATA device and it doesn't get its partitions scanned either. There's no significant difference between the two .config files: --- ../linux-macro/.config +++ .config @@ -1,6 +1,6 @@ # # Automatically generated file; DO NOT EDIT. -# Linux/i386 5.16.0-rc7 Kernel Configuration +# Linux/i386 5.16.0-rc1 Kernel Configuration # CONFIG_CC_VERSION_TEXT="i386-linux-gnu-gcc (GCC) 11.0.0 20200919 (experimental)" CONFIG_CC_IS_GCC=y @@ -518,7 +518,6 @@ CONFIG_HAVE_ARCH_MMAP_RND_BITS=y CONFIG_HAVE_EXIT_THREAD=y CONFIG_ARCH_MMAP_RND_BITS=8 -CONFIG_PAGE_SIZE_LESS_THAN_64KB=y CONFIG_ISA_BUS_API=y CONFIG_CLONE_BACKWARDS=y CONFIG_OLD_SIGSUSPEND3=y @@ -1061,7 +1060,6 @@ # CONFIG_SCSI_MPI3MR is not set # CONFIG_SCSI_SMARTPQI is not set # CONFIG_SCSI_UFSHCD is not set -# CONFIG_SCSI_UFS_HWMON is not set # CONFIG_SCSI_HPTIOP is not set CONFIG_SCSI_BUSLOGIC=y # CONFIG_SCSI_FLASHPOINT is not set Shall I try anything else? Maciej
Maciej, > I have tried your tree and it does not clobber the HBA anymore, Excellent! > however partitions (of the MS-DOS type) are not recognised with any of > the disks including one holding the root device, so the system fails > to mount the root filesystem and therefore does not complete booting: My mistake. An unrelated change to the revalidate logic in the last patch. Fixed and pushed. For your Mylex issue I believe the first patch in the series is all that's needed: 06a471da0937 ("scsi: core: Query VPD size before getting full page") Thanks!
Damien, > That said, it is weird that scsi_get_vpd_page() does not call > scsi_device_supports_vpd(). The first patch in the series already makes that change. I noticed because the allocation for sd_read_cpr() is fairly big so it stuck out in my test runs while reworking scsi_get_vpd_page(). I didn't remove the conditional in sd_revalidate_disk(). While it is superfluous, I do like that the "fancy" protocol features are grouped. Guess we could switch it to a comment instead. I'll think about it...
Martin, > > however partitions (of the MS-DOS type) are not recognised with any of > > the disks including one holding the root device, so the system fails > > to mount the root filesystem and therefore does not complete booting: > > My mistake. An unrelated change to the revalidate logic in the last > patch. Fixed and pushed. No worries, I'm glad I helped catch it early. This version boots multi-user. > For your Mylex issue I believe the first patch in the series is all > that's needed: > > 06a471da0937 ("scsi: core: Query VPD size before getting full page") It is. Thanks for sorting out this issue! Maciej
Maciej, >> For your Mylex issue I believe the first patch in the series is all >> that's needed: >> >> 06a471da0937 ("scsi: core: Query VPD size before getting full page") > > It is. Thanks for sorting out this issue! Excellent, thanks for all the testing and for your work identifying this issue!
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; }
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. 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 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