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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux