Date: Mon, 23 Nov 2009 09:55:50 +0100 From: Hans Petter Selasky <hselasky@freebsd.org> To: Andrew Thompson <thompsa@freebsd.org> Cc: Perforce Change Reviews <perforce@freebsd.org>, Nathan Whitehorn <nwhitehorn@freebsd.org> Subject: Re: PERFORCE change 170842 for review Message-ID: <200911230955.51855.hselasky@freebsd.org> In-Reply-To: <1280352d0911221414i28e58b80o2c1a5958d3ce54ef@mail.gmail.com> References: <200911192235.nAJMZ2XH072195@repoman.freebsd.org> <4B06B19F.7050501@freebsd.org> <1280352d0911221414i28e58b80o2c1a5958d3ce54ef@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sunday 22 November 2009 23:14:04 Andrew Thompson wrote: > 2009/11/21 Nathan Whitehorn <nwhitehorn@freebsd.org>: > > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200911230955.51855.hselasky>