From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966486AbdCYFGG (ORCPT ); Sat, 25 Mar 2017 01:06:06 -0400 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:58670 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751754AbdCYFF6 (ORCPT ); Sat, 25 Mar 2017 01:05:58 -0400 Subject: Re: [PATCH] regulator: core: Limit propagation of parent voltage count and list To: Brian Norris , Matthias Kaehlcke References: <20170324200952.103303-1-mka@chromium.org> <20170324203818.GA33073@google.com> From: Javier Martinez Canillas Cc: Liam Girdwood , Mark Brown , linux-kernel@vger.kernel.org, Douglas Anderson Message-ID: Date: Sat, 25 Mar 2017 02:05:47 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170324203818.GA33073@google.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Matthias, On 03/24/2017 05:38 PM, Brian Norris wrote: > On Fri, Mar 24, 2017 at 01:09:52PM -0700, Matthias Kaehlcke wrote: >> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c >> index 53d4fc70dbd0..121838e0125b 100644 >> --- a/drivers/regulator/core.c >> +++ b/drivers/regulator/core.c >> @@ -2487,6 +2487,10 @@ static int _regulator_list_voltage(struct regulator *regulator, >> if (lock) >> mutex_unlock(&rdev->mutex); >> } else if (rdev->supply) { >> + // Limit propagation of parent values to switch regulators > > The kernel doesn't use C99 comments. Oddly enough, this isn't actually +1 > in the coding style doc (Documentation/process/coding-style.rst), nor is > it caught by scripts/checkpatch.pl (even though it clearly has a 'C99 > comment' rule). > >> + if (ops->get_voltage || ops->get_voltage_sel) It's valid to have a .get_voltage_sel callback without a .list_voltage? At least it seems that _regulator_get_voltage() assumes that having a .get_voltage_sel implies that a .list_voltage will also be available. static int _regulator_get_voltage(struct regulator_dev *rdev) { ... if (rdev->desc->ops->get_voltage_sel) { sel = rdev->desc->ops->get_voltage_sel(rdev); if (sel < 0) return sel; ret = rdev->desc->ops->list_voltage(rdev, sel); } else if (rdev->desc->ops->get_voltage) { ... } So I would only check for if (ops->get_voltage). >> + return -EINVAL; >> + >> ret = _regulator_list_voltage(rdev->supply, selector, lock); >> } else { >> return -EINVAL; >> @@ -2540,6 +2544,7 @@ EXPORT_SYMBOL_GPL(regulator_is_enabled); >> int regulator_count_voltages(struct regulator *regulator) >> { >> struct regulator_dev *rdev = regulator->rdev; >> + const struct regulator_ops *ops = rdev->desc->ops; >> >> if (rdev->desc->n_voltages) >> return rdev->desc->n_voltages; >> @@ -2547,6 +2552,10 @@ int regulator_count_voltages(struct regulator *regulator) >> if (!rdev->supply) >> return -EINVAL; >> >> + // Limit propagation of parent value to switch regulators > > Same here. > >> + if (ops->get_voltage || ops->get_voltage_sel || ops->list_voltage) >> + return -EINVAL; >> + I wonder if instead of always checking if the regulator lacks operations, it wouldn't be better to do it just once and store that the regulator is a switch so that state can be used as explicit check for switch instead. Something like if (!rdev->supply || !rdev->switch) looks more clear to me. >> return regulator_count_voltages(rdev->supply); >> } >> EXPORT_SYMBOL_GPL(regulator_count_voltages); > > I'm not very familiar with this code, but judging by your problem > description in previous threads and by comparing with the logic in > _regulator_get_voltage() (for when to reference the ->supply), this > seems resonable. So: > > Reviewed-by: Brian Norris > Agreed, the logic sounds reasonable indeed and I didn't think of this case when writing the mentioned commit, so feel free to add: Reviewed-by: Javier Martinez Canillas > It's probably worth verifying that this doesn't break whatever Javier > was supporting in the first place, as a sanity check. > I've tested in the system that led to the mentioned commit and I did not find any issue with $SUBJECT. Tested-by: Javier Martinez Canillas > Brian > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America