| Message ID | 20221108233339.412808-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | Prepare for upstreaming Pixel 6 and 7 UFS support | expand |
> From: Eric Biggers <ebiggers@google.com> > > Modify the UFSHCD core to allow 'struct ufshcd_sg_entry' to be > variable-length. The default is the standard length, but variants can > override ufs_hba::sg_entry_size with a larger value if there are > vendor-specific fields following the standard ones. > > This is needed to support inline encryption with ufs-exynos (FMP). > > Cc: Eric Biggers <ebiggers@google.com> > Signed-off-by: Eric Biggers <ebiggers@google.com> > [ bvanassche: edited commit message and introduced > CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE ] > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Avri Altman <avri.altman@wdc.com>
On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote: > From: Eric Biggers <ebiggers@google.com> > > Modify the UFSHCD core to allow 'struct ufshcd_sg_entry' to be > variable-length. The default is the standard length, but variants can > override ufs_hba::sg_entry_size with a larger value if there are > vendor-specific fields following the standard ones. There is absolutely nothing 'vendor' in here, it is all implementation specifc. I have no idea why no touching ufs can grasp this.
On 11/9/22 04:34, Christoph Hellwig wrote: > On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote: >> From: Eric Biggers <ebiggers@google.com> >> >> Modify the UFSHCD core to allow 'struct ufshcd_sg_entry' to be >> variable-length. The default is the standard length, but variants can >> override ufs_hba::sg_entry_size with a larger value if there are >> vendor-specific fields following the standard ones. > > There is absolutely nothing 'vendor' in here, it is all implementation > specifc. I have no idea why no touching ufs can grasp this. Hi Christoph, I'm not sure how to interpret your reply. Anyway, this patch is required to use the encryption functionality of the UFS Exynos controller. The "vendor-specific fields" text in the patch description refers to the encryption fields since these follow the data buffer when using the Exynos controller. Although it makes me unhappy that the UFS Exynos controller is not compliant with the UFS specification, since it is being used widely I think we need support for this controller in the upstream kernel. Thanks, Bart.
On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote: > diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig > index 4cc2dbd79ed0..49017abdac92 100644 > --- a/drivers/ufs/host/Kconfig > +++ b/drivers/ufs/host/Kconfig > @@ -124,3 +124,13 @@ config SCSI_UFS_EXYNOS > > Select this if you have UFS host controller on Samsung Exynos SoC. > If unsure, say N. > + > +config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE > + bool "Variable size UTP physical region descriptor" > + help > + In the UFSHCI 3.0 standard the Physical Region Descriptor (PRD) is a > + data structure used for transferring data between host and UFS > + device. This data structure describes a single region in physical > + memory. Although the standard requires that this data structure has a > + size of 16 bytes, for some controllers this data structure has a > + different size. Enable this option for UFS controllers that need it. This shouldn't be a user-selectable option. Just make it be enabled automatically when it is needed. Like this: config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE bool default y if SCSI_UFS_EXYNOS && SCSI_UFS_CRYPTO Also, this patch doesn't make sense without the code that actually needs it, so I hope you plan to send that upstream too. I haven't been able to do so yet, because no platform with this hardware actually can run the upstream kernel at all, as far as I know. Maybe commit 06874015327 ("arm64: dts: exynos: Add initial device tree support for Exynos7885 SoC") changed that? Any suggestions would be greatly appreciated... How are you testing this? > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index bd45818bf0e8..c6854514e40e 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -754,6 +754,9 @@ struct ufs_hba_monitor { > * @vops: pointer to variant specific operations > * @vps: pointer to variant specific parameters > * @priv: pointer to variant specific private data > +#ifdef CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE > + * @sg_entry_size: size of struct ufshcd_sg_entry (may include variant fields) > +#endif > * @irq: Irq number of the controller It doesn't make sense to have an #ifdef in the middle of a comment. - Eric
On 11/9/22 10:24, Eric Biggers wrote: > On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote: >> +config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE >> + bool "Variable size UTP physical region descriptor" >> + help >> + In the UFSHCI 3.0 standard the Physical Region Descriptor (PRD) is a >> + data structure used for transferring data between host and UFS >> + device. This data structure describes a single region in physical >> + memory. Although the standard requires that this data structure has a >> + size of 16 bytes, for some controllers this data structure has a >> + different size. Enable this option for UFS controllers that need it. > > This shouldn't be a user-selectable option. Just make it be enabled > automatically when it is needed. Like this: > > config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE > bool > default y if SCSI_UFS_EXYNOS && SCSI_UFS_CRYPTO Thanks Eric for having taken a look. I will include the above change in this patch when I repost it. > Also, this patch doesn't make sense without the code that actually needs it, so > I hope you plan to send that upstream too. I haven't been able to do so yet, > because no platform with this hardware actually can run the upstream kernel at > all, as far as I know. Maybe commit 06874015327 ("arm64: dts: exynos: Add > initial device tree support for Exynos7885 SoC") changed that? Any suggestions > would be greatly appreciated... How are you testing this? My approach for maintaining the UFS driver in the Android kernel tree is to keep the diffs between the upstream UFS driver and the driver in the Android kernel tree as small as possible. This patch has been tested in the same way as my other UFS driver patches, namely by applying it on the Android kernel tree and by testing the Android kernel tree. For this patch in particular "applying" came down to integrating the changes from this patch that are not yet in the Android kernel tree. Thanks, Bart.
On Wed, Nov 09, 2022 at 09:29:47AM -0800, Bart Van Assche wrote: > I'm not sure how to interpret your reply. Anyway, this patch is required to > use the encryption functionality of the UFS Exynos controller. The > "vendor-specific fields" text in the patch description refers to the > encryption fields since these follow the data buffer when using the Exynos > controller. Although it makes me unhappy that the UFS Exynos controller is > not compliant with the UFS specification, since it is being used widely I > think we need support for this controller in the upstream kernel. The fact that in UFS no one sticks to the standard, and not one but us in the kernel being more strict and your employer sticking to that can fix it. But that's not the point here - the point is that such fields are always implementation specific and never vendor specific. Any particular vendor can, and often does, have various different implementation specific derivations from or extensions to a spec. Just like there is no 'vendor' driver as there are plenty different drivers for the same device class from the same manufacturer.
> On Wed, Nov 09, 2022 at 09:29:47AM -0800, Bart Van Assche wrote: > > I'm not sure how to interpret your reply. Anyway, this patch is > > required to use the encryption functionality of the UFS Exynos > > controller. The "vendor-specific fields" text in the patch description > > refers to the encryption fields since these follow the data buffer > > when using the Exynos controller. Although it makes me unhappy that > > the UFS Exynos controller is not compliant with the UFS specification, > > since it is being used widely I think we need support for this controller in > the upstream kernel. > > The fact that in UFS no one sticks to the standard, and not one but us in the > kernel being more strict and your employer sticking to that can fix it. This part is usually done by hw. It is usually acceptable to temporarily use quirks while the hw get fixed. Thanks, Avri > > But that's not the point here - the point is that such fields are always > implementation specific and never vendor specific. Any particular vendor > can, and often does, have various different implementation specific > derivations from or extensions to a spec. > > Just like there is no 'vendor' driver as there are plenty different drivers for > the same device class from the same manufacturer.
On 11/14/22 23:48, Christoph Hellwig wrote: > On Wed, Nov 09, 2022 at 09:29:47AM -0800, Bart Van Assche wrote: >> I'm not sure how to interpret your reply. Anyway, this patch is required to >> use the encryption functionality of the UFS Exynos controller. The >> "vendor-specific fields" text in the patch description refers to the >> encryption fields since these follow the data buffer when using the Exynos >> controller. Although it makes me unhappy that the UFS Exynos controller is >> not compliant with the UFS specification, since it is being used widely I >> think we need support for this controller in the upstream kernel. > > The fact that in UFS no one sticks to the standard, and not one but > us in the kernel being more strict and your employer sticking to that > can fix it. The above statement is too strong. The Exynos controller is the least compliant controller I'm aware of. Future Google Pixel devices will use a UFS controller that is compliant with the UFSHCI standard. Additionally, the Exynos UFS controller also occurs in devices produced by other companies than my employer. > But that's not the point here - the point is that such fields are > always implementation specific and never vendor specific. Any > particular vendor can, and often does, have various different > implementation specific derivations from or extensions to a spec. > > Just like there is no 'vendor' driver as there are plenty different > drivers for the same device class from the same manufacturer. Do you perhaps want me to change what Eric Biggers suggested into something more generic? I'm referring to the following suggestion from Eric: config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE bool default y if SCSI_UFS_EXYNOS && SCSI_UFS_CRYPTO Thanks, Bart.
On 11/15/22 00:56, Avri Altman wrote:
> It is usually acceptable to temporarily use quirks while the hw get fixed.
The behavior supported by this patch is not a hardware bug - my
understanding is that it is behavior that has been chosen on purpose by
the Exynos design team.
I can convert this patch such that it uses a "quirk" flag instead of a
kernel config option to enable support for Exynos encryption. However,
doing that will add runtime overhead in the hot path for all UFS
vendors, including those that are compliant with the UFSHCI
specification. Is everyone OK with this?
Thanks,
Bart.
> On 11/15/22 00:56, Avri Altman wrote: > > It is usually acceptable to temporarily use quirks while the hw get fixed. > > The behavior supported by this patch is not a hardware bug - my understanding > is that it is behavior that has been chosen on purpose by the Exynos design > team. > > I can convert this patch such that it uses a "quirk" flag instead of a kernel config > option to enable support for Exynos encryption. However, doing that will add > runtime overhead in the hot path for all UFS vendors, including those that are > compliant with the UFSHCI specification. Is everyone OK with this? No - I guess not. Thanks, Avri > > Thanks, > > Bart.