[v2,1/7] arm: firmware: Check firmware is running or not

Commit Message

Bartlomiej Zolnierkiewicz June 2, 2014, 12:35 p.m. UTC

From: Kyungmin Park <[e-mail]>

To support multi-platform, it needs to know it's running under secure
OS or not.  Sometimes it needs to access physical address by SMC calls.

e.g.,
        if (firmware_run()) {
                addr = physical address;
        } else {
                addr = virtual address;
        }

        call_firmware_ops(read_address, addr, &value);

Signed-off-by: Kyungmin Park <[e-mail]>
Signed-off-by: Bartlomiej Zolnierkiewicz <[e-mail]>
---
 arch/arm/common/firmware.c      | 5 +++++
 arch/arm/include/asm/firmware.h | 3 +++
 2 files changed, 8 insertions(+)

Comments

Tomasz Figa June 2, 2014, 12:51 p.m. UTC | #1

Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> From: Kyungmin Park <[e-mail]>
> 
> To support multi-platform, it needs to know it's running under secure
> OS or not.  Sometimes it needs to access physical address by SMC calls.
> 
> e.g.,
>         if (firmware_run()) {
>                 addr = physical address;
>         } else {
>                 addr = virtual address;
>         }
> 
>         call_firmware_ops(read_address, addr, &value);

Hmm, I don't understand the code above. It first asks whether the
firmware is available and then calls a firmware operation anyway
(assuming that firmware is available regardless of the check above)...

I don't like the idea of this function, because we have designed the
firmware API to not require this kind of checks. Instead, you just call
whatever firmware operation you need and if it returns -ENOSYS you need
to fallback to legacy (firmware-less) way of doing it.

Could you provide your use case for which this doesn't work?

Best regards,
Tomasz

Bartlomiej Zolnierkiewicz June 2, 2014, 1:05 p.m. UTC | #2

Hi,

On Monday, June 02, 2014 02:51:14 PM Tomasz Figa wrote:
> Hi,
> 
> On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> > From: Kyungmin Park <[e-mail]>
> > 
> > To support multi-platform, it needs to know it's running under secure
> > OS or not.  Sometimes it needs to access physical address by SMC calls.
> > 
> > e.g.,
> >         if (firmware_run()) {
> >                 addr = physical address;
> >         } else {
> >                 addr = virtual address;
> >         }
> > 
> >         call_firmware_ops(read_address, addr, &value);
> 
> Hmm, I don't understand the code above. It first asks whether the
> firmware is available and then calls a firmware operation anyway
> (assuming that firmware is available regardless of the check above)...
> 
> I don't like the idea of this function, because we have designed the
> firmware API to not require this kind of checks. Instead, you just call
> whatever firmware operation you need and if it returns -ENOSYS you need
> to fallback to legacy (firmware-less) way of doing it.
> 
> Could you provide your use case for which this doesn't work?

Please take a look at patch #7.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Patch

diff --git a/arch/arm/common/firmware.c b/arch/arm/common/firmware.c
index 27ddccb..e9d9ee5 100644
--- a/arch/arm/common/firmware.c
+++ b/arch/arm/common/firmware.c
@@ -16,3 +16,8 @@ 
 static const struct firmware_ops default_firmware_ops;
 
 const struct firmware_ops *firmware_ops = &default_firmware_ops;
+
+int firmware_run(void)
+{
+	return firmware_ops != &default_firmware_ops;
+}
diff --git a/arch/arm/include/asm/firmware.h b/arch/arm/include/asm/firmware.h
index 2c9f10d..c72ec47 100644
--- a/arch/arm/include/asm/firmware.h
+++ b/arch/arm/include/asm/firmware.h
@@ -46,6 +46,9 @@  struct firmware_ops {
 /* Global pointer for current firmware_ops structure, can't be NULL. */
 extern const struct firmware_ops *firmware_ops;
 
+/* Check firmware is running */
+extern int firmware_run(void);
+
 /*
  * call_firmware_op(op, ...)
  *