linux-kernel - Re: [PATCH V3] mtd: Add DiskOnChip G3 support

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, 20 Sep 2011 17:44:27 +0200
From:	Robert Jarzmik <robert.jarzmik@...e.fr>
To:	dedekind1@...il.com
Cc:	dwmw2@...radead.org, linux-mtd@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3] mtd: Add DiskOnChip G3 support

Artem Bityutskiy <dedekind1@...il.com> writes:

> I really feel unsure about merging this driver because no one reviewed
> it. On the surface it does look neat, though. Could you please CC lkml
> on next submission?
OK, when we have covered your comments, I'll report a V4 to lkml as well.

> On Mon, 2011-09-19 at 19:43 +0200, Robert Jarzmik wrote:
>> +static void doc_delay(struct docg3 *docg3, int nbNOPs)
>> +{
>> +	int i;
>> +
>> +	doc_dbg("NOP x %d\n", nbNOPs);
>> +	for (i = 0; i < nbNOPs; i++)
>> +		doc_writeb(0, DoC_NOP);
>> +}
>
> Why you implement dalaying this way, instead of using udelay/mdelay?
That's from observation, as I have no specification available.

>From my understanding, the clock applied to the chip can be variable, but the
memory bus writes ensure the necessary time, as the NOP write takes as much time
as the DOCG3 decides it to last.

Unless you have a timing to provide (or even better, a specification), I'll
leave the NOP writes.

>> +static int doc_wait_ready(struct docg3 *docg3)
>> +{
>> +	int maxWaitCycles = 100;
>> +
>> +	do {
>> +		doc_delay(docg3, 4);
>> +	} while (!doc_is_ready(docg3) && maxWaitCycles--);
>> +	doc_delay(docg3, 2);
>> +	if (maxWaitCycles > 0)
>> +		return 0;
>> +	else
>> +		return -EIO;
>> +}
>
> There are things like cpu_relax() which are used in busy-loops - did you
> look at those?
No, but I'll amend that loop with cpu_relax, that makes perfect sense.

>
>> +/*
>> + * Debug sysfs entries
>> + */
>> +#ifdef CONFIG_DEBUG_FS
>
> You do not need to use CONFIG_DEBUG_FS - debugfs makes all calls to be
> noop if it is not present.
>
> Either remove all macros or use DEBUG
OK, will remove the ifdef.

>> +#define DEBUGFS_RO_ATTR(name, show_fct) \
>> +	static int name##_open(struct inode *inode, struct file *file) \
>> +	{ return single_open(file, show_fct, inode->i_private); }      \
>> +	static const struct file_operations name##_fops = { \
>> +		.owner = THIS_MODULE, \
>> +		.open = name##_open, \
>> +		.llseek = seq_lseek, \
>> +		.read = seq_read, \
>> +		.release = single_release \
>> +	};
>
> Hmm, looks like something which should be generic, not DoC-specific.
True, but it's not available yet.
And as it's not available, and I don't think debugfs will accept such a patch, I
think I'll leave it here. And if you wish, I'll fill in a separate patch to
debugfs, and *if* it's merged, I'll remove that part then.

Cheers.

-- 
Robert
--
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/