linux-kernel - Re: [PATCH] ASoC: samsung: Implement abox generic structure

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]

Message-ID: <aG4sb7UcqvHz_Z5r@finisterre.sirena.org.uk>
Date: Wed, 9 Jul 2025 09:46:39 +0100
From: Mark Brown <broonie@...nel.org>
To: ew.kim@...sung.com
Cc: s.nawrocki@...sung.com, lgirdwood@...il.com, perex@...ex.cz,
	tiwai@...e.com, linux-sound@...r.kernel.org,
	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ASoC: samsung: Implement abox generic structure

On Wed, Jul 09, 2025 at 09:10:02AM +0900, ew.kim@...sung.com wrote:
> From: ew kim <ew.kim@...sung.com>
> 
> Implemet basic abox generic drivers.
> This driver is a management driver for the generic drivers used in
> Automotive Abox, connecting them to SOC drivers.
> It supports various Exynos Automotive SOCs.

I can't really tell what the driver is supposed to do from this - what
is abox?  Looking at the code I'm not really much clearer, to a large
extent because it doesn't seem to integrate with the subsystem (or other
kernel subsystems) at all.  It looks like this might be intended to
control a DSP offload system, but it's not 100% clear, and it seems like
there's an ioctl() interface which it looks like it's exposing internal
abstractions to userspace.  This needs to be some combination of much
more clearly explained and better integrated with the existing kernel
subsystems, right now I can't really review it effectively because it's
hard for me to tell what the code is trying to do.

I've got a few very supreficial comments below but really there's a
structural problem that needs to be addressed first.

> +++ b/sound/soc/samsung/auto_abox/generic/abox_generic.c
> @@ -0,0 +1,568 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *        http://www.samsung.com/

It's now 2025...

Please also make the entire comment a C++ one so things look more
intentional.

> +//#define DEBUG

Just delete this, it can be added if needed.

> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/delay.h>
> +#include <linux/suspend.h>
> +#include <sound/soc.h>
> +#include <sound/soc-dapm.h>
> +#include <sound/pcm_params.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/sched/clock.h>
> +#include <linux/ktime.h>
> +#include <linux/iommu.h>
> +#include <linux/clk-provider.h>
> +#include <linux/kmod.h>
> +#include <linux/umh.h>
> +#include <linux/string.h>

Do you really need all these headers?

> +static struct abox_generic_data *g_abox_generic_data;

This isn't really the kernel style - neither the g_ naming, nor the
concept of having a global for a driver.

> +/**
> + * @cnotice
> + * @prdcode
> + * @Sub_SW_Component{abox_generic}
> + * @ALM_Link {work item url}
> + * @purpose "get value from virtual address"
> + * @logic "return global abox_generic_data"
> + * \image html
> + * @params
> + * @param{in, -, -, -}
> + * @endparam
> + * @retval {-, struct *abox_generic_data, !NULL, NULL}
> + */

This is not the style for kernel comments, and doesn't seem to make
terribly much sense.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)