[PATCH 1/2] sgiioc4: kill useless address checks

[PATCH 1/2] sgiioc4: kill useless address checks

[Sergei Shtylyov]

The driver performs a number of checks on the virtual/physical addresses which
would always evaluate as true:

- for sgiioc4_init_hwif_ports(), its caller, sgiioc4_ide_setup_pci_device(),
  guarantees that 'ctrl_port' and 'irq_port' parameters are never 0;

- in sgiioc4_read_status(), we always read the IDE status register, so there's
  no need to check the register's address (must be a leftover from the times
  when this function implemented the INB() method);

- in ide_dma_sgiioc4(), 'dma_base' can never be 0 as IOC4_DMA_OFFSET is not 0.

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
This patch is against the recent pata-2.6 series...
Phew, it's been around since August -- sending out at last...

 drivers/ide/pci/sgiioc4.c |   28 ++++++++++------------------
 1 files changed, 10 insertions(+), 18 deletions(-)

Index: linux-2.6/drivers/ide/pci/sgiioc4.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/sgiioc4.c
+++ linux-2.6/drivers/ide/pci/sgiioc4.c
@@ -101,11 +101,8 @@ sgiioc4_init_hwif_ports(hw_regs_t * hw, 
 	for (i = 0; i <= 7; i++)
 		hw->io_ports_array[i] = reg + i * 4;
 
-	if (ctrl_port)
-		hw->io_ports.ctl_addr = ctrl_port;
-
-	if (irq_port)
-		hw->io_ports.irq_addr = irq_port;
+	hw->io_ports.ctl_addr = ctrl_port;
+	hw->io_ports.irq_addr = irq_port;
 }
 
 static int
@@ -303,16 +300,14 @@ static u8 sgiioc4_read_status(ide_hwif_t
 	unsigned long port = hwif->io_ports.status_addr;
 	u8 reg = (u8) readb((void __iomem *) port);
 
-	if ((port & 0xFFF) == 0x11C) {	/* Status register of IOC4 */
-		if (!(reg & ATA_BUSY)) { /* Not busy... check for interrupt */
-			unsigned long other_ir = port - 0x110;
-			unsigned int intr_reg = (u32) readl((void __iomem *) other_ir);
-
-			/* Clear the Interrupt, Error bits on the IOC4 */
-			if (intr_reg & 0x03) {
-				writel(0x03, (void __iomem *) other_ir);
-				intr_reg = (u32) readl((void __iomem *) other_ir);
-			}
+	if (!(reg & ATA_BUSY)) {	/* Not busy... check for interrupt */
+		unsigned long other_ir = port - 0x110;
+		unsigned int intr_reg = (u32) readl((void __iomem *) other_ir);
+
+		/* Clear the Interrupt, Error bits on the IOC4 */
+		if (intr_reg & 0x03) {
+			writel(0x03, (void __iomem *) other_ir);
+			intr_reg = (u32) readl((void __iomem *) other_ir);
 		}
 	}
 
@@ -329,9 +324,6 @@ ide_dma_sgiioc4(ide_hwif_t *hwif, const 
 	int num_ports = sizeof (ioc4_dma_regs_t);
 	void *pad;
 
-	if (dma_base == 0)
-		return -1;
-
 	printk(KERN_INFO "    %s: MMIO-DMA\n", hwif->name);
 
 	if (request_mem_region(dma_base, num_ports, hwif->name) == NULL) {

Re: [PATCH 1/2] sgiioc4: kill useless address checks

[Sergei Shtylyov]

Hello, I wrote:

> The driver performs a number of checks on the virtual/physical addresses which
> would always evaluate as true:
>   

    Oops, s/true/false/, of course -- except in sgiioc4_read_status()

> - for sgiioc4_init_hwif_ports(), its caller, sgiioc4_ide_setup_pci_device(),
>   guarantees that 'ctrl_port' and 'irq_port' parameters are never 0;
>
> - in sgiioc4_read_status(), we always read the IDE status register, so there's
>   no need to check the register's address (must be a leftover from the times
>   when this function implemented the INB() method);
>
> - in ide_dma_sgiioc4(), 'dma_base' can never be 0 as IOC4_DMA_OFFSET is not 0.
>
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

MBR, Sergei

Re: [PATCH 1/2] sgiioc4: kill useless address checks

[Sergei Shtylyov]

Hello, I wrote:

>> would always evaluate as true:
>>   
> The driver performs a number of checks on the virtual/physical 
> addresses which
>
>    Oops, s/true/false/, of course -- except in sgiioc4_read_status()

   I got totally muddled. :-/
  The checks indeed evaluate as true except in ide_dma_sgiioc4().

>>   guarantees that 'ctrl_port' and 'irq_port' parameters are never 0;
>>
>> - in sgiioc4_read_status(), we always read the IDE status register, 
>> so there's
>>   no need to check the register's address (must be a leftover from 
>> the times
>>   when this function implemented the INB() method);
>>
>> - in ide_dma_sgiioc4(), 'dma_base' can never be 0 as IOC4_DMA_OFFSET 
>> is not 0.
>>
>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> - for sgiioc4_init_hwif_ports(), its caller, 
> sgiioc4_ide_setup_pci_device(),

MBR, Sergei

Re: [PATCH 1/2] sgiioc4: kill useless address checks

[Bartlomiej Zolnierkiewicz]

On Wednesday 15 October 2008, Sergei Shtylyov wrote:
> The driver performs a number of checks on the virtual/physical addresses which
> would always evaluate as true:
> 
> - for sgiioc4_init_hwif_ports(), its caller, sgiioc4_ide_setup_pci_device(),
>   guarantees that 'ctrl_port' and 'irq_port' parameters are never 0;
> 
> - in sgiioc4_read_status(), we always read the IDE status register, so there's
>   no need to check the register's address (must be a leftover from the times
>   when this function implemented the INB() method);
> 
> - in ide_dma_sgiioc4(), 'dma_base' can never be 0 as IOC4_DMA_OFFSET is not 0.
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

applied patches #1-2 (#1 w/ fixed patch description)

Re: [PATCH 1/2] sgiioc4: kill useless address checks

[Jeremy Higdon]

On Wed, Oct 15, 2008 at 11:29:19PM +0400, Sergei Shtylyov wrote:
> - in sgiioc4_read_status(), we always read the IDE status register, so there's
>   no need to check the register's address (must be a leftover from the times
>   when this function implemented the INB() method);

Correct.  That code was left over from this:

static u8
sgiioc4_INB(unsigned long port)
{
        u8 reg = (u8) inb(port);

        if ((port & 0xFFF) == 0x11C) {  /* Status register of IOC4 */
                if (reg & 0x51) {       /* Not busy...check for interrupt */
                        unsigned long other_ir = port - 0x110;
                        unsigned int intr_reg = (u32) inl(other_ir);

                        /* Clear the Interrupt, Error bits on the IOC4 */
                        if (intr_reg & 0x03) {
                                outl(0x03, other_ir);
                                intr_reg = (u32) inl(other_ir);
                        }
                }
        }

        return reg;     
}

Thanks for the cleanup.

jeremy