Re: [PATCH] IDE TCQ #4

Martin Dalecki
2002-04-15T13:41:44Z

Jens Axboe wrote:

>>>- memset(ar, 0, sizeof(*ar));
>>>+ memset(ar, 0, sizeof(ar));
>>
>>Please look closer - I'm quite convinced that sizeof(ar) == sizeof(void *)
>>which gives not the desired effect. I have fixed this during the last
>>merge...

No problem here - you see I'm quite good at catching cr*ap like this ;-).

> Irk, where did that come from?! Had you done the incremental patches
> this would not have slipped through! My mistake.

>>>
>>> __set_bit(i, &tag_mask);
>>> len += sprintf(out+len, "%d, ", i);
>>>- if (ar->ar_time > max_jif)
>>>- max_jif = ar->ar_time;
>>>+ if (cur_jif - ar->ar_time > max_jif)
>>>+ max_jif = cur_jif - ar->ar_time;
>>
>>I disgust timer calculartions...- will have to look at a way
>>how to nap in a similar way eth drivers do.
>
>
> ? This is just statistics. There absolutely nothing wrong with the
> above.

Please don't take me wrong - I don't think that there is something
wrong with it... It was more out of the stommach about how it looks...

>>>diff -urN -X /home/axboe/cdrom/exclude
>>>/opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c linux/drivers/ide/ide-dma.c
>>>--- /opt/kernel/linux-2.5.8/drivers/ide/ide-dma.c 2002-04-15
>>>07:59:53.000000000 +0200
>>>+++ linux/drivers/ide/ide-dma.c 2002-04-15 09:17:55.000000000 +0200
>>>
>>>+#endif /* CONFIG_BLK_DEV_IDE_TCQ */
>>>
>>> case ide_dma_read:
>>> reading = 1 << 3;
>>> case ide_dma_write:
>>>- ar = HWGROUP(drive)->rq->special;
>>>+ ar = IDE_CUR_AR(drive);
>>
>>Ahhh!!! I'm gald to see this.
>
>
> :-)
>
>
>>>static inline void drive_ctl_nien(ide_drive_t *drive, int clear)
>>>{
>>>#ifdef IDE_TCQ_NIEN
>>>- int mask = clear ? 0 : 2;
>>>+ int mask = clear ? 0 : 1 << 1;
>>
>>????

> Just more readable that we are setting bit 2.

0x02 is for me always a hint that one should read it binary...
You memmorazied the bit patterns corresopnding to hex digits propably
as well already a long time ago. Didn't you? :-).

>
>
>>>-#define IDE_CUR_AR(drive) \
>>>- ((drive)->using_tcq ? IDE_CUR_TAG((drive)) :
>>>HWGROUP((drive))->rq->special)
>>>+#define IDE_CUR_AR(drive) (HWGROUP((drive))->rq->special)
>>
>>Ahh that's nice :-). Let's look further down how this coexists with
>>ide-cd.c...
>>
>>
>>
>>>-extern inline int ide_get_tag(ide_drive_t *drive)
>>>+static inline int ide_get_tag(ide_drive_t *drive)
>>
>>OK. I have missed this one apparently.
>>
>>ar_timer will make it at least esier to finish the ide-cd.c transition
>>to this data transport base.
>>
>>Should I just quickly remerge this, so we can work further from
>>the same code base to ease the merging pain?
>
>
> Sure, go ahead. And do the changes they way we discussed earlier today,
> ok? :)

OK... agreed First - "raw" merge (and fixes already discuesed here) and
then go on...