Hi,
On Friday 20 April 2007, Sergei Shtylyov wrote:
> Hello, I wrote:
>
> >>>> Index: b/drivers/ide/pci/hpt366.c
> >>>> ===================================================================
> >>>> --- a/drivers/ide/pci/hpt366.c
> >>>> +++ b/drivers/ide/pci/hpt366.c
> >>>> @@ -513,43 +513,31 @@ static int check_in_drive_list(ide_drive
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -static u8 hpt3xx_ratemask(ide_drive_t *drive)
> >>>> -{
> >>>> - struct hpt_info *info = pci_get_drvdata(HWIF(drive)->pci_dev);
> >>>> - u8 mode = info->max_mode;
> >>>> -
> >>>> - if (!eighty_ninty_three(drive) && mode)
> >>>> - mode = min(mode, (u8)1);
> >>>> - return mode;
> >>>> -}
> >>>> -
> >>>> /*
> >>>> * Note for the future; the SATA hpt37x we must set
> >>>> * either PIO or UDMA modes 0,4,5
> >>>> */
> >>>> - -static u8 hpt3xx_ratefilter(ide_drive_t *drive, u8 speed)
> >>>> +
> >>>> +static u8 hpt3xx_udma_filter(ide_drive_t *drive)
> >>>> {
> >>>> struct hpt_info *info = pci_get_drvdata(HWIF(drive)->pci_dev);
> >>>> u8 chip_type = info->chip_type;
> >>>> - u8 mode = hpt3xx_ratemask(drive);
> >>>> -
> >>>> - if (drive->media != ide_disk)
> >>>> - return min(speed, (u8)XFER_PIO_4);
> >>>> + u8 mode = info->max_mode;
> >>>> + u8 mask;
> >>>>
> >>>> switch (mode) {
> >>>> case 0x04:
> >>>> - speed = min_t(u8, speed, XFER_UDMA_6);
> >>>> + mask = 0x7f;
> >>>> break;
> >>>> case 0x03:
> >>>> - speed = min_t(u8, speed, XFER_UDMA_5);
> >>>> + mask = 0x3f;
> >>>> if (chip_type >= HPT374)
> >>>> break;
> >>>> if (!check_in_drive_list(drive, bad_ata100_5))
> >>>> goto check_bad_ata33;
> >>>> /* fall thru */
> >>>> case 0x02:
> >>>> - speed = min_t(u8, speed, XFER_UDMA_4);
> >>>> + mask = 0x1f;
> >>>>
> >>>> /*
> >>>> * CHECK ME, Does this need to be changed to HPT374 ??
> >>>> @@ -560,13 +548,13 @@ static u8 hpt3xx_ratefilter(ide_drive_t
> >>>> !check_in_drive_list(drive, bad_ata66_4))
> >>>> goto check_bad_ata33;
> >>>>
> >>>> - speed = min_t(u8, speed, XFER_UDMA_3);
>
> Hm, found a buglet in my former filtering rewrite -- the condition in the preceding if stmt should be a reverse one, and speed limitation to XFER_UDMA_3 should have been left under it. With the current code, XFER_UDMA_2 limitation wouldn't have been applied if the same drive is not in both 'bad_ata66_4' and 'bad_ata66_3' lists -- this, however, actually is not the case since WDC AC310200R drive is in both these lists (maybe I wrote it this way because of this fact :-).
IIRC I've noticed this during the review of the filtering rewrite
but I though that it was meant to be this way. :)
> >>>> + mask = 0x0f;
> >>>> if (HPT366_ALLOW_ATA66_3 &&
> >>>> !check_in_drive_list(drive, bad_ata66_3))
> >>>> goto check_bad_ata33;
> >>>> /* fall thru */
> >>>> case 0x01:
> >>>> - speed = min_t(u8, speed, XFER_UDMA_2);
> >>>> + mask = 0x07;
> >>>>
> >>>> check_bad_ata33:
> >>>> if (chip_type >= HPT370A)
>
> >>> This case 0x01 will *never* be hit for HPT370 chip with the new
> >>> code, therefore the filter won't get applied.
>
> >> Oh, and for HPT36x chips used with 40c cable too (unless they're
> >> artificaially reduced to UltraDMA/33 by the driver #define's).
>
> > It will still get applied since the code always resorts to looking up
> > the 'bad_ata33' list for HPT36x/370.
> > I've got a bit muddled in my own code -- not sure if it got much clearer
> > after I'd untangled hpt3xx_ratemask() / hpt3xx_ratefilter() puzzle. :-)
>
> Yeah, I'm definitely having trouble understanding my own code after some months have passed... :-/
The filtering code badly needs more comments/documentation
and it was already true for the old code (before your rewrite).
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
