Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN

[Bartosz Golaszewski]

On Tue, 21 Jul 2026 20:29:39 +0200, Demi Marie Obenour
<demiobenour@xxxxxxxxx> said:
> On 7/21/26 04:24, Bartosz Golaszewski wrote:
>> On Fri, Jul 17, 2026 at 06:18:48PM +1000, Herbert Xu wrote:
>>> On Sun, Jul 12, 2026 at 05:31:31PM -0400, Demi Marie Obenour via B4 Relay wrote:
>>>> From: Demi Marie Obenour <demiobenour@xxxxxxxxx>
>>>>
>>>> This driver is harmful:
>>>>
>>>> - It is much slower than the CPU [1] [2].
>>>> - It Has a history of bugs [2] [3].
>>>> - It does not have exclusive access to the hardware [4], causing races
>>>>   with the secure world.
>>>> - It register its implementations with too low a cra_priority for them
>>>>   to be actually used [5].
>>>>
>>>> Therefore, disable it to ensure that nobody builds it into kernels they
>>>> intend to ship.
>>>>
>>>> In the future, the driver will be used for processing restricted media
>>>> content.  However, the kernel does not currently support this.  Since
>>>> the driver will have future uses, allow building it if COMPILE_TEST is
>>>> enabled.
>>>>
>>>> [1]: https://lore.kernel.org/r/20250704070322.20692-1-ebiggers@xxxxxxxxxx/
>>>> [2]: https://lore.kernel.org/r/20250615031807.GA81869@sol/
>>>> [3]: https://lore.kernel.org/r/20260706-qce-fix-self-tests-v5-0-86f461ff1829@xxxxxxxxxxxxxxxx/
>>>> [4]: https://lore.kernel.org/r/20260629-qcom-qce-cmd-descr-v20-0-56f67da84c05@xxxxxxxxxxxxxxxx/
>>>> [5]: https://lore.kernel.org/r/20260524204537.GB110177@quark/
>>>>
>>>> Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
>>>> Signed-off-by: Demi Marie Obenour <demiobenour@xxxxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>> - Add Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
>>>> - Add Cc: stable@xxxxxxxxxxxxxxx
>>>> - Link to v1: https://lore.kernel.org/r/20260712-qce-broken-v1-1-85e2bff17871@xxxxxxxxx
>>>> ---
>>>>  arch/arm/configs/multi_v7_defconfig | 1 -
>>>>  arch/arm64/configs/defconfig        | 1 -
>>>>  drivers/crypto/Kconfig              | 6 +++++-
>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> Patch applied.  Thanks.
>>
>> Hi Herbert,
>>
>> I'd like to ask you to back this patch out. Admittedly I should have NAKed this
>> under the v1 thread but I thought the discussion we had under the Shikra series
>> and the subsequent patch making myself the maintainer of this driver (note: Demi
>> did not Cc me on this nor the Qcom mailing list at linux-arm-msm) as well as the
>> pending series[1] fixing the QCE issues was enough to imply we should not mark
>> it as broken. Please consider applying [1] if the fixes in v6 are correct.
>>
>> Bartosz
>>
>> [1] https://lore.kernel.org/all/20260717-qce-fix-self-tests-v6-0-455775fe5f6c@xxxxxxxxxxxxxxxx/
>
> Is the BAM locking patchset also needed?  My understanding is that
> without it, the QCE driver can still produce wrong results due to races
> with the TrustZone firmware.  Until it is merged and the self-tests
> are fixed, the driver definitely needs to be marked BROKEN.
>

No, we currently don't ship TZ firmware that could cause such interactions.
The only driver that currently suffers from such races is the NAND driver
which races with the modem firmware and it too will use the BAM locking
feature once it's upstream. However we do plan to support DRM using this IP
and this is when it'll need to account for possible races. IOW: BAM locking
is not strictly required to fix the QCE.

> Also, what can the current driver do that cannot be done better using
> software crypto?  I understand that you have future plans to add more
> features to the driver, but those are not relevant to this discussion.
>

We can offload crypto operations to the QCE saving CPU cycles but more
importantly, the QCE provides HW based isolation which some vendors care
about.

> Finally, I very much appreciate the work you have done.  I don't
> expect that the QCE driver will stay BROKEN forever.  This patch is
> purely a reflection of the current state of the driver, the speed of
> CPU-accelerated crypto, and the small message sizes used by the kernel.
>

That may have been true when you sent the v1 but with a set of proposed fixes
it'll no longer be the case.

> That said, is drivers/crypto really the right place for a restricted
> content driver?  A driver that uses dmabufs might be a better
> alternative, and would be easier to integrate with the Direct Rendering
> Manager (DRM) and Video for Linux 2 (V4L2) subsystems.
> --

These are future plans, yes. But right now we do have active users of this
IP and now also a maintainer - yours truly - who stepped up and is actively
working on this module. With the fixes applied, the driver will again pass
crypto self-tests and should not be BROKEN.

Thanks,
Bartosz