Re: [PATCH v2 2/3] staging: vchiq: revert "switch to wait_for_completion_killable"
Hi Dan, thanks for reviewing.
On Mon, 2019-05-06 at 18:20 +0300, Dan Carpenter wrote:
> On Mon, May 06, 2019 at 04:40:29PM +0200, Nicolas Saenz Julienne wrote:
> > @@ -1740,7 +1740,8 @@ parse_rx_slots(struct vchiq_state *state)
> > &service->bulk_rx : &service->bulk_tx;
> >
> > DEBUG_TRACE(PARSE_LINE);
> > - if (mutex_lock_killable(&service->bulk_mutex)) {
> > + if (mutex_lock_killable(
> > + &service->bulk_mutex) != 0) {
>
> This series does't add != 0 consistently... Personally, I would prefer
> we just leave it out. I use != 0 for two things. 1) When I'm talking
> about the number zero.
>
> if (len == 0) {
>
> Or with strcmp():
>
> if (strcmp(a, b) == 0) { // a equals b
> if (strcmp(a, b) < 0) { // a less than b.
>
> But here zero means no errors, so I would just leave it out...
I agree, I'll fix it.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
[PATCH v2 3/3] staging: vchiq: make wait events interruptible
The killable version of wait_event() is meant to be used on situations
where it should not fail at all costs, but still have the convenience of
being able to kill it if really necessary. Wait events in VCHIQ doesn't
fit this criteria, as it's mainly used as an interface to V4L2 and ALSA
devices.
Fixes: 852b2876a8a8 ("staging: vchiq: rework remove_event handling")
Signed-off-by: Nicolas Saenz Julienne
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 50189223f05b..9676f83dcf78 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -425,13 +425,21 @@ remote_event_create(wait_queue_head_t *wq, struct
remote_event *event)
init_waitqueue_head(wq);
}
+/*
+ * All the event waiting routines in VCHIQ used a custom semaphore
+ * implementation that filtered most signals. This achieved a behaviour similar
+ * to the "killable" family of functions. While cleaning up this code all the
+ * routines where switched to the "interruptible" family of functions, as the
+ * former was deemed unjustified and the use "killable" set all VCHIQ's
+ * threads in D state.
+ */
static inline int
remote_event_wait(wait_queue_head_t *wq, struct remote_event *event)
{
if (!event->fired) {
event->armed = 1;
dsb(sy);
- if (wait_event_killable(*wq, event->fired)) {
+ if (wait_event_interruptible(*wq, event->fired)) {
event->armed = 0;
return 0;
}
--
2.21.0
[PATCH v2 2/3] staging: vchiq: revert "switch to wait_for_completion_killable"
The killable version of wait_for_completion() is meant to be used on
situations where it should not fail at all costs, but still have the
convenience of being able to kill it if really necessary. VCHIQ doesn't
fit this criteria, as it's mainly used as an interface to V4L2 and ALSA
devices.
Fixes: a772f116702e ("staging: vchiq: switch to wait_for_completion_killable")
Signed-off-by: Nicolas Saenz Julienne
This reverts commit a772f116702e3f0afdd7e6acadc1b8fb3b20b9ff.
---
.../interface/vchiq_arm/vchiq_arm.c | 21 ++-
.../interface/vchiq_arm/vchiq_core.c | 21 ++-
.../interface/vchiq_arm/vchiq_util.c | 6 +++---
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 064d0db4c51e..ccfb8218b83c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -560,7 +560,8 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T
reason,
vchiq_log_trace(vchiq_arm_log_level,
"%s - completion queue full", __func__);
DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
- if (wait_for_completion_killable( &instance->remove_event)) {
+ if (wait_for_completion_interruptible(
+ &instance->remove_event)) {
vchiq_log_info(vchiq_arm_log_level,
"service_callback interrupted");
return VCHIQ_RETRY;
@@ -671,7 +672,7 @@ service_callback(VCHIQ_REASON_T reason, struct vchiq_header
*header,
}
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
- if (wait_for_completion_killable(
+ if (wait_for_completion_interruptible(
&user_service->remove_event)
!= 0) {
vchiq_log_info(vchiq_arm_log_level,
@@ -1006,7 +1007,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
has been closed until the client library calls the
CLOSE_DELIVERED ioctl, signalling close_event. */
if (user_service->close_pending &&
- wait_for_completion_killable(
+ wait_for_completion_interruptible(
&user_service->close_event))
status = VCHIQ_RETRY;
break;
@@ -1182,7 +1183,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
DEBUG_TRACE(AWAIT_COMPLETION_LINE);
mutex_unlock(&instance->completion_mutex);
- rc = wait_for_completion_killable(
+ rc = wait_for_completion_interruptible(
&instance->insert_event);
mutex_lock(&instance->completion_mutex);
if (rc != 0) {
@@ -1352,7 +1353,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
do {
spin_unlock(&msg_queue_spinlock);
DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
- if (wait_for_completion_killable(
+ if (wait_for_completion_interruptible(
&user_service->insert_event)) {
vchiq_log_info(vchiq_arm_log_level,
"DEQUEUE_MESSAGE interrupted");
@@ -2360,7 +2361,7 @@ vchiq_keepalive_thread_func(void *v)
while (1) {
long rc = 0, uc = 0;
- if (wait_for_completion_killable(&arm_state->ka_evt)
+ if (wait_for_completion_interruptible(&arm_state->ka_evt)
!= 0) {
vchiq_log_error(vchiq_susp_log_level,
"%s interrupted", __func__);
@@ -2611,7 +2612,7 @@ block_resume(struct vchiq_arm_state *arm_state)
write_unlock_bh(&arm_state->susp_res_lock);
vchiq_log_info(vchiq_susp_log_level, "%s wait for previously "
"blocked clients", __func__);
- if (wait_for_completion_killable_timeout(
+ if (wait_for_completion_interruptible_timeout(
&arm_state->blocked_blocker, timeout_val)
<= 0) {
vchiq_log_error(vchiq_susp_log_
[PATCH v2 1/3] staging: vchiq_2835_arm: revert "quit using custom down_interruptible()"
The killable version of down() is meant to be used on situations where
it should not fail at all costs, but still have the convenience of being
able to kill it if really necessary. VCHIQ doesn't fit this criteria, as
it's mainly used as an interface to V4L2 and ALSA devices.
Fixes: ff5979ad8636 ("staging: vchiq_2835_arm: quit using custom
down_interruptible()")
Signed-off-by: Nicolas Saenz Julienne
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index dd4898861b83..bfc064a5f884 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -541,7 +541,7 @@ create_pagelist(char __user *buf, size_t count, unsigned
short type)
(g_cache_line_size - 1 {
char *fragments;
- if (down_killable(&g_free_fragments_sema)) {
+ if (down_interruptible(&g_free_fragments_sema) != 0) {
cleanup_pagelistinfo(pagelistinfo);
return NULL;
}
--
2.21.0
[PATCH v2 0/3] staging: vchiq: use interruptible waits
Hi, this series tries to address an issue that came up in Raspbian's kernel tree [1] and upstream distros [2][3]. We adopted some changes that moved wait calls from a custom implementation to the more standard killable family of functions. Users complained that all the VCHIQ threads showed up in D state (which is the expected behaviour). The custom implementation we deleted tried to mimic the killable family of functions, yet accepted more signals than the later; SIGKILL | SIGINT | SIGQUIT | SIGTRAP | SIGSTOP | SIGCONT for the custom implementation as opposed to plain old SIGKILL. Raspbian maintainers decided roll back some of those changes and leave the wait functions as interruptible. Hence creating some divergence between both trees. One could argue that not liking having the threads stuck in D state is not really a software issue. It's more a cosmetic thing that can scare people when they look at "uptime". On the other hand, if we are ever to unstage this driver, we'd really need a proper justification for using the killable family of functions. Which I think it's not really clear at the moment. As Raspbian's kernel has been working for a while with interruptible waits I propose we follow through. If needed we can always go back to killable. But at least we'll have a proper understanding on the actual needs. In the end the driver is in staging, and the potential for errors small. Regards, Nicolas [1] https://github.com/raspberrypi/linux/issues/2881 [2] https://archlinuxarm.org/forum/viewtopic.php?f=65&t=13485 [3] https://lists.fedoraproject.org/archives/list/a...@lists.fedoraproject.org/message/GBXGJ7DOV5CQQXFPOZCXTRD6W4BEPT4Q/ -- Changes since v1: - Proplery format revert commits - Add code comment to remind of this issue - Add Fixes tags Nicolas Saenz Julienne (3): staging: vchiq_2835_arm: revert "quit using custom down_interruptible()" staging: vchiq: revert "switch to wait_for_completion_killable" staging: vchiq: make wait events interruptible .../interface/vchiq_arm/vchiq_2835_arm.c | 2 +- .../interface/vchiq_arm/vchiq_arm.c | 21 +++-- .../interface/vchiq_arm/vchiq_core.c | 31 --- .../interface/vchiq_arm/vchiq_util.c | 6 ++-- 4 files changed, 35 insertions(+), 25 deletions(-) -- 2.21.0
[PATCH 1/2] input: edt-ft5x06 - add polled input support
Some hardware configurations might pass on providing an interrupt line.
In that case there is always the option to use a polled input approach.
This patch adapts the driver for it.
The polled approach is only triggered if no interrupt is provided by the
firmware or platform data.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/input/touchscreen/edt-ft5x06.c | 100 ++---
1 file changed, 72 insertions(+), 28 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 702bfda7ee77..e58645c72c2f 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#include
#define WORK_REGISTER_THRESHOLD0x00
@@ -97,6 +98,7 @@ struct edt_reg_addr {
struct edt_ft5x06_ts_data {
struct i2c_client *client;
struct input_dev *input;
+ struct input_polled_dev *poll_dev;
struct touchscreen_properties prop;
u16 num_x;
u16 num_y;
@@ -181,9 +183,8 @@ static bool edt_ft5x06_ts_check_crc(struct
edt_ft5x06_ts_data *tsdata,
return true;
}
-static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
+static void edt_ft5x06_process(struct edt_ft5x06_ts_data *tsdata)
{
- struct edt_ft5x06_ts_data *tsdata = dev_id;
struct device *dev = &tsdata->client->dev;
u8 cmd;
u8 rdbuf[63];
@@ -210,7 +211,7 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
break;
default:
- goto out;
+ return;
}
memset(rdbuf, 0, sizeof(rdbuf));
@@ -222,7 +223,7 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
if (error) {
dev_err_ratelimited(dev, "Unable to fetch data, error: %d\n",
error);
- goto out;
+ return;
}
/* M09/M12 does not send header or CRC */
@@ -232,11 +233,11 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void
*dev_id)
dev_err_ratelimited(dev,
"Unexpected header: %02x%02x%02x!\n",
rdbuf[0], rdbuf[1], rdbuf[2]);
- goto out;
+ return;
}
if (!edt_ft5x06_ts_check_crc(tsdata, rdbuf, datalen))
- goto out;
+ return;
}
for (i = 0; i < tsdata->max_support_points; i++) {
@@ -273,11 +274,23 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void
*dev_id)
input_mt_report_pointer_emulation(tsdata->input, true);
input_sync(tsdata->input);
+}
-out:
+static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
+{
+ struct edt_ft5x06_ts_data *tsdata = dev_id;
+
+ edt_ft5x06_process(tsdata);
return IRQ_HANDLED;
}
+static void edt_ft5x06_poll(struct input_polled_dev *dev)
+{
+ struct edt_ft5x06_ts_data *tsdata = dev->private;
+
+ edt_ft5x06_process(tsdata);
+}
+
static int edt_ft5x06_register_write(struct edt_ft5x06_ts_data *tsdata,
u8 addr, u8 value)
{
@@ -1059,7 +1072,9 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
const struct edt_i2c_chip_data *chip_data;
+ struct input_polled_dev *poll_dev = NULL;
struct edt_ft5x06_ts_data *tsdata;
+ bool polled = !(client->irq);
struct input_dev *input;
unsigned long irq_flags;
int error;
@@ -1112,15 +1127,38 @@ static int edt_ft5x06_ts_probe(struct i2c_client
*client,
msleep(300);
}
- input = devm_input_allocate_device(&client->dev);
- if (!input) {
- dev_err(&client->dev, "failed to allocate input device.\n");
- return -ENOMEM;
+ if (polled) {
+ poll_dev = devm_input_allocate_polled_device(&client->dev);
+ if (!poll_dev) {
+ dev_err(&client->dev,
+ "failed to allocate polled input device.\n");
+ return -ENOMEM;
+ }
+
+ poll_dev->poll = edt_ft5x06_poll;
+ poll_dev->private = tsdata;
+
+ tsdata->poll_dev = poll_dev;
+ tsdata->input = poll_dev->input;
+
+ input = poll_dev->input;
+
+ device_property_read_u32(&client->dev, "poll-interval",
+&poll_dev->poll_interval);
+
+ } else {
+ input = devm_input_allocate_device(&client->dev);
+ if (!input) {
+
[PATCH 2/2] Input: edt-ft5x06 - add support for polled configuration
Some devices might not provide an interrupt line for the touchscreen. In that case the driver defaults to using a polled interface. Signed-off-by: Nicolas Saenz Julienne --- .../devicetree/bindings/input/touchscreen/edt-ft5x06.txt | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt index 870b8c5cce9b..2605994a1257 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt @@ -24,10 +24,14 @@ Required properties: or: "focaltech,ft6236" - reg: I2C slave address of the chip (0x38) - - interrupts: interrupt specification for the touchdetect - interrupt Optional properties: +- interrupts: interrupt specification for the touchdetect interrupt, if not + supplied the driver will deafult to polling. + +- poll-interval: Poll interval time in milliseconds, only relevant if no +interrupt was provided. + - reset-gpios: GPIO specification for the RESET input - wake-gpios: GPIO specification for the WAKE input -- 2.21.0
Re: [PATCH] fpga: stratix10-soc: fix use-after-free on s10_init()
Hi Thanks,
On Wed, 2019-04-24 at 07:32 +0800, Wen Yang wrote:
> The refcount of fw_np has already been decreased by of_find_matching_node()
> so it shouldn't be used anymore.
> This patch adds an of_node_get() before of_find_matching_node() to avoid
> the use-after-free problem.
>
> Fixes: e7eef1d7633a ("fpga: add intel stratix10 soc fpga manager driver")
> Signed-off-by: Wen Yang
> Cc: Alan Tull
> Cc: Moritz Fischer
> Cc: Nicolas Saenz Julienne
> Cc: linux-f...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/fpga/stratix10-soc.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> index 13851b3..215d337 100644
> --- a/drivers/fpga/stratix10-soc.c
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -507,12 +507,16 @@ static int __init s10_init(void)
> if (!fw_np)
> return -ENODEV;
>
> + of_node_get(fw_np);
> np = of_find_matching_node(fw_np, s10_of_match);
> - if (!np)
> + if (!np) {
> + of_node_put(fw_np);
> return -ENODEV;
> + }
>
> of_node_put(np);
> ret = of_platform_populate(fw_np, s10_of_match, NULL, NULL);
> + of_node_put(fw_np);
> if (ret)
> return ret;
>
Reviewed-by: Nicolas Saenz Julienne
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 3/3] firmware: stratix10-svc: fix leaked of_node references
On Wed, 2019-04-17 at 10:44 +0800, Wen Yang wrote:
> In stratix10_svc_init function, fw_np is obtained by calling
> of_find_node_by_name(), np is obtained by calling
> of_find_matching_node(), and the reference counts of those
> two device_nodes, fw_np and np, are increased.
> But when the function exits, only of_node_put is called on np,
> and fw_np's reference count is leaked.
>
> Detected by coccinelle with the following warnings:
> ./drivers/firmware/stratix10-svc.c:1020:2-8: ERROR: missing of_node_put;
> acquired a node pointer with refcount incremented on line 1014, but without a
> corresponding object release within this function.
> ./drivers/firmware/stratix10-svc.c:1025:2-8: ERROR: missing of_node_put;
> acquired a node pointer with refcount incremented on line 1014, but without a
> corresponding object release within this function.
> ./drivers/firmware/stratix10-svc.c:1027:1-7: ERROR: missing of_node_put;
> acquired a node pointer with refcount incremented on line 1014, but without a
> corresponding object release within this function.
>
> Signed-off-by: Wen Yang
> Cc: Greg Kroah-Hartman
> Cc: Alan Tull
> Cc: Richard Gong
> Cc: Nicolas Saenz Julienne
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/firmware/stratix10-svc.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-
> svc.c
> index 6e65148..482a6bd 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1016,15 +1016,21 @@ static int __init stratix10_svc_init(void)
> return -ENODEV;
>
> np = of_find_matching_node(fw_np, stratix10_svc_drv_match);
Sorry but this patch isn't right, of_find_matching_node() will free the
reference to fw_np internally.
> - if (!np)
> - return -ENODEV;
> + if (!np) {
> + ret = -ENODEV;
> + goto out_put_fw_np;
> + }
>
> of_node_put(np);
> ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);
> if (ret)
> - return ret;
> + goto out_put_fw_np;
Consequently and assuming I'm not missing something, I think fw_np shouldn't be
used here as is.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/3] Revert "staging: vchiq_2835_arm: quit using custom down_interruptible()"
On Fri, 2019-04-05 at 15:02 +0300, Dan Carpenter wrote:
> On Fri, Apr 05, 2019 at 01:34:20PM +0200, Nicolas Saenz Julienne wrote:
> > This reverts commit ff5979ad86368425b7da3a25f4e84650b51ff5fd.
> > ---
>
> Git kind of sets you up for failure with reverts...
>
> Fix the subject, add a commit message and a Signed off by etc. We need
> all the regular stuff. See commit 75ae193626de ("dm: revert
> 8f50e358153d ("dm: limit the max bio size as BIO_MAX_PAGES *
> PAGE_SIZE")") for an example of how to do it nicely.
>
Will do, Thanks!
> regards,
> dan carpenter
>
signature.asc
Description: This is a digitally signed message part
[PATCH 3/3] staging: vchiq: make wait events interruptible
The killable version of wait_event() is meant to be used on situations
where it's not meant to fail at all costs, but still have the convenience
of being able to kill it if really necessary. For instance it's used
while waiting on an page write on some file systems.
Wait events in VCHIQ don't fit this criteria, as it's mainly used as an
interface to V4L2 and ALSA devices.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 50189223f05b..41257a1df49d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -431,7 +431,7 @@ remote_event_wait(wait_queue_head_t *wq, struct
remote_event *event)
if (!event->fired) {
event->armed = 1;
dsb(sy);
- if (wait_event_killable(*wq, event->fired)) {
+ if (wait_event_interruptible(*wq, event->fired)) {
event->armed = 0;
return 0;
}
--
2.21.0
[PATCH 0/3] staging: vchiq: use interruptible waits
Hi, this series tries to address an issue that came up in Raspbian's kernel tree [1]. After pulling from upstream some changes that moved wait calls from a custom implementation to the more standard killable family some users complained that all the VCHIQ threads showed up in D state (which is the expected behaviour). The custom implementation we deleted tried to mimic the killable family of functions, yet accepted more signals than the later. SIGKILL | SIGINT | SIGQUIT | SIGTRAP | SIGSTOP | SIGCONT for the custom implementation as opposed to plain old SIGKILL. Raspbian maintainers decided roll back some of those changes and leave the wait functions as interruptible. Hence creating some divergence between both trees. One could argue that not liking having the threads stuck in D state is not really a software issue. It's more a cosmetic thing that can scare people when they look at "uptime". On the other hand, if we are ever to unstage this driver, we'd really need a proper justification for using the killable family of functions. Which I think it's not really clear at the moment. As Raspbian's kernel has been working for a while with interruptible waits I propose we follow through. If needed we can always go back to killable. But at least we'll have a proper understanding on the actual needs. In the end the driver is in staging, and the potential for errors small. Regards, Nicolas [1] https://github.com/raspberrypi/linux/issues/2881 --- Nicolas Saenz Julienne (3): Revert "staging: vchiq_2835_arm: quit using custom down_interruptible()" Revert "staging: vchiq: switch to wait_for_completion_killable" staging: vchiq: make wait events interruptible .../interface/vchiq_arm/vchiq_2835_arm.c | 2 +- .../interface/vchiq_arm/vchiq_arm.c | 21 + .../interface/vchiq_arm/vchiq_core.c | 23 ++- .../interface/vchiq_arm/vchiq_util.c | 6 ++--- 4 files changed, 27 insertions(+), 25 deletions(-) -- 2.21.0
[PATCH 1/3] Revert "staging: vchiq_2835_arm: quit using custom down_interruptible()"
This reverts commit ff5979ad86368425b7da3a25f4e84650b51ff5fd.
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index dd4898861b83..bfc064a5f884 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -541,7 +541,7 @@ create_pagelist(char __user *buf, size_t count, unsigned
short type)
(g_cache_line_size - 1 {
char *fragments;
- if (down_killable(&g_free_fragments_sema)) {
+ if (down_interruptible(&g_free_fragments_sema) != 0) {
cleanup_pagelistinfo(pagelistinfo);
return NULL;
}
--
2.21.0
[PATCH 2/3] Revert "staging: vchiq: switch to wait_for_completion_killable"
This reverts commit a772f116702e3f0afdd7e6acadc1b8fb3b20b9ff.
---
.../interface/vchiq_arm/vchiq_arm.c | 21 ++-
.../interface/vchiq_arm/vchiq_core.c | 21 ++-
.../interface/vchiq_arm/vchiq_util.c | 6 +++---
3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 064d0db4c51e..ccfb8218b83c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -560,7 +560,8 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T
reason,
vchiq_log_trace(vchiq_arm_log_level,
"%s - completion queue full", __func__);
DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
- if (wait_for_completion_killable( &instance->remove_event)) {
+ if (wait_for_completion_interruptible(
+ &instance->remove_event)) {
vchiq_log_info(vchiq_arm_log_level,
"service_callback interrupted");
return VCHIQ_RETRY;
@@ -671,7 +672,7 @@ service_callback(VCHIQ_REASON_T reason, struct vchiq_header
*header,
}
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
- if (wait_for_completion_killable(
+ if (wait_for_completion_interruptible(
&user_service->remove_event)
!= 0) {
vchiq_log_info(vchiq_arm_log_level,
@@ -1006,7 +1007,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
has been closed until the client library calls the
CLOSE_DELIVERED ioctl, signalling close_event. */
if (user_service->close_pending &&
- wait_for_completion_killable(
+ wait_for_completion_interruptible(
&user_service->close_event))
status = VCHIQ_RETRY;
break;
@@ -1182,7 +1183,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
DEBUG_TRACE(AWAIT_COMPLETION_LINE);
mutex_unlock(&instance->completion_mutex);
- rc = wait_for_completion_killable(
+ rc = wait_for_completion_interruptible(
&instance->insert_event);
mutex_lock(&instance->completion_mutex);
if (rc != 0) {
@@ -1352,7 +1353,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
do {
spin_unlock(&msg_queue_spinlock);
DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
- if (wait_for_completion_killable(
+ if (wait_for_completion_interruptible(
&user_service->insert_event)) {
vchiq_log_info(vchiq_arm_log_level,
"DEQUEUE_MESSAGE interrupted");
@@ -2360,7 +2361,7 @@ vchiq_keepalive_thread_func(void *v)
while (1) {
long rc = 0, uc = 0;
- if (wait_for_completion_killable(&arm_state->ka_evt)
+ if (wait_for_completion_interruptible(&arm_state->ka_evt)
!= 0) {
vchiq_log_error(vchiq_susp_log_level,
"%s interrupted", __func__);
@@ -2611,7 +2612,7 @@ block_resume(struct vchiq_arm_state *arm_state)
write_unlock_bh(&arm_state->susp_res_lock);
vchiq_log_info(vchiq_susp_log_level, "%s wait for previously "
"blocked clients", __func__);
- if (wait_for_completion_killable_timeout(
+ if (wait_for_completion_interruptible_timeout(
&arm_state->blocked_blocker, timeout_val)
<= 0) {
vchiq_log_error(vchiq_susp_log_level, "%s wait for "
@@ -2637,7 +2638,7 @@ block_resume(struct vchiq_arm_state *arm_state)
write_unlock_bh(&arm_state->susp_res_lock);
vchiq_log_info(vchiq_susp_log_level, "%s wait for resume",
__func__);
- if (wait_for_completion_killable_timeout(
+ if (wait_for_completion_interruptible_timeout(
&arm_state->vc_resume_complete, timeout_val)
<= 0) {
vchiq_log_error(vchiq_susp_log_level, "%s wait for "
@@ -2844,7 +
[PATCH v4] HID: core: move Usage Page concatenation to Main item
As seen on some USB wireless keyboards manufactured by Primax, the HID
parser was using some assumptions that are not always true. In this case
it's s the fact that, inside the scope of a main item, an Usage Page
will always precede an Usage.
The spec is not pretty clear as 6.2.2.7 states "Any usage that follows
is interpreted as a Usage ID and concatenated with the Usage Page".
While 6.2.2.8 states "When the parser encounters a main item it
concatenates the last declared Usage Page with a Usage to form a
complete usage value." Being somewhat contradictory it was decided to
match Window's implementation, which follows 6.2.2.8.
In summary, the patch moves the Usage Page concatenation from the local
item parsing function to the main item parsing function.
Signed-off-by: Nicolas Saenz Julienne
---
v3->v4: - Use u8 instead of __u8
- Remove overly complex usage size calculation
v2->v3: - Update patch title
v1->v2: - Add usage concatenation to hid_scan_main()
- Rework tests in hid-tools, making sure no-one is failing
drivers/hid/hid-core.c | 36
include/linux/hid.h| 1 +
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9993b692598f..852bbd303d9a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct hid_parser
*parser, unsigned type)
* Add a usage to the temporary parser table.
*/
-static int hid_add_usage(struct hid_parser *parser, unsigned usage)
+static int hid_add_usage(struct hid_parser *parser, unsigned usage, u8 size)
{
if (parser->local.usage_index >= HID_MAX_USAGES) {
hid_err(parser->device, "usage index exceeded\n");
return -1;
}
parser->local.usage[parser->local.usage_index] = usage;
+ parser->local.usage_size[parser->local.usage_index] = size;
parser->local.collection_index[parser->local.usage_index] =
parser->collection_stack_ptr ?
parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
@@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
- return hid_add_usage(parser, data);
+ return hid_add_usage(parser, data, item->size);
case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
@@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
parser->local.usage_minimum = data;
return 0;
@@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
count = data - parser->local.usage_minimum;
if (count + parser->local.usage_index >= HID_MAX_USAGES) {
/*
@@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
}
for (n = parser->local.usage_minimum; n <= data; n++)
- if (hid_add_usage(parser, n)) {
+ if (hid_add_usage(parser, n, item->size)) {
dbg_hid("hid_add_usage failed\n");
return -1;
}
@@ -547,6 +539,22 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
+/*
+ * Concatenate Usage Pages into Usages where relevant:
+ * As per specification, 6.2.2.8: "When the parser encounters a main item it
+ * concatenates the last declared Usage Page with a Usage to form a complete
+ * usage value."
+ */
+
+static void hid_concatenate_usage_page(struct hid_parser *parser)
+{
+ int i;
+
+ for (i = 0; i < parser->local.usage_index; i++)
+ if (parser->local.usage_size[i] <= 2)
+ parser->local.usage[i] += parser->global.usage_page <<
16;
+}
+
/*
* Process a main item.
*/
@@ -556,6 +564,8 @@ static int hid_parser_main(struct hid_parser *parser,
struct hid_item *item)
__u32 data;
int ret;
+ hid_concatenate_usage_page(parser);
+
data = item_udata(item);
switch (item->tag) {
@@ -765,6 +775,8 @@ static int hid_scan_main(struct hi
Re: [PATCH v3] HID: core: move Usage Page concatenation to Main item
Hi Oliver, thanks for the review! On Wed, 2019-03-27 at 10:35 +0100, Oliver Neukum wrote: > On Di, 2019-03-26 at 21:03 +0100, Nicolas Saenz Julienne wrote: > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct > > hid_parser *parser, unsigned type) > > * Add a usage to the temporary parser table. > > */ > > > > -static int hid_add_usage(struct hid_parser *parser, unsigned usage) > > +static int hid_add_usage(struct hid_parser *parser, unsigned usage, __u8 > > size) > > No need to use the __u8 style inside the kernel. u8 will do. Noted, fixed for v4. > > Regards > Oliver > signature.asc Description: This is a digitally signed message part
Re: [PATCH v3] HID: core: move Usage Page concatenation to Main item
Hi Terry, thanks for the review!
On Tue, 2019-03-26 at 22:43 +, Junge, Terry wrote:
> Hi Nicolas,
>
> This patch looks good except for one comment/question below.
>
> Thanks,
> Terry
>
> On Tuesday, March 26, 2019 1:04 PM Nicolas Saenz Julienne <
> nsaenzjulie...@suse.de> wrote:
> > As seen on some USB wireless keyboards manufactured by Primax, the HID
> > parser was using some assumptions that are not always true. In this case
> > it's s
> > the fact that, inside the scope of a main item, an Usage Page will always
> > precede an Usage.
> >
> > The spec is not pretty clear as 6.2.2.7 states "Any usage that follows is
> > interpreted as a Usage ID and concatenated with the Usage Page".
> > While 6.2.2.8 states "When the parser encounters a main item it concatenates
> > the last declared Usage Page with a Usage to form a complete usage value."
> > Being somewhat contradictory it was decided to match Window's
> > implementation, which follows 6.2.2.8.
> >
> > In summary, the patch moves the Usage Page concatenation from the local
> > item parsing function to the main item parsing function.
> >
> > Signed-off-by: Nicolas Saenz Julienne
> > ---
> >
> > v2->v3: - Update patch title
> >
> > v1->v2: - Add usage concatenation to hid_scan_main()
> > - Rework tests in hid-tools, making sure no-one is failing
> >
> > drivers/hid/hid-core.c | 40
> > include/linux/hid.h| 1 +
> > 2 files changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index
> > 9993b692598f..40c836ce3248 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct
> > hid_parser *parser, unsigned type)
> > * Add a usage to the temporary parser table.
> > */
> >
> > -static int hid_add_usage(struct hid_parser *parser, unsigned usage)
> > +static int hid_add_usage(struct hid_parser *parser, unsigned usage,
> > +__u8 size)
> > {
> > if (parser->local.usage_index >= HID_MAX_USAGES) {
> > hid_err(parser->device, "usage index exceeded\n");
> > return -1;
> > }
> > parser->local.usage[parser->local.usage_index] = usage;
> > + parser->local.usage_size[parser->local.usage_index] = size;
> > parser->local.collection_index[parser->local.usage_index] =
> > parser->collection_stack_ptr ?
> > parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
> > @@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > - return hid_add_usage(parser, data);
> > + return hid_add_usage(parser, data, item->size);
> >
> > case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
> >
> > @@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > parser->local.usage_minimum = data;
> > return 0;
> >
> > @@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > return 0;
> > }
> >
> > - if (item->size <= 2)
> > - data = (parser->global.usage_page << 16) + data;
> > -
> > count = data - parser->local.usage_minimum;
> > if (count + parser->local.usage_index >= HID_MAX_USAGES) {
> > /*
> > @@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
> > struct hid_item *item)
> > }
> >
> > for (n = parser->local.usage_minimum; n <= data; n++)
> > - if (hid_add_usage(parser, n)) {
> > + if (hid_add_usage(parser, n, item->size)) {
> > dbg_hid("hid_add_usage failed\n");
> > return -1;
> > }
>
[PATCH v3] HID: core: move Usage Page concatenation to Main item
As seen on some USB wireless keyboards manufactured by Primax, the HID
parser was using some assumptions that are not always true. In this case
it's s the fact that, inside the scope of a main item, an Usage Page
will always precede an Usage.
The spec is not pretty clear as 6.2.2.7 states "Any usage that follows
is interpreted as a Usage ID and concatenated with the Usage Page".
While 6.2.2.8 states "When the parser encounters a main item it
concatenates the last declared Usage Page with a Usage to form a
complete usage value." Being somewhat contradictory it was decided to
match Window's implementation, which follows 6.2.2.8.
In summary, the patch moves the Usage Page concatenation from the local
item parsing function to the main item parsing function.
Signed-off-by: Nicolas Saenz Julienne
---
v2->v3: - Update patch title
v1->v2: - Add usage concatenation to hid_scan_main()
- Rework tests in hid-tools, making sure no-one is failing
drivers/hid/hid-core.c | 40
include/linux/hid.h| 1 +
2 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9993b692598f..40c836ce3248 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct hid_parser
*parser, unsigned type)
* Add a usage to the temporary parser table.
*/
-static int hid_add_usage(struct hid_parser *parser, unsigned usage)
+static int hid_add_usage(struct hid_parser *parser, unsigned usage, __u8 size)
{
if (parser->local.usage_index >= HID_MAX_USAGES) {
hid_err(parser->device, "usage index exceeded\n");
return -1;
}
parser->local.usage[parser->local.usage_index] = usage;
+ parser->local.usage_size[parser->local.usage_index] = size;
parser->local.collection_index[parser->local.usage_index] =
parser->collection_stack_ptr ?
parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
@@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
- return hid_add_usage(parser, data);
+ return hid_add_usage(parser, data, item->size);
case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
@@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
parser->local.usage_minimum = data;
return 0;
@@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
count = data - parser->local.usage_minimum;
if (count + parser->local.usage_index >= HID_MAX_USAGES) {
/*
@@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
}
for (n = parser->local.usage_minimum; n <= data; n++)
- if (hid_add_usage(parser, n)) {
+ if (hid_add_usage(parser, n, item->size)) {
dbg_hid("hid_add_usage failed\n");
return -1;
}
@@ -547,6 +539,26 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
+/*
+ * Concatenate Usage Pages into Usages where relevant:
+ * As per specification, 6.2.2.8: "When the parser encounters a main item it
+ * concatenates the last declared Usage Page with a Usage to form a complete
+ * usage value."
+ */
+
+static void hid_concatenate_usage_page(struct hid_parser *parser)
+{
+ unsigned usages;
+ int i;
+
+ usages = max_t(unsigned, parser->local.usage_index,
+parser->global.report_count);
+
+ for (i = 0; i < usages; i++)
+ if (parser->local.usage_size[i] <= 2)
+ parser->local.usage[i] += parser->global.usage_page <<
16;
+}
+
/*
* Process a main item.
*/
@@ -556,6 +568,8 @@ static int hid_parser_main(struct hid_parser *parser,
struct hid_item *item)
__u32 data;
int ret;
+ hid_concatenate_usage_page(parser);
+
data = item_udata(item);
switch (item->tag) {
@@
[PATCH v2] HID: core: move Usage Page concatenation to hid_parser_main()
As seen on some USB wireless keyboards manufactured by Primax, the HID
parser was using some assumptions that are not always true. In this case
it's s the fact that, inside the scope of a main item, an Usage Page
will always precede an Usage.
The spec is not pretty clear as 6.2.2.7 states "Any usage that follows
is interpreted as a Usage ID and concatenated with the Usage Page".
While 6.2.2.8 states "When the parser encounters a main item it
concatenates the last declared Usage Page with a Usage to form a
complete usage value." Being somewhat contradictory it was decided to
match Window's implementation, which follows 6.2.2.8.
In summary, the patch moves the Usage Page concatenation from the local
item parsing function to the main item parsing function.
Signed-off-by: Nicolas Saenz Julienne
---
v1->v2: - Add usage concatenation to hid_scan_main()
- Rework tests in hid-tools, making sure no-one is failing
drivers/hid/hid-core.c | 40
include/linux/hid.h| 1 +
2 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9993b692598f..40c836ce3248 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct hid_parser
*parser, unsigned type)
* Add a usage to the temporary parser table.
*/
-static int hid_add_usage(struct hid_parser *parser, unsigned usage)
+static int hid_add_usage(struct hid_parser *parser, unsigned usage, __u8 size)
{
if (parser->local.usage_index >= HID_MAX_USAGES) {
hid_err(parser->device, "usage index exceeded\n");
return -1;
}
parser->local.usage[parser->local.usage_index] = usage;
+ parser->local.usage_size[parser->local.usage_index] = size;
parser->local.collection_index[parser->local.usage_index] =
parser->collection_stack_ptr ?
parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
@@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
- return hid_add_usage(parser, data);
+ return hid_add_usage(parser, data, item->size);
case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
@@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
parser->local.usage_minimum = data;
return 0;
@@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
count = data - parser->local.usage_minimum;
if (count + parser->local.usage_index >= HID_MAX_USAGES) {
/*
@@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
}
for (n = parser->local.usage_minimum; n <= data; n++)
- if (hid_add_usage(parser, n)) {
+ if (hid_add_usage(parser, n, item->size)) {
dbg_hid("hid_add_usage failed\n");
return -1;
}
@@ -547,6 +539,26 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
+/*
+ * Concatenate Usage Pages into Usages where relevant:
+ * As per specification, 6.2.2.8: "When the parser encounters a main item it
+ * concatenates the last declared Usage Page with a Usage to form a complete
+ * usage value."
+ */
+
+static void hid_concatenate_usage_page(struct hid_parser *parser)
+{
+ unsigned usages;
+ int i;
+
+ usages = max_t(unsigned, parser->local.usage_index,
+parser->global.report_count);
+
+ for (i = 0; i < usages; i++)
+ if (parser->local.usage_size[i] <= 2)
+ parser->local.usage[i] += parser->global.usage_page <<
16;
+}
+
/*
* Process a main item.
*/
@@ -556,6 +568,8 @@ static int hid_parser_main(struct hid_parser *parser,
struct hid_item *item)
__u32 data;
int ret;
+ hid_concatenate_usage_page(parser);
+
data = item_udata(item);
switch (item->tag) {
@@ -765,6 +779,8 @@ static int hid_s
Re: [PATCH] HID: core: move Usage Page concatenation to hid_parser_main()
Hi Benjamin, Thanks for the review!
On Mon, 2019-03-25 at 16:08 +0100, Benjamin Tissoires wrote:
> On Mon, Mar 25, 2019 at 11:39 AM Benjamin Tissoires
> wrote:
> > Hi Nicolas,
> >
> > On Tue, Mar 12, 2019 at 10:37 AM Nicolas Saenz Julienne
> > wrote:
> > > As seen on some USB wireless keyboards manufactured by Primax, the HID
> > > parser was using some assumptions that are not always true. In this case
> > > it's s the fact that, inside the scope of a main item, an Usage Page
> > > will always precede an Usage.
> > >
> > > The spec is not pretty clear as 6.2.2.7 states "Any usage that follows
> > > is interpreted as a Usage ID and concatenated with the Usage Page".
> > > While 6.2.2.8 states "When the parser encounters a main item it
> > > concatenates the last declared Usage Page with a Usage to form a
> > > complete usage value." Being somewhat contradictory it was decided to
> > > match Window's implementation, which follows 6.2.2.8.
> > >
> > > In summary, the patch moves the Usage Page concatenation from the local
> > > item parsing function to the main item parsing function.
> > >
> > > Signed-off-by: Nicolas Saenz Julienne
> > > ---
> >
> > Patch looks good to me.
> >
> > Terry, did you have time to review it?
> >
> > Cheers,
> > Benjamin
> >
> > > Note: A PR in hid-tools shoud show up anytime soon
> > >
> > > drivers/hid/hid-core.c | 30 ++
> > > include/linux/hid.h| 1 +
> > > 2 files changed, 19 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 9993b692598f..158468ef23a6 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct
> > > hid_parser *parser, unsigned type)
> > > * Add a usage to the temporary parser table.
> > > */
> > >
> > > -static int hid_add_usage(struct hid_parser *parser, unsigned usage)
> > > +static int hid_add_usage(struct hid_parser *parser, unsigned usage, __u8
> > > size)
> > > {
> > > if (parser->local.usage_index >= HID_MAX_USAGES) {
> > > hid_err(parser->device, "usage index exceeded\n");
> > > return -1;
> > > }
> > > parser->local.usage[parser->local.usage_index] = usage;
> > > + parser->local.usage_size[parser->local.usage_index] = size;
> > > parser->local.collection_index[parser->local.usage_index] =
> > > parser->collection_stack_ptr ?
> > > parser->collection_stack[parser->collection_stack_ptr - 1]
> > > : 0;
> > > @@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser
> > > *parser, struct hid_item *item)
> > > return 0;
> > > }
> > >
> > > - if (item->size <= 2)
> > > - data = (parser->global.usage_page << 16) + data;
> > > -
> > > - return hid_add_usage(parser, data);
> > > + return hid_add_usage(parser, data, item->size);
> > >
> > > case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
> > >
> > > @@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
> > > struct hid_item *item)
> > > return 0;
> > > }
> > >
> > > - if (item->size <= 2)
> > > - data = (parser->global.usage_page << 16) + data;
> > > -
> > > parser->local.usage_minimum = data;
> > > return 0;
> > >
> > > @@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
> > > struct hid_item *item)
> > > return 0;
> > > }
> > >
> > > - if (item->size <= 2)
> > > - data = (parser->global.usage_page << 16) + data;
> > > -
> > > count = data - parser->local.usage_minimum;
> > > if (count + parser->local.usage_index >= HID_MAX_USAGES) {
> > > /*
> > > @@ -533,7 +
Re: [RFC] xhci: clear port_remote_wakeup on device disconnection
Hi Oliver, thanks for the review!
On Tue, 2019-03-19 at 12:01 +0100, Oliver Neukum wrote:
> On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote:
> > This patch clears "port_remote_wakeup" upon detecting a device
> > disconnection. Making sure the above mentioned situation doesn't trigger
> > a PM busyloop.
>
> Hi,
>
> that is an interesting race condition.
>
> > Signed-off-by: Nicolas Saenz Julienne
> > ---
> > drivers/usb/host/xhci-hub.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> > index e2eece693655..bea853f45aec 100644
> > --- a/drivers/usb/host/xhci-hub.c
> > +++ b/drivers/usb/host/xhci-hub.c
> > @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port
> > *port, u32 *status,
> > bus_state->suspended_ports &= ~(1 << portnum);
> > }
> >
> > + if (!(portsc & PORT_CONNECT))
> > + bus_state->port_remote_wakeup &= ~(1 << portnum);
> > +
> > xhci_hub_report_usb3_link_state(xhci, status, portsc);
> > xhci_del_comp_mod_timer(xhci, portsc, portnum);
> > }
>
> Why are you putting that logic into xhci_get_usb3_port_status()?
> It looks to me like there is already something related in the caller
>
> /* USB2 and USB3 specific bits, including Port Link State */
> if (hcd->speed >= HCD_USB3)
> xhci_get_usb3_port_status(port, &status, raw_port_status);
> else
> xhci_get_usb2_port_status(port, &status, raw_port_status,
> flags);
> /*
> * Clear stale usb2 resume signalling variables in case port changed
> * state during resume signalling. For example on error
> */
> if ((bus_state->resume_done[wIndex] ||
> test_bit(wIndex, &bus_state->resuming_ports)) &&
> (raw_port_status & PORT_PLS_MASK) != XDEV_U3 &&
> (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME) {
> bus_state->resume_done[wIndex] = 0;
> clear_bit(wIndex, &bus_state->resuming_ports);
> usb_hcd_end_port_resume(&hcd->self, wIndex);
> }
You're right, xhci_get_usb3_port_status() is not the ideal spot I'll move the
code there.
>
> Otherwise very good catch!
>
> Regards
> Oliver
>
signature.asc
Description: This is a digitally signed message part
[PATCH] HID: core: move Usage Page concatenation to hid_parser_main()
As seen on some USB wireless keyboards manufactured by Primax, the HID
parser was using some assumptions that are not always true. In this case
it's s the fact that, inside the scope of a main item, an Usage Page
will always precede an Usage.
The spec is not pretty clear as 6.2.2.7 states "Any usage that follows
is interpreted as a Usage ID and concatenated with the Usage Page".
While 6.2.2.8 states "When the parser encounters a main item it
concatenates the last declared Usage Page with a Usage to form a
complete usage value." Being somewhat contradictory it was decided to
match Window's implementation, which follows 6.2.2.8.
In summary, the patch moves the Usage Page concatenation from the local
item parsing function to the main item parsing function.
Signed-off-by: Nicolas Saenz Julienne
---
Note: A PR in hid-tools shoud show up anytime soon
drivers/hid/hid-core.c | 30 ++
include/linux/hid.h| 1 +
2 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9993b692598f..158468ef23a6 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -218,13 +218,14 @@ static unsigned hid_lookup_collection(struct hid_parser
*parser, unsigned type)
* Add a usage to the temporary parser table.
*/
-static int hid_add_usage(struct hid_parser *parser, unsigned usage)
+static int hid_add_usage(struct hid_parser *parser, unsigned usage, __u8 size)
{
if (parser->local.usage_index >= HID_MAX_USAGES) {
hid_err(parser->device, "usage index exceeded\n");
return -1;
}
parser->local.usage[parser->local.usage_index] = usage;
+ parser->local.usage_size[parser->local.usage_index] = size;
parser->local.collection_index[parser->local.usage_index] =
parser->collection_stack_ptr ?
parser->collection_stack[parser->collection_stack_ptr - 1] : 0;
@@ -486,10 +487,7 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
- return hid_add_usage(parser, data);
+ return hid_add_usage(parser, data, item->size);
case HID_LOCAL_ITEM_TAG_USAGE_MINIMUM:
@@ -498,9 +496,6 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
parser->local.usage_minimum = data;
return 0;
@@ -511,9 +506,6 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
return 0;
}
- if (item->size <= 2)
- data = (parser->global.usage_page << 16) + data;
-
count = data - parser->local.usage_minimum;
if (count + parser->local.usage_index >= HID_MAX_USAGES) {
/*
@@ -533,7 +525,7 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
}
for (n = parser->local.usage_minimum; n <= data; n++)
- if (hid_add_usage(parser, n)) {
+ if (hid_add_usage(parser, n, item->size)) {
dbg_hid("hid_add_usage failed\n");
return -1;
}
@@ -553,8 +545,22 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
static int hid_parser_main(struct hid_parser *parser, struct hid_item *item)
{
+ unsigned int usages;
__u32 data;
int ret;
+ int i;
+
+ usages = max_t(unsigned, parser->local.usage_index,
+parser->global.report_count);
+
+ /*
+* As per specification, 6.2.2.8:
+* "When the parser encounters a main item it concatenates the last
+* declared Usage Page with a Usage to form a complete usage value."
+*/
+ for (i = 0; i < usages; i++)
+ if (parser->local.usage_size[i] <= 2)
+ parser->local.usage[i] += parser->global.usage_page <<
16;
data = item_udata(item);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f9707d1dcb58..d1fb4b678873 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -417,6 +417,7 @@ struct hid_global {
struct hid_local {
unsigned usage[HID_MAX_USAGES]; /* usage array */
+ __u8 usage_size[HID_MAX_USAGES]; /* usage size array */
unsigned collection_index[HID_MAX_USAGES]; /* collection index array */
unsigned usage_index;
unsigned usage_minimum;
--
2.21.0
Re: [RFC/RFT] HID: primax: Fix wireless keyboards descriptor
On Fri, 2019-03-01 at 10:48 +0100, Benjamin Tissoires wrote: > On Thu, Feb 28, 2019 at 7:01 PM Nicolas Saenz Julienne > wrote: > > On Thu, 2019-02-28 at 17:02 +, Junge, Terry wrote: > > > This could also be a parser error. In the HID specification section > > > 6.2.2.8 it > > > states that the last declared Usage Page is applied to Usages when the > > > Main > > > item is encountered. > > > > > > "If the bSize field = 1 or 2 then the Usage is interpreted as an unsigned > > > value > > > that selects a Usage ID on the currently defined Usage Page. When the > > > parser > > > encounters a main item it concatenates the last declared Usage Page with a > > > Usage to form a complete usage value. Extended usages can be used to > > > override the currently defined Usage Page for individual usages." > > > > > > > Hi Terry, thanks for the comment. > > Just for the record the paragraph I cited on my patch is the following: > > > > 6.2.2.7 Global Items > > > > [...] > > > > Usage Page: Unsigned integer specifying the current Usage Page. > > Since a > > usage are 32 bit values, Usage Page items can be used to conserve > > space > > in a report descriptor by setting the high order 16 bits of a > > subsequent usages. Any usage that follows which is defines* 16 bits > > or > > less is interpreted as a Usage ID and concatenated with the Usage > > Page > > to form a 32 bit Usage. > > > > * This is a spec errata, I belive it should say "defined" > > > > As you can see they use the word "follows" which in my opinion contradicts > > the > > paragraph you pointed out. That said I may be wrong, I'm not too good at > > reading specs :). > > I think you are both right (that is some decision making skills :-P). > > I never saw a case where the Usage Page was after the Usage. And I > would lean towards Nicolas interpretation. > However, Terry's point is valid too, and by re-reading the two > paragraphs, one could argue that the "follows" of 6.2.2.7 could be > applied when the Main item is processed as in 6.2.2.8. > > So I am going to base my decision on the "reference" driver. How well > behaves Windows with this particular keyboard? > > If it behaves properly, then we better use the 6.2.2.8 version where > the Usage Page is only appended when there is a Main item. This means > we need to remember what size was the last Usage (and Min/Max), to > know if we should concatenate the 2. > > Note that for every change that involves the generic parser, I'd like > a few tests added to `tests/test_keyboard.py in > https://gitlab.freedesktop.org/libevdev/hid-tools > Also note that the HID parser implementation in hid-tools follows the > kernel, so we might need to adjust it there too if we want to add high > level events. If you directly call `.call_input_event()` with raw > events, it should be fine. Thanks for the reply. I might get my hands on one of the faulty keyboards soon so I'll be able to check windows' behaviour and test the patches. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH] Input: raspberrypi-ts: select CONFIG_INPUT_POLLDEV
On Mon, 2019-03-04 at 20:57 +0100, Arnd Bergmann wrote:
> When CONFIG_INPUT_POLLDEV is disabled, we get a link error:
>
> drivers/input/touchscreen/raspberrypi-ts.o: In function `rpi_ts_probe':
> raspberrypi-ts.c:(.text+0xec): undefined reference to
> `devm_input_allocate_polled_device'
> raspberrypi-ts.c:(.text+0xec): relocation truncated to fit: R_AARCH64_CALL26
> against undefined symbol `devm_input_allocate_polled_device'
> raspberrypi-ts.c:(.text+0x19c): undefined reference to
> `input_register_polled_device'
> raspberrypi-ts.c:(.text+0x19c): relocation truncated to fit: R_AARCH64_CALL26
> against undefined symbol `input_register_polled_device'
>
> Select that symbol like we do from the other similar drivers.
>
> Fixes: 0b9f28fed3f7 ("Input: add official Raspberry Pi's touchscreen driver")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/input/touchscreen/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/touchscreen/Kconfig
> b/drivers/input/touchscreen/Kconfig
> index 7c597a49c265..7a4884ad198b 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -699,6 +699,7 @@ config TOUCHSCREEN_EDT_FT5X06
> config TOUCHSCREEN_RASPBERRYPI_FW
> tristate "Raspberry Pi's firmware base touch screen support"
> depends on RASPBERRYPI_FIRMWARE || (RASPBERRYPI_FIRMWARE=n &&
> COMPILE_TEST)
> + select INPUT_POLLDEV
> help
> Say Y here if you have the official Raspberry Pi 7 inch screen on
> your system.
Thanks!
Looks good to me.
Reviewed-by: Nicolas Saenz Julienne
signature.asc
Description: This is a digitally signed message part
Re: [RFC/RFT] HID: primax: Fix wireless keyboards descriptor
On Thu, 2019-02-28 at 17:02 +, Junge, Terry wrote: > This could also be a parser error. In the HID specification section 6.2.2.8 it > states that the last declared Usage Page is applied to Usages when the Main > item is encountered. > > "If the bSize field = 1 or 2 then the Usage is interpreted as an unsigned > value > that selects a Usage ID on the currently defined Usage Page. When the parser > encounters a main item it concatenates the last declared Usage Page with a > Usage to form a complete usage value. Extended usages can be used to > override the currently defined Usage Page for individual usages." > Hi Terry, thanks for the comment. Just for the record the paragraph I cited on my patch is the following: 6.2.2.7 Global Items [...] Usage Page: Unsigned integer specifying the current Usage Page. Since a usage are 32 bit values, Usage Page items can be used to conserve space in a report descriptor by setting the high order 16 bits of a subsequent usages. Any usage that follows which is defines* 16 bits or less is interpreted as a Usage ID and concatenated with the Usage Page to form a 32 bit Usage. * This is a spec errata, I belive it should say "defined" As you can see they use the word "follows" which in my opinion contradicts the paragraph you pointed out. That said I may be wrong, I'm not too good at reading specs :). I checked the HID parser and it's indeed written assuming local items are preceded by a Usage Page. I'd be glad to fix it there, but it would be nice to have the mantainer's opinion on the matter first. Regards, Nicolas signature.asc Description: This is a digitally signed message part
[RFC/RFT] HID: primax: Fix wireless keyboards descriptor
A whole set of Primax manufactured wireless keyboards won't work out of
the box as they provide wrong HID descriptors. In this case the offense
being defining the "Usage Maximum/Minimum" local items before the "Usage
Page". This will make the parser use the previous "Usage Page" on those
local items, generating a wrong HID report. This is not a parser error
as the spec clearly states that "Usage Page" applies to any item that
follows it (see 6.2.2.7).
In other words, we get this:
15 00 Logical Minimum (0),
26 ff 00 Logical Maximum (255),
19 00 Usage Minimum (00h),
2a ff 00 Usage Maximum (FFh),
05 07 Usage Page (Keyboard), ; Keyboard/keypad (07h)
75 08 Report Size (8),
95 06 Report Count (6),
81 00 Input,
Yet they meant this:
15 00 Logical Minimum (0),
26 ff 00 Logical Maximum (255),
05 07 Usage Page (Keyboard), ; Keyboard/keypad (07h)
19 00 Usage Minimum (00h),
2a ff 00 Usage Maximum (FFh),
75 08 Report Size (8),
95 06 Report Count (6),
81 00 Input,
This patch fixes the issue by overwriting the offending report with a
correct one.
This was made thanks to the user's answers provided here, also full HID
descriptors are available there:
https://unix.stackexchange.com/questions/377830/linux-hid-driver-for-primax-wireless-keyboards/
Signed-off-by: Nicolas Saenz Julienne
---
NOTE: This is an RFC as I'm unable to test my fix. Apart from the
devices mentioned in the patch there is a whole list of them, yet I
didn't include them as I was unable to access their HID descriptors.
drivers/hid/hid-ids.h| 3 +++
drivers/hid/hid-primax.c | 30 +-
drivers/hid/hid-quirks.c | 3 +++
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 24f846d67478..b39f9f36d41f 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -714,6 +714,7 @@
#define USB_DEVICE_ID_LENOVO_TPPRODOCK 0x6067
#define USB_DEVICE_ID_LENOVO_X1_COVER 0x6085
#define USB_DEVICE_ID_LENOVO_X1_TAB0x60a3
+#define USB_DEVICE_ID_LENOVO_WIRELESS_PROFESSIONAL 0x609b
#define USB_VENDOR_ID_LG 0x1fd2
#define USB_DEVICE_ID_LG_MULTITOUCH0x0064
@@ -1225,6 +1226,8 @@
#define USB_DEVICE_ID_PRIMAX_REZEL 0x4e72
#define USB_DEVICE_ID_PRIMAX_PIXART_MOUSE_4D0F 0x4d0f
#define USB_DEVICE_ID_PRIMAX_PIXART_MOUSE_4E22 0x4e22
+#define USB_DEVICE_ID_PRIMAX_WIRELESS_KEYBOARD_4E630x4e63
+#define USB_DEVICE_ID_PRIMAX_WIRELESS_KEYBOARD_4E800x4e80
#define USB_VENDOR_ID_RISO_KAGAKU 0x1294 /* Riso Kagaku Corp. */
diff --git a/drivers/hid/hid-primax.c b/drivers/hid/hid-primax.c
index 3a1c3c4c50dc..033f6660e8b6 100644
--- a/drivers/hid/hid-primax.c
+++ b/drivers/hid/hid-primax.c
@@ -2,9 +2,11 @@
* HID driver for primax and similar keyboards with in-band modifiers
*
* Copyright 2011 Google Inc. All Rights Reserved
+ * Copyright 2019 Nicolas Saenz Julienne
*
- * Author:
+ * Authors:
* Terry Lambert
+ * Nicolas Saenz Julienne
*
* This software is licensed under the terms of the GNU General Public
* License version 2, as published by the Free Software Foundation, and
@@ -27,6 +29,9 @@ static int px_raw_event(struct hid_device *hid, struct
hid_report *report,
{
int idx = size;
+ if (hid->product != USB_DEVICE_ID_PRIMAX_KEYBOARD)
+ return 0;
+
switch (report->id) {
case 0: /* keyboard input */
/*
@@ -64,8 +69,29 @@ static int px_raw_event(struct hid_device *hid, struct
hid_report *report,
return 0;
}
+/*
+ * Some Primax manufactured keyboards set "Usage Minimum/Maximum" values before
+ * setting the "Usage Page". This is not supported as per spec 6.2.2.7.
+ */
+static __u8 *px_fixup(struct hid_device *hid, __u8 *rdesc, unsigned int *rsize)
+{
+ __u8 wrong_report[] = { 0x19, 0x00, 0x2a, 0xff, 0x05, 0x07 };
+ __u8 fixed_report[] = { 0x05, 0x07, 0x19, 0x00, 0x2a, 0xff };
+
+ if (*rsize > 57 &&
+ !memcmp(&rdesc[51], wrong_report, ARRAY_SIZE(wrong_report))) {
+ hid_info(hid, "fixing up Primax report descriptor\n");
+ memcpy(&rdesc[51], fixed_report, ARRAY_SIZE(fixed_report));
+ }
+
+ return rdesc;
+}
+
static const struct hid_device_id px_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX,
USB_DEVICE_ID_PRIMAX_WIRELESS_KEYBOARD_4E63) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX,
USB_DEVICE_ID_PRIMAX_WIRELESS_KEYBOARD_4E80) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO,
USB_DEVICE_ID_LENOVO_WIRELESS_PROFESSIONAL) },
{ }
};
MODULE_DEVICE_TABLE(hid, px_devices);
@@ -74,8 +100,10 @@
[PATCH] usb: xhci: add Immediate Data Transfer support
Immediate data transfers (IDT) allow the HCD to copy small chunks of
data (up to 8bytes) directly into its output transfer TRBs. This avoids
the somewhat expensive DMA mappings that are performed by default on
most URBs submissions.
In the case an URB was suitable for IDT. The data is directly copied
into the "Data Buffer Pointer" region of the TRB and the IDT flag is
set. Instead of triggering memory accesses the HC will use the data
directly.
The implementation could cover all kind of output endpoints. Yet
Isochronous endpoints are bypassed as I was unable to find one that
matched IDT's constraints. As we try to bypass the default DMA mappings
on URB buffers we'd need to find a Isochronous device with an
urb->transfer_buffer_length <= 8 bytes.
The implementation takes into account that the 8 byte buffers provided
by the URB will never cross a 64KB boundary.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/usb/host/xhci-ring.c | 12
drivers/usb/host/xhci.c | 16
drivers/usb/host/xhci.h | 17 +
3 files changed, 45 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c4d041..997edc908a0d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
field |= TRB_IOC;
more_trbs_coming = false;
td->last_trb = ring->enqueue;
+
+ if (xhci_urb_suitable_for_idt(urb)) {
+ memcpy(&send_addr, urb->transfer_buffer,
+ trb_buff_len);
+ field |= TRB_IDT;
+ }
}
/* Only set interrupt on short packet for IN endpoints */
@@ -3411,6 +3417,12 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
if (urb->transfer_buffer_length > 0) {
u32 length_field, remainder;
+ if (xhci_urb_suitable_for_idt(urb)) {
+ memcpy(&urb->transfer_dma, urb->transfer_buffer,
+ urb->transfer_buffer_length);
+ field |= TRB_IDT;
+ }
+
remainder = xhci_td_remainder(xhci, 0,
urb->transfer_buffer_length,
urb->transfer_buffer_length,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 005e65922608..f04ad2290884 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
/*-*/
+/*
+ * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
+ * we'll copy the actual data into the TRB address register. This is limited to
+ * transfers up to 8 bytes on output endpoints of any kind with wMaxPacketSize
+ * >= 8 bytes. If suitable for IDT only one Transfer TRB per TD is allowed.
+ */
+static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+ gfp_t mem_flags)
+{
+ if (xhci_urb_suitable_for_idt(urb))
+ return 0;
+
+ return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+}
+
/**
* xhci_get_endpoint_index - Used for passing endpoint bitmasks between the
core and
* HCDs. Find the index for an endpoint given its descriptor. Use the return
@@ -5155,6 +5170,7 @@ static const struct hc_driver xhci_hc_driver = {
/*
* managing i/o requests and associated device resources
*/
+ .map_urb_for_dma = xhci_map_urb_for_dma,
.urb_enqueue = xhci_urb_enqueue,
.urb_dequeue = xhci_urb_dequeue,
.alloc_dev =xhci_alloc_dev,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 652dc36e3012..7dc6d2197641 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
#define TRB_IOC(1<<5)
/* The buffer pointer contains immediate data */
#define TRB_IDT(1<<6)
+/* TDs smaller than this might use IDT */
+#define TRB_IDT_MAX_SIZE 8
/* Block Event Interrupt */
#defineTRB_BEI (1<<9)
@@ -2141,6 +2143,21 @@ static inline struct xhci_ring
*xhci_urb_to_transfer_ring(struct xhci_hcd *xhci,
urb->stream_id);
}
+/*
+ * TODO: As per spec Isochronous IDT transmissions are supported. We bypass
+ * them anyways as we where unable to find a device that matches the
+ * constraints.
+ */
+static inline bool xhci_urb_suitable_for_idt(struct urb *urb)
+{
+ if (!usb_endpoint_xfer_isoc(&urb->ep->desc) &&am
Re: [RFC v2] usb: xhci: add Immediate Data Transfer support
Hi Felipe, thanks for the review!
On Wed, 2019-02-06 at 08:35 +0200, Felipe Balbi wrote:
> Hi,
>
> Nicolas Saenz Julienne writes:
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 40fa25c4d041..a4efbe62a1a3 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -3272,8 +3272,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> > field |= TRB_IOC;
> > more_trbs_coming = false;
> > td->last_trb = ring->enqueue;
> > +
> > + if (xhci_urb_suitable_for_idt(urb)) {
> > + memcpy(&send_addr, urb->transfer_buffer,
> > + trb_buff_len);
> > + field |= TRB_IDT;
> > + }
> > }
> >
> > +
>
> trailing change
Noted
>
> > @@ -3411,6 +3418,12 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> > if (urb->transfer_buffer_length > 0) {
> > u32 length_field, remainder;
> >
> > + if (xhci_urb_suitable_for_idt(urb)) {
> > + memcpy(&urb->transfer_dma, urb->transfer_buffer,
> > + urb->transfer_buffer_length);
> > + field |= TRB_IDT;
> > + }
> > +
> > remainder = xhci_td_remainder(xhci, 0,
> > urb->transfer_buffer_length,
> > urb->transfer_buffer_length,
> > @@ -3420,6 +3433,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> > TRB_INTR_TARGET(0);
> > if (setup->bRequestType & USB_DIR_IN)
> > field |= TRB_DIR_IN;
> > +
>
> trailing change
Noted
>
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 005e65922608..dec62f7f5dc8 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
> >
> > /*-
> > */
> >
> > +/*
> > + * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
> > + * we'll copy the actual data into the TRB address register. This is
> > limited to
> > + * transfers up to 8 bytes on output endpoints of any kind with
> > wMaxPacketSize
> > + * >= 8 bytes.
> > + */
> > +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> > + gfp_t mem_flags)
> > +{
> > + if (xhci_urb_suitable_for_idt(urb))
> > + return 0;
> > +
> > + return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
> > +}
>
> don't you need a matching unmap_urb_for_dma()??
Not really as every DMA mapping sets a matching URB flag to track it. For
example when usb_hcd_map_urb_for_dma() uses dma_map_single() it will set
URB_DMA_MAP_SINGLE in urb->transfer_flags, later on unmap_urb_for_dma() will
catch it and unmap it. As I bypass the mapping altogether there are no
flags set, so unmap_urb_for_dma() won't have any effect.
I could still add it for clarity, and well, I guess it'll save some
instructions on the IDT suitable side.
>
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index 652dc36e3012..9d77b0901ab7 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
> > #define TRB_IOC(1<<5)
> > /* The buffer pointer contains immediate data */
> > #define TRB_IDT(1<<6)
> > +/* TDs smaller than this might use IDT */
>
> Technically, "TDs at most this" since you're 8 itself is an allowed
> size.
>
Noted
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
[RFC v2] usb: xhci: add Immediate Data Transfer support
Immediate data transfers (IDT) allow the HCD to copy small chunks of
data (up to 8bytes) directly into its output transfer TRBs. This avoids
the somewhat expensive DMA mappings that are performed by default on
most URBs submissions.
In the case an URB was suitable for IDT. The data is directly copied
into the "Data Buffer Pointer" region of the TRB and the IDT flag is
set. Instead of triggering memory accesses the HC will use the data
directly.
The implementation covers all kind of output endpoints. All have been
tested successfully with multiple devices except for isochronous ones. I
can't seem to find a device that'll perform transfers with such small
packet sizes.
The implementation takes into account that the 8 byte buffers provided
by the URB will never cross a 64KB boundary.
Signed-off-by: Nicolas Saenz Julienne
---
Chages since first RFC:
- Got rid of URB flag
- Implement feature for Control transfers
- Implement feature for Isoc transfers
drivers/usb/host/xhci-ring.c | 20
drivers/usb/host/xhci.c | 16
drivers/usb/host/xhci.h | 12
3 files changed, 48 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c4d041..a4efbe62a1a3 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3272,8 +3272,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
field |= TRB_IOC;
more_trbs_coming = false;
td->last_trb = ring->enqueue;
+
+ if (xhci_urb_suitable_for_idt(urb)) {
+ memcpy(&send_addr, urb->transfer_buffer,
+ trb_buff_len);
+ field |= TRB_IDT;
+ }
}
+
/* Only set interrupt on short packet for IN endpoints */
if (usb_urb_dir_in(urb))
field |= TRB_ISP;
@@ -3411,6 +3418,12 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
if (urb->transfer_buffer_length > 0) {
u32 length_field, remainder;
+ if (xhci_urb_suitable_for_idt(urb)) {
+ memcpy(&urb->transfer_dma, urb->transfer_buffer,
+ urb->transfer_buffer_length);
+ field |= TRB_IDT;
+ }
+
remainder = xhci_td_remainder(xhci, 0,
urb->transfer_buffer_length,
urb->transfer_buffer_length,
@@ -3420,6 +3433,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
TRB_INTR_TARGET(0);
if (setup->bRequestType & USB_DIR_IN)
field |= TRB_DIR_IN;
+
queue_trb(xhci, ep_ring, true,
lower_32_bits(urb->transfer_dma),
upper_32_bits(urb->transfer_dma),
@@ -3710,6 +3724,12 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci,
gfp_t mem_flags,
if (trb_buff_len > td_remain_len)
trb_buff_len = td_remain_len;
+ if (xhci_urb_suitable_for_idt(urb)) {
+ memcpy(&addr, urb->transfer_buffer,
+ trb_buff_len);
+ field |= TRB_IDT;
+ }
+
/* Set the TRB length, TD size, & interrupter fields. */
remainder = xhci_td_remainder(xhci, running_total,
trb_buff_len, td_len,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 005e65922608..dec62f7f5dc8 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
/*-*/
+/*
+ * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
+ * we'll copy the actual data into the TRB address register. This is limited to
+ * transfers up to 8 bytes on output endpoints of any kind with wMaxPacketSize
+ * >= 8 bytes.
+ */
+static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+ gfp_t mem_flags)
+{
+ if (xhci_urb_suitable_for_idt(urb))
+ return 0;
+
+ return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+}
+
/**
* xhci_get_endpoint_index - Used for passing endpoint bitmasks between the
core and
* HCDs. Find the index for an endpoint given its descriptor. Use the return
@@ -5155,6 +5170,7 @@ static const struct hc_driver xhci_hc_driver = {
/*
* managing i/o requests and associated device res
Re: [RFC] usb: xhci: add Immediate Data Transfers support
Hi Felipe, thanks for the review :)
On Fri, 2019-02-01 at 15:31 +0200, Felipe Balbi wrote:
> Hi,
>
> Nicolas Saenz Julienne writes:
> > Immediate data transfers (IDT) allow the HCD to copy small chunks of
> > data (up to 8bits) directly into its output transfer TRBs. This avoids
> ^
> 8 bytes
Obviously, noted.
>
> > the somewhat expensive DMA mappings that are performed by default on
> > most URBs submissions.
> >
> > In the case an URB was suitable for IDT. The data is directly copied
> > into the "Data Buffer Pointer" region of the TRB and the IDT flag is
> > set. Instead of triggering memory accesses the HC will use the data
> > directly.
> >
> > An additional URB flag was created to mark whenever the URB doesn't need
> > any DMA mapping. Ideally it would have been nice to use a private flag
> > as this is specific to XHCI. Yet it's not possible as the URB private
> > area is allocated only after the DMA mapping is done.
> >
> > Isochronous transfers are not implemented so far as it's hard to find a
> > device that will send such small packets.
> >
> > This was tested using USB/serial adapter and by controlling the leds on
> > an XBOX controller. There where no disruptions on the rest of USB
> > devices attached on the system.
> >
> > Signed-off-by: Nicolas Saenz Julienne
> > ---
> > drivers/usb/host/xhci-ring.c | 6 ++
> > drivers/usb/host/xhci.c | 37
> > drivers/usb/host/xhci.h | 2 ++
> > include/linux/usb.h | 2 ++
> > 4 files changed, 47 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 40fa25c4d041..dd9805fb0566 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> > field |= TRB_IOC;
> > more_trbs_coming = false;
> > td->last_trb = ring->enqueue;
> > +
> > + if (urb->transfer_flags & URB_NO_DMA_MAP) {
>
> do you really need the flag? Why don't you do this unconditionally as
> long as urb->transfer_length is <= 8?
There is a set of limitations, see my comments below.
>
> > + memcpy(&send_addr, urb->transfer_buffer,
> > + full_len);
> > + field |= TRB_IDT;
> > + }
> > }
> >
> > /* Only set interrupt on short packet for IN endpoints */
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 005e65922608..ce3b6663f940 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1238,6 +1238,41 @@ EXPORT_SYMBOL_GPL(xhci_resume);
> >
> > /*-
> > */
> >
> > +static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> > +{
> > + if (urb->transfer_flags & URB_NO_DMA_MAP)
> > + urb->transfer_flags &= ~URB_NO_DMA_MAP;
> > + else
> > + usb_hcd_unmap_urb_for_dma(hcd, urb);
> > +}
> > +
> > +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> > + gfp_t mem_flags)
> > +{
> > + int maxp = usb_endpoint_maxp(&urb->ep->desc);
> > + int len = urb->transfer_buffer_length;
> > + int ret = 0;
> > +
> > + /*
> > +* Checks if URB is suitable for Immediate Transfer (IDT): instead of
> > +* mapping the buffer for DMA and passing the address to the host
> > +* controller, we copy the actual data into the TRB address register.
> > +* This is limited to transfers up to 8 bytes.
> > +*
> > +* IDT is only supported for Bulk and Interrupt endpoints. Apart from
> > +* the size constraints special care is taken to avoid cases where
> > +* wMaxPacketSize is smaller than 8 bytes as it's not supported.
> > +*/
> > + if ((usb_endpoint_is_int_out(&urb->ep->desc) ||
> > + usb_endpoint_is_bulk_out(&urb->ep->desc)) &&
>
> I don't understand the check for endpoint type. IDT is, actually,
> already used for control endpoints because setup packets are composed of
> 8 bytes. You're also showing that this works for INT and
[RFC] usb: xhci: add Immediate Data Transfers support
Immediate data transfers (IDT) allow the HCD to copy small chunks of
data (up to 8bits) directly into its output transfer TRBs. This avoids
the somewhat expensive DMA mappings that are performed by default on
most URBs submissions.
In the case an URB was suitable for IDT. The data is directly copied
into the "Data Buffer Pointer" region of the TRB and the IDT flag is
set. Instead of triggering memory accesses the HC will use the data
directly.
An additional URB flag was created to mark whenever the URB doesn't need
any DMA mapping. Ideally it would have been nice to use a private flag
as this is specific to XHCI. Yet it's not possible as the URB private
area is allocated only after the DMA mapping is done.
Isochronous transfers are not implemented so far as it's hard to find a
device that will send such small packets.
This was tested using USB/serial adapter and by controlling the leds on
an XBOX controller. There where no disruptions on the rest of USB
devices attached on the system.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/usb/host/xhci-ring.c | 6 ++
drivers/usb/host/xhci.c | 37
drivers/usb/host/xhci.h | 2 ++
include/linux/usb.h | 2 ++
4 files changed, 47 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c4d041..dd9805fb0566 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3272,6 +3272,12 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
field |= TRB_IOC;
more_trbs_coming = false;
td->last_trb = ring->enqueue;
+
+ if (urb->transfer_flags & URB_NO_DMA_MAP) {
+ memcpy(&send_addr, urb->transfer_buffer,
+ full_len);
+ field |= TRB_IDT;
+ }
}
/* Only set interrupt on short packet for IN endpoints */
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 005e65922608..ce3b6663f940 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1238,6 +1238,41 @@ EXPORT_SYMBOL_GPL(xhci_resume);
/*-*/
+static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+ if (urb->transfer_flags & URB_NO_DMA_MAP)
+ urb->transfer_flags &= ~URB_NO_DMA_MAP;
+ else
+ usb_hcd_unmap_urb_for_dma(hcd, urb);
+}
+
+static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+ gfp_t mem_flags)
+{
+ int maxp = usb_endpoint_maxp(&urb->ep->desc);
+ int len = urb->transfer_buffer_length;
+ int ret = 0;
+
+ /*
+* Checks if URB is suitable for Immediate Transfer (IDT): instead of
+* mapping the buffer for DMA and passing the address to the host
+* controller, we copy the actual data into the TRB address register.
+* This is limited to transfers up to 8 bytes.
+*
+* IDT is only supported for Bulk and Interrupt endpoints. Apart from
+* the size constraints special care is taken to avoid cases where
+* wMaxPacketSize is smaller than 8 bytes as it's not supported.
+*/
+ if ((usb_endpoint_is_int_out(&urb->ep->desc) ||
+ usb_endpoint_is_bulk_out(&urb->ep->desc)) &&
+ maxp >= TRB_IDT_MAX_SIZE && len <= TRB_IDT_MAX_SIZE)
+ urb->transfer_flags |= URB_NO_DMA_MAP;
+ else
+ ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+
+ return ret;
+}
+
/**
* xhci_get_endpoint_index - Used for passing endpoint bitmasks between the
core and
* HCDs. Find the index for an endpoint given its descriptor. Use the return
@@ -5155,6 +5190,8 @@ static const struct hc_driver xhci_hc_driver = {
/*
* managing i/o requests and associated device resources
*/
+ .map_urb_for_dma = xhci_map_urb_for_dma,
+ .unmap_urb_for_dma =xhci_unmap_urb_for_dma,
.urb_enqueue = xhci_urb_enqueue,
.urb_dequeue = xhci_urb_dequeue,
.alloc_dev =xhci_alloc_dev,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 652dc36e3012..1b51999794b3 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
#define TRB_IOC(1<<5)
/* The buffer pointer contains immediate data */
#define TRB_IDT(1<<6)
+/* TDs smaller 64bits this might use immediate data */
+#define TRB_IDT_MAX_SIZE 8
/* Block Event Interrupt */
#defineTRB_BEI (1<<9)
diff -
[PATCH v4] usb: hub: add retry routine after intr URB submit error
The hub sends hot-plug events to the host trough it's interrupt URB. The
driver takes care of completing the URB and re-submitting it. Completion
errors are handled in the hub_event() work, yet submission errors are
ignored, rendering the device unresponsive. All further events are lost.
It is fairly hard to find this issue in the wild, since you have to time
the USB hot-plug event with the URB submission failure. For instance it
could be the system running out of memory or some malfunction in the USB
controller driver. Nevertheless, it's pretty reasonable to think it'll
happen sometime. One can trigger this issue using eBPF's function
override feature (see BCC's inject.py script).
This patch adds a retry routine to the event of a submission error. The
HUB driver will try to re-submit the URB once every second until it's
successful or the HUB is disconnected.
As some USB subsystems already take care of this issue, the
implementation was inspired from usbhid/hid_core.c's.
Signed-off-by: Nicolas Saenz Julienne
Reviewed-by: Oliver Neukum
---
v4:
- Add Oliver's Reviewed-by
- Make timeout calculation simpler
v3: As per Oliver's request:
- Take care of race condition between disconnect and irq
v2: as per Alan's and Oliver's comments:
- Rename timer
- Delete the timer on disconnect
- Don't reset HUB nor exponential slowdown the timer, 1s fixed retry
period
- Check for -ESHUTDOWN prior kicking the timer
drivers/usb/core/hub.c | 43 --
drivers/usb/core/hub.h | 2 ++
2 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 1d1e61e980f3..713ab85332f8 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -607,6 +607,36 @@ static int hub_port_status(struct usb_hub *hub, int port1,
status, change, NULL);
}
+static void hub_resubmit_irq_urb(struct usb_hub *hub)
+{
+ unsigned long flags;
+ int status;
+
+ spin_lock_irqsave(&hub->irq_urb_lock, flags);
+
+ if (hub->quiescing) {
+ spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
+ return;
+ }
+
+ status = usb_submit_urb(hub->urb, GFP_ATOMIC);
+ if (status && status != -ENODEV && status != -EPERM &&
+ status != -ESHUTDOWN) {
+ dev_err(hub->intfdev, "resubmit --> %d\n", status);
+ mod_timer(&hub->irq_urb_retry, jiffies + HZ);
+ }
+
+ spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
+}
+
+static void hub_retry_irq_urb(struct timer_list *t)
+{
+ struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
+
+ hub_resubmit_irq_urb(hub);
+}
+
+
static void kick_hub_wq(struct usb_hub *hub)
{
struct usb_interface *intf;
@@ -709,12 +739,7 @@ static void hub_irq(struct urb *urb)
kick_hub_wq(hub);
resubmit:
- if (hub->quiescing)
- return;
-
- status = usb_submit_urb(hub->urb, GFP_ATOMIC);
- if (status != 0 && status != -ENODEV && status != -EPERM)
- dev_err(hub->intfdev, "resubmit --> %d\n", status);
+ hub_resubmit_irq_urb(hub);
}
/* USB 2.0 spec Section 11.24.2.3 */
@@ -1264,10 +1289,13 @@ enum hub_quiescing_type {
static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
{
struct usb_device *hdev = hub->hdev;
+ unsigned long flags;
int i;
/* hub_wq and related activity won't re-trigger */
+ spin_lock_irqsave(&hub->irq_urb_lock, flags);
hub->quiescing = 1;
+ spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
if (type != HUB_SUSPEND) {
/* Disconnect all the children */
@@ -1278,6 +1306,7 @@ static void hub_quiesce(struct usb_hub *hub, enum
hub_quiescing_type type)
}
/* Stop hub_wq and related activity */
+ del_timer_sync(&hub->irq_urb_retry);
usb_kill_urb(hub->urb);
if (hub->has_indicators)
cancel_delayed_work_sync(&hub->leds);
@@ -1810,6 +1839,8 @@ static int hub_probe(struct usb_interface *intf, const
struct usb_device_id *id)
INIT_DELAYED_WORK(&hub->leds, led_work);
INIT_DELAYED_WORK(&hub->init_work, NULL);
INIT_WORK(&hub->events, hub_event);
+ spin_lock_init(&hub->irq_urb_lock);
+ timer_setup(&hub->irq_urb_retry, hub_retry_irq_urb, 0);
usb_get_intf(intf);
usb_get_dev(hdev);
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 4accfb63f7dc..a9e24e4b8df1 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -69,6 +69,8 @@ struct usb_hub {
struct delayed_work leds;
struct delayed_work init_work;
struct work_struct events;
+ spinlock_t irq_urb_lock;
+ struct timer_list irq_urb_retry;
struct usb_port **ports;
};
--
2.20.1
Re: [PATCH v2 2/2] input: add official Raspberry Pi's touchscreen driver
On Thu, 2018-12-20 at 10:39 -0800, Eric Anholt wrote: > Nicolas Saenz Julienne writes: > > > Add's support to Raspberry Pi's 7" Touch device. Instead of using a > > conventional bus all information is copied into a memory mapped > > area by > > RPi's firmware. > > > > Based on the driver found in RPi's kernel repository. > > > > Signed-off-by: Nicolas Saenz Julienne > > While I wish we could just have a native driver instead, it does make > sense to have this driver upstream given the constraints. > > Acked-by: Eric Anholt Thanks Eric. FYI I did manage to access the touchscreen after altering the native driver (edt-ft5x06). That said, as commented in the cover letter, it's not good enough as it breaks the camera. I plan on posting the patch and an overlay. I'm just waiting to see how these first patches are received. signature.asc Description: This is a digitally signed message part
Re: [PATCH v2] drm/panel: rpi-touchscreen: Add backlight support
On Thu, 2018-12-20 at 18:36 +, Gordon Hollingworth wrote: > Assuming this is using the firmware interface then yes it's fine. If > it is using the i2c directly then it's possible to clash with the GPU > driving the camera Well we're already in such a situation without this patch, as the panel enables the back-light though the I2C lines. This is only enabled by an overlay, it's left to the user's discretion. As commented previously by Alex Graf and given the constraints, I think that it would make sense to start considering both options as good (using FW & direct I2C access). We are defaulting to the FW based one, but providing support/overlays for the direct access doesn't seem a bad compromise to me. I do understand Gordon's concerns. Maybe it would be nice for the firmware to detect device tree nodes accessing I2C0 and disable the camera. Or providing a FW configuration option. Kind regards, Nicolas signature.asc Description: This is a digitally signed message part
[PATCH v2] drm/panel: rpi-touchscreen: Add backlight support
This patch exposes backlight control into userspace by creating a
backlight device instead of directly accessing the PWM registers.
The backlight driver can't go on a separate file as it shares the I2C
bus & address with the panel.
Signed-off-by: Nicolas Saenz Julienne
---
v1 -> v2:
- Add Kconfig dependency with BACKLIGHT_CLASS_DEVICE
drivers/gpu/drm/panel/Kconfig | 1 +
.../drm/panel/panel-raspberrypi-touchscreen.c | 33 +--
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 6020c30a33b3..1ce35483f342 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -112,6 +112,7 @@ config DRM_PANEL_PANASONIC_VVX10F034N00
config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
tristate "Raspberry Pi 7-inch touchscreen panel"
depends on DRM_MIPI_DSI
+ depends on BACKLIGHT_CLASS_DEVICE
help
Say Y here if you want to enable support for the Raspberry
Pi 7" Touchscreen. To compile this driver as a module,
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
index 2c9c9722734f..838b5c8767bc 100644
--- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -52,6 +52,7 @@
#include
#include
#include
+#include
#include
#include
@@ -196,6 +197,7 @@ struct rpi_touchscreen {
struct drm_panel base;
struct mipi_dsi_device *dsi;
struct i2c_client *i2c;
+ struct backlight_device *backlight;
};
static const struct drm_display_mode rpi_touchscreen_modes[] = {
@@ -256,7 +258,8 @@ static int rpi_touchscreen_disable(struct drm_panel *panel)
{
struct rpi_touchscreen *ts = panel_to_ts(panel);
- rpi_touchscreen_i2c_write(ts, REG_PWM, 0);
+ ts->backlight->props.brightness = 0;
+ backlight_update_status(ts->backlight);
rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
udelay(1);
@@ -300,7 +303,8 @@ static int rpi_touchscreen_enable(struct drm_panel *panel)
msleep(100);
/* Turn on the backlight. */
- rpi_touchscreen_i2c_write(ts, REG_PWM, 255);
+ ts->backlight->props.brightness = 255;
+ backlight_update_status(ts->backlight);
/* Default to the same orientation as the closed source
* firmware used for the panel. Runtime rotation
@@ -358,12 +362,26 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs
= {
.get_modes = rpi_touchscreen_get_modes,
};
+static int raspberrypi_bl_update_status(struct backlight_device *bl)
+{
+ struct rpi_touchscreen *ts = bl_get_data(bl);
+
+ rpi_touchscreen_i2c_write(ts, REG_PWM, bl->props.brightness);
+
+ return 0;
+}
+
+static const struct backlight_ops raspberrypi_bl_ops = {
+ .update_status = raspberrypi_bl_update_status,
+};
+
static int rpi_touchscreen_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
struct device *dev = &i2c->dev;
struct rpi_touchscreen *ts;
struct device_node *endpoint, *dsi_host_node;
+ struct backlight_properties props;
struct mipi_dsi_host *host;
int ret, ver;
struct mipi_dsi_device_info info = {
@@ -398,6 +416,17 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
/* Turn off at boot, so we can cleanly sequence powering on. */
rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+ memset(&props, 0, sizeof(props));
+ props.type = BACKLIGHT_RAW;
+ props.max_brightness = 255;
+ ts->backlight = devm_backlight_device_register(dev, dev_name(dev), dev,
+ ts, &raspberrypi_bl_ops,
+ &props);
+ if (IS_ERR(ts->backlight)) {
+ dev_err(dev, "Failed to create backlight device\n");
+ return PTR_ERR(ts->backlight);
+ }
+
/* Look up the DSI host. It needs to probe before we do. */
endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
dsi_host_node = of_graph_get_remote_port_parent(endpoint);
--
2.19.2
[PATCH] xhci: fix 'broken_suspend' placement in struct xchi_hcd
As commented in the struct's definition there shouldn't be anything
underneath it's 'priv[0]' member as it would break some macros.
The patch converts the broken_suspend into a bit-field and relocates it
next to to the rest of bit-fields.
Fixes: a7d57abcc8a5 ("xhci: workaround CSS timeout on AMD SNPS 3.0 xHC")
Reported-by: Oliver Neukum
Signed-off-by: Nicolas Saenz Julienne
---
drivers/usb/host/xhci.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index c3515bad5dbb..011dd45f8718 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1863,6 +1863,8 @@ struct xhci_hcd {
unsignedsw_lpm_support:1;
/* support xHCI 1.0 spec USB2 hardware LPM */
unsignedhw_lpm_support:1;
+ /* Broken Suspend flag for SNPS Suspend resume issue */
+ unsignedbroken_suspend:1;
/* cached usb2 extened protocol capabilites */
u32 *ext_caps;
unsigned intnum_ext_caps;
@@ -1880,8 +1882,6 @@ struct xhci_hcd {
void*dbc;
/* platform-specific data -- must come last */
unsigned long priv[0] __aligned(sizeof(s64));
- /* Broken Suspend flag for SNPS Suspend resume issue */
- u8 broken_suspend;
};
/* Platform specific overrides to generic XHCI hc_driver ops */
--
2.19.2
[PATCH] drm/panel: rpi-touchscreen: Add backlight support
This patch exposes backlight control into userspace by creating a
backlight device instead of directly accessing the PWM registers.
The backlight driver can't go on a separate file as it shares the I2C
bus & address with the panel.
Signed-off-by: Nicolas Saenz Julienne
---
.../drm/panel/panel-raspberrypi-touchscreen.c | 33 +--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
index 2c9c9722734f..838b5c8767bc 100644
--- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -52,6 +52,7 @@
#include
#include
#include
+#include
#include
#include
@@ -196,6 +197,7 @@ struct rpi_touchscreen {
struct drm_panel base;
struct mipi_dsi_device *dsi;
struct i2c_client *i2c;
+ struct backlight_device *backlight;
};
static const struct drm_display_mode rpi_touchscreen_modes[] = {
@@ -256,7 +258,8 @@ static int rpi_touchscreen_disable(struct drm_panel *panel)
{
struct rpi_touchscreen *ts = panel_to_ts(panel);
- rpi_touchscreen_i2c_write(ts, REG_PWM, 0);
+ ts->backlight->props.brightness = 0;
+ backlight_update_status(ts->backlight);
rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
udelay(1);
@@ -300,7 +303,8 @@ static int rpi_touchscreen_enable(struct drm_panel *panel)
msleep(100);
/* Turn on the backlight. */
- rpi_touchscreen_i2c_write(ts, REG_PWM, 255);
+ ts->backlight->props.brightness = 255;
+ backlight_update_status(ts->backlight);
/* Default to the same orientation as the closed source
* firmware used for the panel. Runtime rotation
@@ -358,12 +362,26 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs
= {
.get_modes = rpi_touchscreen_get_modes,
};
+static int raspberrypi_bl_update_status(struct backlight_device *bl)
+{
+ struct rpi_touchscreen *ts = bl_get_data(bl);
+
+ rpi_touchscreen_i2c_write(ts, REG_PWM, bl->props.brightness);
+
+ return 0;
+}
+
+static const struct backlight_ops raspberrypi_bl_ops = {
+ .update_status = raspberrypi_bl_update_status,
+};
+
static int rpi_touchscreen_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
struct device *dev = &i2c->dev;
struct rpi_touchscreen *ts;
struct device_node *endpoint, *dsi_host_node;
+ struct backlight_properties props;
struct mipi_dsi_host *host;
int ret, ver;
struct mipi_dsi_device_info info = {
@@ -398,6 +416,17 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
/* Turn off at boot, so we can cleanly sequence powering on. */
rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+ memset(&props, 0, sizeof(props));
+ props.type = BACKLIGHT_RAW;
+ props.max_brightness = 255;
+ ts->backlight = devm_backlight_device_register(dev, dev_name(dev), dev,
+ ts, &raspberrypi_bl_ops,
+ &props);
+ if (IS_ERR(ts->backlight)) {
+ dev_err(dev, "Failed to create backlight device\n");
+ return PTR_ERR(ts->backlight);
+ }
+
/* Look up the DSI host. It needs to probe before we do. */
endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
dsi_host_node = of_graph_get_remote_port_parent(endpoint);
--
2.19.2
[PATCH 3/3] staging: vchiq: delete vchiq_killable.h
There are no users for that header file.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 1 -
.../interface/vchiq_arm/vchiq_arm.c | 1 -
.../interface/vchiq_arm/vchiq_connected.c | 1 -
.../interface/vchiq_arm/vchiq_core.c | 1 -
.../interface/vchiq_arm/vchiq_killable.h | 55 ---
.../interface/vchiq_arm/vchiq_util.c | 1 -
6 files changed, 60 deletions(-)
delete mode 100644
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 691038cdfdab..66fbb9ff551a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -48,7 +48,6 @@
#include "vchiq_arm.h"
#include "vchiq_connected.h"
-#include "vchiq_killable.h"
#include "vchiq_pagelist.h"
#define MAX_FRAGMENTS (VCHIQ_NUM_CURRENT_BULKS * 2)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 227c208dd122..acc7b07c7291 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -55,7 +55,6 @@
#include "vchiq_ioctl.h"
#include "vchiq_arm.h"
#include "vchiq_debugfs.h"
-#include "vchiq_killable.h"
#define DEVICE_NAME "vchiq"
diff --git
a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
index 7ea29665bd0c..7d64e2ed7b42 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -33,7 +33,6 @@
#include "vchiq_connected.h"
#include "vchiq_core.h"
-#include "vchiq_killable.h"
#include
#include
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index afe2aa5ac3e9..31e69b445b4c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -32,7 +32,6 @@
*/
#include "vchiq_core.h"
-#include "vchiq_killable.h"
#define VCHIQ_SLOT_HANDLER_STACK 8192
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h
deleted file mode 100644
index 778063ba312a..
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h
+++ /dev/null
@@ -1,55 +0,0 @@
-/**
- * Copyright (c) 2010-2012 Broadcom. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *notice, this list of conditions, and the following disclaimer,
- *without modification.
- * 2. Redistributions in binary form must reproduce the above copyright
- *notice, this list of conditions and the following disclaimer in the
- *documentation and/or other materials provided with the distribution.
- * 3. The names of the above-listed copyright holders may not be used
- *to endorse or promote products derived from this software without
- *specific prior written permission.
- *
- * ALTERNATIVELY, this software may be distributed under the terms of the
- * GNU General Public License ("GPL") version 2, as published by the Free
- * Software Foundation.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
- * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
- * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
- * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
- * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#ifndef VCHIQ_KILLABLE_H
-#define VCHIQ_KILLABLE_H
-
-#include
-#include
-
-#define SHUTDOWN_SIGS (sigmask(SIGKILL) | sigmask(SIGINT) | sigmask(SIGQUIT)
| sigmask(SIGTRAP) | sigmask(SIGSTOP) | sigmask(SIGCONT))
-
-static inline int __must_check down_interruptib
[PATCH 1/3] staging: vchiq: switch to wait_for_completion_killable
This fixes f27e47bc6b8b ("staging: vchiq: use completions instead of
semaphores") as it neglected the subtle down_interruptible() macro
override in vchiq_killable.h. Hence all completions should be killable
instead of interruptible.
Fixes: f27e47bc6b8b ("staging: vchiq: use completions instead of semaphores")
Reported-by: Arnd Bergmann
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 21 +--
.../interface/vchiq_arm/vchiq_core.c | 21 +--
.../interface/vchiq_arm/vchiq_util.c | 6 +++---
3 files changed, 23 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f28f681192dd..227c208dd122 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -559,8 +559,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T
reason,
vchiq_log_trace(vchiq_arm_log_level,
"%s - completion queue full", __func__);
DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
- if (wait_for_completion_interruptible(
- &instance->remove_event)) {
+ if (wait_for_completion_killable( &instance->remove_event)) {
vchiq_log_info(vchiq_arm_log_level,
"service_callback interrupted");
return VCHIQ_RETRY;
@@ -671,7 +670,7 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T
*header,
}
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
- if (wait_for_completion_interruptible(
+ if (wait_for_completion_killable(
&user_service->remove_event)
!= 0) {
vchiq_log_info(vchiq_arm_log_level,
@@ -1006,7 +1005,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
has been closed until the client library calls the
CLOSE_DELIVERED ioctl, signalling close_event. */
if (user_service->close_pending &&
- wait_for_completion_interruptible(
+ wait_for_completion_killable(
&user_service->close_event))
status = VCHIQ_RETRY;
break;
@@ -1182,7 +1181,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
DEBUG_TRACE(AWAIT_COMPLETION_LINE);
mutex_unlock(&instance->completion_mutex);
- rc = wait_for_completion_interruptible(
+ rc = wait_for_completion_killable(
&instance->insert_event);
mutex_lock(&instance->completion_mutex);
if (rc != 0) {
@@ -1351,7 +1350,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
do {
spin_unlock(&msg_queue_spinlock);
DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
- if (wait_for_completion_interruptible(
+ if (wait_for_completion_killable(
&user_service->insert_event)) {
vchiq_log_info(vchiq_arm_log_level,
"DEQUEUE_MESSAGE interrupted");
@@ -2356,7 +2355,7 @@ vchiq_keepalive_thread_func(void *v)
while (1) {
long rc = 0, uc = 0;
- if (wait_for_completion_interruptible(&arm_state->ka_evt)
+ if (wait_for_completion_killable(&arm_state->ka_evt)
!= 0) {
vchiq_log_error(vchiq_susp_log_level,
"%s interrupted", __func__);
@@ -2606,7 +2605,7 @@ block_resume(VCHIQ_ARM_STATE_T *arm_state)
write_unlock_bh(&arm_state->susp_res_lock);
vchiq_log_info(vchiq_susp_log_level, "%s wait for previously "
"blocked clients", __func__);
- if (wait_for_completion_interruptible_timeout(
+ if (wait_for_completion_killable_timeout(
&arm_state->blocked_blocker, timeout_val)
<= 0) {
vchiq_log_error(vchiq_susp_log_level, "%s wait for "
@@ -2632,7 +2631,7 @@ block_resume(VCHIQ_ARM_STATE_T *arm_sta
[PATCH 2/3] staging: vchiq_2835_arm: quit using custom down_interruptible()
vchi_killable.h overrides down_interruptible() by implementing a
function similar to down_killable(). To make things simpler we turn
calls to down_interruptible() into kernel's implementation of
down_killable().
Signed-off-by: Nicolas Saenz Julienne
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index ecee54a31f8d..691038cdfdab 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -543,7 +543,7 @@ create_pagelist(char __user *buf, size_t count, unsigned
short type)
(g_cache_line_size - 1 {
char *fragments;
- if (down_interruptible(&g_free_fragments_sema) != 0) {
+ if (down_killable(&g_free_fragments_sema)) {
cleanup_pagelistinfo(pagelistinfo);
return NULL;
}
--
2.19.2
[PATCH 0/3] staging: vchiq: fix completion routines & semaphores
As pointed out by Arnd Bergman[1] there where some issues on how I converted semaphores to completion routines. I wasn't aware of a macro override happening in vchiq_killable.h, which was changing down_interruptible()'s meaning. This patch changes all completions so they use the proper APIs and gets rid of vchiq_killable.h. I took into account Arnd's commit to avoid merge conflicts. [1] https://www.spinics.net/lists/arm-kernel/msg694422.html === Nicolas Saenz Julienne (3): staging: vchiq: switch to wait_for_completion_killable staging: vchiq_2835_arm: quit using custom down_interruptible() staging: vchiq: delete vchiq_killable.h .../interface/vchiq_arm/vchiq_2835_arm.c | 3 +- .../interface/vchiq_arm/vchiq_arm.c | 22 .../interface/vchiq_arm/vchiq_connected.c | 1 - .../interface/vchiq_arm/vchiq_core.c | 22 .../interface/vchiq_arm/vchiq_killable.h | 55 --- .../interface/vchiq_arm/vchiq_util.c | 7 +-- 6 files changed, 24 insertions(+), 86 deletions(-) delete mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_killable.h -- 2.19.2
Re: [PATCH] staging: vchiq: rework remove_event handling
On Tue, 2018-12-11 at 15:20 +0100, Arnd Bergmann wrote:
> On Tue, Dec 11, 2018 at 1:36 PM Nicolas Saenz Julienne
> wrote:
> > On Mon, 2018-12-10 at 22:11 +0100, Arnd Bergmann wrote:
> > > @@ -447,26 +444,26 @@ remote_event_wait(VCHIQ_STATE_T *state,
> > > REMOTE_EVENT_T *event)
> > > }
> > >
> > > static inline void
> > > -remote_event_signal_local(VCHIQ_STATE_T *state, REMOTE_EVENT_T
> > > *event)
> > > +remote_event_signal_local(wait_queue_head_t *wq, REMOTE_EVENT_T
> > > *event)
> > > {
> > > event->armed = 0;
> > > - complete((struct completion *)((char *)state + event-
> > > >event));
> > > + wake_up_all(wq);
> >
> > Shouldn't this just be "wake_up(wq)"?
>
> I wasn't entirely sure if we could get with more than one thread
> waiting
> for the wakeup. With the semaphore or completion that would already
> be broken because we'd only wake up one of them, but I was hoping
> to stay on the safe side with wake_up_all().
You're right. Had a look at the code and there shouldn't be more than
one thread waiting. wake_up_all() looks OK.
Reviewed-by: Nicolas Saenz Julienne
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] staging: vchiq: rework remove_event handling
Hi Arnd, thanks for the patch!
On Mon, 2018-12-10 at 22:11 +0100, Arnd Bergmann wrote:
> I had started the removal of semaphores in this driver without
> knowing
> that Nicolas Saenz Julienne also worked on this. In case of the
> "remote
> event" infrastructure, my solution seemed significantly better, so
> I'm
> proposing this as a change on top.
>
> The problem with using either semaphores or completions here is that
> it's an overly complex way of waking up a thread, and it looks like
> the
> 'count' of the semaphore can easily get out of sync, even though I
> found
> it hard to come up with a specific example.
>
> Changing it to a 'wait_queue_head_t' instead of a completion
> simplifies
> this by letting us wait directly on the 'event->fired' variable that
> is
> set by the videocore.
>
> Another simplification is passing the wait queue directly into the
> helper
> functions instead of going through the fragile logic of recording the
> offset inside of a structure as part of a shared memory variable.
> This
> also avoids one uncached memory read and should be faster.
>
> Note that I'm changing it back to 'killable' after the previous patch
> changed 'killable' to 'interruptible', apparently based on a
> misunderstanding
> of the subtle down_interruptible() macro override in
> vchiq_killable.h.
>
> Fixes: f27e47bc6b8b ("staging: vchiq: use completions instead of
> semaphores")
> Signed-off-by: Arnd Bergmann
> ---
> .../interface/vchiq_arm/vchiq_core.c | 63 +++
>
> .../interface/vchiq_arm/vchiq_core.h | 12 ++--
> 2 files changed, 30 insertions(+), 45 deletions(-)
>
> diff --git
> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> index 482b5daf6c0c..eda3004a0c6a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> @@ -417,26 +417,23 @@ vchiq_set_conn_state(VCHIQ_STATE_T *state,
> VCHIQ_CONNSTATE_T newstate)
> }
>
> static inline void
> -remote_event_create(REMOTE_EVENT_T *event)
> +remote_event_create(wait_queue_head_t *wq, REMOTE_EVENT_T *event)
> {
> event->armed = 0;
> /* Don't clear the 'fired' flag because it may already have
> been set
> ** by the other side. */
> + init_waitqueue_head(wq);
> }
>
> static inline int
> -remote_event_wait(VCHIQ_STATE_T *state, REMOTE_EVENT_T *event)
> +remote_event_wait(wait_queue_head_t *wq, REMOTE_EVENT_T *event)
> {
> if (!event->fired) {
> event->armed = 1;
> dsb(sy);
> - if (!event->fired) {
> - if (wait_for_completion_interruptible(
> - (struct completion *)
> - ((char *)state + event-
> >event))) {
> - event->armed = 0;
> - return 0;
> - }
> + if (wait_event_killable(*wq, event->fired)) {
> + event->armed = 0;
> + return 0;
> }
> event->armed = 0;
> wmb();
> @@ -447,26 +444,26 @@ remote_event_wait(VCHIQ_STATE_T *state,
> REMOTE_EVENT_T *event)
> }
>
> static inline void
> -remote_event_signal_local(VCHIQ_STATE_T *state, REMOTE_EVENT_T
> *event)
> +remote_event_signal_local(wait_queue_head_t *wq, REMOTE_EVENT_T
> *event)
> {
> event->armed = 0;
> - complete((struct completion *)((char *)state + event->event));
> + wake_up_all(wq);
Shouldn't this just be "wake_up(wq)"?
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] staging: vchiq: rework remove_event handling
On Tue, 2018-12-11 at 13:07 +0300, Dan Carpenter wrote: > On Mon, Dec 10, 2018 at 10:11:58PM +0100, Arnd Bergmann wrote: > > Note that I'm changing it back to 'killable' after the previous > > patch > > changed 'killable' to 'interruptible', apparently based on a > > misunderstanding > > of the subtle down_interruptible() macro override in > > vchiq_killable.h. > > Oh wow... The non standard down_interruptible() macro is ugly. > Same reaction here, *_*. I wasn't even aware of it. Arnd, as long as it you're not doing it already. I'll prepare some extra fixes for the rest of completions and get rid of that file altogether. Regards, Nicolas signature.asc Description: This is a digitally signed message part
[PATCH v2 1/2] dt-bindings: input: Add Raspberry Pi Touchscreen
Adds device tree documentation for Raspberry Pi's official 7"
touchscreen.
This binding is meant to be used as an overlay.
Signed-off-by: Nicolas Saenz Julienne
---
.../touchscreen/raspberrypi,firmware-ts.txt | 26 +++
1 file changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
diff --git
a/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
b/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
new file mode 100644
index ..2a1af240ccc3
--- /dev/null
+++
b/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
@@ -0,0 +1,26 @@
+Raspberry Pi firmware based 7" touchscreen
+=
+
+Required properties:
+ - compatible: "raspberrypi,firmware-ts"
+
+Optional properties:
+ - firmware: Reference to RPi's firmware device node
+ - touchscreen-size-x: See touchscreen.txt
+ - touchscreen-size-y: See touchscreen.txt
+ - touchscreen-inverted-x: See touchscreen.txt
+ - touchscreen-inverted-y: See touchscreen.txt
+ - touchscreen-swapped-x-y: See touchscreen.txt
+
+Example:
+
+firmware: firmware-rpi {
+ compatible = "raspberrypi,bcm2835-firmware";
+ mboxes = <&mailbox>;
+
+ ts: touchscreen {
+ compatible = "raspberrypi,firmware-ts";
+ touchscreen-size-x = <800>;
+ touchscreen-size-y = <480>;
+ };
+};
--
2.19.2
[PATCH v2 0/2] input: driver for RPi's official 7" touchscreen
This small series adds support for Raspberry pi's 7" touchscreen. Which alongside with the backlight driver are the last devices needed to have a functional touchscreen upstream. With this setup the board's VC4 firmware takes care of communicating with the touch chip and provides data though a shared memory area provided by the driver. The driver takes care of polling the firmware whenever at around 60Hz since there is no interrupt line available. The 1.0 revision of the touchscreen is based on the ft5426 chip. Technically, with some changes in edt-ft54x4.c we should be able to access the data directly through I2C. Yet this bus is meant to be owned by RPi's firmware and might access it anytime. For example, to configure RPi's camera device. As sharing the bus master interface is not possible a series of alternatives have been tested unsuccessfully [1]. It seems that we'll be unable to access the chip directly in a "clean" way which leaves us with this firmware based solution. The driver was rewritten based on the one available on the downstream Raspberry Pi kernel tree: https://github.com/raspberrypi/linux/. This series is based on v4.20-rc6 and was tested on a RPi 3 B+. Changelog v1 -> v2: - Remove all references to RPi3 - Spelling errors - Rename config string - Reorder devm_add_action_or_reset() - Remove __set_bit(EV* functions - Remove INPUT_MT_POINTER flag - Add missing header files - Create define to invalidate npointers - Fix commit ordering and titles RFC -> PATCH: - Better dependencies check in Kconfig - Add SPDX tag - Use polled input device API - Use input_mt_sync_frame() - Drop reference from dt node in probe - Use devm where possible - Small cosmetic changes [1] https://lists.infradead.org/pipermail/linux-rpi-kernel/2018-December/008444.html === Nicolas Saenz Julienne (2): dt-bindings: input: Add Raspberry Pi Touchscreen input: add official Raspberry Pi's touchscreen driver .../touchscreen/raspberrypi,firmware-ts.txt | 26 ++ drivers/input/touchscreen/Kconfig | 12 + drivers/input/touchscreen/Makefile| 1 + drivers/input/touchscreen/raspberrypi-ts.c| 227 ++ 4 files changed, 266 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt create mode 100644 drivers/input/touchscreen/raspberrypi-ts.c -- 2.19.2
[PATCH v2 2/2] input: add official Raspberry Pi's touchscreen driver
Add's support to Raspberry Pi's 7" Touch device. Instead of using a
conventional bus all information is copied into a memory mapped area by
RPi's firmware.
Based on the driver found in RPi's kernel repository.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/input/touchscreen/Kconfig | 12 ++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/raspberrypi-ts.c | 227 +
3 files changed, 240 insertions(+)
create mode 100644 drivers/input/touchscreen/raspberrypi-ts.c
diff --git a/drivers/input/touchscreen/Kconfig
b/drivers/input/touchscreen/Kconfig
index 2a80675cfd94..af6027cc7bbf 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -696,6 +696,18 @@ config TOUCHSCREEN_EDT_FT5X06
To compile this driver as a module, choose M here: the
module will be called edt-ft5x06.
+config TOUCHSCREEN_RASPBERRYPI_FW
+ tristate "Raspberry Pi's firmware base touch screen support"
+ depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
+ help
+ Say Y here if you have the official Raspberry Pi 7 inch screen on
+ your system.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called raspberrypi-ts.
+
config TOUCHSCREEN_MIGOR
tristate "Renesas MIGO-R touchscreen"
depends on (SH_MIGOR || COMPILE_TEST) && I2C
diff --git a/drivers/input/touchscreen/Makefile
b/drivers/input/touchscreen/Makefile
index 5911a4190cd2..fcc7605fba8d 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -109,3 +109,4 @@ obj-$(CONFIG_TOUCHSCREEN_ZET6223) += zet6223.o
obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50) += colibri-vf50-ts.o
obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023) += rohm_bu21023.o
+obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o
diff --git a/drivers/input/touchscreen/raspberrypi-ts.c
b/drivers/input/touchscreen/raspberrypi-ts.c
new file mode 100644
index ..7f8873eb2379
--- /dev/null
+++ b/drivers/input/touchscreen/raspberrypi-ts.c
@@ -0,0 +1,227 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Raspberry Pi firmware based touchscreen driver
+ *
+ * Copyright (C) 2015, 2017 Raspberry Pi
+ * Copyright (C) 2018 Nicolas Saenz Julienne
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define RPI_TS_DEFAULT_WIDTH 800
+#define RPI_TS_DEFAULT_HEIGHT 480
+
+#define RPI_TS_MAX_SUPPORTED_POINTS10
+
+#define RPI_TS_FTS_TOUCH_DOWN 0
+#define RPI_TS_FTS_TOUCH_CONTACT 2
+
+#define RPI_TS_POLL_INTERVAL 17 /* 60fps */
+
+#define RPI_TS_NPOINTS_REG_INVALIDATE 99
+
+struct rpi_ts {
+ struct platform_device *pdev;
+ struct input_polled_dev *poll_dev;
+ struct touchscreen_properties prop;
+
+ void __iomem *fw_regs_va;
+ dma_addr_t fw_regs_phys;
+
+ int known_ids;
+};
+
+struct rpi_ts_regs {
+ u8 device_mode;
+ u8 gesture_id;
+ u8 num_points;
+ struct rpi_ts_touch {
+ u8 xh;
+ u8 xl;
+ u8 yh;
+ u8 yl;
+ u8 pressure; /* Not supported */
+ u8 area; /* Not supported */
+ } point[RPI_TS_MAX_SUPPORTED_POINTS];
+};
+
+static void rpi_ts_poll(struct input_polled_dev *dev)
+{
+ struct input_dev *input = dev->input;
+ struct rpi_ts *ts = dev->private;
+ struct rpi_ts_regs regs;
+ int modified_ids = 0;
+ long released_ids;
+ int event_type;
+ int touchid;
+ int x, y;
+ int i;
+
+ memcpy_fromio(®s, ts->fw_regs_va, sizeof(regs));
+ /*
+* We poll the memory based register copy of the touchscreen chip using
+* the number of points register to know whether the copy has been
+* updated (we write 99 to the memory copy, the GPU will write between
+* 0 - 10 points)
+*/
+ iowrite8(RPI_TS_NPOINTS_REG_INVALIDATE,
+ts->fw_regs_va + offsetof(struct rpi_ts_regs, num_points));
+
+ if (regs.num_points == RPI_TS_NPOINTS_REG_INVALIDATE ||
+ (regs.num_points == 0 && ts->known_ids == 0))
+ return;
+
+ for (i = 0; i < regs.num_points; i++) {
+ x = (((int)regs.point[i].xh & 0xf) << 8) + regs.point[i].xl;
+ y = (((int)regs.point[i].yh & 0xf) << 8) + regs.point[i].yl;
+ touchid = (regs.point[i].yh >> 4) & 0xf;
+ event_type = (regs.point[i].xh >> 6) & 0x03;
+
+ modified_ids |= BIT(touchid);
+
+ if (event_type == RPI_TS_FTS_TOUCH_DOWN ||
+ event_type == RPI_TS_FTS_TOUCH_CONTACT) {
+
[PATCH] gpio: raspberrypi-exp: decrease refcount on firmware dt node
We're getting a reference RPi's firmware node in order to be able to
communicate with it's driver. We should decrease the reference count on
the dt node after being done with it.
Fixes: a98d90e7d588 ("gpio: raspberrypi-exp: Driver for RPi3 GPIO expander via
mailbox service")
Signed-off-by: Nicolas Saenz Julienne
---
drivers/gpio/gpio-raspberrypi-exp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-raspberrypi-exp.c
b/drivers/gpio/gpio-raspberrypi-exp.c
index d6d36d537e37..b77ea16ffa03 100644
--- a/drivers/gpio/gpio-raspberrypi-exp.c
+++ b/drivers/gpio/gpio-raspberrypi-exp.c
@@ -206,6 +206,7 @@ static int rpi_exp_gpio_probe(struct platform_device *pdev)
}
fw = rpi_firmware_get(fw_node);
+ of_node_put(fw_node);
if (!fw)
return -EPROBE_DEFER;
--
2.19.2
[PATCH 2/2] Input: raspberrypi-ts - add devicetree binding documentation
Adds device tree documentation for Raspberry Pi's official 7"
touchscreen.
Signed-off-by: Nicolas Saenz Julienne
---
.../touchscreen/raspberrypi,firmware-ts.txt | 26 +++
1 file changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
diff --git
a/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
b/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
new file mode 100644
index ..38e22eb222e9
--- /dev/null
+++
b/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
@@ -0,0 +1,26 @@
+Raspberry Pi 3 firmware based 7" touchscreen
+=
+
+Required properties:
+ - compatible: "raspberrypi,firmware-ts"
+
+Optional properties:
+ - firmware: Reference to RPi's firmware device node
+ - touchscreen-size-x: See touchscreen.txt
+ - touchscreen-size-y: See touchscreen.txt
+ - touchscreen-inverted-x: See touchscreen.txt
+ - touchscreen-inverted-y: See touchscreen.txt
+ - touchscreen-swapped-x-y: See touchscreen.txt
+
+Example:
+
+firmware: firmware-rpi {
+ compatible = "raspberrypi,bcm2835-firmware";
+ mboxes = <&mailbox>;
+
+ ts: touchscreen {
+ compatible = "raspberrypi,firmware-ts";
+ touchscreen-size-x = <800>;
+ touchscreen-size-y = <480>;
+ };
+};
--
2.19.2
[PATCH 1/2] input: add official Raspberry Pi's touchscreen driver
Add's support to Raspberry Pi's 7" Touch device. Instead of using a
conventional bus all information is copied into a memory mapped area by
RPi's firmware.
Based on the driver found in RPi's kernel repository.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/input/touchscreen/Kconfig | 12 ++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/raspberrypi-ts.c | 223 +
3 files changed, 236 insertions(+)
create mode 100644 drivers/input/touchscreen/raspberrypi-ts.c
diff --git a/drivers/input/touchscreen/Kconfig
b/drivers/input/touchscreen/Kconfig
index 2a80675cfd94..a9be434de738 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -696,6 +696,18 @@ config TOUCHSCREEN_EDT_FT5X06
To compile this driver as a module, choose M here: the
module will be called edt-ft5x06.
+config TOUCHSCREEN_RASPBERRYPI_TS
+ tristate "Raspberry Pi's firmware base touch screen support"
+ depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
+ help
+ Say Y here if you have the offitial Raspberry Pi 7' scren on your
+ system.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called raspberrypi-ts.
+
config TOUCHSCREEN_MIGOR
tristate "Renesas MIGO-R touchscreen"
depends on (SH_MIGOR || COMPILE_TEST) && I2C
diff --git a/drivers/input/touchscreen/Makefile
b/drivers/input/touchscreen/Makefile
index 5911a4190cd2..3eccb1925e89 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -109,3 +109,4 @@ obj-$(CONFIG_TOUCHSCREEN_ZET6223) += zet6223.o
obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50) += colibri-vf50-ts.o
obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023) += rohm_bu21023.o
+obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_TS) += raspberrypi-ts.o
diff --git a/drivers/input/touchscreen/raspberrypi-ts.c
b/drivers/input/touchscreen/raspberrypi-ts.c
new file mode 100644
index ..edc92018687e
--- /dev/null
+++ b/drivers/input/touchscreen/raspberrypi-ts.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Raspberry Pi 3 firmware based touchscreen driver
+ *
+ * Copyright (C) 2015, 2017 Raspberry Pi
+ * Copyright (C) 2018 Nicolas Saenz Julienne
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define RPI_TS_DEFAULT_WIDTH 800
+#define RPI_TS_DEFAULT_HEIGHT 480
+
+#define RPI_TS_MAX_SUPPORTED_POINTS10
+
+#define RPI_TS_FTS_TOUCH_DOWN 0
+#define RPI_TS_FTS_TOUCH_CONTACT 2
+
+#define RPI_TS_POLL_INTERVAL 17 /* 60fps */
+
+struct rpi_ts {
+ struct platform_device *pdev;
+ struct input_polled_dev *poll_dev;
+ struct touchscreen_properties prop;
+
+ void __iomem *fw_regs_va;
+ dma_addr_t fw_regs_phys;
+
+ int known_ids;
+};
+
+struct rpi_ts_regs {
+ u8 device_mode;
+ u8 gesture_id;
+ u8 num_points;
+ struct rpi_ts_touch {
+ u8 xh;
+ u8 xl;
+ u8 yh;
+ u8 yl;
+ u8 pressure; /* Not supported */
+ u8 area; /* Not supported */
+ } point[RPI_TS_MAX_SUPPORTED_POINTS];
+};
+
+/*
+ * We poll the memory based register copy of the touchscreen chip using the
+ * number of points register to know whether the copy has been updated (we
+ * write 99 to the memory copy, the GPU will write between 0 - 10 points)
+ */
+static void rpi_ts_poll(struct input_polled_dev *dev)
+{
+ struct input_dev *input = dev->input;
+ struct rpi_ts *ts = dev->private;
+ struct rpi_ts_regs regs;
+ int modified_ids = 0;
+ long released_ids;
+ int event_type;
+ int touchid;
+ int x, y;
+ int i;
+
+ memcpy_fromio(®s, ts->fw_regs_va, sizeof(regs));
+ iowrite8(99, ts->fw_regs_va + offsetof(struct rpi_ts_regs, num_points));
+
+ if (regs.num_points == 99 ||
+ (regs.num_points == 0 && ts->known_ids == 0))
+ return;
+
+ for (i = 0; i < regs.num_points; i++) {
+ x = (((int)regs.point[i].xh & 0xf) << 8) + regs.point[i].xl;
+ y = (((int)regs.point[i].yh & 0xf) << 8) + regs.point[i].yl;
+ touchid = (regs.point[i].yh >> 4) & 0xf;
+ event_type = (regs.point[i].xh >> 6) & 0x03;
+
+ modified_ids |= BIT(touchid);
+
+ if (event_type == RPI_TS_FTS_TOUCH_DOWN ||
+ event_type == RPI_TS_FTS_TOUCH_CONTACT) {
+ input_mt_slot(input, touchid);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER, 1);
+ touchsc
[PATCH 0/2] input: driver for RPi's official 7" touchscreen
This small series adds support for Raspberry pi's 7" touchscreen. Which alongside with the backlight driver are the last devices needed to have a functional touchscreen upstream. With this setup the board's VC4 firmware takes care of communicating with the touch chip and provides data though a shared memory area provided by the driver. The driver takes care of polling the firmware whenever at around 60Hz since there is no interrupt line available. The 1.0 revision of the touchscreen is based on the ft5426 chip. Technically, with some changes in edt-ft54x4.c we should be able to access the data directly through I2C. Yet this bus is meant to be owned by RPi's firmware and might access it anytime. For example, to configure RPi's camera device. As sharing the bus master interface is not possible a series of alternatives have been tested unsuccessfully [1]. It seems that we'll be unable to access the chip directly in a "clean" way which leaves us with this firmware based solution. The driver was rewritten based on the one available on the downstream Raspberry Pi kernel tree: https://github.com/raspberrypi/linux/. This series is based on v4.20-rc6 and was tested on a RPi 3 B+. Changelog RFC -> PATCH: - Better dependencies check in Kconfig - Add SPDX tag - Use polled input device API - Use input_mt_sync_frame() - Drop reference from dt node in probe - Use devm where possible - Small cosmetic changes [1] https://lists.infradead.org/pipermail/linux-rpi-kernel/2018-December/008444.html === Nicolas Saenz Julienne (2): input: add official Raspberry Pi's touchscreen driver Input: raspberrypi-ts - add devicetree binding documentation .../touchscreen/raspberrypi,firmware-ts.txt | 26 ++ drivers/input/touchscreen/Kconfig | 12 + drivers/input/touchscreen/Makefile| 1 + drivers/input/touchscreen/raspberrypi-ts.c| 223 ++ 4 files changed, 262 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt create mode 100644 drivers/input/touchscreen/raspberrypi-ts.c -- 2.19.2
[PATCH RFC 1/2] input: add official Raspberry Pi's 7" touchscreen driver
Adds support to Raspberry Pi's 7" touchscreen device. Instead of using
a conventional bus all information is copied into a memory mapped area
by RPi's VC4 firmware.
Based on the driver found in RPi's downstream kernel repository.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/raspberrypi-ts.c | 252 +
3 files changed, 265 insertions(+)
create mode 100644 drivers/input/touchscreen/raspberrypi-ts.c
diff --git a/drivers/input/touchscreen/Kconfig
b/drivers/input/touchscreen/Kconfig
index 2a80675cfd94..8d0fcb3dc8a8 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -696,6 +696,18 @@ config TOUCHSCREEN_EDT_FT5X06
To compile this driver as a module, choose M here: the
module will be called edt-ft5x06.
+config TOUCHSCREEN_RASPBERRYPI_TS
+ tristate "Raspberry Pi's firmware base touch screen support"
+ depends on OF
+ help
+ Say Y here if you have the official Raspberry Pi 7' touchscren
+ connected on your system.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called raspberrypi-ts.
+
config TOUCHSCREEN_MIGOR
tristate "Renesas MIGO-R touchscreen"
depends on (SH_MIGOR || COMPILE_TEST) && I2C
diff --git a/drivers/input/touchscreen/Makefile
b/drivers/input/touchscreen/Makefile
index 5911a4190cd2..3eccb1925e89 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -109,3 +109,4 @@ obj-$(CONFIG_TOUCHSCREEN_ZET6223) += zet6223.o
obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50) += colibri-vf50-ts.o
obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023) += rohm_bu21023.o
+obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_TS) += raspberrypi-ts.o
diff --git a/drivers/input/touchscreen/raspberrypi-ts.c
b/drivers/input/touchscreen/raspberrypi-ts.c
new file mode 100644
index ..9d29411a5674
--- /dev/null
+++ b/drivers/input/touchscreen/raspberrypi-ts.c
@@ -0,0 +1,252 @@
+/*
+ * Raspberry Pi 3 firmware based touchscreen driver
+ *
+ * Copyright (C) 2015, 2017 Raspberry Pi
+ * Copyright (C) 2018 Nicolas Saenz Julienne
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define RPI_TS_DEFAULT_WIDTH 800
+#define RPI_TS_DEFAULT_HEIGHT 480
+
+#define RPI_TS_MAX_SUPPORTED_POINTS10
+
+#define RPI_TS_FTS_TOUCH_DOWN 0
+#define RPI_TS_FTS_TOUCH_CONTACT 2
+
+struct rpi_ts {
+ struct input_dev *input;
+ struct touchscreen_properties prop;
+
+ void __iomem *ts_base;
+ dma_addr_t bus_addr;
+
+ struct delayed_work work;
+ int known_ids;
+};
+
+struct rpi_ts_regs {
+ uint8_t device_mode;
+ uint8_t gesture_id;
+ uint8_t num_points;
+ struct rpi_ts_touch {
+ uint8_t xh;
+ uint8_t xl;
+ uint8_t yh;
+ uint8_t yl;
+ uint8_t pressure; /* Not supported */
+ uint8_t area; /* Not supported */
+ } point[RPI_TS_MAX_SUPPORTED_POINTS];
+};
+
+/*
+ * This process polls the memory based register copy of the touch screen chip
+ * registers using the number of points register to know whether the copy has
+ * been updated (we write 99 to the memory copy, the GPU will write between 0 -
+ * 10 points)
+ */
+static void rpi_ts_work(struct work_struct *work)
+{
+ struct rpi_ts *ts = container_of(work, struct rpi_ts, work.work);
+ struct input_dev *input = ts->input;
+ struct rpi_ts_regs regs;
+ int modified_ids = 0;
+ int released_ids;
+ int event_type;
+ int touchid;
+ int x, y;
+ int i;
+
+ memcpy_fromio(®s, ts->ts_base, sizeof(struct rpi_ts_regs));
+ iowrite8(99, ts->ts_base + offsetof(struct rpi_ts_regs, num_points));
+
+ if (regs.num_points == 99 ||
+ (regs.num_points == 0 && ts->known_ids == 0))
+ goto out;
+
+ for (i = 0; i < regs.num_points; i++) {
+ x = (((int)regs.point[i].xh & 0xf) << 8) + regs.point[i].xl;
+ y = (((int)regs.point[i].yh & 0xf) << 8) + regs.point[i].yl;
+ touchid = (regs.point[i].yh >> 4) & 0xf;
+ event_type = (regs.point[i].xh >> 6) & 0x03;
+
+ modified_ids |= BIT(touchid);
+
+ if (event_type == RPI_TS_FTS_TOUCH_DOWN ||
+ event_type == RPI_TS_FTS_TOUCH_CONTACT) {
+
[PATCH RFC 2/2] Input: raspberrypi-ts - add devicetree binding documentation
Adds device tree documentation for Raspberry Pi's official 7"
touchscreen. The firmware mailbox interface allows the ARM core to
control the device.
Signed-off-by: Nicolas Saenz Julienne
---
.../touchscreen/raspberrypi,firmware-ts.txt | 25 +++
1 file changed, 25 insertions(+)
create mode 100644
Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
diff --git
a/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
b/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
new file mode 100644
index ..d28189476279
--- /dev/null
+++
b/Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt
@@ -0,0 +1,25 @@
+Raspberry Pi 3 firmware based 7" touchscreen
+=
+
+Required properties:
+ - compatible: "raspberrypi,firmware-ts"
+
+Optional properties:
+ - touchscreen-size-x: See touchscreen.txt
+ - touchscreen-size-y: See touchscreen.txt
+ - touchscreen-inverted-x: See touchscreen.txt
+ - touchscreen-inverted-y: See touchscreen.txt
+ - touchscreen-swapped-x-y: See touchscreen.txt
+
+Example:
+
+firmware: firmware-rpi {
+ compatible = "raspberrypi,bcm2835-firmware", "simple-bus";
+ mboxes = <&mailbox>;
+
+ ts: touchscreen {
+ compatible = "raspberrypi,firmware-ts";
+ touchscreen-size-x = <800>;
+ touchscreen-size-y = <480>;
+ };
+};
--
2.19.2
[PATCH RFC 0/2] input: driver for RPi's official 7" touchscreen
This small series adds support for Raspberry pi's 7" touchscreen. Which alongside with the backlight driver are the last devices needed to have a functional touchscreen upstream. With this setup the board's VC4 firmware takes care of communicating with the touch chip and provides data though a shared memory area provided by the driver. The driver takes care of polling the firmware whenever at around 60Hz since there is no interrupt line available. The 1.0 revision of the touchscreen is based on the ft5426 chip. Technically, with some changes in edt-ft54x4.c we should be able to access the data directly through I2C. Yet this bus is meant to be owned by RPi's firmware and might access it anytime. For example, to configure RPi's camera device. As sharing the bus master interface is not possible a series of alternatives have been tested unsuccessfully [1]. It seems that we'll be unable to access the chip directly in a "clean" way which leaves us with this firmware based solution. The driver was rewritten based on the one available on the downstream Raspberry Pi kernel tree: https://github.com/raspberrypi/linux/. This series is based on v4.20-rc5 and was tested on a RPi 3 B+. [1] https://lists.infradead.org/pipermail/linux-rpi-kernel/2018-December/008444.html === Nicolas Saenz Julienne (2): input: add official Raspberry Pi's 7" touchscreen driver Input: raspberrypi-ts - add devicetree binding documentation .../touchscreen/raspberrypi,firmware-ts.txt | 25 ++ drivers/input/touchscreen/Kconfig | 12 + drivers/input/touchscreen/Makefile| 1 + drivers/input/touchscreen/raspberrypi-ts.c| 252 ++ 4 files changed, 290 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/touchscreen/raspberrypi,firmware-ts.txt create mode 100644 drivers/input/touchscreen/raspberrypi-ts.c -- 2.19.2
[PATCH v2] firmware: stratix10-svc: fix wrong of_node_put() in init function
After finding a "firmware" dt node stratix10 tries to match it's
compatible string with it. To do so it's calling of_find_matching_node()
which already takes care of decreasing the refcount on the "firmware"
node. We are then incorrectly decreasing the refcount on that node
again.
This patch removes the unwarranted call to of_node_put().
Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
Signed-off-by: Nicolas Saenz Julienne
---
v1 -> v2: add actual people to CC & remove unnecessary braces
drivers/firmware/stratix10-svc.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 81f3182e290d..6e6514825ad0 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -1016,14 +1016,11 @@ static int __init stratix10_svc_init(void)
return -ENODEV;
np = of_find_matching_node(fw_np, stratix10_svc_drv_match);
- if (!np) {
- of_node_put(fw_np);
+ if (!np)
return -ENODEV;
- }
of_node_put(np);
ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);
- of_node_put(fw_np);
if (ret)
return ret;
--
2.19.2
Re: [PATCH] firmware: arm_sdei: fix wrong of_node_put() in init function
Hi James, thanks for the review!
On Fri, 2018-11-30 at 18:31 +, James Morse wrote:
> Hi Nicolas,
>
> On 26/11/2018 12:15, Nicolas Saenz Julienne wrote:
> > After finding a "firmware" dt node arm_sdei tries to match it's
> > compatible string with it. To do so it's calling
> > of_find_matching_node()
> > which already takes care of decreasing the refcount on the
> > "firmware"
> > node. We are then incorrectly decreasing the refcount on that node
> > again.
> >
> > This patch removes the unwarranted call to of_node_put().
> >
> > Fixes: ad6eb31ef903 ("firmware: arm_sdei: Add driver for Software
> > Delegated Exceptions")
> > Signed-off-by: Nicolas Saenz Julienne
>
> Thanks!, I agree this is unwarranted.
> Is there a tool that picks these up? I remember sparse giving me a
> headache, but
> I don't remember this one... I probably cargo-culted it from
> somewhere else.
We stumbled upon this one on a test system. TBH I don't really know
much about these tools so I can't tell. That said, I sent 4 more fixes
on this bug (one more in drivers/firmware) so there definitively was
some cargo-culting happening.
Regards,
Nicolas
>
> Acked-by: James Morse
>
>
> Thanks,
>
> James
>
>
> > ---
> > drivers/firmware/arm_sdei.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/firmware/arm_sdei.c
> > b/drivers/firmware/arm_sdei.c
> > index 1ea71640fdc2..dffb47c6b480 100644
> > --- a/drivers/firmware/arm_sdei.c
> > +++ b/drivers/firmware/arm_sdei.c
> > @@ -1017,7 +1017,6 @@ static bool __init sdei_present_dt(void)
> > return false;
> >
> > np = of_find_matching_node(fw_np, sdei_of_match);
> > - of_node_put(fw_np);
> > if (!np)
> > return false;
> >
> >
signature.asc
Description: This is a digitally signed message part
[PATCH] firmware: stratix10-svc: fix wrong of_node_put() in init function
After finding a "firmware" dt node stratix10 tries to match it's
compatible string with it. To do so it's calling of_find_matching_node()
which already takes care of decreasing the refcount on the "firmware"
node. We are then incorrectly decreasing the refcount on that node
again.
This patch removes the unwarranted call to of_node_put().
Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
Signed-off-by: Nicolas Saenz Julienne
---
drivers/firmware/stratix10-svc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 81f3182e290d..4e921564938d 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -1017,13 +1017,11 @@ static int __init stratix10_svc_init(void)
np = of_find_matching_node(fw_np, stratix10_svc_drv_match);
if (!np) {
- of_node_put(fw_np);
return -ENODEV;
}
of_node_put(np);
ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);
- of_node_put(fw_np);
if (ret)
return ret;
--
2.19.2
[PATCH] fpga: stratix10-soc: fix wrong of_node_put() in init function
After finding a "firmware" dt node stratix10 tries to match it's
compatible string with it. To do so it's calling of_find_matching_node()
which already takes care of decreasing the refcount on the "firmware"
node. We are then incorrectly decreasing the refcount on that node
again.
This patch removes the unwarranted call to of_node_put().
Fixes: e7eef1d7633a ("fpga: add intel stratix10 soc fpga manager driver")
Signed-off-by: Nicolas Saenz Julienne
---
drivers/fpga/stratix10-soc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index a1a09e04fab8..e75dbe583152 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -509,13 +509,11 @@ static int __init s10_init(void)
np = of_find_matching_node(fw_np, s10_of_match);
if (!np) {
- of_node_put(fw_np);
return -ENODEV;
}
of_node_put(np);
ret = of_platform_populate(fw_np, s10_of_match, NULL, NULL);
- of_node_put(fw_np);
if (ret)
return ret;
--
2.19.2
[PATCH] w1: fix wrong dt refcount in attach slave device
With each call to of_find_matching_node() we're decresing the reference
count on the master's dt node. This is not what we actually want. So we
get a refernce to the dt node before calling it.
Fixes: fae68031f7fbc ("w1: core: match sub-nodes of bus masters in devicetree")
Signed-off-by: Nicolas Saenz Julienne
---
drivers/w1/w1.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 890c038c25f8..09514fa6c6b9 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -680,15 +680,17 @@ static int w1_family_notify(unsigned long action, struct
w1_slave *sl)
static int __w1_attach_slave_device(struct w1_slave *sl)
{
+ struct device_node *np;
int err;
+ np = of_node_get(sl->master->dev.of_node);
+
sl->dev.parent = &sl->master->dev;
sl->dev.driver = &w1_slave_driver;
sl->dev.bus = &w1_bus_type;
sl->dev.release = &w1_slave_release;
sl->dev.groups = w1_slave_groups;
- sl->dev.of_node = of_find_matching_node(sl->master->dev.of_node,
- sl->family->of_match_table);
+ sl->dev.of_node = of_find_matching_node(np, sl->family->of_match_table);
dev_set_name(&sl->dev, "%02x-%012llx",
(unsigned int) sl->reg_num.family,
--
2.19.2
[PATCH] firmware: arm_sdei: fix wrong of_node_put() in init function
After finding a "firmware" dt node arm_sdei tries to match it's
compatible string with it. To do so it's calling of_find_matching_node()
which already takes care of decreasing the refcount on the "firmware"
node. We are then incorrectly decreasing the refcount on that node
again.
This patch removes the unwarranted call to of_node_put().
Fixes: ad6eb31ef903 ("firmware: arm_sdei: Add driver for Software Delegated
Exceptions")
Signed-off-by: Nicolas Saenz Julienne
---
drivers/firmware/arm_sdei.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index 1ea71640fdc2..dffb47c6b480 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -1017,7 +1017,6 @@ static bool __init sdei_present_dt(void)
return false;
np = of_find_matching_node(fw_np, sdei_of_match);
- of_node_put(fw_np);
if (!np)
return false;
--
2.19.1
[PATCH 00/16] staging: vchiq: dead code removal & misc fixes
Hi All, This series was written in parallel with reading and understanding the vchiq code. So excuse me for the lack of logic in the sequence of patches. The main focus was to delete as much code as possible, I've counted around 550 lines, which is not bad. Apart from that there are some patches enforcing proper kernel APIs usage. The only patch that really changes code is the vchiq_ioc_copy_element_data() rewrite. The last commit updates the TODO list with some of my observations, I realise some of the might be a little opinionated. If anything it's going to force a discussion on the topic, which is nice. It was developed on top of the latest linux-next, and was tested on a RPIv3B+ with audio, video and running vchiq_test. Regards, Nicolas RFC -> PATCH, as per Stefan's comments: - Remove semaphore initialization from remove_event_create() (commit 9) - Join all three semaphore to completion patches (commit 11) - Update probe/init commit message (commit 14) - Update TODO commit message and clean up (commit 16) - Fix spelling on some of the patches === Nicolas Saenz Julienne (16): staging: vchiq_core: rework vchiq_get_config staging: vchiq_arm: rework close/remove_service IOCTLS staging: vchiq_shim: delete vchi_service_create staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list staging: vchiq_arm: get rid of vchi_mh.h staging: vchiq_arm: rework vchiq_ioc_copy_element_data staging: vchiq-core: get rid of is_master distinction staging: vchiq_core: remove unnecessary safety checks in vchiq_init_state staging: vchiq_core: do not initialize semaphores twice staging: vchiq_core: don't add a wmb() before remote_event_signal() staging: vchiq: use completions instead of semaphores staging: vchiq_util: get rid of unneeded memory barriers staging: vchiq_core: fix logic redundancy in parse_open staging: vchiq_arm: rework probe and init functions staging: vchiq_arm: fix open/release cdev functions staging: vchiq: add more tasks to the TODO list .../staging/vc04_services/interface/vchi/TODO | 42 ++ .../vc04_services/interface/vchi/vchi.h | 8 - .../vc04_services/interface/vchi/vchi_mh.h| 42 -- .../interface/vchiq_arm/vchiq_2835_arm.c | 18 +- .../interface/vchiq_arm/vchiq_arm.c | 598 -- .../interface/vchiq_arm/vchiq_core.c | 523 --- .../interface/vchiq_arm/vchiq_core.h | 47 +- .../interface/vchiq_arm/vchiq_if.h| 11 +- .../interface/vchiq_arm/vchiq_shim.c | 32 - .../interface/vchiq_arm/vchiq_util.c | 48 +- .../interface/vchiq_arm/vchiq_util.h | 6 +- 11 files changed, 435 insertions(+), 940 deletions(-) delete mode 100644 drivers/staging/vc04_services/interface/vchi/vchi_mh.h -- 2.19.1
[PATCH 04/16] staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list
The resulting code is way more readeable and intuitive compared to plain
list_for_each.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 52 ++-
1 file changed, 16 insertions(+), 36 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 835152799433..76de36e9e3b6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -280,16 +280,11 @@ VCHIQ_STATUS_T vchiq_shutdown(VCHIQ_INSTANCE_T instance)
"%s(%p): returning %d", __func__, instance, status);
if (status == VCHIQ_SUCCESS) {
- struct list_head *pos, *next;
+ struct bulk_waiter_node *waiter, *next;
- list_for_each_safe(pos, next,
- &instance->bulk_waiter_list) {
- struct bulk_waiter_node *waiter;
-
- waiter = list_entry(pos,
- struct bulk_waiter_node,
- list);
- list_del(pos);
+ list_for_each_entry_safe(waiter, next,
+&instance->bulk_waiter_list, list) {
+ list_del(&waiter->list);
vchiq_log_info(vchiq_arm_log_level,
"bulk_waiter - cleaned up %pK for pid
%d",
waiter, waiter->pid);
@@ -473,7 +468,6 @@ vchiq_blocking_bulk_transfer(VCHIQ_SERVICE_HANDLE_T handle,
void *data,
VCHIQ_SERVICE_T *service;
VCHIQ_STATUS_T status;
struct bulk_waiter_node *waiter = NULL;
- struct list_head *pos;
service = find_service_by_handle(handle);
if (!service)
@@ -484,13 +478,9 @@ vchiq_blocking_bulk_transfer(VCHIQ_SERVICE_HANDLE_T
handle, void *data,
unlock_service(service);
mutex_lock(&instance->bulk_waiter_list_mutex);
- list_for_each(pos, &instance->bulk_waiter_list) {
- if (list_entry(pos, struct bulk_waiter_node,
- list)->pid == current->pid) {
- waiter = list_entry(pos,
- struct bulk_waiter_node,
- list);
- list_del(pos);
+ list_for_each_entry(waiter, &instance->bulk_waiter_list, list) {
+ if (waiter->pid == current->pid) {
+ list_del(&waiter->list);
break;
}
}
@@ -1135,21 +1125,16 @@ vchiq_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
ret = -ENOMEM;
break;
}
+
args.userdata = &waiter->bulk_waiter;
} else if (args.mode == VCHIQ_BULK_MODE_WAITING) {
- struct list_head *pos;
-
mutex_lock(&instance->bulk_waiter_list_mutex);
- list_for_each(pos, &instance->bulk_waiter_list) {
- if (list_entry(pos, struct bulk_waiter_node,
- list)->pid == current->pid) {
- waiter = list_entry(pos,
- struct bulk_waiter_node,
- list);
- list_del(pos);
+ list_for_each_entry(waiter, &instance->bulk_waiter_list,
+ list) {
+ if (waiter->pid == current->pid) {
+ list_del(&waiter->list);
break;
}
-
}
mutex_unlock(&instance->bulk_waiter_list_mutex);
if (!waiter) {
@@ -2163,16 +2148,11 @@ vchiq_release(struct inode *inode, struct file *file)
vchiq_release_internal(instance->state, NULL);
{
- struct list_head *pos, *next;
-
- list_for_each_safe(pos, next,
- &instance->bulk_waiter_list) {
- struct bulk_waiter_node *waiter;
+ struct bulk_waiter_node *waiter, *next;
- waiter = list_entry(pos,
- struct bulk_waiter_node,
- list);
- list_del(pos);
+ list_for_each_en
[PATCH 01/16] staging: vchiq_core: rework vchiq_get_config
The function is overly complicated for what it's ultimately achieving.
It's simply filling up a structure.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 12
.../interface/vchiq_arm/vchiq_core.c | 30 +--
.../interface/vchiq_arm/vchiq_if.h| 3 +-
3 files changed, 14 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 45de21c210c1..5af3f2651bd3 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1480,13 +1480,11 @@ vchiq_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
ret = -EINVAL;
break;
}
- status = vchiq_get_config(instance, args.config_size, &config);
- if (status == VCHIQ_SUCCESS) {
- if (copy_to_user((void __user *)args.pconfig,
- &config, args.config_size) != 0) {
- ret = -EFAULT;
- break;
- }
+
+ vchiq_get_config(&config);
+ if (copy_to_user(args.pconfig, &config, args.config_size)) {
+ ret = -EFAULT;
+ break;
}
} break;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 7642ced31436..89f1ccdc3b98 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3583,28 +3583,14 @@ vchiq_get_peer_version(VCHIQ_SERVICE_HANDLE_T handle,
short *peer_version)
return status;
}
-VCHIQ_STATUS_T
-vchiq_get_config(VCHIQ_INSTANCE_T instance,
- int config_size, VCHIQ_CONFIG_T *pconfig)
-{
- VCHIQ_CONFIG_T config;
-
- (void)instance;
-
- config.max_msg_size = VCHIQ_MAX_MSG_SIZE;
- config.bulk_threshold = VCHIQ_MAX_MSG_SIZE;
- config.max_outstanding_bulks = VCHIQ_NUM_SERVICE_BULKS;
- config.max_services = VCHIQ_MAX_SERVICES;
- config.version= VCHIQ_VERSION;
- config.version_min= VCHIQ_VERSION_MIN;
-
- if (config_size > sizeof(VCHIQ_CONFIG_T))
- return VCHIQ_ERROR;
-
- memcpy(pconfig, &config,
- min(config_size, (int)(sizeof(VCHIQ_CONFIG_T;
-
- return VCHIQ_SUCCESS;
+void vchiq_get_config(VCHIQ_CONFIG_T *config)
+{
+ config->max_msg_size = VCHIQ_MAX_MSG_SIZE;
+ config->bulk_threshold = VCHIQ_MAX_MSG_SIZE;
+ config->max_outstanding_bulks = VCHIQ_NUM_SERVICE_BULKS;
+ config->max_services = VCHIQ_MAX_SERVICES;
+ config->version= VCHIQ_VERSION;
+ config->version_min= VCHIQ_VERSION_MIN;
}
VCHIQ_STATUS_T
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
index e4109a83e628..87829a244465 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
@@ -164,8 +164,7 @@ extern VCHIQ_STATUS_T
vchiq_bulk_receive_handle(VCHIQ_SERVICE_HANDLE_T service,
extern int vchiq_get_client_id(VCHIQ_SERVICE_HANDLE_T service);
extern void *vchiq_get_service_userdata(VCHIQ_SERVICE_HANDLE_T service);
extern int vchiq_get_service_fourcc(VCHIQ_SERVICE_HANDLE_T service);
-extern VCHIQ_STATUS_T vchiq_get_config(VCHIQ_INSTANCE_T instance,
- int config_size, VCHIQ_CONFIG_T *pconfig);
+extern void vchiq_get_config(VCHIQ_CONFIG_T *config);
extern VCHIQ_STATUS_T vchiq_set_service_option(VCHIQ_SERVICE_HANDLE_T service,
VCHIQ_SERVICE_OPTION_T option, int value);
--
2.19.1
[PATCH 06/16] staging: vchiq_arm: rework vchiq_ioc_copy_element_data
The function is passed to vchiq_core.c for it to go trough all the
transfer elements (an array of pointers to data) and copy them into the
actual transfer memory (contiguous memory).
The logic in the function was "copy an element and return, except when
the element is empty, in which case look for the next non-empty element
and copy it. The function will be called as many times as necessary until
all the elements are copied".
Now, this approach already forces the function to loop around elements
and felt convoluted, so it was changed to a more straightforward "Copy
all the elements into memory as long as they fit".
The resulting function is shorter and simpler.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 89 +++
1 file changed, 31 insertions(+), 58 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 525458551a22..7fa734991db9 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -752,74 +752,48 @@ static void close_delivered(USER_SERVICE_T *user_service)
}
struct vchiq_io_copy_callback_context {
- struct vchiq_element *current_element;
- size_t current_element_offset;
+ struct vchiq_element *element;
+ size_t element_offset;
unsigned long elements_to_go;
- size_t current_offset;
};
-static ssize_t
-vchiq_ioc_copy_element_data(
- void *context,
- void *dest,
- size_t offset,
- size_t maxsize)
+static ssize_t vchiq_ioc_copy_element_data(void *context, void *dest,
+ size_t offset, size_t maxsize)
{
- long res;
+ struct vchiq_io_copy_callback_context *cc = context;
+ size_t total_bytes_copied = 0;
size_t bytes_this_round;
- struct vchiq_io_copy_callback_context *copy_context =
- (struct vchiq_io_copy_callback_context *)context;
-
- if (offset != copy_context->current_offset)
- return 0;
-
- if (!copy_context->elements_to_go)
- return 0;
-
- /*
-* Complex logic here to handle the case of 0 size elements
-* in the middle of the array of elements.
-*
-* Need to skip over these 0 size elements.
-*/
- while (1) {
- bytes_this_round = min(copy_context->current_element->size -
- copy_context->current_element_offset,
- maxsize);
-
- if (bytes_this_round)
- break;
- copy_context->elements_to_go--;
- copy_context->current_element++;
- copy_context->current_element_offset = 0;
+ while (total_bytes_copied < maxsize) {
+ if (!cc->elements_to_go)
+ return total_bytes_copied;
- if (!copy_context->elements_to_go)
- return 0;
- }
+ if (!cc->element->size) {
+ cc->elements_to_go--;
+ cc->element++;
+ cc->element_offset = 0;
+ continue;
+ }
- res = copy_from_user(dest,
-copy_context->current_element->data +
-copy_context->current_element_offset,
-bytes_this_round);
+ bytes_this_round = min(cc->element->size - cc->element_offset,
+ maxsize - total_bytes_copied);
- if (res != 0)
- return -EFAULT;
+ if (copy_from_user(dest + total_bytes_copied,
+ cc->element->data + cc->element_offset,
+ bytes_this_round))
+ return -EFAULT;
- copy_context->current_element_offset += bytes_this_round;
- copy_context->current_offset += bytes_this_round;
+ cc->element_offset += bytes_this_round;
+ total_bytes_copied += bytes_this_round;
- /*
-* Check if done with current element, and if so advance to the next.
-*/
- if (copy_context->current_element_offset ==
- copy_context->current_element->size) {
- copy_context->elements_to_go--;
- copy_context->current_element++;
- copy_context->current_element_offset = 0;
+ if (cc->element_offset == cc->element->size) {
+ cc->elements_to_go--;
+ cc->element++;
+ cc->element_offset = 0;
+ }
}
- return b
[PATCH 09/16] staging: vchiq_core: do not initialize semaphores twice
vchiq_init_state() initialises a series of semaphores to then call
remote_event_create() on the same semaphores, which initializes them
again. We get rid of the second initialization.
Signed-off-by: Nicolas Saenz Julienne
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c| 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index dee5ea7bfe4f..8b23ea5322e8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -418,12 +418,11 @@ vchiq_set_conn_state(VCHIQ_STATE_T *state,
VCHIQ_CONNSTATE_T newstate)
}
static inline void
-remote_event_create(VCHIQ_STATE_T *state, REMOTE_EVENT_T *event)
+remote_event_create(REMOTE_EVENT_T *event)
{
event->armed = 0;
/* Don't clear the 'fired' flag because it may already have been set
** by the other side. */
- sema_init((struct semaphore *)((char *)state + event->event), 0);
}
static inline int
@@ -2237,18 +2236,18 @@ vchiq_init_state(VCHIQ_STATE_T *state,
VCHIQ_SLOT_ZERO_T *slot_zero)
state->data_quota = state->slot_queue_available - 1;
local->trigger.event = offsetof(VCHIQ_STATE_T, trigger_event);
- remote_event_create(state, &local->trigger);
+ remote_event_create(&local->trigger);
local->tx_pos = 0;
local->recycle.event = offsetof(VCHIQ_STATE_T, recycle_event);
- remote_event_create(state, &local->recycle);
+ remote_event_create(&local->recycle);
local->slot_queue_recycle = state->slot_queue_available;
local->sync_trigger.event = offsetof(VCHIQ_STATE_T, sync_trigger_event);
- remote_event_create(state, &local->sync_trigger);
+ remote_event_create(&local->sync_trigger);
local->sync_release.event = offsetof(VCHIQ_STATE_T, sync_release_event);
- remote_event_create(state, &local->sync_release);
+ remote_event_create(&local->sync_release);
/* At start-of-day, the slot is empty and available */
((VCHIQ_HEADER_T *)SLOT_DATA_FROM_INDEX(state, local->slot_sync))->msgid
--
2.19.1
[PATCH 07/16] staging: vchiq-core: get rid of is_master distinction
VCHIQ bulk transfers are what most people call DMA transfers. The CPU
sends a list of physical addresses to the VideoCore which then access
the memory directly without the need for CPU interaction. With this
setup we call the CPU the "slave" and the VideoCore the "master".
There seems to be an option to switch roles in vchiq. Which nobody is
using nor is properly implemented. So we get rid of the "is_master == 1"
option, and all the related code.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 12 +-
.../interface/vchiq_arm/vchiq_core.c | 300 +++---
.../interface/vchiq_arm/vchiq_core.h | 11 +-
3 files changed, 38 insertions(+), 285 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 014583cdf367..ecee54a31f8d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -163,7 +163,7 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
- if (vchiq_init_state(state, vchiq_slot_zero, 0) != VCHIQ_SUCCESS)
+ if (vchiq_init_state(state, vchiq_slot_zero) != VCHIQ_SUCCESS)
return -EINVAL;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -278,16 +278,6 @@ vchiq_complete_bulk(VCHIQ_BULK_T *bulk)
bulk->actual);
}
-void
-vchiq_transfer_bulk(VCHIQ_BULK_T *bulk)
-{
- /*
-* This should only be called on the master (VideoCore) side, but
-* provide an implementation to avoid the need for ifdefery.
-*/
- BUG();
-}
-
void
vchiq_dump_platform_state(void *dump_context)
{
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 8c7bda2e7cb6..34a892011296 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -85,8 +85,6 @@ int vchiq_core_log_level = VCHIQ_LOG_DEFAULT;
int vchiq_core_msg_log_level = VCHIQ_LOG_DEFAULT;
int vchiq_sync_log_level = VCHIQ_LOG_DEFAULT;
-static atomic_t pause_bulks_count = ATOMIC_INIT(0);
-
static DEFINE_SPINLOCK(service_spinlock);
DEFINE_SPINLOCK(bulk_waiter_spinlock);
static DEFINE_SPINLOCK(quota_spinlock);
@@ -1222,32 +1220,7 @@ notify_bulks(VCHIQ_SERVICE_T *service,
VCHIQ_BULK_QUEUE_T *queue,
(queue == &service->bulk_tx) ? 't' : 'r',
queue->process, queue->remote_notify, queue->remove);
- if (service->state->is_master) {
- while (queue->remote_notify != queue->process) {
- VCHIQ_BULK_T *bulk =
- &queue->bulks[BULK_INDEX(queue->remote_notify)];
- int msgtype = (bulk->dir == VCHIQ_BULK_TRANSMIT) ?
- VCHIQ_MSG_BULK_RX_DONE : VCHIQ_MSG_BULK_TX_DONE;
- int msgid = VCHIQ_MAKE_MSG(msgtype, service->localport,
- service->remoteport);
- /* Only reply to non-dummy bulk requests */
- if (bulk->remote_data) {
- status = queue_message(
- service->state,
- NULL,
- msgid,
- memcpy_copy_callback,
- &bulk->actual,
- 4,
- 0);
- if (status != VCHIQ_SUCCESS)
- break;
- }
- queue->remote_notify++;
- }
- } else {
- queue->remote_notify = queue->process;
- }
+ queue->remote_notify = queue->process;
if (status == VCHIQ_SUCCESS) {
while (queue->remove != queue->remote_notify) {
@@ -1385,63 +1358,6 @@ poll_services(VCHIQ_STATE_T *state)
}
}
-/* Called by the slot handler or application threads, holding the bulk mutex.
*/
-static int
-resolve_bulks(VCHIQ_SERVICE_T *service, VCHIQ_BULK_QUEUE_T *queue)
-{
- VCHIQ_STATE_T *state = service->state;
- int resolved = 0;
-
- while ((queue->process != queue->local_insert) &&
- (queue->process != queue->remote_insert)) {
- VCHIQ_BULK_T
[PATCH 05/16] staging: vchiq_arm: get rid of vchi_mh.h
The concept of VCHI_MEM_HANDLE_T is introduced by this header file and
was meant to be used with bulk transfers. After a quick look in
vchiq_core.c it is pretty clear that it actually accomplishes nothing
nor alters the bulk transfers in any way.
Signed-off-by: Nicolas Saenz Julienne
---
.../vc04_services/interface/vchi/vchi.h | 3 --
.../vc04_services/interface/vchi/vchi_mh.h| 42 ---
.../interface/vchiq_arm/vchiq_2835_arm.c | 6 +--
.../interface/vchiq_arm/vchiq_arm.c | 27 ++--
.../interface/vchiq_arm/vchiq_core.c | 17
.../interface/vchiq_arm/vchiq_core.h | 10 ++---
.../interface/vchiq_arm/vchiq_if.h| 8 ++--
7 files changed, 27 insertions(+), 86 deletions(-)
delete mode 100644 drivers/staging/vc04_services/interface/vchi/vchi_mh.h
diff --git a/drivers/staging/vc04_services/interface/vchi/vchi.h
b/drivers/staging/vc04_services/interface/vchi/vchi.h
index 379a16ebfd5b..e326926eac31 100644
--- a/drivers/staging/vc04_services/interface/vchi/vchi.h
+++ b/drivers/staging/vc04_services/interface/vchi/vchi.h
@@ -36,7 +36,6 @@
#include "interface/vchi/vchi_cfg.h"
#include "interface/vchi/vchi_common.h"
-#include "vchi_mh.h"
/**
Global defs
@@ -239,7 +238,6 @@ extern int32_t
vchi_bulk_queue_receive(VCHI_SERVICE_HANDLE_T handle,
// Prepare interface for a transfer from the other side into relocatable
memory.
int32_t vchi_bulk_queue_receive_reloc(const VCHI_SERVICE_HANDLE_T handle,
- VCHI_MEM_HANDLE_T h_dst,
uint32_t offset,
uint32_t data_size,
const VCHI_FLAGS_T flags,
@@ -261,7 +259,6 @@ extern int32_t
vchi_bulk_queue_transmit(VCHI_SERVICE_HANDLE_T handle,
#endif
extern int32_t vchi_bulk_queue_transmit_reloc(VCHI_SERVICE_HANDLE_T handle,
- VCHI_MEM_HANDLE_T h_src,
uint32_t offset,
uint32_t data_size,
VCHI_FLAGS_T flags,
diff --git a/drivers/staging/vc04_services/interface/vchi/vchi_mh.h
b/drivers/staging/vc04_services/interface/vchi/vchi_mh.h
deleted file mode 100644
index 198bd076b666..
--- a/drivers/staging/vc04_services/interface/vchi/vchi_mh.h
+++ /dev/null
@@ -1,42 +0,0 @@
-/**
- * Copyright (c) 2010-2012 Broadcom. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *notice, this list of conditions, and the following disclaimer,
- *without modification.
- * 2. Redistributions in binary form must reproduce the above copyright
- *notice, this list of conditions and the following disclaimer in the
- *documentation and/or other materials provided with the distribution.
- * 3. The names of the above-listed copyright holders may not be used
- *to endorse or promote products derived from this software without
- *specific prior written permission.
- *
- * ALTERNATIVELY, this software may be distributed under the terms of the
- * GNU General Public License ("GPL") version 2, as published by the Free
- * Software Foundation.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
- * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
- * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
- * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
- * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#ifndef VCHI_MH_H_
-#define VCHI_MH_H_
-
-#include
-
-typedef int32_t VCHI_MEM_HANDLE_T;
-#define VCHI_MEM_HANDLE_INVALID 0
-
-#endif
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 83d740feab96..014583cdf367 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -247,13 +247,10 @@ remote_event_signal(REMOTE_EVENT_T *event)
}
VCHIQ_STATUS_T
-vchiq_prepare_bulk_data(VCHIQ_BULK_T *bulk, VCHI
[PATCH 10/16] staging: vchiq_core: don't add a wmb() before remote_event_signal()
It's the first thing remote_event_signal() does.
Signed-off-by: Nicolas Saenz Julienne
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.c| 4
1 file changed, 4 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 8b23ea5322e8..5791c2b670fa 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1137,9 +1137,6 @@ queue_message_sync(VCHIQ_STATE_T *state, VCHIQ_SERVICE_T
*service,
size);
}
- /* Make sure the new header is visible to the peer. */
- wmb();
-
remote_event_signal(&state->remote->sync_trigger);
if (VCHIQ_MSG_TYPE(msgid) != VCHIQ_MSG_PAUSE)
@@ -3269,7 +3266,6 @@ static void
release_message_sync(VCHIQ_STATE_T *state, VCHIQ_HEADER_T *header)
{
header->msgid = VCHIQ_MSGID_PADDING;
- wmb();
remote_event_signal(&state->remote->sync_release);
}
--
2.19.1
[PATCH 13/16] staging: vchiq_core: fix logic redundancy in parse_open
We update sync to reflect that the firmware version is compatible with
that option. We don't need to check both of them again further down the
code.
Signed-off-by: Nicolas Saenz Julienne
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.c| 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index a45cdd08e209..5ee667d46eb5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1461,9 +1461,7 @@ parse_open(VCHIQ_STATE_T *state, VCHIQ_HEADER_T *header)
service->sync = 0;
/* Acknowledge the OPEN */
- if (service->sync &&
- (state->version_common >=
-VCHIQ_VERSION_SYNCHRONOUS_MODE)) {
+ if (service->sync) {
if (queue_message_sync(
state,
NULL,
--
2.19.1
[PATCH 08/16] staging: vchiq_core: remove unnecessary safety checks in vchiq_init_state
vchiq_init_state() checks the initial contents of slot_zero are correct.
These are set in vchiq_init_slots(), using the same hard-coded defaults
as the checks. Both functions are called sequentially and Video Core
isn't yet aware of the slot's address. There is no way the contents of
slot_zero changed in between functions, making the checks useless.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_core.c | 59 ---
1 file changed, 59 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 34a892011296..dee5ea7bfe4f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2170,65 +2170,6 @@ vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T
*slot_zero)
return VCHIQ_ERROR;
}
- /* Check the input configuration */
-
- if (slot_zero->magic != VCHIQ_MAGIC) {
- vchiq_loud_error_header();
- vchiq_loud_error("Invalid VCHIQ magic value found.");
- vchiq_loud_error("slot_zero=%pK: magic=%x (expected %x)",
- slot_zero, slot_zero->magic, VCHIQ_MAGIC);
- vchiq_loud_error_footer();
- return VCHIQ_ERROR;
- }
-
- if (slot_zero->version < VCHIQ_VERSION_MIN) {
- vchiq_loud_error_header();
- vchiq_loud_error("Incompatible VCHIQ versions found.");
- vchiq_loud_error("slot_zero=%pK: VideoCore version=%d (minimum
%d)",
- slot_zero, slot_zero->version, VCHIQ_VERSION_MIN);
- vchiq_loud_error("Restart with a newer VideoCore image.");
- vchiq_loud_error_footer();
- return VCHIQ_ERROR;
- }
-
- if (VCHIQ_VERSION < slot_zero->version_min) {
- vchiq_loud_error_header();
- vchiq_loud_error("Incompatible VCHIQ versions found.");
- vchiq_loud_error("slot_zero=%pK: version=%d (VideoCore minimum
%d)",
- slot_zero, VCHIQ_VERSION, slot_zero->version_min);
- vchiq_loud_error("Restart with a newer kernel.");
- vchiq_loud_error_footer();
- return VCHIQ_ERROR;
- }
-
- if ((slot_zero->slot_zero_size != sizeof(VCHIQ_SLOT_ZERO_T)) ||
-(slot_zero->slot_size != VCHIQ_SLOT_SIZE) ||
-(slot_zero->max_slots != VCHIQ_MAX_SLOTS) ||
-(slot_zero->max_slots_per_side != VCHIQ_MAX_SLOTS_PER_SIDE)) {
- vchiq_loud_error_header();
- if (slot_zero->slot_zero_size != sizeof(VCHIQ_SLOT_ZERO_T))
- vchiq_loud_error("slot_zero=%pK: slot_zero_size=%d
(expected %d)",
- slot_zero, slot_zero->slot_zero_size,
- (int)sizeof(VCHIQ_SLOT_ZERO_T));
- if (slot_zero->slot_size != VCHIQ_SLOT_SIZE)
- vchiq_loud_error("slot_zero=%pK: slot_size=%d (expected
%d)",
- slot_zero, slot_zero->slot_size,
- VCHIQ_SLOT_SIZE);
- if (slot_zero->max_slots != VCHIQ_MAX_SLOTS)
- vchiq_loud_error("slot_zero=%pK: max_slots=%d (expected
%d)",
- slot_zero, slot_zero->max_slots,
- VCHIQ_MAX_SLOTS);
- if (slot_zero->max_slots_per_side != VCHIQ_MAX_SLOTS_PER_SIDE)
- vchiq_loud_error("slot_zero=%pK: max_slots_per_side=%d
(expected %d)",
- slot_zero, slot_zero->max_slots_per_side,
- VCHIQ_MAX_SLOTS_PER_SIDE);
- vchiq_loud_error_footer();
- return VCHIQ_ERROR;
- }
-
- if (VCHIQ_VERSION < slot_zero->version)
- slot_zero->version = VCHIQ_VERSION;
-
local = &slot_zero->slave;
remote = &slot_zero->master;
--
2.19.1
[PATCH 12/16] staging: vchiq_util: get rid of unneeded memory barriers
All the memory operations featured in this file modify/access memory that is only accessed by the CPU. So we can assume that all the memory barrier handling done by the completion routines is good enough for us. Signed-off-by: Nicolas Saenz Julienne --- .../interface/vchiq_arm/vchiq_util.c | 32 --- 1 file changed, 32 deletions(-) diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c index 44b954daa74a..4b8554bc647e 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c @@ -84,20 +84,7 @@ void vchiu_queue_push(VCHIU_QUEUE_T *queue, VCHIQ_HEADER_T *header) flush_signals(current); } - /* -* Write to queue->storage must be visible after read from -* queue->read -*/ - smp_mb(); - queue->storage[queue->write & (queue->size - 1)] = header; - - /* -* Write to queue->storage must be visible before write to -* queue->write -*/ - smp_wmb(); - queue->write++; complete(&queue->push); @@ -112,12 +99,6 @@ VCHIQ_HEADER_T *vchiu_queue_peek(VCHIU_QUEUE_T *queue) complete(&queue->push); // We haven't removed anything from the queue. - /* -* Read from queue->storage must be visible after read from -* queue->write -*/ - smp_rmb(); - return queue->storage[queue->read & (queue->size - 1)]; } @@ -130,20 +111,7 @@ VCHIQ_HEADER_T *vchiu_queue_pop(VCHIU_QUEUE_T *queue) flush_signals(current); } - /* -* Read from queue->storage must be visible after read from -* queue->write -*/ - smp_rmb(); - header = queue->storage[queue->read & (queue->size - 1)]; - - /* -* Read from queue->storage must be visible before write to -* queue->read -*/ - smp_mb(); - queue->read++; complete(&queue->pop); -- 2.19.1
[PATCH 11/16] staging: vchiq: use completions instead of semaphores
It is preferred in the kernel to avoid using semaphores to wait for
events, as they are optimised for the opposite situation; where the
common case is that they are available and may block only occasionally.
FYI see this thread: https://lkml.org/lkml/2008/4/11/323.
Also completions are semantically more explicit in this case.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 60 ++-
.../interface/vchiq_arm/vchiq_core.c | 100 +-
.../interface/vchiq_arm/vchiq_core.h | 26 ++---
.../interface/vchiq_arm/vchiq_util.c | 16 +--
.../interface/vchiq_arm/vchiq_util.h | 6 +-
5 files changed, 106 insertions(+), 102 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 7fa734991db9..20359924ed45 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -44,7 +44,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
@@ -121,9 +121,9 @@ typedef struct user_service_struct {
int message_available_pos;
int msg_insert;
int msg_remove;
- struct semaphore insert_event;
- struct semaphore remove_event;
- struct semaphore close_event;
+ struct completion insert_event;
+ struct completion remove_event;
+ struct completion close_event;
VCHIQ_HEADER_T * msg_queue[MSG_QUEUE_SIZE];
} USER_SERVICE_T;
@@ -138,8 +138,8 @@ struct vchiq_instance_struct {
VCHIQ_COMPLETION_DATA_T completions[MAX_COMPLETIONS];
int completion_insert;
int completion_remove;
- struct semaphore insert_event;
- struct semaphore remove_event;
+ struct completion insert_event;
+ struct completion remove_event;
struct mutex completion_mutex;
int connected;
@@ -562,7 +562,8 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T
reason,
vchiq_log_trace(vchiq_arm_log_level,
"%s - completion queue full", __func__);
DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
- if (down_interruptible(&instance->remove_event) != 0) {
+ if (wait_for_completion_interruptible(
+ &instance->remove_event)) {
vchiq_log_info(vchiq_arm_log_level,
"service_callback interrupted");
return VCHIQ_RETRY;
@@ -600,7 +601,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T
reason,
insert++;
instance->completion_insert = insert;
- up(&instance->insert_event);
+ complete(&instance->insert_event);
return VCHIQ_SUCCESS;
}
@@ -673,7 +674,8 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T
*header,
}
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
- if (down_interruptible(&user_service->remove_event)
+ if (wait_for_completion_interruptible(
+ &user_service->remove_event)
!= 0) {
vchiq_log_info(vchiq_arm_log_level,
"%s interrupted", __func__);
@@ -705,7 +707,7 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T
*header,
}
spin_unlock(&msg_queue_spinlock);
- up(&user_service->insert_event);
+ complete(&user_service->insert_event);
header = NULL;
}
@@ -745,7 +747,7 @@ static void close_delivered(USER_SERVICE_T *user_service)
unlock_service(user_service->service);
/* Wake the user-thread blocked in close_ or remove_service */
- up(&user_service->close_event);
+ complete(&user_service->close_event);
user_service->close_pending = 0;
}
@@ -867,7 +869,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
if (status == VCHIQ_SUCCESS) {
/* Wake the completion thread and ask it to exit */
instance->closing = 1;
- up(&instance->insert_event);
+ complete(&instance->insert_event);
}
break;
@@ -948,9 +950,9 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
instance->completion_remove - 1;
user_service->msg_insert = 0;
user_service->msg_remove = 0;
- sema
[PATCH 16/16] staging: vchiq: add more tasks to the TODO list
The TODO list was missing some tasks needed before upstreaming the device. Signed-off-by: Nicolas Saenz Julienne --- .../staging/vc04_services/interface/vchi/TODO | 42 +++ 1 file changed, 42 insertions(+) diff --git a/drivers/staging/vc04_services/interface/vchi/TODO b/drivers/staging/vc04_services/interface/vchi/TODO index 0b3ec75ff627..fc2752bc95b2 100644 --- a/drivers/staging/vc04_services/interface/vchi/TODO +++ b/drivers/staging/vc04_services/interface/vchi/TODO @@ -49,3 +49,45 @@ such as dev_info, dev_dbg, and friends. A short top-down description of this driver's architecture (function of kthreads, userspace, limitations) could be very helpful for reviewers. + +7) Review and comment memory barriers + +There is a heavy use of memory barriers in this driver, it would be very +beneficial to go over all of them and, if correct, comment on their merits. +Extra points to whomever confidently reviews the remote_event_*() family of +functions. + +8) Get rid of custom function return values + +Most functions use a custom set of return values, we should force proper Linux +error numbers. Special care is needed for VCHIQ_RETRY. + +9) Reformat core code with more sane indentations + +The code follows the 80 characters limitation yet tends to go 3 or 4 levels of +indentation deep making it very unpleasant to read. This is specially relevant +in the character driver ioctl code and in the core thread functions. + +10) Reorganize file structure: Move char driver to it's own file and join both +platform files + +The cdev is defined alongside with the platform code in vchiq_arm.c. It would +be nice to completely decouple it from the actual core code. For instance to be +able to use bcm2835-audio without having /dev/vchiq created. One could argue +it's better for security reasons or general cleanliness. It could even be +interesting to create two different kernel modules, something the likes of +vchiq-core.ko and vchiq-dev.ko. This would also ease the upstreaming process. + +The code in vchiq_bcm2835_arm.c should fit in the generic platform file. + +12) Get rid of all the struct typedefs + +Most structs are typedefd, it's not encouraged in the kernel. + +13) Get rid of all non essential global structures and create a proper per +device structure + +The first thing one generally sees in a probe function is a memory allocation +for all the device specific data. This structure is then passed all over the +driver. This is good practice since it makes the driver work regardless of the +number of devices probed. -- 2.19.1
[PATCH 03/16] staging: vchiq_shim: delete vchi_service_create
No one is using the API neither in the actual staging tree nor in the
downstream tree (https://github.com/raspberrypi/linux).
Signed-off-by: Nicolas Saenz Julienne
---
.../vc04_services/interface/vchi/vchi.h | 5 ---
.../interface/vchiq_arm/vchiq_shim.c | 32 ---
2 files changed, 37 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchi/vchi.h
b/drivers/staging/vc04_services/interface/vchi/vchi.h
index 01381904775d..379a16ebfd5b 100644
--- a/drivers/staging/vc04_services/interface/vchi/vchi.h
+++ b/drivers/staging/vc04_services/interface/vchi/vchi.h
@@ -113,11 +113,6 @@ extern uint32_t vchi_current_time(VCHI_INSTANCE_T
instance_handle);
/**
Global service API
*/
-// Routine to create a named service
-extern int32_t vchi_service_create(VCHI_INSTANCE_T instance_handle,
- SERVICE_CREATION_T *setup,
- VCHI_SERVICE_HANDLE_T *handle);
-
// Routine to destroy a service
extern int32_t vchi_service_destroy(const VCHI_SERVICE_HANDLE_T handle);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
index c3223fcdaf87..81cac68f4b78 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
@@ -660,38 +660,6 @@ int32_t vchi_service_open(VCHI_INSTANCE_T instance_handle,
}
EXPORT_SYMBOL(vchi_service_open);
-int32_t vchi_service_create(VCHI_INSTANCE_T instance_handle,
- SERVICE_CREATION_T *setup,
- VCHI_SERVICE_HANDLE_T *handle)
-{
- VCHIQ_INSTANCE_T instance = (VCHIQ_INSTANCE_T)instance_handle;
- struct shim_service *service = service_alloc(instance, setup);
-
- *handle = (VCHI_SERVICE_HANDLE_T)service;
-
- if (service) {
- VCHIQ_SERVICE_PARAMS_T params;
- VCHIQ_STATUS_T status;
-
- memset(¶ms, 0, sizeof(params));
- params.fourcc = setup->service_id;
- params.callback = shim_callback;
- params.userdata = service;
- params.version = setup->version.version;
- params.version_min = setup->version.version_min;
- status = vchiq_add_service(instance, ¶ms, &service->handle);
-
- if (status != VCHIQ_SUCCESS) {
- service_free(service);
- service = NULL;
- *handle = NULL;
- }
- }
-
- return (service != NULL) ? 0 : -1;
-}
-EXPORT_SYMBOL(vchi_service_create);
-
int32_t vchi_service_close(const VCHI_SERVICE_HANDLE_T handle)
{
int32_t ret = -1;
--
2.19.1
[PATCH 14/16] staging: vchiq_arm: rework probe and init functions
Moves the allocation of a chardev region and class creation to the init
function of the driver since those functions are meant to be run on a
per driver basis, as opposed to the code run in the probe function which
is run in a per device basis.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 71 ---
1 file changed, 45 insertions(+), 26 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 20359924ed45..03cc6947c03c 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -166,7 +166,6 @@ static struct cdevvchiq_cdev;
static dev_t vchiq_devid;
static VCHIQ_STATE_T g_state;
static struct class *vchiq_class;
-static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
@@ -3557,34 +3556,19 @@ static int vchiq_probe(struct platform_device *pdev)
if (err != 0)
goto failed_platform_init;
- err = alloc_chrdev_region(&vchiq_devid, VCHIQ_MINOR, 1, DEVICE_NAME);
- if (err != 0) {
- vchiq_log_error(vchiq_arm_log_level,
- "Unable to allocate device number");
- goto failed_platform_init;
- }
cdev_init(&vchiq_cdev, &vchiq_fops);
vchiq_cdev.owner = THIS_MODULE;
err = cdev_add(&vchiq_cdev, vchiq_devid, 1);
if (err != 0) {
vchiq_log_error(vchiq_arm_log_level,
"Unable to register device");
- goto failed_cdev_add;
+ goto failed_platform_init;
}
- /* create sysfs entries */
- vchiq_class = class_create(THIS_MODULE, DEVICE_NAME);
- err = PTR_ERR(vchiq_class);
- if (IS_ERR(vchiq_class))
- goto failed_class_create;
-
- vchiq_dev = device_create(vchiq_class, NULL,
- vchiq_devid, NULL, "vchiq");
- err = PTR_ERR(vchiq_dev);
- if (IS_ERR(vchiq_dev))
+ if (IS_ERR(device_create(vchiq_class, &pdev->dev, vchiq_devid,
+NULL, "vchiq")))
goto failed_device_create;
- /* create debugfs entries */
vchiq_debugfs_init();
vchiq_log_info(vchiq_arm_log_level,
@@ -3599,11 +3583,7 @@ static int vchiq_probe(struct platform_device *pdev)
return 0;
failed_device_create:
- class_destroy(vchiq_class);
-failed_class_create:
cdev_del(&vchiq_cdev);
-failed_cdev_add:
- unregister_chrdev_region(vchiq_devid, 1);
failed_platform_init:
vchiq_log_warning(vchiq_arm_log_level, "could not load vchiq");
return err;
@@ -3614,9 +3594,7 @@ static int vchiq_remove(struct platform_device *pdev)
platform_device_unregister(bcm2835_camera);
vchiq_debugfs_deinit();
device_destroy(vchiq_class, vchiq_devid);
- class_destroy(vchiq_class);
cdev_del(&vchiq_cdev);
- unregister_chrdev_region(vchiq_devid, 1);
return 0;
}
@@ -3629,7 +3607,48 @@ static struct platform_driver vchiq_driver = {
.probe = vchiq_probe,
.remove = vchiq_remove,
};
-module_platform_driver(vchiq_driver);
+
+static int __init vchiq_driver_init(void)
+{
+ int ret;
+
+ vchiq_class = class_create(THIS_MODULE, DEVICE_NAME);
+ if (IS_ERR(vchiq_class)) {
+ pr_err("Failed to create vchiq class\n");
+ return PTR_ERR(vchiq_class);
+ }
+
+ ret = alloc_chrdev_region(&vchiq_devid, VCHIQ_MINOR, 1, DEVICE_NAME);
+ if (ret) {
+ pr_err("Failed to allocate vchiq's chrdev region\n");
+ goto class_destroy;
+ }
+
+ ret = platform_driver_register(&vchiq_driver);
+ if (ret) {
+ pr_err("Failed to register vchiq driver\n");
+ goto region_unregister;
+ }
+
+ return 0;
+
+region_unregister:
+ platform_driver_unregister(&vchiq_driver);
+
+class_destroy:
+ class_destroy(vchiq_class);
+
+ return ret;
+}
+module_init(vchiq_driver_init);
+
+static void __exit vchiq_driver_exit(void)
+{
+ platform_driver_unregister(&vchiq_driver);
+ unregister_chrdev_region(vchiq_devid, 1);
+ class_destroy(vchiq_class);
+}
+module_exit(vchiq_driver_exit);
MODULE_LICENSE("Dual BSD/GPL");
MODULE_DESCRIPTION("Videocore VCHIQ driver");
--
2.19.1
[PATCH 02/16] staging: vchiq_arm: rework close/remove_service IOCTLS
The implementation of both IOCTLS was the same except for one function
call. This joins both implementations and updates the code to avoid
unneeded indentations.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 66 +++
1 file changed, 24 insertions(+), 42 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 5af3f2651bd3..835152799433 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1019,55 +1019,37 @@ vchiq_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
}
} break;
- case VCHIQ_IOC_CLOSE_SERVICE: {
+ case VCHIQ_IOC_CLOSE_SERVICE:
+ case VCHIQ_IOC_REMOVE_SERVICE: {
VCHIQ_SERVICE_HANDLE_T handle = (VCHIQ_SERVICE_HANDLE_T)arg;
+ USER_SERVICE_T *user_service;
service = find_service_for_instance(instance, handle);
- if (service != NULL) {
- USER_SERVICE_T *user_service =
- (USER_SERVICE_T *)service->base.userdata;
- /* close_pending is false on first entry, and when the
- wait in vchiq_close_service has been interrupted. */
- if (!user_service->close_pending) {
- status = vchiq_close_service(service->handle);
- if (status != VCHIQ_SUCCESS)
- break;
- }
-
- /* close_pending is true once the underlying service
- has been closed until the client library calls the
- CLOSE_DELIVERED ioctl, signalling close_event. */
- if (user_service->close_pending &&
- down_interruptible(&user_service->close_event))
- status = VCHIQ_RETRY;
- } else
+ if (!service) {
ret = -EINVAL;
- } break;
+ break;
+ }
- case VCHIQ_IOC_REMOVE_SERVICE: {
- VCHIQ_SERVICE_HANDLE_T handle = (VCHIQ_SERVICE_HANDLE_T)arg;
+ user_service = service->base.userdata;
- service = find_service_for_instance(instance, handle);
- if (service != NULL) {
- USER_SERVICE_T *user_service =
- (USER_SERVICE_T *)service->base.userdata;
- /* close_pending is false on first entry, and when the
- wait in vchiq_close_service has been interrupted. */
- if (!user_service->close_pending) {
- status = vchiq_remove_service(service->handle);
- if (status != VCHIQ_SUCCESS)
- break;
- }
+ /* close_pending is false on first entry, and when the
+ wait in vchiq_close_service has been interrupted. */
+ if (!user_service->close_pending) {
+ status = (cmd == VCHIQ_IOC_CLOSE_SERVICE) ?
+vchiq_close_service(service->handle) :
+vchiq_remove_service(service->handle);
+ if (status != VCHIQ_SUCCESS)
+ break;
+ }
- /* close_pending is true once the underlying service
- has been closed until the client library calls the
- CLOSE_DELIVERED ioctl, signalling close_event. */
- if (user_service->close_pending &&
- down_interruptible(&user_service->close_event))
- status = VCHIQ_RETRY;
- } else
- ret = -EINVAL;
- } break;
+ /* close_pending is true once the underlying service
+ has been closed until the client library calls the
+ CLOSE_DELIVERED ioctl, signalling close_event. */
+ if (user_service->close_pending &&
+ down_interruptible(&user_service->close_event))
+ status = VCHIQ_RETRY;
+ break;
+ }
case VCHIQ_IOC_USE_SERVICE:
case VCHIQ_IOC_RELEASE_SERVICE: {
--
2.19.1
[PATCH 15/16] staging: vchiq_arm: fix open/release cdev functions
Both functions checked the minor number of the cdev prior running the
code. This was useless since the number of devices is already limited by
alloc_chrdev_region.
This removes the check and reindents the code where relevant.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 247 +++---
1 file changed, 100 insertions(+), 147 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 03cc6947c03c..0caee2d6946f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -63,8 +63,6 @@
#undef MODULE_PARAM_PREFIX
#define MODULE_PARAM_PREFIX DEVICE_NAME "."
-#define VCHIQ_MINOR 0
-
/* Some per-instance constants */
#define MAX_COMPLETIONS 128
#define MAX_SERVICES 64
@@ -1955,195 +1953,150 @@ vchiq_compat_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
#endif
-/
-*
-* vchiq_open
-*
-***/
-
-static int
-vchiq_open(struct inode *inode, struct file *file)
+static int vchiq_open(struct inode *inode, struct file *file)
{
- int dev = iminor(inode) & 0x0f;
+ VCHIQ_STATE_T *state = vchiq_get_state();
+ VCHIQ_INSTANCE_T instance;
vchiq_log_info(vchiq_arm_log_level, "vchiq_open");
- switch (dev) {
- case VCHIQ_MINOR: {
- VCHIQ_STATE_T *state = vchiq_get_state();
- VCHIQ_INSTANCE_T instance;
- if (!state) {
- vchiq_log_error(vchiq_arm_log_level,
+ if (!state) {
+ vchiq_log_error(vchiq_arm_log_level,
"vchiq has no connection to VideoCore");
- return -ENOTCONN;
- }
-
- instance = kzalloc(sizeof(*instance), GFP_KERNEL);
- if (!instance)
- return -ENOMEM;
+ return -ENOTCONN;
+ }
- instance->state = state;
- instance->pid = current->tgid;
+ instance = kzalloc(sizeof(*instance), GFP_KERNEL);
+ if (!instance)
+ return -ENOMEM;
- vchiq_debugfs_add_instance(instance);
+ instance->state = state;
+ instance->pid = current->tgid;
- init_completion(&instance->insert_event);
- init_completion(&instance->remove_event);
- mutex_init(&instance->completion_mutex);
- mutex_init(&instance->bulk_waiter_list_mutex);
- INIT_LIST_HEAD(&instance->bulk_waiter_list);
+ vchiq_debugfs_add_instance(instance);
- file->private_data = instance;
- } break;
+ init_completion(&instance->insert_event);
+ init_completion(&instance->remove_event);
+ mutex_init(&instance->completion_mutex);
+ mutex_init(&instance->bulk_waiter_list_mutex);
+ INIT_LIST_HEAD(&instance->bulk_waiter_list);
- default:
- vchiq_log_error(vchiq_arm_log_level,
- "Unknown minor device: %d", dev);
- return -ENXIO;
- }
+ file->private_data = instance;
return 0;
}
-/
-*
-* vchiq_release
-*
-***/
-
-static int
-vchiq_release(struct inode *inode, struct file *file)
+static int vchiq_release(struct inode *inode, struct file *file)
{
- int dev = iminor(inode) & 0x0f;
+ VCHIQ_INSTANCE_T instance = file->private_data;
+ VCHIQ_STATE_T *state = vchiq_get_state();
+ VCHIQ_SERVICE_T *service;
int ret = 0;
+ int i;
- switch (dev) {
- case VCHIQ_MINOR: {
- VCHIQ_INSTANCE_T instance = file->private_data;
- VCHIQ_STATE_T *state = vchiq_get_state();
- VCHIQ_SERVICE_T *service;
- int i;
+ vchiq_log_info(vchiq_arm_log_level, "%s: instance=%lx", __func__,
+ (unsigned long)instance);
- vchiq_log_info(vchiq_arm_log_level,
- "%s: instance=%lx",
- __func__, (unsigned long)instance);
+ if (!state) {
+ ret = -EPERM;
+ goto out;
+ }
- if (!state) {
- ret = -EPERM;
- goto out;
- }
+ /* Ensure videocore is awake to allow termination. */
+ vchiq_use_internal(instance->state, NULL, USE_TYPE_VCHIQ);
- /* Ensur
[PATCH v3] usb: hub: add retry routine after intr URB submit error
The hub sends hot-plug events to the host trough it's interrupt URB. The
driver takes care of completing the URB and re-submitting it. Completion
errors are handled in the hub_event() work, yet submission errors are
ignored, rendering the device unresponsive. All further events are lost.
It is fairly hard to find this issue in the wild, since you have to time
the USB hot-plug event with the URB submission failure. For instance it
could be the system running out of memory or some malfunction in the USB
controller driver. Nevertheless, it's pretty reasonable to think it'll
happen sometime. One can trigger this issue using eBPF's function
override feature (see BCC's inject.py script).
This patch adds a retry routine to the event of a submission error. The
HUB driver will try to re-submit the URB once every second until it's
successful or the HUB is disconnected.
As some USB subsystems already take care of this issue, the
implementation was inspired from usbhid/hid_core.c's.
Signed-off-by: Nicolas Saenz Julienne
---
v3: As per Oliver's request:
- Take care of race condition between disconnect and irq
v2: as per Alan's and Oliver's comments:
- Rename timer
- Delete the timer on disconnect
- Don't reset HUB nor exponential slowdown the timer, 1s fixed retry
period
- Check for -ESHUTDOWN prior kicking the timer
drivers/usb/core/hub.c | 45 --
drivers/usb/core/hub.h | 2 ++
2 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c6077d582d29..630490a35301 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -607,6 +607,38 @@ static int hub_port_status(struct usb_hub *hub, int port1,
status, change, NULL);
}
+static void hub_resubmit_irq_urb(struct usb_hub *hub)
+{
+ unsigned long flags;
+ int status;
+
+ spin_lock_irqsave(&hub->irq_urb_lock, flags);
+
+ if (hub->quiescing) {
+ spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
+ return;
+ }
+
+ status = usb_submit_urb(hub->urb, GFP_ATOMIC);
+ if (status && status != -ENODEV && status != -EPERM &&
+ status != -ESHUTDOWN) {
+ dev_err(hub->intfdev, "resubmit --> %d\n", status);
+ mod_timer(&hub->irq_urb_retry,
+ jiffies + msecs_to_jiffies(MSEC_PER_SEC));
+
+ }
+
+ spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
+}
+
+static void hub_retry_irq_urb(struct timer_list *t)
+{
+ struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
+
+ hub_resubmit_irq_urb(hub);
+}
+
+
static void kick_hub_wq(struct usb_hub *hub)
{
struct usb_interface *intf;
@@ -709,12 +741,7 @@ static void hub_irq(struct urb *urb)
kick_hub_wq(hub);
resubmit:
- if (hub->quiescing)
- return;
-
- status = usb_submit_urb(hub->urb, GFP_ATOMIC);
- if (status != 0 && status != -ENODEV && status != -EPERM)
- dev_err(hub->intfdev, "resubmit --> %d\n", status);
+ hub_resubmit_irq_urb(hub);
}
/* USB 2.0 spec Section 11.24.2.3 */
@@ -1254,10 +1281,13 @@ enum hub_quiescing_type {
static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
{
struct usb_device *hdev = hub->hdev;
+ unsigned long flags;
int i;
/* hub_wq and related activity won't re-trigger */
+ spin_lock_irqsave(&hub->irq_urb_lock, flags);
hub->quiescing = 1;
+ spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
if (type != HUB_SUSPEND) {
/* Disconnect all the children */
@@ -1268,6 +1298,7 @@ static void hub_quiesce(struct usb_hub *hub, enum
hub_quiescing_type type)
}
/* Stop hub_wq and related activity */
+ del_timer_sync(&hub->irq_urb_retry);
usb_kill_urb(hub->urb);
if (hub->has_indicators)
cancel_delayed_work_sync(&hub->leds);
@@ -1800,6 +1831,8 @@ static int hub_probe(struct usb_interface *intf, const
struct usb_device_id *id)
INIT_DELAYED_WORK(&hub->leds, led_work);
INIT_DELAYED_WORK(&hub->init_work, NULL);
INIT_WORK(&hub->events, hub_event);
+ spin_lock_init(&hub->irq_urb_lock);
+ timer_setup(&hub->irq_urb_retry, hub_retry_irq_urb, 0);
usb_get_intf(intf);
usb_get_dev(hdev);
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 4accfb63f7dc..a9e24e4b8df1 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -69,6 +69,8 @@ struct usb_hub {
struct delayed_work leds;
struct delayed_work init_work;
struct work_struct events;
+ spinlock_t irq_urb_lock;
+ struct timer_list irq_urb_retry;
struct usb_port **ports;
};
--
2.19.1
Re: [PATCH 00/16] staging: vchiq: dead code removal & misc fixes
On Tue, 2018-11-20 at 10:57 +0100, Greg KH wrote: > On Sun, Nov 18, 2018 at 04:55:49PM +0100, Stefan Wahren wrote: > > Hi Nicolas, > > > > > Nicolas Saenz Julienne hat am 14. > > > November 2018 um 13:59 geschrieben: > > > > > > > > > Hi All, > > > > > > This series was written in parallel with reading and > > > understanding the > > > vchiq code. So excuse me for the lack of logic in the sequence of > > > patches. > > > > > > The main focus was to delete as much code as possible, I've > > > counted > > > around 550 lines, which is not bad. Apart from that there are > > > some > > > patches enforcing proper kernel APIs usage. > > > > > > The only patch that really changes code is the > > > vchiq_ioc_copy_element_data() rewrite. > > > > > > The last commit updates the TODO list with some of my > > > observations, I > > > realise some of the might be a little opinionated. If anything > > > it's > > > going to force a discussion on the topic, which is nice. > > > > > > It was developed on top of the latest linux-next, and was tested > > > on a > > > RPIv3B+ with audio, video and running vchiq_test. > > > > > > Regards, > > > Nicolas > > > > > > > without a changelog i won't start a review. > > What do you mean by this? The individual patches have a "changelog" > in > them, this is just a summary of the overall changes. > > What do you feel is missing here? Hi Greg, I did send an RFC version of this, to which Stephan made several comments. I should have added the changelog to the proper series submission. I've been busy with work, but I'll send the updated version during the day. Regards, Nicolas > > thanks, > > greg k-h signature.asc Description: This is a digitally signed message part
Re: [PATCH v2] usb: hub: add retry routine after intr URB sumbit error
Hi Oliver,
thanks for the reviews :).
On Mon, 2018-11-19 at 15:04 +0100, Oliver Neukum wrote:
> On Mo, 2018-11-19 at 15:02 +0100, Nicolas Saenz Julienne wrote:
> > +static void hub_retry_irq_urb(struct timer_list *t)
> > +{
> > + struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
> > + int status;
> > +
> > + if (hub->disconnected || hub->quiescing)
> > + return;
> > +
> > + dev_err(hub->intfdev, "retrying int urb\n");
> > + status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> > + if (status && status != -ENODEV && status != -EPERM &&
> > + status != -ESHUTDOWN)
> > + mod_timer(&hub->irq_urb_retry,
> > + jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> > +}
> > +
> > static void kick_hub_wq(struct usb_hub *hub)
> > {
> > struct usb_interface *intf;
> > @@ -713,8 +729,12 @@ static void hub_irq(struct urb *urb)
> > return;
> >
> > status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> > - if (status != 0 && status != -ENODEV && status != -EPERM)
> > + if (status != 0 && status != -ENODEV && status != -EPERM &&
> > + status != -ESHUTDOWN) {
> > dev_err(hub->intfdev, "resubmit --> %d\n", status);
> > + mod_timer(&hub->irq_urb_retry,
> > + jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> > + }
> > }
> >
> > /* USB 2.0 spec Section 11.24.2.3 */
> > @@ -1268,6 +1288,7 @@ static void hub_quiesce(struct usb_hub *hub,
> > enum hub_quiescing_type type)
> > }
> >
> > /* Stop hub_wq and related activity */
> > + del_timer_sync(&hub->irq_urb_retry);
>
> That is a race condition. You kill the timer here, but the URB may
> still be in flight. And if it fails, it will restart the error
> handler. You have to introduce a flag or poison the URB.
I see, wouldn't checking "hub->quiescing" in hub_irq()'s submit error
path do the work? Something the likes of this:
@@ -713,8 +729,12 @@ static void hub_irq(struct urb *urb)
return;
status = usb_submit_urb(hub->urb, GFP_ATOMIC);
- if (status != 0 && status != -ENODEV && status != -EPERM)
+ if (status != 0 && status != -ENODEV && status != -EPERM &&
+ status != -ESHUTDOWN && !hub->quiescing) {
dev_err(hub->intfdev, "resubmit --> %d\n", status);
+ mod_timer(&hub->irq_urb_retry,
+ jiffies + msecs_to_jiffies(MSEC_PER_SEC));
+ }
>
> > usb_kill_urb(hub->urb);
> > if (hub->has_indicators)
> > cancel_delayed_work_sync(&hub->leds);
> >
>
> Regards
> Oliver
>
Also, looking at this made me realize that there is no real need to
check "hub->disconnected" in the timer's function, since "hub-
>quiescing" is also set in that code path before any actual cleanup.
I'll update it in the next revision.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
[PATCH v2] usb: hub: add retry routine after intr URB sumbit error
The hub sends hot-plug events to the host trough it's interrupt URB. The
driver takes care of completing the URB and re-submitting it. Completion
errors are handled in the hub_event() work, yet submission errors are
ignored, rendering the device unresponsive. All further events are lost.
It is fairly hard to find this issue in the wild, since you have to time
the USB hot-plug event with the URB submission failure. For instance it
could be the system running out of memory or some malfunction in the USB
controller driver. Nevertheless, it's pretty reasonable to think it'll
happen sometime. One can trigger this issue using eBPF's function
override feature (see BCC's inject.py script).
This patch adds a retry routine to the event of a submission error. The
HUB driver will try to re-submit the URB once every second until it's
successful or the HUB is disconnected.
As some USB subsystems already take care of this issue, the
implementation was inspired from usbhid/hid_core.c's.
Signed-off-by: Nicolas Saenz Julienne
---
v2: as per Alan's and Oliver's comments:
- Rename timer
- Delete the timer on disconnect
- Don't reset HUB nor exponential slowdown the timer, 1s fixed retry
period
- Check for -ESHUTDOWN prior kicking the timer
drivers/usb/core/hub.c | 24 +++-
drivers/usb/core/hub.h | 1 +
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c6077d582d29..937ece45ea1f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -607,6 +607,22 @@ static int hub_port_status(struct usb_hub *hub, int port1,
status, change, NULL);
}
+static void hub_retry_irq_urb(struct timer_list *t)
+{
+ struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
+ int status;
+
+ if (hub->disconnected || hub->quiescing)
+ return;
+
+ dev_err(hub->intfdev, "retrying int urb\n");
+ status = usb_submit_urb(hub->urb, GFP_ATOMIC);
+ if (status && status != -ENODEV && status != -EPERM &&
+ status != -ESHUTDOWN)
+ mod_timer(&hub->irq_urb_retry,
+ jiffies + msecs_to_jiffies(MSEC_PER_SEC));
+}
+
static void kick_hub_wq(struct usb_hub *hub)
{
struct usb_interface *intf;
@@ -713,8 +729,12 @@ static void hub_irq(struct urb *urb)
return;
status = usb_submit_urb(hub->urb, GFP_ATOMIC);
- if (status != 0 && status != -ENODEV && status != -EPERM)
+ if (status != 0 && status != -ENODEV && status != -EPERM &&
+ status != -ESHUTDOWN) {
dev_err(hub->intfdev, "resubmit --> %d\n", status);
+ mod_timer(&hub->irq_urb_retry,
+ jiffies + msecs_to_jiffies(MSEC_PER_SEC));
+ }
}
/* USB 2.0 spec Section 11.24.2.3 */
@@ -1268,6 +1288,7 @@ static void hub_quiesce(struct usb_hub *hub, enum
hub_quiescing_type type)
}
/* Stop hub_wq and related activity */
+ del_timer_sync(&hub->irq_urb_retry);
usb_kill_urb(hub->urb);
if (hub->has_indicators)
cancel_delayed_work_sync(&hub->leds);
@@ -1800,6 +1821,7 @@ static int hub_probe(struct usb_interface *intf, const
struct usb_device_id *id)
INIT_DELAYED_WORK(&hub->leds, led_work);
INIT_DELAYED_WORK(&hub->init_work, NULL);
INIT_WORK(&hub->events, hub_event);
+ timer_setup(&hub->irq_urb_retry, hub_retry_irq_urb, 0);
usb_get_intf(intf);
usb_get_dev(hdev);
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 4accfb63f7dc..b0740cf5ef19 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -69,6 +69,7 @@ struct usb_hub {
struct delayed_work leds;
struct delayed_work init_work;
struct work_struct events;
+ struct timer_list irq_urb_retry;
struct usb_port **ports;
};
--
2.19.1
[PATCH 00/16] staging: vchiq: dead code removal & misc fixes
Hi All, This series was written in parallel with reading and understanding the vchiq code. So excuse me for the lack of logic in the sequence of patches. The main focus was to delete as much code as possible, I've counted around 550 lines, which is not bad. Apart from that there are some patches enforcing proper kernel APIs usage. The only patch that really changes code is the vchiq_ioc_copy_element_data() rewrite. The last commit updates the TODO list with some of my observations, I realise some of the might be a little opinionated. If anything it's going to force a discussion on the topic, which is nice. It was developed on top of the latest linux-next, and was tested on a RPIv3B+ with audio, video and running vchiq_test. Regards, Nicolas === Nicolas Saenz Julienne (16): staging: vchiq_core: rework vchiq_get_config staging: vchiq_arm: rework close/remove_service IOCTLS staging: vchiq_shim: delete vchi_service_create staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list staging: vchiq_arm: get rid of vchi_mh.h staging: vchiq_arm: rework vchiq_ioc_copy_element_data staging: vchiq-core: get rid of is_master distinction staging: vchiq_core: remove unnecessary safety checks in vchiq_init_state staging: vchiq_core: do not initialize semaphores twice staging: vchiq_core: don't add a wmb() before remote_event_signal() staging: vchiq: use completions instead of semaphores staging: vchiq_util: get rid of unneeded memory barriers staging: vchiq_core: fix logic redundancy in parse_open staging: vchiq_arm: rework probe and init functions staging: vchiq_arm: fix open/release cdev functions staging: vchiq: add more tasks to the TODO list .../staging/vc04_services/interface/vchi/TODO | 42 ++ .../vc04_services/interface/vchi/vchi.h | 8 - .../vc04_services/interface/vchi/vchi_mh.h| 42 -- .../interface/vchiq_arm/vchiq_2835_arm.c | 18 +- .../interface/vchiq_arm/vchiq_arm.c | 598 -- .../interface/vchiq_arm/vchiq_core.c | 523 --- .../interface/vchiq_arm/vchiq_core.h | 47 +- .../interface/vchiq_arm/vchiq_if.h| 11 +- .../interface/vchiq_arm/vchiq_shim.c | 32 - .../interface/vchiq_arm/vchiq_util.c | 48 +- .../interface/vchiq_arm/vchiq_util.h | 6 +- 11 files changed, 435 insertions(+), 940 deletions(-) delete mode 100644 drivers/staging/vc04_services/interface/vchi/vchi_mh.h -- 2.19.1
[PATCH 06/16] staging: vchiq_arm: rework vchiq_ioc_copy_element_data
The function is passed to vchiq_core.c for it to go trough all the
transfer elements (an array of pointers to data) and copy them into the
actual transfer memory (contiguous memory).
The logic in the function was "copy an element and return, except when
the element is empty, in which case look for the next non-empty element
and copy it. The function will be called as many times as necessary until
all the elements are copied".
Now, this approach already forces the function to loop around elements
and felt convoluted, so it was changed to a more straightforward "Copy
all the elements into memory as long as they fit".
The resulting function is shorter and simpler.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 89 +++
1 file changed, 31 insertions(+), 58 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 34160cc3b8bd..1cdfdb714abc 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -752,74 +752,48 @@ static void close_delivered(USER_SERVICE_T *user_service)
}
struct vchiq_io_copy_callback_context {
- struct vchiq_element *current_element;
- size_t current_element_offset;
+ struct vchiq_element *element;
+ size_t element_offset;
unsigned long elements_to_go;
- size_t current_offset;
};
-static ssize_t
-vchiq_ioc_copy_element_data(
- void *context,
- void *dest,
- size_t offset,
- size_t maxsize)
+static ssize_t vchiq_ioc_copy_element_data(void *context, void *dest,
+ size_t offset, size_t maxsize)
{
- long res;
+ struct vchiq_io_copy_callback_context *cc = context;
+ size_t total_bytes_copied = 0;
size_t bytes_this_round;
- struct vchiq_io_copy_callback_context *copy_context =
- (struct vchiq_io_copy_callback_context *)context;
-
- if (offset != copy_context->current_offset)
- return 0;
-
- if (!copy_context->elements_to_go)
- return 0;
-
- /*
-* Complex logic here to handle the case of 0 size elements
-* in the middle of the array of elements.
-*
-* Need to skip over these 0 size elements.
-*/
- while (1) {
- bytes_this_round = min(copy_context->current_element->size -
- copy_context->current_element_offset,
- maxsize);
-
- if (bytes_this_round)
- break;
- copy_context->elements_to_go--;
- copy_context->current_element++;
- copy_context->current_element_offset = 0;
+ while (total_bytes_copied < maxsize) {
+ if (!cc->elements_to_go)
+ return total_bytes_copied;
- if (!copy_context->elements_to_go)
- return 0;
- }
+ if (!cc->element->size) {
+ cc->elements_to_go--;
+ cc->element++;
+ cc->element_offset = 0;
+ continue;
+ }
- res = copy_from_user(dest,
-copy_context->current_element->data +
-copy_context->current_element_offset,
-bytes_this_round);
+ bytes_this_round = min(cc->element->size - cc->element_offset,
+ maxsize - total_bytes_copied);
- if (res != 0)
- return -EFAULT;
+ if (copy_from_user(dest + total_bytes_copied,
+ cc->element->data + cc->element_offset,
+ bytes_this_round))
+ return -EFAULT;
- copy_context->current_element_offset += bytes_this_round;
- copy_context->current_offset += bytes_this_round;
+ cc->element_offset += bytes_this_round;
+ total_bytes_copied += bytes_this_round;
- /*
-* Check if done with current element, and if so advance to the next.
-*/
- if (copy_context->current_element_offset ==
- copy_context->current_element->size) {
- copy_context->elements_to_go--;
- copy_context->current_element++;
- copy_context->current_element_offset = 0;
+ if (cc->element_offset == cc->element->size) {
+ cc->elements_to_go--;
+ cc->element++;
+ cc->element_offset = 0;
+ }
}
- return b
[PATCH 01/16] staging: vchiq_core: rework vchiq_get_config
The function is overly complicated for what it's ultimately achieving.
It's simply filling up a structure.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 12
.../interface/vchiq_arm/vchiq_core.c | 30 +--
.../interface/vchiq_arm/vchiq_if.h| 3 +-
3 files changed, 14 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index ea789376de0f..6d503392341e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1480,13 +1480,11 @@ vchiq_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
ret = -EINVAL;
break;
}
- status = vchiq_get_config(instance, args.config_size, &config);
- if (status == VCHIQ_SUCCESS) {
- if (copy_to_user((void __user *)args.pconfig,
- &config, args.config_size) != 0) {
- ret = -EFAULT;
- break;
- }
+
+ vchiq_get_config(&config);
+ if (copy_to_user(args.pconfig, &config, args.config_size)) {
+ ret = -EFAULT;
+ break;
}
} break;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 7642ced31436..89f1ccdc3b98 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3583,28 +3583,14 @@ vchiq_get_peer_version(VCHIQ_SERVICE_HANDLE_T handle,
short *peer_version)
return status;
}
-VCHIQ_STATUS_T
-vchiq_get_config(VCHIQ_INSTANCE_T instance,
- int config_size, VCHIQ_CONFIG_T *pconfig)
-{
- VCHIQ_CONFIG_T config;
-
- (void)instance;
-
- config.max_msg_size = VCHIQ_MAX_MSG_SIZE;
- config.bulk_threshold = VCHIQ_MAX_MSG_SIZE;
- config.max_outstanding_bulks = VCHIQ_NUM_SERVICE_BULKS;
- config.max_services = VCHIQ_MAX_SERVICES;
- config.version= VCHIQ_VERSION;
- config.version_min= VCHIQ_VERSION_MIN;
-
- if (config_size > sizeof(VCHIQ_CONFIG_T))
- return VCHIQ_ERROR;
-
- memcpy(pconfig, &config,
- min(config_size, (int)(sizeof(VCHIQ_CONFIG_T;
-
- return VCHIQ_SUCCESS;
+void vchiq_get_config(VCHIQ_CONFIG_T *config)
+{
+ config->max_msg_size = VCHIQ_MAX_MSG_SIZE;
+ config->bulk_threshold = VCHIQ_MAX_MSG_SIZE;
+ config->max_outstanding_bulks = VCHIQ_NUM_SERVICE_BULKS;
+ config->max_services = VCHIQ_MAX_SERVICES;
+ config->version= VCHIQ_VERSION;
+ config->version_min= VCHIQ_VERSION_MIN;
}
VCHIQ_STATUS_T
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
index e4109a83e628..87829a244465 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_if.h
@@ -164,8 +164,7 @@ extern VCHIQ_STATUS_T
vchiq_bulk_receive_handle(VCHIQ_SERVICE_HANDLE_T service,
extern int vchiq_get_client_id(VCHIQ_SERVICE_HANDLE_T service);
extern void *vchiq_get_service_userdata(VCHIQ_SERVICE_HANDLE_T service);
extern int vchiq_get_service_fourcc(VCHIQ_SERVICE_HANDLE_T service);
-extern VCHIQ_STATUS_T vchiq_get_config(VCHIQ_INSTANCE_T instance,
- int config_size, VCHIQ_CONFIG_T *pconfig);
+extern void vchiq_get_config(VCHIQ_CONFIG_T *config);
extern VCHIQ_STATUS_T vchiq_set_service_option(VCHIQ_SERVICE_HANDLE_T service,
VCHIQ_SERVICE_OPTION_T option, int value);
--
2.19.1
[PATCH 03/16] staging: vchiq_shim: delete vchi_service_create
No one is using the API neither in the actual staging tree nor in the
downstream tree (https://github.com/raspberrypi/linux).
Signed-off-by: Nicolas Saenz Julienne
---
.../vc04_services/interface/vchi/vchi.h | 5 ---
.../interface/vchiq_arm/vchiq_shim.c | 32 ---
2 files changed, 37 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchi/vchi.h
b/drivers/staging/vc04_services/interface/vchi/vchi.h
index 01381904775d..379a16ebfd5b 100644
--- a/drivers/staging/vc04_services/interface/vchi/vchi.h
+++ b/drivers/staging/vc04_services/interface/vchi/vchi.h
@@ -113,11 +113,6 @@ extern uint32_t vchi_current_time(VCHI_INSTANCE_T
instance_handle);
/**
Global service API
*/
-// Routine to create a named service
-extern int32_t vchi_service_create(VCHI_INSTANCE_T instance_handle,
- SERVICE_CREATION_T *setup,
- VCHI_SERVICE_HANDLE_T *handle);
-
// Routine to destroy a service
extern int32_t vchi_service_destroy(const VCHI_SERVICE_HANDLE_T handle);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
index c3223fcdaf87..81cac68f4b78 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_shim.c
@@ -660,38 +660,6 @@ int32_t vchi_service_open(VCHI_INSTANCE_T instance_handle,
}
EXPORT_SYMBOL(vchi_service_open);
-int32_t vchi_service_create(VCHI_INSTANCE_T instance_handle,
- SERVICE_CREATION_T *setup,
- VCHI_SERVICE_HANDLE_T *handle)
-{
- VCHIQ_INSTANCE_T instance = (VCHIQ_INSTANCE_T)instance_handle;
- struct shim_service *service = service_alloc(instance, setup);
-
- *handle = (VCHI_SERVICE_HANDLE_T)service;
-
- if (service) {
- VCHIQ_SERVICE_PARAMS_T params;
- VCHIQ_STATUS_T status;
-
- memset(¶ms, 0, sizeof(params));
- params.fourcc = setup->service_id;
- params.callback = shim_callback;
- params.userdata = service;
- params.version = setup->version.version;
- params.version_min = setup->version.version_min;
- status = vchiq_add_service(instance, ¶ms, &service->handle);
-
- if (status != VCHIQ_SUCCESS) {
- service_free(service);
- service = NULL;
- *handle = NULL;
- }
- }
-
- return (service != NULL) ? 0 : -1;
-}
-EXPORT_SYMBOL(vchi_service_create);
-
int32_t vchi_service_close(const VCHI_SERVICE_HANDLE_T handle)
{
int32_t ret = -1;
--
2.19.1
[PATCH 08/16] staging: vchiq_core: remove unnecessary safety checks in vchiq_init_state
vchiq_init_state() checks the initial contents of slot_zero are correct.
These are set in vchiq_init_slots(), using the same hard-coded defaults
as the checks. Both functions are called sequentially and Video Core
isn't yet aware of the slot's address. There is no way the contents of
slot_zero changed in between functions, making the checks useless.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_core.c | 59 ---
1 file changed, 59 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 34a892011296..dee5ea7bfe4f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -2170,65 +2170,6 @@ vchiq_init_state(VCHIQ_STATE_T *state, VCHIQ_SLOT_ZERO_T
*slot_zero)
return VCHIQ_ERROR;
}
- /* Check the input configuration */
-
- if (slot_zero->magic != VCHIQ_MAGIC) {
- vchiq_loud_error_header();
- vchiq_loud_error("Invalid VCHIQ magic value found.");
- vchiq_loud_error("slot_zero=%pK: magic=%x (expected %x)",
- slot_zero, slot_zero->magic, VCHIQ_MAGIC);
- vchiq_loud_error_footer();
- return VCHIQ_ERROR;
- }
-
- if (slot_zero->version < VCHIQ_VERSION_MIN) {
- vchiq_loud_error_header();
- vchiq_loud_error("Incompatible VCHIQ versions found.");
- vchiq_loud_error("slot_zero=%pK: VideoCore version=%d (minimum
%d)",
- slot_zero, slot_zero->version, VCHIQ_VERSION_MIN);
- vchiq_loud_error("Restart with a newer VideoCore image.");
- vchiq_loud_error_footer();
- return VCHIQ_ERROR;
- }
-
- if (VCHIQ_VERSION < slot_zero->version_min) {
- vchiq_loud_error_header();
- vchiq_loud_error("Incompatible VCHIQ versions found.");
- vchiq_loud_error("slot_zero=%pK: version=%d (VideoCore minimum
%d)",
- slot_zero, VCHIQ_VERSION, slot_zero->version_min);
- vchiq_loud_error("Restart with a newer kernel.");
- vchiq_loud_error_footer();
- return VCHIQ_ERROR;
- }
-
- if ((slot_zero->slot_zero_size != sizeof(VCHIQ_SLOT_ZERO_T)) ||
-(slot_zero->slot_size != VCHIQ_SLOT_SIZE) ||
-(slot_zero->max_slots != VCHIQ_MAX_SLOTS) ||
-(slot_zero->max_slots_per_side != VCHIQ_MAX_SLOTS_PER_SIDE)) {
- vchiq_loud_error_header();
- if (slot_zero->slot_zero_size != sizeof(VCHIQ_SLOT_ZERO_T))
- vchiq_loud_error("slot_zero=%pK: slot_zero_size=%d
(expected %d)",
- slot_zero, slot_zero->slot_zero_size,
- (int)sizeof(VCHIQ_SLOT_ZERO_T));
- if (slot_zero->slot_size != VCHIQ_SLOT_SIZE)
- vchiq_loud_error("slot_zero=%pK: slot_size=%d (expected
%d)",
- slot_zero, slot_zero->slot_size,
- VCHIQ_SLOT_SIZE);
- if (slot_zero->max_slots != VCHIQ_MAX_SLOTS)
- vchiq_loud_error("slot_zero=%pK: max_slots=%d (expected
%d)",
- slot_zero, slot_zero->max_slots,
- VCHIQ_MAX_SLOTS);
- if (slot_zero->max_slots_per_side != VCHIQ_MAX_SLOTS_PER_SIDE)
- vchiq_loud_error("slot_zero=%pK: max_slots_per_side=%d
(expected %d)",
- slot_zero, slot_zero->max_slots_per_side,
- VCHIQ_MAX_SLOTS_PER_SIDE);
- vchiq_loud_error_footer();
- return VCHIQ_ERROR;
- }
-
- if (VCHIQ_VERSION < slot_zero->version)
- slot_zero->version = VCHIQ_VERSION;
-
local = &slot_zero->slave;
remote = &slot_zero->master;
--
2.19.1
[PATCH 12/16] staging: vchiq_util: get rid of unneeded memory barriers
All the memory operations featured in this file modify/access memory that is only accessed by the CPU. So we can assume that all the memory barrier handling done by the completion routines is good enough for us. Signed-off-by: Nicolas Saenz Julienne --- .../interface/vchiq_arm/vchiq_util.c | 32 --- 1 file changed, 32 deletions(-) diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c index 44b954daa74a..4b8554bc647e 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_util.c @@ -84,20 +84,7 @@ void vchiu_queue_push(VCHIU_QUEUE_T *queue, VCHIQ_HEADER_T *header) flush_signals(current); } - /* -* Write to queue->storage must be visible after read from -* queue->read -*/ - smp_mb(); - queue->storage[queue->write & (queue->size - 1)] = header; - - /* -* Write to queue->storage must be visible before write to -* queue->write -*/ - smp_wmb(); - queue->write++; complete(&queue->push); @@ -112,12 +99,6 @@ VCHIQ_HEADER_T *vchiu_queue_peek(VCHIU_QUEUE_T *queue) complete(&queue->push); // We haven't removed anything from the queue. - /* -* Read from queue->storage must be visible after read from -* queue->write -*/ - smp_rmb(); - return queue->storage[queue->read & (queue->size - 1)]; } @@ -130,20 +111,7 @@ VCHIQ_HEADER_T *vchiu_queue_pop(VCHIU_QUEUE_T *queue) flush_signals(current); } - /* -* Read from queue->storage must be visible after read from -* queue->write -*/ - smp_rmb(); - header = queue->storage[queue->read & (queue->size - 1)]; - - /* -* Read from queue->storage must be visible before write to -* queue->read -*/ - smp_mb(); - queue->read++; complete(&queue->pop); -- 2.19.1
[PATCH 10/16] staging: vchiq_core: don't add a wmb() before remote_event_signal()
It's the first thing remote_event_signal() does.
Signed-off-by: Nicolas Saenz Julienne
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.c| 4
1 file changed, 4 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 8b23ea5322e8..5791c2b670fa 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1137,9 +1137,6 @@ queue_message_sync(VCHIQ_STATE_T *state, VCHIQ_SERVICE_T
*service,
size);
}
- /* Make sure the new header is visible to the peer. */
- wmb();
-
remote_event_signal(&state->remote->sync_trigger);
if (VCHIQ_MSG_TYPE(msgid) != VCHIQ_MSG_PAUSE)
@@ -3269,7 +3266,6 @@ static void
release_message_sync(VCHIQ_STATE_T *state, VCHIQ_HEADER_T *header)
{
header->msgid = VCHIQ_MSGID_PADDING;
- wmb();
remote_event_signal(&state->remote->sync_release);
}
--
2.19.1
[PATCH 09/16] staging: vchiq_core: do not initialize semaphores twice
vchiq_init_state() initialises a series of semaphores to then call
remote_event_create() on the same semaphores, which initializes them
again. We get rid of the second initialization.
Signed-off-by: Nicolas Saenz Julienne
---
.../vc04_services/interface/vchiq_arm/vchiq_core.c| 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index dee5ea7bfe4f..8b23ea5322e8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -418,12 +418,11 @@ vchiq_set_conn_state(VCHIQ_STATE_T *state,
VCHIQ_CONNSTATE_T newstate)
}
static inline void
-remote_event_create(VCHIQ_STATE_T *state, REMOTE_EVENT_T *event)
+remote_event_create(REMOTE_EVENT_T *event)
{
event->armed = 0;
/* Don't clear the 'fired' flag because it may already have been set
** by the other side. */
- sema_init((struct semaphore *)((char *)state + event->event), 0);
}
static inline int
@@ -2237,18 +2236,18 @@ vchiq_init_state(VCHIQ_STATE_T *state,
VCHIQ_SLOT_ZERO_T *slot_zero)
state->data_quota = state->slot_queue_available - 1;
local->trigger.event = offsetof(VCHIQ_STATE_T, trigger_event);
- remote_event_create(state, &local->trigger);
+ remote_event_create(&local->trigger);
local->tx_pos = 0;
local->recycle.event = offsetof(VCHIQ_STATE_T, recycle_event);
- remote_event_create(state, &local->recycle);
+ remote_event_create(&local->recycle);
local->slot_queue_recycle = state->slot_queue_available;
local->sync_trigger.event = offsetof(VCHIQ_STATE_T, sync_trigger_event);
- remote_event_create(state, &local->sync_trigger);
+ remote_event_create(&local->sync_trigger);
local->sync_release.event = offsetof(VCHIQ_STATE_T, sync_release_event);
- remote_event_create(state, &local->sync_release);
+ remote_event_create(&local->sync_release);
/* At start-of-day, the slot is empty and available */
((VCHIQ_HEADER_T *)SLOT_DATA_FROM_INDEX(state, local->slot_sync))->msgid
--
2.19.1
[PATCH 13/16] staging: vchiq_core: fix logic redundancy in parse_open
We update sync to reflect that the firmware version is compatible with
that option. We don't need to check both of them again further down the
code.
Signed-off-by: Nicolas Saenz Julienne
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.c| 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index a45cdd08e209..5ee667d46eb5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -1461,9 +1461,7 @@ parse_open(VCHIQ_STATE_T *state, VCHIQ_HEADER_T *header)
service->sync = 0;
/* Acknowledge the OPEN */
- if (service->sync &&
- (state->version_common >=
-VCHIQ_VERSION_SYNCHRONOUS_MODE)) {
+ if (service->sync) {
if (queue_message_sync(
state,
NULL,
--
2.19.1
[PATCH 11/16] staging: vchiq: use completions instead of semaphores
It is preferred in the kernel to avoid using semaphores to wait for
events, as they are optimised for the opposite situation; where the
common case is that they are available and may block only occasionally.
FYI see this thread: https://lkml.org/lkml/2008/4/11/323.
Also completions are semantically more explicit in this case.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 60 ++-
.../interface/vchiq_arm/vchiq_core.c | 100 +-
.../interface/vchiq_arm/vchiq_core.h | 26 ++---
.../interface/vchiq_arm/vchiq_util.c | 16 +--
.../interface/vchiq_arm/vchiq_util.h | 6 +-
5 files changed, 106 insertions(+), 102 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 1cdfdb714abc..383013a92939 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -44,7 +44,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
@@ -121,9 +121,9 @@ typedef struct user_service_struct {
int message_available_pos;
int msg_insert;
int msg_remove;
- struct semaphore insert_event;
- struct semaphore remove_event;
- struct semaphore close_event;
+ struct completion insert_event;
+ struct completion remove_event;
+ struct completion close_event;
VCHIQ_HEADER_T * msg_queue[MSG_QUEUE_SIZE];
} USER_SERVICE_T;
@@ -138,8 +138,8 @@ struct vchiq_instance_struct {
VCHIQ_COMPLETION_DATA_T completions[MAX_COMPLETIONS];
int completion_insert;
int completion_remove;
- struct semaphore insert_event;
- struct semaphore remove_event;
+ struct completion insert_event;
+ struct completion remove_event;
struct mutex completion_mutex;
int connected;
@@ -562,7 +562,8 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T
reason,
vchiq_log_trace(vchiq_arm_log_level,
"%s - completion queue full", __func__);
DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
- if (down_interruptible(&instance->remove_event) != 0) {
+ if (wait_for_completion_interruptible(
+ &instance->remove_event)) {
vchiq_log_info(vchiq_arm_log_level,
"service_callback interrupted");
return VCHIQ_RETRY;
@@ -600,7 +601,7 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T
reason,
insert++;
instance->completion_insert = insert;
- up(&instance->insert_event);
+ complete(&instance->insert_event);
return VCHIQ_SUCCESS;
}
@@ -673,7 +674,8 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T
*header,
}
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
- if (down_interruptible(&user_service->remove_event)
+ if (wait_for_completion_interruptible(
+ &user_service->remove_event)
!= 0) {
vchiq_log_info(vchiq_arm_log_level,
"%s interrupted", __func__);
@@ -705,7 +707,7 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T
*header,
}
spin_unlock(&msg_queue_spinlock);
- up(&user_service->insert_event);
+ complete(&user_service->insert_event);
header = NULL;
}
@@ -745,7 +747,7 @@ static void close_delivered(USER_SERVICE_T *user_service)
unlock_service(user_service->service);
/* Wake the user-thread blocked in close_ or remove_service */
- up(&user_service->close_event);
+ complete(&user_service->close_event);
user_service->close_pending = 0;
}
@@ -867,7 +869,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
if (status == VCHIQ_SUCCESS) {
/* Wake the completion thread and ask it to exit */
instance->closing = 1;
- up(&instance->insert_event);
+ complete(&instance->insert_event);
}
break;
@@ -948,9 +950,9 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
instance->completion_remove - 1;
user_service->msg_insert = 0;
user_service->msg_remove = 0;
- sema
[PATCH 05/16] staging: vchiq_arm: get rid of vchi_mh.h
The concept of VCHI_MEM_HANDLE_T is introduced by this header file and
was meant to be used with bulk transfers. After a quick look in
vchiq_core.c it is pretty clear that it actually accomplishes nothing
nor alters the bulk transfers in any way.
Signed-off-by: Nicolas Saenz Julienne
---
.../vc04_services/interface/vchi/vchi.h | 3 --
.../vc04_services/interface/vchi/vchi_mh.h| 42 ---
.../interface/vchiq_arm/vchiq_2835_arm.c | 6 +--
.../interface/vchiq_arm/vchiq_arm.c | 27 ++--
.../interface/vchiq_arm/vchiq_core.c | 17
.../interface/vchiq_arm/vchiq_core.h | 10 ++---
.../interface/vchiq_arm/vchiq_if.h| 8 ++--
7 files changed, 27 insertions(+), 86 deletions(-)
delete mode 100644 drivers/staging/vc04_services/interface/vchi/vchi_mh.h
diff --git a/drivers/staging/vc04_services/interface/vchi/vchi.h
b/drivers/staging/vc04_services/interface/vchi/vchi.h
index 379a16ebfd5b..e326926eac31 100644
--- a/drivers/staging/vc04_services/interface/vchi/vchi.h
+++ b/drivers/staging/vc04_services/interface/vchi/vchi.h
@@ -36,7 +36,6 @@
#include "interface/vchi/vchi_cfg.h"
#include "interface/vchi/vchi_common.h"
-#include "vchi_mh.h"
/**
Global defs
@@ -239,7 +238,6 @@ extern int32_t
vchi_bulk_queue_receive(VCHI_SERVICE_HANDLE_T handle,
// Prepare interface for a transfer from the other side into relocatable
memory.
int32_t vchi_bulk_queue_receive_reloc(const VCHI_SERVICE_HANDLE_T handle,
- VCHI_MEM_HANDLE_T h_dst,
uint32_t offset,
uint32_t data_size,
const VCHI_FLAGS_T flags,
@@ -261,7 +259,6 @@ extern int32_t
vchi_bulk_queue_transmit(VCHI_SERVICE_HANDLE_T handle,
#endif
extern int32_t vchi_bulk_queue_transmit_reloc(VCHI_SERVICE_HANDLE_T handle,
- VCHI_MEM_HANDLE_T h_src,
uint32_t offset,
uint32_t data_size,
VCHI_FLAGS_T flags,
diff --git a/drivers/staging/vc04_services/interface/vchi/vchi_mh.h
b/drivers/staging/vc04_services/interface/vchi/vchi_mh.h
deleted file mode 100644
index 198bd076b666..
--- a/drivers/staging/vc04_services/interface/vchi/vchi_mh.h
+++ /dev/null
@@ -1,42 +0,0 @@
-/**
- * Copyright (c) 2010-2012 Broadcom. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *notice, this list of conditions, and the following disclaimer,
- *without modification.
- * 2. Redistributions in binary form must reproduce the above copyright
- *notice, this list of conditions and the following disclaimer in the
- *documentation and/or other materials provided with the distribution.
- * 3. The names of the above-listed copyright holders may not be used
- *to endorse or promote products derived from this software without
- *specific prior written permission.
- *
- * ALTERNATIVELY, this software may be distributed under the terms of the
- * GNU General Public License ("GPL") version 2, as published by the Free
- * Software Foundation.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
- * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
- * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
- * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
- * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#ifndef VCHI_MH_H_
-#define VCHI_MH_H_
-
-#include
-
-typedef int32_t VCHI_MEM_HANDLE_T;
-#define VCHI_MEM_HANDLE_INVALID 0
-
-#endif
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 83d740feab96..014583cdf367 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -247,13 +247,10 @@ remote_event_signal(REMOTE_EVENT_T *event)
}
VCHIQ_STATUS_T
-vchiq_prepare_bulk_data(VCHIQ_BULK_T *bulk, VCHI
[PATCH 14/16] staging: vchiq_arm: rework probe and init functions
Moves the allocation of a chardev region and class creation to the init
function of the driver since those functions are meant to be run on a
per driver basis, as opposed to the code run in the probe function which
is run in a per device basis.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 71 ---
1 file changed, 45 insertions(+), 26 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 383013a92939..a7dcced79980 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -166,7 +166,6 @@ static struct cdevvchiq_cdev;
static dev_t vchiq_devid;
static VCHIQ_STATE_T g_state;
static struct class *vchiq_class;
-static struct device *vchiq_dev;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
@@ -3552,34 +3551,19 @@ static int vchiq_probe(struct platform_device *pdev)
if (err != 0)
goto failed_platform_init;
- err = alloc_chrdev_region(&vchiq_devid, VCHIQ_MINOR, 1, DEVICE_NAME);
- if (err != 0) {
- vchiq_log_error(vchiq_arm_log_level,
- "Unable to allocate device number");
- goto failed_platform_init;
- }
cdev_init(&vchiq_cdev, &vchiq_fops);
vchiq_cdev.owner = THIS_MODULE;
err = cdev_add(&vchiq_cdev, vchiq_devid, 1);
if (err != 0) {
vchiq_log_error(vchiq_arm_log_level,
"Unable to register device");
- goto failed_cdev_add;
+ goto failed_platform_init;
}
- /* create sysfs entries */
- vchiq_class = class_create(THIS_MODULE, DEVICE_NAME);
- err = PTR_ERR(vchiq_class);
- if (IS_ERR(vchiq_class))
- goto failed_class_create;
-
- vchiq_dev = device_create(vchiq_class, NULL,
- vchiq_devid, NULL, "vchiq");
- err = PTR_ERR(vchiq_dev);
- if (IS_ERR(vchiq_dev))
+ if (IS_ERR(device_create(vchiq_class, &pdev->dev, vchiq_devid,
+NULL, "vchiq")))
goto failed_device_create;
- /* create debugfs entries */
vchiq_debugfs_init();
vchiq_log_info(vchiq_arm_log_level,
@@ -3594,11 +3578,7 @@ static int vchiq_probe(struct platform_device *pdev)
return 0;
failed_device_create:
- class_destroy(vchiq_class);
-failed_class_create:
cdev_del(&vchiq_cdev);
-failed_cdev_add:
- unregister_chrdev_region(vchiq_devid, 1);
failed_platform_init:
vchiq_log_warning(vchiq_arm_log_level, "could not load vchiq");
return err;
@@ -3609,9 +3589,7 @@ static int vchiq_remove(struct platform_device *pdev)
platform_device_unregister(bcm2835_camera);
vchiq_debugfs_deinit();
device_destroy(vchiq_class, vchiq_devid);
- class_destroy(vchiq_class);
cdev_del(&vchiq_cdev);
- unregister_chrdev_region(vchiq_devid, 1);
return 0;
}
@@ -3624,7 +3602,48 @@ static struct platform_driver vchiq_driver = {
.probe = vchiq_probe,
.remove = vchiq_remove,
};
-module_platform_driver(vchiq_driver);
+
+static int __init vchiq_driver_init(void)
+{
+ int ret;
+
+ vchiq_class = class_create(THIS_MODULE, DEVICE_NAME);
+ if (IS_ERR(vchiq_class)) {
+ pr_err("Failed to create vchiq class\n");
+ return PTR_ERR(vchiq_class);
+ }
+
+ ret = alloc_chrdev_region(&vchiq_devid, VCHIQ_MINOR, 1, DEVICE_NAME);
+ if (ret) {
+ pr_err("Failed to allocate vchiq's chrdev region\n");
+ goto class_destroy;
+ }
+
+ ret = platform_driver_register(&vchiq_driver);
+ if (ret) {
+ pr_err("Failed to register vchiq driver\n");
+ goto region_unregister;
+ }
+
+ return 0;
+
+region_unregister:
+ platform_driver_unregister(&vchiq_driver);
+
+class_destroy:
+ class_destroy(vchiq_class);
+
+ return ret;
+}
+module_init(vchiq_driver_init);
+
+static void __exit vchiq_driver_exit(void)
+{
+ platform_driver_unregister(&vchiq_driver);
+ unregister_chrdev_region(vchiq_devid, 1);
+ class_destroy(vchiq_class);
+}
+module_exit(vchiq_driver_exit);
MODULE_LICENSE("Dual BSD/GPL");
MODULE_DESCRIPTION("Videocore VCHIQ driver");
--
2.19.1
[PATCH 16/16] staging: vchiq: add more tasks to the TODO list
The TODO list was missing some tasks needed before upstreaming the device. Signed-off-by: Nicolas Saenz Julienne --- .../staging/vc04_services/interface/vchi/TODO | 42 +++ 1 file changed, 42 insertions(+) diff --git a/drivers/staging/vc04_services/interface/vchi/TODO b/drivers/staging/vc04_services/interface/vchi/TODO index 0b3ec75ff627..fc2752bc95b2 100644 --- a/drivers/staging/vc04_services/interface/vchi/TODO +++ b/drivers/staging/vc04_services/interface/vchi/TODO @@ -49,3 +49,45 @@ such as dev_info, dev_dbg, and friends. A short top-down description of this driver's architecture (function of kthreads, userspace, limitations) could be very helpful for reviewers. + +7) Review and comment memory barriers + +There is a heavy use of memory barriers in this driver, it would be very +beneficial to go over all of them and, if correct, comment on their merits. +Extra points to whomever confidently reviews the remote_event_*() family of +functions. + +8) Get rid of custom function return values + +Most functions use a custom set of return values, we should force proper Linux +error numbers. Special care is needed for VCHIQ_RETRY. + +9) Reformat core code with more sane indentations + +The code follows the 80 characters limitation yet tends to go 3 or 4 levels of +indentation deep making it very unpleasant to read. This is specially relevant +in the character driver ioctl code and in the core thread functions. + +10) Reorganize file structure: Move char driver to it's own file and join both +platform files + +The cdev is defined alongside with the platform code in vchiq_arm.c. It would +be nice to completely decouple it from the actual core code. For instance to be +able to use bcm2835-audio without having /dev/vchiq created. One could argue +it's better for security reasons or general cleanliness. It could even be +interesting to create two different kernel modules, something the likes of +vchiq-core.ko and vchiq-dev.ko. This would also ease the upstreaming process. + +The code in vchiq_bcm2835_arm.c should fit in the generic platform file. + +12) Get rid of all the struct typedefs + +Most structs are typedefd, it's not encouraged in the kernel. + +13) Get rid of all non essential global structures and create a proper per +device structure + +The first thing one generally sees in a probe function is a memory allocation +for all the device specific data. This structure is then passed all over the +driver. This is good practice since it makes the driver work regardless of the +number of devices probed. -- 2.19.1
[PATCH 04/16] staging: vchiq_arm: use list_for_each_entry when accessing bulk_waiter_list
The resulting code is way more readeable and intuitive compared to plain
list_for_each.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 52 ++-
1 file changed, 16 insertions(+), 36 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index d88dd7415e1e..5f55c708ade8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -280,16 +280,11 @@ VCHIQ_STATUS_T vchiq_shutdown(VCHIQ_INSTANCE_T instance)
"%s(%p): returning %d", __func__, instance, status);
if (status == VCHIQ_SUCCESS) {
- struct list_head *pos, *next;
+ struct bulk_waiter_node *waiter, *next;
- list_for_each_safe(pos, next,
- &instance->bulk_waiter_list) {
- struct bulk_waiter_node *waiter;
-
- waiter = list_entry(pos,
- struct bulk_waiter_node,
- list);
- list_del(pos);
+ list_for_each_entry_safe(waiter, next,
+&instance->bulk_waiter_list, list) {
+ list_del(&waiter->list);
vchiq_log_info(vchiq_arm_log_level,
"bulk_waiter - cleaned up %pK for pid
%d",
waiter, waiter->pid);
@@ -473,7 +468,6 @@ vchiq_blocking_bulk_transfer(VCHIQ_SERVICE_HANDLE_T handle,
void *data,
VCHIQ_SERVICE_T *service;
VCHIQ_STATUS_T status;
struct bulk_waiter_node *waiter = NULL;
- struct list_head *pos;
service = find_service_by_handle(handle);
if (!service)
@@ -484,13 +478,9 @@ vchiq_blocking_bulk_transfer(VCHIQ_SERVICE_HANDLE_T
handle, void *data,
unlock_service(service);
mutex_lock(&instance->bulk_waiter_list_mutex);
- list_for_each(pos, &instance->bulk_waiter_list) {
- if (list_entry(pos, struct bulk_waiter_node,
- list)->pid == current->pid) {
- waiter = list_entry(pos,
- struct bulk_waiter_node,
- list);
- list_del(pos);
+ list_for_each_entry(waiter, &instance->bulk_waiter_list, list) {
+ if (waiter->pid == current->pid) {
+ list_del(&waiter->list);
break;
}
}
@@ -1135,21 +1125,16 @@ vchiq_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
ret = -ENOMEM;
break;
}
+
args.userdata = &waiter->bulk_waiter;
} else if (args.mode == VCHIQ_BULK_MODE_WAITING) {
- struct list_head *pos;
-
mutex_lock(&instance->bulk_waiter_list_mutex);
- list_for_each(pos, &instance->bulk_waiter_list) {
- if (list_entry(pos, struct bulk_waiter_node,
- list)->pid == current->pid) {
- waiter = list_entry(pos,
- struct bulk_waiter_node,
- list);
- list_del(pos);
+ list_for_each_entry(waiter, &instance->bulk_waiter_list,
+ list) {
+ if (waiter->pid == current->pid) {
+ list_del(&waiter->list);
break;
}
-
}
mutex_unlock(&instance->bulk_waiter_list_mutex);
if (!waiter) {
@@ -2158,16 +2143,11 @@ vchiq_release(struct inode *inode, struct file *file)
vchiq_release_internal(instance->state, NULL);
{
- struct list_head *pos, *next;
-
- list_for_each_safe(pos, next,
- &instance->bulk_waiter_list) {
- struct bulk_waiter_node *waiter;
+ struct bulk_waiter_node *waiter, *next;
- waiter = list_entry(pos,
- struct bulk_waiter_node,
- list);
- list_del(pos);
+ list_for_each_en
[PATCH 15/16] staging: vchiq_arm: fix open/release cdev functions
Both functions checked the minor number of the cdev prior running the
code. This was useless since the number of devices is already limited by
alloc_chrdev_region.
This removes the check and reindents the code where relevant.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 247 +++---
1 file changed, 100 insertions(+), 147 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index a7dcced79980..153a396d21bd 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -63,8 +63,6 @@
#undef MODULE_PARAM_PREFIX
#define MODULE_PARAM_PREFIX DEVICE_NAME "."
-#define VCHIQ_MINOR 0
-
/* Some per-instance constants */
#define MAX_COMPLETIONS 128
#define MAX_SERVICES 64
@@ -1950,195 +1948,150 @@ vchiq_compat_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
#endif
-/
-*
-* vchiq_open
-*
-***/
-
-static int
-vchiq_open(struct inode *inode, struct file *file)
+static int vchiq_open(struct inode *inode, struct file *file)
{
- int dev = iminor(inode) & 0x0f;
+ VCHIQ_STATE_T *state = vchiq_get_state();
+ VCHIQ_INSTANCE_T instance;
vchiq_log_info(vchiq_arm_log_level, "vchiq_open");
- switch (dev) {
- case VCHIQ_MINOR: {
- VCHIQ_STATE_T *state = vchiq_get_state();
- VCHIQ_INSTANCE_T instance;
- if (!state) {
- vchiq_log_error(vchiq_arm_log_level,
+ if (!state) {
+ vchiq_log_error(vchiq_arm_log_level,
"vchiq has no connection to VideoCore");
- return -ENOTCONN;
- }
-
- instance = kzalloc(sizeof(*instance), GFP_KERNEL);
- if (!instance)
- return -ENOMEM;
+ return -ENOTCONN;
+ }
- instance->state = state;
- instance->pid = current->tgid;
+ instance = kzalloc(sizeof(*instance), GFP_KERNEL);
+ if (!instance)
+ return -ENOMEM;
- vchiq_debugfs_add_instance(instance);
+ instance->state = state;
+ instance->pid = current->tgid;
- init_completion(&instance->insert_event);
- init_completion(&instance->remove_event);
- mutex_init(&instance->completion_mutex);
- mutex_init(&instance->bulk_waiter_list_mutex);
- INIT_LIST_HEAD(&instance->bulk_waiter_list);
+ vchiq_debugfs_add_instance(instance);
- file->private_data = instance;
- } break;
+ init_completion(&instance->insert_event);
+ init_completion(&instance->remove_event);
+ mutex_init(&instance->completion_mutex);
+ mutex_init(&instance->bulk_waiter_list_mutex);
+ INIT_LIST_HEAD(&instance->bulk_waiter_list);
- default:
- vchiq_log_error(vchiq_arm_log_level,
- "Unknown minor device: %d", dev);
- return -ENXIO;
- }
+ file->private_data = instance;
return 0;
}
-/
-*
-* vchiq_release
-*
-***/
-
-static int
-vchiq_release(struct inode *inode, struct file *file)
+static int vchiq_release(struct inode *inode, struct file *file)
{
- int dev = iminor(inode) & 0x0f;
+ VCHIQ_INSTANCE_T instance = file->private_data;
+ VCHIQ_STATE_T *state = vchiq_get_state();
+ VCHIQ_SERVICE_T *service;
int ret = 0;
+ int i;
- switch (dev) {
- case VCHIQ_MINOR: {
- VCHIQ_INSTANCE_T instance = file->private_data;
- VCHIQ_STATE_T *state = vchiq_get_state();
- VCHIQ_SERVICE_T *service;
- int i;
+ vchiq_log_info(vchiq_arm_log_level, "%s: instance=%lx", __func__,
+ (unsigned long)instance);
- vchiq_log_info(vchiq_arm_log_level,
- "%s: instance=%lx",
- __func__, (unsigned long)instance);
+ if (!state) {
+ ret = -EPERM;
+ goto out;
+ }
- if (!state) {
- ret = -EPERM;
- goto out;
- }
+ /* Ensure videocore is awake to allow termination. */
+ vchiq_use_internal(instance->state, NULL, USE_TYPE_VCHIQ);
- /* Ensur
[PATCH 07/16] staging: vchiq-core: get rid of is_master distinction
VCHIQ bulk transfers are what most people call DMA transfers. The CPU
sends a list of physical addresses to the VideoCore which then access
the memory directly without the need for CPU interaction. With this
setup we call the CPU the "slave" and the VideoCore the "master".
There seems to be an option to switch roles in vchiq. Which nobody is
using nor is properly implemented. So we get rid of the "is_master == 1"
option, and all the related code.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 12 +-
.../interface/vchiq_arm/vchiq_core.c | 300 +++---
.../interface/vchiq_arm/vchiq_core.h | 11 +-
3 files changed, 38 insertions(+), 285 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 014583cdf367..ecee54a31f8d 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -163,7 +163,7 @@ int vchiq_platform_init(struct platform_device *pdev,
VCHIQ_STATE_T *state)
*(char **)&g_fragments_base[i * g_fragments_size] = NULL;
sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
- if (vchiq_init_state(state, vchiq_slot_zero, 0) != VCHIQ_SUCCESS)
+ if (vchiq_init_state(state, vchiq_slot_zero) != VCHIQ_SUCCESS)
return -EINVAL;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -278,16 +278,6 @@ vchiq_complete_bulk(VCHIQ_BULK_T *bulk)
bulk->actual);
}
-void
-vchiq_transfer_bulk(VCHIQ_BULK_T *bulk)
-{
- /*
-* This should only be called on the master (VideoCore) side, but
-* provide an implementation to avoid the need for ifdefery.
-*/
- BUG();
-}
-
void
vchiq_dump_platform_state(void *dump_context)
{
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 8c7bda2e7cb6..34a892011296 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -85,8 +85,6 @@ int vchiq_core_log_level = VCHIQ_LOG_DEFAULT;
int vchiq_core_msg_log_level = VCHIQ_LOG_DEFAULT;
int vchiq_sync_log_level = VCHIQ_LOG_DEFAULT;
-static atomic_t pause_bulks_count = ATOMIC_INIT(0);
-
static DEFINE_SPINLOCK(service_spinlock);
DEFINE_SPINLOCK(bulk_waiter_spinlock);
static DEFINE_SPINLOCK(quota_spinlock);
@@ -1222,32 +1220,7 @@ notify_bulks(VCHIQ_SERVICE_T *service,
VCHIQ_BULK_QUEUE_T *queue,
(queue == &service->bulk_tx) ? 't' : 'r',
queue->process, queue->remote_notify, queue->remove);
- if (service->state->is_master) {
- while (queue->remote_notify != queue->process) {
- VCHIQ_BULK_T *bulk =
- &queue->bulks[BULK_INDEX(queue->remote_notify)];
- int msgtype = (bulk->dir == VCHIQ_BULK_TRANSMIT) ?
- VCHIQ_MSG_BULK_RX_DONE : VCHIQ_MSG_BULK_TX_DONE;
- int msgid = VCHIQ_MAKE_MSG(msgtype, service->localport,
- service->remoteport);
- /* Only reply to non-dummy bulk requests */
- if (bulk->remote_data) {
- status = queue_message(
- service->state,
- NULL,
- msgid,
- memcpy_copy_callback,
- &bulk->actual,
- 4,
- 0);
- if (status != VCHIQ_SUCCESS)
- break;
- }
- queue->remote_notify++;
- }
- } else {
- queue->remote_notify = queue->process;
- }
+ queue->remote_notify = queue->process;
if (status == VCHIQ_SUCCESS) {
while (queue->remove != queue->remote_notify) {
@@ -1385,63 +1358,6 @@ poll_services(VCHIQ_STATE_T *state)
}
}
-/* Called by the slot handler or application threads, holding the bulk mutex.
*/
-static int
-resolve_bulks(VCHIQ_SERVICE_T *service, VCHIQ_BULK_QUEUE_T *queue)
-{
- VCHIQ_STATE_T *state = service->state;
- int resolved = 0;
-
- while ((queue->process != queue->local_insert) &&
- (queue->process != queue->remote_insert)) {
- VCHIQ_BULK_T
[PATCH 02/16] staging: vchiq_arm: rework close/remove_service IOCTLS
The implementation of both IOCTLS was the same except for one function
call. This joins both implementations and updates the code to avoid
unneeded indentations.
Signed-off-by: Nicolas Saenz Julienne
---
.../interface/vchiq_arm/vchiq_arm.c | 66 +++
1 file changed, 24 insertions(+), 42 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 6d503392341e..d88dd7415e1e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1019,55 +1019,37 @@ vchiq_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
}
} break;
- case VCHIQ_IOC_CLOSE_SERVICE: {
+ case VCHIQ_IOC_CLOSE_SERVICE:
+ case VCHIQ_IOC_REMOVE_SERVICE: {
VCHIQ_SERVICE_HANDLE_T handle = (VCHIQ_SERVICE_HANDLE_T)arg;
+ USER_SERVICE_T *user_service;
service = find_service_for_instance(instance, handle);
- if (service != NULL) {
- USER_SERVICE_T *user_service =
- (USER_SERVICE_T *)service->base.userdata;
- /* close_pending is false on first entry, and when the
- wait in vchiq_close_service has been interrupted. */
- if (!user_service->close_pending) {
- status = vchiq_close_service(service->handle);
- if (status != VCHIQ_SUCCESS)
- break;
- }
-
- /* close_pending is true once the underlying service
- has been closed until the client library calls the
- CLOSE_DELIVERED ioctl, signalling close_event. */
- if (user_service->close_pending &&
- down_interruptible(&user_service->close_event))
- status = VCHIQ_RETRY;
- } else
+ if (!service) {
ret = -EINVAL;
- } break;
+ break;
+ }
- case VCHIQ_IOC_REMOVE_SERVICE: {
- VCHIQ_SERVICE_HANDLE_T handle = (VCHIQ_SERVICE_HANDLE_T)arg;
+ user_service = service->base.userdata;
- service = find_service_for_instance(instance, handle);
- if (service != NULL) {
- USER_SERVICE_T *user_service =
- (USER_SERVICE_T *)service->base.userdata;
- /* close_pending is false on first entry, and when the
- wait in vchiq_close_service has been interrupted. */
- if (!user_service->close_pending) {
- status = vchiq_remove_service(service->handle);
- if (status != VCHIQ_SUCCESS)
- break;
- }
+ /* close_pending is false on first entry, and when the
+ wait in vchiq_close_service has been interrupted. */
+ if (!user_service->close_pending) {
+ status = (cmd == VCHIQ_IOC_CLOSE_SERVICE) ?
+vchiq_close_service(service->handle) :
+vchiq_remove_service(service->handle);
+ if (status != VCHIQ_SUCCESS)
+ break;
+ }
- /* close_pending is true once the underlying service
- has been closed until the client library calls the
- CLOSE_DELIVERED ioctl, signalling close_event. */
- if (user_service->close_pending &&
- down_interruptible(&user_service->close_event))
- status = VCHIQ_RETRY;
- } else
- ret = -EINVAL;
- } break;
+ /* close_pending is true once the underlying service
+ has been closed until the client library calls the
+ CLOSE_DELIVERED ioctl, signalling close_event. */
+ if (user_service->close_pending &&
+ down_interruptible(&user_service->close_event))
+ status = VCHIQ_RETRY;
+ break;
+ }
case VCHIQ_IOC_USE_SERVICE:
case VCHIQ_IOC_RELEASE_SERVICE: {
--
2.19.1
Re: [PATCH RFC 09/18] staging: vchiq_core: do not initialize semaphores twice
On Tue, 2018-11-06 at 17:06 +0100, Stefan Wahren wrote:
> Am 06.11.18 um 16:41 schrieb Nicolas Saenz Julienne:
> > Hi Stefan,
> > thanks for spending the time reviewing the code. I took note of the
> > rest of comments.
> >
> > On Sun, 2018-10-28 at 21:45 +0100, Stefan Wahren wrote:
> > > Hi Nicolas,
> > >
> > > > Nicolas Saenz Julienne hat am 26.
> > > > Oktober
> > > > 2018 um 15:48 geschrieben:
> > > >
> > > >
> > > > vchiq_init_state() initialises a series of semaphores to then
> > > > call
> > > > remote_event_create() on the same semaphores, which initializes
> > > > them
> > > > again.
> > > i would prefer to have all init stuff at one place in
> > > vchiq_init_state() and drop this ugliness from
> > > remote_event_create()
> > > instead. Is this possible?
> > As I'm sure you're aware of, REMOTE_EVENT_T is shared between the
> > CPU
> > and VC4, which can't be expanded. And since storing a pointer is
> > out of
> > question because of arm64, I can only think of storing an index to
> > an
> > array of completions in the shared structure instead of the pointer
> > magic implemented right now. It would be a little more explicit.
> > Then
> > we could completely decouple both initializations. I'm not sure if
> > it's
> > similar to what you had in mind.
>
> I don't think so, this was my intention:
>
> static inline void
> remote_event_create(VCHIQ_STATE_T *state, REMOTE_EVENT_T *event)
> {
> event->armed = 0;
> /* Don't clear the 'fired' flag because it may already have been
> set
> ** by the other side. */
> -sema_init((struct semaphore *)((char *)state + event->event),
> 0);
> }
Fair enough, even simpler.
>
>
> > On a semi-related topic, I'm curious to know why these shared
> > structures aren't set with the "__packed" preprocessor macro. Any
> > ideas? As fas as I've been told, in general, the compiler may
> > reorder
> > or add unexpected padding to any structure. Which would be very bad
> > in
> > this case.
>
> This would be better, but i assume the firmware side uses the same
> source code. So using __packed only on ARM side could also break :-(
True. Yet in that case, aren't we still relying on the fact that both
compilers are going to behave the same way?
>
> > Regards,
> > Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH RFC 09/18] staging: vchiq_core: do not initialize semaphores twice
Hi Stefan, thanks for spending the time reviewing the code. I took note of the rest of comments. On Sun, 2018-10-28 at 21:45 +0100, Stefan Wahren wrote: > Hi Nicolas, > > > Nicolas Saenz Julienne hat am 26. Oktober > > 2018 um 15:48 geschrieben: > > > > > > vchiq_init_state() initialises a series of semaphores to then call > > remote_event_create() on the same semaphores, which initializes > > them > > again. > > i would prefer to have all init stuff at one place in > vchiq_init_state() and drop this ugliness from remote_event_create() > instead. Is this possible? As I'm sure you're aware of, REMOTE_EVENT_T is shared between the CPU and VC4, which can't be expanded. And since storing a pointer is out of question because of arm64, I can only think of storing an index to an array of completions in the shared structure instead of the pointer magic implemented right now. It would be a little more explicit. Then we could completely decouple both initializations. I'm not sure if it's similar to what you had in mind. On a semi-related topic, I'm curious to know why these shared structures aren't set with the "__packed" preprocessor macro. Any ideas? As fas as I've been told, in general, the compiler may reorder or add unexpected padding to any structure. Which would be very bad in this case. Regards, Nicolas signature.asc Description: This is a digitally signed message part
