[PATCHSET pata-2.6] ide: rq->buffer, data, special and misc cleanups, take#2

[PATCHSET pata-2.6] ide: rq->buffer, data, special and misc cleanups, take#2

[Tejun Heo]

Hello, Bartlomiej, Jens, Borislav.

This is the second take of ide-rq-buffer-data-special-cleanups
patchset.  Changes from the last take[L] are

* blk_rq_map_kern_prealloc() dropped in favor of Borislav's
  blk_rq_map_kern() from do_request() approach.

* ide-tape tested and fixed.  (data transfer triggers oom but the bug
  exists before the patchset and things other than data transfer work
  well before and after the patchset.  The data transfer bug will be
  dealt with in the next patchset.)

For general overview of the patchset, please read the head message of
the last take.

This patchset contains the following patches.

 0001-block-clear-req-errors-on-bio-completion-only-for.patch
 0002-ide-tape-remove-back-to-back-REQUEST_SENSE-detectio.patch
 0003-ide-use-blk_run_queue-instead-of-blk_start_queuei.patch
 0004-ide-don-t-set-REQ_SOFTBARRIER.patch
 0005-ide-kill-unused-ide_cmd-special.patch
 0006-ide-cd-clear-sense-buffer-before-issuing-request-se.patch
 0007-ide-floppy-block-pc-always-uses-bio.patch
 0008-ide-taskfile-don-t-abuse-rq-buffer.patch
 0009-ide-atapi-don-t-abuse-rq-buffer.patch
 0010-ide-cd-don-t-abuse-rq-buffer.patch
 0011-ide-add-helpers-for-preparing-sense-requests.patch
 0012-ide-cd-convert-to-using-generic-sense-request.patch
 0013-ide-atapi-convert-ide-floppy-tape-to-using-preall.patch
 0014-ide-cd-atapi-use-bio-for-internal-commands.patch
 0015-ide-pm-don-t-abuse-rq-data.patch

0001 is block layer patch.  Jens acked it and agreed to push it
through ide tree.  0002 is new and fixes an ide-tape oops.  0003-0010
are identical to 0004-0011 of the last take.

0011-0013 are from Borislav[1] and unifies sense rq handling between
cd and atapi.  Note that these patches have been modified quite a bit
to make them fit in the series (use of blk_rq_map_kern() is moved to
later patch) and incorporate Bartlomiej's comments and fixes for bugs
I found during testing.  I retained Borislav's S-O-B in the patches,
if this is a problem, please feel free to replace with other proper
tag.

0014 unifies data buffer handling in cd and atapi so that they always
use bio.  0015 is identical to 0012 of the last take.

Tested against cdrom, floppy and tape, so, hopefully, all the bases
are covered.

This patchset is on top of linux-next pata-2.6 tree as of 2009-04-16.
Git tree is available at the following vector but please DO NOT pull
from the following git tree as pata-2.6 tree is quilt-based and the
base tree I used isn't the one which is gonna go upstream.  The
following tree is for review only.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git ide-phase1

Bartlomiej, please feel free to include this in the pata-2.6 patchset.
Also, is the git tree coming?

This patchset contains the following changes.

 block/blk-core.c           |   10 ++-
 drivers/ide/ide-atapi.c    |  147 ++++++++++++++++++++++++++++++---------------
 drivers/ide/ide-cd.c       |   88 ++++++++------------------
 drivers/ide/ide-cd.h       |    4 -
 drivers/ide/ide-disk.c     |    1 
 drivers/ide/ide-floppy.c   |   20 +++---
 drivers/ide/ide-io.c       |   10 +--
 drivers/ide/ide-ioctls.c   |    1 
 drivers/ide/ide-park.c     |    7 --
 drivers/ide/ide-pm.c       |   38 ++++-------
 drivers/ide/ide-tape.c     |   18 ++---
 drivers/ide/ide-taskfile.c |   18 +++--
 include/linux/ide.h        |   15 +++-
 13 files changed, 197 insertions(+), 180 deletions(-)

Thanks.

--
tejun

[L] http://thread.gmane.org/gmane.linux.ide/39275

[PATCH 01/15] block: clear req->errors on bio completion only for fs requests

[Tejun Heo]

Impact: subtle behavior change

For fs requests, rq is only carrier of bios and rq error status as a
whole doesn't mean much.  This is the reason why rq->errors is being
cleared on each partial completion of a request as on each partial
completion the error status is transferred to the respective bios.

For pc requests, rq->errors is used to carry error status to the
issuer and thus __end_that_request_first() doesn't clear it on such
cases.

The condition was fine till now as only fs and pc requests have used
bio and thus the bio completion path.  However, future changes will
unify data accesses to bio and all non fs users care about rq error
status.  Clear rq->errors on bio completion only for fs requests.

In general, the implicit clearing is a bit too subtle especially as
the meaning of rq->errors is completely dependent on low level
drivers.  Unifying / cleaning up rq->errors usage and letting llds
manage it would be better.  TODO comment added.

Signed-off-by: Tejun Heo <tj@kernel.org>
Acked-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 07ab754..cce7a88 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1739,10 +1739,14 @@ static int __end_that_request_first(struct request *req, int error,
 	trace_block_rq_complete(req->q, req);
 
 	/*
-	 * for a REQ_TYPE_BLOCK_PC request, we want to carry any eventual
-	 * sense key with us all the way through
+	 * For fs requests, rq is just carrier of independent bio's
+	 * and each partial completion should be handled separately.
+	 * Reset per-request error on each partial completion.
+	 *
+	 * TODO: tj: This is too subtle.  It would be better to let
+	 * low level drivers do what they see fit.
 	 */
-	if (!blk_pc_request(req))
+	if (blk_fs_request(req))
 		req->errors = 0;
 
 	if (error && (blk_fs_request(req) && !(req->cmd_flags & REQ_QUIET))) {
-- 
1.6.0.2

[PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Tejun Heo]

Impact: fix an oops which always triggers

ide_tape_issue_pc() assumed drive->pc isn't NULL on invocation when
checking for back-to-back request sense issues but drive->pc can be
NULL and even when it's not NULL, it's not safe to dereference it once
the previous command is complete because pc could have been freed or
was on stack.  Kill back-to-back REQUEST_SENSE detection.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-tape.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index cb942a9..3a53e08 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -614,12 +614,6 @@ static ide_startstop_t ide_tape_issue_pc(ide_drive_t *drive,
 {
 	idetape_tape_t *tape = drive->driver_data;
 
-	if (drive->pc->c[0] == REQUEST_SENSE &&
-	    pc->c[0] == REQUEST_SENSE) {
-		printk(KERN_ERR "ide-tape: possible ide-tape.c bug - "
-			"Two request sense in serial were issued\n");
-	}
-
 	if (drive->failed_pc == NULL && pc->c[0] != REQUEST_SENSE)
 		drive->failed_pc = pc;
 
-- 
1.6.0.2

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Borislav Petkov]

Hi,

On Fri, Apr 17, 2009 at 11:33 AM, Tejun Heo <tj@kernel.org> wrote:
> Impact: fix an oops which always triggers
>
> ide_tape_issue_pc() assumed drive->pc isn't NULL on invocation when
> checking for back-to-back request sense issues but drive->pc can be
> NULL and even when it's not NULL, it's not safe to dereference it once
> the previous command is complete because pc could have been freed or
> was on stack.  Kill back-to-back REQUEST_SENSE detection.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/ide/ide-tape.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index cb942a9..3a53e08 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -614,12 +614,6 @@ static ide_startstop_t ide_tape_issue_pc(ide_drive_t *drive,
>  {
>        idetape_tape_t *tape = drive->driver_data;
>
> -       if (drive->pc->c[0] == REQUEST_SENSE &&
> -           pc->c[0] == REQUEST_SENSE) {
> -               printk(KERN_ERR "ide-tape: possible ide-tape.c bug - "
> -                       "Two request sense in serial were issued\n");
> -       }
> -
>        if (drive->failed_pc == NULL && pc->c[0] != REQUEST_SENSE)
>                drive->failed_pc = pc;
>

I hit that too when debugging an ide-tape problem a user has
(http://bugzilla.kernel.org/show_bug.cgi?id=12874). However, this is not the
proper solution since, currently, ide-tape stuffs all packet commands in
rq->buffer or rq->special now after your changes. It has to get them out of
there in the ->do_request callback and set drive->pc to point to the current
packet command in flight through the IRQ handler. And since ide_tape_issue_pc()
is called by the ->do_request callback we should have the drive->pc always
valid.

How about something like that instead:

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 4e6181c..171dbcd 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -792,6 +792,9 @@ static ide_startstop_t
idetape_do_request(ide_drive_t *drive,
 	struct request *postponed_rq = tape->postponed_rq;
 	u8 stat;

+	if (rq->cmd_type == REQ_TYPE_SPECIAL)
+		drive->pc = (struct ide_atapi_pc *) rq->buffer;
+
 	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
 			" current_nr_sectors: %u\n",
 			(unsigned long long)rq->sector, rq->nr_sectors,


-- 
Regards/Gruss,
Boris

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Tejun Heo]

Hello,

Borislav Petkov wrote:
> I hit that too when debugging an ide-tape problem a user has
> (http://bugzilla.kernel.org/show_bug.cgi?id=12874). However, this is not the
> proper solution since, currently, ide-tape stuffs all packet commands in
> rq->buffer or rq->special now after your changes. It has to get them out of
> there in the ->do_request callback and set drive->pc to point to the current
> packet command in flight through the IRQ handler. And since ide_tape_issue_pc()
> is called by the ->do_request callback we should have the drive->pc always
> valid.
> 
> How about something like that instead:
> 
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 4e6181c..171dbcd 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -792,6 +792,9 @@ static ide_startstop_t
> idetape_do_request(ide_drive_t *drive,
>  	struct request *postponed_rq = tape->postponed_rq;
>  	u8 stat;
> 
> +	if (rq->cmd_type == REQ_TYPE_SPECIAL)
> +		drive->pc = (struct ide_atapi_pc *) rq->buffer;
> +
>  	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
>  			" current_nr_sectors: %u\n",
>  			(unsigned long long)rq->sector, rq->nr_sectors,

I don't really care one way or the other but the error condition
itself looked somewhat pointless to me, so I just killed it.  Is the
error messasge really worth guaranteeing drive->pc can be accessed
after completion?  That's a nasty and fragile guarantee.  If such
check is really necessary, the proper way to do it would be recording
whether the last command was request sense in some persistent data
structure not trying to access a data structure which the code doesn't
own anymore.

Thanks.

-- 
tejun

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Tejun Heo]

Tejun Heo wrote:
> I don't really care one way or the other but the error condition
> itself looked somewhat pointless to me, so I just killed it.  Is the
> error messasge really worth guaranteeing drive->pc can be accessed
> after completion?  That's a nasty and fragile guarantee.  If such
> check is really necessary, the proper way to do it would be recording
> whether the last command was request sense in some persistent data
> structure not trying to access a data structure which the code doesn't
> own anymore.

Yet another problem is that idetape_flush_tape_buffers() uses pc which
is on stack which drive->pc ends up pointing directly to, so it won't
work.  Nobody expects that the pointer it passed into an API should be
accessible by the API implementation after it was done with it.
That's just a silly thing to do.

Thanks.

-- 
tejun

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Borislav Petkov]

Hi

On Fri, Apr 17, 2009 at 12:40 PM, Tejun Heo <htejun@gmail.com> wrote:
> Tejun Heo wrote:
>> I don't really care one way or the other but the error condition
>> itself looked somewhat pointless to me, so I just killed it.  Is the
>> error messasge really worth guaranteeing drive->pc can be accessed
>> after completion?  That's a nasty and fragile guarantee.  If such
>> check is really necessary, the proper way to do it would be recording
>> whether the last command was request sense in some persistent data
>> structure not trying to access a data structure which the code doesn't
>> own anymore.

Honestly, I don't know. The code predates even the initial git commit of the
kernel so I guess nobody knows? And yeah, such a check looks a bit too much so I
won't have any problem with removing it. Nevertheless, we need the small fix
above in the ->do_request callback for all other packet commands since ide-tape
uses currently ide_queue_pc_tail() for sending those. I know, I know, it is ugly
and we're working on it :).


> Yet another problem is that idetape_flush_tape_buffers() uses pc which
> is on stack which drive->pc ends up pointing directly to, so it won't
> work.  Nobody expects that the pointer it passed into an API should be
> accessible by the API implementation after it was done with it.
> That's just a silly thing to do.

The whole on stack passing should be passé :) soon since we're about to kill
those ide_atapi_pc's. As I said before, this is very old code that is really
rusty and we're trying to janitor it slowly.

-- 
Regards/Gruss,
Boris

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Tejun Heo]

Hello,

Borislav Petkov wrote:
> Honestly, I don't know. The code predates even the initial git
> commit of the kernel so I guess nobody knows?

Heh.. maybe Mark does.

> And yeah, such a check looks a bit too much so I won't have any
> problem with removing it. Nevertheless, we need the small fix above
> in the ->do_request callback for all other packet commands since
> ide-tape uses currently ide_queue_pc_tail() for sending those. I
> know, I know, it is ugly and we're working on it :).

Can you explain a bit more about the bug?  I'm not really following.

>> Yet another problem is that idetape_flush_tape_buffers() uses pc which
>> is on stack which drive->pc ends up pointing directly to, so it won't
>> work.  Nobody expects that the pointer it passed into an API should be
>> accessible by the API implementation after it was done with it.
>> That's just a silly thing to do.
> 
> The whole on stack passing should be passe :) soon since we're about to kill
> those ide_atapi_pc's.

Hooray.

> As I said before, this is very old code that is really rusty and
> we're trying to janitor it slowly.

Yeap, sure.

Thanks.

-- 
tejun

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Mark Lord]

Tejun Heo wrote:
> Hello,
> 
> Borislav Petkov wrote:
>> Honestly, I don't know. The code predates even the initial git
>> commit of the kernel so I guess nobody knows?
> 
> Heh.. maybe Mark does.
..

I may have known once -- I'm sure Gadi explained it to me,
but my memory doesn't have it any more.

>>> Yet another problem is that idetape_flush_tape_buffers() uses pc which
>>> is on stack which drive->pc ends up pointing directly to, so it won't
>>> work.  Nobody expects that the pointer it passed into an API should be
>>> accessible by the API implementation after it was done with it.
>>> That's just a silly thing to do.
>> The whole on stack passing should be passe :)
..

Nowadays, for sure.

But back then, the kernel was able to map all physical memory
at once, so lots of code did things that way.  Over time, much of
it has been "fixed" to handle newer machines with tons of RAM.

I guess you've just found more of it to modernize.  :)

Cheers

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Borislav Petkov]

Hi,

On Sat, Apr 18, 2009 at 06:12:07AM +0900, Tejun Heo wrote:
> Hello,
> 
> Borislav Petkov wrote:
> > Honestly, I don't know. The code predates even the initial git
> > commit of the kernel so I guess nobody knows?
> 
> Heh.. maybe Mark does.
> 
> > And yeah, such a check looks a bit too much so I won't have any
> > problem with removing it. Nevertheless, we need the small fix above
> > in the ->do_request callback for all other packet commands since
> > ide-tape uses currently ide_queue_pc_tail() for sending those. I
> > know, I know, it is ugly and we're working on it :).
> 
> Can you explain a bit more about the bug?  I'm not really following.

sorry for I wasn't that clear. We need the drive->pc ptr valid in order to
retry a packet command couple lines below in the ->do_request callback:

        /* Retry a failed packet command */
        if (drive->failed_pc && drive->pc->c[0] == REQUEST_SENSE) {
                pc = drive->failed_pc;
                goto out;
        }

Generally, throughout the request path of a command through the driver,
drive->pc points to it, that's why we need that assignment. As I said
before, this'll be fixed rather sooner than later so consider it a
temporary hack :). Something like the following, for example, should
suffice for now IMHO.

--
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index cb942a9..40e6de7 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -753,6 +753,9 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 	struct ide_cmd cmd;
 	u8 stat;
 
+	if (rq->cmd_type == REQ_TYPE_SPECIAL)
+		drive->pc = pc = (struct ide_atapi_pc *) rq->buffer;
+
 	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
 			" current_nr_sectors: %u\n",
 			(unsigned long long)rq->sector, rq->nr_sectors,
@@ -769,7 +772,7 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 	}
 
 	/* Retry a failed packet command */
-	if (drive->failed_pc && drive->pc->c[0] == REQUEST_SENSE) {
+	if (drive->failed_pc && pc->c[0] == REQUEST_SENSE) {
 		pc = drive->failed_pc;
 		goto out;
 	}

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Tejun Heo]

Hello, Borislav.

Borislav Petkov wrote:
> sorry for I wasn't that clear. We need the drive->pc ptr valid in order to
> retry a packet command couple lines below in the ->do_request callback:
> 
>         /* Retry a failed packet command */
>         if (drive->failed_pc && drive->pc->c[0] == REQUEST_SENSE) {
>                 pc = drive->failed_pc;
>                 goto out;
>         }

No, that's checking whether the _previous_ command was REQUEST_SENSE
which is guaranteed to be set if drive->failed_pc is not NULL.
drive->pc is set to the current command at the start of
ide_tape_issue_pc().

> @@ -753,6 +753,9 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
>  	struct ide_cmd cmd;
>  	u8 stat;
>  
> +	if (rq->cmd_type == REQ_TYPE_SPECIAL)
> +		drive->pc = pc = (struct ide_atapi_pc *) rq->buffer;
> +
>  	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
>  			" current_nr_sectors: %u\n",
>  			(unsigned long long)rq->sector, rq->nr_sectors,
> @@ -769,7 +772,7 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
>  	}
>  
>  	/* Retry a failed packet command */
> -	if (drive->failed_pc && drive->pc->c[0] == REQUEST_SENSE) {
> +	if (drive->failed_pc && pc->c[0] == REQUEST_SENSE) {
>  		pc = drive->failed_pc;

So, if you do this, the retry after REQUEST_SENSE logic will never
trigger.

Thanks.

-- 
tejun

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Borislav Petkov]

Hi,

On Sun, Apr 19, 2009 at 06:39:42AM +0900, Tejun Heo wrote:
> Hello, Borislav.
> 
> Borislav Petkov wrote:
> > sorry for I wasn't that clear. We need the drive->pc ptr valid in order to
> > retry a packet command couple lines below in the ->do_request callback:
> > 
> >         /* Retry a failed packet command */
> >         if (drive->failed_pc && drive->pc->c[0] == REQUEST_SENSE) {
> >                 pc = drive->failed_pc;
> >                 goto out;
> >         }
> 
> No, that's checking whether the _previous_ command was REQUEST_SENSE
> which is guaranteed to be set if drive->failed_pc is not NULL.
> drive->pc is set to the current command at the start of
> ide_tape_issue_pc().

Damn! Now it all falls into place nicely, thanks for clarifying that. My
original bug analysis was simply plain wrong.

So, drive->pc means two things: the previous command - until the moment
when it is overwritten with the upcoming command from the current
request _and_ the current command which is being issued. This is all
quite b0rked...

I think your original fix is just fine, let's go with that.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Tejun Heo]

Hello,

Borislav Petkov wrote:
>> No, that's checking whether the _previous_ command was REQUEST_SENSE
>> which is guaranteed to be set if drive->failed_pc is not NULL.
>> drive->pc is set to the current command at the start of
>> ide_tape_issue_pc().
> 
> Damn! Now it all falls into place nicely, thanks for clarifying that. My
> original bug analysis was simply plain wrong.
> 
> So, drive->pc means two things: the previous command - until the moment
> when it is overwritten with the upcoming command from the current
> request _and_ the current command which is being issued.

And whether the previous pc is accessible or not depends on which
command it actually was and if failed_pc is set, it's guaranteed to be
valid.

> This is all quite b0rked...

Yeah, it's all way too fragile.  :-(

-- 
tejun

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Bartlomiej Zolnierkiewicz]

On Friday 17 April 2009 12:23:13 Borislav Petkov wrote:
> Hi,
> 
> On Fri, Apr 17, 2009 at 11:33 AM, Tejun Heo <tj@kernel.org> wrote:
> > Impact: fix an oops which always triggers
> >
> > ide_tape_issue_pc() assumed drive->pc isn't NULL on invocation when
> > checking for back-to-back request sense issues but drive->pc can be
> > NULL and even when it's not NULL, it's not safe to dereference it once
> > the previous command is complete because pc could have been freed or
> > was on stack.  Kill back-to-back REQUEST_SENSE detection.
> >
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > ---
> >  drivers/ide/ide-tape.c |    6 ------
> >  1 files changed, 0 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> > index cb942a9..3a53e08 100644
> > --- a/drivers/ide/ide-tape.c
> > +++ b/drivers/ide/ide-tape.c
> > @@ -614,12 +614,6 @@ static ide_startstop_t ide_tape_issue_pc(ide_drive_t *drive,
> >  {
> >        idetape_tape_t *tape = drive->driver_data;
> >
> > -       if (drive->pc->c[0] == REQUEST_SENSE &&
> > -           pc->c[0] == REQUEST_SENSE) {
> > -               printk(KERN_ERR "ide-tape: possible ide-tape.c bug - "
> > -                       "Two request sense in serial were issued\n");
> > -       }
> > -
> >        if (drive->failed_pc == NULL && pc->c[0] != REQUEST_SENSE)
> >                drive->failed_pc = pc;
> >
> 
> I hit that too when debugging an ide-tape problem a user has
> (http://bugzilla.kernel.org/show_bug.cgi?id=12874). However, this is not the
> proper solution since, currently, ide-tape stuffs all packet commands in
> rq->buffer or rq->special now after your changes. It has to get them out of
> there in the ->do_request callback and set drive->pc to point to the current
> packet command in flight through the IRQ handler. And since ide_tape_issue_pc()
> is called by the ->do_request callback we should have the drive->pc always
> valid.
> 
> How about something like that instead:

Can't we just apply them both? :)

Could it be that we just need to take care if this case:

        if (rq->cmd[13] & REQ_IDETAPE_PC2) {
                idetape_media_access_finished(drive);
                return ide_stopped;
        }

[all other code-paths set pc before calling ide_tape_issue_pc()]

> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 4e6181c..171dbcd 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -792,6 +792,9 @@ static ide_startstop_t
> idetape_do_request(ide_drive_t *drive,
>  	struct request *postponed_rq = tape->postponed_rq;
>  	u8 stat;
> 
> +	if (rq->cmd_type == REQ_TYPE_SPECIAL)
> +		drive->pc = (struct ide_atapi_pc *) rq->buffer;
> +
>  	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
>  			" current_nr_sectors: %u\n",
>  			(unsigned long long)rq->sector, rq->nr_sectors,

Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> Can't we just apply them both? :)

I don't think Borislav's patch is correct.

> Could it be that we just need to take care if this case:
> 
>         if (rq->cmd[13] & REQ_IDETAPE_PC2) {
>                 idetape_media_access_finished(drive);
>                 return ide_stopped;
>         }
> 
> [all other code-paths set pc before calling ide_tape_issue_pc()]

drive->pc is not supposed to be set in do_request().  It should point
to the previous pc structure for REQUEST_SENSE retry handling.  Note
that in this case the previous command pc structure is guaranteed to
be valid.  It's a fragile and confusing logic but, well, that's how
the current code works.

Thanks.

-- 
tejun

[PATCH 03/15] ide: use blk_run_queue() instead of blk_start_queueing()

[Tejun Heo]

blk_start_queueing() is being phased out in favor of
[__]blk_run_queue().  Switch.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-park.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/ide/ide-park.c b/drivers/ide/ide-park.c
index 310d03f..a914023 100644
--- a/drivers/ide/ide-park.c
+++ b/drivers/ide/ide-park.c
@@ -24,11 +24,8 @@ static void issue_park_cmd(ide_drive_t *drive, unsigned long timeout)
 			start_queue = 1;
 		spin_unlock_irq(&hwif->lock);
 
-		if (start_queue) {
-			spin_lock_irq(q->queue_lock);
-			blk_start_queueing(q);
-			spin_unlock_irq(q->queue_lock);
-		}
+		if (start_queue)
+			blk_run_queue(q);
 		return;
 	}
 	spin_unlock_irq(&hwif->lock);
-- 
1.6.0.2

[PATCH 04/15] ide: don't set REQ_SOFTBARRIER

[Tejun Heo]

ide doesn't have to worry about REQ_SOFTBARRIER.  Don't set it.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-disk.c   |    1 -
 drivers/ide/ide-ioctls.c |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
index a9fbe2c..c243880 100644
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -411,7 +411,6 @@ static void idedisk_prepare_flush(struct request_queue *q, struct request *rq)
 	cmd->protocol = ATA_PROT_NODATA;
 
 	rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
-	rq->cmd_flags |= REQ_SOFTBARRIER;
 	rq->special = cmd;
 }
 
diff --git a/drivers/ide/ide-ioctls.c b/drivers/ide/ide-ioctls.c
index c1c25eb..5991b23 100644
--- a/drivers/ide/ide-ioctls.c
+++ b/drivers/ide/ide-ioctls.c
@@ -231,7 +231,6 @@ static int generic_drive_reset(ide_drive_t *drive)
 	rq->cmd_type = REQ_TYPE_SPECIAL;
 	rq->cmd_len = 1;
 	rq->cmd[0] = REQ_DRIVE_RESET;
-	rq->cmd_flags |= REQ_SOFTBARRIER;
 	if (blk_execute_rq(drive->queue, NULL, rq, 1))
 		ret = rq->errors;
 	blk_put_request(rq);
-- 
1.6.0.2

[PATCH 05/15] ide kill unused ide_cmd->special

[Tejun Heo]

Impact: removal of unused field

No one uses ide_cmd->special anymore.  Kill it.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 include/linux/ide.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/include/linux/ide.h b/include/linux/ide.h
index ff65fff..846a1e1 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -324,7 +324,6 @@ struct ide_cmd {
 	unsigned int		cursg_ofs;
 
 	struct request		*rq;		/* copy of request */
-	void			*special;	/* valid_t generally */
 };
 
 /* ATAPI packet command flags */
-- 
1.6.0.2

[PATCH 06/15] ide-cd: clear sense buffer before issuing request sense

[Tejun Heo]

Impact: code simplification

ide_cd_request_sense_fixup() clears the tail of the sense buffer if
the device didn't completely fill it.  This patch makes
cdrom_queue_request_sense() clear the sense buffer before issuing the
command instead of clearing it afterwards.  This simplifies code and
eases future changes.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-cd.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 3aec19d..509f129 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -217,6 +217,8 @@ static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
 	if (sense == NULL)
 		sense = &info->sense_data;
 
+	memset(sense, 0, 18);
+
 	/* stuff the sense request in front of our current request */
 	blk_rq_init(NULL, rq);
 	rq->cmd_type = REQ_TYPE_ATA_PC;
@@ -504,14 +506,8 @@ static void ide_cd_request_sense_fixup(ide_drive_t *drive, struct ide_cmd *cmd)
 	 * and some drives don't send them.  Sigh.
 	 */
 	if (rq->cmd[0] == GPCMD_REQUEST_SENSE &&
-	    cmd->nleft > 0 && cmd->nleft <= 5) {
-		unsigned int ofs = cmd->nbytes - cmd->nleft;
-
-		while (cmd->nleft > 0) {
-			*((u8 *)rq->data + ofs++) = 0;
-			cmd->nleft--;
-		}
-	}
+	    cmd->nleft > 0 && cmd->nleft <= 5)
+		cmd->nleft = 0;
 }
 
 int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
-- 
1.6.0.2

[PATCH 07/15] ide-floppy: block pc always uses bio

[Tejun Heo]

Impact: remove unnecessary code path

Block pc requests always use bio and rq->data is always NULL.  No need
to worry about !rq->bio cases in idefloppy_block_pc_cmd().  Note that
ide-atapi uses ide_pio_bytes() for bio PIO transfer which handle sg
fine.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/ide/ide-floppy.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 2b4868d..3b22e06 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -216,15 +216,13 @@ static void idefloppy_blockpc_cmd(struct ide_disk_obj *floppy,
 	ide_init_pc(pc);
 	memcpy(pc->c, rq->cmd, sizeof(pc->c));
 	pc->rq = rq;
-	if (rq->data_len && rq_data_dir(rq) == WRITE)
-		pc->flags |= PC_FLAG_WRITING;
-	pc->buf = rq->data;
-	if (rq->bio)
+	if (rq->data_len) {
 		pc->flags |= PC_FLAG_DMA_OK;
-	/*
-	 * possibly problematic, doesn't look like ide-floppy correctly
-	 * handled scattered requests if dma fails...
-	 */
+		if (rq_data_dir(rq) == WRITE)
+			pc->flags |= PC_FLAG_WRITING;
+	}
+	/* pio will be performed by ide_pio_bytes() which handles sg fine */
+	pc->buf = NULL;
 	pc->req_xfer = pc->buf_size = rq->data_len;
 }
 
-- 
1.6.0.2

[PATCH 08/15] ide-taskfile: don't abuse rq->buffer

[Tejun Heo]

Impact: rq->buffer usage cleanup

ide_raw_taskfile() directly uses rq->buffer to carry pointer to the
data buffer.  This complicates both block interface and ide backend
request handling.  Use blk_rq_map_kern() instead and drop special
handling for REQ_TYPE_ATA_TASKFILE from ide_map_sg().

Note that REQ_RW setting is moved upwards as blk_rq_map_kern() uses it
to initialize bio rw flag.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/ide/ide-io.c       |    5 +----
 drivers/ide/ide-taskfile.c |   18 +++++++++++-------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 35dc38d..9b9e8b1 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -248,10 +248,7 @@ void ide_map_sg(ide_drive_t *drive, struct ide_cmd *cmd)
 	struct scatterlist *sg = hwif->sg_table;
 	struct request *rq = cmd->rq;
 
-	if (rq->cmd_type == REQ_TYPE_ATA_TASKFILE) {
-		sg_init_one(sg, rq->buffer, rq->nr_sectors * SECTOR_SIZE);
-		cmd->sg_nents = 1;
-	} else if (!rq->bio) {
+	if (!rq->bio) {
 		sg_init_one(sg, rq->data, rq->data_len);
 		cmd->sg_nents = 1;
 	} else
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index 4aa6223..f400eb4 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -424,7 +424,9 @@ int ide_raw_taskfile(ide_drive_t *drive, struct ide_cmd *cmd, u8 *buf,
 
 	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_ATA_TASKFILE;
-	rq->buffer = buf;
+
+	if (cmd->tf_flags & IDE_TFLAG_WRITE)
+		rq->cmd_flags |= REQ_RW;
 
 	/*
 	 * (ks) We transfer currently only whole sectors.
@@ -432,18 +434,20 @@ int ide_raw_taskfile(ide_drive_t *drive, struct ide_cmd *cmd, u8 *buf,
 	 * if we would find a solution to transfer any size.
 	 * To support special commands like READ LONG.
 	 */
-	rq->hard_nr_sectors = rq->nr_sectors = nsect;
-	rq->hard_cur_sectors = rq->current_nr_sectors = nsect;
-
-	if (cmd->tf_flags & IDE_TFLAG_WRITE)
-		rq->cmd_flags |= REQ_RW;
+	if (nsect) {
+		error = blk_rq_map_kern(drive->queue, rq, buf,
+					nsect * SECTOR_SIZE, __GFP_WAIT);
+		if (error)
+			goto put_req;
+	}
 
 	rq->special = cmd;
 	cmd->rq = rq;
 
 	error = blk_execute_rq(drive->queue, NULL, rq, 0);
-	blk_put_request(rq);
 
+put_req:
+	blk_put_request(rq);
 	return error;
 }
 
-- 
1.6.0.2

[PATCH 09/15] ide-atapi: don't abuse rq->buffer

[Tejun Heo]

Impact: rq->buffer usage cleanup

ide-atapi uses rq->buffer as private opaque value for internal special
requests.  rq->special isn't used for these cases (the only case where
rq->special is used is for ide-tape rw requests).  Use rq->special
instead.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/ide/ide-atapi.c  |    4 ++--
 drivers/ide/ide-floppy.c |    2 +-
 drivers/ide/ide-tape.c   |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 7201b17..2894577 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -90,7 +90,7 @@ static void ide_queue_pc_head(ide_drive_t *drive, struct gendisk *disk,
 	blk_rq_init(NULL, rq);
 	rq->cmd_type = REQ_TYPE_SPECIAL;
 	rq->cmd_flags |= REQ_PREEMPT;
-	rq->buffer = (char *)pc;
+	rq->special = (char *)pc;
 	rq->rq_disk = disk;
 
 	if (pc->req_xfer) {
@@ -119,7 +119,7 @@ int ide_queue_pc_tail(ide_drive_t *drive, struct gendisk *disk,
 
 	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_SPECIAL;
-	rq->buffer = (char *)pc;
+	rq->special = (char *)pc;
 
 	if (pc->req_xfer) {
 		rq->data = pc->buf;
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 3b22e06..9460033 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -264,7 +264,7 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 		pc = &floppy->queued_pc;
 		idefloppy_create_rw_cmd(drive, pc, rq, (unsigned long)block);
 	} else if (blk_special_request(rq)) {
-		pc = (struct ide_atapi_pc *) rq->buffer;
+		pc = (struct ide_atapi_pc *)rq->special;
 	} else if (blk_pc_request(rq)) {
 		pc = &floppy->queued_pc;
 		idefloppy_blockpc_cmd(floppy, pc, rq);
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3a53e08..aadf53c 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -828,7 +828,7 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 		goto out;
 	}
 	if (rq->cmd[13] & REQ_IDETAPE_PC1) {
-		pc = (struct ide_atapi_pc *) rq->buffer;
+		pc = (struct ide_atapi_pc *)rq->special;
 		rq->cmd[13] &= ~(REQ_IDETAPE_PC1);
 		rq->cmd[13] |= REQ_IDETAPE_PC2;
 		goto out;
-- 
1.6.0.2

[PATCH 10/15] ide-cd: don't abuse rq->buffer

[Tejun Heo]

Impact: rq->buffer usage cleanup

ide-cd uses rq->buffer to carry pointer to the original request when
issuing REQUEST_SENSE.  Use rq->special instead.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/ide/ide-cd.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 509f129..eb3c299 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -232,8 +232,8 @@ static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
 	rq->cmd_type = REQ_TYPE_SENSE;
 	rq->cmd_flags |= REQ_PREEMPT;
 
-	/* NOTE! Save the failed command in "rq->buffer" */
-	rq->buffer = (void *) failed_command;
+	/* NOTE! Save the failed command in "rq->special" */
+	rq->special = (void *)failed_command;
 
 	if (failed_command)
 		ide_debug_log(IDE_DBG_SENSE, "failed_cmd: 0x%x",
@@ -247,10 +247,10 @@ static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
 static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
 {
 	/*
-	 * For REQ_TYPE_SENSE, "rq->buffer" points to the original
+	 * For REQ_TYPE_SENSE, "rq->special" points to the original
 	 * failed request
 	 */
-	struct request *failed = (struct request *)rq->buffer;
+	struct request *failed = (struct request *)rq->special;
 	struct cdrom_info *info = drive->driver_data;
 	void *sense = &info->sense_data;
 
-- 
1.6.0.2

[PATCH 11/15] ide: add helpers for preparing sense requests

[Tejun Heo]

From: Borislav Petkov <petkovbb@googlemail.com>

This is in preparation of removing the queueing of a sense request out
of the IRQ handler path.

Use struct request_sense as a general sense buffer for all ATAPI
devices ide-{floppy,tape,cd}.

tj: * blk_get_request(__GFP_WAIT) can't be called from do_request() as
      it can cause deadlock.  Converted to use inline struct request
      and blk_rq_init().

    * Added xfer / cdb len selection depending on device type.

    * All sense prep logics folded into ide_prep_sense() which never
      fails.

    * hwif->rq clearing and sense_rq used handling moved into
      ide_queue_sense_rq().

    * blk_rq_map_kern() conversion is moved to later patch.

CC: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
CC: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-atapi.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/ide.h     |   11 ++++++++
 2 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 2894577..c6e0348 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -191,6 +191,67 @@ void ide_create_request_sense_cmd(ide_drive_t *drive, struct ide_atapi_pc *pc)
 }
 EXPORT_SYMBOL_GPL(ide_create_request_sense_cmd);
 
+void ide_prep_sense(ide_drive_t *drive, struct request *rq)
+{
+	struct request_sense *sense = &drive->sense_data;
+	struct request *sense_rq = &drive->sense_rq;
+	unsigned int cmd_len, sense_len;
+
+	debug_log("%s: enter\n", __func__);
+
+	switch (drive->media) {
+	case ide_floppy:
+		cmd_len = 255;
+		sense_len = 18;
+		break;
+	case ide_tape:
+		cmd_len = 20;
+		sense_len = 20;
+		break;
+	default:
+		cmd_len = 18;
+		sense_len = 18;
+	}
+
+	BUG_ON(sense_len > sizeof(*sense));
+
+	if (blk_sense_request(rq) || drive->sense_rq_armed)
+		return;
+
+	memset(sense, 0, sizeof(*sense));
+
+	blk_rq_init(rq->q, sense_rq);
+	sense_rq->rq_disk = rq->rq_disk;
+
+	sense_rq->data = sense;
+	sense_rq->cmd[0] = GPCMD_REQUEST_SENSE;
+	sense_rq->cmd[4] = cmd_len;
+	sense_rq->data_len = sense_len;
+
+	sense_rq->cmd_type = REQ_TYPE_SENSE;
+	sense_rq->cmd_flags |= REQ_PREEMPT;
+
+	if (drive->media == ide_tape)
+		sense_rq->cmd[13] = REQ_IDETAPE_PC1;
+
+	drive->sense_rq_armed = true;
+}
+EXPORT_SYMBOL_GPL(ide_prep_sense);
+
+void ide_queue_sense_rq(ide_drive_t *drive, void *special)
+{
+	BUG_ON(!drive->sense_rq_armed);
+
+	drive->sense_rq.special = special;
+	drive->sense_rq_armed = false;
+
+	drive->hwif->rq = NULL;
+
+	elv_add_request(drive->queue, &drive->sense_rq,
+			ELEVATOR_INSERT_FRONT, 0);
+}
+EXPORT_SYMBOL_GPL(ide_queue_sense_rq);
+
 /*
  * Called when an error was detected during the last packet command.
  * We queue a request sense packet command in the head of the request list.
diff --git a/include/linux/ide.h b/include/linux/ide.h
index 846a1e1..a69ccac 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -26,6 +26,9 @@
 #include <asm/io.h>
 #include <asm/mutex.h>
 
+/* for request_sense */
+#include <linux/cdrom.h>
+
 #if defined(CONFIG_CRIS) || defined(CONFIG_FRV) || defined(CONFIG_MN10300)
 # define SUPPORT_VLB_SYNC 0
 #else
@@ -602,6 +605,11 @@ struct ide_drive_s {
 
 	struct ide_atapi_pc request_sense_pc;
 	struct request request_sense_rq;
+
+	/* current sense rq and buffer */
+	bool sense_rq_armed;
+	struct request sense_rq;
+	struct request_sense sense_data;
 };
 
 typedef struct ide_drive_s ide_drive_t;
@@ -1175,6 +1183,9 @@ int ide_set_media_lock(ide_drive_t *, struct gendisk *, int);
 void ide_create_request_sense_cmd(ide_drive_t *, struct ide_atapi_pc *);
 void ide_retry_pc(ide_drive_t *, struct gendisk *);
 
+void ide_prep_sense(ide_drive_t *drive, struct request *rq);
+void ide_queue_sense_rq(ide_drive_t *drive, void *special);
+
 int ide_cd_expiry(ide_drive_t *);
 
 int ide_cd_get_xferlen(struct request *);
-- 
1.6.0.2

[PATCH 12/15] ide-cd: convert to using generic sense request

[Tejun Heo]

From: Borislav Petkov <petkovbb@googlemail.com>

Preallocate a sense request in the ->do_request method and reinitialize
it only on demand, in case it's been consumed in the IRQ handler path.
The reason for this is that we don't want to be mapping rq to bio in
the IRQ path and introduce all kinds of unnecessary hacks to the block
layer.

tj: * Both user and kernel PC requests expect sense data to be stored
      in separate storage other than drive->sense_data.  Copy sense
      data to rq->sense on completion if rq->sense is not NULL.  This
      fixes bogus sense data on PC requests.

As a result, remove cdrom_queue_request_sense.

CC: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
CC: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-cd.c |   54 +++++++++++--------------------------------------
 drivers/ide/ide-cd.h |    4 ---
 2 files changed, 12 insertions(+), 46 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index eb3c299..7b21c7e 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -206,44 +206,6 @@ static void cdrom_analyze_sense_data(ide_drive_t *drive,
 	ide_cd_log_error(drive->name, failed_command, sense);
 }
 
-static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
-				      struct request *failed_command)
-{
-	struct cdrom_info *info		= drive->driver_data;
-	struct request *rq		= &drive->request_sense_rq;
-
-	ide_debug_log(IDE_DBG_SENSE, "enter");
-
-	if (sense == NULL)
-		sense = &info->sense_data;
-
-	memset(sense, 0, 18);
-
-	/* stuff the sense request in front of our current request */
-	blk_rq_init(NULL, rq);
-	rq->cmd_type = REQ_TYPE_ATA_PC;
-	rq->rq_disk = info->disk;
-
-	rq->data = sense;
-	rq->cmd[0] = GPCMD_REQUEST_SENSE;
-	rq->cmd[4] = 18;
-	rq->data_len = 18;
-
-	rq->cmd_type = REQ_TYPE_SENSE;
-	rq->cmd_flags |= REQ_PREEMPT;
-
-	/* NOTE! Save the failed command in "rq->special" */
-	rq->special = (void *)failed_command;
-
-	if (failed_command)
-		ide_debug_log(IDE_DBG_SENSE, "failed_cmd: 0x%x",
-					     failed_command->cmd[0]);
-
-	drive->hwif->rq = NULL;
-
-	elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 0);
-}
-
 static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
 {
 	/*
@@ -251,11 +213,16 @@ static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
 	 * failed request
 	 */
 	struct request *failed = (struct request *)rq->special;
-	struct cdrom_info *info = drive->driver_data;
-	void *sense = &info->sense_data;
+	struct request_sense *sense = &drive->sense_data;
 
 	if (failed) {
 		if (failed->sense) {
+			/*
+			 * Sense is always read into drive->sense_data.
+			 * Copy back if the failed request has its
+			 * sense pointer set.
+			 */
+			memcpy(failed->sense, sense, 18);
 			sense = failed->sense;
 			failed->sense_len = rq->sense_len;
 		}
@@ -431,7 +398,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
 
 	/* if we got a CHECK_CONDITION status, queue a request sense command */
 	if (stat & ATA_ERR)
-		cdrom_queue_request_sense(drive, NULL, NULL);
+		ide_queue_sense_rq(drive, NULL);
 	return 1;
 
 end_request:
@@ -445,7 +412,7 @@ end_request:
 
 		hwif->rq = NULL;
 
-		cdrom_queue_request_sense(drive, rq->sense, rq);
+		ide_queue_sense_rq(drive, rq);
 		return 1;
 	} else
 		return 2;
@@ -893,6 +860,9 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
 		goto out_end;
 	}
 
+	/* prepare sense request for this command */
+	ide_prep_sense(drive, rq);
+
 	memset(&cmd, 0, sizeof(cmd));
 
 	if (rq_data_dir(rq))
diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
index 1d97101..93a3cf1 100644
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -87,10 +87,6 @@ struct cdrom_info {
 
 	struct atapi_toc *toc;
 
-	/* The result of the last successful request sense command
-	   on this device. */
-	struct request_sense sense_data;
-
 	u8 max_speed;		/* Max speed of the drive. */
 	u8 current_speed;	/* Current speed of the drive. */
 
-- 
1.6.0.2

Re: [PATCH 12/15] ide-cd: convert to using generic sense request

[Borislav Petkov]

Hi,

On Fri, Apr 17, 2009 at 06:33:19PM +0900, Tejun Heo wrote:
> From: Borislav Petkov <petkovbb@googlemail.com>
> 
> Preallocate a sense request in the ->do_request method and reinitialize
> it only on demand, in case it's been consumed in the IRQ handler path.
> The reason for this is that we don't want to be mapping rq to bio in
> the IRQ path and introduce all kinds of unnecessary hacks to the block
> layer.
> 
> tj: * Both user and kernel PC requests expect sense data to be stored
>       in separate storage other than drive->sense_data.  Copy sense
>       data to rq->sense on completion if rq->sense is not NULL.  This
>       fixes bogus sense data on PC requests.
> 
> As a result, remove cdrom_queue_request_sense.
> 
> CC: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> CC: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/ide/ide-cd.c |   54 +++++++++++--------------------------------------
>  drivers/ide/ide-cd.h |    4 ---
>  2 files changed, 12 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index eb3c299..7b21c7e 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -206,44 +206,6 @@ static void cdrom_analyze_sense_data(ide_drive_t *drive,
>  	ide_cd_log_error(drive->name, failed_command, sense);
>  }
>  
> -static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
> -				      struct request *failed_command)
> -{
> -	struct cdrom_info *info		= drive->driver_data;
> -	struct request *rq		= &drive->request_sense_rq;
> -
> -	ide_debug_log(IDE_DBG_SENSE, "enter");
> -
> -	if (sense == NULL)
> -		sense = &info->sense_data;
> -
> -	memset(sense, 0, 18);
> -
> -	/* stuff the sense request in front of our current request */
> -	blk_rq_init(NULL, rq);
> -	rq->cmd_type = REQ_TYPE_ATA_PC;
> -	rq->rq_disk = info->disk;
> -
> -	rq->data = sense;
> -	rq->cmd[0] = GPCMD_REQUEST_SENSE;
> -	rq->cmd[4] = 18;
> -	rq->data_len = 18;
> -
> -	rq->cmd_type = REQ_TYPE_SENSE;
> -	rq->cmd_flags |= REQ_PREEMPT;
> -
> -	/* NOTE! Save the failed command in "rq->special" */
> -	rq->special = (void *)failed_command;
> -
> -	if (failed_command)
> -		ide_debug_log(IDE_DBG_SENSE, "failed_cmd: 0x%x",
> -					     failed_command->cmd[0]);
> -
> -	drive->hwif->rq = NULL;
> -
> -	elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 0);
> -}
> -
>  static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
>  {
>  	/*
> @@ -251,11 +213,16 @@ static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
>  	 * failed request
>  	 */
>  	struct request *failed = (struct request *)rq->special;
> -	struct cdrom_info *info = drive->driver_data;
> -	void *sense = &info->sense_data;
> +	struct request_sense *sense = &drive->sense_data;
>  
>  	if (failed) {
>  		if (failed->sense) {
> +			/*
> +			 * Sense is always read into drive->sense_data.
> +			 * Copy back if the failed request has its
> +			 * sense pointer set.
> +			 */
> +			memcpy(failed->sense, sense, 18);

shouldn't this line be:

			memcpy(failed->sense, sense, rq->sense);

?

According to SFF8020i, Section 10.8.20 REQUEST SENSE Command, the sense
length can be > 18:

"ATAPI CD-ROM Drives shall be capable of returning at least 18 bytes of
data in response to a REQUEST SENSE command. (...)

Host Computers can determine how much sense data has been returned by
examining the allocation length parameter in the Command Packet and the
additional sense length in the sense data. ATAPI CD-ROM Drives shall
not adjust the additional sense length to reflect truncation if the
allocation length is less than the sense data available."

So, a possible scenario might be:

We issue a REQUEST_SENSE to a device and it returns more than 18 bytes
of sense data which should normally fit in the request_sense buffer but
we only copyback the first 18 bytes. Now, the error handling doesn't
touch any members behind the 18th byte (sense->asb) but we still subtly
carry stale data with us resulting in future bugs if one decides to
touch the additional sense bytes (->asb).

However, is there any reason for copying back the sense bytes at all?
In other words, does the block layer code need sense data when ending a
request or is it used only by LLDDs for error handling? Because if I'm
not missing something, we could just as well use the drive->sense_data
in the failed->sense case and thus alleviate the need for the copy.
ide_cd_complete_failed_rq is called on the IRQ path so drive->sense_data
has to be still valid for the current request is a sense request, no?

As a result and if I'm reading the code correctly, we could simply do
(pasting the whole function instead of a diff for more clarity) :

static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
{
        /*
         * For REQ_TYPE_SENSE, "rq->special" points to the original
         * failed request
         */
        struct request *failed = (struct request *)rq->special;
        struct request_sense *sense = &drive->sense_data;

        if (failed) {
                if (failed->sense)
                        /* Sense is always read into drive->sense_data. */
                        failed->sense_len = rq->sense_len;

                cdrom_analyze_sense_data(drive, failed, sense);

                if (ide_end_rq(drive, failed, -EIO, blk_rq_bytes(failed)))
                        BUG();
        } else
                cdrom_analyze_sense_data(drive, NULL, sense);
}



>  			sense = failed->sense;
>  			failed->sense_len = rq->sense_len;
>  		}
> @@ -431,7 +398,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
>  
>  	/* if we got a CHECK_CONDITION status, queue a request sense command */
>  	if (stat & ATA_ERR)
> -		cdrom_queue_request_sense(drive, NULL, NULL);
> +		ide_queue_sense_rq(drive, NULL);
>  	return 1;
>  
>  end_request:
> @@ -445,7 +412,7 @@ end_request:
>  
>  		hwif->rq = NULL;
>  
> -		cdrom_queue_request_sense(drive, rq->sense, rq);
> +		ide_queue_sense_rq(drive, rq);
>  		return 1;
>  	} else
>  		return 2;
> @@ -893,6 +860,9 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>  		goto out_end;
>  	}
>  
> +	/* prepare sense request for this command */
> +	ide_prep_sense(drive, rq);
> +
>  	memset(&cmd, 0, sizeof(cmd));
>  
>  	if (rq_data_dir(rq))
> diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
> index 1d97101..93a3cf1 100644
> --- a/drivers/ide/ide-cd.h
> +++ b/drivers/ide/ide-cd.h
> @@ -87,10 +87,6 @@ struct cdrom_info {
>  
>  	struct atapi_toc *toc;
>  
> -	/* The result of the last successful request sense command
> -	   on this device. */
> -	struct request_sense sense_data;
> -
>  	u8 max_speed;		/* Max speed of the drive. */
>  	u8 current_speed;	/* Current speed of the drive. */
>  
> -- 
> 1.6.0.2

-- 
Regards/Gruss,
    Boris.

Re: [PATCH 12/15] ide-cd: convert to using generic sense request

[Tejun Heo]

Borislav Petkov wrote:
>> +			/*
>> +			 * Sense is always read into drive->sense_data.
>> +			 * Copy back if the failed request has its
>> +			 * sense pointer set.
>> +			 */
>> +			memcpy(failed->sense, sense, 18);
> 
> shouldn't this line be:
> 
> 			memcpy(failed->sense, sense, rq->sense);
> 
> ?
> 
> According to SFF8020i, Section 10.8.20 REQUEST SENSE Command, the sense
> length can be > 18:

It doesn't really matter in this patch.  This patch shouldn't change
what number of bytes are considered by ide-cd for sense data.  Before
the patch it was 18, so it should remain 18 after the patch.

> "ATAPI CD-ROM Drives shall be capable of returning at least 18 bytes of
> data in response to a REQUEST SENSE command. (...)
> 
> Host Computers can determine how much sense data has been returned by
> examining the allocation length parameter in the Command Packet and the
> additional sense length in the sense data. ATAPI CD-ROM Drives shall
> not adjust the additional sense length to reflect truncation if the
> allocation length is less than the sense data available."
> 
> So, a possible scenario might be:
> 
> We issue a REQUEST_SENSE to a device and it returns more than 18 bytes
> of sense data which should normally fit in the request_sense buffer but
> we only copyback the first 18 bytes. Now, the error handling doesn't
> touch any members behind the 18th byte (sense->asb) but we still subtly
> carry stale data with us resulting in future bugs if one decides to
> touch the additional sense bytes (->asb).
> 
> However, is there any reason for copying back the sense bytes at all?
> In other words, does the block layer code need sense data when ending a
> request or is it used only by LLDDs for error handling? Because if I'm
> not missing something, we could just as well use the drive->sense_data
> in the failed->sense case and thus alleviate the need for the copy.
> ide_cd_complete_failed_rq is called on the IRQ path so drive->sense_data
> has to be still valid for the current request is a sense request, no?
> 
> As a result and if I'm reading the code correctly, we could simply do
> (pasting the whole function instead of a diff for more clarity) :
> 
> static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
> {
>         /*
>          * For REQ_TYPE_SENSE, "rq->special" points to the original
>          * failed request
>          */
>         struct request *failed = (struct request *)rq->special;
>         struct request_sense *sense = &drive->sense_data;
> 
>         if (failed) {
>                 if (failed->sense)
>                         /* Sense is always read into drive->sense_data. */
>                         failed->sense_len = rq->sense_len;
> 
>                 cdrom_analyze_sense_data(drive, failed, sense);
> 
>                 if (ide_end_rq(drive, failed, -EIO, blk_rq_bytes(failed)))
>                         BUG();
>         } else
>                 cdrom_analyze_sense_data(drive, NULL, sense);
> }

So, please feel free to submit a separate patch for this.  :-)

Thanks.

-- 
tejun

Re: [PATCH 12/15] ide-cd: convert to using generic sense request

[Tejun Heo]

Tejun Heo wrote:
>> However, is there any reason for copying back the sense bytes at all?
>> In other words, does the block layer code need sense data when ending a
>> request or is it used only by LLDDs for error handling?

And, yes, SG_IO requests need sense data copied back.

Thanks.

-- 
tejun

[PATCH 13/15] ide-atapi: convert ide-{floppy,tape} to using preallocated sense buffer

[Tejun Heo]

From: Borislav Petkov <petkovbb@googlemail.com>

Since we're issuing REQ_TYPE_SENSE now we need to allow those types of
rqs in the ->do_request callbacks. As a future improvement, sense_len
assignment might be unified across all ATAPI devices. Borislav to
check with specs and test.

As a result, get rid of ide_queue_pc_head() and
drive->request_sense_rq.

tj: * Init request sense ide_atapi_pc from sense request.  In the
      longer timer, it would probably better to fold
      ide_create_request_sense_cmd() into its only current user -
      ide_floppy_get_format_progress().

    * ide_retry_pc() no longer takes @disk.

CC: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
CC: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-atapi.c  |   48 +++++++++++++--------------------------------
 drivers/ide/ide-floppy.c |    4 ++-
 drivers/ide/ide-tape.c   |    7 ++++-
 include/linux/ide.h      |    3 +-
 4 files changed, 23 insertions(+), 39 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index c6e0348..972c522 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -80,34 +80,6 @@ void ide_init_pc(struct ide_atapi_pc *pc)
 EXPORT_SYMBOL_GPL(ide_init_pc);
 
 /*
- * Generate a new packet command request in front of the request queue, before
- * the current request, so that it will be processed immediately, on the next
- * pass through the driver.
- */
-static void ide_queue_pc_head(ide_drive_t *drive, struct gendisk *disk,
-			      struct ide_atapi_pc *pc, struct request *rq)
-{
-	blk_rq_init(NULL, rq);
-	rq->cmd_type = REQ_TYPE_SPECIAL;
-	rq->cmd_flags |= REQ_PREEMPT;
-	rq->special = (char *)pc;
-	rq->rq_disk = disk;
-
-	if (pc->req_xfer) {
-		rq->data = pc->buf;
-		rq->data_len = pc->req_xfer;
-	}
-
-	memcpy(rq->cmd, pc->c, 12);
-	if (drive->media == ide_tape)
-		rq->cmd[13] = REQ_IDETAPE_PC1;
-
-	drive->hwif->rq = NULL;
-
-	elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 0);
-}
-
-/*
  * Add a special packet command request to the tail of the request queue,
  * and wait for it to be serviced.
  */
@@ -254,18 +226,26 @@ EXPORT_SYMBOL_GPL(ide_queue_sense_rq);
 
 /*
  * Called when an error was detected during the last packet command.
- * We queue a request sense packet command in the head of the request list.
+ * We queue a request sense packet command at the head of the request
+ * queue.
  */
-void ide_retry_pc(ide_drive_t *drive, struct gendisk *disk)
+void ide_retry_pc(ide_drive_t *drive)
 {
-	struct request *rq = &drive->request_sense_rq;
+	struct request *sense_rq = &drive->sense_rq;
 	struct ide_atapi_pc *pc = &drive->request_sense_pc;
 
 	(void)ide_read_error(drive);
-	ide_create_request_sense_cmd(drive, pc);
+
+	/* init pc from sense_rq */
+	ide_init_pc(pc);
+	memcpy(pc->c, sense_rq->cmd, 12);
+	pc->buf = sense_rq->data;
+	pc->req_xfer = sense_rq->data_len;
+
 	if (drive->media == ide_tape)
 		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
-	ide_queue_pc_head(drive, disk, pc, rq);
+
+	ide_queue_sense_rq(drive, pc);
 }
 EXPORT_SYMBOL_GPL(ide_retry_pc);
 
@@ -404,7 +384,7 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 			debug_log("[cmd %x]: check condition\n", rq->cmd[0]);
 
 			/* Retry operation */
-			ide_retry_pc(drive, rq->rq_disk);
+			ide_retry_pc(drive);
 
 			/* queued, but not started */
 			return ide_stopped;
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 9460033..d3302cc 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -263,7 +263,7 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 		}
 		pc = &floppy->queued_pc;
 		idefloppy_create_rw_cmd(drive, pc, rq, (unsigned long)block);
-	} else if (blk_special_request(rq)) {
+	} else if (blk_special_request(rq) || blk_sense_request(rq)) {
 		pc = (struct ide_atapi_pc *)rq->special;
 	} else if (blk_pc_request(rq)) {
 		pc = &floppy->queued_pc;
@@ -273,6 +273,8 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
 		goto out_end;
 	}
 
+	ide_prep_sense(drive, rq);
+
 	memset(&cmd, 0, sizeof(cmd));
 
 	if (rq_data_dir(rq))
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index aadf53c..8324dfa 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -695,7 +695,7 @@ static ide_startstop_t idetape_media_access_finished(ide_drive_t *drive)
 				printk(KERN_ERR "ide-tape: %s: I/O error, ",
 						tape->name);
 			/* Retry operation */
-			ide_retry_pc(drive, tape->disk);
+			ide_retry_pc(drive);
 			return ide_stopped;
 		}
 		pc->error = 0;
@@ -752,7 +752,7 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 			(unsigned long long)rq->sector, rq->nr_sectors,
 			rq->current_nr_sectors);
 
-	if (!blk_special_request(rq)) {
+	if (!(blk_special_request(rq) || blk_sense_request(rq))) {
 		/* We do not support buffer cache originated requests. */
 		printk(KERN_NOTICE "ide-tape: %s: Unsupported request in "
 			"request queue (%d)\n", drive->name, rq->cmd_type);
@@ -840,6 +840,9 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 	BUG();
 
 out:
+	/* prepare sense request for this command */
+	ide_prep_sense(drive, rq);
+
 	memset(&cmd, 0, sizeof(cmd));
 
 	if (rq_data_dir(rq))
diff --git a/include/linux/ide.h b/include/linux/ide.h
index a69ccac..9e67cca 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -604,7 +604,6 @@ struct ide_drive_s {
 	unsigned long atapi_flags;
 
 	struct ide_atapi_pc request_sense_pc;
-	struct request request_sense_rq;
 
 	/* current sense rq and buffer */
 	bool sense_rq_armed;
@@ -1181,7 +1180,7 @@ int ide_do_test_unit_ready(ide_drive_t *, struct gendisk *);
 int ide_do_start_stop(ide_drive_t *, struct gendisk *, int);
 int ide_set_media_lock(ide_drive_t *, struct gendisk *, int);
 void ide_create_request_sense_cmd(ide_drive_t *, struct ide_atapi_pc *);
-void ide_retry_pc(ide_drive_t *, struct gendisk *);
+void ide_retry_pc(ide_drive_t *drive);
 
 void ide_prep_sense(ide_drive_t *drive, struct request *rq);
 void ide_queue_sense_rq(ide_drive_t *drive, void *special);
-- 
1.6.0.2

[PATCH 14/15] ide-cd,atapi: use bio for internal commands

[Tejun Heo]

Impact: unify request data buffer handling

rq->data is used mostly to pass kernel buffer through request queue
without using bio.  There are only a couple of places which still do
this in kernel and converting to bio isn't difficult.

This patch converts ide-cd and atapi to use bio instead of rq->data
for request sense and internal pc commands.  With previous change to
unify sense request handling, this is relatively easily achieved by
adding blk_rq_map_kern() during sense_rq prep and PC issue.

If blk_rq_map_kern() fails for sense, the error is deferred till sense
issue and aborts the failed command which triggered the sense.  Note
that this is a slim possibility as sense prep is done on each command
issue, so for the above condition to actually trigger, all preps since
the last sense issue till the issue of the request which would require
a sense should fail.

* do_request functions might sleep now.  This should be okay as ide
  request_fn - do_ide_request() - is invoked only from make_request
  and plug work.  Make sure this is the case by adding might_sleep()
  to do_ide_request().

* Functions which access the read sense data before the sense request
  is complete now should access bio_data(sense_rq->bio) as the sense
  buffer might have been copied during blk_rq_map_kern().

* ide-tape updated to map sg.

* cdrom_do_block_pc() now doesn't have to deal with REQ_TYPE_ATA_PC
  special case.  Simplified.

* tp_ops->output/input_data path dropped from ide_pc_intr().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-atapi.c |   52 ++++++++++++++++++++++++++++-------------------
 drivers/ide/ide-cd.c    |   28 ++++++++++++------------
 drivers/ide/ide-io.c    |    3 ++
 drivers/ide/ide-tape.c  |    3 ++
 include/linux/ide.h     |    2 +-
 5 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 972c522..5cefe12 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -94,16 +94,18 @@ int ide_queue_pc_tail(ide_drive_t *drive, struct gendisk *disk,
 	rq->special = (char *)pc;
 
 	if (pc->req_xfer) {
-		rq->data = pc->buf;
-		rq->data_len = pc->req_xfer;
+		error = blk_rq_map_kern(drive->queue, rq, pc->buf, pc->req_xfer,
+					GFP_NOIO);
+		if (error)
+			goto put_req;
 	}
 
 	memcpy(rq->cmd, pc->c, 12);
 	if (drive->media == ide_tape)
 		rq->cmd[13] = REQ_IDETAPE_PC1;
 	error = blk_execute_rq(drive->queue, disk, rq, 0);
+put_req:
 	blk_put_request(rq);
-
 	return error;
 }
 EXPORT_SYMBOL_GPL(ide_queue_pc_tail);
@@ -168,6 +170,7 @@ void ide_prep_sense(ide_drive_t *drive, struct request *rq)
 	struct request_sense *sense = &drive->sense_data;
 	struct request *sense_rq = &drive->sense_rq;
 	unsigned int cmd_len, sense_len;
+	int err;
 
 	debug_log("%s: enter\n", __func__);
 
@@ -193,13 +196,19 @@ void ide_prep_sense(ide_drive_t *drive, struct request *rq)
 	memset(sense, 0, sizeof(*sense));
 
 	blk_rq_init(rq->q, sense_rq);
-	sense_rq->rq_disk = rq->rq_disk;
 
-	sense_rq->data = sense;
+	err = blk_rq_map_kern(drive->queue, sense_rq, sense, sense_len,
+			      GFP_NOIO);
+	if (unlikely(err)) {
+		if (printk_ratelimit())
+			printk(KERN_WARNING "%s: failed to map sense buffer\n",
+			       drive->name);
+		return;
+	}
+
+	sense_rq->rq_disk = rq->rq_disk;
 	sense_rq->cmd[0] = GPCMD_REQUEST_SENSE;
 	sense_rq->cmd[4] = cmd_len;
-	sense_rq->data_len = sense_len;
-
 	sense_rq->cmd_type = REQ_TYPE_SENSE;
 	sense_rq->cmd_flags |= REQ_PREEMPT;
 
@@ -210,9 +219,14 @@ void ide_prep_sense(ide_drive_t *drive, struct request *rq)
 }
 EXPORT_SYMBOL_GPL(ide_prep_sense);
 
-void ide_queue_sense_rq(ide_drive_t *drive, void *special)
+int ide_queue_sense_rq(ide_drive_t *drive, void *special)
 {
-	BUG_ON(!drive->sense_rq_armed);
+	/* deferred failure from ide_prep_sense() */
+	if (!drive->sense_rq_armed) {
+		printk(KERN_WARNING "%s: failed queue sense request\n",
+		       drive->name);
+		return -ENOMEM;
+	}
 
 	drive->sense_rq.special = special;
 	drive->sense_rq_armed = false;
@@ -221,6 +235,7 @@ void ide_queue_sense_rq(ide_drive_t *drive, void *special)
 
 	elv_add_request(drive->queue, &drive->sense_rq,
 			ELEVATOR_INSERT_FRONT, 0);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(ide_queue_sense_rq);
 
@@ -239,13 +254,14 @@ void ide_retry_pc(ide_drive_t *drive)
 	/* init pc from sense_rq */
 	ide_init_pc(pc);
 	memcpy(pc->c, sense_rq->cmd, 12);
-	pc->buf = sense_rq->data;
+	pc->buf = bio_data(sense_rq->bio);	/* pointer to mapped address */
 	pc->req_xfer = sense_rq->data_len;
 
 	if (drive->media == ide_tape)
 		set_bit(IDE_AFLAG_IGNORE_DSC, &drive->atapi_flags);
 
-	ide_queue_sense_rq(drive, pc);
+	if (ide_queue_sense_rq(drive, pc))
+		ide_complete_rq(drive, -EIO, blk_rq_bytes(drive->hwif->rq));
 }
 EXPORT_SYMBOL_GPL(ide_retry_pc);
 
@@ -317,7 +333,6 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 	struct ide_cmd *cmd = &hwif->cmd;
 	struct request *rq = hwif->rq;
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
-	xfer_func_t *xferfunc;
 	unsigned int timeout, done;
 	u16 bcount;
 	u8 stat, ireason, dsc = 0;
@@ -411,7 +426,7 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 					rq->errors = -EIO;
 			}
 
-			if (drive->media == ide_tape)
+			if (drive->media == ide_tape && !rq->bio)
 				done = ide_rq_bytes(rq); /* FIXME */
 			else
 				done = blk_rq_bytes(rq);
@@ -448,16 +463,11 @@ static ide_startstop_t ide_pc_intr(ide_drive_t *drive)
 		return ide_do_reset(drive);
 	}
 
-	xferfunc = write ? tp_ops->output_data : tp_ops->input_data;
-
-	if (drive->media == ide_floppy && pc->buf == NULL) {
+	if (drive->media == ide_tape && pc->bh)
+		done = drive->pc_io_buffers(drive, pc, bcount, write);
+	else {
 		done = min_t(unsigned int, bcount, cmd->nleft);
 		ide_pio_bytes(drive, cmd, write, done);
-	} else if (drive->media == ide_tape && pc->bh) {
-		done = drive->pc_io_buffers(drive, pc, bcount, write);
-	} else {
-		done = min_t(unsigned int, bcount, pc->req_xfer - pc->xferred);
-		xferfunc(drive, NULL, pc->cur_pos, done);
 	}
 
 	/* Update the current position */
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 7b21c7e..392a5bd 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -210,10 +210,12 @@ static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
 {
 	/*
 	 * For REQ_TYPE_SENSE, "rq->special" points to the original
-	 * failed request
+	 * failed request.  Also, the sense data should be read
+	 * directly from rq which might be different from the original
+	 * sense buffer if it got copied during mapping.
 	 */
 	struct request *failed = (struct request *)rq->special;
-	struct request_sense *sense = &drive->sense_data;
+	void *sense = bio_data(rq->bio);
 
 	if (failed) {
 		if (failed->sense) {
@@ -398,7 +400,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat)
 
 	/* if we got a CHECK_CONDITION status, queue a request sense command */
 	if (stat & ATA_ERR)
-		ide_queue_sense_rq(drive, NULL);
+		return ide_queue_sense_rq(drive, NULL) ? 2 : 1;
 	return 1;
 
 end_request:
@@ -412,8 +414,7 @@ end_request:
 
 		hwif->rq = NULL;
 
-		ide_queue_sense_rq(drive, rq);
-		return 1;
+		return ide_queue_sense_rq(drive, rq) ? 2 : 1;
 	} else
 		return 2;
 }
@@ -507,8 +508,12 @@ int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
 		rq->cmd_flags |= cmd_flags;
 		rq->timeout = timeout;
 		if (buffer) {
-			rq->data = buffer;
-			rq->data_len = *bufflen;
+			error = blk_rq_map_kern(drive->queue, rq, buffer,
+						*bufflen, GFP_NOIO);
+			if (error) {
+				blk_put_request(rq);
+				return error;
+			}
 		}
 
 		error = blk_execute_rq(drive->queue, info->disk, rq, 0);
@@ -802,15 +807,10 @@ static void cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 	drive->dma = 0;
 
 	/* sg request */
-	if (rq->bio || ((rq->cmd_type == REQ_TYPE_ATA_PC) && rq->data_len)) {
+	if (rq->bio) {
 		struct request_queue *q = drive->queue;
+		char *buf = bio_data(rq->bio);
 		unsigned int alignment;
-		char *buf;
-
-		if (rq->bio)
-			buf = bio_data(rq->bio);
-		else
-			buf = rq->data;
 
 		drive->dma = !!(drive->dev_flags & IDE_DFLAG_USING_DMA);
 
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 9b9e8b1..3245c2d 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -481,6 +481,9 @@ void do_ide_request(struct request_queue *q)
 
 	spin_unlock_irq(q->queue_lock);
 
+	/* HLD do_request() callback might sleep, make sure it's okay */
+	might_sleep();
+
 	if (ide_lock_host(host, hwif))
 		goto plug_device_2;
 
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 8324dfa..9b762a2 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -850,6 +850,9 @@ out:
 
 	cmd.rq = rq;
 
+	ide_init_sg_cmd(&cmd, pc->req_xfer);
+	ide_map_sg(drive, &cmd);
+
 	return ide_tape_issue_pc(drive, &cmd, pc);
 }
 
diff --git a/include/linux/ide.h b/include/linux/ide.h
index 9e67cca..1957461 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1183,7 +1183,7 @@ void ide_create_request_sense_cmd(ide_drive_t *, struct ide_atapi_pc *);
 void ide_retry_pc(ide_drive_t *drive);
 
 void ide_prep_sense(ide_drive_t *drive, struct request *rq);
-void ide_queue_sense_rq(ide_drive_t *drive, void *special);
+int ide_queue_sense_rq(ide_drive_t *drive, void *special);
 
 int ide_cd_expiry(ide_drive_t *);
 
-- 
1.6.0.2

[PATCH 15/15] ide-pm: don't abuse rq->data

[Tejun Heo]

Impact: cleanup rq->data usage

ide-pm uses rq->data to carry pointer to struct request_pm_state
through request queue and rq->special is used to carray pointer to
local struct ide_cmd, which isn't necessary.  Use rq->special for
request_pm_state instead and use local ide_cmd in
ide_start_power_step().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/ide/ide-io.c |    2 +-
 drivers/ide/ide-pm.c |   38 +++++++++++++++-----------------------
 2 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 3245c2d..6e3094e 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -368,7 +368,7 @@ static ide_startstop_t start_request (ide_drive_t *drive, struct request *rq)
 		if (rq->cmd_type == REQ_TYPE_ATA_TASKFILE)
 			return execute_drive_cmd(drive, rq);
 		else if (blk_pm_request(rq)) {
-			struct request_pm_state *pm = rq->data;
+			struct request_pm_state *pm = rq->special;
 #ifdef DEBUG_PM
 			printk("%s: start_power_step(step: %d)\n",
 				drive->name, pm->pm_step);
diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
index 0d8a151..ba1488b 100644
--- a/drivers/ide/ide-pm.c
+++ b/drivers/ide/ide-pm.c
@@ -7,7 +7,6 @@ int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 	ide_hwif_t *hwif = drive->hwif;
 	struct request *rq;
 	struct request_pm_state rqpm;
-	struct ide_cmd cmd;
 	int ret;
 
 	/* call ACPI _GTM only once */
@@ -15,11 +14,9 @@ int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 		ide_acpi_get_timing(hwif);
 
 	memset(&rqpm, 0, sizeof(rqpm));
-	memset(&cmd, 0, sizeof(cmd));
 	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_PM_SUSPEND;
-	rq->special = &cmd;
-	rq->data = &rqpm;
+	rq->special = &rqpm;
 	rqpm.pm_step = IDE_PM_START_SUSPEND;
 	if (mesg.event == PM_EVENT_PRETHAW)
 		mesg.event = PM_EVENT_FREEZE;
@@ -41,7 +38,6 @@ int generic_ide_resume(struct device *dev)
 	ide_hwif_t *hwif = drive->hwif;
 	struct request *rq;
 	struct request_pm_state rqpm;
-	struct ide_cmd cmd;
 	int err;
 
 	/* call ACPI _PS0 / _STM only once */
@@ -53,12 +49,10 @@ int generic_ide_resume(struct device *dev)
 	ide_acpi_exec_tfs(drive);
 
 	memset(&rqpm, 0, sizeof(rqpm));
-	memset(&cmd, 0, sizeof(cmd));
 	rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_PM_RESUME;
 	rq->cmd_flags |= REQ_PREEMPT;
-	rq->special = &cmd;
-	rq->data = &rqpm;
+	rq->special = &rqpm;
 	rqpm.pm_step = IDE_PM_START_RESUME;
 	rqpm.pm_state = PM_EVENT_ON;
 
@@ -77,7 +71,7 @@ int generic_ide_resume(struct device *dev)
 
 void ide_complete_power_step(ide_drive_t *drive, struct request *rq)
 {
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->special;
 
 #ifdef DEBUG_PM
 	printk(KERN_INFO "%s: complete_power_step(step: %d)\n",
@@ -107,10 +101,8 @@ void ide_complete_power_step(ide_drive_t *drive, struct request *rq)
 
 ide_startstop_t ide_start_power_step(ide_drive_t *drive, struct request *rq)
 {
-	struct request_pm_state *pm = rq->data;
-	struct ide_cmd *cmd = rq->special;
-
-	memset(cmd, 0, sizeof(*cmd));
+	struct request_pm_state *pm = rq->special;
+	struct ide_cmd cmd = { };
 
 	switch (pm->pm_step) {
 	case IDE_PM_FLUSH_CACHE:	/* Suspend step 1 (flush cache) */
@@ -123,12 +115,12 @@ ide_startstop_t ide_start_power_step(ide_drive_t *drive, struct request *rq)
 			return ide_stopped;
 		}
 		if (ata_id_flush_ext_enabled(drive->id))
-			cmd->tf.command = ATA_CMD_FLUSH_EXT;
+			cmd.tf.command = ATA_CMD_FLUSH_EXT;
 		else
-			cmd->tf.command = ATA_CMD_FLUSH;
+			cmd.tf.command = ATA_CMD_FLUSH;
 		goto out_do_tf;
 	case IDE_PM_STANDBY:		/* Suspend step 2 (standby) */
-		cmd->tf.command = ATA_CMD_STANDBYNOW1;
+		cmd.tf.command = ATA_CMD_STANDBYNOW1;
 		goto out_do_tf;
 	case IDE_PM_RESTORE_PIO:	/* Resume step 1 (restore PIO) */
 		ide_set_max_pio(drive);
@@ -141,7 +133,7 @@ ide_startstop_t ide_start_power_step(ide_drive_t *drive, struct request *rq)
 			ide_complete_power_step(drive, rq);
 		return ide_stopped;
 	case IDE_PM_IDLE:		/* Resume step 2 (idle) */
-		cmd->tf.command = ATA_CMD_IDLEIMMEDIATE;
+		cmd.tf.command = ATA_CMD_IDLEIMMEDIATE;
 		goto out_do_tf;
 	case IDE_PM_RESTORE_DMA:	/* Resume step 3 (restore DMA) */
 		/*
@@ -163,11 +155,11 @@ ide_startstop_t ide_start_power_step(ide_drive_t *drive, struct request *rq)
 	return ide_stopped;
 
 out_do_tf:
-	cmd->valid.out.tf = IDE_VALID_OUT_TF | IDE_VALID_DEVICE;
-	cmd->valid.in.tf  = IDE_VALID_IN_TF  | IDE_VALID_DEVICE;
-	cmd->protocol = ATA_PROT_NODATA;
+	cmd.valid.out.tf = IDE_VALID_OUT_TF | IDE_VALID_DEVICE;
+	cmd.valid.in.tf  = IDE_VALID_IN_TF  | IDE_VALID_DEVICE;
+	cmd.protocol = ATA_PROT_NODATA;
 
-	return do_rw_taskfile(drive, cmd);
+	return do_rw_taskfile(drive, &cmd);
 }
 
 /**
@@ -181,7 +173,7 @@ out_do_tf:
 void ide_complete_pm_rq(ide_drive_t *drive, struct request *rq)
 {
 	struct request_queue *q = drive->queue;
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->special;
 	unsigned long flags;
 
 	ide_complete_power_step(drive, rq);
@@ -207,7 +199,7 @@ void ide_complete_pm_rq(ide_drive_t *drive, struct request *rq)
 
 void ide_check_pm_state(ide_drive_t *drive, struct request *rq)
 {
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->special;
 
 	if (blk_pm_suspend_request(rq) &&
 	    pm->pm_step == IDE_PM_START_SUSPEND)
-- 
1.6.0.2

Re: [PATCHSET pata-2.6] ide: rq->buffer, data, special and misc cleanups, take#2

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday 17 April 2009 11:33:07 Tejun Heo wrote:

[...]

> Tested against cdrom, floppy and tape, so, hopefully, all the bases
> are covered.

Everything looks fine to me and we can always deal with ide-tape issues
left later (no big deal as it is broken upstream anyway), Borislav?

> This patchset is on top of linux-next pata-2.6 tree as of 2009-04-16.
> Git tree is available at the following vector but please DO NOT pull
> from the following git tree as pata-2.6 tree is quilt-based and the
> base tree I used isn't the one which is gonna go upstream.  The
> following tree is for review only.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git ide-phase1
> 
> Bartlomiej, please feel free to include this in the pata-2.6 patchset.
> Also, is the git tree coming?

Care to re-base it on top of 'for-next' branch of:

master.kernel.org:/pub/scm/linux/kernel/git/bart/ide-2.6.git for-next

?

Thanks,
Bart

Re: [PATCHSET pata-2.6] ide: rq->buffer, data, special and misc cleanups, take#2

[Borislav Petkov]

Hello lads,

On Sat, Apr 18, 2009 at 06:32:29PM +0200, Bartlomiej Zolnierkiewicz wrote:
> Everything looks fine to me and we can always deal with ide-tape issues
> left later (no big deal as it is broken upstream anyway), Borislav?

Seems so, see http://bugzilla.kernel.org/show_bug.cgi?id=12874, for
example. Especially, as it turns out, there's no checking whether
the driver replies to a command, (say MODE_SENSE, CAPABILITIES PAGE)
with the page itself or with standard sense data. It's kinda hard to
fix something like that remotely if one doesn't have the hardware. I
probably should look for some old junk on ebay...

-- 
Regards/Gruss,
    Boris.

Re: [PATCHSET pata-2.6] ide: rq->buffer, data, special and misc cleanups, take#2

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> Care to re-base it on top of 'for-next' branch of:
> 
> master.kernel.org:/pub/scm/linux/kernel/git/bart/ide-2.6.git for-next

Ah... git tree, nice.  :-)

Will rebase asap.

-- 
tejun

[GIT PATCH pata-2.6] ide: rq->buffer, data, special and misc cleanups, take#2

[Tejun Heo]

Hello,

Okay, tree rebased.  Please pull from the following git tree.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git ide-phase1

Top commit id is fc38b521dcffcb07447cd98fedc56f495c10b90d.

Thanks.

-- 
tejun

Re: [GIT PATCH pata-2.6] ide: rq->buffer, data, special and misc cleanups, take#2

[Bartlomiej Zolnierkiewicz]

On Sunday 19 April 2009 00:04:11 Tejun Heo wrote:
> Hello,
> 
> Okay, tree rebased.  Please pull from the following git tree.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git ide-phase1
> 
> Top commit id is fc38b521dcffcb07447cd98fedc56f495c10b90d.

I merged it into for-next branch.  Thanks!

Re: [GIT PATCH pata-2.6] ide: rq->buffer, data, special and misc cleanups, take#2

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> On Sunday 19 April 2009 00:04:11 Tejun Heo wrote:
>> Hello,
>>
>> Okay, tree rebased.  Please pull from the following git tree.
>>
>>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git ide-phase1
>>
>> Top commit id is fc38b521dcffcb07447cd98fedc56f495c10b90d.
> 
> I merged it into for-next branch.  Thanks!

Thanks.

-- 
tejun