Fixing audio/oss to use callout instead of timeouts

[Chris Rees]

On 4 January 2020 21:29:10 GMT, Chris Rees <crees at FreeBSD.org> wrote:
>[Keeping -hackers CC'd as again I've failed with from address :/]
>
>On 2020-01-04 21:27, Chris Rees wrote:
>> Hi John,
>>
>> On 2020-01-04 13:39, John Baldwin wrote:
>>> On 1/2/20 3:47 PM, Chris Rees wrote:
>>>> Hi Hackers (and perhaps John, as the author of r355732, sorry for 
>>>> the duplicate),
>>>>
>>>> I've attempted to use the callout functions instead of the now 
>>>> removed timeout functions for audio/oss, and I *think* that the
>code 
>>>> already stores and retrieves the list of handlers, so it should be
>a 
>>>> simple swap out.
>>>>
>>>> I've made this modification and run the module with mpg123 for a 
>>>> while and it hasn't killed my laptop, but I'd just like to check 
>>>> that I have the principle correct and haven't missed anything
>obvious.
>>>>
>>>> Please would you let me know if there is anything else I should
>have 
>>>> done?
>>>>
>>>>
>https://www.bayofrum.net/cgi-bin/fossil/oss/vinfo/ad269d7cbc02bf38?diff=2
>
>>>>
>>>>
>>>> (This is the change I've made to kernel/OS/FreeBSD/os_freebsd.c in
>oss)
>>>>
>>>>
>https://www.bayofrum.net/cgi-bin/fossil/oss/vinfo/ad269d7cbc02bf38?diff=2&dc=9999
>
>>>>
>>> A few suggestions:
>>>
>>> 1) You should do the callout_init() during a SYSINIT or MOD_LOAD 
>>> event to initialize
>>>     all the timers at once instead of doing it in oss_timeout().
>>>
>>> 2) You should then add a SYSUNINIT or MOD_UNLOAD that uses 
>>> callout_drain().
>>>
>>> 3) You should add a mutex to protect the tmouts array and 
>>> timeout_random, etc.
>>>     and use callout_init_mtx with that lock, but probably use 
>>> CALLOUT_RETURNUNLOCKED
>>>     and drop the mutex right before calling the function (you can 
>>> store the function
>>>     pointer and void * argument in local variables before dropping 
>>> the lock to
>>>     be safe.
>>>
>>> 4) If possible, you should really alter the oss API so that drivers 
>>> don't use
>>>     a timeout()-like interface, but instead use a callout directly 
>>> (or an interface
>>>     that wraps callout).  This would let drivers take advantage of 
>>> callout_init_mtx
>>>     with their own locks, etc.
>>>
>>> Does audio/oss contain all the code that makes use of oss_timeout?
>>>
>> Thanks so much for your feedback!  You've probably seen from the
>naive 
>> change my level of kernel hacking.
>>
>> 1-2, I think I've done here:
>>
>>
>https://www.bayofrum.net/cgi-bin/fossil/oss/vdiff?from=ae2a86&to=b4376737b9d4a127
>
>>
>>
>> (full context: 
>>
>https://www.bayofrum.net/cgi-bin/fossil/oss/vdiff?from=ae2a86&to=b4376737b9d4a127&dc=9999)
>>
>> 3. The function pointer is constant, and the argument is a local 
>> variable I think.
>>
>> 4. I agree, but I think it'd kill portability, and to be honest, the 
>> only reason I even use OSS is just for my CMI8788 soundcard. If I've 
>> managed this, I'm looking at porting the driver to FreeBSD's native 
>> sound driver.  I don't think anyone uses OSS for any other reason.
>>
>> (5.) Yes, it doesn't appear elsewhere.
>>
>> Have I correctly understood you?
>>

Ok, I see I slightly misunderstood where you told me to put the locks.

https://www.bayofrum.net/cgi-bin/fossil/oss/vdiff?from=ae2a86&to=a10bd48&dc=9999

I'm fairly confident this is what you meant and should be acceptable...  I've properly done what you said in (3) now.

Chris

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.