[PATCH v3 0/8] hwrng: amd: rework of the amd hwrng driver

[PATCH v3 0/8] hwrng: amd: rework of the amd hwrng driver

[LABBE Corentin]

Changes since v2:
 - split the latest patch in 4
Changes since v1:
 - Keep the hwrng name as "amd"

LABBE Corentin (8):
  hwrng: amd: Fix style problem with blank line
  hwrng: amd: use the BIT macro
  hwrng: amd: Be consitent with the driver name
  hwrng: amd: Remove asm/io.h
  hwrng: amd: release_region must be called after hwrng_unregister
  hwrng: amd: Replace global variable with private struct
  hwrng: amd: Access hardware via ioread32/iowrite32
  hwrng: amd: Convert to new hwrng read() API

 drivers/char/hw_random/amd-rng.c | 150 +++++++++++++++++++++++++--------------
 1 file changed, 96 insertions(+), 54 deletions(-)

-- 
2.7.3

[PATCH v3 1/8] hwrng: amd: Fix style problem with blank line

[LABBE Corentin]

Some blank line are unncessary, and one is missing after declaration.
This patch fix thoses style problems.

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/char/hw_random/amd-rng.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index 48f6a83..45b7965 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -31,10 +31,8 @@
 #include <linux/delay.h>
 #include <asm/io.h>
 
-
 #define PFX	KBUILD_MODNAME ": "
 
-
 /*
  * Data for PCI driver interface
  *
@@ -52,7 +50,6 @@ MODULE_DEVICE_TABLE(pci, pci_tbl);
 
 static struct pci_dev *amd_pdev;
 
-
 static int amd_rng_data_present(struct hwrng *rng, int wait)
 {
 	u32 pmbase = (u32)rng->priv;
@@ -100,7 +97,6 @@ static void amd_rng_cleanup(struct hwrng *rng)
 	pci_write_config_byte(amd_pdev, 0x40, rnen);
 }
 
-
 static struct hwrng amd_rng = {
 	.name		= "amd",
 	.init		= amd_rng_init,
@@ -109,7 +105,6 @@ static struct hwrng amd_rng = {
 	.data_read	= amd_rng_data_read,
 };
 
-
 static int __init mod_init(void)
 {
 	int err = -ENODEV;
@@ -157,6 +152,7 @@ out:
 static void __exit mod_exit(void)
 {
 	u32 pmbase = (unsigned long)amd_rng.priv;
+
 	release_region(pmbase + 0xF0, 8);
 	hwrng_unregister(&amd_rng);
 }
-- 
2.7.3

[PATCH v3 2/8] hwrng: amd: use the BIT macro

[LABBE Corentin]

This patch add usage of the BIT() macro

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/char/hw_random/amd-rng.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index 45b7965..d0042f5 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -78,11 +78,11 @@ static int amd_rng_init(struct hwrng *rng)
 	u8 rnen;
 
 	pci_read_config_byte(amd_pdev, 0x40, &rnen);
-	rnen |= (1 << 7);	/* RNG on */
+	rnen |= BIT(7);	/* RNG on */
 	pci_write_config_byte(amd_pdev, 0x40, rnen);
 
 	pci_read_config_byte(amd_pdev, 0x41, &rnen);
-	rnen |= (1 << 7);	/* PMIO enable */
+	rnen |= BIT(7);	/* PMIO enable */
 	pci_write_config_byte(amd_pdev, 0x41, rnen);
 
 	return 0;
@@ -93,7 +93,7 @@ static void amd_rng_cleanup(struct hwrng *rng)
 	u8 rnen;
 
 	pci_read_config_byte(amd_pdev, 0x40, &rnen);
-	rnen &= ~(1 << 7);	/* RNG off */
+	rnen &= ~BIT(7);	/* RNG off */
 	pci_write_config_byte(amd_pdev, 0x40, rnen);
 }
 
-- 
2.7.3

[PATCH v3 3/8] hwrng: amd: Be consitent with the driver name

[LABBE Corentin]

The driver name is displayed each time differently.
This patch make use of the same name everywhere.

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/char/hw_random/amd-rng.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index d0042f5..93157af 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -31,7 +31,7 @@
 #include <linux/delay.h>
 #include <asm/io.h>
 
-#define PFX	KBUILD_MODNAME ": "
+#define DRV_NAME "AMD768-HWRNG"
 
 /*
  * Data for PCI driver interface
@@ -128,8 +128,8 @@ found:
 	pmbase &= 0x0000FF00;
 	if (pmbase == 0)
 		goto out;
-	if (!request_region(pmbase + 0xF0, 8, "AMD HWRNG")) {
-		dev_err(&pdev->dev, "AMD HWRNG region 0x%x already in use!\n",
+	if (!request_region(pmbase + 0xF0, 8, DRV_NAME)) {
+		dev_err(&pdev->dev, DRV_NAME " region 0x%x already in use!\n",
 			pmbase + 0xF0);
 		err = -EBUSY;
 		goto out;
@@ -137,11 +137,10 @@ found:
 	amd_rng.priv = (unsigned long)pmbase;
 	amd_pdev = pdev;
 
-	pr_info("AMD768 RNG detected\n");
+	pr_info(DRV_NAME " detected\n");
 	err = hwrng_register(&amd_rng);
 	if (err) {
-		pr_err(PFX "RNG registering failed (%d)\n",
-		       err);
+		pr_err(DRV_NAME " registering failed (%d)\n", err);
 		release_region(pmbase + 0xF0, 8);
 		goto out;
 	}
-- 
2.7.3

[PATCH v3 4/8] hwrng: amd: Remove asm/io.h

[LABBE Corentin]

checkpatch complains about <asm/io.h> used instead of linux/io.h.
In fact it is not needed.
This patch remove it, and in the process, alphabetize the other headers.

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/char/hw_random/amd-rng.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index 93157af..de82fe3 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -24,12 +24,11 @@
  * warranty of any kind, whether express or implied.
  */
 
-#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/hw_random.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/pci.h>
-#include <linux/hw_random.h>
-#include <linux/delay.h>
-#include <asm/io.h>
 
 #define DRV_NAME "AMD768-HWRNG"
 
-- 
2.7.3

[PATCH v3 5/8] hwrng: amd: release_region must be called after hwrng_unregister

[LABBE Corentin]

The driver release the memory region before being sure that nobody use
it.

This patch made hwrng_unregister ran before any release was done.

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/char/hw_random/amd-rng.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index de82fe3..383e197 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -151,8 +151,9 @@ static void __exit mod_exit(void)
 {
 	u32 pmbase = (unsigned long)amd_rng.priv;
 
-	release_region(pmbase + 0xF0, 8);
 	hwrng_unregister(&amd_rng);
+
+	release_region(pmbase + 0xF0, 8);
 }
 
 module_init(mod_init);
-- 
2.7.3

[PATCH v3 6/8] hwrng: amd: Replace global variable with private struct

[LABBE Corentin]

Instead of having two global variable, it's better to use a
private struct. This will permit to remove amd_pdev variable

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/char/hw_random/amd-rng.c | 57 ++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index 383e197..4ef94e9 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -47,15 +47,18 @@ static const struct pci_device_id pci_tbl[] = {
 };
 MODULE_DEVICE_TABLE(pci, pci_tbl);
 
-static struct pci_dev *amd_pdev;
+struct amd768_priv {
+	struct pci_dev *pcidev;
+	u32 pmbase;
+};
 
 static int amd_rng_data_present(struct hwrng *rng, int wait)
 {
-	u32 pmbase = (u32)rng->priv;
+	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
 	int data, i;
 
 	for (i = 0; i < 20; i++) {
-		data = !!(inl(pmbase + 0xF4) & 1);
+		data = !!(inl(priv->pmbase + 0xF4) & 1);
 		if (data || !wait)
 			break;
 		udelay(10);
@@ -65,35 +68,37 @@ static int amd_rng_data_present(struct hwrng *rng, int wait)
 
 static int amd_rng_data_read(struct hwrng *rng, u32 *data)
 {
-	u32 pmbase = (u32)rng->priv;
+	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
 
-	*data = inl(pmbase + 0xF0);
+	*data = inl(priv->pmbase + 0xF0);
 
 	return 4;
 }
 
 static int amd_rng_init(struct hwrng *rng)
 {
+	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
 	u8 rnen;
 
-	pci_read_config_byte(amd_pdev, 0x40, &rnen);
+	pci_read_config_byte(priv->pcidev, 0x40, &rnen);
 	rnen |= BIT(7);	/* RNG on */
-	pci_write_config_byte(amd_pdev, 0x40, rnen);
+	pci_write_config_byte(priv->pcidev, 0x40, rnen);
 
-	pci_read_config_byte(amd_pdev, 0x41, &rnen);
+	pci_read_config_byte(priv->pcidev, 0x41, &rnen);
 	rnen |= BIT(7);	/* PMIO enable */
-	pci_write_config_byte(amd_pdev, 0x41, rnen);
+	pci_write_config_byte(priv->pcidev, 0x41, rnen);
 
 	return 0;
 }
 
 static void amd_rng_cleanup(struct hwrng *rng)
 {
+	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
 	u8 rnen;
 
-	pci_read_config_byte(amd_pdev, 0x40, &rnen);
+	pci_read_config_byte(priv->pcidev, 0x40, &rnen);
 	rnen &= ~BIT(7);	/* RNG off */
-	pci_write_config_byte(amd_pdev, 0x40, rnen);
+	pci_write_config_byte(priv->pcidev, 0x40, rnen);
 }
 
 static struct hwrng amd_rng = {
@@ -110,6 +115,7 @@ static int __init mod_init(void)
 	struct pci_dev *pdev = NULL;
 	const struct pci_device_id *ent;
 	u32 pmbase;
+	struct amd768_priv *priv;
 
 	for_each_pci_dev(pdev) {
 		ent = pci_match_id(pci_tbl, pdev);
@@ -117,24 +123,30 @@ static int __init mod_init(void)
 			goto found;
 	}
 	/* Device not found. */
-	goto out;
+	return -ENODEV;
 
 found:
 	err = pci_read_config_dword(pdev, 0x58, &pmbase);
 	if (err)
-		goto out;
-	err = -EIO;
+		return err;
+
 	pmbase &= 0x0000FF00;
 	if (pmbase == 0)
-		goto out;
+		return -EIO;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
 	if (!request_region(pmbase + 0xF0, 8, DRV_NAME)) {
 		dev_err(&pdev->dev, DRV_NAME " region 0x%x already in use!\n",
 			pmbase + 0xF0);
 		err = -EBUSY;
 		goto out;
 	}
-	amd_rng.priv = (unsigned long)pmbase;
-	amd_pdev = pdev;
+	amd_rng.priv = (unsigned long)priv;
+	priv->pmbase = pmbase;
+	priv->pcidev = pdev;
 
 	pr_info(DRV_NAME " detected\n");
 	err = hwrng_register(&amd_rng);
@@ -143,17 +155,24 @@ found:
 		release_region(pmbase + 0xF0, 8);
 		goto out;
 	}
+	return 0;
+
 out:
+	kfree(priv);
 	return err;
 }
 
 static void __exit mod_exit(void)
 {
-	u32 pmbase = (unsigned long)amd_rng.priv;
+	struct amd768_priv *priv;
+
+	priv = (struct amd768_priv *)amd_rng.priv;
 
 	hwrng_unregister(&amd_rng);
 
-	release_region(pmbase + 0xF0, 8);
+	release_region(priv->pmbase + 0xF0, 8);
+
+	kfree(priv);
 }
 
 module_init(mod_init);
-- 
2.7.3

[PATCH v3 7/8] hwrng: amd: Access hardware via ioread32/iowrite32

[LABBE Corentin]

Instead of accessing hw directly via pmbase, it's better to
access after ioport_map() via ioread32/iowrite32.

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/char/hw_random/amd-rng.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index 4ef94e9..bfa14b9 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -32,6 +32,11 @@
 
 #define DRV_NAME "AMD768-HWRNG"
 
+#define RNGDATA		0x00
+#define RNGDONE		0x04
+#define PMBASE_OFFSET	0xF0
+#define PMBASE_SIZE	8
+
 /*
  * Data for PCI driver interface
  *
@@ -48,6 +53,7 @@ static const struct pci_device_id pci_tbl[] = {
 MODULE_DEVICE_TABLE(pci, pci_tbl);
 
 struct amd768_priv {
+	void __iomem *iobase;
 	struct pci_dev *pcidev;
 	u32 pmbase;
 };
@@ -58,7 +64,7 @@ static int amd_rng_data_present(struct hwrng *rng, int wait)
 	int data, i;
 
 	for (i = 0; i < 20; i++) {
-		data = !!(inl(priv->pmbase + 0xF4) & 1);
+		data = !!(ioread32(priv->iobase + RNGDONE) & 1);
 		if (data || !wait)
 			break;
 		udelay(10);
@@ -70,7 +76,7 @@ static int amd_rng_data_read(struct hwrng *rng, u32 *data)
 {
 	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
 
-	*data = inl(priv->pmbase + 0xF0);
+	*data = ioread32(priv->iobase + RNGDATA);
 
 	return 4;
 }
@@ -138,12 +144,20 @@ found:
 	if (!priv)
 		return -ENOMEM;
 
-	if (!request_region(pmbase + 0xF0, 8, DRV_NAME)) {
+	if (!request_region(pmbase + PMBASE_OFFSET, PMBASE_SIZE, DRV_NAME)) {
 		dev_err(&pdev->dev, DRV_NAME " region 0x%x already in use!\n",
 			pmbase + 0xF0);
 		err = -EBUSY;
 		goto out;
 	}
+
+	priv->iobase = ioport_map(pmbase + PMBASE_OFFSET, PMBASE_SIZE);
+	if (!priv->iobase) {
+		pr_err(DRV_NAME "Cannot map ioport\n");
+		err = -EINVAL;
+		goto err_iomap;
+	}
+
 	amd_rng.priv = (unsigned long)priv;
 	priv->pmbase = pmbase;
 	priv->pcidev = pdev;
@@ -152,11 +166,14 @@ found:
 	err = hwrng_register(&amd_rng);
 	if (err) {
 		pr_err(DRV_NAME " registering failed (%d)\n", err);
-		release_region(pmbase + 0xF0, 8);
-		goto out;
+		goto err_hwrng;
 	}
 	return 0;
 
+err_hwrng:
+	ioport_unmap(priv->iobase);
+err_iomap:
+	release_region(pmbase + PMBASE_OFFSET, PMBASE_SIZE);
 out:
 	kfree(priv);
 	return err;
@@ -170,7 +187,9 @@ static void __exit mod_exit(void)
 
 	hwrng_unregister(&amd_rng);
 
-	release_region(priv->pmbase + 0xF0, 8);
+	ioport_unmap(priv->iobase);
+
+	release_region(priv->pmbase + PMBASE_OFFSET, PMBASE_SIZE);
 
 	kfree(priv);
 }
-- 
2.7.3

[PATCH v3 8/8] hwrng: amd: Convert to new hwrng read() API

[LABBE Corentin]

This patch convert the hwrng interface used by amd768-rng to its new API
by replacing data_read()/data_present() by read().

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/char/hw_random/amd-rng.c | 47 ++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index bfa14b9..9959c76 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -58,27 +58,37 @@ struct amd768_priv {
 	u32 pmbase;
 };
 
-static int amd_rng_data_present(struct hwrng *rng, int wait)
+static int amd_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
 {
+	u32 *data = buf;
 	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
-	int data, i;
-
-	for (i = 0; i < 20; i++) {
-		data = !!(ioread32(priv->iobase + RNGDONE) & 1);
-		if (data || !wait)
-			break;
-		udelay(10);
+	size_t read = 0;
+	/* We will wait at maximum one time per read */
+	int timeout = max / 4 + 1;
+
+	/*
+	 * RNG data is available when RNGDONE is set to 1
+	 * New random numbers are generated approximately 128 microseconds
+	 * after RNGDATA is read
+	 */
+	while (read < max) {
+		if (ioread32(priv->iobase + RNGDONE) == 0) {
+			if (wait) {
+				/* Delay given by datasheet */
+				usleep_range(128, 196);
+				if (timeout-- == 0)
+					return read;
+			} else {
+				return 0;
+			}
+		} else {
+			*data = ioread32(priv->iobase + RNGDATA);
+			data++;
+			read += 4;
+		}
 	}
-	return data;
-}
-
-static int amd_rng_data_read(struct hwrng *rng, u32 *data)
-{
-	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
-
-	*data = ioread32(priv->iobase + RNGDATA);
 
-	return 4;
+	return read;
 }
 
 static int amd_rng_init(struct hwrng *rng)
@@ -111,8 +121,7 @@ static struct hwrng amd_rng = {
 	.name		= "amd",
 	.init		= amd_rng_init,
 	.cleanup	= amd_rng_cleanup,
-	.data_present	= amd_rng_data_present,
-	.data_read	= amd_rng_data_read,
+	.read		= amd_rng_read,
 };
 
 static int __init mod_init(void)
-- 
2.7.3

Re: [PATCH v3 6/8] hwrng: amd: Replace global variable with private struct

[Jason Cooper]

Hi Corentin,

On Fri, Aug 26, 2016 at 01:11:34PM +0200, LABBE Corentin wrote:
> Instead of having two global variable, it's better to use a
> private struct. This will permit to remove amd_pdev variable
> 
> Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
> ---
>  drivers/char/hw_random/amd-rng.c | 57 ++++++++++++++++++++++++++--------------
>  1 file changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
> index 383e197..4ef94e9 100644
> --- a/drivers/char/hw_random/amd-rng.c
> +++ b/drivers/char/hw_random/amd-rng.c
> @@ -47,15 +47,18 @@ static const struct pci_device_id pci_tbl[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, pci_tbl);
>  
> -static struct pci_dev *amd_pdev;
> +struct amd768_priv {
> +	struct pci_dev *pcidev;
> +	u32 pmbase;
> +};
>  
>  static int amd_rng_data_present(struct hwrng *rng, int wait)
>  {
> -	u32 pmbase = (u32)rng->priv;
> +	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;

Please remove unnecessary casts...

>  	int data, i;
>  
>  	for (i = 0; i < 20; i++) {
> -		data = !!(inl(pmbase + 0xF4) & 1);
> +		data = !!(inl(priv->pmbase + 0xF4) & 1);
>  		if (data || !wait)
>  			break;
>  		udelay(10);
> @@ -65,35 +68,37 @@ static int amd_rng_data_present(struct hwrng *rng, int wait)
>  
>  static int amd_rng_data_read(struct hwrng *rng, u32 *data)
>  {
> -	u32 pmbase = (u32)rng->priv;
> +	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;

here,

>  
> -	*data = inl(pmbase + 0xF0);
> +	*data = inl(priv->pmbase + 0xF0);
>  
>  	return 4;
>  }
>  
>  static int amd_rng_init(struct hwrng *rng)
>  {
> +	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;

here,

>  	u8 rnen;
>  
> -	pci_read_config_byte(amd_pdev, 0x40, &rnen);
> +	pci_read_config_byte(priv->pcidev, 0x40, &rnen);
>  	rnen |= BIT(7);	/* RNG on */
> -	pci_write_config_byte(amd_pdev, 0x40, rnen);
> +	pci_write_config_byte(priv->pcidev, 0x40, rnen);
>  
> -	pci_read_config_byte(amd_pdev, 0x41, &rnen);
> +	pci_read_config_byte(priv->pcidev, 0x41, &rnen);
>  	rnen |= BIT(7);	/* PMIO enable */
> -	pci_write_config_byte(amd_pdev, 0x41, rnen);
> +	pci_write_config_byte(priv->pcidev, 0x41, rnen);
>  
>  	return 0;
>  }
>  
>  static void amd_rng_cleanup(struct hwrng *rng)
>  {
> +	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;

here,

>  	u8 rnen;
>  
> -	pci_read_config_byte(amd_pdev, 0x40, &rnen);
> +	pci_read_config_byte(priv->pcidev, 0x40, &rnen);
>  	rnen &= ~BIT(7);	/* RNG off */
> -	pci_write_config_byte(amd_pdev, 0x40, rnen);
> +	pci_write_config_byte(priv->pcidev, 0x40, rnen);
>  }
>  
>  static struct hwrng amd_rng = {
> @@ -110,6 +115,7 @@ static int __init mod_init(void)
>  	struct pci_dev *pdev = NULL;
>  	const struct pci_device_id *ent;
>  	u32 pmbase;
> +	struct amd768_priv *priv;
>  
>  	for_each_pci_dev(pdev) {
>  		ent = pci_match_id(pci_tbl, pdev);
> @@ -117,24 +123,30 @@ static int __init mod_init(void)
>  			goto found;
>  	}
>  	/* Device not found. */
> -	goto out;
> +	return -ENODEV;
>  
>  found:
>  	err = pci_read_config_dword(pdev, 0x58, &pmbase);
>  	if (err)
> -		goto out;
> -	err = -EIO;
> +		return err;
> +
>  	pmbase &= 0x0000FF00;
>  	if (pmbase == 0)
> -		goto out;
> +		return -EIO;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
>  	if (!request_region(pmbase + 0xF0, 8, DRV_NAME)) {
>  		dev_err(&pdev->dev, DRV_NAME " region 0x%x already in use!\n",
>  			pmbase + 0xF0);
>  		err = -EBUSY;
>  		goto out;
>  	}
> -	amd_rng.priv = (unsigned long)pmbase;
> -	amd_pdev = pdev;
> +	amd_rng.priv = (unsigned long)priv;

here,

> +	priv->pmbase = pmbase;
> +	priv->pcidev = pdev;
>  
>  	pr_info(DRV_NAME " detected\n");
>  	err = hwrng_register(&amd_rng);
> @@ -143,17 +155,24 @@ found:
>  		release_region(pmbase + 0xF0, 8);
>  		goto out;
>  	}
> +	return 0;
> +
>  out:
> +	kfree(priv);
>  	return err;
>  }
>  
>  static void __exit mod_exit(void)
>  {
> -	u32 pmbase = (unsigned long)amd_rng.priv;
> +	struct amd768_priv *priv;
> +
> +	priv = (struct amd768_priv *)amd_rng.priv;

and here.

thx,

Jason.

>  
>  	hwrng_unregister(&amd_rng);
>  
> -	release_region(pmbase + 0xF0, 8);
> +	release_region(priv->pmbase + 0xF0, 8);
> +
> +	kfree(priv);
>  }
>  
>  module_init(mod_init);
> -- 
> 2.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/8] hwrng: amd: rework of the amd hwrng driver

[Jason Cooper]

Hi Corentin,

On Fri, Aug 26, 2016 at 01:11:28PM +0200, LABBE Corentin wrote:
> Changes since v2:
>  - split the latest patch in 4
> Changes since v1:
>  - Keep the hwrng name as "amd"
> 
> LABBE Corentin (8):
>   hwrng: amd: Fix style problem with blank line
>   hwrng: amd: use the BIT macro
>   hwrng: amd: Be consitent with the driver name
>   hwrng: amd: Remove asm/io.h
>   hwrng: amd: release_region must be called after hwrng_unregister
>   hwrng: amd: Replace global variable with private struct
>   hwrng: amd: Access hardware via ioread32/iowrite32
>   hwrng: amd: Convert to new hwrng read() API
> 
>  drivers/char/hw_random/amd-rng.c | 150 +++++++++++++++++++++++++--------------
>  1 file changed, 96 insertions(+), 54 deletions(-)

Once you've fixed up the casting in #6, you can add my

Reviewed-by: Jason Cooper <jason@lakedaemon.net>

to the series.

thx,

Jason.

Re: [PATCH v3 6/8] hwrng: amd: Replace global variable with private struct

[Jason Cooper]

On Sat, Aug 27, 2016 at 02:43:31PM +0000, Jason Cooper wrote:
> Hi Corentin,
> 
> On Fri, Aug 26, 2016 at 01:11:34PM +0200, LABBE Corentin wrote:
> > Instead of having two global variable, it's better to use a
> > private struct. This will permit to remove amd_pdev variable
> > 
> > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
> > ---
> >  drivers/char/hw_random/amd-rng.c | 57 ++++++++++++++++++++++++++--------------
> >  1 file changed, 38 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
> > index 383e197..4ef94e9 100644
> > --- a/drivers/char/hw_random/amd-rng.c
> > +++ b/drivers/char/hw_random/amd-rng.c
> > @@ -47,15 +47,18 @@ static const struct pci_device_id pci_tbl[] = {
> >  };
> >  MODULE_DEVICE_TABLE(pci, pci_tbl);
> >  
> > -static struct pci_dev *amd_pdev;
> > +struct amd768_priv {
> > +	struct pci_dev *pcidev;
> > +	u32 pmbase;
> > +};
> >  
> >  static int amd_rng_data_present(struct hwrng *rng, int wait)
> >  {
> > -	u32 pmbase = (u32)rng->priv;
> > +	struct amd768_priv *priv = (struct amd768_priv *)rng->priv;
> 
> Please remove unnecessary casts...

Hmm, I was assuming that, like other places in the tree, that priv was
declared void*.  However, it's unsigned long in hw_random.h.

And, it looks like all users cast it.  Either to a struct, or to a void
__iomem *.

So ignore what I said in my previous email.  You can add my reviewed-by
without change.

It does look like /priv/ s/unsigned long/void */ would be a great
cleanup.

thx,

Jason.

Re: [PATCH v3 0/8] hwrng: amd: rework of the amd hwrng driver

[Herbert Xu]

On Fri, Aug 26, 2016 at 01:11:28PM +0200, LABBE Corentin wrote:
> Changes since v2:
>  - split the latest patch in 4
> Changes since v1:
>  - Keep the hwrng name as "amd"

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt