[PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

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

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Russell King <rmk@arm.linux.org.uk>
---
 drivers/ide/arm/icside.c |    2 ++
 1 file changed, 2 insertions(+)

Index: b/drivers/ide/arm/icside.c
===================================================================
--- a/drivers/ide/arm/icside.c
+++ b/drivers/ide/arm/icside.c
@@ -272,6 +272,8 @@ static int icside_set_speed(ide_drive_t 
 	case XFER_SW_DMA_0:
 		cycle_time = 480;
 		break;
+	default:
+		return 1;
 	}
 
 	/*

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

> 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 <bzolnier@gmail.com>

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

MBR, Sergei

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Russell King]

On Wed, Jul 11, 2007 at 11:07:51PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >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 <bzolnier@gmail.com>
> 
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Sergei Shtylyov]

Hello.

Russell King wrote:

>>>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 <bzolnier@gmail.com>

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

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

    This just signified my positive review (as for any other IDE patch).

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

    In what way I wonder?

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

    Why bother doing this?  The speedproc() method shouldn't even try to 
handle an unsupopoted mode, to begin with.,,

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

    It means just the opposite for the speedproc() -- i.e. success.

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

    You're mixing with the ide_dma_check() method.

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

    Then the speedproc() method's return value and the ide_dma_check() method 
must be fixed too -- NAK the patch. I haven't noticed so far that this driver 
is more broken than the others, sorry. :-)

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

    The return value was wrong from the very start -- any non-zero result 
should mean error.

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

    What it does *now* is returning bad result to ide_tune_dma() and the 
possible userspace callers.

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

    More like incomplete fix to an existing problem.

> BIG NAK.

    Indeed. It needs a bigger hammer. ;-)

MBR, Sergei

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 11 July 2007, Russell King wrote:
> On Wed, Jul 11, 2007 at 11:07:51PM +0400, Sergei Shtylyov wrote:
> > Hello.
> > 
> > Bartlomiej Zolnierkiewicz wrote:
> > 
> > >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 <bzolnier@gmail.com>
> > 
> > Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
> 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.

It is a _friendly_ ACK == "I have reviewed the patch and it is OK".

Sergei has proven that he has both the knowledge and the good code taste
to _friendly_ ACK IDE patches.  Extra review from him is valuable thing.

I don't think that he had any intentions in overriding your _final_
judgement wrt icside host driver. :)

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

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

Yep, you are right.

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

The problem exists - currently host driver's ->speedproc can be fed with
the unsupported transfer mode both from user space requests and from internal
IDE subsystem error handling (when dropping to PIO mode).  My other patches
are heavily fixing the problem in the IDE core but still a few cases remain to
be fixed thus this patch.

> BIG NAK.

Are you OK with "take 2"?

[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 <bzolnier@gmail.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
 drivers/ide/arm/icside.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Index: b/drivers/ide/arm/icside.c
===================================================================
--- a/drivers/ide/arm/icside.c
+++ b/drivers/ide/arm/icside.c
@@ -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;
 
 	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;
 	}
 
 	/*
@@ -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)
-		on = 1;
+		on = 0;
 	else
 		drive->drive_data = 480;
 
@@ -321,7 +323,6 @@ static int icside_dma_check(ide_drive_t 
 	struct hd_driveid *id = drive->id;
 	ide_hwif_t *hwif = HWIF(drive);
 	int xfer_mode = XFER_PIO_2;
-	int on;
 
 	if (!(id->capability & 1) || !hwif->autodma)
 		goto out;
@@ -350,9 +351,7 @@ static int icside_dma_check(ide_drive_t 
 	}
 
 out:
-	on = icside_set_speed(drive, xfer_mode);
-
-	return on ? 0 : -1;
+	return icside_set_speed(drive, xfer_mode);
 }
 
 static int icside_dma_end(ide_drive_t *drive)

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

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

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

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

    Yeah, that was rash too. :-<

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

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

>   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 <bzolnier@gmail.com>

    Looks like the patch still needs more polishing:

> Index: b/drivers/ide/arm/icside.c
> ===================================================================
> --- a/drivers/ide/arm/icside.c
> +++ b/drivers/ide/arm/icside.c
> @@ -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.

>  	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;
>  	}
>  
>  	/*
> @@ -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.

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

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

MBR, Sergei

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Bartlomiej Zolnierkiewicz]

On Thursday 12 July 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>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.
> 
> > 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. :-<
> 
> > - icside doesn't (yet!) use ide_tune_dma() in icside_dma_check()
> 
>     Yeah, that was rash too. :-<
> 
> > 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.
> 
>     I don't understand why it needs to bother setting drive->current_speed at 
> all to begin with.
> 
> >   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 <bzolnier@gmail.com>
> 
>     Looks like the patch still needs more polishing:
> 
> > Index: b/drivers/ide/arm/icside.c
> > ===================================================================
> > --- a/drivers/ide/arm/icside.c
> > +++ b/drivers/ide/arm/icside.c
> > @@ -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.
> 
> >  	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;
> >  	}
> >  
> >  	/*
> > @@ -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.
> 
> > -		on = 1;
> > +		on = 0;
> >  	else
> >  		drive->drive_data = 480;
> 
>     I'm not sure that this really makes sense now.

weeeeee, take 3

[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 <bzolnier@gmail.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
 drivers/ide/arm/icside.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

Index: b/drivers/ide/arm/icside.c
===================================================================
--- a/drivers/ide/arm/icside.c
+++ b/drivers/ide/arm/icside.c
@@ -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 cycle_time, use_dma_info = 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;
 	}
 
 	/*
@@ -283,17 +285,10 @@ static int icside_set_speed(ide_drive_t 
 
 	drive->drive_data = cycle_time;
 
-	if (cycle_time && ide_config_drive_speed(drive, xfer_mode) == 0)
-		on = 1;
-	else
-		drive->drive_data = 480;
-
 	printk("%s: %s selected (peak %dMB/s)\n", drive->name,
 		ide_xfer_verbose(xfer_mode), 2000 / drive->drive_data);
 
-	drive->current_speed = xfer_mode;
-
-	return on;
+	return ide_config_drive_speed(drive, xfer_mode);
 }
 
 static void icside_dma_host_off(ide_drive_t *drive)
@@ -321,7 +316,6 @@ static int icside_dma_check(ide_drive_t 
 	struct hd_driveid *id = drive->id;
 	ide_hwif_t *hwif = HWIF(drive);
 	int xfer_mode = XFER_PIO_2;
-	int on;
 
 	if (!(id->capability & 1) || !hwif->autodma)
 		goto out;
@@ -350,9 +344,7 @@ static int icside_dma_check(ide_drive_t 
 	}
 
 out:
-	on = icside_set_speed(drive, xfer_mode);
-
-	return on ? 0 : -1;
+	return icside_set_speed(drive, xfer_mode);
 }
 
 static int icside_dma_end(ide_drive_t *drive)

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Russell King]

On Fri, Jul 13, 2007 at 11:02:03PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 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.

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

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?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Bartlomiej Zolnierkiewicz]

On Friday 13 July 2007, Russell King wrote:
> On Fri, Jul 13, 2007 at 11:02:03PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > 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.
> 
> > 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().
> 
> 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?

If ide_config_drive_speed() fails then icside_dma_check() also fails
so IDE core doesn't enable DMA.  Therefore we never start DMA engine
and icside_dma_setup() (the only place which reads drive->drive_data)
is never called.

Thanks,
Bart

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Alan Cox]

On Fri, 13 Jul 2007 22:39:48 +0100
Russell King <rmk@arm.linux.org.uk> wrote:

> On Fri, Jul 13, 2007 at 11:02:03PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > 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 ?

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Russell King]

On Sat, Jul 14, 2007 at 12:15:23AM +0100, Alan Cox wrote:
> On Fri, 13 Jul 2007 22:39:48 +0100
> Russell King <rmk@arm.linux.org.uk> wrote:
> 
> > On Fri, Jul 13, 2007 at 11:02:03PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > 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 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)

Essentially, the interrupt lines to the drives end up floating if no
drives are attached (don't ask why they didn't fit the resistors.)
This leads to a great number of spurious interrupts if the hardware
is left with the individual channel IRQ enables set.

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.  Eventually, I gave up and
merged the driver as is, with this disgusting hack in:

        /*
         * 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;

so at least it worked here.  The consequence of that is if you attach a
drive to port 0, the interrupts from that drive will be lost unless you
edit the driver.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Alan Cox]

> 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"
 
> 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);
	}
}

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

> [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 <bzolnier@gmail.com>

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

MBR, Sergei

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Russell King]

On Sat, Jul 14, 2007 at 12:54:35AM +0100, Alan Cox wrote:
> > 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"
>  
> > 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);
> 	}
> }

So something like this?

diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index c791a46..91bdb01 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -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;


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Bartlomiej Zolnierkiewicz]

On Saturday 14 July 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> > [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 <bzolnier@gmail.com>
> 
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

added

Russell, are you OK with this revision?

Thanks,
Bart

Re: [PATCH 1/6] icside: fix ->speedproc to return on unsupported modes

[Russell King]

On Sat, Jul 14, 2007 at 08:15:59PM +0100, Russell King wrote:
> On Sat, Jul 14, 2007 at 12:54:35AM +0100, Alan Cox wrote:
> > > 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"
> >  
> > > 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);
> > 	}
> > }
> 
> So something like this?

I never got a response on this, so it hasn't gone any further.

> diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
> index c791a46..91bdb01 100644
> --- a/drivers/ata/pata_icside.c
> +++ b/drivers/ata/pata_icside.c
> @@ -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;

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of: