[PATCH] pci/switchtec: Don't use completion's wait queue

[PATCH] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

The poll callback is using completion's wait_queue_head_t member and
puts it in poll_wait() so the poll() caller gets a wakeup after command
completed. This does not work on RT because we don't have a
wait_queue_head_t in our completion implementation. Nobody in tree does
like that in tree so this is the only driver that breaks.

>From reading the code, the completion is used to signal to the waiter
that a command completed. I tried to wait_queue_head_t cmd_comp instead
using the completion for that. There is a cmd_cnt which is incremented
after each command completed and the user "copies" the value once the
read was successful.

I don't have the HW so I have no idea if it works as expected, so please
test it.
It looks like both wait queues (event_wq and cmd_comp) could be merged
into one. The events can be distinguished and the "command complete"
should be the event with the higher frequency. event_wq is only used on
poll.

Cc: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
Cc: Stephen Bates <stephen.bates@microsemi.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/pci/switch/switchtec.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index af81b2dec42e..879330eb77bd 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -286,7 +286,9 @@ struct switchtec_dev {
 	bool alive;
=20
 	wait_queue_head_t event_wq;
+	wait_queue_head_t cmd_comp;
 	atomic_t event_cnt;
+	atomic_t cmd_cnt;
 };
=20
 static struct switchtec_dev *to_stdev(struct device *dev)
@@ -306,7 +308,6 @@ struct switchtec_user {
=20
 	enum mrpc_state state;
=20
-	struct completion comp;
 	struct kref kref;
 	struct list_head list;
=20
@@ -317,6 +318,7 @@ struct switchtec_user {
 	size_t read_len;
 	unsigned char data[SWITCHTEC_MRPC_PAYLOAD_SIZE];
 	int event_cnt;
+	int cmd_cnt;
 };
=20
 static struct switchtec_user *stuser_create(struct switchtec_dev *stdev)
@@ -331,8 +333,8 @@ static struct switchtec_user *stuser_create(struct swit=
chtec_dev *stdev)
 	stuser->stdev =3D stdev;
 	kref_init(&stuser->kref);
 	INIT_LIST_HEAD(&stuser->list);
-	init_completion(&stuser->comp);
 	stuser->event_cnt =3D atomic_read(&stdev->event_cnt);
+	stuser->cmd_cnt =3D atomic_read(&stdev->cmd_cnt);
=20
 	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
=20
@@ -414,7 +416,6 @@ static int mrpc_queue_cmd(struct switchtec_user *stuser)
 	kref_get(&stuser->kref);
 	stuser->read_len =3D sizeof(stuser->data);
 	stuser_set_state(stuser, MRPC_QUEUED);
-	init_completion(&stuser->comp);
 	list_add_tail(&stuser->list, &stdev->mrpc_queue);
=20
 	mrpc_cmd_submit(stdev);
@@ -451,7 +452,8 @@ static void mrpc_complete_cmd(struct switchtec_dev *std=
ev)
 		      stuser->read_len);
=20
 out:
-	complete_all(&stuser->comp);
+	atomic_inc(&stdev->cmd_cnt);
+	wake_up_interruptible(&stdev->cmd_comp);
 	list_del_init(&stuser->list);
 	stuser_put(stuser);
 	stdev->mrpc_busy =3D 0;
@@ -721,10 +723,11 @@ static ssize_t switchtec_dev_read(struct file *filp, =
char __user *data,
 	mutex_unlock(&stdev->mrpc_mutex);
=20
 	if (filp->f_flags & O_NONBLOCK) {
-		if (!try_wait_for_completion(&stuser->comp))
+		if (stuser->cmd_cnt =3D=3D atomic_read(&stdev->cmd_cnt))
 			return -EAGAIN;
 	} else {
-		rc =3D wait_for_completion_interruptible(&stuser->comp);
+		rc =3D wait_event_interruptible(stdev->cmd_comp,
+			      stuser->cmd_cnt !=3D atomic_read(&stdev->cmd_cnt));
 		if (rc < 0)
 			return rc;
 	}
@@ -752,7 +755,7 @@ static ssize_t switchtec_dev_read(struct file *filp, ch=
ar __user *data,
 		rc =3D -EFAULT;
 		goto out;
 	}
-
+	stuser->cmd_cnt =3D atomic_read(&stdev->cmd_cnt);
 	stuser_set_state(stuser, MRPC_IDLE);
=20
 out:
@@ -772,7 +775,7 @@ static unsigned int switchtec_dev_poll(struct file *fil=
p, poll_table *wait)
 	struct switchtec_dev *stdev =3D stuser->stdev;
 	int ret =3D 0;
=20
-	poll_wait(filp, &stuser->comp.wait, wait);
+	poll_wait(filp, &stdev->cmd_comp, wait);
 	poll_wait(filp, &stdev->event_wq, wait);
=20
 	if (lock_mutex_and_test_alive(stdev))
@@ -780,7 +783,7 @@ static unsigned int switchtec_dev_poll(struct file *fil=
p, poll_table *wait)
=20
 	mutex_unlock(&stdev->mrpc_mutex);
=20
-	if (try_wait_for_completion(&stuser->comp))
+	if (stuser->cmd_cnt !=3D atomic_read(&stdev->cmd_cnt))
 		ret |=3D POLLIN | POLLRDNORM;
=20
 	if (stuser->event_cnt !=3D atomic_read(&stdev->event_cnt))
@@ -1255,7 +1258,6 @@ static void stdev_kill(struct switchtec_dev *stdev)
=20
 	/* Wake up and kill any users waiting on an MRPC request */
 	list_for_each_entry_safe(stuser, tmpuser, &stdev->mrpc_queue, list) {
-		complete_all(&stuser->comp);
 		list_del_init(&stuser->list);
 		stuser_put(stuser);
 	}
@@ -1264,6 +1266,7 @@ static void stdev_kill(struct switchtec_dev *stdev)
=20
 	/* Wake up any users waiting on event_wq */
 	wake_up_interruptible(&stdev->event_wq);
+	wake_up_interruptible(&stdev->cmd_comp);
 }
=20
 static struct switchtec_dev *stdev_create(struct pci_dev *pdev)
@@ -1287,7 +1290,9 @@ static struct switchtec_dev *stdev_create(struct pci_=
dev *pdev)
 	INIT_WORK(&stdev->mrpc_work, mrpc_event_work);
 	INIT_DELAYED_WORK(&stdev->mrpc_timeout, mrpc_timeout_work);
 	init_waitqueue_head(&stdev->event_wq);
+	init_waitqueue_head(&stdev->cmd_comp);
 	atomic_set(&stdev->event_cnt, 0);
+	atomic_set(&stdev->cmd_cnt, 0);
=20
 	dev =3D &stdev->dev;
 	device_initialize(dev);
--=20
2.14.2

Re: [PATCH] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

On 04/10/17 03:50 AM, Sebastian Andrzej Siewior wrote:
> The poll callback is using completion's wait_queue_head_t member and
> puts it in poll_wait() so the poll() caller gets a wakeup after command
> completed. This does not work on RT because we don't have a
> wait_queue_head_t in our completion implementation. Nobody in tree does
> like that in tree so this is the only driver that breaks.

I'm sorry but this seems a bit crazy to me. Driver's can't poll on 
completions because an out of tree implementation changes them in a 
weird way??! Just because no one in-tree does it now doesn't make it 
invalid.

But at the very least, if we _absolutely_ have to patch this out, you 
shouldn't make it so convoluted. Just replace the struct completion with 
a wait queue and a done flag (which is what a completion is). Don't move 
the wait queue into switchtec_dev and don't use an unnecessary counter. 
And certainly don't think about merging it with another wait queue that 
has completely different wake events and waiters.

In any case, I haven't tested the patch, but I'm pretty sure it's not 
correct. There can be multiple switchtec_user's queued up and this patch 
will essentially complete them all once any one of them finishes and 
cmd_cnt increments.

So this patch gets a NAK from me.

Logan

[PATCH v2] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

The poll callback is using completion's wait_queue_head_t member and
puts it in poll_wait() so the poll() caller gets a wakeup after command
completed. This does not work on RT because we don't have a
wait_queue_head_t in our completion implementation. Nobody in tree does
like that in tree so this is the only driver that breaks.

Instead of using the completion here is waitqueue with a status flag as
suggested by Logan.

I don't have the HW so I have no idea if it works as expected, so please
test it.

Cc: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
On 2017-10-04 10:13:15 [-0600], Logan Gunthorpe wrote:

> But at the very least, if we _absolutely_ have to patch this out, you
> shouldn't make it so convoluted. Just replace the struct completion with a
> wait queue and a done flag (which is what a completion is). Don't move the
> wait queue into switchtec_dev and don't use an unnecessary counter. And
> certainly don't think about merging it with another wait queue that has
> completely different wake events and waiters.

here it is.

v1…v2: use a status flag and a wait_queue_head_t instead as suggested by
       Logan.

 drivers/pci/switch/switchtec.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index af81b2dec42e..4c843fca709a 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -306,10 +306,11 @@ struct switchtec_user {
 
 	enum mrpc_state state;
 
-	struct completion comp;
+	wait_queue_head_t cmd_comp;
 	struct kref kref;
 	struct list_head list;
 
+	bool cmd_done;
 	u32 cmd;
 	u32 status;
 	u32 return_code;
@@ -331,7 +332,7 @@ static struct switchtec_user *stuser_create(struct switchtec_dev *stdev)
 	stuser->stdev = stdev;
 	kref_init(&stuser->kref);
 	INIT_LIST_HEAD(&stuser->list);
-	init_completion(&stuser->comp);
+	init_waitqueue_head(&stuser->cmd_comp);
 	stuser->event_cnt = atomic_read(&stdev->event_cnt);
 
 	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
@@ -414,7 +415,7 @@ static int mrpc_queue_cmd(struct switchtec_user *stuser)
 	kref_get(&stuser->kref);
 	stuser->read_len = sizeof(stuser->data);
 	stuser_set_state(stuser, MRPC_QUEUED);
-	init_completion(&stuser->comp);
+	stuser->cmd_done = false;
 	list_add_tail(&stuser->list, &stdev->mrpc_queue);
 
 	mrpc_cmd_submit(stdev);
@@ -451,7 +452,7 @@ static void mrpc_complete_cmd(struct switchtec_dev *stdev)
 		      stuser->read_len);
 
 out:
-	complete_all(&stuser->comp);
+	wake_up_interruptible(&stuser->cmd_comp);
 	list_del_init(&stuser->list);
 	stuser_put(stuser);
 	stdev->mrpc_busy = 0;
@@ -721,10 +722,11 @@ static ssize_t switchtec_dev_read(struct file *filp, char __user *data,
 	mutex_unlock(&stdev->mrpc_mutex);
 
 	if (filp->f_flags & O_NONBLOCK) {
-		if (!try_wait_for_completion(&stuser->comp))
+		if (!stuser->cmd_done)
 			return -EAGAIN;
 	} else {
-		rc = wait_for_completion_interruptible(&stuser->comp);
+		rc = wait_event_interruptible(stuser->cmd_comp,
+					      stuser->cmd_done);
 		if (rc < 0)
 			return rc;
 	}
@@ -772,7 +774,7 @@ static unsigned int switchtec_dev_poll(struct file *filp, poll_table *wait)
 	struct switchtec_dev *stdev = stuser->stdev;
 	int ret = 0;
 
-	poll_wait(filp, &stuser->comp.wait, wait);
+	poll_wait(filp, &stuser->cmd_comp, wait);
 	poll_wait(filp, &stdev->event_wq, wait);
 
 	if (lock_mutex_and_test_alive(stdev))
@@ -780,7 +782,7 @@ static unsigned int switchtec_dev_poll(struct file *filp, poll_table *wait)
 
 	mutex_unlock(&stdev->mrpc_mutex);
 
-	if (try_wait_for_completion(&stuser->comp))
+	if (stuser->cmd_done)
 		ret |= POLLIN | POLLRDNORM;
 
 	if (stuser->event_cnt != atomic_read(&stdev->event_cnt))
@@ -1255,7 +1257,8 @@ static void stdev_kill(struct switchtec_dev *stdev)
 
 	/* Wake up and kill any users waiting on an MRPC request */
 	list_for_each_entry_safe(stuser, tmpuser, &stdev->mrpc_queue, list) {
-		complete_all(&stuser->comp);
+		stuser->cmd_done = true;
+		wake_up_interruptible(&stuser->cmd_comp);
 		list_del_init(&stuser->list);
 		stuser_put(stuser);
 	}
-- 
2.14.2

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

I still don't want this merged as it makes the code less clear, but I 
tested it and it does not work.

On 05/10/17 04:51 AM, Sebastian Andrzej Siewior wrote:

> @@ -451,7 +452,7 @@ static void mrpc_complete_cmd(struct switchtec_dev *stdev)
>   		      stuser->read_len);
>   
>   out:
> -	complete_all(&stuser->comp);
> +	wake_up_interruptible(&stuser->cmd_comp);

I think you forgot to set cmd_done here. Once I add that, it did start 
working correctly.

>   	list_del_init(&stuser->list);
>   	stuser_put(stuser);
>   	stdev->mrpc_busy = 0;
> @@ -721,10 +722,11 @@ static ssize_t switchtec_dev_read(struct file *filp, char __user *data,
>   	mutex_unlock(&stdev->mrpc_mutex);
>   
>   	if (filp->f_flags & O_NONBLOCK) {
> -		if (!try_wait_for_completion(&stuser->comp))
> +		if (!stuser->cmd_done)
>   			return -EAGAIN;

I'd probably also suggest using READ_ONCE when reading cmd_done.

Logan

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

On 2017-10-05 12:46:34 [-0600], Logan Gunthorpe wrote:
> I still don't want this merged as it makes the code less clear, but I tested
> it and it does not work.
> 
> On 05/10/17 04:51 AM, Sebastian Andrzej Siewior wrote:
> 
> > @@ -451,7 +452,7 @@ static void mrpc_complete_cmd(struct switchtec_dev *stdev)
> >   		      stuser->read_len);
> >   out:
> > -	complete_all(&stuser->comp);
> > +	wake_up_interruptible(&stuser->cmd_comp);
> 
> I think you forgot to set cmd_done here. Once I add that, it did start
> working correctly.

oh, makes sense.

> >   	list_del_init(&stuser->list);
> >   	stuser_put(stuser);
> >   	stdev->mrpc_busy = 0;
> > @@ -721,10 +722,11 @@ static ssize_t switchtec_dev_read(struct file *filp, char __user *data,
> >   	mutex_unlock(&stdev->mrpc_mutex);
> >   	if (filp->f_flags & O_NONBLOCK) {
> > -		if (!try_wait_for_completion(&stuser->comp))
> > +		if (!stuser->cmd_done)
> >   			return -EAGAIN;
> 
> I'd probably also suggest using READ_ONCE when reading cmd_done.

Why so? There is no way the compiler could have cached the value
somehow.

> Logan

Sebastian

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

On 11/2/2017 9:07 AM, Sebastian Andrzej Siewior wrote:
> Why so? There is no way the compiler could have cached the value
> somehow.

Well, I think it would be sufficient to notice that the code you were 
replacing used a READ_ONCE and you're putting it into a function that's 
a bit more complicated. But I'd suggest you read [1] which essentially 
says that all shared variables should be accessed with such a macro even 
if only to document that it's a part of a thread synchronization protocol.

Logan


[1] https://github.com/google/ktsan/wiki/READ_ONCE-and-WRITE_ONCE

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

On 2017-11-02 10:03:53 [-0600], Logan Gunthorpe wrote:
> On 11/2/2017 9:07 AM, Sebastian Andrzej Siewior wrote:
> > Why so? There is no way the compiler could have cached the value
> > somehow.
> 
> Well, I think it would be sufficient to notice that the code you were
> replacing used a READ_ONCE and you're putting it into a function that's a
> bit more complicated. But I'd suggest you read [1] which essentially says
> that all shared variables should be accessed with such a macro even if only
> to document that it's a part of a thread synchronization protocol.

It used READ_ONCE in case the code would have been inlined within a loop
which makes sense to some degree. It is not the case here. But hey I can
add, no problem…

> Logan

Sebastian

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

On 11/2/2017 11:17 AM, Sebastian Andrzej Siewior wrote:
>> Well, I think it would be sufficient to notice that the code you were
>> replacing used a READ_ONCE and you're putting it into a function that's a
>> bit more complicated. But I'd suggest you read [1] which essentially says
>> that all shared variables should be accessed with such a macro even if only
>> to document that it's a part of a thread synchronization protocol.
> 
> It used READ_ONCE in case the code would have been inlined within a loop
> which makes sense to some degree. It is not the case here. But hey I can
> add, no problem…

Well, you clearly didn't read the article I sent or even what I said. 
But your reasoning is a bit flawed seeing try_wait_for_completion() is 
not designed to be inlined and will never be inlined unless the kernel 
is compiled with link-time optimization which I don't think many people use.

Logan

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

On 2017-11-02 11:23:16 [-0600], Logan Gunthorpe wrote:
> 
> Well, you clearly didn't read the article I sent or even what I said. But

I did, I just didn't manage to make the link.
The futex example looks like compiler optimisation which should not have
happen and would be ideal for READ_ONCE, yes. But this is not the case
here and this is what I pointed out.
And the ps2 example, well it did not check condition after the wait so
it was not obvious if the condition (for which it was waiting) was true or not.
So if you want this thing to be catched by tools, I don't know how this bug

|	int rc = -1;
|	...
|	wait_event_timeout(ps2dev->wait, !(READ_ONCE(ps2dev->flags) & PS2_FLAG_CMD), timeout);
|	for (i = 0; i < receive; i++)
|		resp[i] = ps2dev->cmdbuf[(receive - 1) - i];
|	if (READ_ONCE(ps2dev->cmdcnt))
|		goto out;
|	rc = 0;
|out:
|	return rc;

could have spotted by anything but a human.

> your reasoning is a bit flawed seeing try_wait_for_completion() is not
> designed to be inlined and will never be inlined unless the kernel is
> compiled with link-time optimization which I don't think many people use.

See 7c34e3180a01 ("sched/completion: Add lock-free checking of the
blocking case")

> Logan

Sebastian

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

On 02/11/17 12:05 PM, Sebastian Andrzej Siewior wrote:
> On 2017-11-02 11:23:16 [-0600], Logan Gunthorpe wrote:
>>
>> Well, you clearly didn't read the article I sent or even what I said. But
> 
> I did, I just didn't manage to make the link.
> The futex example looks like compiler optimisation which should not have
> happen and would be ideal for READ_ONCE, yes. But this is not the case
> here and this is what I pointed out.

And you still missed what I said and the entire point of the article.

> See 7c34e3180a01 ("sched/completion: Add lock-free checking of the
> blocking case")

Sounds questionable. I don't see how that function could be inlined, 
even then.

Logan

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

On 2017-11-02 14:02:23 [-0600], Logan Gunthorpe wrote:
> And you still missed what I said and the entire point of the article.

I'm sorry.

> > See 7c34e3180a01 ("sched/completion: Add lock-free checking of the
> > blocking case")
> 
> Sounds questionable. I don't see how that function could be inlined, even
> then.

It would have made sense if that function would be used also within the
file where it was declared but it is not the case.
So you see the questionable way the READ_ONCE got in there and now you
may understand that I did not see much of big loss if it wasn't there
after the rewrite/change.

> Logan

Sebastian

Re: [PATCH v2] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

On 02/11/17 02:53 PM, Sebastian Andrzej Siewior wrote:
> It would have made sense if that function would be used also within the
> file where it was declared but it is not the case.
> So you see the questionable way the READ_ONCE got in there and now you
> may understand that I did not see much of big loss if it wasn't there
> after the rewrite/change.

Yes, but I feel like it still makes sense for it to be there (per the 
article I linked), despite the incorrect justification.

Logan

[PATCH v3] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

The poll callback is using completion's wait_queue_head_t member and
puts it in poll_wait() so the poll() caller gets a wakeup after command
completed. This does not work on RT because we don't have a
wait_queue_head_t in our completion implementation. Nobody in tree does
like that in tree so this is the only driver that breaks.

Instead of using the completion here is waitqueue with a status flag as
suggested by Logan.

I don't have the HW so I have no idea if it works as expected, so please
test it.

Cc: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3: add a READ_ONCE while accessing ->cmd_done

 drivers/pci/switch/switchtec.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -306,10 +306,11 @@ struct switchtec_user {
 
 	enum mrpc_state state;
 
-	struct completion comp;
+	wait_queue_head_t cmd_comp;
 	struct kref kref;
 	struct list_head list;
 
+	bool cmd_done;
 	u32 cmd;
 	u32 status;
 	u32 return_code;
@@ -331,7 +332,7 @@ static struct switchtec_user *stuser_cre
 	stuser->stdev = stdev;
 	kref_init(&stuser->kref);
 	INIT_LIST_HEAD(&stuser->list);
-	init_completion(&stuser->comp);
+	init_waitqueue_head(&stuser->cmd_comp);
 	stuser->event_cnt = atomic_read(&stdev->event_cnt);
 
 	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
@@ -414,7 +415,7 @@ static int mrpc_queue_cmd(struct switcht
 	kref_get(&stuser->kref);
 	stuser->read_len = sizeof(stuser->data);
 	stuser_set_state(stuser, MRPC_QUEUED);
-	init_completion(&stuser->comp);
+	stuser->cmd_done = false;
 	list_add_tail(&stuser->list, &stdev->mrpc_queue);
 
 	mrpc_cmd_submit(stdev);
@@ -451,7 +452,8 @@ static void mrpc_complete_cmd(struct swi
 		      stuser->read_len);
 
 out:
-	complete_all(&stuser->comp);
+	stuser->cmd_done = true;
+	wake_up_interruptible(&stuser->cmd_comp);
 	list_del_init(&stuser->list);
 	stuser_put(stuser);
 	stdev->mrpc_busy = 0;
@@ -721,10 +723,11 @@ static ssize_t switchtec_dev_read(struct
 	mutex_unlock(&stdev->mrpc_mutex);
 
 	if (filp->f_flags & O_NONBLOCK) {
-		if (!try_wait_for_completion(&stuser->comp))
+		if (!READ_ONCE(stuser->cmd_done))
 			return -EAGAIN;
 	} else {
-		rc = wait_for_completion_interruptible(&stuser->comp);
+		rc = wait_event_interruptible(stuser->cmd_comp,
+					      stuser->cmd_done);
 		if (rc < 0)
 			return rc;
 	}
@@ -772,7 +775,7 @@ static unsigned int switchtec_dev_poll(s
 	struct switchtec_dev *stdev = stuser->stdev;
 	int ret = 0;
 
-	poll_wait(filp, &stuser->comp.wait, wait);
+	poll_wait(filp, &stuser->cmd_comp, wait);
 	poll_wait(filp, &stdev->event_wq, wait);
 
 	if (lock_mutex_and_test_alive(stdev))
@@ -780,7 +783,7 @@ static unsigned int switchtec_dev_poll(s
 
 	mutex_unlock(&stdev->mrpc_mutex);
 
-	if (try_wait_for_completion(&stuser->comp))
+	if (READ_ONCE(stuser->cmd_done))
 		ret |= POLLIN | POLLRDNORM;
 
 	if (stuser->event_cnt != atomic_read(&stdev->event_cnt))
@@ -1255,7 +1258,8 @@ static void stdev_kill(struct switchtec_
 
 	/* Wake up and kill any users waiting on an MRPC request */
 	list_for_each_entry_safe(stuser, tmpuser, &stdev->mrpc_queue, list) {
-		complete_all(&stuser->comp);
+		stuser->cmd_done = true;
+		wake_up_interruptible(&stuser->cmd_comp);
 		list_del_init(&stuser->list);
 		stuser_put(stuser);
 	}

Re: [PATCH v3] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

Hey,

Note: This patch should have been v4.

I tested it and it now works.

However, this whole concept still gets a NAK from me. I think it makes 
the code less clear for no obvious reason.

I feel if RT needs to change the completions they should make two 
versions waitable and non-waitable and use them where appropriate. I'd 
much rather not push the completion logic into the driver layer.

Logan

On 02/11/17 11:19 AM, Sebastian Andrzej Siewior wrote:
> The poll callback is using completion's wait_queue_head_t member and
> puts it in poll_wait() so the poll() caller gets a wakeup after command
> completed. This does not work on RT because we don't have a
> wait_queue_head_t in our completion implementation. Nobody in tree does
> like that in tree so this is the only driver that breaks.
> 
> Instead of using the completion here is waitqueue with a status flag as
> suggested by Logan.
> 
> I don't have the HW so I have no idea if it works as expected, so please
> test it.
> 
> Cc: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v2…v3: add a READ_ONCE while accessing ->cmd_done
> 
>   drivers/pci/switch/switchtec.c |   22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
> 
> --- a/drivers/pci/switch/switchtec.c
> +++ b/drivers/pci/switch/switchtec.c
> @@ -306,10 +306,11 @@ struct switchtec_user {
>   
>   	enum mrpc_state state;
>   
> -	struct completion comp;
> +	wait_queue_head_t cmd_comp;
>   	struct kref kref;
>   	struct list_head list;
>   
> +	bool cmd_done;
>   	u32 cmd;
>   	u32 status;
>   	u32 return_code;
> @@ -331,7 +332,7 @@ static struct switchtec_user *stuser_cre
>   	stuser->stdev = stdev;
>   	kref_init(&stuser->kref);
>   	INIT_LIST_HEAD(&stuser->list);
> -	init_completion(&stuser->comp);
> +	init_waitqueue_head(&stuser->cmd_comp);
>   	stuser->event_cnt = atomic_read(&stdev->event_cnt);
>   
>   	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
> @@ -414,7 +415,7 @@ static int mrpc_queue_cmd(struct switcht
>   	kref_get(&stuser->kref);
>   	stuser->read_len = sizeof(stuser->data);
>   	stuser_set_state(stuser, MRPC_QUEUED);
> -	init_completion(&stuser->comp);
> +	stuser->cmd_done = false;
>   	list_add_tail(&stuser->list, &stdev->mrpc_queue);
>   
>   	mrpc_cmd_submit(stdev);
> @@ -451,7 +452,8 @@ static void mrpc_complete_cmd(struct swi
>   		      stuser->read_len);
>   
>   out:
> -	complete_all(&stuser->comp);
> +	stuser->cmd_done = true;
> +	wake_up_interruptible(&stuser->cmd_comp);
>   	list_del_init(&stuser->list);
>   	stuser_put(stuser);
>   	stdev->mrpc_busy = 0;
> @@ -721,10 +723,11 @@ static ssize_t switchtec_dev_read(struct
>   	mutex_unlock(&stdev->mrpc_mutex);
>   
>   	if (filp->f_flags & O_NONBLOCK) {
> -		if (!try_wait_for_completion(&stuser->comp))
> +		if (!READ_ONCE(stuser->cmd_done))
>   			return -EAGAIN;
>   	} else {
> -		rc = wait_for_completion_interruptible(&stuser->comp);
> +		rc = wait_event_interruptible(stuser->cmd_comp,
> +					      stuser->cmd_done);
>   		if (rc < 0)
>   			return rc;
>   	}
> @@ -772,7 +775,7 @@ static unsigned int switchtec_dev_poll(s
>   	struct switchtec_dev *stdev = stuser->stdev;
>   	int ret = 0;
>   
> -	poll_wait(filp, &stuser->comp.wait, wait);
> +	poll_wait(filp, &stuser->cmd_comp, wait);
>   	poll_wait(filp, &stdev->event_wq, wait);
>   
>   	if (lock_mutex_and_test_alive(stdev))
> @@ -780,7 +783,7 @@ static unsigned int switchtec_dev_poll(s
>   
>   	mutex_unlock(&stdev->mrpc_mutex);
>   
> -	if (try_wait_for_completion(&stuser->comp))
> +	if (READ_ONCE(stuser->cmd_done))
>   		ret |= POLLIN | POLLRDNORM;
>   
>   	if (stuser->event_cnt != atomic_read(&stdev->event_cnt))
> @@ -1255,7 +1258,8 @@ static void stdev_kill(struct switchtec_
>   
>   	/* Wake up and kill any users waiting on an MRPC request */
>   	list_for_each_entry_safe(stuser, tmpuser, &stdev->mrpc_queue, list) {
> -		complete_all(&stuser->comp);
> +		stuser->cmd_done = true;
> +		wake_up_interruptible(&stuser->cmd_comp);
>   		list_del_init(&stuser->list);
>   		stuser_put(stuser);
>   	}
> 

Re: [PATCH v3] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

On 2017-11-02 14:53:19 [-0600], Logan Gunthorpe wrote:
> Hey,
Hey Logan,

> Note: This patch should have been v4.
> 
> I tested it and it now works.

good.

> However, this whole concept still gets a NAK from me. I think it makes the
> code less clear for no obvious reason.

Thank you for letting me go all that way…

> I feel if RT needs to change the completions they should make two versions
> waitable and non-waitable and use them where appropriate. I'd much rather
> not push the completion logic into the driver layer.

You should not touch the inside of an implementation.

> Logan

Sebastian

Re: [PATCH v3] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

On 03/11/17 02:22 AM, Sebastian Andrzej Siewior wrote:
> On 2017-11-02 14:53:19 [-0600], Logan Gunthorpe wrote:
> Thank you for letting me go all that way…

Sorry, but in fairness I did have the same feedback to your first patch 
which you never addressed. This patch would have to be part of a well 
justified patch-set changing how completions works. But, even then, I'd 
rather see a better solution that still allows at least some type of 
completion to be polled.

> You should not touch the inside of an implementation.

This is really only sometimes true[1]. But I feel it's a moot point: had 
I created a poll_on_completion() accessor function, you'd still be in 
the same situation. Frankly, I'm shocked no one else in the kernel is 
polling on a completion, it seems like something that would be common.

Logan


[1]"So it is essential that data types are not overly abstracted, but 
that all details of the implementation are visible so that the 
programmer can make optimal choices when using them."
  -- https://lwn.net/Articles/336255/

Re: [PATCH v3] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

On 2017-11-03 10:06:19 [-0600], Logan Gunthorpe wrote:
> 
> Sorry, but in fairness I did have the same feedback to your first patch
> which you never addressed. This patch would have to be part of a well

never addressed? After the first patch you asked for a different
approach which you got. Followed by the READ_ONCE() you asked for. So I
am not aware of anything I skipped.

Anyway, I keep this in my RT tree. Thank you for your time.

> Logan

Sebastian

Re: [PATCH v3] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

On 03/11/17 10:19 AM, Sebastian Andrzej Siewior wrote:
> never addressed? After the first patch you asked for a different
> approach which you got. Followed by the READ_ONCE() you asked for. So I
> am not aware of anything I skipped.

Quoting myself along the way:

v1: "I'm sorry but this seems a bit crazy to me. Driver's can't poll on 
completions because an out of tree implementation changes them in a 
weird way??! Just because no one in-tree does it now doesn't make it 
invalid."

v2: "I still don't want this merged as it makes the code less clear, but 
I tested it and it does not work."

v3: "However, this whole concept still gets a NAK from me. I think it 
makes the code less clear for no obvious reason."

I feel like I was pretty clear from the beginning. These are the 
comments that you never addressed. Just because you picked up on the 
minor issues and fixed them doesn't mean you can ignore the other feedback.

Logan

Re: [PATCH v3] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

On 2017-11-03 10:33:04 [-0600], Logan Gunthorpe wrote:
> Quoting myself along the way:
> 
> v1: "I'm sorry but this seems a bit crazy to me. Driver's can't poll on
> completions because an out of tree implementation changes them in a weird
> way??! Just because no one in-tree does it now doesn't make it invalid."

You forgot:
On 2017-10-04 10:13:15 [-0600], Logan Gunthorpe wrote:
> But at the very least, if we _absolutely_ have to patch this out, you
> shouldn't make it so convoluted. Just replace the struct completion with a

:)

> v2: "I still don't want this merged as it makes the code less clear, but I
> tested it and it does not work."

and you provided the missing bit that was missing.

> v3: "However, this whole concept still gets a NAK from me. I think it makes
> the code less clear for no obvious reason."
> 
> I feel like I was pretty clear from the beginning. These are the comments
> that you never addressed. Just because you picked up on the minor issues and
> fixed them doesn't mean you can ignore the other feedback.

and this piece is needed to get it working on RT. In longer term we
would want completions based on simple waitqueues upstream, too.

> Logan

Sebastian

Re: [PATCH v3] pci/switchtec: Don't use completion's wait queue

[Logan Gunthorpe]

On 03/11/17 10:39 AM, Sebastian Andrzej Siewior wrote:
> and this piece is needed to get it working on RT. In longer term we
> would want completions based on simple waitqueues upstream, too.

Well in the longer term it seems like you'd want to make simple 
waitqueues pollable. Then you don't need this patch.

Logan

[PATCH v3] pci/switchtec: Don't use completion's wait queue

[Sebastian Andrzej Siewior]

The poll callback is using completion's wait_queue_head_t member and
puts it in poll_wait() so the poll() caller gets a wakeup after command
completed. This does not work on RT because we don't have a
wait_queue_head_t in our completion implementation. Nobody in tree does
like that in tree so this is the only driver that breaks.

Instead of using the completion here is waitqueue with a status flag as
suggested by Logan.

I don't have the HW so I have no idea if it works as expected, so please
test it.

Cc: Kurt Schwemmer <kurt.schwemmer@microsemi.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3: add missing
	  stuser->cmd_done = true;
       spotted by Logan.

 drivers/pci/switch/switchtec.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -306,10 +306,11 @@ struct switchtec_user {
 
 	enum mrpc_state state;
 
-	struct completion comp;
+	wait_queue_head_t cmd_comp;
 	struct kref kref;
 	struct list_head list;
 
+	bool cmd_done;
 	u32 cmd;
 	u32 status;
 	u32 return_code;
@@ -331,7 +332,7 @@ static struct switchtec_user *stuser_cre
 	stuser->stdev = stdev;
 	kref_init(&stuser->kref);
 	INIT_LIST_HEAD(&stuser->list);
-	init_completion(&stuser->comp);
+	init_waitqueue_head(&stuser->cmd_comp);
 	stuser->event_cnt = atomic_read(&stdev->event_cnt);
 
 	dev_dbg(&stdev->dev, "%s: %p\n", __func__, stuser);
@@ -414,7 +415,7 @@ static int mrpc_queue_cmd(struct switcht
 	kref_get(&stuser->kref);
 	stuser->read_len = sizeof(stuser->data);
 	stuser_set_state(stuser, MRPC_QUEUED);
-	init_completion(&stuser->comp);
+	stuser->cmd_done = false;
 	list_add_tail(&stuser->list, &stdev->mrpc_queue);
 
 	mrpc_cmd_submit(stdev);
@@ -451,7 +452,8 @@ static void mrpc_complete_cmd(struct swi
 		      stuser->read_len);
 
 out:
-	complete_all(&stuser->comp);
+	stuser->cmd_done = true;
+	wake_up_interruptible(&stuser->cmd_comp);
 	list_del_init(&stuser->list);
 	stuser_put(stuser);
 	stdev->mrpc_busy = 0;
@@ -721,10 +723,11 @@ static ssize_t switchtec_dev_read(struct
 	mutex_unlock(&stdev->mrpc_mutex);
 
 	if (filp->f_flags & O_NONBLOCK) {
-		if (!try_wait_for_completion(&stuser->comp))
+		if (!stuser->cmd_done)
 			return -EAGAIN;
 	} else {
-		rc = wait_for_completion_interruptible(&stuser->comp);
+		rc = wait_event_interruptible(stuser->cmd_comp,
+					      stuser->cmd_done);
 		if (rc < 0)
 			return rc;
 	}
@@ -772,7 +775,7 @@ static unsigned int switchtec_dev_poll(s
 	struct switchtec_dev *stdev = stuser->stdev;
 	int ret = 0;
 
-	poll_wait(filp, &stuser->comp.wait, wait);
+	poll_wait(filp, &stuser->cmd_comp, wait);
 	poll_wait(filp, &stdev->event_wq, wait);
 
 	if (lock_mutex_and_test_alive(stdev))
@@ -780,7 +783,7 @@ static unsigned int switchtec_dev_poll(s
 
 	mutex_unlock(&stdev->mrpc_mutex);
 
-	if (try_wait_for_completion(&stuser->comp))
+	if (stuser->cmd_done)
 		ret |= POLLIN | POLLRDNORM;
 
 	if (stuser->event_cnt != atomic_read(&stdev->event_cnt))
@@ -1255,7 +1258,8 @@ static void stdev_kill(struct switchtec_
 
 	/* Wake up and kill any users waiting on an MRPC request */
 	list_for_each_entry_safe(stuser, tmpuser, &stdev->mrpc_queue, list) {
-		complete_all(&stuser->comp);
+		stuser->cmd_done = true;
+		wake_up_interruptible(&stuser->cmd_comp);
 		list_del_init(&stuser->list);
 		stuser_put(stuser);
 	}