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

Hi Geert,

On Mon, Dec 6, 2021 at 4:48 PM Geert Uytterhoeven [off-list ref] wrote:

Hi Magnus,

On Sun, Dec 5, 2021 at 2:19 AM Magnus Damm [off-list ref] wrote:

quoted

From: Magnus Damm <redacted>

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 [off-list ref]

Thanks for your patch!

Thanks for feedback!

quoted

--- 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?

quoted

+               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;

quoted

@@ -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.

quoted

        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.

quoted

+                       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