On Wednesday 11 February 2009, Sergei Shtylyov wrote: > Hello. > > Borislav Petkov wrote: > > >>>> From: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> > >>>> 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@xxxxxxxxx> > >>>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> > >>>> --- > >>>> 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-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html