[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/3] x86/irq: remove duplicated clear_domain_irq_pirq calls
On 26.01.2021 17:08, Roger Pau Monné wrote: > On Tue, Jan 26, 2021 at 03:38:27PM +0100, Jan Beulich wrote: >> On 26.01.2021 12:06, Roger Pau Monne wrote: >>> There are two duplicated calls to cleanup_domain_irq_pirq in >>> unmap_domain_pirq. >>> >>> The first one in the for loop will be called with exactly the same >>> arguments as the call placed closer to the loop start. >> >> I'm having trouble figuring out which two instances you refer >> to: To me "first one" and "closer to the start" are two ways >> of expressing the same thing. You don't refer to the call to >> clear_domain_irq_pirq(), do you, when the two calls you >> remove are to cleanup_domain_irq_pirq()? Admittedly quite >> similar names, but entirely different functions. > > Urg, yes, that's impossible to parse sensibly, sorry. > > Also the subject is wrong, should be cleanup_domain_irq_pirq, not > clear_domain_irq_pirq. > > This should instead be: > > "There are two duplicated calls to cleanup_domain_irq_pirq in > unmap_domain_pirq. > > The first removed call to cleanup_domain_irq_pirq will be called with > exactly the same arguments as the previous call placed above it." And which one is "the previous call"? I'm still getting the impression you're mixing up cleanup_domain_irq_pirq() and clear_domain_irq_pirq(). (I guess we need to resort to line numbers in current staging or master, to avoid misunderstandings. Not for the commit message [if any], but for the discussion.) > It's hard to explain this in a commit message. > > Do you agree that the removed calls are duplicated though? I might have > as well missed part of the logic here and be wrong about this. No, for the moment I don't agree yet, because I don't see the redundancy so far. >>> The logic used in the loop seems extremely complex to follow IMO, >>> there are several breaks for exiting the loop, and the index (i) is >>> also updated in different places. >> >> Indeed, and it didn't feel well already back at the time when >> I much extended this to support multi-vector MSI. I simply >> didn't have any good idea how to streamline all of this >> (short of rewriting it altogether, which would have made >> patch review quite difficult). If you have thoughts, I'm all >> ears. > > I would just rewrite it altogether. Code like this is very prone to > cause mistakes in the future IMO. If you want I can try to. I wouldn't mind, but yes, besides review difficulties ... > I guess the problem with this is that we would lose the history of the > code for no functional change. ... this also wouldn't be overly nice (but can be dealt with via a few more steps wading through git history). Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |