linux-kernel - Re: [PATCH v2 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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAK8P3a2XW=9ch58x7Shjd1ZeFka67sxEb5ZfJy-ZNtQe-j7xVg@mail.gmail.com>
Date:   Fri, 3 Dec 2021 13:17:31 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Tony Huang <tonyhuang.sunplus@...il.com>
Cc:     Derek Kiernan <derek.kiernan@...inx.com>,
        Dragan Cvetic <dragan.cvetic@...inx.com>,
        Arnd Bergmann <arnd@...db.de>,
        gregkh <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        DTML <devicetree@...r.kernel.org>, wells.lu@...plus.com,
        Tony Huang <tony.huang@...plus.com>
Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

On Fri, Dec 3, 2021 at 4:48 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>
> ---
> Changes in v2:
>  - Addressed comments from Arnd Bergmann.
>  - Addressed comments from Greg KH.
>  - Addressed comments from kernel test robot.

This looks much better already.

> +#define NORMAL_CODE_MAX_SIZE 0X1000
> +#define STANDBY_CODE_MAX_SIZE 0x4000
> +unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE];
> +unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE];

I think these should be part of the sp_iop structure, not global variables.

> +static struct sp_iop *iop;
> +
> +void iop_normal_mode(void)
> +{
> +       struct regs_iop *p_iop_reg = (struct regs_iop *)iop->iop_regs;

local functions should generally be 'static' and take the 'struct
sp_iop' instance
pointer as the first argument.

Functions that need to be global because they are used by other drivers
(if any) should probably also be exported and have documentation
above their definition.

> +static int  get_normal_code(struct device *dev)
> +{
> +       const struct firmware *fw;
> +       static const char file[] = "normal.bin";
> +       unsigned int err, i;
> +
> +       dev_info(dev, "normal code\n");
> +       err = request_firmware(&fw, file, dev);

The file name needs to clearly identify the device to avoid conflicts
with other drivers.

> +static int sp_iop_open(struct inode *inode, struct file *pfile)
> +{
> +       return 0;
> +}
> +
> +static ssize_t sp_iop_read(struct file *pfile, char __user *ubuf,
> +                       size_t length, loff_t *offset)
> +{
> +       return 0;
> +}
> +
> +static ssize_t sp_iop_write(struct file *pfile, const char __user *ubuf,
> +       size_t length, loff_t *offset)
> +{
> +       return 0;
> +}
> +
> +static int sp_iop_release(struct inode *inode, struct file *pfile)
> +{
> +       //dev_dbg(iop->dev, "Sunplus IOP module release\n");
> +       return 0;
> +}
> +
> +static const struct file_operations sp_iop_fops = {
> +       .owner                  = THIS_MODULE,
> +       .open                   = sp_iop_open,
> +       .read                   = sp_iop_read,
> +       .write                  = sp_iop_write,
> +       .release                = sp_iop_release,
> +};

This does nothing because all the callbacks are empty. You removed the
inappropriate
user space interfaces as I asked you to, but if there is no way for
either kernel or user
space to interact with the hardware, I don't see a point in merging
the driver until
you add a new interface that is usable.

> +static int sp_iop_platform_driver_remove(struct platform_device *pdev)
> +{
> +       return 0;
> +}
> +
> +static int sp_iop_platform_driver_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +       return 0;
> +}
> +
> +static void sp_iop_platform_driver_shutdown(struct platform_device *pdev)
> +{
> +
> +}
> +
> +void sp_iop_platform_driver_poweroff(void)
> +{
> +       iop_standby_mode();
> +       iop_shutdown();
> +}

Something looks wrong here, maybe reread the documentation for runtime
power management
to find a way of putting the device into low-power mode when it is unused.

> diff --git a/drivers/misc/iop/sunplus_iop.h b/drivers/misc/iop/sunplus_iop.h
> new file mode 100644
> index 0000000..fcbfd26
> --- /dev/null
> +++ b/drivers/misc/iop/sunplus_iop.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later*/
> +
> +#ifndef __SP_IOP_H__
> +#define __SP_IOP_H__
> +#include <mach/io_map.h>

mach/io_map.h does not exist, so the driver won't compile. I don't think you
need anything else, so it should be fine to remove the #include

The rest of the header only describes the hardware itself, so I'd
suggest merging
all of it into the .c file.

      Arnd