| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4ukas366gvkl7fljddneulgxwnzdvblkknantgv4iu427sfhor@d3rpvym4ynhn>
Date: Mon, 24 Feb 2025 12:44:52 +0530
From: Jai Luthra <jai.luthra@...ux.dev>
To: Yemike Abhilash Chandra <y-abhilashchandra@...com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, mripard@...nel.org, mchehab@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, devarsht@...com, vaishnav.a@...com,
r-donadkar@...com, u-kumar1@...com
Subject: Re: [PATCH v3 2/2] media: cadence: csi2rx: Enable csi2rx_err_irq
interrupt and add support for VIDIOC_LOG_STATUS
Hi Abhilash,
Thanks for the patch.
On Fri, Feb 21, 2025 at 05:33:37PM +0530, Yemike Abhilash Chandra wrote:
> Enable the csi2rx_err_irq interrupt to record any errors during streaming
> and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
> ioctl can be invoked from user space to retrieve the device status,
> including details about any errors.
>
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@...com>
> ---
>
> Changes in v3:
> - Address Jai's review comment to enable FIFO overflow bits in the mask
> only for the source pads that have an active remote.
> - Drop TI-specific interrupt and have support for only two interrupts
> that are common across all vendors.
> - Address Changhuang's review to use pdev directly to get the interrupt.
> - Set the interrupt mask register only if the interrupt is defined in the DT.
>
>
> drivers/media/platform/cadence/cdns-csi2rx.c | 125 +++++++++++++++++++
> 1 file changed, 125 insertions(+)
>
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index cebcae196eec..81375f11a32f 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -57,6 +57,25 @@
> #define CSI2RX_LANES_MAX 4
> #define CSI2RX_STREAMS_MAX 4
>
> +#define CSI2RX_ERROR_IRQS_REG 0x28
> +#define CSI2RX_ERROR_IRQS_MASK_REG 0x2C
> +
> +#define CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ BIT(19)
> +#define CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ BIT(18)
> +#define CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ BIT(17)
> +#define CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ BIT(16)
> +#define CSI2RX_FRONT_TRUNC_HDR_IRQ BIT(12)
> +#define CSI2RX_PROT_TRUNCATED_PACKET_IRQ BIT(11)
> +#define CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ BIT(10)
> +#define CSI2RX_SP_INVALID_RCVD_IRQ BIT(9)
> +#define CSI2RX_DATA_ID_IRQ BIT(7)
> +#define CSI2RX_HEADER_CORRECTED_ECC_IRQ BIT(6)
> +#define CSI2RX_HEADER_ECC_IRQ BIT(5)
> +#define CSI2RX_PAYLOAD_CRC_IRQ BIT(4)
> +
> +#define CSI2RX_ECC_ERRORS GENMASK(7, 4)
> +#define CSI2RX_PACKET_ERRORS GENMASK(12, 9)
> +
> enum csi2rx_pads {
> CSI2RX_PAD_SINK,
> CSI2RX_PAD_SOURCE_STREAM0,
> @@ -71,9 +90,32 @@ struct csi2rx_fmt {
> u8 bpp;
> };
>
> +struct csi2rx_event {
> + u32 mask;
> + const char *name;
> +};
> +
> +static const struct csi2rx_event csi2rx_events[] = {
> + { CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 3 FIFO detected" },
> + { CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 2 FIFO detected" },
> + { CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 1 FIFO detected" },
> + { CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 0 FIFO detected" },
> + { CSI2RX_FRONT_TRUNC_HDR_IRQ, "A truncated header [short or long] has been received" },
> + { CSI2RX_PROT_TRUNCATED_PACKET_IRQ, "A truncated long packet has been received" },
> + { CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ, "A truncated long packet has been received. No payload" },
> + { CSI2RX_SP_INVALID_RCVD_IRQ, "A reserved or invalid short packet has been received" },
> + { CSI2RX_DATA_ID_IRQ, "Data ID error in the header packet" },
> + { CSI2RX_HEADER_CORRECTED_ECC_IRQ, "ECC error detected and corrected" },
> + { CSI2RX_HEADER_ECC_IRQ, "Unrecoverable ECC error" },
> + { CSI2RX_PAYLOAD_CRC_IRQ, "CRC error" },
> +};
> +
> +#define CSI2RX_NUM_EVENTS ARRAY_SIZE(csi2rx_events)
> +
> struct csi2rx_priv {
> struct device *dev;
> unsigned int count;
> + int error_irq;
>
> /*
> * Used to prevent race conditions between multiple,
> @@ -95,6 +137,7 @@ struct csi2rx_priv {
> u8 max_lanes;
> u8 max_streams;
> bool has_internal_dphy;
> + u32 events[CSI2RX_NUM_EVENTS];
>
> struct v4l2_subdev subdev;
> struct v4l2_async_notifier notifier;
> @@ -124,6 +167,50 @@ static const struct csi2rx_fmt formats[] = {
> { .code = MEDIA_BUS_FMT_BGR888_1X24, .bpp = 24, },
> };
>
> +static void csi2rx_configure_error_irq_mask(void __iomem *base, struct csi2rx_priv *csi2rx)
> +{
> + u32 error_irq_mask = 0;
> +
> + error_irq_mask |= CSI2RX_ECC_ERRORS;
> + error_irq_mask |= CSI2RX_PACKET_ERRORS;
> +
> + /*
> + * iterate through all source pads and check if they are linked
nit: s/iterate/Iterate
> + * to an active remote pad. If an active remote pad is found,
> + * calculate the corresponding bit position and set it in
> + * mask, enabling the stream overflow error in the mask.
> + */
> +
nit: drop this extra whitespace
> + for (int i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) {
> + struct media_pad *remote_pad = media_pad_remote_pad_first(&csi2rx->pads[i]);
> +
> + if (remote_pad) {
> + int bit_position = 16 + (i - CSI2RX_PAD_SOURCE_STREAM0);
It would be cleaner to not use the magic number of 16 here, instead using the
already defined macro:
error_irq_mask |= (CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ
<< (i - CSI2RX_PAD_SOURCE_STREAM0));
> +
> + error_irq_mask |= (1 << bit_position);
> + }
> + }
> +
> + writel(error_irq_mask, base + CSI2RX_ERROR_IRQS_MASK_REG);
> +}
> +
> +static irqreturn_t csi2rx_irq_handler(int irq, void *dev_id)
> +{
> + struct csi2rx_priv *csi2rx = dev_id;
> + int i;
> + u32 error_status;
> +
> + error_status = readl(csi2rx->base + CSI2RX_ERROR_IRQS_REG);
> +
> + for (i = 0; i < CSI2RX_NUM_EVENTS; i++)
> + if (error_status & csi2rx_events[i].mask)
> + csi2rx->events[i]++;
> +
> + writel(error_status, csi2rx->base + CSI2RX_ERROR_IRQS_REG);
> +
> + return IRQ_HANDLED;
> +}
> +
> static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> {
> unsigned int i;
> @@ -220,6 +307,9 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> reset_control_deassert(csi2rx->p_rst);
> csi2rx_reset(csi2rx);
>
> + if (csi2rx->error_irq >= 0)
> + csi2rx_configure_error_irq_mask(csi2rx->base, csi2rx);
> +
> reg = csi2rx->num_lanes << 8;
> for (i = 0; i < csi2rx->num_lanes; i++) {
> reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);
> @@ -332,6 +422,8 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> reset_control_assert(csi2rx->sys_rst);
> clk_disable_unprepare(csi2rx->sys_clk);
>
> + writel(0, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
> +
> for (i = 0; i < csi2rx->max_streams; i++) {
> writel(CSI2RX_STREAM_CTRL_STOP,
> csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> @@ -363,6 +455,21 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> }
> }
>
> +static int csi2rx_log_status(struct v4l2_subdev *sd)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> + unsigned int i;
> +
> + for (i = 0; i < CSI2RX_NUM_EVENTS; i++) {
> + if (csi2rx->events[i])
> + dev_info(csi2rx->dev, "%s events: %d\n",
> + csi2rx_events[i].name,
> + csi2rx->events[i]);
> + }
> +
> + return 0;
> +}
> +
> static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> {
> struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> @@ -468,7 +575,12 @@ static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> .s_stream = csi2rx_s_stream,
> };
>
> +static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
> + .log_status = csi2rx_log_status,
> +};
> +
> static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
> + .core = &csi2rx_core_ops,
> .video = &csi2rx_video_ops,
> .pad = &csi2rx_pad_ops,
> };
> @@ -705,6 +817,19 @@ static int csi2rx_probe(struct platform_device *pdev)
> if (ret)
> goto err_cleanup;
>
> + csi2rx->error_irq = platform_get_irq_byname_optional(pdev, "error_irq");
> +
> + if (csi2rx->error_irq < 0) {
> + dev_dbg(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
> + } else {
> + ret = devm_request_irq(csi2rx->dev, csi2rx->error_irq, csi2rx_irq_handler, 0,
> + "csi2rx-error-irq", csi2rx);
Why "csi2rx-error-irq" is passed in the devname argument instead of the device
name `dev_name(&pdev->dev)`? Also there is an alignment issue:
CHECK: Alignment should match open parenthesis
#204: FILE: drivers/media/platform/cadence/cdns-csi2rx.c:824:
+ ret = devm_request_irq(csi2rx->dev, csi2rx->error_irq, csi2rx_irq_handler, 0,
+ "csi2rx-error-irq", csi2rx);
> + if (ret) {
> + dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
> + return ret;
> + }
> + }
> +
> ret = v4l2_subdev_init_finalize(&csi2rx->subdev);
> if (ret)
> goto err_cleanup;
> --
> 2.34.1
>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists