Re: [alsa-devel] asihpi driver -> kernel
Hi Eliot,
[it seems that you Cc'ed to the old alsa-devel ML so the last one seemed missing.]
At Tue, 16 Oct 2007 14:47:15 +1300, Eliot Blennerhassett wrote:
Hi Takashi,
still trying to move forward on this...
Looking for "what is the first step" change to make in your opinion?
The first step would be to get closer to linux kernel coding style. Namely, - get rid of C++ style function names, variables and field names (no hungarian!) - remove API wrappers
Eventually, we may get rid of unneeded typedefs, too.
Then, we can begin with simplifying the driver structure. Currently, asihpi and hpi are almost indepedent implementations. This makes life harder.
Takashi Iwai wrote:
At Wed, 05 Sep 2007 13:00:35 +1200, Eliot Blennerhassett wrote:
Hi Takashi,
I'd like to talk more about what you think is needed to get asihpi driver ready for kernel tree.
I think for now we should put aside cosmetic coding style changes (eg. comment style, whitespace etc.) these are fixable with combination of automated tools and some monkey work.
So, lets start with the hard part. If we can't solve that there is no point in cosmetics!
A while ago you said "The specific problem I'm concerned is the "glue-code" style in HPI driver. The OS-independent wrapper code is often disliked by kernel maintainers. "
Can you point to specific examples. Even better suggest how you think it should be done better. Either way I need to consider how your concerns could be addressed without breaking our codebase for other OSes etc.
thanks and regards
The problem is that the whole "wrapping" is superfluous when you concentrate on Linux driver. Merging into the linux kernel tree means that the driver would be dedicated to Linux. It's no longer your child, and it's no longer for every other OS. Thus, "common codebase" doesn't exist per se.
For example, the whole definitions in hpios_linux_kernel.h should go away. The driver should use the native kernel API instead.
Even this is not so hard, as it a mechanical process to replace wrapped calls with linux native calls.
OK, then it's fine.
I know it's a very hard decision. Supporting other OSes is very important in most cases. OTOH, the slim-down is the best way to make the driver code maintainable for other people. Honestly, I find it also hard to maintain/debug the current ASIHPI code. It's too different from the normal codes, and understanding it alone takes too much energy before analyzing the problem.
This is where I need more input, because to _me_ (of course) the code is understandable.
Is it the whole source or just parts that are hard. E.g toplevel asihpi.c Ok/No?
The code in asihpi.c is readable enough. But, other parts are different. The difficulty is that the driver structure isn't straight (apart from the coding style issues). We have an entry point for PCI driver _and_ indepdendent HPI initialization. This is already puzzling because HPI is also assigned to the corresponding PCI device.
Also, the message union/struct is another stuff that is pretty hard to follow. Since we use no C++, union is often bad for readability. Could it be a bit simpler?
The coding-style issues are similar problems. When C++-style codes appear in the tree of C-codes, they are strange. Simply so. It makes difficult to find the problematic part. "A buggy part looks buggy" -- it's the whole point of having a uniform coding style.
Again, a mechanical process to change comments and style etc.
Then it's fine with me. Let's try and see the progress.
These problems (different from others) make it impossible to merge to the upstream tree, so far.
Eliot BTW I'm happy to conduct this discussion publically on alsa-devel, some other people might learn something too...
I think it's a good thing, too. Feel free to add alsa-devel to Cc in the next reply with citation.
For reference here is the source tree diagram.
asihpi.c alsa driver toplevel | hpifunc.c message/response pack/unpack | hpimsgx.c hpi stream ownership, backend selection, some init | hpi6205.c hpi6000.c hpi4000.c card family drivers | hpicmn.c, hpios_linux.c hpidspcd.c hpidebug.c library code
hpimod.c probing, HPI ioctl
thanks,
Takashi
thanks,
Takashi
Takashi Iwai wrote:
Hi Eliot,
[it seems that you Cc'ed to the old alsa-devel ML so the last one seemed missing.]
At Tue, 16 Oct 2007 14:47:15 +1300, Eliot Blennerhassett wrote:
Hi Takashi,
still trying to move forward on this...
Looking for "what is the first step" change to make in your opinion?
The first step would be to get closer to linux kernel coding style. Namely,
- get rid of C++ style function names, variables and field names (no hungarian!)
This (though 'frowned on') is not *required* by kernel coding_style document. I don't plan on doing such major cosmetic surgery if not necessary. If I can get source to pass checkpatch.pl, I would hope it would be considered.
- remove API wrappers
Working on it, will submit new patches in the next week or so.
Spin lock wrappers can not be removed because of the way alsa midlevel calls driver with different IRQ disabling context for different functions.
Eventually, we may get rid of unneeded typedefs, too.
Then, we can begin with simplifying the driver structure. Currently, asihpi and hpi are almost indepedent implementations. This makes life harder.
Takashi Iwai wrote:
At Wed, 05 Sep 2007 13:00:35 +1200, Eliot Blennerhassett wrote:
Hi Takashi,
I'd like to talk more about what you think is needed to get asihpi driver ready for kernel tree.
I think for now we should put aside cosmetic coding style changes (eg. comment style, whitespace etc.) these are fixable with combination of automated tools and some monkey work.
So, lets start with the hard part. If we can't solve that there is no point in cosmetics!
A while ago you said "The specific problem I'm concerned is the "glue-code" style in HPI driver. The OS-independent wrapper code is often disliked by kernel maintainers. "
Can you point to specific examples. Even better suggest how you think it should be done better. Either way I need to consider how your concerns could be addressed without breaking our codebase for other OSes etc.
thanks and regards
The problem is that the whole "wrapping" is superfluous when you concentrate on Linux driver. Merging into the linux kernel tree means that the driver would be dedicated to Linux. It's no longer your child, and it's no longer for every other OS. Thus, "common codebase" doesn't exist per se.
For example, the whole definitions in hpios_linux_kernel.h should go away. The driver should use the native kernel API instead.
Even this is not so hard, as it a mechanical process to replace wrapped calls with linux native calls.
OK, then it's fine.
I know it's a very hard decision. Supporting other OSes is very important in most cases. OTOH, the slim-down is the best way to make the driver code maintainable for other people. Honestly, I find it also hard to maintain/debug the current ASIHPI code. It's too different from the normal codes, and understanding it alone takes too much energy before analyzing the problem.
Still I cannot imagine that anyone else will be at the sharp end of debugging and adding extra features to the driver other than ASI personnel. When I see a significant patch I'll revise my opinion.
If porting of new features is not an automated process, it is the linux driver that will start to rot.
I stick to my position that 95% windows users finding bugs benefits the 5% linux users when common code is used, and this is worth much more than how identifiers are spelled.
This is where I need more input, because to _me_ (of course) the code is understandable.
Is it the whole source or just parts that are hard. E.g toplevel asihpi.c Ok/No?
The code in asihpi.c is readable enough. But, other parts are different. The difficulty is that the driver structure isn't straight (apart from the coding style issues). We have an entry point for PCI driver _and_ indepdendent HPI initialization. This is already puzzling because HPI is also assigned to the corresponding PCI device.
HPI is still needed for e.g compressed audio play/record, profile and assert monitoring, existing HPI applications.
It provides the ability to use HPI-only applications while at the same time using channels with ALSA to 'just play some sound'
Also, the message union/struct is another stuff that is pretty hard to follow. Since we use no C++, union is often bad for readability. Could it be a bit simpler?
Very difficult. message/response structure/unions are deeply embedded in the DSP code that runs on the cards. They must remain exactly as specified in hpi.h
The coding-style issues are similar problems. When C++-style codes appear in the tree of C-codes, they are strange. Simply so. It makes difficult to find the problematic part. "A buggy part looks buggy" -- it's the whole point of having a uniform coding style.
Again, a mechanical process to change comments and style etc.
Then it's fine with me. Let's try and see the progress.
regards
Eliot
Eliot Blennerhassett wrote:
Working on it, will submit new patches in the next week or so.
Hi Takashi,
I have been beating our code with a big stick for the last 4 weeks.
I'd appreciate any comment you have on the progress.
You can see the result here: http://audioscience.com/internet/download/beta/alsahg3459_asihpi30905.tar.gz
The matching firmware is here: http://audioscience.com/internet/download/beta/dspbins30905.tar.bz2
Note that dsp6413.bin is replaced by dsp6400.bin, dsp8713 by dsp8700. You can remove all the dsp*.txt from the repo they are not needed. Also remove dsp2400, its not relevant.
The code is copmplete files rather than diffs, there are so many diffs I don't think it is useful. Apart from cosmetic changes, there are some added and removed files compared to curren ALSA Hg repo:
* Merged all files relating to HPI4000 into hpi4000.[ch] (remove hpi56301.[ch], boot4ka.h, dpi56301.h * Removed hpicheck.h * add hpimsginit.[ch] (no new functions)
Cosmetic changes * Code is mostly 'checkpatch clean', though still some real work to do around our use of volatile for structures that are updated by DMA by our adapters
* Only 2 'sparse' warnings
* Got rid of nearly all typedefs, and most thin wrappers around kernel functions.
* hpifunc.c now has no comments. They might return when we get around to grinding them all down to 80 columns... Extracted documentation is available here in the mean time: http://audioscience.com/internet/download/sdk/hpi_usermanual_html/html/index...
regards
Eliot Blennerhassett AudioScience Inc.
At Fri, 07 Dec 2007 12:33:23 +1300, Eliot Blennerhassett wrote:
Eliot Blennerhassett wrote:
Working on it, will submit new patches in the next week or so.
Hi Takashi,
I have been beating our code with a big stick for the last 4 weeks.
I'd appreciate any comment you have on the progress.
You can see the result here: http://audioscience.com/internet/download/beta/alsahg3459_asihpi30905.tar.gz
The matching firmware is here: http://audioscience.com/internet/download/beta/dspbins30905.tar.bz2
Note that dsp6413.bin is replaced by dsp6400.bin, dsp8713 by dsp8700. You can remove all the dsp*.txt from the repo they are not needed. Also remove dsp2400, its not relevant.
The code is copmplete files rather than diffs, there are so many diffs I don't think it is useful. Apart from cosmetic changes, there are some added and removed files compared to curren ALSA Hg repo:
- Merged all files relating to HPI4000 into hpi4000.[ch] (remove
hpi56301.[ch], boot4ka.h, dpi56301.h
- Removed hpicheck.h
- add hpimsginit.[ch] (no new functions)
Cosmetic changes
- Code is mostly 'checkpatch clean', though still some real work to do
around our use of volatile for structures that are updated by DMA by our adapters
Only 2 'sparse' warnings
Got rid of nearly all typedefs, and most thin wrappers around kernel
functions.
- hpifunc.c now has no comments. They might return when we get around
to grinding them all down to 80 columns... Extracted documentation is available here in the mean time: http://audioscience.com/internet/download/sdk/hpi_usermanual_html/html/index...
Thanks, I'll take a look at new codes tomorrow (just back from vacation...)
Takashi
At Fri, 07 Dec 2007 12:33:23 +1300, Eliot Blennerhassett wrote:
Eliot Blennerhassett wrote:
Working on it, will submit new patches in the next week or so.
Hi Takashi,
I have been beating our code with a big stick for the last 4 weeks.
I'd appreciate any comment you have on the progress.
Thanks for your work. Now I committed the codes below to HG tree, so further work could be provided as patches.
You can see the result here: http://audioscience.com/internet/download/beta/alsahg3459_asihpi30905.tar.gz
The matching firmware is here: http://audioscience.com/internet/download/beta/dspbins30905.tar.bz2
Note that dsp6413.bin is replaced by dsp6400.bin, dsp8713 by dsp8700. You can remove all the dsp*.txt from the repo they are not needed. Also remove dsp2400, its not relevant.
The code is copmplete files rather than diffs, there are so many diffs I don't think it is useful. Apart from cosmetic changes, there are some added and removed files compared to curren ALSA Hg repo:
- Merged all files relating to HPI4000 into hpi4000.[ch] (remove
hpi56301.[ch], boot4ka.h, dpi56301.h
- Removed hpicheck.h
- add hpimsginit.[ch] (no new functions)
Cosmetic changes
- Code is mostly 'checkpatch clean',
That's great.
though still some real work to do around our use of volatile for structures that are updated by DMA by our adapters
Yeah, volatile is a thing that is often ambiguous and fishy.
Only 2 'sparse' warnings
Got rid of nearly all typedefs, and most thin wrappers around kernel
functions.
One thing I noticed is the function definition/declaration style.
void HPI_GetErrorText( u16 wError, char *pszErrorText ) { ...
It's uncommon. Normally, it's in a single line, and the line starting with the opening brace. See CodingStyle as reference. (Besides, the hungarian style is hated by many people. Let's avoid it as much as possible.)
Also, in general, the removal of all HpiOs_*() is a good goal. The kernel guys really hate wrappers like that. Let's use native API.
Other things I noticed:
- The ioctl of hpi chrdev is dangerous: it may cause Oops when accessing to a non-existing adapter. I feel the chrdev should be rather bound to the existing ALSA device, e.g. using hwdep instead. Then the whole story about registration would become more straight.
- Replace semaphore with mutex as much as possible.
- Don't use bitfields for data communication. It's higly unportable. Use explit bit shift and mask instead.
- The way of clean up like hpimod_cleanup() is uncommon. (Above all, passing just a digit is a bad style...)
- Remove HPI_UNUSED(). We don't check unused parameters.
- Global variables should have some unique prefix. The kernel code is monolithc C, so they are really global and has no name space.
- HPI_GetErrorText() is dangerous. It may overflow easily. Don't use strcat() unless you are really sure. Use variants with the size limit.
Takashi
Hi Takashi,
thanks for looking over our code. Some of your comments are addressed below.
Takashi Iwai wrote:
One thing I noticed is the function definition/declaration style.
void HPI_GetErrorText( u16 wError, char *pszErrorText ) { ...
It's uncommon. Normally, it's in a single line, and the line starting with the opening brace. See CodingStyle as reference.
Yes. We find that this style leaves room for doxygen comments on each function parameter and keep within 80 columns. In hpifunc.c this is not evident because we are stripping all the comments out for now.
(Besides, the hungarian style is hated by many people. Let's avoid it as much as possible.)
Also, in general, the removal of all HpiOs_*() is a good goal. The kernel guys really hate wrappers like that. Let's use native API.
I think all the thin wrappers have been removed. What remain are
1) Spinlock wrappers, the comments in the code tell why /* The reason for all this evilness is that ALSA calls some of a drivers * operators in atomic context, and some not. But all our functions channel * through the HPI_Message conduit, so we can't handle the different context * per function */
2) LockedMem wrappers. These provide tracking for the DMA areas Need to keep track of all the info about the mem area so it can be freed again at the end
E.g. pci_free_consistent(pMemArea->pdev, pMemArea->size, pMemArea->cpu_addr, pMemArea->dma_addr);
Other things I noticed:
- The ioctl of hpi chrdev is dangerous: it may cause Oops when accessing to a non-existing adapter.
A bug to fix
I feel the chrdev should be rather bound to the existing ALSA device, e.g. using hwdep instead. Then the whole story about registration would become more straight.
...yes I'm interested. Currently HPI userspace lib just works whether ALSA or plain HPI driver is loaded. How would this work with your scenario?
- Replace semaphore with mutex as much as possible.
OK, will look at it. BTW is there an easy way to find out at what kernel version a given API was added/removed/changed?
- Don't use bitfields for data communication. It's higly unportable. Use explit bit shift and mask instead.
Only hpi_version structure I think? Not used by kernel driver at all, we can make it disappear.
- The way of clean up like hpimod_cleanup() is uncommon. (Above all, passing just a digit is a bad style...)
OK. So replace this with checks like this?:
if (asihpi_class) class_device_destroy(asihpi_class... if (chrdev_registered) unregister_chrdev(... if (asihpi_pci_driver) pci_unregister_driver(...
- Remove HPI_UNUSED(). We don't check unused parameters.
OK.
- Global variables should have some unique prefix. The kernel code is monolithc C, so they are really global and has no name space.
Did you have a list? There are a few function names that don't start with HPI.
- HPI_GetErrorText() is dangerous. It may overflow easily. Don't use strcat() unless you are really sure. Use variants with the size limit.
Valid point.
We can get rid of it from kernel driver entirely by using just error number instead.
regards
Eliot Blennerhassett AudioScience Inc.
At Tue, 18 Dec 2007 13:16:38 +1300, Eliot Blennerhassett wrote:
Hi Takashi,
thanks for looking over our code. Some of your comments are addressed below.
Takashi Iwai wrote:
One thing I noticed is the function definition/declaration style.
void HPI_GetErrorText( u16 wError, char *pszErrorText ) { ...
It's uncommon. Normally, it's in a single line, and the line starting with the opening brace. See CodingStyle as reference.
Yes. We find that this style leaves room for doxygen comments on each function parameter and keep within 80 columns. In hpifunc.c this is not evident because we are stripping all the comments out for now.
Well, the doxygen comment is another issue. Usually, the kerneldoc allows you to write (semi-)javadoc style comments for each function. It begins with "/**" and has standard "@parameter description" lines.
So, one thing I forgot to mention is to avoid /** for non-kerneldoc comments.
(Besides, the hungarian style is hated by many people. Let's avoid it as much as possible.)
Also, in general, the removal of all HpiOs_*() is a good goal. The kernel guys really hate wrappers like that. Let's use native API.
I think all the thin wrappers have been removed. What remain are
- Spinlock wrappers, the comments in the code tell why
/* The reason for all this evilness is that ALSA calls some of a drivers
- operators in atomic context, and some not. But all our functions channel
- through the HPI_Message conduit, so we can't handle the different context
- per function
*/
Then this shouldn't be expressed as a spinlock wrapper. You cannot use a single lock for that. And the way you hack doesn't look correct for schedulable contexts...
- LockedMem wrappers. These provide tracking for the DMA areas
Need to keep track of all the info about the mem area so it can be freed again at the end
Hm, but still I don't see the reason to use kmem_cache there.
E.g. pci_free_consistent(pMemArea->pdev, pMemArea->size, pMemArea->cpu_addr, pMemArea->dma_addr);
Use dma_alloc_coherent() / dma_free_coherent() instead. pci_*_consistent() are obsolete.
I feel the chrdev should be rather bound to the existing ALSA device, e.g. using hwdep instead. Then the whole story about registration would become more straight.
...yes I'm interested. Currently HPI userspace lib just works whether ALSA or plain HPI driver is loaded. How would this work with your scenario?
The independent HPI driver doesn't exist with your current version (all merged to snd-asihpi). So, this question sounds irrelevant as now.
If we *really* need to provide an independent HPI access, then hpi should have its own device (most easily via platform device). But, I feel it's just give compliexity in the end.
- Replace semaphore with mutex as much as possible.
OK, will look at it. BTW is there an easy way to find out at what kernel version a given API was added/removed/changed?
The alsa-driver tree has a wrapper, so you can use mutex without any checks in the driver.
- Don't use bitfields for data communication. It's higly unportable. Use explit bit shift and mask instead.
Only hpi_version structure I think? Not used by kernel driver at all, we can make it disappear.
struct hpi_handle has bitfields. I think it's used very commonly in asihpi.
- The way of clean up like hpimod_cleanup() is uncommon. (Above all, passing just a digit is a bad style...)
OK. So replace this with checks like this?:
if (asihpi_class) class_device_destroy(asihpi_class... if (chrdev_registered) unregister_chrdev(... if (asihpi_pci_driver) pci_unregister_driver(...
Yes, it looks better to me.
- Global variables should have some unique prefix. The kernel code is monolithc C, so they are really global and has no name space.
Did you have a list? There are a few function names that don't start with HPI.
Err, sorry, I meant global variables *and functions*.
thanks,
Takashi
Takashi Iwai wrote:
At Tue, 18 Dec 2007 13:16:38 +1300, Eliot Blennerhassett wrote:
Well, the doxygen comment is another issue. Usually, the kerneldoc allows you to write (semi-)javadoc style comments for each function. It begins with "/**" and has standard "@parameter description" lines.
This breaks "define things in only one place" rule. I.e. my style doesn't require the parameter name etc to be repeated in the comment.
So, one thing I forgot to mention is to avoid /** for non-kerneldoc comments.
What! just when I think I have everything covered, another picky requirement pops up. When will this end!
I think all the thin wrappers have been removed. What remain are
- Spinlock wrappers, the comments in the code tell why
/* The reason for all this evilness is that ALSA calls some of a drivers
- operators in atomic context, and some not. But all our functions channel
- through the HPI_Message conduit, so we can't handle the different context
- per function
*/
Then this shouldn't be expressed as a spinlock wrapper. You cannot use a single lock for that. And the way you hack doesn't look correct for schedulable contexts...
AFAIK it works. We can do away with it if you can tell me how to stop alsa midlevel calling HPI functions in both schedulable and non-schedulable contexts.
- LockedMem wrappers. These provide tracking for the DMA areas
Need to keep track of all the info about the mem area so it can be freed again at the end
Hm, but still I don't see the reason to use kmem_cache there.
Its a convenient way to allocate however many small chunks are needed. Otherwise we need to statically allocate or malloc enough space for every possible stream on every possible card. I.e. 32 streams * 16 cards.
Or can do a major redesign of this part and absorb the DMA mem tracking into per-adapter data structures. Not gonna happen soon here.
E.g. pci_free_consistent(pMemArea->pdev, pMemArea->size, pMemArea->cpu_addr, pMemArea->dma_addr);
Use dma_alloc_coherent() / dma_free_coherent() instead. pci_*_consistent() are obsolete.
I feel the chrdev should be rather bound to the existing ALSA device, e.g. using hwdep instead. Then the whole story about registration would become more straight.
...yes I'm interested. Currently HPI userspace lib just works whether ALSA or plain HPI driver is loaded. How would this work with your scenario?
The independent HPI driver doesn't exist with your current version (all merged to snd-asihpi). So, this question sounds irrelevant as now.
It exists for those who build it themselves or get it from 3rd party repos. We have a number of applications that use HPI, we still want to be able to use them to test the cards etc, even if ALSA driver is loaded.
For our existing customers, HPI has been around much longer than ALSA, so we need a transitional period where both interfaces are available simultaneously. I'd say about 5 years from when the driver is accepted into the kernel should be sufficient.
(For example Rivendell http://www.rivendellaudio.org/ radio automation uses HPI to play/record mpeg audio using our on-card processing)
If we *really* need to provide an independent HPI access, then hpi should have its own device (most easily via platform device). But, I feel it's just give compliexity in the end.
Another possibility is to make HPI access via optional loadable module. ALSA driver would only need to export HPI_Message() for this to be workable. Already need to export HPI_Message to support our V4L driver.
- Don't use bitfields for data communication. It's higly unportable. Use explit bit shift and mask instead.
struct hpi_handle has bitfields. I think it's used very commonly in asihpi.
The bitfields are all created and used by the same code module, they are never referenced outside hpifunc.c (so its not "data communication" in my book). IMO it improves readability, and let the compiler figure out the bit shift and mask stuff.
regards
Eliot
At Wed, 19 Dec 2007 09:52:08 +1300, Eliot Blennerhassett wrote:
Takashi Iwai wrote:
At Tue, 18 Dec 2007 13:16:38 +1300, Eliot Blennerhassett wrote:
Well, the doxygen comment is another issue. Usually, the kerneldoc allows you to write (semi-)javadoc style comments for each function. It begins with "/**" and has standard "@parameter description" lines.
This breaks "define things in only one place" rule. I.e. my style doesn't require the parameter name etc to be repeated in the comment.
Then you shouldn't use kerneldoc (javadoc) thingy...
So, one thing I forgot to mention is to avoid /** for non-kerneldoc comments.
What! just when I think I have everything covered, another picky requirement pops up. When will this end!
The /** comment isn't strict, but it's just confusing. People do think that it would be a javadoc comment.
I think all the thin wrappers have been removed. What remain are
- Spinlock wrappers, the comments in the code tell why
/* The reason for all this evilness is that ALSA calls some of a drivers
- operators in atomic context, and some not. But all our functions channel
- through the HPI_Message conduit, so we can't handle the different context
- per function
*/
Then this shouldn't be expressed as a spinlock wrapper. You cannot use a single lock for that. And the way you hack doesn't look correct for schedulable contexts...
AFAIK it works.
spin_lock_bh() is to protect data for user-context and softirqs...
We can do away with it if you can tell me how to stop alsa midlevel calling HPI functions in both schedulable and non-schedulable contexts.
Hm? If you'd like to unify the call, then choose always the spin-locked version,
spin_lock_irqsave(&lock, flags); HPI_something(); spin_unlock_irqrestore(&lock, flags);
It's fine to do this in user context (unless HPI_somthing() takes too long time).
- LockedMem wrappers. These provide tracking for the DMA areas
Need to keep track of all the info about the mem area so it can be freed again at the end
Hm, but still I don't see the reason to use kmem_cache there.
Its a convenient way to allocate however many small chunks are needed. Otherwise we need to statically allocate or malloc enough space for every possible stream on every possible card. I.e. 32 streams * 16 cards.
Or can do a major redesign of this part and absorb the DMA mem tracking into per-adapter data structures. Not gonna happen soon here.
OK, then we can leave as is.
E.g. pci_free_consistent(pMemArea->pdev, pMemArea->size, pMemArea->cpu_addr, pMemArea->dma_addr);
Use dma_alloc_coherent() / dma_free_coherent() instead. pci_*_consistent() are obsolete.
I feel the chrdev should be rather bound to the existing ALSA device, e.g. using hwdep instead. Then the whole story about registration would become more straight.
...yes I'm interested. Currently HPI userspace lib just works whether ALSA or plain HPI driver is loaded. How would this work with your scenario?
The independent HPI driver doesn't exist with your current version (all merged to snd-asihpi). So, this question sounds irrelevant as now.
It exists for those who build it themselves or get it from 3rd party repos. We have a number of applications that use HPI, we still want to be able to use them to test the cards etc, even if ALSA driver is loaded.
For our existing customers, HPI has been around much longer than ALSA, so we need a transitional period where both interfaces are available simultaneously. I'd say about 5 years from when the driver is accepted into the kernel should be sufficient.
(For example Rivendell http://www.rivendellaudio.org/ radio automation uses HPI to play/record mpeg audio using our on-card processing)
The hwdep device can be set up to emulate (accept) HPI ioctls. It's a matter of device name.
If we *really* need to provide an independent HPI access, then hpi should have its own device (most easily via platform device). But, I feel it's just give compliexity in the end.
Another possibility is to make HPI access via optional loadable module. ALSA driver would only need to export HPI_Message() for this to be workable. Already need to export HPI_Message to support our V4L driver.
This is somewhat different...
- Don't use bitfields for data communication. It's higly unportable. Use explit bit shift and mask instead.
struct hpi_handle has bitfields. I think it's used very commonly in asihpi.
The bitfields are all created and used by the same code module, they are never referenced outside hpifunc.c (so its not "data communication" in my book).
This struct is used for data communication between the hardware and the driver, no?
IMO it improves readability, and let the compiler figure out the bit shift and mask stuff.
How bitfields are assigned is undefined in C. It works just coincidentally right now. If you need to access a certain bit in bytes, you must use the explicity bit-shift & mask operation, not via bitfields. The only purpose of bitfields is to save some bytes for small data.
Takashi
Thanks Takashi,
some comments interleaved below.
The spinlock issue is particularly thorny - if anyone can come up with a better solution I'm all ears.
On the subject of "volatile", I'm looking for some concrete examples of how to convert the volatile vars to use memory barriers.
The volatiles are all allocated with dma_alloc_consistent, and are accessed via DMA by our card. 2 scenarios. 1) Write data to variable, interrupt card. How to ensure written data is visible to card DMA before the card gets interrupt?
2) Wait for card to set a given status in the variable. ?use smp_rmb() in the loop?
Takashi Iwai wrote:
- Spinlock wrappers, the comments in the code tell why
/* The reason for all this evilness is that ALSA calls some of a drivers
- operators in atomic context, and some not. But all our functions channel
- through the HPI_Message conduit, so we can't handle the different context
- per function
*/
Then this shouldn't be expressed as a spinlock wrapper. You cannot use a single lock for that. And the way you hack doesn't look correct for schedulable contexts...
AFAIK it works.
spin_lock_bh() is to protect data for user-context and softirqs...
We can do away with it if you can tell me how to stop alsa midlevel calling HPI functions in both schedulable and non-schedulable contexts.
Hm? If you'd like to unify the call, then choose always the spin-locked version,
spin_lock_irqsave(&lock, flags); HPI_something(); spin_unlock_irqrestore(&lock, flags);
It's fine to do this in user context (unless HPI_somthing() takes too long time).
There are some busy-wait loops talking to the HW, we really don't want IRQs disabled there.
Currently its not structured like you suggest above.
we have HPI_<object>_<action>() -> creates a HPI message. HPI_Message() /* every message passes thru this gateway */ some functions access HW, need spinlock some functions don't access HW, dont need lock
(E.g. hpi6205.c, the relevant lock is taken in HW_Message by HpiOs_Dsplock_Lock(). But operations on busmaster streams never get here, so can run in parallel - correct "good citizen" behaviour.)
Above this is alsa driver layer, sometimes calling in atomic context, sometimes not in atomic context.
Our cards dont use HW IRQ at all, so we don't want or need to disable irq for our lock.
I believe it is wrong to use spin_lock_bh inside (alsa executed) spin_lock_irqsave. But because we have timer function (softirq), we need spinlock_bh when not inside spin_lock_irqsave.
The current solution was arrived at after much wailing and gnashing of teeth, and I'm loath to change it without very good reason.
Note also that pure HPI driver doesn't have this nasty wrapper, because it doesn't have anything that disables irqs before calling HPI functions.
======
- LockedMem wrappers. These provide tracking for the DMA areas
Need to keep track of all the info about the mem area so it can be freed
OK, then we can leave as is.
...actually you prompted me to look for a simpler way, it will appear as a patch some time.
(?the overhead for a slab cache is 4K to start with - right?, I will exchange this for smaller overheads elsewhere for common usage)
======
E.g. pci_free_consistent(pMemArea->pdev, pMemArea->size, pMemArea->cpu_addr, pMemArea->dma_addr);
Use dma_alloc_coherent() / dma_free_coherent() instead. pci_*_consistent() are obsolete.
ah, ok will change it.
======
The hwdep device can be set up to emulate (accept) HPI ioctls. It's a matter of device name.
OK. On this I have to say "show me the (pseudo)code", because I wouldn't know where to start.
======
struct hpi_handle has bitfields. I think it's used very commonly in
This struct is used for data communication between the hardware and the driver, no?
No. It encapsulates up to 3 indices and an object type into 32 bits. This is generated by hpifunc for say StreamOpen(), then is used again in *the same compilation unit* by StreamStart() etc.
Takashi
regards
Eliot
Takashi Iwai wrote:
- Global variables should have some unique prefix. The kernel code is monolithc C, so they are really global and has no name space.
Did you have a list? There are a few function names that don't start with HPI.
Err, sorry, I meant global variables *and functions*.
BTW is there any way to not export functions/variables that are used only inside the kernel module? (but used between object files that make up the module so cannot be made static)
I.e. if the module was compiled as one huge sourcefile, almost everything could be static.
regards
Eliot
At Wed, 19 Dec 2007 11:13:25 +1300, Eliot Blennerhassett wrote:
Takashi Iwai wrote:
- Global variables should have some unique prefix. The kernel code is monolithc C, so they are really global and has no name space.
Did you have a list? There are a few function names that don't start with HPI.
Err, sorry, I meant global variables *and functions*.
BTW is there any way to not export functions/variables that are used only inside the kernel module? (but used between object files that make up the module so cannot be made static)
Remember that a driver can be built in a kernel, not as a module.
I.e. if the module was compiled as one huge sourcefile, almost everything could be static.
You can do it in that way, of course :)
Takashi
Takashi Iwai wrote:
At Wed, 19 Dec 2007 11:13:25 +1300, Eliot Blennerhassett wrote:
BTW is there any way to not export functions/variables that are used only inside the kernel module? (but used between object files that make up the module so cannot be made static)
Remember that a driver can be built in a kernel, not as a module.
I.e. if the module was compiled as one huge sourcefile, almost everything could be static.
You can do it in that way, of course :)
Hmmm. Seems to be a general problem whether building a shared library, or a kernel module.
I.e. as soon as I have more than one source file in my lib or module, I suddenly have to make some functions not static, and they become visible globally.
Is there no way to compile and link a.c and b.c so that only specific entry points are visible before statically linking ab.o with the rest of the kernel or making ab.o into a kernel module.
-- Eliot
At Thu, 20 Dec 2007 17:09:16 +1300, Eliot Blennerhassett wrote:
Takashi Iwai wrote:
At Wed, 19 Dec 2007 11:13:25 +1300, Eliot Blennerhassett wrote:
BTW is there any way to not export functions/variables that are used only inside the kernel module? (but used between object files that make up the module so cannot be made static)
Remember that a driver can be built in a kernel, not as a module.
I.e. if the module was compiled as one huge sourcefile, almost everything could be static.
You can do it in that way, of course :)
Hmmm. Seems to be a general problem whether building a shared library, or a kernel module.
I.e. as soon as I have more than one source file in my lib or module, I suddenly have to make some functions not static, and they become visible globally.
Is there no way to compile and link a.c and b.c so that only specific entry points are visible before statically linking ab.o with the rest of the kernel or making ab.o into a kernel module.
The global symbols in modules aren't really exported to outside unless you set EXPORT_SYMBOL*(). The problem is rather a built-in driver. When you compile the driver in a kernel, it becomes all global to the kernel (not to modules) because it's a single object in the end.
(For shared libraries, you can set Versions script to restrict the exported functions, BTW.)
Anyway, a safer way is to use some unique prefix to each global function and variables. In the case of ALSA, usually we use snd_* for global objects.
Takashi
participants (2)
-
Eliot Blennerhassett -
Takashi Iwai