linux-kernel - Re: [PATCH] 3/3 maple: update bus driver to support Dreamcast VMU

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: <20080324195359.GJ2899@logfs.org>
Date:	Mon, 24 Mar 2008 20:54:00 +0100
From:	Jörn Engel <joern@...fs.org>
To:	Adrian McMenamin <adrian@...golddream.dyndns.info>
Cc:	Paul Mundt <lethal@...ux-sh.org>, Greg KH <greg@...ah.com>,
	dwmw2 <dwmw2@...radead.org>, LKML <linux-kernel@...r.kernel.org>,
	MTD <linux-mtd@...ts.infradead.org>,
	linux-sh <linux-sh@...r.kernel.org>,
	Andrew Morton <akpm@...l.org>
Subject: Re: [PATCH] 3/3 maple: update bus driver to support Dreamcast VMU

On Mon, 24 March 2008 17:55:51 +0000, Adrian McMenamin wrote:
> 
> The problem is that the hardware does not support another callback. In
> any case while you are right that the function might be called as second
> time, in the original code it will sleep while waiting for the lock,
> which is allocated per device.
> 
> On the second patch - aiui completions do an uninterruptible wait, that
> means they are surely not a good choice for this - especially as the
> device could be unplugged at any time. (Admittedly my documentation
> migght be of date here)

Neither of those two problem should be visible for the mtd driver.  The
usual way how bus driver are implemented is something like this:

struct foo_request {
	void *data;
	loff_t ofs;
	size_t len;
	void (*complete)(struct foo_request);
	void *private;
};

int do_request(struct foo_request *rq)
{
	/* deal with hardware somehow */
	rq->complete(rq);
}

With such infrastructure your mtd driver could look roughly like this:

static void read_complete(struct foo_request *rq)
{
	complete(rq->private);
}

static int foo_read(struct mtd_info *mtd, loff_t ofs, size_t len, size_t *retlen, u_char *buf)
{
	struct foo_dev = mtd->private;
	struct foo_request *rq;
	DECLARE_COMPLETION_ONSTACK(complete);
	int err;

	rq = kmalloc(sizeof(*rq), GFP_KERNEL);
	if (!rq)
		return -ENOMEM;
	/* fill in appropriate values for rq->data, rq->ofs and rq->len */
	rq->private = &complete;
	rq->complete = read_complete;
	err = do_request(rq);
	if (err)
		goto out;
	wait_for_completion(&complete);
	/* copy read data into buf, set retlen */
out:
	kfree(rq);
	return err;
}

Throw in a loop if you need several hardware accesses to fulfil large
reads.  But that's it.  You don't need any locking in the mtd driver at
all.  Ownership is clearly defined.  Between kmalloc() and do_request()
rq belongs to foo_read().  Between do_request() and the call to
read_complete() it belongs to the bus driver.  After completion it
belongs to foo_read() again.  No race window, no complicated locking.

If your hardware can only handle a finite number of requests at a time,
you can queue requests in do_request().  The caller should not know or
care about it - apart from declaring the completion variable and waiting
on it.

The goal of my patches was to move your code towards such a design.
Alas, my time is limited and my girlfriend already unhappy as it is -
you will have to continue without me.  If in doubt, take a look at the
usb code and try to mimic that.  drivers/mtd/nand/alauda.c uses usb to
talk to the hardware and roughly follows what I described above.

Jörn

-- 
Chance favors only the prepared mind.
-- Louis Pasteur
--
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/