linux-kernel - Re: [PATCH 11/15] ide: make ide_hwif_t.ide_dma_{host_off,off

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <45B281FA.9050107@ru.mvista.com>
Date:	Sat, 20 Jan 2007 23:56:26 +0300
From:	Sergei Shtylyov <sshtylyov@...mvista.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/15] ide: make ide_hwif_t.ide_dma_{host_off,off_quietly}
 void

Hello again. :-)

Bartlomiej Zolnierkiewicz wrote:

> [PATCH] ide: make ide_hwif_t.ide_dma_{host_off,off_quietly} void

    Below are my nits on the patch itself, and the code it changes.

> Index: b/drivers/ide/pci/atiixp.c
> ===================================================================
> --- a/drivers/ide/pci/atiixp.c
> +++ b/drivers/ide/pci/atiixp.c
> @@ -121,7 +121,7 @@ static int atiixp_ide_dma_host_on(ide_dr
>  	return __ide_dma_host_on(drive);
>  }
>  
> -static int atiixp_ide_dma_host_off(ide_drive_t *drive)
> +static void atiixp_ide_dma_host_off(ide_drive_t *drive)
>  {
>  	struct pci_dev *dev = drive->hwif->pci_dev;
>  	unsigned long flags;
[...]
> @@ -306,7 +306,7 @@ static void __devinit init_hwif_atiixp(i
>  		hwif->udma_four = 0;
>  
>  	hwif->ide_dma_host_on = &atiixp_ide_dma_host_on;
> -	hwif->ide_dma_host_off = &atiixp_ide_dma_host_off;
> +	hwif->dma_host_off = &atiixp_ide_dma_host_off;
>  	hwif->ide_dma_check = &atiixp_dma_check;
>  	if (!noautodma)
>  		hwif->autodma = 1;

    Would seem logical to get rid of ide_ in the function's name also...

> Index: b/drivers/ide/pci/sgiioc4.c
> ===================================================================
> --- a/drivers/ide/pci/sgiioc4.c
> +++ b/drivers/ide/pci/sgiioc4.c
> @@ -282,12 +282,11 @@ sgiioc4_ide_dma_on(ide_drive_t * drive)
>  	return HWIF(drive)->ide_dma_host_on(drive);
>  }
>  
> -static int
> -sgiioc4_ide_dma_off_quietly(ide_drive_t * drive)
> +static void sgiioc4_ide_dma_off_quietly(ide_drive_t *drive)
>  {
>  	drive->using_dma = 0;
>  
> -	return HWIF(drive)->ide_dma_host_off(drive);
> +	drive->hwif->dma_host_off(drive);
>  }
>  
>  static int sgiioc4_ide_dma_check(ide_drive_t *drive)
> @@ -317,12 +316,9 @@ sgiioc4_ide_dma_host_on(ide_drive_t * dr
>  	return 1;
>  }
>  
> -static int
> -sgiioc4_ide_dma_host_off(ide_drive_t * drive)
> +static void sgiioc4_ide_dma_host_off(ide_drive_t * drive)
>  {
>  	sgiioc4_clearirq(drive);
> -
> -	return 0;
>  }
>  
>  static int
> @@ -612,10 +608,10 @@ ide_init_sgiioc4(ide_hwif_t * hwif)
>  	hwif->ide_dma_end = &sgiioc4_ide_dma_end;
>  	hwif->ide_dma_check = &sgiioc4_ide_dma_check;
>  	hwif->ide_dma_on = &sgiioc4_ide_dma_on;
> -	hwif->ide_dma_off_quietly = &sgiioc4_ide_dma_off_quietly;
> +	hwif->dma_off_quietly = &sgiioc4_ide_dma_off_quietly;
>  	hwif->ide_dma_test_irq = &sgiioc4_ide_dma_test_irq;
>  	hwif->ide_dma_host_on = &sgiioc4_ide_dma_host_on;
> -	hwif->ide_dma_host_off = &sgiioc4_ide_dma_host_off;
> +	hwif->dma_host_off = &sgiioc4_ide_dma_host_off;
>  	hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
>  	hwif->ide_dma_timeout = &__ide_dma_timeout;

    The same here...

> Index: b/drivers/ide/pci/sl82c105.c
> ===================================================================
> --- a/drivers/ide/pci/sl82c105.c
> +++ b/drivers/ide/pci/sl82c105.c
> @@ -261,26 +261,24 @@ static int sl82c105_ide_dma_on (ide_driv
>  
>  	if (config_for_dma(drive)) {
>  		config_for_pio(drive, 4, 0, 0);

   Ugh, this forces PIO4 on fallback... and dma_on() doesn't select any modes 
in any other driver but this one. :-/

> -		return HWIF(drive)->ide_dma_off_quietly(drive);
> +		drive->hwif->dma_off_quietly(drive);
> +		return 0;
>  	}
>  	printk(KERN_INFO "%s: DMA enabled\n", drive->name);
>  	return __ide_dma_on(drive);
>  }
>  
> -static int sl82c105_ide_dma_off_quietly (ide_drive_t *drive)
> +static void sl82c105_ide_dma_off_quietly(ide_drive_t *drive)

    Also worth renaming...

>  {
>  	u8 speed = XFER_PIO_0;
> -	int rc;
> -	
> +
>  	DBG(("sl82c105_ide_dma_off_quietly(drive:%s)\n", drive->name));
>  
> -	rc = __ide_dma_off_quietly(drive);
> +	ide_dma_off_quietly(drive);
>  	if (drive->pio_speed)

    Should always be non-zero since explicitly initialized.

>  		speed = drive->pio_speed - XFER_PIO_0;
>  	config_for_pio(drive, speed, 0, 1);
>  	drive->current_speed = drive->pio_speed;

    dma_off() shouldn't be changing current_speed IMHO.

> -
> -	return rc;
>  }

    The patch to fix those two functions is also cooking...

MBR, Sergei

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/