Fixing audio/oss to use callout instead of timeouts

[Chris Rees]

[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?
>
> Chris
>
>

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