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

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(-)

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);
>   	}
>

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

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/

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

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

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

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