On 7/5/2022 3:12 PM, Danny van Heumen wrote:
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.
Right. I prefer dropping the interrupt clean-up and rest of the patch is
fine by me.
Thanks,
Arend
