FreeBSD Mail Archives

Date:      Sat, 10 Sep 2011 15:40:16 +0200
From:      Hans Petter Selasky <hselasky@c2i.net>
To:        "Robert N. M. Watson" <rwatson@freebsd.org>
Cc:        "svn-src-stable@freebsd.org" <svn-src-stable@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-stable-8@freebsd.org" <svn-src-stable-8@freebsd.org>, re@freebsd.org
Subject:   Request for patch approval (Re: svn commit: r225458 - in stable/8/sys: dev/usb dev/usb/quirk dev/usb/storage sys)
Message-ID:  <201109101540.16656.hselasky@c2i.net>
In-Reply-To: <3406F3E2-56E6-47FF-8A60-10DD9F3B3D10@FreeBSD.org>
References:  <D2195F50-8958-41DC-AA26-AEF3156F369B@freebsd.org> <3406F3E2-56E6-47FF-8A60-10DD9F3B3D10@FreeBSD.org>

--Boundary-00=_Aj2aOn1c6cYTFr5
Content-Type: Text/Plain;
  charset="windows-1252"
Content-Transfer-Encoding: 7bit


> Right -- exactly my point. If this change breaks third-party compiled USB
> device drivers, then our current approach to device driver KBIs does not
> allow it to be MFC'd in this form. Are there ways you can reformulate the
> change to avoid breaking those drivers? Sometimes this can be done by
> adding new symbols, rather than replacing currently symbols, although
> mileage varies.

Hi,

Here is my proposal:

Implement test for automatic quirks in function which has access to the USB 
device structure. This decouples the structure change in "struct 
usbd_lookup_info".

The only structure which needs change is "struct usb_device". In 9-current 
this structure will be kept as is. In 8-stable the new element will be moved 
to the end of the structure like suggested, and then there shouldn't be any 
problems.

Please find patches attached.

--HPS

Commit message:

Refactor auto-quirk solution so that we break as few external
drivers as possible.

PR:             usb/160299
Approved by:    re (kib)
Suggested by:   rwatson
MFC after:      0 days


--Boundary-00=_Aj2aOn1c6cYTFr5
Content-Type: text/x-patch; charset="windows-1252";
	name="msc_auto_quirk_8_stable.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
	filename="msc_auto_quirk_8_stable.patch"

Index: sys/dev/usb/usb_dynamic.h
===================================================================
--- sys/dev/usb/usb_dynamic.h	(revision 225458)
+++ sys/dev/usb/usb_dynamic.h	(working copy)
@@ -57,6 +57,5 @@
 void	usb_temp_unload(void *);
 void	usb_quirk_unload(void *);
 void	usb_bus_unload(void *);
-usb_test_quirk_t usb_test_quirk_w;
 
 #endif					/* _USB_DYNAMIC_H_ */
Index: sys/dev/usb/quirk/usb_quirk.c
===================================================================
--- sys/dev/usb/quirk/usb_quirk.c	(revision 225458)
+++ sys/dev/usb/quirk/usb_quirk.c	(working copy)
@@ -588,7 +588,7 @@
 	}
 	mtx_unlock(&usb_quirk_mtx);
 done:
-	return (usb_test_quirk_w(info, quirk));
+	return (0);			/* no quirk match */
 }
 
 static struct usb_quirk_entry *
Index: sys/dev/usb/usbdi.h
===================================================================
--- sys/dev/usb/usbdi.h	(revision 225458)
+++ sys/dev/usb/usbdi.h	(working copy)
@@ -353,7 +353,6 @@
 	uint16_t idVendor;
 	uint16_t idProduct;
 	uint16_t bcdDevice;
-	uint16_t autoQuirk[USB_MAX_AUTO_QUIRK];
 	uint8_t	bDeviceClass;
 	uint8_t	bDeviceSubClass;
 	uint8_t	bDeviceProtocol;
Index: sys/dev/usb/usb_device.c
===================================================================
--- sys/dev/usb/usb_device.c	(revision 225458)
+++ sys/dev/usb/usb_device.c	(working copy)
@@ -1239,8 +1239,6 @@
 usb_init_attach_arg(struct usb_device *udev,
     struct usb_attach_arg *uaa)
 {
-	uint8_t x;
-
 	memset(uaa, 0, sizeof(*uaa));
 
 	uaa->device = udev;
@@ -1256,9 +1254,6 @@
 	uaa->info.bDeviceProtocol = udev->ddesc.bDeviceProtocol;
 	uaa->info.bConfigIndex = udev->curr_config_index;
 	uaa->info.bConfigNum = udev->curr_config_no;
-
-	for (x = 0; x != USB_MAX_AUTO_QUIRK; x++)
-		uaa->info.autoQuirk[x] = udev->autoQuirk[x];
 }
 
 /*------------------------------------------------------------------------*
@@ -2389,8 +2384,22 @@
 usb_test_quirk(const struct usb_attach_arg *uaa, uint16_t quirk)
 {
 	uint8_t found;
+	uint8_t x;
 
+	if (quirk == UQ_NONE)
+		return (0);
+
+	/* search the automatic per device quirks first */
+
+	for (x = 0; x != USB_MAX_AUTO_QUIRK; x++) {
+		if (uaa->device->autoQuirk[x] == quirk)
+			return (1);
+	}
+
+	/* search global quirk table, if any */
+
 	found = (usb_test_quirk_p) (&uaa->info, quirk);
+
 	return (found);
 }
 
@@ -2798,7 +2807,8 @@
 	uint8_t x;
 
 	for (x = 0; x != USB_MAX_AUTO_QUIRK; x++) {
-		if (udev->autoQuirk[x] == 0) {
+		if (udev->autoQuirk[x] == 0 ||
+		    udev->autoQuirk[x] == quirk) {
 			udev->autoQuirk[x] = quirk;
 			return (0);	/* success */
 		}
Index: sys/dev/usb/usb_device.h
===================================================================
--- sys/dev/usb/usb_device.h	(revision 225458)
+++ sys/dev/usb/usb_device.h	(working copy)
@@ -149,7 +149,6 @@
 
 	uint16_t power;			/* mA the device uses */
 	uint16_t langid;		/* language for strings */
-	uint16_t autoQuirk[USB_MAX_AUTO_QUIRK];		/* dynamic quirks */
 
 	uint8_t	address;		/* device addess */
 	uint8_t	device_index;		/* device index in "bus->devices" */
@@ -191,6 +190,8 @@
 #endif
 
 	uint32_t clear_stall_errors;	/* number of clear-stall failures */
+
+	uint16_t autoQuirk[USB_MAX_AUTO_QUIRK];		/* dynamic quirks */
 };
 
 /* globals */
Index: sys/dev/usb/usb_dynamic.c
===================================================================
--- sys/dev/usb/usb_dynamic.c	(revision 225458)
+++ sys/dev/usb/usb_dynamic.c	(working copy)
@@ -50,12 +50,12 @@
 #include <dev/usb/usb_process.h>
 #include <dev/usb/usb_device.h>
 #include <dev/usb/usb_dynamic.h>
-#include <dev/usb/quirk/usb_quirk.h>
 
 /* function prototypes */
 static usb_handle_req_t usb_temp_get_desc_w;
 static usb_temp_setup_by_index_t usb_temp_setup_by_index_w;
 static usb_temp_unsetup_t usb_temp_unsetup_w;
+static usb_test_quirk_t usb_test_quirk_w;
 static usb_quirk_ioctl_t usb_quirk_ioctl_w;
 
 /* global variables */
@@ -72,19 +72,9 @@
 	return (USB_ERR_INVAL);
 }
 
-uint8_t
+static uint8_t
 usb_test_quirk_w(const struct usbd_lookup_info *info, uint16_t quirk)
 {
-	uint8_t x;
-
-	if (quirk == UQ_NONE)
-		return (0);	/* no match */
-
-	for (x = 0; x != USB_MAX_AUTO_QUIRK; x++) {
-		if (info->autoQuirk[x] == quirk)
-			return (1);	/* match */
-	}
-
 	return (0);			/* no match */
 }
 
Index: sys/sys/param.h
===================================================================
--- sys/sys/param.h	(revision 225458)
+++ sys/sys/param.h	(working copy)
@@ -58,7 +58,7 @@
  *		in the range 5 to 9.
  */
 #undef __FreeBSD_version
-#define __FreeBSD_version 802511	/* Master, propagated to newvers */
+#define __FreeBSD_version 802510	/* Master, propagated to newvers */
 
 #ifdef _KERNEL
 #define	P_OSREL_SIGSEGV		700004

--Boundary-00=_Aj2aOn1c6cYTFr5
Content-Type: text/x-patch; charset="windows-1252";
	name="msc_auto_quirk_9_current.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
	filename="msc_auto_quirk_9_current.patch"

=== sys/dev/usb/quirk/usb_quirk.c
==================================================================
--- sys/dev/usb/quirk/usb_quirk.c	(revision 225404)
+++ sys/dev/usb/quirk/usb_quirk.c	(local)
@@ -588,7 +588,7 @@
 	}
 	mtx_unlock(&usb_quirk_mtx);
 done:
-	return (usb_test_quirk_w(info, quirk));
+	return (0);			/* no quirk match */
 }
 
 static struct usb_quirk_entry *
=== sys/dev/usb/usb_device.c
==================================================================
--- sys/dev/usb/usb_device.c	(revision 225404)
+++ sys/dev/usb/usb_device.c	(local)
@@ -1239,8 +1239,6 @@
 usb_init_attach_arg(struct usb_device *udev,
     struct usb_attach_arg *uaa)
 {
-	uint8_t x;
-
 	memset(uaa, 0, sizeof(*uaa));
 
 	uaa->device = udev;
@@ -1256,9 +1254,6 @@
 	uaa->info.bDeviceProtocol = udev->ddesc.bDeviceProtocol;
 	uaa->info.bConfigIndex = udev->curr_config_index;
 	uaa->info.bConfigNum = udev->curr_config_no;
-
-	for (x = 0; x != USB_MAX_AUTO_QUIRK; x++)
-		uaa->info.autoQuirk[x] = udev->autoQuirk[x];
 }
 
 /*------------------------------------------------------------------------*
@@ -2389,8 +2384,22 @@
 usb_test_quirk(const struct usb_attach_arg *uaa, uint16_t quirk)
 {
 	uint8_t found;
+	uint8_t x;
 
+	if (quirk == UQ_NONE)
+		return (0);
+
+	/* search the automatic per device quirks first */
+
+	for (x = 0; x != USB_MAX_AUTO_QUIRK; x++) {
+		if (uaa->device->autoQuirk[x] == quirk)
+			return (1);
+	}
+
+	/* search global quirk table, if any */
+
 	found = (usb_test_quirk_p) (&uaa->info, quirk);
+
 	return (found);
 }
 
@@ -2723,7 +2732,8 @@
 	uint8_t x;
 
 	for (x = 0; x != USB_MAX_AUTO_QUIRK; x++) {
-		if (udev->autoQuirk[x] == 0) {
+		if (udev->autoQuirk[x] == 0 ||
+		    udev->autoQuirk[x] == quirk) {
 			udev->autoQuirk[x] = quirk;
 			return (0);	/* success */
 		}
=== sys/dev/usb/usb_dynamic.c
==================================================================
--- sys/dev/usb/usb_dynamic.c	(revision 225404)
+++ sys/dev/usb/usb_dynamic.c	(local)
@@ -50,12 +50,12 @@
 #include <dev/usb/usb_process.h>
 #include <dev/usb/usb_device.h>
 #include <dev/usb/usb_dynamic.h>
-#include <dev/usb/quirk/usb_quirk.h>
 
 /* function prototypes */
 static usb_handle_req_t usb_temp_get_desc_w;
 static usb_temp_setup_by_index_t usb_temp_setup_by_index_w;
 static usb_temp_unsetup_t usb_temp_unsetup_w;
+static usb_test_quirk_t usb_test_quirk_w;
 static usb_quirk_ioctl_t usb_quirk_ioctl_w;
 
 /* global variables */
@@ -72,19 +72,9 @@
 	return (USB_ERR_INVAL);
 }
 
-uint8_t
+static uint8_t
 usb_test_quirk_w(const struct usbd_lookup_info *info, uint16_t quirk)
 {
-	uint8_t x;
-
-	if (quirk == UQ_NONE)
-		return (0);	/* no match */
-
-	for (x = 0; x != USB_MAX_AUTO_QUIRK; x++) {
-		if (info->autoQuirk[x] == quirk)
-			return (1);	/* match */
-	}
-
 	return (0);			/* no match */
 }
 
=== sys/dev/usb/usb_dynamic.h
==================================================================
--- sys/dev/usb/usb_dynamic.h	(revision 225404)
+++ sys/dev/usb/usb_dynamic.h	(local)
@@ -57,6 +57,5 @@
 void	usb_temp_unload(void *);
 void	usb_quirk_unload(void *);
 void	usb_bus_unload(void *);
-usb_test_quirk_t usb_test_quirk_w;
 
 #endif					/* _USB_DYNAMIC_H_ */
=== sys/dev/usb/usbdi.h
==================================================================
--- sys/dev/usb/usbdi.h	(revision 225404)
+++ sys/dev/usb/usbdi.h	(local)
@@ -353,7 +353,6 @@
 	uint16_t idVendor;
 	uint16_t idProduct;
 	uint16_t bcdDevice;
-	uint16_t autoQuirk[USB_MAX_AUTO_QUIRK];
 	uint8_t	bDeviceClass;
 	uint8_t	bDeviceSubClass;
 	uint8_t	bDeviceProtocol;
=== sys/sys/param.h
==================================================================
--- sys/sys/param.h	(revision 225404)
+++ sys/sys/param.h	(local)
@@ -58,7 +58,7 @@
  *		in the range 5 to 9.
  */
 #undef __FreeBSD_version
-#define __FreeBSD_version 900043	/* Master, propagated to newvers */
+#define __FreeBSD_version 900044	/* Master, propagated to newvers */
 
 #ifdef _KERNEL
 #define	P_OSREL_SIGSEGV		700004

--Boundary-00=_Aj2aOn1c6cYTFr5--

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201109101540.16656.hselasky>

Header And Logo

Peripheral Links

Site Navigation

Header And Logo

Peripheral Links

Search

Site Navigation