lkml.org 
[lkml]   [2018]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RESEND PATCH] Revert "pwm: Set class for exported channels in sysfs"
    From
    Date
    On 09/24/2018 05:50 PM, Fabrice Gasnier wrote:
    > On 09/24/2018 04:23 PM, Thierry Reding wrote:
    >> On Mon, Sep 24, 2018 at 03:59:03PM +0200, Fabrice Gasnier wrote:
    >>> On 09/24/2018 01:53 PM, Thierry Reding wrote:
    >>>> On Fri, Sep 21, 2018 at 04:02:47PM +0200, Fabrice Gasnier wrote:
    >>>>> This reverts commit 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00 as it causes
    >>>>> regression with multiple pwm chip. It creates a new entry in
    >>>>> '/sys/class/pwm' every time a 'pwmX' is exported with 'echo X > export':
    >>>>> - 1st time export will create an entry in /sys/class/pwm/pwmX
    >>>>> - when another export happens on another pwmchip, it can't be created
    >>>>> (e.g. -EEXIST)
    >>>>>
    >>>>> This also changes existing ABI (Documentation/ABI/testing/sysfs-class-pwm):
    >>>>> - pmwX should be there: /sys/class/pwm/pwmchipN/pwmX
    >>>>>
    >>>>> Example on stm32 (stm32429i-eval) platform:
    >>>>> $ ls /sys/class/pwm
    >>>>> pwmchip0 pwmchip4
    >>>>>
    >>>>> $ cd /sys/class/pwm/pwmchip0/
    >>>>> $ echo 0 > export
    >>>>> $ ls /sys/class/pwm
    >>>>> pwm0 pwmchip0 pwmchip4
    >>>>>
    >>>>> $ cd /sys/class/pwm/pwmchip4/
    >>>>> $ echo 0 > export
    >>>>> sysfs: cannot create duplicate filename '/class/pwm/pwm0'
    >>>>> ...Exception stack follows...
    >>>>>
    >>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
    >>>>> ---
    >>>>> drivers/pwm/sysfs.c | 1 -
    >>>>> 1 file changed, 1 deletion(-)
    >>>>
    >>>> Can we come up with an alternative that allows us to have both? We want
    >>>> uevent and proper sysfs creation, or is that not possible?
    >>>
    >>> Hi Thierry, all,
    >>>
    >>> With current approach:
    >>> - "export->child.class = parent->class"
    >>> - ABI (e.g. "pwm%d") device name isn't unique with multiple pwm chip.
    >>> I think this is not possible.
    >>>
    >>> Trying to think of an alternative... I just did a quick test, by
    >>> changing device name, to take pwmchip into account:
    >>> + export->child.class = parent->class;
    >>> export->child.release = pwm_export_release;
    >>> export->child.parent = parent;
    >>> export->child.devt = MKDEV(0, 0);
    >>> export->child.groups = pwm_groups;
    >>> - dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
    >>> + dev_set_name(&export->child, "pwmchip%d-pwm%u", chip->base,
    >>> pwm->hwpwm);
    >>>
    >>> But this also impacts existing ABI :-(
    >>> Would you have suggestions to send an uevent, without modifying ABI ?
    >>
    >> I don't quite understand why, in the example you show in the commit
    >> message, the pwmX nodes appear in the top-level /sys/class/pwm
    >> directory. According to Documentation/ABI/testing/sysfs-class-pwm they
    >> should appear as /sys/class/pwm/pwmchipN/pwmX. I can only imagine that
    >> setting the class may have changed that.
    >
    > Yes, adding the class makes the link to be created under /sys/class/pwm:
    > device_register() -> device_add() -> device_add_class_symlinks()
    > In device_add_class_symlinks():
    > ...
    > if (!dev->class)
    > return 0;
    > ...
    > /* link in the class directory pointing to the device */
    > error = sysfs_create_link(&dev->class->p->subsys.kobj,
    > &dev->kobj, dev_name(dev));
    > ...
    >
    >> If so, perhaps we can
    >> workaround that by creating a new class that is not parent->class?

    Hi Thierry,

    Maybe there's a way around, keeping the revert patch, without impacting
    the ABI:
    - pwmX cannot be added to pwm/another class without issue as discussed
    (numbering isn't unique).
    - pwmchipN already belongs to pwm class.

    I did some testing, trying to send uevent on the pwmX directly, without
    success: uevents are filtered as pwmX doesn't belong to a class.

    Still, it is possible to send uevent (KOBJ_CHANGE) on pwmchipN device,
    to notify of a change, e.g. pwmX channel being exported/unexported.

    I run following prototype (with revert patch).

    static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
    {
    + char *pwm_prop[2];
    struct pwm_export *export;
    int ret;
    ...
    kfree(export);
    return ret;
    }
    + pwm_prop[0] = kasprintf(GFP_KERNEL, "EXPORT=pwm%u", pwm->hwpwm);
    + pwm_prop[1] = NULL;
    + kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
    + kfree(pwm_prop[0]);

    return 0;
    }

    static int pwm_unexport_child(struct device *parent, struct pwm_device
    *pwm)
    {
    struct device *child;
    + char *pwm_prop[2];

    if (!test_and_clear_bit(PWMF_EXPORTED, &pwm->flags))
    return -ENODEV;
    ...
    if (!child)
    return -ENODEV;

    + pwm_prop[0] = kasprintf(GFP_KERNEL, "UNEXPORT=pwm%u", pwm->hwpwm);
    + pwm_prop[1] = NULL;
    + kobject_uevent_env(&parent->kobj, KOBJ_CHANGE, pwm_prop);
    + kfree(pwm_prop[0]);
    +
    /* for device_find_child() */

    Then, I run a quick test:

    # udevadm monitor --environment &
    # echo 0 > /sys/class/pwm/pwmchip0/export
    KERNEL[197.321736] change /devices/.../pwm/pwmchip0 (pwm)
    ACTION=change
    DEVPATH=/devices/.../pwm/pwmchip0
    EXPORT=pwm0
    SEQNUM=2045
    SUBSYSTEM=pwm

    # echo 0 > /sys/class/pwm/pwmchip4/export
    KERNEL[202.089676] change /devices/.../pwm/pwmchip4 (pwm)
    ACTION=change
    DEVPATH=/devices/.../pwm/pwmchip4
    EXPORT=pwm0
    SEQNUM=2046
    SUBSYSTEM=pwm


    Also unexport will report change events to userland:

    # echo 0 > /sys/class/pwm/pwmchip0/unexport
    KERNEL[1691.112765] change /devices/.../pwm/pwmchip0 (pwm)
    ACTION=change
    DEVPATH=/devices/.../pwm/pwmchip0
    SEQNUM=2047
    SUBSYSTEM=pwm
    UNEXPORT=pwm0

    Do you think this may be a way around?
    Please let me know if this may satisfy need for uevents.

    Best regards,
    Fabrice
    >
    > And this link is added using dev_name(). So I doubt adding a new class
    > will change the current behavior.
    >
    >>
    >> Thierry
    >>

    \
     
     \ /
      Last update: 2018-09-25 16:00    [W:4.788 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site