From owner-svn-src-stable-8@FreeBSD.ORG Sat Sep 10 13:43:04 2011 Return-Path: Delivered-To: svn-src-stable-8@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 786A3106564A; Sat, 10 Sep 2011 13:43:04 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe09.c2i.net [212.247.155.2]) by mx1.freebsd.org (Postfix) with ESMTP id E68C68FC12; Sat, 10 Sep 2011 13:43:02 +0000 (UTC) Received: from [188.126.198.129] (account mc467741@c2i.net HELO laptop002.hselasky.homeunix.org) by mailfe09.swip.net (CommuniGate Pro SMTP 5.2.19) with ESMTPA id 1389927; Sat, 10 Sep 2011 15:42:59 +0200 From: Hans Petter Selasky To: "Robert N. M. Watson" Date: Sat, 10 Sep 2011 15:40:16 +0200 User-Agent: KMail/1.13.5 (FreeBSD/8.2-STABLE; KDE/4.4.5; amd64; ; ) References: <3406F3E2-56E6-47FF-8A60-10DD9F3B3D10@FreeBSD.org> In-Reply-To: <3406F3E2-56E6-47FF-8A60-10DD9F3B3D10@FreeBSD.org> X-Face: *nPdTl_}RuAI6^PVpA02T?$%Xa^>@hE0uyUIoiha$pC:9TVgl.Oq, NwSZ4V"|LR.+tj}g5 %V,x^qOs~mnU3]Gn; cQLv&.N>TrxmSFf+p6(30a/{)KUU!s}w\IhQBj}[g}bj0I3^glmC( :AuzV9:.hESm-x4h240C`9=w MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_Aj2aOn1c6cYTFr5" Message-Id: <201109101540.16656.hselasky@c2i.net> Cc: "svn-src-stable@freebsd.org" , "svn-src-all@freebsd.org" , "src-committers@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) X-BeenThere: svn-src-stable-8@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for only the 8-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Sep 2011 13:43:04 -0000 --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 #include #include -#include /* 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 #include #include -#include /* 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--