| 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
| ||
|
Message-ID: <20190326132721.52e7ktnptgwvzeor@laureti-dev>
Date: Tue, 26 Mar 2019 14:27:23 +0100
From: Helmut Grohne <helmut.grohne@...enta.de>
To: Naga Sureshkumar Relli <naga.sureshkumar.relli@...inx.com>
CC: <bbrezillon@...nel.org>, <miquel.raynal@...tlin.com>,
<richard@....at>, <dwmw2@...radead.org>,
<computersforpeace@...il.com>, <marek.vasut@...il.com>,
<linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<michals@...inx.com>, <nagasureshkumarrelli@...il.com>
Subject: Re: [LINUX PATCH v13] rawnand: pl353: Add basic driver for arm pl353
smc nand interface
On Sat, Feb 09, 2019 at 12:07:27PM +0530, Naga Sureshkumar Relli wrote:
> +static void pl353_nfc_force_byte_access(struct nand_chip *chip,
> + bool force_8bit)
> +{
> + struct pl353_nand_controller *xnfc =
> + container_of(chip, struct pl353_nand_controller, chip);
This `xnfc' variable is never used anywhere inside this function.
> +/**
> + * pl353_nand_exec_op_cmd - Send command to NAND device
> + * @chip: Pointer to the NAND chip info structure
> + * @subop: Pointer to array of instructions
> + * Return: Always return zero
> + */
> +static int pl353_nand_exec_op_cmd(struct nand_chip *chip,
> + const struct nand_subop *subop)
> +{
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + const struct nand_op_instr *instr;
> + struct pl353_nfc_op nfc_op = {};
> + struct pl353_nand_controller *xnfc =
> + container_of(chip, struct pl353_nand_controller, chip);
> + unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> + unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
> + unsigned int op_id, len, offset;
> + bool reading;
> +
> + pl353_nfc_parse_instructions(chip, subop, &nfc_op);
> + instr = nfc_op.data_instr;
> + op_id = nfc_op.data_instr_idx;
> +
> + offset = nand_subop_get_data_start_off(subop, op_id);
This `offset' variable is never used anywhere inside this function. The
call is unnecessary and should be removed.
Beyond being useless, it also is harmful. When applying this patch on
top of a v5.1-rc2, this can be found in dmesg:
| ------------[ cut here ]------------
| WARNING: CPU: 0 PID: 1 at .../linux/drivers/mtd/nand/raw/nand_base.c:2299 nand_subop_get_data_start_off+0x30/0x6c
| CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc2-dirty #3
| Hardware name: Xilinx Zynq Platform
| [<c001743c>] (unwind_backtrace) from [<c00145a4>] (show_stack+0x18/0x1c)
| [<c00145a4>] (show_stack) from [<c03afe98>] (dump_stack+0xa0/0xcc)
| [<c03afe98>] (dump_stack) from [<c0021c10>] (__warn+0x10c/0x128)
| [<c0021c10>] (__warn) from [<c0021d14>] (warn_slowpath_null+0x48/0x50)
| [<c0021d14>] (warn_slowpath_null) from [<c02703f0>] (nand_subop_get_data_start_off+0x30/0x6c)
| [<c02703f0>] (nand_subop_get_data_start_off) from [<c0279fe0>] (pl353_nand_exec_op_cmd+0x94/0x2f0)
| [<c0279fe0>] (pl353_nand_exec_op_cmd) from [<c027025c>] (nand_op_parser_exec_op+0x460/0x4cc)
| [<c027025c>] (nand_op_parser_exec_op) from [<c026ee4c>] (nand_reset_op+0x134/0x1a0)
| [<c026ee4c>] (nand_reset_op) from [<c0270adc>] (nand_reset+0x60/0xbc)
| [<c0270adc>] (nand_reset) from [<c0272410>] (nand_scan_with_ids+0x288/0x1600)
| [<c0272410>] (nand_scan_with_ids) from [<c027974c>] (pl353_nand_probe+0xf8/0x1a0)
| [<c027974c>] (pl353_nand_probe) from [<c025185c>] (platform_drv_probe+0x3c/0x74)
| [<c025185c>] (platform_drv_probe) from [<c024fd28>] (really_probe+0x278/0x400)
| [<c024fd28>] (really_probe) from [<c024e440>] (bus_for_each_drv+0x68/0x9c)
| [<c024e440>] (bus_for_each_drv) from [<c0250090>] (__device_attach+0xa8/0x11c)
| [<c0250090>] (__device_attach) from [<c024e63c>] (bus_probe_device+0x90/0x98)
| [<c024e63c>] (bus_probe_device) from [<c024cf7c>] (device_add+0x3b4/0x5f0)
| [<c024cf7c>] (device_add) from [<c02b91b8>] (of_platform_device_create_pdata+0x98/0xc8)
| [<c02b91b8>] (of_platform_device_create_pdata) from [<c02beba8>] (pl353_smc_probe+0x194/0x234)
| [<c02beba8>] (pl353_smc_probe) from [<c0223a64>] (amba_probe+0x60/0x74)
| [<c0223a64>] (amba_probe) from [<c024fd28>] (really_probe+0x278/0x400)
| [<c024fd28>] (really_probe) from [<c025062c>] (device_driver_attach+0x60/0x68)
| [<c025062c>] (device_driver_attach) from [<c02506bc>] (__driver_attach+0x88/0x180)
| [<c02506bc>] (__driver_attach) from [<c024df98>] (bus_for_each_dev+0x60/0x9c)
| [<c024df98>] (bus_for_each_dev) from [<c024e894>] (bus_add_driver+0x10c/0x208)
| [<c024e894>] (bus_add_driver) from [<c0250dcc>] (driver_register+0x80/0x114)
| [<c0250dcc>] (driver_register) from [<c04e2194>] (do_one_initcall+0x164/0x374)
| [<c04e2194>] (do_one_initcall) from [<c04e2738>] (kernel_init_freeable+0x394/0x474)
| [<c04e2738>] (kernel_init_freeable) from [<c03c9660>] (kernel_init+0x14/0x100)
| [<c03c9660>] (kernel_init) from [<c00090ac>] (ret_from_fork+0x14/0x28)
| Exception stack(0xdd8c9fb0 to 0xdd8c9ff8)
| 9fa0: 00000000 00000000 00000000 00000000
| 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
| 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
| irq event stamp: 414355
| hardirqs last enabled at (414361): [<c007c318>] console_unlock+0x4c4/0x690
| hardirqs last disabled at (414366): [<c007bf30>] console_unlock+0xdc/0x690
| softirqs last enabled at (414350): [<c000a4cc>] __do_softirq+0x454/0x544
| softirqs last disabled at (414345): [<c0027d98>] irq_exit+0x124/0x128
| ---[ end trace 3be9247df2f8dfb5 ]---
After removing the call (and the variable), this particular problem goes away.
> +/**
> + * pl353_nand_probe - Probe method for the NAND driver
> + * @pdev: Pointer to the platform_device structure
> + *
> + * This function initializes the driver data structures and the hardware.
> + * The NAND driver has dependency with the pl353_smc memory controller
> + * driver for initializing the NAND timing parameters, bus width, ECC modes,
> + * control and status information.
> + *
> + * Return: 0 on success or error value on failure
> + */
> +static int pl353_nand_probe(struct platform_device *pdev)
> +{
> + struct pl353_nand_controller *xnfc;
> + struct mtd_info *mtd;
> + struct nand_chip *chip;
> + struct resource *res;
> + struct device_node *np, *dn;
> + u32 ret, val;
This `val' variable is never used anywhere inside this function.
Even after fixing these, I couldn't make this driver work on actual
hardware.
| nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
| nand: Micron MT29F2G08ABAEAWP
| nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
| nand: WARNING: MT29F2G08ABAEAWP: the ECC used on your system is too weak compared to the one required by the NAND chip
| pl353_nand_calculate_hwecc status failed
| pl353_nand_calculate_hwecc status failed
| pl353_nand_calculate_hwecc status failed
| pl353_nand_calculate_hwecc status failed
| Bad block table not found for chip 0
| pl353_nand_calculate_hwecc status failed
| pl353_nand_calculate_hwecc status failed
| pl353_nand_calculate_hwecc status failed
| pl353_nand_calculate_hwecc status failed
| Bad block table not found for chip 0
The very same device works fine with an older version of the out-of-tree
driver based on a v4.14 tree. Thus far I couldn't figure out why it
fails like this.
I'd appreciate if you could Cc me on future postings of this driver.
Helmut
Powered by blists - more mailing lists