| 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: <5ea6f599-cdeb-46e7-14a8-5fceb331cdb3@quicinc.com>
Date: Wed, 20 Dec 2023 09:10:49 -0800
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Dikshita Agarwal
<quic_dikshita@...cinc.com>
CC: <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<stanimir.k.varbanov@...il.com>, <quic_vgarodia@...cinc.com>,
<agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <mchehab@...nel.org>,
<bryan.odonoghue@...aro.org>, <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v2 01/34] media: introduce common helpers for video
firmware handling
Hi Dmitry
On 12/20/2023 12:12 AM, Dmitry Baryshkov wrote:
> On Wed, 20 Dec 2023 at 10:01, Dikshita Agarwal
> <quic_dikshita@...cinc.com> wrote:
>>
>>
>>
>> On 12/18/2023 11:54 PM, Dmitry Baryshkov wrote:
>>> On 18/12/2023 13:31, Dikshita Agarwal wrote:
>>>> Re-organize the video driver code by introducing a new folder
>>>> 'vcodec' and placing 'venus' driver code inside that.
>>>>
>>>> Introduce common helpers for trustzone based firmware
>>>> load/unload etc. which are placed in common folder
>>>> i.e. 'vcodec'.
>>>> Use these helpers in 'venus' driver. These helpers will be
>>>> used by 'iris' driver as well which is introduced later
>>>> in this patch series.
>>>
>>> But why do you need to move the venus driver to subdir?
>>
>> Currently venus driver is present in drivers/media/platform/qcom folder
>> which also has camss folder. We introduced vcodec to keep common code and
>> moved venus inside that, to indicate that the common code is for vcodec
>> drivers i.e venus and iris. Keeping this in qcom folder would mean, common
>> code will be used for camss only which is not the case here.
>
> you can have .../platform/qcom/camss, .../platform/qcom/vcodec-common,
> .../platform/qcom/venus and .../platform/qcom/iris.
>
> If you were to use build helpers in a proper kernel module, this would
> be more obvious.
>
Although your suggestion is good in terms of avoiding moving venus, I
think the location of venus was wrong to begin with. There should have
always been a vcodec (or similar) folder for venus/iris as that will
establish the boundaries of camss and video sub-system in a cleaner way
I like the mediatek separation that way as it makes the boundaries clear:
drivers/media/platform/mediatek$ ls
jpeg Kconfig Makefile mdp mdp3 vcodec vpu
So I think that this re-org of venus into a vcodec had to happen at some
point. Its just that it aligned with iris addition.
>>>
>>>>
>>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
>>>> ---
>>>> drivers/media/platform/qcom/Kconfig | 2 +-
>>>> drivers/media/platform/qcom/Makefile | 2 +-
>>>> drivers/media/platform/qcom/vcodec/firmware.c | 147 +++++++++
>>>> drivers/media/platform/qcom/vcodec/firmware.h | 21 ++
>>>> .../media/platform/qcom/{ => vcodec}/venus/Kconfig | 0
>>>> .../platform/qcom/{ => vcodec}/venus/Makefile | 4 +-
>>>> .../media/platform/qcom/{ => vcodec}/venus/core.c | 102 +++++-
>>>> .../media/platform/qcom/{ => vcodec}/venus/core.h | 0
>>>> .../media/platform/qcom/{ => vcodec}/venus/dbgfs.c | 0
>>>> .../media/platform/qcom/{ => vcodec}/venus/dbgfs.h | 0
>>>> .../platform/qcom/vcodec/venus/firmware_no_tz.c | 194 +++++++++++
>>>> .../platform/qcom/vcodec/venus/firmware_no_tz.h | 19 ++
>>>> .../platform/qcom/{ => vcodec}/venus/helpers.c | 0
>>>> .../platform/qcom/{ => vcodec}/venus/helpers.h | 0
>>>> .../media/platform/qcom/{ => vcodec}/venus/hfi.c | 0
>>>> .../media/platform/qcom/{ => vcodec}/venus/hfi.h | 0
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_cmds.c | 0
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_cmds.h | 0
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_helper.h | 0
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_msgs.c | 0
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_msgs.h | 0
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_parser.c | 0
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_parser.h | 0
>>>> .../qcom/{ => vcodec}/venus/hfi_plat_bufs.h | 0
>>>> .../qcom/{ => vcodec}/venus/hfi_plat_bufs_v6.c | 0
>>>> .../qcom/{ => vcodec}/venus/hfi_platform.c | 0
>>>> .../qcom/{ => vcodec}/venus/hfi_platform.h | 0
>>>> .../qcom/{ => vcodec}/venus/hfi_platform_v4.c | 0
>>>> .../qcom/{ => vcodec}/venus/hfi_platform_v6.c | 0
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_venus.c | 21 +-
>>>> .../platform/qcom/{ => vcodec}/venus/hfi_venus.h | 0
>>>> .../qcom/{ => vcodec}/venus/hfi_venus_io.h | 0
>>>> .../platform/qcom/{ => vcodec}/venus/pm_helpers.c | 0
>>>> .../platform/qcom/{ => vcodec}/venus/pm_helpers.h | 0
>>>> .../media/platform/qcom/{ => vcodec}/venus/vdec.c | 0
>>>> .../media/platform/qcom/{ => vcodec}/venus/vdec.h | 0
>>>> .../platform/qcom/{ => vcodec}/venus/vdec_ctrls.c | 0
>>>> .../media/platform/qcom/{ => vcodec}/venus/venc.c | 0
>>>> .../media/platform/qcom/{ => vcodec}/venus/venc.h | 0
>>>> .../platform/qcom/{ => vcodec}/venus/venc_ctrls.c | 0
>>>> drivers/media/platform/qcom/venus/firmware.c | 363
>>>> ---------------------
>>>> drivers/media/platform/qcom/venus/firmware.h | 26 --
>>>> 42 files changed, 492 insertions(+), 409 deletions(-)
>>>> create mode 100644 drivers/media/platform/qcom/vcodec/firmware.c
>>>> create mode 100644 drivers/media/platform/qcom/vcodec/firmware.h
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/Kconfig (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/Makefile (83%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/core.c (91%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/core.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/dbgfs.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/dbgfs.h (100%)
>>>> create mode 100644
>>>> drivers/media/platform/qcom/vcodec/venus/firmware_no_tz.c
>>>> create mode 100644
>>>> drivers/media/platform/qcom/vcodec/venus/firmware_no_tz.h
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/helpers.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/helpers.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_cmds.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_cmds.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_helper.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_msgs.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_msgs.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_parser.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_parser.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_plat_bufs.h
>>>> (100%)
>>>> rename drivers/media/platform/qcom/{ =>
>>>> vcodec}/venus/hfi_plat_bufs_v6.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_platform.c
>>>> (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_platform.h
>>>> (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_platform_v4.c
>>>> (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_platform_v6.c
>>>> (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_venus.c (99%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_venus.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_venus_io.h
>>>> (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/pm_helpers.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/pm_helpers.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/vdec.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/vdec.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/vdec_ctrls.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/venc.c (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/venc.h (100%)
>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/venc_ctrls.c (100%)
>>>> delete mode 100644 drivers/media/platform/qcom/venus/firmware.c
>>>> delete mode 100644 drivers/media/platform/qcom/venus/firmware.h
>>>>
>>>> diff --git a/drivers/media/platform/qcom/Kconfig
>>>> b/drivers/media/platform/qcom/Kconfig
>>>> index cc5799b..e94142f 100644
>>>> --- a/drivers/media/platform/qcom/Kconfig
>>>> +++ b/drivers/media/platform/qcom/Kconfig
>>>> @@ -3,4 +3,4 @@
>>>> comment "Qualcomm media platform drivers"
>>>> source "drivers/media/platform/qcom/camss/Kconfig"
>>>> -source "drivers/media/platform/qcom/venus/Kconfig"
>>>> +source "drivers/media/platform/qcom/vcodec/venus/Kconfig"
>>>> diff --git a/drivers/media/platform/qcom/Makefile
>>>> b/drivers/media/platform/qcom/Makefile
>>>> index 4f055c3..3d2d82b 100644
>>>> --- a/drivers/media/platform/qcom/Makefile
>>>> +++ b/drivers/media/platform/qcom/Makefile
>>>> @@ -1,3 +1,3 @@
>>>> # SPDX-License-Identifier: GPL-2.0-only
>>>> obj-y += camss/
>>>> -obj-y += venus/
>>>> +obj-y += vcodec/venus/
>>>> diff --git a/drivers/media/platform/qcom/vcodec/firmware.c
>>>> b/drivers/media/platform/qcom/vcodec/firmware.c
>>>> new file mode 100644
>>>> index 0000000..dbc220a
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/qcom/vcodec/firmware.c
>>>> @@ -0,0 +1,147 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights
>>>> reserved.
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/dma-mapping.h>
>>>> +#include <linux/firmware.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/iommu.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/firmware/qcom/qcom_scm.h>
>>>> +#include <linux/of_reserved_mem.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/soc/qcom/mdt_loader.h>
>>>> +
>>>> +#include "firmware.h"
>>>> +
>>>> +bool use_tz(struct device *core_dev)
>>>
>>> All these functions must get some sane prefix. Otherwise a generic 'use_tz'
>>> function is too polluting for the global namespace.
>>>
>> I understand, will check and do the needful.
>>>> +{
>>>> + struct device_node *np;
>>>> +
>>>> + np = of_get_child_by_name(core_dev->of_node, "video-firmware");
>>>> + if (!np)
>>>> + return true;
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> +int protect_secure_region(u32 cp_start, u32 cp_size, u32 cp_nonpixel_start,
>>>> + u32 cp_nonpixel_size, u32 pas_id)
>>>> +{
>>>> + int ret;
>>>> + /*
>>>> + * Clues for porting using downstream data:
>>>> + * cp_start = 0
>>>> + * cp_size = venus_ns/virtual-addr-pool[0] - yes, address and not size!
>>>> + * This works, as the non-secure context bank is placed
>>>> + * contiguously right after the Content Protection region.
>>>> + *
>>>> + * cp_nonpixel_start = venus_sec_non_pixel/virtual-addr-pool[0]
>>>> + * cp_nonpixel_size = venus_sec_non_pixel/virtual-addr-pool[1]
>>>> + */
>>>> + ret = qcom_scm_mem_protect_video_var(cp_start,
>>>> + cp_size,
>>>> + cp_nonpixel_start,
>>>> + cp_nonpixel_size);
>>>> + if (ret)
>>>> + qcom_scm_pas_shutdown(pas_id);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int load_fw(struct device *dev, const char *fw_name, phys_addr_t *mem_phys,
>>>> + size_t *mem_size, u32 pas_id, bool use_tz)
>>>> +{
>>>> + const struct firmware *firmware = NULL;
>>>> + struct reserved_mem *rmem;
>>>> + struct device_node *node;
>>>> + void *mem_virt = NULL;
>>>> + ssize_t fw_size = 0;
>>>> + int ret;
>>>> +
>>>> + if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) ||
>>>
>>> Why? Can you just depend on it?
>>>
>> Sure, Will check this and get back.
>>>> + (use_tz && !qcom_scm_is_available()))
>>>> + return -EPROBE_DEFER;
>>>> +
>>>> + if (!fw_name || !(*fw_name))
>>>> + return -EINVAL;
>>>> +
>>>> + *mem_phys = 0;
>>>> + *mem_size = 0;
>>>> +
>>>> + node = of_parse_phandle(dev->of_node, "memory-region", 0);
>>>> + if (!node) {
>>>> + dev_err(dev, "no memory-region specified\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + rmem = of_reserved_mem_lookup(node);
>>>> + of_node_put(node);
>>>> + if (!rmem) {
>>>> + dev_err(dev, "failed to lookup reserved memory-region\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = request_firmware(&firmware, fw_name, dev);
>>>> + if (ret) {
>>>> + dev_err(dev, "%s: failed to request fw \"%s\", error %d\n",
>>>> + __func__, fw_name, ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + fw_size = qcom_mdt_get_size(firmware);
>>>> + if (fw_size < 0) {
>>>> + ret = fw_size;
>>>> + dev_err(dev, "%s: out of bound fw image fw size: %ld\n",
>>>> + __func__, fw_size);
>>>> + goto err_release_fw;
>>>> + }
>>>> +
>>>> + *mem_phys = rmem->base;
>>>> + *mem_size = rmem->size;
>>>> +
>>>> + if (*mem_size < fw_size) {
>>>> + ret = -EINVAL;
>>>> + goto err_release_fw;
>>>> + }
>>>> +
>>>> + mem_virt = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
>>>> + if (!mem_virt) {
>>>> + dev_err(dev, "unable to remap fw memory region %pa size %#zx\n",
>>>> + mem_phys, *mem_size);
>>>> + goto err_release_fw;
>>>> + }
>>>> +
>>>> + if (use_tz)
>>>> + ret = qcom_mdt_load(dev, firmware, fw_name, pas_id, mem_virt,
>>>> + *mem_phys, *mem_size, NULL);
>>>> + else
>>>> + ret = qcom_mdt_load_no_init(dev, firmware, fw_name, pas_id,
>>>> mem_virt,
>>>> + *mem_phys, *mem_size, NULL);
>>>> + if (ret) {
>>>> + dev_err(dev, "%s: error %d loading fw \"%s\"\n",
>>>> + __func__, ret, fw_name);
>>>> + }
>>>> +
>>>> + memunmap(mem_virt);
>>>> +err_release_fw:
>>>> + release_firmware(firmware);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int auth_reset_fw(u32 pas_id)
>>>> +{
>>>> + return qcom_scm_pas_auth_and_reset(pas_id);
>>>> +}
>>>> +
>>>> +void unload_fw(u32 pas_id)
>>>> +{
>>>> + qcom_scm_pas_shutdown(pas_id);
>>>> +}
>>>> +
>>>> +int set_hw_state(bool resume)
>>>> +{
>>>> + return qcom_scm_set_remote_state(resume, 0);
>>>> +}
>>>> diff --git a/drivers/media/platform/qcom/vcodec/firmware.h
>>>> b/drivers/media/platform/qcom/vcodec/firmware.h
>>>> new file mode 100644
>>>> index 0000000..7d410a8
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/qcom/vcodec/firmware.h
>>>> @@ -0,0 +1,21 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights
>>>> reserved.
>>>> + */
>>>> +
>>>> +#ifndef _FIRMWARE_H_
>>>> +#define _FIRMWARE_H_
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +bool use_tz(struct device *core_dev);
>>>> +int load_fw(struct device *dev, const char *fw_name, phys_addr_t *mem_phys,
>>>> + size_t *mem_size, u32 pas_id, bool use_tz);
>>>> +int auth_reset_fw(u32 pas_id);
>>>> +int protect_secure_region(u32 cp_start, u32 cp_size, u32 cp_nonpixel_start,
>>>> + u32 cp_nonpixel_size, u32 pas_id);
>>>> +void unload_fw(u32 pas_id);
>>>> +int set_hw_state(bool resume);
>>>> +
>>>> +#endif
>>>> diff --git a/drivers/media/platform/qcom/venus/Kconfig
>>>> b/drivers/media/platform/qcom/vcodec/venus/Kconfig
>>>> similarity index 100%
>>>> rename from drivers/media/platform/qcom/venus/Kconfig
>>>> rename to drivers/media/platform/qcom/vcodec/venus/Kconfig
>>>> diff --git a/drivers/media/platform/qcom/venus/Makefile
>>>> b/drivers/media/platform/qcom/vcodec/venus/Makefile
>>>> similarity index 83%
>>>> rename from drivers/media/platform/qcom/venus/Makefile
>>>> rename to drivers/media/platform/qcom/vcodec/venus/Makefile
>>>> index 91ee6be..f6f3a88 100644
>>>> --- a/drivers/media/platform/qcom/venus/Makefile
>>>> +++ b/drivers/media/platform/qcom/vcodec/venus/Makefile
>>>> @@ -1,7 +1,9 @@
>>>> # SPDX-License-Identifier: GPL-2.0
>>>> # Makefile for Qualcomm Venus driver
>>>> -venus-core-objs += core.o helpers.o firmware.o \
>>>> +venus-core-objs += ../firmware.o
>>>> +
>>>> +venus-core-objs += core.o helpers.o firmware_no_tz.o \
>>>> hfi_venus.o hfi_msgs.o hfi_cmds.o hfi.o \
>>>> hfi_parser.o pm_helpers.o dbgfs.o \
>>>> hfi_platform.o hfi_platform_v4.o \
>>>> diff --git a/drivers/media/platform/qcom/venus/core.c
>>>> b/drivers/media/platform/qcom/vcodec/venus/core.c
>>>> similarity index 91%
>>>> rename from drivers/media/platform/qcom/venus/core.c
>>>> rename to drivers/media/platform/qcom/vcodec/venus/core.c
>>>> index 9cffe97..56d9a53 100644
>>>> --- a/drivers/media/platform/qcom/venus/core.c
>>>> +++ b/drivers/media/platform/qcom/vcodec/venus/core.c
>>>> @@ -22,7 +22,8 @@
>>>> #include <media/v4l2-ioctl.h>
>>>> #include "core.h"
>>>> -#include "firmware.h"
>>>> +#include "../firmware.h"
>>>> +#include "firmware_no_tz.h"
>>>> #include "pm_helpers.h"
>>>> #include "hfi_venus_io.h"
>>>> @@ -86,6 +87,8 @@ static void venus_sys_error_handler(struct
>>>> work_struct *work)
>>>> struct venus_core *core =
>>>> container_of(work, struct venus_core, work.work);
>>>> int ret, i, max_attempts = RPM_WAIT_FOR_IDLE_MAX_ATTEMPTS;
>>>> + const struct venus_resources *res = core->res;
>>>> + const char *fwpath = NULL;
>>>> const char *err_msg = "";
>>>> bool failed = false;
>>>> @@ -107,7 +110,10 @@ static void venus_sys_error_handler(struct
>>>> work_struct *work)
>>>> mutex_lock(&core->lock);
>>>> - venus_shutdown(core);
>>>> + if (core->use_tz)
>>>> + unload_fw(VENUS_PAS_ID);
>>>> + else
>>>> + unload_fw_no_tz(core);
>>>
>>> This is more than introducing helpers.
>>>
>> The new helpers are written to make the code generic for video drivers.
>> which requires changes in the calling function also.
>>>> venus_coredump(core);
>>>> @@ -127,12 +133,39 @@ static void venus_sys_error_handler(struct
>>>> work_struct *work)
>>>> failed = true;
>>>> }
>>>> - ret = venus_boot(core);
>>>> + ret = of_property_read_string_index(core->dev->of_node,
>>>> "firmware-name", 0,
>>>> + &fwpath);
>>>> + if (ret)
>>>> + fwpath = core->res->fwname;
>>>> +
>>>> + ret = load_fw(core->dev, fwpath, &core->fw.mem_phys,
>>>> &core->fw.mem_size,
>>>> + VENUS_PAS_ID, core->use_tz);
>>>
>>> So, we had a nice local 'venus_boot'. Instead we now have a pile of code
>>> with non-generic prefixes, etc. If you are introducing helpers, please
>>> refrain from inlining of calling functions, etc. Just move the code to your
>>> helpers.
>>>
>> As mentioned in above comment, the common helpers are written to make the
>> code generic. I Will try to make it more clear, working on the same.
>
> First, you move the code, then you make it generic. Or vice versa.
> First you split the code, then you move it. Don't do both in the same
> patch.
>
>>> NAK for the rest of the patch.
>
>
Powered by blists - more mailing lists