From owner-freebsd-hackers@freebsd.org Fri May 24 17:43:40 2019 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 4CAB015B133B for ; Fri, 24 May 2019 17:43:40 +0000 (UTC) (envelope-from agapon@gmail.com) Received: from mail-lj1-f175.google.com (mail-lj1-f175.google.com [209.85.208.175]) (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 4BD6170F61; Fri, 24 May 2019 17:43:39 +0000 (UTC) (envelope-from agapon@gmail.com) Received: by mail-lj1-f175.google.com with SMTP id a10so9389498ljf.6; Fri, 24 May 2019 10:43:39 -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:from:to:references:openpgp:autocrypt :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=YzBa+D3AkTY/1tTmZ9SQaM36dfD/XUcfCLomXRlrjQA=; b=poo6DBTF1dZXHosMBQj7UAsarvkbUu7G/3AdHIHfSEv07FLi59++vEg2OaW+ysW0nm sSNXBy06+PFvHjmTMFcxXNBi4RlMzrGgjuHERtt3pXu/ztrYx3Cjg4Lz+yM9AF5H+7Kq D6loSqZaNkHwK70FjV6/peBlRgiwRL614WzZDNT4wOB8iF3QUKoqmSATsxQZ4xk0l7u1 9tcslDmXTzAWuqaHJqjSAesy3kHfEz453YXKqfLGkgcxxBkRI6n7bhPtOgVZ33E1wd0P c4Qo0MkIzBmzhBnyl8v2iMvdx6nAwO516y665Ls0VZVKIaUSZESxezYC5u+0c9YO0v4l CdpA== X-Gm-Message-State: APjAAAWbJsEGPGN/Hi0Et48D0MvprBE6oX2aCC6pYzvSOBthVfhnFXjq jczHAduxi7J4S2mWMExaNcIazqwj X-Google-Smtp-Source: APXvYqxzn+klaPdnnu9ESsL2dy2jQ/GNPaNTZazAp2bdsIKhuxtFFX43VE+4Qgw+RG52H2YLyRjACQ== X-Received: by 2002:a2e:85d1:: with SMTP id h17mr37377172ljj.1.1558719812092; Fri, 24 May 2019 10:43:32 -0700 (PDT) Received: from [192.168.0.88] (east.meadow.volia.net. [93.72.151.96]) by smtp.googlemail.com with ESMTPSA id i12sm719530lfo.67.2019.05.24.10.43.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 24 May 2019 10:43:31 -0700 (PDT) Subject: completely broken IICBUS_IVAR_NOSTOP [Was: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?] From: Andriy Gapon To: Ian Lepore , FreeBSD Hackers , Konstantin Belousov References: <1521383420.99081.87.camel@freebsd.org> <0b0dee4b-e958-e25c-72d9-1ca296495802@FreeBSD.org> Openpgp: preference=signencrypt Autocrypt: addr=avg@FreeBSD.org; prefer-encrypt=mutual; keydata= xsFNBFm4LIgBEADNB/3lT7f15UKeQ52xCFQx/GqHkSxEdVyLFZTmY3KyNPQGBtyvVyBfprJ7 mAeXZWfhat6cKNRAGZcL5EmewdQuUfQfBdYmKjbw3a9GFDsDNuhDA2QwFt8BmkiVMRYyvI7l N0eVzszWCUgdc3qqM6qqcgBaqsVmJluwpvwp4ZBXmch5BgDDDb1MPO8AZ2QZfIQmplkj8Y6Z AiNMknkmgaekIINSJX8IzRzKD5WwMsin70psE8dpL/iBsA2cpJGzWMObVTtCxeDKlBCNqM1i gTXta1ukdUT7JgLEFZk9ceYQQMJJtUwzWu1UHfZn0Fs29HTqawfWPSZVbulbrnu5q55R4PlQ /xURkWQUTyDpqUvb4JK371zhepXiXDwrrpnyyZABm3SFLkk2bHlheeKU6Yql4pcmSVym1AS4 dV8y0oHAfdlSCF6tpOPf2+K9nW1CFA8b/tw4oJBTtfZ1kxXOMdyZU5fiG7xb1qDgpQKgHUX8 7Rd2T1UVLVeuhYlXNw2F+a2ucY+cMoqz3LtpksUiBppJhw099gEXehcN2JbUZ2TueJdt1FdS ztnZmsHUXLxrRBtGwqnFL7GSd6snpGIKuuL305iaOGODbb9c7ne1JqBbkw1wh8ci6vvwGlzx rexzimRaBzJxlkjNfMx8WpCvYebGMydNoeEtkWldtjTNVsUAtQARAQABzR5BbmRyaXkgR2Fw b24gPGF2Z0BGcmVlQlNELm9yZz7CwZQEEwEIAD4WIQS+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/EVHnNe45FVBlvK7k7HFfDDkryM7BTQRZuCyIARAAlq0slcsVboY/+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 HgZY3zfKOqcAEQEAAcLBfAQYAQgAJhYhBL4sQ7ueBCdcDgGOvnfybtNRzXKPBQJZuCyIAhsM 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: Date: Fri, 24 May 2019 20:43:29 +0300 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <0b0dee4b-e958-e25c-72d9-1ca296495802@FreeBSD.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 4BD6170F61 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; spf=pass (mx1.freebsd.org: domain of agapon@gmail.com designates 209.85.208.175 as permitted sender) smtp.mailfrom=agapon@gmail.com X-Spamd-Result: default: False [-4.12 / 15.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[3]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; RCVD_TLS_LAST(0.00)[]; DMARC_NA(0.00)[FreeBSD.org]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; RCVD_COUNT_THREE(0.00)[3]; IP_SCORE(-1.26)[ip: (-0.55), ipnet: 209.85.128.0/17(-3.42), asn: 15169(-2.29), country: US(-0.06)]; TO_DN_ALL(0.00)[]; MX_GOOD(-0.01)[cached: alt3.gmail-smtp-in.l.google.com]; NEURAL_HAM_SHORT(-0.85)[-0.846,0]; RCVD_IN_DNSWL_NONE(0.00)[175.208.85.209.list.dnswl.org : 127.0.5.0]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; FORGED_SENDER(0.30)[avg@FreeBSD.org,agapon@gmail.com]; 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]; SUBJECT_HAS_QUESTION(0.00)[] X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 May 2019 17:43:40 -0000 A lot of time has passed but the problem is still there. The "nostop" thing is completely broken. I have recently thought about this issue a little bit again and now I think that IVARs are not suitable for nostop property. Simply put, there is no bus (in the "newbus" sense) where such an IVAR could be configured for a child. I see two possible solutions. 1. Make "nostop" a property of an iicbus instance. We can keep the current getter and setter, but they won't be IVAR accessors any more. Also, target devices for the calls to the functions would need to be adjusted. 2. Since this property hasn't found a wider use and remains specific to the intel drm driver, we could just subclass iicbb and put the quirk into the subclassed driver. On 23/03/2018 11:56, Andriy Gapon wrote: > On 18/03/2018 16:30, Ian Lepore wrote: >> Now for the bad news:  don't use it.  It doesn't work.  It's 100% a bug >> in the code that maybe kinda-sorta seemed to work for whoever added it, >> because accidentally the right garbage was on the stack in the local >> nostop var.  The generic transfer code doesn't check that the accessor >> failed so it ends up using stack garbage for nostop.  The reason >> there's g'teed to be no such ivar is because the code is asking the >> wrong device, and it doesn't even have a handle to the right child >> device to get the info it wants. > > > Oh, indeed. > I think that there never was an intention to make "nostop" a property of an i2c > slave, a child of an iicbus device. I think that instead it was supposed to be > a property of the iicbus's parent device, an actual i2c adapter driver. > I guess that the reason that "nostop" became an ivar in iicbus was an incorrect > understanding of how a "bit-banger" device (something implementing iicbb_if), > iicbb device and iicbus device are connected. I think that I was among the > reviewers and I probably had a bit of confusion back then. > > > It seems that the only place where iicbus_get_nostop() is used is > iicbus_transfer_gen(). iicbus_transfer_gen is used in several i2c drivers as an > iicbus_transfer method. it's also used in iicbb, thinly wrapped by > iicbb_transfer(). > So, iicbus_get_nostop() actually translates to a call to BUS_READ_IVAR(parent, > device, 1, &v) where I already substituted one for IICBUS_IVAR_NOSTOP. > Here we can see that while the accessor functions lok quite nice they get > expanded to generic calls without much context. So, whether that > BUS_READ_IVAR() call succeeds and what value it gets depends on whether the > parent bus defines an ivar with a value of 1 and what value that ivar might have > for the driver device. Many buses define at least a couple of ivars. > So, a return value of iicbus_get_nostop() could be consistent for a particular > driver while still being arbitrary. But it can be a piece of stack garbage too, > just as you said. > > The only place where iicbus_set_nostop() is used is intel_iicbb_attach(). > In that case the ivar is "set" on a wrong device at all (the bit-banger, not iicbb). > > > This definitely needs to be fixed / reworked. > Perhaps nostop should become a new interface in iicbus_if and iicbb_if... > -- Andriy Gapon