linux-kernel - Re: [PATCH v3 2/2] misc: Add iop driver for Sunplus SP7021

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a2UGr6ZbHk6G=wh5XG_EGdJxGf6SfyN1sTb4aaUgiK8Lw@mail.gmail.com>
Date:   Thu, 9 Dec 2021 10:50:58 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Tony Huang <tonyhuang.sunplus@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        DTML <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Derek Kiernan <derek.kiernan@...inx.com>,
        Dragan Cvetic <dragan.cvetic@...inx.com>,
        Arnd Bergmann <arnd@...db.de>,
        gregkh <gregkh@...uxfoundation.org>,
        Wells Lu 呂芳騰 <wells.lu@...plus.com>,
        Tony Huang <tony.huang@...plus.com>
Subject: Re: [PATCH v3 2/2] misc: Add iop driver for Sunplus SP7021

On Thu, Dec 9, 2021 at 9:58 AM Tony Huang <tonyhuang.sunplus@...il.com> wrote:
>
> IOP (IO Processor) embedded inside SP7021 which is used as
> Processor for I/O control, RTC wake-up and cooperation with
> CPU & PMC in power management purpose.
> The IOP core is DQ8051, so also named IOP8051,
> it supports dedicated JTAG debug pins which share with SP7021.
> In standby mode operation, the power spec reach 400uA.
>
> Signed-off-by: Tony Huang <tonyhuang.sunplus@...il.com>

Thanks for the improvements, this again looks better than the previous version.
I still have some minor comments, and there are a couple of details I have
commented on before that would need to be addressed, but let's focus
on the one main issue for now:

The driver still doesn't actually /do/ anything: you load the firmware when
the driver is loaded, and you shut it down when the driver is removed, but
otherwise there is no way to interact with the iop. You had the miscdevice
earlier, and you still register that, but there are no file_operations
associated
with it, so it still doesn't have any effect.

In the original version you had a couple of user-side interfaces, for which
Greg and I commented that they were not using the correct abstractions,
and you still list them in the changelog text as "I/O control, RTC wake-up
and cooperation with CPU & PMC in power management".

If you want to make any progress with adding the driver, I'd say you
should implement at least two of those high-level interfaces that interact
with the respective kernel subsystems in order to show that the
abstraction works.

        Arnd