From owner-svn-src-all@freebsd.org Fri Apr 24 16:24:07 2020 Return-Path: Delivered-To: svn-src-all@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 AC22A2BA2CD; Fri, 24 Apr 2020 16:24:07 +0000 (UTC) (envelope-from chmeeedalf@gmail.com) Received: from mail-qv1-xf43.google.com (mail-qv1-xf43.google.com [IPv6:2607:f8b0:4864:20::f43]) (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 497zyH4758z4bZN; Fri, 24 Apr 2020 16:24:07 +0000 (UTC) (envelope-from chmeeedalf@gmail.com) Received: by mail-qv1-xf43.google.com with SMTP id fb4so4943540qvb.7; Fri, 24 Apr 2020 09:24:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=iSfGLSoRNTWeAfyVUjikZRj1fMDzjlcqKRjTe4Y6P4Q=; b=JUklKpzSY0ZiRpmwxlks6d0URH7vTd1KCb9CrdpbrRP/wf4IS1HNBHnd4tFdXWeOx8 9On+8TbGj8GC8gUPvxlEV1aT0FxEsquSHiJ/CGHTIPxGUYTKFuVdihbySlSOOV+UlrAC ETLvrltfHB4F/c11cXRJWDuH0La63bWM1xKwgl65cCbtBwQRfC73hQWicwfcwhtoNtST wgJcl5xedQc7qeMFre9ONLrzwl2tAfK/k6kQOumZBYWD6JzsS+/tw0k+MOZPeMgemEHK AgUYTR3h8crLESblTvvkJb2Q6VBmbfQluNNNy0BZ/NE/Vag3kzpSScWhiPEDoGdb70Zp ZdYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=iSfGLSoRNTWeAfyVUjikZRj1fMDzjlcqKRjTe4Y6P4Q=; b=ku0RAhla1jKMtnwVXIXO5KFiTVdKaSY1okWekX+XdazAWD2tY8rawcFSDl+ivqXvhG S/kECQ5kXkTQaHIzPNAWo5NYjNNc3dLxYR9CWsjeA+nUfyhRUl2yGQUuPMMK0IQJ4TIZ RmVXNK84SXOBpcI2wJ7donwv9sTg6TQYyu3HYgj3gYZZ37sDR55hS5e7l/xD394/vLpa DMcqPwxtpgKu+eRv+xK6zqnkUQXycgOCzEKKYlzPKIlLhNUsoSLJ6ExeCFRvmzl8OoN5 ujqo0fmyL7SWjWwVH3jD/FiU/zbSfa1PUVEFY9FK+l1koi59l4jWqZraBOoXnVO3bwkN toUA== X-Gm-Message-State: AGi0PuYa7t4K5V1hnbfeVQxoxMV5wViPOaZaqXO8bgp2e80VRTMoxAtL tCCgpENLptSZU+I4aHnwzdFFPvPUHyY= X-Google-Smtp-Source: APiQypLZjuJv8sWNbmUIqhtwWrqa30FnLTZFTI69jHOjy5cQKWpiNnri/HjadiOVXkFM6uYSmw+pKw== X-Received: by 2002:a0c:b44c:: with SMTP id e12mr10117540qvf.30.1587745446472; Fri, 24 Apr 2020 09:24:06 -0700 (PDT) Received: from titan.knownspace (173-19-125-130.client.mchsi.com. [173.19.125.130]) by smtp.gmail.com with ESMTPSA id h19sm4234969qtk.78.2020.04.24.09.24.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Apr 2020 09:24:06 -0700 (PDT) Date: Fri, 24 Apr 2020 11:23:56 -0500 From: Justin Hibbits To: Andriy Gapon Cc: Warner Losh , John Baldwin , src-committers , svn-src-all , svn-src-head Subject: Re: svn commit: r360241 - head/sys/dev/ichiic Message-ID: <20200424112356.6b8a867e@titan.knownspace> In-Reply-To: <02449f05-6c43-9cb0-b427-34034a3c358f@FreeBSD.org> References: <202004240749.03O7nMSc066344@repo.freebsd.org> <32586115-2c82-2497-ba4c-b3471887518d@FreeBSD.org> <02449f05-6c43-9cb0-b427-34034a3c358f@FreeBSD.org> X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; powerpc64-portbld-freebsd13.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 497zyH4758z4bZN X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-6.00 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Apr 2020 16:24:07 -0000 On Fri, 24 Apr 2020 19:07:35 +0300 Andriy Gapon wrote: > On 24/04/2020 18:11, Warner Losh wrote: > >=20 > >=20 > > On Fri, Apr 24, 2020 at 8:47 AM Andriy Gapon > > wrote: > >=20 > > On 24/04/2020 17:29, Warner Losh wrote: =20 > > > > > > > > > On Fri, Apr 24, 2020 at 1:49 AM Andriy Gapon > > =20 > > =20 > > > >> wrote: > > > > > >=C2=A0 =C2=A0 =C2=A0Author: avg > > >=C2=A0 =C2=A0 =C2=A0Date: Fri Apr 24 07:49:21 2020 > > >=C2=A0 =C2=A0 =C2=A0New Revision: 360241 > > >=C2=A0 =C2=A0 =C2=A0URL: https://svnweb.freebsd.org/changeset/base= /360241 > > > > > >=C2=A0 =C2=A0 =C2=A0Log: > > >=C2=A0 =C2=A0 =C2=A0=C2=A0 ig4: ensure that drivers always attach = in correct order > > > > > >=C2=A0 =C2=A0 =C2=A0=C2=A0 Use DRIVER_MODULE_ORDERED(SI_ORDER_ANY)= so that ig4's > > >ACPI attachment happens after iicbus and acpi_iicbus drivers > > >are registered. > > > > > >=C2=A0 =C2=A0 =C2=A0=C2=A0 I have seen a problem where iicbus atta= ched under ig4 > > >instead of acpi_iicbus when ig4.ko was loaded with kldload.=C2=A0 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.=C2=A0 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. =20 > >=20 > > I think I listed them all: ig4iic with acpi attachment, iicbus > > and acpi_iicbus. acpi > > =C2=A0\--- ig4iic > > =C2=A0 =C2=A0 =C2=A0 =C2=A0\---- iicbus vs acpi_iicbus > >=20 > > I tried to explain the problem and the fix in the commit > > message.=C2=A0 If you want to discuss any specifics, let's continue with > > specifics.=C2=A0 If there is anything unclear in my explanation, I can > > clarify, but I need to understand what needs to be clarified. > >=20 > >=20 > > That won't fix the stated problem. =20 >=20 > It will. It does. You made me write an essay to explain why :) >=20 > > 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). > >=20 > > iicbus returns BUS_PROBE_GENERIC (-100) from its probe routine, > > while acpi_iicbus returns=C2=A0BUS_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). =20 >=20 > This driver, along with all of other I2C controller drivers, has a > particular quirk, so it can be called weird. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > > 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. =20 >=20 > Just for clarity, acpi_iicbus is compiled into iicbus.ko (on relevant > platforms, of course), same as iicbus. It's not a separate kld. >=20 Can you look at how ofw_iicbus does this? Everything works just fine with that, and it's compiled into the iicbus module as well. Perhaps you can pick some ideas from there. One thing I remember doing on the fsl_i2c driver was to just name the driver iichb and everything worked beautifully. Yes, it was sort of a cop-out vs adding another attachment, but it solved the problem, and does make sense. - Justin