linux-kernel - RE: [PATCH v2 3/5] soc: visconti: Add Toshiba Visconti DNN image processing accelerator

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]
Date:   Tue, 26 Jul 2022 06:10:37 +0000
From:   <yuji2.ishikawa@...hiba.co.jp>
To:     <gregkh@...uxfoundation.org>
CC:     <robh+dt@...nel.org>, <hverkuil@...all.nl>,
        <nobuhiro1.iwamatsu@...hiba.co.jp>, <corbet@....net>,
        <sumit.semwal@...aro.org>, <christian.koenig@....com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-media@...r.kernel.org>,
        <dri-devel@...ts.freedesktop.org>, <linaro-mm-sig@...ts.linaro.org>
Subject: RE: [PATCH v2 3/5] soc: visconti: Add Toshiba Visconti DNN image
 processing accelerator

Hi Greg

Thank you for your comments.

> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.org>
> Sent: Monday, July 25, 2022 9:51 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@...hiba.co.jp>
> Cc: Rob Herring <robh+dt@...nel.org>; Hans Verkuil <hverkuil@...all.nl>;
> iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <nobuhiro1.iwamatsu@...hiba.co.jp>; Jonathan Corbet <corbet@....net>;
> Sumit Semwal <sumit.semwal@...aro.org>; Christian König
> <christian.koenig@....com>; linux-arm-kernel@...ts.infradead.org;
> linux-kernel@...r.kernel.org; linux-media@...r.kernel.org;
> dri-devel@...ts.freedesktop.org; linaro-mm-sig@...ts.linaro.org
> Subject: Re: [PATCH v2 3/5] soc: visconti: Add Toshiba Visconti DNN image
> processing accelerator
> 
> On Fri, Jul 22, 2022 at 05:28:56PM +0900, Yuji Ishikawa wrote:
> > --- /dev/null
> > +++ b/drivers/soc/visconti/uapi/dnn.h
> > @@ -0,0 +1,77 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/* Toshiba Visconti DNN Accelerator Support
> > + *
> > + * (C) Copyright 2022 TOSHIBA CORPORATION
> > + * (C) Copyright 2022 Toshiba Electronic Devices & Storage
> > +Corporation  */
> > +
> > +#ifndef _UAPI_LINUX_DNN_H
> > +#define _UAPI_LINUX_DNN_H
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +#include "ipa.h"
> > +
> > +#define DRV_DNN_BIT_CONFIG_DESC_FINAL (0x8000U)
> > +#define DRV_DNN_BUFFER_INDEX_MAX      (15)
> > +
> > +#define DRV_DNN_BASE_ADDR_NUM (8U) /* DNN number of base
> address */
> > +
> > +#define DRV_DNN_BASE_ADDR_PURPOSE_INPUT	    (1U)
> > +#define DRV_DNN_BASE_ADDR_PURPOSE_OUTPUT    (2U)
> > +#define DRV_DNN_BASE_ADDR_PURPOSE_AWB	    (3U)
> > +#define DRV_DNN_BASE_ADDR_PURPOSE_TEMPORARY (4U)
> > +
> > +/**
> > + * struct drv_dnn_status - DNN IPA status for IOC_IPA_GET_STATUS
> > + *
> > + * @state:     State of driver
> > + * @eer_cmd:   Execution error command
> > + * @eer:       Execution error
> > + * @reserved:  Padding
> > + * @eer_flags: Execution error flags
> > + */
> > +struct drv_dnn_status {
> > +	enum drv_ipa_state state;
> > +	__u32 eer_cmd;
> > +	__u32 eer : 1;
> > +	__u32 reserved : 31;
> 
> bitfields will not work like this for uapi files, sorry.

I'll change the type of the member eer from bitfield to bool.

> 
> > +	__u32 eer_flags[32];
> 
> What endian is all of these?  Big?  Little?  Unknown?

The processors and accelerators are little endian in Visconti SoC.
Do I have to use more specific type such as __le32 ?

> 
> > +};
> > +
> > +struct drv_dnn_base_addr {
> > +	__u32 purpose;
> > +	union {
> > +		struct drv_ipa_addr ipa_addr;
> > +		uintptr_t list_addr;
> 
> You really do not ever want a uintptr_t in a uapi file, that's not going to be
> portable at all.  It's also not a valid kernel type :(

I understand. The member list_addr should be typed "struct drv_ipa_addr*".

> No documentation on what "purpose" is for?

I'll add description for struct drv_dnn_base_addr with kernel-doc style.

>
> > +	} addr;
> > +};
> > +
> > +/**
> > + * struct drv_dnn_descriptor - DNN IPA Descriptor for IOC_IPA_START
> > + *
> > + * @configuration:        Address of configuration data
> > + * @configuration_offset: Configuration offset
> > + * @configuration_size:   Configuration data size
> > + * @list_num:             Number of input/output list
> > + * @base_addr:            Base addresses
> > + * @base_addr_flag:       Bit-fields of base_addr list config;
> > + *                        0 for fixed address,
> > + *                        1 for address list.
> 
> Where are the bitfield definitions?
> What about unused bits, what happens with them?  You are checking them,
> right?
>

I'll update comments for "base_addr" and "base_addr_flag".
The member base_addr[n] are handled differently
according to the n'th bit of the member base_addr_flag;
where 0 <= n < HWD_DNN_BASE_ADDR_NUM-1.

> > + * @config_done:          Flags of called configuration
> > + * @buffer_info:          Table of buffer information
> > + * @buffer_info_num:      Number of buffer_info
> > + */
> > +struct drv_dnn_descriptor {
> > +	struct drv_ipa_addr configuration;
> > +	__u32 configuration_offset;
> 
> What endian are any of these?

They are little endian as processors and accelerators are LE.
Do I have to use specific type such as __le32?

Do we need special care for endianness	when userland and kernel are sharing data (a drv_dnn_descriptor instance) ?
I thought there're no endianness problem when the driver is reading/writing HW's 32bit registers.
I understand, generally, special cares are needed for some cases like:
* network byte order --- endianness is specified in a specification.
* data blocks are stored to byte-addressable memory and read by another processing elements (HW or CPU in another system).
* HW designed for little endian is placed in big endian system.

> thanks,
> 
> greg k-h

Regards,
  Yuji