Re: [PATCH] fpga: stratix10-soc: make FPGA task un-interruptible

[Richard Gong <richard.gong@xxxxxxxxxxxxxxx>]

Hi Dinh,

On 7/8/20 1:47 PM, Dinh Nguyen wrote:
> On 7/8/20 1:30 PM, Richard Gong wrote:
> > Hi Dinh,
> >
> >
> > On 7/8/20 12:08 PM, Dinh Nguyen wrote:
> > > Hi
> > >
> > > On 7/7/20 11:14 AM, richard.gong@xxxxxxxxxxxxxxx wrote:
> > > > From: Richard Gong <richard.gong@xxxxxxxxx>
> > > >
> > > > When CTRL+C occurs during the process of FPGA reconfiguration, the FPGA
> > > > reconfiguration process stops and the user can't perform a new FPGA
> > > > reconfiguration properly.
> > > >
> > > > Set FPGA complete task to be not interruptible so that the user can
> > > > properly perform FPGA reconfiguration after CTRL+C event.
> > > >
> > > > Signed-off-by: Richard Gong <richard.gong@xxxxxxxxx>
> > > > ---
> > > >    drivers/fpga/stratix10-soc.c | 23 +++--------------------
> > > >    1 file changed, 3 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> > > > index 44b7c56..657a70c 100644
> > > > --- a/drivers/fpga/stratix10-soc.c
> > > > +++ b/drivers/fpga/stratix10-soc.c
> > > > @@ -196,17 +196,13 @@ static int s10_ops_write_init(struct
> > > > fpga_manager *mgr,
> > > >        if (ret < 0)
> > > >            goto init_done;
> > > >    -    ret = wait_for_completion_interruptible_timeout(
> > > > +    ret = wait_for_completion_timeout(
> > > >            &priv->status_return_completion, S10_RECONFIG_TIMEOUT);
> > > >        if (!ret) {
> > > >            dev_err(dev, "timeout waiting for RECONFIG_REQUEST\n");
> > > >            ret = -ETIMEDOUT;
> > > >            goto init_done;
> > > >        }
> > > > -    if (ret < 0) {
> > > > -        dev_err(dev, "error (%d) waiting for RECONFIG_REQUEST\n", ret);
> > > > -        goto init_done;
> > > > -    }
> > > >          ret = 0;
> > > >        if (!test_and_clear_bit(SVC_STATUS_OK, &priv->status)) {
> > > > @@ -318,7 +314,7 @@ static int s10_ops_write(struct fpga_manager
> > > > *mgr, const char *buf,
> > > >             */
> > > >            wait_status = 1; /* not timed out */
> > > >            if (!priv->status)
> > > > -            wait_status = wait_for_completion_interruptible_timeout(
> > > > +            wait_status = wait_for_completion_timeout(
> > > >                    &priv->status_return_completion,
> > > >                    S10_BUFFER_TIMEOUT);
> > > >    @@ -340,13 +336,6 @@ static int s10_ops_write(struct fpga_manager
> > > > *mgr, const char *buf,
> > > >                ret = -ETIMEDOUT;
> > > >                break;
> > > >            }
> > > > -        if (wait_status < 0) {
> > > > -            ret = wait_status;
> > > > -            dev_err(dev,
> > > > -                "error (%d) waiting for svc layer buffers\n",
> > > > -                ret);
> > > > -            break;
> > > > -        }
> > > >        }
> > > >          if (!s10_free_buffers(mgr))
> > > > @@ -372,7 +361,7 @@ static int s10_ops_write_complete(struct
> > > > fpga_manager *mgr,
> > > >            if (ret < 0)
> > > >                break;
> > > >    -        ret = wait_for_completion_interruptible_timeout(
> > > > +        ret = wait_for_completion_timeout(
> > > >                &priv->status_return_completion, timeout);
> > > >            if (!ret) {
> > > >                dev_err(dev,
> > > > @@ -380,12 +369,6 @@ static int s10_ops_write_complete(struct
> > > > fpga_manager *mgr,
> > > >                ret = -ETIMEDOUT;
> > > >                break;
> > > >            }
> > > > -        if (ret < 0) {
> > > > -            dev_err(dev,
> > > > -                "error (%d) waiting for RECONFIG_COMPLETED\n",
> > > > -                ret);
> > > > -            break;
> > > > -
> > > >            /* Not error or timeout, so ret is # of jiffies until
> > > > timeout */
> > > >            timeout = ret;
> > > >            ret = 0;
> > >
> > > Do you need the same change in drivers/fpga/socfpga.c?
> > It is not required.
>
> Why not?

The original codes doesn't handle the task completion to be signaled, so 
here it serves the same function as wait_for_completion_timeout call.

Regards,
Richard

> > Also, you did not
> > > include Moritz Fisher on this. He's the maintainer.
> > I did include Moritz Fisher <mdf@xxxxxxxxxx> in the submission, is
> > something change recently?
>
> My bad, I didn't see his name in the email.
>
> Dinh