| 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: <4610CDB4.4050708@panasas.com>
Date: Mon, 02 Apr 2007 12:32:36 +0300
From: Boaz Harrosh <bharrosh@...asas.com>
To: Pekka J Enberg <penberg@...helsinki.fi>
CC: Jens Axboe <jens.axboe@...cle.com>,
Rene Herman <rene.herman@...il.com>,
Al Viro <viro@....linux.org.uk>,
Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: mcdx -- do_request(): non-read command to cd!!
Pekka J Enberg wrote:
> On Mon, 2 Apr 2007, Jens Axboe wrote:
>> But as I wrote earlier, it'll take lots more to make this driver close
>> to functional.
>
> Heh, looking at the driver, that's quite an understatement =). Rene,
> here's an untested attempt to use a mutex instead of the horribly broken
> waitqueue "synchronization" in mcdx. It may or may not help so give it a
> spin if you want.
>
> Pekka
>
> ---
> drivers/cdrom/mcdx.c | 121 ++++++++++++++++++---------------------------------
> 1 file changed, 44 insertions(+), 77 deletions(-)
>
> Index: 2.6/drivers/cdrom/mcdx.c
> ===================================================================
> --- 2.6.orig/drivers/cdrom/mcdx.c 2007-04-02 11:50:40.000000000 +0300
> +++ 2.6/drivers/cdrom/mcdx.c 2007-04-02 11:51:20.000000000 +0300
> @@ -58,6 +58,7 @@ = "$Id: mcdx.c,v 1.21 1997/01/26 07:
>
> #include <linux/module.h>
>
> +#include <linux/delay.h>
> #include <linux/errno.h>
> #include <linux/interrupt.h>
> #include <linux/fs.h>
> @@ -151,15 +152,14 @@ struct s_version {
> /* Per drive/controller stuff **************************************/
>
> struct s_drive_stuff {
> + struct mutex mutex;
> +
> /* waitqueues */
> wait_queue_head_t busyq;
> - wait_queue_head_t lockq;
> - wait_queue_head_t sleepq;
>
> /* flags */
> volatile int introk; /* status of last irq operation */
> volatile int busy; /* drive performs an operation */
> - volatile int lock; /* exclusive usage */
>
> /* cd infos */
> struct s_diskinfo di;
> @@ -266,7 +266,6 @@ static unsigned int uint2bcd(unsigned in
> static unsigned int bcd2uint(unsigned char);
> static unsigned port(int *);
> static int irq(int *);
> -static void mcdx_delay(struct s_drive_stuff *, long jifs);
> static int mcdx_transfer(struct s_drive_stuff *, char *buf, int sector,
> int nr_sectors);
> static int mcdx_xfer(struct s_drive_stuff *, char *buf, int sector,
> @@ -287,7 +286,7 @@ static int mcdx_requestmultidiskinfo(str
> static int mcdx_requesttocdata(struct s_drive_stuff *, struct s_diskinfo *,
> int);
> static int mcdx_getstatus(struct s_drive_stuff *, int);
> -static int mcdx_getval(struct s_drive_stuff *, int to, int delay, char *);
> +static int mcdx_getval(struct s_drive_stuff *, int to, int delay_sec, char *);
> static int mcdx_talk(struct s_drive_stuff *,
> const unsigned char *cmd, size_t,
> void *buffer, size_t size, unsigned int timeout, int);
> @@ -577,56 +576,57 @@ static void do_mcdx_request(request_queu
> if (!req)
> return;
>
> + if (!blk_fs_request(req)) {
> + end_request(req, 0);
> + goto again;
> + }
> +
> stuffp = req->rq_disk->private_data;
>
> if (!stuffp->present) {
> xwarn("do_request(): bad device: %s\n",req->rq_disk->disk_name);
> xtrace(REQUEST, "end_request(0): bad device\n");
> end_request(req, 0);
> - return;
> + goto again;
> }
>
> if (stuffp->audio) {
> xwarn("do_request() attempt to read from audio cd\n");
> xtrace(REQUEST, "end_request(0): read from audio\n");
> end_request(req, 0);
> - return;
> + goto again;
> }
>
> xtrace(REQUEST, "do_request() (%lu + %lu)\n",
> req->sector, req->nr_sectors);
>
> - if (req->cmd != READ) {
> + if (rq_data_dir(req) != READ) {
> xwarn("do_request(): non-read command to cd!!\n");
> xtrace(REQUEST, "end_request(0): write\n");
> end_request(req, 0);
> - return;
> - }
> - else {
> - stuffp->status = 0;
> - while (req->nr_sectors) {
> - int i;
> -
> - i = mcdx_transfer(stuffp,
> - req->buffer,
> - req->sector,
> - req->nr_sectors);
> -
> - if (i == -1) {
> - end_request(req, 0);
> - goto again;
> - }
> - req->sector += i;
> - req->nr_sectors -= i;
> - req->buffer += (i * 512);
> - }
> - end_request(req, 1);
> goto again;
> -
> - xtrace(REQUEST, "end_request(1)\n");
> - end_request(req, 1);
> }
>
> + stuffp->status = 0;
> + while (req->nr_sectors) {
> + int i;
> +
> + spin_unlock_irq(q->queue_lock);
> + i = mcdx_transfer(stuffp,
> + req->buffer,
> + req->sector,
> + req->nr_sectors);
> + spin_lock_irq(q->queue_lock);
> +
> + if (i == -1) {
> + end_request(req, 0);
> + goto again;
> + }
> + req->sector += i;
> + req->nr_sectors -= i;
> + req->buffer += (i * 512);
> + }
> + end_request(req, 1);
> goto again;
> }
>
> @@ -825,26 +825,6 @@ __setup("mcdx=", mcdx_setup);
>
> /* DIRTY PART ******************************************************/
>
> -static void mcdx_delay(struct s_drive_stuff *stuff, long jifs)
> -/* This routine is used for sleeping.
> - * A jifs value <0 means NO sleeping,
> - * =0 means minimal sleeping (let the kernel
> - * run for other processes)
> - * >0 means at least sleep for that amount.
> - * May be we could use a simple count loop w/ jumps to itself, but
> - * I wanna make this independent of cpu speed. [1 jiffy is 1/HZ] sec */
> -{
> - if (jifs < 0)
> - return;
> -
> - xtrace(SLEEP, "*** delay: sleepq\n");
> - interruptible_sleep_on_timeout(&stuff->sleepq, jifs);
> - xtrace(SLEEP, "delay awoken\n");
> - if (signal_pending(current)) {
> - xtrace(SLEEP, "got signal\n");
> - }
> -}
> -
> static irqreturn_t mcdx_intr(int irq, void *dev_id)
> {
> struct s_drive_stuff *stuffp = dev_id;
> @@ -902,13 +882,7 @@ static int mcdx_talk(struct s_drive_stuf
> if ((discard = (buffer == NULL)))
> buffer = &c;
>
> - while (stuffp->lock) {
> - xtrace(SLEEP, "*** talk: lockq\n");
> - interruptible_sleep_on(&stuffp->lockq);
> - xtrace(SLEEP, "talk: awoken\n");
> - }
> -
> - stuffp->lock = 1;
> + mutex_lock(&stuffp->mutex);
>
> /* An operation other then reading data destroys the
> * data already requested and remembered in stuffp->request, ... */
> @@ -992,8 +966,7 @@ xtrace(TALK, "talk() got 0x%02x\n", *
> xinfo("talk() giving up\n");
> #endif
>
> - stuffp->lock = 0;
> - wake_up_interruptible(&stuffp->lockq);
> + mutex_unlock(&stuffp->mutex);
>
> xtrace(TALK, "talk() done with 0x%02x\n", st);
> return st;
> @@ -1106,9 +1079,9 @@ stuffp->present = 0; /* this should be
> stuffp->wreg_hcon = stuffp->wreg_reset + 1;
> stuffp->wreg_chn = stuffp->wreg_hcon + 1;
>
> + mutex_init(&stuffp->mutex);
> +
> init_waitqueue_head(&stuffp->busyq);
> - init_waitqueue_head(&stuffp->lockq);
> - init_waitqueue_head(&stuffp->sleepq);
>
> /* check if i/o addresses are available */
> if (!request_region(stuffp->wreg_data, MCDX_IO_SIZE, "mcdx")) {
> @@ -1203,7 +1176,7 @@ return 0;
> xtrace(INIT, "init() get garbage\n");
> {
> int i;
> - mcdx_delay(stuffp, HZ / 2);
> + msleep_interruptible(500);
> for (i = 100; i; i--)
> (void) inb(stuffp->rreg_status);
> }
> @@ -1327,10 +1300,6 @@ int done = 0;
> return -1;
> }
>
> - while (stuffp->lock) {
> - interruptible_sleep_on(&stuffp->lockq);
> - }
> -
> if (stuffp->valid && (sector >= stuffp->pending)
> && (sector < stuffp->low_border)) {
>
> @@ -1346,7 +1315,7 @@ int done = 0;
> sector + nr_sectors)
> ? stuffp->high_border : border;
>
> - stuffp->lock = current->pid;
> + mutex_lock(&stuffp->mutex);
>
> do {
>
> @@ -1366,11 +1335,11 @@ int done = 0;
> } else
> continue;
>
> - stuffp->lock = 0;
> + mutex_unlock(&stuffp->mutex);
> +
> stuffp->busy = 0;
> stuffp->valid = 0;
>
> - wake_up_interruptible(&stuffp->lockq);
> xtrace(XFER, "transfer() done (-1)\n");
> return -1;
> }
> @@ -1405,9 +1374,7 @@ insb(stuffp->rreg_data, &dummy[0], C
> }
> } while (++(stuffp->pending) < border);
>
> - stuffp->lock = 0;
> - wake_up_interruptible(&stuffp->lockq);
> -
> + mutex_unlock(&stuffp->mutex);
> } else {
>
> /* The requested sector(s) is/are out of the
> @@ -1654,7 +1621,7 @@ cmd[0], cmd[1], cmd[2], cmd[3],
>
> outsb(stuffp->wreg_data, cmd, sizeof cmd);
>
> - if (-1 == mcdx_getval(stuffp, 3 * HZ, 0, NULL)) {
> + if (-1 == mcdx_getval(stuffp, 3, 0, NULL)) {
> xwarn("playmsf() timeout\n");
> return -1;
> }
> @@ -1907,7 +1874,7 @@ return mcdx_talk(stuffp, "\x40", 1, NUL
> }
>
> static int
> -mcdx_getval(struct s_drive_stuff *stuffp, int to, int delay, char *buf)
> +mcdx_getval(struct s_drive_stuff *stuffp, int to, int delay_sec, char *buf)
> {
> unsigned long timeout = to + jiffies;
> char c;
> @@ -1918,7 +1885,7 @@ mcdx_getval(struct s_drive_stuff *stuffp
> while (inb(stuffp->rreg_status) & MCDX_RBIT_STEN) {
> if (time_after(jiffies, timeout))
> return -1;
> - mcdx_delay(stuffp, delay);
> + msleep_interruptible(delay_sec * 1000);
> }
>
> *buf = (unsigned char) inb(stuffp->rreg_data) & 0xff;
> -
> 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/
NACK
Why are you using req->buffer in new code. req->buffer is a leftover from a
very old block pc era and is supposed to be killed. Please do not use it in
any new code.
you should use bio_data(rq->bio) and if you must have a virtual memory pointer
hanging around than keep it in private space.
Boaz
-
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/
Powered by blists - more mailing lists