Subject: Re: [PATCH v2 1/3] mux: Add mux_control_get_optional() API
To: Stephen Boyd <stephen.boyd@linaro.org>
Cc: linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        Rob Herring <robh+dt@kernel.org>, Rob Clark <robdclark@gmail.com>,
        Peter Chen <Peter.Chen@nxp.com>, Andy Gross <andy.gross@linaro.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>
References: <20170714214005.14967-1-stephen.boyd@linaro.org>
 <20170714214005.14967-2-stephen.boyd@linaro.org>
 <aac61f46-f963-2631-00e0-2d62baac6bf4@axentia.se>
 <150043011983.23422.10872793103087420242@sboyd-linaro>
From: Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <1c20e9bf-9b87-a021-bb4b-8add6c468c87@axentia.se>
Date: Wed, 19 Jul 2017 09:15:38 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <150043011983.23422.10872793103087420242@sboyd-linaro>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jul 2017 07:15:43.3216 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0202MB2553
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 5900
Lines: 146

On 2017-07-19 04:08, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-07-17 01:20:14)
>> On 2017-07-14 23:40, Stephen Boyd wrote:
>>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>>> index 90b8995f07cb..a0e5bf16f02f 100644
>>> --- a/drivers/mux/mux-core.c
>>> +++ b/drivers/mux/mux-core.c
>>> @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
>>>   */
>>>  unsigned int mux_control_states(struct mux_control *mux)
>>>  {
>>> +     if (!mux)
>>> +             return 0;
>>> +
>>
>> I don't think this is appropriate. For this function, it might be ok,
>> but...
>>
>>>       return mux->states;
>>>  }
>>>  EXPORT_SYMBOL_GPL(mux_control_states);
>>> @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state)
>>>  {
>>>       int ret;
>>>  
>>> +     if (!mux)
>>> +             return 0;
>>> +
>>
>> ...here and for other cases below it's very odd to return "ok", when
>> -EINVAL or something seems much more appropriate. And if -EINVAL is
>> returned here, the benefit of returning fake values for anything
>> pretty much falls apart.
>>
>> I simply don't like it, and prefer if the consumer code is arranged
>> to not call the mux functions when the optional get() does not find
>> the mux.
> 
> Do you want the callers of the mux APIs to know that an optional mux
> isn't there, and then have checks at all callsites on optional muxes to
> make sure consumers don't call the mux functions? Won't that duplicate
> lots of checks in drivers for something the core could treat as a don't
> care case? Sorry, I don't understand why the consumer cares that it was
> there or not when it is optional.

Ok, I had a look around to figure out how others handle this, and e.g.
gpio has (ugly) macros (VALIDATE_DESC) to handle this. I guess you are
right and I'm wrong. So, please keep all the if (!mux) checks.

Thanks for insisting!

>>
>>>       ret = down_killable(&mux->lock);
>>>       if (ret < 0)
>>>               return ret;
>>> @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state)
>>>  {
>>>       int ret;
>>>  
>>> +     if (!mux)
>>> +             return 0;
>>> +
>>>       if (down_trylock(&mux->lock))
>>>               return -EBUSY;
>>>  
>>> @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
>>>  {
>>>       int ret = 0;
>>>  
>>> +     if (!mux)
>>> +             return 0;
>>> +
>>>       if (mux->idle_state != MUX_IDLE_AS_IS &&
>>>           mux->idle_state != mux->cached_state)
>>>               ret = mux_control_set(mux, mux->idle_state);
>>> @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>>>       return dev ? to_mux_chip(dev) : NULL;
>>>  }
>>>  
>>> -/**
>>> - * mux_control_get() - Get the mux-control for a device.
>>> - * @dev: The device that needs a mux-control.
>>> - * @mux_name: The name identifying the mux-control.
>>> - *
>>> - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
>>> - */
>>> -struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>> +struct mux_control *
>>> +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
>>>  {
>>>       struct device_node *np = dev->of_node;
>>>       struct of_phandle_args args;
>>> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>>       if (mux_name) {
>>>               index = of_property_match_string(np, "mux-control-names",
>>>                                                mux_name);
>>> +             if (index == -EINVAL && optional)
>>> +                     return NULL;
>>
>> What about -ENODATA?
> 
> At this point in the code we're finding the index of a mux-control-names
> property so I was trying to handle the case where there isn't a
> mux-control-names property at all

Yes, you indeed need to check for -EINVAL to catch that. No argument
about that.

>                                   but we're looking for something
> optional anyway. If there is a property, then we would see some other
> error if something went wrong and then pass the error up. There is a
> hole where there isn't a mux-control-names property and we're looking
> for something that's optional, but there is a mux-control property. Do
> we care though? The DT seems broken then.

I was thinking about the case where mux-control-names names *other* muxes
but not the one asked for in this call. That's not broken and should be
handled. The way I read it, you get -ENODATA in that case?

>> And if an optional mux is found here, but lookup
>> fails later in e.g. the of_parse_phandle_with_args call, then I think
>> an error should be returned. Because that seems like an indication that
>> DT specifies that there *should* be a mux, but then there isn't one.
> 
> of_parse_phandle_with_args() would return ENOENT when there isn't a
> mux-control property in DT. So I've trapped that case and returned an
> "optional mux" pointer of NULL. I think we handle the case you mention,
> where some index is found but it returns an error, because that would
> return some error besides -ENOENT.
> 
> Sorry, I'm not really following what you're suggesting. Maybe it got
> mixed up with the NULL vs. non-NULL return value from mux_control_get().

What I mean is that if you have passed a mux_name and the index of that
name was indeed found in the of_property_match_string call, then any
failure from of_parse_phandle_with_args indicates a bad DT and should
IMO result in an error. I.e., when evaluating the result from
of_parse_phandle_with_args, you should account for the optional param
if and only if mux_name is NULL.

You can do that by e.g. setting optional to false after looking up the
mux_name index (because at that point the mux is no longer considered
optional). E.g. as the last statement in the if (!mux_name) block.

Cheers,
peda