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

[Richard Gong]

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