Re: [PATCH] IDE TCQ #4

[Martin Dalecki]

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...