| 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: <7d2401d6-d897-49d7-a3be-50de0727b037@kernel.org>
Date: Wed, 9 Jul 2025 15:57:38 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: ew.kim@...sung.com, s.nawrocki@...sung.com, lgirdwood@...il.com,
broonie@...nel.org, perex@...ex.cz, tiwai@...e.com
Cc: 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 09/07/2025 02:10, ew.kim@...sung.com wrote:
> +/**
> + * @cnotice
> + * @prdcode
> + * @Sub_SW_Component{abox generic}
> + * @ALM_Link {work item url}
> + * @purpose "Disbaling the abox generic"
> + * @logic "Disbale the abox generic"
> + * \image html
> + * @params
> + * @param{in, pdev->dev, struct::device, !NULL}
> + * @endparam
> + * @noret
> + */
> +static void samsung_abox_generic_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct abox_generic_data *data = dev_get_drvdata(dev);
> +
> + dev_info(dev, "%s\n", __func__);
This is just poor code. Clean it up from all such oddities popular in
downstream. Look at upstream code. Do you see such code there? No.
> +
> + if (!data) {
> + dev_err(dev, "%s: Invalid abox generic data\n", __func__);
> + return;
> + }
> + return;
> +}
> +
> +/**
> + * @cnotice
> + * @prdcode
> + * @Sub_SW_Component{abox generic}
> + * @ALM_Link {work item url}
> + * @purpose "shutdown of the abox generic"
> + * @logic "Disbale the abox hardware by calling the following function
> + * pm_runtime_disable(dev)"
> + * \image html
> + * @params
> + * @param{in, pdev->dev, struct:: device, !NULL}
> + * @endparam
> + * @noret
> + */
> +static void samsung_abox_generic_shutdown(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct abox_generic_data *data = dev_get_drvdata(dev);
> +
> + if (!data) {
> + dev_err(dev, "%s: Invalid abox generic data\n", __func__);
> + return;
> + }
> + return;
> +}
> +
> +static const struct of_device_id samsung_abox_generic_match[] = {
> + {
> + .compatible = "samsung,abox_generic",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, samsung_abox_generic_match);
> +
> +static const struct dev_pm_ops samsung_abox_generic_pm = {
> + SET_SYSTEM_SLEEP_PM_OPS(abox_generic_suspend, abox_generic_resume)
> +};
> +
> +struct platform_driver samsung_abox_generic_driver = {
> + .probe = samsung_abox_generic_probe,
> + .remove = samsung_abox_generic_remove,
> + .shutdown = samsung_abox_generic_shutdown,
> + .driver = {
> + .name = "samsung-abox-generic",
> + .owner = THIS_MODULE,
So that's indeed 2013 code you upstream. We really want you to clean it
up before you post some ancient stuff like that.
> + .of_match_table = of_match_ptr(samsung_abox_generic_match),
> + .pm = &samsung_abox_generic_pm,
> + },
> +};
> +
> +module_platform_driver(samsung_abox_generic_driver);
> +/* Module information */
Useless comment.
> +MODULE_AUTHOR("Eunwoo Kim, <ew.kim@...sung.com>");
> +MODULE_DESCRIPTION("Samsung ASoC A-Box Generic Driver");
> +MODULE_ALIAS("platform:samsung-abox-generic");
No, drop. This was raised so many times already...
> +MODULE_LICENSE("GPL v2");
> +
> diff --git a/sound/soc/samsung/auto_abox/generic/include/abox_generic.h b/sound/soc/samsung/auto_abox/generic/include/abox_generic.h
> new file mode 100644
> index 000000000000..1c954272e2b5
> --- /dev/null
> +++ b/sound/soc/samsung/auto_abox/generic/include/abox_generic.h
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ALSA SoC - Samsung ABOX Share Function and Data structure
> + * for Exynos specific extensions
> + *
> + * Copyright (C) 2013-2020 Samsung Electronics Co., Ltd.
> + *
> + * EXYNOS - sound/soc/samsung/abox/include/abox_generic.h
Same with paths. Do you see them anywhere in the kernel?
> + */
> +
> +#ifndef __SND_SOC_ABOX_GENERIC_BASE_H
> +#define __SND_SOC_ABOX_GENERIC_BASE_H
> +
> +#define ABOX_GENERIC_DATA abox_generic_get_abox_generic_data();
> +
> +struct snd_soc_pcm_runtime;
> +
> +enum abox_soc_ioctl_cmd {
> + ABOX_SOC_IOCTL_GET_NUM_OF_RDMA,
> + ABOX_SOC_IOCTL_GET_NUM_OF_WDMA,
> + ABOX_SOC_IOCTL_GET_NUM_OF_UAIF,
> + ABOX_SOC_IOCTL_GET_SOC_TIMER,
> + ABOX_SOC_IOCTL_SET_DMA_BUFFER,
> + ABOX_SOC_IOCTL_SET_PP_POINTER,
> + ABOX_SOC_IOCTL_SET_PERF_PERIOD,
> + ABOX_SOC_IOCTL_CHECK_TIME_MUTEX,
> + ABOX_SOC_IOCTL_CHECK_TIME_NO_MUTEX,
> + ABOX_SOC_IOCTL_PCM_DUMP_INTR,
> + ABOX_SOC_IOCTL_PCM_DUMP_CLOSE,
> + ABOX_SOC_IOCTL_PCM_DUMP_ADD_CONTROL,
> + ABOX_SOC_IOCTL_MAX
> +};
> +
> +typedef int (*SOC_IOCTL)(struct device *soc_dev, enum abox_soc_ioctl_cmd cmd, void *data);
Follow coding style.
> +
> +struct abox_generic_data {
> + struct platform_device *pdev;
> + struct platform_device **pdev_pcm_playback;
> + struct platform_device **pdev_pcm_capture;
> + unsigned int num_of_pcm_playback;
> + unsigned int num_of_pcm_capture;
> + unsigned int num_of_i2s_dummy;
> + unsigned int num_of_rdma;
> + unsigned int num_of_wdma;
> + unsigned int num_of_uaif;
> + struct device *soc_dev;
> + SOC_IOCTL soc_ioctl;
> +};
> +
> +
> +/************ Internal API ************/
Then why do you expose it via header?
> +
> +struct abox_generic_data *abox_generic_get_abox_generic_data(void);
> +
> +int abox_generic_set_dma_buffer(struct device *pcm_dev);
> +
> +int abox_generic_request_soc_ioctl(struct device *generic_dev, enum abox_soc_ioctl_cmd cmd,
> + void *data);
> +
> +int abox_generic_set_pp_pointer(struct device *pcm_dev);
> +
> +
> +
> +
> +/************ External API ************/
> +
> +extern struct device *abox_generic_find_fe_dev_from_rtd(struct snd_soc_pcm_runtime *be);
You cannot have external API. All API is internal first.
> +
> +extern struct platform_device *abox_generic_get_pcm_platform_dev(int pcm_id,
> + int stream_type);
> +
Best regards,
Krzysztof
Powered by blists - more mailing lists