Re: asihpi driver -> kernel

Re: asihpi driver -> kernel

[Takashi Iwai]

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

Re: asihpi driver -> kernel

[Eliot Blennerhassett]

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

Re: asihpi driver -> kernel

[Eliot Blennerhassett]

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.html

regards

Eliot Blennerhassett
AudioScience Inc.

Re: asihpi driver -> kernel

[Takashi Iwai]

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.html

Thanks, I'll take a look at new codes tomorrow (just back from
vacation...)


Takashi

Re: asihpi driver -> kernel

[Takashi Iwai]

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

Re: asihpi driver -> kernel

[Eliot Blennerhassett]

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.

Re: asihpi driver -> kernel

[Takashi Iwai]

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
> 
> 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
>  */

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...

> 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

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

Re: asihpi driver -> kernel

[Eliot Blennerhassett]

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
>>
>> 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
>>  */
> 
> 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.

>> 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
> 
> 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

Re: asihpi driver -> kernel

[Takashi Iwai]

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
> >>
> >> 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
> >>  */
> > 
> > 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).


> >> 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
> > 
> > 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

Re: asihpi driver -> kernel

[Eliot Blennerhassett]

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:

>>>> 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
>>>>  */
>>> 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.

======

>>>> 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

> 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

Re: asihpi driver -> kernel

[Eliot Blennerhassett]

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

Re: asihpi driver -> kernel

[Takashi Iwai]

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

[OT] visible symbols was Re: asihpi driver -> kernel

[Eliot Blennerhassett]

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

Re: [OT] visible symbols was Re: asihpi driver -> kernel

[Takashi Iwai]

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