2023-03-09 10:32:44

by Meadhbh Fitzpatrick

[permalink] [raw]
Subject: [PATCH] crypto: qat - change size of status variable

The `status` variable is of type int but it is used to store the
output of an s8 type returned by the functions
qat_comp_get_xlt_status() and qat_comp_get_cmp_status().

Declare `status` as `s8` to match the output type of the two functions.

This commit does not implement any functional changes.

Signed-off-by: Meadhbh Fitzpatrick <[email protected]>
Reviewed-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Damian Muszynski <[email protected]>
---
drivers/crypto/qat/qat_common/qat_comp_algs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/qat/qat_common/qat_comp_algs.c b/drivers/crypto/qat/qat_common/qat_comp_algs.c
index b533984906ec..726b71d2a4a8 100644
--- a/drivers/crypto/qat/qat_common/qat_comp_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_comp_algs.c
@@ -183,7 +183,7 @@ static void qat_comp_generic_callback(struct qat_compression_req *qat_req,
int consumed, produced;
s8 cmp_err, xlt_err;
int res = -EBADMSG;
- int status;
+ s8 status;
u8 cnv;

status = qat_comp_get_cmp_status(resp);
--
2.37.3

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.



2023-03-09 10:42:08

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH] crypto: qat - change size of status variable

On Thu, Mar 09, 2023 at 11:33:06AM +0000, Meadhbh Fitzpatrick wrote:
>
> diff --git a/drivers/crypto/qat/qat_common/qat_comp_algs.c b/drivers/crypto/qat/qat_common/qat_comp_algs.c
> index b533984906ec..726b71d2a4a8 100644
> --- a/drivers/crypto/qat/qat_common/qat_comp_algs.c
> +++ b/drivers/crypto/qat/qat_common/qat_comp_algs.c
> @@ -183,7 +183,7 @@ static void qat_comp_generic_callback(struct qat_compression_req *qat_req,
> int consumed, produced;
> s8 cmp_err, xlt_err;
> int res = -EBADMSG;
> - int status;
> + s8 status;

Sorry, but this makes no sense. Why are these s8's to begin
with? It doesn't look like you even allow negative values.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2023-03-09 11:10:48

by Cabiddu, Giovanni

[permalink] [raw]
Subject: Re: [PATCH] crypto: qat - change size of status variable

On Thu, Mar 09, 2023 at 06:39:45PM +0800, Herbert Xu wrote:
> On Thu, Mar 09, 2023 at 11:33:06AM +0000, Meadhbh Fitzpatrick wrote:
> >
> > diff --git a/drivers/crypto/qat/qat_common/qat_comp_algs.c b/drivers/crypto/qat/qat_common/qat_comp_algs.c
> > index b533984906ec..726b71d2a4a8 100644
> > --- a/drivers/crypto/qat/qat_common/qat_comp_algs.c
> > +++ b/drivers/crypto/qat/qat_common/qat_comp_algs.c
> > @@ -183,7 +183,7 @@ static void qat_comp_generic_callback(struct qat_compression_req *qat_req,
> > int consumed, produced;
> > s8 cmp_err, xlt_err;
> > int res = -EBADMSG;
> > - int status;
> > + s8 status;
>
> Sorry, but this makes no sense. Why are these s8's to begin
> with? It doesn't look like you even allow negative values.
You are right. Thanks for spotting this.
When reviewing it I mixed up the cmp_err and xlt_err with the status which
is a u8.

The functions qat_comp_get_cmp_status() and qat_comp_get_xlt_status()
should be changed to return an u8 as the field returned by the firmware
is an unsigned byte [1].

The `status` variable in qat_comp_generic_callback() can be changed to s8
or stay as int. It is just used to check if a value is set.

...
if (unlikely(status != ICP_QAT_FW_COMN_STATUS_FLAG_OK))
goto end;
...

where ICP_QAT_FW_COMN_STATUS_FLAG_OK is 0.

[1] https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/crypto/qat/qat_common/icp_qat_fw.h#L104

Regards,

--
Giovanni

2023-03-10 04:22:07

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH] crypto: qat - change size of status variable

On Thu, Mar 09, 2023 at 11:04:18AM +0000, Giovanni Cabiddu wrote:
>
> You are right. Thanks for spotting this.
> When reviewing it I mixed up the cmp_err and xlt_err with the status which
> is a u8.

In general s8/u8 should only be used when you are directly reading
or writing from hardware. Once the value has entered your driver,
you should use int/unsigned int to deal with them.

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