| Date | Sun, 26 Aug 2012 21:17:49 +0200 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH 00/19 v2] Improve IRQ remapping abstraction in x86 core code |
| |
On Mon, Aug 20, 2012 at 03:55:46PM +0200, Joerg Roedel wrote: > Please review.
Finally. Usually you don't add/change code but you just move common irq remapping pieces out of geric io-apic code and put them in once place. I think it would be good, if you would note this in the description of your patch. Altogether it makes a good impression.
After browsing through the new functions in irq_remapping_modify_x86_ops() I see that some of them test for "remap_ops" which is pointless because you don't call irq_remapping_modify_x86_ops() if it is not the case. This also goes mostly for irq_remapping_enabled. The only reason when you can disable (or say irq_remapping_disable() is called) is in the suspend path. And the remap is enabled again in via irq_remapping_reenable() in resume. Now if this goes wrong what is next? You don't even return an error if the callback is missing. The variable irq_remapping_enabled does not save your ass here because some function behave now different.
But back to the realisitic world: If something goes wrong in resume and you can't re-enable irq remapping, the system is not really useable or is it (even before your series)?
> Thanks, > > Joerg >
Sebastian
|