Re: [RFC PATCH] ide-floppy: use rq->cmd for preparing and sending packet cmds to the drive

!MAILaRCHIVE_VOTE_RePLACE
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
To: <petkovbb@...>
Cc: <linux-kernel@...>, <linux-ide@...>
Date: Tuesday, February 12, 2008 - 5:39 pm

Hi Borislav,

On Tuesday 12 February 2008, Borislav Petkov wrote:

I think that this _really_ should be done _after_ unifying ATAPI handling [*].
Otherwise you will be making some of the same changes to the _three_ copies
of (more or less) identical code and more importantly we will have to delay
unification after _all_ drivers are converted to rq->cmd[] (+ lets not forget
that I'll have more changes to review ;).

(*) please take a closer look at *_issue_pc(), *_transfer_pc() and *_pc_intr()
    in ide-{floppy,tape,scsi} (the useful hint is that after making these
    functions free of references to device driver specific objects/functions
    we can use drive->media == ide_{floppy,tape,scsi} checks for handling
    not yet fully unified / media type specific code).


It sems that different callback types can be easily removed by merging code
from all callbacks into the common one which decides what to do basing on
the ATAPI command (pc->c[0] / rq->cmd[0]).

[ Probably same can be done in ide-tape's case. ]


Yes but this is also premature w.r.t. the current state of these drivers.


Hmm, we shouldn't be worried about increased stack usage - please take look
at the current code workflow:

- idefloppy_queue_pc_head() is only called from idefloppy_retry_pc()
  (which gets rq from floppy->pc_stack[]).

- idefloppy_queue_pc_tail() already gets rq from the stack
  (the changes just move the allocation from this function up)


no need for these wrappers now => s/merging/removing/


It is not the safe until you review both IDE subsystem and block layer w.r.t.
handling of REQ_TYPE_ATA_PC vs REQ_TYPE_SPECIAL.  After quick audit it seems
to be safe on the block layer level but ironically ide-floppy itself uses
blk_special_request() checks (IOW this change just broke ide-floppy ;).

Please do _not_ mix such changes with purely cleanup ones which shouldn't
(in theory) cause any functional changes (doing it in a post-patch after
having anylised potential consequences is fine).


The one more concern is with many places extracting rq from pc.  This looks
backwards and un-intuitive (your patch is not to be blamed for this because
it was like that before).

Maybe we should just set rq->special to pc, pass rq where necessary and
remove pc->rq (this may be a worthy change to do in pre-patch but again,
ATAPI handling unification should come first).

In summary: the change is good but it looks a bit premature as IMO we should
try to cleanup as much as possible (ATAPI handling unification / ->callbacks
/ pc->rq removal / anything else?) before attacking things like pc->c[] ->
rq->cmd[] / removing static pc/rq allocations / etc.

Thanks,
Bart

--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
Re: [RFC PATCH] ide-floppy: use rq->cmd for preparing and..., Bartlomiej Zolnierkiewicz..., (Tue Feb 12, 5:39 pm)
Re: [RFC PATCH] ide-floppy: use rq-&gt;cmd for preparing and..., Bartlomiej Zolnierkiewicz..., (Wed Feb 13, 9:30 am)