Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Yosry Ahmed <yosry@kernel.org>]

> > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > return ret;
> > >
> > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > Does crypto_alloc_acomp_node() ever return NULL?
> > Looking at the error handling just below this check, if this were to
> > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > the function to incorrectly return 0 (success) instead of an error code,
> > hiding the allocation failure.
> 
> This is a good catch. Just to provide context, this patch was
> introduced based on Yosry's earlier comments in [1].
> 
> [1]: https://patchwork.kernel.org/comment/26282128/
> 
> crypto_alloc_acomp_node() currently does not return NULL. However, it
> could, in future.
> Since the rest of zswap_cpu_comp_prepare() dereferences
> acomp_ctx->acomp, it depends on whether we want to future-proof the
> code to handle a possible eventuality of crypto_alloc_acomp_node()
> returning NULL.

Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
better documentation for the API, and the incossitency between this code
and acomp_ctx_dealloc() is arguably documenting that the function can
only return an ERR, but it can also be NULL-initialized by zswap.

> 
> If the maintainers think future-proofing is beneficial, I would need
> to handle the PTR_ERR(NULL) which would send a false success status.
> If we don't think we need to handle a future NULL return from
> crypto_alloc_acomp_node(), then I don't think this change is needed.
> We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> maintainers' inputs on how to proceed.
> 
> > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > - if (!acomp_ctx->req) {
> > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > Is this change necessary for acomp_request_alloc()?
> > This function strictly returns NULL on allocation failure, not an error
> > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > without providing a functional benefit.
> 
> As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> of an error. Same question as above. The only benefit would be making
> the code more robust to handle changes in the acomp API in future.

For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
to begin with? If acomp_request_alloc() only returns NULL, maybe that
should also be a NULL check?

In this case, we don't really need to make any changes here, and I think
this patch can just be dropped.