lkml.org 
[lkml]   [2009]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()
    Date
    On Wednesday 11 February 2009, Sergei Shtylyov wrote:
    > Hello.
    >
    > Borislav Petkov wrote:
    >
    > >>>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
    > >>>> Subject: [PATCH] ide: remove ide_execute_pkt_cmd()
    > >>>>
    > >>>> * Pass command structure to ide_execute_command() and skip
    > >>>> __ide_set_handler() for ATAPI protocols.
    > >>>>
    > >>>> * Convert ide_issue_pc() to always use ide_execute_command()
    > >>>> and remove no longer needed ide_execute_pkt_cmd().
    > >>>>
    > >>>> There should be no functional changes caused by this patch.
    > >>>>
    > >>>> Cc: Borislav Petkov <petkovbb@gmail.com>
    > >>>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
    > >>>> ---
    > >>>> drivers/ide/ide-atapi.c | 15 +++++++--------
    > >>>> drivers/ide/ide-iops.c | 23 ++++++-----------------
    > >>>> drivers/ide/ide-taskfile.c | 6 ++----
    > >>>> include/linux/ide.h | 5 ++---
    > >>>> 4 files changed, 17 insertions(+), 32 deletions(-)
    > >>>>
    > >>>> Index: b/drivers/ide/ide-atapi.c
    > >>>> ===================================================================
    > >>>> --- a/drivers/ide/ide-atapi.c
    > >>>> +++ b/drivers/ide/ide-atapi.c
    > >>>> @@ -481,6 +481,7 @@ static void ide_init_packet_cmd(struct i
    > >>>> cmd->protocol = dma ? ATAPI_PROT_DMA : ATAPI_PROT_PIO;
    > >>>> cmd->tf_flags |= IDE_TFLAG_OUT_LBAH | IDE_TFLAG_OUT_LBAM |
    > >>>> IDE_TFLAG_OUT_FEATURE | tf_flags;
    > >>>> + cmd->tf.command = ATA_CMD_PACKET;
    > >>>> cmd->tf.feature = dma; /* Use PIO/DMA */
    > >>>> cmd->tf.lbam = bcount & 0xff;
    > >>>> cmd->tf.lbah = (bcount >> 8) & 0xff;
    > >>>> @@ -626,6 +627,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t
    > >>>> unsigned int timeout;
    > >>>> u32 tf_flags;
    > >>>> u16 bcount;
    > >>>> + u8 drq_int = !!(drive->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT);
    > >>>>
    > >>>>
    > >>> How about we finally add those check macros in block layer fashion like
    > >>> blk_pc_request et al and do
    > >>>
    > >>> #define drv_can_drq_interrupt(drive) ((drive)->atapi_flags &
    > >>> IDE_AFLAG_DRQ_INTERRUPT)
    > >>>
    > >>>
    > >> I suppose it's for the devices that interrupt on packet DRQ? Then it's
    > >> hardly a good name because it's not like this is some optional capability.
    > >>
    > >
    > > No, I was alluding to the command packet DRQ type used by the device as it is
    > > put in SFF8020i, 7.1.7.1 General Configuration Word.
    > >
    >
    > I was talking about exactly the same feature. :-)
    >
    > >>> or similar instead of wasting stack space?
    > >>>
    > >> It doesn't necessarily waste stack space. Haven't you heard about compiler
    > >> putting local vairables into registers?
    > >>
    > >
    > > Yes, have you heard of unnecessary register spilling?
    > >
    >
    > No -- only about stack spilling on CPUs "caching" the top of stack in
    > their register file (like SPARC).
    > Linux runs not only on x86 and many RISCs can store several local
    > variables in the dedicated registers -- it's the part of say MIPS ABIs...
    >
    > >>> It'll also read better in the if() check:
    > >>>
    > >>> if (drv_can_irq_interrupt(drive)) { ...
    > >>>
    > >>>
    > >> It's faster to checj a local variable than to dereference drv several times
    > >> -- unless gcc optimizes that away (by creating an implicit local variable
    > >> :-).
    > >>
    > >
    > > I hope gcc is smart enough to do that.
    > >
    >
    > Then where's the win?

    In having shorter & cleaner code:

    ide_dev_drq_int(drive)

    vs

    ((drive)->atapi_flags & IDE_AFLAG_DRQ_INTERRUPT)

    We can of course have both things -- using macros for all checks
    but still cache the result when it makes sense.

    The idea of having macros for ->dev_flags and ->atapi_flags sounds
    fine to me (+ it abstracts the actual ->dev_flags + ->atapi_flags
    split allowing easier sanitizations / enhancements later).

    Thanks,
    Bart
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2009-02-11 17:53    [W:0.034 / U:32.912 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site