Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Date:   Mon, 16 Oct 2023 09:05:31 +0200
From:   Christoph Hellwig <hch@lst.de>
To:     Milan Broz <gmazyland@gmail.com>
Cc:     linux-scsi@vger.kernel.org, jejb@linux.ibm.com,
        martin.petersen@oracle.com, hch@lst.de,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] scsi: use ATA-12 pass-thru for OPAL as fallback
Message-ID: <20231016070531.GA28537@lst.de>
References: <20231016070211.39502-1-gmazyland@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231016070211.39502-1-gmazyland@gmail.com>
User-Agent: Mutt/1.5.17 (2007-11-01)
Precedence: bulk

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.