Re: [PATCH v2] firmware: qcom_scm: Fix to allow COMPILE_TEST-ing

From: Olof Johansson
Date: Mon Jun 19 2017 - 17:05:32 EST

Next message: Bjorn Helgaas: "Re: [PATCH] pci-hyperv: Use only 16 bit integer for PCI domain"
Previous message: Kees Cook: "Re: [PATCH v5] add the option of fortified string.h functions"
In reply to: Stanimir Varbanov: "Re: [PATCH v2] firmware: qcom_scm: Fix to allow COMPILE_TEST-ing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Jun 19, 2017 at 4:25 AM, Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> wrote:
> Hi Olof,
>
> On 06/19/2017 08:35 AM, Olof Johansson wrote:
>> Hi,
>>
>> On Thu, Jun 8, 2017 at 8:23 AM, Stanimir Varbanov
>> <stanimir.varbanov@xxxxxxxxxx> wrote:
>>> Unfortunatly previous attempt to allow consumer drivers to
>>> use COMPILE_TEST option in Kconfig is not enough, because in the
>>> past the consumer drivers used 'depends on' Kconfig option but
>>> now they are using 'select' Kconfig option which means on non ARM
>>> arch'es compilation is triggered. Thus we need to move the ifdefery
>>> one level below by touching the private qcom_scm.h header.
>>>
>>> Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
>>> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
>>> ---
>>> This is second version of the patch with comments addressed:
>>> * proper identation for static inline functions
>>> * to avoid duplicating defines group them on top of the header
>>>
>>> The first version has been part of the venus driver patchset
>>> v8 and can be found at:
>>> https://patchwork.kernel.org/patch/9704275/
>>>
>>> drivers/firmware/Kconfig | 2 +-
>>> drivers/firmware/qcom_scm.h | 122 ++++++++++++++++++++++++++++++++++++--------
>>> include/linux/qcom_scm.h | 32 ------------
>>> 3 files changed, 101 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>> index 6e4ed5a9c6fd..480578c3691a 100644
>>> --- a/drivers/firmware/Kconfig
>>> +++ b/drivers/firmware/Kconfig
>>> @@ -204,7 +204,7 @@ config FW_CFG_SYSFS_CMDLINE
>>>
>>> config QCOM_SCM
>>> bool
>>> - depends on ARM || ARM64
>>> + depends on ARM || ARM64 || COMPILE_TEST
>>> select RESET_CONTROLLER
>>>
>>> config QCOM_SCM_32
>>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>>> index 9bea691f30fb..60689fc8a567 100644
>>> --- a/drivers/firmware/qcom_scm.h
>>> +++ b/drivers/firmware/qcom_scm.h
>>> @@ -16,31 +16,20 @@
>>> #define QCOM_SCM_BOOT_ADDR 0x1
>>> #define QCOM_SCM_BOOT_ADDR_MC 0x11
>>> #define QCOM_SCM_SET_REMOTE_STATE 0xa
>>> -extern int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id);
>>>
>>> #define QCOM_SCM_FLAG_HLOS 0x01
>>> #define QCOM_SCM_FLAG_COLDBOOT_MC 0x02
>>> #define QCOM_SCM_FLAG_WARMBOOT_MC 0x04
>>> -extern int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> - const cpumask_t *cpus);
>>> -extern int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
>>>
>>> #define QCOM_SCM_CMD_TERMINATE_PC 0x2
>>> #define QCOM_SCM_FLUSH_FLAG_MASK 0x3
>>> #define QCOM_SCM_CMD_CORE_HOTPLUGGED 0x10
>>> -extern void __qcom_scm_cpu_power_down(u32 flags);
>>>
>>> #define QCOM_SCM_SVC_INFO 0x6
>>> #define QCOM_IS_CALL_AVAIL_CMD 0x1
>>> -extern int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> - u32 cmd_id);
>>>
>>> #define QCOM_SCM_SVC_HDCP 0x11
>>> #define QCOM_SCM_CMD_HDCP 0x01
>>> -extern int __qcom_scm_hdcp_req(struct device *dev,
>>> - struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);
>>> -
>>> -extern void __qcom_scm_init(void);
>>>
>>> #define QCOM_SCM_SVC_PIL 0x2
>>> #define QCOM_SCM_PAS_INIT_IMAGE_CMD 0x1
>>> @@ -49,6 +38,27 @@ extern void __qcom_scm_init(void);
>>> #define QCOM_SCM_PAS_SHUTDOWN_CMD 0x6
>>> #define QCOM_SCM_PAS_IS_SUPPORTED_CMD 0x7
>>> #define QCOM_SCM_PAS_MSS_RESET 0xa
>>> +
>>> +#define QCOM_SCM_SVC_MP 0xc
>>> +#define QCOM_SCM_RESTORE_SEC_CFG 2
>>> +
>>> +#define QCOM_SCM_IOMMU_SECURE_PTBL_SIZE 3
>>> +#define QCOM_SCM_IOMMU_SECURE_PTBL_INIT 4
>>> +
>>> +#define QCOM_SCM_SVC_HDCP 0x11
>>> +#define QCOM_SCM_CMD_HDCP 0x01
>>> +
>>> +#if IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64)
>>
>> This isn't right -- this should be IS_ENABLED(QCOM_SCM)
>
> The idea was to use the __qcom_scm_xxx functions only on ARM and ARM64
> and fallback to the empty stubs on every other architecture.
>
>>
>>> +extern int __qcom_scm_set_remote_state(struct device *dev, u32 state, u32 id);
>>> +extern int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> + const cpumask_t *cpus);
>>> +extern int __qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
>>> +extern void __qcom_scm_cpu_power_down(u32 flags);
>>> +extern int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> + u32 cmd_id);
>>> +extern int __qcom_scm_hdcp_req(struct device *dev,
>>> + struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);
>>> +extern void __qcom_scm_init(void);
>>> extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
>>> extern int __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
>>> dma_addr_t metadata_phys);
>>> @@ -57,6 +67,85 @@ extern int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>>> extern int __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
>>> extern int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
>>> extern int __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
>>> +extern int __qcom_scm_restore_sec_cfg(struct device *dev, u32 device_id,
>>> + u32 spare);
>>> +extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
>>> + size_t *size);
>>> +extern int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr,
>>> + u32 size, u32 spare);
>>> +#else /* !ARM and !ARM64 */
>>> +static inline int __qcom_scm_set_remote_state(struct device *dev, u32 state,
>>> + u32 id)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_set_warm_boot_addr(struct device *dev, void *entry,
>>> + const cpumask_t *cpus)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_set_cold_boot_addr(void *entry,
>>> + const cpumask_t *cpus)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline void __qcom_scm_cpu_power_down(u32 flags) {}
>>> +static inline int __qcom_scm_is_call_available(struct device *dev, u32 svc_id,
>>> + u32 cmd_id)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_hdcp_req(struct device *dev,
>>> + struct qcom_scm_hdcp_req *req,
>>> + u32 req_cnt, u32 *resp)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline void __qcom_scm_init(void) {}
>>> +static inline bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral)
>>> +{
>>> + return false;
>>> +}
>>> +static inline int __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
>>> + dma_addr_t metadata_phys)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>>> + phys_addr_t addr, phys_addr_t size)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_pas_auth_and_reset(struct device *dev,
>>> + u32 peripheral)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_restore_sec_cfg(struct device *dev, u32 device_id,
>>> + u32 spare)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_iommu_secure_ptbl_size(struct device *dev,
>>> + u32 spare, size_t *size)
>>> +{
>>> + return -ENODEV;
>>> +}
>>> +static inline int __qcom_scm_iommu_secure_ptbl_init(struct device *dev,
>>> + u64 addr, u32 size,
>>> + u32 spare)
>>> +{
>>> + return -ENODEV;
>>> +}
>>
>> The amount of boilerplate random stubs here indiciates that the
>> abstraction point for this is wrong.
>
> This was the best solution I came to.
>
>>
>> Why are you looking to COMPILE_TEST a very platform-tied driver like
>> this on other architectures?
>
> In fact the aim is to make possible to COMPILE_TEST drivers which are
> using QCOM_SCM interface. So that qcom_scm driver itself doesn't need
> compile test coverage, and compile testing qcom_scm is impossible on
> non-ARM architectures.

Seems to me that the abstraction level is wrong then, and too much
details are leaking out of qcom_scm to the users.

>> It probably makes more sense to stub the driver->scm interface than
>> the internal scm interface if what you're looking for is driver
>> compile_test coverage.
>
> Actually, this is the state of qcom_scm if we don't apply the this
> patch, and it didn't help. Thinking more on that it looks like that
> adding COMPILE_TEST in 'config QCOM_SCM' is controversial.
>
> Arnd, Andy any ideas how to proceed. If this patch is not get merged
> (and I/we cannot find better solution) the video driver for qualcomm
> platforms will be rejected for 4.13.

Fixing the qcom_scm abstractions is probably the best way forward here.

-Olof

Next message: Bjorn Helgaas: "Re: [PATCH] pci-hyperv: Use only 16 bit integer for PCI domain"
Previous message: Kees Cook: "Re: [PATCH v5] add the option of fortified string.h functions"
In reply to: Stanimir Varbanov: "Re: [PATCH v2] firmware: qcom_scm: Fix to allow COMPILE_TEST-ing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]