LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: MyungJoo Ham <myungjoo.ham@samsung.com>
To: 최찬우 <cw00.choi@samsung.com>
Cc: "kgene@kernel.org" <kgene@kernel.org>,
	박경민 <kyungmin.park@samsung.com>,
	"rafael.j.wysocki@intel.com" <rafael.j.wysocki@intel.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"ABHILASH KESAVAN" <a.kesavan@samsung.com>,
	"tomasz.figa@gmail.com" <tomasz.figa@gmail.com>,
	"Krzysztof Kozlowski" <k.kozlowski@samsung.com>,
	"Bartlomiej Zolnierkiewicz" <b.zolnierkie@samsung.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	대인기 <inki.dae@samsung.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-samsung-soc@vger.kernel.org"
	<linux-samsung-soc@vger.kernel.org>
Subject: Re: Re: [PATCHv8 1/9] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
Date: Tue, 20 Jan 2015 06:59:30 +0000 (GMT)	[thread overview]
Message-ID: <213049974.1319841421737167629.JavaMail.weblogic@epmlwas04a> (raw)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 5027 bytes --]

>  
> Dear Myungjoo,
>
>On 01/20/2015 01:34 PM, MyungJoo Ham wrote:
>>>   
[]
>>> +
>>> +	mutex_lock(&edev->lock);
>>> +	if (edev->desc->ops && edev->desc->ops->enable) {
>>> +		ret = edev->desc->ops->enable(edev);
>>> +		if (ret < 0)
>>> +			goto err;
>>> +	}
>> 
>> Is there any reason to call enable(edev) even when enable_count is already > 0 
>> while you do not call disable(edev) while enable_count > 0?
>> 
>> I think this may incur errors in the related device drivers.
>> (e.g., incorrect pairing of clk/runtime-pm/regulator enable/disable
>> at the device driver side)
>
>You're right. This part has potential errors. I'll fix it as following:
>If edev is already enabled, devfreq_event_enable_edev() will just return
>without any operation because devfreq-event(edev) can handle only one event
>at the same time.
> 
>	mutex_lock(&edev->lock);
>	if (edev->enable_count)
>		dev_warn(&edev->dev, "%s is already enabled\n", edev->desc->name);
>		ret = -EINVAL;
>		goto err;
>	}
>
>	if (edev->desc->ops && edev->desc->ops->enable) {		
>		ret = edev->desc->ops->enable(edev);
>		if (ret < 0)
>			goto err;
>	}
>	edev->enable_count++;

No, your suggested modification creates another bug.

It should not emit "warn" when enable_count > 0 at enable().
It is a natural behavior from drivers.
- You may have multiple drivers using edev.
- You may have multiple threads using edev.

Thus, the above 12 lines should be replaced with:

	if (edev->desc->ops && edev->desc->ops->enable &&
	    edev->enable_count == 0) {
		ret = edev->desc->ops->enable(edev);
		if (ret < 0)
			goto err;
	}
	edev->enable_count++;

>	
>
>> 
>>> +	edev->enable_count++;
>>> +err:
>>> +	mutex_unlock(&edev->lock);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devfreq_event_enable_edev);
>>> +
>>> +/**
>>> + * devfreq_event_disable_edev() - Disable the devfreq-event dev and decrease
>>> + *				  the enable_count of the devfreq-event dev.
>>> + * @edev	: the devfreq-event device
>>> + *
>>> + * Note that this function decrease the enable_count and disable the
>>> + * devfreq-event device. After the devfreq-event device is disabled,
>>> + * devfreq device can't use the devfreq-event device for get/set/reset
>>> + * operations.
>>> + */
>>> +int devfreq_event_disable_edev(struct devfreq_event_dev *edev)
>>> +{
>>> +	int ret = 0;
>>> +
>>> +	if (!edev || !edev->desc)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&edev->lock);
>>> +	if (edev->enable_count > 0) {
>>> +		edev->enable_count--;
>>> +	} else {
>>> +		dev_warn(&edev->dev, "unbalanced enable_count\n");
>>> +		ret = -EINVAL;
>>> +		goto err;
>>> +	}
>>> +
>>> +	if (edev->desc->ops && edev->desc->ops->disable) {
>>> +		ret = edev->desc->ops->disable(edev);
>>> +		if (ret < 0) {
>>> +			edev->enable_count++;
>>> +			goto err;
>>> +		}

Anyway, have you seen other subsystems doing fall-back operations as you've
done by "edev->enable_count++" here? Or is this your own idea on falling back
from errors with a disable callback?

>>> +	}
>> 
>> You did it correctly with disable here;
>> not calling it when it is not required.

Uh..yeah.. the original patch was incorrect..

>
>As I explained, I'll fix it as following:
>
>	mutex_lock(&edev->lock);
>	if (!edev->enable_count) {
>		dev_warn(&edev->dev, "%s is already disabled\n", edev->desc->name);
>		ret = -EINVAL;
>		goto err;
>	}
>
>	if (edev->desc->ops && edev->desc->ops->disable) {
>		ret = edev->desc->ops->disable(edev);
>		if (ret < 0)
>			goto err;		
>	}
>	edev->enable_count--;

Uh.... I'd say it is still incorrect.

	mutex_lock(&edev->lock);
	if (!edev->enable_count) {
		dev_warn(&edev->dev, "%s is already disabled\n", edev->desc->name);
		ret = -EINVAL;
		goto err;
	}

	edev->enable_count--;
	if (edev->desc->ops && edev->desc->ops->disable &&
	    !edev->enable_count) {
		ret = edev->desc->ops->disable(edev);
		if (ret < 0)
			goto err;		
	}


>
>> 
>>> +err:
>>> +	mutex_unlock(&edev->lock);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devfreq_event_disable_edev);
>>> +
>> 
>> []
>>> +EXPORT_SYMBOL_GPL(devfreq_event_is_enabled);
>> []
>> 
>>> +EXPORT_SYMBOL_GPL(devfreq_event_set_event);
>> []
>> 
[]
>>> +int devfreq_event_reset_event(struct devfreq_event_dev *edev)
>>> +{
>>> +	int ret = 0;
>>> +
>>> +	if (!edev || !edev->desc)
>>> +		return -EINVAL;
>>> +
>>> +	if (!devfreq_event_is_enabled(edev))
>>> +		return -EPERM;
>>> +
>>> +	mutex_lock(&edev->lock);
>>> +	if (edev->desc->ops && edev->desc->ops->reset)
>>> +		ret = edev->desc->ops->reset(edev);
>>> +	mutex_unlock(&edev->lock);
>> 
>> In the context of the get_event() handling "load",
>> aren't you supposed to set total_event = event = 0; here?
>
>But, devfreq_event_reset_event() function cannot handle edata instance
>because edata is not included in edev. The edata instance is only used in devfreq_event_get_event().

Ah.. ok then.

>
[]

Cheers,
MyungJoo
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

             reply	other threads:[~2015-01-20  6:59 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-20  6:59 MyungJoo Ham [this message]
2015-01-20  7:25 ` Chanwoo Choi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=213049974.1319841421737167629.JavaMail.weblogic@epmlwas04a \
    --to=myungjoo.ham@samsung.com \
    --cc=a.kesavan@samsung.com \
    --cc=b.zolnierkie@samsung.com \
    --cc=cw00.choi@samsung.com \
    --cc=inki.dae@samsung.com \
    --cc=k.kozlowski@samsung.com \
    --cc=kgene@kernel.org \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=robh+dt@kernel.org \
    --cc=tomasz.figa@gmail.com \
    --subject='Re: Re: [PATCHv8 1/9] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).