Re: [PATCH 00/19 v2] Improve IRQ remapping abstraction in x86 core code

[Joerg Roedel <joerg.roedel@amd.com>]

Hi Sebastian,

finally I am back from vacation and resume work on this :)

On Sun, Aug 26, 2012 at 09:17:49PM +0200, Sebastian Andrzej Siewior wrote:
> 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.

Thanks, and thanks for your review.

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

Okay, I am usually a bit conservative when it comes to checking function
pointers and conditions. One reason is that the Linux kernel had severe
security bugs in the past with NULL functions pointers being called.
Especially when future changes modify the code path these function
pointers it is easy to forget re-adding the checks. But I will look
again at the patches to check which checks can be removed and which
should better stay.

> 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)?

Well, when something goes wrong on resume and irq-remapping doesn't work
anymore the system is usually doomed (with and without this patch-set).
When irq-remapping does not work the dma-remapping will likely also not
work anymore which is even worse.

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632