linux-kernel - Re: [PATCH v10 1/5] mtd: nand: vf610_nfc: Freescale NFC for VF610, MPC5125 and others

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: <bf1ed158fe08b51c4668faffde44083f@agner.ch>
Date:	Wed, 26 Aug 2015 18:02:31 -0700
From:	Stefan Agner <stefan@...er.ch>
To:	Brian Norris <computersforpeace@...il.com>
Cc:	dwmw2@...radead.org, sebastian@...akpoint.cc, robh+dt@...nel.org,
	pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	shawn.guo@...aro.org, kernel@...gutronix.de,
	boris.brezillon@...e-electrons.com, marb@...at.de,
	aaron@...tycactus.com, bpringlemeir@...il.com,
	linux-mtd@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	albert.aribaud@...ev.fr, klimov.linux@...il.com,
	Bill Pringlemeir <bpringlemeir@...ps.com>
Subject: Re: [PATCH v10 1/5] mtd: nand: vf610_nfc: Freescale NFC for VF610,
 MPC5125 and others

On 2015-08-25 13:16, Brian Norris wrote:
> A few more comments.
> 
> On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
>> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> new file mode 100644
>> index 0000000..5c8dfe8
>> --- /dev/null
>> +++ b/drivers/mtd/nand/vf610_nfc.c
>> @@ -0,0 +1,645 @@
> 
> ...
> 
>> +/*
>> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>> + */
>> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
>> +{
>> +#ifdef CONFIG_SOC_VF610
> 
> Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.
> 
> If this is trying to handle the comment above ("This function supports
> Vybrid only (MPC5125 would have full RB and four CS)") then that's the
> wrong way of doing it, as you need to support multiplatform kernels.
> You'll need to have a way to differentiate the different platform
> support at runtime, not compile time.

Yes it is trying to handle the comment above. Well, the other two
platforms I am aware of are also different architectures... (PowerPC and
ColdFire). I think we won't have a multi-architecture kernel anytime
soon, hence I think removing the code at compile time is the right thing
todo.

However, probably CONFIG_SOC_VF610 is the wrong symbol then, I could
just use CONFIG_ARM and add a comment that this might be different on
another other ARM SoC than VF610.

Just checked CodingStyle, and I see that IS_ENABLED is the preferred way
for conditional compiling.

So my suggestion:

static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
{
	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);

	if (!IS_ENABLED(CONFIG_ARM))
		return;

	/*
	 * This code is only tested on the ARM platform VF610
	 * PowerPC based MPC5125 would have full RB and four CS
	 */
....

With that the compiler should be able to remove this (currently) ARM
VF610 specific code on the other supported architectures...

What do you think?


> 
>> +	struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> +	u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
>> +
>> +	tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
>> +	tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
>> +
>> +	if (chip == 0)
>> +		tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
>> +	else if (chip == 1)
>> +		tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;
> 
> 	else ... ?
> 
> Maybe you can write this as a formulaic pattern (e.g.:
> 
> 	tmp |= (chip + 1) << ROW_ADDR_CHIP_SEL_SHIFT;
> 
> ) and just do the "max # of chips" checks on a per-platform basis in the
> probe(). Then I'm guessing this same function can apply to both
> platforms. (I'm not looking at HW datasheets for this, BTW, just
> guessing based on the context here.)

It seems that MCP5125 is different than VF610. MCP5125 has 4 chip
selects and 4 R/B signals, whereas VF610 has only 2 chip selects and
just 1 R/B signals...

> But wait...I see that you call nand_scan_ident() with a max of 1 chip.
> So you won't ever see the chip > 0 case, right?
> 
> So does this driver support multiple flash attached or not? Looks like
> you're assuming you'll only be using chip-select 0. (This is fine for
> now, but at least your code should acknowledge this. Perhaps a comment
> at the top under "limitations.")
> 

Ok, will add that information under limitations.


>> +
>> +	vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
>> +#endif
>> +}
> 
> ...
> 
>> +static int vf610_nfc_probe(struct platform_device *pdev)
>> +{
> 
> ...
> 
>> +	/* first scan to find the device and get the page size */
>> +	if (nand_scan_ident(mtd, 1, NULL)) {
>> +		err = -ENXIO;
>> +		goto error;
>> +	}

--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/