Re: [PATCH] scsi: use ATA-12 pass-thru for OPAL as fallback
From: Christoph Hellwig
Date: Mon Oct 16 2023 - 03:05:42 EST
On Mon, Oct 16, 2023 at 09:02:11AM +0200, Milan Broz wrote:
> All common USB/SATA or USB/NVMe adapters I tested need this patch.
>
> In short, these steps are run for OPAL support check:
> 1) Storage driver enables security driver flag (security_supported).
> USB-attached storage drivers will enable it in a separate patchset.
> SCSI and NNVMe drivers do it already. If the flag is not enabled,
> no following steps are run, and OPAL remains disabled.
> 2) SCSI device enumerates SECURITY IN/OUT command support. If detected,
> SECURITY ON/OUT wrapper is used (as in the current code).
> If not, new ATA-12 pass-thru wrapper is used instead.
> 3) SED OPAL code tries OPAL discovery command for the device.
> If it receives a correct reply, OPAL is enabled for the device.
> If SCSI SECURITY or ATA-12 command with discovery command is rejected,
> OPAL remains disabled.
>
> Note, USB attached storage needs an additional patchset sent separately
> as requested by USB driver maintainers (it contains required changes
> related to USB quirk processing).
This just feels wrong. These adapters are broken if they can't
translated, and we should not put ATA command submission into
sd.c.
> + cdb[0] = ATA_12;
> + cdb[1] = (send ? 5 /* ATA_PROTOCOL_PIO_DATA_IN */ : 4 /* ATA_PROTOCOL_PIO_DATA_OUT */) << 1;
> + cdb[2] = 2 /* t_length */ | (1 << 2) /* byt_blok */ | ((send ? 0 : 1) << 3) /* t_dir */;
> + cdb[3] = secp;
> + put_unaligned_le16(len / 512, &cdb[4]);
> + put_unaligned_le16(spsp, &cdb[6]);
> + cdb[9] = send ? 0x5e /* ATA_CMD_TRUSTED_SND */: 0x5c /* ATA_CMD_TRUSTED_RCV */;
Also avoid all these crazy long lines, and please use the actual
constants. Using a good old if/else is actually a very good way to
structure the code in a somewhat readable way.
> + if (sdkp->security)
> + sdkp->opal_dev = init_opal_dev(sdkp, &sd_sec_submit);
> + else
> + sdkp->opal_dev = init_opal_dev(sdkp, &sd_ata12_submit);
Messed up indentation here.
besides the fact that the statement is fundamentally wrong and you'll
start sending ATA command to random devices.