| Message ID | 200906232329.11648.bzolnier@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ;). -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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).. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);