lkml.org 
[lkml]   [2013]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/4] intel_idle: stop using driver_data for static flags
    On 02/01/2013 07:40 PM, Len Brown wrote:
    > On 02/01/2013 03:44 AM, Daniel Lezcano wrote:
    >> On 02/01/2013 05:11 AM, Len Brown wrote:
    >>> From: Len Brown <len.brown@intel.com>
    >>>
    >>> The commit, 4202735e8ab6ecfb0381631a0d0b58fefe0bd4e2
    >>> (cpuidle: Split cpuidle_state structure and move per-cpu statistics fields)
    >>> observed that the MWAIT flags for Cn on every processor to date were the
    >>> same, and created get_driver_data() to supply them.
    >>>
    >>> Unfortunately, that assumption is false, going forward.
    >>> So here we restore the MWAIT flags to the cpuidle_state table.
    >>> However, instead restoring the old "driver_data" field,
    >>> we put the flags into the existing "flags" field,
    >>> where they probalby should have lived all along.
    >>
    >> Removing the driver_data is a good thing but I am not sure it is the
    >> case by moving the MWAIT flags to the cpuidle_state's flags field. We
    >> are mixing arch specific flags with a generic code.
    >>
    >> This is prone to errors because new flags could appear for the cpuidle
    >> core code and could collide with the arch specific flags.
    >>
    >> Wouldn't make sense to add a private field in the struct cpuidle_state
    >> structure to let the driver/arch specific to store whatever is needed ?
    >>
    >> struct cpuidle_state {
    >>
    >> ...
    >> unsigned long private;
    >> ...
    >>
    >> }
    >
    > The top half of the flags are reserved for the driver,
    > as noted by the definition of CPUIDLE_DRIVER_FLAGS_MASK
    > with the generic flag definitions:
    >
    > #define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */
    > #define CPUIDLE_FLAG_COUPLED (0x02) /* state applies to multiple cpus */
    >
    > #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
    >
    > intel_idle already uses a driver-specific flag:
    >
    > #define CPUIDLE_FLAG_TLB_FLUSHED 0x10000
    >
    > This patch just uses 4 more bits along with that one.

    Ok. In this case it would make sense to move this flag out of the
    generic core code to the intel_idle.c file ? and move the [dec/en]coding
    macro flags_2_MWAIT_EAX and MWAIT_EAX_2_flags (with a more appropriate
    name for a generic code) to the cpuidle.h file ?

    -- Daniel



    > Sure, if we run out of space, we can add an additional field.
    > But I don't see an immediate need for it.
    >
    > thanks,
    > Len Brown
    > Intel Open Source Technology Center
    >


    --
    <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

    Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
    <http://twitter.com/#!/linaroorg> Twitter |
    <http://www.linaro.org/linaro-blog/> Blog

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2013-02-02 00:02    [W:3.408 / U:2.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site