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] [day] [month] [year] [list]
Message-ID: <20170419120749.GB10395@distanz.ch>
Date:   Wed, 19 Apr 2017 14:07:49 +0200
From:   Tobias Klauser <tklauser@...tanz.ch>
To:     matthew.gerlach@...ux.intel.com
Cc:     kbuild test robot <lkp@...el.com>, kbuild-all@...org,
        Alan Tull <atull@...nel.org>,
        Moritz Fischer <moritz.fischer@...us.com>,
        linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fpga: allow to compile-test Altera FPGA bridge drivers

Hi Matthew

Thanks for the detailed analysis!

On 2017-04-18 at 17:27:15 +0200, matthew.gerlach@...ux.intel.com <matthew.gerlach@...ux.intel.com> wrote:

[...]

> >[auto build test WARNING on linus/master]
> >[also build test WARNING on v4.11-rc6 next-20170411]
> >[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> >url:    https://github.com/0day-ci/linux/commits/Tobias-Klauser/fpga-allow-to-compile-test-Altera-FPGA-bridge-drivers/20170411-181401
> >config: m32r-allmodconfig (attached as .config)
> >compiler: m32r-linux-gcc (GCC) 6.2.0
> >reproduce:
> >       wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >       chmod +x ~/bin/make.cross
> >       # save the attached .config to linux build tree
> >       make.cross ARCH=m32r
> >
> >All warnings (new ones prefixed by >>):
> >
> >  In file included from include/linux/printk.h:329:0,
> >                   from include/linux/kernel.h:13,
> >                   from include/linux/delay.h:21,
> >                   from drivers/fpga/altera-freeze-bridge.c:18:
> >  drivers/fpga/altera-freeze-bridge.c: In function 'altera_freeze_br_do_freeze':
> >>>drivers/fpga/altera-freeze-bridge.c:107:15: warning: format '%d' expects argument of type 'int', but argument 6 has type 'long unsigned int' [-Wformat=]
> >    dev_dbg(dev, "%s %d %d\n", __func__, status, readl(csr_ctrl_addr));
> >                 ^
> 
> The line above compiles cleanly for x86 and 32-bit ARM.
> Interestingly, the variable, status, is declared locally as an u32
> and is assigned by a
> call to readl(), and there is no warning.  On x86 and 32-bit arm,
> readl(), is defined as returning u32, but on this m32r processor,
> readl() returns unsigned long, thus the warning.
> 
> A possilbe fix to this warning is to declare another local u32, ctrl, and
> assign a value with a call to readl().  In other words, mimic what
> is done with the variable, status.  While this would get rid of the
> warning, I can't help but think there is either a compiler problem
> or a problem with m32r.

I'd say this is a problem with how readl() is declared on m32r. On almost
all architectures readl() returns a u32 (which seems to be unsigned int
on all architectures).

Judging from a quick m32r allmodconfig build, it seems the warning
occurs at quite a few places in the kernel already and a quick Google
search also turns up quite some patches, where the same warning occured
[1], [2], [3].

[1] https://www.spinics.net/lists/linux-ide/msg53956.html
[2] https://lists.01.org/pipermail/kbuild-all/2016-September/025251.html
[3] https://www.spinics.net/lists/linux-bluetooth/msg70076.html

IMO the proper fix would probably be to fix the readl() implementation
on m32r.

> 
> >  include/linux/dynamic_debug.h:134:39: note: in definition of macro 'dynamic_dev_dbg'
> >     __dynamic_dev_dbg(&descriptor, dev, fmt, \
> >                                         ^~~
> >>>drivers/fpga/altera-freeze-bridge.c:107:2: note: in expansion of macro 'dev_dbg'
> >    dev_dbg(dev, "%s %d %d\n", __func__, status, readl(csr_ctrl_addr));
> >    ^~~~~~~
> >  drivers/fpga/altera-freeze-bridge.c: In function 'altera_freeze_br_do_unfreeze':
> >  drivers/fpga/altera-freeze-bridge.c:144:15: warning: format '%d' expects argument of type 'int', but argument 6 has type 'long unsigned int' [-Wformat=]
> >    dev_dbg(dev, "%s %d %d\n", __func__, status, readl(csr_ctrl_addr));
> >                 ^
> >  include/linux/dynamic_debug.h:134:39: note: in definition of macro 'dynamic_dev_dbg'
> >     __dynamic_dev_dbg(&descriptor, dev, fmt, \
> >                                         ^~~
> >  drivers/fpga/altera-freeze-bridge.c:144:2: note: in expansion of macro 'dev_dbg'
> >    dev_dbg(dev, "%s %d %d\n", __func__, status, readl(csr_ctrl_addr));
> >    ^~~~~~~
> >  drivers/fpga/altera-freeze-bridge.c:162:15: warning: format '%d' expects argument of type 'int', but argument 6 has type 'long unsigned int' [-Wformat=]
> >    dev_dbg(dev, "%s %d %d\n", __func__, status, readl(csr_ctrl_addr));
> >                 ^
> >  include/linux/dynamic_debug.h:134:39: note: in definition of macro 'dynamic_dev_dbg'
> >     __dynamic_dev_dbg(&descriptor, dev, fmt, \
> >                                         ^~~
> >  drivers/fpga/altera-freeze-bridge.c:162:2: note: in expansion of macro 'dev_dbg'
> >    dev_dbg(dev, "%s %d %d\n", __func__, status, readl(csr_ctrl_addr));
> >    ^~~~~~~
> >
> >vim +107 drivers/fpga/altera-freeze-bridge.c
> >
> >ca24a648 Alan Tull 2016-11-01   12   * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> >ca24a648 Alan Tull 2016-11-01   13   * more details.
> >ca24a648 Alan Tull 2016-11-01   14   *
> >ca24a648 Alan Tull 2016-11-01   15   * You should have received a copy of the GNU General Public License along with
> >ca24a648 Alan Tull 2016-11-01   16   * this program.  If not, see <http://www.gnu.org/licenses/>.
> >ca24a648 Alan Tull 2016-11-01   17   */
> >ca24a648 Alan Tull 2016-11-01  @18  #include <linux/delay.h>
> >ca24a648 Alan Tull 2016-11-01   19  #include <linux/io.h>
> >ca24a648 Alan Tull 2016-11-01   20  #include <linux/kernel.h>
> >ca24a648 Alan Tull 2016-11-01   21  #include <linux/of_device.h>
> >ca24a648 Alan Tull 2016-11-01   22  #include <linux/module.h>
> >ca24a648 Alan Tull 2016-11-01   23  #include <linux/fpga/fpga-bridge.h>
> >ca24a648 Alan Tull 2016-11-01   24
> >ca24a648 Alan Tull 2016-11-01   25  #define FREEZE_CSR_STATUS_OFFSET		0
> >ca24a648 Alan Tull 2016-11-01   26  #define FREEZE_CSR_CTRL_OFFSET			4
> >ca24a648 Alan Tull 2016-11-01   27  #define FREEZE_CSR_ILLEGAL_REQ_OFFSET		8
> >ca24a648 Alan Tull 2016-11-01   28  #define FREEZE_CSR_REG_VERSION			12
> >ca24a648 Alan Tull 2016-11-01   29
> >ca24a648 Alan Tull 2016-11-01   30  #define FREEZE_CSR_SUPPORTED_VERSION		2
> >ca24a648 Alan Tull 2016-11-01   31
> >ca24a648 Alan Tull 2016-11-01   32  #define FREEZE_CSR_STATUS_FREEZE_REQ_DONE	BIT(0)
> >ca24a648 Alan Tull 2016-11-01   33  #define FREEZE_CSR_STATUS_UNFREEZE_REQ_DONE	BIT(1)
> >ca24a648 Alan Tull 2016-11-01   34
> >ca24a648 Alan Tull 2016-11-01   35  #define FREEZE_CSR_CTRL_FREEZE_REQ		BIT(0)
> >ca24a648 Alan Tull 2016-11-01   36  #define FREEZE_CSR_CTRL_RESET_REQ		BIT(1)
> >ca24a648 Alan Tull 2016-11-01   37  #define FREEZE_CSR_CTRL_UNFREEZE_REQ		BIT(2)
> >ca24a648 Alan Tull 2016-11-01   38
> >ca24a648 Alan Tull 2016-11-01   39  #define FREEZE_BRIDGE_NAME			"freeze"
> >ca24a648 Alan Tull 2016-11-01   40
> >ca24a648 Alan Tull 2016-11-01   41  struct altera_freeze_br_data {
> >ca24a648 Alan Tull 2016-11-01   42  	struct device *dev;
> >ca24a648 Alan Tull 2016-11-01   43  	void __iomem *base_addr;
> >ca24a648 Alan Tull 2016-11-01   44  	bool enable;
> >ca24a648 Alan Tull 2016-11-01   45  };
> >ca24a648 Alan Tull 2016-11-01   46
> >ca24a648 Alan Tull 2016-11-01   47  /*
> >ca24a648 Alan Tull 2016-11-01   48   * Poll status until status bit is set or we have a timeout.
> >ca24a648 Alan Tull 2016-11-01   49   */
> >ca24a648 Alan Tull 2016-11-01   50  static int altera_freeze_br_req_ack(struct altera_freeze_br_data *priv,
> >ca24a648 Alan Tull 2016-11-01   51  				    u32 timeout, u32 req_ack)
> >ca24a648 Alan Tull 2016-11-01   52  {
> >ca24a648 Alan Tull 2016-11-01   53  	struct device *dev = priv->dev;
> >ca24a648 Alan Tull 2016-11-01   54  	void __iomem *csr_illegal_req_addr = priv->base_addr +
> >ca24a648 Alan Tull 2016-11-01   55  					     FREEZE_CSR_ILLEGAL_REQ_OFFSET;
> >ca24a648 Alan Tull 2016-11-01   56  	u32 status, illegal, ctrl;
> >ca24a648 Alan Tull 2016-11-01   57  	int ret = -ETIMEDOUT;
> >ca24a648 Alan Tull 2016-11-01   58
> >ca24a648 Alan Tull 2016-11-01   59  	do {
> >ca24a648 Alan Tull 2016-11-01   60  		illegal = readl(csr_illegal_req_addr);
> >ca24a648 Alan Tull 2016-11-01   61  		if (illegal) {
> >ca24a648 Alan Tull 2016-11-01   62  			dev_err(dev, "illegal request detected 0x%x", illegal);
> >ca24a648 Alan Tull 2016-11-01   63
> >ca24a648 Alan Tull 2016-11-01   64  			writel(1, csr_illegal_req_addr);
> >ca24a648 Alan Tull 2016-11-01   65
> >ca24a648 Alan Tull 2016-11-01   66  			illegal = readl(csr_illegal_req_addr);
> >ca24a648 Alan Tull 2016-11-01   67  			if (illegal)
> >ca24a648 Alan Tull 2016-11-01   68  				dev_err(dev, "illegal request not cleared 0x%x",
> >ca24a648 Alan Tull 2016-11-01   69  					illegal);
> >ca24a648 Alan Tull 2016-11-01   70
> >ca24a648 Alan Tull 2016-11-01   71  			ret = -EINVAL;
> >ca24a648 Alan Tull 2016-11-01   72  			break;
> >ca24a648 Alan Tull 2016-11-01   73  		}
> >ca24a648 Alan Tull 2016-11-01   74
> >ca24a648 Alan Tull 2016-11-01   75  		status = readl(priv->base_addr + FREEZE_CSR_STATUS_OFFSET);
> >ca24a648 Alan Tull 2016-11-01   76  		dev_dbg(dev, "%s %x %x\n", __func__, status, req_ack);
> >ca24a648 Alan Tull 2016-11-01   77  		status &= req_ack;
> >ca24a648 Alan Tull 2016-11-01   78  		if (status) {
> >ca24a648 Alan Tull 2016-11-01   79  			ctrl = readl(priv->base_addr + FREEZE_CSR_CTRL_OFFSET);
> >ca24a648 Alan Tull 2016-11-01   80  			dev_dbg(dev, "%s request %x acknowledged %x %x\n",
> >ca24a648 Alan Tull 2016-11-01   81  				__func__, req_ack, status, ctrl);
> >ca24a648 Alan Tull 2016-11-01   82  			ret = 0;
> >ca24a648 Alan Tull 2016-11-01   83  			break;
> >ca24a648 Alan Tull 2016-11-01   84  		}
> >ca24a648 Alan Tull 2016-11-01   85
> >ca24a648 Alan Tull 2016-11-01   86  		udelay(1);
> >ca24a648 Alan Tull 2016-11-01   87  	} while (timeout--);
> >ca24a648 Alan Tull 2016-11-01   88
> >ca24a648 Alan Tull 2016-11-01   89  	if (ret == -ETIMEDOUT)
> >ca24a648 Alan Tull 2016-11-01   90  		dev_err(dev, "%s timeout waiting for 0x%x\n",
> >ca24a648 Alan Tull 2016-11-01   91  			__func__, req_ack);
> >ca24a648 Alan Tull 2016-11-01   92
> >ca24a648 Alan Tull 2016-11-01   93  	return ret;
> >ca24a648 Alan Tull 2016-11-01   94  }
> >ca24a648 Alan Tull 2016-11-01   95
> >ca24a648 Alan Tull 2016-11-01   96  static int altera_freeze_br_do_freeze(struct altera_freeze_br_data *priv,
> >ca24a648 Alan Tull 2016-11-01   97  				      u32 timeout)
> >ca24a648 Alan Tull 2016-11-01   98  {
> >ca24a648 Alan Tull 2016-11-01   99  	struct device *dev = priv->dev;
> >ca24a648 Alan Tull 2016-11-01  100  	void __iomem *csr_ctrl_addr = priv->base_addr +
> >ca24a648 Alan Tull 2016-11-01  101  				      FREEZE_CSR_CTRL_OFFSET;
> >ca24a648 Alan Tull 2016-11-01  102  	u32 status;
> >ca24a648 Alan Tull 2016-11-01  103  	int ret;
> >ca24a648 Alan Tull 2016-11-01  104
> >ca24a648 Alan Tull 2016-11-01  105  	status = readl(priv->base_addr + FREEZE_CSR_STATUS_OFFSET);
> >ca24a648 Alan Tull 2016-11-01  106
> >ca24a648 Alan Tull 2016-11-01 @107  	dev_dbg(dev, "%s %d %d\n", __func__, status, readl(csr_ctrl_addr));
> >ca24a648 Alan Tull 2016-11-01  108
> >ca24a648 Alan Tull 2016-11-01  109  	if (status & FREEZE_CSR_STATUS_FREEZE_REQ_DONE) {
> >ca24a648 Alan Tull 2016-11-01  110  		dev_dbg(dev, "%s bridge already disabled %d\n",
> >
> >:::::: The code at line 107 was first introduced by commit
> >:::::: ca24a648f535a02b4163ca4f4d2e51869f155a3a fpga: add altera freeze bridge support
> >
> >:::::: TO: Alan Tull <atull@...nsource.altera.com>
> >:::::: CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> >
> >---
> >0-DAY kernel test infrastructure                Open Source Technology Center
> >https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> >
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ