Repository navigation
upstream: rework snd_sof_dsp_register_poll #358
Description
-
No need to have an additional variable (done) when refactoring can provide
you an existing variable (time) to be tested instead. -
There are generic macros readx_poll_timeout() that are doing this kind of
busy loop with IO.
Activity
ranj063 commentedon Dec 4, 2018
@plbossart #366 addresses the first point in this request.
I have crashed my board over 20 times trying to get readx_poll_timeout() to work instead. I will submit another one later if I ever get it working.
kv2019i commentedon Mar 22, 2019
Created a patch for this now, will send pull req later after a bit more reviews on the kernel style issues (it's been a while):
https://github.com/kv2019i/linux/tree/topic/issue358-fix
plbossart commentedon Mar 24, 2019
@kv2019i see my comments directly on your branch. Thanks!
kv2019i commentedon Mar 25, 2019
@plbossart Thanks Pierre for the review comments! I was actually planning to still polish a bit and send a proper PR, but you beat me to it. :)
But yeah, I'll copy my reply to one of the core opens to here as well. I'll fix the rest and leave the patch in otherwise clean state.
On the snd_sof_dsp_read_poll_timeout() macro I added:
""Yes. This is a verbatim copy of include/linux/iopoll.h. It's a good question whether this is worth the trouble, but at least in regmem a similar approach is taken an copy of iopoll.h with identical interface is provided (regmap_read_poll_timeout). If we start modifying the macro, then you can't make the connection to iopoll.h anymore and the change become somewhat moot.
So I think we have a few options:
I'm not sure it makes sense to touch the current code unless we can align with rest of the kernel. If we anyways have a custom code, why not just use the current one?""
plbossart commentedon Mar 25, 2019
@kv2019i i'd really like to keep this two-resolution method, otherwise we'll be changing the functionality and enter uncharted waters with a behavior that differs from what previous solutions used. If it ain't broke don't fix it applies here.
ranj063 commentedon Mar 26, 2019
@plbossart the two-resolution was added only recently in January. And the main reason was suspend stress test failures. We have come a long way in terms of fixing the device hierarchy and the PM flow since then, I think it might be worthwhile to give @kv2019i's approach a try.
ranj063 commentedon Mar 26, 2019
@plbossart the two-resolution was added only recently in January. And the main reason was suspend stress test failures. We have come a long way in terms of fixing the device hierarchy and the PM flow since then, I think it might be worthwhile to give @kv2019i's approach a try.
plbossart commentedon Mar 28, 2019
@ranj063 the 2-step approach came from the cAVS driver and has been in use for a long time, if we have a working solution let's keep it. We have additional programming sequences to double-check to harden our code.
ranj063 commentedon Mar 28, 2019
@plbossart that sounds fair. Although I have to mention that the cavs driver added this logic exactly for the same reasons as us and I truly believe it is a bit of an overkill.
kv2019i commentedon Apr 1, 2019
Done with PR366 merged.
Done with PR750 merged.
Closing the issue, thanks all!
selftests/bpf: Extend netkit tests to validate skb meta data