I don’t mind all. I’ll remove disable_dma_mrpc(). That will make things about the size as the old stdev_release() again. It’s pretty gratuitous. I’ll keep switchtec_exit_pci(), because it helps switchtec_pci_remove to remain a carefully ordered sequence of one-liners. I looked a little more through the code, and noticed that the two non-delayed work structs, mrpc_work and mrpc_timeout, are not cancelled in stdev_kill(). We seem to stop the ISR which schedules them, but do not stop the bh before teardown. And these handlers do not recognize stdev->alive (ok, they should not, because they do not hold a strong reference to stdev.) So there’s probably more coming soon-ish. Just mentioning it in case I get hit by a piano until then. Cheers, Daniel > On Nov 10, 2023, at 2:49 PM, Logan Gunthorpe <logang@xxxxxxxxxxxx> wrote: > > > > On 2023-11-10 14:30, Daniel Stodden wrote: >> >> Hi. >> >> [Sorry, sending this twice, again, hoping this time it won't bounce, because html] >> >>> Nice catch, thanks! >>> >>> The solution looks good to me, though I might quibble slightly about >>> style: I'm not sure why we need two helper functions (disable_dma_mrpc() >>> and switchtec_exit_pci()) that are only called in one place. I'd >>> probably just open code both of them. >>> >>> Other than that: >>> >>> Reviewed-by: Logan Gunthorpe <logang@xxxxxxxxxxxx> >> >> I'm fine with it either way. I added those mainly because of the enable_dma_mrpc/switctec_init_pci >> counterparts were already there. And likewise single-use, as would be usual. >> >> So the subroutines would maintain symmetry between probe() and remove(). >> >> So.. just don't add the new remove()-relevant ones? (One alternative would be to send another change right after, >> which goes after single-use pairs altogether.) > > Ah, I see. Just because it's single use doesn't necessarily mean it's > wrong. I would say switchtec_init_pci() should not be inlined because > it's rather long and keeps switchtec_pci_probe() a little more readable. > > enable_dma_mrpc() more questionable. I'm not sure why it was done that > way. Perhaps something changed. > > In any case, I'm not all that concerned. If you want to leave it the way > it is, I wouldn't mind. > > Thanks, > > Logan
