From owner-p4-projects@FreeBSD.ORG Mon Nov 23 08:54:21 2009 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 4E5021065694; Mon, 23 Nov 2009 08:54:21 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E4C6D106566C; Mon, 23 Nov 2009 08:54:20 +0000 (UTC) (envelope-from hselasky@freebsd.org) Received: from swip.net (mailfe06.swip.net [212.247.154.161]) by mx1.freebsd.org (Postfix) with ESMTP id DA0C08FC08; Mon, 23 Nov 2009 08:54:19 +0000 (UTC) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.0 c=1 a=IUOrzmqebuoA:10 a=MnI1ikcADjEx7bvsp0jZvQ==:17 a=6I5d2MoRAAAA:8 a=dT19lrtdAAAA:8 a=q5UOD905aazlUW_grYMA:9 a=RhFLDsNr4xPMmAy1bIUA:7 a=O2KSP9-ODAnT6hHHuaVfAjd7nsoA:4 a=SV7veod9ZcQA:10 Received: from [188.126.201.140] (account mc467741@c2i.net HELO laptop.adsl.tele2.no) by mailfe06.swip.net (CommuniGate Pro SMTP 5.2.16) with ESMTPA id 1319496969; Mon, 23 Nov 2009 09:54:16 +0100 Received-SPF: softfail receiver=mailfe06.swip.net; client-ip=188.126.201.140; envelope-from=hselasky@freebsd.org From: Hans Petter Selasky To: Andrew Thompson Date: Mon, 23 Nov 2009 09:55:50 +0100 User-Agent: KMail/1.11.4 (FreeBSD/9.0-CURRENT; KDE/4.2.4; i386; ; ) References: <200911192235.nAJMZ2XH072195@repoman.freebsd.org> <4B06B19F.7050501@freebsd.org> <1280352d0911221414i28e58b80o2c1a5958d3ce54ef@mail.gmail.com> In-Reply-To: <1280352d0911221414i28e58b80o2c1a5958d3ce54ef@mail.gmail.com> X-Face: (%:6u[ldzJ`0qjD7sCkfdMmD*RxpO< =?iso-8859-1?q?Q0yAl=7E=3F=60=27F=3FjDVb=5DE6TQ7=27=23h-VlLs=7Dk/=0A=09?=(yxg(p!IL.`#ng"%`BMrham7%UK,}VH\wUOm=^>wEEQ+KWt[{J#x6ow~JO:,zwp.(t; @ =?iso-8859-1?q?Aq=0A=09=3A4=3A=26nFCgDb8=5B3oIeTb=5E=27?=",; u{5{}C9>"PuY\)!=#\u9SSM-nz8+SR~B\!qBv MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200911230955.51855.hselasky@freebsd.org> Cc: Perforce Change Reviews , Nathan Whitehorn Subject: Re: PERFORCE change 170842 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Nov 2009 08:54:21 -0000 On Sunday 22 November 2009 23:14:04 Andrew Thompson wrote: > 2009/11/21 Nathan Whitehorn : > > Hans Petter Selasky wrote: > >> On Thursday 19 November 2009 23:47:59 Nathan Whitehorn wrote: > >>>> @@ -1530,7 +1574,7 @@ > >>>> return (ENXIO); > >>>> > >>>> if (usbd_lookup_id_by_uaa(atp_devs, sizeof(atp_devs), uaa) == > >>>> 0) - return BUS_PROBE_SPECIFIC; > >>>> + return 0; > >>>> else > >>>> return ENXIO; > >>>> } > >>> > >>> Why are you replacing symbolic constants with less informative numeric > >>> ones? -Nathan > >> > >> Because returning zero in probe has special meaning and is hardcoded in > >> the subr_bus.c code aswell. The other return values will not be changed. > > > > It's the same thing as far as the code is concerned, of course, my > > complaint was merely a style issue. Using the constant makes the meaning > > of the return value clearer, especially since this driver uses this > > return value to override the BUS_PROBE_GENERIC priority of ums(4). > > Changing it from the constant that was already there seemed like a step > > backward in readability. > > I think we should really bring back the probe returns for usb, to > allow drivers to be overridden. > ie, UMATCH_VENDOR_PRODUCT vs UMATCH_IFACECLASS > > http://fxr.watson.org/fxr/source/dev/usb/usbdi.h?v=FREEBSD72#L236 > > Something like... > > Index: usb_lookup.c > =================================================================== > --- usb_lookup.c (revision 199667) > +++ usb_lookup.c (working copy) > @@ -133,6 +133,33 @@ done: > return (NULL); > } > > +int > +usbd_probe_priority(const struct usb_device_id *id) > +{ > + int pri; > + > + pri = UMATCH_GENERIC; > + > + /* Probe priority, lowest to highest */ > + if (id->match_flag_int_class) > + pri = UMATCH_IFACECLASS; > + if (id->match_flag_int_subclass) > + pri = UMATCH_IFACECLASS_IFACESUBCLASS; > + if (id->match_flag_int_protocol) > + pri = UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO; > + if (id->match_flag_dev_protocol) > + pri = UMATCH_DEVCLASS_DEVSUBCLASS_DEVPROTO; > + if (id->match_flag_dev_class && id->match_flag_dev_subclass) > + pri = UMATCH_DEVCLASS_DEVSUBCLASS; > + if (id->match_flag_vendor && id->match_flag_product) > + pri = UMATCH_VENDOR_PRODUCT; > + if (id->match_flag_vendor && id->match_flag_product && > + (id->match_flag_dev_lo || id->match_flag_dev_hi)) > + pri = UMATCH_VENDOR_PRODUCT_REV; > + > + return (pri); > +} > + > > /*------------------------------------------------------------------------* > * usbd_lookup_id_by_uaa - factored out code > * > @@ -148,7 +175,7 @@ usbd_lookup_id_by_uaa(const struct usb_device_id * > if (id) { > /* copy driver info */ > uaa->driver_info = id->driver_info; > - return (0); > + return (usbd_probe_priority(id)); > } > return (ENXIO); > } You need to improve the example a little bit, because multiple of those prioroties you are checking can be set at the same time. Else your patch makes sense, except for that fact that we need some additional logic around the uaa->driver_info field, that it is only updated when the priority increases towards zero? --HPS