From owner-freebsd-hackers@freebsd.org Fri Mar 23 10:02:53 2018 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 DED9AF5ACE6 for ; Fri, 23 Mar 2018 10:02:52 +0000 (UTC) (envelope-from agapon@gmail.com) Received: from mail-lf0-f46.google.com (mail-lf0-f46.google.com [209.85.215.46]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4686B6ADDE; Fri, 23 Mar 2018 10:02:52 +0000 (UTC) (envelope-from agapon@gmail.com) Received: by mail-lf0-f46.google.com with SMTP id v207-v6so17434194lfa.10; Fri, 23 Mar 2018 03:02:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=bPwT2CEU/aUGablCLwFH9OQvSCUmn+MTE/aJyi65wk4=; b=OSEuoIrOh4NuQYQ6grUtBR9sIjar090BRKspMJkQGr6R/rQ7g7r/coQfTnRAPLtaF6 fWQqTr0yDea3FgCfi5wqq7j+JEJecgxVtNvSSitfheD+EAKWdF9jnkmPoIeCadCnREX9 CDZFJxq/Ubxol6iPOpyjA9IFHhnM98RzIjks1bwjTiYPZGGArCa7MeaT7oJa9M+lEEIv tkqMySg8DRC/NnuCXfrHCxxdTpdsE3SIaRDV9dQnGH23kImf46ySU41mmxU+hqQOpK0h F7P0m6r5/hSn2T+ERZzdKiFc0xd0mkmlukVv2ER3jUIqloXBtCLMcGZOCriiwe1pVMDS nlrA== X-Gm-Message-State: AElRT7ElOxxkQ0bv4lahBSDprWcJymOoMqfEEmM7swUV+QPCY9WENcCV 4JDJNnr+SLFAziF60zWf9oJvsGnF5vk= X-Google-Smtp-Source: AG47ELuVsK+H8tqy00KJnttmmPBKOZSv9DvOObp1WqaqlNQeOnzQDgVcToYvz5r6QORy6yTMcAoG+w== X-Received: by 2002:a19:e112:: with SMTP id y18-v6mr18497943lfg.102.1521798978394; Fri, 23 Mar 2018 02:56:18 -0700 (PDT) Received: from [192.168.0.88] (east.meadow.volia.net. [93.72.151.96]) by smtp.googlemail.com with ESMTPSA id o88-v6sm2132511lfg.34.2018.03.23.02.56.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 23 Mar 2018 02:56:17 -0700 (PDT) From: Andriy Gapon Subject: Re: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined? To: Ian Lepore , FreeBSD Hackers Cc: Konstantin Belousov References: <1521383420.99081.87.camel@freebsd.org> X-Mozilla-News-Host: news://news.gmane.org Message-ID: <0b0dee4b-e958-e25c-72d9-1ca296495802@FreeBSD.org> Date: Fri, 23 Mar 2018 11:56:16 +0200 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1521383420.99081.87.camel@freebsd.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Mar 2018 10:02:53 -0000 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