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 -- With Best Regards, Andy Shevchenko
