From: Roman Kiryanov <[email protected]>
Some comment lines are longer than 80 symbols.
Signed-off-by: Roman Kiryanov <[email protected]>
---
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
From: Roman Kiryanov <[email protected]>
goldfish_pipe is distributed under GPL v2.
Signed-off-by: Roman Kiryanov <[email protected]>
---
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 <[email protected]>");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
--
2.18.0.865.gffc8e1a3cd6-goog
From: Roman Kiryanov <[email protected]>
Function's opening brace has to be at the beginning of the next line.
Signed-off-by: Roman Kiryanov <[email protected]>
---
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
From: Roman Kiryanov <[email protected]>
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 <[email protected]>
---
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
From: Roman Kiryanov <[email protected]>
Provide an explanation why GFP_ATOMIC is needed to prevent changing it to
other values.
Signed-off-by: Roman Kiryanov <[email protected]>
---
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
From: Roman Kiryanov <[email protected]>
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 <[email protected]>
---
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
From: Roman Kiryanov <[email protected]>
There is no reason to have an array of 1.
Signed-off-by: Roman Kiryanov <[email protected]>
---
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
From: Roman Kiryanov <[email protected]>
dev_ is preferred if struct device is available.
Signed-off-by: Roman Kiryanov <[email protected]>
---
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
On Mon, 2018-08-13 at 16:38 -0700, [email protected] 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?
On Mon, 2018-08-13 at 16:38 -0700, [email protected] wrote:
> From: Roman Kiryanov <[email protected]>
>
> 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
*/
On Mon, 2018-08-13 at 16:38 -0700, [email protected] wrote:
> From: Roman Kiryanov <[email protected]>
>
> 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);
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
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 <[email protected]> wrote:
>
> On Mon, 2018-08-13 at 16:38 -0700, [email protected] wrote:
> > From: Roman Kiryanov <[email protected]>
> >
> > 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);
>
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