| 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: <AGgA2gDdCHG-zKbHjo6AX4qd.3.1587715104260.Hmail.wenhu.wang@vivo.com>
Date: Fri, 24 Apr 2020 15:58:24 +0800 (GMT+08:00)
From: 王文虎 <wenhu.wang@...o.com>
To: Christophe Leroy <christophe.leroy@....fr>
Cc: gregkh@...uxfoundation.org, arnd@...db.de,
linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
oss@...error.net, kernel@...o.com, robh@...nel.org,
benh@...nel.crashing.org, paulus@...ba.org,
Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH v3,5/5] powerpc: sysdev: support userspace access of fsl_85xx_sram
>>>> create mode 100644 arch/powerpc/sysdev/fsl_85xx_sram_uapi.c
>>>
>>> We shouldn't add more stuff in arch/powerpc/sysdev/
>>>
>>> Either it is dedicated to 85xx, and it should go into
>>> arch/powerpc/platform/85xx/ , or it is common to several
>>> platforms/architectures and should be moved to drivers/soc/fsl/
>>>
>>
>> Sure, actually I tried to find a better place, but did not recognize
>> the driver/soc. Thanks, and I will put fsl_85xx_sram_uapi there.
>>
>>>>
>>>> diff --git a/arch/powerpc/include/asm/fsl_85xx_cache_sram.h b/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
>>>> index 0235a0447baa..99cb7e202c38 100644
>>>> --- a/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
>>>> +++ b/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
>>>> @@ -26,6 +26,10 @@ struct mpc85xx_cache_sram {
>>>> unsigned int size;
>>>> rh_info_t *rh;
>>>> spinlock_t lock;
>>>> +
>>>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI
>>>> + struct device *dev;
>>>> +#endif
>>>> };
>>>>
>>>> extern void mpc85xx_cache_sram_free(void *ptr);
>>>> diff --git a/arch/powerpc/platforms/85xx/Kconfig b/arch/powerpc/platforms/85xx/Kconfig
>>>> index fa3d29dcb57e..3a6f6af973eb 100644
>>>> --- a/arch/powerpc/platforms/85xx/Kconfig
>>>> +++ b/arch/powerpc/platforms/85xx/Kconfig
>>>> @@ -16,6 +16,16 @@ if FSL_SOC_BOOKE
>>>>
>>>> if PPC32
>>>>
>>>> +config FSL_85XX_SRAM_UAPI
>>>> + tristate "Freescale MPC85xx SRAM UAPI Support"
>>>> + depends on FSL_SOC_BOOKE && SRAM_DYNAMIC
>>>
>>> Is SRAM_DYNAMIC usefull on its own, without a driver like this one ? Is
>>> that worth allowing tiny selection of both drivers ? Shouldn't one of
>>> them imply the other one ?
>>
>> Truely the module like this is the top level selection, and SRAM_DYNAMIC
>> should be selected by any caller modules. As SRAM_DYNAMIC may be used by
>> other drivers(in the future, but currently only us here), I think make it
>> seleted by this is better? (show below)
>>
>> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig
>> index 4df32bc4c7a6..ceeebb22f6d3 100644
>> --- a/drivers/soc/fsl/Kconfig
>> +++ b/drivers/soc/fsl/Kconfig
>> @@ -50,4 +50,16 @@ config FSL_RCPM
>> tasks associated with power management, such as wakeup source control.
>> Note that currently this driver will not support PowerPC based
>> QorIQ processor.
>> +
>> +config FSL_85XX_SRAM_UAPI
>> + tristate "Freescale MPC85xx SRAM UAPI Support"
>> + depends on FSL_SOC_BOOKE && PPC32
>> + select FSL_85XX_CACHE_SRAM
>> + select SRAM_DYNAMIC
>> + help
>> + This registers a device of struct sram_device type which would act as
>> + an interface for user level applications to access the Freescale 85xx
>> + Cache-SRAM memory dynamically, meaning allocate on demand dynamically
>> + while they are running.
>> +
>
>And then in patch 4, I'm not sure it is worth to keep SRAM_DYNAMIC as
>user selectable.
>
Maybe it could be used as a module probed dynamically or so,
Currently seems there is not need to make it selectable for users.
>> endmenu
>> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
>> index 906f1cd8af01..716e38f75735 100644
>> --- a/drivers/soc/fsl/Makefile
>> +++ b/drivers/soc/fsl/Makefile
>> @@ -10,3 +10,4 @@ obj-$(CONFIG_FSL_RCPM) += rcpm.o
>> obj-$(CONFIG_FSL_GUTS) += guts.o
>> obj-$(CONFIG_FSL_MC_DPIO) += dpio/
>> obj-$(CONFIG_DPAA2_CONSOLE) += dpaa2-console.o
>> +obj-$(CONFIG_FSL_85XX_SRAM_UAPI) += fsl_85xx_sram_uapi.o
>>
>>>>
>>>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI
>>>> +extern struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void);
>>>
>>> 'extern' keywork is meaningless here, remove it.
>>>
>>
>> I will remove it in patch v4.
>>
>>>> +#endif
>>>> +
>>>> extern int instantiate_cache_sram(struct platform_device *dev,
>>>> struct sram_parameters sram_params);
>>>> extern void remove_cache_sram(struct platform_device *dev);
>>>> diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>>>> index 3de5ac8382c0..0156ea63a3a2 100644
>>>> --- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>>>> +++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>>>> @@ -23,6 +23,14 @@
>>>>
>>>> struct mpc85xx_cache_sram *cache_sram;
>>>>
>>>> +
>>>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI
>>>> +struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void)
>>>> +{
>>>> + return cache_sram;
>>>> +}
>>>> +#endif
>>>
>>> This function is not worth the mess of an #ifdef in a .c file.
>>> cache_sram is already globaly visible, so this function should go in
>>> fsl_85xx_cache_ctlr.h as a 'static inline'
>>>
>>
>> Yes, and I will change it like this, with an extern of cache_sram.
>>
>> #define L2CR_SRAM_ZERO 0x00000000 /* L2SRAM zero size */
>> @@ -81,6 +83,15 @@ struct sram_parameters {
>> phys_addr_t sram_offset;
>> };
>>
>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI
>> +extern struct mpc85xx_cache_sram *cache_sram;
>> +
>> +static inline struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void)
>> +{
>> + return cache_sram;
>> +}
>> +#endif
>> +
>> extern int instantiate_cache_sram(struct platform_device *dev,
>>
If the module is moved to drivers/soc/fsl, fsl_85xx_cache_ctlr.h in
arch/powerpc/sysdev is not available anymore to include directly.
Is it OK to just use a extern within fsl_85xx_sram_uapi.c,
(Seems it is not prefered to user extern in c files, but I can find
any documentation in Kernel.)
Or just add a header file fsl_85xx_sram.h containing only followings:
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef __FSL_85XX_SRAM_H__
#define __FSL_85XX_SRAM_H__
extern struct mpc85xx_cache_sram *cache_sram;
static inline struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void)
{
return cache_sram;
}
#endif /* __FSL_85XX_SRAM_H__ */
>>>> +
>>>> void *mpc85xx_cache_sram_alloc(unsigned int size,
>>>> phys_addr_t *phys, unsigned int align)
>>>> {
>>>> @@ -115,6 +123,10 @@ int instantiate_cache_sram(struct platform_device *dev,
>>>> rh_attach_region(cache_sram->rh, 0, cache_sram->size);
>>>> spin_lock_init(&cache_sram->lock);
>>>>
>>>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI
>>>> + cache_sram->dev = &dev->dev;
>>>> +#endif
>>>
>>> Can we avoid the #ifdef in .c file ? (see
>>> https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation)
>>>
>>
>> Definitely, and I will change it as below in patch v4:
>>
>> + if (IS_ENABLED(CONFIG_FSL_85XX_SRAM_UAPI))
>> + cache_sram->dev = &dev->dev;
>> +
>
>This will work only if is defined all the time in the .h regardless of
>CONFIG_FSL_85XX_SRAM_UAPI. Otherwise you should have something like that
>in the .h, that you call all the time from the .c:
>
>#ifdef CONFIG_FSL_85XX_SRAM_UAPI
>static inline void set_cache_sram_dev(struct mpc85xx_cache_sram *sram,
>struct device *dev)
>{
> sram->dev = dev;
>}
>#else
>static inline void set_cache_sram_dev(struct mpc85xx_cache_sram *sram,
>struct device *dev) { }
>#endif
>
Sure, better.
>
>> dev_info(&dev->dev, "[base:0x%llx, size:0x%x] configured and loaded\n",
>>
>> Thanks, for your suggestions, as these are minor modifications,
>> I will send a new patch series v4 soon.
>>
>> Regards,
>> Wenhu
>>
>
>Christophe
Wenhu
Powered by blists - more mailing lists