| Message ID | 20210927204326.612555-7-uwe@kleine-koenig.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | PCI: Drop duplicated tracking of a pci_dev's bound driver | expand |
Hi Uwe, On Mon, Sep 27, 2021 at 10:43:24PM +0200, Uwe Kleine-König wrote: > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > 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@pengutronix.de> > --- this patch does not build. drivers/crypto/qat/qat_c3xxx/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) 36 | .err_handler = adf_err_handler, | ^~~~~~~~~~~~~~~ drivers/crypto/qat/qat_4xxx/adf_drv.c:303:24: error: ‘adf_err_handler’ undeclared here (not in a function) 303 | .err_handler = adf_err_handler, | ^~~~~~~~~~~~~~~ drivers/crypto/qat/qat_c62x/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) 36 | .err_handler = adf_err_handler, | ^~~~~~~~~~~~~~~ drivers/crypto/qat/qat_dh895xcc/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) 36 | .err_handler = adf_err_handler, | ^~~~~~~~~~~~~~~ make[2]: *** [scripts/Makefile.build:277: drivers/crypto/qat/qat_c3xxx/adf_drv.o] Error 1 Below is an updated version of your patch that fixes the issues. After fixing the patch you can add: Acked-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com> Regards, GC ----8<---- From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de> Date: Mon, 27 Sep 2021 22:43:24 +0200 Subject: [PATCH] crypto: qat - simplify adf_enable_aer() 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@pengutronix.de> --- 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 | 4 +++- drivers/crypto/qat/qat_dh895xcc/adf_drv.c | 7 ++----- 6 files changed, 14 insertions(+), 28 deletions(-) diff --git a/drivers/crypto/qat/qat_4xxx/adf_drv.c b/drivers/crypto/qat/qat_4xxx/adf_drv.c index 359fb7989dfb..71ef065914b2 100644 --- a/drivers/crypto/qat/qat_4xxx/adf_drv.c +++ b/drivers/crypto/qat/qat_4xxx/adf_drv.c @@ -247,11 +247,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); if (pci_save_state(pdev)) { dev_err(&pdev->dev, "Failed to save pci state.\n"); @@ -304,6 +300,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = &adf_err_handler, }; module_pci_driver(adf_driver); diff --git a/drivers/crypto/qat/qat_c3xxx/adf_drv.c b/drivers/crypto/qat/qat_c3xxx/adf_drv.c index cc6e75dc60de..2aef0bb791df 100644 --- a/drivers/crypto/qat/qat_c3xxx/adf_drv.c +++ b/drivers/crypto/qat/qat_c3xxx/adf_drv.c @@ -33,6 +33,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = &adf_err_handler, }; static void adf_cleanup_pci_dev(struct adf_accel_dev *accel_dev) @@ -192,11 +193,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_free_reg; - } + adf_enable_aer(accel_dev); if (pci_save_state(pdev)) { dev_err(&pdev->dev, "Failed to save pci state\n"); diff --git a/drivers/crypto/qat/qat_c62x/adf_drv.c b/drivers/crypto/qat/qat_c62x/adf_drv.c index bf251dfe74b3..56163083f161 100644 --- a/drivers/crypto/qat/qat_c62x/adf_drv.c +++ b/drivers/crypto/qat/qat_c62x/adf_drv.c @@ -33,6 +33,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = &adf_err_handler, }; static void adf_cleanup_pci_dev(struct adf_accel_dev *accel_dev) @@ -192,11 +193,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_free_reg; - } + adf_enable_aer(accel_dev); if (pci_save_state(pdev)) { dev_err(&pdev->dev, "Failed to save pci state\n"); diff --git a/drivers/crypto/qat/qat_common/adf_aer.c b/drivers/crypto/qat/qat_common/adf_aer.c index ed3e40bc56eb..fe9bb2f3536a 100644 --- a/drivers/crypto/qat/qat_common/adf_aer.c +++ b/drivers/crypto/qat/qat_common/adf_aer.c @@ -166,11 +166,12 @@ static void adf_resume(struct pci_dev *pdev) dev_info(&pdev->dev, "Device is up and running\n"); } -static const struct pci_error_handlers adf_err_handler = { +const struct pci_error_handlers adf_err_handler = { .error_detected = adf_error_detected, .slot_reset = adf_slot_reset, .resume = adf_resume, }; +EXPORT_SYMBOL_GPL(adf_err_handler); /** * adf_enable_aer() - Enable Advance Error Reporting for acceleration device @@ -179,17 +180,12 @@ static const struct pci_error_handlers adf_err_handler = { * Function enables PCI Advance Error Reporting for the * QAT acceleration device accel_dev. * To be used by QAT device specific drivers. - * - * Return: 0 on success, error code otherwise. */ -int adf_enable_aer(struct adf_accel_dev *accel_dev) +void adf_enable_aer(struct adf_accel_dev *accel_dev) { struct pci_dev *pdev = accel_to_pci_dev(accel_dev); - struct pci_driver *pdrv = pdev->driver; - pdrv->err_handler = &adf_err_handler; pci_enable_pcie_error_reporting(pdev); - return 0; } EXPORT_SYMBOL_GPL(adf_enable_aer); diff --git a/drivers/crypto/qat/qat_common/adf_common_drv.h b/drivers/crypto/qat/qat_common/adf_common_drv.h index 4261749fae8d..2141e0b22315 100644 --- a/drivers/crypto/qat/qat_common/adf_common_drv.h +++ b/drivers/crypto/qat/qat_common/adf_common_drv.h @@ -95,7 +95,9 @@ 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); -int adf_enable_aer(struct adf_accel_dev *accel_dev); +extern const struct pci_error_handlers adf_err_handler; + +void adf_enable_aer(struct adf_accel_dev *accel_dev); void adf_disable_aer(struct adf_accel_dev *accel_dev); void adf_reset_sbr(struct adf_accel_dev *accel_dev); void adf_reset_flr(struct adf_accel_dev *accel_dev); diff --git a/drivers/crypto/qat/qat_dh895xcc/adf_drv.c b/drivers/crypto/qat/qat_dh895xcc/adf_drv.c index 3976a81bd99b..acca56752aa0 100644 --- a/drivers/crypto/qat/qat_dh895xcc/adf_drv.c +++ b/drivers/crypto/qat/qat_dh895xcc/adf_drv.c @@ -33,6 +33,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = &adf_err_handler, }; static void adf_cleanup_pci_dev(struct adf_accel_dev *accel_dev) @@ -192,11 +193,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_free_reg; - } + adf_enable_aer(accel_dev); if (pci_save_state(pdev)) { dev_err(&pdev->dev, "Failed to save pci state\n");
Hey Giovanni, On Tue, Sep 28, 2021 at 12:11:38PM +0100, Giovanni Cabiddu wrote: > On Mon, Sep 27, 2021 at 10:43:24PM +0200, Uwe Kleine-König wrote: > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > 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@pengutronix.de> > > --- > > this patch does not build. > > drivers/crypto/qat/qat_c3xxx/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) > 36 | .err_handler = adf_err_handler, > | ^~~~~~~~~~~~~~~ > drivers/crypto/qat/qat_4xxx/adf_drv.c:303:24: error: ‘adf_err_handler’ undeclared here (not in a function) > 303 | .err_handler = adf_err_handler, > | ^~~~~~~~~~~~~~~ > drivers/crypto/qat/qat_c62x/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) > 36 | .err_handler = adf_err_handler, > | ^~~~~~~~~~~~~~~ > drivers/crypto/qat/qat_dh895xcc/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) > 36 | .err_handler = adf_err_handler, > | ^~~~~~~~~~~~~~~ > make[2]: *** [scripts/Makefile.build:277: drivers/crypto/qat/qat_c3xxx/adf_drv.o] Error 1 Hmm, I thought that was one of the things I actually tested after rebasing. Will recheck. > Below is an updated version of your patch that fixes the issues. > > After fixing the patch you can add: > Acked-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com> Thanks Uwe
Hello, On Tue, Sep 28, 2021 at 01:26:37PM +0200, Uwe Kleine-König wrote: > On Tue, Sep 28, 2021 at 12:11:38PM +0100, Giovanni Cabiddu wrote: > > On Mon, Sep 27, 2021 at 10:43:24PM +0200, Uwe Kleine-König wrote: > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > > > 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@pengutronix.de> > > > --- > > > > this patch does not build. > > > > drivers/crypto/qat/qat_c3xxx/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) > > 36 | .err_handler = adf_err_handler, > > | ^~~~~~~~~~~~~~~ > > drivers/crypto/qat/qat_4xxx/adf_drv.c:303:24: error: ‘adf_err_handler’ undeclared here (not in a function) > > 303 | .err_handler = adf_err_handler, > > | ^~~~~~~~~~~~~~~ > > drivers/crypto/qat/qat_c62x/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) > > 36 | .err_handler = adf_err_handler, > > | ^~~~~~~~~~~~~~~ > > drivers/crypto/qat/qat_dh895xcc/adf_drv.c:36:24: error: ‘adf_err_handler’ undeclared here (not in a function) > > 36 | .err_handler = adf_err_handler, > > | ^~~~~~~~~~~~~~~ > > make[2]: *** [scripts/Makefile.build:277: drivers/crypto/qat/qat_c3xxx/adf_drv.o] Error 1 > > Hmm, I thought that was one of the things I actually tested after > rebasing. Will recheck. I fixed it now, comparing with your patch the only thing I did differently is ordering in the header file. I pushed the fixed series to https://git.pengutronix.de/git/ukl/linux pci-dedup Thanks Uwe
On Tue, Sep 28, 2021 at 03:57:04PM +0200, Uwe Kleine-König wrote: > I fixed it now, comparing with your patch the only thing I did > differently is ordering in the header file. > > I pushed the fixed series to > > https://git.pengutronix.de/git/ukl/linux pci-dedup Would you mind sending the new version to the ML? Thanks,
On 9/28/21 5:01 PM, Giovanni Cabiddu wrote: > On Tue, Sep 28, 2021 at 03:57:04PM +0200, Uwe Kleine-König wrote: >> I fixed it now, comparing with your patch the only thing I did >> differently is ordering in the header file. >> >> I pushed the fixed series to >> >> https://git.pengutronix.de/git/ukl/linux pci-dedup > Would you mind sending the new version to the ML? No, I don't mind. I will just wait a bit more for more feedback and not to annoy in a high frequency :-) Best regards Uwe
diff --git a/drivers/crypto/qat/qat_4xxx/adf_drv.c b/drivers/crypto/qat/qat_4xxx/adf_drv.c index 359fb7989dfb..b401f7541699 100644 --- a/drivers/crypto/qat/qat_4xxx/adf_drv.c +++ b/drivers/crypto/qat/qat_4xxx/adf_drv.c @@ -247,11 +247,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); if (pci_save_state(pdev)) { dev_err(&pdev->dev, "Failed to save pci state.\n"); @@ -304,6 +300,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = adf_err_handler, }; module_pci_driver(adf_driver); diff --git a/drivers/crypto/qat/qat_c3xxx/adf_drv.c b/drivers/crypto/qat/qat_c3xxx/adf_drv.c index cc6e75dc60de..4570b644574d 100644 --- a/drivers/crypto/qat/qat_c3xxx/adf_drv.c +++ b/drivers/crypto/qat/qat_c3xxx/adf_drv.c @@ -33,6 +33,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = adf_err_handler, }; static void adf_cleanup_pci_dev(struct adf_accel_dev *accel_dev) @@ -192,11 +193,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_free_reg; - } + adf_enable_aer(accel_dev); if (pci_save_state(pdev)) { dev_err(&pdev->dev, "Failed to save pci state\n"); diff --git a/drivers/crypto/qat/qat_c62x/adf_drv.c b/drivers/crypto/qat/qat_c62x/adf_drv.c index bf251dfe74b3..80de12e3e552 100644 --- a/drivers/crypto/qat/qat_c62x/adf_drv.c +++ b/drivers/crypto/qat/qat_c62x/adf_drv.c @@ -33,6 +33,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = adf_err_handler, }; static void adf_cleanup_pci_dev(struct adf_accel_dev *accel_dev) @@ -192,11 +193,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_free_reg; - } + adf_enable_aer(accel_dev); if (pci_save_state(pdev)) { dev_err(&pdev->dev, "Failed to save pci state\n"); diff --git a/drivers/crypto/qat/qat_common/adf_aer.c b/drivers/crypto/qat/qat_common/adf_aer.c index ed3e40bc56eb..fe9bb2f3536a 100644 --- a/drivers/crypto/qat/qat_common/adf_aer.c +++ b/drivers/crypto/qat/qat_common/adf_aer.c @@ -166,11 +166,12 @@ static void adf_resume(struct pci_dev *pdev) dev_info(&pdev->dev, "Device is up and running\n"); } -static const struct pci_error_handlers adf_err_handler = { +const struct pci_error_handlers adf_err_handler = { .error_detected = adf_error_detected, .slot_reset = adf_slot_reset, .resume = adf_resume, }; +EXPORT_SYMBOL_GPL(adf_err_handler); /** * adf_enable_aer() - Enable Advance Error Reporting for acceleration device @@ -179,17 +180,12 @@ static const struct pci_error_handlers adf_err_handler = { * Function enables PCI Advance Error Reporting for the * QAT acceleration device accel_dev. * To be used by QAT device specific drivers. - * - * Return: 0 on success, error code otherwise. */ -int adf_enable_aer(struct adf_accel_dev *accel_dev) +void adf_enable_aer(struct adf_accel_dev *accel_dev) { struct pci_dev *pdev = accel_to_pci_dev(accel_dev); - struct pci_driver *pdrv = pdev->driver; - pdrv->err_handler = &adf_err_handler; pci_enable_pcie_error_reporting(pdev); - return 0; } EXPORT_SYMBOL_GPL(adf_enable_aer); diff --git a/drivers/crypto/qat/qat_common/adf_common_drv.h b/drivers/crypto/qat/qat_common/adf_common_drv.h index 4261749fae8d..54ccbd91d0c5 100644 --- a/drivers/crypto/qat/qat_common/adf_common_drv.h +++ b/drivers/crypto/qat/qat_common/adf_common_drv.h @@ -95,7 +95,7 @@ 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); -int adf_enable_aer(struct adf_accel_dev *accel_dev); +void adf_enable_aer(struct adf_accel_dev *accel_dev); void adf_disable_aer(struct adf_accel_dev *accel_dev); void adf_reset_sbr(struct adf_accel_dev *accel_dev); void adf_reset_flr(struct adf_accel_dev *accel_dev); diff --git a/drivers/crypto/qat/qat_dh895xcc/adf_drv.c b/drivers/crypto/qat/qat_dh895xcc/adf_drv.c index 3976a81bd99b..c4b85b182e68 100644 --- a/drivers/crypto/qat/qat_dh895xcc/adf_drv.c +++ b/drivers/crypto/qat/qat_dh895xcc/adf_drv.c @@ -33,6 +33,7 @@ static struct pci_driver adf_driver = { .probe = adf_probe, .remove = adf_remove, .sriov_configure = adf_sriov_configure, + .err_handler = adf_err_handler, }; static void adf_cleanup_pci_dev(struct adf_accel_dev *accel_dev) @@ -192,11 +193,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_free_reg; - } + adf_enable_aer(accel_dev); if (pci_save_state(pdev)) { dev_err(&pdev->dev, "Failed to save pci state\n");