[PATCH v3 01/15] platform: goldfish: pipe: Move the file-scope goldfish_interrupt_tasklet variable into the driver state

[PATCH v3 01/15] platform: goldfish: pipe: Move the file-scope goldfish_interrupt_tasklet variable into the driver state

[rkir]

From: Roman Kiryanov <rkir@google.com>

This is a series of patches to move mutable file-scope variables
into the driver state. This change will help to introduce another
version of the pipe driver (with different state) for the older
host interface or having several instances of this device.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - Updated the commit message.

 drivers/platform/goldfish/goldfish_pipe.c | 24 +++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 56665e879e5a..ba9aede17d57 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -208,6 +208,9 @@ struct goldfish_pipe_dev {
 	int irq;
 	int version;
 	unsigned char __iomem *base;
+
+	/* an irq tasklet to run goldfish_interrupt_task */
+	struct tasklet_struct irq_tasklet;
 };
 
 static struct goldfish_pipe_dev goldfish_pipe_dev;
@@ -582,14 +585,14 @@ static struct goldfish_pipe *signalled_pipes_pop_front(
 	return pipe;
 }
 
-static void goldfish_interrupt_task(unsigned long unused)
+static void goldfish_interrupt_task(unsigned long dev_addr)
 {
 	/* Iterate over the signalled pipes and wake them one by one */
+	struct goldfish_pipe_dev *dev = (struct goldfish_pipe_dev *)dev_addr;
 	struct goldfish_pipe *pipe;
 	int wakes;
 
-	while ((pipe = signalled_pipes_pop_front(&goldfish_pipe_dev, &wakes)) !=
-			NULL) {
+	while ((pipe = signalled_pipes_pop_front(dev, &wakes)) != NULL) {
 		if (wakes & PIPE_WAKE_CLOSED) {
 			pipe->flags = 1 << BIT_CLOSED_ON_HOST;
 		} else {
@@ -605,7 +608,6 @@ static void goldfish_interrupt_task(unsigned long unused)
 		wake_up_interruptible(&pipe->wake_queue);
 	}
 }
-static DECLARE_TASKLET(goldfish_interrupt_tasklet, goldfish_interrupt_task, 0);
 
 /*
  * The general idea of the interrupt handling:
@@ -648,7 +650,7 @@ static irqreturn_t goldfish_pipe_interrupt(int irq, void *dev_id)
 
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	tasklet_schedule(&goldfish_interrupt_tasklet);
+	tasklet_schedule(&dev->irq_tasklet);
 	return IRQ_HANDLED;
 }
 
@@ -800,9 +802,14 @@ static void write_pa_addr(void *addr, void __iomem *portl, void __iomem *porth)
 static int goldfish_pipe_device_init(struct platform_device *pdev)
 {
 	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
-	int err = devm_request_irq(&pdev->dev, dev->irq,
-				goldfish_pipe_interrupt,
-				IRQF_SHARED, "goldfish_pipe", dev);
+	int err;
+
+	tasklet_init(&dev->irq_tasklet, &goldfish_interrupt_task,
+		     (unsigned long)dev);
+
+	err = devm_request_irq(&pdev->dev, dev->irq,
+			       goldfish_pipe_interrupt,
+			       IRQF_SHARED, "goldfish_pipe", dev);
 	if (err) {
 		dev_err(&pdev->dev, "unable to allocate IRQ for v2\n");
 		return err;
@@ -854,6 +861,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
 static void goldfish_pipe_device_deinit(struct platform_device *pdev)
 {
 	misc_deregister(&goldfish_pipe_miscdev);
+	tasklet_kill(&goldfish_pipe_dev.irq_tasklet);
 	kfree(goldfish_pipe_dev.pipes);
 	free_page((unsigned long)goldfish_pipe_dev.buffers);
 }
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 02/15] platform: goldfish: pipe: Move the file-scope goldfish_pipe_miscdev variable into the driver state

[rkir]

From: Roman Kiryanov <rkir@google.com>

This is a series of patches to move mutable file-scope variables
into the driver state. This change will help to introduce another
version of the pipe driver (with different state) for the older
host interface or having several instances of this device.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - Updated the commit message.

 drivers/platform/goldfish/goldfish_pipe.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index ba9aede17d57..8ca709b45e1f 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -211,6 +211,8 @@ struct goldfish_pipe_dev {
 
 	/* an irq tasklet to run goldfish_interrupt_task */
 	struct tasklet_struct irq_tasklet;
+
+	struct miscdevice miscdev;
 };
 
 static struct goldfish_pipe_dev goldfish_pipe_dev;
@@ -785,11 +787,14 @@ static const struct file_operations goldfish_pipe_fops = {
 	.release = goldfish_pipe_release,
 };
 
-static struct miscdevice goldfish_pipe_miscdev = {
-	.minor = MISC_DYNAMIC_MINOR,
-	.name = "goldfish_pipe",
-	.fops = &goldfish_pipe_fops,
-};
+static void init_miscdevice(struct miscdevice *miscdev)
+{
+	memset(miscdev, 0, sizeof(*miscdev));
+
+	miscdev->minor = MISC_DYNAMIC_MINOR;
+	miscdev->name = "goldfish_pipe";
+	miscdev->fops = &goldfish_pipe_fops;
+}
 
 static void write_pa_addr(void *addr, void __iomem *portl, void __iomem *porth)
 {
@@ -815,7 +820,8 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
 		return err;
 	}
 
-	err = misc_register(&goldfish_pipe_miscdev);
+	init_miscdevice(&dev->miscdev);
+	err = misc_register(&dev->miscdev);
 	if (err) {
 		dev_err(&pdev->dev, "unable to register v2 device\n");
 		return err;
@@ -860,7 +866,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
 
 static void goldfish_pipe_device_deinit(struct platform_device *pdev)
 {
-	misc_deregister(&goldfish_pipe_miscdev);
+	misc_deregister(&goldfish_pipe_dev.miscdev);
 	tasklet_kill(&goldfish_pipe_dev.irq_tasklet);
 	kfree(goldfish_pipe_dev.pipes);
 	free_page((unsigned long)goldfish_pipe_dev.buffers);
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 03/15] platform: goldfish: pipe: Move the file-scope goldfish_pipe_dev variable into the driver state

[rkir]

From: Roman Kiryanov <rkir@google.com>

This is the last patch in the series of patches to move file-scope
variables into the driver state. This change will help to introduce
another version of the pipe driver (with different state) for the
older host interface or having several instances of this device.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - Updated the commit message.

 drivers/platform/goldfish/goldfish_pipe.c | 66 +++++++++++++----------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 8ca709b45e1f..4013832f38fb 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -169,6 +169,9 @@ struct goldfish_pipe {
  * waiting to be awoken.
  */
 struct goldfish_pipe_dev {
+	/* A magic number to check if this is an instance of this struct */
+	void *magic;
+
 	/*
 	 * Global device spinlock. Protects the following members:
 	 *  - pipes, pipes_capacity
@@ -215,8 +218,6 @@ struct goldfish_pipe_dev {
 	struct miscdevice miscdev;
 };
 
-static struct goldfish_pipe_dev goldfish_pipe_dev;
-
 static int goldfish_pipe_cmd_locked(struct goldfish_pipe *pipe,
 				    enum PipeCmdCode cmd)
 {
@@ -611,6 +612,9 @@ static void goldfish_interrupt_task(unsigned long dev_addr)
 	}
 }
 
+static void goldfish_pipe_device_deinit(struct platform_device *pdev,
+					struct goldfish_pipe_dev *dev);
+
 /*
  * The general idea of the interrupt handling:
  *
@@ -631,7 +635,7 @@ static irqreturn_t goldfish_pipe_interrupt(int irq, void *dev_id)
 	unsigned long flags;
 	struct goldfish_pipe_dev *dev = dev_id;
 
-	if (dev != &goldfish_pipe_dev)
+	if (dev->magic != &goldfish_pipe_device_deinit)
 		return IRQ_NONE;
 
 	/* Request the signalled pipes from the device */
@@ -683,6 +687,14 @@ static int get_free_pipe_id_locked(struct goldfish_pipe_dev *dev)
 	return id;
 }
 
+/* A helper function to get the instance of goldfish_pipe_dev from file */
+static struct goldfish_pipe_dev *to_goldfish_pipe_dev(struct file *file)
+{
+	struct miscdevice *miscdev = file->private_data;
+
+	return container_of(miscdev, struct goldfish_pipe_dev, miscdev);
+}
+
 /**
  *	goldfish_pipe_open - open a channel to the AVD
  *	@inode: inode of device
@@ -696,7 +708,7 @@ static int get_free_pipe_id_locked(struct goldfish_pipe_dev *dev)
  */
 static int goldfish_pipe_open(struct inode *inode, struct file *file)
 {
-	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
+	struct goldfish_pipe_dev *dev = to_goldfish_pipe_dev(file);
 	unsigned long flags;
 	int id;
 	int status;
@@ -804,9 +816,9 @@ static void write_pa_addr(void *addr, void __iomem *portl, void __iomem *porth)
 	writel(lower_32_bits(paddr), portl);
 }
 
-static int goldfish_pipe_device_init(struct platform_device *pdev)
+static int goldfish_pipe_device_init(struct platform_device *pdev,
+				     struct goldfish_pipe_dev *dev)
 {
-	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
 	int err;
 
 	tasklet_init(&dev->irq_tasklet, &goldfish_interrupt_task,
@@ -861,26 +873,29 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
 		      dev->base + PIPE_REG_OPEN_BUFFER,
 		      dev->base + PIPE_REG_OPEN_BUFFER_HIGH);
 
+	platform_set_drvdata(pdev, dev);
 	return 0;
 }
 
-static void goldfish_pipe_device_deinit(struct platform_device *pdev)
+static void goldfish_pipe_device_deinit(struct platform_device *pdev,
+					struct goldfish_pipe_dev *dev)
 {
-	misc_deregister(&goldfish_pipe_dev.miscdev);
-	tasklet_kill(&goldfish_pipe_dev.irq_tasklet);
-	kfree(goldfish_pipe_dev.pipes);
-	free_page((unsigned long)goldfish_pipe_dev.buffers);
+	misc_deregister(&dev->miscdev);
+	tasklet_kill(&dev->irq_tasklet);
+	kfree(dev->pipes);
+	free_page((unsigned long)dev->buffers);
 }
 
 static int goldfish_pipe_probe(struct platform_device *pdev)
 {
-	int err;
 	struct resource *r;
-	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
+	struct goldfish_pipe_dev *dev;
 
-	/* not thread safe, but this should not happen */
-	WARN_ON(dev->base);
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
 
+	dev->magic = &goldfish_pipe_device_deinit;
 	spin_lock_init(&dev->lock);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -895,10 +910,9 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	}
 
 	r = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!r) {
-		err = -EINVAL;
-		goto error;
-	}
+	if (!r)
+		return -EINVAL;
+
 	dev->irq = r->start;
 
 	/*
@@ -913,20 +927,14 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	if (WARN_ON(dev->version < PIPE_CURRENT_DEVICE_VERSION))
 		return -EINVAL;
 
-	err = goldfish_pipe_device_init(pdev);
-	if (!err)
-		return 0;
-
-error:
-	dev->base = NULL;
-	return err;
+	return goldfish_pipe_device_init(pdev, dev);
 }
 
 static int goldfish_pipe_remove(struct platform_device *pdev)
 {
-	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
-	goldfish_pipe_device_deinit(pdev);
-	dev->base = NULL;
+	struct goldfish_pipe_dev *dev = platform_get_drvdata(pdev);
+
+	goldfish_pipe_device_deinit(pdev, dev);
 	return 0;
 }
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 04/15] platform: goldfish: pipe: Call misc_deregister if init fails

[rkir]

From: Roman Kiryanov <rkir@google.com>

Undo effects of misc_register if driver's init fails after
misc_register.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 4013832f38fb..c386aaf40034 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -844,8 +844,10 @@ static int goldfish_pipe_device_init(struct platform_device *pdev,
 	dev->pipes_capacity = INITIAL_PIPES_CAPACITY;
 	dev->pipes = kcalloc(dev->pipes_capacity, sizeof(*dev->pipes),
 			     GFP_KERNEL);
-	if (!dev->pipes)
+	if (!dev->pipes) {
+		misc_deregister(&dev->miscdev);
 		return -ENOMEM;
+	}
 
 	/*
 	 * We're going to pass two buffers, open_command_params and
@@ -858,6 +860,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev,
 		__get_free_page(GFP_KERNEL);
 	if (!dev->buffers) {
 		kfree(dev->pipes);
+		misc_deregister(&dev->miscdev);
 		return -ENOMEM;
 	}
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 05/15] platform: goldfish: pipe: Remove redundant casting

[rkir]

From: Roman Kiryanov <rkir@google.com>

This casting is not required.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index c386aaf40034..bc431f04c4cf 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -925,7 +925,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	 *  reading device version back: this allows the host implementation to
 	 *  detect the old driver (if there was no version write before read).
 	 */
-	writel((u32)PIPE_DRIVER_VERSION, dev->base + PIPE_REG_VERSION);
+	writel(PIPE_DRIVER_VERSION, dev->base + PIPE_REG_VERSION);
 	dev->version = readl(dev->base + PIPE_REG_VERSION);
 	if (WARN_ON(dev->version < PIPE_CURRENT_DEVICE_VERSION))
 		return -EINVAL;
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init

[rkir]

From: Roman Kiryanov <rkir@google.com>

There will be two separate init functions for v1 and v2
(different driver versions) and they will allocate different
state.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe.c | 42 +++++++++++++----------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index bc431f04c4cf..445c0c0c66c4 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -208,8 +208,6 @@ struct goldfish_pipe_dev {
 	struct device *pdev_dev;
 
 	/* Some device-specific data */
-	int irq;
-	int version;
 	unsigned char __iomem *base;
 
 	/* an irq tasklet to run goldfish_interrupt_task */
@@ -817,14 +815,23 @@ static void write_pa_addr(void *addr, void __iomem *portl, void __iomem *porth)
 }
 
 static int goldfish_pipe_device_init(struct platform_device *pdev,
-				     struct goldfish_pipe_dev *dev)
+				     char __iomem *base,
+				     int irq)
 {
+	struct goldfish_pipe_dev *dev;
 	int err;
 
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->magic = &goldfish_pipe_device_deinit;
+	spin_lock_init(&dev->lock);
+
 	tasklet_init(&dev->irq_tasklet, &goldfish_interrupt_task,
 		     (unsigned long)dev);
 
-	err = devm_request_irq(&pdev->dev, dev->irq,
+	err = devm_request_irq(&pdev->dev, irq,
 			       goldfish_pipe_interrupt,
 			       IRQF_SHARED, "goldfish_pipe", dev);
 	if (err) {
@@ -839,6 +846,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev,
 		return err;
 	}
 
+	dev->base = base;
 	dev->pdev_dev = &pdev->dev;
 	dev->first_signalled_pipe = NULL;
 	dev->pipes_capacity = INITIAL_PIPES_CAPACITY;
@@ -892,22 +900,18 @@ static void goldfish_pipe_device_deinit(struct platform_device *pdev,
 static int goldfish_pipe_probe(struct platform_device *pdev)
 {
 	struct resource *r;
-	struct goldfish_pipe_dev *dev;
-
-	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
-	if (!dev)
-		return -ENOMEM;
-
-	dev->magic = &goldfish_pipe_device_deinit;
-	spin_lock_init(&dev->lock);
+	char __iomem *base;
+	int irq;
+	int version;
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r || resource_size(r) < PAGE_SIZE) {
 		dev_err(&pdev->dev, "can't allocate i/o page\n");
 		return -EINVAL;
 	}
-	dev->base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
-	if (!dev->base) {
+
+	base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
+	if (!base) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		return -EINVAL;
 	}
@@ -916,7 +920,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	if (!r)
 		return -EINVAL;
 
-	dev->irq = r->start;
+	irq = r->start;
 
 	/*
 	 * Exchange the versions with the host device
@@ -925,12 +929,12 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	 *  reading device version back: this allows the host implementation to
 	 *  detect the old driver (if there was no version write before read).
 	 */
-	writel(PIPE_DRIVER_VERSION, dev->base + PIPE_REG_VERSION);
-	dev->version = readl(dev->base + PIPE_REG_VERSION);
-	if (WARN_ON(dev->version < PIPE_CURRENT_DEVICE_VERSION))
+	writel(PIPE_DRIVER_VERSION, base + PIPE_REG_VERSION);
+	version = readl(base + PIPE_REG_VERSION);
+	if (WARN_ON(version < PIPE_CURRENT_DEVICE_VERSION))
 		return -EINVAL;
 
-	return goldfish_pipe_device_init(pdev, dev);
+	return goldfish_pipe_device_init(pdev, base, irq);
 }
 
 static int goldfish_pipe_remove(struct platform_device *pdev)
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init

[Greg KH]

On Wed, Oct 03, 2018 at 10:17:11AM -0700, rkir@google.com wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> There will be two separate init functions for v1 and v2
> (different driver versions) and they will allocate different
> state.

You should only allocate memory at probe time, not init time as what
happens if the hardware is not present yet your driver is loaded?

You should do almost nothing at init time except register with the
proper bus so that your probe function can be called if the hardware is
present.

So the patch here is going backwards from what it should be working
toward.

thanks,

greg k-h

Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init

[Roman Kiryanov]

> You should only allocate memory at probe time

probe does not know what memory to allocate. We have several versions
of the driver (with different init) and different versions allocate
different state.

>, not init time as what
> happens if the hardware is not present yet your driver is loaded?

init will have to rollback what it allocated.

Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init

[Greg KH]

On Mon, Oct 15, 2018 at 11:48:28AM -0700, Roman Kiryanov wrote:
> > You should only allocate memory at probe time
> 
> probe does not know what memory to allocate. We have several versions
> of the driver (with different init) and different versions allocate
> different state.

I only see one driver here.

Why does probe not know what to allocate?  That is exactly when the
device is bound to the driver, you have _way_ more information than you
do at init time.

> >, not init time as what
> > happens if the hardware is not present yet your driver is loaded?
> 
> init will have to rollback what it allocated.

But those resources it will sit there wasted until unload happens.  And
unload _never_ happens on a system unless you are a developer working on
the module.

thanks,

greg k-h

Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init

[Roman Kiryanov]

> > probe does not know what memory to allocate. We have several versions
> > of the driver (with different init) and different versions allocate
> > different state.
>
> I only see one driver here.

It will be added in "PATCH v3 15/15". There will be two init functions
allocating
different states.

> Why does probe not know what to allocate?  That is exactly when the
> device is bound to the driver, you have _way_ more information than you
> do at init time.

We have two versions of the driver. Probe asks for the version and
calls the init function for this version.
I don't want probe to know everything about all versions.

> > >, not init time as what
> > > happens if the hardware is not present yet your driver is loaded?
> >
> > init will have to rollback what it allocated.
>
> But those resources it will sit there wasted until unload happens.  And
> unload _never_ happens on a system unless you are a developer working on
> the module.

If probe fails I expect the kernel to release all resources. Is this
not the case?

Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init

[Greg KH]

On Mon, Oct 15, 2018 at 12:06:12PM -0700, Roman Kiryanov wrote:
> > > probe does not know what memory to allocate. We have several versions
> > > of the driver (with different init) and different versions allocate
> > > different state.
> >
> > I only see one driver here.
> 
> It will be added in "PATCH v3 15/15". There will be two init functions
> allocating different states.

I think we are getting our terms confused here.

"init" usually means module_init(), is that what you are referring to
here?

probe means when your bus-specific driver's probe function is called
because the kernel recognizes your hardware as being present.

At module_init time, you should do nothing except register your bus
specific driver.

At probe time, allocate memory all you want, that is the correct point
in time to do it.

So yes, if you have two separate drivers, you will have two separate
init functions, but both of them should just register the bus specific
driver.

> > Why does probe not know what to allocate?  That is exactly when the
> > device is bound to the driver, you have _way_ more information than you
> > do at init time.
> 
> We have two versions of the driver. Probe asks for the version and
> calls the init function for this version.
> I don't want probe to know everything about all versions.

Why would your probe function know, or care, about versions?  That's up
to the driver core.  How is your probe function learning about the
"version" to use here?

> > > >, not init time as what
> > > > happens if the hardware is not present yet your driver is loaded?
> > >
> > > init will have to rollback what it allocated.
> >
> > But those resources it will sit there wasted until unload happens.  And
> > unload _never_ happens on a system unless you are a developer working on
> > the module.
> 
> If probe fails I expect the kernel to release all resources. Is this
> not the case?

You have to clean up after your probe function failing before you return
from it.

thanks,

greg k-h

Re: [PATCH v3 06/15] platform: goldfish: pipe: Move memory allocation from probe to init

[Roman Kiryanov]

> I think we are getting our terms confused here.
>
> "init" usually means module_init(), is that what you are referring to
> here?

This is just a function that probe calls to, goldfish_pipe_device_v2_init.

https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/goldfish_pipe_v2.c#1128

> Why would your probe function know, or care, about versions?

Different driver versions allocate different state.

> How is your probe function learning about the "version" to use here?

writel(PIPE_DRIVER_VERSION, base + PIPE_V2_REG_VERSION);
version = readl(base + PIPE_V2_REG_VERSION);

https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/goldfish_pipe.c#106

> You have to clean up after your probe function failing before you return
> from it.

Are you saying that devm_kzalloc does not free memory if probe fails?
Is this useful?
I can free memory myself then.

[PATCH v3 07/15] platform: goldfish: pipe: Return status from "deinit" since "remove" does not do much

[rkir]

From: Roman Kiryanov <rkir@google.com>

This way deinit will have a chance to report an error.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 445c0c0c66c4..1822d4146778 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -888,13 +888,15 @@ static int goldfish_pipe_device_init(struct platform_device *pdev,
 	return 0;
 }
 
-static void goldfish_pipe_device_deinit(struct platform_device *pdev,
-					struct goldfish_pipe_dev *dev)
+static int goldfish_pipe_device_deinit(struct platform_device *pdev,
+				       struct goldfish_pipe_dev *dev)
 {
 	misc_deregister(&dev->miscdev);
 	tasklet_kill(&dev->irq_tasklet);
 	kfree(dev->pipes);
 	free_page((unsigned long)dev->buffers);
+
+	return 0;
 }
 
 static int goldfish_pipe_probe(struct platform_device *pdev)
@@ -941,8 +943,7 @@ static int goldfish_pipe_remove(struct platform_device *pdev)
 {
 	struct goldfish_pipe_dev *dev = platform_get_drvdata(pdev);
 
-	goldfish_pipe_device_deinit(pdev, dev);
-	return 0;
+	return goldfish_pipe_device_deinit(pdev, dev);
 }
 
 static const struct acpi_device_id goldfish_pipe_acpi_match[] = {
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v3 07/15] platform: goldfish: pipe: Return status from "deinit" since "remove" does not do much

[Greg KH]

On Wed, Oct 03, 2018 at 10:17:12AM -0700, rkir@google.com wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> This way deinit will have a chance to report an error.
> 
> Signed-off-by: Roman Kiryanov <rkir@google.com>
> ---
> Changes in v3:
>  - No change.
> Changes in v2:
>  - No change.
> 
>  drivers/platform/goldfish/goldfish_pipe.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
> index 445c0c0c66c4..1822d4146778 100644
> --- a/drivers/platform/goldfish/goldfish_pipe.c
> +++ b/drivers/platform/goldfish/goldfish_pipe.c
> @@ -888,13 +888,15 @@ static int goldfish_pipe_device_init(struct platform_device *pdev,
>  	return 0;
>  }
>  
> -static void goldfish_pipe_device_deinit(struct platform_device *pdev,
> -					struct goldfish_pipe_dev *dev)
> +static int goldfish_pipe_device_deinit(struct platform_device *pdev,
> +				       struct goldfish_pipe_dev *dev)
>  {
>  	misc_deregister(&dev->miscdev);
>  	tasklet_kill(&dev->irq_tasklet);
>  	kfree(dev->pipes);
>  	free_page((unsigned long)dev->buffers);
> +
> +	return 0;
>  }

This function can not fail, why are you returning 0 always?  That
doesn't make sense.

thanks,

greg k-h

Re: [PATCH v3 07/15] platform: goldfish: pipe: Return status from "deinit" since "remove" does not do much

[Roman Kiryanov]

> This function can not fail, why are you returning 0 always?  That
> doesn't make sense.

remove in struct platform_driver requires returning something, we have
to have "return" somewhere.

I think we want to return closer to the place where we do something useful.

Re: [PATCH v3 07/15] platform: goldfish: pipe: Return status from "deinit" since "remove" does not do much

[Greg KH]

On Mon, Oct 15, 2018 at 03:04:47PM -0700, Roman Kiryanov wrote:
> > This function can not fail, why are you returning 0 always?  That
> > doesn't make sense.
> 
> remove in struct platform_driver requires returning something, we have
> to have "return" somewhere.

But this code always returns 0, you are changing the function return
value to int, but it can not fail.  I do not understand the need for
this change at all, sorry.

> I think we want to return closer to the place where we do something useful.

I do not understand this.

greg k-h

[PATCH v3 08/15] platform: goldfish: pipe: Add a blank line to separate varibles and code

[rkir]

From: Roman Kiryanov <rkir@google.com>

checkpacth: Missing a blank line after declarations

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 1822d4146778..a1fbbf49cc3f 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -713,6 +713,7 @@ static int goldfish_pipe_open(struct inode *inode, struct file *file)
 
 	/* Allocate new pipe kernel object */
 	struct goldfish_pipe *pipe = kzalloc(sizeof(*pipe), GFP_KERNEL);
+
 	if (!pipe)
 		return -ENOMEM;
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 09/15] platform: goldfish: pipe: Move goldfish_pipe to goldfish_pipe_v2

[rkir]

From: Roman Kiryanov <rkir@google.com>

This is the v2 driver. v1 will be added later.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/Makefile                              | 2 +-
 .../platform/goldfish/{goldfish_pipe.c => goldfish_pipe_v2.c}   | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/platform/goldfish/{goldfish_pipe.c => goldfish_pipe_v2.c} (100%)

diff --git a/drivers/platform/goldfish/Makefile b/drivers/platform/goldfish/Makefile
index e0c202df9674..81f899a987a2 100644
--- a/drivers/platform/goldfish/Makefile
+++ b/drivers/platform/goldfish/Makefile
@@ -1,4 +1,4 @@
 #
 # Makefile for Goldfish platform specific drivers
 #
-obj-$(CONFIG_GOLDFISH_PIPE)	+= goldfish_pipe.o
+obj-$(CONFIG_GOLDFISH_PIPE)	+= goldfish_pipe_v2.o
diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe_v2.c
similarity index 100%
rename from drivers/platform/goldfish/goldfish_pipe.c
rename to drivers/platform/goldfish/goldfish_pipe_v2.c
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v3 09/15] platform: goldfish: pipe: Move goldfish_pipe to goldfish_pipe_v2

[Greg KH]

On Wed, Oct 03, 2018 at 10:17:14AM -0700, rkir@google.com wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> This is the v2 driver. v1 will be added later.

This does not make any sense.  Why are you renaming a driver that has
been in the tree already?  It's not "v2", it is what we have today.

If you want to add a new driver later, great, but don't change the name
of an existing driver for no good reason.  Userspace has a tendancy to
break when that happens.

thanks,

greg k-h

Re: [PATCH v3 09/15] platform: goldfish: pipe: Move goldfish_pipe to goldfish_pipe_v2

[Roman Kiryanov]

> > This is the v2 driver. v1 will be added later.
>
> If you want to add a new driver later, great, but don't change the name
> of an existing driver for no good reason.

I want our "v2" driver to be in a "v2" file and our "v1" driver in a
"v1" file. I think this is reasonable.

Re: [PATCH v3 09/15] platform: goldfish: pipe: Move goldfish_pipe to goldfish_pipe_v2

[Greg KH]

On Mon, Oct 15, 2018 at 11:55:26AM -0700, Roman Kiryanov wrote:
> > > This is the v2 driver. v1 will be added later.
> >
> > If you want to add a new driver later, great, but don't change the name
> > of an existing driver for no good reason.
> 
> I want our "v2" driver to be in a "v2" file and our "v1" driver in a
> "v1" file. I think this is reasonable.

The in kernel driver is the "v1" one.  Why do you need a totally new
driver file at all anyway?

And again, can you GUARANTEE that userspace will not break if you rename
the kernel module?  Also, "vX" is an odd naming schemes for drivers,
don't take naming lessons from qualcomm :)

thanks,

greg k-h

Re: [PATCH v3 09/15] platform: goldfish: pipe: Move goldfish_pipe to goldfish_pipe_v2

[Roman Kiryanov]

> > I want our "v2" driver to be in a "v2" file and our "v1" driver in a
> > "v1" file. I think this is reasonable.
>
> The in kernel driver is the "v1" one.

I believe v2 (on our end) was upstream as goldfish_pipe.c instead of
goldfish_pipe_v2.c.

>  Why do you need a totally new driver file at all anyway?

I don't want to mix v1 and v2.

> And again, can you GUARANTEE that userspace will not break if you rename
> the kernel module?

Yes, it works:
https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/

I don't see how userspace could be affected, unless we refer to
__FILE__ inside the driver for some important things. I believe we
don't.

Re: [PATCH v3 09/15] platform: goldfish: pipe: Move goldfish_pipe to goldfish_pipe_v2

[Greg KH]

On Mon, Oct 15, 2018 at 01:23:35PM -0700, Roman Kiryanov wrote:
> > > I want our "v2" driver to be in a "v2" file and our "v1" driver in a
> > > "v1" file. I think this is reasonable.
> >
> > The in kernel driver is the "v1" one.
> 
> I believe v2 (on our end) was upstream as goldfish_pipe.c instead of
> goldfish_pipe_v2.c.

I do not understand this, sorry.  My point of view is the in-kernel
code, I know nothing about out-of-tree code, nor do I care :)

> >  Why do you need a totally new driver file at all anyway?
> 
> I don't want to mix v1 and v2.

Why?  What is the difference?  Why do you even need/want 2 different
drivers here for something that really should be done with the virtio
interface instead?

> > And again, can you GUARANTEE that userspace will not break if you rename
> > the kernel module?
> 
> Yes, it works:
> https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/

That's a kernel, not userspace, code.

> I don't see how userspace could be affected, unless we refer to
> __FILE__ inside the driver for some important things. I believe we
> don't.

How does your userspace know what module to automatically load?  That's
what usually breaks if you do not have proper platform binding happening
automatically.

thanks,

greg k-h

Re: [PATCH v3 09/15] platform: goldfish: pipe: Move goldfish_pipe to goldfish_pipe_v2

[Roman Kiryanov]

> > I don't want to mix v1 and v2.
>
> Why?  What is the difference?
> Why do you even need/want 2 different drivers here

In old emulators we implemented the device differently, so we need a
different driver.

>  for something that really should be done with the virtio
> interface instead?

We are already looking into virtio. But we did not find a userspace
API that virtio provides to
replace our pipe. The closest we found is virtqueue, but it is a
kernel thing. So we will
need goldfish_pipe_v3.

> > I don't see how userspace could be affected, unless we refer to
> > __FILE__ inside the driver for some important things. I believe we
> > don't.
>
> How does your userspace know what module to automatically load?  That's
> what usually breaks if you do not have proper platform binding happening
> automatically.

I am not sure if I understand your question.

Userspace opens the device by its name. The device name is hardcoded:

#define DEVICE_NAME "goldfish_pipe"

https://android.googlesource.com/kernel/goldfish/+/android-goldfish-4.14-dev/drivers/platform/goldfish/goldfish_pipe.h#19

I still don't understand how kernel filenames are involved (and
whether should they) into userpspace.

[PATCH v3 10/15] platform: goldfish: pipe: Remove the license boilerplate

[rkir]

From: Roman Kiryanov <rkir@google.com>

Not required with the SPDX-License-Identifier header.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe_v2.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe_v2.c b/drivers/platform/goldfish/goldfish_pipe_v2.c
index a1fbbf49cc3f..ff5d88e7959a 100644
--- a/drivers/platform/goldfish/goldfish_pipe_v2.c
+++ b/drivers/platform/goldfish/goldfish_pipe_v2.c
@@ -1,21 +1,4 @@
 // SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (C) 2012 Intel, Inc.
- * Copyright (C) 2013 Intel, Inc.
- * Copyright (C) 2014 Linaro Limited
- * Copyright (C) 2011-2016 Google, Inc.
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
 /* This source file contains the implementation of a special device driver
  * that intends to provide a *very* fast communication channel between the
  * guest system and the QEMU emulator.
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 11/15] platform: goldfish: pipe: Split the driver to v2 specific and the rest

[rkir]

From: Roman Kiryanov <rkir@google.com>

Move probe/remove and other driver stuff to a separate file
to plug v1 there later.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/Makefile           |   3 +-
 drivers/platform/goldfish/goldfish_pipe.c    | 124 +++++++++++++++++++
 drivers/platform/goldfish/goldfish_pipe.h    |  10 ++
 drivers/platform/goldfish/goldfish_pipe_v2.c | 112 +++--------------
 drivers/platform/goldfish/goldfish_pipe_v2.h |  10 ++
 5 files changed, 161 insertions(+), 98 deletions(-)
 create mode 100644 drivers/platform/goldfish/goldfish_pipe.c
 create mode 100644 drivers/platform/goldfish/goldfish_pipe.h
 create mode 100644 drivers/platform/goldfish/goldfish_pipe_v2.h

diff --git a/drivers/platform/goldfish/Makefile b/drivers/platform/goldfish/Makefile
index 81f899a987a2..016769e003d8 100644
--- a/drivers/platform/goldfish/Makefile
+++ b/drivers/platform/goldfish/Makefile
@@ -1,4 +1,5 @@
 #
 # Makefile for Goldfish platform specific drivers
 #
-obj-$(CONFIG_GOLDFISH_PIPE)	+= goldfish_pipe_v2.o
+obj-$(CONFIG_GOLDFISH_PIPE)    += goldfish_pipe_all.o
+goldfish_pipe_all-objs := goldfish_pipe.o goldfish_pipe_v2.o
diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
new file mode 100644
index 000000000000..792b20bdf76c
--- /dev/null
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0
+/* This source file contains the implementation of a special device driver
+ * that intends to provide a *very* fast communication channel between the
+ * guest system and the QEMU emulator.
+ *
+ * Usage from the guest is simply the following (error handling simplified):
+ *
+ *    int  fd = open("/dev/qemu_pipe",O_RDWR);
+ *    .... write() or read() through the pipe.
+ *
+ * This driver doesn't deal with the exact protocol used during the session.
+ * It is intended to be as simple as something like:
+ *
+ *    // do this _just_ after opening the fd to connect to a specific
+ *    // emulator service.
+ *    const char*  msg = "<pipename>";
+ *    if (write(fd, msg, strlen(msg)+1) < 0) {
+ *       ... could not connect to <pipename> service
+ *       close(fd);
+ *    }
+ *
+ *    // after this, simply read() and write() to communicate with the
+ *    // service. Exact protocol details left as an exercise to the reader.
+ *
+ * This driver is very fast because it doesn't copy any data through
+ * intermediate buffers, since the emulator is capable of translating
+ * guest user addresses into host ones.
+ *
+ * Note that we must however ensure that each user page involved in the
+ * exchange is properly mapped during a transfer.
+ */
+
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/acpi.h>
+#include "goldfish_pipe_qemu.h"
+#include "goldfish_pipe.h"
+#include "goldfish_pipe_v2.h"
+
+/*
+ * Update this when something changes in the driver's behavior so the host
+ * can benefit from knowing it
+ */
+enum {
+	PIPE_DRIVER_VERSION = 2,
+	PIPE_CURRENT_DEVICE_VERSION = 2
+};
+
+static int goldfish_pipe_probe(struct platform_device *pdev)
+{
+	struct resource *r;
+	char __iomem *base;
+	int irq;
+	int version;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r || resource_size(r) < PAGE_SIZE) {
+		dev_err(&pdev->dev, "can't allocate i/o page\n");
+		return -EINVAL;
+	}
+	base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
+	if (!base) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		return -EINVAL;
+	}
+
+	r = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!r)
+		return -EINVAL;
+
+	irq = r->start;
+
+	/*
+	 * Exchange the versions with the host device
+	 *
+	 * Note: v1 driver used to not report its version, so we write it before
+	 *  reading device version back: this allows the host implementation to
+	 *  detect the old driver (if there was no version write before read).
+	 */
+	writel(PIPE_DRIVER_VERSION, base + PIPE_REG_VERSION);
+	version = readl(base + PIPE_REG_VERSION);
+
+	if (WARN_ON(version < PIPE_CURRENT_DEVICE_VERSION))
+		return -EINVAL;
+
+	return goldfish_pipe_device_init(pdev, base, irq);
+}
+
+static int goldfish_pipe_remove(struct platform_device *pdev)
+{
+	struct goldfish_pipe_dev_base *dev = platform_get_drvdata(pdev);
+
+	return dev->deinit(dev, pdev);
+}
+
+static const struct acpi_device_id goldfish_pipe_acpi_match[] = {
+	{ "GFSH0003", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, goldfish_pipe_acpi_match);
+
+static const struct of_device_id goldfish_pipe_of_match[] = {
+	{ .compatible = "google,android-pipe", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, goldfish_pipe_of_match);
+
+static struct platform_driver goldfish_pipe_driver = {
+	.probe = goldfish_pipe_probe,
+	.remove = goldfish_pipe_remove,
+	.driver = {
+		.name = "goldfish_pipe",
+		.of_match_table = goldfish_pipe_of_match,
+		.acpi_match_table = ACPI_PTR(goldfish_pipe_acpi_match),
+	}
+};
+
+module_platform_driver(goldfish_pipe_driver);
+MODULE_AUTHOR("David Turner <digit@google.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/goldfish/goldfish_pipe.h b/drivers/platform/goldfish/goldfish_pipe.h
new file mode 100644
index 000000000000..ee0b54bcb165
--- /dev/null
+++ b/drivers/platform/goldfish/goldfish_pipe.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef GOLDFISH_PIPE_H
+#define GOLDFISH_PIPE_H
+
+struct goldfish_pipe_dev_base {
+	/* the destructor, the pointer is set in init */
+	int (*deinit)(void *pipe_dev, struct platform_device *pdev);
+};
+
+#endif /* GOLDFISH_PIPE_H */
diff --git a/drivers/platform/goldfish/goldfish_pipe_v2.c b/drivers/platform/goldfish/goldfish_pipe_v2.c
index ff5d88e7959a..641dfdcc3ffd 100644
--- a/drivers/platform/goldfish/goldfish_pipe_v2.c
+++ b/drivers/platform/goldfish/goldfish_pipe_v2.c
@@ -30,8 +30,6 @@
  * exchange is properly mapped during a transfer.
  */
 
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
@@ -44,18 +42,9 @@
 #include <linux/io.h>
 #include <linux/dma-mapping.h>
 #include <linux/mm.h>
-#include <linux/acpi.h>
 #include <linux/bug.h>
 #include "goldfish_pipe_qemu.h"
-
-/*
- * Update this when something changes in the driver's behavior so the host
- * can benefit from knowing it
- */
-enum {
-	PIPE_DRIVER_VERSION = 2,
-	PIPE_CURRENT_DEVICE_VERSION = 2
-};
+#include "goldfish_pipe.h"
 
 enum {
 	MAX_BUFFERS_PER_COMMAND = 336,
@@ -65,6 +54,9 @@ enum {
 
 struct goldfish_pipe_dev;
 
+static int goldfish_pipe_device_deinit(void *raw_dev,
+				       struct platform_device *pdev);
+
 /* A per-pipe command structure, shared with the host */
 struct goldfish_pipe_command {
 	s32 cmd;	/* PipeCmdCode, guest -> host */
@@ -152,8 +144,8 @@ struct goldfish_pipe {
  * waiting to be awoken.
  */
 struct goldfish_pipe_dev {
-	/* A magic number to check if this is an instance of this struct */
-	void *magic;
+	/* Needed for 'remove' */
+	struct goldfish_pipe_dev_base super;
 
 	/*
 	 * Global device spinlock. Protects the following members:
@@ -593,9 +585,6 @@ static void goldfish_interrupt_task(unsigned long dev_addr)
 	}
 }
 
-static void goldfish_pipe_device_deinit(struct platform_device *pdev,
-					struct goldfish_pipe_dev *dev);
-
 /*
  * The general idea of the interrupt handling:
  *
@@ -616,7 +605,7 @@ static irqreturn_t goldfish_pipe_interrupt(int irq, void *dev_id)
 	unsigned long flags;
 	struct goldfish_pipe_dev *dev = dev_id;
 
-	if (dev->magic != &goldfish_pipe_device_deinit)
+	if (dev->super.deinit != &goldfish_pipe_device_deinit)
 		return IRQ_NONE;
 
 	/* Request the signalled pipes from the device */
@@ -798,9 +787,9 @@ static void write_pa_addr(void *addr, void __iomem *portl, void __iomem *porth)
 	writel(lower_32_bits(paddr), portl);
 }
 
-static int goldfish_pipe_device_init(struct platform_device *pdev,
-				     char __iomem *base,
-				     int irq)
+int goldfish_pipe_device_init(struct platform_device *pdev,
+			      char __iomem *base,
+			      int irq)
 {
 	struct goldfish_pipe_dev *dev;
 	int err;
@@ -809,7 +798,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev,
 	if (!dev)
 		return -ENOMEM;
 
-	dev->magic = &goldfish_pipe_device_deinit;
+	dev->super.deinit = &goldfish_pipe_device_deinit;
 	spin_lock_init(&dev->lock);
 
 	tasklet_init(&dev->irq_tasklet, &goldfish_interrupt_task,
@@ -872,9 +861,11 @@ static int goldfish_pipe_device_init(struct platform_device *pdev,
 	return 0;
 }
 
-static int goldfish_pipe_device_deinit(struct platform_device *pdev,
-				       struct goldfish_pipe_dev *dev)
+static int goldfish_pipe_device_deinit(void *raw_dev,
+				       struct platform_device *pdev)
 {
+	struct goldfish_pipe_dev *dev = raw_dev;
+
 	misc_deregister(&dev->miscdev);
 	tasklet_kill(&dev->irq_tasklet);
 	kfree(dev->pipes);
@@ -882,76 +873,3 @@ static int goldfish_pipe_device_deinit(struct platform_device *pdev,
 
 	return 0;
 }
-
-static int goldfish_pipe_probe(struct platform_device *pdev)
-{
-	struct resource *r;
-	char __iomem *base;
-	int irq;
-	int version;
-
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!r || resource_size(r) < PAGE_SIZE) {
-		dev_err(&pdev->dev, "can't allocate i/o page\n");
-		return -EINVAL;
-	}
-
-	base = devm_ioremap(&pdev->dev, r->start, PAGE_SIZE);
-	if (!base) {
-		dev_err(&pdev->dev, "ioremap failed\n");
-		return -EINVAL;
-	}
-
-	r = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!r)
-		return -EINVAL;
-
-	irq = r->start;
-
-	/*
-	 * Exchange the versions with the host device
-	 *
-	 * Note: v1 driver used to not report its version, so we write it before
-	 *  reading device version back: this allows the host implementation to
-	 *  detect the old driver (if there was no version write before read).
-	 */
-	writel(PIPE_DRIVER_VERSION, base + PIPE_REG_VERSION);
-	version = readl(base + PIPE_REG_VERSION);
-	if (WARN_ON(version < PIPE_CURRENT_DEVICE_VERSION))
-		return -EINVAL;
-
-	return goldfish_pipe_device_init(pdev, base, irq);
-}
-
-static int goldfish_pipe_remove(struct platform_device *pdev)
-{
-	struct goldfish_pipe_dev *dev = platform_get_drvdata(pdev);
-
-	return goldfish_pipe_device_deinit(pdev, dev);
-}
-
-static const struct acpi_device_id goldfish_pipe_acpi_match[] = {
-	{ "GFSH0003", 0 },
-	{ },
-};
-MODULE_DEVICE_TABLE(acpi, goldfish_pipe_acpi_match);
-
-static const struct of_device_id goldfish_pipe_of_match[] = {
-	{ .compatible = "google,android-pipe", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, goldfish_pipe_of_match);
-
-static struct platform_driver goldfish_pipe_driver = {
-	.probe = goldfish_pipe_probe,
-	.remove = goldfish_pipe_remove,
-	.driver = {
-		.name = "goldfish_pipe",
-		.of_match_table = goldfish_pipe_of_match,
-		.acpi_match_table = ACPI_PTR(goldfish_pipe_acpi_match),
-	}
-};
-
-module_platform_driver(goldfish_pipe_driver);
-MODULE_AUTHOR("David Turner <digit@google.com>");
-MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/goldfish/goldfish_pipe_v2.h b/drivers/platform/goldfish/goldfish_pipe_v2.h
new file mode 100644
index 000000000000..03b476fb9978
--- /dev/null
+++ b/drivers/platform/goldfish/goldfish_pipe_v2.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef GOLDFISH_PIPE_V2_H
+#define GOLDFISH_PIPE_V2_H
+
+/* The entry point to the pipe v2 driver */
+int goldfish_pipe_device_init(struct platform_device *pdev,
+			      char __iomem *base,
+			      int irq);
+
+#endif /* #define GOLDFISH_PIPE_V2_H */
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 12/15] platform: goldfish: pipe: Rename the init function (add "v2")

[rkir]

From: Roman Kiryanov <rkir@google.com>

This is the v2 driver. v1 will be added later.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe.c    | 2 +-
 drivers/platform/goldfish/goldfish_pipe_v2.c | 6 +++---
 drivers/platform/goldfish/goldfish_pipe_v2.h | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 792b20bdf76c..7b0920e962eb 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -87,7 +87,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	if (WARN_ON(version < PIPE_CURRENT_DEVICE_VERSION))
 		return -EINVAL;
 
-	return goldfish_pipe_device_init(pdev, base, irq);
+	return goldfish_pipe_device_v2_init(pdev, base, irq);
 }
 
 static int goldfish_pipe_remove(struct platform_device *pdev)
diff --git a/drivers/platform/goldfish/goldfish_pipe_v2.c b/drivers/platform/goldfish/goldfish_pipe_v2.c
index 641dfdcc3ffd..9857ce07d0e6 100644
--- a/drivers/platform/goldfish/goldfish_pipe_v2.c
+++ b/drivers/platform/goldfish/goldfish_pipe_v2.c
@@ -787,9 +787,9 @@ static void write_pa_addr(void *addr, void __iomem *portl, void __iomem *porth)
 	writel(lower_32_bits(paddr), portl);
 }
 
-int goldfish_pipe_device_init(struct platform_device *pdev,
-			      char __iomem *base,
-			      int irq)
+int goldfish_pipe_device_v2_init(struct platform_device *pdev,
+				 char __iomem *base,
+				 int irq)
 {
 	struct goldfish_pipe_dev *dev;
 	int err;
diff --git a/drivers/platform/goldfish/goldfish_pipe_v2.h b/drivers/platform/goldfish/goldfish_pipe_v2.h
index 03b476fb9978..70bf4ec1fd66 100644
--- a/drivers/platform/goldfish/goldfish_pipe_v2.h
+++ b/drivers/platform/goldfish/goldfish_pipe_v2.h
@@ -3,8 +3,8 @@
 #define GOLDFISH_PIPE_V2_H
 
 /* The entry point to the pipe v2 driver */
-int goldfish_pipe_device_init(struct platform_device *pdev,
-			      char __iomem *base,
-			      int irq);
+int goldfish_pipe_device_v2_init(struct platform_device *pdev,
+				 char __iomem *base,
+				 int irq);
 
 #endif /* #define GOLDFISH_PIPE_V2_H */
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 13/15] platform: goldfish: pipe: Add a dedicated constant for the device name

[rkir]

From: Roman Kiryanov <rkir@google.com>

Create a constant to refer to the device name instead if several copies
of a string.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe.h    | 2 ++
 drivers/platform/goldfish/goldfish_pipe_v2.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.h b/drivers/platform/goldfish/goldfish_pipe.h
index ee0b54bcb165..0fa6ecb32c6d 100644
--- a/drivers/platform/goldfish/goldfish_pipe.h
+++ b/drivers/platform/goldfish/goldfish_pipe.h
@@ -2,6 +2,8 @@
 #ifndef GOLDFISH_PIPE_H
 #define GOLDFISH_PIPE_H
 
+#define DEVICE_NAME "goldfish_pipe"
+
 struct goldfish_pipe_dev_base {
 	/* the destructor, the pointer is set in init */
 	int (*deinit)(void *pipe_dev, struct platform_device *pdev);
diff --git a/drivers/platform/goldfish/goldfish_pipe_v2.c b/drivers/platform/goldfish/goldfish_pipe_v2.c
index 9857ce07d0e6..0e2a62322477 100644
--- a/drivers/platform/goldfish/goldfish_pipe_v2.c
+++ b/drivers/platform/goldfish/goldfish_pipe_v2.c
@@ -775,7 +775,7 @@ static void init_miscdevice(struct miscdevice *miscdev)
 	memset(miscdev, 0, sizeof(*miscdev));
 
 	miscdev->minor = MISC_DYNAMIC_MINOR;
-	miscdev->name = "goldfish_pipe";
+	miscdev->name = DEVICE_NAME;
 	miscdev->fops = &goldfish_pipe_fops;
 }
 
@@ -806,7 +806,7 @@ int goldfish_pipe_device_v2_init(struct platform_device *pdev,
 
 	err = devm_request_irq(&pdev->dev, irq,
 			       goldfish_pipe_interrupt,
-			       IRQF_SHARED, "goldfish_pipe", dev);
+			       IRQF_SHARED, DEVICE_NAME, dev);
 	if (err) {
 		dev_err(&pdev->dev, "unable to allocate IRQ for v2\n");
 		return err;
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 14/15] platform: goldfish: pipe: Rename PIPE_REG to PIPE_V2_REG

[rkir]

From: Roman Kiryanov <rkir@google.com>

PIPE_V1_REG will be introduced later for v1 support.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/goldfish_pipe.c      |  4 ++--
 drivers/platform/goldfish/goldfish_pipe_qemu.h | 18 +++++++++---------
 drivers/platform/goldfish/goldfish_pipe_v2.c   | 16 ++++++++--------
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 7b0920e962eb..353f7ce94aa7 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -81,8 +81,8 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	 *  reading device version back: this allows the host implementation to
 	 *  detect the old driver (if there was no version write before read).
 	 */
-	writel(PIPE_DRIVER_VERSION, base + PIPE_REG_VERSION);
-	version = readl(base + PIPE_REG_VERSION);
+	writel(PIPE_DRIVER_VERSION, base + PIPE_V2_REG_VERSION);
+	version = readl(base + PIPE_V2_REG_VERSION);
 
 	if (WARN_ON(version < PIPE_CURRENT_DEVICE_VERSION))
 		return -EINVAL;
diff --git a/drivers/platform/goldfish/goldfish_pipe_qemu.h b/drivers/platform/goldfish/goldfish_pipe_qemu.h
index b4d78c108afd..24b02710769f 100644
--- a/drivers/platform/goldfish/goldfish_pipe_qemu.h
+++ b/drivers/platform/goldfish/goldfish_pipe_qemu.h
@@ -62,19 +62,19 @@ enum PipeFlagsBits {
 	BIT_WAKE_ON_READ   = 2,  /* want to be woken on reads */
 };
 
-enum PipeRegs {
-	PIPE_REG_CMD = 0,
+enum PipeV2Regs {
+	PIPE_V2_REG_CMD = 0,
 
-	PIPE_REG_SIGNAL_BUFFER_HIGH = 4,
-	PIPE_REG_SIGNAL_BUFFER = 8,
-	PIPE_REG_SIGNAL_BUFFER_COUNT = 12,
+	PIPE_V2_REG_SIGNAL_BUFFER_HIGH = 4,
+	PIPE_V2_REG_SIGNAL_BUFFER = 8,
+	PIPE_V2_REG_SIGNAL_BUFFER_COUNT = 12,
 
-	PIPE_REG_OPEN_BUFFER_HIGH = 20,
-	PIPE_REG_OPEN_BUFFER = 24,
+	PIPE_V2_REG_OPEN_BUFFER_HIGH = 20,
+	PIPE_V2_REG_OPEN_BUFFER = 24,
 
-	PIPE_REG_VERSION = 36,
+	PIPE_V2_REG_VERSION = 36,
 
-	PIPE_REG_GET_SIGNALLED = 48,
+	PIPE_V2_REG_GET_SIGNALLED = 48,
 };
 
 enum PipeCmdCode {
diff --git a/drivers/platform/goldfish/goldfish_pipe_v2.c b/drivers/platform/goldfish/goldfish_pipe_v2.c
index 0e2a62322477..c99317548128 100644
--- a/drivers/platform/goldfish/goldfish_pipe_v2.c
+++ b/drivers/platform/goldfish/goldfish_pipe_v2.c
@@ -197,7 +197,7 @@ static int goldfish_pipe_cmd_locked(struct goldfish_pipe *pipe,
 	pipe->command_buffer->cmd = cmd;
 	/* failure by default */
 	pipe->command_buffer->status = PIPE_ERROR_INVAL;
-	writel(pipe->id, pipe->dev->base + PIPE_REG_CMD);
+	writel(pipe->id, pipe->dev->base + PIPE_V2_REG_CMD);
 	return pipe->command_buffer->status;
 }
 
@@ -214,7 +214,7 @@ static int goldfish_pipe_cmd(struct goldfish_pipe *pipe, enum PipeCmdCode cmd)
 
 /*
  * This function converts an error code returned by the emulator through
- * the PIPE_REG_STATUS i/o register into a valid negative errno value.
+ * the PIPE_V2_REG_STATUS i/o register into a valid negative errno value.
  */
 static int goldfish_pipe_error_convert(int status)
 {
@@ -611,7 +611,7 @@ static irqreturn_t goldfish_pipe_interrupt(int irq, void *dev_id)
 	/* Request the signalled pipes from the device */
 	spin_lock_irqsave(&dev->lock, flags);
 
-	count = readl(dev->base + PIPE_REG_GET_SIGNALLED);
+	count = readl(dev->base + PIPE_V2_REG_GET_SIGNALLED);
 	if (count == 0) {
 		spin_unlock_irqrestore(&dev->lock, flags);
 		return IRQ_NONE;
@@ -847,15 +847,15 @@ int goldfish_pipe_device_v2_init(struct platform_device *pdev,
 
 	/* Send the buffer addresses to the host */
 	write_pa_addr(&dev->buffers->signalled_pipe_buffers,
-		      dev->base + PIPE_REG_SIGNAL_BUFFER,
-		      dev->base + PIPE_REG_SIGNAL_BUFFER_HIGH);
+		      dev->base + PIPE_V2_REG_SIGNAL_BUFFER,
+		      dev->base + PIPE_V2_REG_SIGNAL_BUFFER_HIGH);
 
 	writel(MAX_SIGNALLED_PIPES,
-	       dev->base + PIPE_REG_SIGNAL_BUFFER_COUNT);
+	       dev->base + PIPE_V2_REG_SIGNAL_BUFFER_COUNT);
 
 	write_pa_addr(&dev->buffers->open_command_params,
-		      dev->base + PIPE_REG_OPEN_BUFFER,
-		      dev->base + PIPE_REG_OPEN_BUFFER_HIGH);
+		      dev->base + PIPE_V2_REG_OPEN_BUFFER,
+		      dev->base + PIPE_V2_REG_OPEN_BUFFER_HIGH);
 
 	platform_set_drvdata(pdev, dev);
 	return 0;
-- 
2.19.0.605.g01d371f741-goog

[PATCH v3 15/15] platform: goldfish: pipe: Add the goldfish_pipe_v1 driver

[rkir]

From: Roman Kiryanov <rkir@google.com>

This is the v1 goldfish pipe driver.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
Changes in v3:
 - No change.
Changes in v2:
 - No change.

 drivers/platform/goldfish/Makefile            |   2 +-
 drivers/platform/goldfish/goldfish_pipe.c     |   9 +-
 .../platform/goldfish/goldfish_pipe_qemu.h    |  27 +
 drivers/platform/goldfish/goldfish_pipe_v1.c  | 632 ++++++++++++++++++
 drivers/platform/goldfish/goldfish_pipe_v1.h  |  24 +
 5 files changed, 689 insertions(+), 5 deletions(-)
 create mode 100644 drivers/platform/goldfish/goldfish_pipe_v1.c
 create mode 100644 drivers/platform/goldfish/goldfish_pipe_v1.h

diff --git a/drivers/platform/goldfish/Makefile b/drivers/platform/goldfish/Makefile
index 016769e003d8..3fb7427b9dd8 100644
--- a/drivers/platform/goldfish/Makefile
+++ b/drivers/platform/goldfish/Makefile
@@ -2,4 +2,4 @@
 # Makefile for Goldfish platform specific drivers
 #
 obj-$(CONFIG_GOLDFISH_PIPE)    += goldfish_pipe_all.o
-goldfish_pipe_all-objs := goldfish_pipe.o goldfish_pipe_v2.o
+goldfish_pipe_all-objs := goldfish_pipe.o goldfish_pipe_v1.o goldfish_pipe_v2.o
diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 353f7ce94aa7..05a67895cc06 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -39,6 +39,7 @@
 #include <linux/acpi.h>
 #include "goldfish_pipe_qemu.h"
 #include "goldfish_pipe.h"
+#include "goldfish_pipe_v1.h"
 #include "goldfish_pipe_v2.h"
 
 /*
@@ -84,10 +85,10 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	writel(PIPE_DRIVER_VERSION, base + PIPE_V2_REG_VERSION);
 	version = readl(base + PIPE_V2_REG_VERSION);
 
-	if (WARN_ON(version < PIPE_CURRENT_DEVICE_VERSION))
-		return -EINVAL;
-
-	return goldfish_pipe_device_v2_init(pdev, base, irq);
+	if (version < PIPE_CURRENT_DEVICE_VERSION)
+		return goldfish_pipe_device_v1_init(pdev, base, irq);
+	else
+		return goldfish_pipe_device_v2_init(pdev, base, irq);
 }
 
 static int goldfish_pipe_remove(struct platform_device *pdev)
diff --git a/drivers/platform/goldfish/goldfish_pipe_qemu.h b/drivers/platform/goldfish/goldfish_pipe_qemu.h
index 24b02710769f..9a8275f3583d 100644
--- a/drivers/platform/goldfish/goldfish_pipe_qemu.h
+++ b/drivers/platform/goldfish/goldfish_pipe_qemu.h
@@ -62,6 +62,33 @@ enum PipeFlagsBits {
 	BIT_WAKE_ON_READ   = 2,  /* want to be woken on reads */
 };
 
+enum PipeV1Regs {
+	/* write: value = command */
+	PIPE_V1_REG_COMMAND		= 0x00,
+	/* read */
+	PIPE_V1_REG_STATUS		= 0x04,
+	/* read/write: channel id */
+	PIPE_V1_REG_CHANNEL		= 0x08,
+	/* read/write: channel id */
+	PIPE_V1_REG_CHANNEL_HIGH	= 0x30,
+	/* read/write: buffer size */
+	PIPE_V1_REG_SIZE		= 0x0C,
+	/* write: physical address */
+	PIPE_V1_REG_ADDRESS		= 0x10,
+	/* write: physical address */
+	PIPE_V1_REG_ADDRESS_HIGH	= 0x34,
+	/* read: wake flags */
+	PIPE_V1_REG_WAKES		= 0x14,
+	/* read/write: batch data address */
+	PIPE_V1_REG_PARAMS_ADDR_LOW	= 0x18,
+	/* read/write: batch data address */
+	PIPE_V1_REG_PARAMS_ADDR_HIGH	= 0x1C,
+	/* write: batch access */
+	PIPE_V1_REG_ACCESS_PARAMS	= 0x20,
+	/* read: device version */
+	PIPE_V1_REG_VERSION		= 0x24,
+};
+
 enum PipeV2Regs {
 	PIPE_V2_REG_CMD = 0,
 
diff --git a/drivers/platform/goldfish/goldfish_pipe_v1.c b/drivers/platform/goldfish/goldfish_pipe_v1.c
new file mode 100644
index 000000000000..6e603204dd62
--- /dev/null
+++ b/drivers/platform/goldfish/goldfish_pipe_v1.c
@@ -0,0 +1,632 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2011 Google, Inc.
+ * Copyright (C) 2012 Intel, Inc.
+ * Copyright (C) 2013 Intel, Inc.
+ * Copyright (C) 2014 Linaro Limited
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+/* This source file contains the implementation of the legacy version of
+ * a goldfish pipe device driver. See goldfish_pipe_v2.c for the current
+ * version.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/miscdevice.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/bitops.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/dma-mapping.h>
+#include <linux/mm.h>
+#include <linux/bug.h>
+#include <linux/goldfish.h>
+#include "goldfish_pipe_qemu.h"
+#include "goldfish_pipe.h"
+
+#define MAX_PAGES_TO_GRAB 32
+
+/* A value that will not be set by qemu emulator */
+#define INITIAL_BATCH_RESULT (0xdeadbeaf)
+
+struct goldfish_pipe_dev;
+
+/* This data type models a given pipe instance */
+struct goldfish_pipe {
+	struct goldfish_pipe_dev *dev;
+
+	/* The wake flags pipe is waiting for
+	 * Note: not protected with any lock, uses atomic operations
+	 *  and barriers to make it thread-safe.
+	 */
+	unsigned long flags;
+
+	wait_queue_head_t wake_queue;
+
+	/* protects access to the pipe */
+	struct mutex lock;
+};
+
+struct access_params {
+	unsigned long channel;
+	u32 size;
+	unsigned long address;
+	u32 cmd;
+	u32 result;
+	/* reserved for future extension */
+	u32 flags;
+};
+
+/* The driver state. Holds a reference to the i/o page used to
+ * communicate with the emulator, and a wake queue for blocked tasks
+ * waiting to be awoken.
+ */
+struct goldfish_pipe_dev {
+	/* Needed for the 'remove' call */
+	struct goldfish_pipe_dev_base super;
+
+	/* ptr to platform device's device struct */
+	struct device *pdev_dev;
+
+	/* the base address for MMIO */
+	char __iomem *base;
+
+	struct access_params *aps;
+
+	struct miscdevice miscdev;
+
+	/* Global device spinlock */
+	spinlock_t lock;
+};
+
+static int goldfish_pipe_device_deinit(void *raw_dev,
+				       struct platform_device *pdev);
+
+static u32 goldfish_cmd_status(struct goldfish_pipe *pipe, u32 cmd)
+{
+	unsigned long flags;
+	u32 status;
+	struct goldfish_pipe_dev *dev = pipe->dev;
+
+	spin_lock_irqsave(&dev->lock, flags);
+	gf_write_ptr(pipe, dev->base + PIPE_V1_REG_CHANNEL,
+		     dev->base + PIPE_V1_REG_CHANNEL_HIGH);
+	writel(cmd, dev->base + PIPE_V1_REG_COMMAND);
+	status = readl(dev->base + PIPE_V1_REG_STATUS);
+	spin_unlock_irqrestore(&dev->lock, flags);
+	return status;
+}
+
+static void goldfish_cmd(struct goldfish_pipe *pipe, u32 cmd)
+{
+	unsigned long flags;
+	struct goldfish_pipe_dev *dev = pipe->dev;
+
+	spin_lock_irqsave(&dev->lock, flags);
+	gf_write_ptr(pipe, dev->base + PIPE_V1_REG_CHANNEL,
+		     dev->base + PIPE_V1_REG_CHANNEL_HIGH);
+	writel(cmd, dev->base + PIPE_V1_REG_COMMAND);
+	spin_unlock_irqrestore(&dev->lock, flags);
+}
+
+/* This function converts an error code returned by the emulator through
+ * the PIPE_V1_REG_STATUS i/o register into a valid negative errno value.
+ */
+static int goldfish_pipe_error_convert(int status)
+{
+	switch (status) {
+	case PIPE_ERROR_AGAIN:
+		return -EAGAIN;
+	case PIPE_ERROR_NOMEM:
+		return -ENOMEM;
+	case PIPE_ERROR_IO:
+		return -EIO;
+	default:
+		return -EINVAL;
+	}
+}
+
+/*
+ * Notice: QEMU will return 0 for un-known register access, indicating
+ * access_params is supported or not
+ */
+static int valid_batchbuffer_addr(struct goldfish_pipe_dev *dev,
+				  struct access_params *aps)
+{
+	u32 aph, apl;
+	u64 paddr;
+
+	aph = readl(dev->base + PIPE_V1_REG_PARAMS_ADDR_HIGH);
+	apl = readl(dev->base + PIPE_V1_REG_PARAMS_ADDR_LOW);
+
+	paddr = ((u64)aph << 32) | apl;
+	return paddr == (__pa(aps));
+}
+
+static int setup_access_params_addr(struct platform_device *pdev,
+				    struct goldfish_pipe_dev *dev)
+{
+	u64 paddr;
+	struct access_params *aps;
+
+	aps = devm_kzalloc(&pdev->dev, sizeof(struct access_params),
+			   GFP_KERNEL);
+	if (!aps)
+		return -ENOMEM;
+
+	paddr = __pa(aps);
+	writel((u32)(paddr >> 32), dev->base + PIPE_V1_REG_PARAMS_ADDR_HIGH);
+	writel((u32)paddr, dev->base + PIPE_V1_REG_PARAMS_ADDR_LOW);
+
+	if (valid_batchbuffer_addr(dev, aps)) {
+		dev->aps = aps;
+		return 0;
+	}
+
+	devm_kfree(&pdev->dev, aps);
+	return -EFAULT;
+}
+
+static int access_with_param(struct goldfish_pipe_dev *dev, const int cmd,
+			     unsigned long address, unsigned long avail,
+			     struct goldfish_pipe *pipe, int *status)
+{
+	struct access_params *aps = dev->aps;
+
+	if (!aps)
+		return -EINVAL;
+
+	aps->result = INITIAL_BATCH_RESULT;
+	aps->channel = (unsigned long)pipe;
+	aps->size = avail;
+	aps->address = address;
+	aps->cmd = cmd;
+	writel(cmd, dev->base + PIPE_V1_REG_ACCESS_PARAMS);
+
+	/*
+	 * If the aps->result has not changed, that means
+	 * that the batch command failed
+	 */
+	if (aps->result == INITIAL_BATCH_RESULT)
+		return -EINVAL;
+
+	*status = aps->result;
+	return 0;
+}
+
+static int transfer_pages(struct goldfish_pipe_dev *dev,
+			  struct goldfish_pipe *pipe,
+			  int cmd,
+			  unsigned long xaddr,
+			  unsigned long size)
+{
+	unsigned long irq_flags;
+	int status = 0;
+
+	spin_lock_irqsave(&dev->lock, irq_flags);
+	if (access_with_param(dev, cmd, xaddr, size, pipe, &status)) {
+		gf_write_ptr(pipe, dev->base + PIPE_V1_REG_CHANNEL,
+			     dev->base + PIPE_V1_REG_CHANNEL_HIGH);
+
+		writel(size, dev->base + PIPE_V1_REG_SIZE);
+
+		gf_write_ptr((void *)xaddr,
+			     dev->base + PIPE_V1_REG_ADDRESS,
+			     dev->base + PIPE_V1_REG_ADDRESS_HIGH);
+
+		writel(cmd, dev->base + PIPE_V1_REG_COMMAND);
+
+		status = readl(dev->base + PIPE_V1_REG_STATUS);
+	}
+	spin_unlock_irqrestore(&dev->lock, irq_flags);
+
+	return status;
+}
+
+static unsigned long translate_address(const struct page *page,
+				       unsigned long addr)
+{
+	return page_to_phys(page) | (addr & ~PAGE_MASK);
+}
+
+static ssize_t goldfish_pipe_read_write(struct file *filp, char __user *buffer,
+					size_t bufflen, int is_write)
+{
+	struct goldfish_pipe *pipe = filp->private_data;
+	struct goldfish_pipe_dev *dev = pipe->dev;
+	unsigned long address;
+	unsigned long address_end;
+	const int wake_bit = is_write ? BIT_WAKE_ON_WRITE : BIT_WAKE_ON_READ;
+	const int pipe_cmd = is_write ? PIPE_CMD_WRITE : PIPE_CMD_READ;
+	int count = 0;
+	int ret = -EINVAL;
+
+	/* If the emulator already closed the pipe, no need to go further */
+	if (test_bit(BIT_CLOSED_ON_HOST, &pipe->flags))
+		return -EIO;
+
+	/* Null reads or writes succeeds */
+	if (unlikely(bufflen == 0))
+		return 0;
+
+	/* Check the buffer range for access */
+	if (!access_ok(is_write ? VERIFY_WRITE : VERIFY_READ,
+		       buffer, bufflen))
+		return -EFAULT;
+
+	address = (unsigned long)buffer;
+	address_end = address + bufflen;
+
+	/* Serialize access to the pipe */
+	if (mutex_lock_interruptible(&pipe->lock))
+		return -ERESTARTSYS;
+
+	while (address < address_end) {
+		struct page *pages[MAX_PAGES_TO_GRAB];
+		unsigned long page_end = (address & PAGE_MASK) + PAGE_SIZE;
+		unsigned long avail;
+		unsigned long xaddr;
+		unsigned long xaddr_prev;
+		long first_page;
+		long last_page;
+		long requested_pages;
+		int status;
+		int n_pages;
+		int page_i;
+		int num_contiguous_pages;
+
+		/*
+		 * Attempt to grab multiple physically contiguous pages.
+		 */
+		first_page = address & PAGE_MASK;
+		last_page = (address_end - 1) & PAGE_MASK;
+		requested_pages =
+			min(((last_page - first_page) >> PAGE_SHIFT) + 1,
+			    (long)MAX_PAGES_TO_GRAB);
+
+		ret = get_user_pages_fast(first_page, requested_pages,
+					  !is_write, pages);
+		if (ret < 0) {
+			dev_err(dev->pdev_dev,
+				"%s: get_user_pages_fast failed: %d\n",
+				__func__, ret);
+			break;
+		} else if (!ret) {
+			dev_err(dev->pdev_dev,
+				"%s: error: no pages returned, requested %ld\n",
+				__func__, requested_pages);
+			break;
+		}
+
+		n_pages = ret;
+		xaddr = translate_address(pages[0], address);
+		xaddr_prev = xaddr;
+		num_contiguous_pages = 1;
+		for (page_i = 1; page_i < n_pages; page_i++) {
+			unsigned long xaddr_i;
+
+			xaddr_i = translate_address(pages[page_i], address);
+			if (xaddr_i == xaddr_prev + PAGE_SIZE) {
+				page_end += PAGE_SIZE;
+				xaddr_prev = xaddr_i;
+				num_contiguous_pages++;
+			} else {
+				dev_err(dev->pdev_dev,
+					"%s: discontinuous page boundary: %d "
+					"pages instead\n",
+					__func__, page_i);
+				break;
+			}
+		}
+		avail = min(page_end, address_end) - address;
+
+		status = transfer_pages(dev, pipe, pipe_cmd, xaddr, avail);
+
+		for (page_i = 0; page_i < n_pages; page_i++) {
+			if (status > 0 && !is_write &&
+			    page_i < num_contiguous_pages)
+				set_page_dirty(pages[page_i]);
+
+			put_page(pages[page_i]);
+		}
+
+		if (status > 0) { /* Correct transfer */
+			count += status;
+			address += status;
+			continue;
+		} else if (status == 0) { /* EOF */
+			ret = 0;
+			break;
+		} else if (status < 0 && count > 0) {
+			/*
+			 * An error occurred and we already transferred
+			 * something on one of the previous pages.
+			 * Just return what we already copied and log this
+			 * err.
+			 *
+			 * Note: This seems like an incorrect approach but
+			 * cannot change it until we check if any user space
+			 * ABI relies on this behavior.
+			 */
+			if (status != PIPE_ERROR_AGAIN)
+				dev_err_ratelimited(dev->pdev_dev,
+					"backend returned error %d on %s\n",
+					status, is_write ? "write" : "read");
+			ret = 0;
+			break;
+		}
+
+		/*
+		 * If the error is not PIPE_ERROR_AGAIN, or if we are not in
+		 * non-blocking mode, just return the error code.
+		 */
+		if (status != PIPE_ERROR_AGAIN ||
+		    (filp->f_flags & O_NONBLOCK) != 0) {
+			ret = goldfish_pipe_error_convert(status);
+			break;
+		}
+
+		/*
+		 * The backend blocked the read/write, wait until the backend
+		 * tells us it's ready to process more data.
+		 */
+		set_bit(wake_bit, &pipe->flags);
+
+		/* Tell the emulator we're going to wait for a wake event */
+		goldfish_cmd(pipe, pipe_cmd);
+
+		/* Unlock the pipe, then wait for the wake signal */
+		mutex_unlock(&pipe->lock);
+
+		while (test_bit(wake_bit, &pipe->flags)) {
+			if (wait_event_interruptible(pipe->wake_queue,
+					!test_bit(wake_bit, &pipe->flags)))
+				return -ERESTARTSYS;
+
+			if (test_bit(BIT_CLOSED_ON_HOST, &pipe->flags))
+				return -EIO;
+		}
+
+		/* Try to re-acquire the lock */
+		if (mutex_lock_interruptible(&pipe->lock))
+			return -ERESTARTSYS;
+	}
+	mutex_unlock(&pipe->lock);
+
+	return (ret < 0) ? ret : count;
+}
+
+static ssize_t goldfish_pipe_read(struct file *filp, char __user *buffer,
+				  size_t bufflen, loff_t *ppos)
+{
+	return goldfish_pipe_read_write(filp, buffer, bufflen,
+					/* is_write */ 0);
+}
+
+static ssize_t goldfish_pipe_write(struct file *filp,
+				   const char __user *buffer, size_t bufflen,
+				   loff_t *ppos)
+{
+	return goldfish_pipe_read_write(filp, (char __user *)buffer,
+					bufflen, /* is_write */ 1);
+}
+
+static __poll_t goldfish_pipe_poll(struct file *filp, poll_table *wait)
+{
+	struct goldfish_pipe *pipe = filp->private_data;
+	__poll_t mask = 0;
+	int status;
+
+	if (mutex_lock_interruptible(&pipe->lock))
+		return -ERESTARTSYS;
+
+	poll_wait(filp, &pipe->wake_queue, wait);
+
+	status = goldfish_cmd_status(pipe, PIPE_CMD_POLL);
+
+	mutex_unlock(&pipe->lock);
+
+	if (status & PIPE_POLL_IN)
+		mask |= EPOLLIN | EPOLLRDNORM;
+
+	if (status & PIPE_POLL_OUT)
+		mask |= EPOLLOUT | EPOLLWRNORM;
+
+	if (status & PIPE_POLL_HUP)
+		mask |= EPOLLHUP;
+
+	if (test_bit(BIT_CLOSED_ON_HOST, &pipe->flags))
+		mask |= EPOLLERR;
+
+	return mask;
+}
+
+static irqreturn_t goldfish_pipe_interrupt(int irq, void *dev_id)
+{
+	struct goldfish_pipe_dev *dev = dev_id;
+	unsigned long irq_flags;
+	int count = 0;
+
+	/*
+	 * We're going to read from the emulator a list of (channel,flags)
+	 * pairs corresponding to the wake events that occurred on each
+	 * blocked pipe (i.e. channel).
+	 */
+	spin_lock_irqsave(&dev->lock, irq_flags);
+	for (;;) {
+		/* First read the channel, 0 means the end of the list */
+		struct goldfish_pipe *pipe;
+		unsigned long wakes;
+		unsigned long channel = 0;
+
+#ifdef CONFIG_64BIT
+		channel =
+			(u64)readl(dev->base + PIPE_V1_REG_CHANNEL_HIGH) << 32;
+#endif
+		channel |= readl(dev->base + PIPE_V1_REG_CHANNEL);
+		if (!channel)
+			break;
+
+		/* Convert channel to struct pipe pointer + read wake flags */
+		wakes = readl(dev->base + PIPE_V1_REG_WAKES);
+		pipe  = (struct goldfish_pipe *)(ptrdiff_t)channel;
+
+		/* Did the emulator just closed a pipe? */
+		if (wakes & PIPE_WAKE_CLOSED) {
+			set_bit(BIT_CLOSED_ON_HOST, &pipe->flags);
+			wakes |= PIPE_WAKE_READ | PIPE_WAKE_WRITE;
+		}
+		if (wakes & PIPE_WAKE_READ)
+			clear_bit(BIT_WAKE_ON_READ, &pipe->flags);
+		if (wakes & PIPE_WAKE_WRITE)
+			clear_bit(BIT_WAKE_ON_WRITE, &pipe->flags);
+
+		wake_up_interruptible(&pipe->wake_queue);
+		count++;
+	}
+	spin_unlock_irqrestore(&dev->lock, irq_flags);
+
+	return (count == 0) ? IRQ_NONE : IRQ_HANDLED;
+}
+
+/* A helper function to get the instance of goldfish_pipe_dev from file */
+static struct goldfish_pipe_dev *to_goldfish_pipe_dev(struct file *file)
+{
+	struct miscdevice *miscdev = file->private_data;
+
+	return container_of(miscdev, struct goldfish_pipe_dev, miscdev);
+}
+
+/**
+ *	goldfish_pipe_open - open a channel to the AVD
+ *	@inode: inode of device
+ *	@file: file struct of opener
+ *
+ *	Create a new pipe link between the emulator and the use application.
+ *	Each new request produces a new pipe.
+ *
+ *	Note: we use the pipe ID as a mux. All goldfish emulations are 32bit
+ *	right now so this is fine. A move to 64bit will need this addressing
+ */
+static int goldfish_pipe_open(struct inode *inode, struct file *file)
+{
+	struct goldfish_pipe_dev *dev = to_goldfish_pipe_dev(file);
+	struct goldfish_pipe *pipe;
+	int status;
+
+	/* Allocate new pipe kernel object */
+	pipe = kzalloc(sizeof(*pipe), GFP_KERNEL);
+	if (!pipe)
+		return -ENOMEM;
+
+	pipe->dev = dev;
+	init_waitqueue_head(&pipe->wake_queue);
+	mutex_init(&pipe->lock);
+
+	/*
+	 * Now, tell the emulator we're opening a new pipe. We use the
+	 * pipe object's address as the channel identifier for simplicity.
+	 */
+
+	status = goldfish_cmd_status(pipe, PIPE_CMD_OPEN);
+	if (status < 0) {
+		kfree(pipe);
+		return status;
+	}
+
+	/* All is done, save the pipe into the file's private data field */
+	file->private_data = pipe;
+	return 0;
+}
+
+static int goldfish_pipe_release(struct inode *inode, struct file *filp)
+{
+	struct goldfish_pipe *pipe = filp->private_data;
+
+	pr_debug("%s: call. pipe=%p file=%p\n", __func__, pipe, filp);
+	/* The guest is closing the channel, so tell the emulator right now */
+	goldfish_cmd(pipe, PIPE_CMD_CLOSE);
+	kfree(pipe);
+	filp->private_data = NULL;
+	return 0;
+}
+
+static const struct file_operations goldfish_pipe_fops = {
+	.owner = THIS_MODULE,
+	.read = goldfish_pipe_read,
+	.write = goldfish_pipe_write,
+	.poll = goldfish_pipe_poll,
+	.open = goldfish_pipe_open,
+	.release = goldfish_pipe_release,
+};
+
+static void init_miscdevice(struct miscdevice *miscdev)
+{
+	memset(miscdev, 0, sizeof(*miscdev));
+
+	miscdev->minor = MISC_DYNAMIC_MINOR;
+	miscdev->name = DEVICE_NAME;
+	miscdev->fops = &goldfish_pipe_fops;
+};
+
+static int goldfish_pipe_device_deinit(void *raw_dev,
+				       struct platform_device *pdev);
+
+int goldfish_pipe_device_v1_init(struct platform_device *pdev,
+				 void __iomem *base,
+				 int irq)
+{
+	struct goldfish_pipe_dev *dev;
+	int err;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->super.deinit = &goldfish_pipe_device_deinit;
+	dev->pdev_dev = &pdev->dev;
+	spin_lock_init(&dev->lock);
+
+	err = devm_request_irq(&pdev->dev, irq,
+			       &goldfish_pipe_interrupt, IRQF_SHARED,
+			       DEVICE_NAME, dev);
+	if (err) {
+		dev_err(&pdev->dev, "unable to allocate IRQ for v1\n");
+		return err;
+	}
+
+	init_miscdevice(&dev->miscdev);
+	err = misc_register(&dev->miscdev);
+	if (err) {
+		dev_err(&pdev->dev, "unable to register v1 device\n");
+		return err;
+	}
+
+	setup_access_params_addr(pdev, dev);
+
+	platform_set_drvdata(pdev, dev);
+	return 0;
+}
+
+static int goldfish_pipe_device_deinit(void *raw_dev,
+				       struct platform_device *pdev)
+{
+	struct goldfish_pipe_dev *dev = raw_dev;
+
+	misc_deregister(&dev->miscdev);
+	return 0;
+}
diff --git a/drivers/platform/goldfish/goldfish_pipe_v1.h b/drivers/platform/goldfish/goldfish_pipe_v1.h
new file mode 100644
index 000000000000..6d61441e2a7f
--- /dev/null
+++ b/drivers/platform/goldfish/goldfish_pipe_v1.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef GOLDFISH_PIPE_V1_H
+#define GOLDFISH_PIPE_V1_H
+
+/* The entry point to the pipe v1 driver */
+int goldfish_pipe_device_v1_init(struct platform_device *pdev,
+				 void __iomem *base,
+				 int irq);
+
+#endif /* #define GOLDFISH_PIPE_V1_H */
-- 
2.19.0.605.g01d371f741-goog