Re: [PATCH v2 12/15] crypto: iaa - Add support for iaa_crypto deflate compression algorithm

From: Herbert Xu <herbert@gondor.apana.org.au>
To: Tom Zanussi <tom.zanussi@linux.intel.com>
Cc: davem@davemloft.net, fenghua.yu@intel.com, vkoul@kernel.org,
	dave.jiang@intel.com, tony.luck@intel.com,
	wajdi.k.feghali@intel.com, james.guilford@intel.com,
	kanchana.p.sridhar@intel.com, giovanni.cabiddu@intel.com,
	linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
	dmaengine@vger.kernel.org
Subject: Re: [PATCH v2 12/15] crypto: iaa - Add support for iaa_crypto deflate compression algorithm
Date: Thu, 6 Apr 2023 16:00:38 +0800	[thread overview]
Message-ID: <ZC58JggIXgpJ1tpD@gondor.apana.org.au> (raw)
In-Reply-To: <20230328153535.126223-13-tom.zanussi@linux.intel.com>

On Tue, Mar 28, 2023 at 10:35:32AM -0500, Tom Zanussi wrote:
>
> @@ -881,12 +1574,26 @@ static int iaa_crypto_probe(struct idxd_dev *idxd_dev)
>  
>  	rebalance_wq_table();
>  
> +	if (first_wq) {
> +		iaa_crypto_enabled = true;
> +		ret = iaa_register_compression_device();
> +		if (ret != 0) {
> +			iaa_crypto_enabled = false;
> +			dev_dbg(dev, "IAA compression device registration failed\n");
> +			goto err_register;
> +		}
> +
> +		pr_info("iaa_crypto now ENABLED\n");
> +	}
> +

Sorry for picking on your driver but I've got to start somewhere :)

A long standing problem shared by almost all crypto drivers is that
the hardware removal handling is completely broken.

This is because hardware can be removed at any time, including during
a crypto operatin.  So drivers must work carefully around that fact.

Here is a recipe for dealing with this safely:

1) Never unregister your crypto algorithms, even after the last
piece of hardware has been unplugged.  The algorithms should only
be unregistered (if they have been registered through the first
successful probe call) in the module unload function.

2) Never free any software state for your hardware without some form
of synchronisation with oustanding operations.

Any mechanism would do, for example, you could use a spinlock if the
critical path isn't very long.  The operational path would take the
lock, check the hardware state, and if present proceed with the
operation (but still being prepared to cope if the hardware goes
AWAL because even if the driver state is still present the actual
hardware may be gone already).

Then the removal path would simply take the spinlock, set a flag
indicating the hardware is gone and then you could safely unlock
and free your driver states.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt