Hi Geert,
On Mon, Dec 6, 2021 at 4:48 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> 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!
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
