From owner-freebsd-hackers@freebsd.org Wed May 29 21:02:08 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 6047115ADAA4 for ; Wed, 29 May 2019 21:02:08 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from outbound1.eu.mailhop.org (outbound1.eu.mailhop.org [52.28.251.132]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3BF858796A for ; Wed, 29 May 2019 21:02:06 +0000 (UTC) (envelope-from ian@freebsd.org) ARC-Seal: i=1; a=rsa-sha256; t=1559163720; cv=none; d=outbound.mailhop.org; s=arc-outbound20181012; b=G2az5quBB6kLXUA2p40xHUro9qgrMhnU7WD0GPE7yXHodNsye+r0jTnxdUq2EgFR5n3EEnucKWH0O ogQFFo1pPPzB0i+/nwUn/AoPicFCRm5EQ2Szq4bUoyQmW6Xp0OnIyVhaT6yBFgkCwAHNIITjgN69uN x41Ou4lCl3m8lc+KN4wvhzwTtSE7LNdx6nh7ocy4u/ryUzIpQYleletwRhs18yq2P71y9YleLT+wAg OKgR2pQo2i5N1oYslWOCXuTEaWChpyL4U3EwBCk35HVSf/yiG4jp6RqEr3JYcBW94nfn4WEsGtx3I6 J/V42slLqJdSLdMi/ugFKGB/rN73eiQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=arc-outbound20181012; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:to:from:subject:message-id:dkim-signature:from; bh=jR3ch9wkGYgVZvp7J45LxnmJD20sit1qLaByl18htKM=; b=AB0lqthLhufs+Vw1ko+sKHl++81/b6336BYlTSLiPIWJf1COcgQmOZdhq4r+gtZK+BkmQGqes8nKb Yv9IfVDq961JsJ+Vfa8IPTR0Fc7iQguHXlnGHokgVMkceZo0ZuHaeYRk6n/v2O//CUCbvTbRrK3Jhg GG7lvg7Kf0NuM90wU5l1qhoZJXlxG3BaPWfTR2UmBMTqY0nP9ozeZDxyz8Gr2kIb14HIRW53Wlkiri kXivY5A9Rw8lAecnlgV2yvu8iFZmP4lXZGSjzQV9hSLUGdBbllL7K+x0SvLx4+sN2IlrFGJXtzIfFp 3+RYweVbec+MVQaA4Fln/+UkOMjvvDw== ARC-Authentication-Results: i=1; outbound3.eu.mailhop.org; spf=softfail smtp.mailfrom=freebsd.org smtp.remote-ip=67.177.211.60; dmarc=none header.from=freebsd.org; arc=none header.oldest-pass=0; DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=dkim-high; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:to:from:subject:message-id:from; bh=jR3ch9wkGYgVZvp7J45LxnmJD20sit1qLaByl18htKM=; b=jI1xApvlRTPL3R/oHzpWgxrjwaJE2mtZ8u9QnLQv17wGUq8VY/Wg5TRJ01K/JAsQL1yhDlynt07YQ QL+DX0Uoaxl8s3NLliBrveOu8GDuzJiq9HnZsZejtcWGwIvpb9udSnPF9jOdh1sP9uVdYBKuTTYZce qC92Bn0hZa43r61qrpsMzNEIvXXOldohUfI5uD5lvS/hGpG2jlbOwluFmB8aAXqeX52+/y+4j/YUH5 DubJexj+nto4XfzjvBUy9o5LCuoBpz897XG1gpBWIRdINZslwUk5P7bxdImAQsizpV1k8SUxPWaSPf 19fbGGXGrErFTqEBfCnO37eGmRKZqig== X-MHO-RoutePath: aGlwcGll X-MHO-User: fdac9a4d-8254-11e9-91aa-b56e4e6b5865 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 67.177.211.60 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (unknown [67.177.211.60]) by outbound3.eu.mailhop.org (Halon) with ESMTPSA id fdac9a4d-8254-11e9-91aa-b56e4e6b5865; Wed, 29 May 2019 21:01:57 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id x4TL1tDA036094; Wed, 29 May 2019 15:01:55 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: Subject: Re: completely broken IICBUS_IVAR_NOSTOP [Was: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?] From: Ian Lepore To: Andriy Gapon , FreeBSD Hackers , Konstantin Belousov Date: Wed, 29 May 2019 15:01:55 -0600 In-Reply-To: References: <1521383420.99081.87.camel@freebsd.org> <0b0dee4b-e958-e25c-72d9-1ca296495802@FreeBSD.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 FreeBSD GNOME Team Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 3BF858796A X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-2.98 / 15.00]; local_wl_from(0.00)[freebsd.org]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.98)[-0.975,0]; ASN(0.00)[asn:16509, ipnet:52.28.0.0/16, country:US]; NEURAL_HAM_LONG(-1.00)[-1.000,0] 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: Wed, 29 May 2019 21:02:08 -0000 On Fri, 2019-05-24 at 20:43 +0300, Andriy Gapon wrote: > 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. > Sorry for the delay in responding to this, I got busy with $work for a few days. I think the right thing to do would be to implement iicbus_transfer directly in the intel_iicbb_driver, then it could do whatever it wants in terms of ignoring STOP without perturbing other drivers. But I'm not sure it's practical to do so unless someone notices a problem, because I'm not sure how we'd test it otherwise. -- Ian > > 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... > > > >