Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582243 > unrolled thread

[PATCH v4 20/36] media: imx: Add CSI subdev driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 20/36] media: imx: Add CSI subdev driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-02-16 03:30 +0100
    Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-16 13:00 +0100
      Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-16 13:50 +0100
        Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-16 14:20 +0100
          Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-16 15:30 +0100
            Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-02-16 20:10 +0100
        Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-02-16 19:50 +0100
          Re: [PATCH v4 20/36] media: imx: Add CSI subdev driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-16 20:20 +0100

#1582243 — [PATCH v4 20/36] media: imx: Add CSI subdev driver

This is a media entity subdevice for the i.MX Camera
Sensor Interface module.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/Kconfig         |   13 +
 drivers/staging/media/imx/Makefile        |    2 +
 drivers/staging/media/imx/imx-media-csi.c | 1220 +++++++++++++++++++++++++++++
 3 files changed, 1235 insertions(+)
 create mode 100644 drivers/staging/media/imx/imx-media-csi.c

diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
index 722ed55..e27ad6d 100644
--- a/drivers/staging/media/imx/Kconfig
+++ b/drivers/staging/media/imx/Kconfig
@@ -5,3 +5,16 @@ config VIDEO_IMX_MEDIA
 	  Say yes here to enable support for video4linux media controller
 	  driver for the i.MX5/6 SOC.
 
+if VIDEO_IMX_MEDIA
+menu "i.MX5/6 Media Sub devices"
+
+config VIDEO_IMX_CSI
+	tristate "i.MX5/6 Camera Sensor Interface driver"
+	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
+	select VIDEOBUF2_DMA_CONTIG
+	default y
+	---help---
+	  A video4linux camera sensor interface driver for i.MX5/6.
+
+endmenu
+endif
diff --git a/drivers/staging/media/imx/Makefile b/drivers/staging/media/imx/Makefile
index 4606a3a..c054490 100644
--- a/drivers/staging/media/imx/Makefile
+++ b/drivers/staging/media/imx/Makefile
@@ -4,3 +4,5 @@ imx-media-common-objs := imx-media-utils.o imx-media-fim.o
 obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media.o
 obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-common.o
 obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-capture.o
+
+obj-$(CONFIG_VIDEO_IMX_CSI) += imx-media-csi.o
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
new file mode 100644
index 0000000..0343fc3
--- /dev/null
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -0,0 +1,1220 @@
+/*
+ * V4L2 Capture CSI Subdev for Freescale i.MX5/6 SOC
+ *
+ * Copyright (c) 2014-2016 Mentor Graphics Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-mc.h>
+#include <media/v4l2-of.h>
+#include <media/v4l2-subdev.h>
+#include <media/videobuf2-dma-contig.h>
+#include <video/imx-ipu-v3.h>
+#include <media/imx.h>
+#include "imx-media.h"
+
+/*
+ * Min/Max supported width and heights.
+ *
+ * We allow planar output, so we have to align width by 16 pixels
+ * to meet IDMAC alignment requirements.
+ *
+ * TODO: move this into pad format negotiation, if capture device
+ * has not requested planar formats, we should allow 8 pixel
+ * alignment.
+ */
+#define MIN_W       176
+#define MIN_H       144
+#define MAX_W      4096
+#define MAX_H      4096
+#define W_ALIGN    4 /* multiple of 16 pixels */
+#define H_ALIGN    1 /* multiple of 2 lines */
+#define S_ALIGN    1 /* multiple of 2 */
+
+struct csi_priv {
+	struct device *dev;
+	struct ipu_soc *ipu;
+	struct imx_media_dev *md;
+	struct v4l2_subdev sd;
+	struct media_pad pad[CSI_NUM_PADS];
+	int active_output_pad;
+	int csi_id;
+	int smfc_id;
+
+	struct ipuv3_channel *idmac_ch;
+	struct ipu_smfc *smfc;
+	struct ipu_csi *csi;
+
+	struct v4l2_mbus_framefmt format_mbus[CSI_NUM_PADS];
+	const struct imx_media_pixfmt *cc[CSI_NUM_PADS];
+	struct v4l2_rect crop;
+
+	/* the video device at IDMAC output pad */
+	struct imx_media_video_dev *vdev;
+
+	/* active vb2 buffers to send to video dev sink */
+	struct imx_media_buffer *active_vb2_buf[2];
+	struct imx_media_dma_buf underrun_buf;
+
+	int ipu_buf_num;  /* ipu double buffer index: 0-1 */
+
+	/* the sink for the captured frames */
+	struct media_entity *sink;
+	enum ipu_csi_dest dest;
+	/* the source subdev */
+	struct v4l2_subdev *src_sd;
+
+	/* the mipi virtual channel number at link validate */
+	int vc_num;
+
+	/* the attached sensor at stream on */
+	struct imx_media_subdev *sensor;
+
+	spinlock_t irqlock; /* protect eof_irq handler */
+	struct timer_list eof_timeout_timer;
+	int eof_irq;
+	int nfb4eof_irq;
+
+	struct v4l2_ctrl_handler ctrl_hdlr;
+	struct imx_media_fim *fim;
+
+	bool power_on;  /* power is on */
+	bool stream_on; /* streaming is on */
+	bool last_eof;  /* waiting for last EOF at stream off */
+	struct completion last_eof_comp;
+};
+
+static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev)
+{
+	return container_of(sdev, struct csi_priv, sd);
+}
+
+static void csi_idmac_put_ipu_resources(struct csi_priv *priv)
+{
+	if (!IS_ERR_OR_NULL(priv->idmac_ch))
+		ipu_idmac_put(priv->idmac_ch);
+	priv->idmac_ch = NULL;
+
+	if (!IS_ERR_OR_NULL(priv->smfc))
+		ipu_smfc_put(priv->smfc);
+	priv->smfc = NULL;
+}
+
+static int csi_idmac_get_ipu_resources(struct csi_priv *priv)
+{
+	int ch_num, ret;
+
+	ch_num = IPUV3_CHANNEL_CSI0 + priv->smfc_id;
+
+	priv->smfc = ipu_smfc_get(priv->ipu, ch_num);
+	if (IS_ERR(priv->smfc)) {
+		v4l2_err(&priv->sd, "failed to get SMFC\n");
+		ret = PTR_ERR(priv->smfc);
+		goto out;
+	}
+
+	priv->idmac_ch = ipu_idmac_get(priv->ipu, ch_num);
+	if (IS_ERR(priv->idmac_ch)) {
+		v4l2_err(&priv->sd, "could not get IDMAC channel %u\n",
+			 ch_num);
+		ret = PTR_ERR(priv->idmac_ch);
+		goto out;
+	}
+
+	return 0;
+out:
+	csi_idmac_put_ipu_resources(priv);
+	return ret;
+}
+
+static void csi_vb2_buf_done(struct csi_priv *priv)
+{
+	struct imx_media_video_dev *vdev = priv->vdev;
+	struct imx_media_buffer *done, *next;
+	struct vb2_buffer *vb;
+	dma_addr_t phys;
+
+	done = priv->active_vb2_buf[priv->ipu_buf_num];
+	if (done) {
+		vb = &done->vbuf.vb2_buf;
+		vb->timestamp = ktime_get_ns();
+		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
+	}
+
+	/* get next queued buffer */
+	next = imx_media_capture_device_next_buf(vdev);
+	if (next) {
+		phys = vb2_dma_contig_plane_dma_addr(&next->vbuf.vb2_buf, 0);
+		priv->active_vb2_buf[priv->ipu_buf_num] = next;
+	} else {
+		phys = priv->underrun_buf.phys;
+		priv->active_vb2_buf[priv->ipu_buf_num] = NULL;
+	}
+
+	if (ipu_idmac_buffer_is_ready(priv->idmac_ch, priv->ipu_buf_num))
+		ipu_idmac_clear_buffer(priv->idmac_ch, priv->ipu_buf_num);
+
+	ipu_cpmem_set_buffer(priv->idmac_ch, priv->ipu_buf_num, phys);
+}
+
+static void csi_call_fim(struct csi_priv *priv)
+{
+	if (priv->fim) {
+		struct timespec cur_ts;
+
+		ktime_get_ts(&cur_ts);
+		/* call frame interval monitor */
+		imx_media_fim_eof_monitor(priv->fim, &cur_ts);
+	}
+}
+
+static irqreturn_t csi_idmac_eof_interrupt(int irq, void *dev_id)
+{
+	struct csi_priv *priv = dev_id;
+
+	spin_lock(&priv->irqlock);
+
+	if (priv->last_eof) {
+		complete(&priv->last_eof_comp);
+		priv->last_eof = false;
+		goto unlock;
+	}
+
+	csi_call_fim(priv);
+
+	csi_vb2_buf_done(priv);
+
+	/* select new IPU buf */
+	ipu_idmac_select_buffer(priv->idmac_ch, priv->ipu_buf_num);
+	/* toggle IPU double-buffer index */
+	priv->ipu_buf_num ^= 1;
+
+	/* bump the EOF timeout timer */
+	mod_timer(&priv->eof_timeout_timer,
+		  jiffies + msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
+
+unlock:
+	spin_unlock(&priv->irqlock);
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t csi_idmac_nfb4eof_interrupt(int irq, void *dev_id)
+{
+	struct csi_priv *priv = dev_id;
+	static const struct v4l2_event ev = {
+		.type = V4L2_EVENT_IMX_NFB4EOF,
+	};
+
+	v4l2_err(&priv->sd, "NFB4EOF\n");
+
+	v4l2_subdev_notify_event(&priv->sd, &ev);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * EOF timeout timer function.
+ */
+static void csi_idmac_eof_timeout(unsigned long data)
+{
+	struct csi_priv *priv = (struct csi_priv *)data;
+	static const struct v4l2_event ev = {
+		.type = V4L2_EVENT_FRAME_TIMEOUT,
+	};
+
+	v4l2_err(&priv->sd, "EOF timeout\n");
+
+	v4l2_subdev_notify_event(&priv->sd, &ev);
+}
+
+static void csi_idmac_setup_vb2_buf(struct csi_priv *priv, dma_addr_t *phys)
+{
+	struct imx_media_video_dev *vdev = priv->vdev;
+	struct imx_media_buffer *buf;
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		buf = imx_media_capture_device_next_buf(vdev);
+		priv->active_vb2_buf[i] = buf;
+		phys[i] = vb2_dma_contig_plane_dma_addr(&buf->vbuf.vb2_buf, 0);
+	}
+}
+
+static void csi_idmac_unsetup_vb2_buf(struct csi_priv *priv)
+{
+	struct imx_media_buffer *buf;
+	int i;
+
+	/* return any remaining active frames with error */
+	for (i = 0; i < 2; i++) {
+		buf = priv->active_vb2_buf[i];
+		if (buf) {
+			struct vb2_buffer *vb = &buf->vbuf.vb2_buf;
+
+			vb->timestamp = ktime_get_ns();
+			vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
+		}
+	}
+}
+
+/* init the SMFC IDMAC channel */
+static int csi_idmac_setup_channel(struct csi_priv *priv)
+{
+	struct imx_media_video_dev *vdev = priv->vdev;
+	struct v4l2_of_endpoint *sensor_ep;
+	struct v4l2_mbus_framefmt *infmt;
+	unsigned int burst_size;
+	struct ipu_image image;
+	dma_addr_t phys[2];
+	bool passthrough;
+	int ret;
+
+	infmt = &priv->format_mbus[CSI_SINK_PAD];
+	sensor_ep = &priv->sensor->sensor_ep;
+
+	ipu_cpmem_zero(priv->idmac_ch);
+
+	memset(&image, 0, sizeof(image));
+	image.pix = vdev->fmt.fmt.pix;
+	image.rect.width = image.pix.width;
+	image.rect.height = image.pix.height;
+
+	csi_idmac_setup_vb2_buf(priv, phys);
+
+	image.phys0 = phys[0];
+	image.phys1 = phys[1];
+
+	ret = ipu_cpmem_set_image(priv->idmac_ch, &image);
+	if (ret)
+		return ret;
+
+	burst_size = (image.pix.width & 0xf) ? 8 : 16;
+
+	ipu_cpmem_set_burstsize(priv->idmac_ch, burst_size);
+
+	/*
+	 * If the sensor uses 16-bit parallel CSI bus, we must handle
+	 * the data internally in the IPU as 16-bit generic, aka
+	 * passthrough mode.
+	 */
+	passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 &&
+		       sensor_ep->bus.parallel.bus_width >= 16);
+
+	if (passthrough)
+		ipu_cpmem_set_format_passthrough(priv->idmac_ch, 16);
+
+	/*
+	 * Set the channel for the direct CSI-->memory via SMFC
+	 * use-case to very high priority, by enabling the watermark
+	 * signal in the SMFC, enabling WM in the channel, and setting
+	 * the channel priority to high.
+	 *
+	 * Refer to the i.mx6 rev. D TRM Table 36-8: Calculated priority
+	 * value.
+	 *
+	 * The WM's are set very low by intention here to ensure that
+	 * the SMFC FIFOs do not overflow.
+	 */
+	ipu_smfc_set_watermark(priv->smfc, 0x02, 0x01);
+	ipu_cpmem_set_high_priority(priv->idmac_ch);
+	ipu_idmac_enable_watermark(priv->idmac_ch, true);
+	ipu_cpmem_set_axi_id(priv->idmac_ch, 0);
+	ipu_idmac_lock_enable(priv->idmac_ch, 8);
+
+	burst_size = ipu_cpmem_get_burstsize(priv->idmac_ch);
+	burst_size = passthrough ?
+		(burst_size >> 3) - 1 : (burst_size >> 2) - 1;
+
+	ipu_smfc_set_burstsize(priv->smfc, burst_size);
+
+	if (image.pix.field == V4L2_FIELD_NONE &&
+	    V4L2_FIELD_HAS_BOTH(infmt->field))
+		ipu_cpmem_interlaced_scan(priv->idmac_ch,
+					  image.pix.bytesperline);
+
+	ipu_idmac_set_double_buffer(priv->idmac_ch, true);
+
+	return 0;
+}
+
+static void csi_idmac_unsetup(struct csi_priv *priv)
+{
+	ipu_idmac_disable_channel(priv->idmac_ch);
+	ipu_smfc_disable(priv->smfc);
+
+	csi_idmac_unsetup_vb2_buf(priv);
+}
+
+static int csi_idmac_setup(struct csi_priv *priv)
+{
+	int ret;
+
+	ret = csi_idmac_setup_channel(priv);
+	if (ret)
+		return ret;
+
+	ipu_cpmem_dump(priv->idmac_ch);
+	ipu_dump(priv->ipu);
+
+	ipu_smfc_enable(priv->smfc);
+
+	/* set buffers ready */
+	ipu_idmac_select_buffer(priv->idmac_ch, 0);
+	ipu_idmac_select_buffer(priv->idmac_ch, 1);
+
+	/* enable the channels */
+	ipu_idmac_enable_channel(priv->idmac_ch);
+
+	return 0;
+}
+
+static int csi_idmac_start(struct csi_priv *priv)
+{
+	struct imx_media_video_dev *vdev = priv->vdev;
+	struct v4l2_pix_format *outfmt;
+	int ret;
+
+	ret = csi_idmac_get_ipu_resources(priv);
+	if (ret)
+		return ret;
+
+	ipu_smfc_map_channel(priv->smfc, priv->csi_id, priv->vc_num);
+
+	outfmt = &vdev->fmt.fmt.pix;
+
+	ret = imx_media_alloc_dma_buf(priv->md, &priv->underrun_buf,
+				      outfmt->sizeimage);
+	if (ret)
+		goto out_put_ipu;
+
+	priv->ipu_buf_num = 0;
+
+	/* init EOF completion waitq */
+	init_completion(&priv->last_eof_comp);
+	priv->last_eof = false;
+
+	ret = csi_idmac_setup(priv);
+	if (ret) {
+		v4l2_err(&priv->sd, "csi_idmac_setup failed: %d\n", ret);
+		goto out_free_dma_buf;
+	}
+
+	priv->nfb4eof_irq = ipu_idmac_channel_irq(priv->ipu,
+						 priv->idmac_ch,
+						 IPU_IRQ_NFB4EOF);
+	ret = devm_request_irq(priv->dev, priv->nfb4eof_irq,
+			       csi_idmac_nfb4eof_interrupt, 0,
+			       "imx-smfc-nfb4eof", priv);
+	if (ret) {
+		v4l2_err(&priv->sd,
+			 "Error registering NFB4EOF irq: %d\n", ret);
+		goto out_unsetup;
+	}
+
+	priv->eof_irq = ipu_idmac_channel_irq(priv->ipu, priv->idmac_ch,
+					      IPU_IRQ_EOF);
+
+	ret = devm_request_irq(priv->dev, priv->eof_irq,
+			       csi_idmac_eof_interrupt, 0,
+			       "imx-smfc-eof", priv);
+	if (ret) {
+		v4l2_err(&priv->sd,
+			 "Error registering eof irq: %d\n", ret);
+		goto out_free_nfb4eof_irq;
+	}
+
+	/* start the EOF timeout timer */
+	mod_timer(&priv->eof_timeout_timer,
+		  jiffies + msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
+
+	return 0;
+
+out_free_nfb4eof_irq:
+	devm_free_irq(priv->dev, priv->nfb4eof_irq, priv);
+out_unsetup:
+	csi_idmac_unsetup(priv);
+out_free_dma_buf:
+	imx_media_free_dma_buf(priv->md, &priv->underrun_buf);
+out_put_ipu:
+	csi_idmac_put_ipu_resources(priv);
+	return ret;
+}
+
+static void csi_idmac_stop(struct csi_priv *priv)
+{
+	unsigned long flags;
+	int ret;
+
+	/* mark next EOF interrupt as the last before stream off */
+	spin_lock_irqsave(&priv->irqlock, flags);
+	priv->last_eof = true;
+	spin_unlock_irqrestore(&priv->irqlock, flags);
+
+	/*
+	 * and then wait for interrupt handler to mark completion.
+	 */
+	ret = wait_for_completion_timeout(
+		&priv->last_eof_comp, msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
+	if (ret == 0)
+		v4l2_warn(&priv->sd, "wait last EOF timeout\n");
+
+	devm_free_irq(priv->dev, priv->eof_irq, priv);
+	devm_free_irq(priv->dev, priv->nfb4eof_irq, priv);
+
+	csi_idmac_unsetup(priv);
+
+	imx_media_free_dma_buf(priv->md, &priv->underrun_buf);
+
+	/* cancel the EOF timeout timer */
+	del_timer_sync(&priv->eof_timeout_timer);
+
+	csi_idmac_put_ipu_resources(priv);
+}
+
+/* Update the CSI whole sensor and active windows */
+static int csi_setup(struct csi_priv *priv)
+{
+	struct v4l2_mbus_framefmt *infmt, *outfmt;
+	struct v4l2_mbus_config sensor_mbus_cfg;
+	struct v4l2_of_endpoint *sensor_ep;
+	struct v4l2_mbus_framefmt if_fmt;
+
+	infmt = &priv->format_mbus[CSI_SINK_PAD];
+	outfmt = &priv->format_mbus[priv->active_output_pad];
+	sensor_ep = &priv->sensor->sensor_ep;
+
+	/* compose mbus_config from sensor endpoint */
+	sensor_mbus_cfg.type = sensor_ep->bus_type;
+	sensor_mbus_cfg.flags = (sensor_ep->bus_type == V4L2_MBUS_CSI2) ?
+		sensor_ep->bus.mipi_csi2.flags :
+		sensor_ep->bus.parallel.flags;
+
+	/*
+	 * we need to pass input sensor frame to CSI interface, but
+	 * with translated field type from output format
+	 */
+	if_fmt = *infmt;
+	if_fmt.field = outfmt->field;
+
+	ipu_csi_set_window(priv->csi, &priv->crop);
+
+	ipu_csi_init_interface(priv->csi, &sensor_mbus_cfg, &if_fmt);
+
+	ipu_csi_set_dest(priv->csi, priv->dest);
+
+	ipu_csi_dump(priv->csi);
+
+	return 0;
+}
+
+static int csi_start(struct csi_priv *priv)
+{
+	int ret;
+
+	if (!priv->sensor) {
+		v4l2_err(&priv->sd, "no sensor attached\n");
+		return -EINVAL;
+	}
+
+	if (priv->dest == IPU_CSI_DEST_IDMAC) {
+		ret = csi_idmac_start(priv);
+		if (ret)
+			return ret;
+	}
+
+	ret = csi_setup(priv);
+	if (ret)
+		goto idmac_stop;
+
+	/* start the frame interval monitor */
+	if (priv->fim) {
+		ret = imx_media_fim_set_stream(priv->fim, priv->sensor, true);
+		if (ret)
+			goto idmac_stop;
+	}
+
+	ret = ipu_csi_enable(priv->csi);
+	if (ret) {
+		v4l2_err(&priv->sd, "CSI enable error: %d\n", ret);
+		goto fim_off;
+	}
+
+	return 0;
+
+fim_off:
+	if (priv->fim)
+		imx_media_fim_set_stream(priv->fim, priv->sensor, false);
+idmac_stop:
+	if (priv->dest == IPU_CSI_DEST_IDMAC)
+		csi_idmac_stop(priv);
+	return ret;
+}
+
+static void csi_stop(struct csi_priv *priv)
+{
+	if (priv->dest == IPU_CSI_DEST_IDMAC)
+		csi_idmac_stop(priv);
+
+	/* stop the frame interval monitor */
+	if (priv->fim)
+		imx_media_fim_set_stream(priv->fim, priv->sensor, false);
+
+	ipu_csi_disable(priv->csi);
+}
+
+static int csi_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	int ret = 0;
+
+	if (!priv->src_sd || !priv->sink)
+		return -EPIPE;
+
+	dev_dbg(priv->dev, "stream %s\n", enable ? "ON" : "OFF");
+
+	if (enable && !priv->stream_on)
+		ret = csi_start(priv);
+	else if (!enable && priv->stream_on)
+		csi_stop(priv);
+
+	if (!ret)
+		priv->stream_on = enable;
+	return ret;
+}
+
+static int csi_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	int ret = 0;
+
+	dev_dbg(priv->dev, "power %s\n", on ? "ON" : "OFF");
+
+	if (priv->fim && on != priv->power_on)
+		ret = imx_media_fim_set_power(priv->fim, on);
+
+	if (!ret)
+		priv->power_on = on;
+	return ret;
+}
+
+static int csi_link_setup(struct media_entity *entity,
+			  const struct media_pad *local,
+			  const struct media_pad *remote, u32 flags)
+{
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct imx_media_video_dev *vdev = priv->vdev;
+	struct v4l2_subdev *remote_sd;
+	int ret;
+
+	dev_dbg(priv->dev, "link setup %s -> %s", remote->entity->name,
+		local->entity->name);
+
+	if (local->flags & MEDIA_PAD_FL_SINK) {
+		if (!is_media_entity_v4l2_subdev(remote->entity))
+			return -EINVAL;
+
+		remote_sd = media_entity_to_v4l2_subdev(remote->entity);
+
+		if (flags & MEDIA_LNK_FL_ENABLED) {
+			if (priv->src_sd)
+				return -EBUSY;
+			priv->src_sd = remote_sd;
+		} else {
+			priv->src_sd = NULL;
+		}
+
+		return 0;
+	}
+
+	/* this is a source pad */
+
+	if (flags & MEDIA_LNK_FL_ENABLED) {
+		if (priv->sink)
+			return -EBUSY;
+	} else {
+		/* reset video device controls */
+		v4l2_ctrl_handler_free(vdev->vfd->ctrl_handler);
+		v4l2_ctrl_handler_init(vdev->vfd->ctrl_handler, 0);
+
+		priv->sink = NULL;
+		return 0;
+	}
+
+	/* record which output pad is now active */
+	priv->active_output_pad = local->index;
+
+	/* set CSI destination */
+	if (local->index == CSI_SRC_PAD_IDMAC) {
+		if (!is_media_entity_v4l2_video_device(remote->entity))
+			return -EINVAL;
+
+		/* reset video device controls to refresh from subdevs */
+		v4l2_ctrl_handler_free(vdev->vfd->ctrl_handler);
+		v4l2_ctrl_handler_init(vdev->vfd->ctrl_handler, 0);
+
+		ret = __v4l2_pipeline_inherit_controls(vdev->vfd,
+						       &priv->sd.entity);
+		if (ret)
+			return ret;
+
+		priv->dest = IPU_CSI_DEST_IDMAC;
+	} else {
+		if (!is_media_entity_v4l2_subdev(remote->entity))
+			return -EINVAL;
+
+		remote_sd = media_entity_to_v4l2_subdev(remote->entity);
+		switch (remote_sd->grp_id) {
+		case IMX_MEDIA_GRP_ID_VDIC:
+			priv->dest = IPU_CSI_DEST_VDIC;
+			break;
+		case IMX_MEDIA_GRP_ID_IC_PRP:
+			priv->dest = IPU_CSI_DEST_IC;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	priv->sink = remote->entity;
+
+	return 0;
+}
+
+static int csi_link_validate(struct v4l2_subdev *sd,
+			     struct media_link *link,
+			     struct v4l2_subdev_format *source_fmt,
+			     struct v4l2_subdev_format *sink_fmt)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct v4l2_of_endpoint *sensor_ep;
+	bool is_csi2;
+	int ret;
+
+	ret = v4l2_subdev_link_validate_default(sd, link,
+						source_fmt, sink_fmt);
+	if (ret)
+		return ret;
+
+	priv->sensor = __imx_media_find_sensor(priv->md, &priv->sd.entity);
+	if (IS_ERR(priv->sensor)) {
+		v4l2_err(&priv->sd, "no sensor attached\n");
+		ret = PTR_ERR(priv->sensor);
+		priv->sensor = NULL;
+		return ret;
+	}
+
+	sensor_ep = &priv->sensor->sensor_ep;
+
+	is_csi2 = (sensor_ep->bus_type == V4L2_MBUS_CSI2);
+
+	if (is_csi2) {
+		int vc_num = 0;
+		/*
+		 * NOTE! It seems the virtual channels from the mipi csi-2
+		 * receiver are used only for routing by the video mux's,
+		 * or for hard-wired routing to the CSI's. Once the stream
+		 * enters the CSI's however, they are treated internally
+		 * in the IPU as virtual channel 0.
+		 */
+#if 0
+		vc_num = imx_media_find_mipi_csi2_channel(priv->md,
+							  &priv->sd.entity);
+		if (vc_num < 0)
+			return vc_num;
+#endif
+		ipu_csi_set_mipi_datatype(priv->csi, vc_num,
+					  &priv->format_mbus[CSI_SINK_PAD]);
+	}
+
+	/* select either parallel or MIPI-CSI2 as input to CSI */
+	ipu_set_csi_src_mux(priv->ipu, priv->csi_id, is_csi2);
+
+	return 0;
+}
+
+static int csi_eof_isr(struct v4l2_subdev *sd, u32 status, bool *handled)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+
+	csi_call_fim(priv);
+
+	return 0;
+}
+
+static int csi_try_crop(struct csi_priv *priv, struct v4l2_rect *crop,
+			struct imx_media_subdev *sensor)
+{
+	struct v4l2_of_endpoint *sensor_ep;
+	struct v4l2_mbus_framefmt *infmt;
+	v4l2_std_id std;
+	int ret;
+
+	infmt = &priv->format_mbus[CSI_SINK_PAD];
+	sensor_ep = &sensor->sensor_ep;
+
+	crop->width = min_t(__u32, infmt->width, crop->width);
+	if (crop->left + crop->width > infmt->width)
+		crop->left = infmt->width - crop->width;
+	/* adjust crop left/width to h/w alignment restrictions */
+	crop->left &= ~0x3;
+	crop->width &= ~0x7;
+
+	/*
+	 * FIXME: not sure why yet, but on interlaced bt.656,
+	 * changing the vertical cropping causes loss of vertical
+	 * sync, so fix it to NTSC/PAL active lines. NTSC contains
+	 * 2 extra lines of active video that need to be cropped.
+	 */
+	if (sensor_ep->bus_type == V4L2_MBUS_BT656) {
+		ret = v4l2_subdev_call(sensor->sd, video, g_std, &std);
+		if (ret)
+			return ret;
+		if (std & V4L2_STD_525_60) {
+			crop->top = 2;
+			crop->height = 480;
+		} else {
+			crop->top = 0;
+			crop->height = 576;
+		}
+	} else {
+		crop->height = min_t(__u32, infmt->height, crop->height);
+		if (crop->top + crop->height > infmt->height)
+			crop->top = infmt->height - crop->height;
+	}
+
+	return 0;
+}
+
+static int csi_enum_mbus_code(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_pad_config *cfg,
+			      struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->pad >= CSI_NUM_PADS)
+		return -EINVAL;
+
+	if (code->pad == CSI_SRC_PAD_DIRECT)
+		return imx_media_enum_ipu_format(NULL, &code->code,
+						 code->index, true);
+
+	return imx_media_enum_format(NULL, &code->code, code->index,
+				     true, false);
+}
+
+static int csi_get_fmt(struct v4l2_subdev *sd,
+		       struct v4l2_subdev_pad_config *cfg,
+		       struct v4l2_subdev_format *sdformat)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+
+	if (sdformat->pad >= CSI_NUM_PADS)
+		return -EINVAL;
+
+	sdformat->format = priv->format_mbus[sdformat->pad];
+
+	return 0;
+}
+
+static int csi_set_fmt(struct v4l2_subdev *sd,
+		       struct v4l2_subdev_pad_config *cfg,
+		       struct v4l2_subdev_format *sdformat)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	const struct imx_media_pixfmt *cc, *incc;
+	struct v4l2_mbus_framefmt *infmt;
+	struct imx_media_subdev *sensor;
+	struct v4l2_rect crop;
+	u32 code;
+	int ret;
+
+	if (sdformat->pad >= CSI_NUM_PADS)
+		return -EINVAL;
+
+	if (priv->stream_on)
+		return -EBUSY;
+
+	infmt = &priv->format_mbus[CSI_SINK_PAD];
+
+	sensor = imx_media_find_sensor(priv->md, &priv->sd.entity);
+	if (IS_ERR(sensor)) {
+		v4l2_err(&priv->sd, "no sensor attached\n");
+		return PTR_ERR(sensor);
+	}
+
+	v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
+			      W_ALIGN, &sdformat->format.height,
+			      MIN_H, MAX_H, H_ALIGN, S_ALIGN);
+
+	switch (sdformat->pad) {
+	case CSI_SRC_PAD_DIRECT:
+	case CSI_SRC_PAD_IDMAC:
+		crop.left = priv->crop.left;
+		crop.top = priv->crop.top;
+		crop.width = sdformat->format.width;
+		crop.height = sdformat->format.height;
+		ret = csi_try_crop(priv, &crop, sensor);
+		if (ret)
+			return ret;
+		sdformat->format.width = crop.width;
+		sdformat->format.height = crop.height;
+
+		if (sdformat->pad == CSI_SRC_PAD_IDMAC) {
+			cc = imx_media_find_format(0, sdformat->format.code,
+						   true, false);
+			if (!cc) {
+				imx_media_enum_format(NULL, &code, 0,
+						      true, false);
+				cc = imx_media_find_format(0, code,
+							   true, false);
+				sdformat->format.code = cc->codes[0];
+			}
+
+			incc = priv->cc[CSI_SINK_PAD];
+			if (cc->cs != incc->cs) {
+				sdformat->format.code = infmt->code;
+				cc = imx_media_find_format(
+					0, sdformat->format.code,
+					true, false);
+			}
+
+			if (sdformat->format.field != V4L2_FIELD_NONE)
+				sdformat->format.field = infmt->field;
+		} else {
+			cc = imx_media_find_ipu_format(0, sdformat->format.code,
+						       true);
+			if (!cc) {
+				imx_media_enum_ipu_format(NULL, &code, 0, true);
+				cc = imx_media_find_ipu_format(0, code, true);
+				sdformat->format.code = cc->codes[0];
+			}
+
+			sdformat->format.field = infmt->field;
+		}
+
+		/*
+		 * translate V4L2_FIELD_ALTERNATE to SEQ_TB or SEQ_BT
+		 * depending on video standard from sensor
+		 */
+		if (sdformat->format.field == V4L2_FIELD_ALTERNATE) {
+			v4l2_std_id std;
+
+			ret = v4l2_subdev_call(sensor->sd, video, g_std, &std);
+			if (ret)
+				return ret;
+			sdformat->format.field = (std & V4L2_STD_525_60) ?
+				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
+		}
+		break;
+	case CSI_SINK_PAD:
+		cc = imx_media_find_format(0, sdformat->format.code,
+					   true, false);
+		if (!cc) {
+			imx_media_enum_format(NULL, &code, 0, true, false);
+			cc = imx_media_find_format(0, code, true, false);
+			sdformat->format.code = cc->codes[0];
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
+		cfg->try_fmt = sdformat->format;
+	} else {
+		priv->format_mbus[sdformat->pad] = sdformat->format;
+		priv->cc[sdformat->pad] = cc;
+		/* Update the crop window if this is an output pad  */
+		if (sdformat->pad == CSI_SRC_PAD_DIRECT ||
+		    sdformat->pad == CSI_SRC_PAD_IDMAC)
+			priv->crop = crop;
+	}
+
+	return 0;
+}
+
+static int csi_get_selection(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_pad_config *cfg,
+			     struct v4l2_subdev_selection *sel)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct v4l2_mbus_framefmt *infmt;
+
+	if (sel->pad >= CSI_NUM_PADS || sel->pad == CSI_SINK_PAD)
+		return -EINVAL;
+
+	infmt = &priv->format_mbus[CSI_SINK_PAD];
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.left = 0;
+		sel->r.top = 0;
+		sel->r.width = infmt->width;
+		sel->r.height = infmt->height;
+		break;
+	case V4L2_SEL_TGT_CROP:
+		sel->r = priv->crop;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int csi_set_selection(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_pad_config *cfg,
+			     struct v4l2_subdev_selection *sel)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct v4l2_mbus_framefmt *outfmt;
+	struct imx_media_subdev *sensor;
+	int ret;
+
+	if (sel->pad >= CSI_NUM_PADS ||
+	    sel->pad == CSI_SINK_PAD ||
+	    sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	if (priv->stream_on)
+		return -EBUSY;
+
+	sensor = imx_media_find_sensor(priv->md, &priv->sd.entity);
+	if (IS_ERR(sensor)) {
+		v4l2_err(&priv->sd, "no sensor attached\n");
+		return PTR_ERR(sensor);
+	}
+
+	/*
+	 * Modifying the crop rectangle always changes the format on the source
+	 * pad. If the KEEP_CONFIG flag is set, just return the current crop
+	 * rectangle.
+	 */
+	if (sel->flags & V4L2_SEL_FLAG_KEEP_CONFIG) {
+		sel->r = priv->crop;
+		if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
+			cfg->try_crop = sel->r;
+		return 0;
+	}
+
+	outfmt = &priv->format_mbus[sel->pad];
+
+	ret = csi_try_crop(priv, &sel->r, sensor);
+	if (ret)
+		return ret;
+
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		cfg->try_crop = sel->r;
+	} else {
+		priv->crop = sel->r;
+		/* Update the source format */
+		outfmt->width = sel->r.width;
+		outfmt->height = sel->r.height;
+	}
+
+	return 0;
+}
+
+/*
+ * retrieve our pads parsed from the OF graph by the media device
+ */
+static int csi_registered(struct v4l2_subdev *sd)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	int i, ret;
+	u32 code;
+
+	/* get media device */
+	priv->md = dev_get_drvdata(sd->v4l2_dev->dev);
+
+	/* get handle to IPU CSI */
+	priv->csi = ipu_csi_get(priv->ipu, priv->csi_id);
+	if (IS_ERR(priv->csi)) {
+		v4l2_err(&priv->sd, "failed to get CSI%d\n", priv->csi_id);
+		return PTR_ERR(priv->csi);
+	}
+
+	for (i = 0; i < CSI_NUM_PADS; i++) {
+		priv->pad[i].flags = (i == CSI_SINK_PAD) ?
+			MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
+
+		code = 0;
+		if (i == CSI_SRC_PAD_DIRECT)
+			imx_media_enum_ipu_format(NULL, &code, 0, true);
+
+		/* set a default mbus format  */
+		ret = imx_media_init_mbus_fmt(&priv->format_mbus[i],
+					      640, 480, code, V4L2_FIELD_NONE,
+					      &priv->cc[i]);
+		if (ret)
+			goto put_csi;
+	}
+
+	priv->fim = imx_media_fim_init(&priv->sd);
+	if (IS_ERR(priv->fim)) {
+		ret = PTR_ERR(priv->fim);
+		goto put_csi;
+	}
+
+	ret = media_entity_pads_init(&sd->entity, CSI_NUM_PADS, priv->pad);
+	if (ret)
+		goto free_fim;
+
+	ret = imx_media_capture_device_register(priv->vdev);
+	if (ret)
+		goto free_fim;
+
+	return 0;
+
+free_fim:
+	if (priv->fim)
+		imx_media_fim_free(priv->fim);
+put_csi:
+	ipu_csi_put(priv->csi);
+	return ret;
+}
+
+static void csi_unregistered(struct v4l2_subdev *sd)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+
+	imx_media_capture_device_unregister(priv->vdev);
+
+	if (priv->fim)
+		imx_media_fim_free(priv->fim);
+
+	if (!IS_ERR_OR_NULL(priv->csi))
+		ipu_csi_put(priv->csi);
+}
+
+static struct media_entity_operations csi_entity_ops = {
+	.link_setup = csi_link_setup,
+	.link_validate = v4l2_subdev_link_validate,
+};
+
+static struct v4l2_subdev_core_ops csi_core_ops = {
+	.s_power = csi_s_power,
+	.interrupt_service_routine = csi_eof_isr,
+};
+
+static struct v4l2_subdev_video_ops csi_video_ops = {
+	.s_stream = csi_s_stream,
+};
+
+static struct v4l2_subdev_pad_ops csi_pad_ops = {
+	.enum_mbus_code = csi_enum_mbus_code,
+	.get_fmt = csi_get_fmt,
+	.set_fmt = csi_set_fmt,
+	.get_selection = csi_get_selection,
+	.set_selection = csi_set_selection,
+	.link_validate = csi_link_validate,
+};
+
+static struct v4l2_subdev_ops csi_subdev_ops = {
+	.core = &csi_core_ops,
+	.video = &csi_video_ops,
+	.pad = &csi_pad_ops,
+};
+
+static struct v4l2_subdev_internal_ops csi_internal_ops = {
+	.registered = csi_registered,
+	.unregistered = csi_unregistered,
+};
+
+static int imx_csi_probe(struct platform_device *pdev)
+{
+	struct ipu_client_platformdata *pdata;
+	struct csi_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, &priv->sd);
+	priv->dev = &pdev->dev;
+
+	ret = dma_set_coherent_mask(priv->dev, DMA_BIT_MASK(32));
+	if (ret)
+		return ret;
+
+	/* get parent IPU */
+	priv->ipu = dev_get_drvdata(priv->dev->parent);
+
+	/* get our CSI id */
+	pdata = priv->dev->platform_data;
+	priv->csi_id = pdata->csi;
+	priv->smfc_id = (priv->csi_id == 0) ? 0 : 2;
+
+	init_timer(&priv->eof_timeout_timer);
+	priv->eof_timeout_timer.data = (unsigned long)priv;
+	priv->eof_timeout_timer.function = csi_idmac_eof_timeout;
+	spin_lock_init(&priv->irqlock);
+
+	v4l2_subdev_init(&priv->sd, &csi_subdev_ops);
+	v4l2_set_subdevdata(&priv->sd, priv);
+	priv->sd.internal_ops = &csi_internal_ops;
+	priv->sd.entity.ops = &csi_entity_ops;
+	priv->sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
+	priv->sd.dev = &pdev->dev;
+	priv->sd.owner = THIS_MODULE;
+	priv->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+	priv->sd.grp_id = priv->csi_id ?
+		IMX_MEDIA_GRP_ID_CSI1 : IMX_MEDIA_GRP_ID_CSI0;
+	imx_media_grp_id_to_sd_name(priv->sd.name, sizeof(priv->sd.name),
+				    priv->sd.grp_id, ipu_get_num(priv->ipu));
+
+	priv->vdev = imx_media_capture_device_init(&priv->sd,
+						   CSI_SRC_PAD_IDMAC);
+	if (IS_ERR(priv->vdev))
+		return PTR_ERR(priv->vdev);
+
+	v4l2_ctrl_handler_init(&priv->ctrl_hdlr, 0);
+	priv->sd.ctrl_handler = &priv->ctrl_hdlr;
+
+	ret = v4l2_async_register_subdev(&priv->sd);
+	if (ret)
+		v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
+
+	return ret;
+}
+
+static int imx_csi_remove(struct platform_device *pdev)
+{
+	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+	struct csi_priv *priv = sd_to_dev(sd);
+
+	imx_media_capture_device_remove(priv->vdev);
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+
+	return 0;
+}
+
+static const struct platform_device_id imx_csi_ids[] = {
+	{ .name = "imx-ipuv3-csi" },
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, imx_csi_ids);
+
+static struct platform_driver imx_csi_driver = {
+	.probe = imx_csi_probe,
+	.remove = imx_csi_remove,
+	.id_table = imx_csi_ids,
+	.driver = {
+		.name = "imx-ipuv3-csi",
+	},
+};
+module_platform_driver(imx_csi_driver);
+
+MODULE_DESCRIPTION("i.MX CSI subdev driver");
+MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@mentor.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:imx-ipuv3-csi");
-- 
2.7.4

[toc] | [next] | [standalone]

#1582516

On Wed, Feb 15, 2017 at 06:19:22PM -0800, Steve Longerbeam wrote:
> +static const struct platform_device_id imx_csi_ids[] = {
> +	{ .name = "imx-ipuv3-csi" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(platform, imx_csi_ids);
> +
> +static struct platform_driver imx_csi_driver = {
> +	.probe = imx_csi_probe,
> +	.remove = imx_csi_remove,
> +	.id_table = imx_csi_ids,
> +	.driver = {
> +		.name = "imx-ipuv3-csi",
> +	},
> +};
> +module_platform_driver(imx_csi_driver);
> +
> +MODULE_DESCRIPTION("i.MX CSI subdev driver");
> +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@mentor.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imx-ipuv3-csi");

Just a reminder that automatic module loading of this is completely
broken right now (not your problem) due to this stupid idea in the
IPUv3 code:

		if (!ret)
			ret = platform_device_add(pdev);
		if (ret) {
			platform_device_put(pdev);
			goto err_register;
		}

		/*
		 * Set of_node only after calling platform_device_add. Otherwise
		 * the platform:imx-ipuv3-crtc modalias won't be used.
		 */
		pdev->dev.of_node = of_node;

setting pdev->dev.of_node changes the modalias exported to userspace,
so udev sees a DT based modalias, which causes it to totally miss any
driver using a non-DT based modalias.

The IPUv3 code needs fixing, not only for imx-media-csi, but also for
imx-ipuv3-crtc too, because that module will also suffer the same
issue.

The only solution is... don't fsck with dev->of_node assignment.  In
this case, it's probably much better to pass it in via platform data.
If you then absolutely must have dev->of_node, doing it in the driver
means that you avoid the modalias mess before the appropriate driver
is loaded.  However, that's still not a nice solution because the
modalias file still ends up randomly changing its contents.

As I say, not _your_ problem, but it's still a problem that needs
solving, and I don't want it forgotten about.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1582532

On Thu, Feb 16, 2017 at 11:52:06AM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 15, 2017 at 06:19:22PM -0800, Steve Longerbeam wrote:
> > +static const struct platform_device_id imx_csi_ids[] = {
> > +	{ .name = "imx-ipuv3-csi" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(platform, imx_csi_ids);
> > +
> > +static struct platform_driver imx_csi_driver = {
> > +	.probe = imx_csi_probe,
> > +	.remove = imx_csi_remove,
> > +	.id_table = imx_csi_ids,
> > +	.driver = {
> > +		.name = "imx-ipuv3-csi",
> > +	},
> > +};
> > +module_platform_driver(imx_csi_driver);
> > +
> > +MODULE_DESCRIPTION("i.MX CSI subdev driver");
> > +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@mentor.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:imx-ipuv3-csi");
> 
> Just a reminder that automatic module loading of this is completely
> broken right now (not your problem) due to this stupid idea in the
> IPUv3 code:
> 
> 		if (!ret)
> 			ret = platform_device_add(pdev);
> 		if (ret) {
> 			platform_device_put(pdev);
> 			goto err_register;
> 		}
> 
> 		/*
> 		 * Set of_node only after calling platform_device_add. Otherwise
> 		 * the platform:imx-ipuv3-crtc modalias won't be used.
> 		 */
> 		pdev->dev.of_node = of_node;
> 
> setting pdev->dev.of_node changes the modalias exported to userspace,
> so udev sees a DT based modalias, which causes it to totally miss any
> driver using a non-DT based modalias.
> 
> The IPUv3 code needs fixing, not only for imx-media-csi, but also for
> imx-ipuv3-crtc too, because that module will also suffer the same
> issue.
> 
> The only solution is... don't fsck with dev->of_node assignment.  In
> this case, it's probably much better to pass it in via platform data.
> If you then absolutely must have dev->of_node, doing it in the driver
> means that you avoid the modalias mess before the appropriate driver
> is loaded.  However, that's still not a nice solution because the
> modalias file still ends up randomly changing its contents.
> 
> As I say, not _your_ problem, but it's still a problem that needs
> solving, and I don't want it forgotten about.

I've just hacked up a solution to this, and unfortunately it reveals a
problem with Steve's code.  Picking out the imx & media-related messages:

[    8.012191] imx_media_common: module is from the staging directory, the quality is unknown, you have been warned.
[    8.018175] imx_media: module is from the staging directory, the quality is unknown, you have been warned.
[    8.748345] imx-media: Registered subdev ipu1_csi0_mux
[    8.753451] imx-media: Registered subdev ipu2_csi1_mux
[    9.055196] imx219 0-0010: detected IMX219 sensor
[    9.090733] imx6_mipi_csi2: module is from the staging directory, the quality is unknown, you have been warned.
[    9.092247] imx-media: Registered subdev imx219 0-0010
[    9.334338] imx-media: Registered subdev imx6-mipi-csi2
[    9.372452] imx_media_capture: module is from the staging directory, the quality is unknown, you have been warned.
[    9.378163] imx_media_capture: module is from the staging directory, the quality is unknown, you have been warned.
[    9.390033] imx_media_csi: module is from the staging directory, the quality is unknown, you have been warned.
[    9.394362] imx-media: Received unknown subdev ipu1_csi0
[    9.394699] imx-ipuv3-csi: probe of imx-ipuv3-csi.0 failed with error -22
[    9.394840] imx-media: Received unknown subdev ipu1_csi1
[    9.394887] imx-ipuv3-csi: probe of imx-ipuv3-csi.1 failed with error -22
[    9.394992] imx-media: Received unknown subdev ipu2_csi0
[    9.395026] imx-ipuv3-csi: probe of imx-ipuv3-csi.4 failed with error -22
[    9.395119] imx-media: Received unknown subdev ipu2_csi1
[    9.395159] imx-ipuv3-csi: probe of imx-ipuv3-csi.5 failed with error -22
[    9.411722] imx_media_vdic: module is from the staging directory, the quality is unknown, you have been warned.
[    9.412820] imx-media: Registered subdev ipu1_vdic
[    9.424687] imx-media: Registered subdev ipu2_vdic
[    9.436074] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
[    9.437455] imx-media: Registered subdev ipu1_ic_prp
[    9.437788] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
[    9.447542] imx-media: Registered subdev ipu1_ic_prpenc
[    9.455225] ipu1_ic_prpenc: Registered ipu1_ic_prpenc capture as /dev/video3
[    9.459203] imx-media: Registered subdev ipu1_ic_prpvf
[    9.460484] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
[    9.460726] ipu1_ic_prpvf: Registered ipu1_ic_prpvf capture as /dev/video4
[    9.460983] imx-media: Registered subdev ipu2_ic_prp
[    9.461161] imx-media: Registered subdev ipu2_ic_prpenc
[    9.461737] ipu2_ic_prpenc: Registered ipu2_ic_prpenc capture as /dev/video5
[    9.463767] imx-media: Registered subdev ipu2_ic_prpvf
[    9.464294] ipu2_ic_prpvf: Registered ipu2_ic_prpvf capture as /dev/video6
[    9.464345] imx-media: imx_media_create_link: (null):1 -> ipu1_ic_prp:0
[    9.464413] ------------[ cut here ]------------
[    9.469134] kernel BUG at /home/rmk/git/linux-rmk/drivers/media/media-entity.c:628!
[    9.476924] Internal error: Oops - BUG: 0 [#1] SMP ARM
[    9.482246] Modules linked in: imx_media_ic(C+) imx_media_vdic(C) imx_media_csi(C) imx_media_capture(C) uvcvideo imx6_mipi_csi2(C) snd_soc_imx_audmux imx219 snd_soc_sgtl5000 video_multiplexer caam imx_sdma imx2_wdt snd_soc_fsl_ssi snd_soc_fsl_spdif imx_pcm_dma coda imx_thermal v4l2_mem2mem videobuf2_v4l2 videobuf2_dma_contig videobuf2_core videobuf2_vmalloc videobuf2_memops imx_media(C) imx_media_common(C) rc_pinnacle_pctv_hd nfsd dw_hdmi_cec dw_hdmi_ahb_audio etnaviv
[    9.524500] CPU: 1 PID: 263 Comm: systemd-udevd Tainted: G         C      4.10.0-rc7+ #2112
[    9.532995] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    9.539619] task: edef1880 task.stack: d03ca000
[    9.544313] PC is at media_create_pad_link+0x134/0x140
[    9.549541] LR is at imx_media_probe_complete+0x164/0x24c [imx_media]
[    9.556080] pc : [<c04f0eb0>]    lr : [<bf052524>]    psr: 60070013
               sp : d03cbbc8  ip : d03cbbf8  fp : d03cbbf4
[    9.567712] r10: 00000001  r9 : 00000000  r8 : d0170d14
[    9.573007] r7 : 00000000  r6 : 00000001  r5 : 00000000  r4 : d0170d14
[    9.579612] r3 : 00000000  r2 : d0170d14  r1 : 00000001  r0 : 00000000
[    9.586256] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none[    9.593486] Control: 10c5387d  Table: 3e77c04a  DAC: 00000051
[    9.599317] Process systemd-udevd (pid: 263, stack limit = 0xd03ca210)
[    9.605950] Stack: (0xd03cbbc8 to 0xd03cc000)
[    9.610368] bbc0:                   00000000 00000000 ee980410 00000000 00000000 d0170d14
[    9.618658] bbe0: 00000000 00000001 d03cbc54 d03cbbf8 bf052524 c04f0d88 00000000 d0170d88
[    9.626961] bc00: 00000000 c0a57dc4 0004a364 ee98011c 00000000 00000003 00000001 ee980230
[    9.635267] bc20: ee980274 ee980010 d03cbc54 d0170f14 ee9ca4cc ee9974c4 bf0523c0 c0a57dc4
[    9.643539] bc40: f184bb30 00000026 d03cbc74 d03cbc58 c0502f50 bf0523cc ee9ca4cc d0170f14
[    9.651824] bc60: c0a57e08 d0170fc0 d03cbc9c d03cbc78 c0502fdc c0502e70 00000000 d0170f10
[    9.660132] bc80: 00000000 d02c0c10 bf122cd0 d0170f14 d03cbcc4 d03cbca0 bf121154 c0502f68
[    9.668423] bca0: bf12104c ffffffed d02c0c10 fffffdfb bf123248 00000000 d03cbce4 d03cbcc8
[    9.676713] bcc0: c041aeb4 bf121058 d02c0c10 c1419d70 00000000 bf123248 d03cbd0c d03cbce8
[    9.684992] bce0: c0418ec4 c041ae68 d02c0c10 bf123248 d02c0c44 00000000 00000001 00000124
[    9.693282] bd00: d03cbd2c d03cbd10 c0419044 c0418ccc 00000000 00000000 bf123248 c0418f88
[    9.701618] bd20: d03cbd54 d03cbd30 c04172e4 c0418f94 ef0f64a4 d01b8cd0 d03d9858 bf123248
[    9.709900] bd40: d03d9c00 c0a45e10 d03cbd64 d03cbd58 c0418728 c0417294 d03cbd8c d03cbd68
[    9.718203] bd60: c0418428 c0418710 bf122e48 d03cbd78 bf123248 c0a704a8 bf126000 00000000
[    9.729180] bd80: d03cbda4 d03cbd90 c0419ec4 c0418340 bf123480 c0a704a8 d03cbdb4 d03cbda8
[    9.739950] bda0: c041ad88 c0419e50 d03cbdc4 d03cbdb8 bf126018 c041ad4c d03cbe34 d03cbdc8
[    9.751089] bdc0: c00098ac bf12600c d03cbdec d03cbdd8 c00a8888 c0087240 00000000 ed4a9440
[    9.761941] bde0: d03cbe34 d03cbdf0 c016c690 c00a8814 c016b554 c016aa60 00000001 c015f3f8
[    9.772940] be00: 00000005 0000000c edef1880 bf123480 c0a704a8 bf123480 c0a704a8 ed4a9440
[    9.784008] be20: bf123480 00000001 d03cbe5c d03cbe38 c011b1e4 c0009874 d03cbe5c d03cbe48
[    9.795017] be40: c09f5ea7 c0a704a8 c09e04ec bf123480 d03cbf14 d03cbe60 c00d2dd0 c011b188
[    9.806069] be60: bf12348c 00007fff bf123480 c00d09f0 f1847000 bf12792c f18495c0 bf123680
[    9.817194] be80: bf12348c bf1236f0 00000000 bf1234c8 c017c1d0 c017bfac f1847000 00004f68
[    9.828347] bea0: c017c2e8 00000000 edef1880 00000000 00000000 00000000 00000000 00000000
[    9.839542] bec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    9.850754] bee0: 00000000 00000000 00000003 7fffffff 00000000 00000000 00000007 b6c9e63c
[    9.861991] bf00: d03ca000 00000000 d03cbfa4 d03cbf18 c00d36cc c00d1480 7fffffff 00000000
[    9.873266] bf20: 00000003 ee0384d4 d03cbf74 f1847000 00004f68 00000000 00000002 f1847000
[    9.884593] bf40: 00004f68 f184bb30 f184989b f184a7d8 000026f0 00002dd0 00000000 00000000
[    9.895998] bf60: 00000000 0000192c 00000019 0000001a 00000011 00000000 0000000a 00000000
[    9.907408] bf80: c008b848 80c36630 00000000 2529fc00 0000017b c000ff04 00000000 d03cbfa8
[    9.918858] bfa0: c000fd60 c00d3644 80c36630 00000000 00000007 b6c9e63c 00000000 80c38178
[    9.930354] bfc0: 80c36630 00000000 2529fc00 0000017b 00020000 7f96eb0c 80c37848 00000000
[    9.941900] bfe0: bed55928 bed55918 b6c988ff b6bea572 600f0030 00000007 3fffd861 3fffdc61
[    9.953532] Backtrace:
[    9.959442] [<c04f0d7c>] (media_create_pad_link) from [<bf052524>] (imx_media_probe_complete+0x164/0x24c [imx_media])
[    9.973644]  r10:00000001 r9:00000000 r8:d0170d14 r7:00000000 r6:00000000 r5:ee980410
[    9.985112]  r4:00000000 r3:00000000
[    9.992696] [<bf0523c0>] (imx_media_probe_complete [imx_media]) from [<c0502f50>] (v4l2_async_test_notify+0xec/0xf8)
[   10.007413]  r10:00000026 r9:f184bb30 r8:c0a57dc4 r7:bf0523c0 r6:ee9974c4 r5:ee9ca4cc
[   10.019212]  r4:d0170f14
[   10.025650] [<c0502e64>] (v4l2_async_test_notify) from [<c0502fdc>] (v4l2_async_register_subdev+0x80/0xdc)
[   10.039736]  r7:d0170fc0 r6:c0a57e08 r5:d0170f14 r4:ee9ca4cc
[   10.049613] [<c0502f5c>] (v4l2_async_register_subdev) from [<bf121154>] (imx_ic_probe+0x108/0x144 [imx_media_ic])
[   10.063953]  r8:d0170f14 r7:bf122cd0 r6:d02c0c10 r5:00000000 r4:d0170f10 r3:00000000
[   10.075786] [<bf12104c>] (imx_ic_probe [imx_media_ic]) from [<c041aeb4>] (platform_drv_probe+0x58/0xb8)
[   10.089118]  r8:00000000 r7:bf123248 r6:fffffdfb r5:d02c0c10 r4:ffffffed r3:bf12104c
[   10.100683] [<c041ae5c>] (platform_drv_probe) from [<c0418ec4>] (driver_probe_device+0x204/0x2c8)
[   10.113279]  r7:bf123248 r6:00000000 r5:c1419d70 r4:d02c0c10
[   10.122765] [<c0418cc0>] (driver_probe_device) from [<c0419044>] (__driver_attach+0xbc/0xc0)
[   10.135098]  r10:00000124 r8:00000001 r7:00000000 r6:d02c0c44 r5:bf123248 r4:d02c0c10
[   10.146785] [<c0418f88>] (__driver_attach) from [<c04172e4>] (bus_for_each_dev+0x5c/0x90)
[   10.158811]  r6:c0418f88 r5:bf123248 r4:00000000 r3:00000000
[   10.168375] [<c0417288>] (bus_for_each_dev) from [<c0418728>] (driver_attach+0x24/0x28)
[   10.180413]  r6:c0a45e10 r5:d03d9c00 r4:bf123248
[   10.188959] [<c0418704>] (driver_attach) from [<c0418428>] (bus_add_driver+0xf4/0x200)
[   10.200775] [<c0418334>] (bus_add_driver) from [<c0419ec4>] (driver_register+0x80/0xfc)
[   10.212707]  r7:00000000 r6:bf126000 r5:c0a704a8 r4:bf123248
[   10.222254] [<c0419e44>] (driver_register) from [<c041ad88>] (__platform_driver_register+0x48/0x4c)
[   10.235212]  r5:c0a704a8 r4:bf123480
[   10.242694] [<c041ad40>] (__platform_driver_register) from [<bf126018>] (imx_ic_driver_init+0x18/0x24 [imx_media_ic])
[   10.257308] [<bf126000>] (imx_ic_driver_init [imx_media_ic]) from [<c00098ac>] (do_one_initcall+0x44/0x170)
[   10.271043] [<c0009868>] (do_one_initcall) from [<c011b1e4>] (do_init_module+0x68/0x1d8)
[   10.283139]  r8:00000001 r7:bf123480 r6:ed4a9440 r5:c0a704a8 r4:bf123480
[   10.293849] [<c011b17c>] (do_init_module) from [<c00d2dd0>] (load_module+0x195c/0x2080)
[   10.305867]  r7:bf123480 r6:c09e04ec r5:c0a704a8 r4:c09f5ea7
[   10.315523] [<c00d1474>] (load_module) from [<c00d36cc>] (SyS_finit_module+0x94/0xa0)
[   10.327382]  r10:00000000 r9:d03ca000 r8:b6c9e63c r7:00000007 r6:00000000 r5:00000000
[   10.339238]  r4:7fffffff
[   10.345766] [<c00d3638>] (SyS_finit_module) from [<c000fd60>] (ret_fast_syscall+0x0/0x1c)
[   10.357994]  r8:c000ff04 r7:0000017b r6:2529fc00 r5:00000000 r4:80c36630
[   10.368747] Code: e1a01007 ebfffce1 e3e0000b e89daff8 (e7f001f2)
[   10.378883] ---[ end trace 2051fac455b36c5a ]---
[   11.228961] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
[   11.247536] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
[   11.301366] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.

So there's probably some sort of race going on.

However, the following is primerily directed at Laurent as the one who
introduced the BUG_ON() in question...

NEVER EVER USE BUG_ON() IN A PATH THAT CAN RETURN AN ERROR.

It's possible to find Linus rants about this, eg,
https://www.spinics.net/lists/stable/msg146439.html

 I should have reacted to the damn added BUG_ON() lines. I suspect I
 will have to finally just remove the idiotic BUG_ON() concept once and
 for all, because there is NO F*CKING EXCUSE to knowingly kill the
 kernel.

Also: http://yarchive.net/comp/linux/BUG.html

 Rule of thumb: BUG() is only good for something that never happens and
 that we really have no other option for (ie state is so corrupt that
 continuing is deadly).

So, _unless_ people want to see BUG_ON() removed from the kernel, I
strongly suggest to _STOP_ using it as "we didn't like the function
arguments, let's use it as an assert() statement instead of returning
an error."

There's no excuse what so ever to be killing the machine in
media_create_pad_link().  If it doesn't like a NULL pointer, it's damn
well got an error path to report that fact.  Use that mechanism and
stop needlessly killing the kernel.

BUG_ON() IS NOT ASSERT().  DO NOT USE IT AS SUCH.

Linus is absolutely right about BUG_ON() - it hurts debuggability,
because now the only way to do further tests is to reboot the damned
machine after removing those fscking BUG_ON()s that should *never*
have been there in the first place.

As Linus went on to say:

 And dammit, if anybody else feels that they had done "debugging
 messages with BUG_ON()", I would suggest you

  (a) rethink your approach to programming

  (b) send me patches to remove the crap entirely, or make them real
 *DEBUGGING* messages, not "kill the whole machine" messages.

 I've ranted against people using BUG_ON() for debugging in the past.
 Why the f*ck does this still happen? And Andrew - please stop taking
 those kinds of patches! Lookie here:

     https://lwn.net/Articles/13183/

 so excuse me for being upset that people still do this shit almost 15
 years later.

So I suggest people heed that advice and start fixing these stupid
BUG_ON()s that they've created.

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1582565

On Thu, Feb 16, 2017 at 12:40:27PM +0000, Russell King - ARM Linux wrote:
> However, the following is primerily directed at Laurent as the one who
> introduced the BUG_ON() in question...
> 
> NEVER EVER USE BUG_ON() IN A PATH THAT CAN RETURN AN ERROR.
> 
> It's possible to find Linus rants about this, eg,
> https://www.spinics.net/lists/stable/msg146439.html
> 
>  I should have reacted to the damn added BUG_ON() lines. I suspect I
>  will have to finally just remove the idiotic BUG_ON() concept once and
>  for all, because there is NO F*CKING EXCUSE to knowingly kill the
>  kernel.
> 
> Also: http://yarchive.net/comp/linux/BUG.html
> 
>  Rule of thumb: BUG() is only good for something that never happens and
>  that we really have no other option for (ie state is so corrupt that
>  continuing is deadly).
> 
> So, _unless_ people want to see BUG_ON() removed from the kernel, I
> strongly suggest to _STOP_ using it as "we didn't like the function
> arguments, let's use it as an assert() statement instead of returning
> an error."
> 
> There's no excuse what so ever to be killing the machine in
> media_create_pad_link().  If it doesn't like a NULL pointer, it's damn
> well got an error path to report that fact.  Use that mechanism and
> stop needlessly killing the kernel.
> 
> BUG_ON() IS NOT ASSERT().  DO NOT USE IT AS SUCH.
> 
> Linus is absolutely right about BUG_ON() - it hurts debuggability,
> because now the only way to do further tests is to reboot the damned
> machine after removing those fscking BUG_ON()s that should *never*
> have been there in the first place.
> 
> As Linus went on to say:
> 
>  And dammit, if anybody else feels that they had done "debugging
>  messages with BUG_ON()", I would suggest you
> 
>   (a) rethink your approach to programming
> 
>   (b) send me patches to remove the crap entirely, or make them real
>  *DEBUGGING* messages, not "kill the whole machine" messages.
> 
>  I've ranted against people using BUG_ON() for debugging in the past.
>  Why the f*ck does this still happen? And Andrew - please stop taking
>  those kinds of patches! Lookie here:
> 
>      https://lwn.net/Articles/13183/
> 
>  so excuse me for being upset that people still do this shit almost 15
>  years later.
> 
> So I suggest people heed that advice and start fixing these stupid
> BUG_ON()s that they've created.

More crap.

If the "complete" method fails (or, in fact, anything in
v4l2_async_test_notify() fails) then all hell breaks loose, because
of the total lack of clean up (and no, this isn't anything to do with
some stupid justification of those BUG_ON()s above.)

v4l2_async_notifier_register() gets called, it adds the notifier to
the global notifier list.  v4l2_async_test_notify() gets called.  It
returns an error, which is propagated out of
v4l2_async_notifier_register().

So the caller thinks that v4l2_async_notifier_register() failed, which
will cause imx_media_probe() to fail, causing imxmd->subdev_notifier
to be kfree()'d.  We now have a use-after free bug.

Second case.  v4l2_async_register_subdev().  Almost exactly the same,
except in this case adding sd->async_list to the notifier->done list
may have succeeded, and failure after that, again, results in an
in-use list_head being kfree()'d.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1582609

On Thu, Feb 16, 2017 at 01:09:35PM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 16, 2017 at 12:40:27PM +0000, Russell King - ARM Linux wrote:
> > However, the following is primerily directed at Laurent as the one who
> > introduced the BUG_ON() in question...
> > 
> > NEVER EVER USE BUG_ON() IN A PATH THAT CAN RETURN AN ERROR.
> > 
> > It's possible to find Linus rants about this, eg,
> > https://www.spinics.net/lists/stable/msg146439.html
> > 
> >  I should have reacted to the damn added BUG_ON() lines. I suspect I
> >  will have to finally just remove the idiotic BUG_ON() concept once and
> >  for all, because there is NO F*CKING EXCUSE to knowingly kill the
> >  kernel.
> > 
> > Also: http://yarchive.net/comp/linux/BUG.html
> > 
> >  Rule of thumb: BUG() is only good for something that never happens and
> >  that we really have no other option for (ie state is so corrupt that
> >  continuing is deadly).
> > 
> > So, _unless_ people want to see BUG_ON() removed from the kernel, I
> > strongly suggest to _STOP_ using it as "we didn't like the function
> > arguments, let's use it as an assert() statement instead of returning
> > an error."
> > 
> > There's no excuse what so ever to be killing the machine in
> > media_create_pad_link().  If it doesn't like a NULL pointer, it's damn
> > well got an error path to report that fact.  Use that mechanism and
> > stop needlessly killing the kernel.
> > 
> > BUG_ON() IS NOT ASSERT().  DO NOT USE IT AS SUCH.
> > 
> > Linus is absolutely right about BUG_ON() - it hurts debuggability,
> > because now the only way to do further tests is to reboot the damned
> > machine after removing those fscking BUG_ON()s that should *never*
> > have been there in the first place.
> > 
> > As Linus went on to say:
> > 
> >  And dammit, if anybody else feels that they had done "debugging
> >  messages with BUG_ON()", I would suggest you
> > 
> >   (a) rethink your approach to programming
> > 
> >   (b) send me patches to remove the crap entirely, or make them real
> >  *DEBUGGING* messages, not "kill the whole machine" messages.
> > 
> >  I've ranted against people using BUG_ON() for debugging in the past.
> >  Why the f*ck does this still happen? And Andrew - please stop taking
> >  those kinds of patches! Lookie here:
> > 
> >      https://lwn.net/Articles/13183/
> > 
> >  so excuse me for being upset that people still do this shit almost 15
> >  years later.
> > 
> > So I suggest people heed that advice and start fixing these stupid
> > BUG_ON()s that they've created.
> 
> More crap.
> 
> If the "complete" method fails (or, in fact, anything in
> v4l2_async_test_notify() fails) then all hell breaks loose, because
> of the total lack of clean up (and no, this isn't anything to do with
> some stupid justification of those BUG_ON()s above.)
> 
> v4l2_async_notifier_register() gets called, it adds the notifier to
> the global notifier list.  v4l2_async_test_notify() gets called.  It
> returns an error, which is propagated out of
> v4l2_async_notifier_register().
> 
> So the caller thinks that v4l2_async_notifier_register() failed, which
> will cause imx_media_probe() to fail, causing imxmd->subdev_notifier
> to be kfree()'d.  We now have a use-after free bug.
> 
> Second case.  v4l2_async_register_subdev().  Almost exactly the same,
> except in this case adding sd->async_list to the notifier->done list
> may have succeeded, and failure after that, again, results in an
> in-use list_head being kfree()'d.

And here's a patch which, combined with the fixes for ipuv3, results in
everything appearing to work properly.  Feel free to tear out the bits
for your area and turn them into proper patches.

 drivers/gpu/ipu-v3/ipu-common.c           |  6 ---
 drivers/media/media-entity.c              |  7 +--
 drivers/media/v4l2-core/v4l2-async.c      | 71 +++++++++++++++++++++++--------
 drivers/staging/media/imx/imx-media-csi.c |  1 +
 drivers/staging/media/imx/imx-media-dev.c |  2 +-
 5 files changed, 59 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
index 97218af4fe75..8368e6f766ee 100644
--- a/drivers/gpu/ipu-v3/ipu-common.c
+++ b/drivers/gpu/ipu-v3/ipu-common.c
@@ -1238,12 +1238,6 @@ static int ipu_add_client_devices(struct ipu_soc *ipu, unsigned long ipu_base)
 			platform_device_put(pdev);
 			goto err_register;
 		}
-
-		/*
-		 * Set of_node only after calling platform_device_add. Otherwise
-		 * the platform:imx-ipuv3-crtc modalias won't be used.
-		 */
-		pdev->dev.of_node = of_node;
 	}
 
 	return 0;
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index f9f723f5e4f0..154593a168df 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -625,9 +625,10 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
 	struct media_link *link;
 	struct media_link *backlink;
 
-	BUG_ON(source == NULL || sink == NULL);
-	BUG_ON(source_pad >= source->num_pads);
-	BUG_ON(sink_pad >= sink->num_pads);
+	if (WARN_ON(source == NULL || sink == NULL) ||
+	    WARN_ON(source_pad >= source->num_pads) ||
+	    WARN_ON(sink_pad >= sink->num_pads))
+		return -EINVAL;
 
 	link = media_add_link(&source->links);
 	if (link == NULL)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 5bada202b2d3..09934fb96a8d 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -94,7 +94,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
 }
 
 static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
-				  struct v4l2_subdev *sd,
+				  struct list_head *new, struct v4l2_subdev *sd,
 				  struct v4l2_async_subdev *asd)
 {
 	int ret;
@@ -107,22 +107,36 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
 	if (notifier->bound) {
 		ret = notifier->bound(notifier, sd, asd);
 		if (ret < 0)
-			return ret;
+			goto err_bind;
 	}
+
 	/* Move from the global subdevice list to notifier's done */
-	list_move(&sd->async_list, &notifier->done);
+	list_move(&sd->async_list, new);
 
 	ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
-	if (ret < 0) {
-		if (notifier->unbind)
-			notifier->unbind(notifier, sd, asd);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_register;
 
-	if (list_empty(&notifier->waiting) && notifier->complete)
-		return notifier->complete(notifier);
+	if (list_empty(&notifier->waiting) && notifier->complete) {
+		ret = notifier->complete(notifier);
+		if (ret < 0)
+			goto err_complete;
+	}
 
 	return 0;
+
+err_complete:
+	v4l2_device_unregister_subdev(sd);
+err_register:
+	if (notifier->unbind)
+		notifier->unbind(notifier, sd, asd);
+err_bind:
+	sd->notifier = NULL;
+	sd->asd = NULL;
+	list_add(&asd->list, &notifier->waiting);
+	/* always take this off the list on error */
+	list_del(&sd->async_list);
+	return ret;
 }
 
 static void v4l2_async_cleanup(struct v4l2_subdev *sd)
@@ -139,7 +153,8 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 {
 	struct v4l2_subdev *sd, *tmp;
 	struct v4l2_async_subdev *asd;
-	int i;
+	LIST_HEAD(new);
+	int ret, i;
 
 	if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
 		return -EINVAL;
@@ -172,22 +187,39 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 	list_add(&notifier->list, &notifier_list);
 
 	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
-		int ret;
-
 		asd = v4l2_async_belongs(notifier, sd);
 		if (!asd)
 			continue;
 
-		ret = v4l2_async_test_notify(notifier, sd, asd);
+		ret = v4l2_async_test_notify(notifier, &new, sd, asd);
 		if (ret < 0) {
-			mutex_unlock(&list_lock);
-			return ret;
+			/*
+			 * On failure, v4l2_async_test_notify() takes the
+			 * sd off the subdev list.  Add it back.
+			 */
+			list_add(&sd->async_list, &subdev_list);
+			goto err_notify;
 		}
 	}
 
+	list_splice(&new, &notifier->done);
+
 	mutex_unlock(&list_lock);
 
 	return 0;
+
+err_notify:
+	list_del(&notifier->list);
+	list_for_each_entry_safe(sd, tmp, &new, async_list) {
+		v4l2_device_unregister_subdev(sd);
+		list_move(&sd->async_list, &subdev_list);
+		if (notifier->unbind)
+			notifier->unbind(notifier, sd, sd->asd);
+		sd->notifier = NULL;
+		sd->asd = NULL;
+	}
+	mutex_unlock(&list_lock);
+	return ret;
 }
 EXPORT_SYMBOL(v4l2_async_notifier_register);
 
@@ -213,6 +245,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	list_del(&notifier->list);
 
 	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
+		struct v4l2_async_subdev *asd = sd->asd;
 		struct device *d;
 
 		d = get_device(sd->dev);
@@ -223,7 +256,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 		device_release_driver(d);
 
 		if (notifier->unbind)
-			notifier->unbind(notifier, sd, sd->asd);
+			notifier->unbind(notifier, sd, asd);
 
 		/*
 		 * Store device at the device cache, in order to call
@@ -288,7 +321,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 	list_for_each_entry(notifier, &notifier_list, list) {
 		struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd);
 		if (asd) {
-			int ret = v4l2_async_test_notify(notifier, sd, asd);
+			int ret = v4l2_async_test_notify(notifier,
+							 &notifier->done,
+							 sd, asd);
 			mutex_unlock(&list_lock);
 			return ret;
 		}
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 9d9ec03436e4..507026feee91 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1427,6 +1427,7 @@ static int imx_csi_probe(struct platform_device *pdev)
 	priv->sd.entity.ops = &csi_entity_ops;
 	priv->sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
 	priv->sd.dev = &pdev->dev;
+	priv->sd.of_node = pdata->of_node;
 	priv->sd.owner = THIS_MODULE;
 	priv->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	priv->sd.grp_id = priv->csi_id ?
diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index 60f45fe4b506..5b4dfc1fb6ab 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -197,7 +197,7 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
 	struct imx_media_subdev *imxsd;
 	int ret = -EINVAL;
 
-	imxsd = imx_media_find_async_subdev(imxmd, sd->dev->of_node,
+	imxsd = imx_media_find_async_subdev(imxmd, sd->of_node,
 					    dev_name(sd->dev));
 	if (!imxsd)
 		goto out;


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1582873

On 02/16/2017 06:20 AM, Russell King - ARM Linux wrote:
> On Thu, Feb 16, 2017 at 01:09:35PM +0000, Russell King - ARM Linux wrote:
>>
>> <snip>
>> More crap.
>>
>> If the "complete" method fails (or, in fact, anything in
>> v4l2_async_test_notify() fails) then all hell breaks loose, because
>> of the total lack of clean up (and no, this isn't anything to do with
>> some stupid justification of those BUG_ON()s above.)
>>
>> v4l2_async_notifier_register() gets called, it adds the notifier to
>> the global notifier list.  v4l2_async_test_notify() gets called.  It
>> returns an error, which is propagated out of
>> v4l2_async_notifier_register().
>>
>> So the caller thinks that v4l2_async_notifier_register() failed, which
>> will cause imx_media_probe() to fail, causing imxmd->subdev_notifier
>> to be kfree()'d.  We now have a use-after free bug.
>>
>> Second case.  v4l2_async_register_subdev().  Almost exactly the same,
>> except in this case adding sd->async_list to the notifier->done list
>> may have succeeded, and failure after that, again, results in an
>> in-use list_head being kfree()'d.
>
> And here's a patch which, combined with the fixes for ipuv3, results in
> everything appearing to work properly.  Feel free to tear out the bits
> for your area and turn them into proper patches.
>
>  drivers/gpu/ipu-v3/ipu-common.c           |  6 ---
>  drivers/media/media-entity.c              |  7 +--
>  drivers/media/v4l2-core/v4l2-async.c      | 71 +++++++++++++++++++++++--------
>  drivers/staging/media/imx/imx-media-csi.c |  1 +
>  drivers/staging/media/imx/imx-media-dev.c |  2 +-
>  5 files changed, 59 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
> index 97218af4fe75..8368e6f766ee 100644
> --- a/drivers/gpu/ipu-v3/ipu-common.c
> +++ b/drivers/gpu/ipu-v3/ipu-common.c
> @@ -1238,12 +1238,6 @@ static int ipu_add_client_devices(struct ipu_soc *ipu, unsigned long ipu_base)
>  			platform_device_put(pdev);
>  			goto err_register;
>  		}
> -
> -		/*
> -		 * Set of_node only after calling platform_device_add. Otherwise
> -		 * the platform:imx-ipuv3-crtc modalias won't be used.
> -		 */
> -		pdev->dev.of_node = of_node;
>  	}


Ah, never mind my question earlier, I see now why the CSI's were likely
not recognized, probably because of this. Anyway I agree with this 
change and I made the accompanying requisite change to imx-media-csi.c
and imx-media-dev.c below.

Steve




>
>  	return 0;
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index f9f723f5e4f0..154593a168df 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -625,9 +625,10 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
>  	struct media_link *link;
>  	struct media_link *backlink;
>
> -	BUG_ON(source == NULL || sink == NULL);
> -	BUG_ON(source_pad >= source->num_pads);
> -	BUG_ON(sink_pad >= sink->num_pads);
> +	if (WARN_ON(source == NULL || sink == NULL) ||
> +	    WARN_ON(source_pad >= source->num_pads) ||
> +	    WARN_ON(sink_pad >= sink->num_pads))
> +		return -EINVAL;
>
>  	link = media_add_link(&source->links);
>  	if (link == NULL)
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 5bada202b2d3..09934fb96a8d 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -94,7 +94,7 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
>  }
>
>  static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> -				  struct v4l2_subdev *sd,
> +				  struct list_head *new, struct v4l2_subdev *sd,
>  				  struct v4l2_async_subdev *asd)
>  {
>  	int ret;
> @@ -107,22 +107,36 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>  	if (notifier->bound) {
>  		ret = notifier->bound(notifier, sd, asd);
>  		if (ret < 0)
> -			return ret;
> +			goto err_bind;
>  	}
> +
>  	/* Move from the global subdevice list to notifier's done */
> -	list_move(&sd->async_list, &notifier->done);
> +	list_move(&sd->async_list, new);
>
>  	ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> -	if (ret < 0) {
> -		if (notifier->unbind)
> -			notifier->unbind(notifier, sd, asd);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto err_register;
>
> -	if (list_empty(&notifier->waiting) && notifier->complete)
> -		return notifier->complete(notifier);
> +	if (list_empty(&notifier->waiting) && notifier->complete) {
> +		ret = notifier->complete(notifier);
> +		if (ret < 0)
> +			goto err_complete;
> +	}
>
>  	return 0;
> +
> +err_complete:
> +	v4l2_device_unregister_subdev(sd);
> +err_register:
> +	if (notifier->unbind)
> +		notifier->unbind(notifier, sd, asd);
> +err_bind:
> +	sd->notifier = NULL;
> +	sd->asd = NULL;
> +	list_add(&asd->list, &notifier->waiting);
> +	/* always take this off the list on error */
> +	list_del(&sd->async_list);
> +	return ret;
>  }
>
>  static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> @@ -139,7 +153,8 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>  {
>  	struct v4l2_subdev *sd, *tmp;
>  	struct v4l2_async_subdev *asd;
> -	int i;
> +	LIST_HEAD(new);
> +	int ret, i;
>
>  	if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>  		return -EINVAL;
> @@ -172,22 +187,39 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>  	list_add(&notifier->list, &notifier_list);
>
>  	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> -		int ret;
> -
>  		asd = v4l2_async_belongs(notifier, sd);
>  		if (!asd)
>  			continue;
>
> -		ret = v4l2_async_test_notify(notifier, sd, asd);
> +		ret = v4l2_async_test_notify(notifier, &new, sd, asd);
>  		if (ret < 0) {
> -			mutex_unlock(&list_lock);
> -			return ret;
> +			/*
> +			 * On failure, v4l2_async_test_notify() takes the
> +			 * sd off the subdev list.  Add it back.
> +			 */
> +			list_add(&sd->async_list, &subdev_list);
> +			goto err_notify;
>  		}
>  	}
>
> +	list_splice(&new, &notifier->done);
> +
>  	mutex_unlock(&list_lock);
>
>  	return 0;
> +
> +err_notify:
> +	list_del(&notifier->list);
> +	list_for_each_entry_safe(sd, tmp, &new, async_list) {
> +		v4l2_device_unregister_subdev(sd);
> +		list_move(&sd->async_list, &subdev_list);
> +		if (notifier->unbind)
> +			notifier->unbind(notifier, sd, sd->asd);
> +		sd->notifier = NULL;
> +		sd->asd = NULL;
> +	}
> +	mutex_unlock(&list_lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL(v4l2_async_notifier_register);
>
> @@ -213,6 +245,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  	list_del(&notifier->list);
>
>  	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
> +		struct v4l2_async_subdev *asd = sd->asd;
>  		struct device *d;
>
>  		d = get_device(sd->dev);
> @@ -223,7 +256,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  		device_release_driver(d);
>
>  		if (notifier->unbind)
> -			notifier->unbind(notifier, sd, sd->asd);
> +			notifier->unbind(notifier, sd, asd);
>
>  		/*
>  		 * Store device at the device cache, in order to call
> @@ -288,7 +321,9 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  	list_for_each_entry(notifier, &notifier_list, list) {
>  		struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd);
>  		if (asd) {
> -			int ret = v4l2_async_test_notify(notifier, sd, asd);
> +			int ret = v4l2_async_test_notify(notifier,
> +							 &notifier->done,
> +							 sd, asd);
>  			mutex_unlock(&list_lock);
>  			return ret;
>  		}
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 9d9ec03436e4..507026feee91 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1427,6 +1427,7 @@ static int imx_csi_probe(struct platform_device *pdev)
>  	priv->sd.entity.ops = &csi_entity_ops;
>  	priv->sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
>  	priv->sd.dev = &pdev->dev;
> +	priv->sd.of_node = pdata->of_node;
>  	priv->sd.owner = THIS_MODULE;
>  	priv->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>  	priv->sd.grp_id = priv->csi_id ?
> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> index 60f45fe4b506..5b4dfc1fb6ab 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -197,7 +197,7 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
>  	struct imx_media_subdev *imxsd;
>  	int ret = -EINVAL;
>
> -	imxsd = imx_media_find_async_subdev(imxmd, sd->dev->of_node,
> +	imxsd = imx_media_find_async_subdev(imxmd, sd->of_node,
>  					    dev_name(sd->dev));
>  	if (!imxsd)
>  		goto out;
>
>

[toc] | [prev] | [next] | [standalone]

#1582859

On 02/16/2017 04:40 AM, Russell King - ARM Linux wrote:
> On Thu, Feb 16, 2017 at 11:52:06AM +0000, Russell King - ARM Linux wrote:
>> On Wed, Feb 15, 2017 at 06:19:22PM -0800, Steve Longerbeam wrote:
>>> +static const struct platform_device_id imx_csi_ids[] = {
>>> +	{ .name = "imx-ipuv3-csi" },
>>> +	{ },
>>> +};
>>> +MODULE_DEVICE_TABLE(platform, imx_csi_ids);
>>> +
>>> +static struct platform_driver imx_csi_driver = {
>>> +	.probe = imx_csi_probe,
>>> +	.remove = imx_csi_remove,
>>> +	.id_table = imx_csi_ids,
>>> +	.driver = {
>>> +		.name = "imx-ipuv3-csi",
>>> +	},
>>> +};
>>> +module_platform_driver(imx_csi_driver);
>>> +
>>> +MODULE_DESCRIPTION("i.MX CSI subdev driver");
>>> +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@mentor.com>");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_ALIAS("platform:imx-ipuv3-csi");
>>
>> Just a reminder that automatic module loading of this is completely
>> broken right now (not your problem) due to this stupid idea in the
>> IPUv3 code:
>>
>> 		if (!ret)
>> 			ret = platform_device_add(pdev);
>> 		if (ret) {
>> 			platform_device_put(pdev);
>> 			goto err_register;
>> 		}
>>
>> 		/*
>> 		 * Set of_node only after calling platform_device_add. Otherwise
>> 		 * the platform:imx-ipuv3-crtc modalias won't be used.
>> 		 */
>> 		pdev->dev.of_node = of_node;
>>
>> setting pdev->dev.of_node changes the modalias exported to userspace,
>> so udev sees a DT based modalias, which causes it to totally miss any
>> driver using a non-DT based modalias.
>>
>> The IPUv3 code needs fixing, not only for imx-media-csi, but also for
>> imx-ipuv3-crtc too, because that module will also suffer the same
>> issue.
>>
>> The only solution is... don't fsck with dev->of_node assignment.  In
>> this case, it's probably much better to pass it in via platform data.
>> If you then absolutely must have dev->of_node, doing it in the driver
>> means that you avoid the modalias mess before the appropriate driver
>> is loaded.  However, that's still not a nice solution because the
>> modalias file still ends up randomly changing its contents.
>>
>> As I say, not _your_ problem, but it's still a problem that needs
>> solving, and I don't want it forgotten about.
>
> I've just hacked up a solution to this, and unfortunately it reveals a
> problem with Steve's code.  Picking out the imx & media-related messages:
>
> [    8.012191] imx_media_common: module is from the staging directory, the quality is unknown, you have been warned.
> [    8.018175] imx_media: module is from the staging directory, the quality is unknown, you have been warned.
> [    8.748345] imx-media: Registered subdev ipu1_csi0_mux
> [    8.753451] imx-media: Registered subdev ipu2_csi1_mux
> [    9.055196] imx219 0-0010: detected IMX219 sensor
> [    9.090733] imx6_mipi_csi2: module is from the staging directory, the quality is unknown, you have been warned.
> [    9.092247] imx-media: Registered subdev imx219 0-0010
> [    9.334338] imx-media: Registered subdev imx6-mipi-csi2
> [    9.372452] imx_media_capture: module is from the staging directory, the quality is unknown, you have been warned.
> [    9.378163] imx_media_capture: module is from the staging directory, the quality is unknown, you have been warned.
> [    9.390033] imx_media_csi: module is from the staging directory, the quality is unknown, you have been warned.
> [    9.394362] imx-media: Received unknown subdev ipu1_csi0

The root problem is here. I don't know why the CSI entities are not
being recognized. Can you share the changes you made?

So imx_media_subdev_bound() returns error because it didn't recognize
the subdev that was bound.

And for some reason, even though some of the subdev bound ops return
error, v4l2-core still calls the async completion notifier
(imx_media_probe_complete()).

I'll add some checks to imx_media_probe_complete() to try and detect
when not all subdevs were bound correctly to get around this issue.
That should prevent the kernel BUG() below.

Steve


> [    9.394699] imx-ipuv3-csi: probe of imx-ipuv3-csi.0 failed with error -22
> [    9.394840] imx-media: Received unknown subdev ipu1_csi1
> [    9.394887] imx-ipuv3-csi: probe of imx-ipuv3-csi.1 failed with error -22
> [    9.394992] imx-media: Received unknown subdev ipu2_csi0
> [    9.395026] imx-ipuv3-csi: probe of imx-ipuv3-csi.4 failed with error -22
> [    9.395119] imx-media: Received unknown subdev ipu2_csi1
> [    9.395159] imx-ipuv3-csi: probe of imx-ipuv3-csi.5 failed with error -22
> [    9.411722] imx_media_vdic: module is from the staging directory, the quality is unknown, you have been warned.
> [    9.412820] imx-media: Registered subdev ipu1_vdic
> [    9.424687] imx-media: Registered subdev ipu2_vdic
> [    9.436074] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
> [    9.437455] imx-media: Registered subdev ipu1_ic_prp
> [    9.437788] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
> [    9.447542] imx-media: Registered subdev ipu1_ic_prpenc
> [    9.455225] ipu1_ic_prpenc: Registered ipu1_ic_prpenc capture as /dev/video3
> [    9.459203] imx-media: Registered subdev ipu1_ic_prpvf
> [    9.460484] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
> [    9.460726] ipu1_ic_prpvf: Registered ipu1_ic_prpvf capture as /dev/video4
> [    9.460983] imx-media: Registered subdev ipu2_ic_prp
> [    9.461161] imx-media: Registered subdev ipu2_ic_prpenc
> [    9.461737] ipu2_ic_prpenc: Registered ipu2_ic_prpenc capture as /dev/video5
> [    9.463767] imx-media: Registered subdev ipu2_ic_prpvf
> [    9.464294] ipu2_ic_prpvf: Registered ipu2_ic_prpvf capture as /dev/video6
> [    9.464345] imx-media: imx_media_create_link: (null):1 -> ipu1_ic_prp:0
> [    9.464413] ------------[ cut here ]------------
> [    9.469134] kernel BUG at /home/rmk/git/linux-rmk/drivers/media/media-entity.c:628!
> [    9.476924] Internal error: Oops - BUG: 0 [#1] SMP ARM
> [    9.482246] Modules linked in: imx_media_ic(C+) imx_media_vdic(C) imx_media_csi(C) imx_media_capture(C) uvcvideo imx6_mipi_csi2(C) snd_soc_imx_audmux imx219 snd_soc_sgtl5000 video_multiplexer caam imx_sdma imx2_wdt snd_soc_fsl_ssi snd_soc_fsl_spdif imx_pcm_dma coda imx_thermal v4l2_mem2mem videobuf2_v4l2 videobuf2_dma_contig videobuf2_core videobuf2_vmalloc videobuf2_memops imx_media(C) imx_media_common(C) rc_pinnacle_pctv_hd nfsd dw_hdmi_cec dw_hdmi_ahb_audio etnaviv
> [    9.524500] CPU: 1 PID: 263 Comm: systemd-udevd Tainted: G         C      4.10.0-rc7+ #2112
> [    9.532995] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [    9.539619] task: edef1880 task.stack: d03ca000
> [    9.544313] PC is at media_create_pad_link+0x134/0x140
> [    9.549541] LR is at imx_media_probe_complete+0x164/0x24c [imx_media]
> [    9.556080] pc : [<c04f0eb0>]    lr : [<bf052524>]    psr: 60070013
>                sp : d03cbbc8  ip : d03cbbf8  fp : d03cbbf4
> [    9.567712] r10: 00000001  r9 : 00000000  r8 : d0170d14
> [    9.573007] r7 : 00000000  r6 : 00000001  r5 : 00000000  r4 : d0170d14
> [    9.579612] r3 : 00000000  r2 : d0170d14  r1 : 00000001  r0 : 00000000
> [    9.586256] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none[    9.593486] Control: 10c5387d  Table: 3e77c04a  DAC: 00000051
> [    9.599317] Process systemd-udevd (pid: 263, stack limit = 0xd03ca210)
> [    9.605950] Stack: (0xd03cbbc8 to 0xd03cc000)
> [    9.610368] bbc0:                   00000000 00000000 ee980410 00000000 00000000 d0170d14
> [    9.618658] bbe0: 00000000 00000001 d03cbc54 d03cbbf8 bf052524 c04f0d88 00000000 d0170d88
> [    9.626961] bc00: 00000000 c0a57dc4 0004a364 ee98011c 00000000 00000003 00000001 ee980230
> [    9.635267] bc20: ee980274 ee980010 d03cbc54 d0170f14 ee9ca4cc ee9974c4 bf0523c0 c0a57dc4
> [    9.643539] bc40: f184bb30 00000026 d03cbc74 d03cbc58 c0502f50 bf0523cc ee9ca4cc d0170f14
> [    9.651824] bc60: c0a57e08 d0170fc0 d03cbc9c d03cbc78 c0502fdc c0502e70 00000000 d0170f10
> [    9.660132] bc80: 00000000 d02c0c10 bf122cd0 d0170f14 d03cbcc4 d03cbca0 bf121154 c0502f68
> [    9.668423] bca0: bf12104c ffffffed d02c0c10 fffffdfb bf123248 00000000 d03cbce4 d03cbcc8
> [    9.676713] bcc0: c041aeb4 bf121058 d02c0c10 c1419d70 00000000 bf123248 d03cbd0c d03cbce8
> [    9.684992] bce0: c0418ec4 c041ae68 d02c0c10 bf123248 d02c0c44 00000000 00000001 00000124
> [    9.693282] bd00: d03cbd2c d03cbd10 c0419044 c0418ccc 00000000 00000000 bf123248 c0418f88
> [    9.701618] bd20: d03cbd54 d03cbd30 c04172e4 c0418f94 ef0f64a4 d01b8cd0 d03d9858 bf123248
> [    9.709900] bd40: d03d9c00 c0a45e10 d03cbd64 d03cbd58 c0418728 c0417294 d03cbd8c d03cbd68
> [    9.718203] bd60: c0418428 c0418710 bf122e48 d03cbd78 bf123248 c0a704a8 bf126000 00000000
> [    9.729180] bd80: d03cbda4 d03cbd90 c0419ec4 c0418340 bf123480 c0a704a8 d03cbdb4 d03cbda8
> [    9.739950] bda0: c041ad88 c0419e50 d03cbdc4 d03cbdb8 bf126018 c041ad4c d03cbe34 d03cbdc8
> [    9.751089] bdc0: c00098ac bf12600c d03cbdec d03cbdd8 c00a8888 c0087240 00000000 ed4a9440
> [    9.761941] bde0: d03cbe34 d03cbdf0 c016c690 c00a8814 c016b554 c016aa60 00000001 c015f3f8
> [    9.772940] be00: 00000005 0000000c edef1880 bf123480 c0a704a8 bf123480 c0a704a8 ed4a9440
> [    9.784008] be20: bf123480 00000001 d03cbe5c d03cbe38 c011b1e4 c0009874 d03cbe5c d03cbe48
> [    9.795017] be40: c09f5ea7 c0a704a8 c09e04ec bf123480 d03cbf14 d03cbe60 c00d2dd0 c011b188
> [    9.806069] be60: bf12348c 00007fff bf123480 c00d09f0 f1847000 bf12792c f18495c0 bf123680
> [    9.817194] be80: bf12348c bf1236f0 00000000 bf1234c8 c017c1d0 c017bfac f1847000 00004f68
> [    9.828347] bea0: c017c2e8 00000000 edef1880 00000000 00000000 00000000 00000000 00000000
> [    9.839542] bec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    9.850754] bee0: 00000000 00000000 00000003 7fffffff 00000000 00000000 00000007 b6c9e63c
> [    9.861991] bf00: d03ca000 00000000 d03cbfa4 d03cbf18 c00d36cc c00d1480 7fffffff 00000000
> [    9.873266] bf20: 00000003 ee0384d4 d03cbf74 f1847000 00004f68 00000000 00000002 f1847000
> [    9.884593] bf40: 00004f68 f184bb30 f184989b f184a7d8 000026f0 00002dd0 00000000 00000000
> [    9.895998] bf60: 00000000 0000192c 00000019 0000001a 00000011 00000000 0000000a 00000000
> [    9.907408] bf80: c008b848 80c36630 00000000 2529fc00 0000017b c000ff04 00000000 d03cbfa8
> [    9.918858] bfa0: c000fd60 c00d3644 80c36630 00000000 00000007 b6c9e63c 00000000 80c38178
> [    9.930354] bfc0: 80c36630 00000000 2529fc00 0000017b 00020000 7f96eb0c 80c37848 00000000
> [    9.941900] bfe0: bed55928 bed55918 b6c988ff b6bea572 600f0030 00000007 3fffd861 3fffdc61
> [    9.953532] Backtrace:
> [    9.959442] [<c04f0d7c>] (media_create_pad_link) from [<bf052524>] (imx_media_probe_complete+0x164/0x24c [imx_media])
> [    9.973644]  r10:00000001 r9:00000000 r8:d0170d14 r7:00000000 r6:00000000 r5:ee980410
> [    9.985112]  r4:00000000 r3:00000000
> [    9.992696] [<bf0523c0>] (imx_media_probe_complete [imx_media]) from [<c0502f50>] (v4l2_async_test_notify+0xec/0xf8)
> [   10.007413]  r10:00000026 r9:f184bb30 r8:c0a57dc4 r7:bf0523c0 r6:ee9974c4 r5:ee9ca4cc
> [   10.019212]  r4:d0170f14
> [   10.025650] [<c0502e64>] (v4l2_async_test_notify) from [<c0502fdc>] (v4l2_async_register_subdev+0x80/0xdc)
> [   10.039736]  r7:d0170fc0 r6:c0a57e08 r5:d0170f14 r4:ee9ca4cc
> [   10.049613] [<c0502f5c>] (v4l2_async_register_subdev) from [<bf121154>] (imx_ic_probe+0x108/0x144 [imx_media_ic])
> [   10.063953]  r8:d0170f14 r7:bf122cd0 r6:d02c0c10 r5:00000000 r4:d0170f10 r3:00000000
> [   10.075786] [<bf12104c>] (imx_ic_probe [imx_media_ic]) from [<c041aeb4>] (platform_drv_probe+0x58/0xb8)
> [   10.089118]  r8:00000000 r7:bf123248 r6:fffffdfb r5:d02c0c10 r4:ffffffed r3:bf12104c
> [   10.100683] [<c041ae5c>] (platform_drv_probe) from [<c0418ec4>] (driver_probe_device+0x204/0x2c8)
> [   10.113279]  r7:bf123248 r6:00000000 r5:c1419d70 r4:d02c0c10
> [   10.122765] [<c0418cc0>] (driver_probe_device) from [<c0419044>] (__driver_attach+0xbc/0xc0)
> [   10.135098]  r10:00000124 r8:00000001 r7:00000000 r6:d02c0c44 r5:bf123248 r4:d02c0c10
> [   10.146785] [<c0418f88>] (__driver_attach) from [<c04172e4>] (bus_for_each_dev+0x5c/0x90)
> [   10.158811]  r6:c0418f88 r5:bf123248 r4:00000000 r3:00000000
> [   10.168375] [<c0417288>] (bus_for_each_dev) from [<c0418728>] (driver_attach+0x24/0x28)
> [   10.180413]  r6:c0a45e10 r5:d03d9c00 r4:bf123248
> [   10.188959] [<c0418704>] (driver_attach) from [<c0418428>] (bus_add_driver+0xf4/0x200)
> [   10.200775] [<c0418334>] (bus_add_driver) from [<c0419ec4>] (driver_register+0x80/0xfc)
> [   10.212707]  r7:00000000 r6:bf126000 r5:c0a704a8 r4:bf123248
> [   10.222254] [<c0419e44>] (driver_register) from [<c041ad88>] (__platform_driver_register+0x48/0x4c)
> [   10.235212]  r5:c0a704a8 r4:bf123480
> [   10.242694] [<c041ad40>] (__platform_driver_register) from [<bf126018>] (imx_ic_driver_init+0x18/0x24 [imx_media_ic])
> [   10.257308] [<bf126000>] (imx_ic_driver_init [imx_media_ic]) from [<c00098ac>] (do_one_initcall+0x44/0x170)
> [   10.271043] [<c0009868>] (do_one_initcall) from [<c011b1e4>] (do_init_module+0x68/0x1d8)
> [   10.283139]  r8:00000001 r7:bf123480 r6:ed4a9440 r5:c0a704a8 r4:bf123480
> [   10.293849] [<c011b17c>] (do_init_module) from [<c00d2dd0>] (load_module+0x195c/0x2080)
> [   10.305867]  r7:bf123480 r6:c09e04ec r5:c0a704a8 r4:c09f5ea7
> [   10.315523] [<c00d1474>] (load_module) from [<c00d36cc>] (SyS_finit_module+0x94/0xa0)
> [   10.327382]  r10:00000000 r9:d03ca000 r8:b6c9e63c r7:00000007 r6:00000000 r5:00000000
> [   10.339238]  r4:7fffffff
> [   10.345766] [<c00d3638>] (SyS_finit_module) from [<c000fd60>] (ret_fast_syscall+0x0/0x1c)
> [   10.357994]  r8:c000ff04 r7:0000017b r6:2529fc00 r5:00000000 r4:80c36630
> [   10.368747] Code: e1a01007 ebfffce1 e3e0000b e89daff8 (e7f001f2)
> [   10.378883] ---[ end trace 2051fac455b36c5a ]---
> [   11.228961] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
> [   11.247536] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
> [   11.301366] imx_media_ic: module is from the staging directory, the quality is unknown, you have been warned.
>
> So there's probably some sort of race going on.
>
> However, the following is primerily directed at Laurent as the one who
> introduced the BUG_ON() in question...
>
> NEVER EVER USE BUG_ON() IN A PATH THAT CAN RETURN AN ERROR.
>
> It's possible to find Linus rants about this, eg,
> https://www.spinics.net/lists/stable/msg146439.html
>
>  I should have reacted to the damn added BUG_ON() lines. I suspect I
>  will have to finally just remove the idiotic BUG_ON() concept once and
>  for all, because there is NO F*CKING EXCUSE to knowingly kill the
>  kernel.
>
> Also: http://yarchive.net/comp/linux/BUG.html
>
>  Rule of thumb: BUG() is only good for something that never happens and
>  that we really have no other option for (ie state is so corrupt that
>  continuing is deadly).
>
> So, _unless_ people want to see BUG_ON() removed from the kernel, I
> strongly suggest to _STOP_ using it as "we didn't like the function
> arguments, let's use it as an assert() statement instead of returning
> an error."
>
> There's no excuse what so ever to be killing the machine in
> media_create_pad_link().  If it doesn't like a NULL pointer, it's damn
> well got an error path to report that fact.  Use that mechanism and
> stop needlessly killing the kernel.
>
> BUG_ON() IS NOT ASSERT().  DO NOT USE IT AS SUCH.
>
> Linus is absolutely right about BUG_ON() - it hurts debuggability,
> because now the only way to do further tests is to reboot the damned
> machine after removing those fscking BUG_ON()s that should *never*
> have been there in the first place.
>
> As Linus went on to say:
>
>  And dammit, if anybody else feels that they had done "debugging
>  messages with BUG_ON()", I would suggest you
>
>   (a) rethink your approach to programming
>
>   (b) send me patches to remove the crap entirely, or make them real
>  *DEBUGGING* messages, not "kill the whole machine" messages.
>
>  I've ranted against people using BUG_ON() for debugging in the past.
>  Why the f*ck does this still happen? And Andrew - please stop taking
>  those kinds of patches! Lookie here:
>
>      https://lwn.net/Articles/13183/
>
>  so excuse me for being upset that people still do this shit almost 15
>  years later.
>
> So I suggest people heed that advice and start fixing these stupid
> BUG_ON()s that they've created.
>
> Thanks.
>

[toc] | [prev] | [next] | [standalone]

#1582875

On Thu, Feb 16, 2017 at 10:44:16AM -0800, Steve Longerbeam wrote:
> On 02/16/2017 04:40 AM, Russell King - ARM Linux wrote:
> >[    8.012191] imx_media_common: module is from the staging directory, the quality is unknown, you have been warned.
> >[    8.018175] imx_media: module is from the staging directory, the quality is unknown, you have been warned.
> >[    8.748345] imx-media: Registered subdev ipu1_csi0_mux
> >[    8.753451] imx-media: Registered subdev ipu2_csi1_mux
> >[    9.055196] imx219 0-0010: detected IMX219 sensor
> >[    9.090733] imx6_mipi_csi2: module is from the staging directory, the quality is unknown, you have been warned.
> >[    9.092247] imx-media: Registered subdev imx219 0-0010
> >[    9.334338] imx-media: Registered subdev imx6-mipi-csi2
> >[    9.372452] imx_media_capture: module is from the staging directory, the quality is unknown, you have been warned.
> >[    9.378163] imx_media_capture: module is from the staging directory, the quality is unknown, you have been warned.
> >[    9.390033] imx_media_csi: module is from the staging directory, the quality is unknown, you have been warned.
> >[    9.394362] imx-media: Received unknown subdev ipu1_csi0
> 
> The root problem is here. I don't know why the CSI entities are not
> being recognized. Can you share the changes you made?

No, it's not the root problem that's causing the BUG/etc, but it is
_a_ problem.  Nevertheless, it's something I fixed - disconnecting
the of_node from the struct device needed one other change in the
imx-media code that was missing at this time.

However, that's no excuse what so ever for the BUG_ON() and lack of
error cleanup (causing use-after-free, which is just another way of
saying "data corruption waiting to happen") that I identified.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web