linux-kernel - Re: [PATCH v8 3/8] media: staging: media: starfive: camss: Add core driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Aug 2023 20:31:44 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Jack Zhu <jack.zhu@...rfivetech.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Foss <rfoss@...nel.org>,
        Todor Tomov <todor.too@...il.com>, bryan.odonoghue@...aro.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-staging@...ts.linux.dev,
        changhuang.liang@...rfivetech.com
Subject: Re: [PATCH v8 3/8] media: staging: media: starfive: camss: Add core
 driver

Le 24/08/2023 à 10:01, Jack Zhu a écrit :
> Add core driver for StarFive Camera Subsystem. The code parses
> the device platform resources and registers related devices.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
> Signed-off-by: Jack Zhu <jack.zhu@...rfivetech.com>
> ---

...

> diff --git a/drivers/staging/media/starfive/camss/Kconfig b/drivers/staging/media/starfive/camss/Kconfig
> new file mode 100644
> index 000000000000..8d20e2bd2559
> --- /dev/null
> +++ b/drivers/staging/media/starfive/camss/Kconfig
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config VIDEO_STARFIVE_CAMSS
> +	tristate "Starfive Camera Subsystem driver"
> +	depends on V4L_PLATFORM_DRIVERS
> +	depends on VIDEO_DEV && OF
> +	depends on HAS_DMA
> +	depends on PM
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	select VIDEOBUF2_DMA_CONTIG
> +	select V4L2_FWNODE
> +	help
> +	   Enable this to support for the Starfive Camera subsystem
> +	   found on Starfive JH7110 SoC.
> +
> +	   To compile this driver as a module, choose M here: the
> +	   module will be called stf-camss.

stf_camss? (s/-/_)

> diff --git a/drivers/staging/media/starfive/camss/Makefile b/drivers/staging/media/starfive/camss/Makefile
> new file mode 100644
> index 000000000000..f53c5cbe958f
> --- /dev/null
> +++ b/drivers/staging/media/starfive/camss/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for StarFive Camera Subsystem driver
> +#
> +
> +starfive-camss-objs += \
> +		stf_camss.o
> +
> +obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o

I'm not an expert in Makefile files, but this stf_camss.o and 
starfive-camss.o look strange to me.

> diff --git a/drivers/staging/media/starfive/camss/stf_camss.c b/drivers/staging/media/starfive/camss/stf_camss.c
> new file mode 100644
> index 000000000000..75ebc3a35218
> --- /dev/null
> +++ b/drivers/staging/media/starfive/camss/stf_camss.c

...

> +static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
> +{
> +	struct device_node *node = NULL;
> +	int ret, num_subdevs = 0;
> +
> +	for_each_endpoint_of_node(stfcamss->dev->of_node, node) {
> +		struct stfcamss_async_subdev *csd;
> +
> +		if (!of_device_is_available(node))
> +			continue;
> +
> +		csd = v4l2_async_nf_add_fwnode_remote(&stfcamss->notifier,
> +						      of_fwnode_handle(node),
> +						      struct stfcamss_async_subdev);
> +		if (IS_ERR(csd)) {
> +			ret = PTR_ERR(csd);
> +			dev_err(stfcamss->dev, "failed to add async notifier\n");
> +			v4l2_async_nf_cleanup(&stfcamss->notifier);

having it here, looks strange to me.
It is already called in the error handling path of the probe.

Should there be a "of_node_put(node);" if we return here?

> +			return ret;
> +		}
> +
> +		ret = stfcamss_of_parse_endpoint_node(stfcamss, node, csd);
> +		if (ret)
> +			return ret;
> +
> +		num_subdevs++;
> +	}
> +
> +	return num_subdevs;
> +}

...

> +static int stfcamss_remove(struct platform_device *pdev)
> +{
> +	struct stfcamss *stfcamss = platform_get_drvdata(pdev);
> +
> +	v4l2_device_unregister(&stfcamss->v4l2_dev);
> +	media_device_cleanup(&stfcamss->media_dev);

Is a "v4l2_async_nf_cleanup(&stfcamss->notifier);" missing to match the 
error handling path of the probe?

> +	pm_runtime_disable(&pdev->dev);
> +
> +	return 0;
> +}
> +

...