* [PATCH/RFC] serdev: BREAK/FRAME/PARITY/OVERRUN notification prototype
@ 2021-12-05 0:28 Magnus Damm
2021-12-06 7:48 ` Geert Uytterhoeven
0 siblings, 1 reply; 3+ messages in thread
From: Magnus Damm @ 2021-12-05 0:28 UTC (permalink / raw)
To: linux-serial
Cc: robh, Magnus Damm, geert+renesas, gregkh, linux-renesas-soc,
wsa+renesas, jirislaby
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>
---
Applies to linux-5.16-rc3
Not for upstream merge. Only lightly tested. The function names could
probably be improved. Instead of reusing serport->flags it might be
slightly cleaner to store a SERDEV_ERROR_* mask as a separate variable.
drivers/tty/serdev/core.c | 11 +++++++
drivers/tty/serdev/serdev-ttyport.c | 55 ++++++++++++++++++++++++++++++++++-
include/linux/serdev.h | 22 ++++++++++++++
3 files changed, 87 insertions(+), 1 deletion(-)
--- 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)
+ 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;
--- 0001/drivers/tty/serdev/serdev-ttyport.c
+++ work/drivers/tty/serdev/serdev-ttyport.c 2021-12-04 15:08:31.300823495 +0900
@@ -2,6 +2,7 @@
/*
* Copyright (C) 2016-2017 Linaro Ltd., Rob Herring <robh@kernel.org>
*/
+#include <linux/bits.h>
#include <linux/kernel.h>
#include <linux/serdev.h>
#include <linux/tty.h>
@@ -9,6 +10,10 @@
#include <linux/poll.h>
#define SERPORT_ACTIVE 1
+#define SERPORT_NOTIFY_BREAK 2
+#define SERPORT_NOTIFY_FRAME 3
+#define SERPORT_NOTIFY_PARITY 4
+#define SERPORT_NOTIFY_OVERRUN 5
struct serport {
struct tty_port *port;
@@ -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;
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);
+ 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))
+ 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);
@@ -190,12 +199,24 @@ static inline int serdev_controller_rece
return serdev->ops->receive_buf(serdev, data, count);
}
+static inline void serdev_controller_error(struct serdev_controller *ctrl,
+ unsigned long errors)
+{
+ struct serdev_device *serdev = ctrl->serdev;
+
+ if (!serdev || !serdev->ops->error)
+ return;
+
+ serdev->ops->error(serdev, errors);
+}
+
#if IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
int serdev_device_open(struct serdev_device *);
void serdev_device_close(struct serdev_device *);
int devm_serdev_device_open(struct device *, struct serdev_device *);
unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
+void serdev_device_set_error_mask(struct serdev_device *, unsigned long);
void serdev_device_set_flow_control(struct serdev_device *, bool);
int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
void serdev_device_wait_until_sent(struct serdev_device *, long);
@@ -238,6 +259,7 @@ static inline unsigned int serdev_device
{
return 0;
}
+static inline void serdev_device_set_error_mask(struct serdev_device *sdev, unsigned long mask) {}
static inline void serdev_device_set_flow_control(struct serdev_device *sdev, bool enable) {}
static inline int serdev_device_write_buf(struct serdev_device *serdev,
const unsigned char *buf,
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH/RFC] serdev: BREAK/FRAME/PARITY/OVERRUN notification prototype
2021-12-05 0:28 [PATCH/RFC] serdev: BREAK/FRAME/PARITY/OVERRUN notification prototype Magnus Damm
@ 2021-12-06 7:48 ` Geert Uytterhoeven
2021-12-07 3:08 ` Magnus Damm
0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2021-12-06 7:48 UTC (permalink / raw)
To: Magnus Damm
Cc: open list:SERIAL DRIVERS, Rob Herring, Greg KH, Linux-Renesas,
Wolfram Sang, Jiri Slaby
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH/RFC] serdev: BREAK/FRAME/PARITY/OVERRUN notification prototype
2021-12-06 7:48 ` Geert Uytterhoeven
@ 2021-12-07 3:08 ` Magnus Damm
0 siblings, 0 replies; 3+ messages in thread
From: Magnus Damm @ 2021-12-07 3:08 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: open list:SERIAL DRIVERS, Rob Herring, Greg KH, Linux-Renesas,
Wolfram Sang, Jiri Slaby
Hi Geert,
On Mon, Dec 6, 2021 at 4:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> 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!
Thanks for feedback!
> > --- 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().
Good question.
The code above is following the same approach with callbacks as the
rest of the serdev functions in core.c. I think there are more than 10
functions that validate pointers such as:
ctrl
ctrl->ops->pointer
Interestingly enough ctrl->ops does not seem to be checked at all.
What would be a better approach here I wonder?
> > + 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;
Will fix.
> >
> > 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.
Switching to the non-atomic variants makes sense, will do.
> > + 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.
Agreed.
I'll cook up a V2 later this week.
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-07 3:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-05 0:28 [PATCH/RFC] serdev: BREAK/FRAME/PARITY/OVERRUN notification prototype Magnus Damm
2021-12-06 7:48 ` Geert Uytterhoeven
2021-12-07 3:08 ` Magnus Damm
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.