Bug 50859

Summary: Request for support for Authentec AES2810
Product: libfprint Reporter: Thomas Vogt <acc-bugz-freedesktop>
Component: libfprintAssignee: libfprint-bugs
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: anarsoul, tatsaechlich
Version: unspecified   
Hardware: All   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: aes2550/aes2810 driver
aes2550/aes2810 driver, v2

Description Thomas Vogt 2012-06-07 14:57:05 UTC
I know that newer devices have different fingerprint devices. But the AES2810 used to be in many Thinkpad Laptops (X200, X300, T400, T500 ...) and many of them are still in use today.

Very recently Greg Kerr from Authentec made clear that he is willing to support the open source community in this respect and offered his help if somebody contacts him: https://bugs.launchpad.net/ubuntu/+source/libfprint/+bug/657031/comments/16

Unfortunately, I'm not a developer. But I see a chance here in getting support for a device that had been an ugly spot on our hardware blacklist for long enough :)

I would really appreciate, if somebody took the effort to make the AES2810 work under Linux!
Comment 1 Vasily Khoruzhick 2012-10-10 09:03:36 UTC
Please test libfprint from my repo:
https://github.com/anarsoul/libfprint branch aes2550-v2,

It works on my AES2550 and I have one report that it works on AES2810, but it still requires some testing for sure.
Comment 2 Thomas Vogt 2012-10-10 15:23:10 UTC
Unfortunately, I don't have the possibility to test this myself at the moment. But I asked on thinkpad-forum.de if there is somebody willing and able to test it. I'll report back as soon as I know something new.
Comment 3 Vasily Khoruzhick 2012-10-10 15:41:50 UTC
Created attachment 68404 [details] [review]
aes2550/aes2810 driver
Comment 4 Bastien Nocera 2012-10-10 16:10:42 UTC
Comment on attachment 68404 [details] [review]
aes2550/aes2810 driver

Review of attachment 68404 [details] [review]:
-----------------------------------------------------------------

No need for a signed-off-by.

The main problems I can see are under-documented TODO items, and some magic numbers for some calls, which need to be fixed.

::: libfprint/drivers/aes2550.c
@@ +68,5 @@
> +	unsigned char *second_frame, unsigned int *min_error)
> +{
> +	unsigned int dy;
> +	unsigned int not_overlapped_height = 0;
> +	*min_error = 255 * FRAME_SIZE;

What's the 255 value?

@@ +440,5 @@
> +			fpi_ssm_mark_aborted(ssm, -ENOMEM);
> +			break;
> +		}
> +
> +		data = g_malloc(8192);

What's the magic value here?

@@ +552,5 @@
> +	g_free(transfer->buffer);
> +	libusb_free_transfer(transfer);
> +}
> +
> +/* TODO: use calibration table */

What exactly is left to do here?

@@ +598,5 @@
> +			fpi_ssm_mark_aborted(ssm, -ENOMEM);
> +			break;
> +		}
> +
> +		data = g_malloc(8192);

What's the magic value here?

@@ +636,5 @@
> +			fpi_ssm_mark_aborted(ssm, -ENOMEM);
> +			break;
> +		}
> +
> +		data = g_malloc(8192);

What's the magic value here?

@@ +692,5 @@
> +}
> +
> +static int dev_init(struct fp_img_dev *dev, unsigned long driver_data)
> +{
> +	/* FIXME check endpoints */

What's there to do here?
Comment 5 Vasily Khoruzhick 2012-10-10 16:53:17 UTC
(In reply to comment #4)
> Comment on attachment 68404 [details] [review] [review]
> aes2550/aes2810 driver
> 
> Review of attachment 68404 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> No need for a signed-off-by.

Ok
 
> The main problems I can see are under-documented TODO items, and some magic
> numbers for some calls, which need to be fixed.
> 
> ::: libfprint/drivers/aes2550.c
> @@ +68,5 @@
> > +	unsigned char *second_frame, unsigned int *min_error)
> > +{
> > +	unsigned int dy;
> > +	unsigned int not_overlapped_height = 0;
> > +	*min_error = 255 * FRAME_SIZE;
> 
> What's the 255 value?

Taken from aes2501.c. Actually I wrote this code for aes2501 driver back in 2007, and I don't remember how it works. Let's left it here for now, I'm planning to unify stripes assembling code for all swipe sensors from Authentec in a separate patch.

> @@ +440,5 @@
> > +			fpi_ssm_mark_aborted(ssm, -ENOMEM);
> > +			break;
> > +		}
> > +
> > +		data = g_malloc(8192);
> 
> What's the magic value here?

Just a buffer size, no magic here, it can be any value larger than frame size, will add define anyway.

> @@ +552,5 @@
> > +	g_free(transfer->buffer);
> > +	libusb_free_transfer(transfer);
> > +}
> > +
> > +/* TODO: use calibration table */
> 
> What exactly is left to do here?

Not sure yet, datasheet is rather terse on that, planning to ask AuthenTec.

> @@ +598,5 @@
> > +			fpi_ssm_mark_aborted(ssm, -ENOMEM);
> > +			break;
> > +		}
> > +
> > +		data = g_malloc(8192);
> 
> What's the magic value here?

Same buffer size.

> @@ +636,5 @@
> > +			fpi_ssm_mark_aborted(ssm, -ENOMEM);
> > +			break;
> > +		}
> > +
> > +		data = g_malloc(8192);
> 
> What's the magic value here?

Same buffer size.
 
> @@ +692,5 @@
> > +}
> > +
> > +static int dev_init(struct fp_img_dev *dev, unsigned long driver_data)
> > +{
> > +	/* FIXME check endpoints */
> 
> What's there to do here?

Well, it came from aes2501 driver, in theory it would be nice to check that device _has_ bulk endpoints we're using. In practice usbids check is enough. Will remove FIXME.
Comment 6 Vasily Khoruzhick 2012-10-10 17:16:18 UTC
Created attachment 68408 [details] [review]
aes2550/aes2810 driver, v2

2nd patch version

- added macro for input buffer size
- documented TODOs a bit more
Comment 7 Vasily Khoruzhick 2012-10-11 11:05:18 UTC
Btw, got positive feedback from few testers with AES2810
Comment 8 Bastien Nocera 2012-10-11 13:01:13 UTC
Comment on attachment 68408 [details] [review]
aes2550/aes2810 driver, v2

Review of attachment 68408 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 9 Bastien Nocera 2012-10-11 13:25:03 UTC
Committed to master, many thanks!

commit 83333bce3f06ca55651a7516666ee63a5892f09f
Author: Vasily Khoruzhick <anarsoul@gmail.com>
Date:   Tue Oct 9 01:20:44 2012 +0300

    lib: Add AES2550/AES2810 driver
    
    Initial implementation of AES2550/AES2810 driver.
    Does not support AES2810 crypto engine and external flash.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=50859

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.