| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Yoav5KjnbIlpkR6c@oden.dyn.berto.se>
Date: Thu, 19 May 2022 23:00:20 +0200
From: Niklas Söderlund
<niklas.soderlund@...natech.se>
To: Michael Rodin <mrodin@...adit-jv.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, michael@...in.online,
erosca@...adit-jv.com
Subject: Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and
stop streaming if required
Hi Michael,
Thanks for your work.
I like this patch, I think it captures the issue discussed in the
previous thread quiet nicely. One small nit below.
On 2022-05-19 20:00:09 +0200, Michael Rodin wrote:
> When a subdevice sends a transfer error event during streaming and we can
> not capture new frames, then we know for sure that this is an unrecoverable
> failure and not just a temporary glitch. In this case we can not ignore the
> transfer error any more and have to notify userspace. In response to the
> transfer error event userspace can try to restart streaming and hope that
> it works again.
>
> This patch is based on the patch [1] from Niklas Söderlund, however it adds
> more logic to check whether the VIN hardware module is actually affected by
> the transfer errors reported by the usptream device. For this it takes some
> ideas from the imx driver where EOF interrupts are monitored by the
> eof_timeout_timer added by commit 4a34ec8e470c ("[media] media: imx: Add
> CSI subdev driver").
>
> [1] https://lore.kernel.org/linux-renesas-soc/20211108160220.767586-4-niklas.soderlund+renesas@ragnatech.se/
>
> Signed-off-by: Michael Rodin <mrodin@...adit-jv.com>
> ---
> drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 34 ++++++++++++++++++++++
> .../media/platform/renesas/rcar-vin/rcar-v4l2.c | 18 +++++++++++-
> drivers/media/platform/renesas/rcar-vin/rcar-vin.h | 7 +++++
> 3 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index 2272f1c..596a367 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -13,6 +13,7 @@
> #include <linux/delay.h>
> #include <linux/interrupt.h>
> #include <linux/pm_runtime.h>
> +#include <media/v4l2-event.h>
>
> #include <media/videobuf2-dma-contig.h>
>
> @@ -1060,6 +1061,9 @@ static irqreturn_t rvin_irq(int irq, void *data)
> vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> }
>
> + cancel_delayed_work(&vin->frame_timeout);
> + schedule_delayed_work(&vin->frame_timeout, msecs_to_jiffies(FRAME_TIMEOUT_MS));
> +
> vin->sequence++;
>
> /* Prepare for next frame */
> @@ -1283,6 +1287,7 @@ int rvin_start_streaming(struct rvin_dev *vin)
> spin_lock_irqsave(&vin->qlock, flags);
>
> vin->sequence = 0;
> + vin->xfer_error = false;
>
> ret = rvin_capture_start(vin);
> if (ret)
> @@ -1290,6 +1295,10 @@ int rvin_start_streaming(struct rvin_dev *vin)
>
> spin_unlock_irqrestore(&vin->qlock, flags);
>
> + /* We start the frame watchdog only after we have successfully started streaming */
> + if (!ret)
> + schedule_delayed_work(&vin->frame_timeout, msecs_to_jiffies(FRAME_TIMEOUT_MS));
> +
> return ret;
> }
>
> @@ -1332,6 +1341,12 @@ void rvin_stop_streaming(struct rvin_dev *vin)
> }
>
> vin->state = STOPPING;
> + /*
> + * Since we are now stopping and don't expect more frames to be captured, make sure that
> + * there is no pending work for error handling.
> + */
> + cancel_delayed_work_sync(&vin->frame_timeout);
> + vin->xfer_error = false;
Do we need to set xfer_error to false here? The delayed work is canceled
and we reset the xfer_error when we start in rvin_start_streaming().
>
> /* Wait until only scratch buffer is used, max 3 interrupts. */
> retries = 0;
> @@ -1424,6 +1439,23 @@ void rvin_dma_unregister(struct rvin_dev *vin)
> v4l2_device_unregister(&vin->v4l2_dev);
> }
>
> +static void rvin_frame_timeout(struct work_struct *work)
> +{
> + struct delayed_work *dwork = to_delayed_work(work);
> + struct rvin_dev *vin = container_of(dwork, struct rvin_dev, frame_timeout);
> + struct v4l2_event event = {
> + .type = V4L2_EVENT_XFER_ERROR,
> + };
> +
> + vin_dbg(vin, "Frame timeout!\n");
> +
> + if (!vin->xfer_error)
> + return;
> + vin_err(vin, "Unrecoverable transfer error detected, stopping streaming\n");
> + vb2_queue_error(&vin->queue);
> + v4l2_event_queue(&vin->vdev, &event);
> +}
> +
> int rvin_dma_register(struct rvin_dev *vin, int irq)
> {
> struct vb2_queue *q = &vin->queue;
> @@ -1470,6 +1502,8 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> goto error;
> }
>
> + INIT_DELAYED_WORK(&vin->frame_timeout, rvin_frame_timeout);
> +
> return 0;
> error:
> rvin_dma_unregister(vin);
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> index 2e2aa9d..bd7f6fe2 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> @@ -648,6 +648,8 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
> switch (sub->type) {
> case V4L2_EVENT_SOURCE_CHANGE:
> return v4l2_event_subscribe(fh, sub, 4, NULL);
> + case V4L2_EVENT_XFER_ERROR:
> + return v4l2_event_subscribe(fh, sub, 1, NULL);
> }
> return v4l2_ctrl_subscribe_event(fh, sub);
> }
> @@ -1000,9 +1002,23 @@ void rvin_v4l2_unregister(struct rvin_dev *vin)
> static void rvin_notify_video_device(struct rvin_dev *vin,
> unsigned int notification, void *arg)
> {
> + const struct v4l2_event *event;
> +
> switch (notification) {
> case V4L2_DEVICE_NOTIFY_EVENT:
> - v4l2_event_queue(&vin->vdev, arg);
> + event = arg;
> +
> + switch (event->type) {
> + case V4L2_EVENT_XFER_ERROR:
> + if (vin->state != STOPPED && vin->state != STOPPING) {
> + vin_dbg(vin, "Subdevice signaled transfer error.\n");
> + vin->xfer_error = true;
> + }
> + break;
> + default:
> + break;
> + }
> +
> break;
> default:
> break;
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> index 1f94589..4726a69 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> @@ -31,6 +31,9 @@
> /* Max number on VIN instances that can be in a system */
> #define RCAR_VIN_NUM 32
>
> +/* maximum time we wait before signalling an error to userspace */
> +#define FRAME_TIMEOUT_MS 1000
> +
> struct rvin_group;
>
> enum model_id {
> @@ -207,6 +210,8 @@ struct rvin_info {
> * @std: active video standard of the video source
> *
> * @alpha: Alpha component to fill in for supported pixel formats
> + * @xfer_error: Indicates if any transfer errors occurred in the current streaming session.
> + * @frame_timeout: Watchdog for monitoring regular capturing of frames in rvin_irq.
> */
> struct rvin_dev {
> struct device *dev;
> @@ -251,6 +256,8 @@ struct rvin_dev {
> v4l2_std_id std;
>
> unsigned int alpha;
> + bool xfer_error;
> + struct delayed_work frame_timeout;
> };
>
> #define vin_to_source(vin) ((vin)->parallel.subdev)
> --
> 2.7.4
>
--
Kind Regards,
Niklas Söderlund
Powered by blists - more mailing lists