Re: [PATCH v3 6/8] crypto: qat - simplify adf_enable_aer()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 11, 2021 at 10:06:35AM +0200, Uwe Kleine-König wrote:
> A struct pci_driver is supposed to be constant and assigning .err_handler
> once per bound device isn't really sensible. Also as the function returns
> zero unconditionally let it return no value instead and simplify the
> callers accordingly.
> 
> As a side effect this removes one user of struct pci_dev::driver. This
> member is planned to be removed.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
Thanks Uwe for the rework.

> ---
>  drivers/crypto/qat/qat_4xxx/adf_drv.c          |  7 ++-----
>  drivers/crypto/qat/qat_c3xxx/adf_drv.c         |  7 ++-----
>  drivers/crypto/qat/qat_c62x/adf_drv.c          |  7 ++-----
>  drivers/crypto/qat/qat_common/adf_aer.c        | 10 +++-------
>  drivers/crypto/qat/qat_common/adf_common_drv.h |  2 +-
>  drivers/crypto/qat/qat_dh895xcc/adf_drv.c      |  7 ++-----
>  6 files changed, 12 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/crypto/qat/qat_4xxx/adf_drv.c b/drivers/crypto/qat/qat_4xxx/adf_drv.c
> index a8805c815d16..1620281a9ed8 100644
> --- a/drivers/crypto/qat/qat_4xxx/adf_drv.c
> +++ b/drivers/crypto/qat/qat_4xxx/adf_drv.c
> @@ -253,11 +253,7 @@ static int adf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	pci_set_master(pdev);
>  
> -	if (adf_enable_aer(accel_dev)) {
> -		dev_err(&pdev->dev, "Failed to enable aer.\n");
> -		ret = -EFAULT;
> -		goto out_err;
> -	}
> +	adf_enable_aer(accel_dev);
After seeing your patch, I'm thinking to get rid of both adf_enable_aer()
(and adf_disable_aer()) and call directly pci_enable_pcie_error_reporting()
here.

There is another patch around this area that I reworked (but not sent
yet - https://patchwork.kernel.org/project/linux-crypto/patch/a19132d07a65dbef5e818f84b2bc971f26cc3805.1625983602.git.christophe.jaillet@xxxxxxxxxx/).
Would you mind if your patch goes through Herbert's tree?
If you want I can also send a reworked version.

>  	if (pci_save_state(pdev)) {
>  		dev_err(&pdev->dev, "Failed to save pci state.\n");
> @@ -310,6 +306,7 @@ static struct pci_driver adf_driver = {
>  	.probe = adf_probe,
>  	.remove = adf_remove,
>  	.sriov_configure = adf_sriov_configure,
> +	.err_handler = adf_err_handler,
Compilation fails here:
    drivers/crypto/qat/qat_4xxx/adf_drv.c:309:24: error: ‘adf_err_handler’ undeclared here (not in a function)
      309 |         .err_handler = adf_err_handler,
          |                        ^~~~~~~~~~~~~~~

Were you thinking to change it this way?

	--- a/drivers/crypto/qat/qat_common/adf_common_drv.h
	+++ b/drivers/crypto/qat/qat_common/adf_common_drv.h
	@@ -95,8 +95,11 @@ void adf_ae_fw_release(struct adf_accel_dev *accel_dev);
	 int adf_ae_start(struct adf_accel_dev *accel_dev);
	 int adf_ae_stop(struct adf_accel_dev *accel_dev);

	+extern const struct pci_error_handlers adf_err_handler;

	--- a/drivers/crypto/qat/qat_4xxx/adf_drv.c
	+++ b/drivers/crypto/qat/qat_4xxx/adf_drv.c
	@@ -306,7 +306,7 @@ static struct pci_driver adf_driver = {
		.probe = adf_probe,
		.remove = adf_remove,
		.sriov_configure = adf_sriov_configure,
	-       .err_handler = adf_err_handler,
	+       .err_handler = &adf_err_handler,
	 };

I think the main reason why the driver was dereferencing the pci_driver
in adf_enable_aer() was to avoid that extern struct in adf_common_drv.h.
I am ok with that. Any objections from others?

Regards,

-- 
Giovanni



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux