| 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: <20250114204240.GA29414@pendragon.ideasonboard.com>
Date: Tue, 14 Jan 2025 22:42:40 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Hans Verkuil <hverkuil@...all.nl>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Tomasz Figa <tfiga@...omium.org>,
Marek Szyprowski <m.szyprowski@...sung.com>,
Maxim Levitsky <maximlevitsky@...il.com>,
Sean Young <sean@...s.org>, Olli Salonen <olli.salonen@....fi>,
Abylay Ospan <aospan@...zon.com>, Jemma Denson <jdenson@...il.com>,
Patrick Boettcher <patrick.boettcher@...teo.de>,
Jacopo Mondi <jacopo+renesas@...ndi.org>,
Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
Tim Harvey <tharvey@...eworks.com>,
Andy Walls <awalls@...metrocast.net>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Bingbu Cao <bingbu.cao@...el.com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] media: platform: Use str_enable_disable-like helpers
Hi Krzysztof,
Thank you for the patch.
On Tue, Jan 14, 2025 at 08:46:21PM +0100, Krzysztof Kozlowski wrote:
> Replace ternary (condition ? "enable" : "disable") syntax with helpers
> from string_choices.h because:
> 1. Simple function call with one argument is easier to read. Ternary
> operator has three arguments and with wrapping might lead to quite
> long code.
It's more difficult to read for me.
> 2. Is slightly shorter thus also easier to read.
> 3. It brings uniformity in the text - same string.
> 4. Allows deduping by the linker, which results in a smaller binary
> file.
I don't see why the linker can't de-dup string in the current code.
I'm sorry, I just don't see the point in doing this. I'd like to avoid
those changes in the Linux media subsystem, or at the very least in
drivers I maintain.
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> ---
> drivers/media/platform/amphion/venc.c | 3 ++-
> drivers/media/platform/amphion/vpu_dbg.c | 3 ++-
> drivers/media/platform/aspeed/aspeed-video.c | 5 +++--
> drivers/media/platform/chips-media/coda/coda-bit.c | 3 ++-
> drivers/media/platform/chips-media/coda/imx-vdoa.c | 3 ++-
> drivers/media/platform/st/sti/hva/hva-debugfs.c | 7 ++++---
> drivers/media/platform/ti/cal/cal-camerarx.c | 3 ++-
> drivers/media/platform/ti/omap3isp/ispstat.c | 3 ++-
> drivers/media/platform/xilinx/xilinx-csi2rxss.c | 19 ++++++++++---------
> 9 files changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/platform/amphion/venc.c b/drivers/media/platform/amphion/venc.c
> index c5c1f1fbaa80366d5b18b8f24699eb9c3a18ef92..65b1665eece8cd6efc535281b3be593abaf9ba64 100644
> --- a/drivers/media/platform/amphion/venc.c
> +++ b/drivers/media/platform/amphion/venc.c
> @@ -13,6 +13,7 @@
> #include <linux/videodev2.h>
> #include <linux/ktime.h>
> #include <linux/rational.h>
> +#include <linux/string_choices.h>
> #include <linux/vmalloc.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-event.h>
> @@ -1215,7 +1216,7 @@ static int venc_get_debug_info(struct vpu_inst *inst, char *str, u32 size, u32 i
> break;
> case 8:
> num = scnprintf(str, size, "rc: %s, mode = %d, bitrate = %d(%d), qp = %d\n",
> - venc->params.rc_enable ? "enable" : "disable",
> + str_enable_disable(venc->params.rc_enable),
> venc->params.rc_mode,
> venc->params.bitrate,
> venc->params.bitrate_max,
> diff --git a/drivers/media/platform/amphion/vpu_dbg.c b/drivers/media/platform/amphion/vpu_dbg.c
> index 940e5bda5fa391c47552c275bd1266f47d57f475..b726d884086d306cd4298dc46440a2235b311b86 100644
> --- a/drivers/media/platform/amphion/vpu_dbg.c
> +++ b/drivers/media/platform/amphion/vpu_dbg.c
> @@ -11,6 +11,7 @@
> #include <linux/kernel.h>
> #include <linux/types.h>
> #include <linux/pm_runtime.h>
> +#include <linux/string_choices.h>
> #include <media/v4l2-device.h>
> #include <linux/debugfs.h>
> #include "vpu.h"
> @@ -256,7 +257,7 @@ static int vpu_dbg_core(struct seq_file *s, void *data)
> return 0;
>
> num = scnprintf(str, sizeof(str), "power %s\n",
> - vpu_iface_get_power_state(core) ? "on" : "off");
> + str_on_off(vpu_iface_get_power_state(core)));
> if (seq_write(s, str, num))
> return 0;
> num = scnprintf(str, sizeof(str), "state = %d\n", core->state);
> diff --git a/drivers/media/platform/aspeed/aspeed-video.c b/drivers/media/platform/aspeed/aspeed-video.c
> index 54cae0da9aca3fd74800b51f79136a261aec241a..d9cf12af05b75c76c506f6e7d72dfc41c9e05317 100644
> --- a/drivers/media/platform/aspeed/aspeed-video.c
> +++ b/drivers/media/platform/aspeed/aspeed-video.c
> @@ -19,6 +19,7 @@
> #include <linux/sched.h>
> #include <linux/spinlock.h>
> #include <linux/string.h>
> +#include <linux/string_choices.h>
> #include <linux/v4l2-controls.h>
> #include <linux/videodev2.h>
> #include <linux/wait.h>
> @@ -1227,7 +1228,7 @@ static void aspeed_video_update_regs(struct aspeed_video *video)
> v4l2_dbg(1, debug, &video->v4l2_dev, "compression quality(%d)\n",
> video->jpeg_quality);
> v4l2_dbg(1, debug, &video->v4l2_dev, "hq_mode(%s) hq_quality(%d)\n",
> - video->hq_mode ? "on" : "off", video->jpeg_hq_quality);
> + str_on_off(video->hq_mode), video->jpeg_hq_quality);
>
> if (video->format == VIDEO_FMT_ASPEED)
> aspeed_video_update(video, VE_BCD_CTRL, 0, VE_BCD_CTRL_EN_BCD);
> @@ -1939,7 +1940,7 @@ static int aspeed_video_debugfs_show(struct seq_file *s, void *data)
> seq_printf(s, " %-20s:\t%d\n", "Quality", v->jpeg_quality);
> if (v->format == VIDEO_FMT_ASPEED) {
> seq_printf(s, " %-20s:\t%s\n", "HQ Mode",
> - v->hq_mode ? "on" : "off");
> + str_on_off(v->hq_mode));
> seq_printf(s, " %-20s:\t%d\n", "HQ Quality",
> v->hq_mode ? v->jpeg_hq_quality : 0);
> }
> diff --git a/drivers/media/platform/chips-media/coda/coda-bit.c b/drivers/media/platform/chips-media/coda/coda-bit.c
> index 84ded154adfe37147218d60278a1c1fac88ecadc..2cb0c04003da750f7108578e274da31778c3f2d2 100644
> --- a/drivers/media/platform/chips-media/coda/coda-bit.c
> +++ b/drivers/media/platform/chips-media/coda/coda-bit.c
> @@ -16,6 +16,7 @@
> #include <linux/ratelimit.h>
> #include <linux/reset.h>
> #include <linux/slab.h>
> +#include <linux/string_choices.h>
> #include <linux/videodev2.h>
>
> #include <media/v4l2-common.h>
> @@ -1881,7 +1882,7 @@ static int __coda_decoder_seq_init(struct coda_ctx *ctx)
> lockdep_assert_held(&dev->coda_mutex);
>
> coda_dbg(1, ctx, "Video Data Order Adapter: %s\n",
> - ctx->use_vdoa ? "Enabled" : "Disabled");
> + str_enabled_disabled(ctx->use_vdoa));
>
> /* Start decoding */
> q_data_src = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> diff --git a/drivers/media/platform/chips-media/coda/imx-vdoa.c b/drivers/media/platform/chips-media/coda/imx-vdoa.c
> index c3561fcecb98c7d3cd741c28afcb2a3854eaa0e7..abdff181d417788460b7f6230ea54789b242d436 100644
> --- a/drivers/media/platform/chips-media/coda/imx-vdoa.c
> +++ b/drivers/media/platform/chips-media/coda/imx-vdoa.c
> @@ -15,6 +15,7 @@
> #include <linux/platform_device.h>
> #include <linux/videodev2.h>
> #include <linux/slab.h>
> +#include <linux/string_choices.h>
>
> #include "imx-vdoa.h"
>
> @@ -117,7 +118,7 @@ static irqreturn_t vdoa_irq_handler(int irq, void *data)
> writel(val, vdoa->regs + VDOAIST);
> if (val & VDOAIST_TERR) {
> val = readl(vdoa->regs + VDOASR) & VDOASR_ERRW;
> - dev_err(vdoa->dev, "AXI %s error\n", val ? "write" : "read");
> + dev_err(vdoa->dev, "AXI %s error\n", str_write_read(val));
> } else if (!(val & VDOAIST_EOT)) {
> dev_warn(vdoa->dev, "Spurious interrupt\n");
> }
> diff --git a/drivers/media/platform/st/sti/hva/hva-debugfs.c b/drivers/media/platform/st/sti/hva/hva-debugfs.c
> index a86a07b6fbc792fc06db2dbbb3934694136a7813..1cb5bca44939606f39911a41e5f464be888848c2 100644
> --- a/drivers/media/platform/st/sti/hva/hva-debugfs.c
> +++ b/drivers/media/platform/st/sti/hva/hva-debugfs.c
> @@ -6,6 +6,7 @@
> */
>
> #include <linux/debugfs.h>
> +#include <linux/string_choices.h>
>
> #include "hva.h"
> #include "hva-hw.h"
> @@ -71,12 +72,12 @@ static void format_ctx(struct seq_file *s, struct hva_ctx *ctx)
> " | |- SEI frame packing type=%s\n",
> v4l2_ctrl_get_menu(entropy)[ctrls->entropy_mode],
> ctrls->cpb_size,
> - ctrls->dct8x8 ? "true" : "false",
> + str_true_false(ctrls->dct8x8),
> ctrls->qpmin,
> ctrls->qpmax,
> - ctrls->vui_sar ? "true" : "false",
> + str_true_false(ctrls->vui_sar),
> v4l2_ctrl_get_menu(vui_sar)[ctrls->vui_sar_idc],
> - ctrls->sei_fp ? "true" : "false",
> + str_true_false(ctrls->sei_fp),
> v4l2_ctrl_get_menu(sei_fp)[ctrls->sei_fp_type]);
> }
>
> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> index 42dfe08b765f6bbdb0ab8cca0f7d6d87f2ff18eb..a70814cbada82654e926b12bcde73300107aaa8a 100644
> --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> @@ -17,6 +17,7 @@
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/slab.h>
> +#include <linux/string_choices.h>
>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-fwnode.h>
> @@ -191,7 +192,7 @@ static void cal_camerarx_power(struct cal_camerarx *phy, bool enable)
>
> if (i == 10)
> phy_err(phy, "Failed to power %s complexio\n",
> - enable ? "up" : "down");
> + str_up_down(enable));
> }
>
> static void cal_camerarx_wait_reset(struct cal_camerarx *phy)
> diff --git a/drivers/media/platform/ti/omap3isp/ispstat.c b/drivers/media/platform/ti/omap3isp/ispstat.c
> index 359a846205b0ffe9e736c7ed37c22677991cc9f2..f1293d412415d3fe36a87e7aa93a60e7daf693d8 100644
> --- a/drivers/media/platform/ti/omap3isp/ispstat.c
> +++ b/drivers/media/platform/ti/omap3isp/ispstat.c
> @@ -14,6 +14,7 @@
>
> #include <linux/dma-mapping.h>
> #include <linux/slab.h>
> +#include <linux/string_choices.h>
> #include <linux/timekeeping.h>
> #include <linux/uaccess.h>
>
> @@ -768,7 +769,7 @@ int omap3isp_stat_enable(struct ispstat *stat, u8 enable)
> unsigned long irqflags;
>
> dev_dbg(stat->isp->dev, "%s: user wants to %s module.\n",
> - stat->subdev.name, enable ? "enable" : "disable");
> + stat->subdev.name, str_enable_disable(enable));
>
> /* Prevent enabling while configuring */
> mutex_lock(&stat->ioctl_lock);
> diff --git a/drivers/media/platform/xilinx/xilinx-csi2rxss.c b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> index 146131b8f37e5a30b168164d4eaedc9641d6af31..a5074f05cee50e117256fdb8496b977332757e27 100644
> --- a/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> +++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> @@ -16,6 +16,7 @@
> #include <linux/of.h>
> #include <linux/of_irq.h>
> #include <linux/platform_device.h>
> +#include <linux/string_choices.h>
> #include <linux/v4l2-subdev.h>
> #include <media/media-entity.h>
> #include <media/mipi-csi2.h>
> @@ -400,19 +401,19 @@ static int xcsi2rxss_log_status(struct v4l2_subdev *sd)
> dev_info(dev, "***** Core Status *****\n");
> data = xcsi2rxss_read(xcsi2rxss, XCSI_CSR_OFFSET);
> dev_info(dev, "Short Packet FIFO Full = %s\n",
> - data & XCSI_CSR_SPFIFOFULL ? "true" : "false");
> + str_true_false(data & XCSI_CSR_SPFIFOFULL));
> dev_info(dev, "Short Packet FIFO Not Empty = %s\n",
> - data & XCSI_CSR_SPFIFONE ? "true" : "false");
> + str_true_false(data & XCSI_CSR_SPFIFONE));
> dev_info(dev, "Stream line buffer full = %s\n",
> - data & XCSI_CSR_SLBF ? "true" : "false");
> + str_true_false(data & XCSI_CSR_SLBF));
> dev_info(dev, "Soft reset/Core disable in progress = %s\n",
> - data & XCSI_CSR_RIPCD ? "true" : "false");
> + str_true_false(data & XCSI_CSR_RIPCD));
>
> /* Clk & Lane Info */
> dev_info(dev, "******** Clock Lane Info *********\n");
> data = xcsi2rxss_read(xcsi2rxss, XCSI_CLKINFR_OFFSET);
> dev_info(dev, "Clock Lane in Stop State = %s\n",
> - data & XCSI_CLKINFR_STOP ? "true" : "false");
> + str_true_false(data & XCSI_CLKINFR_STOP));
>
> dev_info(dev, "******** Data Lane Info *********\n");
> dev_info(dev, "Lane\tSoT Error\tSoT Sync Error\tStop State\n");
> @@ -421,9 +422,9 @@ static int xcsi2rxss_log_status(struct v4l2_subdev *sd)
> data = xcsi2rxss_read(xcsi2rxss, reg);
>
> dev_info(dev, "%d\t%s\t\t%s\t\t%s\n", i,
> - data & XCSI_DLXINFR_SOTERR ? "true" : "false",
> - data & XCSI_DLXINFR_SOTSYNCERR ? "true" : "false",
> - data & XCSI_DLXINFR_STOP ? "true" : "false");
> + str_true_false(data & XCSI_DLXINFR_SOTERR),
> + str_true_false(data & XCSI_DLXINFR_SOTSYNCERR),
> + str_true_false(data & XCSI_DLXINFR_STOP));
>
> reg += XCSI_NEXTREG_OFFSET;
> }
> @@ -889,7 +890,7 @@ static int xcsi2rxss_parse_of(struct xcsi2rxss_state *xcsi2rxss)
> fwnode_handle_put(ep);
>
> dev_dbg(dev, "vcx %s, %u data lanes (%s), data type 0x%02x\n",
> - xcsi2rxss->en_vcx ? "enabled" : "disabled",
> + str_enabled_disabled(xcsi2rxss->en_vcx),
> xcsi2rxss->max_num_lanes,
> xcsi2rxss->enable_active_lanes ? "dynamic" : "static",
> xcsi2rxss->datatype);
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists