From owner-svn-src-head@freebsd.org Fri Jun 15 06:13:07 2018 Return-Path: Delivered-To: svn-src-head@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 836F5101FDDB; Fri, 15 Jun 2018 06:13:07 +0000 (UTC) (envelope-from melounmichal@gmail.com) Received: from mail-wm0-x22f.google.com (mail-wm0-x22f.google.com [IPv6:2a00:1450:400c:c09::22f]) (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 E7D3F7B11A; Fri, 15 Jun 2018 06:13:06 +0000 (UTC) (envelope-from melounmichal@gmail.com) Received: by mail-wm0-x22f.google.com with SMTP id n5-v6so1722572wmc.5; Thu, 14 Jun 2018 23:13:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:reply-to:subject:to:references:openpgp:autocrypt:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=LG0fnmDppAgXo7L9Y951gZOinP+Hmk1KnxwnruNV6RM=; b=obQZNxAbLhKMDvJU0HvW2xdzCPon5+jlD+7bWtytXAVXumPMiAdLbVNG8MFlsDWKLc imLcertiluhbrmhWbRSUxLmXHFHIwE7TBOs3T/rM9ZY4bL8ZKs654FQT1IKcLkWyiar6 TtlOMCfzyBounpxv33pk8Z/zfcrCYTjIPZFkwJogivrhuZSePLRWEAN7StEgBafL1SPc yx8RYy3+pcWr/Rb5TB/scA8nqGblWaUvCUHrd3b3GPsms9L+aEed/N4gJGRVBvhPoMsQ ejJ/aWcke3sSahMeuPPuYiqkhTaS0ha91ShIAWdkCRcpd9d+atoX6PEAltC4/klZjRoT UP6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:reply-to:subject:to:references:openpgp :autocrypt:message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=LG0fnmDppAgXo7L9Y951gZOinP+Hmk1KnxwnruNV6RM=; b=KusMjiTHbDMb1yVR0eU1y8znzcleN4ci1+htABAQgymk1W65jk3Q/gm6OWwfBaJ/ci bRQ5X/BwIg7VA66HVxam6hw0PENonYOC1746T5kZETHlnnFzIpKrSbsDwJ7I3SDhA+mz nkRp6Pu6KdJok0GGjGfkbs90gbVbpelmaAePyx6D2Zs7sTHZ2a5OKHLjuK5C+LRVfLt7 1hWbN9Kf+Oqy3d7ulLfTnZ2mVmzCu14NDYsMr5rN+Xif/LcZVprZ0f5qmLYdlP56UEfj Wjunh2+IJWqsRgKtnlBcNqMwZRi3t2fuAHaliSBaxfrbnOy8Eg+P9ETmt2qQpIhzx8FE sRkw== X-Gm-Message-State: APt69E11i8Vmwr89MoB6wZfnBxz+9hp9NndAWNyUXDn4eRGw/+aiGVaH q3Edn+VDlLoF18dRSkI/IarGlV30 X-Google-Smtp-Source: ADUXVKKU7IpQ5QpfPZyMzaQHcbaexJ8YDVo4T3MI83kVy4Zfq1K0yr/j0DYSP2gaPWbNaC+DQ3PiBg== X-Received: by 2002:a1c:e618:: with SMTP id d24-v6mr97910wmh.154.1529043185032; Thu, 14 Jun 2018 23:13:05 -0700 (PDT) Received: from [88.208.79.100] (halouny.humusoft.cz. [88.208.79.100]) by smtp.gmail.com with ESMTPSA id j131-v6sm1436617wmg.24.2018.06.14.23.13.04 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 14 Jun 2018 23:13:04 -0700 (PDT) From: Michal Meloun X-Google-Original-From: Michal Meloun Reply-To: mmel@freebsd.org Subject: Re: svn commit: r335173 - head/sys/dev/extres/regulator To: Kyle Evans , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201806142037.w5EKbP6S096772@repo.freebsd.org> Openpgp: preference=signencrypt Autocrypt: addr=mmel@freebsd.org; prefer-encrypt=mutual; keydata= xsBNBFYuVRkBCADZiwLCCne3wG9b9k+R2Neo5zVo2bLaZRfNNY/v9kg283i0sb1Da4EdEiNT 15El5UyozhphUIbIR/zrVpxF1TvvFdoCyzx6a68bNY2d9dBrDcNDZC+XnyDdHQoobN87DWT1 mRVkmbg9LHZ/SVUOkGYuWyE+8UYeDAcUizuXwSK5zFWmeTyIoWNa68ifrWLfQe0p4x5jC/AI VURCi17p360vU4fhgwoMvEEhrRBWCr4DYHToFjIt2WdBy3GR1qoO0+Xkd6G+OoBULo+XDfgu L2WdPvh0K69F9/LgHkMmG5Il7SCe62QGpG2vaCgRV7BQhLX+kxlvM+WrdRatWRml4Y/3ABEB AAHNIE1pY2hhbCBNZWxvdW4gPG1tZWxAZnJlZWJzZC5vcmc+wsCABBMBCgAqAhsDBQsJCAcD BRUKCQgLBRYDAgEAAh4BAheAAhkBBQJZjBHDBQkHICOqAAoJEGkesmtexaqqIKMIAJ9xTp1w ge86ns2ZYOac5++mAgpFatohSlxYUR3gwud3Y3Ej0eumavpv/C26N6dsLnspwRenKdLbIPKe 0N8lI7CcDBIJGiFyY3c4H79QjIkYpRgbWFyCM85zEyVJpB+U7BhsgXE2uwVjE9RNhEP0KBoj sp357uqq1B1+VUO4GJ+RjdmYSOcNrjR8tTfy02456qovGjJ4JcJBlhyK6GzBKvnZSoA0s+QP OMn3gd8gdomMLEJdS3kTsfhLh2rQPZa9EmzafIyjXrirWq4+4fVFgd8SiMZyyTM+Kz30ZSUe 6SmfaQTQ/WLRIl5jku2uYQWlrRIKT9xaQzRWtZO9UgtXFRHOwE0EVi5VGQEIALqgRkfS21D/ OqWE9mXfh2bIjrp9uC8T0MCuimbsrAdLKNNorGu2nE+rebgX8n5nYM377HOnalPGyOuXvCbQ 8MFVRdWOHxenJjXJialNdBsOf2wLva3vSSVsdoPzibWDIcJqhBOQ3EuhsILyWSPvYYKEiy95 mfhrDtuTTOAYVR9aNQBOENztB2TDJyMx/qZmtGroGV3N0Hqde/znHPtQO8RG5/FQGMfHMI5G FMuycr1ceHnLo/ovrqAl4TYV+UHSHJ+FDE9dt9wXHclWbWbC0yNugchZq6rho5Jjfv4a2v7P pyn3HoDinh1lWP7hYA0ZNExGHekLnXWVqO/lzGS6bMEAEQEAAcLAZQQYAQoADwIbDAUCWYwR wwUJByAjqgAKCRBpHrJrXsWqqrsrB/4g4ESK5TLxUxi8pLWcLPyvwtN4Fmf7VsCVefkhakaG rDPmfvfnG+OFwN60Xqoni7GBeakl01xwT4RINfvVfShDy6cHpLS7QL/M8pzfulVX38MkVkOD yGZhwjE+jyT/kZNA1Olaw3N3IefHq3brskQ7G4d9oPep2DDbw7C4Q76uOBjxy34JVB0WOsB6 NyMQB9h6LGljQtdEddyUqwnRZzzHiGvp0hPtdYQHQZlqbj4FV9lTRK7a8Ega+y7MgmeMiztG zeXyjNP02r3PRHCPagwa57bPxH2aAh4Q7UzBBZ0GTMm7DLKNtCP58WDxblrrhZ+7kHqGK8Fs bdeUpDdEYLVd Message-ID: Date: Fri, 15 Jun 2018 08:13:06 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <201806142037.w5EKbP6S096772@repo.freebsd.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.26 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, 15 Jun 2018 06:13:07 -0000 On 14.06.2018 22:37, Kyle Evans wrote: > Author: kevans > Date: Thu Jun 14 20:37:25 2018 > New Revision: 335173 > URL: https://svnweb.freebsd.org/changeset/base/335173 > > Log: > extres/regulator: Properly refcount gpio regulators > > regnode::enable_cnt is generally used to refcount regulator nodes. For > GPIOs, the refcount was done on the gpio_entry since more than one regulator > can share a GPIO. > > GPIO regulators were not taking part in the node refcount, since they had > their own mechanism. This caused some fallout after manu started disabling > everybody's unused regulators in r331989. > > Refcount it. This is, imho, wrong solution. The regnode::enable_cnt counts number of enable request from consumer and regulator framework calls regnode_enable() method only when first consumer wants enable it or last consumer disable it. regnode::enable_cnt doesn't reflect actual status (enabled/disabled) of given regulator. The gpio_entry::enable_cnt track number of enabled regulators on same gpio line and is unrelated to sum of regnode::enable_cnt or actual state of enable pin. Just look whats happen for regulator without boot_on or always_on property after standard regulator_enable() then regulator_disable() sequence. For regulator_enable(), the framework increments regnode::enable_cnt then invoke regnode_enable(..., true, ...) (because regnode::enable_cnt was zero). The regnode_enable() also increments regnode::enable_cnt. So final value of counter is 2. Subsequent regulator_disable() decrement regnode::enable_cnt and because it is not zero, it returns without any action. The real bug is that regulator framework transforms regulator_stop() (not refcounting method) to regnode_enable(..., false, ...) (refcounting method) and thus regnode_fixed_enable() cannot handle gpio_entry::enable_cnt properly. Can you, please, revert this? > > Glanced over by: manu > > Modified: > head/sys/dev/extres/regulator/regulator.c > head/sys/dev/extres/regulator/regulator.h > head/sys/dev/extres/regulator/regulator_fixed.c > > Modified: head/sys/dev/extres/regulator/regulator.c > ============================================================================== > --- head/sys/dev/extres/regulator/regulator.c Thu Jun 14 20:36:55 2018 (r335172) > +++ head/sys/dev/extres/regulator/regulator.c Thu Jun 14 20:37:25 2018 (r335173) > @@ -507,6 +507,20 @@ struct regnode_std_param *regnode_get_stdparam(struct > return (®node->std_param); > } > > +void > +regnode_enable_cnt_inc(struct regnode *regnode) > +{ > + > + regnode->enable_cnt++; > +} > + > +void > +regnode_enable_cnt_dec(struct regnode *regnode) > +{ > + > + regnode->enable_cnt--; > +} > + > void regnode_topo_unlock(void) > { > > > Modified: head/sys/dev/extres/regulator/regulator.h > ============================================================================== > --- head/sys/dev/extres/regulator/regulator.h Thu Jun 14 20:36:55 2018 (r335172) > +++ head/sys/dev/extres/regulator/regulator.h Thu Jun 14 20:37:25 2018 (r335173) > @@ -106,6 +106,8 @@ int regnode_get_flags(struct regnode *regnode); > void *regnode_get_softc(struct regnode *regnode); > device_t regnode_get_device(struct regnode *regnode); > struct regnode_std_param *regnode_get_stdparam(struct regnode *regnode); > +void regnode_enable_cnt_inc(struct regnode *regnode); > +void regnode_enable_cnt_dec(struct regnode *regnode); > void regnode_topo_unlock(void); > void regnode_topo_xlock(void); > void regnode_topo_slock(void); > > Modified: head/sys/dev/extres/regulator/regulator_fixed.c > ============================================================================== > --- head/sys/dev/extres/regulator/regulator_fixed.c Thu Jun 14 20:36:55 2018 (r335172) > +++ head/sys/dev/extres/regulator/regulator_fixed.c Thu Jun 14 20:37:25 2018 (r335173) > @@ -156,6 +156,8 @@ regnode_fixed_init(struct regnode *regnode) > if (sc->gpio_open_drain) > flags |= GPIO_PIN_OPENDRAIN; > enable = sc->param->boot_on || sc->param->always_on; > + if (enable) > + regnode_enable_cnt_inc(regnode); > if (!sc->param->enable_active_high) > enable = !enable; > rv = GPIO_PIN_SET(pin->dev, pin->pin, enable); > @@ -194,12 +196,14 @@ regnode_fixed_enable(struct regnode *regnode, bool ena > return (0); > pin = &sc->gpio_entry->gpio_pin; > if (enable) { > + regnode_enable_cnt_inc(regnode); > sc->gpio_entry->enable_cnt++; > if (sc->gpio_entry->enable_cnt > 1) > return (0); > } else { > KASSERT(sc->gpio_entry->enable_cnt > 0, > ("Invalid enable count")); > + regnode_enable_cnt_dec(regnode); > sc->gpio_entry->enable_cnt--; > if (sc->gpio_entry->enable_cnt >= 1) > return (0); >