From owner-freebsd-hackers@FreeBSD.ORG Tue May 24 12:19:01 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D6C481065670 for ; Tue, 24 May 2011 12:19:01 +0000 (UTC) (envelope-from philip-dev@soeberg.net) Received: from pasmtpA.tele.dk (pasmtpa.tele.dk [80.160.77.114]) by mx1.freebsd.org (Postfix) with ESMTP id 5673F8FC17 for ; Tue, 24 May 2011 12:19:01 +0000 (UTC) Received: from mail.soeberg.net (0x573f534a.cpe.ge-1-1-0-1109.bynqu1.customer.tele.dk [87.63.83.74]) by pasmtpA.tele.dk (Postfix) with ESMTP id 7587F8004EE; Tue, 24 May 2011 13:56:59 +0200 (CEST) Received: from [10.240.10.87] ([188.120.77.114]) (authenticated user philip@soeberg.net) by mail.soeberg.net (using TLSv1/SSLv3 with cipher AES256-SHA (256 bits)); Tue, 24 May 2011 13:57:02 +0200 Message-ID: <4DDB9D0E.5030309@soeberg.net> Date: Tue, 24 May 2011 13:57:02 +0200 From: Philip Soeberg User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.17) Gecko/20110414 Lightning/1.0b2 Thunderbird/3.1.10 MIME-Version: 1.0 To: freebsd-hackers@freebsd.org References: <4DDA6B95.3090704@soeberg.net> <201105231032.20084.jhb@freebsd.org> In-Reply-To: <201105231032.20084.jhb@freebsd.org> Content-Type: multipart/mixed; boundary="------------080803000504030803000001" Cc: freebsd@intel.com Subject: Re: device_detach() on a device used by ixgbe driver (FreeBSD 7-STABLE through to 9-CURRENT) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: philip-dev@soeberg.net List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 May 2011 12:19:01 -0000 This is a multi-part message in MIME format. --------------080803000504030803000001 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Hi, Attached is a patch against FreeBSD HEAD (svn 222248, aka. FreeBSD 9-CURRENT) which solves the Intel IXGBE driver problem reported below. Changes: * IXGBE's probe() function now return BUS_PROBE_DEFAULT as it should. * IXGBE's attach() now inquire resource_disabled() to enable hints to explicitly disable supported adapters. * device_if.m's wording has been updated. (This file's descriptions of DEVICE_* functions seem redundant as it is also written in man 9 DEVICE_*.9) * IXGBE's module build Makefile now use name if_ixgbe instead of old-style ixgbe. Man pages and forth's loader.conf already reflect this. The wording in device_if.m is likely to provide you with a good laugh, so I suggest rewording it further :) It is an attempt at ensuring future driver developers does -not- return zero in their probe() function, unless absolutely required. I am however not sure if we need this file's descriptions any longer, as the man pages seem far better? The attached file also patch nicely against RELENG_8, which is still open. I assume it is enough to submit this patch to the freebsd-hackers mailinglist? Philip Soeberg //// philip-dev@soeberg.net (@ @) soeberg.net ------------------------------oOO--(_)--OOo------------------ On 23-05-2011 16:32, John Baldwin wrote: > On Monday, May 23, 2011 10:13:41 am Philip Soeberg wrote: >> Hi fellow FreeBSD hackers, >> >> I've just completed designing a new driver for the Intels IXGBE suite of >> network adapters, but is building my driver as a kernel module to be >> loaded after system boot. >> >> The current sys/dev/ixgbe/ixgbe.c driver which attach to Intels adapters >> return a zero in it's probe() function (which equals to >> BUS_PROBE_SPECIFIC).. This has the distinct disadvantage that I cannot, >> through my module, call a device_detach() on the devices I support, and >> afterward expect being probed for them. A BUS_PROBE_SPECIFIC, according >> to wording in sys/sys/bus.h, inform the OS that "Only I can use this >> device". >> >> I assume this (transcanding from FreeBSD 7.0-STABLE through to FreeBSD >> 9-CURRENT) is in error? I would expect sys/dev/ixgbe/ixgbe.c's probe() >> function to return BUS_PROBE_DEFAULT, which is the "Base OS default >> driver".. > Yes, that is true. > >> If this is true, then we should probably also update >> sys/kern/device_if.m's description of the probe() method as to reflect >> the BUS_PROBE_* return values in a clearer way than is currently described. >> Do you want me to provide a patch? (it's really a one liner for ixgbe.c >> and a couple of alterations to the device_if.m, if need be) > device_if.m was probably just never updated from when BUS_PROBE_* were added. > Updating it would be a good thing. > >> I would also expect the ixgbe.c driver to do a quick resource_disabled() >> in it's attach() function, so that we can disable specific adapters >> through kenv hint.ix.0.disabled=1.. > That is not universally supported (i.e. it's not a part of new-bus > specifically). For buses that support hinted devices, they do all generally > support being able to disable a hinted device, but disabling bus-enumerated > devices is not generally supported. > >> Given that I can't use device_detach() on a device hogged by the IXGBE >> driver, can any one of you help me with a way around this problem? I >> can't use the hints, and I can't detach() the device.. how can I get my >> kernel module to attach the device? > I think ixgbe has to be fixed to use BUS_PROBE_DEFAULT. Very few drivers > should use '0' for their probe return value. > --------------080803000504030803000001 Content-Type: text/plain; name="freebsd-head-svn-222248.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="freebsd-head-svn-222248.patch" Index: sys/kern/device_if.m =================================================================== --- sys/kern/device_if.m (revision 222248) +++ sys/kern/device_if.m (working copy) @@ -97,10 +97,18 @@ * used to generate an informative message when DEVICE_ATTACH() * is called. * + * Driver election is determined by the negative return value of + * the probe function on a highest-value-best-match type effort. + * Drivers included in the kernel normally return -20 (via + * convenience define BUS_PROBE_DEFAULT) which allows for a more + * specific driver to be installed at a later time. + * * As a special case, if a driver returns zero, the driver election * is cut short and that driver will attach to the device - * immediately. + * immediately. A return value of zero is rarely used. * + * Please see man page for DEVICE_PROBE() for more information. + * * For example, a probe method for a pci device driver might look * like this: * @@ -110,7 +118,7 @@ * if (pci_get_vendor(dev) == FOOVENDOR && * pci_get_device(dev) == FOODEVICE) { * device_set_desc(dev, "Foo device"); - * return (0); + * return (BUS_PROBE_DEFAULT); * } * return (ENXIO); * } @@ -128,7 +136,7 @@ * @retval 0 if the driver strongly matches this device * @retval negative if the driver can match this device - the * least negative value is used to select the - * driver + * driver. Typical return value is BUS_PROBE_DEFAULT. * @retval ENXIO if the driver does not match the device * @retval positive if some kind of error was detected during * the probe, a regular unix error code should Index: sys/modules/ixgbe/Makefile =================================================================== --- sys/modules/ixgbe/Makefile (revision 222248) +++ sys/modules/ixgbe/Makefile (working copy) @@ -1,6 +1,6 @@ #$FreeBSD$ .PATH: ${.CURDIR}/../../dev/ixgbe -KMOD = ixgbe +KMOD = if_ixgbe SRCS = device_if.h bus_if.h pci_if.h SRCS += ixgbe.c ixv.c # Shared source Index: sys/dev/ixgbe/ixgbe.c =================================================================== --- sys/dev/ixgbe/ixgbe.c (revision 222248) +++ sys/dev/ixgbe/ixgbe.c (working copy) @@ -318,7 +318,7 @@ * ixgbe_probe determines if the driver should be loaded on * adapter based on PCI vendor/device id of the adapter. * - * return 0 on success, positive on failure + * return BUS_PROBE_DEFAULT on success, ENXIO on failure *********************************************************************/ static int @@ -357,7 +357,7 @@ ixgbe_driver_version); device_set_desc_copy(dev, adapter_name); ++ixgbe_total_ports; - return (0); + return (BUS_PROBE_DEFAULT); } ent++; } @@ -383,6 +383,13 @@ u16 csum; u32 ctrl_ext; + /* If device unit is disabled through hints, e.g. + (hint.ix..disabled=1), don't attach it */ + if (resource_disabled(device_get_name(dev), device_get_unit(dev))) { + device_printf(dev, "device is disabled\n"); + return (ENXIO); + } + INIT_DEBUGOUT("ixgbe_attach: begin"); /* Allocate, clear, and link in our adapter structure */ --------------080803000504030803000001--