Date:   Thu, 9 Mar 2023 11:04:18 +0000
From:   Giovanni Cabiddu <giovanni.cabiddu@intel.com>
To:     Herbert Xu <herbert@gondor.apana.org.au>
CC:     Meadhbh Fitzpatrick <meadhbh.fitzpatrick@intel.com>,
        <linux-crypto@vger.kernel.org>,
        Damian Muszynski <damian.muszynski@intel.com>
Subject: Re: [PATCH] crypto: qat - change size of status variable
Message-ID: <ZAm9MpyFnU1q2pmc@gcabiddu-mobl1.ger.corp.intel.com>
References: <20230309113306.4008-1-meadhbh.fitzpatrick@intel.com>
 <ZAm3cSjNderM7gzn@gondor.apana.org.au>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZAm3cSjNderM7gzn@gondor.apana.org.au>
Organization: Intel Research and Development Ireland Ltd - Co. Reg. #308263 -
 Collinstown Industrial Park, Leixlip, County Kildare - Ireland
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?u5Sb7zMSyLx+cL1uagcOMEQONrXx660cgHS3MkihibBJaPC/pBMMVq5dH9W9?=
 =?us-ascii?Q?3lsVSmFNtXlHIMg7pNLlo6+fFVoEcv03W+UjvyoHCOZVyNz0MI1QyPrEqQ3p?=
 =?us-ascii?Q?C2ND6A8hHergV7zYvYEVFZ2vN6W+t3hDf1UwSoEVPgah+R+Ur9fmfBDdWbaX?=
 =?us-ascii?Q?VhFZD5v9HxmVWlFE/SdZ+/mWIMo6VuyebLSG0k0VM9yd7cgrV5H9A48OqiFZ?=
 =?us-ascii?Q?MfXzI4z/PvmOQqI9xjoqDF7ocpdXGdUAdBCzhidxBrpNvzHkIx/Bk2t5oW72?=
 =?us-ascii?Q?+N25f/XQBUoYrkxgfkyXejyhGY2l6YvtGGwoJlHTldzeQr9ZWwh8VCEFfyg2?=
 =?us-ascii?Q?AUQlwz2C1HCx2WHnpppvaXL1fcLxAR1i16YIKQHmEkTrdfBvfTMA4dk2Eltz?=
 =?us-ascii?Q?e+hpS2PW1SSa5xuoTdqOfvYoWaVBsyZUqP3Krt4v31kZ+ZISBCF6mZO2f60B?=
 =?us-ascii?Q?4wOzYBC8R9ufwfg++MQCaiSt21ak/AG05nhlTur2jVEkZpLV9JRjUlvD2Yjm?=
 =?us-ascii?Q?lfrMf+uosmG6nHnOUCA1ToSj+pDCztDc5gxiROUKftaYUR2lLVmSxbhFghoM?=
 =?us-ascii?Q?czskkjOdc7yPK9g6l2VeetNwWcOp+OWsSEGTEDSHXB6JWufTzvgAcbvk2aTC?=
 =?us-ascii?Q?e05FWaF9UhcadGWfZF4Bzo/tgX5v1wkv/r/c9umApwt6sp0wS3uAJzcxOXhc?=
 =?us-ascii?Q?gz/JcH8QXTV3ITisnq5j+gs3LPyARMxyA3C6bbvRFIrW2+F5cFpiCaZJ6H7r?=
 =?us-ascii?Q?LDgmawqrYY+pk5GaV1oYS92jjQQJu6wPwlak+htV1lxWIDglPniiG1eX7EU1?=
 =?us-ascii?Q?I20BppKRRaHGXJfAJ591E1CP7rsuISoJEKHR/ucJKnMJl0mn1VAtdI7eHZUA?=
 =?us-ascii?Q?cFQZM6Y9ahc0QKgvWAjt8s/rKw6LxNLyP7c4AK6bUtfuNffDV4FV6MyEGsLd?=
 =?us-ascii?Q?giV8QgFdMX+k1awZVRFMiQco9rUXb1tgNMz2+nzdwHCLEEBthgYZV2WUfJX5?=
 =?us-ascii?Q?8rBaC4zqcAW5eHF4oxbpz0o9ViyliLO7x8wWYUOuuSF6fNDfTHpR1+GWpaeX?=
 =?us-ascii?Q?P+6fEO78fxvHj/YgPx/n3HHE6j8F6opAuaxB1deGmbGvH3pB4Ipa6aAWrf0F?=
 =?us-ascii?Q?7/avGa4unIZMXBPayY7JC0UZaRP3YLY1WCEXaQ7PWL43G8IKlAs8gW5Di2Jo?=
 =?us-ascii?Q?+9l3F5oh3727/OcVf68Si1rnKgBTilZJpZjKS8pNeBDTqf1FKeBpr8E1fUGq?=
 =?us-ascii?Q?WQCpctOT1MHrA7EKDej4NKRHBr+ZVh5FvkTflpTG7uD6c+UlxbjCziEZQhFo?=
 =?us-ascii?Q?redqqASVIvc8ejPqW6GTb4TXqjug1xwzxSyFiV1PBg/2Gngyi7+7vSpxshd8?=
 =?us-ascii?Q?dpT53A6iz9D658QAkjd2QNq/yCBt62qm0dToF510rhsS2AFEMfozel53jtxd?=
 =?us-ascii?Q?ZgGAmRfKlu99jbrGxGVcluMyo+Aw6JIP+Vt4LvKPkZGtgQ23cCHfJT/puZeY?=
 =?us-ascii?Q?i4+hYXQPXLoJNp/qitu4WUdReUXylqZqYov50NdYk2g4JkimHkcRbrJObvT9?=
 =?us-ascii?Q?HscrUi9dbsRIQDIobSzIX3RhoZmyMIM5v564dUKq5acDqMQ3OZEVf2u91QzF?=
 =?us-ascii?Q?Lg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bdd186e1-5fd3-4122-ddb5-08db208e0e71
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6366.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2023 11:04:30.4720
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 5IFxpurhRiHft/7BcKle+yZk51RQBzVuBL8nK8f4RwECrf5Bgej3VffnOnEZ80ofBA0lZyMiSyFOmdvSBt9fsPkuTR1jh7DW1E8V+9s5+qs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6979
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

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