On Sun, Aug 16, 2020 at 11:33:14AM +0300, Andy Shevchenko wrote: > On Sat, Aug 15, 2020 at 4:50 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > On Sat, Aug 15, 2020 at 12:55:57AM +0300, Andy Shevchenko wrote: > > > On Friday, August 14, 2020, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > > > On Fri, Aug 14, 2020 at 09:07:20PM +0300, Andy Shevchenko wrote: > > > > > On Fri, Aug 14, 2020 at 06:18:16PM +0100, John Garry wrote: > > ... > > > > > > > - usb_put_hcd(hcd); > > > > > > if ((hcd->driver->flags & HCD_MASK) < HCD_USB3) > > > > > > pci_free_irq_vectors(dev); > > > > > > + usb_put_hcd(hcd); > > > > > > > > > > It's not correct approach. > > > > > We need to copy flags to a temporary variable. > > > > > I will send a new patch soon to test, thanks! > > > > > > > > Just out of curiosity, can you explain what is wrong with John's > > > > approach? The problem isn't obvious to me. > > > > > > > > > Alloc vector -> create HCD -> put HCD -> free vector > > > > > > VS. > > > > > > Alloc vector -> create HCD -> free vector -> put HCD > > > > > > Of course I might miss something... > > > > Sure, the difference in ordering was pretty obvious. What is not > > obvious is why this should cause a problem. > > It may be not causing any problem right now, but with all these small > steps we may come to the case like DWC3 removal mess. > > > Do you think that the host controller driver is going to try to use the > > IRQ vector somewhere between the pci_free_irq_vectors call and the > > usb_put_hcd call? If that's not going to happen then I don't see what > > difference the order of the two calls makes. > > I think that this is a bit incorrect to rely on side-effects to ruin > the clear understanding of what ordering is going on. If you insist, > you can take John's solution, but I won't give a tag on such. > > Also take into consideration the possible copy'n'paste of this example > to other drivers. I have seen a lot of bad examples were > copied'n'pasted all over the kernel during my reviews. I don't want to > give another one. > > So, the review process, in my opinion, should be slightly broader that > we usually understand it, i.e. take into account: > - *run-time* bisectability > - possible copy'n'paste of the code excerpts I see. So you use "correct" in the broader sense of "good form" as well as strict correctness. (It was confusing because I wouldn't conflate those two different concepts.) Okay, now your reply makes sense. Alan Stern
