Re: [PATCH/RFC] serdev: BREAK/FRAME/PARITY/OVERRUN notification prototype

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Magnus,

On Sun, Dec 5, 2021 at 2:19 AM Magnus Damm <damm@opensource.se> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> A prototype patch to let serdev device drivers get notified by hardware
> errors such as BREAK, FRAME, PARITY and OVERRUN.
>
> With this patch, in the event of an error detected in the UART device driver
> the serdev_device_driver will get the newly introduced ->error() callback
> invoked if serdev_device_set_error_mask() has previously been used to enable
> the type of error. The errors are taken straight from the TTY layer and fed
> into the serdev_device_driver after filtering out only enabled errors.
>
> Without this patch the hardware errors never reach the serdev_device_driver.
>
> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

Thanks for your patch!

> --- 0001/drivers/tty/serdev/core.c
> +++ work/drivers/tty/serdev/core.c      2021-12-04 15:04:48.108809440 +0900
> @@ -349,6 +349,17 @@ unsigned int serdev_device_set_baudrate(
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_set_baudrate);
>
> +void serdev_device_set_error_mask(struct serdev_device *serdev, unsigned long mask)
> +{
> +       struct serdev_controller *ctrl = serdev->ctrl;
> +
> +       if (!ctrl || !ctrl->ops->set_error_mask)

Can this happen?
There's only a single serdev_controller_ops structure, and it provides
.set_error_mask().

> +               return;
> +
> +       ctrl->ops->set_error_mask(ctrl, mask);
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_set_error_mask);
> +
>  void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
>  {
>         struct serdev_controller *ctrl = serdev->ctrl;

> @@ -27,11 +32,38 @@ static int ttyport_receive_buf(struct tt
>  {
>         struct serdev_controller *ctrl = port->client_data;
>         struct serport *serport = serdev_controller_get_drvdata(ctrl);
> -       int ret;
> +       unsigned long errors = 0;
> +       int i, ret;

unsigned int i;

>
>         if (!test_bit(SERPORT_ACTIVE, &serport->flags))
>                 return 0;
>
> +       for (i = 0; fp && i < count; i++) {
> +               switch (fp[i]) {
> +               case TTY_BREAK:
> +                       if (test_bit(SERPORT_NOTIFY_BREAK, &serport->flags))
> +                               set_bit(SERDEV_ERROR_BREAK, &errors);

No need to use atomic ops for setting bits in errors.

> +                       break;
> +
> +               case TTY_FRAME:
> +                       if (test_bit(SERPORT_NOTIFY_FRAME, &serport->flags))
> +                               set_bit(SERDEV_ERROR_FRAME, &errors);
> +                       break;
> +
> +               case TTY_PARITY:
> +                       if (test_bit(SERPORT_NOTIFY_PARITY, &serport->flags))
> +                               set_bit(SERDEV_ERROR_PARITY, &errors);
> +                       break;
> +
> +               case TTY_OVERRUN:
> +                       if (test_bit(SERPORT_NOTIFY_OVERRUN, &serport->flags))
> +                               set_bit(SERDEV_ERROR_OVERRUN, &errors);
> +                       break;
> +               }
> +       }
> +       if (errors)
> +               serdev_controller_error(ctrl, errors);
> +
>         ret = serdev_controller_receive_buf(ctrl, cp, count);
>
>         dev_WARN_ONCE(&ctrl->dev, ret < 0 || ret > count,
> @@ -180,6 +212,26 @@ static unsigned int ttyport_set_baudrate
>         return ktermios.c_ospeed;
>  }
>
> +static void ttyport_set_flags(struct serport *serport, unsigned long nflag,
> +                             unsigned long mask, unsigned long eflag)
> +{
> +       if (test_bit(eflag, &mask))

No need to use atomic ops for testing bits in mask.

> +               set_bit(nflag, &serport->flags);
> +       else
> +               clear_bit(nflag, &serport->flags);
> +}
> +
> +static void ttyport_set_error_mask(struct serdev_controller *ctrl,
> +                                  unsigned long m)
> +{
> +       struct serport *sp = serdev_controller_get_drvdata(ctrl);
> +
> +       ttyport_set_flags(sp, SERPORT_NOTIFY_BREAK, m, SERDEV_ERROR_BREAK);
> +       ttyport_set_flags(sp, SERPORT_NOTIFY_FRAME, m, SERDEV_ERROR_FRAME);
> +       ttyport_set_flags(sp, SERPORT_NOTIFY_PARITY, m, SERDEV_ERROR_PARITY);
> +       ttyport_set_flags(sp, SERPORT_NOTIFY_OVERRUN, m, SERDEV_ERROR_OVERRUN);
> +}
> +
>  static void ttyport_set_flow_control(struct serdev_controller *ctrl, bool enable)
>  {
>         struct serport *serport = serdev_controller_get_drvdata(ctrl);
> @@ -253,6 +305,7 @@ static const struct serdev_controller_op
>         .write_room = ttyport_write_room,
>         .open = ttyport_open,
>         .close = ttyport_close,
> +       .set_error_mask = ttyport_set_error_mask,
>         .set_flow_control = ttyport_set_flow_control,
>         .set_parity = ttyport_set_parity,
>         .set_baudrate = ttyport_set_baudrate,
> --- 0001/include/linux/serdev.h
> +++ work/include/linux/serdev.h 2021-12-04 15:06:26.852815658 +0900
> @@ -19,12 +19,15 @@ struct serdev_device;
>
>  /**
>   * struct serdev_device_ops - Callback operations for a serdev device
> + * @error:             Function called with errors received from device;
> + *                     may sleep.
>   * @receive_buf:       Function called with data received from device;
>   *                     returns number of bytes accepted; may sleep.
>   * @write_wakeup:      Function called when ready to transmit more data; must
>   *                     not sleep.
>   */
>  struct serdev_device_ops {
> +       void (*error)(struct serdev_device *, unsigned long);
>         int (*receive_buf)(struct serdev_device *, const unsigned char *, size_t);
>         void (*write_wakeup)(struct serdev_device *);
>  };
> @@ -76,6 +79,11 @@ enum serdev_parity {
>         SERDEV_PARITY_ODD,
>  };
>
> +#define SERDEV_ERROR_BREAK 0
> +#define SERDEV_ERROR_FRAME 1
> +#define SERDEV_ERROR_PARITY 2
> +#define SERDEV_ERROR_OVERRUN 3
> +
>  /*
>   * serdev controller structures
>   */
> @@ -85,6 +93,7 @@ struct serdev_controller_ops {
>         int (*write_room)(struct serdev_controller *);
>         int (*open)(struct serdev_controller *);
>         void (*close)(struct serdev_controller *);
> +       void (*set_error_mask)(struct serdev_controller *, unsigned long);
>         void (*set_flow_control)(struct serdev_controller *, bool);
>         int (*set_parity)(struct serdev_controller *, enum serdev_parity);
>         unsigned int (*set_baudrate)(struct serdev_controller *, unsigned int);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds