[PATCH 1/9] platform: goldfish: pipe: Fix comments to fit 80 columns

[PATCH 1/9] platform: goldfish: pipe: Fix comments to fit 80 columns

[rkir]

From: Roman Kiryanov <rkir@google.com>

Some comment lines are longer than 80 symbols.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 drivers/platform/goldfish/goldfish_pipe.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 3e32a4c14d5f..567005fe11f8 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -84,7 +84,10 @@ enum PipePollFlags {
 	PIPE_POLL_HUP	= 1 << 2
 };
 
-/* Possible status values used to signal errors - see goldfish_pipe_error_convert */
+/*
+ * Possible status values used to signal errors - see
+ * goldfish_pipe_error_convert
+ */
 enum PipeErrors {
 	PIPE_ERROR_INVAL  = -1,
 	PIPE_ERROR_AGAIN  = -2,
@@ -149,9 +152,9 @@ struct goldfish_pipe_command;
 
 /* A per-pipe command structure, shared with the host */
 struct goldfish_pipe_command {
-	s32 cmd;		/* PipeCmdCode, guest -> host */
-	s32 id;			/* pipe id, guest -> host */
-	s32 status;		/* command execution status, host -> guest */
+	s32 cmd;	/* PipeCmdCode, guest -> host */
+	s32 id;		/* pipe id, guest -> host */
+	s32 status;	/* command execution status, host -> guest */
 	s32 reserved;	/* to pad to 64-bit boundary */
 	union {
 		/* Parameters for PIPE_CMD_{READ,WRITE} */
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 2/9] platform: goldfish: pipe: Update license

[rkir]

From: Roman Kiryanov <rkir@google.com>

goldfish_pipe is distributed under GPL v2.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 drivers/platform/goldfish/goldfish_pipe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 567005fe11f8..065b2eaab9cc 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) 2012 Intel, Inc.
  * Copyright (C) 2013 Intel, Inc.
@@ -983,4 +984,4 @@ static struct platform_driver goldfish_pipe_driver = {
 
 module_platform_driver(goldfish_pipe_driver);
 MODULE_AUTHOR("David Turner <digit@google.com>");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 3/9] platform: goldfish: pipe: Fix checkpatch warning

[rkir]

From: Roman Kiryanov <rkir@google.com>

Function's opening brace has to be at the beginning of the next line.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 drivers/platform/goldfish/goldfish_pipe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 065b2eaab9cc..0607897c6a59 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -587,7 +587,8 @@ static void signalled_pipes_add_locked(struct goldfish_pipe_dev *dev,
 }
 
 static void signalled_pipes_remove_locked(struct goldfish_pipe_dev *dev,
-	struct goldfish_pipe *pipe) {
+	struct goldfish_pipe *pipe)
+{
 	if (pipe->prev_signalled)
 		pipe->prev_signalled->next_signalled = pipe->next_signalled;
 	if (pipe->next_signalled)
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 4/9] platform: goldfish: pipe: Separate the host interface to a separate header

[rkir]

From: Roman Kiryanov <rkir@google.com>

These are several enums that must kept in sync with the host side.
This change explicitly separates them into a dedicated header file.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 drivers/platform/goldfish/goldfish_pipe.c     |  69 +----------
 .../platform/goldfish/goldfish_pipe_qemu.h    | 112 ++++++++++++++++++
 2 files changed, 113 insertions(+), 68 deletions(-)
 create mode 100644 drivers/platform/goldfish/goldfish_pipe_qemu.h

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 0607897c6a59..8f9580454154 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -63,6 +63,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/mm.h>
 #include <linux/acpi.h>
+#include "goldfish_pipe_qemu.h"
 
 /*
  * Update this when something changes in the driver's behavior so the host
@@ -73,74 +74,6 @@ enum {
 	PIPE_CURRENT_DEVICE_VERSION = 2
 };
 
-/*
- * IMPORTANT: The following constants must match the ones used and defined
- * in external/qemu/hw/goldfish_pipe.c in the Android source tree.
- */
-
-/* List of bitflags returned in status of CMD_POLL command */
-enum PipePollFlags {
-	PIPE_POLL_IN	= 1 << 0,
-	PIPE_POLL_OUT	= 1 << 1,
-	PIPE_POLL_HUP	= 1 << 2
-};
-
-/*
- * Possible status values used to signal errors - see
- * goldfish_pipe_error_convert
- */
-enum PipeErrors {
-	PIPE_ERROR_INVAL  = -1,
-	PIPE_ERROR_AGAIN  = -2,
-	PIPE_ERROR_NOMEM  = -3,
-	PIPE_ERROR_IO     = -4
-};
-
-/* Bit-flags used to signal events from the emulator */
-enum PipeWakeFlags {
-	PIPE_WAKE_CLOSED = 1 << 0,  /* emulator closed pipe */
-	PIPE_WAKE_READ   = 1 << 1,  /* pipe can now be read from */
-	PIPE_WAKE_WRITE  = 1 << 2  /* pipe can now be written to */
-};
-
-/* Bit flags for the 'flags' field */
-enum PipeFlagsBits {
-	BIT_CLOSED_ON_HOST = 0,  /* pipe closed by host */
-	BIT_WAKE_ON_WRITE  = 1,  /* want to be woken on writes */
-	BIT_WAKE_ON_READ   = 2,  /* want to be woken on reads */
-};
-
-enum PipeRegs {
-	PIPE_REG_CMD = 0,
-
-	PIPE_REG_SIGNAL_BUFFER_HIGH = 4,
-	PIPE_REG_SIGNAL_BUFFER = 8,
-	PIPE_REG_SIGNAL_BUFFER_COUNT = 12,
-
-	PIPE_REG_OPEN_BUFFER_HIGH = 20,
-	PIPE_REG_OPEN_BUFFER = 24,
-
-	PIPE_REG_VERSION = 36,
-
-	PIPE_REG_GET_SIGNALLED = 48,
-};
-
-enum PipeCmdCode {
-	PIPE_CMD_OPEN = 1,	/* to be used by the pipe device itself */
-	PIPE_CMD_CLOSE,
-	PIPE_CMD_POLL,
-	PIPE_CMD_WRITE,
-	PIPE_CMD_WAKE_ON_WRITE,
-	PIPE_CMD_READ,
-	PIPE_CMD_WAKE_ON_READ,
-
-	/*
-	 * TODO(zyy): implement a deferred read/write execution to allow
-	 * parallel processing of pipe operations on the host.
-	 */
-	PIPE_CMD_WAKE_ON_DONE_IO,
-};
-
 enum {
 	MAX_BUFFERS_PER_COMMAND = 336,
 	MAX_SIGNALLED_PIPES = 64,
diff --git a/drivers/platform/goldfish/goldfish_pipe_qemu.h b/drivers/platform/goldfish/goldfish_pipe_qemu.h
new file mode 100644
index 000000000000..51f3d9f82af6
--- /dev/null
+++ b/drivers/platform/goldfish/goldfish_pipe_qemu.h
@@ -0,0 +1,112 @@
+/* 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.
+ *
+ */
+
+/*
+ * IMPORTANT: The following constants must match the ones used and defined in
+ * external/qemu/include/hw/misc/goldfish_pipe.h
+ */
+
+#ifndef ANDROID_PIPE_COMMON_H
+#define ANDROID_PIPE_COMMON_H
+
+/* List of bitflags returned in status of CMD_POLL command */
+enum PipePollFlags {
+	PIPE_POLL_IN	= 1 << 0,
+	PIPE_POLL_OUT	= 1 << 1,
+	PIPE_POLL_HUP	= 1 << 2
+};
+
+/* Possible status values used to signal errors */
+enum PipeErrors {
+	PIPE_ERROR_INVAL	= -1,
+	PIPE_ERROR_AGAIN	= -2,
+	PIPE_ERROR_NOMEM	= -3,
+	PIPE_ERROR_IO		= -4
+};
+
+/* Bit-flags used to signal events from the emulator */
+enum PipeWakeFlags {
+	/* emulator closed pipe */
+	PIPE_WAKE_CLOSED		= 1 << 0,
+
+	/* pipe can now be read from */
+	PIPE_WAKE_READ			= 1 << 1,
+
+	/* pipe can now be written to */
+	PIPE_WAKE_WRITE			= 1 << 2,
+
+	/* unlock this pipe's DMA buffer */
+	PIPE_WAKE_UNLOCK_DMA		= 1 << 3,
+
+	/* unlock DMA buffer of the pipe shared to this pipe */
+	PIPE_WAKE_UNLOCK_DMA_SHARED	= 1 << 4,
+};
+
+/* Possible pipe closing reasons */
+enum PipeCloseReason {
+	/* guest sent a close command */
+	PIPE_CLOSE_GRACEFUL		= 0,
+
+	/* guest rebooted, we're closing the pipes */
+	PIPE_CLOSE_REBOOT		= 1,
+
+	/* close old pipes on snapshot load */
+	PIPE_CLOSE_LOAD_SNAPSHOT	= 2,
+
+	/* some unrecoverable error on the pipe */
+	PIPE_CLOSE_ERROR		= 3,
+};
+
+/* Bit flags for the 'flags' field */
+enum PipeFlagsBits {
+	BIT_CLOSED_ON_HOST = 0,  /* pipe closed by host */
+	BIT_WAKE_ON_WRITE  = 1,  /* want to be woken on writes */
+	BIT_WAKE_ON_READ   = 2,  /* want to be woken on reads */
+};
+
+enum PipeRegs {
+	PIPE_REG_CMD = 0,
+
+	PIPE_REG_SIGNAL_BUFFER_HIGH = 4,
+	PIPE_REG_SIGNAL_BUFFER = 8,
+	PIPE_REG_SIGNAL_BUFFER_COUNT = 12,
+
+	PIPE_REG_OPEN_BUFFER_HIGH = 20,
+	PIPE_REG_OPEN_BUFFER = 24,
+
+	PIPE_REG_VERSION = 36,
+
+	PIPE_REG_GET_SIGNALLED = 48,
+};
+
+enum PipeCmdCode {
+	/* to be used by the pipe device itself */
+	PIPE_CMD_OPEN		= 1,
+
+	PIPE_CMD_CLOSE,
+	PIPE_CMD_POLL,
+	PIPE_CMD_WRITE,
+	PIPE_CMD_WAKE_ON_WRITE,
+	PIPE_CMD_READ,
+	PIPE_CMD_WAKE_ON_READ,
+
+	/*
+	 * TODO(zyy): implement a deferred read/write execution to allow
+	 * parallel processing of pipe operations on the host.
+	 */
+	PIPE_CMD_WAKE_ON_DONE_IO,
+};
+
+#endif /* ANDROID_PIPE_COMMON_H */
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 5/9] platform: goldfish: pipe: Update the comment for GFP_ATOMIC

[rkir]

From: Roman Kiryanov <rkir@google.com>

Provide an explanation why GFP_ATOMIC is needed to prevent changing it to
other values.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 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 8f9580454154..a4db4d12b09c 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -639,7 +639,10 @@ static int get_free_pipe_id_locked(struct goldfish_pipe_dev *dev)
 			return id;
 
 	{
-		/* Reallocate the array */
+		/* Reallocate the array.
+		 * Since get_free_pipe_id_locked runs with interrupts disabled,
+		 * we don't want to make calls that could lead to sleep.
+		 */
 		u32 new_capacity = 2 * dev->pipes_capacity;
 		struct goldfish_pipe **pipes =
 			kcalloc(new_capacity, sizeof(*pipes), GFP_ATOMIC);
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 6/9] platform: goldfish: pipe: Fail compilation if structs are too large

[rkir]

From: Roman Kiryanov <rkir@google.com>

Since the driver provides no workaround prevent in cases if structs do
no fit into a memory page, it is better to fail complation to find about
the issue earlt instead of returning errors at runtime.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 drivers/platform/goldfish/goldfish_pipe.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index a4db4d12b09c..5eed4c824a53 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -63,6 +63,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/mm.h>
 #include <linux/acpi.h>
+#include <linux/bug.h>
 #include "goldfish_pipe_qemu.h"
 
 /*
@@ -797,9 +798,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
 	 * needs to be contained in a single physical page. The easiest choice
 	 * is to just allocate a page and place the buffers in it.
 	 */
-	if (WARN_ON(sizeof(*dev->buffers) > PAGE_SIZE))
-		return -ENOMEM;
-
+	BUILD_BUG_ON(sizeof(*dev->buffers) > PAGE_SIZE);
 	page = (char *)__get_free_page(GFP_KERNEL);
 	if (!page) {
 		kfree(dev->pipes);
@@ -842,8 +841,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 	struct resource *r;
 	struct goldfish_pipe_dev *dev = pipe_dev;
 
-	if (WARN_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE))
-		return -ENOMEM;
+	BUILD_BUG_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE);
 
 	/* not thread safe, but this should not happen */
 	WARN_ON(dev->base != NULL);
-- 
2.18.0.865.gffc8e1a3cd6-goog

Re: [PATCH 6/9] platform: goldfish: pipe: Fail compilation if structs are too large

[Joe Perches]

On Mon, 2018-08-13 at 16:38 -0700, rkir@google.com wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> Since the driver provides no workaround prevent in cases if structs do
> no fit into a memory page, it is better to fail complation to find about
> the issue earlt instead of returning errors at runtime.

Minor earlt/early typo

> diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
[]
> @@ -797,9 +798,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
>  	 * needs to be contained in a single physical page. The easiest choice
>  	 * is to just allocate a page and place the buffers in it.
>  	 */
> -	if (WARN_ON(sizeof(*dev->buffers) > PAGE_SIZE))
> -		return -ENOMEM;
> -
> +	BUILD_BUG_ON(sizeof(*dev->buffers) > PAGE_SIZE);
>  	page = (char *)__get_free_page(GFP_KERNEL);
>  	if (!page) {
>  		kfree(dev->pipes);
> @@ -842,8 +841,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
>  	struct resource *r;
>  	struct goldfish_pipe_dev *dev = pipe_dev;
>  
> -	if (WARN_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE))
> -		return -ENOMEM;
> +	BUILD_BUG_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE);
>  
>  	/* not thread safe, but this should not happen */
>  	WARN_ON(dev->base != NULL);

Why separate these BUILD_BUG_ONs into 2 different functions?

Why not just
	BUILD_BUG_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE);
	BUILD_BUG_ON(sizeof(struct goldfish_pipe_dev_buffers) > PAGE_SIZE);

Re: [PATCH 6/9] platform: goldfish: pipe: Fail compilation if structs are too large

[Roman Kiryanov]

Hi,

thank you for reviewing my patches. I decided to put BUILD_BUG_ON
close to places where it is important that these structs fit into a
memory page to give some context.

Regards,
Roman.
On Mon, Aug 13, 2018 at 6:48 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2018-08-13 at 16:38 -0700, rkir@google.com wrote:
> > From: Roman Kiryanov <rkir@google.com>
> >
> > Since the driver provides no workaround prevent in cases if structs do
> > no fit into a memory page, it is better to fail complation to find about
> > the issue earlt instead of returning errors at runtime.
>
> Minor earlt/early typo
>
> > diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
> []
> > @@ -797,9 +798,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
> >        * needs to be contained in a single physical page. The easiest choice
> >        * is to just allocate a page and place the buffers in it.
> >        */
> > -     if (WARN_ON(sizeof(*dev->buffers) > PAGE_SIZE))
> > -             return -ENOMEM;
> > -
> > +     BUILD_BUG_ON(sizeof(*dev->buffers) > PAGE_SIZE);
> >       page = (char *)__get_free_page(GFP_KERNEL);
> >       if (!page) {
> >               kfree(dev->pipes);
> > @@ -842,8 +841,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
> >       struct resource *r;
> >       struct goldfish_pipe_dev *dev = pipe_dev;
> >
> > -     if (WARN_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE))
> > -             return -ENOMEM;
> > +     BUILD_BUG_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE);
> >
> >       /* not thread safe, but this should not happen */
> >       WARN_ON(dev->base != NULL);
>
> Why separate these BUILD_BUG_ONs into 2 different functions?
>
> Why not just
>         BUILD_BUG_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE);
>         BUILD_BUG_ON(sizeof(struct goldfish_pipe_dev_buffers) > PAGE_SIZE);
>

Re: [PATCH 6/9] platform: goldfish: pipe: Fail compilation if structs are too large

[Joe Perches]

On Mon, 2018-08-13 at 20:47 -0700, Roman Kiryanov wrote:
> Hi,
> 
> thank you for reviewing my patches. I decided to put BUILD_BUG_ON
> close to places where it is important that these structs fit into a
> memory page to give some context.

And you make the reader figure out what type dev->buffers is
unnecessarily when sizeof(struct goldfish_pipe_dev_buffers)
is pretty obvious.

> Regards,
> Roman.

cheers, Joe

[PATCH 7/9] platform: goldfish: pipe: Replace an array of 1 with a variable

[rkir]

From: Roman Kiryanov <rkir@google.com>

There is no reason to have an array of 1.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 drivers/platform/goldfish/goldfish_pipe.c | 28 +++++++++++------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 5eed4c824a53..6f3fc4b30fba 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -204,7 +204,7 @@ struct goldfish_pipe_dev {
 	unsigned char __iomem *base;
 };
 
-static struct goldfish_pipe_dev pipe_dev[1] = {};
+struct goldfish_pipe_dev goldfish_pipe_dev;
 
 static int goldfish_cmd_locked(struct goldfish_pipe *pipe, enum PipeCmdCode cmd)
 {
@@ -563,12 +563,12 @@ static struct goldfish_pipe *signalled_pipes_pop_front(
 
 static void goldfish_interrupt_task(unsigned long unused)
 {
-	struct goldfish_pipe_dev *dev = pipe_dev;
 	/* Iterate over the signalled pipes and wake them one by one */
 	struct goldfish_pipe *pipe;
 	int wakes;
 
-	while ((pipe = signalled_pipes_pop_front(dev, &wakes)) != NULL) {
+	while ((pipe = signalled_pipes_pop_front(&goldfish_pipe_dev, &wakes)) !=
+			NULL) {
 		if (wakes & PIPE_WAKE_CLOSED) {
 			pipe->flags = 1 << BIT_CLOSED_ON_HOST;
 		} else {
@@ -606,7 +606,7 @@ static irqreturn_t goldfish_pipe_interrupt(int irq, void *dev_id)
 	unsigned long flags;
 	struct goldfish_pipe_dev *dev = dev_id;
 
-	if (dev != pipe_dev)
+	if (dev != &goldfish_pipe_dev)
 		return IRQ_NONE;
 
 	/* Request the signalled pipes from the device */
@@ -671,7 +671,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 = pipe_dev;
+	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
 	unsigned long flags;
 	int id;
 	int status;
@@ -761,7 +761,7 @@ static const struct file_operations goldfish_pipe_fops = {
 	.release = goldfish_pipe_release,
 };
 
-static struct miscdevice goldfish_pipe_dev = {
+static struct miscdevice goldfish_pipe_miscdev = {
 	.minor = MISC_DYNAMIC_MINOR,
 	.name = "goldfish_pipe",
 	.fops = &goldfish_pipe_fops,
@@ -769,8 +769,8 @@ static struct miscdevice goldfish_pipe_dev = {
 
 static int goldfish_pipe_device_init(struct platform_device *pdev)
 {
+	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
 	char *page;
-	struct goldfish_pipe_dev *dev = pipe_dev;
 	int err = devm_request_irq(&pdev->dev, dev->irq,
 				goldfish_pipe_interrupt,
 				IRQF_SHARED, "goldfish_pipe", dev);
@@ -779,7 +779,7 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
 		return err;
 	}
 
-	err = misc_register(&goldfish_pipe_dev);
+	err = misc_register(&goldfish_pipe_miscdev);
 	if (err) {
 		dev_err(&pdev->dev, "unable to register v2 device\n");
 		return err;
@@ -828,18 +828,16 @@ static int goldfish_pipe_device_init(struct platform_device *pdev)
 
 static void goldfish_pipe_device_deinit(struct platform_device *pdev)
 {
-	struct goldfish_pipe_dev *dev = pipe_dev;
-
-	misc_deregister(&goldfish_pipe_dev);
-	kfree(dev->pipes);
-	free_page((unsigned long)dev->buffers);
+	misc_deregister(&goldfish_pipe_miscdev);
+	kfree(goldfish_pipe_dev.pipes);
+	free_page((unsigned long)goldfish_pipe_dev.buffers);
 }
 
 static int goldfish_pipe_probe(struct platform_device *pdev)
 {
 	int err;
 	struct resource *r;
-	struct goldfish_pipe_dev *dev = pipe_dev;
+	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
 
 	BUILD_BUG_ON(sizeof(struct goldfish_pipe_command) > PAGE_SIZE);
 
@@ -889,7 +887,7 @@ static int goldfish_pipe_probe(struct platform_device *pdev)
 
 static int goldfish_pipe_remove(struct platform_device *pdev)
 {
-	struct goldfish_pipe_dev *dev = pipe_dev;
+	struct goldfish_pipe_dev *dev = &goldfish_pipe_dev;
 	goldfish_pipe_device_deinit(pdev);
 	dev->base = NULL;
 	return 0;
-- 
2.18.0.865.gffc8e1a3cd6-goog

[PATCH 8/9] platform: goldfish: pipe: Replace pr_ with dev_ for logging

[rkir]

From: Roman Kiryanov <rkir@google.com>

dev_ is preferred if struct device is available.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 drivers/platform/goldfish/goldfish_pipe.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 6f3fc4b30fba..3f8e440d62ae 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -384,6 +384,7 @@ static ssize_t goldfish_pipe_read_write(struct file *filp,
 	int count = 0, ret = -EINVAL;
 	unsigned long address, address_end, last_page;
 	unsigned int last_page_size;
+	struct device *pdev_dev;
 
 	/* If the emulator already closed the pipe, no need to go further */
 	if (unlikely(test_bit(BIT_CLOSED_ON_HOST, &pipe->flags)))
@@ -401,6 +402,8 @@ static ssize_t goldfish_pipe_read_write(struct file *filp,
 	last_page = (address_end - 1) & PAGE_MASK;
 	last_page_size = ((address_end - 1) & ~PAGE_MASK) + 1;
 
+	pdev_dev = pipe->dev->pdev_dev;
+
 	while (address < address_end) {
 		s32 consumed_size;
 		int status;
@@ -433,7 +436,8 @@ static ssize_t goldfish_pipe_read_write(struct file *filp,
 			 * err.
 			 */
 			if (status != PIPE_ERROR_AGAIN)
-				pr_info_ratelimited("goldfish_pipe: backend error %d on %s\n",
+				dev_err_ratelimited(pdev_dev,
+					"goldfish_pipe: backend error %d on %s\n",
 					status, is_write ? "write" : "read");
 			break;
 		}
-- 
2.18.0.865.gffc8e1a3cd6-goog

Re: [PATCH 8/9] platform: goldfish: pipe: Replace pr_ with dev_ for logging

[Joe Perches]

On Mon, 2018-08-13 at 16:38 -0700, rkir@google.com wrote:
> dev_ is preferred if struct device is available.
[]
> diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
[]
> @@ -384,6 +384,7 @@ static ssize_t goldfish_pipe_read_write(struct file *filp,
>  	int count = 0, ret = -EINVAL;
>  	unsigned long address, address_end, last_page;
>  	unsigned int last_page_size;
> +	struct device *pdev_dev;
>  
>  	/* If the emulator already closed the pipe, no need to go further */
>  	if (unlikely(test_bit(BIT_CLOSED_ON_HOST, &pipe->flags)))
> @@ -401,6 +402,8 @@ static ssize_t goldfish_pipe_read_write(struct file *filp,
>  	last_page = (address_end - 1) & PAGE_MASK;
>  	last_page_size = ((address_end - 1) & ~PAGE_MASK) + 1;
>  
> +	pdev_dev = pipe->dev->pdev_dev;
> +
>  	while (address < address_end) {
>  		s32 consumed_size;
>  		int status;
> @@ -433,7 +436,8 @@ static ssize_t goldfish_pipe_read_write(struct file *filp,
>  			 * err.
>  			 */
>  			if (status != PIPE_ERROR_AGAIN)
> -				pr_info_ratelimited("goldfish_pipe: backend error %d on %s\n",
> +				dev_err_ratelimited(pdev_dev,
> +					"goldfish_pipe: backend error %d on %s\n",
>  					status, is_write ? "write" : "read");
>  			break;
>  		}

Wouldn't it be simpler to use pipe->dev->pdev_dev here instead
of creating and assigning a probably unused use-once pointer?

What does the output look like now?

Is the "pipe->dev->pdev_dev->name" then "goldfish_pipe: " output
useful?

Re: [PATCH 8/9] platform: goldfish: pipe: Replace pr_ with dev_ for logging

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3822 bytes --]

Hi Roman,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18 next-20180813]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/rkir-google-com/platform-goldfish-pipe-Fix-comments-to-fit-80-columns/20180814-104717
config: i386-randconfig-x008-201832 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/platform//goldfish/goldfish_pipe.c: In function 'goldfish_pipe_read_write':
>> drivers/platform//goldfish/goldfish_pipe.c:405:22: error: 'struct goldfish_pipe_dev' has no member named 'pdev_dev'
     pdev_dev = pipe->dev->pdev_dev;
                         ^~

vim +405 drivers/platform//goldfish/goldfish_pipe.c

   379	
   380	static ssize_t goldfish_pipe_read_write(struct file *filp,
   381		char __user *buffer, size_t bufflen, int is_write)
   382	{
   383		struct goldfish_pipe *pipe = filp->private_data;
   384		int count = 0, ret = -EINVAL;
   385		unsigned long address, address_end, last_page;
   386		unsigned int last_page_size;
   387		struct device *pdev_dev;
   388	
   389		/* If the emulator already closed the pipe, no need to go further */
   390		if (unlikely(test_bit(BIT_CLOSED_ON_HOST, &pipe->flags)))
   391			return -EIO;
   392		/* Null reads or writes succeeds */
   393		if (unlikely(bufflen == 0))
   394			return 0;
   395		/* Check the buffer range for access */
   396		if (unlikely(!access_ok(is_write ? VERIFY_WRITE : VERIFY_READ,
   397				buffer, bufflen)))
   398			return -EFAULT;
   399	
   400		address = (unsigned long)buffer;
   401		address_end = address + bufflen;
   402		last_page = (address_end - 1) & PAGE_MASK;
   403		last_page_size = ((address_end - 1) & ~PAGE_MASK) + 1;
   404	
 > 405		pdev_dev = pipe->dev->pdev_dev;
   406	
   407		while (address < address_end) {
   408			s32 consumed_size;
   409			int status;
   410	
   411			ret = transfer_max_buffers(pipe, address, address_end, is_write,
   412					last_page, last_page_size, &consumed_size,
   413					&status);
   414			if (ret < 0)
   415				break;
   416	
   417			if (consumed_size > 0) {
   418				/* No matter what's the status, we've transferred
   419				 * something.
   420				 */
   421				count += consumed_size;
   422				address += consumed_size;
   423			}
   424			if (status > 0)
   425				continue;
   426			if (status == 0) {
   427				/* EOF */
   428				ret = 0;
   429				break;
   430			}
   431			if (count > 0) {
   432				/*
   433				 * An error occurred, but we already transferred
   434				 * something on one of the previous iterations.
   435				 * Just return what we already copied and log this
   436				 * err.
   437				 */
   438				if (status != PIPE_ERROR_AGAIN)
   439					dev_err_ratelimited(pdev_dev,
   440						"goldfish_pipe: backend error %d on %s\n",
   441						status, is_write ? "write" : "read");
   442				break;
   443			}
   444	
   445			/*
   446			 * If the error is not PIPE_ERROR_AGAIN, or if we are in
   447			 * non-blocking mode, just return the error code.
   448			 */
   449			if (status != PIPE_ERROR_AGAIN ||
   450				(filp->f_flags & O_NONBLOCK) != 0) {
   451				ret = goldfish_pipe_error_convert(status);
   452				break;
   453			}
   454	
   455			status = wait_for_host_signal(pipe, is_write);
   456			if (status < 0)
   457				return status;
   458		}
   459	
   460		if (count > 0)
   461			return count;
   462		return ret;
   463	}
   464	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30796 bytes --]

Re: [PATCH 1/9] platform: goldfish: pipe: Fix comments to fit 80 columns

[Joe Perches]

On Mon, 2018-08-13 at 16:38 -0700, rkir@google.com wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> Some comment lines are longer than 80 symbols.
[]
> diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
[]
> @@ -84,7 +84,10 @@ enum PipePollFlags {
>  	PIPE_POLL_HUP	= 1 << 2
>  };
>  
> -/* Possible status values used to signal errors - see goldfish_pipe_error_convert */
> +/*
> + * Possible status values used to signal errors - see
> + * goldfish_pipe_error_convert

If this is to be wrapped at all, and this really doesn't
need to be wrapped, it would probably be more sensible as:

/*
 * Possible status values used to signal errors
 * see: goldfish_pipe_error_convert
 */