lkml.org 
[lkml]   [2010]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4] power: introduce library for device-specific OPPs
    Paul E. McKenney had written, on 09/24/2010 04:40 PM, the following:
    [...]
    >>>> +/**
    >>>> + * opp_find_freq_ceil() - Search for an rounded ceil freq
    >>>> + * @dev: device for which we do this operation
    >>>> + * @freq: Start frequency
    >>>> + *
    >>>> + * Search for the matching ceil *available* OPP from a starting freq
    >>>> + * for a device.
    >>>> + *
    >>>> + * Returns matching *opp and refreshes *freq accordingly, else returns
    >>>> + * ERR_PTR in case of error and should be handled using IS_ERR.
    >>>> + *
    >>>> + * Locking: RCU reader.
    >>>> + */
    >>>> +struct opp *opp_find_freq_ceil(struct device *dev, unsigned long *freq)
    >>>> +{
    >>>> + struct device_opp *dev_opp;
    >>>> + struct opp *temp_opp, *opp = ERR_PTR(-ENODEV);
    >>>> +
    >>>> + if (!dev || !freq) {
    >>>> + pr_err("%s: invalid param dev=%p freq=%p\n", __func__,
    >>>> + dev, freq);
    >>>> + return ERR_PTR(-EINVAL);
    >>>> + }
    >>>> +
    >>>> + dev_opp = find_device_opp(dev);
    >>>> + if (IS_ERR(dev_opp))
    >>>> + return opp;
    >>>> +
    >>>> + rcu_read_lock();
    >>>> + list_for_each_entry_rcu(temp_opp, &dev_opp->opp_list, node) {
    >>>> + if (temp_opp->available && temp_opp->rate >= *freq) {
    >>>> + opp = temp_opp;
    >>>> + *freq = opp->rate;
    >>>> + break;
    >>>> + }
    >>>> + }
    >>>> + rcu_read_unlock();
    >>> And this one also has the same problem that find_device_opp() does.
    >> guessing opp ptr here.. am I right? if it is about device_opp, it is
    >> not going to be freed as I mentioned above - at least we dont give
    >> an function to update(hence free) it.
    >
    > It really does look to me that you are passing a pointer to the thing
    > being freed out of an RCU read-side critical section. If you are really
    > doing this, you do need to do something to fix it. I outlined some of
    > the options earlier.
    ack. will try to fix in v5.

    >
    >>>> + return opp;
    >>>> +}
    >>>> +
    >>>> +/**
    >>>> + * opp_find_freq_floor() - Search for a rounded floor freq
    >>>> + * @dev: device for which we do this operation
    >>>> + * @freq: Start frequency
    >>>> + *
    >>>> + * Search for the matching floor *available* OPP from a starting freq
    >>>> + * for a device.
    >>>> + *
    >>>> + * Returns matching *opp and refreshes *freq accordingly, else returns
    >>>> + * ERR_PTR in case of error and should be handled using IS_ERR.
    >>>> + *
    >>>> + * Locking: RCU reader.
    >>>> + */
    >>>> +struct opp *opp_find_freq_floor(struct device *dev, unsigned long *freq)
    >>>> +{
    >>>> + struct device_opp *dev_opp;
    >>>> + struct opp *temp_opp, *opp = ERR_PTR(-ENODEV);
    >>>> +
    >>>> + if (!dev || !freq) {
    >>>> + pr_err("%s: invalid param dev=%p freq=%p\n", __func__,
    >>>> + dev, freq);
    >>>> + return ERR_PTR(-EINVAL);
    >>>> + }
    >>>> +
    >>>> + dev_opp = find_device_opp(dev);
    >>>> + if (IS_ERR(dev_opp))
    >>>> + return opp;
    >>>> +
    >>>> + rcu_read_lock();
    >>>> + list_for_each_entry_rcu(temp_opp, &dev_opp->opp_list, node) {
    >>>> + if (temp_opp->available) {
    >>>> + /* go to the next node, before choosing prev */
    >>>> + if (temp_opp->rate > *freq)
    >>>> + break;
    >>>> + else
    >>>> + opp = temp_opp;
    >>>> + }
    >>>> + }
    >>>> + if (!IS_ERR(opp))
    >>>> + *freq = opp->rate;
    >>>> + rcu_read_unlock();
    >>> As does this one.
    >> guessing opp ptr here.. am I right?
    >
    > Again, here it looks to me like you are passing a pointer out of an RCU
    > read-side critical section that could be freed out from under you. If
    > so, again, this must be fixed.
    >
    [...]
    >>>> +static int opp_set_availability(struct opp *opp, bool availability_req)
    >>>> +{
    >>>> + struct opp *new_opp, *tmp_opp;
    >>>> + bool is_available;
    >>>> +
    >>>> + if (unlikely(!opp || IS_ERR(opp))) {
    >>>> + pr_err("%s: Invalid parameters being passed\n", __func__);
    >>>> + return -EINVAL;
    >>>> + }
    >>>> +
    >>>> + new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
    >>>> + if (!new_opp) {
    >>>> + pr_warning("%s: unable to allocate opp\n", __func__);
    >>>> + return -ENOMEM;
    >>>> + }
    >>>> +
    >>>> + mutex_lock(&opp->dev_opp->lock);
    >>>> +
    >>>> + rcu_read_lock();
    >>>> + tmp_opp = rcu_dereference(opp);
    >>>> + is_available = tmp_opp->available;
    >>>> + rcu_read_unlock();
    >>>> +
    >>>> + /* Is update really needed? */
    >>>> + if (is_available == availability_req) {
    >>>> + mutex_unlock(&opp->dev_opp->lock);
    >>>> + kfree(tmp_opp);
    >>>> + return 0;
    >>>> + }
    >>>> +
    >>>> + *new_opp = *opp;
    >>>> + new_opp->available = availability_req;
    >>>> + list_replace_rcu(&opp->node, &new_opp->node);
    >>>> +
    >>>> + mutex_unlock(&opp->dev_opp->lock);
    >>>> + synchronize_rcu();
    >>> If you decide to rely on reference counts to fix the problem in
    >>> find_device_opp(), you will need to check the reference counts here.
    >>> Again, please see Documentation/RCU/rcuref.txt.
    >> Does the original point about not needing to free dev_opp resolve this?
    >
    > For the dev_opp case, yes. However, I believe that my point is still
    > valid for the opp case.
    Ack. I missed that :(.. let me relook at the logic yet again. hopefully
    fix in v5.

    >
    >>>> + kfree(opp);
    >>>> +
    >>>> + return 0;
    >>>> +}
    >>>> +
    >>>> +/**
    >>>> + * opp_enable() - Enable a specific OPP
    >>>> + * @opp: Pointer to opp
    >>>> + *
    >>>> + * Enables a provided opp. If the operation is valid, this returns 0, else the
    >>>> + * corresponding error value. It is meant to be used for users an OPP available
    >>>> + * after being temporarily made unavailable with opp_disable.
    >>>> + *
    >>>> + * Locking: RCU, mutex
    >>> By "Locking: RCU", you presumably don't mean that the caller must do
    >>> an rcu_read_lock() -- this would result in a synchronize_rcu() being
    >>> invoked in an RCU read-side critical section, which is illegal.
    >> aye..thx. I will make it more verbose. Does the following sound right?
    >>
    >> Locking used internally: RCU copy-update and read_lock used, mutex
    >>
    >> and for the readers:
    >>
    >> Locking used internally: RCU read_lock used
    >>
    >> or do we need to go all verbatim about the implementation here?
    >>
    >> I intended the user to know the context in which they can call it,
    >> for example, since mutex is used, dont think of using this in
    >> interrupt context. since read_locks are already used, dont need to
    >> double lock it.. opp library takes care of it's own exclusivity.
    >
    > I would stick to the constraints on the caller, and describe the internals
    > elsewhere, for example, near the data-structure definitions. But tastes
    > do vary on this.

    okay. let me see how to clean this up.

    [..]
    >>>> +void opp_init_cpufreq_table(struct device *dev,
    >>>> + struct cpufreq_frequency_table **table)
    >>>> +{
    >>>> + struct device_opp *dev_opp;
    >>>> + struct opp *opp;
    >>>> + struct cpufreq_frequency_table *freq_table;
    >>>> + int i = 0;
    >>>> +
    >>>> + dev_opp = find_device_opp(dev);
    >>>> + if (IS_ERR(dev_opp)) {
    >>>> + pr_warning("%s: unable to find device\n", __func__);
    >>>> + return;
    >>>> + }
    >>>> +
    >>>> + freq_table = kzalloc(sizeof(struct cpufreq_frequency_table) *
    >>>> + (opp_get_opp_count(dev) + 1), GFP_ATOMIC);
    >>>> + if (!freq_table) {
    >>>> + pr_warning("%s: failed to allocate frequency table\n",
    >>>> + __func__);
    >>>> + return;
    >>> How does the caller tell that the allocation failed? Should the caller
    >>> set the pointer passed in through the "table" argument to NULL before
    >>> calling this function? Or should this function set *table to NULL
    >>> before returning in this case?
    >> Good catch. Thanks. I would rather change the return to int and pass
    >> proper errors to caller so that they can handle it appropriately.
    >
    > Works for me!
    thx.

    --
    Regards,
    Nishanth Menon


    \
     
     \ /
      Last update: 2010-09-27 20:11    [W:0.043 / U:0.712 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site