Since Jan''s patch to print and mask bogus PIC vectors, we have found some issues on older hardware were supurious PIC vectors are being repeatedly logged, as spurious vectors will ignore the relevant mask bit. The log message is deceptive in the case of a spurious vector. I have attached an RFC patch which changes the bogus_8259A_irq logic to be able to detect spurious vectors and be rather less verbose about them. The new bogus_8259A_irq() function is basically a copy of _mask_and_ack_8259A_irq(), but returning a boolean indicating whether it was a genuine interrupt or not, which controls whether the "No irq handler" message in do_IRQ gets printed or not. Jan: are you happy with the style of the adjustment, or could you suggest a better way of doing it? -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On 28/08/2012 20:59, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:> Since Jan''s patch to print and mask bogus PIC vectors, we have found > some issues on older hardware were supurious PIC vectors are being > repeatedly logged, as spurious vectors will ignore the relevant mask bit. > > The log message is deceptive in the case of a spurious vector. I have > attached an RFC patch which changes the bogus_8259A_irq logic to be able > to detect spurious vectors and be rather less verbose about them. > > The new bogus_8259A_irq() function is basically a copy of > _mask_and_ack_8259A_irq(), but returning a boolean indicating whether it > was a genuine interrupt or not, which controls whether the "No irq > handler" message in do_IRQ gets printed or not. > > Jan: are you happy with the style of the adjustment, or could you > suggest a better way of doing it?No, you should make the change to _mask_and_ack_8259A_irq() itself, and callers which do not care about the return code can simply discard it. I hardly even want one instance of that appalling function in the tree, let alone two! Terrible abuse of goto, and I''m pretty laid back about goto use. And we don''t use True/False anywhere else in Xen. We don''t even have any centralised definition of true/false of any kind, so you should really just use 1/0 directly. Alternatively you could propose a patch to define true/false in our types.h, and use those. I don''t know where the capitalised True/False came from! -- Keir
On 28/08/12 22:32, Keir Fraser wrote:> On 28/08/2012 20:59, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote: > >> Since Jan''s patch to print and mask bogus PIC vectors, we have found >> some issues on older hardware were supurious PIC vectors are being >> repeatedly logged, as spurious vectors will ignore the relevant mask bit. >> >> The log message is deceptive in the case of a spurious vector. I have >> attached an RFC patch which changes the bogus_8259A_irq logic to be able >> to detect spurious vectors and be rather less verbose about them. >> >> The new bogus_8259A_irq() function is basically a copy of >> _mask_and_ack_8259A_irq(), but returning a boolean indicating whether it >> was a genuine interrupt or not, which controls whether the "No irq >> handler" message in do_IRQ gets printed or not. >> >> Jan: are you happy with the style of the adjustment, or could you >> suggest a better way of doing it? > No, you should make the change to _mask_and_ack_8259A_irq() itself, and > callers which do not care about the return code can simply discard it.Ok - I initially avoided that because _mask_and_ack_8259A_irq() is used to fill a function pointer structure, and preferred less change to the core. I will re-design somewhat with these points in mind. ~Andrew> > I hardly even want one instance of that appalling function in the tree, let > alone two! Terrible abuse of goto, and I''m pretty laid back about goto use. > > And we don''t use True/False anywhere else in Xen. We don''t even have any > centralised definition of true/false of any kind, so you should really just > use 1/0 directly. Alternatively you could propose a patch to define > true/false in our types.h, and use those. I don''t know where the capitalised > True/False came from! > > -- Keir > >-- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
On 29/08/2012 10:08, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:>> No, you should make the change to _mask_and_ack_8259A_irq() itself, and >> callers which do not care about the return code can simply discard it. > > Ok - I initially avoided that because _mask_and_ack_8259A_irq() is used > to fill a function pointer structure, and preferred less change to the core. > > I will re-design somewhat with these points in mind.Worst case rename to __mask_and_ack_8259A_irq() and implement new _mask_and_ack_8259A_irq() which simply calls it and discards the return code. Still much better than duplicating the function. -- Keir