Hi, ------- Original Message ------- On Tuesday, July 5th, 2022 at 10:50, Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx> wrote: > [..] > > > > The SDIODEV struct is hard-coded for 2 functions. The function > > `brcmf_sdiod_intr_unregister` unregisters for the whole devices, i.e. > > two functions simultaneously. The OOB interrupt handling is > > function-independent. It takes a sdiodev pointer to work with. In > > addition, the code facilitates the request of a single OOB interrupt, > > i.e. a single one for the device as a whole. > > Okay. Forgot the internals of brcmf_sdiod_intr_unregister(). My elephant > brain is failing ;-) Okay. > > Regarding the sdio-irq-claim/release, these are function-bound. > > However, as mentioned before, `brcmf_sdiod_intr_unregister` handles > > both functions at once. This code does not handle `func` for the function > > currently being iterated on. Only a whole device. > > > > From how I read the code, this logic is scoped to SDIO-based devices. > > Plz correct if this is interpretation is wrong. > > > I stand corrected in this. Okay. > > > So your change does > > > not add anything for devices/platforms employing the SDIO interrupt, but > > > it does break those using OOB interrupt. > > > > It adds for SDIO-based interrupt handling that the interrupt gets released > > for the function that is being iterated on for removal. Therefore, it > > satisfies the expectations of the SDIO subsystem which otherwise emits a > > warning about an irq not having been freed. > > > > AFAICT OOB handling does not change: it still executes once. After that, > > the flag `oob_irq_requested` is false. The benefit we create, is that > > `brcmf_sdiod_intr_unregister` now only executes for func1, instead of > > either func1 or func2 depending on iteration order. > > But does that matter. brcmf_sdiod_intr_unregister() will do any real > stuff only once, right? Regardless which func comes first it will > release both func1 and func2 irq. Does it matter? I don't see enough > benefit to add code for it. Does it? You can probably judge this better than I can. What I read, is that the interrupt release logic is processed twice. And you have recently put a lot of emphasis on potential ordering problems. So, this is my proposal to somewhat stream-line the logic to per-func behavior, instead of doing everything at func1 and func2 is or isn't processed at all. I do not have a strong opinion. > > [..] > > > > That is correct, but in `brcmf_ops_sdio_remove`: > > > > `dev_set_drvdata(&sdiodev->func1->dev, NULL); dev_set_drvdata(&sdiodev->func2->dev, NULL);` > > > > set those pointers to NULL. So, in the use case where we process functions > > in increasing order, `func1` will result in full device clean-up. `func2` > > will result in `bus_if` (func->dev) being NULL, therefore exits immediately. > > That's a relieve. > > > So, I see two use cases: > > > > 1.) we iterate in decreasing order: release irq for func2 first, then do > > full clean-up in func1. > > 2.) we iterate in increasing order: do full clean-up in func1, then skip > > clean-up for func2 due to NULL bus_if. > > > > > In the portion above you have: > > > > > > sdio_claim_host(sdiodev->func1); > > > > > > When brcmf_ops_sdio_remove is called for func 2 (and even func 3) [..] > > > > `func3` does not exist from what I read in the code. Can you point me to > > the logic that I have missed? > > We don't use/claim func3, but some devices report a func3 and the > mmc/sdio subsystem will probe the driver with a func3. However, as we > never claim it we will not be called for func3 so please ignore my > blabbering about that. Okay, so what I understand from the response right now, is that we mostly agree on the implementation except maybe for the interrupt clean up. For the interrupt clean-up, I guess it may be determined by how strongly you want to tie the clean-up logic to func1 vs. the first func to be iterated on. Regards, Danny
