Re: AMD Geode LX crypto accelerator (glxsb) (and padlock.ko)

On Thursday 07 August 2008 05:58:54 pm Patrick Lamaizière wrote:
> Le Thu, 07 Aug 2008 16:45:37 -0400,
>
> Mike Tancsa <mike_at_sentex.net> a écrit :
> > At 10:34 PM 8/6/2008, Mike Tancsa wrote:
> > >At 05:42 PM 7/6/2008, Patrick Lamaizière wrote:
> > >>I think that the driver is ready and works fine, i didn't change
> > >>anything since the latest version "22/06/08"
> > >>http://user.lamaiziere.net/patrick/glxsb-220608.tar.gz
> > >
> > >Hi,
> > >         I was doing some testing with openvpn
> > > and was wondering if there is perhaps a memory leak ?
>
> Yes, padlock and glxsb may not reuse the free sessions. See:
> http://www.nabble.com/Recent-Padlock-changes-break-ssh-tt18582674.html#a185
>82674
>
> For glxsb, i've made a diff between current that include a fix:
> http://user.lamaiziere.net/patrick/diff-glxsb-8.txt
>
> Thank you, regards.

A few suggestions of things I noticed in the patch:

- Use __RCSID("$FreeBSD"); in place of the KERNEL_RCSID stuff.
- You shouldn't call pci_enable_io().  bus_alloc_resource() will do that
  for you after it has programmed the BAR.
- Don't use bus space tag/handle.  Instead of bus_space_foo(tag, handle, ...)
  use bus_foo(resource, ...)
- Functions w/o any local variables should start with a blank line before
  the code (style(9)), e.g. the probe() routine.
- Less whitespace in certain areas.  For example, I would reduce the
  two lines after setting the bus tag/handle down to 1, and I would remove
  the blank lines after the "Disable interrupts" and
  "Initialize our task queue" comments.
- No need to explicitly zero the softc in attach(), new-bus already does this
  for you.
- Just use 'device_set_desc()' in probe.  device_set_desc_copy() is for when
  you are working with a string that will go away (such as if you generate
  a description into a buffer on the stack).
- You need to do a taskqueue_drain() in your detach routine before
  taskqueue_free(), but after detaching the driver from the crypto interface
  and anything else that could schedule the task to run.  Probably fine to
  do this either right before or right after the current call to
  callout_stop().
- You should do a callout_drain() rather than a callout_stop() in your detach
  routine.

-- 
John Baldwin