From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bartlomiej Zolnierkiewicz Subject: Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd() Date: Wed, 11 Feb 2009 17:37:07 +0100 Message-ID: <200902111737.07551.bzolnier@gmail.com> References: <20090209231945.32406.14874.sendpatchset@localhost.localdomain> <9ea470500902110537g4d6c67a3o35d1d8a4dcca0927@mail.gmail.com> <4992D77F.3090802@ru.mvista.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-fx0-f20.google.com ([209.85.220.20]:50389 "EHLO mail-fx0-f20.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756510AbZBKQt6 convert rfc822-to-8bit (ORCPT ); Wed, 11 Feb 2009 11:49:58 -0500 In-Reply-To: <4992D77F.3090802@ru.mvista.com> Content-Disposition: inline Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Sergei Shtylyov Cc: petkovbb@gmail.com, linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org On Wednesday 11 February 2009, Sergei Shtylyov wrote: > Hello. >=20 > Borislav Petkov wrote: >=20 > >>>> From: Bartlomiej Zolnierkiewicz > >>>> 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 > >>>> Signed-off-by: Bartlomiej Zolnierkiewicz > >>>> --- > >>>> 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 > >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > >>>> --- 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 =3D dma ? ATAPI_PROT_DMA : ATAPI_PROT_PIO; > >>>> cmd->tf_flags |=3D IDE_TFLAG_OUT_LBAH | IDE_TFLAG_OUT_LBA= M | > >>>> IDE_TFLAG_OUT_FEATURE | tf_flags; > >>>> + cmd->tf.command =3D ATA_CMD_PACKET; > >>>> cmd->tf.feature =3D dma; /* Use PIO/DMA */ > >>>> cmd->tf.lbam =3D bcount & 0xff; > >>>> cmd->tf.lbah =3D (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 =3D !!(drive->atapi_flags & IDE_AFLAG_DRQ_INT= ERRUPT); > >>>> > >>>> =20 > >>> How about we finally add those check macros in block layer fashio= n like > >>> blk_pc_request et al and do > >>> > >>> #define drv_can_drq_interrupt(drive) ((drive)->atapi_flags & > >>> IDE_AFLAG_DRQ_INTERRUPT) > >>> > >>> =20 > >> 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 cap= ability. > >> =20 > > > > No, I was alluding to the command packet DRQ type used by the devic= e as it is > > put in SFF8020i, 7.1.7.1 General Con=EF=AC=81guration Word. > > =20 >=20 > I was talking about exactly the same feature. :-) >=20 > >>> or similar instead of wasting stack space? > >>> =20 > >> It doesn't necessarily waste stack space. Haven't you heard about= compiler > >> putting local vairables into registers? > >> =20 > > > > Yes, have you heard of unnecessary register spilling? > > =20 >=20 > No -- only about stack spilling on CPUs "caching" the top of stack= in=20 > their register file (like SPARC). > Linux runs not only on x86 and many RISCs can store several local=20 > variables in the dedicated registers -- it's the part of say MIPS ABI= s... >=20 > >>> It'll also read better in the if() check: > >>> > >>> if (drv_can_irq_interrupt(drive)) { ... > >>> > >>> =20 > >> It's faster to checj a local variable than to dereference drv sev= eral times > >> -- unless gcc optimizes that away (by creating an implicit local v= ariable > >> :-). > >> =20 > > > > I hope gcc is smart enough to do that. > > =20 >=20 > 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