| 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: <aTCsS52Bbtld/vTk@lizhi-Precision-Tower-5810>
Date: Wed, 3 Dec 2025 16:31:55 -0500
From: Frank Li <Frank.li@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Philipp Zabel <p.zabel@...gutronix.de>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
Rui Miguel Silva <rmfrfs@...il.com>,
Martin Kepplinger-Novakovic <martink@...teo.de>,
Purism Kernel Team <kernel@...i.sm>, linux-media@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] media: nxp: use cleanup __free(fwnode_handle)
simplify code
On Wed, Nov 19, 2025 at 02:02:39PM +0900, Laurent Pinchart wrote:
> On Wed, Nov 19, 2025 at 01:25:29PM +0900, Laurent Pinchart wrote:
> > On Mon, Nov 17, 2025 at 01:58:14PM -0500, Frank Li wrote:
> > > Use cleanup __free(fwnode_handle) simplify code. Change to dev_err_probe()
> > > because replace goto with return.
> > >
> > > Add missed "\n" at error message.
> > >
> > > No functional change.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > ---
> > > drivers/media/platform/nxp/imx-mipi-csis.c | 31 +++++++++------------------
> > > drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 31 +++++++++------------------
> > > 2 files changed, 20 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > index ce93d868746f002c22e2f86b1e0aa84ec1a76061..d924adb406a30797b66f0094ab17e98ad44fefac 100644
> > > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > @@ -12,6 +12,7 @@
> > > *
> > > */
> > >
> > > +#include <linux/cleanup.h>
> > > #include <linux/clk.h>
> > > #include <linux/debugfs.h>
> > > #include <linux/delay.h>
> > > @@ -1349,28 +1350,25 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> > > .bus_type = V4L2_MBUS_CSI2_DPHY,
> > > };
> > > struct v4l2_async_connection *asd;
> > uuu> - struct fwnode_handle *ep;
> > > unsigned int i;
> > > int ret;
> > >
> > > v4l2_async_subdev_nf_init(&csis->notifier, &csis->sd);
> > >
> > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
> > > - FWNODE_GRAPH_ENDPOINT_NEXT);
> > > + struct fwnode_handle *ep __free(fwnode_handle) =
> > > + fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
> > > + FWNODE_GRAPH_ENDPOINT_NEXT);
> >
> > Let's avoid mixing variable declarations and code, this is a style
> > change that is not widely accepted (yet). You can write
> >
> > struct fwnode_handle *ep __free(fwnode_handle) = NULL;
> > struct v4l2_async_connection *asd;
> > unsigned int i;
> > int ret;
> >
> > v4l2_async_subdev_nf_init(&csis->notifier, &csis->sd);
> >
> > ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csis->dev), 0, 0,
> > FWNODE_GRAPH_ENDPOINT_NEXT);
>
> Quite a coincidence, there's an ongoing mail thread on LKML about this
> topic:
>
> http://lore.kernel.org/r/CAHk-=whPZoi03ZwphxiW6cuWPtC3nyKYS8_BThgztCdgPWP1WA@mail.gmail.com
Laurent:
After read above thread and cleanup.h.
Look like my method is prefered.
Frank
>
> > > if (!ep)
> > > return -ENOTCONN;
> > >
> > > ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > if (ret)
> > > - goto err_parse;
> > > + return ret;
> > >
> > > for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> > > - if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> > > - dev_err(csis->dev,
> > > - "data lanes reordering is not supported");
> > > - ret = -EINVAL;
> > > - goto err_parse;
> > > - }
> > > + if (vep.bus.mipi_csi2.data_lanes[i] != i + 1)
> > > + return dev_err_probe(csis->dev, -EINVAL,
> > > + "data lanes reordering is not supported\n");
> >
> > To switch to dev_err_probe(), we should drop the error message in the
> > probe() function when mipi_csis_async_register() fails, and make sure
> > every error path in this function prints a message. I'd prefer splitting
> > that to a separate patch.
> >
> > > }
> > >
> > > csis->bus = vep.bus.mipi_csi2;
> > > @@ -1381,12 +1379,8 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> > >
> > > asd = v4l2_async_nf_add_fwnode_remote(&csis->notifier, ep,
> > > struct v4l2_async_connection);
> > > - if (IS_ERR(asd)) {
> > > - ret = PTR_ERR(asd);
> > > - goto err_parse;
> > > - }
> > > -
> > > - fwnode_handle_put(ep);
> > > + if (IS_ERR(asd))
> > > + return PTR_ERR(asd);
> > >
> > > csis->notifier.ops = &mipi_csis_notify_ops;
> > >
> > > @@ -1395,11 +1389,6 @@ static int mipi_csis_async_register(struct mipi_csis_device *csis)
> > > return ret;
> > >
> > > return v4l2_async_register_subdev(&csis->sd);
> > > -
> > > -err_parse:
> > > - fwnode_handle_put(ep);
> > > -
> > > - return ret;
> > > }
> > >
> > > /* -----------------------------------------------------------------------------
> > > diff --git a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> > > index 75709161fb26a61239b94430365849e022fdc14f..94882568405db55593c5c51722db2233a64d53e4 100644
> > > --- a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> > > +++ b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c
> > > @@ -6,6 +6,7 @@
> > > */
> > >
> > > #include <linux/bitfield.h>
> > > +#include <linux/cleanup.h>
> > > #include <linux/clk.h>
> > > #include <linux/clk-provider.h>
> > > #include <linux/delay.h>
> > > @@ -717,28 +718,25 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
> > > .bus_type = V4L2_MBUS_CSI2_DPHY,
> > > };
> > > struct v4l2_async_connection *asd;
> > > - struct fwnode_handle *ep;
> > > unsigned int i;
> > > int ret;
> > >
> > > v4l2_async_subdev_nf_init(&state->notifier, &state->sd);
> > >
> > > - ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
> > > - FWNODE_GRAPH_ENDPOINT_NEXT);
> > > + struct fwnode_handle *ep __free(fwnode_handle) =
> > > + fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
> > > + FWNODE_GRAPH_ENDPOINT_NEXT);
> >
> > Same comment as above.
> >
> > > if (!ep)
> > > return -ENOTCONN;
> > >
> > > ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> > > if (ret)
> > > - goto err_parse;
> > > + return ret;
> > >
> > > for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> > > - if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> > > - dev_err(state->dev,
> > > - "data lanes reordering is not supported");
> > > - ret = -EINVAL;
> > > - goto err_parse;
> > > - }
> > > + if (vep.bus.mipi_csi2.data_lanes[i] != i + 1)
> > > + return dev_err_probe(state->dev, -EINVAL,
> > > + "data lanes reordering is not supported\n");
> >
> > And here too.
> >
> > Usage of __free(fwnode_handle) looks good, it just needs to be split to
> > a patch of its own.
> >
> > > }
> > >
> > > state->bus = vep.bus.mipi_csi2;
> > > @@ -749,12 +747,8 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
> > >
> > > asd = v4l2_async_nf_add_fwnode_remote(&state->notifier, ep,
> > > struct v4l2_async_connection);
> > > - if (IS_ERR(asd)) {
> > > - ret = PTR_ERR(asd);
> > > - goto err_parse;
> > > - }
> > > -
> > > - fwnode_handle_put(ep);
> > > + if (IS_ERR(asd))
> > > + return PTR_ERR(asd);
> > >
> > > state->notifier.ops = &imx8mq_mipi_csi_notify_ops;
> > >
> > > @@ -763,11 +757,6 @@ static int imx8mq_mipi_csi_async_register(struct csi_state *state)
> > > return ret;
> > >
> > > return v4l2_async_register_subdev(&state->sd);
> > > -
> > > -err_parse:
> > > - fwnode_handle_put(ep);
> > > -
> > > - return ret;
> > > }
> > >
> > > /* -----------------------------------------------------------------------------
>
> --
> Regards,
>
> Laurent Pinchart
Powered by blists - more mailing lists