Hi Magnus,
On Sun, Dec 5, 2021 at 2:19 AM Magnus Damm <damm@xxxxxxxxxxxxx> wrote:
> From: Magnus Damm <damm+renesas@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx
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
