kern/96743: [sk] [patch] broken 32-bit register operations

Thu May 4 02:10:20 UTC 2006

>Number:         96743
>Category:       kern
>Synopsis:       [sk] [patch] broken 32-bit register operations
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu May 04 02:10:19 GMT 2006
>Closed-Date:
>Last-Modified:
>Originator:     Craig Leres
>Release:        FreeBSD 6.1-BETA4 i386
>Organization:
Lawrence Berkeley National Laboratory
>Environment:
	fox 15 % uname -a
	FreeBSD fox.ee.lbl.gov 6.1-BETA4 FreeBSD 6.1-BETA4 #29: Wed May  3 17:45:44 PDT 2006     leres at fox.ee.lbl.gov:/usr/src/6.1-BETA4/sys/i386/compile/LBLSMP  i386

	SysKonnect SK-9843 gige adapter

>Description:
	While attempting to apply a local patch that implements
	receive hardware timestamps (nice for research and intrusion
	detection applications) with the syskonnect SK-9843, I
	noticed that some 32-bit register operations are broken.
	Most of the registers, including XM_MODE and XM_TSTAMP_READ,
	must be accessed 16-bits at a time. One step in enabling
	timestamps is to set the XM_MODE_TIMESTAMP bit in the XM_MODE
	register but it was not possible to access the high bytes
	of the register.

	I developed my timestamp patch with 4.8-RELEASE using version
	1.8.2.1 of if_skreg.h which defines SK_XM_READ_4() and
	SK_XM_WRITE_4() macros that use sk_win_read_2() and
	sk_win_read_2() to read and write 16 bits at a time, as
	required by the hardware.

	Version 1.8.2.2 of the include file change the macros to
	use sk_win_read_4() and sk_win_write_4() which only return
	the low order 16-bits.

	This problem probably wasn't detected before now because
	the current driver only seems to ever access bits in the
	low bytes of the registers (e.g. XM_MODE_RX_PROMISC in
	XM_MODE).

>How-To-Repeat:
	This patch helps demonstrate the problem:

==============================================================================

*** if_sk.c.virgin	Wed May  3 17:56:30 2006
--- if_sk.c	Wed May  3 17:56:37 2006
***************
*** 2532,2537 ****
--- 2532,2540 ----
  
  	/* Save the XMAC II revision */
  	sc_if->sk_xmac_rev = XM_XMAC_REV(SK_XM_READ_4(sc_if, XM_DEVID));
+ printf("sk%d: XM_DEVID 0x%x (0x%x/0x%x)\n",
+     sc_if->sk_unit, SK_XM_READ_4(sc_if, XM_DEVID),
+     SK_XM_READ_2(sc_if, XM_DEVID_HI), SK_XM_READ_2(sc_if, XM_DEVID_LO));
  
  	/*
  	 * Perform additional initialization for external PHYs,

==============================================================================

	After applying the patch and building and booting the kernel,
	the driver reports:

	    skc0: <SysKonnect Gigabit Ethernet (V1.0)> port 0x3000-0x30ff mem 0xdd300000-0xd
	    d303fff irq 48 at device 1.0 on pci3
	    skc0: SysKonnect SK-NET Gigabit Ethernet Adapter SK-9843 SX rev. (0x0)
	    sk0: <XaQti Corp. XMAC II> on skc0
	    sk0: Ethernet address: 00:00:5a:9a:80:18
	    sk0: XM_DEVID 0xae20 (0xe0/0xae20)

	Notice that SK_XM_READ_4() returned the same value as the
	low bytes SK_XM_READ_2() and that the high bytes have 0xe0,
	not zero.

>Fix:
	The appended patch (vs. 1.29.2.1 of if_skreg.h) re-enables
	the 2-byte versions if the SK_XM_READ_4() and SK_XM_WRITE_4()
	macros. It also protects the SK_XM_WRITE_4() macro.

	After booting the new kernel, the debugging printout
	shows the complete device id value:

	    sk0: XM_DEVID 0xe0ae20 (0xe0/0xae20)

	It looks like this patch would also apply to the -current
	version (1.35).

==============================================================================

*** if_skreg.h.virgin	Wed May  3 18:28:19 2006
--- if_skreg.h	Wed May  3 18:29:56 2006
***************
*** 1150,1156 ****
  #define SK_XMAC_REG(sc, reg)	(((reg) * 2) + SK_XMAC1_BASE +		\
  	(((sc)->sk_port) * (SK_XMAC2_BASE - SK_XMAC1_BASE)))
  
! #if 0
  #define SK_XM_READ_4(sc, reg)						\
  	((sk_win_read_2(sc->sk_softc,					\
  	SK_XMAC_REG(sc, reg)) & 0xFFFF) |				\
--- 1150,1160 ----
  #define SK_XMAC_REG(sc, reg)	(((reg) * 2) + SK_XMAC1_BASE +		\
  	(((sc)->sk_port) * (SK_XMAC2_BASE - SK_XMAC1_BASE)))
  
! /* 
!  * For at least some registers (e.g. XM_MODE) with at least some
!  * boards (e.g. SK-9843) you can only read/write 16 bits at a time.
!  */
! #if 1
  #define SK_XM_READ_4(sc, reg)						\
  	((sk_win_read_2(sc->sk_softc,					\
  	SK_XMAC_REG(sc, reg)) & 0xFFFF) |				\
***************
*** 1158,1167 ****
  	SK_XMAC_REG(sc, reg + 2)) & 0xFFFF) << 16))
  
  #define SK_XM_WRITE_4(sc, reg, val)					\
! 	sk_win_write_2(sc->sk_softc, SK_XMAC_REG(sc, reg),		\
! 	((val) & 0xFFFF));						\
! 	sk_win_write_2(sc->sk_softc, SK_XMAC_REG(sc, reg + 2),		\
! 	((val) >> 16) & 0xFFFF)
  #else
  #define SK_XM_READ_4(sc, reg)		\
  	sk_win_read_4(sc->sk_softc, SK_XMAC_REG(sc, reg))
--- 1162,1173 ----
  	SK_XMAC_REG(sc, reg + 2)) & 0xFFFF) << 16))
  
  #define SK_XM_WRITE_4(sc, reg, val)					\
! 	do {								\
! 		sk_win_write_2(sc->sk_softc, SK_XMAC_REG(sc, reg),	\
! 		((val) & 0xFFFF));					\
! 		sk_win_write_2(sc->sk_softc, SK_XMAC_REG(sc, reg + 2),	\
! 		((val) >> 16) & 0xFFFF);				\
! 	} while (0)
  #else
  #define SK_XM_READ_4(sc, reg)		\
  	sk_win_read_4(sc->sk_softc, SK_XMAC_REG(sc, reg))
>Release-Note:
>Audit-Trail:
>Unformatted: