| 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: <3da78776-f126-8815-e397-c5090c84d83a@c-s.fr>
Date: Fri, 24 Apr 2020 09:21:10 +0200
From: Christophe Leroy <christophe.leroy@....fr>
To: 王文虎 <wenhu.wang@...o.com>
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
Le 24/04/2020 à 09:05, 王文虎 a écrit :
>> Le 24/04/2020 à 04:45, Wang Wenhu a écrit :
>>> New module which registers its memory allocation and free APIs to the
>>> sram_dynamic module, which would create a device of struct sram_device
>>> type to act as an interface for user level applications to access the
>>> backend hardware device, fsl_85xx_cache_sram, which is drived by the
>>> FSL_85XX_CACHE_SRAM module.
>>>
>>> Signed-off-by: Wang Wenhu <wenhu.wang@...o.com>
>>> Cc: Christophe Leroy <christophe.leroy@....fr>
>>> Cc: Scott Wood <oss@...error.net>
>>> Cc: Michael Ellerman <mpe@...erman.id.au>
>>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>>> Cc: Arnd Bergmann <arnd@...db.de>
>>> Cc: linuxppc-dev@...ts.ozlabs.org
>>> ---
>>> .../powerpc/include/asm/fsl_85xx_cache_sram.h | 4 ++
>>> arch/powerpc/platforms/85xx/Kconfig | 10 +++++
>>> arch/powerpc/sysdev/Makefile | 1 +
>>> arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h | 6 +++
>>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 12 ++++++
>>> arch/powerpc/sysdev/fsl_85xx_sram_uapi.c | 39 +++++++++++++++++++
>>> 6 files changed, 72 insertions(+)
>>> 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.
> 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,
>
>>> +
>>> 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
> 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
Powered by blists - more mailing lists