[patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] ide: allow ide_dev_read_id() to be called from the IRQ context

* Un-static __ide_wait_stat().

* Allow ide_dev_read_id() helper to be called from the IRQ context by
  adding irq_ctx flag and using mdelay()/__ide_wait_stat() when needed.

* Switch ide_driveid_update() to set irq_ctx flag.

This change is needed for the consecutive patch which fixes races in
handling of user-space SET XFER commands but for improved bisectability
and clarity it is better to do it in a separate patch.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
I'm not completely happy with this version of patch since I originally
intended to unify ide_busy_sleep() with __ide_wait_stat() first..

 drivers/ide/ide-iops.c  |    6 +++---
 drivers/ide/ide-probe.c |   31 +++++++++++++++++++++----------
 include/linux/ide.h     |    3 ++-
 3 files changed, 26 insertions(+), 14 deletions(-)

Index: b/drivers/ide/ide-iops.c
===================================================================
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -102,8 +102,8 @@ EXPORT_SYMBOL(ide_fixstring);
  * setting a timer to wake up at half second intervals thereafter,
  * until timeout is achieved, before timing out.
  */
-static int __ide_wait_stat(ide_drive_t *drive, u8 good, u8 bad,
-			   unsigned long timeout, u8 *rstat)
+int __ide_wait_stat(ide_drive_t *drive, u8 good, u8 bad,
+		    unsigned long timeout, u8 *rstat)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
@@ -316,7 +316,7 @@ int ide_driveid_update(ide_drive_t *driv
 		return 0;
 
 	SELECT_MASK(drive, 1);
-	rc = ide_dev_read_id(drive, ATA_CMD_ID_ATA, id);
+	rc = ide_dev_read_id(drive, ATA_CMD_ID_ATA, id, 1);
 	SELECT_MASK(drive, 0);
 
 	if (rc)
Index: b/drivers/ide/ide-probe.c
===================================================================
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -238,6 +238,7 @@ static void do_identify(ide_drive_t *dri
  *	@drive: drive to identify
  *	@cmd: command to use
  *	@id: buffer for IDENTIFY data
+ *	@irq_ctx: flag set when called from the IRQ context
  *
  *	Sends an ATA(PI) IDENTIFY request to a drive and waits for a response.
  *
@@ -246,7 +247,7 @@ static void do_identify(ide_drive_t *dri
  *			2  device aborted the command (refused to identify itself)
  */
 
-int ide_dev_read_id(ide_drive_t *drive, u8 cmd, u16 *id)
+int ide_dev_read_id(ide_drive_t *drive, u8 cmd, u16 *id, int irq_ctx)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	struct ide_io_ports *io_ports = &hwif->io_ports;
@@ -263,7 +264,10 @@ int ide_dev_read_id(ide_drive_t *drive, 
 		tp_ops->write_devctl(hwif, ATA_NIEN | ATA_DEVCTL_OBS);
 
 	/* take a deep breath */
-	msleep(50);
+	if (irq_ctx)
+		mdelay(50);
+	else
+		msleep(50);
 
 	if (io_ports->ctl_addr &&
 	    (hwif->host_flags & IDE_HFLAG_BROKEN_ALTSTATUS) == 0) {
@@ -295,12 +299,19 @@ int ide_dev_read_id(ide_drive_t *drive, 
 
 	timeout = ((cmd == ATA_CMD_ID_ATA) ? WAIT_WORSTCASE : WAIT_PIDENTIFY) / 2;
 
-	if (ide_busy_sleep(drive, timeout, use_altstatus))
-		return 1;
-
 	/* wait for IRQ and ATA_DRQ */
-	msleep(50);
-	s = tp_ops->read_status(hwif);
+	if (irq_ctx) {
+		rc = __ide_wait_stat(drive, ATA_DRQ, BAD_R_STAT, timeout, &s);
+		if (rc)
+			return 1;
+	} else {
+		rc = ide_busy_sleep(drive, timeout, use_altstatus);
+		if (rc)
+			return 1;
+
+		msleep(50);
+		s = tp_ops->read_status(hwif);
+	}
 
 	if (OK_STAT(s, ATA_DRQ, BAD_R_STAT)) {
 		/* drive returned ID */
@@ -406,10 +417,10 @@ static int do_probe (ide_drive_t *drive,
 
 	if (OK_STAT(stat, ATA_DRDY, ATA_BUSY) ||
 	    present || cmd == ATA_CMD_ID_ATAPI) {
-		rc = ide_dev_read_id(drive, cmd, id);
+		rc = ide_dev_read_id(drive, cmd, id, 0);
 		if (rc)
 			/* failed: try again */
-			rc = ide_dev_read_id(drive, cmd, id);
+			rc = ide_dev_read_id(drive, cmd, id, 0);
 
 		stat = tp_ops->read_status(hwif);
 
@@ -424,7 +435,7 @@ static int do_probe (ide_drive_t *drive,
 			msleep(50);
 			tp_ops->exec_command(hwif, ATA_CMD_DEV_RESET);
 			(void)ide_busy_sleep(drive, WAIT_WORSTCASE, 0);
-			rc = ide_dev_read_id(drive, cmd, id);
+			rc = ide_dev_read_id(drive, cmd, id, 0);
 		}
 
 		/* ensure drive IRQ is clear */
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1081,6 +1081,7 @@ extern void ide_fixstring(u8 *, const in
 
 int ide_busy_sleep(ide_drive_t *, unsigned long, int);
 
+int __ide_wait_stat(ide_drive_t *, u8, u8, unsigned long, u8 *);
 int ide_wait_stat(ide_startstop_t *, ide_drive_t *, u8, u8, unsigned long);
 
 ide_startstop_t ide_do_park_unpark(ide_drive_t *, struct request *);
@@ -1169,7 +1170,7 @@ int ide_no_data_taskfile(ide_drive_t *, 
 
 int ide_taskfile_ioctl(ide_drive_t *, unsigned long);
 
-int ide_dev_read_id(ide_drive_t *, u8, u16 *);
+int ide_dev_read_id(ide_drive_t *, u8, u16 *, int);
 
 extern int ide_driveid_update(ide_drive_t *);
 extern int ide_config_drive_speed(ide_drive_t *, u8);

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 23 Jun 2009 23:29:11 +0200

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] ide: allow ide_dev_read_id() to be called from the IRQ context
> 
> * Un-static __ide_wait_stat().
> 
> * Allow ide_dev_read_id() helper to be called from the IRQ context by
>   adding irq_ctx flag and using mdelay()/__ide_wait_stat() when needed.
> 
> * Switch ide_driveid_update() to set irq_ctx flag.
> 
> This change is needed for the consecutive patch which fixes races in
> handling of user-space SET XFER commands but for improved bisectability
> and clarity it is better to do it in a separate patch.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> I'm not completely happy with this version of patch since I originally
> intended to unify ide_busy_sleep() with __ide_wait_stat() first..

This and patch #5 seems way overkill for the problem being solved.

All requests programmed into the device get synchronized through the
block request queue, even these raw taskfile commands.

It seems to me that it should be possible to use the block request
layer facilities to plug the queue, wait all executing commands to
complete and for the device to quiesce, then push this single RAW
command from the user to the device, and finally unplug the queue.

Alternatively, if the block request layer can't help, we can put a
mutex around submissions, and have this user RAW command submission
take that mutex.

Anything other than moving code that does significant delays into
interrupt context.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 June 2009 01:22:56 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Tue, 23 Jun 2009 23:29:11 +0200
> 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] ide: allow ide_dev_read_id() to be called from the IRQ context
> > 
> > * Un-static __ide_wait_stat().
> > 
> > * Allow ide_dev_read_id() helper to be called from the IRQ context by
> >   adding irq_ctx flag and using mdelay()/__ide_wait_stat() when needed.
> > 
> > * Switch ide_driveid_update() to set irq_ctx flag.
> > 
> > This change is needed for the consecutive patch which fixes races in
> > handling of user-space SET XFER commands but for improved bisectability
> > and clarity it is better to do it in a separate patch.
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> > I'm not completely happy with this version of patch since I originally
> > intended to unify ide_busy_sleep() with __ide_wait_stat() first..
> 
> This and patch #5 seems way overkill for the problem being solved.

They fix a real bug, they do it in a clean way and their total impact on
the code source is 40 lines _changed_ (not a single line of "new" code)..

They remove disable_irq_nosync()/enable_irq() use as an added bonus..

> All requests programmed into the device get synchronized through the
> block request queue, even these raw taskfile commands.
> 
> It seems to me that it should be possible to use the block request
> layer facilities to plug the queue, wait all executing commands to
> complete and for the device to quiesce, then push this single RAW
> command from the user to the device, and finally unplug the queue.

Probably.  These patches are in the accordance with the new policy
of "the minimal impact and the maximum outcome".

> Alternatively, if the block request layer can't help, we can put a
> mutex around submissions, and have this user RAW command submission
> take that mutex.

Feel free to try other approaches..

These patches were just some bugfixes that I was working recently
and thought that it would be a good idea to finish them and send them
to you (possibly saving you some work in the future) instead of simply
deleting them (like I did with cleanup patches that I was working on).

I will not cry if they got rejected for whatever reason (valid or not)
or try to push them to Linus independently..

IOW if there are really some technical issues left to be addressed with
these patches I'll be happy to address them but I'm not doing any more
new IDE stuff.  I can still help with reviewing or explaining things from
time to time but you are the one responsible for the end effect now..

> Anything other than moving code that does significant delays into
> interrupt context.

The code in question is only for a special commands doing user requested
xfer mode changes (they shall not be used at all in the first place because
the kernel does all the needed job nowadays).

IOW these patches only affect users of 'hdparm -X' (which is hardly an IRQ
latency problem ;).

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Wed, 24 Jun 2009 03:36:24 +0200

> IOW if there are really some technical issues left to be addressed with
> these patches I'll be happy to address them but I'm not doing any more
> new IDE stuff.

Discussing alternative approaches to fixing the problem is a technical
issue.

If you just want to dump your pending fixes and have no interest in
anything other than minor fixups and typo cures, then it's likely most
of these patches will be used only as guides for others rather than
being applied.

Specifically in this case I really think this is a very unclean way to
solve this problem, which is why I suggested alternative
implementations in the first place.

Anything that requires a set of if() blocks checking "in interrupt
context" or not is a big red flag in my book.

But I understand now that you're not willing to work on such things,
which I'll happily respect.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 June 2009 06:35:35 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Wed, 24 Jun 2009 03:36:24 +0200
> 
> > IOW if there are really some technical issues left to be addressed with
> > these patches I'll be happy to address them but I'm not doing any more
> > new IDE stuff.
> 
> Discussing alternative approaches to fixing the problem is a technical
> issue.
> 
> If you just want to dump your pending fixes and have no interest in
> anything other than minor fixups and typo cures, then it's likely most
> of these patches will be used only as guides for others rather than
> being applied.

I'm perfectly happy with it.

> Specifically in this case I really think this is a very unclean way to
> solve this problem, which is why I suggested alternative
> implementations in the first place.
> 
> Anything that requires a set of if() blocks checking "in interrupt
> context" or not is a big red flag in my book.

It is also a big red flag in my book.  However after long hours put into
analysis of the issue I came to conclusions that they are needed/justified
in this specific case unless we are going to do really major surgery there
(which we shouldn't according to the new policy).

You've put less than 2h (because that was the time since my post till your
reply) into analysis of the bug, the related problems and the solution.

It could be that if you had put a bit more time into it and/or asked detailed
technical questions related to the solution (i.e. "Why x needs to be there
and we can't do y?") instead of keeping the technical discussion on the very
vague level (which sounded like "can't we use block layer to process block
requests because drive commands are block requests and raw commands are drive
commands so we should use block layer") you would come to very different
conclusions than you did initially.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Wed, 24 Jun 2009 11:51:16 +0200

> You've put less than 2h (because that was the time since my post till your
> reply) into analysis of the bug, the related problems and the solution.
> 
> It could be that if you had put a bit more time into it and/or asked detailed
> technical questions related to the solution (i.e. "Why x needs to be there
> and we can't do y?") instead of keeping the technical discussion on the very
> vague level (which sounded like "can't we use block layer to process block
> requests because drive commands are block requests and raw commands are drive
> commands so we should use block layer") you would come to very different
> conclusions than you did initially.

I'm investigating alternative ways to this problem, less because
of the time I put into the investigation, but moreso because I'm
able to put fresh eyes onto the problem.

And also I don't know the IDE code as well as you do, which is
an advantage insofar as not falling into "oh I know how this
stuff works, therefore we can't..." kinds of mental traps.

Back to the technical side, ignoring the interrupt-or-not uglies,
there are two other reasons why using synchronization is better
here:

1) You want the device to be quiescent anyways when you do this
   SET_XFER command.  What better way than to plug the queue
   and make sure all currently outstanding requests complete?

   And as already discussed, we even already have logic to support
   this kind of thing for the sake of power-management.

2) All commands going into the device do so from a context from
   which we could take a sleeping lock such as a mutex.  It's
   therefore the most natural way to synchonize things.

I know you're busy, so I'll try to draft something up myself.

Thanks.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 June 2009 11:55:18 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Wed, 24 Jun 2009 11:51:16 +0200
> 
> > You've put less than 2h (because that was the time since my post till your
> > reply) into analysis of the bug, the related problems and the solution.
> > 
> > It could be that if you had put a bit more time into it and/or asked detailed
> > technical questions related to the solution (i.e. "Why x needs to be there
> > and we can't do y?") instead of keeping the technical discussion on the very
> > vague level (which sounded like "can't we use block layer to process block
> > requests because drive commands are block requests and raw commands are drive
> > commands so we should use block layer") you would come to very different
> > conclusions than you did initially.
> 
> I'm investigating alternative ways to this problem, less because
> of the time I put into the investigation, but moreso because I'm
> able to put fresh eyes onto the problem.

Fresh look is always good, the problem is that you seem to be walking
on the thin line between fresh look and ignorance a bit too often at
the moment.  Yeah.. I know that this is temporary and that I'm acting
like an old IDE geezer.. ;)

> And also I don't know the IDE code as well as you do, which is
> an advantage insofar as not falling into "oh I know how this
> stuff works, therefore we can't..." kinds of mental traps.

We are heavily constrained by the difficult hardware, by the way other
kernel subsystems work, by some legacy user-space interfaces, by some
complex/fragile code parts and now also by the policy.

> Back to the technical side, ignoring the interrupt-or-not uglies,
> there are two other reasons why using synchronization is better
> here:
> 
> 1) You want the device to be quiescent anyways when you do this
>    SET_XFER command.  What better way than to plug the queue
>    and make sure all currently outstanding requests complete?
> 
>    And as already discussed, we even already have logic to support
>    this kind of thing for the sake of power-management.

Power management requests are kind of special and need block layer support.

Please take a look at REQ_DEVSET_EXEC special requests from ide-devsets.c
instead if you would like to investigate possibility of a cleaner (although
more invasive) solution.

> 2) All commands going into the device do so from a context from
>    which we could take a sleeping lock such as a mutex.  It's
>    therefore the most natural way to synchonize things.

The fact is that we need to synchronize against all commands going into
the device and they are synchronized using block queue (which is protected
with spinlock by block layer). Adding an extra mutex (even if possible)
into IDE request processing hot-path solely to fix this particular bug
would be an overkill from performance and the future maintainability POVs.

> I know you're busy, so I'll try to draft something up myself.

Ok, thanks.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 June 2009 12:48:20 Bartlomiej Zolnierkiewicz wrote:

> > 1) You want the device to be quiescent anyways when you do this
> >    SET_XFER command.  What better way than to plug the queue
> >    and make sure all currently outstanding requests complete?
> > 
> >    And as already discussed, we even already have logic to support
> >    this kind of thing for the sake of power-management.
> 
> Power management requests are kind of special and need block layer support.
> 
> Please take a look at REQ_DEVSET_EXEC special requests from ide-devsets.c
> instead if you would like to investigate possibility of a cleaner (although
> more invasive) solution.
> 
> > 2) All commands going into the device do so from a context from
> >    which we could take a sleeping lock such as a mutex.  It's
> >    therefore the most natural way to synchonize things.
> 
> The fact is that we need to synchronize against all commands going into
> the device and they are synchronized using block queue (which is protected
> with spinlock by block layer). Adding an extra mutex (even if possible)

We need to also take the synchronization between block queues of all devices
on the port and the serialized ports into account..  This is quite complex
and fragile code (vide cmd64x screaming IRQ issue ;)..

I would personally try going with 1) and avoid 2) at all costs..

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 June 2009 13:04:14 Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 24 June 2009 12:48:20 Bartlomiej Zolnierkiewicz wrote:
> 
> > > 1) You want the device to be quiescent anyways when you do this
> > >    SET_XFER command.  What better way than to plug the queue
> > >    and make sure all currently outstanding requests complete?

BTW this is the way we do it currently.

We just back-ride on ide_finish_cmd() call for the special user-initiated
REQ_TYPE_ATA_TASKFILE request.  Doing it the other way will only result in:

* shorter IRQ latency for user-space requested xfer mode change request
  (irrelevant as the whole interface is just a rarely used legacy)

* much more invasive changes to the IDE core (I would be more than happy
  to welcome them but this is against our newly established policy)

Anyway I'll be happy with whatever solution that will end up in Linus tree
and is OK from the correctness POV.

> > >    And as already discussed, we even already have logic to support
> > >    this kind of thing for the sake of power-management.
> > 
> > Power management requests are kind of special and need block layer support.
> > 
> > Please take a look at REQ_DEVSET_EXEC special requests from ide-devsets.c
> > instead if you would like to investigate possibility of a cleaner (although
> > more invasive) solution.
> > 
> > > 2) All commands going into the device do so from a context from
> > >    which we could take a sleeping lock such as a mutex.  It's
> > >    therefore the most natural way to synchonize things.
> > 
> > The fact is that we need to synchronize against all commands going into
> > the device and they are synchronized using block queue (which is protected
> > with spinlock by block layer). Adding an extra mutex (even if possible)
> 
> We need to also take the synchronization between block queues of all devices
> on the port and the serialized ports into account..  This is quite complex
> and fragile code (vide cmd64x screaming IRQ issue ;)..
> 
> I would personally try going with 1) and avoid 2) at all costs..

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 24 June 2009 12:48:20 Bartlomiej Zolnierkiewicz wrote:
> 
>>> 1) You want the device to be quiescent anyways when you do this
>>>    SET_XFER command.  What better way than to plug the queue
>>>    and make sure all currently outstanding requests complete?
>>>
>>>    And as already discussed, we even already have logic to support
>>>    this kind of thing for the sake of power-management.
>> Power management requests are kind of special and need block layer support.
>>
>> Please take a look at REQ_DEVSET_EXEC special requests from ide-devsets.c
>> instead if you would like to investigate possibility of a cleaner (although
>> more invasive) solution.
>>
>>> 2) All commands going into the device do so from a context from
>>>    which we could take a sleeping lock such as a mutex.  It's
>>>    therefore the most natural way to synchonize things.
>> The fact is that we need to synchronize against all commands going into
>> the device and they are synchronized using block queue (which is protected
>> with spinlock by block layer). Adding an extra mutex (even if possible)
> 
> We need to also take the synchronization between block queues of all devices
> on the port and the serialized ports into account..  This is quite complex
> and fragile code (vide cmd64x screaming IRQ issue ;)..
> 
> I would personally try going with 1) and avoid 2) at all costs..

FWIW, on Promise chipsets, SETFEATURES_XFER requires synchronization 
across all ports on the controller, otherwise SETFEATURES_XFER on port M 
can potentially cause data corruption on unrelated port N.

This has to do with the snooping of ATA commands performed by the 
Promise PATA and SATA chips.  The chip sees SETFEATURES_XFER, and does a 
bit of internal programming.

Silicon Image (siimage or sata_sil) also snoops ATA commands such as 
SETFEATURES_XFER; ditto a few other chipsets.

SETFEATURES_XFER has always required very, very careful, often 
chipset-specific handling.

	Jeff

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 June 2009 21:30:40 Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 24 June 2009 12:48:20 Bartlomiej Zolnierkiewicz wrote:
> > 
> >>> 1) You want the device to be quiescent anyways when you do this
> >>>    SET_XFER command.  What better way than to plug the queue
> >>>    and make sure all currently outstanding requests complete?
> >>>
> >>>    And as already discussed, we even already have logic to support
> >>>    this kind of thing for the sake of power-management.
> >> Power management requests are kind of special and need block layer support.
> >>
> >> Please take a look at REQ_DEVSET_EXEC special requests from ide-devsets.c
> >> instead if you would like to investigate possibility of a cleaner (although
> >> more invasive) solution.
> >>
> >>> 2) All commands going into the device do so from a context from
> >>>    which we could take a sleeping lock such as a mutex.  It's
> >>>    therefore the most natural way to synchonize things.
> >> The fact is that we need to synchronize against all commands going into
> >> the device and they are synchronized using block queue (which is protected
> >> with spinlock by block layer). Adding an extra mutex (even if possible)
> > 
> > We need to also take the synchronization between block queues of all devices
> > on the port and the serialized ports into account..  This is quite complex
> > and fragile code (vide cmd64x screaming IRQ issue ;)..
> > 
> > I would personally try going with 1) and avoid 2) at all costs..
> 
> FWIW, on Promise chipsets, SETFEATURES_XFER requires synchronization 
> across all ports on the controller, otherwise SETFEATURES_XFER on port M 
> can potentially cause data corruption on unrelated port N.
> 
> This has to do with the snooping of ATA commands performed by the 
> Promise PATA and SATA chips.  The chip sees SETFEATURES_XFER, and does a 
> bit of internal programming.
> 
> Silicon Image (siimage or sata_sil) also snoops ATA commands such as 
> SETFEATURES_XFER; ditto a few other chipsets.
> 
> SETFEATURES_XFER has always required very, very careful, often 
> chipset-specific handling.

Thanks for the reminder.

David, we should probably disallow user-space requested transfer mode
changes on some chipsets (especially Promise ones) in IDE until somebody
provides the needed synchronization infrastructure for handling such
special cases (libata avoids the issue by not allowing any user-space
requested transfer mode changes).  However since this is an independent
issue from IRQ handler vs user context races it would be best done in
the separate patch.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Wednesday 24 June 2009 12:48:20 Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 24 June 2009 11:55:18 David Miller wrote:

[...]

> > I know you're busy, so I'll try to draft something up myself.
> 
> Ok, thanks.

Any news about these fixes?

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Wed, 1 Jul 2009 18:35:27 +0200

> On Wednesday 24 June 2009 12:48:20 Bartlomiej Zolnierkiewicz wrote:
>> On Wednesday 24 June 2009 11:55:18 David Miller wrote:
> 
> [...]
> 
>> > I know you're busy, so I'll try to draft something up myself.
>> 
>> Ok, thanks.
> 
> Any news about these fixes?

Will get to it before the end of the weekend.

If I fail or abort, I'll integrate your version of the fix Bart, don't
worry :)

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Wednesday 01 July 2009 22:47:29 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Wed, 1 Jul 2009 18:35:27 +0200
> 
> > On Wednesday 24 June 2009 12:48:20 Bartlomiej Zolnierkiewicz wrote:
> >> On Wednesday 24 June 2009 11:55:18 David Miller wrote:
> > 
> > [...]
> > 
> >> > I know you're busy, so I'll try to draft something up myself.
> >> 
> >> Ok, thanks.
> > 
> > Any news about these fixes?
> 
> Will get to it before the end of the weekend.
> 
> If I fail or abort, I'll integrate your version of the fix Bart, don't
> worry :)

Not that I'm worried but just out of curiosity..

Could you please tell me which week/month/year are we talking about here?

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Fri, 7 Aug 2009 13:55:21 +0200

> On Wednesday 01 July 2009 22:47:29 David Miller wrote:
>> Will get to it before the end of the weekend.
>> 
>> If I fail or abort, I'll integrate your version of the fix Bart, don't
>> worry :)
> 
> Not that I'm worried but just out of curiosity..
> 
> Could you please tell me which week/month/year are we talking about here?

I became busier than I expected here in New York :-)

Actually both patches #4 and #5 are in my ide-next-2.6 tree, I just
need to publish it out to kernel.org and update the patchwork status
on these patches.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Friday 07 August 2009 18:01:58 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Fri, 7 Aug 2009 13:55:21 +0200
> 
> > On Wednesday 01 July 2009 22:47:29 David Miller wrote:
> >> Will get to it before the end of the weekend.
> >> 
> >> If I fail or abort, I'll integrate your version of the fix Bart, don't
> >> worry :)
> > 
> > Not that I'm worried but just out of curiosity..
> > 
> > Could you please tell me which week/month/year are we talking about here?
> 
> I became busier than I expected here in New York :-)

I would say that this is a bit unprofessional attitude which normally
is not accepted by PNAELV people.. :-)

> Actually both patches #4 and #5 are in my ide-next-2.6 tree, I just
> need to publish it out to kernel.org and update the patchwork status
> on these patches.

Are you trying to tell me that after 6 weeks there is still no public
tree to hold future IDE changes?

If this is indeed a case here I hope that it will get addressed ASAP.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Fri, 7 Aug 2009 19:27:36 +0200

> Are you trying to tell me that after 6 weeks there is still no public
> tree to hold future IDE changes?
> 
> If this is indeed a case here I hope that it will get addressed ASAP.

IDE is deep maintainence, we shouldn't be doing lots of changes,
cleanups, and things of this nature.  So urgency and timeliness
is on a sincerely different scale for it.

On the other hand, if you've been looking, I've been applying
networking patches to net-2.6 and net-next-2.6 by the truckload
on a daily basis because it is in active development.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Friday 07 August 2009 19:36:10 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Fri, 7 Aug 2009 19:27:36 +0200
> 
> > Are you trying to tell me that after 6 weeks there is still no public
> > tree to hold future IDE changes?
> > 
> > If this is indeed a case here I hope that it will get addressed ASAP.
> 
> IDE is deep maintainence, we shouldn't be doing lots of changes,
> cleanups, and things of this nature.  So urgency and timeliness
> is on a sincerely different scale for it.

Nobody is saying anything about cleanups and things of this nature..

> On the other hand, if you've been looking, I've been applying
> networking patches to net-2.6 and net-next-2.6 by the truckload
> on a daily basis because it is in active development.

Excellence in one area doesn't magically translate to other areas,
especially when said area needs a completely different approach:

I've been looking and I've seen you pushing non-trivial IDE bug-fix
patches with almost no wider hardware coverage to upstream (i.e. one
of my own patches which has been _explicitly_ marked as needing more
linux-next exposure)..

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Fri, 7 Aug 2009 19:46:07 +0200

> I've been looking and I've seen you pushing non-trivial IDE bug-fix
> patches with almost no wider hardware coverage to upstream (i.e. one
> of my own patches which has been _explicitly_ marked as needing more
> linux-next exposure)..

"When are the patches to be applied?"

"You're a hyprocrite for applying these patches without hardware
 regression testing you said is necessary."

Ok, you're position is clear :-)

Anyways, the ide-next-2.6 tree is up on kernel.org for all to
enjoy and test, I look forward to everyone's feedback.

Re: [patch 4/6] ide: allow ide_dev_read_id() to be called from the IRQ context

[Bartlomiej Zolnierkiewicz]

On Friday 07 August 2009 20:09:52 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Fri, 7 Aug 2009 19:46:07 +0200
> 
> > I've been looking and I've seen you pushing non-trivial IDE bug-fix
> > patches with almost no wider hardware coverage to upstream (i.e. one
> > of my own patches which has been _explicitly_ marked as needing more
> > linux-next exposure)..

Please note that the above incident caused unexpected/urgent work for
me so I would very much prefer to avoid such things in the future..

> "When are the patches to be applied?"
> 
> "You're a hyprocrite for applying these patches without hardware
>  regression testing you said is necessary."
> 
> Ok, you're position is clear :-)

Those are your own words (not mine), same goes for interpretation.. :-)

> Anyways, the ide-next-2.6 tree is up on kernel.org for all to
> enjoy and test, I look forward to everyone's feedback.

Thanks!