icside: fix ->speedproc to return on unsupported modes

icside_set_speed() happily accepts unsupported transfer modes which results in drive->drive_data being set to the maximum value (480) and drive->current_speed being set to the unsupported transfer mode.

Fix it.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Hello.

Quoted 3 lines · Bartlomiej Zolnierkiewicz

icside_set_speed() happily accepts unsupported transfer modes which results in drive->drive_data being set to the maximum value (480) and drive->current_speed being set to the unsupported transfer mode.

Quoted 3 lines

Fix it.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Acked-by Sergei Shtylyov <[email protected]>

Quoted 5 lines

Quoted 5 lines

icside_set_speed() happily accepts unsupported transfer modes which results in drive->drive_data being set to the maximum value (480) and drive->current_speed being set to the unsupported transfer mode.

Fix it.

Quoted 3 lines

Quoted 1 line

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Acked-by Sergei Shtylyov <[email protected]>

I wonder why Sergei's acking this patch - do you have the hardware to test it on? Are you somehow involved in this driver? I think the answer to both is most likely no.

Moreover, I think the patch is quite broken. If an invalid DMA mode

is passed, currently the driver sets the cycle time to 480ns (stored in drive_data) since both cycle_time and use_dma_info will be zero.

Moreover, 'on' will be zero, causing icside_set_speed() to return zero

which means the DMA setting was not successful (iow, use PIO).

This causes icside_dma_check() to return -1 to the IDE layer which should fail the setting.

With your patch, you make icside_set_speed() return '1' meaning DMA was succesfully configured. That then causes icside_dma_check() to

return success to the IDE layer, so we now allow invalid speeds.

Ergo, what wasn't broken before is now broken. So the patch is completely wrong and was trying to fix a problem which didn't exist in the first place.

BIG NAK.

Quoted 10 lines · Sergei Shtylyov

Hello.

Quoted 5 lines · Bartlomiej Zolnierkiewicz

icside_set_speed() happily accepts unsupported transfer modes which results in drive->drive_data being set to the maximum value (480) and drive->current_speed being set to the unsupported transfer mode.

Fix it.

Quoted 3 lines

Quoted 1 line

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Acked-by Sergei Shtylyov <[email protected]>

I wonder why Sergei's acking this patch - do you have the hardware to test it on? Are you somehow involved in this driver? I think the answer to both is most likely no.

Moreover, I think the patch is quite broken. If an invalid DMA mode is passed, currently the driver sets the cycle time to 480ns (stored in drive_data) since both cycle_time and use_dma_info will be zero. Moreover, 'on' will be zero, causing icside_set_speed() to return zero which means the DMA setting was not successful (iow, use PIO).

This causes icside_dma_check() to return -1 to the IDE layer which should fail the setting.

With your patch, you make icside_set_speed() return '1' meaning DMA was succesfully configured. That then causes icside_dma_check() to return success to the IDE layer, so we now allow invalid speeds.

Ergo, what wasn't broken before is now broken. So the patch is completely wrong and was trying to fix a problem which didn't exist in the first place.

BIG NAK.

Quoted 5 lines

Moreover, I think the patch is quite broken. If an invalid DMA mode is passed, currently the driver sets the cycle time to 480ns (stored in drive_data) since both cycle_time and use_dma_info will be zero. Moreover, 'on' will be zero, causing icside_set_speed() to return zero which means the DMA setting was not successful (iow, use PIO).

Thanks for spotting this. The patch in the current form is indeed broken as I haven't noticed that icside_set_speed() returns zero on failure.

However all other implementations of ->speedproc return zero on success and non-zero on failure. Currently it doesn't matter for icside host driver and isn't a bug per se since:

- ide_set_xfer_rate() return value is ignored by all IDE core users

- icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()

but sooner or later we will need to fix anyway - so lets do it now.

It also seems to cleanup icside_dma_check() a bit since return value invertion is no longer needed.

[PATCH] icside: fix ->speedproc to return on unsupported modes (take 2)

* All other implementations of ->speedproc return zero on success and non-zero on failure. Currently it doesn't matter for icside host driver and isn't a bug per se since:

- ide_set_xfer_rate() return value is ignored by all IDE core users

- icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()

but sooner or later we will need to fix anyway - so lets do it now.

* icside_set_speed() happily accepts unsupported transfer modes which results in drive->drive_data being set to the maximum value (480) and drive->current_speed being set to the unsupported transfer mode.

Fix it.

v2:
- The initial version of the patch was broken because it didn't take into the account (the different from usual) return values of icside_set_speed(). (Noticed by Russell).

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

drivers/ide/arm/icside.c +2 -0

 	switch (xfer_mode) {
 	case XFER_MW_DMA_2:

@@ -272,6 +272,8 @@ static int icside_set_speed(ide_drive_t 
 	case XFER_SW_DMA_0:
 		cycle_time = 480;
 		break;
+	default:
+		return 1;
 	}
 
 	/*

Hello.

Quoted 8 lines · Bartlomiej Zolnierkiewicz

Quoted 5 lines

Moreover, I think the patch is quite broken. If an invalid DMA mode is passed, currently the driver sets the cycle time to 480ns (stored in drive_data) since both cycle_time and use_dma_info will be zero. Moreover, 'on' will be zero, causing icside_set_speed() to return zero which means the DMA setting was not successful (iow, use PIO).

Thanks for spotting this. The patch in the current form is indeed broken as I haven't noticed that icside_set_speed() returns zero on failure.

You're not the only one.

Quoted 5 lines

However all other implementations of ->speedproc return zero on success and non-zero on failure. Currently it doesn't matter for icside host driver and isn't a bug per se since:

- ide_set_xfer_rate() return value is ignored by all IDE core users

Indeed. So my accusation of breaking the usermode interface (and UltraDMA
downgrade) was too rash. :-<

Quoted 1 line

- icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()

Yeah, that was rash too. :-<

Quoted 14 lines

but sooner or later we will need to fix anyway - so lets do it now.

It also seems to cleanup icside_dma_check() a bit since return value invertion is no longer needed.

[PATCH] icside: fix ->speedproc to return on unsupported modes (take 2)

* All other implementations of ->speedproc return zero on success and non-zero on failure. Currently it doesn't matter for icside host driver and isn't a bug per se since:

- ide_set_xfer_rate() return value is ignored by all IDE core users

Quoted 7 lines

- icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()

but sooner or later we will need to fix anyway - so lets do it now.

* icside_set_speed() happily accepts unsupported transfer modes which results in drive->drive_data being set to the maximum value (480) and drive->current_speed being set to the unsupported transfer mode.

    I don't understand why it needs to bother setting drive->current_speed at 

all to begin with.

Quoted 8 lines

Fix it.

v2:
- The initial version of the patch was broken because it didn't take into the account (the different from usual) return values of icside_set_speed(). (Noticed by Russell).

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Looks like the patch still needs more polishing:

drivers/ide/arm/icside.c static void icside_build_sglist(ide_driv · line 253

Quoted 10 lines

Index: b/drivers/ide/arm/icside.c
===================================================================

drivers/ide/arm/icside.c +1 -1

@@ -250,7 +250,7 @@ static void icside_build_sglist(ide_driv
  */
 static int icside_set_speed(ide_drive_t *drive, const u8 xfer_mode)
 {
-	int on = 0, cycle_time = 0, use_dma_info = 0;
+	int on = -1, cycle_time = 0, use_dma_info = 0;

We don't need to pre-initialize cycle_time anymore.

Quoted 12 lines

drivers/ide/arm/icside.c +2 -0

 	switch (xfer_mode) {
 	case XFER_MW_DMA_2:

@@ -272,6 +272,8 @@ static int icside_set_speed(ide_drive_t 
 	case XFER_SW_DMA_0:
 		cycle_time = 480;
 		break;
+	default:
+		return 1;
 	}
 
 	/*

drivers/ide/arm/icside.c static int icside_set_speed(ide_drive_t · line 288

Quoted 3 lines

drivers/ide/arm/icside.c +0 -0

@@ -284,7 +286,7 @@ static int icside_set_speed(ide_drive_t 
 	drive->drive_data = cycle_time;
 
 	if (cycle_time && ide_config_drive_speed(drive, xfer_mode) == 0)

Now cycle_time can't be 0 here, so the first condition may be dropped.

drivers/ide/arm/icside.c static int icside_set_speed(ide_drive_t · line 291

Quoted 4 lines

drivers/ide/arm/icside.c +1 -1

-		on = 1;
+		on = 0;
 	else
 		drive->drive_data = 480;

I'm not sure that this really makes sense now.

weeeeee, take 3

v3:
* Remove no longer needed initialization/checking of cycle_time (Noticed by Sergei).

* No need to set drive->drive_data if DMA is not going to be used (Noticed by Sergei).

* Remove incorrect setting of drive->current_speed (Noticed by Sergei).

* Move ide_config_drive_speed() at the end of icside_set_speed().

Quoted 1 line · Bartlomiej Zolnierkiewicz

weeeeee, take 3

I'm probably going to drop this driver as soon as the PATA people get their finger(s) out and respond to the issues I raised when merging the pata_icside driver.

Quoted 7 lines

v3:
* Remove no longer needed initialization/checking of cycle_time (Noticed by Sergei).

* No need to set drive->drive_data if DMA is not going to be used (Noticed by Sergei).

Quoted 4 lines

* Remove incorrect setting of drive->current_speed (Noticed by Sergei).

* Move ide_config_drive_speed() at the end of icside_set_speed().

What happens if we set a DMA mode but ide_config_drive_speed() fails? Wouldn't we set drive_data to the timing for that mode and start to use it for future DMA accesses?

Quoted 1 line · Bartlomiej Zolnierkiewicz

weeeeee, take 3

I'm probably going to drop this driver as soon as the PATA people get their finger(s) out and respond to the issues I raised when merging the pata_icside driver.

Quoted 6 lines · On Fri, 13 Jul 2007 22:39:48 +0100 Russell King <[email protected]> wrote:

Quoted 1 line · Bartlomiej Zolnierkiewicz

weeeeee, take 3

I'm probably going to drop this driver as soon as the PATA people get their finger(s) out and respond to the issues I raised when merging the pata_icside driver.

Which issues are you still waiting solutions for ?

We need to set the IRQ enable initially so that the identity information can be read and the drive accessed etc. However, if no drives are detected on a port, there is no callback into the low level driver - the port_disable method is never called. (I believe it is with SATA.)

I tried asking about this several times before the driver was merged and every time it was completely ignored.

Quoted 4 lines

We need to set the IRQ enable initially so that the identity information can be read and the drive accessed etc. However, if no drives are detected on a port, there is no callback into the low level driver - the port_disable method is never called. (I believe it is with SATA.)

No. It actually makes no sense to do so as libata is hot plug capable thus the usual case is "no drives are currently detected"

Quoted 2 lines

I tried asking about this several times before the driver was merged and every time it was completely ignored.

Provide a private postreset method thats

static void my_postreset(struct ata_port *ap, unsigned int *classes)

{
	if (classes[0] == ATA_DEV_NONE && classes[1] == ATA_DEV_NONE) {
		force IRQ off [should be off anyway]
	} else {
		return ata_std_postreset(ap, classes);
	}
}

Quoted 14 lines · Alan Cox

Quoted 4 lines

We need to set the IRQ enable initially so that the identity information can be read and the drive accessed etc. However, if no drives are detected on a port, there is no callback into the low level driver - the port_disable method is never called. (I believe it is with SATA.)

No. It actually makes no sense to do so as libata is hot plug capable thus the usual case is "no drives are currently detected"

Quoted 2 lines

I tried asking about this several times before the driver was merged and every time it was completely ignored.

Provide a private postreset method thats

Quoted 8 lines

static void my_postreset(struct ata_port *ap, unsigned int *classes)

{
	if (classes[0] == ATA_DEV_NONE && classes[1] == ATA_DEV_NONE) {
		force IRQ off [should be off anyway]
	} else {
		return ata_std_postreset(ap, classes);
	}
}

So something like this?

drivers/ata/pata_icside.c +11 -17

index c791a46..91bdb01 100644
@@ -330,17 +330,12 @@ static void ata_dummy_noret(struct ata_port *port)
 {
 }
 
-/*
- * We need to shut down unused ports to prevent spurious interrupts.
- * FIXME: the libata core doesn't call this function for PATA interfaces.
- */
-static void pata_icside_port_disable(struct ata_port *ap)
+static void pata_icside_postreset(struct ata_port *ap, unsigned int *classes)
 {
 	struct pata_icside_state *state = ap->host->private_data;
 
-	ata_port_printk(ap, KERN_ERR, "disabling icside port\n");
-
-	ata_port_disable(ap);
+	if (classes[0] != ATA_DEV_NONE || classes[1] != ATA_DEV_NONE)
+		return ata_std_postreset(ap, classes);
 
 	state->port[ap->port_no].disabled = 1;
 

@@ -356,6 +351,12 @@ static void pata_icside_port_disable(struct ata_port *ap)
 	}
 }
 
+static void pata_icside_error_handler(struct ata_port *ap)
+{
+	ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, NULL,
+			   pata_icside_postreset);
+}
+
 static u8 pata_icside_irq_ack(struct ata_port *ap, unsigned int chk_drq)
 {
 	unsigned int bits = chk_drq ? ATA_BUSY | ATA_DRQ : ATA_BUSY;

@@ -374,7 +375,7 @@ static u8 pata_icside_irq_ack(struct ata_port *ap, unsigned int chk_drq)
 }
 
 static struct ata_port_operations pata_icside_port_ops = {
-	.port_disable		= pata_icside_port_disable,
+	.port_disable		= ata_port_disable,
 
 	.set_dmamode		= pata_icside_set_dmamode,
 

@@ -397,7 +398,7 @@ static struct ata_port_operations pata_icside_port_ops = {
 
 	.freeze			= ata_bmdma_freeze,
 	.thaw			= ata_bmdma_thaw,
-	.error_handler		= ata_bmdma_error_handler,
+	.error_handler		= pata_icside_error_handler,
 	.post_internal_cmd	= pata_icside_bmdma_stop,
 
 	.irq_clear		= ata_dummy_noret,

@@ -484,13 +485,6 @@ static int __devinit pata_icside_register_v6(struct pata_icside_info *info)
 	state->port[0].port_sel = sel;
 	state->port[1].port_sel = sel | 1;
 
-	/*
-	 * FIXME: work around libata's aversion to calling port_disable.
-	 * This permanently disables interrupts on port 0 - bad luck if
-	 * you have a drive on that port.
-	 */
-	state->port[0].disabled = 1;
-
 	info->base = easi_base;
 	info->irqops = &pata_icside_ops_arcin_v6;
 	info->nr_ports = 2;

[PATCH] icside: fix ->speedproc to return on unsupported modes (take 3)

* All other implementations of ->speedproc return zero on success and non-zero on failure. Currently it doesn't matter for icside host driver and isn't a bug per se since:

- ide_set_xfer_rate() return value is ignored by all IDE core users

- icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()

but sooner or later we will need to fix anyway - so lets do it now.

* icside_set_speed() happily accepts unsupported transfer modes which results in drive->drive_data being set to the maximum value (480) and drive->current_speed being set to the unsupported transfer mode.

Fix it.

v2:
* The initial version of the patch was broken because it didn't take into the account (the different from usual) return values of icside_set_speed() (Noticed by Russell).

v3:
* Remove no longer needed initialization/checking of cycle_time (Noticed by Sergei).

* No need to set drive->drive_data if DMA is not going to be used (Noticed by Sergei).

* Remove incorrect setting of drive->current_speed (Noticed by Sergei).

* Move ide_config_drive_speed() at the end of icside_set_speed().

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Quoted 29 lines · Bartlomiej Zolnierkiewicz

[PATCH] icside: fix ->speedproc to return on unsupported modes (take 3)

* All other implementations of ->speedproc return zero on success and non-zero on failure. Currently it doesn't matter for icside host driver and isn't a bug per se since:

- ide_set_xfer_rate() return value is ignored by all IDE core users

- icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()

but sooner or later we will need to fix anyway - so lets do it now.

* icside_set_speed() happily accepts unsupported transfer modes which results in drive->drive_data being set to the maximum value (480) and drive->current_speed being set to the unsupported transfer mode.

Fix it.

v2:
* The initial version of the patch was broken because it didn't take into the account (the different from usual) return values of icside_set_speed() (Noticed by Russell).

v3:
* Remove no longer needed initialization/checking of cycle_time (Noticed by Sergei).

* No need to set drive->drive_data if DMA is not going to be used (Noticed by Sergei).

Quoted 6 lines

* Remove incorrect setting of drive->current_speed (Noticed by Sergei).

* Move ide_config_drive_speed() at the end of icside_set_speed().

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Acked-by Sergei Shtylyov <[email protected]>