Date: Sun, 05 Jan 2020 10:26:50 +0000 From: Chris Rees <crees@FreeBSD.org> To: Chris Rees <crees@FreeBSD.org>, hackers@FreeBSD.org, jhb@freebsd.org Subject: Re: Fixing audio/oss to use callout instead of timeouts Message-ID: <78949DD3-1152-429C-A685-F736B4B80E55@FreeBSD.org> In-Reply-To: <39ea5ba8-00d6-c5b2-4719-fba23bb53856@bayofrum.net> References: <F3750395-7793-4121-8EF1-39884C63E30F@FreeBSD.org> <33a74368-73a7-fdc0-4655-cfce88d865e6@FreeBSD.org> <ce758663-7517-ad31-284b-ca9985a4f7b0@bayofrum.net> <39ea5ba8-00d6-c5b2-4719-fba23bb53856@bayofrum.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On 4 January 2020 21:29:10 GMT, Chris Rees <crees@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=20 >>>> the duplicate), >>>> >>>> I've attempted to use the callout functions instead of the now=20 >>>> removed timeout functions for audio/oss, and I *think* that the >code=20 >>>> already stores and retrieves the list of handlers, so it should be >a=20 >>>> simple swap out. >>>> >>>> I've made this modification and run the module with mpg123 for a=20 >>>> while and it hasn't killed my laptop, but I'd just like to check=20 >>>> 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=20 >>>> done? >>>> >>>> >https://www.bayofrum.net/cgi-bin/fossil/oss/vinfo/ad269d7cbc02bf38?diff=3D2 > >>>> >>>> >>>> (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=3D= 2&dc=3D9999 > >>>> >>> A few suggestions: >>> >>> 1) You should do the callout_init() during a SYSINIT or MOD_LOAD=20 >>> event to initialize >>> =C2=A0=C2=A0=C2=A0 all the timers at once instead of doing it in oss_ti= meout(). >>> >>> 2) You should then add a SYSUNINIT or MOD_UNLOAD that uses=20 >>> callout_drain(). >>> >>> 3) You should add a mutex to protect the tmouts array and=20 >>> timeout_random, etc. >>> =C2=A0=C2=A0=C2=A0 and use callout_init_mtx with that lock, but probabl= y use=20 >>> CALLOUT_RETURNUNLOCKED >>> =C2=A0=C2=A0=C2=A0 and drop the mutex right before calling the function= (you can=20 >>> store the function >>> =C2=A0=C2=A0=C2=A0 pointer and void * argument in local variables befor= e dropping=20 >>> the lock to >>> =C2=A0=C2=A0=C2=A0 be safe. >>> >>> 4) If possible, you should really alter the oss API so that drivers=20 >>> don't use >>> =C2=A0=C2=A0=C2=A0 a timeout()-like interface, but instead use a callou= t directly=20 >>> (or an interface >>> =C2=A0=C2=A0=C2=A0 that wraps callout).=C2=A0 This would let drivers ta= ke advantage of=20 >>> callout_init_mtx >>> =C2=A0=C2=A0=C2=A0 with their own locks, etc. >>> >>> Does audio/oss contain all the code that makes use of oss_timeout? >>> >> Thanks so much for your feedback!=C2=A0 You've probably seen from the >naive=20 >> change my level of kernel hacking. >> >> 1-2, I think I've done here: >> >> >https://www.bayofrum.net/cgi-bin/fossil/oss/vdiff?from=3Dae2a86&to=3Db4376= 737b9d4a127 > >> >> >> (full context:=20 >> >https://www.bayofrum.net/cgi-bin/fossil/oss/vdiff?from=3Dae2a86&to=3Db4376= 737b9d4a127&dc=3D9999) >> >> 3. The function pointer is constant, and the argument is a local=20 >> variable I think. >> >> 4. I agree, but I think it'd kill portability, and to be honest, the=20 >> only reason I even use OSS is just for my CMI8788 soundcard. If I've=20 >> managed this, I'm looking at porting the driver to FreeBSD's native=20 >> sound driver.=C2=A0 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=3Dae2a86&to=3Da10bd4= 8&dc=3D9999 I'm fairly confident this is what you meant and should be acceptable... I'= ve properly done what you said in (3) now. Chris --=20 This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?78949DD3-1152-429C-A685-F736B4B80E55>