diff mbox

[dpdk-dev,v7,01/11] crypto/dpaa: replace rte_panic instances in crypto/dpaa driver

Message ID 1524608213-2080-2-git-send-email-arnon@qwilt.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Arnon Warshavsky April 24, 2018, 10:16 p.m. UTC 
  replace panic calls with log and return value.

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  8 +++++---
 drivers/crypto/dpaa_sec/dpaa_sec.c          | 10 ++++++----
 2 files changed, 11 insertions(+), 7 deletions(-)

Comments

Kevin Traynor April 26, 2018, 4:05 p.m. UTC | #1 
On 04/24/2018 11:16 PM, Arnon Warshavsky wrote:
> replace panic calls with log and return value.
> 
> Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
> ---
>  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  8 +++++---
>  drivers/crypto/dpaa_sec/dpaa_sec.c          | 10 ++++++----
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> index 58cbce8..a78f3a2 100644
> --- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> +++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> @@ -2883,9 +2883,11 @@ struct rte_security_ops dpaa2_sec_security_ops = {
>  					RTE_CACHE_LINE_SIZE,
>  					rte_socket_id());
>  
> -		if (cryptodev->data->dev_private == NULL)
> -			rte_panic("Cannot allocate memzone for private "
> -				  "device data");
> +		if (cryptodev->data->dev_private == NULL) {
> +			DPAA_SEC_ERR("%s() Cannot allocate memzone for private device data",
> +			__func__);
> +			return -ENOMEM;

I'm not familiar with the code but there was a successful allocate
already, so it seems you should jump to the cleanup section at the end
of the function before returning.

> +		}
>  	}
>  
>  	dpaa2_dev->cryptodev = cryptodev;
> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
> index e456fd5..a4670bf 100644
> --- a/drivers/crypto/dpaa_sec/dpaa_sec.c
> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
> @@ -2352,7 +2352,7 @@ struct rte_security_ops dpaa_sec_security_ops = {
>  		}
>  	}
>  
> -	RTE_LOG(INFO, PMD, "%s cryptodev init\n", cryptodev->data->name);
> +	DPAA_SEC_INFO("%s cryptodev init\n", cryptodev->data->name);

This fix is an unrelated to the patchset. Perhaps as it's trivial the
maintainer will allow it

>  	return 0;
>  
>  init_error:
> @@ -2384,9 +2384,11 @@ struct rte_security_ops dpaa_sec_security_ops = {
>  					RTE_CACHE_LINE_SIZE,
>  					rte_socket_id());
>  
> -		if (cryptodev->data->dev_private == NULL)
> -			rte_panic("Cannot allocate memzone for private "
> -					"device data");
> +		if (cryptodev->data->dev_private == NULL) {
> +			DPAA_SEC_ERR("%s() Cannot allocate memzone for private device data",
> +			__func__);
> +			return -ENOMEM;

Same comment as above

> +		}
>  	}
>  
>  	dpaa_dev->crypto_dev = cryptodev;
>
Kevin Traynor April 26, 2018, 4:16 p.m. UTC | #2 
On 04/26/2018 05:05 PM, Kevin Traynor wrote:
> On 04/24/2018 11:16 PM, Arnon Warshavsky wrote:
>> replace panic calls with log and return value.
>>

Replied to wrong version. Comments are still relevant for v9,

Kevin.

>> Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
>> ---
>>  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |  8 +++++---
>>  drivers/crypto/dpaa_sec/dpaa_sec.c          | 10 ++++++----
>>  2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
>> index 58cbce8..a78f3a2 100644
>> --- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
>> +++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
>> @@ -2883,9 +2883,11 @@ struct rte_security_ops dpaa2_sec_security_ops = {
>>  					RTE_CACHE_LINE_SIZE,
>>  					rte_socket_id());
>>  
>> -		if (cryptodev->data->dev_private == NULL)
>> -			rte_panic("Cannot allocate memzone for private "
>> -				  "device data");
>> +		if (cryptodev->data->dev_private == NULL) {
>> +			DPAA_SEC_ERR("%s() Cannot allocate memzone for private device data",
>> +			__func__);
>> +			return -ENOMEM;
> 
> I'm not familiar with the code but there was a successful allocate
> already, so it seems you should jump to the cleanup section at the end
> of the function before returning.
> 
>> +		}
>>  	}
>>  
>>  	dpaa2_dev->cryptodev = cryptodev;
>> diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
>> index e456fd5..a4670bf 100644
>> --- a/drivers/crypto/dpaa_sec/dpaa_sec.c
>> +++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
>> @@ -2352,7 +2352,7 @@ struct rte_security_ops dpaa_sec_security_ops = {
>>  		}
>>  	}
>>  
>> -	RTE_LOG(INFO, PMD, "%s cryptodev init\n", cryptodev->data->name);
>> +	DPAA_SEC_INFO("%s cryptodev init\n", cryptodev->data->name);
> 
> This fix is an unrelated to the patchset. Perhaps as it's trivial the
> maintainer will allow it
> 
>>  	return 0;
>>  
>>  init_error:
>> @@ -2384,9 +2384,11 @@ struct rte_security_ops dpaa_sec_security_ops = {
>>  					RTE_CACHE_LINE_SIZE,
>>  					rte_socket_id());
>>  
>> -		if (cryptodev->data->dev_private == NULL)
>> -			rte_panic("Cannot allocate memzone for private "
>> -					"device data");
>> +		if (cryptodev->data->dev_private == NULL) {
>> +			DPAA_SEC_ERR("%s() Cannot allocate memzone for private device data",
>> +			__func__);
>> +			return -ENOMEM;
> 
> Same comment as above
> 
>> +		}
>>  	}
>>  
>>  	dpaa_dev->crypto_dev = cryptodev;
>>
>
Arnon Warshavsky April 26, 2018, 9:28 p.m. UTC | #3 
> > -             if (cryptodev->data->dev_private == NULL)
> > -                     rte_panic("Cannot allocate memzone for private "
> > -                               "device data");
> > +             if (cryptodev->data->dev_private == NULL) {
> > +                     DPAA_SEC_ERR("%s() Cannot allocate memzone for
> private device data",
> > +                     __func__);
> > +                     return -ENOMEM;
>
> I'm not familiar with the code but there was a successful allocate
> already, so it seems you should jump to the cleanup section at the end
> of the function before returning.
>
> Hi Kevin,
The purpose of this patchset is not to offer a recoverable alternative for
panic,
rather allow the process to abort in an orderly manner.
It does not cover in this version all the panic instances on the init
sequence.
Other than in places where it seemed straight forward I tend not to perform
in this patchset
partial resource release where panic was before.

Thanks
/Arnon
Kevin Traynor April 27, 2018, 10:08 a.m. UTC | #4 
On 04/26/2018 10:28 PM, Arnon Warshavsky wrote:
> 
>     > -             if (cryptodev->data->dev_private == NULL)
>     > -                     rte_panic("Cannot allocate memzone for private "
>     > -                               "device data");
>     > +             if (cryptodev->data->dev_private == NULL) {
>     > +                     DPAA_SEC_ERR("%s() Cannot allocate memzone for private device data",
>     > +                     __func__);
>     > +                     return -ENOMEM;
> 
>     I'm not familiar with the code but there was a successful allocate
>     already, so it seems you should jump to the cleanup section at the end
>     of the function before returning.
> 
> Hi Kevin,
> The purpose of this patchset is not to offer a recoverable alternative
> for panic,
> rather allow the process to abort in an orderly manner.
> It does not cover in this version all the panic instances on the init
> sequence.
> Other than in places where it seemed straight forward I tend not to
> perform in this patchset
> partial resource release where panic was before.
> 

Ok, I understand the intention better now.

> Thanks
> /Arnon
> 
> 
>
diff mbox

Patch

diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
index 58cbce8..a78f3a2 100644
--- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
+++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
@@ -2883,9 +2883,11 @@  struct rte_security_ops dpaa2_sec_security_ops = {
 					RTE_CACHE_LINE_SIZE,
 					rte_socket_id());
 
-		if (cryptodev->data->dev_private == NULL)
-			rte_panic("Cannot allocate memzone for private "
-				  "device data");
+		if (cryptodev->data->dev_private == NULL) {
+			DPAA_SEC_ERR("%s() Cannot allocate memzone for private device data",
+			__func__);
+			return -ENOMEM;
+		}
 	}
 
 	dpaa2_dev->cryptodev = cryptodev;
diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
index e456fd5..a4670bf 100644
--- a/drivers/crypto/dpaa_sec/dpaa_sec.c
+++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
@@ -2352,7 +2352,7 @@  struct rte_security_ops dpaa_sec_security_ops = {
 		}
 	}
 
-	RTE_LOG(INFO, PMD, "%s cryptodev init\n", cryptodev->data->name);
+	DPAA_SEC_INFO("%s cryptodev init\n", cryptodev->data->name);
 	return 0;
 
 init_error:
@@ -2384,9 +2384,11 @@  struct rte_security_ops dpaa_sec_security_ops = {
 					RTE_CACHE_LINE_SIZE,
 					rte_socket_id());
 
-		if (cryptodev->data->dev_private == NULL)
-			rte_panic("Cannot allocate memzone for private "
-					"device data");
+		if (cryptodev->data->dev_private == NULL) {
+			DPAA_SEC_ERR("%s() Cannot allocate memzone for private device data",
+			__func__);
+			return -ENOMEM;
+		}
 	}
 
 	dpaa_dev->crypto_dev = cryptodev;