[dpdk-dev,v7,01/11] crypto/dpaa: replace rte_panic instances in crypto/dpaa driver
Checks
Commit Message
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
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;
>
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;
>>
>
> > - 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
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
>
>
>
@@ -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;
@@ -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;