[PATCH linux dev-4.7 v3 2/2] drivers/fsi: Cleanup and retry op on error

[Joel Stanley]

On Fri, Feb 24, 2017 at 6:28 AM, Christopher Bostic
<cbostic at linux.vnet.ibm.com> wrote:
> On detect of FSI bus error clean up the bus and then retry
> the original operation.
>
> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
>
> v3 - Move comon read/write code into a retry utility
>
>    - Add comment on why trying again
>
>    - Change nested error check from atomic bit type to atomic_t
>
>    - loop on retry instead of duplicating the same code

Thanks for including the changelog. As I mentioned on the list to
Eddie yesterday, the convention is to place the review comments below
the --- on the next line:


> ---
>  drivers/fsi/fsi-core.c        | 30 ++++++++++++++++++++++++++++++
>  drivers/fsi/fsi-master-gpio.c | 28 ++++++++++++++++++++++++++--
>  drivers/fsi/fsi-master.h      |  1 +
>  3 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 02ca3c0..a7b705d 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -42,6 +42,9 @@
>
>  #define FSI_PEEK_BASE                  0x410
>  #define        FSI_SLAVE_BASE                  0x800
> +#define        FSI_HUB_CONTROL                 0x3400
> +
> +#define        FSI_SLAVE_SMODE_DFLT            0xa0ff0100
>
>  #define FSI_IPOLL_PERIOD               msecs_to_jiffies(fsi_ipoll_period_ms)
>
> @@ -55,6 +58,7 @@ static const int engine_page_size = 0x400;
>  static struct task_struct *master_ipoll;
>  static unsigned int fsi_ipoll_period_ms = 100;
>  struct class *hub_master_class;
> +static atomic_t in_err_cleanup = ATOMIC_INIT(-1);

As I said in my initial review:

"Instead use a atomic type for in_err_cleanup, declared in the scope
of fsi_master_gpio_handle_error."

The variable should be declared inside fsi_master_handle_error.

>
>  static DEFINE_IDA(master_ida);
>
> @@ -653,6 +657,32 @@ static int fsi_master_break(struct fsi_master *master, int link)
>         return 0;
>  }
>
> +void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
> +{
> +       uint32_t smode = FSI_SLAVE_SMODE_DFLT;
> +
> +       if (!atomic_inc_and_test(&in_err_cleanup))
> +               return;
> +
> +       fsi_master_break(master, 0);
> +       udelay(200);
> +       master->write(master, 0, 0, FSI_SLAVE_BASE + FSI_SMODE, &smode,
> +                       sizeof(smode));
> +       smode = FSI_MRESB_RST_GEN | FSI_MRESB_RST_ERR;
> +       master->write(master, 0, 0, FSI_HUB_CONTROL + FSI_MRESB0, &smode,
> +               sizeof(smode));
> +
> +       if (addr > FSI_HUB_LINK_OFFSET) {
> +               smode = FSI_BREAK;
> +               master->write(master, 0, 0, 0x100004, &smode, sizeof(smode));
> +               smode = FSI_SLAVE_SMODE_DFLT;
> +               master->write(master, 0, 0, 0x100800, &smode, sizeof(smode));
> +       }
> +
> +       atomic_set(&in_err_cleanup, -1);
> +}
> +EXPORT_SYMBOL(fsi_master_handle_error);
> +
>  static int fsi_master_scan(struct fsi_master *master)
>  {
>         int link, slave_id, rc;
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index b6feec1..c5f4f9c 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -370,6 +370,28 @@ static int send_command(struct fsi_master_gpio *master,
>         return rc;
>  }
>
> +static int fsi_master_gpio_send(struct fsi_master_gpio *master,
> +                               struct fsi_gpio_msg *cmd, uint8_t expected,
> +                               uint32_t addr, size_t size, void *val)
> +{
> +       static const int master_retries = 2;
> +       uint8_t retries;
> +       int rc;
> +
> +       /*
> +        * In case the FSI bus is in a bad state a retry may help
> +        * the operation to complete successfully
> +        */
> +       for (retries = 0; retries < master_retries; retries++) {
> +               rc = send_command(master, cmd, expected, size, val);
> +               if (!rc)
> +                       break;
> +               fsi_master_handle_error(&master->master, addr);
> +       }
> +
> +       return rc;
> +}
> +
>  static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>                 uint8_t slave, uint32_t addr, void *val, size_t size)
>  {
> @@ -380,7 +402,8 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>                 return -ENODEV;
>
>         build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +       return fsi_master_gpio_send(master, &cmd, FSI_GPIO_RESP_ACKD, addr,
> +                               size, val);
>  }
>
>  static int fsi_master_gpio_write(struct fsi_master *_master, int link,
> @@ -393,7 +416,8 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>                 return -ENODEV;
>
>         build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +       return fsi_master_gpio_send(master, &cmd, FSI_GPIO_RESP_ACK, addr,
> +                               size, NULL);
>  }
>
>  /*
> diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
> index 4a3176b..54723a7 100644
> --- a/drivers/fsi/fsi-master.h
> +++ b/drivers/fsi/fsi-master.h
> @@ -85,6 +85,7 @@ struct fsi_master {
>  extern int fsi_master_register(struct fsi_master *master);
>  extern void fsi_master_unregister(struct fsi_master *master);
>  extern int fsi_master_start_ipoll(struct fsi_master *master);
> +extern void fsi_master_handle_error(struct fsi_master *master, uint32_t addr);
>
>  /**
>   * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x,
> --
> 1.8.2.2
>