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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <4bd765590e3e4a5da2cf79be921bac5b@sphcmbx02.sunplus.com.tw>
Date:   Mon, 6 Dec 2021 03:50:03 +0000
From:   Tony Huang 黃懷厚 <tony.huang@...plus.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Tony Huang <tonyhuang.sunplus@...il.com>
CC:     "derek.kiernan@...inx.com" <derek.kiernan@...inx.com>,
        "dragan.cvetic@...inx.com" <dragan.cvetic@...inx.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Wells Lu 呂芳騰 <wells.lu@...plus.com>
Subject: RE: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

Dear Greg KH:

> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.org>
> Sent: Friday, December 3, 2021 6:39 PM
> To: Tony Huang <tonyhuang.sunplus@...il.com>
> Cc: derek.kiernan@...inx.com; dragan.cvetic@...inx.com; arnd@...db.de;
> linux-kernel@...r.kernel.org; robh+dt@...nel.org; devicetree@...r.kernel.org;
> Wells Lu 呂芳騰 <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 03, 2021 at 11:48:45AM +0800, Tony Huang wrote:
> > +#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];
> 
> Please make your local variables static so that yhou do not polute the kernel's
> global symbol table.
> 

I will modify it.

> > +/*
> > +---------------------------------------------------------------------
> > +------------------------- */ resource_size_t SP_IOP_RESERVE_BASE;
> > +resource_size_t SP_IOP_RESERVE_SIZE;
> > +/*
> > +---------------------------------------------------------------------
> > +------------------------- */
> 
> Again, static.
> 
> And why the odd comment lines?
> 

I will remove it.

> And those are not good variable names.
> 
> > +struct sp_iop {
> > +	struct miscdevice dev;			// iop device
> > +	struct mutex write_lock;
> > +	void __iomem *iop_regs;
> > +	void __iomem *pmc_regs;
> > +	void __iomem *moon0_regs;
> > +	int irq;
> > +};
> >
> +/**************************************************************
> ***
> > + *						  G L O B A L	 D A T A
> > +
> ****************************************************************
> **/
> 
> Global where?  What about the ones above?  :)
> 

I will remove it.

> > +static struct sp_iop *iop;
> 
> Why do you think you only have one device in the system?  Please do not use
> a single variable like this.  It is easy to make your driver handle an unlimited
> number of devices just as easy as to handle 1 device.
> Please do that instead and hang your device-specific data off of the correct
> data structures that the driver core gives you.
> 

I will modify it.

> > +
> > +void iop_normal_mode(void)
> 
> Did you run sparse on this code?  Please do so.
> 
> Also, no need for a .h file for a driver that only has one .c file.
> 

I need to keep sunplus_iop.h. Other files will use sp_iop_platform_driver_poweroff(void) in poweroff flow.

> thanks,
> 
> greg k-h