From owner-svn-src-head@freebsd.org Fri Apr 24 16:07:42 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 57E5E2B9AE3; Fri, 24 Apr 2020 16:07:42 +0000 (UTC) (envelope-from agapon@gmail.com) Received: from mail-lf1-f67.google.com (mail-lf1-f67.google.com [209.85.167.67]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 497zbK3PqJz4ZJ7; Fri, 24 Apr 2020 16:07:41 +0000 (UTC) (envelope-from agapon@gmail.com) Received: by mail-lf1-f67.google.com with SMTP id m2so8123637lfo.6; Fri, 24 Apr 2020 09:07:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:autocrypt :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=6InkqwFP4H0NiNFR0K7iz9aXFXR2yuodPYg63Fb/MdA=; b=Srn8ov8E+A0wTViwzj8MsJK4iERbCR/+/CLqRtziw+OdjgcgpOlkOvpLk3gcmsoEP8 dtpUfZ/2CClEGq2ib7d1feHL9iP/ATlbfoVdWR9SXyjvYPEALaQjXbNORink2BSJGKd/ P2vv47T+0YgFNI8fBckX465+HaU+esYloypeaUbqpRKDCVC8Tez/chdJWxX6ZlhE1A06 6Bijwmf/GH7iBwk77oObHqWqUiusCNh/4BGxTY4Ci7XriXUMw3juHSHELpnESl1J/rlo XEXldIIynlZ4gSyigWkTJap0B3t19fmqX5ihfN2cCtik3aWZ9tMeg5tOVm0c1p4PTNUK myVw== X-Gm-Message-State: AGi0PuZsnNOEKSu8+7RmVf/EwLvbZfo4zlrOJvm+IC37oOFJt4zM628a oLm7+ZvNFD3HpoDooALLvH/c4D/K4dg= X-Google-Smtp-Source: APiQypKX2KNu6Su+WxsFGRjqPRnoO/+scc/rPcbnmq8gQsMZqTyiBCQMymRIVA1Srj2wY0igfz71dA== X-Received: by 2002:a19:7411:: with SMTP id v17mr6842688lfe.27.1587744458457; Fri, 24 Apr 2020 09:07:38 -0700 (PDT) Received: from [192.168.0.88] (east.meadow.volia.net. [93.72.151.96]) by smtp.googlemail.com with ESMTPSA id i3sm4511106ljg.82.2020.04.24.09.07.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 24 Apr 2020 09:07:37 -0700 (PDT) Subject: Re: svn commit: r360241 - head/sys/dev/ichiic To: Warner Losh , John Baldwin Cc: src-committers , svn-src-all , svn-src-head References: <202004240749.03O7nMSc066344@repo.freebsd.org> <32586115-2c82-2497-ba4c-b3471887518d@FreeBSD.org> From: Andriy Gapon Openpgp: preference=signencrypt Autocrypt: addr=avg@FreeBSD.org; prefer-encrypt=mutual; keydata= mQINBFm4LIgBEADNB/3lT7f15UKeQ52xCFQx/GqHkSxEdVyLFZTmY3KyNPQGBtyvVyBfprJ7 mAeXZWfhat6cKNRAGZcL5EmewdQuUfQfBdYmKjbw3a9GFDsDNuhDA2QwFt8BmkiVMRYyvI7l N0eVzszWCUgdc3qqM6qqcgBaqsVmJluwpvwp4ZBXmch5BgDDDb1MPO8AZ2QZfIQmplkj8Y6Z AiNMknkmgaekIINSJX8IzRzKD5WwMsin70psE8dpL/iBsA2cpJGzWMObVTtCxeDKlBCNqM1i gTXta1ukdUT7JgLEFZk9ceYQQMJJtUwzWu1UHfZn0Fs29HTqawfWPSZVbulbrnu5q55R4PlQ /xURkWQUTyDpqUvb4JK371zhepXiXDwrrpnyyZABm3SFLkk2bHlheeKU6Yql4pcmSVym1AS4 dV8y0oHAfdlSCF6tpOPf2+K9nW1CFA8b/tw4oJBTtfZ1kxXOMdyZU5fiG7xb1qDgpQKgHUX8 7Rd2T1UVLVeuhYlXNw2F+a2ucY+cMoqz3LtpksUiBppJhw099gEXehcN2JbUZ2TueJdt1FdS ztnZmsHUXLxrRBtGwqnFL7GSd6snpGIKuuL305iaOGODbb9c7ne1JqBbkw1wh8ci6vvwGlzx rexzimRaBzJxlkjNfMx8WpCvYebGMydNoeEtkWldtjTNVsUAtQARAQABtB5BbmRyaXkgR2Fw b24gPGF2Z0BGcmVlQlNELm9yZz6JAlQEEwEIAD4WIQS+LEO7ngQnXA4Bjr538m7TUc1yjwUC WbgsiAIbIwUJBaOagAULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAKCRB38m7TUc1yj+JAEACV l9AK/nOWAt/9cufV2fRj0hdOqB1aCshtSrwHk/exXsDa4/FkmegxXQGY+3GWX3deIyesbVRL rYdtdK0dqJyT1SBqXK1h3/at9rxr9GQA6KWOxTjUFURsU7ok/6SIlm8uLRPNKO+yq0GDjgaO LzN+xykuBA0FlhQAXJnpZLcVfPJdWv7sSHGedL5ln8P8rxR+XnmsA5TUaaPcbhTB+mG+iKFj GghASDSfGqLWFPBlX/fpXikBDZ1gvOr8nyMY9nXhgfXpq3B6QCRYKPy58ChrZ5weeJZ29b7/ QdEO8NFNWHjSD9meiLdWQaqo9Y7uUxN3wySc/YUZxtS0bhAd8zJdNPsJYG8sXgKjeBQMVGuT eCAJFEYJqbwWvIXMfVWop4+O4xB+z2YE3jAbG/9tB/GSnQdVSj3G8MS80iLS58frnt+RSEw/ psahrfh0dh6SFHttE049xYiC+cM8J27Aaf0i9RflyITq57NuJm+AHJoU9SQUkIF0nc6lfA+o JRiyRlHZHKoRQkIg4aiKaZSWjQYRl5Txl0IZUP1dSWMX4s3XTMurC/pnja45dge/4ESOtJ9R 8XuIWg45Oq6MeIWdjKddGhRj3OohsltKgkEU3eLKYtB6qRTQypHHUawCXz88uYt5e3w4V16H lCpSTZV/EVHnNe45FVBlvK7k7HFfDDkryLkCDQRZuCyIARAAlq0slcsVboY/+IUJdcbEiJRW be9HKVz4SUchq0z9MZPX/0dcnvz/gkyYA+OuM78dNS7Mbby5dTvOqfpLJfCuhaNYOhlE0wY+ 1T6Tf1f4c/uA3U/YiadukQ3+6TJuYGAdRZD5EqYFIkreARTVWg87N9g0fT9BEqLw9lJtEGDY EWUE7L++B8o4uu3LQFEYxcrb4K/WKmgtmFcm77s0IKDrfcX4doV92QTIpLiRxcOmCC/OCYuO jB1oaaqXQzZrCutXRK0L5XN1Y1PYjIrEzHMIXmCDlLYnpFkK+itlXwlE2ZQxkfMruCWdQXye syl2fynAe8hvp7Mms9qU2r2K9EcJiR5N1t1C2/kTKNUhcRv7Yd/vwusK7BqJbhlng5ZgRx0m WxdntU/JLEntz3QBsBsWM9Y9wf2V4tLv6/DuDBta781RsCB/UrU2zNuOEkSixlUiHxw1dccI 6CVlaWkkJBxmHX22GdDFrcjvwMNIbbyfQLuBq6IOh8nvu9vuItup7qemDG3Ms6TVwA7BD3j+ 3fGprtyW8Fd/RR2bW2+LWkMrqHffAr6Y6V3h5kd2G9Q8ZWpEJk+LG6Mk3fhZhmCnHhDu6CwN MeUvxXDVO+fqc3JjFm5OxhmfVeJKrbCEUJyM8ESWLoNHLqjywdZga4Q7P12g8DUQ1mRxYg/L HgZY3zfKOqcAEQEAAYkCPAQYAQgAJhYhBL4sQ7ueBCdcDgGOvnfybtNRzXKPBQJZuCyIAhsM BQkFo5qAAAoJEHfybtNRzXKPBVwQAKfFy9P7N3OsLDMB56A4Kf+ZT+d5cIx0Yiaf4n6w7m3i ImHHHk9FIetI4Xe54a2IXh4Bq5UkAGY0667eIs+Z1Ea6I2i27Sdo7DxGwq09Qnm/Y65ADvXs 3aBvokCcm7FsM1wky395m8xUos1681oV5oxgqeRI8/76qy0hD9WR65UW+HQgZRIcIjSel9vR XDaD2HLGPTTGr7u4v00UeTMs6qvPsa2PJagogrKY8RXdFtXvweQFz78NbXhluwix2Tb9ETPk LIpDrtzV73CaE2aqBG/KrboXT2C67BgFtnk7T7Y7iKq4/XvEdDWscz2wws91BOXuMMd4c/c4 OmGW9m3RBLufFrOag1q5yUS9QbFfyqL6dftJP3Zq/xe+mr7sbWbhPVCQFrH3r26mpmy841ym dwQnNcsbIGiBASBSKksOvIDYKa2Wy8htPmWFTEOPRpFXdGQ27awcjjnB42nngyCK5ukZDHi6 w0qK5DNQQCkiweevCIC6wc3p67jl1EMFY5+z+zdTPb3h7LeVnGqW0qBQl99vVFgzLxchKcl0 R/paSFgwqXCZhAKMuUHncJuynDOP7z5LirUeFI8qsBAJi1rXpQoLJTVcW72swZ42IdPiboqx NbTMiNOiE36GqMcTPfKylCbF45JNX4nF9ElM0E+Y8gi4cizJYBRr2FBJgay0b9Cp Message-ID: <02449f05-6c43-9cb0-b427-34034a3c358f@FreeBSD.org> Date: Fri, 24 Apr 2020 19:07:35 +0300 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:60.0) Gecko/20100101 Firefox/60.0 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 497zbK3PqJz4ZJ7 X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of agapon@gmail.com designates 209.85.167.67 as permitted sender) smtp.mailfrom=agapon@gmail.com X-Spamd-Result: default: False [-2.21 / 15.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; FROM_HAS_DN(0.00)[]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17:c]; RCVD_TLS_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.00)[FreeBSD.org]; NEURAL_HAM_LONG(-1.00)[-0.999,0]; RCPT_COUNT_FIVE(0.00)[5]; RCVD_COUNT_THREE(0.00)[3]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[67.167.85.209.list.dnswl.org : 127.0.5.0]; NEURAL_HAM_MEDIUM(-1.00)[-0.997,0]; IP_SCORE(-0.22)[ip: (-0.22), ipnet: 209.85.128.0/17(-0.40), asn: 15169(-0.43), country: US(-0.05)]; FORGED_SENDER(0.30)[avg@FreeBSD.org,agapon@gmail.com]; RWL_MAILSPIKE_POSSIBLE(0.00)[67.167.85.209.rep.mailspike.net : 127.0.0.17]; MIME_TRACE(0.00)[0:+]; R_DKIM_NA(0.00)[]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; FROM_NEQ_ENVFROM(0.00)[avg@FreeBSD.org,agapon@gmail.com]; RECEIVED_SPAMHAUS_PBL(0.00)[96.151.72.93.khpj7ygk5idzvmvt5x4ziurxhy.zen.dq.spamhaus.net : 127.0.0.10] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Apr 2020 16:07:42 -0000 On 24/04/2020 18:11, Warner Losh wrote: > > > On Fri, Apr 24, 2020 at 8:47 AM Andriy Gapon > wrote: > > On 24/04/2020 17:29, Warner Losh wrote: > > > > > > On Fri, Apr 24, 2020 at 1:49 AM Andriy Gapon > > >> wrote: > > > >     Author: avg > >     Date: Fri Apr 24 07:49:21 2020 > >     New Revision: 360241 > >     URL: https://svnweb.freebsd.org/changeset/base/360241 > > > >     Log: > >       ig4: ensure that drivers always attach in correct order > > > >       Use DRIVER_MODULE_ORDERED(SI_ORDER_ANY) so that ig4's ACPI attachment > >       happens after iicbus and acpi_iicbus drivers are registered. > > > >       I have seen a problem where iicbus attached under ig4 instead of > >       acpi_iicbus when ig4.ko was loaded with kldload.  I believe that that > >       happened because ig4 driver was a first driver to register, it attached > >       and created an iicbus child.  Then iicbus driver was registered and, > >       since it was the only driver that could attach to the iicbus child > >       device, it did exactly that.  After that acpi_iicbus driver was > >       registered.  It would be able to attach to the iicbus device, but it was > >       already attached, so nothing happened. > > > > > > Can you post more details of which devices are affected? From the description > > and the patch, I don't see how this could fix things. > > I think I listed them all: ig4iic with acpi attachment, iicbus and acpi_iicbus. > acpi >  \--- ig4iic >        \---- iicbus vs acpi_iicbus > > I tried to explain the problem and the fix in the commit message.  If you want > to discuss any specifics, let's continue with specifics.  If there is anything > unclear in my explanation, I can clarify, but I need to understand what needs to > be clarified. > > > That won't fix the stated problem. It will. It does. You made me write an essay to explain why :) > If changing the module order fixes something, > it's the wrong fix. Which is why I asked to make sure I understood the issue (it > was unclear if it was at the level indicated, or for the children of the iicbus). > > iicbus returns BUS_PROBE_GENERIC (-100) from its probe routine, while > acpi_iicbus returns BUS_PROBE_DEFAULT (-20). This means that acpi_iicbus should > always win. So something else is going on. We don't specify order on other > devices except for some weird, special needs drivers (this is not such a driver). This driver, along with all of other I2C controller drivers, has a particular quirk, so it can be called weird. It does not matter what the probe routines return if one of the drivers is not added to "newbus" at all (even if its code is loaded). Its probe method is not executed. And that's what I tried to explain in the commit message. Now, why this driver as well as all SMBus and I2C controller drivers are somewhat weird... There is a multitude of drivers for SMBus and I2C controllers. Those drivers have different names. So, using newbus speak, smbus and iicbus drivers can attach to [newbus] devices under many different [newbus] buses. For that reason, the corresponding DRIVER_MODULE declarations are not consolidated in smbus.c and iicbus.c; instead, they are spread over individual controller drivers. So, loading of smbus or iicbus module does not result in a call to devclass_add_driver() that would register these drivers under some bus. And that's an unusual thing comparing to the most straightforward drivers. Let's use ig4 as an example. Across its source files we had the following DRIVER_MODULE declarations: DRIVER_MODULE(ig4iic, acpi, ... -- in ig4_acpi.c DRIVER_MODULE(iicbus, ig4iic, ... -- in ig4_iic.c DRIVER_MODULE(acpi_iicbus, ig4iic, ... -- in ig4_iic.c The first one is needed to register ig4iic driver under acpi bus. Other two are needed to register iicbus and acpi_iicbus drivers under ig4iic bus. The order is not explicitly defined, so the corresponding declaration can be processed in any order when ig4.ko is loaded and so the corresponding devclass_add_driver() can be called in any order. So, I observed in practice the scenario where the drivers were added in the written order. First, ig4iic was added under acpi, it found the corresponding device, probed and attached to it. In its attach method it added a new device named "iicbus" as a child. Then, iicbus driver was added under ig4iic bus. That driver found the child device and attached to it. Only then acpi_iicbus was added and it was too late to the party. So, this is really about an order in which DRIVER_MODULE-s (which translate to sysinit-s) _within a kld_ are processed. Essentially, about an order of objects in the corresponding section of a loadable ELF object. MODULE_DEPEND does not affect that order. Just as an aside, for a contrast, gpiobus uses a different model. There, all controller drivers must have "gpio" as their driver names (driver->name). So, a single DRIVER_MODULE(gpiobus, gpio, ...) in the gpiobus.c code is sufficient for gpiobus driver to be added under all controllers. And that registration always happens before a controller driver is added because of MODULE_DEPEND between it and gpiobus. > Unless I'm mistaken, the real problem is that the following line is missing instead. > MODULE_DEPEND(ig4iic, acpi_iicbus, 1, 1, 1); > which makes the module dependencies explicit. Can you add that to ig4_acpi.c, > revert your change and see if that works? It will ensure that the acpi_iicbus > driver is loaded first. If things break because it isn't, it sounds like a hard > dependency for ig4. Since we're already pulling in acpi, that doesn't seem > unreasonable to me. Just for clarity, acpi_iicbus is compiled into iicbus.ko (on relevant platforms, of course), same as iicbus. It's not a separate kld. -- Andriy Gapon