On Wed, Aug 11, 2021 at 12:56:39PM +0100, Giovanni Cabiddu wrote:

On Wed, Aug 11, 2021 at 10:06:35AM +0200, Uwe Kleine-König wrote:

quoted

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 <redacted>

Thanks Uwe for the rework.

quoted

---
 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@wanadoo.fr/).
Would you mind if your patch goes through Herbert's tree?
If you want I can also send a reworked version.

As patch #8 of my series depends on this one I think the best option is
to create a tag and pull that into both, the pci and the crypto tree?!

@Bjorn: Would you agree to this procedure? There has to be a v4, if it
helps I can provide a signed tag for pulling?! Just tell me what would
be helpful here.

quoted

 	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,
	 };

Yeah, the other three drivers need an adaption, too. I will send a v4 in
the next few days. The current state is available at

	https://git.pengutronix.de/git/ukl/linux pci-dedup

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |